From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Currently the SPI driver sets up a software node rerefencing the GPIO
controller exposing the chip-select GPIO but this node never gets
attached to the actual GPIO provider. The lookup uses the weird way GPIO
core performs the software node lookup by the swnode's name. We want to
switch to a true firmware node lookup so the actual link must exist.
Move the configuration to the MFD core and connect the SPI and pinctrl
sub-devices with software node references.

Fixes: 439fbc97502a ("spi: cs42l43: Add bridged cs35l56 amplifiers")
Reported-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Closes: https://lore.kernel.org/all/aRyf7qDdHKABppP8@opensource.cirrus.com/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Hi Charles!

Please give this a try. It's only build-tested so far. I hope I
understood correctly that it's the SPI driver that needs the "cs" GPIO
from the pinctrl.

Bartosz
---
 drivers/mfd/cs42l43.c     | 25 +++++++++++++++++++++++--
 drivers/spi/spi-cs42l43.c | 33 ---------------------------------
 2 files changed, 23 insertions(+), 35 deletions(-)

diff --git a/drivers/mfd/cs42l43.c b/drivers/mfd/cs42l43.c
index 107cfb983fec416bbdd31caa1e6a569026467735..629faf8674af651eb64aa63ec768ba18c1c8b311 100644
--- a/drivers/mfd/cs42l43.c
+++ b/drivers/mfd/cs42l43.c
@@ -14,6 +14,8 @@
 #include <linux/err.h>
 #include <linux/firmware.h>
 #include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
+#include <linux/gpio/property.h>
 #include <linux/jiffies.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/cs42l43.h>
