Re: [PATCH v2 3/3] Input: soc_button_array: Only debounce cherryview and baytrail systems
From: Mario Limonciello
Date: Thu Jun 26 2025 - 17:39:58 EST
On 6/26/2025 2:54 PM, Dmitry Torokhov wrote:
On Thu, Jun 26, 2025 at 02:37:19PM -0500, Mario Limonciello wrote:
On 6/26/2025 2:32 PM, Dmitry Torokhov wrote:
On Thu, Jun 26, 2025 at 09:04:22PM +0200, Hans de Goede wrote:
Hi Dmitry,
On 26-Jun-25 20:53, Dmitry Torokhov wrote:
On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote:
On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
On 6/25/25 2:42 PM, Hans de Goede wrote:
Hi,
On 25-Jun-25 9:23 PM, Mario Limonciello wrote:
On 6/25/25 2:03 PM, Hans de Goede wrote:
Hi,
On 25-Jun-25 8:13 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
}
Many cherryview and baytrail systems don't have accurate values in the
ASL for debouncing and thus use software debouncing in gpio_keys. The
value to use is programmed in soc_button_array. Detect Cherry View
and Baytrail using ACPI HID IDs used for those GPIO controllers and apply
the 50ms only for those systems.
Cc: Hans de Goede <hansg@xxxxxxxxxx>
Fixes: 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons")
Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
I'm not a fan of this approach, I believe that we need to always debounce
when dealing with mechanical buttons otherwise we will get unreliable /
spurious input events.
My suggestion to deal with the issue where setting up debouncing at
the GPIO controller level is causing issues is to always use software
debouncing (which I suspect is what Windows does).
Let me copy and pasting my reply from the v1 thread with
a bit more detail on my proposal:
My proposal is to add a "no_hw_debounce" flag to
struct gpio_keys_platform_data and make the soc_button_array
driver set that regardless of which platform it is running on.
And then in gpio_keys.c do something like this:
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index f9db86da0818..2788d1e5782c 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -552,8 +552,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
bool active_low = gpiod_is_active_low(bdata->gpiod);
if (button->debounce_interval) {
- error = gpiod_set_debounce(bdata->gpiod,
- button->debounce_interval * 1000);
+ if (ddata->pdata->no_hw_debounce)
+ error = -EINVAL;
+ else
+ error = gpiod_set_debounce(bdata->gpiod,
+ button->debounce_interval * 1000);
/* use timer if gpiolib doesn't provide debounce */
if (error < 0)
bdata->software_debounce =
So keep debouncing, as that will always be necessary when dealing with
mechanical buttons, but always use software debouncing to avoid issues
like the issue you are seeing.
My mention of the BYT/CHT behavior in my previous email was to point
out that those already always use software debouncing for the 50 ms
debounce-period. It was *not* my intention to suggest to solve this
with platform specific quirks/behavior.
Regards,
Hans
I mentioned on the v1 too, but let's shift conversation here.
Ack.
So essentially all platforms using soc_button_array would always turn on software debouncing of 50ms?
Yes that is what my proposal entails.
In that case what happens if the hardware debounce was ALSO set from the ASL? You end up with double debouncing I would expect.
A hardware debounce of say 25 ms would still report the button down
immediately, it just won't report any state changes for 25 ms
after that, at least that is how I would expect this to work.
So the 50 ms ignore-button-releases for the sw debounce will start
at the same time as the hw ignore-button-release window and basically
the longest window will win. So having both active should not really
cause any problems.
Still only using one or the other as you propose below would
be better.
Shouldn't you only turn on software debouncing when it's required?
I'm wondering if considering the first two patches we should have gpio-keys look up if hardware can support debounce, and then "only if it can't" we program the value from soc button array.
It can be done by having gpio_keys do a "get()" on debounce. Iff the driver returns -ENOTSUPP /then/ program the software debounce.
Any special handling here should be done in soc_button_array since
this is specific to how with ACPI we have the GPIO resource
descriptors setting up the hw-debounce and then the need to do
software debounce when that was not setup.
As for checking for -ENOTSUPP I would make soc_button_array
do something like this.
ret = debounce_get()
if (ret <= 0)
use-sw-debounce;
If hw-debounce is supported but not setup, either because
the exact debounce value being requested is not supported
or because the DSDT specified 0, then sw-debouncing should
also be used.
Note this will still require the use of a new no_hw_debounce
flag so that we don't end up enabling hw-debounce in
the hw-debounce is supported but not setup case.
Regards,
Hans
I did some experiments with your proposal (letting SW debounce get
programmed) and everything seems to work fine*. I think you're right that
setting a double debounce would be worst one wins.
I am confused, can you explain why do we need this new no_hw_debounce
flag? If AMD gpio driver is unable to program 50 ms debounce for a given
pin but does not return an error (or returns an error but leaves system
in a bad state) that is the issue in that driver and needs to be fixed
there? Why do we need to change soc_button_driver at all?
Thanks.
The requested 50ms HW debounce gets programmed to the hardware register
successfully. It is within bound that the GPIO controller can support.
The problem is the power button does not function with a 50ms debounce.
The firmware asserted that 0ms should have been programmed (by the _CRS
value in GpioInt).
I do not understand how debounce that is within the controller's
supported range can not work. The button is a switch that reports on and
off, there is nothing more to it, is there?
I feel there is a deeper problem that we simply trying to paper over.
Note that on x86 wakeup events and GPIO IRQs typically use a different
event mechanism / path under the hood (PME events to resume from suspend).
It is not just a case of marking the IRQ used while running as a wakeup
source.
So it is possible that setting the hw-debouncing is in some way interfering
with the reporting of x86 PME events while the system is suspended.
Still this looks like platform issue, not driver issue. Should GPIO
driver refuse programming debounce if pin is configured as potential
wakeup source then?
How can the driver intricate details about the hardware connected to the
GPIO and how it behaves?
The driver is "dumb" and programs the registers according to the calls given
to it.
I do not think driver needs to know details of hardware connected to
GPIO. I know you mentioned that it may be connected to an EC that does
its own debouncing, however this should make no difference from AP
standpoint: you are still dealing with a GPIO line and your GPIO
controller does debouncing for that line (which may be unnecessary
because the line will not be bouncing).
Hans mentioned that it is possible that this debounce interferes with
the platform reporting wakeup events. I can easily believe that. But
that means that if there is another peripheral device similarly attached
to such GPIO configured for wakeup, device that does not use gpio_keys
driver, it will have the same issue. And I wonder if the solution is for
your GPIO provider driver to refuse programming debounce for GPIOs that
are marked as wake capable.
Hmm; how would we guarantee there was no regressions for other systems
with such a heuristics change?
IE what if there is a system that doesn't use soc-button-array and has a
non-zero debounce value in the GpioInt from _AEI() and is wake capable?
"Today" that would be programmed to the GPIO register.
If we're aiming for a driver specific heuristic another alternative is
to clear debounce for everything at suspend and restore it at resume.
I actually did that and it seems to work fine on this affected system,
so I'll draft it up as an alternative to patch v3 3/4.
Most systems where soc_button_array is used don't support hw-debouncing
in the first place, so on most systems this change is a no-op.
The change is not limited to soc_button_array driver, we need to add
flags to gpio_keys as well...
That's exactly what the patch v3 3/4 is doing.
What I was trying to say is that we are not only touching
soc_button_array driver but also have to make changes to more generic
gpio_keys driver which I would like to avoid if possible.
Thanks.