-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Saturday, February 27, 2010 2:30 PM
To: Govindarajan, Sriramakrishnan
Cc: Felipe Balbi; linux-omap@vger.kernel.org; linux-input@vger.kernel.org
Subject: Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys
interfaced to TCA6416

---snip---

quoted

quoted

quoted

I see,

shouldn't you instead provide a gpiolib driver for tca6416 and use

the

quoted

quoted

quoted

generic gpio_keys driver ??

Right. The fact that the driver precludes all otehr gpios from being
used is a major drawback.

quoted

[Sriram] gpio_keys implementation assumes that every key can generate
an interrupt to the processor and also the state machine to process
events will handle each line individually. Imagine if 16 keys are
connected to the TCA controller and you have to query(assume it is
polling, interrupt mode not possible - as you have single interrupt
line to the processor for event on any of the 16 input lines) 16 lines
individually over i2c bus - the associated overhead is too high.
Instead this implementation handles the necessary book-keeping in one
simple function (get status of all 16 lines in one read transaction).
Building on a GPIO implementation and using gpio-keys above will be
both clumsy and incur runtime overhead as against a direct keypad
driver.

More often(as on AM3517EVM) the keypad keys are not mixed with other
GPIO keys. The initial implementation includes key_mask to identify
the keys to be used for keypad and the others are logically not
modified. Hence the implementation can be extended if need arises to
overcome this restriction.

In this case you should not build on gpio_keys at all but select some
other keyboard as an example that smply uses keytable with set of
keycodes, single interrupt, etc. The overhead of all gpio_keys data
structures is not needed here.

[Sriram] I am working on cleaning up the driver to use a simpler book keeping structure(move away from gpio_keys). Will post the reworked patch shortly.

---snip ---

quoted

[Sriram] All the core logic (including I2C transactions) is
implemented through a work queue implementation. The ISR is slim and
only schedules the work queue. This also enables the implementation to
be easily adaptable for both interrupt/polled mode.

Fair enough but you need to ensure that you do not leave irq unbalances
(in regards to enable/disable) when you using workqueue and disabling
interrupt in the ISR.

[Sriram] Will ensure that this is addressed appropriately (including cleanup on error conditions).

---
Sriram