Message-ID: <55b4cd56-1812-4048-bf16-4b5b94a842d7@kernel.org>
Date: Thu, 26 Jun 2025 13:30:15 -0500
From: Mario Limonciello <superm1@...nel.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: 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>, "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/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@....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 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).

Powered by blists - more mailing lists