Re: [PATCH 2/7] gpiolib: of: consolidate simple renames into a single quirk

[Daniel Thompson <daniel.thompson@linaro.org>]

On Wed, Oct 12, 2022 at 12:20:50PM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 12, 2022 at 11:12:03AM +0100, Daniel Thompson wrote:
> > On Tue, Oct 11, 2022 at 03:19:30PM -0700, Dmitry Torokhov wrote:
> > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > > index cef4f6634125..619aae0c5476 100644
> > > @@ -365,127 +365,83 @@ struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
> > > +static struct gpio_desc *of_find_gpio_rename(struct device_node *np,
> > >  					     const char *con_id,
> > >  					     unsigned int idx,
> > >  					     enum of_gpio_flags *of_flags)
> > >  {
> > > +	static const struct of_rename_gpio {
> > > +		const char *con_id;
> > > +		const char *legacy_id;	/* NULL - same as con_id */
> > > +		const char *compatible; /* NULL - don't check */
> >
> > "don't check" doesn't seem desirable. It's not too big a deal here
> > because everything affected has a vendor prefix (meaning incorrect
> > matching is unlikely). Should there be a comment about the general care
> > needed for a NULL compatible?

There were certainly a lot of compatibles affected by this translation
and given the structure of the drivers it is a tough code review to be
sure you have picked up *all* of them!


> I'll add the wording that NULL is only acceptable if property has a
> vendor prefi, Will that be OK? Otherwise I'll have to add a lot of
> entries for Arizona and Madera.
>
> >
> >
> > > +	} gpios[] = {
> > > +#if IS_ENABLED(CONFIG_MFD_ARIZONA)
> > > +		{ "wlf,reset",	NULL,		NULL },
> >
> > CONFIG_REGULATOR_ARIZONA_LDO1 is better guard for this con id.
>
> Are you sure? I see reset handling happening in
> drivers/mfd/arizona-core.c independently of regulator code...

Looks like I grepped for the wrong string so I was completely wrong
here... and in two different ways!

Firstly I'm wrong about replacing the guard. Existing guard is correct!

Secondly, I didn't notice until now that wm8804 also uses the
"wlf,reset" and it is a little odd that the wm8804 driver will accept
or refuse a misspelled binding based whether the kernel has enabled the
arizona drivers.

Overall I can live with the code we have today but this makes me wonder
if the comment discussed above should be stronger. Something like:
"the NULL compatible code is used there to support legacy entries in
the table; try to avoid adding new NULL entries".


Daniel.