[PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Saravana Kannan]

To provide backward compatibility for boards that use deprecated DT
bindings, we need to add fw_devlink support for "gpio" and "gpios".

Cc: linux-tegra <linux-tegra@vger.kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 5f9eed79a8aa..1c8c65c4a887 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1258,6 +1258,8 @@ DEFINE_SIMPLE_PROP(pinctrl5, "pinctrl-5", NULL)
 DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
 DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
 DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
+DEFINE_SIMPLE_PROP(gpio_compat, "gpio", "#gpio-cells")
+DEFINE_SIMPLE_PROP(gpios_compat, "gpios", "#gpio-cells")
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
 DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
@@ -1296,6 +1298,8 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_pinctrl6, },
 	{ .parse_prop = parse_pinctrl7, },
 	{ .parse_prop = parse_pinctrl8, },
+	{ .parse_prop = parse_gpio_compat, },
+	{ .parse_prop = parse_gpios_compat, },
 	{ .parse_prop = parse_regulators, },
 	{ .parse_prop = parse_gpio, },
 	{ .parse_prop = parse_gpios, },
-- 
2.30.0.284.gd98b1dd5eaa7-goog

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Linus Walleij]

On Fri, Jan 15, 2021 at 10:02 PM Saravana Kannan <saravanak@google.com> wrote:

> To provide backward compatibility for boards that use deprecated DT
> bindings, we need to add fw_devlink support for "gpio" and "gpios".
>
> Cc: linux-tegra <linux-tegra@vger.kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> Tested-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Saravana Kannan <saravanak@google.com>

"gpios" is a valid non legacy property I think.

Anyways:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Saravana Kannan]

On Mon, Jan 18, 2021 at 7:32 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Jan 15, 2021 at 10:02 PM Saravana Kannan <saravanak@google.com> wrote:
>
> > To provide backward compatibility for boards that use deprecated DT
> > bindings, we need to add fw_devlink support for "gpio" and "gpios".
> >
> > Cc: linux-tegra <linux-tegra@vger.kernel.org>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> > Tested-by: Jon Hunter <jonathanh@nvidia.com>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> "gpios" is a valid non legacy property I think.

I checked :) Quoting the documentation [1]:
"While a non-existent <name> is considered valid for compatibility
reasons (resolving to the "gpios" property), it is not allowed for new
bindings."

[1] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio.txt#n8

> Anyways:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

Greg/Rob,

Can we pull this into driver-core-next please? It fixes issues on some
boards with fw_devlink=on.

-Saravana

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Geert Uytterhoeven]

Hi Saravana,

On Mon, Jan 18, 2021 at 11:14 PM Saravana Kannan <saravanak@google.com> wrote:
> On Mon, Jan 18, 2021 at 7:32 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Fri, Jan 15, 2021 at 10:02 PM Saravana Kannan <saravanak@google.com> wrote:
> > > To provide backward compatibility for boards that use deprecated DT
> > > bindings, we need to add fw_devlink support for "gpio" and "gpios".
> > >
> > > Cc: linux-tegra <linux-tegra@vger.kernel.org>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> > > Tested-by: Jon Hunter <jonathanh@nvidia.com>
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>

Thanks for your patch!

> > "gpios" is a valid non legacy property I think.
>
> I checked :) Quoting the documentation [1]:
> "While a non-existent <name> is considered valid for compatibility
> reasons (resolving to the "gpios" property), it is not allowed for new
> bindings."
>
> [1] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio.txt#n8
>
> > Anyways:
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Thanks!
>
> Greg/Rob,
>
> Can we pull this into driver-core-next please? It fixes issues on some
> boards with fw_devlink=on.

On r8a77951-salvator-xs.dts, it introduces one more failure:

    OF: /soc/i2c@e66d8000/gpio@20/pcie-sata-switch-hog: could not get
#gpio-cells for /cpus/cpu@102

Seems like it doesn't parse gpios properties in GPIO hogs correctly.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Linus Walleij]

On Tue, Jan 19, 2021 at 9:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > Can we pull this into driver-core-next please? It fixes issues on some
> > boards with fw_devlink=on.
>
> On r8a77951-salvator-xs.dts, it introduces one more failure:
>
>     OF: /soc/i2c@e66d8000/gpio@20/pcie-sata-switch-hog: could not get
> #gpio-cells for /cpus/cpu@102
>
> Seems like it doesn't parse gpios properties in GPIO hogs correctly.

Could it be that the code assumes no self-referencing phandles?
(Just guessing...)

