Message-ID: <75wqpbcfhtvwv7ity77obmwsyswk5crxjpxmboayqj6lvvq7jg@7ojjp25blz5q>
Date: Thu, 26 Jun 2025 22:08:52 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Mario Limonciello <superm1@...nel.org>
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 Thu, Jun 26, 2025 at 04:39:34PM -0500, Mario Limonciello wrote:
> 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.

That is exactly what I am saying. I am asserting that if there is
another system that does not use soc-button-array (or rather gpio-keys)
and it programs GPIO debounce for GPIO that is used for wakeup then on
the same platform it will be similarly broken. It is not GPIO-consumer
driver specific, it is platform/GPIO provider specific behavior.

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

I guess this also an option...

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists