[PATCH] spi: cs42l43: Workaround for wrong speaker ID on Dell XPS 13 DX13260

[PATCH] spi: cs42l43: Workaround for wrong speaker ID on Dell XPS 13 DX13260

[Richard Fitzgerald]

Create an acpi_gpio_mapping so that speaker ID GPIOs can be read directly
from the ACPI GpioIo() instead of relying on the spk-id-gpios property to
have the correct number of pins. Change the reading loop to read each pin
one at a time until it reaches a pin index that doesn't exist.

This fixes problems on Dell XPS 13 DX13260:

- No speaker audio
- The wrong firmware was loaded so the speaker protection did not match
  the speaker characteristics.

The Dell XPS 13 DX13260 has two speaker ID GPIOs, to form a 2-bit ID. The
ACPI GpioIo() has both pins but the Linux-specific spk-id-gpios property
only has a mapping to the first pin. This meant that the speaker ID was
wrong in most cases, and that would lead to the codec driver loading the
wrong amp firmware, or not finding a firmware (as 0 is not valid on this
laptop).

Instead of quirking this specific system, the code can be rewritten so
that it will always use the direct GpioIo() mapping. This works on all
ACPI systems, so avoids having to keep adding quirks for other systems
that have the same ACPI error.

Assisted-by: Codex:gpt-5.6-sol
Reported-by: Wiza Jalakasi <wjalakasi@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221956
Tested-by: Wiza Jalakasi <wjalakasi@gmail.com>
Fixes: f3c605147741e ("spi: cs42l43: Add GPIO speaker id support to the bridge configuration")
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 drivers/spi/spi-cs42l43.c | 131 ++++++++++++++++++++++++++++++++------
 1 file changed, 113 insertions(+), 18 deletions(-)

diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c
index 7106a8a4f2805..ee227205010f7 100644
--- a/drivers/spi/spi-cs42l43.c
+++ b/drivers/spi/spi-cs42l43.c
@@ -19,6 +19,7 @@
 #include <linux/mfd/cs42l43-regs.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/overflow.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
@@ -29,6 +30,7 @@
 #define CS42L43_FIFO_SIZE		16
 #define CS42L43_SPI_ROOT_HZ		49152000
 #define CS42L43_SPI_MAX_LENGTH		65532
+#define CS42L43_MAX_SPK_ID_GPIOS	8
 
 enum cs42l43_spi_cmd {
 	CS42L43_WRITE,
@@ -41,6 +43,12 @@ struct cs42l43_spi {
 	struct spi_controller *ctlr;
 };
 
+struct cs42l43_spk_id_gpio_mapping {
+	struct fwnode_handle *fwnode;
+	struct acpi_gpio_params params[CS42L43_MAX_SPK_ID_GPIOS];
+	struct acpi_gpio_mapping mappings[2];
+};
+
 static const unsigned int cs42l43_clock_divs[] = {
 	2, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30
 };
@@ -215,31 +223,118 @@ static size_t cs42l43_spi_max_length(struct spi_device *spi)
 	return CS42L43_SPI_MAX_LENGTH;
 }
 
