Specifying a valid pullup value for pinctrl_intel from an ACPI table

Specifying a valid pullup value for pinctrl_intel from an ACPI table

[Jamie McClymont]

Background
==========

Hi all

I am looking to write a driver for a fingerprint sensor found in my laptop. The
device has an SPI plus interrupt and reset lines, specified like so:

   Method (_CRS, 0, Serialized)  // _CRS: Current Resource Settings
   {
       Name (RBUF, ResourceTemplate ()
       {
           // SPI
           SpiSerialBusV2 (0x0000, PolarityLow, FourWireMode, 0x08,
               ControllerInitiated, 0x00989680, ClockPolarityLow,
               ClockPhaseFirst, "\\_SB.PCI0.SPI1",
               0x00, ResourceConsumer, , Exclusive,
               )

           // Interrupt
           GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
               "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,
               )
               {   // Pin list
                   0x0000
               }

           // Reset
           GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
               "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,
               )
               {   // Pin list
                   0x0008
               }
       })
       CreateWordField (RBUF, 0x3B, GPIN)
       CreateWordField (RBUF, 0x63, SPIN)
       GPIN = GNUM (0x02000017)
       SPIN = GNUM (0x0202000A)
       Return (RBUF) /* \_SB_.SPBA._CRS.RBUF */
   }

The issue
=========

Currently, I call devm_acpi_dev_add_driver_gpios (index 1 for the reset line),
then try to access it with devm_gpoid_get, which fails with -EINVAL.

From some kprobe use, I see that the EINVAL appears to stem from
intel_config_set, which gpiod_direction_output calls (indirectly) with the
config 0x205 -- I understand this to mean PIN_CONFIG_BIAS_PULL_UP with the
argument 1, whereas the function expects a specific resistance value; one of {20000,
5000, 2000, 1000}.

The problematic call stack, as ftrace sees it (the leaf function is in fact
intel_config_set_pull):

    devm_gpiod_get
    devm_gpiod_get_index
    gpiod_get_index
    gpiod_configure_flags
    gpiod_direction_output
    gpio_set_bias
    gpiochip_generic_config
    pinctrl_gpio_set_config
    pinconf_set_config
    intel_config_set

The question
============

Any suggestions as to how this invalid configuration gets produced from the
DSDT, and what the best workaround would be?

I'm assuming that pullups sometimes get correctly configured purely from ACPI,
and this is hitting some edge-case?

I'm running 5.8.13 -- if anyone has an inkling that this is a bug that has since
been fixed, I'm happy to try and use a more recent kernel.

Hardware
========

Laptop is a Huawei Matebook X Pro
CPU is an Intel i5-8250U (the specific pinctrl is pinctrl_sunrisepoint)
Peripheral is a Goodix GXFP5187
The pin number is 58, seemingly named UART0_RTSB

Disclaimer
==========

This is my first foray into kernel development and I've been guessing at a lot of things -- please excuse any silly mistakes :)



Thanks
- Jamie McClymont

Re: Specifying a valid pullup value for pinctrl_intel from an ACPI table

[Andy Shevchenko]

+Cc: relevant people (Hans just in case mostly for his information)


On Thu, Oct 8, 2020 at 8:58 AM Jamie McClymont <jamie@kwiius.com> wrote:
>
> Background
> ==========
>
> Hi all
>
> I am looking to write a driver for a fingerprint sensor found in my laptop.

Can we know more about it? Model? Vendor? Ah, I see below.

>  The
> device has an SPI plus interrupt and reset lines, specified like so:

Cool!

>    Method (_CRS, 0, Serialized)  // _CRS: Current Resource Settings
>    {
>        Name (RBUF, ResourceTemplate ()
>        {
>            // SPI
>            SpiSerialBusV2 (0x0000, PolarityLow, FourWireMode, 0x08,
>                ControllerInitiated, 0x00989680, ClockPolarityLow,
>                ClockPhaseFirst, "\\_SB.PCI0.SPI1",
>                0x00, ResourceConsumer, , Exclusive,
>                )
>
>            // Interrupt
>            GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>                "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,
>                )
>                {   // Pin list
>                    0x0000
>                }
>
>            // Reset
>            GpioIo (Exclusive, PullUp, 0x0000, 0x0000, IoRestrictionOutputOnly,
>                "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,
>                )
>                {   // Pin list
>                    0x0008
>                }
>        })
>        CreateWordField (RBUF, 0x3B, GPIN)
>        CreateWordField (RBUF, 0x63, SPIN)
>        GPIN = GNUM (0x02000017)
>        SPIN = GNUM (0x0202000A)
>        Return (RBUF) /* \_SB_.SPBA._CRS.RBUF */
>    }

