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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>]

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

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

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?

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

-- 
With Best Regards,
Andy Shevchenko