The interrupt handler always provide runtime->period_size data, but it
should provide additional residual data when the pointer back to zero.
This patch fixes periodic click noise when runtime->buffer_size was
not multiple of runtime->period_size.
Signed-off-by: Atsushi Nemoto <[email protected]>
---
This patch depends on a patch titled "at73c213: Monaural support".
diff --git a/sound/spi/at73c213.c b/sound/spi/at73c213.c
index 7c077c6..d614a8c 100644
--- a/sound/spi/at73c213.c
+++ b/sound/spi/at73c213.c
@@ -355,6 +355,7 @@ static irqreturn_t snd_at73c213_interrupt(int irq, void *dev_id)
u32 status;
int offset;
int block_size;
+ int size;
int next_period;
int retval = IRQ_NONE;
@@ -372,11 +373,14 @@ static irqreturn_t snd_at73c213_interrupt(int irq, void *dev_id)
next_period = 0;
offset = block_size * next_period;
+ size = runtime->period_size * runtime->channels;
+ if (next_period == runtime->periods - 1)
+ size += (runtime->buffer_size % runtime->period_size)
+ * runtime->channels;
ssc_writel(chip->ssc->regs, PDC_TNPR,
(long)runtime->dma_addr + offset);
- ssc_writel(chip->ssc->regs, PDC_TNCR,
- runtime->period_size * runtime->channels);
+ ssc_writel(chip->ssc->regs, PDC_TNCR, size);
retval = IRQ_HANDLED;
}
On Mon, 10 Mar 2008 23:43:06 +0900 (JST)
Atsushi Nemoto <[email protected]> wrote:
> + size = runtime->period_size * runtime->channels;
> + if (next_period == runtime->periods - 1)
> + size += (runtime->buffer_size % runtime->period_size)
> + * runtime->channels;
Ow. That looks expensive. Isn't there any way we can force the client
to select sane values of buffer_size and period_size?
It seems like a reasonable demand that buffer_size is a multiple of
period_size, doesn't it?
Haavard
On Fri, 14 Mar 2008 10:44:45 +0100, Haavard Skinnemoen <[email protected]> wrote:
> > + size = runtime->period_size * runtime->channels;
> > + if (next_period == runtime->periods - 1)
> > + size += (runtime->buffer_size % runtime->period_size)
> > + * runtime->channels;
>
> Ow. That looks expensive. Isn't there any way we can force the client
> to select sane values of buffer_size and period_size?
Well, I suppose it is not _too_ expensive. :)
> It seems like a reasonable demand that buffer_size is a multiple of
> period_size, doesn't it?
But actually it can happen. And I gave up understanding how are these
parameters determined... If there were any way the driver can enforce
that constraint, it would be better fix.
Iwai-san, any comments from alsa guru?
---
Atsushi Nemoto
At Fri, 14 Mar 2008 22:29:32 +0900 (JST),
Atsushi Nemoto wrote:
>
> On Fri, 14 Mar 2008 10:44:45 +0100, Haavard Skinnemoen <[email protected]> wrote:
> > > + size = runtime->period_size * runtime->channels;
> > > + if (next_period == runtime->periods - 1)
> > > + size += (runtime->buffer_size % runtime->period_size)
> > > + * runtime->channels;
> >
> > Ow. That looks expensive. Isn't there any way we can force the client
> > to select sane values of buffer_size and period_size?
>
> Well, I suppose it is not _too_ expensive. :)
>
> > It seems like a reasonable demand that buffer_size is a multiple of
> > period_size, doesn't it?
>
> But actually it can happen. And I gave up understanding how are these
> parameters determined... If there were any way the driver can enforce
> that constraint, it would be better fix.
>
> Iwai-san, any comments from alsa guru?
Add the following constraint in the open callback:
err = snd_pcm_hw_constraint_integer(runtime,
SNDRV_PCM_HW_PARAM_PERIODS);
if (err < 0)
return err;
This will guarantee that the period size fits with the buffer size.
Takashi
On Fri, 14 Mar 2008 14:39:42 +0100, Takashi Iwai <[email protected]> wrote:
> Add the following constraint in the open callback:
>
> err = snd_pcm_hw_constraint_integer(runtime,
> SNDRV_PCM_HW_PARAM_PERIODS);
> if (err < 0)
> return err;
>
> This will guarantee that the period size fits with the buffer size.
Thank you! It works fine. Here is a new patch.
------------------------------------------------------
Subject: [PATCH] at73c213: Add constraints for periods value
From: Atsushi Nemoto <[email protected]>
The interrupt handler always provide runtime->period_size data, so it
works correctly only if buffer_size was a multiple of period_size.
This patch fixes periodic click noise.
Signed-off-by: Atsushi Nemoto <[email protected]>
---
This patch obsoletes a patch titled "at73c213: fix DMA size at the end
of DMA buffer" in git-alsa-tiwai.patch in mm tree.
diff --git a/sound/spi/at73c213.c b/sound/spi/at73c213.c
index 7c077c6..9a5c118 100644
--- a/sound/spi/at73c213.c
+++ b/sound/spi/at73c213.c
@@ -210,7 +210,13 @@ static int snd_at73c213_pcm_open(struct snd_pcm_substream *substream)
{
struct snd_at73c213 *chip = snd_pcm_substream_chip(substream);
struct snd_pcm_runtime *runtime = substream->runtime;
+ int err;
+ /* ensure buffer_size is a multiple of period_size */
+ err = snd_pcm_hw_constraint_integer(runtime,
+ SNDRV_PCM_HW_PARAM_PERIODS);
+ if (err < 0)
+ return err;
snd_at73c213_playback_hw.rate_min = chip->bitrate;
snd_at73c213_playback_hw.rate_max = chip->bitrate;
runtime->hw = snd_at73c213_playback_hw;
At Mon, 17 Mar 2008 22:00:27 +0900 (JST),
Atsushi Nemoto wrote:
>
> On Fri, 14 Mar 2008 14:39:42 +0100, Takashi Iwai <[email protected]> wrote:
> > Add the following constraint in the open callback:
> >
> > err = snd_pcm_hw_constraint_integer(runtime,
> > SNDRV_PCM_HW_PARAM_PERIODS);
> > if (err < 0)
> > return err;
> >
> > This will guarantee that the period size fits with the buffer size.
>
> Thank you! It works fine. Here is a new patch.
Thanks for the patch. So, I should revert your last patch, right?
It's already on ALSA tree...
Takashi
>
> ------------------------------------------------------
> Subject: [PATCH] at73c213: Add constraints for periods value
> From: Atsushi Nemoto <[email protected]>
>
> The interrupt handler always provide runtime->period_size data, so it
> works correctly only if buffer_size was a multiple of period_size.
>
> This patch fixes periodic click noise.
>
> Signed-off-by: Atsushi Nemoto <[email protected]>
> ---
> This patch obsoletes a patch titled "at73c213: fix DMA size at the end
> of DMA buffer" in git-alsa-tiwai.patch in mm tree.
>
> diff --git a/sound/spi/at73c213.c b/sound/spi/at73c213.c
> index 7c077c6..9a5c118 100644
> --- a/sound/spi/at73c213.c
> +++ b/sound/spi/at73c213.c
> @@ -210,7 +210,13 @@ static int snd_at73c213_pcm_open(struct snd_pcm_substream *substream)
> {
> struct snd_at73c213 *chip = snd_pcm_substream_chip(substream);
> struct snd_pcm_runtime *runtime = substream->runtime;
> + int err;
>
> + /* ensure buffer_size is a multiple of period_size */
> + err = snd_pcm_hw_constraint_integer(runtime,
> + SNDRV_PCM_HW_PARAM_PERIODS);
> + if (err < 0)
> + return err;
> snd_at73c213_playback_hw.rate_min = chip->bitrate;
> snd_at73c213_playback_hw.rate_max = chip->bitrate;
> runtime->hw = snd_at73c213_playback_hw;
>
On Mon, 17 Mar 2008 14:21:16 +0100, Takashi Iwai <[email protected]> wrote:
> > Thank you! It works fine. Here is a new patch.
>
> Thanks for the patch. So, I should revert your last patch, right?
> It's already on ALSA tree...
Yes, please. Thank you.
---
Atsushi Nemoto
At Mon, 17 Mar 2008 22:32:29 +0900 (JST),
Atsushi Nemoto wrote:
>
> On Mon, 17 Mar 2008 14:21:16 +0100, Takashi Iwai <[email protected]> wrote:
> > > Thank you! It works fine. Here is a new patch.
> >
> > Thanks for the patch. So, I should revert your last patch, right?
> > It's already on ALSA tree...
>
> Yes, please. Thank you.
Done.
thanks,
Takashi