Re: [PATCH] spi: Try to get ACPI GPIO IRQ earlier

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

Hi,

On 3-Nov-25 11:13 AM, Andy Shevchenko wrote:
> On Mon, Nov 03, 2025 at 10:57:44AM +0100, Hans de Goede wrote:
>> On 3-Nov-25 10:35 AM, Andy Shevchenko wrote:
>>> On Sun, Nov 02, 2025 at 08:09:21PM +0100, Hans de Goede wrote:
>>>> Since commit d24cfee7f63d ("spi: Fix acpi deferred irq probe"), the
>>>> acpi_dev_gpio_irq_get() call gets delayed till spi_probe() is called
>>>> on the SPI device.
>>>>
>>>> If there is no driver for the SPI device then the move to spi_probe()
>>>> results in acpi_dev_gpio_irq_get() never getting called. This may
>>>> cause problems by leaving the GPIO pin floating because this call is
>>>> responsible for setting up the GPIO pin direction and/or bias according
>>>> to the values from the ACPI tables.
>>>>
>>>> Re-add the removed acpi_dev_gpio_irq_get() in acpi_register_spi_device()
>>>> to ensure the GPIO pin is always correctly setup, while keeping the
>>>> acpi_dev_gpio_irq_get() call added to spi_probe() to deal with
>>>> -EPROBE_DEFER returns caused by the GPIO controller not having a driver
>>>> yet.
>>>
>>> Even before following the link to some papering over module via the link below
>>> I wondered, if the I²C case should be covered as well. The
>>> https://github.com/alexpevzner/hotfix-kvadra-touchpad refers to I²C enabled
>>> touchpads.
>>>
>>>> Link: https://bbs.archlinux.org/viewtopic.php?id=302348
> 
> ...
> 
>>> I'm not against the SPI fix, but is it confirmed that it really fixes the issue?
>>
>> Yes Mark and I got an offlist email bisecting this to the:
>>
>> Fixes: d24cfee7f63d ("spi: Fix acpi deferred irq probe")
>>
>> commit (on a stable kernel series) and a later email confirming that this
>> patch fixes it.
> 
> Shouldn't we use Closes in this case instead of Link?

I guess so.

>> It seems that leaving the fingerprint reader enable pin (the first GPIO
>> listed in _CRS which is an output only pin, is likely the enable pin)
>> floating is causing these issues. So in a way the acpi_dev_gpio_irq_get()
>> fixing this by forcing the enable pin to no longer float is a bit of
>> luck. But things did work before d24cfee7f63d ("spi: Fix acpi deferred irq probe")
>> so we need this to fix a regression
> 
> Yeah, fixing a regression is good thing, but not papering over the issue.

I agree in principle, but this is a quick and safe way to fix
the regression, where as the generic fix you describe below is
likely months away and also has significant risks of causing
regressions in various places, see below.

>> and as you indicate it seems
>> like a good idea in general and maybe we should also do this for i2c.
> 
>> As for doing something similar for I2C devices, that is an interesting
>> question. Even though it is possible I'm not aware of any i2c-devices
>> which have a userspace driver like SPI/USB fingerprint readers do,
>> so on i2c I would expect probe() to always get called. So I'm not sure
>> it is necessary there.
> 
> Reading the problem statement (the second paragraph) I lean towards
> a generic solution residing somewhere in drivers/acpi/scan.c (like
> acpi_init_device_object() / acpi_bus_attach() calls), although I don't
> see a quick way how to achieve this. It seems would require a bit of
> refactoring to allow ACPI glue code to call specific subsystem calls
> or making a GPIOLIB to provide some "early" initialisation flow(s).

I guess that you want to do the direction and bias init on all
GPIOs listed in _CRS, at least for devices with status == present ?

I was wondering about the same thing, but ACPI tables are full
of, well, erm, garbage in various places so I'm afraid that doing
this for all GPIO _CRS resources is likely to cause a whole lot
of pain.

Typically the firmware already sets up the direction + bias
of all used pins. I'm pretty sure the BIOS-es have some GPIO
init table especially for this somewhere.

Now those init-tables may have bugs, but I'm seriously worried
about the implication of doing the direction + bios setup for
all _CRS GPIO entries. I have simply seen too much non sense
devices (with _STA returning 0x0f) listing GPIOs also actually
used elsewhere to think this is a good idea.

Regards,

Hans