Message-ID: <92ab85ff-6314-4db0-ae12-9803ddde5037@kernel.org>
Date: Wed, 25 Jun 2025 16:31:22 +0200
From: Hans de Goede <hansg@...nel.org>
To: Mario Limonciello <superm1@...nel.org>,
 Mika Westerberg <westeri@...nel.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski
 <brgl@...ev.pl>, Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: "open list:GPIO ACPI SUPPORT" <linux-gpio@...r.kernel.org>,
 "open list:GPIO ACPI SUPPORT" <linux-acpi@...r.kernel.org>,
 open list <linux-kernel@...r.kernel.org>,
 "open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)..."
 <linux-input@...r.kernel.org>, Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the
 buttons"

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

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?

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

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

Regards,

Hans





>>> Cc: Hans de Goede <hansg@...nel.org>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>> ---
>>>   drivers/input/misc/soc_button_array.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
>>> index b8cad415c62ca..99490df42b6f2 100644
>>> --- a/drivers/input/misc/soc_button_array.c
>>> +++ b/drivers/input/misc/soc_button_array.c
>>> @@ -219,8 +219,6 @@ soc_button_device_create(struct platform_device *pdev,
>>>           gpio_keys[n_buttons].active_low = info->active_low;
>>>           gpio_keys[n_buttons].desc = info->name;
>>>           gpio_keys[n_buttons].wakeup = info->wakeup;
>>> -        /* These devices often use cheap buttons, use 50 ms debounce */
>>> -        gpio_keys[n_buttons].debounce_interval = 50;
>>>           n_buttons++;
>>>       }
>>>   
>>
> 

Powered by blists - more mailing lists