Message-ID: <a9bed0b4-b050-468b-91cb-bc4c81352046@kernel.org>
Date: Wed, 25 Jun 2025 21:42:19 +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 v2 3/3] Input: soc_button_array: Only debounce cherryview
 and baytrail systems

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@....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
>>> }
>>>
>>> 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@...nel.org>
>>> Fixes: 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons")
>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>
>> 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

Powered by blists - more mailing lists