Re: [PATCH 5/6 v2] sound: Add n64 driver

[Takashi Iwai <tiwai@suse.de>]

On Mon, 11 Jan 2021 10:43:23 +0100,
Lauri Kasanen wrote:
> 
> On Mon, 11 Jan 2021 09:05:04 +0100
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Sun, 10 Jan 2021 18:41:46 +0100,
> > Lauri Kasanen wrote:
> > > It was returning nextpos, but the pointer printk was in bytes. 8192
> > > bytes = 2048 frames.
> >
> > OK, then it must be right.
> >
> > Then I suppose that the update of pos should be changed in a different
> > way; it should always point to the previous nextpos.  That is,
> > something like:
> >
> > static void n64audio_push(struct n64audio_t *priv, uint8_t irq)
> > {
> > 	....
> > 	if (irq)
> > 		priv->chan.pos = priv->chan.nextpos;
> > 	priv->chan.nextpos += count;
> > 	priv->chan.nextpos %= priv->chan.bufsize;
> >
> > If we use nextpos as the position, it'll lead to the double steps at
> > the first IRQ handling without snd_pcm_period_elapsed() call (the
> > first step missed it), and this may confuse PCM core.
> 
> This almost works, speed is correct, but the last part is played twice.

Oh yes, at the last IRQ, the push should be avoided.
I guess that the code order should be changed to the following way:

  1. advance the position for a period size
  2. call snd_pcm_period_elapsed()
  3. check if the stream is still running
  4. copy the next chunk and update nextpos

So, it's something like:

static irqreturn_t n64audio_isr(int irq, void *dev_id)
{
	struct n64audio *priv = dev_id;

	// Check it's ours
	const u32 intrs = n64mi_read_reg(MI_INTR_REG);
	if (!(intrs & MI_INTR_AI))
		return IRQ_NONE;

	n64audio_write_reg(AI_STATUS_REG, 1);

	priv->chan.pos += priv->chan.nextpos;
	snd_pcm_period_elapsed(priv->chan.substream);
	if (priv->chan.substream == SNDRV_PCM_STATE_RUNNING)
		n64audio_push(priv);

	return IRQ_HANDLED;
}

By calling snd_pcm_period_elapsed(), PCM core detects the end of the
stream and it stops with the state change.  Then we can avoid the
unnecessary push after the stop.

The irq argument in n64audio_push() is dropped in the above, as it's
superfluous now.


Takashi