Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Ben Dooks <ben@trinity.fluff.org>]

On Fri, Mar 16, 2012 at 09:48:03AM +0100, David Jander wrote:
> On Fri, 16 Mar 2012 01:32:01 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Fri, Mar 16, 2012 at 09:17:01AM +0100, David Jander wrote:
> > > On Fri, 16 Mar 2012 00:20:04 -0700
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > 
> > > > Hi David,
> > > > 
> > > > On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
> > > > > Use a threaded interrupt handler in order to permit the handler to use
> > > > > a GPIO driver that causes things like I2C transactions being done inside
> > > > > the handler context.
> > > > > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure
> > > > > all needed GPIO drivers have been loaded if the drivers are built into the
> > > > > kernel.
> > > > 
> > > > Don't want to resurrect the whole initcall discussion, but could you
> > > > tell me again why the interrup handler needs to be threaded? We do not
> > > > access hardware from it, hardware is accessed from workqueue context.
> > > > Here is the ISR in its entirety:
> > > 
> > > Sorry, the reason described is apparently not very clear. The real reason seems
> > > to be that I would like this driver to work with I2C GPIO expanders, and its
> > > the GPIO expanders "interrupt controller" which has itself a threaded handler
> > > (due to I2C transfers done in it to ack an IRQ). So this is actually a nested
> > > and threaded interrupt controller (because the IRQ line of the GPIO expander
> > > is connected to a different GPIO acting itself also as interrupt line).
> > > In irq/manage.c, function __setup_irq():
> > > 
> > > ...
> > > 	/*
> > > 	 * Check whether the interrupt nests into another interrupt
> > > 	 * thread.
> > > 	 */
> > > 	nested = irq_settings_is_nested_thread(desc);
> > > 	if (nested) {
> > > 		if (!new->thread_fn) {
> > > 			ret = -EINVAL;
> > > 			goto out_mput;
> > > 		}
> > > ...
> > > 
> > > This is were requesting a non-threaded IRQ from this GPIO controller will fail.
> > > 
> > > I know this is not a trivial setup, but IMHO it is very useful (for
> > > connecting keyboards), and a nice demonstration of the powerful features this
> > > GPIO driver has :-)
> > 
> > Thanks for the explanation of your setup.
> > 
> > > 
> > > > static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
> > > > {
> > > >         struct gpio_button_data *bdata = dev_id;
> > > >         const struct gpio_keys_button *button = bdata->button;
> > > > 
> > > >         BUG_ON(irq != gpio_to_irq(button->gpio));
> > > > 
> > > >         if (bdata->timer_debounce)
> > > >                 mod_timer(&bdata->timer,
> > > >                         jiffies + msecs_to_jiffies(bdata->timer_debounce));
> > > >         else
> > > >                 schedule_work(&bdata->work);
> > > > 
> > > >         return IRQ_HANDLED;
> > > > }
> > > > 
> > > > It looks to me that non-threaded handler would work as well? Or
> > > > gpio_to_irq() can sleep with certain chips?
> > > 
> > > Not in my case. I just checked again. If I change request_threaded_irq() to
> > > request_irq(), I get this:
> > > 
> > > ...
> > > [    6.409810] gpio-keys gpio_keys.0: Unable to claim irq 0; error -22
> > > [    6.416106] gpio-keys: probe of gpio_keys.0 failed with error -22
> > > ...
> > > 
> > > This error -22 (-EINVAL) is returned from __setup_irq() (see above).
> > 
> > But the original code used request_any_context_irq() which should have
> > taken care of your nested IRQ setup:
> 
> Hmm. You are right. Apparently this change was introduced in 2.6.38, and I
> must have missed it. Before 2.6.38, this place called request_irq(), which was
> broken for my case.
> 
> I just checked, and indeed, using request_any_context_irq() seems to work fine
> for me.

I'd say that would be better, seems an un-necesary use of threaded
interrupt work.

-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.