[PATCH v2] gpio/omap: implement irq_enable/disable using mask/unmask.

[PATCH v2] gpio/omap: implement irq_enable/disable using mask/unmask.

[Andreas Fenkart]

This is a resend. The single change is the added changelog
https://patchwork.kernel.org/patch/1886421/

Andreas Fenkart (1):
  gpio/omap: implement irq_enable/disable using mask/unmask.

 drivers/gpio/gpio-omap.c |    2 ++
 1 file changed, 2 insertions(+)

-- 
1.7.10.4

[PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Andreas Fenkart]

In PM suspend, some omaps can't detect sdio irqs via the sdio interface.
The way to implement this, is to declare the corresponding pin as part
of the sdio interface and as a gpio input via pinctrl-single states.
The MMC driver request states "default" "active" and "idle" during the
probe, then toggles between active and idle during the runtime. This
requires low overhead functions for enable/disable of gpio irqs.

For level triggered interrupt there is no difference between masking
and disabling an interrupt. For edge interrupt interrupts there is.
When masked, interrupts should still be latched to the interrupt status
register so when unmasked later there is an interrupt straight away.
However, if the interrupt is disabled then gpio events occurring will not
be latched/stored. Hence proposed patch is incomplete for edge type
interrupts.

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
---
 drivers/gpio/gpio-omap.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..69ef2d8 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -815,6 +815,8 @@ static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.irq_disable    = gpio_mask_irq, /* FIXME for edge type IRQ */
+	.irq_enable     = gpio_unmask_irq,
 };
 
 /*---------------------------------------------------------------------*/
-- 
1.7.10.4

Re: [PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Santosh Shilimkar]

On Friday 12 April 2013 02:43 PM, Andreas Fenkart wrote:
> In PM suspend, some omaps can't detect sdio irqs via the sdio interface.
> The way to implement this, is to declare the corresponding pin as part
> of the sdio interface and as a gpio input via pinctrl-single states.
> The MMC driver request states "default" "active" and "idle" during the
> probe, then toggles between active and idle during the runtime. This
> requires low overhead functions for enable/disable of gpio irqs.
> 
> For level triggered interrupt there is no difference between masking
> and disabling an interrupt. For edge interrupt interrupts there is.
> When masked, interrupts should still be latched to the interrupt status
> register so when unmasked later there is an interrupt straight away.
> However, if the interrupt is disabled then gpio events occurring will not
> be latched/stored. Hence proposed patch is incomplete for edge type
> interrupts.
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
Patch is incomplete and still confusing ;-) if some one reads the
patch without the thread. I think you have already ask the question/
suggestion in past but its better to split masking/disabling functions
and make them behave properly. Mapping enable/disable to mask/unmask
to get around the issue seems more of a hack.

Also I think, we can address the edge type IRQ issue along with this
patch to be complete.

Before you go ahead with the update, I would like to hear Jon's
opinion on the edge IRQ related fixing.

>  drivers/gpio/gpio-omap.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 159f5c5..69ef2d8 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -815,6 +815,8 @@ static struct irq_chip gpio_irq_chip = {
>  	.irq_unmask	= gpio_unmask_irq,
>  	.irq_set_type	= gpio_irq_type,
>  	.irq_set_wake	= gpio_wake_enable,
> +	.irq_disable    = gpio_mask_irq, /* FIXME for edge type IRQ */
> +	.irq_enable     = gpio_unmask_irq,
>  };
>  
>  /*---------------------------------------------------------------------*/
> 
Sorry to make you wait for the proposal discussion.

Regards,
Santosh

Re: [PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]

Hi,

