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

["Limonciello, Mario" <Mario.Limonciello@amd.com>]

On 6/25/25 9:41 AM, Mario Limonciello wrote:
> 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
> 

One more comment to share because there is a confusing result in this 
above debug log.

Systems that "don't use" soc-button-array program the "s0i3" / "s3" wake 
control bits at runtime.
Systems using "do use" soc-button-array don't program these until 
suspend time using gpio_keys_suspend() and disable them at resume time 
with gpio_keys_resume().

"Functionally" this is not a problem, but it was another rabbit hole 
that I went down debugging this issue, so I want to make sure anyone who 
comes across this thread is aware of it.

https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/input/keyboard/gpio_keys.c#L1049

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