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

[Mario Limonciello]

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.

So essentially all platforms using soc_button_array would always turn on 
software debouncing of 50ms?

In that case what happens if the hardware debounce was ALSO set from the 
ASL?  You end up with double debouncing I would expect.

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.