On Fri, Apr 12, 2013 at 03:49:52PM +0530, Santosh Shilimkar wrote:
> On Friday 12 April 2013 02:43 PM, Andreas Fenkart wrote:
> > In PM suspend, some omaps can't detect sdio irqs via the sdio interface.
> > The way to implement this, is to declare the corresponding pin as part
> > of the sdio interface and as a gpio input via pinctrl-single states.
> > The MMC driver request states "default" "active" and "idle" during the
> > probe, then toggles between active and idle during the runtime. This
> > requires low overhead functions for enable/disable of gpio irqs.
> > 
> > For level triggered interrupt there is no difference between masking
> > and disabling an interrupt. For edge interrupt interrupts there is.
> > When masked, interrupts should still be latched to the interrupt status
> > register so when unmasked later there is an interrupt straight away.
> > However, if the interrupt is disabled then gpio events occurring will not
> > be latched/stored. Hence proposed patch is incomplete for edge type
> > interrupts.
> > 
> > Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> > ---
> Patch is incomplete and still confusing ;-) if some one reads the
> patch without the thread. I think you have already ask the question/
> suggestion in past but its better to split masking/disabling functions
> and make them behave properly. Mapping enable/disable to mask/unmask
> to get around the issue seems more of a hack.

right, specially since IRQ susystem will already do that for
irq_enable():

kernel/irq/chip.c::irq_enable()

192 void irq_enable(struct irq_desc *desc)
193 {
194         irq_state_clr_disabled(desc);
195         if (desc->irq_data.chip->irq_enable)
196                 desc->irq_data.chip->irq_enable(&desc->irq_data);
197         else
198                 desc->irq_data.chip->irq_unmask(&desc->irq_data);
199         irq_state_clr_masked(desc);
200 }

In fact this patch shouldn't be necessary if only IRQ subsystem would do
the same for irq_disable() (though it doesn't and I haven't fully read
the code you to understand why, however there's definitely a reason):

202 void irq_disable(struct irq_desc *desc)
203 {
204         irq_state_set_disabled(desc);
205         if (desc->irq_data.chip->irq_disable) {
206                 desc->irq_data.chip->irq_disable(&desc->irq_data);
207                 irq_state_set_masked(desc);
208         }
209 }

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2] gpio/omap: implement irq mask/disable with proper semantic.

[Andreas Fenkart]

When a gpio interrupt is masked, the gpio event will still be latched in
the interrupt status register so when you unmask it later you may get an
interrupt straight away. However, if the interrupt is disabled then gpio
events occurring will not be latched/stored.

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
---
 drivers/gpio/gpio-omap.c |   45 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..9ab7ba5 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -780,7 +780,7 @@ static void gpio_mask_irq(struct irq_data *d)
 
 	spin_lock_irqsave(&bank->lock, flags);
 	_set_gpio_irqenable(bank, gpio, 0);
-	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+	/* latching new IRQ, but don't signal */
 	spin_unlock_irqrestore(&bank->lock, flags);
 }
 
@@ -789,24 +789,48 @@ static void gpio_unmask_irq(struct irq_data *d)
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
 	unsigned int gpio = irq_to_gpio(bank, d->irq);
 	unsigned int irq_mask = GPIO_BIT(bank, gpio);
-	u32 trigger = irqd_get_trigger_type(d);
 	unsigned long flags;
 
 	spin_lock_irqsave(&bank->lock, flags);
-	if (trigger)
-		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
 
-	/* For level-triggered GPIOs, the clearing must be done after
-	 * the HW source is cleared, thus after the handler has run */
-	if (bank->level_mask & irq_mask) {
-		_set_gpio_irqenable(bank, gpio, 0);
+	/* For level-triggered GPIOs, clear the IRQ. If the HW
+	 * still needs service, IRQ will be latched again */
+	if (bank->level_mask & irq_mask)
 		_clear_gpio_irqstatus(bank, gpio);
-	}
 
 	_set_gpio_irqenable(bank, gpio, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
 }
 
