Message-ID: <du46jt3mmkvceestjadbqmxbztp5xcurg4pzwzmqavo3pnfmak@tcfnufcu6de5>
Date: Thu, 26 Jun 2025 11:27:03 -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 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.

-- 
Dmitry

Powered by blists - more mailing lists