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;
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);
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);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);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
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
dri-devel@lists.freedesktop.org
-
Thierry Reding -
Tristan Bastian -
Tristan Bastian
-
Wolfram Sang