[PATCH] ASoC: sgtl5000: Fix VAG_POWER enabling/disabling order

[PATCH] ASoC: sgtl5000: Fix VAG_POWER enabling/disabling order

[Marek Vasut]

The VAG_POWER must be enabled after all other bits in CHIP_ANA_POWER
and disabled before any other bit in CHIP_ANA_POWER. See the SGTL5000
datasheet (Table 31, BIT 7, page 42-43). Failing to follow this order
will result in ugly loud "POP" noise at the end of playback.

To achieve such order, use the _PRE and _POST DAPM widgets to trigger
the power_vag_event, where the event type check has to be fixed
accordingly as well.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Dong Aisheng <dong.aisheng@linaro.org>
Cc: Eric Nelson <eric.nelson@boundarydevices.com>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/sgtl5000.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 92bbfec..2068c36 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -151,15 +151,15 @@ static int power_vag_event(struct snd_soc_dapm_widget *w,
 	struct snd_kcontrol *kcontrol, int event)
 {
 	switch (event) {
-	case SND_SOC_DAPM_PRE_PMU:
+	case SND_SOC_DAPM_POST_PMU:
 		snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
 			SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
 		break;
 
-	case SND_SOC_DAPM_POST_PMD:
+	case SND_SOC_DAPM_PRE_PMD:
 		snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
 			SGTL5000_VAG_POWERUP, 0);
-		msleep(400);
+		mdelay(400);
 		break;
 	default:
 		break;
@@ -217,12 +217,11 @@ static const struct snd_soc_dapm_widget sgtl5000_dapm_widgets[] = {
 				0, SGTL5000_CHIP_DIG_POWER,
 				1, 0),
 
-	SND_SOC_DAPM_SUPPLY("VAG_POWER", SGTL5000_CHIP_ANA_POWER, 7, 0,
-			    power_vag_event,
-			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
-
 	SND_SOC_DAPM_ADC("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0),
 	SND_SOC_DAPM_DAC("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0),
+
+	SND_SOC_DAPM_PRE("VAG_POWER_PRE", power_vag_event),
+	SND_SOC_DAPM_POST("VAG_POWER_POST", power_vag_event),
 };
 
 /* routes for sgtl5000 */
@@ -230,16 +229,13 @@ static const struct snd_soc_dapm_route sgtl5000_dapm_routes[] = {
 	{"Capture Mux", "LINE_IN", "LINE_IN"},	/* line_in --> adc_mux */
 	{"Capture Mux", "MIC_IN", "MIC_IN"},	/* mic_in --> adc_mux */
 
-	{"ADC", NULL, "VAG_POWER"},
 	{"ADC", NULL, "Capture Mux"},		/* adc_mux --> adc */
 	{"AIFOUT", NULL, "ADC"},		/* adc --> i2s_out */
 
-	{"DAC", NULL, "VAG_POWER"},
 	{"DAC", NULL, "AIFIN"},			/* i2s-->dac,skip audio mux */
 	{"Headphone Mux", "DAC", "DAC"},	/* dac --> hp_mux */
 	{"LO", NULL, "DAC"},			/* dac --> line_out */
 
-	{"LINE_IN", NULL, "VAG_POWER"},
 	{"Headphone Mux", "LINE_IN", "LINE_IN"},/* line_in --> hp_mux */
 	{"HP", NULL, "Headphone Mux"},		/* hp_mux --> hp */
 
-- 
1.7.10.4

Re: [PATCH] ASoC: sgtl5000: Fix VAG_POWER enabling/disabling order

[Fabio Estevam]

Hi Marek,

On Fri, May 24, 2013 at 12:34 AM, Marek Vasut <marex@denx.de> wrote:
> The VAG_POWER must be enabled after all other bits in CHIP_ANA_POWER
> and disabled before any other bit in CHIP_ANA_POWER. See the SGTL5000
> datasheet (Table 31, BIT 7, page 42-43). Failing to follow this order
> will result in ugly loud "POP" noise at the end of playback.
>
> To achieve such order, use the _PRE and _POST DAPM widgets to trigger
> the power_vag_event, where the event type check has to be fixed
> accordingly as well.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Dong Aisheng <dong.aisheng@linaro.org>
> Cc: Eric Nelson <eric.nelson@boundarydevices.com>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>

Thanks, tested on a mx51evk and it removes an annoying 'click' after
an audio track is played.

> -       case SND_SOC_DAPM_POST_PMD:
> +       case SND_SOC_DAPM_PRE_PMD:
>                 snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
>                         SGTL5000_VAG_POWERUP, 0);
> -               msleep(400);
> +               mdelay(400);

What is the reasoning for this change?

Anyway:

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

Re: [PATCH] ASoC: sgtl5000: Fix VAG_POWER enabling/disabling order

[Mark Brown]

On Fri, May 24, 2013 at 05:34:56AM +0200, Marek Vasut wrote:

> -	case SND_SOC_DAPM_POST_PMD:
> +	case SND_SOC_DAPM_PRE_PMD:
>  		snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
>  			SGTL5000_VAG_POWERUP, 0);
> -		msleep(400);
> +		mdelay(400);
>  		break;

This looks to be both unrelated and a regression - mdelay() should be
more resource intensive than sleeping and if it's 400ms we shouldn't be
that worried about delaying slightly longer.

Please note my updated mail address which I've been using for a while
now, the Wolfson one will go bad at the end of the week.

Re: [PATCH] ASoC: sgtl5000: Fix VAG_POWER enabling/disabling order

[Marek Vasut]

Dear Fabio Estevam,

[Fixing Mark's address in the CC.]

> Hi Marek,
> 
> On Fri, May 24, 2013 at 12:34 AM, Marek Vasut <marex@denx.de> wrote:
> > The VAG_POWER must be enabled after all other bits in CHIP_ANA_POWER
> > and disabled before any other bit in CHIP_ANA_POWER. See the SGTL5000
> > datasheet (Table 31, BIT 7, page 42-43). Failing to follow this order
> > will result in ugly loud "POP" noise at the end of playback.
> > 
> > To achieve such order, use the _PRE and _POST DAPM widgets to trigger
> > the power_vag_event, where the event type check has to be fixed
> > accordingly as well.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Dong Aisheng <dong.aisheng@linaro.org>
> > Cc: Eric Nelson <eric.nelson@boundarydevices.com>
> > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> Thanks, tested on a mx51evk and it removes an annoying 'click' after
> an audio track is played.

That's good. But you still hear some noise when the headphone/lineout drivers 
are powered up/down, right?

> > -       case SND_SOC_DAPM_POST_PMD:
> > 
> > +       case SND_SOC_DAPM_PRE_PMD:
> >                 snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
> >                 
> >                         SGTL5000_VAG_POWERUP, 0);
> > 
> > -               msleep(400);
> > +               mdelay(400);
> 
> What is the reasoning for this change?

Me being a moron, that's the full reasoning. Sorry.

Best regards,
Marek Vasut

Re: [PATCH] ASoC: sgtl5000: Fix VAG_POWER enabling/disabling order

[Marek Vasut]

Dear Mark Brown,

> On Fri, May 24, 2013 at 05:34:56AM +0200, Marek Vasut wrote:
> > -	case SND_SOC_DAPM_POST_PMD:
> > 
> > +	case SND_SOC_DAPM_PRE_PMD:
> >  		snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
> >  		
> >  			SGTL5000_VAG_POWERUP, 0);
> > 
> > -		msleep(400);
> > +		mdelay(400);
> > 
> >  		break;
> 
> This looks to be both unrelated and a regression - mdelay() should be
> more resource intensive than sleeping and if it's 400ms we shouldn't be
> that worried about delaying slightly longer.

Definitelly, this was not supposed to be part of the patch. Fixed in V2.

> Please note my updated mail address which I've been using for a while
> now, the Wolfson one will go bad at the end of the week.

Roger. I hope you have a good time at your new position :)

Best regards,
Marek Vasut

Re: [PATCH] ASoC: sgtl5000: Fix VAG_POWER enabling/disabling order

[Fabio Estevam]

On Tue, May 28, 2013 at 3:23 PM, Marek Vasut <marex@denx.de> wrote:

> That's good. But you still hear some noise when the headphone/lineout drivers
> are powered up/down, right?

Only tested on mx51evk and I don't hear no more noises after your
patch is applied.

Re: [PATCH] ASoC: sgtl5000: Fix VAG_POWER enabling/disabling order

[Marek Vasut]

Dear Fabio Estevam,

> On Tue, May 28, 2013 at 3:23 PM, Marek Vasut <marex@denx.de> wrote:
> > That's good. But you still hear some noise when the headphone/lineout
> > drivers are powered up/down, right?
> 
> Only tested on mx51evk and I don't hear no more noises after your
> patch is applied.

Crank the volume up, make it _REALLY_ loud and just before you start hearing the 
audio you're supposed to hear, the HP driver is enabled. This makes such a minor 
noise it seems. It's hard to hear when it's not loud enough ;-)

Best regards,
Marek Vasut