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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Nov 03, 2025 at 11:20:30AM +0100, Hans de Goede wrote:
> 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.

Perhaps we should add a TODO / FIXME there as well?
So at least we will know that this is not a proper solution.

> >> 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 ?

For the devices that are serial busses only (I²C, SPI, UART).

> 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.

Btw, other GPIO initialisation issues we have been solving by adding quirks.
Why this one can't be targeted with the same approach?

-- 
With Best Regards,
Andy Shevchenko