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

[Daniel Thompson <daniel.thompson@xxxxxxxxxx>]

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?


> +	} gpios[] = {
> +#if IS_ENABLED(CONFIG_MFD_ARIZONA)
> +		{ "wlf,reset",	NULL,		NULL },

CONFIG_REGULATOR_ARIZONA_LDO1 is better guard for this con id.


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


> +		{ "wlf,ldo1ena", NULL,		NULL }, /* WM8994 */
> +		{ "wlf,ldo2ena", NULL,		NULL }, /* WM8994 */

CONFIG_REGULATOR_WM8994 is a better guard for these.


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


>
> +		/*
> +		 * 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.

> +		{ "cs",		"gpios",	"ibm,ppc4xx-spi" },

CONFIG_SPI_PPC4xx for this.


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