diff mbox series

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

Message ID 20190125131142.26837-1-thierry.reding@gmail.com
State Deferred
Headers show
Series [v2] i2c: of: Try to find an I2C adapter matching the parent | expand

Commit Message

Thierry Reding Jan. 25, 2019, 1:11 p.m. UTC 
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.

This fixes a regression on Tegra124-based Chromebooks (Nyan) where the
eDP controller registers an I2C adapter that is used to read to EDID.
After commit 993a815dcbb2 ("dt-bindings: panel: Add missing .txt
suffix") this stopped working because the I2C adapter could no longer
be found. The approach in this patch fixes the regression without
introducing the issues that the above commit solved.

Fixes: 17ab7806de0c ("drm: don't link DP aux i2c adapter to the hardware device node")
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- check for both device and parent device tree nodes for each device
  instead of looping through the list of devices twice

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

Comments

Tristan Bastian Jan. 26, 2019, 12:37 p.m. UTC | #1 
Am 25.01.19 um 14:11 schrieb Thierry Reding:
> 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.
>
> This fixes a regression on Tegra124-based Chromebooks (Nyan) where the
> eDP controller registers an I2C adapter that is used to read to EDID.
> After commit 993a815dcbb2 ("dt-bindings: panel: Add missing .txt
> suffix") this stopped working because the I2C adapter could no longer
> be found. The approach in this patch fixes the regression without
> introducing the issues that the above commit solved.
>
> Fixes: 17ab7806de0c ("drm: don't link DP aux i2c adapter to the hardware device node")
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - check for both device and parent device tree nodes for each device
>    instead of looping through the list of devices twice
>
>   drivers/i2c/i2c-core-of.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index 6cb7ad608bcd..0f01cdba9d2c 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -121,6 +121,17 @@ static int of_dev_node_match(struct device *dev, void *data)
>   	return dev->of_node == data;
>   }
>   
> +static int of_dev_or_parent_node_match(struct device *dev, void *data)
> +{
> +	if (dev->of_node == data)
> +		return 1;
> +
> +	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)
>   {
> @@ -145,7 +156,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
>   	struct device *dev;
>   	struct i2c_adapter *adapter;
>   
> -	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
> +	dev = bus_find_device(&i2c_bus_type, NULL, node,
> +			      of_dev_or_parent_node_match);
>   	if (!dev)
>   		return NULL;
>   

I've tested this and can confirm that this fixes the issue on the 
nyan-big chromebook.
Is this fix going to be applied to the LTS kernels too?

Thanks.
Thierry Reding Jan. 28, 2019, 8:08 a.m. UTC | #2 
On Sat, Jan 26, 2019 at 01:37:34PM +0100, Tristan Bastian wrote:
> Am 25.01.19 um 14:11 schrieb Thierry Reding:
> > 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.
> > 
> > This fixes a regression on Tegra124-based Chromebooks (Nyan) where the
> > eDP controller registers an I2C adapter that is used to read to EDID.
> > After commit 993a815dcbb2 ("dt-bindings: panel: Add missing .txt
> > suffix") this stopped working because the I2C adapter could no longer
> > be found. The approach in this patch fixes the regression without
> > introducing the issues that the above commit solved.
> > 
> > Fixes: 17ab7806de0c ("drm: don't link DP aux i2c adapter to the hardware device node")
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v2:
> > - check for both device and parent device tree nodes for each device
> >    instead of looping through the list of devices twice
> > 
> >   drivers/i2c/i2c-core-of.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> > index 6cb7ad608bcd..0f01cdba9d2c 100644
> > --- a/drivers/i2c/i2c-core-of.c
> > +++ b/drivers/i2c/i2c-core-of.c
> > @@ -121,6 +121,17 @@ static int of_dev_node_match(struct device *dev, void *data)
> >   	return dev->of_node == data;
> >   }
> > +static int of_dev_or_parent_node_match(struct device *dev, void *data)
> > +{
> > +	if (dev->of_node == data)
> > +		return 1;
> > +
> > +	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)
> >   {
> > @@ -145,7 +156,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> >   	struct device *dev;
> >   	struct i2c_adapter *adapter;
> > -	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
> > +	dev = bus_find_device(&i2c_bus_type, NULL, node,
> > +			      of_dev_or_parent_node_match);
> >   	if (!dev)
> >   		return NULL;
> 
> I've tested this and can confirm that this fixes the issue on the nyan-big
> chromebook.

Excellent, thanks for testing! Typically if you've tested a patch and
verified that it fixes the problem that you were seeing, it's good to
send this on a line by itself along with your reply:

Tested-by: Tristan Bastian <tristan-c.bastian@gmx.de>

Patchwork will pick this up and it will become part of the commit
message when the patch is applied. This gives you the credit you deserve
for going through the trouble of testing the change.

> Is this fix going to be applied to the LTS kernels too?

The "Fixes:" line in the commit message should ensure that this does get
backported to relevant stable kernels.

Thierry
Thierry Reding Jan. 28, 2019, 8:10 a.m. UTC | #3 
On Mon, Jan 28, 2019 at 09:08:15AM +0100, Thierry Reding wrote:
> On Sat, Jan 26, 2019 at 01:37:34PM +0100, Tristan Bastian wrote:
> > Am 25.01.19 um 14:11 schrieb Thierry Reding:
> > > 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.
> > > 
> > > This fixes a regression on Tegra124-based Chromebooks (Nyan) where the
> > > eDP controller registers an I2C adapter that is used to read to EDID.
> > > After commit 993a815dcbb2 ("dt-bindings: panel: Add missing .txt
> > > suffix") this stopped working because the I2C adapter could no longer
> > > be found. The approach in this patch fixes the regression without
> > > introducing the issues that the above commit solved.
> > > 
> > > Fixes: 17ab7806de0c ("drm: don't link DP aux i2c adapter to the hardware device node")
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > > Changes in v2:
> > > - check for both device and parent device tree nodes for each device
> > >    instead of looping through the list of devices twice
> > > 
> > >   drivers/i2c/i2c-core-of.c | 14 +++++++++++++-
> > >   1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> > > index 6cb7ad608bcd..0f01cdba9d2c 100644
> > > --- a/drivers/i2c/i2c-core-of.c
> > > +++ b/drivers/i2c/i2c-core-of.c
> > > @@ -121,6 +121,17 @@ static int of_dev_node_match(struct device *dev, void *data)
> > >   	return dev->of_node == data;
> > >   }
> > > +static int of_dev_or_parent_node_match(struct device *dev, void *data)
> > > +{
> > > +	if (dev->of_node == data)
> > > +		return 1;
> > > +
> > > +	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)
> > >   {
> > > @@ -145,7 +156,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> > >   	struct device *dev;
> > >   	struct i2c_adapter *adapter;
> > > -	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
> > > +	dev = bus_find_device(&i2c_bus_type, NULL, node,
> > > +			      of_dev_or_parent_node_match);
> > >   	if (!dev)
> > >   		return NULL;
> > 
> > I've tested this and can confirm that this fixes the issue on the nyan-big
> > chromebook.
> 
> Excellent, thanks for testing! Typically if you've tested a patch and
> verified that it fixes the problem that you were seeing, it's good to
> send this on a line by itself along with your reply:
> 
> Tested-by: Tristan Bastian <tristan-c.bastian@gmx.de>

Oh my... that was stupid. I think patchwork might now pick this up...

This also made me realize that I should've credited both Tristan and
Vlado as reporters in the commit message. I'll resend this.

Tristan, is it okay if I include your Tested-by: tag in the v2?

Thierry
Tristan Bastian Jan. 28, 2019, 9:26 a.m. UTC | #4 
On Mon, Jan 28, 2019 at 09:08:15AM +0100, Thierry Reding wrote:
> On Sat, Jan 26, 2019 at 01:37:34PM +0100, Tristan Bastian wrote:
> > Am 25.01.19 um 14:11 schrieb Thierry Reding:
> > > 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.
> > >
> > > This fixes a regression on Tegra124-based Chromebooks (Nyan) where the
> > > eDP controller registers an I2C adapter that is used to read to EDID.
> > > After commit 993a815dcbb2 ("dt-bindings: panel: Add missing .txt
> > > suffix") this stopped working because the I2C adapter could no longer
> > > be found. The approach in this patch fixes the regression without
> > > introducing the issues that the above commit solved.
> > >
> > > Fixes: 17ab7806de0c ("drm: don't link DP aux i2c adapter to the hardware device node")
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > > Changes in v2:
> > > - check for both device and parent device tree nodes for each device
> > > instead of looping through the list of devices twice
> > >
> > > drivers/i2c/i2c-core-of.c | 14 +++++++++++++-
> > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> > > index 6cb7ad608bcd..0f01cdba9d2c 100644
> > > --- a/drivers/i2c/i2c-core-of.c
> > > +++ b/drivers/i2c/i2c-core-of.c
> > > @@ -121,6 +121,17 @@ static int of_dev_node_match(struct device *dev, void *data)
> > > return dev->of_node == data;
> > > }
> > > +static int of_dev_or_parent_node_match(struct device *dev, void *data)
> > > +{
> > > + if (dev->of_node == data)
> > > + return 1;
> > > +
> > > + 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)
> > > {
> > > @@ -145,7 +156,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
> > > struct device *dev;
> > > struct i2c_adapter *adapter;
> > > - dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
> > > + dev = bus_find_device(&i2c_bus_type, NULL, node,
> > > + of_dev_or_parent_node_match);
> > > if (!dev)
> > > return NULL;
> >
> > I've tested this and can confirm that this fixes the issue on the nyan-big
> > chromebook.
>
> Excellent, thanks for testing! Typically if you've tested a patch and
> verified that it fixes the problem that you were seeing, it's good to
> send this on a line by itself along with your reply:
>
> Tested-by: Tristan Bastian <tristan-c.bastian@gmx.de>

Resending because first mail contained html and was blocked by the mailing lists..
Didn't knew about that line, sorry..
Sure you can include that.
I'm hoping patchwork won't thing it got tested twice by me..
 
Tested-by: Tristan Bastian <tristan-c.bastian@gmx.de>
Wolfram Sang Feb. 5, 2019, 12:44 p.m. UTC | #5 
On Fri, Jan 25, 2019 at 02:11:42PM +0100, 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.
> 
> This fixes a regression on Tegra124-based Chromebooks (Nyan) where the
> eDP controller registers an I2C adapter that is used to read to EDID.
> After commit 993a815dcbb2 ("dt-bindings: panel: Add missing .txt
> suffix") this stopped working because the I2C adapter could no longer
> be found. The approach in this patch fixes the regression without
> introducing the issues that the above commit solved.
> 
> Fixes: 17ab7806de0c ("drm: don't link DP aux i2c adapter to the hardware device node")
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Removed the duplicated Tested-by and applied to for-next, thanks!

I applied to -next because I want this core change more regression
testing in next. If this goes good, I will do a cleanup series to not
use the of_node of the parent twice.
Wolfram Sang Feb. 6, 2019, 9:38 a.m. UTC | #6 
On Tue, Feb 05, 2019 at 01:44:44PM +0100, Wolfram Sang wrote:
> On Fri, Jan 25, 2019 at 02:11:42PM +0100, 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.
> > 
> > This fixes a regression on Tegra124-based Chromebooks (Nyan) where the
> > eDP controller registers an I2C adapter that is used to read to EDID.
> > After commit 993a815dcbb2 ("dt-bindings: panel: Add missing .txt
> > suffix") this stopped working because the I2C adapter could no longer
> > be found. The approach in this patch fixes the regression without
> > introducing the issues that the above commit solved.
> > 
> > Fixes: 17ab7806de0c ("drm: don't link DP aux i2c adapter to the hardware device node")
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> Removed the duplicated Tested-by and applied to for-next, thanks!
> 
> I applied to -next because I want this core change more regression
> testing in next. If this goes good, I will do a cleanup series to not
> use the of_node of the parent twice.

And there is a regression! Good that I didn't push out before
double-checking. No one noticed that this breaks registering child
devices because of_i2c_register_devices() doesn't have a pointer to work
with anymore?

Removing that patch from the queue.
Wolfram Sang Feb. 6, 2019, 9:49 a.m. UTC | #7 
> And there is a regression! Good that I didn't push out before
> double-checking. No one noticed that this breaks registering child
> devices because of_i2c_register_devices() doesn't have a pointer to work
> with anymore?

Well, sorry, I forgot an important detail. There is no regression
because most drivers still populate adap->dev.of_data with the node
pointer of their parent. I experimentally removed this from my driver
under test motivated by this comment from the commit in the Fixes: tag:

"Linking it to the device node of the parent device is wrong, as it
leads to 2 devices sharing the same device node, which is bad practice,"

But removing this bad practice from I2C core is more work. I wonder now
if we are in some inconsistent in-between state if I apply this patch as
is?
Thierry Reding Feb. 6, 2019, 12:07 p.m. UTC | #8 
On Wed, Feb 06, 2019 at 10:49:12AM +0100, Wolfram Sang wrote:
> 
> > And there is a regression! Good that I didn't push out before
> > double-checking. No one noticed that this breaks registering child
> > devices because of_i2c_register_devices() doesn't have a pointer to work
> > with anymore?
> 
> Well, sorry, I forgot an important detail. There is no regression
> because most drivers still populate adap->dev.of_data with the node
> pointer of their parent. I experimentally removed this from my driver
> under test motivated by this comment from the commit in the Fixes: tag:
> 
> "Linking it to the device node of the parent device is wrong, as it
> leads to 2 devices sharing the same device node, which is bad practice,"
> 
> But removing this bad practice from I2C core is more work. I wonder now
> if we are in some inconsistent in-between state if I apply this patch as
> is?

I think this patch would serve as preparatory work to remove the sharing
of device nodes. There shouldn't be any regressions here because we only
fall back to the parent's ->of_node if the I2C adapter's ->of_node does
not match. Since the I2C adapter's ->of_node match is what we currently
do, the only thing that this patch does is add a fallback for the cases
where the I2C adapter's ->of_node is not set.

As far as I can tell, the only code where this should matter is the
drm_dp_aux helpers where the I2C adapter's ->of_node is no longer being
set because of the commit that introduced the regression for Tegra124
Nyan (and Venice2) boards.

So I think this patch is safe to apply and as you suggested this can be
used as the baseline for cleaning up all the cases where we reuse the
parent's ->of_node for the I2C adapter's ->of_node.

So I guess you could say we're in some in-between state, but I don't
think it's inconsistent. It just allows us to do this step by step,
which I think is good.

Thierry
Wolfram Sang Feb. 8, 2019, 6:35 p.m. UTC | #9 
> So I guess you could say we're in some in-between state, but I don't
> think it's inconsistent. It just allows us to do this step by step,
> which I think is good.

Well, I am still not super happy, but it fixes a regression, so I will
keep it in for-next.
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 6cb7ad608bcd..0f01cdba9d2c 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -121,6 +121,17 @@  static int of_dev_node_match(struct device *dev, void *data)
 	return dev->of_node == data;
 }
 
+static int of_dev_or_parent_node_match(struct device *dev, void *data)
+{
+	if (dev->of_node == data)
+		return 1;
+
+	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)
 {
@@ -145,7 +156,8 @@  struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 	struct device *dev;
 	struct i2c_adapter *adapter;
 
-	dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
+	dev = bus_find_device(&i2c_bus_type, NULL, node,
+			      of_dev_or_parent_node_match);
 	if (!dev)
 		return NULL;