LinuxLists.cc - [PATCH] of: property: do not create device links from *nr-gpios

[permalink] [raw]

Subject: Re: [PATCH] of: property: do not create device links from *nr-gpios

On Sun, Apr 4, 2021 at 8:14 PM Ilya Lipnitskiy
<[email protected]> wrote:
>
> [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> not configured by #gpio-cells and can't be parsed along with other
> "*-gpios" properties.
>
> scripts/dtc/checks.c also has a special case for nr-gpio{s}. However,
> nr-gpio is not really special, so we only need to fix nr-gpios suffix
> here.

The only example of this that I see is "snps,nr-gpios". I personally
would like to deprecate such overlapping/ambiguous definitions.

Maybe fix up the DT? This warning is a nice reminder that the DT needs
to be updated (if it can be). Outside of that, it's not causing any
issues that I know of.

If they are, then we can pick up a patch similar to this. I'd also
limit this fix to "snps,nr-gpios" so that future attempts to use
-gpios for anything other than listing GPIOs triggers a warning.

Rob, thoughts?

Thanks,
Saravana

>
> [0]: nr-gpios is referenced in Documentation/devicetree/bindings/gpio:
> - gpio-adnp.txt
> - gpio-xgene-sb.txt
> - gpio-xlp.txt
> - snps,dw-apb-gpio.yaml
>
> Fixes errors such as:
> OF: /palmbus@300000/gpio@600: could not find phandle
>
> Call Trace:
> of_phandle_iterator_next+0x8c/0x16c
> __of_parse_phandle_with_args+0x38/0xb8
> of_parse_phandle_with_args+0x28/0x3c
> parse_suffix_prop_cells+0x80/0xac
> parse_gpios+0x20/0x2c
> of_link_to_suppliers+0x18c/0x288
> of_link_to_suppliers+0x1fc/0x288
> device_add+0x4e0/0x734
> of_platform_device_create_pdata+0xb8/0xfc
> of_platform_bus_create+0x170/0x214
> of_platform_populate+0x88/0xf4
> __dt_register_buses+0xbc/0xf0
> plat_of_setup+0x1c/0x34
>
> Fixes: 7f00be96f125 ("of: property: Add device link support for interrupt-parent, dmas and -gpio(s)")
> Signed-off-by: Ilya Lipnitskiy <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: <[email protected]> # 5.5.x
> ---
> drivers/of/property.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 2bb3158c9e43..24672c295603 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1271,7 +1271,16 @@ DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> DEFINE_SIMPLE_PROP(remote_endpoint, "remote-endpoint", NULL)
> DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> +
> +static struct device_node *parse_gpios(struct device_node *np,
> + const char *prop_name, int index)
> +{
> + if (!strcmp_suffix(prop_name, "nr-gpios"))
> + return NULL;
> +
> + return parse_suffix_prop_cells(np, prop_name, index, "-gpios",
> + "#gpio-cells");
> +}
>
> static struct device_node *parse_iommu_maps(struct device_node *np,
> const char *prop_name, int index)
> --
> 2.31.1
>

2021-04-06 09:08:51

[permalink] [raw]

Subject: Re: [PATCH] of: property: do not create device links from *nr-gpios

Hi Saravana,

On Mon, Apr 5, 2021 at 1:01 PM Saravana Kannan <[email protected]> wrote:
>
> On Sun, Apr 4, 2021 at 8:14 PM Ilya Lipnitskiy
> <[email protected]> wrote:
> >
> > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > not configured by #gpio-cells and can't be parsed along with other
> > "*-gpios" properties.
> >
> > scripts/dtc/checks.c also has a special case for nr-gpio{s}. However,
> > nr-gpio is not really special, so we only need to fix nr-gpios suffix
> > here.
>
> The only example of this that I see is "snps,nr-gpios".
arch/arm64/boot/dts/apm/apm-shadowcat.dtsi uses "apm,nr-gpios", with
parsing code in drivers/gpio/gpio-xgene-sb.c. There is also code in
drivers/gpio/gpio-adnp.c and drivers/gpio/gpio-mockup.c using
"nr-gpios" without any vendor prefix.

I personally don't think causing regressions is good for any reason,
so I think we need to fix this in stable releases. The patch can be
reverted when nr-gpios is no longer special. The logic here should
also be aligned with scripts/dtc/checks.c, I actually submitted a
patch to warn about "nr-gpios" only and not "nr-gpio" in dtc as well:
https://www.spinics.net/lists/devicetree-compiler/msg03619.html

Ilya

2021-04-06 09:37:15

[permalink] [raw]

Subject: Re: [PATCH] of: property: do not create device links from *nr-gpios

On Mon, Apr 5, 2021 at 1:10 PM Ilya Lipnitskiy
<[email protected]> wrote:
>
> Hi Saravana,
>
> On Mon, Apr 5, 2021 at 1:01 PM Saravana Kannan <[email protected]> wrote:
> >
> > On Sun, Apr 4, 2021 at 8:14 PM Ilya Lipnitskiy
> > <[email protected]> wrote:
> > >
> > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > not configured by #gpio-cells and can't be parsed along with other
> > > "*-gpios" properties.
> > >
> > > scripts/dtc/checks.c also has a special case for nr-gpio{s}. However,
> > > nr-gpio is not really special, so we only need to fix nr-gpios suffix
> > > here.
> >
> > The only example of this that I see is "snps,nr-gpios".
> arch/arm64/boot/dts/apm/apm-shadowcat.dtsi uses "apm,nr-gpios", with
> parsing code in drivers/gpio/gpio-xgene-sb.c. There is also code in
> drivers/gpio/gpio-adnp.c and drivers/gpio/gpio-mockup.c using
> "nr-gpios" without any vendor prefix.

Ah ok. I just grepped the DT files. I'm not sure what Rob's position
is on supporting DT files not in upstream. Thanks for the
clarification.

> I personally don't think causing regressions is good for any reason,

I agree, but this is not a functional regression. Just a warning
that's spit out. I don't have a strong opinion on the stack dump vs
not, but I think we should at least reject future additions like this
and limit the exceptions to exactly what's allowed today. nr-gpios
(without any vendor prefix) is especially annoying to me.

Looks like even the DT spec has an exception only for vendor,nr and not just nr.
https://github.com/devicetree-org/dt-schema/blob/master/schemas/gpio/gpio-consumer.yaml#L20

-Saravana

> so I think we need to fix this in stable releases. The patch can be
> reverted when nr-gpios is no longer special. The logic here should
> also be aligned with scripts/dtc/checks.c, I actually submitted a
> patch to warn about "nr-gpios" only and not "nr-gpio" in dtc as well:
> https://www.spinics.net/lists/devicetree-compiler/msg03619.html
>
> Ilya

2021-04-06 09:59:30

[permalink] [raw]

Subject: Re: [PATCH] of: property: do not create device links from *nr-gpios

On Mon, Apr 5, 2021 at 1:19 PM Saravana Kannan <[email protected]> wrote:
>
> On Mon, Apr 5, 2021 at 1:10 PM Ilya Lipnitskiy
> <[email protected]> wrote:
> >
> > Hi Saravana,
> >
> > On Mon, Apr 5, 2021 at 1:01 PM Saravana Kannan <[email protected]> wrote:
> > >
> > > On Sun, Apr 4, 2021 at 8:14 PM Ilya Lipnitskiy
> > > <[email protected]> wrote:
> > > >
> > > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > > not configured by #gpio-cells and can't be parsed along with other
> > > > "*-gpios" properties.
> > > >
> > > > scripts/dtc/checks.c also has a special case for nr-gpio{s}. However,
> > > > nr-gpio is not really special, so we only need to fix nr-gpios suffix
> > > > here.
> > >
> > > The only example of this that I see is "snps,nr-gpios".
> > arch/arm64/boot/dts/apm/apm-shadowcat.dtsi uses "apm,nr-gpios", with
> > parsing code in drivers/gpio/gpio-xgene-sb.c. There is also code in
> > drivers/gpio/gpio-adnp.c and drivers/gpio/gpio-mockup.c using
> > "nr-gpios" without any vendor prefix.
>
> Ah ok. I just grepped the DT files. I'm not sure what Rob's position
> is on supporting DT files not in upstream. Thanks for the
> clarification.
For the offending drivers and docs that don't have any dts/dtsi files
in-tree, can we just "sed -i 's/nr-gpios/ngpios'" and call it good?

> Looks like even the DT spec has an exception only for vendor,nr and not just nr.
> https://github.com/devicetree-org/dt-schema/blob/master/schemas/gpio/gpio-consumer.yaml#L20
Thanks for linking the spec. I can re-spin the patch with ",nr-gpios"
as the special suffix to align with the spec.

Ilya

2021-04-06 10:20:46

[permalink] [raw]

Subject: [PATCH v2] of: property: fw_devlink: do not link ".*,nr-gpios"

[<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
the number of GPIOs present on a system, not define a GPIO. nr-gpios is
not configured by #gpio-cells and can't be parsed along with other
"*-gpios" properties.

nr-gpios without the "<vendor>," prefix is not allowed by the DT
spec[1], so only add exception for the ",nr-gpios" suffix and let the
error message continue being printed for non-compliant implementations.

[0]: nr-gpios is referenced in Documentation/devicetree/bindings/gpio:
- gpio-adnp.txt
- gpio-xgene-sb.txt
- gpio-xlp.txt
- snps,dw-apb-gpio.yaml

[1]:
Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20

Fixes errors such as:
OF: /palmbus@300000/gpio@600: could not find phandle

Fixes: 7f00be96f125 ("of: property: Add device link support for interrupt-parent, dmas and -gpio(s)")
Signed-off-by: Ilya Lipnitskiy <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: <[email protected]> # 5.5.x
---
drivers/of/property.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 2046ae311322..1793303e84ac 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1281,7 +1281,16 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
-DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
+
+static struct device_node *parse_gpios(struct device_node *np,
+ const char *prop_name, int index)
+{
+ if (!strcmp_suffix(prop_name, ",nr-gpios"))
+ return NULL;
+
+ return parse_suffix_prop_cells(np, prop_name, index, "-gpios",
+ "#gpio-cells");
+}

static struct device_node *parse_iommu_maps(struct device_node *np,
const char *prop_name, int index)
--
2.31.1

2021-04-07 10:30:45

[permalink] [raw]

Subject: Re: [PATCH] of: property: do not create device links from *nr-gpios

On Mon, Apr 05, 2021 at 01:18:56PM -0700, Saravana Kannan wrote:
> On Mon, Apr 5, 2021 at 1:10 PM Ilya Lipnitskiy
> <[email protected]> wrote:
> >
> > Hi Saravana,
> >
> > On Mon, Apr 5, 2021 at 1:01 PM Saravana Kannan <[email protected]> wrote:
> > >
> > > On Sun, Apr 4, 2021 at 8:14 PM Ilya Lipnitskiy
> > > <[email protected]> wrote:
> > > >
> > > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > > not configured by #gpio-cells and can't be parsed along with other
> > > > "*-gpios" properties.
> > > >
> > > > scripts/dtc/checks.c also has a special case for nr-gpio{s}. However,
> > > > nr-gpio is not really special, so we only need to fix nr-gpios suffix
> > > > here.
> > >
> > > The only example of this that I see is "snps,nr-gpios".
> > arch/arm64/boot/dts/apm/apm-shadowcat.dtsi uses "apm,nr-gpios", with
> > parsing code in drivers/gpio/gpio-xgene-sb.c. There is also code in
> > drivers/gpio/gpio-adnp.c and drivers/gpio/gpio-mockup.c using
> > "nr-gpios" without any vendor prefix.
>
> Ah ok. I just grepped the DT files. I'm not sure what Rob's position
> is on supporting DT files not in upstream. Thanks for the
> clarification.

If it's something we had documented, then we have to support it (also
conditioned on someone noticing). I'm hoping we can just delete APM and
other defunct ARM server DTs soon, but we could update them to use
'ngpios' instead.

gpio-mockup doesn't have a binding, so no DT ABI. Hard to tell if
gpio-adnp.c has any users.

Rob

2021-04-07 12:11:14

[permalink] [raw]

Subject: Re: [PATCH] of: property: do not create device links from *nr-gpios

On Tue, Apr 6, 2021 at 10:40 AM Rob Herring <[email protected]> wrote:
>
> On Mon, Apr 05, 2021 at 01:18:56PM -0700, Saravana Kannan wrote:
> > On Mon, Apr 5, 2021 at 1:10 PM Ilya Lipnitskiy
> > <[email protected]> wrote:
> > >
> > > Hi Saravana,
> > >
> > > On Mon, Apr 5, 2021 at 1:01 PM Saravana Kannan <[email protected]> wrote:
> > > >
> > > > On Sun, Apr 4, 2021 at 8:14 PM Ilya Lipnitskiy
> > > > <[email protected]> wrote:
> > > > >
> > > > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > > > not configured by #gpio-cells and can't be parsed along with other
> > > > > "*-gpios" properties.
> > > > >
> > > > > scripts/dtc/checks.c also has a special case for nr-gpio{s}. However,
> > > > > nr-gpio is not really special, so we only need to fix nr-gpios suffix
> > > > > here.
> > > >
> > > > The only example of this that I see is "snps,nr-gpios".
> > > arch/arm64/boot/dts/apm/apm-shadowcat.dtsi uses "apm,nr-gpios", with
> > > parsing code in drivers/gpio/gpio-xgene-sb.c. There is also code in
> > > drivers/gpio/gpio-adnp.c and drivers/gpio/gpio-mockup.c using
> > > "nr-gpios" without any vendor prefix.
> >
> > Ah ok. I just grepped the DT files. I'm not sure what Rob's position
> > is on supporting DT files not in upstream. Thanks for the
> > clarification.
>
> If it's something we had documented, then we have to support it
Do I read this correctly as a sort-of Ack of my proposed [PATCH v2] in
this thread, since it aligns the code with the published DT schema?

Ilya

2021-04-07 13:04:09

[permalink] [raw]

Subject: Re: [PATCH] of: property: do not create device links from *nr-gpios

On Tue, Apr 6, 2021 at 4:28 PM Saravana Kannan <[email protected]> wrote:
>
> On Tue, Apr 6, 2021 at 12:28 PM Ilya Lipnitskiy
> <[email protected]> wrote:
> >
> > On Tue, Apr 6, 2021 at 10:40 AM Rob Herring <[email protected]> wrote:
> > >
> > > On Mon, Apr 05, 2021 at 01:18:56PM -0700, Saravana Kannan wrote:
> > > > On Mon, Apr 5, 2021 at 1:10 PM Ilya Lipnitskiy
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Saravana,
> > > > >
> > > > > On Mon, Apr 5, 2021 at 1:01 PM Saravana Kannan <[email protected]> wrote:
> > > > > >
> > > > > > On Sun, Apr 4, 2021 at 8:14 PM Ilya Lipnitskiy
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > > > > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > > > > > not configured by #gpio-cells and can't be parsed along with other
> > > > > > > "*-gpios" properties.
> > > > > > >
> > > > > > > scripts/dtc/checks.c also has a special case for nr-gpio{s}. However,
> > > > > > > nr-gpio is not really special, so we only need to fix nr-gpios suffix
> > > > > > > here.
> > > > > >
> > > > > > The only example of this that I see is "snps,nr-gpios".
> > > > > arch/arm64/boot/dts/apm/apm-shadowcat.dtsi uses "apm,nr-gpios", with
> > > > > parsing code in drivers/gpio/gpio-xgene-sb.c. There is also code in
> > > > > drivers/gpio/gpio-adnp.c and drivers/gpio/gpio-mockup.c using
> > > > > "nr-gpios" without any vendor prefix.
> > > >
> > > > Ah ok. I just grepped the DT files. I'm not sure what Rob's position
> > > > is on supporting DT files not in upstream. Thanks for the
> > > > clarification.
> > >
> > > If it's something we had documented, then we have to support it
> > Do I read this correctly as a sort-of Ack of my proposed [PATCH v2] in
> > this thread, since it aligns the code with the published DT schema?
>
> He's talking about the DT binding documentation in the kernel.
>
> I interpret Rob's reply as, you can do all of this:
> 1. Just fix up all drivers that use "*nr-gpios" that don't have
> binding documentation in the kernel. Change them to use ngpios.
> 2. Try to switch away old defunct ARM server DTs from nr-gpios to
> ngpios (both drivers and DT) and see if people notice.
> 3. Change the fw_devlink parsing code to have exceptions only for
> cases that are using nr-gpios after (1) and (2).

Yes, but (3) is not gated on (1) and (2). I'm applying v2.

Rob

2021-04-07 13:45:15

[permalink] [raw]

Subject: Re: [PATCH v2] of: property: fw_devlink: do not link ".*,nr-gpios"

On Tue, Apr 6, 2021 at 5:34 PM Rob Herring <[email protected]> wrote:
>
> On Tue, Apr 06, 2021 at 04:09:10PM -0700, Saravana Kannan wrote:
> > On Mon, Apr 5, 2021 at 3:26 PM Ilya Lipnitskiy
> > <[email protected]> wrote:
> > >
> > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > not configured by #gpio-cells and can't be parsed along with other
> > > "*-gpios" properties.
> > >
> > > nr-gpios without the "<vendor>," prefix is not allowed by the DT
> > > spec[1], so only add exception for the ",nr-gpios" suffix and let the
> > > error message continue being printed for non-compliant implementations.
> > >
> > > [0]: nr-gpios is referenced in Documentation/devicetree/bindings/gpio:
> > > - gpio-adnp.txt
> > > - gpio-xgene-sb.txt
> > > - gpio-xlp.txt
> > > - snps,dw-apb-gpio.yaml
> > >
> > > [1]:
> > > Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
> > >
> > > Fixes errors such as:
> > > OF: /palmbus@300000/gpio@600: could not find phandle
> > >
> > > Fixes: 7f00be96f125 ("of: property: Add device link support for interrupt-parent, dmas and -gpio(s)")
> > > Signed-off-by: Ilya Lipnitskiy <[email protected]>
> > > Cc: Saravana Kannan <[email protected]>
> > > Cc: <[email protected]> # 5.5.x
> > > ---
> > > drivers/of/property.c | 11 ++++++++++-
> > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 2046ae311322..1793303e84ac 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1281,7 +1281,16 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> > > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> > > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> > > +
> > > +static struct device_node *parse_gpios(struct device_node *np,
> > > + const char *prop_name, int index)
> > > +{
> > > + if (!strcmp_suffix(prop_name, ",nr-gpios"))
> > > + return NULL;
> >
> > Ah I somehow missed this patch. This gives a blanked exception for
> > vendor,nr-gpios. I'd prefer explicit exceptions for all the instances
> > of ",nr-gpios" we are grandfathering in. Any future additions should
> > be rejected. Can we do that please?
> >
> > Rob, you okay with making this list more explicit?
>
> Not the kernel's job IMO. A schema is the right way to handle that.

Ok, that's fine by me. Btw, let's land this in driver-core? I've made
changes there and this might cause conflicts. Not sure.

-Saravana

2021-04-07 13:46:36

[permalink] [raw]

Subject: Re: [PATCH v2] of: property: fw_devlink: do not link ".*,nr-gpios"

On Tue, Apr 6, 2021 at 7:46 PM Saravana Kannan <[email protected]> wrote:
>
> On Tue, Apr 6, 2021 at 5:34 PM Rob Herring <[email protected]> wrote:
> >
> > On Tue, Apr 06, 2021 at 04:09:10PM -0700, Saravana Kannan wrote:
> > > On Mon, Apr 5, 2021 at 3:26 PM Ilya Lipnitskiy
> > > <[email protected]> wrote:
> > > >
> > > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > > not configured by #gpio-cells and can't be parsed along with other
> > > > "*-gpios" properties.
> > > >
> > > > nr-gpios without the "<vendor>," prefix is not allowed by the DT
> > > > spec[1], so only add exception for the ",nr-gpios" suffix and let the
> > > > error message continue being printed for non-compliant implementations.
> > > >
> > > > [0]: nr-gpios is referenced in Documentation/devicetree/bindings/gpio:
> > > > - gpio-adnp.txt
> > > > - gpio-xgene-sb.txt
> > > > - gpio-xlp.txt
> > > > - snps,dw-apb-gpio.yaml
> > > >
> > > > [1]:
> > > > Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
> > > >
> > > > Fixes errors such as:
> > > > OF: /palmbus@300000/gpio@600: could not find phandle
> > > >
> > > > Fixes: 7f00be96f125 ("of: property: Add device link support for interrupt-parent, dmas and -gpio(s)")
> > > > Signed-off-by: Ilya Lipnitskiy <[email protected]>
> > > > Cc: Saravana Kannan <[email protected]>
> > > > Cc: <[email protected]> # 5.5.x
> > > > ---
> > > > drivers/of/property.c | 11 ++++++++++-
> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > index 2046ae311322..1793303e84ac 100644
> > > > --- a/drivers/of/property.c
> > > > +++ b/drivers/of/property.c
> > > > @@ -1281,7 +1281,16 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> > > > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> > > > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > > > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > > > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> > > > +
> > > > +static struct device_node *parse_gpios(struct device_node *np,
> > > > + const char *prop_name, int index)
> > > > +{
> > > > + if (!strcmp_suffix(prop_name, ",nr-gpios"))
> > > > + return NULL;
> > >
> > > Ah I somehow missed this patch. This gives a blanked exception for
> > > vendor,nr-gpios. I'd prefer explicit exceptions for all the instances
> > > of ",nr-gpios" we are grandfathering in. Any future additions should
> > > be rejected. Can we do that please?
> > >
> > > Rob, you okay with making this list more explicit?
> >
> > Not the kernel's job IMO. A schema is the right way to handle that.
>
> Ok, that's fine by me. Btw, let's land this in driver-core? I've made
> changes there and this might cause conflicts. Not sure.

It merges with linux-next fine. You'll need to resend this to Greg if
you want to do that.

Reviewed-by: Rob Herring <[email protected]>

2021-04-07 15:28:17

[permalink] [raw]

Subject: Re: [PATCH] of: property: do not create device links from *nr-gpios

On Tue, Apr 6, 2021 at 12:28 PM Ilya Lipnitskiy
<[email protected]> wrote:
>
> On Tue, Apr 6, 2021 at 10:40 AM Rob Herring <[email protected]> wrote:
> >
> > On Mon, Apr 05, 2021 at 01:18:56PM -0700, Saravana Kannan wrote:
> > > On Mon, Apr 5, 2021 at 1:10 PM Ilya Lipnitskiy
> > > <[email protected]> wrote:
> > > >
> > > > Hi Saravana,
> > > >
> > > > On Mon, Apr 5, 2021 at 1:01 PM Saravana Kannan <[email protected]> wrote:
> > > > >
> > > > > On Sun, Apr 4, 2021 at 8:14 PM Ilya Lipnitskiy
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > > > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > > > > not configured by #gpio-cells and can't be parsed along with other
> > > > > > "*-gpios" properties.
> > > > > >
> > > > > > scripts/dtc/checks.c also has a special case for nr-gpio{s}. However,
> > > > > > nr-gpio is not really special, so we only need to fix nr-gpios suffix
> > > > > > here.
> > > > >
> > > > > The only example of this that I see is "snps,nr-gpios".
> > > > arch/arm64/boot/dts/apm/apm-shadowcat.dtsi uses "apm,nr-gpios", with
> > > > parsing code in drivers/gpio/gpio-xgene-sb.c. There is also code in
> > > > drivers/gpio/gpio-adnp.c and drivers/gpio/gpio-mockup.c using
> > > > "nr-gpios" without any vendor prefix.
> > >
> > > Ah ok. I just grepped the DT files. I'm not sure what Rob's position
> > > is on supporting DT files not in upstream. Thanks for the
> > > clarification.
> >
> > If it's something we had documented, then we have to support it
> Do I read this correctly as a sort-of Ack of my proposed [PATCH v2] in
> this thread, since it aligns the code with the published DT schema?

He's talking about the DT binding documentation in the kernel.

I interpret Rob's reply as, you can do all of this:
1. Just fix up all drivers that use "*nr-gpios" that don't have
binding documentation in the kernel. Change them to use ngpios.
2. Try to switch away old defunct ARM server DTs from nr-gpios to
ngpios (both drivers and DT) and see if people notice.
3. Change the fw_devlink parsing code to have exceptions only for
cases that are using nr-gpios after (1) and (2).

-Saravana

2021-04-07 16:07:33

[permalink] [raw]

Subject: Re: [PATCH v2] of: property: fw_devlink: do not link ".*,nr-gpios"

On Mon, Apr 5, 2021 at 3:26 PM Ilya Lipnitskiy
<[email protected]> wrote:
>
> [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> not configured by #gpio-cells and can't be parsed along with other
> "*-gpios" properties.
>
> nr-gpios without the "<vendor>," prefix is not allowed by the DT
> spec[1], so only add exception for the ",nr-gpios" suffix and let the
> error message continue being printed for non-compliant implementations.
>
> [0]: nr-gpios is referenced in Documentation/devicetree/bindings/gpio:
> - gpio-adnp.txt
> - gpio-xgene-sb.txt
> - gpio-xlp.txt
> - snps,dw-apb-gpio.yaml
>
> [1]:
> Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
>
> Fixes errors such as:
> OF: /palmbus@300000/gpio@600: could not find phandle
>
> Fixes: 7f00be96f125 ("of: property: Add device link support for interrupt-parent, dmas and -gpio(s)")
> Signed-off-by: Ilya Lipnitskiy <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: <[email protected]> # 5.5.x
> ---
> drivers/of/property.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 2046ae311322..1793303e84ac 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1281,7 +1281,16 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> +
> +static struct device_node *parse_gpios(struct device_node *np,
> + const char *prop_name, int index)
> +{
> + if (!strcmp_suffix(prop_name, ",nr-gpios"))
> + return NULL;

Ah I somehow missed this patch. This gives a blanked exception for
vendor,nr-gpios. I'd prefer explicit exceptions for all the instances
of ",nr-gpios" we are grandfathering in. Any future additions should
be rejected. Can we do that please?

Rob, you okay with making this list more explicit?

-Saravana

2021-04-07 16:10:16

[permalink] [raw]

Subject: Re: [PATCH v2] of: property: fw_devlink: do not link ".*,nr-gpios"

On Tue, Apr 06, 2021 at 04:09:10PM -0700, Saravana Kannan wrote:
> On Mon, Apr 5, 2021 at 3:26 PM Ilya Lipnitskiy
> <[email protected]> wrote:
> >
> > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > not configured by #gpio-cells and can't be parsed along with other
> > "*-gpios" properties.
> >
> > nr-gpios without the "<vendor>," prefix is not allowed by the DT
> > spec[1], so only add exception for the ",nr-gpios" suffix and let the
> > error message continue being printed for non-compliant implementations.
> >
> > [0]: nr-gpios is referenced in Documentation/devicetree/bindings/gpio:
> > - gpio-adnp.txt
> > - gpio-xgene-sb.txt
> > - gpio-xlp.txt
> > - snps,dw-apb-gpio.yaml
> >
> > [1]:
> > Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
> >
> > Fixes errors such as:
> > OF: /palmbus@300000/gpio@600: could not find phandle
> >
> > Fixes: 7f00be96f125 ("of: property: Add device link support for interrupt-parent, dmas and -gpio(s)")
> > Signed-off-by: Ilya Lipnitskiy <[email protected]>
> > Cc: Saravana Kannan <[email protected]>
> > Cc: <[email protected]> # 5.5.x
> > ---
> > drivers/of/property.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 2046ae311322..1793303e84ac 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1281,7 +1281,16 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> > +
> > +static struct device_node *parse_gpios(struct device_node *np,
> > + const char *prop_name, int index)
> > +{
> > + if (!strcmp_suffix(prop_name, ",nr-gpios"))
> > + return NULL;
>
> Ah I somehow missed this patch. This gives a blanked exception for
> vendor,nr-gpios. I'd prefer explicit exceptions for all the instances
> of ",nr-gpios" we are grandfathering in. Any future additions should
> be rejected. Can we do that please?
>
> Rob, you okay with making this list more explicit?

Not the kernel's job IMO. A schema is the right way to handle that.

Rob

2021-04-07 16:10:54

[permalink] [raw]

Subject: Re: [PATCH v2] of: property: fw_devlink: do not link ".*,nr-gpios"

On Tue, Apr 6, 2021 at 6:10 PM Rob Herring <[email protected]> wrote:
>
> On Tue, Apr 6, 2021 at 7:46 PM Saravana Kannan <[email protected]> wrote:
> >
> > On Tue, Apr 6, 2021 at 5:34 PM Rob Herring <[email protected]> wrote:
> > >
> > > On Tue, Apr 06, 2021 at 04:09:10PM -0700, Saravana Kannan wrote:
> > > > On Mon, Apr 5, 2021 at 3:26 PM Ilya Lipnitskiy
> > > > <[email protected]> wrote:
> > > > >
> > > > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > > > not configured by #gpio-cells and can't be parsed along with other
> > > > > "*-gpios" properties.
> > > > >
> > > > > nr-gpios without the "<vendor>," prefix is not allowed by the DT
> > > > > spec[1], so only add exception for the ",nr-gpios" suffix and let the
> > > > > error message continue being printed for non-compliant implementations.
> > > > >
> > > > > [0]: nr-gpios is referenced in Documentation/devicetree/bindings/gpio:
> > > > > - gpio-adnp.txt
> > > > > - gpio-xgene-sb.txt
> > > > > - gpio-xlp.txt
> > > > > - snps,dw-apb-gpio.yaml
> > > > >
> > > > > [1]:
> > > > > Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
> > > > >
> > > > > Fixes errors such as:
> > > > > OF: /palmbus@300000/gpio@600: could not find phandle
> > > > >
> > > > > Fixes: 7f00be96f125 ("of: property: Add device link support for interrupt-parent, dmas and -gpio(s)")
> > > > > Signed-off-by: Ilya Lipnitskiy <[email protected]>
> > > > > Cc: Saravana Kannan <[email protected]>
> > > > > Cc: <[email protected]> # 5.5.x
> > > > > ---
> > > > > drivers/of/property.c | 11 ++++++++++-
> > > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > index 2046ae311322..1793303e84ac 100644
> > > > > --- a/drivers/of/property.c
> > > > > +++ b/drivers/of/property.c
> > > > > @@ -1281,7 +1281,16 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> > > > > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> > > > > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > > > > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > > > > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> > > > > +
> > > > > +static struct device_node *parse_gpios(struct device_node *np,
> > > > > + const char *prop_name, int index)
> > > > > +{
> > > > > + if (!strcmp_suffix(prop_name, ",nr-gpios"))
> > > > > + return NULL;
> > > >
> > > > Ah I somehow missed this patch. This gives a blanked exception for
> > > > vendor,nr-gpios. I'd prefer explicit exceptions for all the instances
> > > > of ",nr-gpios" we are grandfathering in. Any future additions should
> > > > be rejected. Can we do that please?
> > > >
> > > > Rob, you okay with making this list more explicit?
> > >
> > > Not the kernel's job IMO. A schema is the right way to handle that.
> >
> > Ok, that's fine by me. Btw, let's land this in driver-core? I've made
> > changes there and this might cause conflicts. Not sure.
>
> It merges with linux-next fine. You'll need to resend this to Greg if
> you want to do that.
>
> Reviewed-by: Rob Herring <[email protected]>

Hi Greg,

Can you pull this into driver-core please? I touch this file a lot and
might need to do so again if any fw_devlink=on issues come up. So
trying to preemptively avoid conflicts.

-Saravana

2021-04-07 22:04:31

[permalink] [raw]

Subject: Re: [PATCH v2] of: property: fw_devlink: do not link ".*,nr-gpios"

On Tue, Apr 6, 2021 at 6:24 PM Saravana Kannan <[email protected]> wrote:
>
> On Tue, Apr 6, 2021 at 6:10 PM Rob Herring <[email protected]> wrote:
> >
> > On Tue, Apr 6, 2021 at 7:46 PM Saravana Kannan <[email protected]> wrote:
> > >
> > > On Tue, Apr 6, 2021 at 5:34 PM Rob Herring <[email protected]> wrote:
> > > >
> > > > On Tue, Apr 06, 2021 at 04:09:10PM -0700, Saravana Kannan wrote:
> > > > > On Mon, Apr 5, 2021 at 3:26 PM Ilya Lipnitskiy
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > > > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > > > > not configured by #gpio-cells and can't be parsed along with other
> > > > > > "*-gpios" properties.
> > > > > >
> > > > > > nr-gpios without the "<vendor>," prefix is not allowed by the DT
> > > > > > spec[1], so only add exception for the ",nr-gpios" suffix and let the
> > > > > > error message continue being printed for non-compliant implementations.
> > > > > >
> > > > > > [0]: nr-gpios is referenced in Documentation/devicetree/bindings/gpio:
> > > > > > - gpio-adnp.txt
> > > > > > - gpio-xgene-sb.txt
> > > > > > - gpio-xlp.txt
> > > > > > - snps,dw-apb-gpio.yaml
> > > > > >
> > > > > > [1]:
> > > > > > Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
> > > > > >
> > > > > > Fixes errors such as:
> > > > > > OF: /palmbus@300000/gpio@600: could not find phandle
> > > > > >
> > > > > > Fixes: 7f00be96f125 ("of: property: Add device link support for interrupt-parent, dmas and -gpio(s)")
> > > > > > Signed-off-by: Ilya Lipnitskiy <[email protected]>
> > > > > > Cc: Saravana Kannan <[email protected]>
> > > > > > Cc: <[email protected]> # 5.5.x
> > > > > > ---
> > > > > > drivers/of/property.c | 11 ++++++++++-
> > > > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > > index 2046ae311322..1793303e84ac 100644
> > > > > > --- a/drivers/of/property.c
> > > > > > +++ b/drivers/of/property.c
> > > > > > @@ -1281,7 +1281,16 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> > > > > > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> > > > > > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > > > > > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > > > > > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> > > > > > +
> > > > > > +static struct device_node *parse_gpios(struct device_node *np,
> > > > > > + const char *prop_name, int index)
> > > > > > +{
> > > > > > + if (!strcmp_suffix(prop_name, ",nr-gpios"))
> > > > > > + return NULL;
> > > > >
> > > > > Ah I somehow missed this patch. This gives a blanked exception for
> > > > > vendor,nr-gpios. I'd prefer explicit exceptions for all the instances
> > > > > of ",nr-gpios" we are grandfathering in. Any future additions should
> > > > > be rejected. Can we do that please?
> > > > >
> > > > > Rob, you okay with making this list more explicit?
> > > >
> > > > Not the kernel's job IMO. A schema is the right way to handle that.
> > >
> > > Ok, that's fine by me. Btw, let's land this in driver-core? I've made
> > > changes there and this might cause conflicts. Not sure.
> >
> > It merges with linux-next fine. You'll need to resend this to Greg if
> > you want to do that.
> >
> > Reviewed-by: Rob Herring <[email protected]>
>
> Hi Greg,
>
> Can you pull this into driver-core please?
Do you want me to re-spin on top of driver-core? The patch is
currently based on dt/next in robh/linux.git

Ilya

2021-04-09 19:30:01

[permalink] [raw]

Subject: Re: [PATCH v2] of: property: fw_devlink: do not link ".*,nr-gpios"

On Wed, Apr 7, 2021 at 3:45 PM Ilya Lipnitskiy
<[email protected]> wrote:
>
> On Tue, Apr 6, 2021 at 6:24 PM Saravana Kannan <[email protected]> wrote:
> >
> > On Tue, Apr 6, 2021 at 6:10 PM Rob Herring <[email protected]> wrote:
> > >
> > > On Tue, Apr 6, 2021 at 7:46 PM Saravana Kannan <[email protected]> wrote:
> > > >
> > > > On Tue, Apr 6, 2021 at 5:34 PM Rob Herring <[email protected]> wrote:
> > > > >
> > > > > On Tue, Apr 06, 2021 at 04:09:10PM -0700, Saravana Kannan wrote:
> > > > > > On Mon, Apr 5, 2021 at 3:26 PM Ilya Lipnitskiy
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > [<vendor>,]nr-gpios property is used by some GPIO drivers[0] to indicate
> > > > > > > the number of GPIOs present on a system, not define a GPIO. nr-gpios is
> > > > > > > not configured by #gpio-cells and can't be parsed along with other
> > > > > > > "*-gpios" properties.
> > > > > > >
> > > > > > > nr-gpios without the "<vendor>," prefix is not allowed by the DT
> > > > > > > spec[1], so only add exception for the ",nr-gpios" suffix and let the
> > > > > > > error message continue being printed for non-compliant implementations.
> > > > > > >
> > > > > > > [0]: nr-gpios is referenced in Documentation/devicetree/bindings/gpio:
> > > > > > > - gpio-adnp.txt
> > > > > > > - gpio-xgene-sb.txt
> > > > > > > - gpio-xlp.txt
> > > > > > > - snps,dw-apb-gpio.yaml
> > > > > > >
> > > > > > > [1]:
> > > > > > > Link: https://github.com/devicetree-org/dt-schema/blob/cb53a16a1eb3e2169ce170c071e47940845ec26e/schemas/gpio/gpio-consumer.yaml#L20
> > > > > > >
> > > > > > > Fixes errors such as:
> > > > > > > OF: /palmbus@300000/gpio@600: could not find phandle
> > > > > > >
> > > > > > > Fixes: 7f00be96f125 ("of: property: Add device link support for interrupt-parent, dmas and -gpio(s)")
> > > > > > > Signed-off-by: Ilya Lipnitskiy <[email protected]>
> > > > > > > Cc: Saravana Kannan <[email protected]>
> > > > > > > Cc: <[email protected]> # 5.5.x
> > > > > > > ---
> > > > > > > drivers/of/property.c | 11 ++++++++++-
> > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > > > index 2046ae311322..1793303e84ac 100644
> > > > > > > --- a/drivers/of/property.c
> > > > > > > +++ b/drivers/of/property.c
> > > > > > > @@ -1281,7 +1281,16 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> > > > > > > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> > > > > > > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > > > > > > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > > > > > > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> > > > > > > +
> > > > > > > +static struct device_node *parse_gpios(struct device_node *np,
> > > > > > > + const char *prop_name, int index)
> > > > > > > +{
> > > > > > > + if (!strcmp_suffix(prop_name, ",nr-gpios"))
> > > > > > > + return NULL;
> > > > > >
> > > > > > Ah I somehow missed this patch. This gives a blanked exception for
> > > > > > vendor,nr-gpios. I'd prefer explicit exceptions for all the instances
> > > > > > of ",nr-gpios" we are grandfathering in. Any future additions should
> > > > > > be rejected. Can we do that please?
> > > > > >
> > > > > > Rob, you okay with making this list more explicit?
> > > > >
> > > > > Not the kernel's job IMO. A schema is the right way to handle that.
> > > >
> > > > Ok, that's fine by me. Btw, let's land this in driver-core? I've made
> > > > changes there and this might cause conflicts. Not sure.
> > >
> > > It merges with linux-next fine. You'll need to resend this to Greg if
> > > you want to do that.
> > >
> > > Reviewed-by: Rob Herring <[email protected]>
> >
> > Hi Greg,
> >
> > Can you pull this into driver-core please?
> Do you want me to re-spin on top of driver-core? The patch is
> currently based on dt/next in robh/linux.git

I did say you need to resend the patch to Greg, but since there's no
movement on this and I have other things to send upstream, I've
applied it.

Rob

2021-04-09 19:38:56