Message-ID: <90ff69f1-0308-4591-9374-5319922f0870@kernel.org>
Date: Thu, 26 Jun 2025 16:39:34 -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 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@....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).
>>>>>
>>>>> 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.
> 

Powered by blists - more mailing lists