drm: encoder_slave: respect of_node on i2c encoder init

Commit Message

Sebastian Hesselbarth June 10, 2013, 9:23 p.m. UTC

Current DRM slave encoder API conflicts with auto-registration of i2c client
when using DT probed clients. To allow DRM slave encoders passed by DT, this
patch adds a check to drm_i2c_encoder_init for a non-NULL .of_node on
i2c_board_info and calls an of_i2c helper to get the i2c client device
instead of registering a new device.

Signed-off-by: Sebastian Hesselbarth <[e-mail]>
---
Cc: David Airlie <[e-mail]>
Cc: Russell King - ARM Linux <[e-mail]>
Cc: Darren Etheridge <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 drivers/gpu/drm/drm_encoder_slave.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

Comments

Daniel Vetter June 11, 2013, 7:24 a.m. UTC | #1

On Mon, Jun 10, 2013 at 11:23:42PM +0200, Sebastian Hesselbarth wrote:
> Current DRM slave encoder API conflicts with auto-registration of i2c client
> when using DT probed clients. To allow DRM slave encoders passed by DT, this
> patch adds a check to drm_i2c_encoder_init for a non-NULL .of_node on
> i2c_board_info and calls an of_i2c helper to get the i2c client device
> instead of registering a new device.
> 
> Signed-off-by: Sebastian Hesselbarth <[e-mail]>

Hm, I don't have much clue about how dt/of really works, but since of has
this nice probing/auto-instantiation thing I think we could simplify this
some more:
- Create a new drm_i2c_encoder_init_of which only takes a struct
  device_node *of_node argument instead of adap and info.
- I think we could also drop the call to ->set_config since presumably an
  of-enabled driver grabbed any required info already from the dt.

I think this way we could still share encoder slaves across tons of
platforms, only the init sequence (and specifically how they get at their
config data) would be different. That would also be extensible quite
easily (*cough* intel platforms could setup encoder slaves from
information out of the vbt *cough*).

And ofc, a (totally broken) exemplary conversion patch for tilcdc would be
neat ;-)

