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

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2020/11/10 2:45, Serge Semin wrote:
> Hello Andy,
> 
> On Mon, Nov 09, 2020 at 05:14:21PM +0200, Andy Shevchenko wrote:
>> On Sat, Nov 7, 2020 at 10:14 AM Damien Le Moal <damien.lemoal@wdc.com> wrote:
>>
>>> @@ -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")
>>
>> Sorry, but the above doesn't sound right to me.
>> It's a generic code and you may imagine how many systems you broke by
>> this change.
> 
> Damien replaced the macro above with the code below (your removed it from your
> message):
> 
> +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
> +        * 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");
> +}
> 
> So AFAICS removing the macro shouldn't cause any problem.
> 
> My concern was whether the quirk has been really needed. As I said the
> "snps,nr-gpios" property has been marked as deprecated in favor of the standard
> "ngpios" one. Due to the problem noted by Damien any deprecated property
> utilization will cause the DW APB SSI DT-nodes probe malfunction. That
> though implicitly but is supposed to encourage people to provide fixes for
> the dts-files with the deprecated property replaced with "ngpios".
> 
> On the other hand an encouragement based on breaking the kernel doesn't seem a
> good solution. So as I see it either we should accept the solution provided by
> Damien, or replace it with a series of fixes for all dts-es with DW APB SSI
> DT-node defined. I suggest to hear the OF-subsystem maintainers out what
> solution would they prefer.

As Rob mentioned, there are still a lot of DTS out there using "snps,nr-gpios",
so I think the fix is needed, albeit with an added warning as Rob suggested so
that board maintainers can notice and update their DT. And I can send a patch
for the DW gpio apb driver to first try the default "ngpios" property, and if it
is not defined, fallback to the legacy "snps,nr-gpios". With that, these new
RISC-V boards will not add another use case of the deprecated "snsps,nr-gpios".
Does that sound like a good plan ?


> 
> -Sergey
> 
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko
> 


-- 
Damien Le Moal
Western Digital Research