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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi Andy,

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.
> 
> 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.

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 suggest we do the following:

1. Reject value < 0 || value > 24 ms values (avoiding the gpio_keys case)
2. Determine rounded-up value using modified switch-case as you suggest 
3. Check vs last set rounded-debounce value (if set) and warn + fail
   the set_debounce if it is different
4. Remember the last set debounce value

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).

(we can then also skip the debounce-time programming when it is unchanged)

How does that sound ?

Regards,

Hans