+static void gpio_disable_irq(struct irq_data *d)
+{
+	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+	/* stop latching new IRQ */
+	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+	_set_gpio_irqenable(bank, gpio, 0);
+	spin_unlock_irqrestore(&bank->lock, flags);
+}
+
+static void gpio_enable_irq(struct irq_data *d)
+{
+	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int irq_mask = GPIO_BIT(bank, gpio);
+	u32 trigger = irqd_get_trigger_type(d);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+	if (trigger)
+		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
+	_clear_gpio_irqstatus(bank, gpio);
+	_set_gpio_irqenable(bank, gpio, 1);
+	spin_unlock_irqrestore(&bank->lock, flags);
+}
+
 static struct irq_chip gpio_irq_chip = {
 	.name		= "GPIO",
 	.irq_shutdown	= gpio_irq_shutdown,
@@ -815,6 +839,9 @@ static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.irq_disable    = gpio_disable_irq,
+	.irq_enable     = gpio_enable_irq,
+
 };
 
 /*---------------------------------------------------------------------*/
-- 
1.7.10.4

Re: [PATCH] gpio/omap: implement irq_enable/disable using mask/unmask.

[Andreas Fenkart]

Hi Felipe,


On Fri, Apr 12, 2013 at 02:07:01PM +0300, Felipe Balbi wrote:
[snip]
> > > Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> > > ---
> > Patch is incomplete and still confusing ;-) if some one reads the
> > patch without the thread. I think you have already ask the question/
> > suggestion in past but its better to split masking/disabling functions
> > and make them behave properly. Mapping enable/disable to mask/unmask
> > to get around the issue seems more of a hack.
> 
> right, specially since IRQ susystem will already do that for
> irq_enable():
> 
> kernel/irq/chip.c::irq_enable()
> 
> 192 void irq_enable(struct irq_desc *desc)
> 193 {
> 194         irq_state_clr_disabled(desc);
> 195         if (desc->irq_data.chip->irq_enable)
> 196                 desc->irq_data.chip->irq_enable(&desc->irq_data);
> 197         else
> 198                 desc->irq_data.chip->irq_unmask(&desc->irq_data);
> 199         irq_state_clr_masked(desc);
> 200 }
> 
> In fact this patch shouldn't be necessary if only IRQ subsystem would do
> the same for irq_disable() (though it doesn't and I haven't fully read
> the code you to understand why, however there's definitely a reason):
> 
> 202 void irq_disable(struct irq_desc *desc)
> 203 {
> 204         irq_state_set_disabled(desc);
> 205         if (desc->irq_data.chip->irq_disable) {
> 206                 desc->irq_data.chip->irq_disable(&desc->irq_data);
> 207                 irq_state_set_masked(desc);
> 208         }
> 209 }

I started some other thread over here:
[PATCH] genirq: use irq_mask as fallback for irq_disable.
https://lkml.org/lkml/2013/4/19/229

/Andi

Re: [PATCH v2] gpio/omap: implement irq mask/disable with proper semantic.

[Santosh Shilimkar]

On Saturday 20 April 2013 12:50 AM, Andreas Fenkart wrote:
> When a gpio interrupt is masked, the gpio event will still be latched in
> the interrupt status register so when you unmask it later you may get an
> interrupt straight away. However, if the interrupt is disabled then gpio
> events occurring will not be latched/stored.
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
>  drivers/gpio/gpio-omap.c |   45 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 159f5c5..9ab7ba5 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -780,7 +780,7 @@ static void gpio_mask_irq(struct irq_data *d)
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	_set_gpio_irqenable(bank, gpio, 0);
> -	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
> +	/* latching new IRQ, but don't signal */
You can add the comment in begining of the function. Comment just
before spin_lock release will be confusing.

>  	spin_unlock_irqrestore(&bank->lock, flags);
>  }
>  
> @@ -789,24 +789,48 @@ static void gpio_unmask_irq(struct irq_data *d)
>  	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>  	unsigned int gpio = irq_to_gpio(bank, d->irq);
>  	unsigned int irq_mask = GPIO_BIT(bank, gpio);
> -	u32 trigger = irqd_get_trigger_type(d);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&bank->lock, flags);
> -	if (trigger)
> -		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
>  
> -	/* For level-triggered GPIOs, the clearing must be done after
> -	 * the HW source is cleared, thus after the handler has run */
> -	if (bank->level_mask & irq_mask) {
> -		_set_gpio_irqenable(bank, gpio, 0);
> +	/* For level-triggered GPIOs, clear the IRQ. If the HW
> +	 * still needs service, IRQ will be latched again */
While at this, please fix the comment style
/*
 *
 */
> +	if (bank->level_mask & irq_mask)
>  		_clear_gpio_irqstatus(bank, gpio);
> -	}
>  
>  	_set_gpio_irqenable(bank, gpio, 1);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  }
>  
> +static void gpio_disable_irq(struct irq_data *d)
> +{
> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +	unsigned int gpio = irq_to_gpio(bank, d->irq);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->lock, flags);
> +	/* stop latching new IRQ */
Best thing is you add kernel doc for all these function and
describe them clearly. mask/unmask, disable/enable

