| Message ID | 20160705121114.10650-1-thierry.reding@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jul 5, 2016 at 9:11 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > From: Thierry Reding <treding@nvidia.com> > > When registering a GPIO chip, drivers can override the device tree node > associated with the chip by setting the chip's ->of_node field. If set, > this field is supposed to take precedence over the ->parent->of_node > field, but the code doesn't actually do that. > > Commit 762c2e46c059 ("gpio: of: remove of_gpiochip_and_xlate() and > struct gg_data") exposes this because it now no longer matches on the > GPIO chip's ->of_node field, but the GPIO device's ->of_node field that > is set using the procedure described above. Acked-by: Alexandre Courbot <acourbot@nvidia.com> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 5, 2016 at 2:11 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > From: Thierry Reding <treding@nvidia.com> > > When registering a GPIO chip, drivers can override the device tree node > associated with the chip by setting the chip's ->of_node field. If set, > this field is supposed to take precedence over the ->parent->of_node > field, but the code doesn't actually do that. > > Commit 762c2e46c059 ("gpio: of: remove of_gpiochip_and_xlate() and > struct gg_data") exposes this because it now no longer matches on the > GPIO chip's ->of_node field, but the GPIO device's ->of_node field that > is set using the procedure described above. > > Signed-off-by: Thierry Reding <treding@nvidia.com> Thanks for catching this, patch applied with Alexandre's ACK. Masahiro: does this look all right to you? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi. 2016-07-06 0:25 GMT+09:00 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Jul 5, 2016 at 2:11 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > >> From: Thierry Reding <treding@nvidia.com> >> >> When registering a GPIO chip, drivers can override the device tree node >> associated with the chip by setting the chip's ->of_node field. If set, >> this field is supposed to take precedence over the ->parent->of_node >> field, but the code doesn't actually do that. >> >> Commit 762c2e46c059 ("gpio: of: remove of_gpiochip_and_xlate() and >> struct gg_data") exposes this because it now no longer matches on the >> GPIO chip's ->of_node field, but the GPIO device's ->of_node field that >> is set using the procedure described above. >> >> Signed-off-by: Thierry Reding <treding@nvidia.com> > > Thanks for catching this, patch applied with Alexandre's ACK. > > Masahiro: does this look all right to you? Yes. Now, the code matches to the comment. Nice! Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com> Question: When we reference the node of gpiochip, we should use chip->gpiodev->dev->of_node instead of chip->of_node because we can make chip->of_node optional as long as chip->parent is set in the probe method. Correct?
On Wed, Jul 6, 2016 at 3:05 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Question: > > When we reference the node of gpiochip, > we should use > chip->gpiodev->dev->of_node > > instead of > chip->of_node > > because we can make chip->of_node optional > as long as chip->parent is set in the probe method. > > Correct? Yes, if you see any bug with respect to this, just fix them. Overall my appraisal of the situation is that it's a mess. chip->of_node should not exist, we need to investigate all cases where this is actually used by drivers and kill them all off, they should all be using the parent to pass the device containing the right of node so we can get rid of this: #ifdef CONFIG_OF_GPIO /* If the gpiochip has an assigned OF node this takes precedence */ if (chip->of_node) gdev->dev.of_node = chip->of_node; #endif I don't know why this extra OF node is there in the first place and in which circumstances it may be used, maybe Grant knows. I understand so much as that it would be a situation where you have a GPIO device node but it does not have a corresponding device of any type, but I fail to see what circumstance would make that happen. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 47a3cc0e437b..eae7664b2723 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1047,13 +1047,14 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data) if (chip->parent) { gdev->dev.parent = chip->parent; gdev->dev.of_node = chip->parent->of_node; - } else { + } + #ifdef CONFIG_OF_GPIO /* If the gpiochip has an assigned OF node this takes precedence */ - if (chip->of_node) - gdev->dev.of_node = chip->of_node; + if (chip->of_node) + gdev->dev.of_node = chip->of_node; #endif - } + gdev->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL); if (gdev->id < 0) { status = gdev->id;