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

[Andreas Fenkart <andreas.fenkart@streamunlimited.com>]

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,
> >>   };
> >>