[PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Shawn Guo]

From: Zeng Zhaoming <zengzm.kernel@gmail.com>

As manual described, VAG is an internal voltage reference of DAC/ADC,
So enabled it before DAC/ADC up.  As the result, the issue that
captured wave file has no sound gets fixed.

One more thing should care about is VAG fully ramped down requires 400ms,
wait it to avoid pop.

Signed-off-by: Zeng Zhaoming <zengzm.kernel@gmail.com>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 sound/soc/codecs/sgtl5000.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index d192626..d6f5aaf 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -143,11 +143,11 @@ static int mic_bias_event(struct snd_soc_dapm_widget *w,
 }
 
 /*
- * using codec assist to small pop, hp_powerup or lineout_powerup
- * should stay setting until vag_powerup is fully ramped down,
- * vag fully ramped down require 400ms.
+ * As manual described, ADC/DAC only works when VAG powerup,
+ * So enabled VAG before ADC/DAC up.
+ * In power down case, we need wait 400ms when vag fully ramped down.
  */
-static int small_pop_event(struct snd_soc_dapm_widget *w,
+static int power_vag_event(struct snd_soc_dapm_widget *w,
 	struct snd_kcontrol *kcontrol, int event)
 {
 	switch (event) {
@@ -156,7 +156,7 @@ static int small_pop_event(struct snd_soc_dapm_widget *w,
 			SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
 		break;
 
-	case SND_SOC_DAPM_PRE_PMD:
+	case SND_SOC_DAPM_POST_PMD:
 		snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
 			SGTL5000_VAG_POWERUP, 0);
 		msleep(400);
@@ -201,12 +201,8 @@ static const struct snd_soc_dapm_widget sgtl5000_dapm_widgets[] = {
 				mic_bias_event,
 				SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
 
-	SND_SOC_DAPM_PGA_E("HP", SGTL5000_CHIP_ANA_POWER, 4, 0, NULL, 0,
-			small_pop_event,
-			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD),
-	SND_SOC_DAPM_PGA_E("LO", SGTL5000_CHIP_ANA_POWER, 0, 0, NULL, 0,
-			small_pop_event,
-			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD),
+	SND_SOC_DAPM_PGA("HP", SGTL5000_CHIP_ANA_POWER, 4, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("LO", SGTL5000_CHIP_ANA_POWER, 0, 0, NULL, 0),
 
 	SND_SOC_DAPM_MUX("Capture Mux", SND_SOC_NOPM, 0, 0, &adc_mux),
 	SND_SOC_DAPM_MUX("Headphone Mux", SND_SOC_NOPM, 0, 0, &dac_mux),
@@ -221,15 +217,20 @@ static const struct snd_soc_dapm_widget sgtl5000_dapm_widgets[] = {
 				0, SGTL5000_CHIP_DIG_POWER,
 				1, 0),
 
-	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_ADC_E("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0,
+			power_vag_event,
+			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_DAC_E("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0,
+			power_vag_event,
+			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
 };
 
 /* routes for sgtl5000 */
 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 */
+	{"Mic Bias", NULL, "MIC_IN"},		/* mic_in --> mic bias */
+	{"Capture Mux", "MIC_IN", "Mic Bias"},	/* mic bias --> adc_mux */
 
 	{"ADC", NULL, "Capture Mux"},		/* adc_mux --> adc */
 	{"AIFOUT", NULL, "ADC"},		/* adc --> i2s_out */
-- 
1.7.5.4

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 611 bytes --]

On Thu, Mar 29, 2012 at 03:17:00PM +0800, Zeng Zhaoming wrote:

> So the code should be:
> +      {"MIC_IN", NULL, "Mic Bias},	       /* mic bias --> mic_in */
>        {"Capture Mux", "MIC_IN", "MIC_IN"},    /* mic_in --> adc_mux */
> -+     {"Mic Bias", NULL, "MIC_IN"},           /* mic_in --> mic bias */
> -+     {"Capture Mux", "MIC_IN", "Mic Bias"},  /* mic bias --> adc_mux */

> and platform code
> +      {"Mic Bias", NULL, "MIC"},
> -      {"MIC_IN", NULL, "MIC"},

No, the bias should be a supply for the microphone - ensuring that this
happens is the whole point of the patch I sent the other day.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Shawn Guo]

On Thu, Mar 29, 2012 at 11:58:32AM +0100, Mark Brown wrote:
> On Thu, Mar 29, 2012 at 03:17:00PM +0800, Zeng Zhaoming wrote:
> 
> > So the code should be:
> > +      {"MIC_IN", NULL, "Mic Bias},	       /* mic bias --> mic_in */
> >        {"Capture Mux", "MIC_IN", "MIC_IN"},    /* mic_in --> adc_mux */
> > -+     {"Mic Bias", NULL, "MIC_IN"},           /* mic_in --> mic bias */
> > -+     {"Capture Mux", "MIC_IN", "Mic Bias"},  /* mic bias --> adc_mux */
> 
> > and platform code
> > +      {"Mic Bias", NULL, "MIC"},
> > -      {"MIC_IN", NULL, "MIC"},
> 
> No, the bias should be a supply for the microphone - ensuring that this
> happens is the whole point of the patch I sent the other day.

The "MIC" above is confusing.  I guess the following that I have in
machine driver might be a little clearer.

        {"Mic Bias", NULL, "Mic Jack"},
        {"MIC_IN", NULL, "Mic Bias"},

And I need to present it in device tree though.

-- 
Regards,
Shawn

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 907 bytes --]

On Thu, Mar 29, 2012 at 10:04:23PM +0800, Shawn Guo wrote:
> On Thu, Mar 29, 2012 at 11:58:32AM +0100, Mark Brown wrote:

> > No, the bias should be a supply for the microphone - ensuring that this
> > happens is the whole point of the patch I sent the other day.

> The "MIC" above is confusing.  I guess the following that I have in
> machine driver might be a little clearer.

>         {"Mic Bias", NULL, "Mic Jack"},
>         {"MIC_IN", NULL, "Mic Bias"},

This should be something like:

	{ "Mic Jack", NULL, "Mic Bias" },
	{ "MIC_IN", NULL, "Mic Bias" },

(with the conversion of Mic Bias to a supply widget) which has the bonus
of being a whole lot easier to understand.

> And I need to present it in device tree though.

Use the generic stuff that Stephen put into the core for reading machine
driver routing maps out of the device tree (unless there's some problem
with it, but it should work).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Shawn Guo]

On Thu, Mar 29, 2012 at 03:10:20PM +0100, Mark Brown wrote:
...
> This should be something like:
> 
> 	{ "Mic Jack", NULL, "Mic Bias" },
> 	{ "MIC_IN", NULL, "Mic Bias" },
> 
> (with the conversion of Mic Bias to a supply widget) which has the bonus
> of being a whole lot easier to understand.
> 
Indeed.  With your patch, Mic Bias is a supply widget now.

-- 
Regards,
Shawn

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Richard Zhao]

Hi Mark,

On Thu, Mar 29, 2012 at 10:10 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Mar 29, 2012 at 10:04:23PM +0800, Shawn Guo wrote:
>> On Thu, Mar 29, 2012 at 11:58:32AM +0100, Mark Brown wrote:
>
>> > No, the bias should be a supply for the microphone - ensuring that this
>> > happens is the whole point of the patch I sent the other day.
>
>> The "MIC" above is confusing.  I guess the following that I have in
>> machine driver might be a little clearer.
>
>>         {"Mic Bias", NULL, "Mic Jack"},
>>         {"MIC_IN", NULL, "Mic Bias"},
>
> This should be something like:
>
>        { "Mic Jack", NULL, "Mic Bias" },
>        { "MIC_IN", NULL, "Mic Bias" },
"MiC Bias" --> "Mic Jack" should be enough.
Why do we have "Mic Bias" --> "MIC_IN" ?

Thanks
Richard
>
> (with the conversion of Mic Bias to a supply widget) which has the bonus
> of being a whole lot easier to understand.
>
>> And I need to present it in device tree though.
>
> Use the generic stuff that Stephen put into the core for reading machine
> driver routing maps out of the device tree (unless there's some problem
> with it, but it should work).
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 348 bytes --]

On Wed, Apr 25, 2012 at 10:08:05AM +0800, Richard Zhao wrote:

> >        { "Mic Jack", NULL, "Mic Bias" },
> >        { "MIC_IN", NULL, "Mic Bias" },

> "MiC Bias" --> "Mic Jack" should be enough.
> Why do we have "Mic Bias" --> "MIC_IN" ?

Probably because I've misread what you're trying to do above - if it's
just a single variation.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 855 bytes --]

On Thu, Mar 29, 2012 at 01:51:45PM +0800, Shawn Guo wrote:

> +	SND_SOC_DAPM_ADC_E("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0,
> +			power_vag_event,
> +			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
> +	SND_SOC_DAPM_DAC_E("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0,
> +			power_vag_event,
> +			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),

What would be more idiomatic here would be to make VAG a supply widget
and then connect it to DAC and ADC rather than have the event on both -
It looks like with your current code simultaneous playback and capture
will break, you'll run through the power sequences twice and if you stop
one stream it'll power down the other.

> +	{"Mic Bias", NULL, "MIC_IN"},		/* mic_in --> mic bias */
> +	{"Capture Mux", "MIC_IN", "Mic Bias"},	/* mic bias --> adc_mux */

This should be in the machine driver.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Dong Aisheng-B29396]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> bounces@alsa-project.org] On Behalf Of Mark Brown
> Sent: Thursday, March 29, 2012 7:16 PM
> To: Shawn Guo
> Cc: alsa-devel@alsa-project.org; Zeng Zhaoming
> Subject: Re: [alsa-devel] [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC
> up
> 
> On Thu, Mar 29, 2012 at 01:51:45PM +0800, Shawn Guo wrote:
> 
> > +	SND_SOC_DAPM_ADC_E("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0,
> > +			power_vag_event,
> > +			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
> > +	SND_SOC_DAPM_DAC_E("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0,
> > +			power_vag_event,
> > +			SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
> 
> What would be more idiomatic here would be to make VAG a supply widget
> and then connect it to DAC and ADC rather than have the event on both -
> It looks like with your current code simultaneous playback and capture
> will break, you'll run through the power sequences twice and if you stop
> one stream it'll power down the other.
> 
It's true.

Regards
Dong Aisheng

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 497 bytes --]

On Thu, Mar 29, 2012 at 08:09:33PM +0800, Zeng Zhaoming wrote:
> On Thu 2012-03-29 12:16:05, Mark Brown wrote:

> > > +	{"Mic Bias", NULL, "MIC_IN"},		/* mic_in --> mic bias */
> > > +	{"Capture Mux", "MIC_IN", "Mic Bias"},	/* mic bias --> adc_mux */

> > This should be in the machine driver.

> This "Mic Bias" is codec internal controller, Machine code should not
> touch this.

What is it actually doing?  Calling it "Mic Bias" if it's not a bias
source for microphones is horribly confusing.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: sgtl5000: Enable VAG when DAC/ADC up

[Shawn Guo]

Zhaoming,

On Thu, Mar 29, 2012 at 08:09:33PM +0800, Zeng Zhaoming wrote:
...
> This "Mic Bias" is codec internal controller, Machine code should not touch this.

In the end, machine driver will have to use it to specify the map.
You even said the below.

and platform code
+      {"Mic Bias", NULL, "MIC"},
-      {"MIC_IN", NULL, "MIC"},

-- 
Regards,
Shawn