[PATCH 0/2] Fix soc-button-array debounce

[PATCH 0/2] Fix soc-button-array debounce

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

I have some hardware in front of me that uses the soc-button-array
driver but the power button doesn't work.

Digging into it, it's because the ASL prescribes a debounce of 0 for
the power button, but the soc-button-array driver hardcodes 50ms.

Hardcoding it to what the ASL expects the power button works.

I looked at the callpath into the GPIO core and I believe it's
because the debounce value from _CRS is never programmed to the
hardware the way that the GPIO gets setup.

This series add that programming path and then drops the hardcoded
value.  Hopefully Hans can confirm this continues to work on the
hardware that he originally developed the hardcoding for.

If it doesn't work on that hardware, I think it's more scalable
to introduce a quirk for it so that the kernel can at least set
the values intended by the firmware.

Mario Limonciello (2):
  gpiolib: acpi: Program debounce when finding GPIO
  Revert "Input: soc_button_array - debounce the buttons"

 drivers/gpio/gpiolib-acpi-core.c      | 4 ++++
 drivers/input/misc/soc_button_array.c | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/2] gpiolib: acpi: Program debounce when finding GPIO

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

When soc-button-array looks up the GPIO to use it calls acpi_find_gpio()
which will parse _CRS.

acpi_find_gpio.cold (drivers/gpio/gpiolib-acpi-core.c:953)
gpiod_find_and_request (drivers/gpio/gpiolib.c:4598 drivers/gpio/gpiolib.c:4625)
gpiod_get_index (drivers/gpio/gpiolib.c:4877)

The GPIO is setup basically, but the debounce information is discarded.
The platform will assert what debounce should be in _CRS, so program it
at the time it's available.

Cc: Hans de Goede <hansg@kernel.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpio/gpiolib-acpi-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index 12b24a717e43f..475cac2d95aa1 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -944,6 +944,7 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 	bool can_fallback = acpi_can_fallback_to_crs(adev, con_id);
 	struct acpi_gpio_info info;
 	struct gpio_desc *desc;
+	int ret;
 
 	desc = __acpi_find_gpio(fwnode, con_id, idx, can_fallback, &info);
 	if (IS_ERR(desc))
@@ -957,6 +958,9 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 
 	acpi_gpio_update_gpiod_flags(dflags, &info);
 	acpi_gpio_update_gpiod_lookup_flags(lookupflags, &info);
+	ret = gpio_set_debounce_timeout(desc, info.debounce * 10);
+	if (ret)
+		return ERR_PTR(ret);
 	return desc;
 }
 
-- 
2.43.0

Re: [PATCH 1/2] gpiolib: acpi: Program debounce when finding GPIO

[Hans de Goede]

Hi Mario,

On 24-Jun-25 10:22 PM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> When soc-button-array looks up the GPIO to use it calls acpi_find_gpio()
> which will parse _CRS.
> 
> acpi_find_gpio.cold (drivers/gpio/gpiolib-acpi-core.c:953)
> gpiod_find_and_request (drivers/gpio/gpiolib.c:4598 drivers/gpio/gpiolib.c:4625)
> gpiod_get_index (drivers/gpio/gpiolib.c:4877)
> 
> The GPIO is setup basically, but the debounce information is discarded.
> The platform will assert what debounce should be in _CRS, so program it
> at the time it's available.
> 
> Cc: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpio/gpiolib-acpi-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
> index 12b24a717e43f..475cac2d95aa1 100644
> --- a/drivers/gpio/gpiolib-acpi-core.c
> +++ b/drivers/gpio/gpiolib-acpi-core.c
> @@ -944,6 +944,7 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
>  	bool can_fallback = acpi_can_fallback_to_crs(adev, con_id);
>  	struct acpi_gpio_info info;
>  	struct gpio_desc *desc;
> +	int ret;
>  
>  	desc = __acpi_find_gpio(fwnode, con_id, idx, can_fallback, &info);
>  	if (IS_ERR(desc))
> @@ -957,6 +958,9 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
>  
>  	acpi_gpio_update_gpiod_flags(dflags, &info);
>  	acpi_gpio_update_gpiod_lookup_flags(lookupflags, &info);
> +	ret = gpio_set_debounce_timeout(desc, info.debounce * 10);
> +	if (ret)
> +		return ERR_PTR(ret);

IIRC this is going to fail sometimes, depending on which range of
debounce values the GPIO controller support. Note that there already
is another code-path in gpiolib-acpi-core.c which calls
gpio_set_debounce_timeout() in acpi_request_own_gpiod() and it does:

        /* ACPI uses hundredths of milliseconds units */
        ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout * 10);
        if (ret)
                dev_warn(chip->parent,
                         "Failed to set debounce-timeout for pin 0x%04X, err %d\n",
                         pin, ret);

Making this a warning was done in commit cef0d022f553 ("gpiolib: acpi: Make
set-debounce-timeout failures non fatal").

Otherwise I think this is fine.

Regards,

Hans

Re: [PATCH 1/2] gpiolib: acpi: Program debounce when finding GPIO

[Andy Shevchenko]

On Tue, Jun 24, 2025 at 03:22:10PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> When soc-button-array looks up the GPIO to use it calls acpi_find_gpio()
> which will parse _CRS.
> 
> acpi_find_gpio.cold (drivers/gpio/gpiolib-acpi-core.c:953)
> gpiod_find_and_request (drivers/gpio/gpiolib.c:4598 drivers/gpio/gpiolib.c:4625)
> gpiod_get_index (drivers/gpio/gpiolib.c:4877)
> 
> The GPIO is setup basically, but the debounce information is discarded.
> The platform will assert what debounce should be in _CRS, so program it
> at the time it's available.

If this will be needed we can factor out acpi_gpio_set_debounce() with
the warning and use it in both cases.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Mario Limonciello]

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
}

Let the GPIO core program the debounce instead of hardcoding it into a
driver.

This reverts commit 5c4fa2a6da7fbc76290d1cb54a7e35633517a522.

Cc: Hans de Goede <hansg@kernel.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/input/misc/soc_button_array.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index b8cad415c62ca..99490df42b6f2 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -219,8 +219,6 @@ soc_button_device_create(struct platform_device *pdev,
 		gpio_keys[n_buttons].active_low = info->active_low;
 		gpio_keys[n_buttons].desc = info->name;
 		gpio_keys[n_buttons].wakeup = info->wakeup;
-		/* These devices often use cheap buttons, use 50 ms debounce */
-		gpio_keys[n_buttons].debounce_interval = 50;
 		n_buttons++;
 	}
 
-- 
2.43.0

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Hans de Goede]

Hi Mario,

On 24-Jun-25 10:22 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
> }
> 
> Let the GPIO core program the debounce instead of hardcoding it into a
> driver.
> 
> This reverts commit 5c4fa2a6da7fbc76290d1cb54a7e35633517a522.

This is going to cause problems I'm afraid I just checked and
based on randomly checking a few DSDTs of the tablets this driver
is used on, it seems the DSDT always specifies a debounce timeout
of 0 like your example above. And on many many devices using
the soc_button_array driver debouncing is actually necessary.

May I ask what problem you are seeing with the 50ms debounce timeout /
what problem you are exactly trying to fix here ?

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.

Regards,

Hans



> 
> Cc: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/input/misc/soc_button_array.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> index b8cad415c62ca..99490df42b6f2 100644
> --- a/drivers/input/misc/soc_button_array.c
> +++ b/drivers/input/misc/soc_button_array.c
> @@ -219,8 +219,6 @@ soc_button_device_create(struct platform_device *pdev,
>  		gpio_keys[n_buttons].active_low = info->active_low;
>  		gpio_keys[n_buttons].desc = info->name;
>  		gpio_keys[n_buttons].wakeup = info->wakeup;
> -		/* These devices often use cheap buttons, use 50 ms debounce */
> -		gpio_keys[n_buttons].debounce_interval = 50;
>  		n_buttons++;
>  	}
>  

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Mario Limonciello]

On 6/25/25 4:09 AM, Hans de Goede wrote:
> Hi Mario,
> 
> On 24-Jun-25 10:22 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
>> }
>>
>> Let the GPIO core program the debounce instead of hardcoding it into a
>> driver.
>>
>> This reverts commit 5c4fa2a6da7fbc76290d1cb54a7e35633517a522.
> 
> This is going to cause problems I'm afraid I just checked and
> based on randomly checking a few DSDTs of the tablets this driver
> is used on, it seems the DSDT always specifies a debounce timeout
> of 0 like your example above. And on many many devices using
> the soc_button_array driver debouncing is actually necessary.

That's unfortunate to hear.

> 
> May I ask what problem you are seeing with the 50ms debounce timeout /
> what problem you are exactly trying to fix here ?

The power button doesn't work to wake from suspend.  I bisected it down 
to your commit and then later traced that debounce from the ASL never 
gets set (pinctrl-amd's amd_gpio_set_debounce() is never called).

Also comparing the GPIO register in Windows (where things work) Windows 
never programs a debounce.

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?

> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>> Cc: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/input/misc/soc_button_array.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
>> index b8cad415c62ca..99490df42b6f2 100644
>> --- a/drivers/input/misc/soc_button_array.c
>> +++ b/drivers/input/misc/soc_button_array.c
>> @@ -219,8 +219,6 @@ soc_button_device_create(struct platform_device *pdev,
>>   		gpio_keys[n_buttons].active_low = info->active_low;
>>   		gpio_keys[n_buttons].desc = info->name;
>>   		gpio_keys[n_buttons].wakeup = info->wakeup;
>> -		/* These devices often use cheap buttons, use 50 ms debounce */
>> -		gpio_keys[n_buttons].debounce_interval = 50;
>>   		n_buttons++;
>>   	}
>>   
> 

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Hans de Goede]

Hi Mario,

On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
> On 6/25/25 4:09 AM, Hans de Goede wrote:
>> Hi Mario,
>>
>> On 24-Jun-25 10:22 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
>>> }
>>>
>>> Let the GPIO core program the debounce instead of hardcoding it into a
>>> driver.
>>>
>>> This reverts commit 5c4fa2a6da7fbc76290d1cb54a7e35633517a522.
>>
>> This is going to cause problems I'm afraid I just checked and
>> based on randomly checking a few DSDTs of the tablets this driver
>> is used on, it seems the DSDT always specifies a debounce timeout
>> of 0 like your example above. And on many many devices using
>> the soc_button_array driver debouncing is actually necessary.
> 
> That's unfortunate to hear.
> 
>>
>> May I ask what problem you are seeing with the 50ms debounce timeout /
>> what problem you are exactly trying to fix here ?
> 
> The power button doesn't work to wake from suspend.  I bisected it down to your commit and then later traced that debounce from the ASL never gets set (pinctrl-amd's amd_gpio_set_debounce() is never called).

Ok, so specifically the gpiod_set_debounce() call with 50 ms
done by gpio_keys.c is the problem I guess?

So amd_gpio_set_debounce() does accept the 50 ms debounce
passed to it by gpio_keys.c as a valid value and then setting
that breaks the wake from suspend?

> Also comparing the GPIO register in Windows (where things work) Windows never programs a debounce.

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.

> 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.

Regards,

Hans





>>> Cc: Hans de Goede <hansg@kernel.org>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   drivers/input/misc/soc_button_array.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
>>> index b8cad415c62ca..99490df42b6f2 100644
>>> --- a/drivers/input/misc/soc_button_array.c
>>> +++ b/drivers/input/misc/soc_button_array.c
>>> @@ -219,8 +219,6 @@ soc_button_device_create(struct platform_device *pdev,
>>>           gpio_keys[n_buttons].active_low = info->active_low;
>>>           gpio_keys[n_buttons].desc = info->name;
>>>           gpio_keys[n_buttons].wakeup = info->wakeup;
>>> -        /* These devices often use cheap buttons, use 50 ms debounce */
>>> -        gpio_keys[n_buttons].debounce_interval = 50;
>>>           n_buttons++;
>>>       }
>>>   
>>
> 

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Mario Limonciello]

On 6/25/25 9:31 AM, Hans de Goede wrote:
> Hi Mario,
> 
> On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
>> On 6/25/25 4:09 AM, Hans de Goede wrote:
>>> Hi Mario,
>>>
>>> On 24-Jun-25 10:22 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
>>>> }
>>>>
>>>> Let the GPIO core program the debounce instead of hardcoding it into a
>>>> driver.
>>>>
>>>> This reverts commit 5c4fa2a6da7fbc76290d1cb54a7e35633517a522.
>>>
>>> This is going to cause problems I'm afraid I just checked and
>>> based on randomly checking a few DSDTs of the tablets this driver
>>> is used on, it seems the DSDT always specifies a debounce timeout
>>> of 0 like your example above. And on many many devices using
>>> the soc_button_array driver debouncing is actually necessary.
>>
>> That's unfortunate to hear.
>>
>>>
>>> May I ask what problem you are seeing with the 50ms debounce timeout /
>>> what problem you are exactly trying to fix here ?
>>
>> The power button doesn't work to wake from suspend.  I bisected it down to your commit and then later traced that debounce from the ASL never gets set (pinctrl-amd's amd_gpio_set_debounce() is never called).
> 
> Ok, so specifically the gpiod_set_debounce() call with 50 ms
> done by gpio_keys.c is the problem I guess?

Yep.

> 
> So amd_gpio_set_debounce() does accept the 50 ms debounce
> passed to it by gpio_keys.c as a valid value and then setting
> that breaks the wake from suspend?

That's right.

Here is what /sys/kernel/debug/gpio has for the bad case (no patches):

  gpio     int|active|trigger|S0i3| S3|S4/S5| Z|wake|pull|  orient| 
  debounce|reg
#0        😛|     b|   edge|    |   |     |⏰|    |  ↑ |input  ↑|b (🕑 
046875us)|0x8151ce3

And then for the good case (these two patches):

  gpio     int|active|trigger|S0i3| S3|S4/S5| Z|wake|pull|  orient| 
  debounce|reg
#0        😛|     b|   edge|    |   |     |⏰|    |  ↑ |input  ↑| 
       |0x8151c00


> 
>> Also comparing the GPIO register in Windows (where things work) Windows never programs a debounce.
> 
> 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?

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.

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Limonciello, Mario]

On 6/25/25 9:41 AM, Mario Limonciello wrote:
> On 6/25/25 9:31 AM, Hans de Goede wrote:
>> Hi Mario,
>>
>> On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
>>> On 6/25/25 4:09 AM, Hans de Goede wrote:
>>>> Hi Mario,
>>>>
>>>> On 24-Jun-25 10:22 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
>>>>> }
>>>>>
>>>>> Let the GPIO core program the debounce instead of hardcoding it into a
>>>>> driver.
>>>>>
>>>>> This reverts commit 5c4fa2a6da7fbc76290d1cb54a7e35633517a522.
>>>>
>>>> This is going to cause problems I'm afraid I just checked and
>>>> based on randomly checking a few DSDTs of the tablets this driver
>>>> is used on, it seems the DSDT always specifies a debounce timeout
>>>> of 0 like your example above. And on many many devices using
>>>> the soc_button_array driver debouncing is actually necessary.
>>>
>>> That's unfortunate to hear.
>>>
>>>>
>>>> May I ask what problem you are seeing with the 50ms debounce timeout /
>>>> what problem you are exactly trying to fix here ?
>>>
>>> The power button doesn't work to wake from suspend.  I bisected it 
>>> down to your commit and then later traced that debounce from the ASL 
>>> never gets set (pinctrl-amd's amd_gpio_set_debounce() is never called).
>>
>> Ok, so specifically the gpiod_set_debounce() call with 50 ms
>> done by gpio_keys.c is the problem I guess?
> 
> Yep.
> 
>>
>> So amd_gpio_set_debounce() does accept the 50 ms debounce
>> passed to it by gpio_keys.c as a valid value and then setting
>> that breaks the wake from suspend?
> 
> That's right.
> 
> Here is what /sys/kernel/debug/gpio has for the bad case (no patches):
> 
>   gpio     int|active|trigger|S0i3| S3|S4/S5| Z|wake|pull|  orient| 
>   debounce|reg
> #0        😛|     b|   edge|    |   |     |⏰|    |  ↑ |input  ↑|b (🕑 
> 046875us)|0x8151ce3
> 
> And then for the good case (these two patches):
> 
>   gpio     int|active|trigger|S0i3| S3|S4/S5| Z|wake|pull|  orient| 
>   debounce|reg
> #0        😛|     b|   edge|    |   |     |⏰|    |  ↑ |input  ↑|       
> |0x8151c00
> 

One more comment to share because there is a confusing result in this 
above debug log.

Systems that "don't use" soc-button-array program the "s0i3" / "s3" wake 
control bits at runtime.
Systems using "do use" soc-button-array don't program these until 
suspend time using gpio_keys_suspend() and disable them at resume time 
with gpio_keys_resume().

"Functionally" this is not a problem, but it was another rabbit hole 
that I went down debugging this issue, so I want to make sure anyone who 
comes across this thread is aware of it.

https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/input/keyboard/gpio_keys.c#L1049

> 
>>
>>> Also comparing the GPIO register in Windows (where things work) 
>>> Windows never programs a debounce.
>>
>> 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?
> 
> 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.
> 
> 

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 03:02:18PM +0000, Limonciello, Mario wrote:
> On 6/25/25 9:41 AM, Mario Limonciello wrote:
> > On 6/25/25 9:31 AM, Hans de Goede wrote:
> >> On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
> >>> On 6/25/25 4:09 AM, Hans de Goede wrote:
> >>>> On 24-Jun-25 10:22 PM, Mario Limonciello wrote:

...

> >> Ok, so specifically the gpiod_set_debounce() call with 50 ms
> >> done by gpio_keys.c is the problem I guess?
> > 
> > Yep.
> > 
> >> So amd_gpio_set_debounce() does accept the 50 ms debounce
> >> passed to it by gpio_keys.c as a valid value and then setting
> >> that breaks the wake from suspend?
> > 
> > That's right.

> >>> Also comparing the GPIO register in Windows (where things work) 
> >>> Windows never programs a debounce.
> >>
> >> 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.

Thinking a bit more of this, perhaps the HW debounce support flag should be
per-GPIO-descriptor thingy. In such cases we don't need to distinguish the
platforms, the GPIO ACPI lib may simply set that flag based on 0 read from
the ACPI tables. It will also give a clue to any driver that uses GPIOs
(not only gpio-keys).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Limonciello, Mario]

On 6/25/25 10:10 AM, Andy Shevchenko wrote:
> On Wed, Jun 25, 2025 at 03:02:18PM +0000, Limonciello, Mario wrote:
>> On 6/25/25 9:41 AM, Mario Limonciello wrote:
>>> On 6/25/25 9:31 AM, Hans de Goede wrote:
>>>> On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
>>>>> On 6/25/25 4:09 AM, Hans de Goede wrote:
>>>>>> On 24-Jun-25 10:22 PM, Mario Limonciello wrote:
> 
> ...
> 
>>>> Ok, so specifically the gpiod_set_debounce() call with 50 ms
>>>> done by gpio_keys.c is the problem I guess?
>>>
>>> Yep.
>>>
>>>> So amd_gpio_set_debounce() does accept the 50 ms debounce
>>>> passed to it by gpio_keys.c as a valid value and then setting
>>>> that breaks the wake from suspend?
>>>
>>> That's right.
> 
>>>>> Also comparing the GPIO register in Windows (where things work)
>>>>> Windows never programs a debounce.
>>>>
>>>> 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.
> 
> Thinking a bit more of this, perhaps the HW debounce support flag should be
> per-GPIO-descriptor thingy. In such cases we don't need to distinguish the
> platforms, the GPIO ACPI lib may simply set that flag based on 0 read from
> the ACPI tables. It will also give a clue to any driver that uses GPIOs
> (not only gpio-keys).
> 

But 0 doesn't mean hardware debounce support is there, 0 means that 
hardware debounce is not required to be programmed for this GPIO.

That is - if another system had a non-zero value in the GpioInt entry I 
would expect this to be translated into the GPIO register.

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 03:14:40PM +0000, Limonciello, Mario wrote:
> On 6/25/25 10:10 AM, Andy Shevchenko wrote:
> > On Wed, Jun 25, 2025 at 03:02:18PM +0000, Limonciello, Mario wrote:
> >> On 6/25/25 9:41 AM, Mario Limonciello wrote:
> >>> On 6/25/25 9:31 AM, Hans de Goede wrote:
> >>>> On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
> >>>>> On 6/25/25 4:09 AM, Hans de Goede wrote:
> >>>>>> On 24-Jun-25 10:22 PM, Mario Limonciello wrote:

...

> >>>> Ok, so specifically the gpiod_set_debounce() call with 50 ms
> >>>> done by gpio_keys.c is the problem I guess?
> >>>
> >>> Yep.
> >>>
> >>>> So amd_gpio_set_debounce() does accept the 50 ms debounce
> >>>> passed to it by gpio_keys.c as a valid value and then setting
> >>>> that breaks the wake from suspend?
> >>>
> >>> That's right.
> > 
> >>>>> Also comparing the GPIO register in Windows (where things work)
> >>>>> Windows never programs a debounce.
> >>>>
> >>>> 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.
> > 
> > Thinking a bit more of this, perhaps the HW debounce support flag should be
> > per-GPIO-descriptor thingy. In such cases we don't need to distinguish the
> > platforms, the GPIO ACPI lib may simply set that flag based on 0 read from
> > the ACPI tables. It will also give a clue to any driver that uses GPIOs
> > (not only gpio-keys).
> 
> But 0 doesn't mean hardware debounce support is there, 0 means that 
> hardware debounce is not required to be programmed for this GPIO.
> 
> That is - if another system had a non-zero value in the GpioInt entry I 
> would expect this to be translated into the GPIO register.

Correct. The question is only about 0. So the flow will look like

1) if the GPIO is defined with 0 debounce, set the flag;
2) if the GPIO is defined with non-zero value, try to apply it;
3) if the step 2) fails, warn and set the flag.

Would it make sense?
Hans?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Limonciello, Mario]

On 6/25/25 10:17 AM, Andy Shevchenko wrote:
> On Wed, Jun 25, 2025 at 03:14:40PM +0000, Limonciello, Mario wrote:
>> On 6/25/25 10:10 AM, Andy Shevchenko wrote:
>>> On Wed, Jun 25, 2025 at 03:02:18PM +0000, Limonciello, Mario wrote:
>>>> On 6/25/25 9:41 AM, Mario Limonciello wrote:
>>>>> On 6/25/25 9:31 AM, Hans de Goede wrote:
>>>>>> On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
>>>>>>> On 6/25/25 4:09 AM, Hans de Goede wrote:
>>>>>>>> On 24-Jun-25 10:22 PM, Mario Limonciello wrote:
> 
> ...
> 
>>>>>> Ok, so specifically the gpiod_set_debounce() call with 50 ms
>>>>>> done by gpio_keys.c is the problem I guess?
>>>>>
>>>>> Yep.
>>>>>
>>>>>> So amd_gpio_set_debounce() does accept the 50 ms debounce
>>>>>> passed to it by gpio_keys.c as a valid value and then setting
>>>>>> that breaks the wake from suspend?
>>>>>
>>>>> That's right.
>>>
>>>>>>> Also comparing the GPIO register in Windows (where things work)
>>>>>>> Windows never programs a debounce.
>>>>>>
>>>>>> 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.
>>>
>>> Thinking a bit more of this, perhaps the HW debounce support flag should be
>>> per-GPIO-descriptor thingy. In such cases we don't need to distinguish the
>>> platforms, the GPIO ACPI lib may simply set that flag based on 0 read from
>>> the ACPI tables. It will also give a clue to any driver that uses GPIOs
>>> (not only gpio-keys).
>>
>> But 0 doesn't mean hardware debounce support is there, 0 means that
>> hardware debounce is not required to be programmed for this GPIO.
>>
>> That is - if another system had a non-zero value in the GpioInt entry I
>> would expect this to be translated into the GPIO register.
> 
> Correct. The question is only about 0. So the flow will look like
> 
> 1) if the GPIO is defined with 0 debounce, set the flag;
> 2) if the GPIO is defined with non-zero value, try to apply it;
> 3) if the step 2) fails, warn and set the flag.
> 
> Would it make sense?
> Hans?
> 

