| 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 |
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
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
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
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;
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(-)