Yes, looks pretty much standard. I guess I have to insist our firmware
engineers to tell me a bit more about GNUM() macro so we can extend
documentation (in short, it takes 4 parameters, like bytes in 32-bit
value to define pin, group of pins, community in GPIO controller and
converts that to a GPIO number related to the controller).

> The issue
> =========
>
> Currently, I call devm_acpi_dev_add_driver_gpios (index 1 for the reset line),

(Also possible to use index 0 with GPIOIO_ONLY quirk, but it's not recommended)

> then try to access it with devm_gpoid_get, which fails with -EINVAL.
>
> From some kprobe use, I see that the EINVAL appears to stem from
> intel_config_set, which gpiod_direction_output calls (indirectly) with the
> config 0x205 -- I understand this to mean PIN_CONFIG_BIAS_PULL_UP with the
> argument 1,

Hmm... Should be 0x105, otherwise 2 is an argument.
Can you confirm it's for real 0x205?

> whereas the function expects a specific resistance value; one of {20000,
> 5000, 2000, 1000}.

Nice catch! I need to check this.
Yes, it seems it misses the fact that arg=1 when bias is set via GPIO subsystem.

> The problematic call stack, as ftrace sees it (the leaf function is in fact
> intel_config_set_pull):
>
>     devm_gpiod_get
>     devm_gpiod_get_index
>     gpiod_get_index
>     gpiod_configure_flags
>     gpiod_direction_output
>     gpio_set_bias
>     gpiochip_generic_config
>     pinctrl_gpio_set_config
>     pinconf_set_config
>     intel_config_set
>
> The question
> ============
>
> Any suggestions as to how this invalid configuration gets produced from the
> DSDT, and what the best workaround would be?

Fixing the driver would be the best.
Thanks for the report, I'm on it!

> I'm assuming that pullups sometimes get correctly configured purely from ACPI,
> and this is hitting some edge-case?
>
> I'm running 5.8.13 -- if anyone has an inkling that this is a bug that has since
> been fixed, I'm happy to try and use a more recent kernel.

It's always recommended to test at least last vanilla (as of today
v5.9-rc8) and latest linux-next [1]

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/

> Hardware
> ========
>
> Laptop is a Huawei Matebook X Pro
> CPU is an Intel i5-8250U (the specific pinctrl is pinctrl_sunrisepoint)
> Peripheral is a Goodix GXFP5187
> The pin number is 58, seemingly named UART0_RTSB
>
> Disclaimer
> ==========
>
> This is my first foray into kernel development and I've been guessing at a lot of things -- please excuse any silly mistakes :)

It's fine, thanks for the detailed report!

--
With Best Regards,
Andy Shevchenko

Re: Specifying a valid pullup value for pinctrl_intel from an ACPI table

[Jamie McClymont]

> Hmm... Should be 0x105, otherwise 2 is an argument.
> Can you confirm it's for real 0x205?

Ah yes, you're right (I had the 2 in my head having converted it from the decimal 261)

> Fixing the driver would be the best.
> Thanks for the report, I'm on it!

Fantastic, thank you!

- Jamie

Re: Specifying a valid pullup value for pinctrl_intel from an ACPI table

[Andy Shevchenko]

On Sat, Oct 10, 2020 at 1:57 AM Jamie McClymont <jamie@kwiius.com> wrote:
>
> > Hmm... Should be 0x105, otherwise 2 is an argument.
> > Can you confirm it's for real 0x205?
>
> Ah yes, you're right (I had the 2 in my head having converted it from the decimal 261)
>
> > Fixing the driver would be the best.
> > Thanks for the report, I'm on it!
>
> Fantastic, thank you!

I have just sent a series of two patches that should address this
report. Can you, please, test it and confirm that it's fixing the
issue? Note, the GpioInt() one I will going to address in another
series.

Also, you might add [1] to the bucket just in case.

[1]: https://lore.kernel.org/linux-gpio/20201009184359.16427-1-andriy.shevchenko@linux.intel.com/


-- 
With Best Regards,
Andy Shevchenko