[PATCH] ASoC: MPC5200: Support for buffer wrap around

[PATCH] ASoC: MPC5200: Support for buffer wrap around

[John Bonesio]

We've encountered strange behavior in the alsamixer settings using the wm9712
codec. If we unmute the headphone output and then unmute the PCM output, the
headphone output gets reset to mute in the hardware register. At this point
the hardware register does not match the value in the register cache.

I've spent some time debugging this, and the headphone setting is set outside
of any code path that would call the ac97_write() routine. As best as I can
tell, there is something strange going on in hardware.

I've provided this patch that works around the problem.

Have any of you seen this before? Is this patch the right approach?

- John

The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
s->runtime->control->appl_ptr can wrap around to the beginning of the
buffer. This change fixes this problem.

Signed-off-by: John Bonesio <bones@secretlab.ca>
---

 sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
index cfe0ea4..2551c58 100644
--- a/sound/soc/fsl/mpc5200_dma.c
+++ b/sound/soc/fsl/mpc5200_dma.c
@@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
 
 static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
 {
+	if (s->appl_ptr > s->runtime->control->appl_ptr) {
+		/*
+		 * In this case s->runtime->control->appl_ptr has wrapped around.
+		 * Play the data to the end of the boundary, then wrap our own
+		 * appl_ptr back around.
+		 */
+		while (s->appl_ptr < s->runtime->boundary) {
+			if (bcom_queue_full(s->bcom_task))
+				return;
+
+			s->appl_ptr += s->period_size;
+
+			psc_dma_bcom_enqueue_next_buffer(s);
+		}
+		s->appl_ptr -= s->runtime->boundary;
+	}
+
 	while (s->appl_ptr < s->runtime->control->appl_ptr) {
 
 		if (bcom_queue_full(s->bcom_task))

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Grant Likely]

On Friday, July 31, 2009, John Bonesio <bones@secretlab.ca> wrote:
> We've encountered strange behavior in the alsamixer settings using the wm9712
> codec. If we unmute the headphone output and then unmute the PCM output, the
> headphone output gets reset to mute in the hardware register. At this point
> the hardware register does not match the value in the register cache.
>
> I've spent some time debugging this, and the headphone setting is set outside
> of any code path that would call the ac97_write() routine. As best as I can
> tell, there is something strange going on in hardware.
>
> I've provided this patch that works around the problem.
>
> Have any of you seen this before? Is this patch the right approach?
>
> - John
>
> The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
> s->runtime->control->appl_ptr can wrap around to the beginning of the
> buffer. This change fixes this problem.
>
> Signed-off-by: John Bonesio <bones@secretlab.ca>

Hey John,

It would appear that you've attached the wrong patch.

g.

> ---
>
>  sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index cfe0ea4..2551c58 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
>
>  static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
>  {
> +       if (s->appl_ptr > s->runtime->control->appl_ptr) {
> +               /*
> +                * In this case s->runtime->control->appl_ptr has wrapped around.
> +                * Play the data to the end of the boundary, then wrap our own
> +                * appl_ptr back around.
> +                */
> +               while (s->appl_ptr < s->runtime->boundary) {
> +                       if (bcom_queue_full(s->bcom_task))
> +                               return;
> +
> +                       s->appl_ptr += s->period_size;
> +
> +                       psc_dma_bcom_enqueue_next_buffer(s);
> +               }
> +               s->appl_ptr -= s->runtime->boundary;
> +       }
> +
>         while (s->appl_ptr < s->runtime->control->appl_ptr) {
>
>                 if (bcom_queue_full(s->bcom_task))
>
>

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Mark Brown]

On Fri, Jul 31, 2009 at 04:08:55PM -0700, John Bonesio wrote:
> We've encountered strange behavior in the alsamixer settings using the wm9712
> codec. If we unmute the headphone output and then unmute the PCM output, the
> headphone output gets reset to mute in the hardware register. At this point
> the hardware register does not match the value in the register cache.

Which specific controls are you looking at here?

> I've spent some time debugging this, and the headphone setting is set outside
> of any code path that would call the ac97_write() routine. As best as I can
> tell, there is something strange going on in hardware.

> I've provided this patch that works around the problem.

Like Grant says I suspect this isn't the patch you meant...

> Have any of you seen this before? Is this patch the right approach?

I've never heard of that before and the WM9712 is very widely deployed
so if it were a hardware issue in the CODEC I'd imagine it would have
come up.  On the other hand, it's not beyond the bounds of possibility.
If you could post the patch showing exactly what you're doing I could
probably say more.

If you could get a scope on the bus that might be interesting...

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Jon Smirl]

On Fri, Jul 31, 2009 at 7:08 PM, John Bonesio<bones@secretlab.ca> wrote:
> We've encountered strange behavior in the alsamixer settings using the wm9712
> codec. If we unmute the headphone output and then unmute the PCM output, the
> headphone output gets reset to mute in the hardware register. At this point
> the hardware register does not match the value in the register cache.
>
> I've spent some time debugging this, and the headphone setting is set outside
> of any code path that would call the ac97_write() routine. As best as I can
> tell, there is something strange going on in hardware.

This could be something wrong in the mpc5200 code that writes the AC97
registers too. Maybe a register write command is getting generated
when it wasn't supposed to.

You can set the mpc5200 to generate an interrupt everything it
finishes writing a AC97 register. If you get more interrupts than you
expected the problem is in the driver.

> I've provided this patch that works around the problem.
>
> Have any of you seen this before? Is this patch the right approach?
>
> - John
>
> The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
> s->runtime->control->appl_ptr can wrap around to the beginning of the
> buffer. This change fixes this problem.
>
> Signed-off-by: John Bonesio <bones@secretlab.ca>
> ---
>
>  sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index cfe0ea4..2551c58 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
> @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
>
>  static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
>  {
> +       if (s->appl_ptr > s->runtime->control->appl_ptr) {
> +               /*
> +                * In this case s->runtime->control->appl_ptr has wrapped around.
> +                * Play the data to the end of the boundary, then wrap our own
> +                * appl_ptr back around.
> +                */
> +               while (s->appl_ptr < s->runtime->boundary) {
> +                       if (bcom_queue_full(s->bcom_task))
> +                               return;
> +
> +                       s->appl_ptr += s->period_size;
> +
> +                       psc_dma_bcom_enqueue_next_buffer(s);
> +               }
> +               s->appl_ptr -= s->runtime->boundary;
> +       }
> +
>        while (s->appl_ptr < s->runtime->control->appl_ptr) {
>
>                if (bcom_queue_full(s->bcom_task))
>
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[John Bonesio]

[-- Attachment #1: Type: text/plain, Size: 5434 bytes --]

Hi All,

Woops! Yep, this is the wrong patch. I've included the correct one. I've
also attached the patch in case email messes with its formatting. Sorry
about the confusion.

Jon,

I put in a debug hook in the out_be32() routine. It would printk output
anytime the AC97_HEADPHONE register was written to. This printk text
would come out as expected when the Headphone mixer setting was
muted/unmuted.

When I umuted the PCM mixer setting, this printk text did not appear,
yet when I read the mixer register setting for the headphone, it was set
back to mute.

If there is code in software doing this, it's very subtle.

- John

----- correct patch -----

>From 24b52cf2836f711118fd6d2c8895c91c944978cd Mon Sep 17 00:00:00 2001
From: John Bonesio <bones@secretlab.ca>
Date: Fri, 31 Jul 2009 16:01:33 -0700
Subject: [PATCH] ASoC: WM9712 Codec: Workaround an unmute  problem

When setting the PCM mixer (unuting), the main Headphone mixer setting gets
set back to mute. At this time it appears this is occuring in hardware.

Signed-off-by: John Bonesio <bones@secretlab.ca>
---
 sound/soc/codecs/wm9712.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index b57c817..6f8d164 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -28,6 +28,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec,
 	unsigned int reg);
 static int ac97_write(struct snd_soc_codec *codec,
 	unsigned int reg, unsigned int val);
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg);
 
 /*
  * WM9712 register cache
@@ -177,9 +178,14 @@ static int mixer_event(struct snd_soc_dapm_widget *w,
 	else
 		ac97_write(w->codec, AC97_VIDEO, mic | 0x8000);
 
-	if (l & 0x2 || r & 0x2)
+	if (l & 0x2 || r & 0x2) {
 		ac97_write(w->codec, AC97_PCM, pcm & 0x7fff);
-	else
+		/*
+		 * Workaround an apparent bug where the headphone mute setting
+		 * is modified when the PCM mute setting is enabled.
+		 */
+		ac97_flush(w->codec, AC97_HEADPHONE);
+	} else
 		ac97_write(w->codec, AC97_PCM, pcm | 0x8000);
 
 	if (l & 0x4 || r & 0x4)
@@ -472,6 +478,17 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
 	return 0;
 }
 
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg)
+{
+	unsigned int val;
+	u16 *cache = codec->reg_cache;
+
+	if ((reg >> 1) < (ARRAY_SIZE(wm9712_reg))) {
+		val = cache[reg >> 1];
+		soc_ac97_ops.write(codec->ac97, reg, val);
+	}
+}
+
 static int ac97_prepare(struct snd_pcm_substream *substream,
 			struct snd_soc_dai *dai)
 {
-- 
1.6.0.4


-------------------------



On Sat, 2009-08-01 at 09:47 -0400, Jon Smirl wrote:
> On Fri, Jul 31, 2009 at 7:08 PM, John Bonesio<bones@secretlab.ca> wrote:
> > We've encountered strange behavior in the alsamixer settings using the wm9712
> > codec. If we unmute the headphone output and then unmute the PCM output, the
> > headphone output gets reset to mute in the hardware register. At this point
> > the hardware register does not match the value in the register cache.
> >
> > I've spent some time debugging this, and the headphone setting is set outside
> > of any code path that would call the ac97_write() routine. As best as I can
> > tell, there is something strange going on in hardware.
> 
> This could be something wrong in the mpc5200 code that writes the AC97
> registers too. Maybe a register write command is getting generated
> when it wasn't supposed to.
> 
> You can set the mpc5200 to generate an interrupt everything it
> finishes writing a AC97 register. If you get more interrupts than you
> expected the problem is in the driver.
> 
> > I've provided this patch that works around the problem.
> >
> > Have any of you seen this before? Is this patch the right approach?
> >
> > - John
> >
> > The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
> > s->runtime->control->appl_ptr can wrap around to the beginning of the
> > buffer. This change fixes this problem.
> >
> > Signed-off-by: John Bonesio <bones@secretlab.ca>
> > ---
> >
> >  sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
> >  1 files changed, 17 insertions(+), 0 deletions(-)
> >
> > diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> > index cfe0ea4..2551c58 100644
> > --- a/sound/soc/fsl/mpc5200_dma.c
> > +++ b/sound/soc/fsl/mpc5200_dma.c
> > @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
> >
> >  static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
> >  {
> > +       if (s->appl_ptr > s->runtime->control->appl_ptr) {
> > +               /*
> > +                * In this case s->runtime->control->appl_ptr has wrapped around.
> > +                * Play the data to the end of the boundary, then wrap our own
> > +                * appl_ptr back around.
> > +                */
> > +               while (s->appl_ptr < s->runtime->boundary) {
> > +                       if (bcom_queue_full(s->bcom_task))
> > +                               return;
> > +
> > +                       s->appl_ptr += s->period_size;
> > +
> > +                       psc_dma_bcom_enqueue_next_buffer(s);
> > +               }
> > +               s->appl_ptr -= s->runtime->boundary;
> > +       }
> > +
> >        while (s->appl_ptr < s->runtime->control->appl_ptr) {
> >
> >                if (bcom_queue_full(s->bcom_task))
> >
> >
> 
> 
> 

[-- Attachment #2: 0001-ASoC-WM9712-Codec-Workaround-an-unmute-problem.patch --]
[-- Type: text/x-patch, Size: 1958 bytes --]

>From 24b52cf2836f711118fd6d2c8895c91c944978cd Mon Sep 17 00:00:00 2001
From: John Bonesio <bones@secretlab.ca>
Date: Fri, 31 Jul 2009 16:01:33 -0700
Subject: [PATCH] ASoC: WM9712 Codec: Workaround an unmute  problem

When setting the PCM mixer (unuting), the main Headphone mixer setting gets
set back to mute. At this time it appears this is occuring in hardware.

Signed-off-by: John Bonesio <bones@secretlab.ca>
---
 sound/soc/codecs/wm9712.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index b57c817..6f8d164 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -28,6 +28,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec,
 	unsigned int reg);
 static int ac97_write(struct snd_soc_codec *codec,
 	unsigned int reg, unsigned int val);
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg);
 
 /*
  * WM9712 register cache
@@ -177,9 +178,14 @@ static int mixer_event(struct snd_soc_dapm_widget *w,
 	else
 		ac97_write(w->codec, AC97_VIDEO, mic | 0x8000);
 
-	if (l & 0x2 || r & 0x2)
+	if (l & 0x2 || r & 0x2) {
 		ac97_write(w->codec, AC97_PCM, pcm & 0x7fff);
-	else
+		/*
+		 * Workaround an apparent bug where the headphone mute setting
+		 * is modified when the PCM mute setting is enabled.
+		 */
+		ac97_flush(w->codec, AC97_HEADPHONE);
+	} else
 		ac97_write(w->codec, AC97_PCM, pcm | 0x8000);
 
 	if (l & 0x4 || r & 0x4)
@@ -472,6 +478,17 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
 	return 0;
 }
 
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg)
+{
+	unsigned int val;
+	u16 *cache = codec->reg_cache;
+
+	if ((reg >> 1) < (ARRAY_SIZE(wm9712_reg))) {
+		val = cache[reg >> 1];
+		soc_ac97_ops.write(codec->ac97, reg, val);
+	}
+}
+
 static int ac97_prepare(struct snd_pcm_substream *substream,
 			struct snd_soc_dai *dai)
 {
-- 
1.6.0.4


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Jon Smirl]