@@ -512,9 +514,28 @@ static const char * const cs42l43_core_supplies[] = {
 
 static const char * const cs42l43_parent_supplies[] = { "vdd-amp" };
 
+static const struct software_node cs42l43_gpiochip_swnode = {
+	.name = "cs42l43-pinctrl",
+};
+
+static const struct property_entry cs42l43_cs_props[] = {
+	PROPERTY_ENTRY_GPIO("cs-gpios", &cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW),
+	{ }
+};
+
+static const struct software_node cs42l43_spi_swnode = {
+	.properties = cs42l43_cs_props,
+};
+
 static const struct mfd_cell cs42l43_devs[] = {
-	{ .name = "cs42l43-pinctrl", },
-	{ .name = "cs42l43-spi", },
+	{
+		.name = "cs42l43-pinctrl",
+		.swnode = &cs42l43_gpiochip_swnode,
+	},
+	{
+		.name = "cs42l43-spi",
+		.swnode = &cs42l43_spi_swnode,
+	},
 	{
 		.name = "cs42l43-codec",
 		.parent_supplies = cs42l43_parent_supplies,
diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c
index 14307dd800b744fee17edd864688a68c65666c68..fa844ce6cd7539bf764f792b99e8f8e191fc0c15 100644
--- a/drivers/spi/spi-cs42l43.c
+++ b/drivers/spi/spi-cs42l43.c
@@ -52,20 +52,6 @@ static struct spi_board_info amp_info_template = {
 	.mode			= SPI_MODE_0,
 };
 
-static const struct software_node cs42l43_gpiochip_swnode = {
-	.name			= "cs42l43-pinctrl",
-};
-
-static const struct software_node_ref_args cs42l43_cs_refs[] = {
-	SOFTWARE_NODE_REFERENCE(&cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW),
-	SOFTWARE_NODE_REFERENCE(&swnode_gpio_undefined),
-};
-
-static const struct property_entry cs42l43_cs_props[] = {
-	PROPERTY_ENTRY_REF_ARRAY("cs-gpios", cs42l43_cs_refs),
-	{}
-};
-
 static int cs42l43_spi_tx(struct regmap *regmap, const u8 *buf, unsigned int len)
 {
 	const u8 *end = buf + len;
@@ -324,11 +310,6 @@ static void cs42l43_release_of_node(void *data)
 	fwnode_handle_put(data);
 }
 
-static void cs42l43_release_sw_node(void *data)
-{
-	software_node_unregister(&cs42l43_gpiochip_swnode);
-}
-
 static int cs42l43_spi_probe(struct platform_device *pdev)
 {
 	struct cs42l43 *cs42l43 = dev_get_drvdata(pdev->dev.parent);
@@ -402,20 +383,6 @@ static int cs42l43_spi_probe(struct platform_device *pdev)
 				return dev_err_probe(priv->dev, ret,
 						     "Failed to get spk-id-gpios\n");
 		}
-
-		ret = software_node_register(&cs42l43_gpiochip_swnode);
-		if (ret)
-			return dev_err_probe(priv->dev, ret,
-					     "Failed to register gpio swnode\n");
-
-		ret = devm_add_action_or_reset(priv->dev, cs42l43_release_sw_node, NULL);
-		if (ret)
-			return ret;
-
-		ret = device_create_managed_software_node(&priv->ctlr->dev,
-							  cs42l43_cs_props, NULL);
-		if (ret)
-			return dev_err_probe(priv->dev, ret, "Failed to add swnode\n");
 	} else {
 		device_set_node(&priv->ctlr->dev, fwnode);
 	}

---
base-commit: fe4d0dea039f2befb93f27569593ec209843b0f5
change-id: 20251119-cs42l43-gpio-swnodes-8d4a7e732d09

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

On Wed, Nov 19, 2025 at 10:10:24AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Currently the SPI driver sets up a software node rerefencing the GPIO
> controller exposing the chip-select GPIO but this node never gets
> attached to the actual GPIO provider. The lookup uses the weird way GPIO
> core performs the software node lookup by the swnode's name. We want to
> switch to a true firmware node lookup so the actual link must exist.
> Move the configuration to the MFD core and connect the SPI and pinctrl
> sub-devices with software node references.
> 
> Fixes: 439fbc97502a ("spi: cs42l43: Add bridged cs35l56 amplifiers")
> Reported-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Closes: https://lore.kernel.org/all/aRyf7qDdHKABppP8@opensource.cirrus.com/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> Hi Charles!
> 
> Please give this a try. It's only build-tested so far. I hope I
> understood correctly that it's the SPI driver that needs the "cs" GPIO
> from the pinctrl.

Unfortunately it fails with an -EBUSY on adding the MFD children.
I will investigate a little more and report back.

> +static const struct software_node cs42l43_gpiochip_swnode = {
> +	.name = "cs42l43-pinctrl",
> +};
> +
> +static const struct property_entry cs42l43_cs_props[] = {
> +	PROPERTY_ENTRY_GPIO("cs-gpios", &cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW),
> +	{ }
> +};

This drops the undefined node, that is important as it lets the
SPI core know that device is using an internal chip select rather
than a GPIO.

> +static const struct software_node cs42l43_spi_swnode = {
> +	.properties = cs42l43_cs_props,
> +};
> +
>  static const struct mfd_cell cs42l43_devs[] = {
> -	{ .name = "cs42l43-pinctrl", },
> -	{ .name = "cs42l43-spi", },
> +	{
> +		.name = "cs42l43-pinctrl",
> +		.swnode = &cs42l43_gpiochip_swnode,
> +	},
> +	{
> +		.name = "cs42l43-spi",
> +		.swnode = &cs42l43_spi_swnode,

I will need to think about this a little more but I suspect this
no longer being conditional on the magic ACPI properties could
cause issues if a system was to use the SPI controller more
traditionally and actually have CS GPIOs in the firmware.
Although maybe that real node would take preference over the
swnode?

Thanks,
Charles

On Wed, Nov 19, 2025 at 10:31 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Wed, Nov 19, 2025 at 10:10:24AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Currently the SPI driver sets up a software node rerefencing the GPIO
> > controller exposing the chip-select GPIO but this node never gets
> > attached to the actual GPIO provider. The lookup uses the weird way GPIO
> > core performs the software node lookup by the swnode's name. We want to
> > switch to a true firmware node lookup so the actual link must exist.
> > Move the configuration to the MFD core and connect the SPI and pinctrl
> > sub-devices with software node references.
> >
> > Fixes: 439fbc97502a ("spi: cs42l43: Add bridged cs35l56 amplifiers")
> > Reported-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > Closes: https://lore.kernel.org/all/aRyf7qDdHKABppP8@opensource.cirrus.com/
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> > Hi Charles!
> >
> > Please give this a try. It's only build-tested so far. I hope I
> > understood correctly that it's the SPI driver that needs the "cs" GPIO
> > from the pinctrl.
>
> Unfortunately it fails with an -EBUSY on adding the MFD children.
> I will investigate a little more and report back.
>

Does it fail in device_add_software_node() in MFD core? Is it possible
that the device already has a software node for some reason?

> > +static const struct software_node cs42l43_gpiochip_swnode = {
> > +     .name = "cs42l43-pinctrl",
> > +};
> > +
> > +static const struct property_entry cs42l43_cs_props[] = {
> > +     PROPERTY_ENTRY_GPIO("cs-gpios", &cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW),
> > +     { }
> > +};
>
> This drops the undefined node, that is important as it lets the
> SPI core know that device is using an internal chip select rather
> than a GPIO.
>

I really dislike the whole concept of this undefined software node.
This driver is literally the only user and I'd love to just get rid of
it. HOW exactly does it let the driver know to use internal chip
select? Can we do it differently?

> > +static const struct software_node cs42l43_spi_swnode = {
> > +     .properties = cs42l43_cs_props,
> > +};
> > +
> >  static const struct mfd_cell cs42l43_devs[] = {
> > -     { .name = "cs42l43-pinctrl", },
> > -     { .name = "cs42l43-spi", },
> > +     {
> > +             .name = "cs42l43-pinctrl",
> > +             .swnode = &cs42l43_gpiochip_swnode,
> > +     },
> > +     {
> > +             .name = "cs42l43-spi",
> > +             .swnode = &cs42l43_spi_swnode,
>
> I will need to think about this a little more but I suspect this
> no longer being conditional on the magic ACPI properties could
> cause issues if a system was to use the SPI controller more
> traditionally and actually have CS GPIOs in the firmware.
> Although maybe that real node would take preference over the
> swnode?
>

The lookup goes to the primary firmware node first, it would typically
be DT or ACPI node. Then if no match, it will go to the secondary
node.

Bart

On Wed, Nov 19, 2025 at 10:40:30AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 19, 2025 at 10:31 AM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> >
> > On Wed, Nov 19, 2025 at 10:10:24AM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Currently the SPI driver sets up a software node rerefencing the GPIO
> > > controller exposing the chip-select GPIO but this node never gets
> > > attached to the actual GPIO provider. The lookup uses the weird way GPIO
> > > core performs the software node lookup by the swnode's name. We want to
> > > switch to a true firmware node lookup so the actual link must exist.
> > > Move the configuration to the MFD core and connect the SPI and pinctrl
> > > sub-devices with software node references.
> > >
> > > Fixes: 439fbc97502a ("spi: cs42l43: Add bridged cs35l56 amplifiers")
> > > Reported-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > > Closes: https://lore.kernel.org/all/aRyf7qDdHKABppP8@opensource.cirrus.com/
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> > > Hi Charles!
> > >
> > > Please give this a try. It's only build-tested so far. I hope I
> > > understood correctly that it's the SPI driver that needs the "cs" GPIO
> > > from the pinctrl.
> >
> > Unfortunately it fails with an -EBUSY on adding the MFD children.
> > I will investigate a little more and report back.
> 
> Does it fail in device_add_software_node() in MFD core? Is it possible
> that the device already has a software node for some reason?

Yeah it seems there is already a software node, although somewhat
at a loss as to why, the only ones we add are inside the SPI
driver. I will poke further and see if I can find out how it
acquires that node.

> > > +static const struct software_node cs42l43_gpiochip_swnode = {
> > > +     .name = "cs42l43-pinctrl",
> > > +};
> > > +
> > > +static const struct property_entry cs42l43_cs_props[] = {
> > > +     PROPERTY_ENTRY_GPIO("cs-gpios", &cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW),
> > > +     { }
> > > +};
> >
> > This drops the undefined node, that is important as it lets the
> > SPI core know that device is using an internal chip select rather
> > than a GPIO.
> 
> I really dislike the whole concept of this undefined software node.
> This driver is literally the only user and I'd love to just get rid of
> it. HOW exactly does it let the driver know to use internal chip
> select? Can we do it differently?

The SPI code lives in spi_get_gpio_descs(). The basic gist is
it will use a native chip select if the gpiod_get_index return
NULL. The system came from device tree (see Documentation
spi-controller.yaml) initially, then I added an analogue to
swnodes a while back in 9d50f95bc0d5 ("gpio: swnode: Add
ability to specify native chip selects for SPI").  I have no
great attachment to this way of doing it, however, it does
seem logical to me that the system is at least fairly similar
between the different firmware types.

Thanks,
Charles

On Wed, Nov 19, 2025 at 10:58 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> > > > Hi Charles!
> > > >
> > > > Please give this a try. It's only build-tested so far. I hope I
> > > > understood correctly that it's the SPI driver that needs the "cs" GPIO
> > > > from the pinctrl.
> > >
> > > Unfortunately it fails with an -EBUSY on adding the MFD children.
> > > I will investigate a little more and report back.
> >
> > Does it fail in device_add_software_node() in MFD core? Is it possible
> > that the device already has a software node for some reason?
>
> Yeah it seems there is already a software node, although somewhat
> at a loss as to why, the only ones we add are inside the SPI
> driver. I will poke further and see if I can find out how it
> acquires that node.
>

Let me know if you figure it out, I'll submit a v2 that doesn't drop
the undefined swnode.

Bart

On Wed, Nov 19, 2025 at 11:38:35AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 19, 2025 at 10:58 AM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> >
> > > > > Hi Charles!
> > > > >
> > > > > Please give this a try. It's only build-tested so far. I hope I
> > > > > understood correctly that it's the SPI driver that needs the "cs" GPIO
> > > > > from the pinctrl.
> > > >
> > > > Unfortunately it fails with an -EBUSY on adding the MFD children.
> > > > I will investigate a little more and report back.
> > >
> > > Does it fail in device_add_software_node() in MFD core? Is it possible
> > > that the device already has a software node for some reason?
> >
> > Yeah it seems there is already a software node, although somewhat
> > at a loss as to why, the only ones we add are inside the SPI
> > driver. I will poke further and see if I can find out how it
> > acquires that node.
> >
> 
> Let me know if you figure it out, I'll submit a v2 that doesn't drop
> the undefined swnode.

Cool, thanks for all your help here. Might take me a couple hours
but I will get to the bottom of the EBUSY thing, and report back.

Thanks,
Charles

On Wed, Nov 19, 2025 at 11:47 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Wed, Nov 19, 2025 at 11:38:35AM +0100, Bartosz Golaszewski wrote:
> > On Wed, Nov 19, 2025 at 10:58 AM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > >
> > > > > > Hi Charles!
> > > > > >
> > > > > > Please give this a try. It's only build-tested so far. I hope I
> > > > > > understood correctly that it's the SPI driver that needs the "cs" GPIO
> > > > > > from the pinctrl.
> > > > >
> > > > > Unfortunately it fails with an -EBUSY on adding the MFD children.
> > > > > I will investigate a little more and report back.
> > > >
> > > > Does it fail in device_add_software_node() in MFD core? Is it possible
> > > > that the device already has a software node for some reason?
> > >
> > > Yeah it seems there is already a software node, although somewhat
> > > at a loss as to why, the only ones we add are inside the SPI
> > > driver. I will poke further and see if I can find out how it
> > > acquires that node.
> > >
> >
> > Let me know if you figure it out, I'll submit a v2 that doesn't drop
> > the undefined swnode.
>
> Cool, thanks for all your help here. Might take me a couple hours
> but I will get to the bottom of the EBUSY thing, and report back.
>

Sure. Could you just post the stack trace down to where the EBUSY is
returned in drivers/base/swnode.c? I'd like to analyze the logic
myself too if that's not too much work for you.

Bart

On Wed, Nov 19, 2025 at 11:50:09AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 19, 2025 at 11:47 AM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Wed, Nov 19, 2025 at 11:38:35AM +0100, Bartosz Golaszewski wrote:

...

> > Cool, thanks for all your help here. Might take me a couple hours
> > but I will get to the bottom of the EBUSY thing, and report back.
> 
> Sure. Could you just post the stack trace down to where the EBUSY is
> returned in drivers/base/swnode.c? I'd like to analyze the logic
> myself too if that's not too much work for you.


FWIW, there is also an interesting debug technique. Put in your driver where
you get this error code something like this (after all #include directives):

#undef EBUSY
#define EBUSY __LINE__

And then you get some error code which is line number in some C file. You can
narrow down grepping of the EBUSY in the suspected files and get the one.

	git grep -n -w EBUSY -- ...files of interest...

Hope this saves some minutes for you.

-- 
With Best Regards,
Andy Shevchenko

On Wed, Nov 19, 2025 at 11:58 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Nov 19, 2025 at 11:50:09AM +0100, Bartosz Golaszewski wrote:
> > On Wed, Nov 19, 2025 at 11:47 AM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > On Wed, Nov 19, 2025 at 11:38:35AM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > Cool, thanks for all your help here. Might take me a couple hours
> > > but I will get to the bottom of the EBUSY thing, and report back.
> >
> > Sure. Could you just post the stack trace down to where the EBUSY is
> > returned in drivers/base/swnode.c? I'd like to analyze the logic
> > myself too if that's not too much work for you.
>
>
> FWIW, there is also an interesting debug technique. Put in your driver where
> you get this error code something like this (after all #include directives):
>
> #undef EBUSY
> #define EBUSY __LINE__
>
> And then you get some error code which is line number in some C file. You can
> narrow down grepping of the EBUSY in the suspected files and get the one.
>
>         git grep -n -w EBUSY -- ...files of interest...
>
> Hope this saves some minutes for you.
>

Charles confirmed he gets it from device_add_software_node(). I want
to know how we're getting there and how we could end up already having
a software node.

Bartosz

On Wed, Nov 19, 2025 at 12:06:57PM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 19, 2025 at 11:58 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Wed, Nov 19, 2025 at 11:50:09AM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Nov 19, 2025 at 11:47 AM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:
> > > > On Wed, Nov 19, 2025 at 11:38:35AM +0100, Bartosz Golaszewski wrote:
> >
> > ...
> >
> > > > Cool, thanks for all your help here. Might take me a couple hours
> > > > but I will get to the bottom of the EBUSY thing, and report back.
> > >
> > > Sure. Could you just post the stack trace down to where the EBUSY is
> > > returned in drivers/base/swnode.c? I'd like to analyze the logic
> > > myself too if that's not too much work for you.
> >
> >
> > FWIW, there is also an interesting debug technique. Put in your driver where
> > you get this error code something like this (after all #include directives):
> >
> > #undef EBUSY
> > #define EBUSY __LINE__
> >
> > And then you get some error code which is line number in some C file. You can
> > narrow down grepping of the EBUSY in the suspected files and get the one.
> >
> >         git grep -n -w EBUSY -- ...files of interest...
> >
> > Hope this saves some minutes for you.
> >
> 
> Charles confirmed he gets it from device_add_software_node(). I want
> to know how we're getting there and how we could end up already having
> a software node.

Ok I think I see what is happening now, the swnode is created on
the first cell (the pinctrl). Then it moves onto the second cell,
but mfd_acpi_add_device() copies the firmware node into both
devices, the device_set_node() call at the bottom. So it inherits
the swnode node through that primary fwnode.

I am guessing this code has perhaps been more heavily tested on
device tree where it is more common to have nodes for each cell,
whereas ACPI is far more likely to have a single firmware node for
the whole device.

Stack dump as requested:

[    8.130022] Tainted: [S]=CPU_OUT_OF_SPEC, [E]=UNSIGNED_MODULE
[    8.130023] Hardware name: AAEON UPX-TGL01/UPX-TGL01, BIOS UXTGBM16 03/31/2022
[    8.130025] Workqueue: events_long cs42l43_boot_work [cs42l43]
[    8.130032] Call Trace:
[    8.130034]  <TASK>
[    8.130037]  dump_stack_lvl+0x5d/0x80
[    8.130048]  device_add_software_node+0x5c/0xb2
[    8.130054]  mfd_add_device+0x248/0x447 [mfd_core]
[    8.130061]  ? _printk+0x6b/0x90
[    8.130069]  devm_mfd_add_devices.cold+0x3b/0x70a [mfd_core]
[    8.130077]  cs42l43_boot_work.cold+0x1d3/0x7f7 [cs42l43]
[    8.130084]  process_one_work+0x237/0x5c0
[    8.130096]  worker_thread+0x1e1/0x3d0
[    8.130103]  ? __pfx_worker_thread+0x10/0x10
[    8.130107]  kthread+0x10d/0x250
[    8.130112]  ? __pfx_kthread+0x10/0x10
[    8.130117]  ret_from_fork+0x182/0x1d0
[    8.130120]  ? __pfx_kthread+0x10/0x10
[    8.130123]  ret_from_fork_asm+0x1a/0x30
[    8.130134]  </TASK>

Thanks,
Charles

On Wed, 19 Nov 2025 12:24:09 +0100, Charles Keepax
<ckeepax@opensource.cirrus.com> said:
> On Wed, Nov 19, 2025 at 12:06:57PM +0100, Bartosz Golaszewski wrote:
>> On Wed, Nov 19, 2025 at 11:58 AM Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> >
>> > On Wed, Nov 19, 2025 at 11:50:09AM +0100, Bartosz Golaszewski wrote:
>> > > On Wed, Nov 19, 2025 at 11:47 AM Charles Keepax
>> > > <ckeepax@opensource.cirrus.com> wrote:
>> > > > On Wed, Nov 19, 2025 at 11:38:35AM +0100, Bartosz Golaszewski wrote:
>> >
>> > ...
>> >
>> > > > Cool, thanks for all your help here. Might take me a couple hours
>> > > > but I will get to the bottom of the EBUSY thing, and report back.
>> > >
>> > > Sure. Could you just post the stack trace down to where the EBUSY is
>> > > returned in drivers/base/swnode.c? I'd like to analyze the logic
>> > > myself too if that's not too much work for you.
>> >
>> >
>> > FWIW, there is also an interesting debug technique. Put in your driver where
>> > you get this error code something like this (after all #include directives):
>> >
>> > #undef EBUSY
>> > #define EBUSY __LINE__
>> >
>> > And then you get some error code which is line number in some C file. You can
>> > narrow down grepping of the EBUSY in the suspected files and get the one.
>> >
>> >         git grep -n -w EBUSY -- ...files of interest...
>> >
>> > Hope this saves some minutes for you.
>> >
>>
>> Charles confirmed he gets it from device_add_software_node(). I want
>> to know how we're getting there and how we could end up already having
>> a software node.
>
> Ok I think I see what is happening now, the swnode is created on
> the first cell (the pinctrl). Then it moves onto the second cell,
> but mfd_acpi_add_device() copies the firmware node into both
> devices, the device_set_node() call at the bottom. So it inherits
> the swnode node through that primary fwnode.
>

You probably mean this line:

     device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));

What is the actual device whose node we copy here? Would doing the following
help?

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 7d14a1e7631ee..f7843f179e129 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -55,6 +55,7 @@ static void mfd_acpi_add_device(const struct mfd_cell *cell,
 				struct platform_device *pdev)
 {
 	const struct mfd_cell_acpi_match *match = cell->acpi_match;
+	struct fwnode_handle *acpi_fwnode;
 	struct acpi_device *adev = NULL;
 	struct acpi_device *parent;

@@ -87,7 +88,10 @@ static void mfd_acpi_add_device(const struct mfd_cell *cell,
 		}
 	}

-	device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));
+	acpi_fwnode = acpi_fwnode_handle(adev ?: parent);
+
+	if (!is_software_node(acpi_fwnode) || !cell->swnode)
+		device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));
 }
 #else
 static inline void mfd_acpi_add_device(const struct mfd_cell *cell,

> I am guessing this code has perhaps been more heavily tested on
> device tree where it is more common to have nodes for each cell,
> whereas ACPI is far more likely to have a single firmware node for
> the whole device.
>

If my logic above is right, we should not set the node here unless it's
an actual node coming from firmware OR the cell doesn't define its own
software node.

Bart

> Stack dump as requested:
>
> [    8.130022] Tainted: [S]=CPU_OUT_OF_SPEC, [E]=UNSIGNED_MODULE
> [    8.130023] Hardware name: AAEON UPX-TGL01/UPX-TGL01, BIOS UXTGBM16 03/31/2022
> [    8.130025] Workqueue: events_long cs42l43_boot_work [cs42l43]
> [    8.130032] Call Trace:
> [    8.130034]  <TASK>
> [    8.130037]  dump_stack_lvl+0x5d/0x80
> [    8.130048]  device_add_software_node+0x5c/0xb2
> [    8.130054]  mfd_add_device+0x248/0x447 [mfd_core]
> [    8.130061]  ? _printk+0x6b/0x90
> [    8.130069]  devm_mfd_add_devices.cold+0x3b/0x70a [mfd_core]
> [    8.130077]  cs42l43_boot_work.cold+0x1d3/0x7f7 [cs42l43]
> [    8.130084]  process_one_work+0x237/0x5c0
> [    8.130096]  worker_thread+0x1e1/0x3d0
> [    8.130103]  ? __pfx_worker_thread+0x10/0x10
> [    8.130107]  kthread+0x10d/0x250
> [    8.130112]  ? __pfx_kthread+0x10/0x10
> [    8.130117]  ret_from_fork+0x182/0x1d0
> [    8.130120]  ? __pfx_kthread+0x10/0x10
> [    8.130123]  ret_from_fork_asm+0x1a/0x30
> [    8.130134]  </TASK>
>
> Thanks,
> Charles
>

On Wed, Nov 19, 2025 at 03:58:08AM -0800, Bartosz Golaszewski wrote:
> On Wed, 19 Nov 2025 12:24:09 +0100, Charles Keepax
> <ckeepax@opensource.cirrus.com> said:
> > On Wed, Nov 19, 2025 at 12:06:57PM +0100, Bartosz Golaszewski wrote:
> >> On Wed, Nov 19, 2025 at 11:58 AM Andy Shevchenko
> > Ok I think I see what is happening now, the swnode is created on
> > the first cell (the pinctrl). Then it moves onto the second cell,
> > but mfd_acpi_add_device() copies the firmware node into both
> > devices, the device_set_node() call at the bottom. So it inherits
> > the swnode node through that primary fwnode.
> >
> 
> You probably mean this line:
> 
>      device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));

Indeed yeah that one.

> What is the actual device whose node we copy here? Would doing the following
> help?

Its the actual ACPI node for the device, the cs42l43, sorry if
that isn't what you are looking for not sure I totally follow the
question here.

> -	device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));
> +	acpi_fwnode = acpi_fwnode_handle(adev ?: parent);
> +
> +	if (!is_software_node(acpi_fwnode) || !cell->swnode)
> +		device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));
>  }
>  #else
>  static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
> 
> > I am guessing this code has perhaps been more heavily tested on
> > device tree where it is more common to have nodes for each cell,
> > whereas ACPI is far more likely to have a single firmware node for
> > the whole device.
> >
> 
> If my logic above is right, we should not set the node here unless it's
> an actual node coming from firmware OR the cell doesn't define its own
> software node.

