Re: [PATCH v2] gpio: lynxpoint: lock IRQs when starting them

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Tue, Nov 26, 2013 at 01:27:21PM +0100, Linus Walleij wrote:
> On Tue, Nov 26, 2013 at 12:49 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Nov 26, 2013 at 11:23:39AM +0100, Linus Walleij wrote:
> 
> >> +static unsigned int lp_irq_startup(struct irq_data *d)
> >> +{
> >> +     struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
> >> +
> >> +     if (gpio_lock_as_irq(&lg->chip, irqd_to_hwirq(d)))
> >> +             dev_err(lg->chip.dev,
> >> +                     "unable to lock HW IRQ %lu for IRQ\n",
> >> +                     irqd_to_hwirq(d));
> >> +     lp_irq_enable(d);
> >
> > I may be missing something but doesn't this now end up calling
> > lp_irq_enable() twice? First in ->irq_startup() and then later on in
> > ->irq_enable().
> 
> kernel/irq/chip.c:
> 
> int irq_startup(struct irq_desc *desc, bool resend)
> {
>         int ret = 0;
> 
>         irq_state_clr_disabled(desc);
>         desc->depth = 0;
> 
>         if (desc->irq_data.chip->irq_startup) {
>                 ret = desc->irq_data.chip->irq_startup(&desc->irq_data);
>                 irq_state_clr_masked(desc);
>         } else {
>                 irq_enable(desc);
>         }
>         if (resend)
>                 check_irq_resend(desc, desc->irq_data.irq);
>         return ret;
> }
> 
> If this hook exists, calls irq_startup() on the chip.
> Else, calls irq_enable() which looks like this:
> 
> void irq_enable(struct irq_desc *desc)
> {
>         irq_state_clr_disabled(desc);
>         if (desc->irq_data.chip->irq_enable)
>                 desc->irq_data.chip->irq_enable(&desc->irq_data);
>         else
>                 desc->irq_data.chip->irq_unmask(&desc->irq_data);
>         irq_state_clr_masked(desc);
> }
> 
> I.e. calls .enable() or .unmask().
> 
> So there is a strict semantic requirement that if you implement
> startup() it should perform the same as .enable(), or .unmask()
> depending on whether the former is implemented.

Thanks for the explanation, got it now :)

> I think this patch is OK ... can you test it on the Lynxpoint?

Tried on haswell/lynxpoint with a slightly modified i2c-hid driver (so that
it is able to use GPIOs as interrupts) and here's what I got:

# cat /sys/kernel/debug/gpio 
GPIOs 162-255, platform/INT33C7:00, INT33C7:00:
 gpio-217 (hid-irq             ) in  hi IRQ

Also the device itself works, so

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>