Re: [PATCH v2 3/3] Input: soc_button_array: Only debounce cherryview and baytrail systems

[Mario Limonciello]

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@xxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > > Fixes: 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons")
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > >
> > > 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 think we can revisit double debounce if a situation arises.

* I did find a problem at wakeup with a spurious event, and I'll include 
a patch in the next spin of my series for it.