Cheers, Daniel
> ---
> Cc: David Airlie <[e-mail]>
> Cc: Russell King - ARM Linux <[e-mail]>
> Cc: Darren Etheridge <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/gpu/drm/drm_encoder_slave.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_encoder_slave.c b/drivers/gpu/drm/drm_encoder_slave.c
> index 0cfb60f..36c0aa7 100644
> --- a/drivers/gpu/drm/drm_encoder_slave.c
> +++ b/drivers/gpu/drm/drm_encoder_slave.c
> @@ -25,6 +25,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/of_i2c.h>
>  
>  #include <drm/drm_encoder_slave.h>
>  
> @@ -61,7 +62,10 @@ int drm_i2c_encoder_init(struct drm_device *dev,
>  
>  	request_module("%s%s", I2C_MODULE_PREFIX, info->type);
>  
> -	client = i2c_new_device(adap, info);
> +	if (info->of_node)
> +		client = of_find_i2c_device_by_node(info->of_node);
> +	else
> +		client = i2c_new_device(adap, info);
>  	if (!client) {
>  		err = -ENOMEM;
>  		goto fail;
> -- 
> 1.7.2.5
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Sebastian Hesselbarth June 11, 2013, 8 a.m. UTC | #2

On 06/11/13 09:24, Daniel Vetter wrote:
> On Mon, Jun 10, 2013 at 11:23:42PM +0200, Sebastian Hesselbarth wrote:
>> Current DRM slave encoder API conflicts with auto-registration of i2c client
>> when using DT probed clients. To allow DRM slave encoders passed by DT, this
>> patch adds a check to drm_i2c_encoder_init for a non-NULL .of_node on
>> i2c_board_info and calls an of_i2c helper to get the i2c client device
>> instead of registering a new device.
>>
>> Signed-off-by: Sebastian Hesselbarth <[e-mail]>
>
> Hm, I don't have much clue about how dt/of really works, but since of has
> this nice probing/auto-instantiation thing I think we could simplify this
> some more:
> - Create a new drm_i2c_encoder_init_of which only takes a struct
>    device_node *of_node argument instead of adap and info.
> - I think we could also drop the call to ->set_config since presumably an
>    of-enabled driver grabbed any required info already from the dt.

Daniel,

this patch is not about making DRM API DT aware but make it not _break_
on DT aware platforms. I am way out of league to rework DRM slave
encoder stuff for DT. This is the smallest non-intrusive modification
I can think of to allow i2c encoders to be passed by DT.

> I think this way we could still share encoder slaves across tons of
> platforms, only the init sequence (and specifically how they get at their

IMHO, the whole i2c encoder stuff is just wrong. Not any i2c slave
driver is even really using its probe(). Everything is packed somewhere
because it just worked.. this is at least a start.

I suggest this to get merged to at least allow to have DT slaves,
then start with improving tda998x as an example, then maybe rethink
drm_slave_encoder completely, e.g.

- generalize the concept to SPI attached encoders, "internal" encoders..
- find a way to setup .encoder_type and .connector_type correctly
- have more common of_drm_blabla helpers

> config data) would be different. That would also be extensible quite
> easily (*cough* intel platforms could setup encoder slaves from
> information out of the vbt *cough*).
>
> And ofc, a (totally broken) exemplary conversion patch for tilcdc would be
> neat ;-)

I don't have tilcdc to test so - no, I am not giving an example how to
use of_parse_phandle and assign the returned pointer to board_info's
.of_node. But it is no more than that, a single line of code.

Sebastian

Francisco Jerez June 11, 2013, 3:10 p.m. UTC | #3

Hi,

Sebastian Hesselbarth <[e-mail]> writes:

>> - I think we could also drop the call to ->set_config since presumably an
>>    of-enabled driver grabbed any required info already from the dt.
>[...]
>> I think this way we could still share encoder slaves across tons of
>> platforms, only the init sequence (and specifically how they get at their

The "set_config" hook is just the way a DRM driver communicates those
board-specific differences in the init sequence to the slave encoder
driver.  I don't think it would make sense to remove it unless we make
sure it's being called elsewhere.

>
> IMHO, the whole i2c encoder stuff is just wrong. Not any i2c slave
> driver is even really using its probe(). Everything is packed somewhere
> because it just worked.. this is at least a start.
>
Why is that?  What do you mean by the probe hooks not being used?
ch7006 and sil164 rely on it being called on initialization.

> I suggest this to get merged to at least allow to have DT slaves,
> then start with improving tda998x as an example, then maybe rethink
> drm_slave_encoder completely, e.g.
>
> - generalize the concept to SPI attached encoders, "internal" encoders..

drm_encoder_slave is bus-agnostic.  drm_i2c_encoder_init() is just a
helper function taking care of bus-specific details like the creation of
the underlying I2C device object, which cannot be made bus-agnostic for
obvious reasons.  You're welcome to implement SPI and internal
counterparts of drm_i2c_encoder_init().

> - find a way to setup .encoder_type and .connector_type correctly

I guess encoder_type could be initialized correctly from the slave
encoder_init() hook -- that hasn't been necessary until now because the
DRM driver making use of the slave encoder has been expected to have
some other means to find out encoder types [e.g. device-specific BIOS
tables].  OTOH, I don't think that setting connector types is the slave
encoder's business.

> - have more common of_drm_blabla helpers
>
>> config data) would be different. That would also be extensible quite
>> easily (*cough* intel platforms could setup encoder slaves from
>> information out of the vbt *cough*).
>>
>[...]

Sebastian Hesselbarth June 11, 2013, 9:31 p.m. UTC | #4

On 06/11/2013 05:10 PM, Francisco Jerez wrote:
> Sebastian Hesselbarth<[e-mail]>  writes:
>>> - I think we could also drop the call to ->set_config since presumably an
>>>     of-enabled driver grabbed any required info already from the dt.
>> [...]
>>> I think this way we could still share encoder slaves across tons of
>>> platforms, only the init sequence (and specifically how they get at their
>
> The "set_config" hook is just the way a DRM driver communicates those
> board-specific differences in the init sequence to the slave encoder
> driver.  I don't think it would make sense to remove it unless we make
> sure it's being called elsewhere.

Francisco,

for the non-DT case all probe related stuff could be passed with 
board_info.platform_data. For the DT case, the i2c device driver
will parse properties into .platform_data. IMHO in the end,
.set_config can be safely removed. But for now and especially because
I only have a DT-only platform to test, leave it there and rework
existing drivers and drm master encoders to pass it that way.

>> IMHO, the whole i2c encoder stuff is just wrong. Not any i2c slave
>> driver is even really using its probe(). Everything is packed somewhere
>> because it just worked.. this is at least a start.
>>
> Why is that?  What do you mean by the probe hooks not being used?
> ch7006 and sil164 rely on it being called on initialization.

You are right, I remembered wrong.

>> I suggest this to get merged to at least allow to have DT slaves,
>> then start with improving tda998x as an example, then maybe rethink
>> drm_slave_encoder completely, e.g.
>>
>> - generalize the concept to SPI attached encoders, "internal" encoders..
>
> drm_encoder_slave is bus-agnostic.  drm_i2c_encoder_init() is just a
> helper function taking care of bus-specific details like the creation of
> the underlying I2C device object, which cannot be made bus-agnostic for
> obvious reasons.  You're welcome to implement SPI and internal
> counterparts of drm_i2c_encoder_init().

Hehe, true again. I like to help and improve it wherever possible. But
to be honest, DRM is not an easy starter for blindly touch commonly
shared functions. If I break Dove, there is little people complaining.
If I also break x86 complaining quickly becomes ranting ;)

>> - find a way to setup .encoder_type and .connector_type correctly
>
> I guess encoder_type could be initialized correctly from the slave
> encoder_init() hook -- that hasn't been necessary until now because the
> DRM driver making use of the slave encoder has been expected to have
> some other means to find out encoder types [e.g. device-specific BIOS
> tables].  OTOH, I don't think that setting connector types is the slave
> encoder's business.

Well, I do not completely agree here. Actually, the connector type
cannot be set from any encoder in the chain in a consitent way. But
at least the slave encoder is closer to it.

For Dove (and many other SoCs) the lcd controller is just a dumb rgb
scan-out controller. It makes no difference if you directly connect an
LCD panel or have a HDMI encoder finally connected to an DVI plug.
The LCD controller shouldn't really care because it always sees a dumb
rgb receiver, the HDMI encoder at least can say it is either HDMI or
DVI plug.

For DT, I tend to have a video-card node comprising lcd controllers,
video memory range, external encoder phandle. Maybe put the board-
specific connector here, and then it is up the the master encoder
(or video card "driver") to set the correct connector.

Sebastian

Patch

diff --git a/drivers/gpu/drm/drm_encoder_slave.c b/drivers/gpu/drm/drm_encoder_slave.c
index 0cfb60f..36c0aa7 100644
--- a/drivers/gpu/drm/drm_encoder_slave.c
+++ b/drivers/gpu/drm/drm_encoder_slave.c
@@ -25,6 +25,7 @@ 
  */
 
 #include <linux/module.h>
+#include <linux/of_i2c.h>
 
 #include <drm/drm_encoder_slave.h>
 
@@ -61,7 +62,10 @@  int drm_i2c_encoder_init(struct drm_device *dev,
 
 	request_module("%s%s", I2C_MODULE_PREFIX, info->type);
 
-	client = i2c_new_device(adap, info);
+	if (info->of_node)
+		client = of_find_i2c_device_by_node(info->of_node);
+	else
+		client = i2c_new_device(adap, info);
 	if (!client) {
 		err = -ENOMEM;
 		goto fail;