Will that not leave the MFD children without access to the actual ACPI
node though? (Not tested just eye-balling).

Can we tackle this the other way around? Since there is only a
single fwnode for the device, can we find a way to get away with
a single software node for the device too?

Thanks,
Charles

On Wed, Nov 19, 2025 at 1:53 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Wed, Nov 19, 2025 at 03:58:08AM -0800, Bartosz Golaszewski wrote:
> > On Wed, 19 Nov 2025 12:24:09 +0100, Charles Keepax
> > <ckeepax@opensource.cirrus.com> said:
> > > On Wed, Nov 19, 2025 at 12:06:57PM +0100, Bartosz Golaszewski wrote:
> > >> On Wed, Nov 19, 2025 at 11:58 AM Andy Shevchenko
> > > Ok I think I see what is happening now, the swnode is created on
> > > the first cell (the pinctrl). Then it moves onto the second cell,
> > > but mfd_acpi_add_device() copies the firmware node into both
> > > devices, the device_set_node() call at the bottom. So it inherits
> > > the swnode node through that primary fwnode.
> > >
> >
> > You probably mean this line:
> >
> >      device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));
>
> Indeed yeah that one.
>
> > What is the actual device whose node we copy here? Would doing the following
> > help?
>
> Its the actual ACPI node for the device, the cs42l43, sorry if
> that isn't what you are looking for not sure I totally follow the
> question here.
>

