| Message ID | 20210110190529.46135-1-tony@atomide.com |
|---|---|
| Headers | show |
| Series | Lost key-up interrupt handling for omap4-keypad | expand |
On Sun, Jan 10, 2021 at 09:05:27PM +0200, Tony Lindgren wrote: > Let's move rest of the key scanning code to omap4_keypad_scan_keys(). > We will use omap4_keypad_scan_keys() also for implementing errata > handling to clear the stuck last key in the following patch. And this one will become then...
* Dmitry Torokhov <dmitry.torokhov@gmail.com> [210111 04:50]: > On Sun, Jan 10, 2021 at 09:05:27PM +0200, Tony Lindgren wrote: > > Let's move rest of the key scanning code to omap4_keypad_scan_keys(). > > We will use omap4_keypad_scan_keys() also for implementing errata > > handling to clear the stuck last key in the following patch. > > And this one will become then... Yes great, this works for me. Thanks, Tony > > Input: omap4-keypad - move rest of key scanning to a separate function > > From: Tony Lindgren <tony@atomide.com> > > Let's move rest of the key scanning code to omap4_keypad_scan_keys(). > We will use omap4_keypad_scan_keys() also for implementing errata > handling to clear the stuck last key in the following patch. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/keyboard/omap4-keypad.c | 39 ++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 13 deletions(-) > > diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c > index 6dcf27af856d..c48705dd049b 100644 > --- a/drivers/input/keyboard/omap4-keypad.c > +++ b/drivers/input/keyboard/omap4-keypad.c > @@ -71,6 +71,7 @@ struct omap4_keypad { > > void __iomem *base; > unsigned int irq; > + struct mutex lock; /* for key scan */ > > unsigned int rows; > unsigned int cols; > @@ -135,6 +136,28 @@ static int omap4_keypad_report_keys(struct omap4_keypad *keypad_data, > return events; > } > > +static void omap4_keypad_scan_keys(struct omap4_keypad *keypad_data, u64 keys) > +{ > + u64 changed; > + > + mutex_lock(&keypad_data->lock); > + > + changed = keys ^ keypad_data->keys; > + > + /* > + * Report key up events separately and first. This matters in case we > + * lost key-up interrupt and just now catching up. > + */ > + omap4_keypad_report_keys(keypad_data, changed & ~keys, false); > + > + /* Report key down events */ > + omap4_keypad_report_keys(keypad_data, changed & keys, true); > + > + keypad_data->keys = keys; > + > + mutex_unlock(&keypad_data->lock); > +} > + > /* Interrupt handlers */ > static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id) > { > @@ -150,24 +173,13 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id) > { > struct omap4_keypad *keypad_data = dev_id; > u32 low, high; > - u64 keys, changed; > + u64 keys; > > low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0); > high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32); > keys = low | (u64)high << 32; > > - changed = keys ^ keypad_data->keys; > - > - /* > - * Report key up events separately and first. This matters in case we > - * lost key-up interrupt and just now catching up. > - */ > - omap4_keypad_report_keys(keypad_data, changed & ~keys, false); > - > - /* Report key down events */ > - omap4_keypad_report_keys(keypad_data, changed & keys, true); > - > - keypad_data->keys = keys; > + omap4_keypad_scan_keys(keypad_data, keys); > > /* clear pending interrupts */ > kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS, > @@ -300,6 +312,7 @@ static int omap4_keypad_probe(struct platform_device *pdev) > } > > keypad_data->irq = irq; > + mutex_init(&keypad_data->lock); > > error = omap4_keypad_parse_dt(dev, keypad_data); > if (error)
* Tony Lindgren <tony@atomide.com> [210110 19:08]: > --- a/drivers/input/keyboard/omap4-keypad.c > +++ b/drivers/input/keyboard/omap4-keypad.c > +/* > + * Errata ID i689 "1.32 Keyboard Key Up Event Can Be Missed". > + * Interrupt may not happen for key-up events. We must clear stuck > + * key-up events after the keyboard hardware has auto-idled. > + */ > +static int __maybe_unused omap4_keypad_runtime_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct omap4_keypad *keypad_data = platform_get_drvdata(pdev); > + u32 active; > + > + active = kbd_readl(keypad_data, OMAP4_KBD_STATEMACHINE); > + if (active) { > + pm_runtime_mark_last_busy(dev); > + return -EBUSY; > + } > + > + omap4_keypad_scan_keys(keypad_data, true); > + > + return 0; > +} So with the improvments done, here we need to replace the true above with 0 so when the hardware is idle we clear any stuck keys. Updated patch below. Regards, Tony 8< -----------------------
On Sun, Jan 10, 2021 at 09:05:25PM +0200, Tony Lindgren wrote: > We are not using the long events and they produce extra interrupts. > Let's not enable them at all. > > Note that also the v3.0.8 Linux Android kernel has long interrupts > disabled. > > Cc: Arthur Demchenkov <spinal.by@gmail.com> > Cc: Carl Philipp Klemm <philipp@uvos.xyz> > Cc: Merlijn Wajer <merlijn@wizzup.org> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: ruleh <ruleh@gmx.de> > Cc: Sebastian Reichel <sre@kernel.org> > Signed-off-by: Tony Lindgren <tony@atomide.com> Applied, thank you.
On Mon, Jan 11, 2021 at 07:57:39AM +0200, Tony Lindgren wrote: > * Dmitry Torokhov <dmitry.torokhov@gmail.com> [210111 04:50]: > > On Sun, Jan 10, 2021 at 09:05:27PM +0200, Tony Lindgren wrote: > > > Let's move rest of the key scanning code to omap4_keypad_scan_keys(). > > > We will use omap4_keypad_scan_keys() also for implementing errata > > > handling to clear the stuck last key in the following patch. > > > > And this one will become then... > > Yes great, this works for me. OK, thanks, applied. -- Dmitry
On Mon, Jan 11, 2021 at 08:10:18AM +0200, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [210110 19:08]: > > --- a/drivers/input/keyboard/omap4-keypad.c > > +++ b/drivers/input/keyboard/omap4-keypad.c > > +/* > > + * Errata ID i689 "1.32 Keyboard Key Up Event Can Be Missed". > > + * Interrupt may not happen for key-up events. We must clear stuck > > + * key-up events after the keyboard hardware has auto-idled. > > + */ > > +static int __maybe_unused omap4_keypad_runtime_suspend(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + struct omap4_keypad *keypad_data = platform_get_drvdata(pdev); > > + u32 active; > > + > > + active = kbd_readl(keypad_data, OMAP4_KBD_STATEMACHINE); > > + if (active) { > > + pm_runtime_mark_last_busy(dev); > > + return -EBUSY; > > + } > > + > > + omap4_keypad_scan_keys(keypad_data, true); > > + > > + return 0; > > +} > > So with the improvments done, here we need to replace the true above with 0 > so when the hardware is idle we clear any stuck keys. Updated patch below. Applied, thank you.
Hi! > We are still missing handling for errata i689 related issues for the > case where we never see a key up interrupt for the last pressed key. > > To fix the issue, we must scan the key state again after the keyboard > controller has idled to check if a key up event was missed. This is > described in the omap4 silicon errata documentation for Errata ID i689 > "1.32 Keyboard Key Up Event Can Be Missed": > > "When a key is released for a time shorter than the debounce time, > in-between 2 key press (KP1 and KP2), the keyboard state machine will go > to idle mode and will never detect the key release (after KP1, and also > after KP2), and thus will never generate a new IRQ indicating the key > release." > > We can use PM runtime autosuspend features to check the keyboard state > after it enters idle. I thought about this and... is it reasonable? Autosuspend is now required for correct operation. But autosuspend is optional feature, configurable by user, and may not be available depending on .config. Do we need some other solution? Pavel
* Pavel Machek <pavel@ucw.cz> [210111 08:34]: > Hi! > > > We are still missing handling for errata i689 related issues for the > > case where we never see a key up interrupt for the last pressed key. > > > > To fix the issue, we must scan the key state again after the keyboard > > controller has idled to check if a key up event was missed. This is > > described in the omap4 silicon errata documentation for Errata ID i689 > > "1.32 Keyboard Key Up Event Can Be Missed": > > > > "When a key is released for a time shorter than the debounce time, > > in-between 2 key press (KP1 and KP2), the keyboard state machine will go > > to idle mode and will never detect the key release (after KP1, and also > > after KP2), and thus will never generate a new IRQ indicating the key > > release." > > > > We can use PM runtime autosuspend features to check the keyboard state > > after it enters idle. > > I thought about this and... is it reasonable? > > Autosuspend is now required for correct operation. But autosuspend is > optional feature, configurable by user, and may not be available > depending on .config. Well suspending hardware that has (lost) events pending is wrong. So we need to do this delayed hardware check at least when runtime suspending the device. > Do we need some other solution? Not sure if other places make sense to do this check as we need to wait about 50ms for hardware to autoidle, and only then check if there are events pending, and then clear the pending events. The PM runtime suspend function seems like a natural place to do this. If PM runtime autosuspend is disabled, the issue with last pressed key getting stuck sometimes can still happen like it did earlier. That issue has always been there for past 10 or so years and nobody else bothered to fix it so I'm not too worried about it. With this series we already fix the bigger issue anyways where rapidly pressing the keys would have the previous key stuck. Like rapid pressing of shift shift j would produce an upper case J instead of j. Naturally there is nothing stopping us from adding additional other places to call omap4_keypad_scan_keys() as needed now though if you have some good ideas for that :) Regards, Tony
Hi, This series updates omap4-keypad to disable unused long interrupts, and implements the missing parts for the lost key-up interrupt quirk as described in the silicon errata pdf. These are based on Linux next with the following pending patch applied: "Input: omap4-keypad - switch to use managed resources" Regards, Tony Changes since v4: - Rebase on Dmitry's devm patch - Fix up kernel test robot warnings and mismerge of pm_ops when I rebased from v5.10 to Linux next - Split up the patches further as suggested by Dmitry Changes since v3: - Use PM runtime to check the last pressed key is not stuck - Simplify probe with devm Changes since v2: - Drop bogus level change, that already comes from device tree - Scan keyboard in two phases and simplify using a bitmask - Use mod_delayed_work and cancel_delayed_work_sync for the quirk Tony Lindgren (5): Input: omap4-keypad - disable unused long interrupts Input: omap4-keypad - scan keys in two phases and simplify with bitmask Input: omap4-keypad - move rest of key scanning to a separate function Input: omap4-keypad - use PM runtime autosuspend Input: omap4-keypad - implement errata check for lost key-up events drivers/input/keyboard/omap4-keypad.c | 172 ++++++++++++++++++++------ 1 file changed, 132 insertions(+), 40 deletions(-)