| Message ID | 20201119060921.311747-2-damien.lemoal@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix dwapb gpio snps,nr-gpios property handling | expand |
On Thu, Nov 19, 2020 at 03:09:20PM +0900, Damien Le Moal wrote: > The DesignWare gpio-dwapb GPIO driver ("snps,dw-apb-gpio" or > "apm,xgene-gpio-v2" compatible string) defines the now deprecated > property "snps,nr-gpios" to specify the number of GPIOs available > on a port. However, if this property is used in a device tree, its > "-gpios" suffix causes the generic property supplier parsing code to > interpret it as a cell reference when properties are parsed in > of_link_to_suppliers(), leading to an error messages such as: > > OF: /soc/bus@50200000/gpio-controller@50200000/gpio-port@0: could not > find phandle > > Fix this by manually defining a parse_gpios() function which ignores > this deprecated property that is still present in many device trees, > skipping the search for the supplier and thus avoiding the device tree > parsing error. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/of/property.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 408a7b5f06a9..4eefe8efc2fe 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1308,7 +1308,6 @@ 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_iommu_maps(struct device_node *np, > const char *prop_name, int index) > @@ -1319,6 +1318,21 @@ static struct device_node *parse_iommu_maps(struct device_node *np, > return of_parse_phandle(np, prop_name, (index * 4) + 1); > } > > +static struct device_node *parse_gpios(struct device_node *np, > + const char *prop_name, int index) > +{ > + /* > + * Quirk for the deprecated "snps,nr-gpios" property of the > + * DesignWare gpio-dwapb GPIO driver: as this property parsing > + * conflicts with the "xx-gpios" phandle reference property, ignore it. > + */ > + if (strcmp(prop_name, "snps,nr-gpios") == 0) > + return NULL; What about printing the warning from instead of doing that from the driver? Like this: + if (strcmp(prop_name, "snps,nr-gpios") == 0) { + pr_warn("%pOF: %s is deprecated in favor of ngpios\n"); + return NULL; + } So when the property is removed from all dts'es we wouldn't forget to discard the quirk? -Sergey > + > + return parse_suffix_prop_cells(np, prop_name, index, > + "-gpios", "#gpio-cells"); > +} > + > static const struct supplier_bindings of_supplier_bindings[] = { > { .parse_prop = parse_clocks, }, > { .parse_prop = parse_interconnects, }, > -- > 2.28.0 >
On Wed, Nov 25, 2020 at 2:06 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Thu, Nov 19, 2020 at 03:09:20PM +0900, Damien Le Moal wrote: > > The DesignWare gpio-dwapb GPIO driver ("snps,dw-apb-gpio" or > > "apm,xgene-gpio-v2" compatible string) defines the now deprecated > > property "snps,nr-gpios" to specify the number of GPIOs available > > on a port. However, if this property is used in a device tree, its > > "-gpios" suffix causes the generic property supplier parsing code to > > interpret it as a cell reference when properties are parsed in > > of_link_to_suppliers(), leading to an error messages such as: > > > > OF: /soc/bus@50200000/gpio-controller@50200000/gpio-port@0: could not > > find phandle > > > > Fix this by manually defining a parse_gpios() function which ignores > > this deprecated property that is still present in many device trees, > > skipping the search for the supplier and thus avoiding the device tree > > parsing error. > > > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > > --- > > drivers/of/property.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c > > index 408a7b5f06a9..4eefe8efc2fe 100644 > > --- a/drivers/of/property.c > > +++ b/drivers/of/property.c > > @@ -1308,7 +1308,6 @@ 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_iommu_maps(struct device_node *np, > > const char *prop_name, int index) > > @@ -1319,6 +1318,21 @@ static struct device_node *parse_iommu_maps(struct device_node *np, > > return of_parse_phandle(np, prop_name, (index * 4) + 1); > > } > > > > +static struct device_node *parse_gpios(struct device_node *np, > > + const char *prop_name, int index) > > +{ > > + /* > > + * Quirk for the deprecated "snps,nr-gpios" property of the > > + * DesignWare gpio-dwapb GPIO driver: as this property parsing > > + * conflicts with the "xx-gpios" phandle reference property, ignore it. > > + */ > > > + if (strcmp(prop_name, "snps,nr-gpios") == 0) > > + return NULL; > > What about printing the warning from instead of doing that from the driver? > Like this: > > + if (strcmp(prop_name, "snps,nr-gpios") == 0) { > + pr_warn("%pOF: %s is deprecated in favor of ngpios\n"); > + return NULL; > + } > > So when the property is removed from all dts'es we wouldn't > forget to discard the quirk? Why do we need this change at all? We've already got a message printed and devlink is still default off. If someone cares about devlink and the error message, then they should fix their dts file. Now if there's a stable/mature platform using "snps,nr-gpios" and we enable devlink by default (which needs to happen at some point), then I'd feel differently and we'll need to handle this. But until then, I don't want to carry this quirk. Rob
On 2020/12/01 3:22, Rob Herring wrote: > On Wed, Nov 25, 2020 at 2:06 PM Serge Semin <fancer.lancer@gmail.com> wrote: >> >> On Thu, Nov 19, 2020 at 03:09:20PM +0900, Damien Le Moal wrote: >>> The DesignWare gpio-dwapb GPIO driver ("snps,dw-apb-gpio" or >>> "apm,xgene-gpio-v2" compatible string) defines the now deprecated >>> property "snps,nr-gpios" to specify the number of GPIOs available >>> on a port. However, if this property is used in a device tree, its >>> "-gpios" suffix causes the generic property supplier parsing code to >>> interpret it as a cell reference when properties are parsed in >>> of_link_to_suppliers(), leading to an error messages such as: >>> >>> OF: /soc/bus@50200000/gpio-controller@50200000/gpio-port@0: could not >>> find phandle >>> >>> Fix this by manually defining a parse_gpios() function which ignores >>> this deprecated property that is still present in many device trees, >>> skipping the search for the supplier and thus avoiding the device tree >>> parsing error. >>> >>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>> --- >>> drivers/of/property.c | 16 +++++++++++++++- >>> 1 file changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/of/property.c b/drivers/of/property.c >>> index 408a7b5f06a9..4eefe8efc2fe 100644 >>> --- a/drivers/of/property.c >>> +++ b/drivers/of/property.c >>> @@ -1308,7 +1308,6 @@ 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_iommu_maps(struct device_node *np, >>> const char *prop_name, int index) >>> @@ -1319,6 +1318,21 @@ static struct device_node *parse_iommu_maps(struct device_node *np, >>> return of_parse_phandle(np, prop_name, (index * 4) + 1); >>> } >>> >>> +static struct device_node *parse_gpios(struct device_node *np, >>> + const char *prop_name, int index) >>> +{ >>> + /* >>> + * Quirk for the deprecated "snps,nr-gpios" property of the >>> + * DesignWare gpio-dwapb GPIO driver: as this property parsing >>> + * conflicts with the "xx-gpios" phandle reference property, ignore it. >>> + */ >> >>> + if (strcmp(prop_name, "snps,nr-gpios") == 0) >>> + return NULL; >> >> What about printing the warning from instead of doing that from the driver? >> Like this: >> >> + if (strcmp(prop_name, "snps,nr-gpios") == 0) { >> + pr_warn("%pOF: %s is deprecated in favor of ngpios\n"); >> + return NULL; >> + } >> >> So when the property is removed from all dts'es we wouldn't >> forget to discard the quirk? > > Why do we need this change at all? We've already got a message printed > and devlink is still default off. If someone cares about devlink and > the error message, then they should fix their dts file. > > Now if there's a stable/mature platform using "snps,nr-gpios" and we > enable devlink by default (which needs to happen at some point), then > I'd feel differently and we'll need to handle this. But until then, I > don't want to carry this quirk. I have the device tree fixed for my use case, so there is no problem anymore. The improvement this patch brings is a clearer error message. The one that shows up without the patch is fairly obscure and it took me a while to figure out what was wrong. But again, no problems for me since the DT is fixed. So sure, we can drop this patch. > > Rob >
diff --git a/drivers/of/property.c b/drivers/of/property.c index 408a7b5f06a9..4eefe8efc2fe 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1308,7 +1308,6 @@ 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_iommu_maps(struct device_node *np, const char *prop_name, int index) @@ -1319,6 +1318,21 @@ static struct device_node *parse_iommu_maps(struct device_node *np, return of_parse_phandle(np, prop_name, (index * 4) + 1); } +static struct device_node *parse_gpios(struct device_node *np, + const char *prop_name, int index) +{ + /* + * Quirk for the deprecated "snps,nr-gpios" property of the + * DesignWare gpio-dwapb GPIO driver: as this property parsing + * conflicts with the "xx-gpios" phandle reference property, ignore it. + */ + if (strcmp(prop_name, "snps,nr-gpios") == 0) + return NULL; + + return parse_suffix_prop_cells(np, prop_name, index, + "-gpios", "#gpio-cells"); +} + static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_clocks, }, { .parse_prop = parse_interconnects, },
The DesignWare gpio-dwapb GPIO driver ("snps,dw-apb-gpio" or "apm,xgene-gpio-v2" compatible string) defines the now deprecated property "snps,nr-gpios" to specify the number of GPIOs available on a port. However, if this property is used in a device tree, its "-gpios" suffix causes the generic property supplier parsing code to interpret it as a cell reference when properties are parsed in of_link_to_suppliers(), leading to an error messages such as: OF: /soc/bus@50200000/gpio-controller@50200000/gpio-port@0: could not find phandle Fix this by manually defining a parse_gpios() function which ignores this deprecated property that is still present in many device trees, skipping the search for the supplier and thus avoiding the device tree parsing error. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/of/property.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)