| Message ID | 20210106125822.31315-4-tony@atomide.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Tony,
I love your patch! Perhaps something to improve:
[auto build test WARNING on input/next]
[also build test WARNING on linus/master v5.11-rc2 next-20210104]
[cannot apply to hid/for-next linux/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Tony-Lindgren/Lost-key-up-interrupt-handling-for-omap4-keypad/20210106-210045
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: openrisc-randconfig-s031-20210106 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-208-g46a52ca4-dirty
# https://github.com/0day-ci/linux/commit/69f44d8d3d1568dd3f330a46f6173a1bfc372ebd
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tony-Lindgren/Lost-key-up-interrupt-handling-for-omap4-keypad/20210106-210045
git checkout 69f44d8d3d1568dd3f330a46f6173a1bfc372ebd
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=openrisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/input/keyboard/omap4-keypad.c: In function 'omap4_keypad_scan_keys':
drivers/input/keyboard/omap4-keypad.c:162:6: warning: variable 'keys_up' set but not used [-Wunused-but-set-variable]
162 | int keys_up, keys_down;
| ^~~~~~~
drivers/input/keyboard/omap4-keypad.c: In function 'omap4_keypad_irq_thread_fn':
>> drivers/input/keyboard/omap4-keypad.c:192:7: warning: variable 'down_events' set but not used [-Wunused-but-set-variable]
192 | bool down_events;
| ^~~~~~~~~~~
At top level:
drivers/input/keyboard/omap4-keypad.c:536:32: warning: 'omap4_keypad_pm_ops' defined but not used [-Wunused-const-variable=]
536 | static const struct dev_pm_ops omap4_keypad_pm_ops = {
| ^~~~~~~~~~~~~~~~~~~
vim +/down_events +192 drivers/input/keyboard/omap4-keypad.c
158
159 static bool omap4_keypad_scan_keys(struct omap4_keypad *keypad_data, bool clear)
160 {
161 struct input_dev *input_dev = keypad_data->input;
> 162 int keys_up, keys_down;
163 u32 low, high;
164 u64 keys = 0;
165
166 mutex_lock(&keypad_data->lock);
167 if (!clear) {
168 low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
169 high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
170 keys = low | (u64)high << 32;
171 }
172
173 /* Scan for key up events for lost key-up interrupts */
174 keys_up = omap4_keypad_scan_state(keypad_data, keys, false);
175
176 /* Scan for key down events */
177 keys_down = omap4_keypad_scan_state(keypad_data, keys, true);
178
179 input_sync(input_dev);
180
181 keypad_data->keys = keys;
182
183 mutex_unlock(&keypad_data->lock);
184
185 return keys_down;
186 }
187
188 static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
189 {
190 struct omap4_keypad *keypad_data = dev_id;
191 struct device *dev = keypad_data->input->dev.parent;
> 192 bool down_events;
193 int error;
194
195 error = pm_runtime_get_sync(dev);
196 if (error < 0) {
197 pm_runtime_put_noidle(dev);
198
199 return IRQ_NONE;
200 }
201
202 down_events = omap4_keypad_scan_keys(keypad_data, false);
203
204 /* clear pending interrupts */
205 kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
206 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
207
208 pm_runtime_mark_last_busy(dev);
209 pm_runtime_put_autosuspend(dev);
210
211 return IRQ_HANDLED;
212 }
213
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Tony, On Wed, Jan 06, 2021 at 02:58:21PM +0200, Tony Lindgren wrote: > @@ -301,6 +348,7 @@ static int omap4_keypad_probe(struct platform_device *pdev) > } > > keypad_data->irq = irq; > + mutex_init(&keypad_data->lock); > > error = omap4_keypad_parse_dt(&pdev->dev, keypad_data); > if (error) > @@ -320,6 +368,8 @@ static int omap4_keypad_probe(struct platform_device *pdev) > goto err_release_mem; > } > > + pm_runtime_use_autosuspend(&pdev->dev); > + pm_runtime_set_autosuspend_delay(&pdev->dev, OMAP4_KEYPAD_AUTOIDLE_MS); This, and corresponding changes in open() and close() seem like a separate improvement. Do you mind splitting them into a separate patch, and have the missing key release fix go on top of it? Thanks. -- Dmitry
* Dmitry Torokhov <dmitry.torokhov@gmail.com> [210110 06:34]: > Hi Tony, > > On Wed, Jan 06, 2021 at 02:58:21PM +0200, Tony Lindgren wrote: > > @@ -301,6 +348,7 @@ static int omap4_keypad_probe(struct platform_device *pdev) > > } > > > > keypad_data->irq = irq; > > + mutex_init(&keypad_data->lock); > > > > error = omap4_keypad_parse_dt(&pdev->dev, keypad_data); > > if (error) > > @@ -320,6 +368,8 @@ static int omap4_keypad_probe(struct platform_device *pdev) > > goto err_release_mem; > > } > > > > + pm_runtime_use_autosuspend(&pdev->dev); > > + pm_runtime_set_autosuspend_delay(&pdev->dev, OMAP4_KEYPAD_AUTOIDLE_MS); > > This, and corresponding changes in open() and close() seem like a > separate improvement. Do you mind splitting them into a separate patch, > and have the missing key release fix go on top of it? Sure will do. Thanks, Tony
diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c --- a/drivers/input/keyboard/omap4-keypad.c +++ b/drivers/input/keyboard/omap4-keypad.c @@ -60,6 +60,7 @@ ((((dbms) * 1000) / ((1 << ((ptv) + 1)) * (1000000 / 32768))) - 1) #define OMAP4_VAL_DEBOUNCINGTIME_16MS \ OMAP4_KEYPAD_DEBOUNCINGTIME_MS(16, OMAP4_KEYPAD_PTV_DIV_128) +#define OMAP4_KEYPAD_AUTOIDLE_MS 50 /* Approximate measured time */ enum { KBD_REVISION_OMAP4 = 0, @@ -71,6 +72,7 @@ struct omap4_keypad { void __iomem *base; unsigned int irq; + struct mutex lock; /* for key scan */ unsigned int rows; unsigned int cols; @@ -154,17 +156,19 @@ static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id) return IRQ_NONE; } -static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id) +static bool omap4_keypad_scan_keys(struct omap4_keypad *keypad_data, bool clear) { - struct omap4_keypad *keypad_data = dev_id; struct input_dev *input_dev = keypad_data->input; int keys_up, keys_down; u32 low, high; - u64 keys; + u64 keys = 0; - low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0); - high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32); - keys = low | (u64)high << 32; + mutex_lock(&keypad_data->lock); + if (!clear) { + low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0); + high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32); + keys = low | (u64)high << 32; + } /* Scan for key up events for lost key-up interrupts */ keys_up = omap4_keypad_scan_state(keypad_data, keys, false); @@ -176,18 +180,49 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id) keypad_data->keys = keys; + mutex_unlock(&keypad_data->lock); + + return keys_down; +} + +static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id) +{ + struct omap4_keypad *keypad_data = dev_id; + struct device *dev = keypad_data->input->dev.parent; + bool down_events; + int error; + + error = pm_runtime_get_sync(dev); + if (error < 0) { + pm_runtime_put_noidle(dev); + + return IRQ_NONE; + } + + down_events = omap4_keypad_scan_keys(keypad_data, false); + /* clear pending interrupts */ kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS, kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS)); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + return IRQ_HANDLED; } static int omap4_keypad_open(struct input_dev *input) { struct omap4_keypad *keypad_data = input_get_drvdata(input); + struct device *dev = input->dev.parent; + int error; - pm_runtime_get_sync(input->dev.parent); + error = pm_runtime_get_sync(dev); + if (error < 0) { + pm_runtime_put_noidle(dev); + + return error; + } disable_irq(keypad_data->irq); @@ -206,6 +241,9 @@ static int omap4_keypad_open(struct input_dev *input) enable_irq(keypad_data->irq); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); + return 0; } @@ -223,14 +261,23 @@ static void omap4_keypad_stop(struct omap4_keypad *keypad_data) static void omap4_keypad_close(struct input_dev *input) { - struct omap4_keypad *keypad_data; + struct omap4_keypad *keypad_data = input_get_drvdata(input); + struct device *dev = input->dev.parent; + int error; + + error = pm_runtime_get_sync(dev); + if (error < 0) { + pm_runtime_put_noidle(dev); + + return; + } - keypad_data = input_get_drvdata(input); disable_irq(keypad_data->irq); omap4_keypad_stop(keypad_data); enable_irq(keypad_data->irq); - pm_runtime_put_sync(input->dev.parent); + pm_runtime_mark_last_busy(dev); + pm_runtime_put_autosuspend(dev); } static int omap4_keypad_parse_dt(struct device *dev, @@ -301,6 +348,7 @@ static int omap4_keypad_probe(struct platform_device *pdev) } keypad_data->irq = irq; + mutex_init(&keypad_data->lock); error = omap4_keypad_parse_dt(&pdev->dev, keypad_data); if (error) @@ -320,6 +368,8 @@ static int omap4_keypad_probe(struct platform_device *pdev) goto err_release_mem; } + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_set_autosuspend_delay(&pdev->dev, OMAP4_KEYPAD_AUTOIDLE_MS); pm_runtime_enable(&pdev->dev); /* @@ -337,7 +387,6 @@ static int omap4_keypad_probe(struct platform_device *pdev) /* Ensure device does not raise interrupts */ omap4_keypad_stop(keypad_data); } - pm_runtime_put_sync(&pdev->dev); } if (error) goto err_pm_disable; @@ -406,6 +455,9 @@ static int omap4_keypad_probe(struct platform_device *pdev) platform_set_drvdata(pdev, keypad_data); + pm_runtime_mark_last_busy(&pdev->dev); + pm_runtime_put_autosuspend(&pdev->dev); + return 0; err_free_irq: @@ -415,6 +467,8 @@ static int omap4_keypad_probe(struct platform_device *pdev) err_free_input: input_free_device(input_dev); err_pm_disable: + pm_runtime_put_sync(&pdev->dev); + pm_runtime_dont_use_autosuspend(&pdev->dev); pm_runtime_disable(&pdev->dev); iounmap(keypad_data->base); err_release_mem: @@ -433,6 +487,7 @@ static int omap4_keypad_remove(struct platform_device *pdev) free_irq(keypad_data->irq, keypad_data); + pm_runtime_dont_use_autosuspend(&pdev->dev); pm_runtime_disable(&pdev->dev); input_unregister_device(keypad_data->input); @@ -454,6 +509,34 @@ static const struct of_device_id omap_keypad_dt_match[] = { }; MODULE_DEVICE_TABLE(of, omap_keypad_dt_match); +/* + * Errata ID i689 "1.32 Keyboard Key Up Event Can Be Missed". + * Interrupt may not happen for key-up events. + */ +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); + bool events; + u32 active; + + active = kbd_readl(keypad_data, OMAP4_KBD_STATEMACHINE); + if (active) { + pm_runtime_mark_last_busy(dev); + return -EBUSY; + } + + events = omap4_keypad_scan_keys(keypad_data, true); + if (events) + dev_info(dev, "cleared stuck events on idle\n"); + + return 0; +} + +static const struct dev_pm_ops omap4_keypad_pm_ops = { + SET_RUNTIME_PM_OPS(omap4_keypad_runtime_suspend, NULL, NULL) +}; + static struct platform_driver omap4_keypad_driver = { .probe = omap4_keypad_probe, .remove = omap4_keypad_remove,
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 again after it enters idle. We have the hardware support for clock auto gating, and the keyboard is capable of generating wake-up events in runtime suspended state. 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> --- drivers/input/keyboard/omap4-keypad.c | 105 +++++++++++++++++++++++--- 1 file changed, 94 insertions(+), 11 deletions(-)