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

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

[Linus Walleij]

This uses the new API for tagging GPIO lines as in use by
IRQs. This enforces a few semantic checks on how the underlying
GPIO line is used.

Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Explicitly call .enable()/.disable() callbacks from the
  startup()/shutdown() callbacks to satisfy implicit semantics.
---
 drivers/gpio/gpio-lynxpoint.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpio/gpio-lynxpoint.c b/drivers/gpio/gpio-lynxpoint.c
index a0804740a0b7..70831e4b2c8b 100644
--- a/drivers/gpio/gpio-lynxpoint.c
+++ b/drivers/gpio/gpio-lynxpoint.c
@@ -301,6 +301,26 @@ static void lp_irq_disable(struct irq_data *d)
 	spin_unlock_irqrestore(&lg->lock, flags);
 }
 
+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);
+	return 0;
+}
+
+static void lp_irq_shutdown(struct irq_data *d)
+{
+	struct lp_gpio *lg = irq_data_get_irq_chip_data(d);
+
+	lp_irq_disable(d);
+	gpio_unlock_as_irq(&lg->chip, irqd_to_hwirq(d));
+}
+
 static struct irq_chip lp_irqchip = {
 	.name = "LP-GPIO",
 	.irq_mask = lp_irq_mask,
@@ -308,6 +328,8 @@ static struct irq_chip lp_irqchip = {
 	.irq_enable = lp_irq_enable,
 	.irq_disable = lp_irq_disable,
 	.irq_set_type = lp_irq_type,
+	.irq_startup = lp_irq_startup,
+	.irq_shutdown = lp_irq_shutdown,
 	.flags = IRQCHIP_SKIP_SET_WAKE,
 };
 
-- 
1.8.3.1

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

[Mika Westerberg]

On Tue, Nov 26, 2013 at 11:23:39AM +0100, Linus Walleij wrote:
> This uses the new API for tagging GPIO lines as in use by
> IRQs. This enforces a few semantic checks on how the underlying
> GPIO line is used.
> 
> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Explicitly call .enable()/.disable() callbacks from the
>   startup()/shutdown() callbacks to satisfy implicit semantics.
> ---
>  drivers/gpio/gpio-lynxpoint.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-lynxpoint.c b/drivers/gpio/gpio-lynxpoint.c
> index a0804740a0b7..70831e4b2c8b 100644
> --- a/drivers/gpio/gpio-lynxpoint.c
> +++ b/drivers/gpio/gpio-lynxpoint.c
> @@ -301,6 +301,26 @@ static void lp_irq_disable(struct irq_data *d)
>  	spin_unlock_irqrestore(&lg->lock, flags);
>  }
>  
> +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().

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

[Javier Martinez Canillas]

Hi Mika,

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:
>> This uses the new API for tagging GPIO lines as in use by
>> IRQs. This enforces a few semantic checks on how the underlying
>> GPIO line is used.
>>
>> Cc: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> ChangeLog v1->v2:
>> - Explicitly call .enable()/.disable() callbacks from the
>>   startup()/shutdown() callbacks to satisfy implicit semantics.
>> ---
>>  drivers/gpio/gpio-lynxpoint.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpio/gpio-lynxpoint.c b/drivers/gpio/gpio-lynxpoint.c
>> index a0804740a0b7..70831e4b2c8b 100644
>> --- a/drivers/gpio/gpio-lynxpoint.c
>> +++ b/drivers/gpio/gpio-lynxpoint.c
>> @@ -301,6 +301,26 @@ static void lp_irq_disable(struct irq_data *d)
>>       spin_unlock_irqrestore(&lg->lock, flags);
>>  }
>>
>> +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().
> --

As far as I understood from Grygorii explanation chip->irq_startup()
and chip->irq_enable() are mutually exclusive at
request_[threaded]_irq(). And he seems to be correct by looking at
irq_startup() in irq/chip.c [0].

Thanks a lot and best regards,
Javier

[0]: http://lxr.free-electrons.com/source/kernel/irq/chip.c#L175

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

[Linus Walleij]

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.

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

Yours,
Linus Walleij

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

[Mika Westerberg]

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>