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:   Thu, 13 Oct 2022 13:59:55 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
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: <Y0gLy84EvqpOmXdd@maple.lan>
References: <20221011-gpiolib-quirks-v1-0-e01d9d3e7b29@gmail.com>
 <20221011-gpiolib-quirks-v1-2-e01d9d3e7b29@gmail.com>
 <Y0aS80PlA/T3mx2d@maple.lan>
 <Y0cTkubAA4637o5y@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y0cTkubAA4637o5y@google.com>
Precedence: bulk

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.