Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Oct 2022 12:20:50 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org
Subject: Re: [PATCH 2/7] gpiolib: of: consolidate simple renames into a
 single quirk
Message-ID: <Y0cTkubAA4637o5y@google.com>
References: <20221011-gpiolib-quirks-v1-0-e01d9d3e7b29@gmail.com>
 <20221011-gpiolib-quirks-v1-2-e01d9d3e7b29@gmail.com>
 <Y0aS80PlA/T3mx2d@maple.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y0aS80PlA/T3mx2d@maple.lan>
Precedence: bulk

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@gmail.com>
> > ---
> >  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@linaro.org>
> 
> 
> > 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