So it's !is_software_node() and  is_acpi_device_node() instead? Then
disregard my suggestion.

> > -     device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));
> > +     acpi_fwnode = acpi_fwnode_handle(adev ?: parent);
> > +
> > +     if (!is_software_node(acpi_fwnode) || !cell->swnode)
> > +             device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));
> >  }
> >  #else
> >  static inline void mfd_acpi_add_device(const struct mfd_cell *cell,
> >
> > > I am guessing this code has perhaps been more heavily tested on
> > > device tree where it is more common to have nodes for each cell,
> > > whereas ACPI is far more likely to have a single firmware node for
> > > the whole device.
> > >
> >
> > If my logic above is right, we should not set the node here unless it's
> > an actual node coming from firmware OR the cell doesn't define its own
> > software node.
>
> Will that not leave the MFD children without access to the actual ACPI
> node though? (Not tested just eye-balling).
>

Yeah, nevermind it.

> Can we tackle this the other way around? Since there is only a
> single fwnode for the device, can we find a way to get away with
> a single software node for the device too?
>

I still don't understand what the software node that's already
assigned to the SPI device is though? device_add_software_node()
should work just fine if the only other firmware node the device has
is the ACPI device node.

Bart

On Wed, Nov 19, 2025 at 02:07:55PM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 19, 2025 at 1:53 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Wed, Nov 19, 2025 at 03:58:08AM -0800, Bartosz Golaszewski wrote:
> > > On Wed, 19 Nov 2025 12:24:09 +0100, Charles Keepax
> > > <ckeepax@opensource.cirrus.com> said:
> > > > On Wed, Nov 19, 2025 at 12:06:57PM +0100, Bartosz Golaszewski wrote:
> > > >> On Wed, Nov 19, 2025 at 11:58 AM Andy Shevchenko
> > Can we tackle this the other way around? Since there is only a
> > single fwnode for the device, can we find a way to get away with
> > a single software node for the device too?
> 
> I still don't understand what the software node that's already
> assigned to the SPI device is though? device_add_software_node()
> should work just fine if the only other firmware node the device has
> is the ACPI device node.

Its the software node we assigned to the first MFD cell, that one
succeeds but attaches itself to the ACPI node as a secondary.
When we get to the second cell we try to attach a new node but we
get the one from the first cell since they share an ACPI node.

I think as Andy pointed out though the first 4 patches in your
chain do loosely want we want. Previously, we used the name to
point to the actual pinctrl driver, your patches should let us
do that properly through the fwnode. So we can drop the pinctrl
swnode and just have the cs-gpios bit point at the actual fwnode
instead. I am trying to hack together a strawman but its failing
in a lightly odd way. Hopefully I can get that sorted fairly
soon and post, or I guess I could post a version earlier if you
wanted a look in the knowledge it still doesn't work?

Thanks,
Charles

On Wed, Nov 19, 2025 at 2:27 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> I think as Andy pointed out though the first 4 patches in your
> chain do loosely want we want. Previously, we used the name to
> point to the actual pinctrl driver, your patches should let us
> do that properly through the fwnode. So we can drop the pinctrl
> swnode and just have the cs-gpios bit point at the actual fwnode
> instead. I am trying to hack together a strawman but its failing
> in a lightly odd way. Hopefully I can get that sorted fairly
> soon and post, or I guess I could post a version earlier if you
> wanted a look in the knowledge it still doesn't work?
>

If your solution won't work after all, we can try using machine lookup
instead. We have all the blocks except for support for the "undefined"
GPIO in machine lookup. That would need to be added.

Bart

On Wed, Nov 19, 2025 at 3:09 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Wed, Nov 19, 2025 at 2:27 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> >
> > I think as Andy pointed out though the first 4 patches in your
> > chain do loosely want we want. Previously, we used the name to
> > point to the actual pinctrl driver, your patches should let us
> > do that properly through the fwnode. So we can drop the pinctrl
> > swnode and just have the cs-gpios bit point at the actual fwnode
> > instead. I am trying to hack together a strawman but its failing
> > in a lightly odd way. Hopefully I can get that sorted fairly
> > soon and post, or I guess I could post a version earlier if you
> > wanted a look in the knowledge it still doesn't work?
> >
>
> If your solution won't work after all, we can try using machine lookup
> instead. We have all the blocks except for support for the "undefined"
> GPIO in machine lookup. That would need to be added.
>

Just noting an idea: we could set the key field in the lookup entry to
"ERR_PTR(-ENOENT)" and use it to signal that we should return -ENOENT
from gpiod_find() if it matches the consumer device.

Bart

On Wed, Nov 19, 2025 at 2:27 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Wed, Nov 19, 2025 at 02:07:55PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Nov 19, 2025 at 1:53 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > On Wed, Nov 19, 2025 at 03:58:08AM -0800, Bartosz Golaszewski wrote:
> > > > On Wed, 19 Nov 2025 12:24:09 +0100, Charles Keepax
> > > > <ckeepax@opensource.cirrus.com> said:
> > > > > On Wed, Nov 19, 2025 at 12:06:57PM +0100, Bartosz Golaszewski wrote:
> > > > >> On Wed, Nov 19, 2025 at 11:58 AM Andy Shevchenko
> > > Can we tackle this the other way around? Since there is only a
> > > single fwnode for the device, can we find a way to get away with
> > > a single software node for the device too?
> >
> > I still don't understand what the software node that's already
> > assigned to the SPI device is though? device_add_software_node()
> > should work just fine if the only other firmware node the device has
> > is the ACPI device node.
>
> Its the software node we assigned to the first MFD cell, that one
> succeeds but attaches itself to the ACPI node as a secondary.
> When we get to the second cell we try to attach a new node but we
> get the one from the first cell since they share an ACPI node.
>

Ah, I see now. That's indeed a fundamental problem that can't be
easily solved. Andy is right about the needed change but phew that'll
be a big one...

> I think as Andy pointed out though the first 4 patches in your
> chain do loosely want we want. Previously, we used the name to
> point to the actual pinctrl driver, your patches should let us
> do that properly through the fwnode. So we can drop the pinctrl
> swnode and just have the cs-gpios bit point at the actual fwnode
> instead. I am trying to hack together a strawman but its failing
> in a lightly odd way. Hopefully I can get that sorted fairly
> soon and post, or I guess I could post a version earlier if you
> wanted a look in the knowledge it still doesn't work?
>

Yeah, I'd like that. I want to get it upstream and have interest in
getting it fixed ASAP.

For the record: I believe the logic behind this patch is the correct
approach. It uses the existing MFD infrastructure that's there for
exactly this reason. However without being able to have an arbitrary
number of firmware nodes attached to a device, that'll be impossible.

Just an idea: we could try to do the conversion gradually - by first
adding that list of fwnodes to struct device in parallel to the
current approach of having the secondary pointer and then go from
there step by step.

Bart

On Wed, Nov 19, 2025 at 3:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Wed, Nov 19, 2025 at 2:27 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Wed, Nov 19, 2025 at 02:07:55PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Nov 19, 2025 at 1:53 PM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:

...

> > > I still don't understand what the software node that's already
> > > assigned to the SPI device is though? device_add_software_node()
> > > should work just fine if the only other firmware node the device has
> > > is the ACPI device node.
> >
> > Its the software node we assigned to the first MFD cell, that one
> > succeeds but attaches itself to the ACPI node as a secondary.
> > When we get to the second cell we try to attach a new node but we
> > get the one from the first cell since they share an ACPI node.
>
> Ah, I see now. That's indeed a fundamental problem that can't be
> easily solved. Andy is right about the needed change but phew that'll
> be a big one...
>
> > I think as Andy pointed out though the first 4 patches in your
> > chain do loosely want we want. Previously, we used the name to
> > point to the actual pinctrl driver, your patches should let us
> > do that properly through the fwnode. So we can drop the pinctrl
> > swnode and just have the cs-gpios bit point at the actual fwnode
> > instead. I am trying to hack together a strawman but its failing
> > in a lightly odd way. Hopefully I can get that sorted fairly
> > soon and post, or I guess I could post a version earlier if you
> > wanted a look in the knowledge it still doesn't work?
>
> Yeah, I'd like that. I want to get it upstream and have interest in
> getting it fixed ASAP.

I also have a side interest that the reset-gpio becomes an agnostic driver.

> For the record: I believe the logic behind this patch is the correct
> approach. It uses the existing MFD infrastructure that's there for
> exactly this reason. However without being able to have an arbitrary
> number of firmware nodes attached to a device, that'll be impossible.
>
> Just an idea: we could try to do the conversion gradually - by first
> adding that list of fwnodes to struct device in parallel to the
> current approach of having the secondary pointer and then go from
> there step by step.

My idea was to mark the fwnode with __private and fix the (ab)users,
should not be so many. Can somebody mock up a coccinelle script to
find all dereferences of fwnode from struct device?

-- 
With Best Regards,
Andy Shevchenko

On Wed, Nov 19, 2025 at 3:11 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Nov 19, 2025 at 3:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Wed, Nov 19, 2025 at 2:27 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > On Wed, Nov 19, 2025 at 02:07:55PM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Nov 19, 2025 at 1:53 PM Charles Keepax
> > > > <ckeepax@opensource.cirrus.com> wrote:
>
> ...
>
> > > > I still don't understand what the software node that's already
> > > > assigned to the SPI device is though? device_add_software_node()
> > > > should work just fine if the only other firmware node the device has
> > > > is the ACPI device node.
> > >
> > > Its the software node we assigned to the first MFD cell, that one
> > > succeeds but attaches itself to the ACPI node as a secondary.
> > > When we get to the second cell we try to attach a new node but we
> > > get the one from the first cell since they share an ACPI node.
> >
> > Ah, I see now. That's indeed a fundamental problem that can't be
> > easily solved. Andy is right about the needed change but phew that'll
> > be a big one...
> >
> > > I think as Andy pointed out though the first 4 patches in your
> > > chain do loosely want we want. Previously, we used the name to
> > > point to the actual pinctrl driver, your patches should let us
> > > do that properly through the fwnode. So we can drop the pinctrl
> > > swnode and just have the cs-gpios bit point at the actual fwnode
> > > instead. I am trying to hack together a strawman but its failing
> > > in a lightly odd way. Hopefully I can get that sorted fairly
> > > soon and post, or I guess I could post a version earlier if you
> > > wanted a look in the knowledge it still doesn't work?
> >
> > Yeah, I'd like that. I want to get it upstream and have interest in
> > getting it fixed ASAP.
>
> I also have a side interest that the reset-gpio becomes an agnostic driver.
>
> > For the record: I believe the logic behind this patch is the correct
> > approach. It uses the existing MFD infrastructure that's there for
> > exactly this reason. However without being able to have an arbitrary
> > number of firmware nodes attached to a device, that'll be impossible.
> >
> > Just an idea: we could try to do the conversion gradually - by first
> > adding that list of fwnodes to struct device in parallel to the
> > current approach of having the secondary pointer and then go from
> > there step by step.
>
> My idea was to mark the fwnode with __private and fix the (ab)users,
> should not be so many. Can somebody mock up a coccinelle script to
> find all dereferences of fwnode from struct device?
>

I think you're underestimating the level of complexity. What about the
concept of dev_fwnode()? It literally makes no sense if we switch to a
list of fwnodes.

For it to make sense we'd have to have a kind of "dynamic" firmware
node attached to a device which we'd fill with an aggregation of all
properties from firmware nodes in the list.

Bart

On Wed, Nov 19, 2025 at 4:15 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Wed, Nov 19, 2025 at 3:11 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Nov 19, 2025 at 3:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Wed, Nov 19, 2025 at 2:27 PM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:
> > > > On Wed, Nov 19, 2025 at 02:07:55PM +0100, Bartosz Golaszewski wrote:
> > > > > On Wed, Nov 19, 2025 at 1:53 PM Charles Keepax
> > > > > <ckeepax@opensource.cirrus.com> wrote:

...

> > > > > I still don't understand what the software node that's already
> > > > > assigned to the SPI device is though? device_add_software_node()
> > > > > should work just fine if the only other firmware node the device has
> > > > > is the ACPI device node.
> > > >
> > > > Its the software node we assigned to the first MFD cell, that one
> > > > succeeds but attaches itself to the ACPI node as a secondary.
> > > > When we get to the second cell we try to attach a new node but we
> > > > get the one from the first cell since they share an ACPI node.
> > >
> > > Ah, I see now. That's indeed a fundamental problem that can't be
> > > easily solved. Andy is right about the needed change but phew that'll
> > > be a big one...
> > >
> > > > I think as Andy pointed out though the first 4 patches in your
> > > > chain do loosely want we want. Previously, we used the name to
> > > > point to the actual pinctrl driver, your patches should let us
> > > > do that properly through the fwnode. So we can drop the pinctrl
> > > > swnode and just have the cs-gpios bit point at the actual fwnode
> > > > instead. I am trying to hack together a strawman but its failing
> > > > in a lightly odd way. Hopefully I can get that sorted fairly
> > > > soon and post, or I guess I could post a version earlier if you
> > > > wanted a look in the knowledge it still doesn't work?
> > >
> > > Yeah, I'd like that. I want to get it upstream and have interest in
> > > getting it fixed ASAP.
> >
> > I also have a side interest that the reset-gpio becomes an agnostic driver.
> >
> > > For the record: I believe the logic behind this patch is the correct
> > > approach. It uses the existing MFD infrastructure that's there for
> > > exactly this reason. However without being able to have an arbitrary
> > > number of firmware nodes attached to a device, that'll be impossible.
> > >
> > > Just an idea: we could try to do the conversion gradually - by first
> > > adding that list of fwnodes to struct device in parallel to the
> > > current approach of having the secondary pointer and then go from
> > > there step by step.
> >
> > My idea was to mark the fwnode with __private and fix the (ab)users,
> > should not be so many. Can somebody mock up a coccinelle script to
> > find all dereferences of fwnode from struct device?
>
> I think you're underestimating the level of complexity. What about the
> concept of dev_fwnode()? It literally makes no sense if we switch to a
> list of fwnodes.

Why not? It will return the pointer to the primary node. You can look
for example how the list of the DMA descriptors is done in
drivers/dma/dw/core.c. Not the best solution, but gives you an idea of
how it may look.

> For it to make sense we'd have to have a kind of "dynamic" firmware
> node attached to a device which we'd fill with an aggregation of all
> properties from firmware nodes in the list.

"Dynamic" is just a node in the list. The only potential problem here
is prioritisation. Should we add to the head, tail or insert? But
converting current approach will be straightforward.

-- 
With Best Regards,
Andy Shevchenko

On Wed, Nov 19, 2025 at 3:24 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> > >
> > > My idea was to mark the fwnode with __private and fix the (ab)users,
> > > should not be so many. Can somebody mock up a coccinelle script to
> > > find all dereferences of fwnode from struct device?
> >
> > I think you're underestimating the level of complexity. What about the
> > concept of dev_fwnode()? It literally makes no sense if we switch to a
> > list of fwnodes.
>
> Why not? It will return the pointer to the primary node. You can look
> for example how the list of the DMA descriptors is done in
> drivers/dma/dw/core.c. Not the best solution, but gives you an idea of
> how it may look.

What even is a primary node though? You can have auxiliary devices
without an ACPI or OF node. Only with software nodes. Which one is the
primary? The first one we add?

>
> > For it to make sense we'd have to have a kind of "dynamic" firmware
> > node attached to a device which we'd fill with an aggregation of all
> > properties from firmware nodes in the list.
>
> "Dynamic" is just a node in the list. The only potential problem here
> is prioritisation. Should we add to the head, tail or insert? But
> converting current approach will be straightforward.
>

What I have in my mind is not another firmware node in the list in
struct device but rather a new firmware node implementation, that
would be assigned to the device via a dedicated pointer and would be
filled with a logical OR of properties from other firmware nodes added
to the list. Then dev_fwnode() would return this rather than any one
of the firmware nodes from the list. Think of it as the "master
fwnode" of a struct device.

Bart

On Wed, Nov 19, 2025 at 03:30:46PM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 19, 2025 at 3:24 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > > > My idea was to mark the fwnode with __private and fix the (ab)users,
> > > > should not be so many. Can somebody mock up a coccinelle script to
> > > > find all dereferences of fwnode from struct device?
> > >
> > > I think you're underestimating the level of complexity. What about the
> > > concept of dev_fwnode()? It literally makes no sense if we switch to a
> > > list of fwnodes.
> >
> > Why not? It will return the pointer to the primary node. You can look
> > for example how the list of the DMA descriptors is done in
> > drivers/dma/dw/core.c. Not the best solution, but gives you an idea of
> > how it may look.
> 
> What even is a primary node though? You can have auxiliary devices
> without an ACPI or OF node.

The one which gives the main set of the properties for the device.
The devices that don't have it, simply have a list of fwnodes empty.

> Only with software nodes. Which one is the
> primary? The first one we add?

Yes. The problem here might be if we add the SW node before the actual FW node
appear (it can be if we created some devices before the actual FW based
enumeration happens. It would probably need to have some kind of weight
(or priority value) and list should be sorted based on that number.

> > > For it to make sense we'd have to have a kind of "dynamic" firmware
> > > node attached to a device which we'd fill with an aggregation of all
> > > properties from firmware nodes in the list.
> >
> > "Dynamic" is just a node in the list. The only potential problem here
> > is prioritisation. Should we add to the head, tail or insert? But
> > converting current approach will be straightforward.
> 
> What I have in my mind is not another firmware node in the list in
> struct device but rather a new firmware node implementation, that
> would be assigned to the device via a dedicated pointer and would be
> filled with a logical OR of properties from other firmware nodes added
> to the list.

Oh no, this won't work in corner cases. What if we actually need to "fix"
an existing primary node (there were discussions long time ago about inverting
primary/secondary in some corner cases, but it didn't appear so far as
it most likely will give tons of issues in the _current_ design)?

> Then dev_fwnode() would return this rather than any one
> of the firmware nodes from the list. Think of it as the "master
> fwnode" of a struct device.

fwnode should not be in any relations to the device, I mean when we do fwnodes,
we should not assume that it's backing the device. In the idea you shared it
won't be possible ("dedicated pointer") in mine it is (just a list of something
that may belong to the device, or to another object, doesn't matter).


-- 
With Best Regards,
Andy Shevchenko

On Wed, Nov 19, 2025 at 9:38 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Nov 19, 2025 at 03:30:46PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Nov 19, 2025 at 3:24 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > > > My idea was to mark the fwnode with __private and fix the (ab)users,
> > > > > should not be so many. Can somebody mock up a coccinelle script to
> > > > > find all dereferences of fwnode from struct device?
> > > >
> > > > I think you're underestimating the level of complexity. What about the
> > > > concept of dev_fwnode()? It literally makes no sense if we switch to a
> > > > list of fwnodes.
> > >
> > > Why not? It will return the pointer to the primary node. You can look
> > > for example how the list of the DMA descriptors is done in
> > > drivers/dma/dw/core.c. Not the best solution, but gives you an idea of
> > > how it may look.
> >
> > What even is a primary node though? You can have auxiliary devices
> > without an ACPI or OF node.
>
> The one which gives the main set of the properties for the device.
> The devices that don't have it, simply have a list of fwnodes empty.
>

Well, what *is* the *main* set of properties? Who declares that? I
agree, it's easy for device-tree - the OF node is the main one, but
then what if a sub-device inherits the OF node of the parent while we
also add a software node? It's not that straightforward and we'll run
into issues for sure just like what we're seeing now with secondary
fwnodes.

> > Only with software nodes. Which one is the
> > primary? The first one we add?
>
> Yes. The problem here might be if we add the SW node before the actual FW node
> appear (it can be if we created some devices before the actual FW based
> enumeration happens. It would probably need to have some kind of weight
> (or priority value) and list should be sorted based on that number.
>

Ugh, please no. Firmware node priority?? With magic numbers?

> > > > For it to make sense we'd have to have a kind of "dynamic" firmware
> > > > node attached to a device which we'd fill with an aggregation of all
> > > > properties from firmware nodes in the list.
> > >
> > > "Dynamic" is just a node in the list. The only potential problem here
> > > is prioritisation. Should we add to the head, tail or insert? But
> > > converting current approach will be straightforward.
> >
> > What I have in my mind is not another firmware node in the list in
> > struct device but rather a new firmware node implementation, that
> > would be assigned to the device via a dedicated pointer and would be
> > filled with a logical OR of properties from other firmware nodes added
> > to the list.
>
> Oh no, this won't work in corner cases. What if we actually need to "fix"
> an existing primary node (there were discussions long time ago about inverting
> primary/secondary in some corner cases, but it didn't appear so far as
> it most likely will give tons of issues in the _current_ design)?
>

What do you mean by "fix"? Like repair? Or fix in place? I'm not following.

> > Then dev_fwnode() would return this rather than any one
> > of the firmware nodes from the list. Think of it as the "master
> > fwnode" of a struct device.
>
> fwnode should not be in any relations to the device, I mean when we do fwnodes,
> we should not assume that it's backing the device. In the idea you shared it
> won't be possible ("dedicated pointer") in mine it is (just a list of something
> that may belong to the device, or to another object, doesn't matter).
>

That's not true, we expect fwnodes to back real devices all the time.
Look at all the find_device_by_fwnode() functions we have everywhere.

The crux of the problem Charles identified is the fact that the
secondary fwnode is a field of struct fwnode_handle and not of a
struct device. This really doesn't make sense as we see where multiple
devices use a single "real" fwnode but want to have different
secondary software nodes.

Moving the secondary fwnode to struct device would already help a lot
but if we're doing that then we may as well switch to a list of
fwnodes.

Bart

On Thu, Nov 20, 2025 at 10:12:39AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 19, 2025 at 9:38 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Nov 19, 2025 at 03:30:46PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Nov 19, 2025 at 3:24 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:

...

> > > > > > My idea was to mark the fwnode with __private and fix the (ab)users,
> > > > > > should not be so many. Can somebody mock up a coccinelle script to
> > > > > > find all dereferences of fwnode from struct device?
> > > > >
> > > > > I think you're underestimating the level of complexity. What about the
> > > > > concept of dev_fwnode()? It literally makes no sense if we switch to a
> > > > > list of fwnodes.
> > > >
> > > > Why not? It will return the pointer to the primary node. You can look
> > > > for example how the list of the DMA descriptors is done in
> > > > drivers/dma/dw/core.c. Not the best solution, but gives you an idea of
> > > > how it may look.
> > >
> > > What even is a primary node though? You can have auxiliary devices
> > > without an ACPI or OF node.
> >
> > The one which gives the main set of the properties for the device.
> > The devices that don't have it, simply have a list of fwnodes empty.
> 
> Well, what *is* the *main* set of properties? Who declares that? I
> agree, it's easy for device-tree - the OF node is the main one, but
> then what if a sub-device inherits the OF node of the parent while we
> also add a software node? It's not that straightforward and we'll run
> into issues for sure just like what we're seeing now with secondary
> fwnodes.

First rule is that swnode NEVER is the "main" property provider.
But it could be a single property provider.
The rest becomes easier after this.

> > > Only with software nodes. Which one is the
> > > primary? The first one we add?
> >
> > Yes. The problem here might be if we add the SW node before the actual FW node
> > appear (it can be if we created some devices before the actual FW based
> > enumeration happens. It would probably need to have some kind of weight
> > (or priority value) and list should be sorted based on that number.
> 
> Ugh, please no. Firmware node priority?? With magic numbers?

Why not? Many tools and things rely on weights.
But again, first problem first, the design. The additional property bundles
(as swnodes or OF overlays on ACPI-based systems) can come later on.

> > > > > For it to make sense we'd have to have a kind of "dynamic" firmware
> > > > > node attached to a device which we'd fill with an aggregation of all
> > > > > properties from firmware nodes in the list.
> > > >
> > > > "Dynamic" is just a node in the list. The only potential problem here
> > > > is prioritisation. Should we add to the head, tail or insert? But
> > > > converting current approach will be straightforward.
> > >
> > > What I have in my mind is not another firmware node in the list in
> > > struct device but rather a new firmware node implementation, that
> > > would be assigned to the device via a dedicated pointer and would be
> > > filled with a logical OR of properties from other firmware nodes added
> > > to the list.
> >
> > Oh no, this won't work in corner cases. What if we actually need to "fix"
> > an existing primary node (there were discussions long time ago about inverting
> > primary/secondary in some corner cases, but it didn't appear so far as
> > it most likely will give tons of issues in the _current_ design)?
> 
> What do you mean by "fix"? Like repair? Or fix in place? I'm not following.

When the primary already has a property that needs to be rewritten
(same name, different value). Note, that there are use cases already
for this AFAIR, but probably they were worked around in not too hackish
way or abandoned for now.

> > > Then dev_fwnode() would return this rather than any one
> > > of the firmware nodes from the list. Think of it as the "master
> > > fwnode" of a struct device.
> >
> > fwnode should not be in any relations to the device, I mean when we do fwnodes,
> > we should not assume that it's backing the device. In the idea you shared it
> > won't be possible ("dedicated pointer") in mine it is (just a list of something
> > that may belong to the device, or to another object, doesn't matter).
> 
> That's not true, we expect fwnodes to back real devices all the time.

No. Even OF doesn't do that. OF node can be a separate thing without struct
device being associated with it.

> Look at all the find_device_by_fwnode() functions we have everywhere.

What do you mean, please?

$ git grep -n -w find_device_by_fwnode | wc -l
0

Even if you refer to *_find_device_by_fwnode(), still it's not everywhere,
just in a dozen of modules.

> The crux of the problem Charles identified is the fact that the
> secondary fwnode is a field of struct fwnode_handle and not of a
> struct device. This really doesn't make sense as we see where multiple
> devices use a single "real" fwnode but want to have different
> secondary software nodes.
> 
> Moving the secondary fwnode to struct device would already help a lot
> but if we're doing that then we may as well switch to a list of
> fwnodes.

They all should be an independent entity that can be part of the struct device
but it's not obligatory relation ship.

Again, device may have a backing fwnode, while fwnode might not have a struct
device consumer.

-- 
With Best Regards,
Andy Shevchenko

On Thu, Nov 20, 2025 at 10:56 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> > Look at all the find_device_by_fwnode() functions we have everywhere.
>
> What do you mean, please?
>
> $ git grep -n -w find_device_by_fwnode | wc -l
> 0
>

I made up a placeholder name for all different variants of functions
finding devices of different types by fwnode.

> Even if you refer to *_find_device_by_fwnode(), still it's not everywhere,
> just in a dozen of modules.
>
> > The crux of the problem Charles identified is the fact that the
> > secondary fwnode is a field of struct fwnode_handle and not of a
> > struct device. This really doesn't make sense as we see where multiple
> > devices use a single "real" fwnode but want to have different
> > secondary software nodes.
> >
> > Moving the secondary fwnode to struct device would already help a lot
> > but if we're doing that then we may as well switch to a list of
> > fwnodes.
>
> They all should be an independent entity that can be part of the struct device
> but it's not obligatory relation ship.
>
> Again, device may have a backing fwnode, while fwnode might not have a struct
> device consumer.
>

I never said it must have one. Just that it's common and we do use this fact.

Bart

On Wed, Nov 19, 2025 at 11:24:09AM +0000, Charles Keepax wrote:
> On Wed, Nov 19, 2025 at 12:06:57PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Nov 19, 2025 at 11:58 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Nov 19, 2025 at 11:50:09AM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Nov 19, 2025 at 11:47 AM Charles Keepax
> > > > <ckeepax@opensource.cirrus.com> wrote:
> > > > > On Wed, Nov 19, 2025 at 11:38:35AM +0100, Bartosz Golaszewski wrote:

...

> > > > > Cool, thanks for all your help here. Might take me a couple hours
> > > > > but I will get to the bottom of the EBUSY thing, and report back.
> > > >
> > > > Sure. Could you just post the stack trace down to where the EBUSY is
> > > > returned in drivers/base/swnode.c? I'd like to analyze the logic
> > > > myself too if that's not too much work for you.
> > >
> > > FWIW, there is also an interesting debug technique. Put in your driver where
> > > you get this error code something like this (after all #include directives):
> > >
> > > #undef EBUSY
> > > #define EBUSY __LINE__
> > >
> > > And then you get some error code which is line number in some C file. You can
> > > narrow down grepping of the EBUSY in the suspected files and get the one.
> > >
> > >         git grep -n -w EBUSY -- ...files of interest...
> > >
> > > Hope this saves some minutes for you.
> > 
> > Charles confirmed he gets it from device_add_software_node(). I want
> > to know how we're getting there and how we could end up already having
> > a software node.
> 
> Ok I think I see what is happening now, the swnode is created on
> the first cell (the pinctrl). Then it moves onto the second cell,
> but mfd_acpi_add_device() copies the firmware node into both
> devices, the device_set_node() call at the bottom. So it inherits
> the swnode node through that primary fwnode.

Okay, you got into fundamental problem of fwnode design it seems.

(below is the summary related to that, but it may be not related here)

If it's the case, there is no easy solution for it right now.

And going ahead, please, don't even try hacks like recreating a copy
of needed properties from the parent fwnode to get an independent child
fwnode.

The proper solution OTOH should be decoupling fwnode from struct device
and making there a list of fwnodes.

	struct fwnode_handle {
		struct list_head node;
		...
	}

	struct device {
		// asumming dropped of_node and fw_node
		struct list_head nodes;
	}

This will require at first to make sure no code dereferencing fwnode
(and of_node) from struct device. With that enormous task done, the
rest is much easier to achieve as it will be just API's internals
refactoring.

With that done, we may stitch as many fwnodes as we want where the order in
the list will define match priority.

> I am guessing this code has perhaps been more heavily tested on
> device tree where it is more common to have nodes for each cell,
> whereas ACPI is far more likely to have a single firmware node for
> the whole device.

-- 
With Best Regards,
Andy Shevchenko

On Wed, Nov 19, 2025 at 12:53 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Okay, you got into fundamental problem of fwnode design it seems.
>

The problem here is not that we can have a single secondary firmware
node, it's that we can only have a single software node. But that may
not be a problem because the question here is: do we in this case
really need the software node assigned by the ACPI part of the MFD
core. I'd say no, see my other response for a proposed fix.

> (below is the summary related to that, but it may be not related here)
>
> If it's the case, there is no easy solution for it right now.
>
> And going ahead, please, don't even try hacks like recreating a copy
> of needed properties from the parent fwnode to get an independent child
> fwnode.
>

Are any of these properties needed/used? If so, then maybe we should
consider allowing the primary and secondary nodes to be software
nodes?

> The proper solution OTOH should be decoupling fwnode from struct device
> and making there a list of fwnodes.
>
>         struct fwnode_handle {
>                 struct list_head node;
>                 ...
>         }
>
>         struct device {
>                 // asumming dropped of_node and fw_node
>                 struct list_head nodes;
>         }
>
> This will require at first to make sure no code dereferencing fwnode
> (and of_node) from struct device. With that enormous task done, the
> rest is much easier to achieve as it will be just API's internals
> refactoring.
>

Yeah, let's talk realistic things that we can improve now and not a
task that will take years. :)

> With that done, we may stitch as many fwnodes as we want where the order in
> the list will define match priority.
>

FWIW we can already do it without changing the fwnode_handle
structure. The secondary fwnode also has the secondary pointer so it's
effectively a linked list already.

> > I am guessing this code has perhaps been more heavily tested on
> > device tree where it is more common to have nodes for each cell,
> > whereas ACPI is far more likely to have a single firmware node for
> > the whole device.
>

Bart

On Wed, Nov 19, 2025 at 10:58 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Wed, Nov 19, 2025 at 10:40:30AM +0100, Bartosz Golaszewski wrote:
> > On Wed, Nov 19, 2025 at 10:31 AM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > >
> > > On Wed, Nov 19, 2025 at 10:10:24AM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Currently the SPI driver sets up a software node rerefencing the GPIO
> > > > controller exposing the chip-select GPIO but this node never gets
> > > > attached to the actual GPIO provider. The lookup uses the weird way GPIO
> > > > core performs the software node lookup by the swnode's name. We want to
> > > > switch to a true firmware node lookup so the actual link must exist.
> > > > Move the configuration to the MFD core and connect the SPI and pinctrl
> > > > sub-devices with software node references.
> > > >
> > > > Fixes: 439fbc97502a ("spi: cs42l43: Add bridged cs35l56 amplifiers")
> > > > Reported-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > > > Closes: https://lore.kernel.org/all/aRyf7qDdHKABppP8@opensource.cirrus.com/
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > ---
> > > > Hi Charles!
> > > >
> > > > Please give this a try. It's only build-tested so far. I hope I
> > > > understood correctly that it's the SPI driver that needs the "cs" GPIO
> > > > from the pinctrl.
> > >
> > > Unfortunately it fails with an -EBUSY on adding the MFD children.
> > > I will investigate a little more and report back.
> >
> > Does it fail in device_add_software_node() in MFD core? Is it possible
> > that the device already has a software node for some reason?
>
> Yeah it seems there is already a software node, although somewhat
> at a loss as to why, the only ones we add are inside the SPI
> driver. I will poke further and see if I can find out how it
> acquires that node.
>
> > > > +static const struct software_node cs42l43_gpiochip_swnode = {
> > > > +     .name = "cs42l43-pinctrl",
> > > > +};
> > > > +
> > > > +static const struct property_entry cs42l43_cs_props[] = {
> > > > +     PROPERTY_ENTRY_GPIO("cs-gpios", &cs42l43_gpiochip_swnode, 0, GPIO_ACTIVE_LOW),
> > > > +     { }
> > > > +};
> > >
> > > This drops the undefined node, that is important as it lets the
> > > SPI core know that device is using an internal chip select rather
> > > than a GPIO.
> >
> > I really dislike the whole concept of this undefined software node.
> > This driver is literally the only user and I'd love to just get rid of
> > it. HOW exactly does it let the driver know to use internal chip
> > select? Can we do it differently?
>
> The SPI code lives in spi_get_gpio_descs(). The basic gist is
> it will use a native chip select if the gpiod_get_index return
> NULL. The system came from device tree (see Documentation
> spi-controller.yaml) initially, then I added an analogue to
> swnodes a while back in 9d50f95bc0d5 ("gpio: swnode: Add
> ability to specify native chip selects for SPI").  I have no
> great attachment to this way of doing it, however, it does
> seem logical to me that the system is at least fairly similar
> between the different firmware types.
>

I see. Then maybe we should at least generalize it to a high-level
"swnode-undefined", move it out of GPIOLIB and provide a proper swnode
interface "swnode_is_undefined()" so that we don't have to open-code
the string comparison like that. That's not related to this patch of
course, just throwing out an idea.

Bart

On Wed, Nov 19, 2025 at 11:31 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Wed, Nov 19, 2025 at 10:10:24AM +0100, Bartosz Golaszewski wrote:

...

> Although maybe that real node would take preference over the
> swnode?

The current design of fwnode assumes primary (usually from real FW)
and secondary (usually the SW node) and the code looks first for the
property in the primary node. Hence, if that will give you the match,
the secondary will be ignored.


-- 
With Best Regards,
Andy Shevchenko