[PATCH v2 0/3] Fix soc-button-array debounce

[PATCH v2 0/3] 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 sets the hardcoded
value on on some quirked systems.  Hopefully Hans can confirm this
continues to work on the hardware that he originally developed the
hardcoding for.

---
v2:
 * Add a helper for making ACPI debounce program nonfatal
 * Use a quirk instead of a revert

Mario Limonciello (3):
  gpiolib: acpi: Add a helper for programming debounce
  gpiolib: acpi: Program debounce when finding GPIO
  Input: soc_button_array: Only debounce cherryview and baytrail systems

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

-- 
2.43.0

[PATCH v2 1/3] gpiolib: acpi: Add a helper for programming debounce

[Mario Limonciello]

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

Debounce is programmed in two places and considered non-fatal in one of
them. Introduce a helper for programming debounce and show a warning
when failing to program.  This is a difference in behavior for the call
in acpi_dev_gpio_irq_wake_get_by().

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

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index 12b24a717e43f..1895e45bd9f16 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -291,6 +291,17 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio, int polarity)
 	return GPIOD_ASIS;
 }
 
+static void acpi_set_debounce_timeout(struct gpio_desc *desc, unsigned int timeout)
+{
+	int ret;
+
+	/* ACPI uses hundredths of milliseconds units */
+	ret = gpio_set_debounce_timeout(desc, timeout * 10);
+	if (ret)
+		dev_warn(&desc->gdev->dev,
+			 "Failed to set debounce-timeout: %d\n", ret);
+}
+
 static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 						struct acpi_resource_gpio *agpio,
 						unsigned int index,
@@ -300,18 +311,12 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 	enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio, polarity);
 	unsigned int pin = agpio->pin_table[index];
 	struct gpio_desc *desc;
-	int ret;
 
 	desc = gpiochip_request_own_desc(chip, pin, label, polarity, flags);
 	if (IS_ERR(desc))
 		return desc;
 
-	/* 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);
+	acpi_set_debounce_timeout(desc, agpio->debounce_timeout);
 
 	return desc;
 }
@@ -1025,10 +1030,7 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
 			if (ret < 0)
 				return ret;
 
-			/* ACPI uses hundredths of milliseconds units */
-			ret = gpio_set_debounce_timeout(desc, info.debounce * 10);
-			if (ret)
-				return ret;
+			acpi_set_debounce_timeout(desc, info.debounce);
 
 			irq_flags = acpi_dev_get_irq_type(info.triggering,
 							  info.polarity);
-- 
2.43.0

Re: [PATCH v2 1/3] gpiolib: acpi: Add a helper for programming debounce

[Hans de Goede]

Hi,

On 25-Jun-25 8:13 PM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> Debounce is programmed in two places and considered non-fatal in one of
> them. Introduce a helper for programming debounce and show a warning
> when failing to program.  This is a difference in behavior for the call
> in acpi_dev_gpio_irq_wake_get_by().
> 
> Cc: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans



> ---
>  drivers/gpio/gpiolib-acpi-core.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
> index 12b24a717e43f..1895e45bd9f16 100644
> --- a/drivers/gpio/gpiolib-acpi-core.c
> +++ b/drivers/gpio/gpiolib-acpi-core.c
> @@ -291,6 +291,17 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio, int polarity)
>  	return GPIOD_ASIS;
>  }
>  
> +static void acpi_set_debounce_timeout(struct gpio_desc *desc, unsigned int timeout)
> +{
> +	int ret;
> +
> +	/* ACPI uses hundredths of milliseconds units */
> +	ret = gpio_set_debounce_timeout(desc, timeout * 10);
> +	if (ret)
> +		dev_warn(&desc->gdev->dev,
> +			 "Failed to set debounce-timeout: %d\n", ret);
> +}
> +
>  static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
>  						struct acpi_resource_gpio *agpio,
>  						unsigned int index,
> @@ -300,18 +311,12 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
>  	enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio, polarity);
>  	unsigned int pin = agpio->pin_table[index];
>  	struct gpio_desc *desc;
> -	int ret;
>  
>  	desc = gpiochip_request_own_desc(chip, pin, label, polarity, flags);
>  	if (IS_ERR(desc))
>  		return desc;
>  
> -	/* 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);
> +	acpi_set_debounce_timeout(desc, agpio->debounce_timeout);
>  
>  	return desc;
>  }
> @@ -1025,10 +1030,7 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
>  			if (ret < 0)
>  				return ret;
>  
> -			/* ACPI uses hundredths of milliseconds units */
> -			ret = gpio_set_debounce_timeout(desc, info.debounce * 10);
> -			if (ret)
> -				return ret;
> +			acpi_set_debounce_timeout(desc, info.debounce);
>  
>  			irq_flags = acpi_dev_get_irq_type(info.triggering,
>  							  info.polarity);

