Message-ID: <676b774e-5111-4eea-bc6e-968840193009@kernel.org>
Date: Wed, 25 Jun 2025 14:10:29 -0500
From: Mario Limonciello <superm1@...nel.org>
To: 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>, Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: "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 2/2] Revert "Input: soc_button_array - debounce the
 buttons"

On 6/25/25 1:57 PM, Hans de Goede wrote:
> On 25-Jun-25 4:41 PM, Mario Limonciello wrote:
>> On 6/25/25 9:31 AM, Hans de Goede wrote:
> 
> <snip>
> 
>>> So maybe the windows ACPI0011 driver always uses a software-
>>> debounce for the buttons? Windows not debouncing the mechanical
>>> switches at all seems unlikely.
>>>
>>> I think the best way to fix this might be to add a no-hw-debounce
>>> flag to the data passed from soc_button_array.c to gpio_keys.c
>>> and have gpio_keys.c not call gpiod_set_debounce()  when the
>>> no-hw-debounce flag is set.
>>>
>>> I've checked and both on Bay Trail and Cherry Trail devices
>>> where soc_button_array is used a lot hw-debouncing is already
>>> unused. pinctrl-baytrail.c does not accept 50 ms as a valid
>>> value and pinctrl-cherryview.c does not support hw debounce
>>> at all.
>>
>> That sounds a like a generally good direction to me.
>>
>> I think I would still like to see the ASL values translated into the hardware even if the ASL has a "0" value.
>> So I would keep patch 1 but adjust for the warning you guys both called out.
>>
>> As you have this hardware would you be able to work out that quirk?
> 
> I think we've a bit of miscommunication going on here.
> 
> 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, which I believe will always be necessary when
> dealing with mechanical buttons, but always use software debouncing
> (which I suspect is what Windows does) to avoid issues like the issue
> you are seeing.

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?

> 
> My mention of the BYT/CHT behavior in my previous email was to point
> out that those already do use software debouncing for the 50 ms
> debounce-period. It was *not* my intention to suggest to solve this
> with platform specific quirks/behavior.
> 
> <semi offtopic>
> Hmm, I did found one interesting thing looking at further DSDTs
> the Dell Venue 10 Pro 5056 DSDT actually specifies a non 0
> debounce time in the ACPI0011 device's GPIO descriptors
> it uses a value of 30 ms. This device being one of the few
> actually specifying a debounce time in the ACPI is ironic
> since it uses drivers/pinctrl/intel/pinctrl-cherryview.c
> which does not support PIN_CONFIG_INPUT_DEBOUNCE...
> </semi offtopic>
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
>>
>> Or if you want me to do it, I'll need something to go on how to how to effectively detect BYT and CYT hardware.
>>
>>>
>>>> So that's where both patches in this series came from.
>>>>
>>>>>
>>>>> drivers/input/keyboard/gpio_keys.c first will call gpiod_set_debounce()
>>>>> it self with the 50 ms provided by soc_button_array and if that does
>>>>> not work it will fall back to software debouncing. So I don't see how
>>>>> the 50 ms debounce can cause problems, other then maybe making
>>>>> really really (impossible?) fast double-clicks register as a single
>>>>> click .
>>>>>
>>>>> These buttons (e.g. volume up/down) are almost always simply mechanical
>>>>> switches and these definitely will need debouncing, the 0 value from
>>>>> the DSDT is plainly just wrong. There is no such thing as a not bouncing
>>>>> mechanical switch.
>>>>
>>>> On one of these tablets can you check the GPIO in Windows to see if it's using any debounce?
>>>
>>> I'm afraid I don't have Windows installed on any of these.
>>>
>>> But based on your testing + the DSDT specifying no debounce
>>> for the GPIO I guess Windows just follows the DSDt when it
>>> comes to setting up the hw debounce-settings and then uses
>>> sw-debouncing on top to actually avoid very quick
>>> press-release-press event cycles caused by the bouncing.
>>>
>>
>> Yeah that sounds like a plausible hypothesis.
>>
>>
> 

Powered by blists - more mailing lists