Hi Javier
On Thu, 9 Feb 2012, javier Martin wrote:
> Hi Guennadi,
> I understand you are probably quite busy right now but it would be
> great if you could ack this patch. The sooner you merge it the sooner
> I will start working on the cleanup series. I've got some time on my
> hands now.
Yes, I can take this version, at the same time, I have a couple of
comments, that you might find useful to address in a clean-up patch;-) Or
just leave them as they are...
[anip]
> > @@ -1274,6 +1298,15 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
> > struct mx2_camera_dev *pcdev = data;
> > unsigned int status = readl(pcdev->base_emma + PRP_INTRSTATUS);
> > struct mx2_buffer *buf;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&pcdev->lock, flags);
It wasn't an accident, that I wrote "spin_lock()" - without the "_irqsave"
part. You are in an ISR here, and this is the only IRQ, that your driver
has to protect against, so, here, I think, you don't have to block other
IRQs.
> > +
> > + if (list_empty(&pcdev->active_bufs)) {
> > + dev_warn(pcdev->dev, "%s: called while active list is empty\n",
> > + __func__);
> > + goto irq_ok;
This means, you return IRQ_HANDLED here without even checking whether any
of your status bits are actually set. So, if you get an interrupt here
with an empty list, it might indeed be the case of a shared IRQ, in which
case you'd have to return IRQ_NONE.
> > + }
> >
> > if (status & (1 << 7)) { /* overflow */
> > u32 cntl;
As I said - we can keep this version, but maybe you'll like to improve at
least the latter of the above two snippets.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
