Hi Dmitry, >-----Original Message----- >From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] >> + struct matrix_keymap_data *keymap_data; > >const please. Yes. I think I have missed this. You had pointed out this earlier also; but I think the re-spin lost them. >No naked returns at the end of void functions please. Okay. >> +} >> + >> +static void ske_keypad_timer(unsigned long data) >> +{ >> + struct platform_device *pdev = (struct platform_device *) data; >> + struct ske_keypad *keypad = platform_get_drvdata(pdev); >> + int ske_kpason; >> + int timeout = keypad->board->debounce_ms; >> + >> + ske_kpason = readl(keypad->reg_base + SKE_CR) & SKE_KPASON; >> + if (ske_kpason) { >> + mod_timer(&keypad->timer, jiffies + msecs_to_jiffies(timeout)); >> + return; >> + } >> + >> + /* read data registers and report event */ >> + ske_keypad_read_data(keypad); >> + >> + return; >> +} >> + >> +static irqreturn_t ske_keypad_irq(int irq, void *dev_id) >> +{ >> + struct ske_keypad *keypad = dev_id; >> + int timeout = keypad->board->debounce_ms; >> + >> + /* disable auto scan interrupt; mask the interrupt generated */ >> + ske_keypad_set_bits(keypad, SKE_IMSC, ~SKE_KPIMA, 0x0); >> + ske_keypad_set_bits(keypad, SKE_ICR, 0x0, SKE_KPICA); >> + >> + /* >> + * if KPASON is not ready, we cannot read data registers >> + * so wait for a debounce period; if still not settled, >> + * we fire a timer and exit the IRQ >> + */ >> + while ((readl(keypad->reg_base + SKE_CR) & SKE_KPASON) && timeout--) >> + cpu_relax(); >> + if (!timeout) { >> + mod_timer(&keypad->timer, jiffies + msecs_to_jiffies(timeout)); > >timeout is 0 here, I do not think that's what you want. > Sorry. I will replace it with keypad->board->debounce_ms. >> + goto out; >> + } >> + >> + /* >> + * KPASON behaved nicely and we can read data registers and report event >> + */ >> + ske_keypad_read_data(keypad); >> + >> +out: >> + /* enable auto scan interrupts */ >> + ske_keypad_set_bits(keypad, SKE_IMSC, 0x0, SKE_KPIMA); > >You sure you want do do it here even when SKE_KPASON is set and timer is >pending? I'd expect re-enabling interrupts from the timer. > I did have that originally as you pointed out. But I ran into an issue where multi-key presses were lost in a case of continued key press. >I'd probably move registration past requesting IRQ - it will simplify >error handling I think. It is safe to send events through input device >as long as it has been allocated with input_allocate_device() even if it >has not been registered yet. > Okay. Will do so. >> + /* go through board initialiazation helpers */ >> + if (keypad->board->init) { >> + keypad->board->init(); > >I do not think you actually compiled this - closing brace is missing >(well, braces are actually not needed at all). Ohh yes, Linus W pointed me out. These are last minute fixups, which I lost. Sorry for that >You are already aware of the issue here... > Yes. Thanx for your review, Regards, Sundar -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