Yours,
Linus Walleij

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

On Tue, Jan 19, 2021 at 11:20:24AM +0100, Linus Walleij wrote:
> On Tue, Jan 19, 2021 at 9:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > > Can we pull this into driver-core-next please? It fixes issues on some
> > > boards with fw_devlink=on.
> >
> > On r8a77951-salvator-xs.dts, it introduces one more failure:
> >
> >     OF: /soc/i2c@e66d8000/gpio@20/pcie-sata-switch-hog: could not get
> > #gpio-cells for /cpus/cpu@102
> >
> > Seems like it doesn't parse gpios properties in GPIO hogs correctly.
> 
> Could it be that the code assumes no self-referencing phandles?
> (Just guessing...)

Well, since this is scanning the DT and creating device links between
producers and consumers, there's really no point in handling self-
references, so I think that's fine.

However, what this probably should do is skip the node if it's marked
as a GPIO hog to avoid the error message.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Saravana Kannan]

On Tue, Jan 19, 2021 at 2:20 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Jan 19, 2021 at 9:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > > Can we pull this into driver-core-next please? It fixes issues on some
> > > boards with fw_devlink=on.
> >
> > On r8a77951-salvator-xs.dts, it introduces one more failure:
> >
> >     OF: /soc/i2c@e66d8000/gpio@20/pcie-sata-switch-hog: could not get
> > #gpio-cells for /cpus/cpu@102

Geert,

One good thing is that it's noticing this being weird and ignoring it
in your particular board. I *think* it interprets the "7" as a phandle
and that's cpu@102 and realizes it's not a gpio-controller. For at
least in your case, it's a safe failure.

> >
> > Seems like it doesn't parse gpios properties in GPIO hogs correctly.
>
> Could it be that the code assumes no self-referencing phandles?
> (Just guessing...)
>

Linus,

Ok I tried to understand what gpio-hogs means. It's not fully clear to
me. But it looks like if a gpio-controller has a gpio-hog, then it
doesn't have/need gpio-cells? Is that right?

So if a gpio-controller has a gpio-hog, can it ever be referred to by
another consumer in DT using blah-gpios = ...? If so, I don't see any
obvious code that's handling the missing gpio-cells in this case.

Long story short, please help me understand gpio-hog in the context of
finding dependencies in DT.

Thanks,
Saravana

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Geert Uytterhoeven]

Hi Saravana,

On Tue, Jan 19, 2021 at 6:54 PM Saravana Kannan <saravanak@google.com> wrote:
> On Tue, Jan 19, 2021 at 2:20 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Jan 19, 2021 at 9:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > Can we pull this into driver-core-next please? It fixes issues on some
> > > > boards with fw_devlink=on.
> > >
> > > On r8a77951-salvator-xs.dts, it introduces one more failure:
> > >
> > >     OF: /soc/i2c@e66d8000/gpio@20/pcie-sata-switch-hog: could not get
> > > #gpio-cells for /cpus/cpu@102
>
> Geert,
>
> One good thing is that it's noticing this being weird and ignoring it
> in your particular board. I *think* it interprets the "7" as a phandle
> and that's cpu@102 and realizes it's not a gpio-controller. For at
> least in your case, it's a safe failure.

While 7 is the GPIO index, relative to the current GPIO controller,
represented by the parent device node.

> > > Seems like it doesn't parse gpios properties in GPIO hogs correctly.
> >
> > Could it be that the code assumes no self-referencing phandles?
> > (Just guessing...)
>
> Ok I tried to understand what gpio-hogs means. It's not fully clear to
> me. But it looks like if a gpio-controller has a gpio-hog, then it
> doesn't have/need gpio-cells? Is that right?

A GPIO hog is a way to fix (strap) a GPIO line to a specific value.
Usually this is done to enable a piece of hardware on a board, or
control a mux.

The controller still needs gpio-cells.

> So if a gpio-controller has a gpio-hog, can it ever be referred to by
> another consumer in DT using blah-gpios = ...? If so, I don't see any
> obvious code that's handling the missing gpio-cells in this case.

Yes it can.

> Long story short, please help me understand gpio-hog in the context of
> finding dependencies in DT.

The hog references a GPIO on the current controller.  As this is always
the parent device node, the hog's gpios properties lack the phandle.

E.g. a normal reference to the first GPIO of gpio5 looks like:

    gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;

A hog on the first GPIO of gpio5 would be a subnode of gpio5,
and would just use:

    gpios = <0 GPIO_ACTIVE_LOW>;

