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

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

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 :-)

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

BTW: The connections of CPU-GPIO -> IRQ of PCA9539 -> GPIO -> gpio_key is
entirely done in the device tree, which is also sort of cool ;-)

Best regards,

-- 
David Jander
Protonic Holland.