On Tue, Oct 27, 2020 at 06:09:49PM +0200, Andy Shevchenko wrote:

On Tue, Oct 27, 2020 at 6:01 PM Hans de Goede [off-list ref] wrote:

quoted

On 10/27/20 4:13 PM, Andy Shevchenko wrote:

quoted

On Tue, Oct 27, 2020 at 4:31 PM Hans de Goede [off-list ref] wrote:

quoted

On 10/26/20 11:54 PM, Coiby Xu wrote:

quoted

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

Nope, it was added to get rid of disambiguation when both Gpio*() and
PinConfig() are given.
So, 0 means default *if and only if* PinConfig() is present.

I.o.w. the OS layers should do this:

 - if Gpio*() provides Debounce != 0, we use it, otherwise
 - if PinConfig() is present for this pin with a debounce set, use it, otherwise
 - debounce is disabled.

Now we missed a midentry implementation in the Linux kernel, hence go
to last, i.e. disable debounce.
But it should be rather done in gpiolib-acpi.c.

Hope this helps.

I Cc'ed this to Mika as co-author of that part of specification, he
may correct me if I'm wrong.

I see, so then the right thing to do for the bug which we are seeing
on some AMD platforms would be to honor the debounce setting I guess ?

Can you and/or Mika write a patch(set) for this ?

I will look at it, but meanwhile I would postpone until having a
Mika's Ack on the action that my understanding and course of actions
is correct.

From what I recall this sounds correct :)
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees