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

[Felipe Balbi <balbi@ti.com>]

[-- 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 --]