[PATCH v2 2/3] 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>
---
v2:
 * Make non fatal by using helper from patch 1 (Andy)
---
 drivers/gpio/gpiolib-acpi-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index 1895e45bd9f16..15222bfc25bb2 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -962,6 +962,7 @@ 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);
+	acpi_set_debounce_timeout(desc, info.debounce);
 	return desc;
 }
 
-- 
2.43.0

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

[Hans de Goede]

Hi,

On 25-Jun-25 8:13 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>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans



> ---
> v2:
>  * Make non fatal by using helper from patch 1 (Andy)
> ---
>  drivers/gpio/gpiolib-acpi-core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
> index 1895e45bd9f16..15222bfc25bb2 100644
> --- a/drivers/gpio/gpiolib-acpi-core.c
> +++ b/drivers/gpio/gpiolib-acpi-core.c
> @@ -962,6 +962,7 @@ 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);
> +	acpi_set_debounce_timeout(desc, info.debounce);
>  	return desc;
>  }
>  

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

[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
}

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>
---
v2:
 * commit message
 * quirk systems instead of revert
---
 drivers/input/misc/soc_button_array.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index b8cad415c62ca..56abc93f23e0c 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -129,6 +129,13 @@ static const struct dmi_system_id dmi_invalid_acpi_index[] = {
 	{} /* Terminating entry */
 };
 
+static const struct acpi_device_id force_software_debounce_ids[] = {
+	{ "INT33FF" },
+	{ "INT33B2" },
+	{ "INT33FC" },
+	{ }
+};
+
 /*
  * Get the Nth GPIO number from the ACPI object.
  */
@@ -149,11 +156,17 @@ static int soc_button_lookup_gpio(struct device *dev, int acpi_index,
 	return 0;
 }
 
