Hi Mario,
On 25-Jun-25 9:10 PM, Mario Limonciello wrote:
> On 6/25/25 1:57 PM, Hans de Goede wrote:
>> On 25-Jun-25 4:41 PM, Mario Limonciello wrote:
>>> On 6/25/25 9:31 AM, Hans de Goede wrote:
>>
>> <snip>
>>
>>>> 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.
>>>
>>> I think I would still like to see the ASL values translated into the hardware even if the ASL has a "0" value.
>>> So I would keep patch 1 but adjust for the warning you guys both called out.
>>>
>>> As you have this hardware would you be able to work out that quirk?
>>
>> I think we've a bit of miscommunication going on here.
>>
>> 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, which I believe will always be necessary when
>> dealing with mechanical buttons, but always use software debouncing
>> (which I suspect is what Windows does) to avoid issues like the issue
>> you are seeing.
>
> 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?
Lets continue this discussion in the v2 thread.
Regards,
Hans
>>> Or if you want me to do it, I'll need something to go on how to how to effectively detect BYT and CYT hardware.
>>>
>>>>
>>>>> So that's where both patches in this series came from.
>>>>>
>>>>>>
>>>>>> drivers/input/keyboard/gpio_keys.c first will call gpiod_set_debounce()
>>>>>> it self with the 50 ms provided by soc_button_array and if that does
>>>>>> not work it will fall back to software debouncing. So I don't see how
>>>>>> the 50 ms debounce can cause problems, other then maybe making
>>>>>> really really (impossible?) fast double-clicks register as a single
>>>>>> click .
>>>>>>
>>>>>> These buttons (e.g. volume up/down) are almost always simply mechanical
>>>>>> switches and these definitely will need debouncing, the 0 value from
>>>>>> the DSDT is plainly just wrong. There is no such thing as a not bouncing
>>>>>> mechanical switch.
>>>>>
>>>>> On one of these tablets can you check the GPIO in Windows to see if it's using any debounce?
>>>>
>>>> I'm afraid I don't have Windows installed on any of these.
>>>>
>>>> But based on your testing + the DSDT specifying no debounce
>>>> for the GPIO I guess Windows just follows the DSDt when it
>>>> comes to setting up the hw debounce-settings and then uses
>>>> sw-debouncing on top to actually avoid very quick
>>>> press-release-press event cycles caused by the bouncing.
>>>>
>>>
>>> Yeah that sounds like a plausible hypothesis.
>>>
>>>
>>
>