> +	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
> +	_set_gpio_irqenable(bank, gpio, 0);
> +	spin_unlock_irqrestore(&bank->lock, flags);
> +}
> +
> +static void gpio_enable_irq(struct irq_data *d)
> +{
> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +	unsigned int gpio = irq_to_gpio(bank, d->irq);
> +	unsigned int irq_mask = GPIO_BIT(bank, gpio);
> +	u32 trigger = irqd_get_trigger_type(d);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->lock, flags);
> +	if (trigger)
> +		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
> +	_clear_gpio_irqstatus(bank, gpio);
> +	_set_gpio_irqenable(bank, gpio, 1);
> +	spin_unlock_irqrestore(&bank->lock, flags);
> +}
> +
>  static struct irq_chip gpio_irq_chip = {
>  	.name		= "GPIO",
>  	.irq_shutdown	= gpio_irq_shutdown,
> @@ -815,6 +839,9 @@ static struct irq_chip gpio_irq_chip = {
>  	.irq_unmask	= gpio_unmask_irq,
>  	.irq_set_type	= gpio_irq_type,
>  	.irq_set_wake	= gpio_wake_enable,
> +	.irq_disable    = gpio_disable_irq,
> +	.irq_enable     = gpio_enable_irq,
> +
Drop above extra line.
>  };
>  
>  /*---------------------------------------------------------------------*/
> 

[PATCH v3] gpio/omap: implement irq mask/disable with proper

[Andreas Fenkart]

Added kernel doc for mask/unmask, disable/enable functions.

Andreas Fenkart (1):
  gpio/omap: implement irq mask/disable with proper semantic.

 drivers/gpio/gpio-omap.c |   69 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 60 insertions(+), 9 deletions(-)

-- 
1.7.10.4

[PATCH v3] gpio/omap: implement irq mask/disable with proper semantic.

[Andreas Fenkart]

When a gpio interrupt is masked, the gpio event will still be latched in
the interrupt status register so when you unmask it later you may get an
interrupt straight away. However, if the interrupt is disabled then gpio
events occurring will not be latched/stored.

Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
---
 drivers/gpio/gpio-omap.c |   69 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 159f5c5..0b66c45 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -772,6 +772,12 @@ static void gpio_ack_irq(struct irq_data *d)
 	_clear_gpio_irqstatus(bank, gpio);
 }
 
+/**
+ * gpio_mask_irq - mask IRQ signalling
+ * @d : the gpio data we're acting upon
+ *
+ * Only signalling disabled. New IRQ still latched to IRQ status register.
+ */
 static void gpio_mask_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
@@ -780,33 +786,76 @@ static void gpio_mask_irq(struct irq_data *d)
 
 	spin_lock_irqsave(&bank->lock, flags);
 	_set_gpio_irqenable(bank, gpio, 0);
-	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
 	spin_unlock_irqrestore(&bank->lock, flags);
 }
 
