On Fri, Feb 26, 2010 at 08:13:28PM +0530, Govindarajan, Sriramakrishnan wrote: > > > -----Original Message----- > > From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] > > Sent: Friday, February 26, 2010 1:58 PM > > To: Felipe Balbi > > Cc: Govindarajan, Sriramakrishnan; linux-omap@xxxxxxxxxxxxxxx; linux- > > input@xxxxxxxxxxxxxxx > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