instead.

Hope this helps.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Saravana Kannan]

On Tue, Jan 19, 2021 at 10:10 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Tue, Jan 19, 2021 at 6:54 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Tue, Jan 19, 2021 at 2:20 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Tue, Jan 19, 2021 at 9:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > Can we pull this into driver-core-next please? It fixes issues on some
> > > > > boards with fw_devlink=on.
> > > >
> > > > On r8a77951-salvator-xs.dts, it introduces one more failure:
> > > >
> > > >     OF: /soc/i2c@e66d8000/gpio@20/pcie-sata-switch-hog: could not get
> > > > #gpio-cells for /cpus/cpu@102
> >
> > Geert,
> >
> > One good thing is that it's noticing this being weird and ignoring it
> > in your particular board. I *think* it interprets the "7" as a phandle
> > and that's cpu@102 and realizes it's not a gpio-controller. For at
> > least in your case, it's a safe failure.
>
> While 7 is the GPIO index, relative to the current GPIO controller,
> represented by the parent device node.
>
> > > > Seems like it doesn't parse gpios properties in GPIO hogs correctly.
> > >
> > > Could it be that the code assumes no self-referencing phandles?
> > > (Just guessing...)
> >
> > Ok I tried to understand what gpio-hogs means. It's not fully clear to
> > me. But it looks like if a gpio-controller has a gpio-hog, then it
> > doesn't have/need gpio-cells? Is that right?
>
> A GPIO hog is a way to fix (strap) a GPIO line to a specific value.
> Usually this is done to enable a piece of hardware on a board, or
> control a mux.
>
> The controller still needs gpio-cells.
>
> > So if a gpio-controller has a gpio-hog, can it ever be referred to by
> > another consumer in DT using blah-gpios = ...? If so, I don't see any
> > obvious code that's handling the missing gpio-cells in this case.
>
> Yes it can.
>
> > Long story short, please help me understand gpio-hog in the context of
> > finding dependencies in DT.
>
> The hog references a GPIO on the current controller.  As this is always
> the parent device node, the hog's gpios properties lack the phandle.
>
> E.g. a normal reference to the first GPIO of gpio5 looks like:
>
>     gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
>
> A hog on the first GPIO of gpio5 would be a subnode of gpio5,
> and would just use:
>
>     gpios = <0 GPIO_ACTIVE_LOW>;
>
> instead.
>
> Hope this helps.

I'm still not sure if I've understood this fully, but does this just
boil down to:
Don't parse [name-]gpio[s] to find dependencies if the node has
gpio-hog property?

-Saravana

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Geert Uytterhoeven]

Hi Saravana,

On Tue, Jan 19, 2021 at 7:19 PM Saravana Kannan <saravanak@google.com> wrote:
> On Tue, Jan 19, 2021 at 10:10 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Tue, Jan 19, 2021 at 6:54 PM Saravana Kannan <saravanak@google.com> wrote:
> > > On Tue, Jan 19, 2021 at 2:20 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > On Tue, Jan 19, 2021 at 9:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > Can we pull this into driver-core-next please? It fixes issues on some
> > > > > > boards with fw_devlink=on.
> > > > >
> > > > > On r8a77951-salvator-xs.dts, it introduces one more failure:
> > > > >
> > > > >     OF: /soc/i2c@e66d8000/gpio@20/pcie-sata-switch-hog: could not get
> > > > > #gpio-cells for /cpus/cpu@102
> > >
> > > Geert,
> > >
> > > One good thing is that it's noticing this being weird and ignoring it
> > > in your particular board. I *think* it interprets the "7" as a phandle
> > > and that's cpu@102 and realizes it's not a gpio-controller. For at
> > > least in your case, it's a safe failure.
> >
> > While 7 is the GPIO index, relative to the current GPIO controller,
> > represented by the parent device node.
> >
> > > > > Seems like it doesn't parse gpios properties in GPIO hogs correctly.
> > > >
> > > > Could it be that the code assumes no self-referencing phandles?
> > > > (Just guessing...)
> > >
> > > Ok I tried to understand what gpio-hogs means. It's not fully clear to
> > > me. But it looks like if a gpio-controller has a gpio-hog, then it
> > > doesn't have/need gpio-cells? Is that right?
> >
> > A GPIO hog is a way to fix (strap) a GPIO line to a specific value.
> > Usually this is done to enable a piece of hardware on a board, or
> > control a mux.
> >
> > The controller still needs gpio-cells.
> >
> > > So if a gpio-controller has a gpio-hog, can it ever be referred to by
> > > another consumer in DT using blah-gpios = ...? If so, I don't see any
> > > obvious code that's handling the missing gpio-cells in this case.
> >
> > Yes it can.
> >
> > > Long story short, please help me understand gpio-hog in the context of
> > > finding dependencies in DT.
> >
> > The hog references a GPIO on the current controller.  As this is always
> > the parent device node, the hog's gpios properties lack the phandle.
> >
> > E.g. a normal reference to the first GPIO of gpio5 looks like:
> >
> >     gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> >
> > A hog on the first GPIO of gpio5 would be a subnode of gpio5,
> > and would just use:
> >
> >     gpios = <0 GPIO_ACTIVE_LOW>;
> >
> > instead.
> >
> > Hope this helps.
>
> I'm still not sure if I've understood this fully, but does this just
> boil down to:
> Don't parse [name-]gpio[s] to find dependencies if the node has
> gpio-hog property?

