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.
