Re: Any other ways to debug GPIO interrupt controller (pinctrl-amd) for broken touchpads of a new laptop model?

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 10/26/20 11:54 PM, Coiby Xu wrote:
> Hi Hans and Linus,
> 
> Will you interpret the 0x0000 value for debounce timeout in GPIO
> Interrupt Connection Resource Descriptor as disabling debouncing
> filter?
> 
> GpioInt (EdgeLevel, ActiveLevel, Shared, PinConfig, DebounceTimeout, ResourceSource,
> ResourceSourceIndex, ResourceUsage, DescriptorName, VendorData) {PinList}
> 
> I'm not sure if Windows' implementation is the de facto standard like
> i2c-hid. But if we are going to conform to the ACPI specs and we would
> regard 0x0000 debounce timeout as disabling debouncing filter, then we
> can fix this touchpad issue and potentially some related issues by
> implementing the feature of supporting configuring debounce timeout in
> drivers/gpio/gpiolib-acpi.c and removing all debounce filter
> configuration in amd_gpio_irq_set_type of drivers/pinctrl/pinctrl-amd.c.
> What do you think?
> 
> A favorable evidence is I've collected five DSDT tables when
> investigating this issue. All 5 DSDT tables have an GpioInt specifying
> an non-zero debounce timeout value for the edge type irq and for all
> the level type irq, the debounce timeout is set to 0x0000.

That is a very interesting observation and this matches with my
instincts which say that we should just disable the debounce filter
for level triggered interrupts in pinctrl-amd.c

Yes that is a bit of a shortcut vs reading the valie from the ACPI
table, but I'm not sure that 0 always means disabled.

Specifically the ACPI 6.2 spec also has a notion of pinconf settings
and the docs on "PinConfig()"  say:

Note: There is some overlap between the properties set by GpioIo/GpioInt/ PinFunction and
PinConfig descriptors. For example, both are setting properties such as pull-ups. If the same
property is specified by multiple descriptors for the same pins, the order in which these properties
are applied is undetermined. To avoid any conflicts, GpioInt/GpioIo/PinFunction should provide a
default value for these properties when PinConfig is used. If PinConfig is used to set pin bias,
PullDefault should be used for GpioIo/GpioInt/ PinFunction. *If PinConfig is used to set debounce
timeout, 0 should be used for GpioIo/GpioInt.*

So that suggests that a value of 0 does not necessarily mean "disabled" but
it means use a default, or possibly get the value from somewhere else such
as from a ACPI PinConfig description (if present).

So I see 2 ways to move forward with his:

1. Just disable the debounce filter for level type IRQs; or
2. Add a helper to sanitize the debounce pulse-duration setting and
   call that when setting the IRQ type.
   This helper would read the setting check it is not crazy long for
   an IRQ-line (lets say anything above 1 ms is crazy long) and if it
   is crazy long then overwrite it with a saner value.

2. is a bit tricky, because if the IRQ line comes from a chip then
obviously max 1ms debouncing to catch eletrical interference should be
fine. But sometimes cheap buttons for things like volume up/down on tablets
are directly connected to GPIOs and then we may want longer debouncing...

So if we do 2. we may want to limit it to only level type IRQs too.

Note I have contacted AMD about this and asked them for some input on this,
ideally they can tell us how exactly we should program the debounce filter
and based on which data we should do that.

Regards,

Hans