Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys interfaced to TCA6416

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: "Govindarajan, Sriramakrishnan" <srk@ti.com>
Cc: Felipe Balbi <me@felipebalbi.com>,
	"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>
Subject: Re: [PATCH 1/3] TCA6416 keypad : Implement keypad driver for keys interfaced to TCA6416
Date: Sat, 27 Feb 2010 01:00:17 -0800	[thread overview]
Message-ID: <20100227090017.GI793@core.coreip.homeip.net> (raw)
In-Reply-To: <FCCFB4CDC6E5564B9182F639FC3560870304194AD0@dbde02.ent.ti.com>

On Fri, Feb 26, 2010 at 08:13:28PM +0530, Govindarajan, Sriramakrishnan wrote:
> 
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Friday, February 26, 2010 1:58 PM
> > To: Felipe Balbi
> > Cc: Govindarajan, Sriramakrishnan; 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
> > 
> > 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.

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.

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

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.

-- 
Dmitry