i2c: of: Try to find an I2C adapter matching the parent

From: Thierry Reding <treding@nvidia.com>

If an I2C adapter doesn't match the provided device tree node, also try
matching the parent's device tree node. This allows finding an adapter
based on the device node of the parent device that was used to register
it.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Hi Wolfram,

This is a fix for the issue discussed in a long email thread a couple of
months ago. In a nutshell the issue is that we want to be able to find a
I2C-over-AUX adapter based on the device tree node (this is to hook up a
I2C adapter for use as DDC to query EDID from a display panel, for
example).

Here's a link to the prior discussion, which ended up getting split into
two for some reason, possibly because the initial submission and
subsequent discussion took place over an extended period of time:

	https://patchwork.kernel.org/patch/9516105/
	https://patchwork.kernel.org/patch/10200879/

I never got around to submitting the patch properly, but here it is. As
mentioned in the discussion linked to above, I think the problem here is
that the new lookup via the parent device only happens when looking for
the adapter from its device tree node. However, this also means that the
children for the controller won't be added for these devices, because it
only happens for adapters that have the dev->of_node set, not for the
node reference by dev->parent->of_node. However, I'm not sure if that's
really a problem. A device that doesn't have dev->of_node set would be a
"virtual" I2C bus, as in the case of I2C-over-AUX. Proper I2C busses are
still going to have to have their dev->of_node set, otherwise any child
devices listed in device tree won't be added.

Anyway, let me know what you think of this. Also adding Lucas, Rob and
Andrzej to the discussion since they were involved back at the time.

Thanks,
Thierry

 drivers/i2c/i2c-core-of.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Tested-by: Vlado Plaga <rechner@vlado-do.de>

This patch works for me - without it my screen stays black (Acer CB5-311).

