> -----Original Message-----
> From: Dmitry Torokhov [mailto:[email protected]]
> Sent: Friday, February 26, 2010 1:58 PM
> To: Felipe Balbi
> Cc: Govindarajan, Sriramakrishnan; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys
> interfaced to TCA6416
>
> On Thu, Feb 25, 2010 at 08:47:03PM +0200, Felipe Balbi wrote:
> > Hi,
> >
> > On Thu, Feb 25, 2010 at 06:44:59PM +0530, Sriramakrishnan wrote:
> > > This patch implements a simple Keypad driver that functions
> > > as an I2C client. It handles key press events for keys
> > > connected to TCA6416 I2C based IO expander.
> >
> > what's wrong with gpio-keys ??
> >
> > > + * Implementation based on drivers/input/keyboard/gpio_keys.c
> >
> > I see,
> >
> > shouldn't you instead provide a gpiolib driver for tca6416 and use the
> > generic gpio_keys driver ??
> >
>
> Right. The fact that the driver precludes all otehr gpios from being
> used is a major drawback.
[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.
>
>
> > > + if (!chip->use_polling) {
> >
> > IMO, you should only use polling if the irq line isn't connected.
> >
> > > + if (pdata->irq_is_gpio)
> > > + chip->irqnum = gpio_to_irq(pdata->irqnum);
> >
> > you can pass the irq number via i2c_board_info. Use it.
> >
> > > + else
> > > + chip->irqnum = pdata->irqnum;
> > > +
> > > + ret = request_irq(chip->irqnum, tca6416_keys_isr,
> >
> > it's an i2c driver!!! this should be request_threaded_irq()
> >
>
> Threaded IRQ probably does not fit well when you want to support both
> interrupt and polling in the same driver...
[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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
