[PATCH regression fix v2] gpiolib: acpi: Make set-debounce-timeout failures non fatal

Commit 8dcb7a15a585 ("gpiolib: acpi: Take into account debounce settings")
made the gpiolib-acpi code call gpio_set_debounce_timeout() when requesting
GPIOs.

This in itself is fine, but it also made gpio_set_debounce_timeout()
errors fatal, causing the requesting of the GPIO to fail. This is causing
regressions. E.g. on a HP ElitePad 1000 G2 various _AEI specified GPIO
ACPI event sources specify a debouncy timeout of 20 ms, but the
pinctrl-baytrail.c only supports certain fixed values, the closest
ones being 12 or 24 ms and pinctrl-baytrail.c responds with -EINVAL
when specified a value which is not one of the fixed values.

This is causing the acpi_request_own_gpiod() call to fail for 3
ACPI event sources on the HP ElitePad 1000 G2, which in turn is causing
e.g. the battery charging vs discharging status to never get updated,
even though a charger has been plugged-in or unplugged.

Make gpio_set_debounce_timeout() errors non fatal, warning about the
failure instead, to fix this regression.

Note we should probably also fix various pinctrl drivers to just
pick the first bigger discrete value rather then returning -EINVAL but
this will need to be done on a per driver basis, where as this fix
at least gets us back to where things were before and thus restores
functionality on devices where this was lost due to
gpio_set_debounce_timeout() errors.

Fixes: 8dcb7a15a585 ("gpiolib: acpi: Take into account debounce settings")
Depends-on: 2e2b496cebef ("gpiolib: acpi: Extract acpi_request_own_gpiod() helper")
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Hans de Goede <redacted>
---
Changes in v2:
-Fix typo in commit msg
-Add Mika's Reviewed-by
-Add Depends-on tag
---
 drivers/gpio/gpiolib-acpi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 411525ac4cc4..47712b6903b5 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c

@@ -313,9 +313,11 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 
 	ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout);
 	if (ret)
-		gpiochip_free_own_desc(desc);
+		dev_warn(chip->parent,
+			 "Failed to set debounce-timeout for pin 0x%04X, err %d\n",
+			 pin, ret);
 
-	return ret ? ERR_PTR(ret) : desc;
+	return desc;
 }
 
 static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)

-- 
2.31.1

On Mon, Aug 16, 2021 at 12:41:19PM +0200, Hans de Goede wrote:

Commit 8dcb7a15a585 ("gpiolib: acpi: Take into account debounce settings")
made the gpiolib-acpi code call gpio_set_debounce_timeout() when requesting
GPIOs.

This in itself is fine, but it also made gpio_set_debounce_timeout()
errors fatal, causing the requesting of the GPIO to fail. This is causing
regressions. E.g. on a HP ElitePad 1000 G2 various _AEI specified GPIO
ACPI event sources specify a debouncy timeout of 20 ms, but the
pinctrl-baytrail.c only supports certain fixed values, the closest
ones being 12 or 24 ms and pinctrl-baytrail.c responds with -EINVAL
when specified a value which is not one of the fixed values.

This is causing the acpi_request_own_gpiod() call to fail for 3
ACPI event sources on the HP ElitePad 1000 G2, which in turn is causing
e.g. the battery charging vs discharging status to never get updated,
even though a charger has been plugged-in or unplugged.

Make gpio_set_debounce_timeout() errors non fatal, warning about the
failure instead, to fix this regression.

Note we should probably also fix various pinctrl drivers to just
pick the first bigger discrete value rather then returning -EINVAL but
this will need to be done on a per driver basis, where as this fix
at least gets us back to where things were before and thus restores
functionality on devices where this was lost due to
gpio_set_debounce_timeout() errors.

Yes, I also think that we need to choose upper debounce instead of rejecting
the settings. And yes, I agree that for now it's not suitable as a fix.

That said,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Hi,

On 8/16/21 2:15 PM, Andy Shevchenko wrote:

On Mon, Aug 16, 2021 at 12:41:19PM +0200, Hans de Goede wrote:

quoted

Commit 8dcb7a15a585 ("gpiolib: acpi: Take into account debounce settings")
made the gpiolib-acpi code call gpio_set_debounce_timeout() when requesting
GPIOs.

This in itself is fine, but it also made gpio_set_debounce_timeout()
errors fatal, causing the requesting of the GPIO to fail. This is causing
regressions. E.g. on a HP ElitePad 1000 G2 various _AEI specified GPIO
ACPI event sources specify a debouncy timeout of 20 ms, but the
pinctrl-baytrail.c only supports certain fixed values, the closest
ones being 12 or 24 ms and pinctrl-baytrail.c responds with -EINVAL
when specified a value which is not one of the fixed values.

This is causing the acpi_request_own_gpiod() call to fail for 3
ACPI event sources on the HP ElitePad 1000 G2, which in turn is causing
e.g. the battery charging vs discharging status to never get updated,
even though a charger has been plugged-in or unplugged.

Make gpio_set_debounce_timeout() errors non fatal, warning about the
failure instead, to fix this regression.

Note we should probably also fix various pinctrl drivers to just
pick the first bigger discrete value rather then returning -EINVAL but
this will need to be done on a per driver basis, where as this fix
at least gets us back to where things were before and thus restores
functionality on devices where this was lost due to
gpio_set_debounce_timeout() errors.

Yes, I also think that we need to choose upper debounce instead of rejecting
the settings. And yes, I agree that for now it's not suitable as a fix.

That said,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you.

FYI, I've prepared a patch to choose the upper debounce time for
pintctrl-baytrail . I'll test it when I'm back home tonight and
then submit it upstream.

Regards,

Hans

quoted

Fixes: 8dcb7a15a585 ("gpiolib: acpi: Take into account debounce settings")
Depends-on: 2e2b496cebef ("gpiolib: acpi: Extract acpi_request_own_gpiod() helper")
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Hans de Goede <redacted>
---
Changes in v2:
-Fix typo in commit msg
-Add Mika's Reviewed-by
-Add Depends-on tag
---
 drivers/gpio/gpiolib-acpi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 411525ac4cc4..47712b6903b5 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c

@@ -313,9 +313,11 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 
 	ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout);
 	if (ret)
-		gpiochip_free_own_desc(desc);
+		dev_warn(chip->parent,
+			 "Failed to set debounce-timeout for pin 0x%04X, err %d\n",
+			 pin, ret);
 
-	return ret ? ERR_PTR(ret) : desc;
+	return desc;
 }
 
 static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)

-- 
2.31.1

On Mon, Aug 16, 2021 at 02:28:07PM +0200, Hans de Goede wrote:

On 8/16/21 2:15 PM, Andy Shevchenko wrote:

quoted

On Mon, Aug 16, 2021 at 12:41:19PM +0200, Hans de Goede wrote:

quoted

Commit 8dcb7a15a585 ("gpiolib: acpi: Take into account debounce settings")
made the gpiolib-acpi code call gpio_set_debounce_timeout() when requesting
GPIOs.

This in itself is fine, but it also made gpio_set_debounce_timeout()
errors fatal, causing the requesting of the GPIO to fail. This is causing
regressions. E.g. on a HP ElitePad 1000 G2 various _AEI specified GPIO
ACPI event sources specify a debouncy timeout of 20 ms, but the
pinctrl-baytrail.c only supports certain fixed values, the closest
ones being 12 or 24 ms and pinctrl-baytrail.c responds with -EINVAL
when specified a value which is not one of the fixed values.

This is causing the acpi_request_own_gpiod() call to fail for 3
ACPI event sources on the HP ElitePad 1000 G2, which in turn is causing
e.g. the battery charging vs discharging status to never get updated,
even though a charger has been plugged-in or unplugged.

Make gpio_set_debounce_timeout() errors non fatal, warning about the
failure instead, to fix this regression.

Note we should probably also fix various pinctrl drivers to just
pick the first bigger discrete value rather then returning -EINVAL but
this will need to be done on a per driver basis, where as this fix
at least gets us back to where things were before and thus restores
functionality on devices where this was lost due to
gpio_set_debounce_timeout() errors.

Yes, I also think that we need to choose upper debounce instead of rejecting
the settings. And yes, I agree that for now it's not suitable as a fix.

That said,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you.

FYI, I've prepared a patch to choose the upper debounce time for
pintctrl-baytrail . I'll test it when I'm back home tonight and
then submit it upstream.

