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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Mon, Nov 16, 2020 at 07:30:15AM +0000, Damien Le Moal wrote:
> 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@xxxxxxx> 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,

Yes.

> albeit with an added warning as Rob suggested so
> that board maintainers can notice and update their DT.

Yes.

> 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 ?

It has already been added in 5.10:
https://elixir.bootlin.com/linux/v5.10-rc4/source/drivers/gpio/gpio-dwapb.c#L585
so there is no need in sending a patch for the gpio-dwapb.c driver.

-Sergey

> 
> 
> > 
> > -Sergey
> > 
> >>
> >> -- 
> >> With Best Regards,
> >> Andy Shevchenko
> > 
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research