Re: [PATCH 01/32] of: Fix property supplier parsing

From: Serge Semin <fancer.lancer@gmail.com>
To: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	linux-riscv@lists.infradead.org, Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	devicetree@vger.kernel.org, Mark Brown <broonie@kernel.org>,
	linux-spi@vger.kernel.org, Stephen Boyd <sboyd@kernel.org>,
	linux-clk@vger.kernel.org,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-gpio@vger.kernel.org,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Sean Anderson <seanga2@gmail.com>
Subject: Re: [PATCH 01/32] of: Fix property supplier parsing
Date: Mon, 9 Nov 2020 18:05:03 +0300	[thread overview]
Message-ID: <20201109150503.ijc72z76i6wjxfhx@mobilestation> (raw)
In-Reply-To: <20201107081420.60325-2-damien.lemoal@wdc.com>

On Sat, Nov 07, 2020 at 05:13:49PM +0900, Damien Le Moal wrote:
> The DesignWare GPIO driver gpio-dwapb ("snps,dw-apb-gpio" or
> "apm,xgene-gpio-v2" compatible string) defines the property
> "snps,nr-gpios" for the user to specify the number of GPIOs available
> on a port. The "-gpios" suffix of this property name ends up being
> interpreted as a cell reference when properties are parsed in
> of_link_to_suppliers(), leading to 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 property, skipping the search for the supplier and thus avoiding
> the device tree parsing error.

That's why I have introduced the "ngpios" property support and marked the 
"snps,nr-gpios" as deprecated here:
https://lkml.org/lkml/2020/7/22/1298

to encourage the later one from being used in favor of the first one. So I
suggest for you to convert your dts'es (if you have ones) to using the
"ngpios" property anyway.

> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/of/property.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 408a7b5f06a9..d16111c0d6da 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,22 @@ 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)
> +{
> +	/*
> +	 * Quirck for the DesignWare gpio-dwapb GPIO driver which defines
           ^
           |
           Quirk?     
> +	 * the "snps,nr-gpios" property to indicate the total number of GPIOs
> +	 * available. As this conflict with "xx-gpios" reference properties,
> +	 * ignore it.
> +	 */
> +	if (strcmp(prop_name, "snps,nr-gpios") == 0)
> +		return NULL;
> +
> +	return parse_suffix_prop_cells(np, prop_name, index,
> +				       "-gpios", "#gpio-cells");
> +}
> +

Personally I'd prefer to convert all the dts-es to using the "ngpios' instead of
the vendor-specific property. That's why I haven't fixed the problem the way you
suggest in the first place, to encourage people to send the patches with such
fixes. Anyway it's up to the OF-subsystem maintainers to decide whether to accept
this quirk.

-Sergey

>  static const struct supplier_bindings of_supplier_bindings[] = {
>  	{ .parse_prop = parse_clocks, },
>  	{ .parse_prop = parse_interconnects, },
> -- 
> 2.28.0
> 