[PATCH] i2c: core: don't try to OF populate DDC i2c buses

[PATCH] i2c: core: don't try to OF populate DDC i2c buses

[Lucas Stach]

DDC buses are manually managed by their consumers to communicate
with the display. There is no need to try to populate OF childs.

This gets rid of the device create failed warning caused by the
core trying to populate a DDC bus below a OF device, which has
other childs nodes, that aren't i2c devices.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/i2c/i2c-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 5ab67219f71e..fbf7aade2ca7 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1686,6 +1686,10 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
 	if (!adap->dev.of_node)
 		return;
 
+	/* DDC buses have no OF populated childs */
+	if (adap->class == I2C_CLASS_DDC)
+		return;
+
 	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");
 
 	bus = of_get_child_by_name(adap->dev.of_node, "i2c-bus");
-- 
2.10.2

Re: [PATCH] i2c: core: don't try to OF populate DDC i2c buses

[Uwe Kleine-König]

On Wed, Nov 30, 2016 at 12:50:05PM +0100, Lucas Stach wrote:
> DDC buses are manually managed by their consumers to communicate
> with the display. There is no need to try to populate OF childs.

s/childs/children/ (in sum 3 times)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] i2c: core: don't try to OF populate DDC i2c buses

[Vladimir Zapolskiy]

Hello Lucas,

On 11/30/2016 01:50 PM, Lucas Stach wrote:
> DDC buses are manually managed by their consumers to communicate
> with the display. There is no need to try to populate OF childs.
> 
> This gets rid of the device create failed warning caused by the
> core trying to populate a DDC bus below a OF device, which has
> other childs nodes, that aren't i2c devices.

what kind of devices on a DDC bus represended by children nodes
do you reference here?

--
With best wishes,
Vladimir

Re: [PATCH] i2c: core: don't try to OF populate DDC i2c buses

[Lucas Stach]

Am Mittwoch, den 30.11.2016, 15:54 +0200 schrieb Vladimir Zapolskiy:
> Hello Lucas,
> 
> On 11/30/2016 01:50 PM, Lucas Stach wrote:
> > DDC buses are manually managed by their consumers to communicate
> > with the display. There is no need to try to populate OF childs.
> > 
> > This gets rid of the device create failed warning caused by the
> > core trying to populate a DDC bus below a OF device, which has
> > other childs nodes, that aren't i2c devices.
> 
> what kind of devices on a DDC bus represended by children nodes
> do you reference here?

None. The device registering the DDC i2c adapter might have an of_node.
The children of this device are not i2c devices, but for example port
nodes for the of_graph binding.

The same issue can be worked around if we make it explicit by placing an
"i2c-bus" subnode with no children into the parent device OF node. But
as the OF probing of devices on a DDC bus just doesn't make sense I
figured it would be good to just skip all this.

Regards,
Lucas

Re: [PATCH] i2c: core: don't try to OF populate DDC i2c buses

[Vladimir Zapolskiy]

On 11/30/2016 04:06 PM, Lucas Stach wrote:
> Am Mittwoch, den 30.11.2016, 15:54 +0200 schrieb Vladimir Zapolskiy:
>> Hello Lucas,
>>
>> On 11/30/2016 01:50 PM, Lucas Stach wrote:
>>> DDC buses are manually managed by their consumers to communicate
>>> with the display. There is no need to try to populate OF childs.
>>>
>>> This gets rid of the device create failed warning caused by the
>>> core trying to populate a DDC bus below a OF device, which has
>>> other childs nodes, that aren't i2c devices.
>>
>> what kind of devices on a DDC bus represended by children nodes
>> do you reference here?
> 
> None. The device registering the DDC i2c adapter might have an of_node.
> The children of this device are not i2c devices, but for example port
> nodes for the of_graph binding.

Port node shall not be a child of any DDC bus device node, otherwise it
contradicts to the hierarchy of hardware blocks. It is a common practice
to describe port nodes and DDC bus adapter as siblings (devices which
share the same display controller / encoder device). Do I miss something?

> The same issue can be worked around if we make it explicit by placing an
> "i2c-bus" subnode with no children into the parent device OF node. But
> as the OF probing of devices on a DDC bus just doesn't make sense I
> figured it would be good to just skip all this.
> 

Right, as a micro optimization the change makes sense (*), but the second
paragraph in the commit message is questionable.

