diff mbox series

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

Message ID 20210819203952.785132-1-hdegoede@redhat.com
State New
Headers show
Series pinctrl: baytrail: Pick first supported debounce value larger then the requested value | expand

Commit Message

Hans de Goede Aug. 19, 2021, 8:39 p.m. UTC
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(-)

Comments

Andy Shevchenko Aug. 20, 2021, 1:11 p.m. UTC | #1
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
Hans de Goede Aug. 23, 2021, 10:32 a.m. UTC | #2
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
diff mbox series

Patch

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;