RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller

[Sundar R IYER <sundar.iyer@stericsson.com>]

Hi Dmitry,

>-----Original Message-----
>From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]

>> +	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