In my opinion it makes sense to include the change, and the warning
you mention in the commit message needs its own attention as an indicator
of potentially wrongly chosen representation of the device hierarchy.

(*) experimentally I successfully performed test communications with
AT24 connected to DW HDMI DDC lines on i.MX6Q, so I can imagine exotic
boards, where DDC bus operates as an extra I2C bus. I'm unaware of any
such real cases, but if they exist your commit breaks them.

--
With best wishes,
Vladimir

Re: [PATCH] i2c: core: don't try to OF populate DDC i2c buses

[Lucas Stach]

Am Mittwoch, den 30.11.2016, 22:18 +0200 schrieb Vladimir Zapolskiy:
> On 11/30/2016 04:06 PM, Lucas Stach wrote:
> > Am Mittwoch, den 30.11.2016, 15:54 +0200 schrieb Vladimir Zapolskiy:
> >> Hello Lucas,
> >>
> >> On 11/30/2016 01:50 PM, Lucas Stach wrote:
> >>> DDC buses are manually managed by their consumers to communicate
> >>> with the display. There is no need to try to populate OF childs.
> >>>
> >>> This gets rid of the device create failed warning caused by the
> >>> core trying to populate a DDC bus below a OF device, which has
> >>> other childs nodes, that aren't i2c devices.
> >>
> >> what kind of devices on a DDC bus represended by children nodes
> >> do you reference here?
> > 
> > None. The device registering the DDC i2c adapter might have an of_node.
> > The children of this device are not i2c devices, but for example port
> > nodes for the of_graph binding.
> 
> Port node shall not be a child of any DDC bus device node, otherwise it
> contradicts to the hierarchy of hardware blocks. It is a common practice
> to describe port nodes and DDC bus adapter as siblings (devices which
> share the same display controller / encoder device). Do I miss something?
> 
> > The same issue can be worked around if we make it explicit by placing an
> > "i2c-bus" subnode with no children into the parent device OF node. But
> > as the OF probing of devices on a DDC bus just doesn't make sense I
> > figured it would be good to just skip all this.
> > 
> 
> Right, as a micro optimization the change makes sense (*), but the second
> paragraph in the commit message is questionable.
> 
> In my opinion it makes sense to include the change, and the warning
> you mention in the commit message needs its own attention as an indicator
> of potentially wrongly chosen representation of the device hierarchy.
> 
Okay to get away from the purely theoretical speaking here: the issue
this patch fixes is seen with the tc358767 parallel-to-eDP bridge.
(Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt)

The bridge itself is represented in the DT, so it has a OF node. It then
goes on to register the i2c over DP AUX channel i2c adapter with its own
device node, which is what all consumers of the i2c_add_adapter() API so
AFAICS.
The i2c adapter has no own representation in the DT, as it's a pure
software construct, after all it's just an emulated i2c bus over DP,
there are no physical i2c wires.

The i2c adapter then tries to populate the bridges children as if they
were i2c devices, which is clearly wrong. We could fix this by creating
an own device for the i2c adapter, but that's not how things are handled
today and would require changes to all consumers of this API.

Regards,
Lucas

Re: [PATCH] i2c: core: don't try to OF populate DDC i2c buses

[Vladimir Zapolskiy]