On 25/09/2018 18:06, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> If an I2C adapter doesn't match the provided device tree node, also try
> matching the parent's device tree node. This allows finding an adapter
> based on the device node of the parent device that was used to register
> it.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
[...]
>  drivers/i2c/i2c-core-of.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index 6cb7ad608bcd..37d34885ea2d 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -121,6 +121,14 @@ static int of_dev_node_match(struct device *dev, void *data)
>  	return dev->of_node == data;
>  }
>  
> +static int of_parent_node_match(struct device *dev, void *data)
> +{
> +	if (dev->parent)
> +		return dev->parent->of_node == data;
> +
> +	return 0;
> +}
> +
>  /* must call put_device() when done with returned i2c_client device */
>  struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
>  {
> @@ -146,6 +154,9 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
>  	struct i2c_adapter *adapter;
>  
>  	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
> +	if (!dev)
> +		dev = bus_find_device(&i2c_bus_type, NULL, node, of_parent_node_match);
> +
>  	if (!dev)
>  		return NULL;
>

On Tue, Sep 25, 2018 at 06:06:11PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> If an I2C adapter doesn't match the provided device tree node, also try
> matching the parent's device tree node. This allows finding an adapter
> based on the device node of the parent device that was used to register
> it.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Hi Wolfram,
> 
> This is a fix for the issue discussed in a long email thread a couple of
> months ago. In a nutshell the issue is that we want to be able to find a
> I2C-over-AUX adapter based on the device tree node (this is to hook up a
> I2C adapter for use as DDC to query EDID from a display panel, for
> example).
> 
> Here's a link to the prior discussion, which ended up getting split into
> two for some reason, possibly because the initial submission and
> subsequent discussion took place over an extended period of time:
> 
> 	https://patchwork.kernel.org/patch/9516105/
> 	https://patchwork.kernel.org/patch/10200879/

Ok, I read that...

> 
> I never got around to submitting the patch properly, but here it is. As
> mentioned in the discussion linked to above, I think the problem here is
> that the new lookup via the parent device only happens when looking for
> the adapter from its device tree node. However, this also means that the
> children for the controller won't be added for these devices, because it
> only happens for adapters that have the dev->of_node set, not for the
> node reference by dev->parent->of_node. However, I'm not sure if that's
> really a problem. A device that doesn't have dev->of_node set would be a
> "virtual" I2C bus, as in the case of I2C-over-AUX. Proper I2C busses are
> still going to have to have their dev->of_node set, otherwise any child
> devices listed in device tree won't be added.
> 
> Anyway, let me know what you think of this. Also adding Lucas, Rob and
> Andrzej to the discussion since they were involved back at the time.

... and it looks okay from that discussion point of view. And we could
stop reusing the parent's of_node for the adapter's of_node. Which is
better practice. Rob, can you confirm.

> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index 6cb7ad608bcd..37d34885ea2d 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -121,6 +121,14 @@ static int of_dev_node_match(struct device *dev, void *data)
>  	return dev->of_node == data;
>  }
>  
> +static int of_parent_node_match(struct device *dev, void *data)
> +{
> +	if (dev->parent)
> +		return dev->parent->of_node == data;
> +
> +	return 0;
> +}
> +
>  /* must call put_device() when done with returned i2c_client device */
>  struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
>  {
> @@ -146,6 +154,9 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
>  	struct i2c_adapter *adapter;
>  
>  	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
> +	if (!dev)
> +		dev = bus_find_device(&i2c_bus_type, NULL, node, of_parent_node_match);
> +

To avoid the double loop iteration, maybe introduce
'of_dev_or_parent_node_match' and merge those two functionalities?

Regards,

   Wolfram

On 25.09.2018 18:06, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> If an I2C adapter doesn't match the provided device tree node, also try
> matching the parent's device tree node. This allows finding an adapter
> based on the device node of the parent device that was used to register
> it.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Hi Wolfram,
>
> This is a fix for the issue discussed in a long email thread a couple of
> months ago. In a nutshell the issue is that we want to be able to find a
> I2C-over-AUX adapter based on the device tree node (this is to hook up a
> I2C adapter for use as DDC to query EDID from a display panel, for
> example).
>
> Here's a link to the prior discussion, which ended up getting split into
> two for some reason, possibly because the initial submission and
> subsequent discussion took place over an extended period of time:
>
> 	https://patchwork.kernel.org/patch/9516105/
> 	https://patchwork.kernel.org/patch/10200879/
>
> I never got around to submitting the patch properly, but here it is. As
> mentioned in the discussion linked to above, I think the problem here is
> that the new lookup via the parent device only happens when looking for
> the adapter from its device tree node. However, this also means that the
> children for the controller won't be added for these devices, because it
> only happens for adapters that have the dev->of_node set, not for the
> node reference by dev->parent->of_node. However, I'm not sure if that's
> really a problem. A device that doesn't have dev->of_node set would be a
> "virtual" I2C bus, as in the case of I2C-over-AUX. Proper I2C busses are
> still going to have to have their dev->of_node set, otherwise any child
> devices listed in device tree won't be added.
>
> Anyway, let me know what you think of this. Also adding Lucas, Rob and
> Andrzej to the discussion since they were involved back at the time.
>
> Thanks,
> Thierry
>
>  drivers/i2c/i2c-core-of.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index 6cb7ad608bcd..37d34885ea2d 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -121,6 +121,14 @@ static int of_dev_node_match(struct device *dev, void *data)
>  	return dev->of_node == data;
>  }
>  
> +static int of_parent_node_match(struct device *dev, void *data)
> +{
> +	if (dev->parent)
> +		return dev->parent->of_node == data;
> +
> +	return 0;
> +}
> +
>  /* must call put_device() when done with returned i2c_client device */
>  struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
>  {
> @@ -146,6 +154,9 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
>  	struct i2c_adapter *adapter;
>  
>  	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
> +	if (!dev)
> +		dev = bus_find_device(&i2c_bus_type, NULL, node, of_parent_node_match);
> +

Wouldn't be better to merge of_dev_node_match and of_parent_node_match 
functions and call bus_find_device once?
Then expression:

dev->of_node ?: dev->parent->of_node

would identify adapter created from DT (with assumption dev  has always
parent).
There are few questions then:
1. Is there a case when dev->parent can be NULL? If not, NULL check can
be added to adapter registration and it can be removed from here.
2. Is there a case when dev->of_node != NULL and dev->of_node !=
dev->parent->of_node? If not, just dev->parent->of_node can be tested in
match function.

Regards
Andrzej

>  	if (!dev)
>  		return NULL;
>

Any update on this patch? I thought it was interesting.

BTW adding Rob (finally) to CC...

On Mon, Oct 01, 2018 at 01:02:30AM +0200, Wolfram Sang wrote:
> On Tue, Sep 25, 2018 at 06:06:11PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > If an I2C adapter doesn't match the provided device tree node, also try
> > matching the parent's device tree node. This allows finding an adapter
> > based on the device node of the parent device that was used to register
> > it.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Hi Wolfram,
> > 
> > This is a fix for the issue discussed in a long email thread a couple of
> > months ago. In a nutshell the issue is that we want to be able to find a
> > I2C-over-AUX adapter based on the device tree node (this is to hook up a
> > I2C adapter for use as DDC to query EDID from a display panel, for
> > example).
> > 
> > Here's a link to the prior discussion, which ended up getting split into
> > two for some reason, possibly because the initial submission and
> > subsequent discussion took place over an extended period of time:
> > 
> > 	https://patchwork.kernel.org/patch/9516105/
> > 	https://patchwork.kernel.org/patch/10200879/
> 
> Ok, I read that...
> 
> > 
> > I never got around to submitting the patch properly, but here it is. As
> > mentioned in the discussion linked to above, I think the problem here is
> > that the new lookup via the parent device only happens when looking for
> > the adapter from its device tree node. However, this also means that the
> > children for the controller won't be added for these devices, because it
> > only happens for adapters that have the dev->of_node set, not for the
> > node reference by dev->parent->of_node. However, I'm not sure if that's
> > really a problem. A device that doesn't have dev->of_node set would be a
> > "virtual" I2C bus, as in the case of I2C-over-AUX. Proper I2C busses are
> > still going to have to have their dev->of_node set, otherwise any child
> > devices listed in device tree won't be added.
> > 
> > Anyway, let me know what you think of this. Also adding Lucas, Rob and
> > Andrzej to the discussion since they were involved back at the time.
> 
> ... and it looks okay from that discussion point of view. And we could
> stop reusing the parent's of_node for the adapter's of_node. Which is
> better practice. Rob, can you confirm.
> 
> > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> > index 6cb7ad608bcd..37d34885ea2d 100644
> > --- a/drivers/i2c/i2c-core-of.c
> > +++ b/drivers/i2c/i2c-core-of.c
> > @@ -121,6 +121,14 @@ static int of_dev_node_match(struct device *dev, void *data)
> >  	return dev->of_node == data;
> >  }
> >  
> > +static int of_parent_node_match(struct device *dev, void *data)
> > +{
> > +	if (dev->parent)
> > +		return dev->parent->of_node == data;
> > +
> > +	return 0;
> > +}
> > +
> >  /* must call put_device() when done with returned i2c_client device */
> >  struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
> >  {
> > @@ -146,6 +154,9 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> >  	struct i2c_adapter *adapter;
> >  
> >  	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
> > +	if (!dev)
> > +		dev = bus_find_device(&i2c_bus_type, NULL, node, of_parent_node_match);
> > +
> 
> To avoid the double loop iteration, maybe introduce
> 'of_dev_or_parent_node_match' and merge those two functionalities?
> 
> Regards,
> 
>    Wolfram