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

[Hans de Goede <hansg@kernel.org>]

Hi,

On 13-Apr-26 2:51 PM, Richard Fitzgerald wrote:
> On 13/04/2026 12:43 pm, Hans de Goede wrote:
>> Hi,
>>
>> 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:
>>>
> 
> <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.


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 ?

Regards,

Hans