| Message ID | 20190125131142.26837-1-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] i2c: of: Try to find an I2C adapter matching the parent | expand |
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.
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
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
<html><head></head><body><div style="font-family: Verdana;font-size: 12.0px;"><div>
<div>Didn't knew about that line, sorry..</div>
<div>Sure you can include that.</div>
<div>I'm hoping patchwork won't thing it got tested twice by me..</div>
<div> </div>
<div>Tested-by: Tristan Bastian <tristan-c.bastian@gmx.de></div>
<div>
<div name="quote" style="margin:10px 5px 5px 10px; padding: 10px 0 10px 10px; border-left:2px solid #C3D9E5; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">
<div style="margin:0 0 10px 0;"><b>Gesendet:</b> Montag, 28. Januar 2019 um 09:10 Uhr<br/>
<b>Von:</b> "Thierry Reding" <thierry.reding@gmail.com><br/>
<b>An:</b> "Tristan Bastian" <tristan-c.bastian@gmx.de><br/>
<b>Cc:</b> "Wolfram Sang" <wsa@the-dreams.de>, "Rob Herring" <robh+dt@kernel.org>, "Lucas Stach" <l.stach@pengutronix.de>, "Andrzej Hajda" <a.hajda@samsung.com>, "Vlado Plaga" <rechner@vlado-do.de>, linux-i2c@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org<br/>
<b>Betreff:</b> Re: [PATCH v2] i2c: of: Try to find an I2C adapter matching the parent</div>
<div name="quoted-content">On Mon, Jan 28, 2019 at 09:08:15AM +0100, Thierry Reding wrote:<br/>
> On Sat, Jan 26, 2019 at 01:37:34PM +0100, Tristan Bastian wrote:<br/>
> > Am 25.01.19 um 14:11 schrieb Thierry Reding:<br/>
> > > From: Thierry Reding <treding@nvidia.com><br/>
> > ><br/>
> > > If an I2C adapter doesn't match the provided device tree node, also try<br/>
> > > matching the parent's device tree node. This allows finding an adapter<br/>
> > > based on the device node of the parent device that was used to register<br/>
> > > it.<br/>
> > ><br/>
> > > This fixes a regression on Tegra124-based Chromebooks (Nyan) where the<br/>
> > > eDP controller registers an I2C adapter that is used to read to EDID.<br/>
> > > After commit 993a815dcbb2 ("dt-bindings: panel: Add missing .txt<br/>
> > > suffix") this stopped working because the I2C adapter could no longer<br/>
> > > be found. The approach in this patch fixes the regression without<br/>
> > > introducing the issues that the above commit solved.<br/>
> > ><br/>
> > > Fixes: 17ab7806de0c ("drm: don't link DP aux i2c adapter to the hardware device node")<br/>
> > > Signed-off-by: Thierry Reding <treding@nvidia.com><br/>
> > > ---<br/>
> > > Changes in v2:<br/>
> > > - check for both device and parent device tree nodes for each device<br/>
> > > instead of looping through the list of devices twice<br/>
> > ><br/>
> > > drivers/i2c/i2c-core-of.c | 14 +++++++++++++-<br/>
> > > 1 file changed, 13 insertions(+), 1 deletion(-)<br/>
> > ><br/>
> > > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c<br/>
> > > index 6cb7ad608bcd..0f01cdba9d2c 100644<br/>
> > > --- a/drivers/i2c/i2c-core-of.c<br/>
> > > +++ b/drivers/i2c/i2c-core-of.c<br/>
> > > @@ -121,6 +121,17 @@ static int of_dev_node_match(struct device *dev, void *data)<br/>
> > > return dev->of_node == data;<br/>
> > > }<br/>
> > > +static int of_dev_or_parent_node_match(struct device *dev, void *data)<br/>
> > > +{<br/>
> > > + if (dev->of_node == data)<br/>
> > > + return 1;<br/>
> > > +<br/>
> > > + if (dev->parent)<br/>
> > > + return dev->parent->of_node == data;<br/>
> > > +<br/>
> > > + return 0;<br/>
> > > +}<br/>
> > > +<br/>
> > > /* must call put_device() when done with returned i2c_client device */<br/>
> > > struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)<br/>
> > > {<br/>
> > > @@ -145,7 +156,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)<br/>
> > > struct device *dev;<br/>
> > > struct i2c_adapter *adapter;<br/>
> > > - dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);<br/>
> > > + dev = bus_find_device(&i2c_bus_type, NULL, node,<br/>
> > > + of_dev_or_parent_node_match);<br/>
> > > if (!dev)<br/>
> > > return NULL;<br/>
> ><br/>
> > I've tested this and can confirm that this fixes the issue on the nyan-big<br/>
> > chromebook.<br/>
><br/>
> Excellent, thanks for testing! Typically if you've tested a patch and<br/>
> verified that it fixes the problem that you were seeing, it's good to<br/>
> send this on a line by itself along with your reply:<br/>
><br/>
> Tested-by: Tristan Bastian <tristan-c.bastian@gmx.de><br/>
<br/>
Oh my... that was stupid. I think patchwork might now pick this up...<br/>
<br/>
This also made me realize that I should've credited both Tristan and<br/>
Vlado as reporters in the commit message. I'll resend this.<br/>
<br/>
Tristan, is it okay if I include your Tested-by: tag in the v2?<br/>
<br/>
Thierry</div>
</div>
</div>
</div></div></body></html>
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>
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.
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.
> 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?
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
> 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 --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;