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

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

On Mon, Nov 03, 2025 at 03:15:15PM +0200, Andy Shevchenko wrote:
> 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.

Assuming it's added
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
as a quick fix of the regression. But in long term I would like to see the real
solution here.

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