Indeed. You can just ignore all nodes with a gpio-hog property, as they're
always handled by their parent.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1] of: property: Add fw_devlink support for "gpio" and "gpios" binding

[Saravana Kannan]

On Tue, Jan 19, 2021 at 10:33 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Saravana,
>
> On Tue, Jan 19, 2021 at 7:19 PM Saravana Kannan <saravanak@google.com> wrote:
> > On Tue, Jan 19, 2021 at 10:10 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Tue, Jan 19, 2021 at 6:54 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > On Tue, Jan 19, 2021 at 2:20 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > > On Tue, Jan 19, 2021 at 9:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > Can we pull this into driver-core-next please? It fixes issues on some
> > > > > > > boards with fw_devlink=on.
> > > > > >
> > > > > > On r8a77951-salvator-xs.dts, it introduces one more failure:
> > > > > >
> > > > > >     OF: /soc/i2c@e66d8000/gpio@20/pcie-sata-switch-hog: could not get
> > > > > > #gpio-cells for /cpus/cpu@102
> > > >
> > > > Geert,
> > > >
> > > > One good thing is that it's noticing this being weird and ignoring it
> > > > in your particular board. I *think* it interprets the "7" as a phandle
> > > > and that's cpu@102 and realizes it's not a gpio-controller. For at
> > > > least in your case, it's a safe failure.
> > >
> > > While 7 is the GPIO index, relative to the current GPIO controller,
> > > represented by the parent device node.
> > >
> > > > > > Seems like it doesn't parse gpios properties in GPIO hogs correctly.
> > > > >
> > > > > Could it be that the code assumes no self-referencing phandles?
> > > > > (Just guessing...)
> > > >
> > > > Ok I tried to understand what gpio-hogs means. It's not fully clear to
> > > > me. But it looks like if a gpio-controller has a gpio-hog, then it
> > > > doesn't have/need gpio-cells? Is that right?
> > >
> > > A GPIO hog is a way to fix (strap) a GPIO line to a specific value.
> > > Usually this is done to enable a piece of hardware on a board, or
> > > control a mux.
> > >
> > > The controller still needs gpio-cells.
> > >
> > > > So if a gpio-controller has a gpio-hog, can it ever be referred to by
> > > > another consumer in DT using blah-gpios = ...? If so, I don't see any
> > > > obvious code that's handling the missing gpio-cells in this case.
> > >
> > > Yes it can.
> > >
> > > > Long story short, please help me understand gpio-hog in the context of
> > > > finding dependencies in DT.
> > >
> > > The hog references a GPIO on the current controller.  As this is always
> > > the parent device node, the hog's gpios properties lack the phandle.
> > >
> > > E.g. a normal reference to the first GPIO of gpio5 looks like:
> > >
> > >     gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> > >
> > > A hog on the first GPIO of gpio5 would be a subnode of gpio5,
> > > and would just use:
> > >
> > >     gpios = <0 GPIO_ACTIVE_LOW>;
> > >
> > > instead.
> > >
> > > Hope this helps.
> >
> > I'm still not sure if I've understood this fully, but does this just
> > boil down to:
> > Don't parse [name-]gpio[s] to find dependencies if the node has
> > gpio-hog property?
>
> Indeed. You can just ignore all nodes with a gpio-hog property, as they're
> always handled by their parent.

Ok, I'll send out an updated patch later (next week probably). Or
maybe we can merge this now and I can fix up gpio-hog handling later
since I'd need to do it anyway for the name-gpios stuff anyway? Or
will those never be combined with gpio-hog?

-Saravana