On Thu, Dec 24, 2015 at 07:31:37PM +0000, Mark Brown wrote:
> On Wed, Dec 23, 2015 at 09:58:06AM +0000, Charles Keepax wrote:
> > 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 int wm_adsp_buffer_ack_irq(struct wm_adsp_compr_buf *buf)
> > > > +{
> > > > + if (buf->irq_ack & 0x01)
>
> > > 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.
>
> This doesn't sound like it's really acknowledging an IRQ - you have
> level triggered interrupts here so if the interrupt isn't acknowledged
> the interrupt handler will constantly be called.
It kinda is acking the IRQ just at the firmware level, not the
hardware level. The physical IRQ all gets acked through regmap
so that is all handled. This code here lets the firmware know,
which it will then use to decide whether it should send a new IRQ
or not.
I could perhaps rename the function to
wm_adsp_buffer_request_irq? and buf->irq_ack to buf->irq_count?
That might make the usage a little more clear.
Thanks,
Charles
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
