Message-ID: <8fc9051f-bef3-43fc-83a1-172a0eb599dc@kernel.org>
Date: Wed, 25 Jun 2025 15:34:07 -0500
From: Mario Limonciello <superm1@...nel.org>
To: Hans de Goede <hansg@...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

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

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 think we can revisit double debounce if a situation arises.

* I did find a problem at wakeup with a spurious event, and I'll include 
a patch in the next spin of my series for it.

Powered by blists - more mailing lists