On 12/01/2016 12:07 PM, Lucas Stach wrote:
> Am Mittwoch, den 30.11.2016, 22:18 +0200 schrieb Vladimir Zapolskiy:
>> On 11/30/2016 04:06 PM, Lucas Stach wrote:
>>> Am Mittwoch, den 30.11.2016, 15:54 +0200 schrieb Vladimir Zapolskiy:
>>>> Hello Lucas,
>>>>
>>>> On 11/30/2016 01:50 PM, Lucas Stach wrote:
>>>>> DDC buses are manually managed by their consumers to communicate
>>>>> with the display. There is no need to try to populate OF childs.
>>>>>
>>>>> This gets rid of the device create failed warning caused by the
>>>>> core trying to populate a DDC bus below a OF device, which has
>>>>> other childs nodes, that aren't i2c devices.
>>>>
>>>> what kind of devices on a DDC bus represended by children nodes
>>>> do you reference here?
>>>
>>> None. The device registering the DDC i2c adapter might have an of_node.
>>> The children of this device are not i2c devices, but for example port
>>> nodes for the of_graph binding.
>>
>> Port node shall not be a child of any DDC bus device node, otherwise it
>> contradicts to the hierarchy of hardware blocks. It is a common practice
>> to describe port nodes and DDC bus adapter as siblings (devices which
>> share the same display controller / encoder device). Do I miss something?
>>
>>> The same issue can be worked around if we make it explicit by placing an
>>> "i2c-bus" subnode with no children into the parent device OF node. But
>>> as the OF probing of devices on a DDC bus just doesn't make sense I
>>> figured it would be good to just skip all this.
>>>
>>
>> Right, as a micro optimization the change makes sense (*), but the second
>> paragraph in the commit message is questionable.
>>
>> In my opinion it makes sense to include the change, and the warning
>> you mention in the commit message needs its own attention as an indicator
>> of potentially wrongly chosen representation of the device hierarchy.
>>
> Okay to get away from the purely theoretical speaking here: the issue
> this patch fixes is seen with the tc358767 parallel-to-eDP bridge.
> (Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.txt)
> 
> The bridge itself is represented in the DT, so it has a OF node. It then
> goes on to register the i2c over DP AUX channel i2c adapter with its own
> device node, which is what all consumers of the i2c_add_adapter() API so
> AFAICS.
> The i2c adapter has no own representation in the DT, as it's a pure
> software construct, after all it's just an emulated i2c bus over DP,
> there are no physical i2c wires.
> 
> The i2c adapter then tries to populate the bridges children as if they
> were i2c devices, which is clearly wrong. We could fix this by creating
> an own device for the i2c adapter, but that's not how things are handled
> today and would require changes to all consumers of this API.
> 

Is it correct to register dpaux device node as an I2C bus controller device
node, what is the purpose? If it is has to be done, then bindings/i2c/i2c.txt
documentation must be updated to remove #address-cells and #size-cells
properties from the list of required properties for DDC class adapters.

Shallow review of DTS files and dpaux drivers let me say that the change
below has no regressions (the change is untested):

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 3e6fe82..f91ade1 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1020,7 +1020,6 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
 	aux->ddc.class = I2C_CLASS_DDC;
 	aux->ddc.owner = THIS_MODULE;
 	aux->ddc.dev.parent = aux->dev;
-	aux->ddc.dev.of_node = aux->dev->of_node;
 
 	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
 		sizeof(aux->ddc.name));


--
With best wishes,
Vladimir

Re: [PATCH] i2c: core: don't try to OF populate DDC i2c buses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

Lucas,

> Shallow review of DTS files and dpaux drivers let me say that the change
> below has no regressions (the change is untested):
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 3e6fe82..f91ade1 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1020,7 +1020,6 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>  	aux->ddc.class = I2C_CLASS_DDC;
>  	aux->ddc.owner = THIS_MODULE;
>  	aux->ddc.dev.parent = aux->dev;
> -	aux->ddc.dev.of_node = aux->dev->of_node;
>  
>  	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
>  		sizeof(aux->ddc.name));

Is this an acceptable approach? It makes sense to me from an I2C PoV.

I have to say that I second Vladimir's arguments. There shouldn't be any
other I2C devices on DDC, but surely there is somebody somewhere hacking
this bus to do something.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c: core: don't try to OF populate DDC i2c buses

[Lucas Stach]

Am Sonntag, den 11.12.2016, 23:16 +0100 schrieb Wolfram Sang:
> Lucas,
> 
> > Shallow review of DTS files and dpaux drivers let me say that the change
> > below has no regressions (the change is untested):
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 3e6fe82..f91ade1 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -1020,7 +1020,6 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> >  	aux->ddc.class = I2C_CLASS_DDC;
> >  	aux->ddc.owner = THIS_MODULE;
> >  	aux->ddc.dev.parent = aux->dev;
> > -	aux->ddc.dev.of_node = aux->dev->of_node;
> >  
> >  	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
> >  		sizeof(aux->ddc.name));
> 
> Is this an acceptable approach? It makes sense to me from an I2C PoV.
> 
> I have to say that I second Vladimir's arguments. There shouldn't be any
> other I2C devices on DDC, but surely there is somebody somewhere hacking
> this bus to do something.

Yes, I've convinced myself that this is the better way.

Please drop this patch.

Regards,
Lucas