+static int soc_button_match_acpi_device_ids(struct device *dev, const void *data)
+{
+	return acpi_match_device(data, dev) ? 1 : 0;
+}
+
 static struct platform_device *
 soc_button_device_create(struct platform_device *pdev,
 			 const struct soc_button_info *button_info,
 			 bool autorepeat)
 {
+	struct device *quirkdev __free(put_device) = NULL;
 	const struct soc_button_info *info;
 	struct platform_device *pd;
 	struct gpio_keys_button *gpio_keys;
@@ -181,6 +194,10 @@ soc_button_device_create(struct platform_device *pdev,
 	if (dmi_id)
 		invalid_acpi_index = (long)dmi_id->driver_data;
 
+	quirkdev = bus_find_device(&platform_bus_type, NULL,
+				   force_software_debounce_ids,
+				   soc_button_match_acpi_device_ids);
+
 	for (info = button_info; info->name; info++) {
 		if (info->autorepeat != autorepeat)
 			continue;
@@ -220,7 +237,8 @@ soc_button_device_create(struct platform_device *pdev,
 		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;
+		if (quirkdev)
+			gpio_keys[n_buttons].debounce_interval = 50;
 		n_buttons++;
 	}
 
-- 
2.43.0

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

[Hans de Goede]

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




> ---
> v2:
>  * commit message
>  * quirk systems instead of revert
> ---
>  drivers/input/misc/soc_button_array.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> index b8cad415c62ca..56abc93f23e0c 100644
> --- a/drivers/input/misc/soc_button_array.c
> +++ b/drivers/input/misc/soc_button_array.c
> @@ -129,6 +129,13 @@ static const struct dmi_system_id dmi_invalid_acpi_index[] = {
>  	{} /* Terminating entry */
>  };
>  
> +static const struct acpi_device_id force_software_debounce_ids[] = {
> +	{ "INT33FF" },
> +	{ "INT33B2" },
> +	{ "INT33FC" },
> +	{ }
> +};
> +
>  /*
>   * Get the Nth GPIO number from the ACPI object.
>   */
> @@ -149,11 +156,17 @@ static int soc_button_lookup_gpio(struct device *dev, int acpi_index,
>  	return 0;
>  }
>  
> +static int soc_button_match_acpi_device_ids(struct device *dev, const void *data)
> +{
> +	return acpi_match_device(data, dev) ? 1 : 0;
> +}
> +
>  static struct platform_device *
>  soc_button_device_create(struct platform_device *pdev,
>  			 const struct soc_button_info *button_info,
>  			 bool autorepeat)
>  {
> +	struct device *quirkdev __free(put_device) = NULL;
>  	const struct soc_button_info *info;
>  	struct platform_device *pd;
>  	struct gpio_keys_button *gpio_keys;
> @@ -181,6 +194,10 @@ soc_button_device_create(struct platform_device *pdev,
>  	if (dmi_id)
>  		invalid_acpi_index = (long)dmi_id->driver_data;
>  
> +	quirkdev = bus_find_device(&platform_bus_type, NULL,
> +				   force_software_debounce_ids,
> +				   soc_button_match_acpi_device_ids);
> +
>  	for (info = button_info; info->name; info++) {
>  		if (info->autorepeat != autorepeat)
>  			continue;
> @@ -220,7 +237,8 @@ soc_button_device_create(struct platform_device *pdev,
>  		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;
> +		if (quirkdev)
> +			gpio_keys[n_buttons].debounce_interval = 50;
>  		n_buttons++;
>  	}
>  

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

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

[Hans de Goede]

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

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

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

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

[Dmitry Torokhov]

On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
> 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@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.
> > 
> > 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 am confused, can you explain why do we need this new no_hw_debounce
flag? If AMD gpio driver is unable to program 50 ms debounce for a given
pin but does not return an error (or returns an error but leaves system
in a bad state) that is the issue in that driver and needs to be fixed
there? Why do we need to change soc_button_driver at all?

Thanks.

-- 
Dmitry

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

[Mario Limonciello]

On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
> On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
>> 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@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.
>>>
>>> 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 am confused, can you explain why do we need this new no_hw_debounce
> flag? If AMD gpio driver is unable to program 50 ms debounce for a given
> pin but does not return an error (or returns an error but leaves system
> in a bad state) that is the issue in that driver and needs to be fixed
> there? Why do we need to change soc_button_driver at all?
> 
> Thanks.
> 

The requested 50ms HW debounce gets programmed to the hardware register 
successfully.  It is within bound that the GPIO controller can support.

The problem is the power button does not function with a 50ms debounce.
The firmware asserted that 0ms should have been programmed (by the _CRS 
value in GpioInt).

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

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote:
> On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
> > On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
> > > 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@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.
> > > > 
> > > > 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 am confused, can you explain why do we need this new no_hw_debounce
> > flag? If AMD gpio driver is unable to program 50 ms debounce for a given
> > pin but does not return an error (or returns an error but leaves system
> > in a bad state) that is the issue in that driver and needs to be fixed
> > there? Why do we need to change soc_button_driver at all?
> > 
> > Thanks.
> > 
> 
> The requested 50ms HW debounce gets programmed to the hardware register
> successfully.  It is within bound that the GPIO controller can support.
> 
> The problem is the power button does not function with a 50ms debounce.
> The firmware asserted that 0ms should have been programmed (by the _CRS
> value in GpioInt).

I do not understand how debounce that is within the controller's
supported range can not work. The button is a switch that reports on and
off, there is nothing more to it, is there?

I feel there is a deeper problem that we simply trying to paper over.

-- 
Dmitry

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

[Mario Limonciello]

On 6/26/2025 1:53 PM, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote:
>> On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
>>> On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
>>>> 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@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.
>>>>>
>>>>> 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 am confused, can you explain why do we need this new no_hw_debounce
>>> flag? If AMD gpio driver is unable to program 50 ms debounce for a given
>>> pin but does not return an error (or returns an error but leaves system
>>> in a bad state) that is the issue in that driver and needs to be fixed
>>> there? Why do we need to change soc_button_driver at all?
>>>
>>> Thanks.
>>>
>>
>> The requested 50ms HW debounce gets programmed to the hardware register
>> successfully.  It is within bound that the GPIO controller can support.
>>
>> The problem is the power button does not function with a 50ms debounce.
>> The firmware asserted that 0ms should have been programmed (by the _CRS
>> value in GpioInt).
> 
> I do not understand how debounce that is within the controller's
> supported range can not work. The button is a switch that reports on and
> off, there is nothing more to it, is there?
> 
> I feel there is a deeper problem that we simply trying to paper over.
> 

I never said the power button is a simple mechanical switch connected to 
the GPIO.

A lot of designs I've seen have the EC connected to the power button 
GPIO and the EC does it's own debouncing of a physical switch to decide 
to assert GPIO 0.

My point here is if the firmware is asserting that there shouldn't be a 
debounce programmed, programming one can lead to problems.

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

[Hans de Goede]

Hi Dmitry,

On 26-Jun-25 20:53, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote:
>> On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
>>> On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
>>>> 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@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.
>>>>>
>>>>> 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 am confused, can you explain why do we need this new no_hw_debounce
>>> flag? If AMD gpio driver is unable to program 50 ms debounce for a given
>>> pin but does not return an error (or returns an error but leaves system
>>> in a bad state) that is the issue in that driver and needs to be fixed
>>> there? Why do we need to change soc_button_driver at all?
>>>
>>> Thanks.
>>>
>>
>> The requested 50ms HW debounce gets programmed to the hardware register
>> successfully.  It is within bound that the GPIO controller can support.
>>
>> The problem is the power button does not function with a 50ms debounce.
>> The firmware asserted that 0ms should have been programmed (by the _CRS
>> value in GpioInt).
> 
> I do not understand how debounce that is within the controller's
> supported range can not work. The button is a switch that reports on and
> off, there is nothing more to it, is there?
> 
> I feel there is a deeper problem that we simply trying to paper over.

Note that on x86 wakeup events and GPIO IRQs typically use a different
event mechanism / path under the hood (PME events to resume from suspend).
It is not just a case of marking the IRQ used while running as a wakeup
source.

So it is possible that setting the hw-debouncing is in some way interfering
with the reporting of x86 PME events while the system is suspended.

Most systems where soc_button_array is used don't support hw-debouncing
in the first place, so on most systems this change is a no-op.

As such I'm in favor of the fix for this from the v3 series to disable
hw-debouncing in gpio_keys.c when used from soc_button_array.c code.

Regards,

Hans

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

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 09:04:22PM +0200, Hans de Goede wrote:
> Hi Dmitry,
> 
> On 26-Jun-25 20:53, Dmitry Torokhov wrote:
> > On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote:
> >> On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
> >>> On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
> >>>> 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@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.
> >>>>>
> >>>>> 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 am confused, can you explain why do we need this new no_hw_debounce
> >>> flag? If AMD gpio driver is unable to program 50 ms debounce for a given
> >>> pin but does not return an error (or returns an error but leaves system
> >>> in a bad state) that is the issue in that driver and needs to be fixed
> >>> there? Why do we need to change soc_button_driver at all?
> >>>
> >>> Thanks.
> >>>
> >>
> >> The requested 50ms HW debounce gets programmed to the hardware register
> >> successfully.  It is within bound that the GPIO controller can support.
> >>
> >> The problem is the power button does not function with a 50ms debounce.
> >> The firmware asserted that 0ms should have been programmed (by the _CRS
> >> value in GpioInt).
> > 
> > I do not understand how debounce that is within the controller's
> > supported range can not work. The button is a switch that reports on and
> > off, there is nothing more to it, is there?
> > 
> > I feel there is a deeper problem that we simply trying to paper over.
> 
> Note that on x86 wakeup events and GPIO IRQs typically use a different
> event mechanism / path under the hood (PME events to resume from suspend).
> It is not just a case of marking the IRQ used while running as a wakeup
> source.
> 
> So it is possible that setting the hw-debouncing is in some way interfering
> with the reporting of x86 PME events while the system is suspended.

Still this looks like platform issue, not driver issue. Should GPIO
driver refuse programming debounce if pin is configured as potential
wakeup source then?

> 
> Most systems where soc_button_array is used don't support hw-debouncing
> in the first place, so on most systems this change is a no-op.

The change is not limited to soc_button_array driver, we need to add
flags to gpio_keys as well... 

Thanks.

-- 
Dmitry

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

[Mario Limonciello]

On 6/26/2025 2:32 PM, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 09:04:22PM +0200, Hans de Goede wrote:
>> Hi Dmitry,
>>
>> On 26-Jun-25 20:53, Dmitry Torokhov wrote:
>>> On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote:
>>>> On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
>>>>> On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
>>>>>> 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@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.
>>>>>>>
>>>>>>> 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 am confused, can you explain why do we need this new no_hw_debounce
>>>>> flag? If AMD gpio driver is unable to program 50 ms debounce for a given
>>>>> pin but does not return an error (or returns an error but leaves system
>>>>> in a bad state) that is the issue in that driver and needs to be fixed
>>>>> there? Why do we need to change soc_button_driver at all?
>>>>>
>>>>> Thanks.
>>>>>
>>>>
>>>> The requested 50ms HW debounce gets programmed to the hardware register
>>>> successfully.  It is within bound that the GPIO controller can support.
>>>>
>>>> The problem is the power button does not function with a 50ms debounce.
>>>> The firmware asserted that 0ms should have been programmed (by the _CRS
>>>> value in GpioInt).
>>>
>>> I do not understand how debounce that is within the controller's
>>> supported range can not work. The button is a switch that reports on and
>>> off, there is nothing more to it, is there?
>>>
>>> I feel there is a deeper problem that we simply trying to paper over.
>>
>> Note that on x86 wakeup events and GPIO IRQs typically use a different
>> event mechanism / path under the hood (PME events to resume from suspend).
>> It is not just a case of marking the IRQ used while running as a wakeup
>> source.
>>
>> So it is possible that setting the hw-debouncing is in some way interfering
>> with the reporting of x86 PME events while the system is suspended.
> 
> Still this looks like platform issue, not driver issue. Should GPIO
> driver refuse programming debounce if pin is configured as potential
> wakeup source then?

How can the driver intricate details about the hardware connected to the 
GPIO and how it behaves?

The driver is "dumb" and programs the registers according to the calls 
given to it.

> 
>>
>> Most systems where soc_button_array is used don't support hw-debouncing
>> in the first place, so on most systems this change is a no-op.
> 
> The change is not limited to soc_button_array driver, we need to add
> flags to gpio_keys as well...

That's exactly what the patch v3 3/4 is doing.

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

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 02:37:19PM -0500, Mario Limonciello wrote:
> On 6/26/2025 2:32 PM, Dmitry Torokhov wrote:
> > On Thu, Jun 26, 2025 at 09:04:22PM +0200, Hans de Goede wrote:
> > > Hi Dmitry,
> > > 
> > > On 26-Jun-25 20:53, Dmitry Torokhov wrote:
> > > > On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote:
> > > > > On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
> > > > > > On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
> > > > > > > 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@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.
> > > > > > > > 
> > > > > > > > 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 am confused, can you explain why do we need this new no_hw_debounce
> > > > > > flag? If AMD gpio driver is unable to program 50 ms debounce for a given
> > > > > > pin but does not return an error (or returns an error but leaves system
> > > > > > in a bad state) that is the issue in that driver and needs to be fixed
> > > > > > there? Why do we need to change soc_button_driver at all?
> > > > > > 
> > > > > > Thanks.
> > > > > > 
> > > > > 
> > > > > The requested 50ms HW debounce gets programmed to the hardware register
> > > > > successfully.  It is within bound that the GPIO controller can support.
> > > > > 
> > > > > The problem is the power button does not function with a 50ms debounce.
> > > > > The firmware asserted that 0ms should have been programmed (by the _CRS
> > > > > value in GpioInt).
> > > > 
> > > > I do not understand how debounce that is within the controller's
> > > > supported range can not work. The button is a switch that reports on and
> > > > off, there is nothing more to it, is there?
> > > > 
> > > > I feel there is a deeper problem that we simply trying to paper over.
> > > 
> > > Note that on x86 wakeup events and GPIO IRQs typically use a different
> > > event mechanism / path under the hood (PME events to resume from suspend).
> > > It is not just a case of marking the IRQ used while running as a wakeup
> > > source.
> > > 
> > > So it is possible that setting the hw-debouncing is in some way interfering
> > > with the reporting of x86 PME events while the system is suspended.
> > 
> > Still this looks like platform issue, not driver issue. Should GPIO
> > driver refuse programming debounce if pin is configured as potential
> > wakeup source then?
> 
> How can the driver intricate details about the hardware connected to the
> GPIO and how it behaves?
> 
> The driver is "dumb" and programs the registers according to the calls given
> to it.

I do not think driver needs to know details of hardware connected to
GPIO. I know you mentioned that it may be connected to an EC that does
its own debouncing, however this should make no difference from AP
standpoint: you are still dealing with a GPIO line and your GPIO
controller does debouncing for that line (which may be unnecessary
because the line will not be bouncing).

Hans mentioned that it is possible that this debounce interferes with
the platform reporting wakeup events. I can easily believe that. But
that means that if there is another peripheral device similarly attached
to such GPIO configured for wakeup, device that does not use gpio_keys
driver, it will have the same issue. And I wonder if the solution is for
your GPIO provider driver to refuse programming debounce for GPIOs that
are marked as wake capable.

> 
> > 
> > > 
> > > Most systems where soc_button_array is used don't support hw-debouncing
> > > in the first place, so on most systems this change is a no-op.
> > 
> > The change is not limited to soc_button_array driver, we need to add
> > flags to gpio_keys as well...
> 
> That's exactly what the patch v3 3/4 is doing.

What I was trying to say is that we are not only touching
soc_button_array driver but also have to make changes to more generic
gpio_keys driver which I would like to avoid if possible.

Thanks.

-- 
Dmitry

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

[Mario Limonciello]

On 6/26/2025 2:54 PM, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 02:37:19PM -0500, Mario Limonciello wrote:
>> On 6/26/2025 2:32 PM, Dmitry Torokhov wrote:
>>> On Thu, Jun 26, 2025 at 09:04:22PM +0200, Hans de Goede wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 26-Jun-25 20:53, Dmitry Torokhov wrote:
>>>>> On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote:
>>>>>> On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
>>>>>>> On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
>>>>>>>> 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@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.
>>>>>>>>>
>>>>>>>>> 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 am confused, can you explain why do we need this new no_hw_debounce
>>>>>>> flag? If AMD gpio driver is unable to program 50 ms debounce for a given
>>>>>>> pin but does not return an error (or returns an error but leaves system
>>>>>>> in a bad state) that is the issue in that driver and needs to be fixed
>>>>>>> there? Why do we need to change soc_button_driver at all?
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>
>>>>>> The requested 50ms HW debounce gets programmed to the hardware register
>>>>>> successfully.  It is within bound that the GPIO controller can support.
>>>>>>
>>>>>> The problem is the power button does not function with a 50ms debounce.
>>>>>> The firmware asserted that 0ms should have been programmed (by the _CRS
>>>>>> value in GpioInt).
>>>>>
>>>>> I do not understand how debounce that is within the controller's
>>>>> supported range can not work. The button is a switch that reports on and
>>>>> off, there is nothing more to it, is there?
>>>>>
>>>>> I feel there is a deeper problem that we simply trying to paper over.
>>>>
>>>> Note that on x86 wakeup events and GPIO IRQs typically use a different
>>>> event mechanism / path under the hood (PME events to resume from suspend).
>>>> It is not just a case of marking the IRQ used while running as a wakeup
>>>> source.
>>>>
>>>> So it is possible that setting the hw-debouncing is in some way interfering
>>>> with the reporting of x86 PME events while the system is suspended.
>>>
>>> Still this looks like platform issue, not driver issue. Should GPIO
>>> driver refuse programming debounce if pin is configured as potential
>>> wakeup source then?
>>
>> How can the driver intricate details about the hardware connected to the
>> GPIO and how it behaves?
>>
>> The driver is "dumb" and programs the registers according to the calls given
>> to it.
> 
> I do not think driver needs to know details of hardware connected to
> GPIO. I know you mentioned that it may be connected to an EC that does
> its own debouncing, however this should make no difference from AP
> standpoint: you are still dealing with a GPIO line and your GPIO
> controller does debouncing for that line (which may be unnecessary
> because the line will not be bouncing).
> 
> Hans mentioned that it is possible that this debounce interferes with
> the platform reporting wakeup events. I can easily believe that. But
> that means that if there is another peripheral device similarly attached
> to such GPIO configured for wakeup, device that does not use gpio_keys
> driver, it will have the same issue. And I wonder if the solution is for
> your GPIO provider driver to refuse programming debounce for GPIOs that
> are marked as wake capable.

Hmm; how would we guarantee there was no regressions for other systems 
with such a heuristics change?

IE what if there is a system that doesn't use soc-button-array and has a 
non-zero debounce value in the GpioInt from _AEI() and is wake capable? 
"Today" that would be programmed to the GPIO register.

If we're aiming for a driver specific heuristic another alternative is 
to clear debounce for everything at suspend and restore it at resume.

I actually did that and it seems to work fine on this affected system, 
so I'll draft it up as an alternative to patch v3 3/4.

> 
>>
>>>
>>>>
>>>> Most systems where soc_button_array is used don't support hw-debouncing
>>>> in the first place, so on most systems this change is a no-op.
>>>
>>> The change is not limited to soc_button_array driver, we need to add
>>> flags to gpio_keys as well...
>>
>> That's exactly what the patch v3 3/4 is doing.
> 
> What I was trying to say is that we are not only touching
> soc_button_array driver but also have to make changes to more generic
> gpio_keys driver which I would like to avoid if possible.
> 
> Thanks.
> 

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

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 04:39:34PM -0500, Mario Limonciello wrote:
> On 6/26/2025 2:54 PM, Dmitry Torokhov wrote:
> > On Thu, Jun 26, 2025 at 02:37:19PM -0500, Mario Limonciello wrote:
> > > On 6/26/2025 2:32 PM, Dmitry Torokhov wrote:
> > > > On Thu, Jun 26, 2025 at 09:04:22PM +0200, Hans de Goede wrote:
> > > > > Hi Dmitry,
> > > > > 
> > > > > On 26-Jun-25 20:53, Dmitry Torokhov wrote:
> > > > > > On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote:
> > > > > > > On 6/26/2025 1:27 PM, Dmitry Torokhov wrote:
> > > > > > > > On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote:
> > > > > > > > > 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@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.
> > > > > > > > > > 
> > > > > > > > > > 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 am confused, can you explain why do we need this new no_hw_debounce
> > > > > > > > flag? If AMD gpio driver is unable to program 50 ms debounce for a given
> > > > > > > > pin but does not return an error (or returns an error but leaves system
> > > > > > > > in a bad state) that is the issue in that driver and needs to be fixed
> > > > > > > > there? Why do we need to change soc_button_driver at all?
> > > > > > > > 
> > > > > > > > Thanks.
> > > > > > > > 
> > > > > > > 
> > > > > > > The requested 50ms HW debounce gets programmed to the hardware register
> > > > > > > successfully.  It is within bound that the GPIO controller can support.
> > > > > > > 
> > > > > > > The problem is the power button does not function with a 50ms debounce.
> > > > > > > The firmware asserted that 0ms should have been programmed (by the _CRS
> > > > > > > value in GpioInt).
> > > > > > 
> > > > > > I do not understand how debounce that is within the controller's
> > > > > > supported range can not work. The button is a switch that reports on and
> > > > > > off, there is nothing more to it, is there?
> > > > > > 
> > > > > > I feel there is a deeper problem that we simply trying to paper over.
> > > > > 
> > > > > Note that on x86 wakeup events and GPIO IRQs typically use a different
> > > > > event mechanism / path under the hood (PME events to resume from suspend).
> > > > > It is not just a case of marking the IRQ used while running as a wakeup
> > > > > source.
> > > > > 
> > > > > So it is possible that setting the hw-debouncing is in some way interfering
> > > > > with the reporting of x86 PME events while the system is suspended.
> > > > 
> > > > Still this looks like platform issue, not driver issue. Should GPIO
> > > > driver refuse programming debounce if pin is configured as potential
> > > > wakeup source then?
> > > 
> > > How can the driver intricate details about the hardware connected to the
> > > GPIO and how it behaves?
> > > 
> > > The driver is "dumb" and programs the registers according to the calls given
> > > to it.
> > 
> > I do not think driver needs to know details of hardware connected to
> > GPIO. I know you mentioned that it may be connected to an EC that does
> > its own debouncing, however this should make no difference from AP
> > standpoint: you are still dealing with a GPIO line and your GPIO
> > controller does debouncing for that line (which may be unnecessary
> > because the line will not be bouncing).
> > 
> > Hans mentioned that it is possible that this debounce interferes with
> > the platform reporting wakeup events. I can easily believe that. But
> > that means that if there is another peripheral device similarly attached
> > to such GPIO configured for wakeup, device that does not use gpio_keys
> > driver, it will have the same issue. And I wonder if the solution is for
> > your GPIO provider driver to refuse programming debounce for GPIOs that
> > are marked as wake capable.
> 
> Hmm; how would we guarantee there was no regressions for other systems with
> such a heuristics change?
> 
> IE what if there is a system that doesn't use soc-button-array and has a
> non-zero debounce value in the GpioInt from _AEI() and is wake capable?
> "Today" that would be programmed to the GPIO register.

That is exactly what I am saying. I am asserting that if there is
another system that does not use soc-button-array (or rather gpio-keys)
and it programs GPIO debounce for GPIO that is used for wakeup then on
the same platform it will be similarly broken. It is not GPIO-consumer
driver specific, it is platform/GPIO provider specific behavior.

> 
> If we're aiming for a driver specific heuristic another alternative is to
> clear debounce for everything at suspend and restore it at resume.
> 
> I actually did that and it seems to work fine on this affected system, so
> I'll draft it up as an alternative to patch v3 3/4.

I guess this also an option...

Thanks.

-- 
Dmitry