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

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

[Hans de Goede]

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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 59 ++++++++----------------
 1 file changed, 18 insertions(+), 41 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 394a421a19d5..fb3f84c1c136 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -981,6 +981,8 @@ static int byt_pin_config_set(struct pinctrl_dev *pctl_dev,
 			      unsigned long *configs,
 			      unsigned int num_configs)
 {
+	/* See BYT_DEBOUNCE_REG definitions */
+	const unsigned int db_timeouts[] = { 0, 375, 750, 1500, 3000, 6000, 12000, 24000 };
 	struct intel_pinctrl *vg = pinctrl_dev_get_drvdata(pctl_dev);
 	unsigned int param, arg;
 	void __iomem *conf_reg = byt_gpio_reg(vg, offset, BYT_CONF0_REG);
@@ -988,7 +990,7 @@ static int byt_pin_config_set(struct pinctrl_dev *pctl_dev,
 	void __iomem *db_reg = byt_gpio_reg(vg, offset, BYT_DEBOUNCE_REG);
 	unsigned long flags;
 	u32 conf, val, debounce;
-	int i, ret = 0;
+	int i, j, ret = 0;
 
 	raw_spin_lock_irqsave(&byt_lock, flags);
 
@@ -1048,50 +1050,25 @@ static int byt_pin_config_set(struct pinctrl_dev *pctl_dev,
 
 			break;
 		case PIN_CONFIG_INPUT_DEBOUNCE:
-			debounce = readl(db_reg);
-
-			if (arg)
-				conf |= BYT_DEBOUNCE_EN;
-			else
+			if (!arg) {
 				conf &= ~BYT_DEBOUNCE_EN;
-
-			switch (arg) {
-			case 375:
-				debounce &= ~BYT_DEBOUNCE_PULSE_MASK;
-				debounce |= BYT_DEBOUNCE_PULSE_375US;
-				break;
-			case 750:
-				debounce &= ~BYT_DEBOUNCE_PULSE_MASK;
-				debounce |= BYT_DEBOUNCE_PULSE_750US;
-				break;
-			case 1500:
-				debounce &= ~BYT_DEBOUNCE_PULSE_MASK;
-				debounce |= BYT_DEBOUNCE_PULSE_1500US;
-				break;
-			case 3000:
-				debounce &= ~BYT_DEBOUNCE_PULSE_MASK;
-				debounce |= BYT_DEBOUNCE_PULSE_3MS;
-				break;
-			case 6000:
-				debounce &= ~BYT_DEBOUNCE_PULSE_MASK;
-				debounce |= BYT_DEBOUNCE_PULSE_6MS;
-				break;
-			case 12000:
-				debounce &= ~BYT_DEBOUNCE_PULSE_MASK;
-				debounce |= BYT_DEBOUNCE_PULSE_12MS;
-				break;
-			case 24000:
-				debounce &= ~BYT_DEBOUNCE_PULSE_MASK;
-				debounce |= BYT_DEBOUNCE_PULSE_24MS;
-				break;
-			default:
-				if (arg)
-					ret = -EINVAL;
 				break;
 			}
 
-			if (!ret)
-				writel(debounce, db_reg);
+			conf |= BYT_DEBOUNCE_EN;
+
+			for (j = 0; arg > db_timeouts[j]; j++) {
+				if ((j + 1) == ARRAY_SIZE(db_timeouts)) {
+					dev_dbg(vg->dev,
+						"pin %u requested timeout of %u exceeds max timeout of %u\n",
+						offset, arg, db_timeouts[j]);
+					break;
+				}
+			}
+			debounce = readl(db_reg);
+			debounce &= ~BYT_DEBOUNCE_PULSE_MASK;
+			debounce |= j;
+			writel(debounce, db_reg);
 			break;
 		default:
 			ret = -ENOTSUPP;
-- 
2.31.1

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

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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

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

[Andy Shevchenko]

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

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

[Hans de Goede]

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?

Regards,

Hans

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

[Hans de Goede]

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@linux.intel.com>
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@linux.intel.com>
    Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

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

[Andy Shevchenko]

On Sat, Jan 15, 2022 at 6:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 1/13/22 12:56, Hans de Goede wrote:
> > On 1/13/22 12:31, Andy Shevchenko wrote:

...

> > Spoiler: I believe the best thing we can do here is to just
> > never touch the debounce settings at all, see below.

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

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

Hesitating between enabling this feature and practical applications, I
think you are right. No need to keep the code that never has been
properly enabled on the real products (as I read from your research).

-- 
With Best Regards,
Andy Shevchenko