-static int cs42l43_get_speaker_id_gpios(struct cs42l43_spi *priv, int *result)
+static struct cs42l43_spk_id_gpio_mapping *
+cs42l43_add_speaker_id_gpio_mapping(struct cs42l43_spi *priv)
 {
-	struct gpio_descs *descs;
-	u32 spkid;
+	struct fwnode_reference_args args;
+	struct acpi_device *adev;
 	int i, ret;
 
-	descs = gpiod_get_array_optional(priv->dev, "spk-id", GPIOD_IN);
-	if (!descs)
-		return 0;
-	else if (IS_ERR(descs))
-		return PTR_ERR(descs);
-
-	spkid = 0;
-	for (i = 0; i < descs->ndescs; i++) {
-		ret = gpiod_get_value_cansleep(descs->desc[i]);
+	struct cs42l43_spk_id_gpio_mapping *mapping __free(kfree) = kzalloc_obj(*mapping,
+										GFP_KERNEL);
+
+	if (!mapping)
+		return ERR_PTR(-ENOMEM);
+
+	ret = fwnode_property_get_reference_args(dev_fwnode(priv->dev), "spk-id-gpios",
+						 NULL, 3, 0, &args);
+	if (ret)
+		return ERR_PTR(ret);
+
+	struct fwnode_handle *fwnode __free(fwnode_handle) = args.fwnode;
+
+	if (args.nargs < 3)
+		return ERR_PTR(-EINVAL);
+
+	if (overflows_type(args.args[0], mapping->params[0].crs_entry_index))
+		return ERR_PTR(-EOVERFLOW);
+
+	adev = to_acpi_device_node(fwnode);
+	if (!adev)
+		return ERR_PTR(-EINVAL);
+
+	for (i = 0; i < ARRAY_SIZE(mapping->params); i++) {
+		mapping->params[i].crs_entry_index = args.args[0];
+		mapping->params[i].line_index = i;
+		mapping->params[i].active_low = !!args.args[2];
+	}
+
+	mapping->mappings[0] = (struct acpi_gpio_mapping) {
+		.name = "spk-id-gpios",
+		.data = mapping->params,
+		.size = ARRAY_SIZE(mapping->params),
+	};
+
+	ret = acpi_dev_add_driver_gpios(adev, mapping->mappings);
+	if (ret)
+		return ERR_PTR(ret);
+
+	mapping->fwnode = no_free_ptr(fwnode);
+
+	return no_free_ptr(mapping);
+}
+
+static void cs42l43_remove_speaker_id_gpio_mapping(struct cs42l43_spk_id_gpio_mapping *mapping)
+{
+	if (!mapping)
+		return;
+
+	if (mapping->fwnode) {
+		acpi_dev_remove_driver_gpios(to_acpi_device_node(mapping->fwnode));
+		fwnode_handle_put(mapping->fwnode);
+	}
+
+	kfree(mapping);
+}
+
+static int cs42l43_get_speaker_id_gpios(struct cs42l43_spi *priv, int *result)
+{
+	struct fwnode_handle *gpio_fwnode = dev_fwnode(priv->dev);
+	struct cs42l43_spk_id_gpio_mapping *mapping = NULL;
+	struct gpio_desc *desc;
+	u32 spkid = 0;
+	int i, ret = 0;
+
+	if (is_acpi_node(gpio_fwnode)) {
+		mapping = cs42l43_add_speaker_id_gpio_mapping(priv);
+		if (IS_ERR(mapping)) {
+			ret = PTR_ERR(mapping);
+			if (ret == -ENOENT)
+				return 0;
+
+			return ret;
+		}
+
+		gpio_fwnode = mapping->fwnode;
+	}
+
+	for (i = 0; i < CS42L43_MAX_SPK_ID_GPIOS; i++) {
+		desc = fwnode_gpiod_get_index(gpio_fwnode, "spk-id", i, GPIOD_IN,
+					      dev_name(priv->dev));
+		if (IS_ERR(desc)) {
+			ret = PTR_ERR(desc);
+			if (ret == -ENOENT)
+				break;
+
+			goto out;
+		}
+
+		ret = gpiod_get_value_cansleep(desc);
+		gpiod_put(desc);
 		if (ret < 0)
-			goto err;
+			goto out;
 
-		spkid |= (ret << i);
+		spkid |= (u32)ret << i;
 	}
 
-	dev_dbg(priv->dev, "spk-id-gpios = %d\n", spkid);
-	*result = spkid;
-err:
-	gpiod_put_array(descs);
+	if (i) {
+		dev_dbg(priv->dev, "spk-id-gpios = %u from %d GPIOs\n", spkid, i);
+		*result = spkid;
+	}
+
+	ret = 0;
+out:
+	cs42l43_remove_speaker_id_gpio_mapping(mapping);
 
 	return ret;
 }
-- 
2.47.3

Re: [PATCH] spi: cs42l43: Workaround for wrong speaker ID on Dell XPS 13 DX13260

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

On Sat, Sep 19, 2026 at 02:47:30PM +0100, Richard Fitzgerald wrote:
> Create an acpi_gpio_mapping so that speaker ID GPIOs can be read directly
> from the ACPI GpioIo() instead of relying on the spk-id-gpios property to
> have the correct number of pins. Change the reading loop to read each pin
> one at a time until it reaches a pin index that doesn't exist.

...

> Instead of quirking this specific system, the code can be rewritten so
> that it will always use the direct GpioIo() mapping. This works on all
> ACPI systems, so avoids having to keep adding quirks for other systems
> that have the same ACPI error.

Is there an overlap with cs35l56_try_get_broken_sdca_spkid_gpio() here?
Didn't check thoroughly, and if they are doing the same thing it's
probably reasonable to factor out incrementally.

> +       struct cs42l43_spk_id_gpio_mapping *mapping __free(kfree) = kzalloc_obj(*mapping,

kzalloc_obj() needs slab.h (which will be implicitly included anyway but
still).

> +static int cs42l43_get_speaker_id_gpios(struct cs42l43_spi *priv, int *result)
> +{
> +	struct fwnode_handle *gpio_fwnode = dev_fwnode(priv->dev);
> +	struct cs42l43_spk_id_gpio_mapping *mapping = NULL;
> +	struct gpio_desc *desc;
> +	u32 spkid = 0;
> +	int i, ret = 0;
> +
> +	if (is_acpi_node(gpio_fwnode)) {
> +		mapping = cs42l43_add_speaker_id_gpio_mapping(priv);
> +		if (IS_ERR(mapping)) {
> +			ret = PTR_ERR(mapping);
> +			if (ret == -ENOENT)
> +				return 0;
> +
> +			return ret;
> +		}

Could we see an -EBUSY if there's something else using another GPIO from
the same provider?  I'm a bit fuzzy on the ACPI mappings so possibly
that can't happen.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: cs42l43: Workaround for wrong speaker ID on Dell XPS 13 DX13260

[Richard Fitzgerald]

On 20/9/26 16:14, Mark Brown wrote:
> On Sat, Sep 19, 2026 at 02:47:30PM +0100, Richard Fitzgerald wrote:
>> Create an acpi_gpio_mapping so that speaker ID GPIOs can be read directly
>> from the ACPI GpioIo() instead of relying on the spk-id-gpios property to
...
> 
> Is there an overlap with cs35l56_try_get_broken_sdca_spkid_gpio() here?
> Didn't check thoroughly, and if they are doing the same thing it's
> probably reasonable to factor out incrementally.
Only slightly. That is fixing a completely different problem that 
affects a couple of old Dell models. The normal non-broken speaker ID in
the cs35l56 driver will need the same change. As this is a workaround I
didn't want to complicate backporting by entangling it with the codec
driver just to avoid code duplication. I'll worry about that later.

> kzalloc_obj() needs slab.h (which will be implicitly included anyway but
> still).
Acked

> Could we see an -EBUSY if there's something else using another GPIO from
> the same provider?

I'm not sure what you mean by "another GPIO from the same provider".
If you mean can the codec driver also try to read the same GPIOs:

1. No. It's read here in the SPI bus driver because on these systems the
cs42l43 is the only device that appears in ACPI, so it's the only one
that has access to the ACPI GpioIo(). And there's only one instance.

2. The code only creates the mapping temporarily while it reads the GPIO
state and then removes the mapping

3. If it could return -EBUSY it could have returned that with the _DSD
spk-id-gpios property and the original code would have returned an
error.

 > I'm a bit fuzzy on the ACPI mappings so possibly
 > that can't happen.

Linux used to have a function to read the ACPI GpioIo() definitions,
same way Windows does. That was removed a few years ago so that GPIOs
now must use the DT-style "something-gpios" way, and an ACPI _DSD
property provides the mapping from that property to the GpioIo().
Of course, that _DSD property is Linux-specific so won't be in any ACPI
written for Windows, so the acpi_gpio_mapping had to be added to provide
a way to read the GpioIo(), but in a more complex way than before.
It's just a mapping from Linux-specific named GPIOs to the actual ACPI
GPIO definition. So we've gone in a circle removing the ACPI-specific
API and then adding a ACPI-specific API, but I assume something was
fixed by doing it the new wat.

The ACPI property takes precedence and overrides the mapping, but the
specific case of the cs42l43 SPI driver we know that the node containing
the defective _DSD property is always different from the node containing
the GpioIo() definition.

On the Sashiko complaints:
1. Code doesn't handle the singular form "spk-id-gpio". The script that
generates this ACPI is hardcoded to the plural form, so this isn't a
issue I'll do a change for that anyway just for completeness, but I'd
prefer to do that as a separate patch so I can keep the "tested-by" on
this patch. (I'll do a spin to add it to this patch if you prefer.)

2. "Does this ignore the ACPI pin offset". Yes, that's precisely what
the patch is intended to do.

3. The stuff about overwriting a GPIO provider's node. Sashiko seems to
be assuming that the spk-id-gpios is pointing at the GPIO driver node,
which it would be on DT. On ACPI it's just pointing at our node that
contains the ACPI GpioIo() that points to the GPIO driver node. adev
is the adev of our child node.

4. The "use-after-free" seems be the same misunderstanding, it thinks
we're changing the node of the GPIO provider driver. We're not, we're
adding the mapping to our child node and only the cs42l43 driver uses
that.

Re: [PATCH] spi: cs42l43: Workaround for wrong speaker ID on Dell XPS 13 DX13260

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

On Fri, Sep 25, 2026 at 02:14:34PM +0100, Richard Fitzgerald wrote:
> On 20/9/26 16:14, Mark Brown wrote:

> > Could we see an -EBUSY if there's something else using another GPIO from
> > the same provider?

> I'm not sure what you mean by "another GPIO from the same provider".
> If you mean can the codec driver also try to read the same GPIOs:

I mean we're looping over all the GPIOs from the provider, what if
something else already requested one of the others?

> 1. No. It's read here in the SPI bus driver because on these systems the
> cs42l43 is the only device that appears in ACPI, so it's the only one
> that has access to the ACPI GpioIo(). And there's only one instance.

So this is all hyper specific to some specific systems?  I can't see
what in the code is limiting the workaround to those systems, but it's
possible that's in wider context than I looked at.

> The ACPI property takes precedence and overrides the mapping, but the
> specific case of the cs42l43 SPI driver we know that the node containing
> the defective _DSD property is always different from the node containing
> the GpioIo() definition.
> 
> On the Sashiko complaints:

If I care about Sashiko output I'll say something.  There's no harm in
looking at it and it's possible that some of the stuff I don't mention
is valid since the false positive rate is such that I can't generally be
bothered to figure out if something that's not immediately obviously
valid is actually valid.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: cs42l43: Workaround for wrong speaker ID on Dell XPS 13 DX13260

[Richard Fitzgerald]

On 25/9/26 15:51, Mark Brown wrote:
> On Fri, Sep 25, 2026 at 02:14:34PM +0100, Richard Fitzgerald wrote:
>> On 20/9/26 16:14, Mark Brown wrote:
> 
>>> Could we see an -EBUSY if there's something else using another GPIO from
>>> the same provider?
> 
>> I'm not sure what you mean by "another GPIO from the same provider".
>> If you mean can the codec driver also try to read the same GPIOs:
> 
> I mean we're looping over all the GPIOs from the provider, what if
> something else already requested one of the others?

This is only done for ACPI. We're looping over all the pins in the
consumer's GPIO request.

In DT the "x-gpios" property points at the GPIO provider.

In ACPI the "x-gpios" property is a Linux-specific compatibility fudge
that points to the consumer's *real* ACPI-syntax GPIO request.

Take a look at Documentation/firmware-guide/acpi/gpio-properties.rst

GpioIo() is the ACPI way to request GPIOs. The numbers inside the brace
list are the pin numbers being requested from the provider. The loop
is walking through that list trying to get each requested pin.

Re: [PATCH] spi: cs42l43: Workaround for wrong speaker ID on Dell XPS 13 DX13260

[Richard Fitzgerald]

On 19/9/26 14:47, Richard Fitzgerald wrote:
> Create an acpi_gpio_mapping so that speaker ID GPIOs can be read directly
> from the ACPI GpioIo() instead of relying on the spk-id-gpios property to
> have the correct number of pins. Change the reading loop to read each pin
> one at a time until it reaches a pin index that doesn't exist.
> 
> This fixes problems on Dell XPS 13 DX13260:
Don't merge this.

I've heard there are plans to change the way speaker ID GPIOs are
defined in ACPI for future products. It will make them more compatible
with the "Linux way" and will work with the existing code in the cs42l43
and cs35l56 drivers. But would no longer be the "Windows way" so won't
work with the Windows-style method in this patch.

Which means this Dell model will likely be the only one with speaker ID
defined this particular way so will need to be a model-specific quirk.