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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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


On Thu, Oct 8, 2020 at 8:58 AM Jamie McClymont <jamie@xxxxxxxxxx> 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