But so on these problematic BYT/CYT tablets which "layer" should be 
setting the 50ms debounce?
That should still be a quirk at the soc_button_array layer, right?

Because gpio_keys_setup_key() will already fallback to software 
debounce, and the goal here is that both of those only use the 50ms 
specifically with software debouncing.

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 03:34:55PM +0000, Limonciello, Mario wrote:
> On 6/25/25 10:17 AM, Andy Shevchenko wrote:
> > On Wed, Jun 25, 2025 at 03:14:40PM +0000, Limonciello, Mario wrote:
> >> On 6/25/25 10:10 AM, Andy Shevchenko wrote:
> >>> On Wed, Jun 25, 2025 at 03:02:18PM +0000, Limonciello, Mario wrote:
> >>>> On 6/25/25 9:41 AM, Mario Limonciello wrote:
> >>>>> On 6/25/25 9:31 AM, Hans de Goede wrote:
> >>>>>> On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
> >>>>>>> On 6/25/25 4:09 AM, Hans de Goede wrote:
> >>>>>>>> On 24-Jun-25 10:22 PM, Mario Limonciello wrote:

...

> >>>>>> Ok, so specifically the gpiod_set_debounce() call with 50 ms
> >>>>>> done by gpio_keys.c is the problem I guess?
> >>>>>
> >>>>> Yep.
> >>>>>
> >>>>>> So amd_gpio_set_debounce() does accept the 50 ms debounce
> >>>>>> passed to it by gpio_keys.c as a valid value and then setting
> >>>>>> that breaks the wake from suspend?
> >>>>>
> >>>>> That's right.
> >>>
> >>>>>>> Also comparing the GPIO register in Windows (where things work)
> >>>>>>> Windows never programs a debounce.
> >>>>>>
> >>>>>> 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.
> >>>
> >>> Thinking a bit more of this, perhaps the HW debounce support flag should be
> >>> per-GPIO-descriptor thingy. In such cases we don't need to distinguish the
> >>> platforms, the GPIO ACPI lib may simply set that flag based on 0 read from
> >>> the ACPI tables. It will also give a clue to any driver that uses GPIOs
> >>> (not only gpio-keys).
> >>
> >> But 0 doesn't mean hardware debounce support is there, 0 means that
> >> hardware debounce is not required to be programmed for this GPIO.
> >>
> >> That is - if another system had a non-zero value in the GpioInt entry I
> >> would expect this to be translated into the GPIO register.
> > 
> > Correct. The question is only about 0. So the flow will look like
> > 
> > 1) if the GPIO is defined with 0 debounce, set the flag;
> > 2) if the GPIO is defined with non-zero value, try to apply it;
> > 3) if the step 2) fails, warn and set the flag.
> > 
> > Would it make sense?
> > Hans?
> 
> But so on these problematic BYT/CYT tablets which "layer" should be 
> setting the 50ms debounce?
> That should still be a quirk at the soc_button_array layer, right?
> 
> Because gpio_keys_setup_key() will already fallback to software 
> debounce, and the goal here is that both of those only use the 50ms 
> specifically with software debouncing.

Probably gpiod_set_debounce() should become a no-op in this case?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Limonciello, Mario]

