diff mbox

gpio/omap: implement irq_enable/disable using mask/unmask.

Message ID 1355736437-14186-1-git-send-email-andreas.fenkart@streamunlimited.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andreas Fenkart Dec. 17, 2012, 9:27 a.m. UTC
Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
---
 drivers/gpio/gpio-omap.c |    2 ++
 1 file changed, 2 insertions(+)

Comments

Santosh Shilimkar Dec. 20, 2012, 5:59 a.m. UTC | #1
On Monday 17 December 2012 02:57 PM, Andreas Fenkart wrote:

Please add some changelog here too.

> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> ---
Patch seems straight forward thought will be interesting where you found
the need of it.

>   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 d335af1..c1951ec 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,
> +	.irq_enable     = gpio_unmask_irq,
>   };
>
>   /*---------------------------------------------------------------------*/
>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hunter, Jon Dec. 20, 2012, 4:16 p.m. UTC | #2
On 12/19/2012 11:59 PM, Santosh Shilimkar wrote:
> On Monday 17 December 2012 02:57 PM, Andreas Fenkart wrote:
> 
> Please add some changelog here too.
> 
>> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
>> ---
> Patch seems straight forward thought will be interesting where you found
> the need of it.

The only item that I was thinking of if the behaviour of mask/unmask
should be different from enable/disable?

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.

I am also interested in the need for this, and if we should have a true
enable/disable here.

Cheers
Jon

> 
>>   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 d335af1..c1951ec 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,
>> +    .irq_enable     = gpio_unmask_irq,
>>   };
>>
>>  
>> /*---------------------------------------------------------------------*/
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Fenkart March 25, 2013, 10:24 p.m. UTC | #3
Hi,

On Thu, Dec 20, 2012 at 10:16:56AM -0600, Jon Hunter wrote:
> 
> On 12/19/2012 11:59 PM, Santosh Shilimkar wrote:
> > On Monday 17 December 2012 02:57 PM, Andreas Fenkart wrote:
> > 
> > Please add some changelog here too.

::

In pm suspend the omap_hsmmc driver can't detect SDIO IRQs. This
is worked around by reconfiguring the SDIO IRQ pin (dat1) as a
GPIO before entering suspend and back upon resume. This requires
low overhead functions for enable/disable of GPIO IRQs.

> > 
> >> Signed-off-by: Andreas Fenkart <andreas.fenkart@streamunlimited.com>
> >> ---
> > Patch seems straight forward thought will be interesting where you found
> > the need of it.
> 
> The only item that I was thinking of if the behaviour of mask/unmask
> should be different from enable/disable?
> 
> 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.

Thanks for clarification.

The HW seems to support this, see Fig 25-3 in the manual. 
https://www.google.com/search?q=spruh73c

This problem though, applies only to edge triggered irqs.

In the case of level triggered the mask/unmask implementation
clears irqs upon resume. This is safe, because if the level still
indicates irq, it will be latched again and if not, the HW needs
no service.

For edge triggered the current implementation of mask/unmask
seems to behave more like enable/disable:

- irq detection is disabled during the masking period, which is
  like disable according above description. But leaves a small
  window for latching another one that is not served immediately,
  which is more like masking

	static void gpio_mask_irq(struct irq_data *d)                                                      
	{
													   
		spin_lock_irqsave(&bank->lock, flags);
		_set_gpio_irqenable(bank, gpio, 0);
                <--- new irqs latched here --->
		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
		spin_unlock_irqrestore(&bank->lock, flags);
	}

- does not clear latched irqs upon resume. So not all irq raised
  after unmask are new, on the other hand some irqs that
  occured during the masking period might be lost.

> I am also interested in the need for this, and if we should have a true
> enable/disable here.

Since SDIO irqs are level triggered, I'm still okay with my patch.
For edge triggered though, it probably needs some more thoughts.

Suggestions? Should I split masking/disabling functions and make
them behave properly according above semantics or are you fine with
the interim solution of mapping enable/disable to mask/unmask?


rgds,
Andi


> >>   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 d335af1..c1951ec 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,
> >> +    .irq_enable     = gpio_unmask_irq,
> >>   };
> >>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index d335af1..c1951ec 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,
+	.irq_enable     = gpio_unmask_irq,
 };
 
 /*---------------------------------------------------------------------*/