+/**
+ * gpio_unmask_irq - unmask IRQ signalling
+ * @d : the gpio data we're acting upon
+ *
+ * If an IRQ occured while IRQ was masked, you will get an IRQ straight away.
+ */
 static void gpio_unmask_irq(struct irq_data *d)
 {
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
 	unsigned int gpio = irq_to_gpio(bank, d->irq);
 	unsigned int irq_mask = GPIO_BIT(bank, gpio);
-	u32 trigger = irqd_get_trigger_type(d);
 	unsigned long flags;
 
 	spin_lock_irqsave(&bank->lock, flags);
-	if (trigger)
-		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
 
-	/* For level-triggered GPIOs, the clearing must be done after
-	 * the HW source is cleared, thus after the handler has run */
-	if (bank->level_mask & irq_mask) {
-		_set_gpio_irqenable(bank, gpio, 0);
+	/*
+	 * For level-triggered GPIOs, clear the IRQ. If the HW
+	 * still needs service, IRQ will be latched again
+	 */
+	if (bank->level_mask & irq_mask)
 		_clear_gpio_irqstatus(bank, gpio);
-	}
 
 	_set_gpio_irqenable(bank, gpio, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
 }
 
+/**
+ * gpio_disable_irq - disable the interrupt
+ * @d : the gpio data we're acting upon
+ *
+ * While disabled all IRQ events are ignored. No latching to IRQ status
+ * register.
+ */
+static void gpio_disable_irq(struct irq_data *d)
+{
+	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+	_set_gpio_irqenable(bank, gpio, 0);
+	spin_unlock_irqrestore(&bank->lock, flags);
+}
+
+/**
+ * gpio_enable_irq - enable the interrupt
+ * @d : the gpio data we're acting upon
+ *
+ * Enables latching and signalling IRQ.
+ */
+static void gpio_enable_irq(struct irq_data *d)
+{
+	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+	unsigned int gpio = irq_to_gpio(bank, d->irq);
+	unsigned int irq_mask = GPIO_BIT(bank, gpio);
+	u32 trigger = irqd_get_trigger_type(d);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->lock, flags);
+	if (trigger)
+		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
+	_clear_gpio_irqstatus(bank, gpio);
+	_set_gpio_irqenable(bank, gpio, 1);
+	spin_unlock_irqrestore(&bank->lock, flags);
+}
+
 static struct irq_chip gpio_irq_chip = {
 	.name		= "GPIO",
 	.irq_shutdown	= gpio_irq_shutdown,
@@ -815,6 +864,8 @@ static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.irq_disable    = gpio_disable_irq,
+	.irq_enable     = gpio_enable_irq,
 };
 
 /*---------------------------------------------------------------------*/
-- 
1.7.10.4

Re: [PATCH v3] gpio/omap: implement irq mask/disable with proper semantic.

[Kevin Hilman]

Andreas Fenkart <andreas.fenkart@streamunlimited.com> writes:

> When a gpio interrupt is masked, the gpio event will still be latched in
> the interrupt status register so when you unmask it later you may get an
> interrupt straight away. However, if the interrupt is disabled then gpio
> events occurring will not be latched/stored.
>
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>

The approach seems OK to me, but the changelog is still missing quite a
bit of information.

I know it's been hashed over in the various threads, but the changelog
needs all that discussion summarized.  The changelog should at least:

- describe the problem/bug being fixed
- describe the fix
- answer "why" the approach was taken (not how.)  This part is very
  important to reviewers and maintainers.
- how it was tested, and on what platforms

In addition, I'd like to see a description of how (or whether) this
affects the genirq lazy disable functionality, and how it was tested.

Kevin

Re: [PATCH v3] gpio/omap: implement irq mask/disable with proper semantic.

[Jon Hunter]

