Message-ID: <CAMRc=MfNf+WMtSW=Wag0QHAaYzcRe9igrbOeRZiY92KmOH70oQ@mail.gmail.com>
Date: Wed, 19 Nov 2025 09:35:17 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc: Linus Walleij <linus.walleij@...aro.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Daniel Scally <djrscally@...il.com>, 
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Danilo Krummrich <dakr@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>, 
	Krzysztof Kozlowski <krzk@...nel.org>, linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-acpi@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, patches@...nsource.cirrus.com
Subject: Re: [PATCH v4 04/10] gpio: swnode: don't use the swnode's name as the
 key for GPIO lookup

On Tue, Nov 18, 2025 at 7:16 PM Charles Keepax
<ckeepax@...nsource.cirrus.com> wrote:
>
> On Tue, Nov 18, 2025 at 07:01:24PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 18, 2025 at 5:34 PM Charles Keepax
> > <ckeepax@...nsource.cirrus.com> wrote:
> > >
> > > On Mon, Nov 03, 2025 at 10:35:24AM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > > >
> > > > Looking up a GPIO controller by label that is the name of the software
> > > > node is wonky at best - the GPIO controller driver is free to set
> > > > a different label than the name of its firmware node. We're already being
> > > > passed a firmware node handle attached to the GPIO device to
> > > > swnode_get_gpio_device() so use it instead for a more precise lookup.
> > > >
> > > > Acked-by: Linus Walleij <linus.walleij@...aro.org>
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > > > ---
> > > >  drivers/gpio/gpiolib-swnode.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
> > > > index f21dbc28cf2c8c2d06d034b7c89d302cc52bb9b5..e3806db1c0e077d76fcc71a50ca40bbf6872ca40 100644
> > > > --- a/drivers/gpio/gpiolib-swnode.c
> > > > +++ b/drivers/gpio/gpiolib-swnode.c
> > > > @@ -41,7 +41,7 @@ static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
> > > >           !strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME))
> > > >               return ERR_PTR(-ENOENT);
> > > >
> > > > -     gdev = gpio_device_find_by_label(gdev_node->name);
> > > > +     gdev = gpio_device_find_by_fwnode(fwnode);
> > > >       return gdev ?: ERR_PTR(-EPROBE_DEFER);
> > > >  }
> > >
> > > One small problem is this does break drivers/spi/spi-cs42l43.c.
> >
> > I'd say it's a big problem. :)
> >
> > > That driver has to register some swnodes to specify some GPIO
> > > chip selects due to some squiffy ACPI from Windows land. Currently
> > > it relies on the sw node being called cs42l43-pinctrl to match
> > > the driver.
> > >
> >
> > What is the problem exactly? The "cs42l43-pinctrl" swnode is
> > associated with a GPIO device I suppose? Does it not find it? I'd need
> > some more information in order to figure out a way to fix it.
>
> Yeah doesn't find the GPIO device. Apologies the background is
> fairly lenghty here but to give a high level summary. The cs42l43
> is an audio CODEC but it has a SPI controller on it, in some
> configurations there are a couple of speaker amps connected to
> this SPI controller. For Window reasons this SPI controller isn't
> properly represented in ACPI, so we have to depend on a couple
> magic properties and then use software nodes to register the
> speaker amps. However, as part of this we need to register a
> cs-gpios property for the chip selects used by the amps.
>
> This chip select gpios property is where the problem happens, we
> need to refer to the gpio/pinctrl driver of the cs42l43, but
> software nodes only seem to allow referring to other software
> nodes. Previously this worked as we gave the node the same name
> as the real driver, which meant it found the real driver.
> However, after switching to look up by fwnode, it is looking for
> a gpio controller attached to the software node.
>

As mentioned by Andy: this sounds precisely like what this series
addresses with the reset-gpio driver. We now CAN reference fwnodes
from software nodes.


> static const struct software_node cs42l43_gpiochip_swnode = {
>         /* Previously matched the driver name for the pinctrl driver */
>         .name   =  "cs42l43-pinctrl",
> };
>
> static const struct software_node_ref_args cs42l43_cs_refs[] = {
>         /* This needs to point to the genuine pinctrl driver? */
>         SOFTWARE_NODE_REFERENCE(&cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW),
>         /* This is a mark that indicates the native chip select is used*/
>         SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined),
> };
>
> The bit that is unclear to me is how we get that software node
> property to point to the real pinctrl driver rather than the
> dummy software node. I think maybe we need to add a SW node as a
> secondary node on the pinctrl driver itself and link to that?
>
> Also happy from my side to spend some time working on this as I
> am not convinced what the driver is doing now is totally legit.

Well, it is sketchy. We register the cs42l43_gpiochip_swnode and
reference it but never assign it to the GPIO device. If you assigned
it as a secondary fwnode to the relevant GPIO device, it would have
been found during the lookup.

Right now, with how the lookup-by-label works in gpiolib-swnode.c, you
get the referenced software node and read its name but there's no real
link between the swnode and the GPIO device. It's just a big hack.

I have an idea for fixing it, let me cook up a patch. It'll still be a
bit hacky but will at least create a true link.

Bart

Powered by blists - more mailing lists