Message-ID: <vmjnwfg2mqr2anugefjtzezimcep27gi64d4wsctiu476w73rl@oo6r4o33jk44>
Date: Thu, 26 Jun 2025 11:53:53 -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 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.

-- 
Dmitry

Powered by blists - more mailing lists