On 6/25/25 12:54 PM, Andy Shevchenko wrote:
> On Wed, Jun 25, 2025 at 03:34:55PM +0000, Limonciello, Mario wrote:
>> On 6/25/25 10:17 AM, Andy Shevchenko wrote:
>>> On Wed, Jun 25, 2025 at 03:14:40PM +0000, Limonciello, Mario wrote:
>>>> On 6/25/25 10:10 AM, Andy Shevchenko wrote:
>>>>> On Wed, Jun 25, 2025 at 03:02:18PM +0000, Limonciello, Mario wrote:
>>>>>> On 6/25/25 9:41 AM, Mario Limonciello wrote:
>>>>>>> On 6/25/25 9:31 AM, Hans de Goede wrote:
>>>>>>>> On 25-Jun-25 4:09 PM, Mario Limonciello wrote:
>>>>>>>>> On 6/25/25 4:09 AM, Hans de Goede wrote:
>>>>>>>>>> On 24-Jun-25 10:22 PM, Mario Limonciello wrote:
> 
> ...
> 
>>>>>>>> Ok, so specifically the gpiod_set_debounce() call with 50 ms
>>>>>>>> done by gpio_keys.c is the problem I guess?
>>>>>>>
>>>>>>> Yep.
>>>>>>>
>>>>>>>> So amd_gpio_set_debounce() does accept the 50 ms debounce
>>>>>>>> passed to it by gpio_keys.c as a valid value and then setting
>>>>>>>> that breaks the wake from suspend?
>>>>>>>
>>>>>>> That's right.
>>>>>
>>>>>>>>> Also comparing the GPIO register in Windows (where things work)
>>>>>>>>> Windows never programs a debounce.
>>>>>>>>
>>>>>>>> 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.
>>>>>
>>>>> Thinking a bit more of this, perhaps the HW debounce support flag should be
>>>>> per-GPIO-descriptor thingy. In such cases we don't need to distinguish the
>>>>> platforms, the GPIO ACPI lib may simply set that flag based on 0 read from
>>>>> the ACPI tables. It will also give a clue to any driver that uses GPIOs
>>>>> (not only gpio-keys).
>>>>
>>>> But 0 doesn't mean hardware debounce support is there, 0 means that
>>>> hardware debounce is not required to be programmed for this GPIO.
>>>>
>>>> That is - if another system had a non-zero value in the GpioInt entry I
>>>> would expect this to be translated into the GPIO register.
>>>
>>> Correct. The question is only about 0. So the flow will look like
>>>
>>> 1) if the GPIO is defined with 0 debounce, set the flag;
>>> 2) if the GPIO is defined with non-zero value, try to apply it;
>>> 3) if the step 2) fails, warn and set the flag.
>>>
>>> Would it make sense?
>>> Hans?
>>
>> But so on these problematic BYT/CYT tablets which "layer" should be
>> setting the 50ms debounce?
>> That should still be a quirk at the soc_button_array layer, right?
>>
>> Because gpio_keys_setup_key() will already fallback to software
>> debounce, and the goal here is that both of those only use the 50ms
>> specifically with software debouncing.
> 
> Probably gpiod_set_debounce() should become a no-op in this case?
> 

But my point is this 50 needs to be a quirk /somewhere/.  It shouldn't 
be a default behavior.

It can be in the GPIO driver(s), it can be in soc-button-array, or it 
can be in gpio_keys.

I've got an idea mocked up for a v2, I'll send that out and I think we 
can discuss the merits of it on that series.

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 05:59:50PM +0000, Limonciello, Mario wrote:
> On 6/25/25 12:54 PM, Andy Shevchenko wrote:
> > On Wed, Jun 25, 2025 at 03:34:55PM +0000, Limonciello, Mario wrote:
> >> On 6/25/25 10:17 AM, Andy Shevchenko wrote:

...

> > Probably gpiod_set_debounce() should become a no-op in this case?
> 
> But my point is this 50 needs to be a quirk /somewhere/.  It shouldn't 
> be a default behavior.
> 
> It can be in the GPIO driver(s), it can be in soc-button-array, or it 
> can be in gpio_keys.
> 
> I've got an idea mocked up for a v2, I'll send that out and I think we 
> can discuss the merits of it on that series.

Sure, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Hans de Goede]

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.

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.
> 
> 

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Mario Limonciello]

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.
>>
>>
> 

Re: [PATCH 2/2] Revert "Input: soc_button_array - debounce the buttons"

[Hans de Goede]

Hi Mario,

On 25-Jun-25 9:10 PM, Mario Limonciello wrote:
> 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?

Lets continue this discussion in the v2 thread.

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.
>>>
>>>
>>
>