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

[Mario Limonciello <superm1@kernel.org>]

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@amd.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@kernel.org>
>> Fixes: 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.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.

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.