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

On Fri, 16 Mar 2012 01:32:01 -0700
Dmitry Torokhov [off-list ref] wrote:

On Fri, Mar 16, 2012 at 09:17:01AM +0100, David Jander wrote:

quoted

On Fri, 16 Mar 2012 00:20:04 -0700
Dmitry Torokhov [off-list ref] wrote:

quoted

Hi David,

On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:

quoted

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.

quoted

quoted

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.

Best regards,

-- 
David Jander
Protonic Holland.