Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1436957 > unrolled thread

[PATCH] gpio: of: Allow overriding the device node

Back to article view | Back to linux.kernel

Contents

  [PATCH] gpio: of: Allow overriding the device node Thierry Reding <thierry.reding@gmail.com> - 2016-07-05 14:20 +0200
    Re: [PATCH] gpio: of: Allow overriding the device node Alexandre Courbot <gnurou@gmail.com> - 2016-07-05 15:20 +0200
    Re: [PATCH] gpio: of: Allow overriding the device node Linus Walleij <linus.walleij@linaro.org> - 2016-07-05 17:30 +0200
      Re: [PATCH] gpio: of: Allow overriding the device node Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-07-06 03:10 +0200
        Re: [PATCH] gpio: of: Allow overriding the device node Linus Walleij <linus.walleij@linaro.org> - 2016-07-06 11:10 +0200

#1436957 — [PATCH] gpio: of: Allow overriding the device node

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>
---
 drivers/gpio/gpiolib.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

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;
-- 
2.8.3

[toc] | [next] | [standalone]

#1436988

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>

[toc] | [prev] | [next] | [standalone]

#1437123

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

[toc] | [prev] | [next] | [standalone]

#1437350

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?






-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1437578

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web