On 04/22/2013 03:54 AM, Andreas Fenkart wrote:
> When a gpio interrupt is masked, the gpio event will still be latched in
> the interrupt status register so when you unmask it later you may get an
> interrupt straight away. However, if the interrupt is disabled then gpio
> events occurring will not be latched/stored.
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
>  drivers/gpio/gpio-omap.c |   69 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 60 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 159f5c5..0b66c45 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -772,6 +772,12 @@ static void gpio_ack_irq(struct irq_data *d)
>  	_clear_gpio_irqstatus(bank, gpio);
>  }
>  
> +/**
> + * gpio_mask_irq - mask IRQ signalling
> + * @d : the gpio data we're acting upon
> + *
> + * Only signalling disabled. New IRQ still latched to IRQ status register.
> + */
>  static void gpio_mask_irq(struct irq_data *d)
>  {
>  	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> @@ -780,33 +786,76 @@ static void gpio_mask_irq(struct irq_data *d)
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	_set_gpio_irqenable(bank, gpio, 0);
> -	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);

My only concern here is that _set_gpio_triggering() is also handling the
wake-up enable register. So by clearing the triggering here it was also
clearing the wake-up enable. Ideally, I would think that when masking
the interrupt we would also want to disable the wake-up generation too
(if enabled). I think it would only be a problem if someone called
gpio_enable_irq() and the gpio_mask_irq().

Ideally the wake-up enable stuff should be removed from the triggering
function. May be we could add explicit calls to _set_gpio_wakeup().

>  	spin_unlock_irqrestore(&bank->lock, flags);
>  }
>  
> +/**
> + * gpio_unmask_irq - unmask IRQ signalling
> + * @d : the gpio data we're acting upon
> + *
> + * If an IRQ occured while IRQ was masked, you will get an IRQ straight away.
> + */
>  static void gpio_unmask_irq(struct irq_data *d)
>  {
>  	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>  	unsigned int gpio = irq_to_gpio(bank, d->irq);
>  	unsigned int irq_mask = GPIO_BIT(bank, gpio);
> -	u32 trigger = irqd_get_trigger_type(d);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&bank->lock, flags);
> -	if (trigger)
> -		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
>  
> -	/* For level-triggered GPIOs, the clearing must be done after
> -	 * the HW source is cleared, thus after the handler has run */
> -	if (bank->level_mask & irq_mask) {
> -		_set_gpio_irqenable(bank, gpio, 0);
> +	/*
> +	 * For level-triggered GPIOs, clear the IRQ. If the HW
> +	 * still needs service, IRQ will be latched again
> +	 */
> +	if (bank->level_mask & irq_mask)
>  		_clear_gpio_irqstatus(bank, gpio);
> -	}
>  
>  	_set_gpio_irqenable(bank, gpio, 1);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  }

Similarly here, can we ensure the wake-up enable is set?

Cheers
Jon

Re: [PATCH v3] gpio/omap: implement irq mask/disable with proper semantic.

[Jon Hunter]

