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

["Limonciello, Mario" <Mario.Limonciello@amd.com>]

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

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

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

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