Re: [PATCH v2 2/3] platform/x86: serial-multi-instantiate: Fix SPI chip select on platforms with incomplete ACPI cs-gpios

[Andy Shevchenko]

On Mon, Apr 13, 2026 at 6:05 PM Hans de Goede <hansg@xxxxxxxxxx> wrote:
> On 13-Apr-26 2:51 PM, Richard Fitzgerald wrote:
> > On 13/04/2026 12:43 pm, Hans de Goede wrote:
> >> On 13-Apr-26 12:05 PM, Khalil wrote:
> >>> Some HP laptops with Intel Lunar Lake and dual Cirrus Logic CS35L56
> >>> amplifiers over SPI have an incomplete cs-gpios property in the SPI
> >>> controller's _DSD - it only declares the first chip select. The
> >>> remaining chip select GPIOs are defined as GpioIo resources in the
> >>> peripheral's ACPI node but are not referenced by the controller.
> >>>
> >>> This causes the SPI framework to reject devices whose chip select
> >>> exceeds num_chipselect with -EINVAL, preventing the second amplifier
> >>> from probing.
> >>>
> >>> Fix this on known affected platforms by:

If I understood the problem correctly, this should be fixed in SPI
core. And I tried already a few years ago to push the patch for that
[1]. I have locally an updated version which I need to revisit again
and probably submit.

<SNIP>

> >>> +/*
> >>> + * ACPI GPIO mapping for the chip select GpioIo resource.
> >>> + * Maps "cs-gpios" to the GpioIo resource at index 0 of the ACPI _CRS.
> >>> + */
> >>> +static const struct acpi_gpio_params smi_cs_gpio_params = { 0, 0, false };
> >>> +
> >>> +static const struct acpi_gpio_mapping smi_cs_gpio_mapping[] = {
> >>> +    { "cs-gpios", &smi_cs_gpio_params, 1 },
> >>> +    { }
> >>>   };
> >>
> >> I'm confused here you say that the chip-select for the second amplifier
> >> is missing, but I would expect that to be the GpioIo resource at index 1
> >> not 0?
> >>
> >> I would expect the GpioIo resource at index 0 to be for the first
> >> amplifier which does have a GPIO already in the _DSD cs-gpios property ?
> >>
> >> Since this is a multi-serial device ACPI node, there is only one
> >> set of resources for both amplifiers (with their also being 2 SPI
> >> resources in the _CRS list).
> >>
> >> So I would expect there to also be multiple GPIO entries in the
> >> _CRS list, with the order of the chip-selects GPIOs resources matching
> >> the order of the SPI resources.
> >
> > You can't assume that. There is no ACPI reason why the GpioIo should be
> > in order of chip selects. In fact, some manufacturers scramble them
> > (e.g. CS2, CS1, CS3). You have to remember that the ACPI is written so
> > that it works with Windows (which is the reason we need this serial
> > multi-instantiate driver.) so it may contain Windowsisms.
>
> Right, in that case maybe we do need the DMI quirk and the DMI quirk
> needs to point to a specific struct acpi_gpio_mapping for that model
> laptop ?
>
> Either there is some logic and the code can be generic, or there
> is no logic and then we need to not pretend that the code is generic.
>
> And in my mind the current code at least pretends to be somewhat
> generic.
>
> Thinking more about this, I do not think that the current patch does
> what it is supposed to do at all.
>
> According to the commit msg there is 1 GPIO listed in the _DSD
> props and the ACPI lookup added is using the same "cs-gpios" conid
> as the _DSD dependent code in drivers/spi/spi.c and it is getting
> the gpio with:
>
> smi->cs_gpio = devm_gpiod_get(dev, "cs", GPIOD_OUT_HIGH);
>
> which is equivalent to:
>
> smi->cs_gpio = devm_gpiod_get_index(dev, "cs", 0, GPIOD_OUT_HIGH);
>
> which is the same lookup as the already succeeded lookup of CS0
> in drivers/spi/spi.c .
>
> for CS1 the lookup should be:
>
> smi->cs_gpio = devm_gpiod_get_index(dev, "cs", 1, GPIOD_OUT_HIGH);
>
> and the struct acpi_gpio_mapping should point to an array of
> acpi_gpio_params[[] with 2 entries, with the first entry
> duplicating the GPIO from the _DSD info and the second entry
> pointing to the actual CS1 chip-select.

In general duplicate is not needed, the priority is FW, so the dup
entry won't be used, but since it's an array that leads us to the
necessity of having all elements being included. So, yes, in this case
we have to duplicate.

> Andy, you know the ACPI gpio-lookup code better then me, can
> you confirm that since the con_id between _DSD props and
> the added acpi_gpio_mapping is the same that calling:
>
> smi->cs_gpio = devm_gpiod_get(dev, "cs", GPIOD_OUT_HIGH);
>
> aka:
>
> smi->cs_gpio = devm_gpiod_get_index(dev, "cs", 0, GPIOD_OUT_HIGH);
>
> will just return the GPIO from the _DSD again ?

I haven't refreshed my memories, but that's exactly my understanding
of how it works. I can check it for sure if it doesn't work in
practice. But see also the link I shared.

[1]: https://lore.kernel.org/all/20190326212918.18541-1-andriy.shevchenko@xxxxxxxxxxxxxxx/

-- 
With Best Regards,
Andy Shevchenko