On Sat, Aug 1, 2009 at 11:04 AM, John Bonesio<bones@secretlab.ca> wrote:
> Hi All,
>
> Woops! Yep, this is the wrong patch. I've included the correct one. I've
> also attached the patch in case email messes with its formatting. Sorry
> about the confusion.
>
> Jon,
>
> I put in a debug hook in the out_be32() routine. It would printk output
> anytime the AC97_HEADPHONE register was written to. This printk text
> would come out as expected when the Headphone mixer setting was
> muted/unmuted.

I was thinking a stale register write may have gotten into the mpc5200
AC97 hardware somehow and not been sent immediately to the codec.
Something you do later causes it to be sent.  I don't trust the
mpc5200 AC97 register write hardware 100%, it was an after thought
added in the mpc5200b.

Some logic analyzers have AC97 protocol decode capability. That would
isolate easily isolate the problem.

>
> When I umuted the PCM mixer setting, this printk text did not appear,
> yet when I read the mixer register setting for the headphone, it was set
> back to mute.
>
> If there is code in software doing this, it's very subtle.
>
> - John
>
> ----- correct patch -----
>
> >From 24b52cf2836f711118fd6d2c8895c91c944978cd Mon Sep 17 00:00:00 2001
> From: John Bonesio <bones@secretlab.ca>
> Date: Fri, 31 Jul 2009 16:01:33 -0700
> Subject: [PATCH] ASoC: WM9712 Codec: Workaround an unmute  problem
>
> When setting the PCM mixer (unuting), the main Headphone mixer setting gets
> set back to mute. At this time it appears this is occuring in hardware.
>
> Signed-off-by: John Bonesio <bones@secretlab.ca>
> ---
>  sound/soc/codecs/wm9712.c |   21 +++++++++++++++++++--
>  1 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
> index b57c817..6f8d164 100644
> --- a/sound/soc/codecs/wm9712.c
> +++ b/sound/soc/codecs/wm9712.c
> @@ -28,6 +28,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec,
>        unsigned int reg);
>  static int ac97_write(struct snd_soc_codec *codec,
>        unsigned int reg, unsigned int val);
> +static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg);
>
>  /*
>  * WM9712 register cache
> @@ -177,9 +178,14 @@ static int mixer_event(struct snd_soc_dapm_widget *w,
>        else
>                ac97_write(w->codec, AC97_VIDEO, mic | 0x8000);
>
> -       if (l & 0x2 || r & 0x2)
> +       if (l & 0x2 || r & 0x2) {
>                ac97_write(w->codec, AC97_PCM, pcm & 0x7fff);
> -       else
> +               /*
> +                * Workaround an apparent bug where the headphone mute setting
> +                * is modified when the PCM mute setting is enabled.
> +                */
> +               ac97_flush(w->codec, AC97_HEADPHONE);
> +       } else
>                ac97_write(w->codec, AC97_PCM, pcm | 0x8000);
>
>        if (l & 0x4 || r & 0x4)
> @@ -472,6 +478,17 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg,
>        return 0;
>  }
>
> +static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg)
> +{
> +       unsigned int val;
> +       u16 *cache = codec->reg_cache;
> +
> +       if ((reg >> 1) < (ARRAY_SIZE(wm9712_reg))) {
> +               val = cache[reg >> 1];
> +               soc_ac97_ops.write(codec->ac97, reg, val);
> +       }
> +}
> +
>  static int ac97_prepare(struct snd_pcm_substream *substream,
>                        struct snd_soc_dai *dai)
>  {
> --
> 1.6.0.4
>
>
> -------------------------
>
>
>
> On Sat, 2009-08-01 at 09:47 -0400, Jon Smirl wrote:
>> On Fri, Jul 31, 2009 at 7:08 PM, John Bonesio<bones@secretlab.ca> wrote:
>> > We've encountered strange behavior in the alsamixer settings using the wm9712
>> > codec. If we unmute the headphone output and then unmute the PCM output, the
>> > headphone output gets reset to mute in the hardware register. At this point
>> > the hardware register does not match the value in the register cache.
>> >
>> > I've spent some time debugging this, and the headphone setting is set outside
>> > of any code path that would call the ac97_write() routine. As best as I can
>> > tell, there is something strange going on in hardware.
>>
>> This could be something wrong in the mpc5200 code that writes the AC97
>> registers too. Maybe a register write command is getting generated
>> when it wasn't supposed to.
>>
>> You can set the mpc5200 to generate an interrupt everything it
>> finishes writing a AC97 register. If you get more interrupts than you
>> expected the problem is in the driver.
>>
>> > I've provided this patch that works around the problem.
>> >
>> > Have any of you seen this before? Is this patch the right approach?
>> >
>> > - John
>> >
>> > The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that
>> > s->runtime->control->appl_ptr can wrap around to the beginning of the
>> > buffer. This change fixes this problem.
>> >
>> > Signed-off-by: John Bonesio <bones@secretlab.ca>
>> > ---
>> >
>> >  sound/soc/fsl/mpc5200_dma.c |   17 +++++++++++++++++
>> >  1 files changed, 17 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
>> > index cfe0ea4..2551c58 100644
>> > --- a/sound/soc/fsl/mpc5200_dma.c
>> > +++ b/sound/soc/fsl/mpc5200_dma.c
>> > @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
>> >
>> >  static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s)
>> >  {
>> > +       if (s->appl_ptr > s->runtime->control->appl_ptr) {
>> > +               /*
>> > +                * In this case s->runtime->control->appl_ptr has wrapped around.
>> > +                * Play the data to the end of the boundary, then wrap our own
>> > +                * appl_ptr back around.
>> > +                */
>> > +               while (s->appl_ptr < s->runtime->boundary) {
>> > +                       if (bcom_queue_full(s->bcom_task))
>> > +                               return;
>> > +
>> > +                       s->appl_ptr += s->period_size;
>> > +
>> > +                       psc_dma_bcom_enqueue_next_buffer(s);
>> > +               }
>> > +               s->appl_ptr -= s->runtime->boundary;
>> > +       }
>> > +
>> >        while (s->appl_ptr < s->runtime->control->appl_ptr) {
>> >
>> >                if (bcom_queue_full(s->bcom_task))
>> >
>> >
>>
>>
>>
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around

[Mark Brown]

On Sat, Aug 01, 2009 at 08:04:56AM -0700, John Bonesio wrote:

> When I umuted the PCM mixer setting, this printk text did not appear,
> yet when I read the mixer register setting for the headphone, it was set
> back to mute.

Like I said before, exactly which control are you adjusting here?

> If there is code in software doing this, it's very subtle.

My money would be on the AC97 controller having problems; the quality of
SoC AC97 controllers is variable.  It certainly doesn't sound like a
WM9712 issue; as I say I'd be very surprised if such an issue hadn't
come up before given how widely deployed the part is.

Re: [PATCH] ASoC: WM9712 Codec: Workaround an unmute problem (Was: Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around)

[John Bonesio]

Hi Mark,

I've retitled the email to better reflect the real patch. I believe
there has been some general confusion because I originally sent the
wrong patch.

You wrote:

> Like I said before, exactly which control are you adjusting here?
> 
My description of this got lost in all the confusion. Let me try again.
We are adjusting the mixer bits for mute/unmute on two of the mixer
settings. The first one is general headphone mute setting on register
0x4 (bit 15). The second one is the PCM mute setting on register 0x18
(bit 15).

What we are seeing is that if we first unmute the general headphone (reg
0x4 bit15), then unmute the PCM (reg 0x18 bit 15) [HPL PCM in the
alsamixer application], the general headphone gets muted again, even
though software didn't write to that register.

> 
> > If there is code in software doing this, it's very subtle.
> 
> My money would be on the AC97 controller having problems; the quality of
> SoC AC97 controllers is variable.  It certainly doesn't sound like a
> WM9712 issue; as I say I'd be very surprised if such an issue hadn't
> come up before given how widely deployed the part is.

We don't have access to an AC97 analyzer. Do you have any suggestions on
other ways we can pinpoint the error?

- John


Here's the patch again for reference:

Author: John Bonesio <bones@secretlab.ca>
Date:   Fri Jul 31 16:01:33 2009 -0700

    ASoC: WM9712 Codec: Workaround an unmute  problem
    
    When setting the PCM mixer (unuting), the main Headphone mixer
setting gets
    set back to mute. At this time it appears this is occuring in
hardware.
    
    Signed-off-by: John Bonesio <bones@secretlab.ca>

diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index b57c817..6f8d164 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -28,6 +28,7 @@ static unsigned int ac97_read(struct snd_soc_codec
*codec,
 	unsigned int reg);
 static int ac97_write(struct snd_soc_codec *codec,
 	unsigned int reg, unsigned int val);
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg);
 
 /*
  * WM9712 register cache
@@ -177,9 +178,14 @@ static int mixer_event(struct snd_soc_dapm_widget
*w,
 	else
 		ac97_write(w->codec, AC97_VIDEO, mic | 0x8000);
 
-	if (l & 0x2 || r & 0x2)
+	if (l & 0x2 || r & 0x2) {
 		ac97_write(w->codec, AC97_PCM, pcm & 0x7fff);
-	else
+		/*
+		 * Workaround an apparent bug where the headphone mute setting
+		 * is modified when the PCM mute setting is enabled.
+		 */
+		ac97_flush(w->codec, AC97_HEADPHONE);
+	} else
 		ac97_write(w->codec, AC97_PCM, pcm | 0x8000);
 
 	if (l & 0x4 || r & 0x4)
@@ -472,6 +478,17 @@ static int ac97_write(struct snd_soc_codec *codec,
unsigned int reg,
 	return 0;
 }
 
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg)
+{
+	unsigned int val;
+	u16 *cache = codec->reg_cache;
+
+	if ((reg >> 1) < (ARRAY_SIZE(wm9712_reg))) {
+		val = cache[reg >> 1];
+		soc_ac97_ops.write(codec->ac97, reg, val);
+	}
+}
+
 static int ac97_prepare(struct snd_pcm_substream *substream,
 			struct snd_soc_dai *dai)
 {


On Sun, 2009-08-02 at 12:49 +0100, Mark Brown wrote:
> On Sat, Aug 01, 2009 at 08:04:56AM -0700, John Bonesio wrote:
> 
> > When I umuted the PCM mixer setting, this printk text did not appear,
> > yet when I read the mixer register setting for the headphone, it was set
> > back to mute.
> 
> Like I said before, exactly which control are you adjusting here?
> 
> > If there is code in software doing this, it's very subtle.
> 
> My money would be on the AC97 controller having problems; the quality of
> SoC AC97 controllers is variable.  It certainly doesn't sound like a
> WM9712 issue; as I say I'd be very surprised if such an issue hadn't
> come up before given how widely deployed the part is.

MPC5200/WM9712 mute problem

[Mark Brown]

On Wed, Aug 05, 2009 at 09:30:53AM -0700, John Bonesio wrote:

> I've retitled the email to better reflect the real patch. I believe
> there has been some general confusion because I originally sent the
> wrong patch.

Retitling again; I don't think we've diagnosed what is causing issues
here.  Doing a quick test here I wasn't able to reproduce this behaviour
so I suspect either the AC97 controller by itself or some interaction
between it and the WM9712.  Like I say, the WM9712 is not a new part and
the drivers aren't new either so it'd be surprising to find an issue
like this now.

> > Like I said before, exactly which control are you adjusting here?

> My description of this got lost in all the confusion. Let me try again.
> We are adjusting the mixer bits for mute/unmute on two of the mixer
> settings. The first one is general headphone mute setting on register
> 0x4 (bit 15). The second one is the PCM mute setting on register 0x18
> (bit 15).

Hrm, so the same bit in both registers.  Suspicious...

> What we are seeing is that if we first unmute the general headphone (reg
> 0x4 bit15), then unmute the PCM (reg 0x18 bit 15) [HPL PCM in the
> alsamixer application], the general headphone gets muted again, even
> though software didn't write to that register.

Are any other bits in register 4 affected or is it only bit 15?

> > My money would be on the AC97 controller having problems; the quality of
> > SoC AC97 controllers is variable.  It certainly doesn't sound like a
> > WM9712 issue; as I say I'd be very surprised if such an issue hadn't
> > come up before given how widely deployed the part is.

> We don't have access to an AC97 analyzer. Do you have any suggestions on
> other ways we can pinpoint the error?

Any scope that can capture would be useful to see what's going on;
obviously decode would have to be by hand.  Coding out the register
cache may also be useful - it'd allow you to see the current hardware
register values.

Re: MPC5200/WM9712 mute problem

[John Bonesio]

[-- Attachment #1: Type: text/plain, Size: 4287 bytes --]

On Wed, 2009-08-05 at 18:03 +0100, Mark Brown wrote:
> On Wed, Aug 05, 2009 at 09:30:53AM -0700, John Bonesio wrote:
> 
> > I've retitled the email to better reflect the real patch. I believe
> > there has been some general confusion because I originally sent the
> > wrong patch.
> 
> Retitling again; I don't think we've diagnosed what is causing issues
> here.  Doing a quick test here I wasn't able to reproduce this behaviour
> so I suspect either the AC97 controller by itself or some interaction
> between it and the WM9712.  Like I say, the WM9712 is not a new part and
> the drivers aren't new either so it'd be surprising to find an issue
> like this now.

Ok. At this point I'm not trying to get the patch approved so much as
I'd like to see if we can get to the root cause of the problem.

> 
> > > Like I said before, exactly which control are you adjusting here?
> 
> > My description of this got lost in all the confusion. Let me try again.
> > We are adjusting the mixer bits for mute/unmute on two of the mixer
> > settings. The first one is general headphone mute setting on register
> > 0x4 (bit 15). The second one is the PCM mute setting on register 0x18
> > (bit 15).
> 
> Hrm, so the same bit in both registers.  Suspicious...
> 
> > What we are seeing is that if we first unmute the general headphone (reg
> > 0x4 bit15), then unmute the PCM (reg 0x18 bit 15) [HPL PCM in the
> > alsamixer application], the general headphone gets muted again, even
> > though software didn't write to that register.
> 
> Are any other bits in register 4 affected or is it only bit 15?

So far, I've only seen this one bit affected. There are lots of
combinations that could be tested, and I've not done comprehensive test
of every bit. It's possible that other bits are affected in ways that
don't show up in my testing.

Below I describe in more detail how I've tested.

> 
> > > My money would be on the AC97 controller having problems; the quality of
> > > SoC AC97 controllers is variable.  It certainly doesn't sound like a
> > > WM9712 issue; as I say I'd be very surprised if such an issue hadn't
> > > come up before given how widely deployed the part is.
> 
> > We don't have access to an AC97 analyzer. Do you have any suggestions on
> > other ways we can pinpoint the error?
> 
> Any scope that can capture would be useful to see what's going on;
> obviously decode would have to be by hand.  Coding out the register
> cache may also be useful - it'd allow you to see the current hardware
> register values.

In case anyone else sees this problem, and would like to jump in, here
is what I've done to investigate this problem.

First I added a debugfs file that displays nearly all of the mixer
registers and decodes their bits for convenience. I've attached a patch
for this change. I believe this patch is just for debugging. I don't
expect that this patch needs to go into the main source.

The debugfs file bypasses the register cache in the codec. It also reads
the value in the cache and compares them. If the register value is
different than that of the cache, text is added to the debugfs file to
alert the viewer of this.

When the error occurs, the cache no longer reflects the value of the
hardware register.

As mentioned before, I've also hooked the out_be32() routine, and had it
print a message whenever register 0x4 of the codec (The headphone mixer
register) is modified by software.

The register is written indirectly, so to be clear, the hook in
out_be32() checked for writes to the ac97_cmd register and the register
number field of the value written had 0x4 in it. (e.g. (value >> 24) &
0x7f) == 0x4)

The hook didn't check for the "write" bit so it also ended up displaying
a message whenever register 0x4 was read as well as written.

In my tests register 0x4 is being modified outside the software path
that would deliberately modify this register using out_be32().

I didn't include a patch for this hook, as it was very hacky, and I
wasn't so sure about its usefulness.

This issue has been seen on MPC5200 based boards.

I've chatted with Grant a bit. We'll see what we can scrounge up to hook
up some test equipment. I'm not sure when we'll get to this.

I'm open to more suggestions or thoughts on the approach I've taken so
far.

- John

[-- Attachment #2: asoc-wm9712-debugfs-for-mixer--0.patch --]
[-- Type: text/x-patch, Size: 10063 bytes --]

ASoC: WM9712: debugfs for mixer registers

From: John Bonesio <bones@secretlab.ca>

debugfs file added to display mixer registers.
---

 sound/soc/codecs/wm9712.c |  309 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 309 insertions(+), 0 deletions(-)


diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c
index b57c817..8d13ece 100644
--- a/sound/soc/codecs/wm9712.c
+++ b/sound/soc/codecs/wm9712.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/device.h>
+#include <linux/debugfs.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/ac97_codec.h>
@@ -154,6 +155,50 @@ SOC_SINGLE("Mic 2 Volume", AC97_MIC, 0, 31, 1),
 SOC_SINGLE("Mic 20dB Boost Switch", AC97_MIC, 7, 1, 0),
 };
 
+typedef struct reg_map_type {
+	unsigned int reg;
+	char *name;
+} reg_map_type;
+
+reg_map_type reg_map[] = {
+	{ 0x02, "spkr out (1/2)" },
+	{ 0x04, "hp out (l/r)" },
+	{ 0x06, "phone out (mono)" },
+	{ 0x0a, "pcbeep" },
+	{ 0x0c, "phone" },
+	{ 0x0e, "mic to phone" },
+	{ 0x10, "linein" },
+	{ 0x12, "AUXDAC" },
+	{ 0x14, "mic to hp (sidetone)" },
+	{ 0x16, "OUT3" },
+	{ 0x18, "DAC" },
+	{ 0x20, "ADC" },
+	{ 0x24, "PowMgmnt #1" },
+	{ 0x26, "PowMgmnt #2" },
+	{ HPL_MIXER, "HPL_MIXER" },
+	{ HPR_MIXER, "HPR_MIXER" },
+	{ 0x00, NULL }
+};
+
+char *reg_name(unsigned int reg) {
+	unsigned int idx;
+
+	for (idx=0; reg_map[idx].name != NULL; idx++) {
+		if (reg_map[idx].reg == reg)
+			break;
+	}
+
+	if (reg_map[idx].name == NULL) {
+		return "(unknown register)";
+	}
+	return reg_map[idx].name;
+}
+
+#ifdef CONFIG_DEBUG_FS
+static struct dentry *debugfs_root;
+#endif
+
+
 /* We have to create a fake left and right HP mixers because
  * the codec only has a single control that is shared by both channels.
  * This makes it impossible to determine the audio path.
@@ -429,6 +474,254 @@ static const struct snd_soc_dapm_route audio_map[] = {
 	{"ROUT2", NULL, "Speaker PGA"},
 };
 
+#ifdef CONFIG_DEBUG_FS
+static int wm9712_reg_open_file(struct inode *inode, struct file *file)
+{
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static char tmp_str[120];
+static char *decode_bits(unsigned int reg, unsigned int value) {
+	switch (reg) {
+		case 0x2:
+			/* lout2/rout2 */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"%smute; lout2 vol: %d; zc: %s inv: %s; rout2 vol: %d",
+				(value & 0x8000) ? "" : "un",
+				(value >> 8) & 0x3F,
+				(value & 0x80) ? "on" : "off", 
+				(value & 0x40) ? "on" : "off", 
+				value & 0x3F);
+			break;
+		case 0x4:
+			/* hpout l/r */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"%smute; hpout2 vol: %d; zc: %s hpout2 vol: %d",
+				(value & 0x8000) ? "" : "un",
+				(value >> 8) & 0x3F,
+				(value & 0x80) ? "on" : "off", 
+				value & 0x3F);
+			break;
+		case 0x6:
+			/* monoout */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"%smute; zc: %s monoout vol: %d",
+				(value & 0x8000) ? "" : "un",
+				(value & 0x80) ? "on" : "off", 
+				value & 0x3F);
+			break;
+		case 0xa:
+			/* pcbeep */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"b-hph %smute; b-hph vol: %d; b-spk %smute; b-spk vol: %d; b-ph %smute; b-ph vol: %d;",
+				(value & 0x8000) ? "" : "un",
+				(value >> 12) & 0x7,
+				(value & 0x800) ? "" : "un", 
+				(value >> 8) & 0x7,
+				(value & 0x80) ? "" : "un", 
+				(value >> 4) & 0x7);
+			break;
+		case 0xc:
+			/* phone */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"p-hph %smute; p-spk %smute; phone vol: %d",
+				(value & 0x8000) ? "" : "un",
+				(value & 0x4000) ? "" : "un", 
+				value & 0x1F);
+			break;
+		case 0xe:
+			/* mic1/mic2 */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"m1 %smute; m2 %smute; lmic(1) vol: %d; 20db: %s; ms: %x; mic(2) vol: %d",
+				(value & 0x4000) ? "" : "un", 
+				(value & 0x2000) ? "" : "un",
+				(value >> 8) & 0x1f,
+				(value & 0x80) ? "on" : "off",
+				((value >> 5) & 0x3),
+				value & 0x1F);
+			break;
+		case 0x10:
+			/* linein */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"l-hph %smute; l-spk %smute; l-ph %smute; lineinl vol: %d; linein2 vol: %d",
+				(value & 0x8000) ? "" : "un",
+				(value & 0x4000) ? "" : "un", 
+				(value & 0x2000) ? "" : "un", 
+				(value >> 8) & 0x1f,
+				value & 0x1F);
+			break;
+		case 0x12:
+			/* auxadc */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"a-hph %smute; a-hph vol: %d; a-spk %smute; a-spk vol: %d; a-ph %smute; a-ph vol: %d; axe: %s",
+				(value & 0x8000) ? "" : "un",
+				(value >> 12) & 0x7,
+				(value & 0x800) ? "" : "un", 
+				(value >> 8) & 0x7,
+				(value & 0x80) ? "" : "un", 
+				(value >> 4) & 0x7,
+				(value & 0x1) ? "on" : "off");
+			break;
+		case 0x14:
+			/* side tone */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"st %smute; st vol: %d; alc-hph mute: %d; alc vol: %d",
+				(value & 0x8000) ? "" : "un",
+				(value >> 12) & 0x7,
+				(value >> 10) & 0x3,
+				(value >> 7) & 0x7);
+			break;
+		case 0x16:
+			/* out3 */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"%smute; out3 src: %d; lout2/rout2 src: %s; zc: %s; alc vol: %d",
+				(value & 0x8000) ? "" : "un",
+				(value >> 9) & 0x3,
+				(value & 0x100) ? "spk" : "hph",
+				(value & 0x80) ? "on" : "off",
+				value & 0x3f);
+			break;
+		case 0x18:
+			/* dac */
+			snprintf(tmp_str, sizeof(tmp_str),
+				"d-hph %smute; d-spk %smute; d-ph %smute; ldac vol: %d; rdac vol: %d",
+				(value & 0x8000) ? "" : "un",
+				(value & 0x4000) ? "" : "un", 
+				(value & 0x2000) ? "" : "un", 
+				(value >> 8) & 0x1f,
+				value & 0x1F);
+			break;
+		default:
+			snprintf(tmp_str, sizeof(tmp_str), "(not decoded)");
+	};
+	return tmp_str;
+}
+
+extern unsigned *mixer_addr;
+static ssize_t wm9712_mixer_reg_show(struct snd_soc_codec *codec, char *buf)
+{
+	int idx, count = 0;
+	unsigned int value, cached_value;
+
+	if (!codec->reg_cache_size)
+		return 0;
+
+	count += sprintf(buf, "%s Mixer registers:\n", codec->name);
+	for (idx = 0; reg_map[idx].name != NULL; idx++) {
+		cached_value = ac97_read(codec, reg_map[idx].reg);
+		if ((reg_map[idx].reg != HPL_MIXER) && (reg_map[idx].reg != HPR_MIXER)) {
+			value = soc_ac97_ops.read(codec->ac97, reg_map[idx].reg);
+		} else {
+			value = cached_value;
+		}
+
+		count += snprintf(buf + count, PAGE_SIZE - count,
+			"%12s: 0x%0x: %s", reg_map[idx].name,
+			value, decode_bits(reg_map[idx].reg, value));
+		if (cached_value != value) {
+			count += snprintf(buf + count, PAGE_SIZE - count,
+				"(cached WRONG!!: 0x%0x)\n", cached_value);
+		} else {
+			count += snprintf(buf + count, PAGE_SIZE - count, "\n");
+		}
+
+		if (count >= PAGE_SIZE - 1)
+			break;
+	}
+	count += snprintf(buf + count, PAGE_SIZE - count,
+		"global mixer_addr: %p\n", mixer_addr);
+
+	/* Truncate count; min() would cause a warning */
+	if (count >= PAGE_SIZE)
+		count = PAGE_SIZE - 1;
+
+	return count;
+}
+
+
+static ssize_t wm9712_reg_read_file(struct file *file, char __user *user_buf,
+			       size_t count, loff_t *ppos)
+{
+	ssize_t ret;
+	struct snd_soc_codec *codec = file->private_data;
+	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = wm9712_mixer_reg_show(codec, buf);
+	if (ret >= 0)
+		ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+	kfree(buf);
+	return ret;
+}
+
+static ssize_t wm9712_reg_write_file(struct file *file,
+		const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	char buf[32];
+	int buf_size;
+	char *start = buf;
+	unsigned long reg, value;
+	int step = 1;
+	struct snd_soc_codec *codec = file->private_data;
+
+return -EINVAL;
+
+	buf_size = min(count, (sizeof(buf)-1));
+	if (copy_from_user(buf, user_buf, buf_size))
+		return -EFAULT;
+	buf[buf_size] = 0;
+
+	if (codec->reg_cache_step)
+		step = codec->reg_cache_step;
+
+	while (*start == ' ')
+		start++;
+	reg = simple_strtoul(start, &start, 16);
+	if ((reg >= codec->reg_cache_size) || (reg % step))
+		return -EINVAL;
+	while (*start == ' ')
+		start++;
+	if (strict_strtoul(start, 16, &value))
+		return -EINVAL;
+	codec->write(codec, reg, value);
+	return buf_size;
+}
+
+static const struct file_operations codec_reg_fops = {
+	.open = wm9712_reg_open_file,
+	.read = wm9712_reg_read_file,
+	.write = wm9712_reg_write_file,
+};
+
+static void wm9712_init_debugfs(struct snd_soc_codec *codec)
+{
+	codec->debugfs_reg = debugfs_create_file("mixer_reg", 0644,
+						 debugfs_root, codec,
+						 &codec_reg_fops);
+	if (!codec->debugfs_reg)
+		printk(KERN_WARNING
+		       "ASoC: Failed to create codec register debugfs file\n");
+}
+
+static void wm9712_cleanup_debugfs(struct snd_soc_codec *codec)
+{
+	debugfs_remove(codec->debugfs_pop_time);
+	debugfs_remove(codec->debugfs_reg);
+}
+
+#else
+
+static inline void wm9712_init_debugfs(struct snd_soc_codec *codec)
+{
+}
+
+static inline void wm9712_cleanup_debugfs(struct snd_soc_codec *codec)
+{
+}
+#endif
+
 static int wm9712_add_widgets(struct snd_soc_codec *codec)
 {
 	snd_soc_dapm_new_controls(codec, wm9712_dapm_widgets,
@@ -699,6 +992,8 @@ static int wm9712_soc_probe(struct platform_device *pdev)
 		goto reset_err;
 	}
 
+	wm9712_init_debugfs(codec);
+
 	return 0;
 
 reset_err:
@@ -724,6 +1019,8 @@ static int wm9712_soc_remove(struct platform_device *pdev)
 	if (codec == NULL)
 		return 0;
 
+	wm9712_cleanup_debugfs(codec);
+
 	snd_soc_dapm_free(socdev);
 	snd_soc_free_pcms(socdev);
 	snd_soc_free_ac97_codec(codec);
@@ -742,12 +1039,24 @@ EXPORT_SYMBOL_GPL(soc_codec_dev_wm9712);
 
 static int __init wm9712_modinit(void)
 {
+#ifdef CONFIG_DEBUG_FS
+        debugfs_root = debugfs_create_dir("wm9712", NULL);
+        if (IS_ERR(debugfs_root) || !debugfs_root) {
+                printk(KERN_WARNING
+                       "WM9712: Failed to create debugfs directory\n");
+                debugfs_root = NULL;
+        }
+#endif
 	return snd_soc_register_dais(wm9712_dai, ARRAY_SIZE(wm9712_dai));
 }
 module_init(wm9712_modinit);
 
 static void __exit wm9712_exit(void)
 {
+#ifdef CONFIG_DEBUG_FS
+        debugfs_remove_recursive(debugfs_root);
+#endif
+
 	snd_soc_unregister_dais(wm9712_dai, ARRAY_SIZE(wm9712_dai));
 }
 module_exit(wm9712_exit);

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: MPC5200/WM9712 mute problem

[Mark Brown]

On Thu, Aug 06, 2009 at 10:28:41AM -0700, John Bonesio wrote:

> I'm open to more suggestions or thoughts on the approach I've taken so
> far.

The approach you've been taking sounds reasonable; at this point I'd be
looking at trying to confirm that the controller is doing what's been
asked of it.  Checking that the CODEC power rails are all within spec
and looking at other bits in the same register may also be useful.

Is the system otherwise idle when the problem occurs?

Re: MPC5200/WM9712 mute problem

[John Bonesio]

On Fri, 2009-08-07 at 10:56 +0100, Mark Brown wrote:
> On Thu, Aug 06, 2009 at 10:28:41AM -0700, John Bonesio wrote:
> 
> > I'm open to more suggestions or thoughts on the approach I've taken so
> > far.
> 
> The approach you've been taking sounds reasonable; at this point I'd be
> looking at trying to confirm that the controller is doing what's been
> asked of it.  Checking that the CODEC power rails are all within spec
> and looking at other bits in the same register may also be useful.
> 
> Is the system otherwise idle when the problem occurs?

Yes the system is idle - just me fiddling with the mute settings.

Re: MPC5200/WM9712 mute problem

[Jon Smirl]

On Fri, Aug 7, 2009 at 10:48 AM, John Bonesio<bones@secretlab.ca> wrote:
> On Fri, 2009-08-07 at 10:56 +0100, Mark Brown wrote:
>> On Thu, Aug 06, 2009 at 10:28:41AM -0700, John Bonesio wrote:
>>
>> > I'm open to more suggestions or thoughts on the approach I've taken so
>> > far.
>>
>> The approach you've been taking sounds reasonable; at this point I'd be
>> looking at trying to confirm that the controller is doing what's been
>> asked of it.  Checking that the CODEC power rails are all within spec
>> and looking at other bits in the same register may also be useful.

You can turn on an interrupt that gets triggered when the register
wire completes. That would give you a clue if too many register writes
are happening.

A DSO or logical analyzer would make this much faster to isolate.

>>
>> Is the system otherwise idle when the problem occurs?
>
> Yes the system is idle - just me fiddling with the mute settings.
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] ASoC: WM9712 Codec: Workaround an unmute problem (Was: Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around)

[Jon Smirl]

On Wed, Aug 5, 2009 at 12:30 PM, John Bonesio<bones@secretlab.ca> wrote:
> Hi Mark,
>
> I've retitled the email to better reflect the real patch. I believe
> there has been some general confusion because I originally sent the
> wrong patch.
>
> You wrote:
>
>> Like I said before, exactly which control are you adjusting here?
>>
> My description of this got lost in all the confusion. Let me try again.
> We are adjusting the mixer bits for mute/unmute on two of the mixer
> settings. The first one is general headphone mute setting on register
> 0x4 (bit 15). The second one is the PCM mute setting on register 0x18
> (bit 15).
>
> What we are seeing is that if we first unmute the general headphone (reg
> 0x4 bit15), then unmute the PCM (reg 0x18 bit 15) [HPL PCM in the
> alsamixer application], the general headphone gets muted again, even
> though software didn't write to that register.
>
>>
>> > If there is code in software doing this, it's very subtle.
>>
>> My money would be on the AC97 controller having problems; the quality of
>> SoC AC97 controllers is variable.  It certainly doesn't sound like a
>> WM9712 issue; as I say I'd be very surprised if such an issue hadn't
>> come up before given how widely deployed the part is.
>
> We don't have access to an AC97 analyzer. Do you have any suggestions on
> other ways we can pinpoint the error?

