diff mbox series

[v2,1/2] of: Fix property supplier parsing

Message ID 20201119060921.311747-2-damien.lemoal@wdc.com
State Superseded
Headers show
Series Fix dwapb gpio snps,nr-gpios property handling | expand

Commit Message

Damien Le Moal Nov. 19, 2020, 6:09 a.m. UTC
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(-)

Comments

Serge Semin Nov. 25, 2020, 9:06 p.m. UTC | #1
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

>
Rob Herring Nov. 30, 2020, 6:22 p.m. UTC | #2
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
Damien Le Moal Nov. 30, 2020, 9:45 p.m. UTC | #3
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 mbox series

Patch

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, },