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

From: Dmitry Torokhov
Date: Wed Oct 12 2022 - 15:21:00 EST


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:
> > This consolidates all quirks doing simple renames (either allowing
> > suffix-less names or trivial renames, when index changes are not
> > required) into a single quirk.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > ---
> > drivers/gpio/gpiolib-of.c | 176 +++++++++++++++++-----------------------------
> > 1 file changed, 64 insertions(+), 112 deletions(-)
>
> Nice diffstat, almost a shame that the diff algo itself has latched onto
> spurious anchor points to generate something that is so hard to read
> ;-) .
>
> I've reviewed this pretty closely and AFAICT it does exactly what the
> preivous code does. Thus the comments below are all related to things
> that the new table makes obvious that the previous code handled in a
> rather inconsistent way. Maybe that means these could/should be fixed
> in an extra patch within this patch set.
>
> I guess that means, despite the feedback below, *this* patch is:
> Reviewed-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
>
>
> > 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?

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

>
>
> > +#endif
> > +#if IS_ENABLED(CONFIG_REGULATOR)
> > + /*
> > + * Some regulator bindings happened before we managed to
> > + * establish that GPIO properties should be named
> > + * "foo-gpios" so we have this special kludge for them.
> > + */
> > + { "wlf,ldoena", NULL, NULL }, /* Arizona */
>
> CONFIG_REGULATOR_ARIZONA_LDO1 is better for this one too.

Good idea.

>
>
> > + { "wlf,ldo1ena", NULL, NULL }, /* WM8994 */
> > + { "wlf,ldo2ena", NULL, NULL }, /* WM8994 */
>
> CONFIG_REGULATOR_WM8994 is a better guard for these.

Yep.

>
>
> > +#endif
> > +#if IS_ENABLED(CONFIG_SPI_MASTER)
> > + /*
> > + * The SPI GPIO bindings happened before we managed to
> > + * establish that GPIO properties should be named
> > + * "foo-gpios" so we have this special kludge for them.
> > + */
> > + { "miso", "gpio-miso", "spi-gpio" },
> > + { "mosi", "gpio-mosi", "spi-gpio" },
> > + { "sck", "gpio-sck", "spi-gpio" },
>
> CONFIG_SPI_GPIO is a better guard for these.

OK.

>
>
> >
> > + /*
> > + * The old Freescale bindings use simply "gpios" as name
> > + * for the chip select lines rather than "cs-gpios" like
> > + * all other SPI hardware. Allow this specifically for
> > + * Freescale and PPC devices.
> > + */
> > + { "cs", "gpios", "fsl,spi" },
> > + { "cs", "gpios", "aeroflexgaisler,spictrl" },
>
> CONFIG_SPI_FSL_SPI for these.

OK.

>
> > + { "cs", "gpios", "ibm,ppc4xx-spi" },
>
> CONFIG_SPI_PPC4xx for this.

OK.

>
>
> > +#endif
> > +#if IS_ENABLED(CONFIG_TYPEC_FUSB302)
> > + /*
> > + * Fairchild FUSB302 host is using undocumented "fcs,int_n"
> > + * property without the compulsory "-gpios" suffix.
> > + */
> > + { "fcs,int_n", NULL, "fcs,fusb302" },
> > +#endif
> > };
> > + struct gpio_desc *desc;
> > + const char *legacy_id;
> > + unsigned int i;
> >
> > if (!con_id)
> > return ERR_PTR(-ENOENT);
> >
> > + for (i = 0; i < ARRAY_SIZE(gpios); i++) {
> > + if (strcmp(con_id, gpios[i].con_id))
> > + continue;
> >
> > + if (gpios[i].compatible &&
> > + !of_device_is_compatible(np, gpios[i].compatible))
> > + continue;
> >
> > + legacy_id = gpios[i].legacy_id ?: gpios[i].con_id;
> > + desc = of_get_named_gpiod_flags(np, legacy_id, idx, of_flags);
> > + if (!gpiod_not_found(desc)) {
> > + pr_info("%s uses legacy gpio name '%s' instead of '%s-gpios'\n",
> > + of_node_full_name(np), legacy_id, con_id);
> > + return desc;
> > + }
> > + }
> >
> > + return ERR_PTR(-ENOENT);
> > }
>
> I would normally trim last but this but given what git did to this particular
> patch I left it as a public service ;-) (it has the - parts of the
> patch removed).
>
>
> Daniel.

Thanks.

--
Dmitry