Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Mario Limonciello <superm1@kernel.org>]

On 6/25/25 9:31 AM, Hans de Goede wrote:
> Hi Mario,
> 
> On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
>> On 6/25/25 4:09 AM, Hans de Goede wrote:
>>> Hi Mario,
>>>
>>> On 24-Jun-25 10:22 PM, Mario Limonciello wrote:
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> commit 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons")
>>>> hardcoded all soc-button-array devices to use a 50ms debounce timeout
>>>> but this doesn't work on all hardware.  The hardware I have on hand
>>>> actually prescribes in the ASL that the timeout should be 0:
>>>>
>>>> GpioInt (Edge, ActiveBoth, Exclusive, PullUp, 0x0000,
>>>>            "\\_SB.GPIO", 0x00, ResourceConsumer, ,)
>>>> {   // Pin list
>>>>       0x0000
>>>> }
>>>>
>>>> Let the GPIO core program the debounce instead of hardcoding it into a
>>>> driver.
>>>>
>>>> This reverts commit 5c4fa2a6da7fbc76290d1cb54a7e35633517a522.
>>>
>>> This is going to cause problems I'm afraid I just checked and
>>> based on randomly checking a few DSDTs of the tablets this driver
>>> is used on, it seems the DSDT always specifies a debounce timeout
>>> of 0 like your example above. And on many many devices using
>>> the soc_button_array driver debouncing is actually necessary.
>>
>> That's unfortunate to hear.
>>
>>>
>>> May I ask what problem you are seeing with the 50ms debounce timeout /
>>> what problem you are exactly trying to fix here ?
>>
>> The power button doesn't work to wake from suspend.  I bisected it down to your commit and then later traced that debounce from the ASL never gets set (pinctrl-amd's amd_gpio_set_debounce() is never called).
> 
> Ok, so specifically the gpiod_set_debounce() call with 50 ms
> done by gpio_keys.c is the problem I guess?

Yep.

> 
> So amd_gpio_set_debounce() does accept the 50 ms debounce
> passed to it by gpio_keys.c as a valid value and then setting
> that breaks the wake from suspend?

That's right.

Here is what /sys/kernel/debug/gpio has for the bad case (no patches):

  gpio     int|active|trigger|S0i3| S3|S4/S5| Z|wake|pull|  orient| 
  debounce|reg
#0        😛|     b|   edge|    |   |     |⏰|    |  ↑ |input  ↑|b (🕑 
046875us)|0x8151ce3

And then for the good case (these two patches):

  gpio     int|active|trigger|S0i3| S3|S4/S5| Z|wake|pull|  orient| 
  debounce|reg
#0        😛|     b|   edge|    |   |     |⏰|    |  ↑ |input  ↑| 
       |0x8151c00


> 
>> Also comparing the GPIO register in Windows (where things work) Windows never programs a debounce.
> 
> So maybe the windows ACPI0011 driver always uses a software-
> debounce for the buttons? Windows not debouncing the mechanical
> switches at all seems unlikely.
> 
> I think the best way to fix this might be to add a no-hw-debounce
> flag to the data passed from soc_button_array.c to gpio_keys.c
> and have gpio_keys.c not call gpiod_set_debounce()  when the
> no-hw-debounce flag is set.
> 
> I've checked and both on Bay Trail and Cherry Trail devices
> where soc_button_array is used a lot hw-debouncing is already
> unused. pinctrl-baytrail.c does not accept 50 ms as a valid
> value and pinctrl-cherryview.c does not support hw debounce
> at all.

That sounds a like a generally good direction to me.

I think I would still like to see the ASL values translated into the 
hardware even if the ASL has a "0" value.
So I would keep patch 1 but adjust for the warning you guys both called out.

As you have this hardware would you be able to work out that quirk?

Or if you want me to do it, I'll need something to go on how to how to 
effectively detect BYT and CYT hardware.

> 
>> So that's where both patches in this series came from.
>>
>>>
>>> drivers/input/keyboard/gpio_keys.c first will call gpiod_set_debounce()
>>> it self with the 50 ms provided by soc_button_array and if that does
>>> not work it will fall back to software debouncing. So I don't see how
>>> the 50 ms debounce can cause problems, other then maybe making
>>> really really (impossible?) fast double-clicks register as a single
>>> click .
>>>
>>> These buttons (e.g. volume up/down) are almost always simply mechanical
>>> switches and these definitely will need debouncing, the 0 value from
>>> the DSDT is plainly just wrong. There is no such thing as a not bouncing
>>> mechanical switch.
>>
>> On one of these tablets can you check the GPIO in Windows to see if it's using any debounce?
> 
> I'm afraid I don't have Windows installed on any of these.
> 
> But based on your testing + the DSDT specifying no debounce
> for the GPIO I guess Windows just follows the DSDt when it
> comes to setting up the hw debounce-settings and then uses
> sw-debouncing on top to actually avoid very quick
> press-release-press event cycles caused by the bouncing.
> 

Yeah that sounds like a plausible hypothesis.