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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 8/16/21 2:28 PM, Hans de Goede wrote:
> 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:
>>> 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@xxxxxxxxxxxxxxx>
> 
> 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



>>> 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@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>> ---
>>> 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
>>>
>>