On 04/22/2013 03:54 AM, Andreas Fenkart wrote:
> When a gpio interrupt is masked, the gpio event will still be latched in
> the interrupt status register so when you unmask it later you may get an
> interrupt straight away. However, if the interrupt is disabled then gpio
> events occurring will not be latched/stored.
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
>  drivers/gpio/gpio-omap.c |   69 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 60 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 159f5c5..0b66c45 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -772,6 +772,12 @@ static void gpio_ack_irq(struct irq_data *d)
>  	_clear_gpio_irqstatus(bank, gpio);
>  }
>  
> +/**
> + * gpio_mask_irq - mask IRQ signalling
> + * @d : the gpio data we're acting upon
> + *
> + * Only signalling disabled. New IRQ still latched to IRQ status register.
> + */
>  static void gpio_mask_irq(struct irq_data *d)
>  {
>  	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> @@ -780,33 +786,76 @@ static void gpio_mask_irq(struct irq_data *d)
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	_set_gpio_irqenable(bank, gpio, 0);
> -	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  }
>  
> +/**
> + * gpio_unmask_irq - unmask IRQ signalling
> + * @d : the gpio data we're acting upon
> + *
> + * If an IRQ occured while IRQ was masked, you will get an IRQ straight away.
> + */
>  static void gpio_unmask_irq(struct irq_data *d)
>  {
>  	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>  	unsigned int gpio = irq_to_gpio(bank, d->irq);
>  	unsigned int irq_mask = GPIO_BIT(bank, gpio);
> -	u32 trigger = irqd_get_trigger_type(d);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&bank->lock, flags);
> -	if (trigger)
> -		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
>  
> -	/* For level-triggered GPIOs, the clearing must be done after
> -	 * the HW source is cleared, thus after the handler has run */
> -	if (bank->level_mask & irq_mask) {
> -		_set_gpio_irqenable(bank, gpio, 0);
> +	/*
> +	 * For level-triggered GPIOs, clear the IRQ. If the HW
> +	 * still needs service, IRQ will be latched again
> +	 */
> +	if (bank->level_mask & irq_mask)
>  		_clear_gpio_irqstatus(bank, gpio);
> -	}
>  
>  	_set_gpio_irqenable(bank, gpio, 1);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  }
>  
> +/**
> + * gpio_disable_irq - disable the interrupt
> + * @d : the gpio data we're acting upon
> + *
> + * While disabled all IRQ events are ignored. No latching to IRQ status
> + * register.
> + */
> +static void gpio_disable_irq(struct irq_data *d)
> +{
> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +	unsigned int gpio = irq_to_gpio(bank, d->irq);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->lock, flags);
> +	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
> +	_set_gpio_irqenable(bank, gpio, 0);
> +	spin_unlock_irqrestore(&bank->lock, flags);
> +}
> +
> +/**
> + * gpio_enable_irq - enable the interrupt
> + * @d : the gpio data we're acting upon
> + *
> + * Enables latching and signalling IRQ.
> + */
> +static void gpio_enable_irq(struct irq_data *d)
> +{
> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +	unsigned int gpio = irq_to_gpio(bank, d->irq);
> +	unsigned int irq_mask = GPIO_BIT(bank, gpio);
> +	u32 trigger = irqd_get_trigger_type(d);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->lock, flags);
> +	if (trigger)
> +		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
> +	_clear_gpio_irqstatus(bank, gpio);
> +	_set_gpio_irqenable(bank, gpio, 1);
> +	spin_unlock_irqrestore(&bank->lock, flags);
> +}

This generates the following warning ...

drivers/gpio/gpio-omap.c: In function 'gpio_enable_irq':
drivers/gpio/gpio-omap.c:852:15: warning: unused variable 'irq_mask'
[-Wunused-variable]

So you can get rid of irq_mask in the above function.

Jon

Re: [PATCH v2] gpio/omap: implement irq mask/disable with proper semantic.

[Linus Walleij]

On Fri, Apr 19, 2013 at 9:20 PM, Andreas Fenkart
<andreas.fenkart@streamunlimited.com> wrote:

> When a gpio interrupt is masked, the gpio event will still be latched in
> the interrupt status register so when you unmask it later you may get an
> interrupt straight away. However, if the interrupt is disabled then gpio
> events occurring will not be latched/stored.
>
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>

Have to defer this to the next development cycle as no consensus
seem to have been reached with the maintainers of this driver.
Keep going.

Thanks,
Linus Walleij

Re: [PATCH v3] gpio/omap: implement irq mask/disable with proper semantic.

[Jon Hunter]

