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