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

On Fri, 16 Mar 2012 10:18:15 +0000
Ben Dooks [off-list ref] wrote:

On Fri, Mar 16, 2012 at 12:20:04AM -0700, Dmitry Torokhov 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:

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

Why on earth do we need this? this looks like something that is not
necessary and in my view a waste of cpu cycles.

No idea... catch some weird (hardware-/setup-)bug? Not _that_ many CPU cycles
anyway, plus I am not the author of that line.... maybe ask Uwe Kleine-König
(CC'd)?

quoted

        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?

See above comment, I'd go with just remove it and unthread.

Not unthread, but use request_any_context_irq(), please!

Best regards,

-- 
David Jander
Protonic Holland.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html