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