Re: [PATCH] pinctrl: baytrail: Pick first supported debounce value larger then the requested value

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 1/13/22 12:56, Hans de Goede wrote:
> Hi,
> 
> On 1/13/22 12:31, Andy Shevchenko wrote:
>> On Mon, Aug 23, 2021 at 12:32:18PM +0200, Hans de Goede wrote:
>>> On 8/20/21 3:11 PM, Andy Shevchenko wrote:
>>>> On Thu, Aug 19, 2021 at 10:39:52PM +0200, Hans de Goede wrote:
>>>>> Bay Trail pin control only supports a couple of discrete debounce timeout
>>>>> values. Instead of returning -EINVAL for other values, pick the first
>>>>> supported debounce timeout value larger then the requested timeout.
>>>>>
>>>>> E.g. on a HP ElitePad 1000 G2, where the ACPI tables specify a timeout of
>>>>> 20 ms for various _AIE ACPI event sources, this will result in a timeout
>>>>> of 24 ms instead of returning -EINVAL to the caller.
>>
>> Old thread which I forgot to answer to...
> 
> No problem I did happen to accidentally do some work on this over
> the Christmas Holidays and made some notes.
> 
> Spoiler: I believe the best thing we can do here is to just
> never touch the debounce settings at all, see below.
> 
>>>> I would prefer to see case 1...375: case 376...750: It makes it more explicit
>>>> what we choose. Also it will be in align with debounce getter switch-case.
>>>
>>> Ok, that works for me.
>>>
>>>> Nevertheless, there is the bigger problem here, which is that the debounce
>>>> setting is global per community and neither current nor new code addresses.
>>>>
>>>> What we need is to have a bitmap of size 3-bit * N_pins (per community) to save
>>>> settings and each time we try to adjust them, we have to go through that bitmap
>>>> and check actual users and see if we have conflicting requests.
>>>>
>>>> You may ask why we have to keep the actual debounce value and not the biggest
>>>> one. The reason why is simple, if the following flow of requests comes we will
>>>> have better setting at the end:
>>>>
>>>> 1) A asks for debounce of 1ms (we set to 1.5ms)
>>>> 2) B asks for 10ms (we set likely to 12ms *)
>>>> 3) B gone (we should return to 1.5ms)
>>>> 4) C asks for 400us (*)
>>>>
>>>> *) TBH I have no idea what to do with these cases, i.e. when better to satisfy
>>>>    the bigger, when issue warning, when just print a debug message. I admit
>>>>    that debounce on BYT has been not thought through on HW level.
>>>
>>> I believe that most DSDTs only use 1 value, so the whole bitmap thing seems
>>> over-complicated.
>>
>> Since you are in possession of plenty of them, can you confirm?
> 
> I could go look at this, yes, but I would need to make some time
> for that. And since last time I'm not so sure this is the case
> anymore, from the top of my head most DSDTs don't use this at all
> and those that do use nice round numbers, not the very specific
> supported values which suggests that the DSDT authors are not
> aware that all the pins in a single community need to have the
> same debounce setting.
> 
>>> I did noticed the dev_dbg which I added triggering by a requested value of 50 ms.
>>> I've tracked that down to  drivers/input/misc/soc_button_array.c setting
>>> debounce_interval to 50, which then gets passed to gpiod_set_debounce() by
>>> drivers/input/keyboard/gpio_keys.c. gpio_keys.c will fallback to sw debouncing using
>>> mod_delayed_work() if gpiod_set_debounce() fails, so I think we should deny
>>> big debounce values to keep that fallback working.
>>
>> I'm not sure I got this. If DSDT asks for big debounce timeout how can we be
>> sure it's incorrect request?
> 
> This is not coming from the DSDT, the DSDT is merely listing GPIOs to
> use for the volume and power-buttons. The 50 ms debounce value is hardcoded in
> drivers/input/misc/soc_button_array.c
> 
> That code creates platform-devs + pdata for drivers/input/keyboard/gpio_keys.c
> and that latter code tries to use hw debouncing before falling back to sw
> debouncing. I still had the patch from this thread in my personal WIP tree
> and the version in my tree simply picked 24 ms for this as yuo suggest
> below (continued below).
> 
>>
>> At least I see the way out (yes, over complicated) in keeping bitmap and / or
>> (not sure about bitmap) the mean / average debounce timeout. In such case if
>> 5x users wants 10ms and one wants 50ms, we will set something like 12ms.
>>
>>> I suggest we do the following:
>>>
>>> 1. Reject value < 0 || value > 24 ms values (avoiding the gpio_keys case)
>>
>> Why not to set 24ms? Perhaps we need some heuristics here. If there ask for
>> 30ms, 24ms sounds good enough, no?
> 
> True, there is a bigger problem though as said my the code in my WIP tree
> was selecting 24ms for the 50ms debounce of the volume buttons and
> I noticed that the volume buttons where not properly debounced resulting
> in multiple evdev keypress events and those were grouped much closer together
> in time then 24ms, it was more like 1ms or less even in between the bounces.
> 
> So it seems that the 24ms debounce setting does not work. IIRC (not sure)
> I did check that there was not another later call changing the shared
> debounce time to a lower value.
> 
> Looking back at my notes my intermediate conclusion was that (24ms)
> debouncing seems to just not work; and that this needs more investigation.
> 
>>> 2. Determine rounded-up value using modified switch-case as you suggest 
>>
>> Ack.
>>
>>> 3. Check vs last set rounded-debounce value (if set) and warn + fail
>>>    the set_debounce if it is different
>>
>> Ack.
>>
>>> 4. Remember the last set debounce value
>>
>> Ack with the above comment that perhaps better to keep mean / average.
>>
>>> If the warnings turn out to trigger, we can then look at the DSDT of
>>> that specific device and figure out a way forward from there, with the
>>> knowledge of how a troublesome device actually uses this (I know a sample
>>> of 1 troublesome device is not necessarily representative, but it is
>>> better then no samples and I expect / hope there to simply be no samples).
>>
>> Ack.
>>
>>> (we can then also skip the debounce-time programming when it is unchanged)
>>
>> Or close enough, sounds like we need margin in percentage of the asked value,
>> like ±20% is okay.
> 
> I'm fine with adding a margin, but:
> 
> 1. Given what a mess this is with the shared debounce register
> 2. That the current code expects the exact values from the datasheet,
>    and otherwise returns -EINVAL.
> 3. AFAICT no DSDTs actually use the exact datasheet values.
> 4. 2 + 3 mean that in essence setting the debounce time currently is
>    a no-op since it always returns -EINVAL.
> 
> I wonder if it is not better to preserve the current behavior of
> setting the debounce-time being a no-op by just ripping out support
> for setting it all together?
> 
> This seems like a nice KISS solution to all potential problems here,
> just rely on whatever the BIOS has setup, which is in essence what
> we have been doing so far.
> 
> Note I suggest to just keep the "get" path, it is is fine and I guess
> it might be useful for some cases, or at least for debugging?

So one more argument in favor of just disabling support for
setting the debounce value, while looking at a git log of
the baytrail pinctrl driver I noticed this:

commit ccd025eaddaeb99e982029446197c544252108e2
Author: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Date:   Wed Dec 11 19:32:54 2019 +0200

    pinctrl: baytrail: Enable pin configuration setting for GPIO chip

    It appears that pin configuration for GPIO chip hasn't been enabled yet
    due to absence of ->set_config() callback.

    Enable it here for Intel Baytrail.

    Fixes: c501d0b149de ("pinctrl: baytrail: Add pin control operations")
    Depends-on: 2956b5d94a76 ("pinctrl / gpio: Introduce .set_config() callback for GPIO chips
    Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
    Acked-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>

So before that (which is not that long ago) we were never doing any
of the pinctrl stuff at all AFAICT, which to me seems like another
argument that the best answer to the debounce settings challenges
is to just not do it ?

Regards,

Hans