Bart, can you pick this up? Or do you expect me to send a PR with this one?

-- 
With Best Regards,
Andy Shevchenko

Hi,

On 8/16/21 2:28 PM, Hans de Goede wrote:

Hi,

On 8/16/21 2:15 PM, Andy Shevchenko wrote:

quoted

On Mon, Aug 16, 2021 at 12:41:19PM +0200, Hans de Goede wrote:

quoted

Commit 8dcb7a15a585 ("gpiolib: acpi: Take into account debounce settings")
made the gpiolib-acpi code call gpio_set_debounce_timeout() when requesting
GPIOs.

This in itself is fine, but it also made gpio_set_debounce_timeout()
errors fatal, causing the requesting of the GPIO to fail. This is causing
regressions. E.g. on a HP ElitePad 1000 G2 various _AEI specified GPIO
ACPI event sources specify a debouncy timeout of 20 ms, but the
pinctrl-baytrail.c only supports certain fixed values, the closest
ones being 12 or 24 ms and pinctrl-baytrail.c responds with -EINVAL
when specified a value which is not one of the fixed values.

This is causing the acpi_request_own_gpiod() call to fail for 3
ACPI event sources on the HP ElitePad 1000 G2, which in turn is causing
e.g. the battery charging vs discharging status to never get updated,
even though a charger has been plugged-in or unplugged.

Make gpio_set_debounce_timeout() errors non fatal, warning about the
failure instead, to fix this regression.

Note we should probably also fix various pinctrl drivers to just
pick the first bigger discrete value rather then returning -EINVAL but
this will need to be done on a per driver basis, where as this fix
at least gets us back to where things were before and thus restores
functionality on devices where this was lost due to
gpio_set_debounce_timeout() errors.

Yes, I also think that we need to choose upper debounce instead of rejecting
the settings. And yes, I agree that for now it's not suitable as a fix.

That said,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you.

FYI, I've prepared a patch to choose the upper debounce time for
pintctrl-baytrail . I'll test it when I'm back home tonight and
then submit it upstream.

Ugh, I just noticed that this is still not upstream (not in v5.15-rc2), while this is
a regression fix!

Can we please get this merged and send to Linus ASAP ?

Regards,

Hans

quoted

quoted

Fixes: 8dcb7a15a585 ("gpiolib: acpi: Take into account debounce settings")
Depends-on: 2e2b496cebef ("gpiolib: acpi: Extract acpi_request_own_gpiod() helper")
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Hans de Goede <redacted>
---
Changes in v2:
-Fix typo in commit msg
-Add Mika's Reviewed-by
-Add Depends-on tag
---
 drivers/gpio/gpiolib-acpi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 411525ac4cc4..47712b6903b5 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c

@@ -313,9 +313,11 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 
 	ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout);
 	if (ret)
-		gpiochip_free_own_desc(desc);
+		dev_warn(chip->parent,
+			 "Failed to set debounce-timeout for pin 0x%04X, err %d\n",
+			 pin, ret);
 
-	return ret ? ERR_PTR(ret) : desc;
+	return desc;
 }
 
 static bool acpi_gpio_in_ignore_list(const char *controller_in, int pin_in)

-- 
2.31.1

On Mon, Sep 20, 2021 at 7:47 PM Hans de Goede [off-list ref] wrote:

On 8/16/21 2:28 PM, Hans de Goede wrote:

quoted

On 8/16/21 2:15 PM, Andy Shevchenko wrote:

quoted

On Mon, Aug 16, 2021 at 12:41:19PM +0200, Hans de Goede wrote:

...

quoted

quoted

Yes, I also think that we need to choose upper debounce instead of rejecting
the settings. And yes, I agree that for now it's not suitable as a fix.

That said,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you.

FYI, I've prepared a patch to choose the upper debounce time for
pintctrl-baytrail . I'll test it when I'm back home tonight and
then submit it upstream.

Ugh, I just noticed that this is still not upstream (not in v5.15-rc2), while this is
a regression fix!

I think it was late for Bart to pick this up.

Can we please get this merged and send to Linus ASAP ?

I'm on vacations now, but if Mika or Bart or Linus can handle this, my
tag is there :)

-- 
With Best Regards,
Andy Shevchenko