On Wed, Dec 23, 2015 at 12:19:07AM +0000, Mark Brown wrote:
> On Tue, Dec 15, 2015 at 11:29:48AM +0000, Charles Keepax wrote:
>
> > +static irqreturn_t wm5110_adsp2_irq(int irq, void *data)
> > +{
> > + struct wm5110_priv *florida = data;
> > +
> > + wm_adsp_compr_handle_irq(&florida->core.adsp[2]);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> We unconditionally handle the IRQ...
>
> > +int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
> > +{
> > + struct wm_adsp_compr_buf *buf = dsp->buffer;
> > + int ret = 0;
> > +
> > + mutex_lock(&dsp->pwr_lock);
> > +
> > + if (!buf) {
> > + adsp_err(dsp, "Spurious buffer IRQ\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
>
> ...though we even have code to handle spurious IRQs. I'd expect
> IRQ_NONE if the interrupt wasn't handled, allowing genirq's error
> handling and diagnostics to take effect.
Yeah that seems sensible I will fix that up.
>
> > +static int wm_adsp_buffer_ack_irq(struct wm_adsp_compr_buf *buf)
> > +{
> > + if (buf->irq_ack & 0x01)
> > + return 0;
> > +
> > + adsp_dbg(buf->dsp, "Acking buffer IRQ(0x%x)\n", buf->irq_ack);
> > +
> > + buf->irq_ack |= 0x01;
> > +
> > + return wm_adsp_buffer_write(buf, HOST_BUFFER_FIELD(irq_ack),
> > + buf->irq_ack);
> > +}
>
> This is confusing, this isn't actually in the interrupt path...
Acking the last IRQ basically tells the firmware that it is free
to send another. There is no point in doing so until we have
to wait for data. As we are just actively streaming available
data we can progress along fine without enabling the IRQ.
I am somewhat torn between a comment and renaming the function. I
will try to add some sort of reasonable comment.
Thanks,
Charles
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