Do you have a normal logic analyzer? You can decode the AC97 by hand,
its only a couple of frames.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] ASoC: WM9712 Codec: Workaround an unmute problem (Was: Re: [PATCH] ASoC: MPC5200: Support for buffer wrap around)

[Jon Smirl]

On Wed, Aug 5, 2009 at 1:16 PM, Jon Smirl<jonsmirl@gmail.com> wrote:
> On Wed, Aug 5, 2009 at 12:30 PM, John Bonesio<bones@secretlab.ca> wrote:
>> Hi Mark,
>>
>> I've retitled the email to better reflect the real patch. I believe
>> there has been some general confusion because I originally sent the
>> wrong patch.
>>
>> You wrote:
>>
>>> Like I said before, exactly which control are you adjusting here?
>>>
>> My description of this got lost in all the confusion. Let me try again.
>> We are adjusting the mixer bits for mute/unmute on two of the mixer
>> settings. The first one is general headphone mute setting on register
>> 0x4 (bit 15). The second one is the PCM mute setting on register 0x18
>> (bit 15).
>>
>> What we are seeing is that if we first unmute the general headphone (reg
>> 0x4 bit15), then unmute the PCM (reg 0x18 bit 15) [HPL PCM in the
>> alsamixer application], the general headphone gets muted again, even
>> though software didn't write to that register.
>>
>>>
>>> > If there is code in software doing this, it's very subtle.
>>>
>>> My money would be on the AC97 controller having problems; the quality of
>>> SoC AC97 controllers is variable.  It certainly doesn't sound like a
>>> WM9712 issue; as I say I'd be very surprised if such an issue hadn't
>>> come up before given how widely deployed the part is.
>>
>> We don't have access to an AC97 analyzer. Do you have any suggestions on
>> other ways we can pinpoint the error?
>
> Do you have a normal logic analyzer? You can decode the AC97 by hand,
> its only a couple of frames.

My hardware engineer has this one, but it is in Seattle and I am in
Florida. The OpenMoko guys use it.
http://www.pctestinstruments.com/

You can write your own interpreters, someone may have done AC97 for it
but I haven't checked.

He also has a Rigol DS1102E which he says is an awesome scope for
$636. Another tip from OpenMoku.
http://www.tequipment.net/RigolDS1102E.html


-- 
Jon Smirl
jonsmirl@gmail.com