On 04/22/2013 03:54 AM, Andreas Fenkart wrote:
> When a gpio interrupt is masked, the gpio event will still be latched in
> the interrupt status register so when you unmask it later you may get an
> interrupt straight away. However, if the interrupt is disabled then gpio
> events occurring will not be latched/stored.
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
>  drivers/gpio/gpio-omap.c |   69 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 60 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 159f5c5..0b66c45 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -772,6 +772,12 @@ static void gpio_ack_irq(struct irq_data *d)
>  	_clear_gpio_irqstatus(bank, gpio);
>  }
>  
> +/**
> + * gpio_mask_irq - mask IRQ signalling
> + * @d : the gpio data we're acting upon
> + *
> + * Only signalling disabled. New IRQ still latched to IRQ status register.
> + */
>  static void gpio_mask_irq(struct irq_data *d)
>  {
>  	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> @@ -780,33 +786,76 @@ static void gpio_mask_irq(struct irq_data *d)
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	_set_gpio_irqenable(bank, gpio, 0);
> -	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  }
>  
> +/**
> + * gpio_unmask_irq - unmask IRQ signalling
> + * @d : the gpio data we're acting upon
> + *
> + * If an IRQ occured while IRQ was masked, you will get an IRQ straight away.
> + */
>  static void gpio_unmask_irq(struct irq_data *d)
>  {
>  	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>  	unsigned int gpio = irq_to_gpio(bank, d->irq);
>  	unsigned int irq_mask = GPIO_BIT(bank, gpio);
> -	u32 trigger = irqd_get_trigger_type(d);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&bank->lock, flags);
> -	if (trigger)
> -		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
>  
> -	/* For level-triggered GPIOs, the clearing must be done after
> -	 * the HW source is cleared, thus after the handler has run */
> -	if (bank->level_mask & irq_mask) {
> -		_set_gpio_irqenable(bank, gpio, 0);
> +	/*
> +	 * For level-triggered GPIOs, clear the IRQ. If the HW
> +	 * still needs service, IRQ will be latched again
> +	 */
> +	if (bank->level_mask & irq_mask)
>  		_clear_gpio_irqstatus(bank, gpio);
> -	}
>  
>  	_set_gpio_irqenable(bank, gpio, 1);
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  }
>  
> +/**
> + * gpio_disable_irq - disable the interrupt
> + * @d : the gpio data we're acting upon
> + *
> + * While disabled all IRQ events are ignored. No latching to IRQ status
> + * register.
> + */
> +static void gpio_disable_irq(struct irq_data *d)
> +{
> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +	unsigned int gpio = irq_to_gpio(bank, d->irq);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->lock, flags);
> +	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
> +	_set_gpio_irqenable(bank, gpio, 0);
> +	spin_unlock_irqrestore(&bank->lock, flags);
> +}
> +
> +/**
> + * gpio_enable_irq - enable the interrupt
> + * @d : the gpio data we're acting upon
> + *
> + * Enables latching and signalling IRQ.
> + */
> +static void gpio_enable_irq(struct irq_data *d)
> +{
> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +	unsigned int gpio = irq_to_gpio(bank, d->irq);
> +	unsigned int irq_mask = GPIO_BIT(bank, gpio);
> +	u32 trigger = irqd_get_trigger_type(d);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->lock, flags);
> +	if (trigger)
> +		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
> +	_clear_gpio_irqstatus(bank, gpio);
> +	_set_gpio_irqenable(bank, gpio, 1);
> +	spin_unlock_irqrestore(&bank->lock, flags);
> +}
> +
>  static struct irq_chip gpio_irq_chip = {
>  	.name		= "GPIO",
>  	.irq_shutdown	= gpio_irq_shutdown,
> @@ -815,6 +864,8 @@ static struct irq_chip gpio_irq_chip = {
>  	.irq_unmask	= gpio_unmask_irq,
>  	.irq_set_type	= gpio_irq_type,
>  	.irq_set_wake	= gpio_wake_enable,
> +	.irq_disable    = gpio_disable_irq,
> +	.irq_enable     = gpio_enable_irq,
>  };

I tested the above patch on some omap2/3/4 boards and this appears to
break ethernet support (which is using a gpio as an interrupt). I am
guessing this is related to the wake-up enable, so this needs some more
work.

Cheers
Jon