Re: Audio glitch with SGTL5000

[Nikolay Dimitrov <picmaster@mail.bg>]

Hi Fabio,

On 01/21/2015 12:23 AM, Fabio Estevam wrote:
> On Tue, Jan 20, 2015 at 8:12 PM, Nikolay Dimitrov <picmaster@mail.bg> wrote:
>> Hi guys,
>>
>> I'm observing a specific issue with SGTL5000 - after I stop the audio
>> playback, after several seconds there's a single strong audio glitch
>> (thump, pulse). In my case the audio codec is directly connected to the
>> product speaker amps (without any attenuators) and the experience can
>> be hardly described as pleasant...
>
> Does this patch help?
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/sound/soc/codecs/sgtl5000.c?id=c251ea7bd7a04f1f2575467e0de76e803cf59149

Thanks for your fast answer! Just tried the patch, but unfortunately it
doesn't solve my issue - the pop is still observed on line-out.

I tried to reproduce the issue also with the integrated headphone amp,
just to find out that my board design has an issue (oops!) - the
headphones' ground is connected to the power supply ground instead of
the virtual ground, so I can't test the VAG ramp fix this way. So I can
only speculate that you tested your fix on a properly wired headphone
output, and that's where it worked OK.

Btw, I verified with a protocol analyzer whether the kernel actually
sends the proper I2C commands to the audio codec, and I can confirm
that all commands are sent and acknowledged as expected (including the
SMALL_POP bit setting).

My quick-fix for this was to remove almost all DAPM widgets and
configure the registers only once at start, and this avoid the
pop/click on line-out:


diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index ea47938..1223fb7 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -142,32 +142,6 @@ static int mic_bias_event(struct 
snd_soc_dapm_widget *w,
  	return 0;
  }

-/*
- * 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 power_vag_event(struct snd_soc_dapm_widget *w,
-	struct snd_kcontrol *kcontrol, int event)
-{
-	switch (event) {
-	case SND_SOC_DAPM_PRE_PMU:
-		snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
-			SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
-		break;
-
-	case SND_SOC_DAPM_POST_PMD:
-		snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
-			SGTL5000_VAG_POWERUP, 0);
-		msleep(400);
-		break;
-	default:
-		break;
-	}
-
-	return 0;
-}
-
  /* input sources for ADC */
  static const char *adc_mux_text[] = {
  	"MIC_IN", "LINE_IN"
@@ -201,50 +175,15 @@ 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("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),
-
-	/* aif for i2s input */
-	SND_SOC_DAPM_AIF_IN("AIFIN", "Playback",
-				0, SGTL5000_CHIP_DIG_POWER,
-				0, 0),
-
-	/* aif for i2s output */
-	SND_SOC_DAPM_AIF_OUT("AIFOUT", "Capture",
-				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),
  };

  /* 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 */
-
-	{"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 */
-
-	{"LINE_OUT", NULL, "LO"},
-	{"HP_OUT", NULL, "HP"},
  };

  /* custom function to fetch info of PCM playback volume */
@@ -1100,7 +1039,13 @@ static int sgtl5000_set_power_regs(struct 
snd_soc_codec *codec)
  	ana_pwr = snd_soc_read(codec, SGTL5000_CHIP_ANA_POWER);
  	ana_pwr |= SGTL5000_DAC_STEREO |
  			SGTL5000_ADC_STEREO |
-			SGTL5000_REFTOP_POWERUP;
+			SGTL5000_VAG_POWERUP |
+			SGTL5000_REFTOP_POWERUP |
+			SGTL5000_DAC_POWERUP |
+			SGTL5000_CAPLESS_HP_POWERUP |
+			SGTL5000_ADC_POWERUP |
+			SGTL5000_LINE_OUT_POWERUP;
+
  	lreg_ctrl = snd_soc_read(codec, SGTL5000_CHIP_LINREG_CTRL);

  	if (vddio < 3100 && vdda < 3100) {
@@ -1330,7 +1275,10 @@ static int sgtl5000_probe(struct snd_soc_codec 
*codec)
  	snd_soc_write(codec, SGTL5000_CHIP_SSS_CTRL,
  			SGTL5000_DAC_SEL_I2S_IN << SGTL5000_DAC_SEL_SHIFT);
  	snd_soc_write(codec, SGTL5000_CHIP_DIG_POWER,
-			SGTL5000_ADC_EN | SGTL5000_DAC_EN);
+			SGTL5000_ADC_EN |
+			SGTL5000_DAC_EN |
+			SGTL5000_I2S_OUT_POWERUP |
+			SGTL5000_I2S_IN_POWERUP);

  	/* enable dac volume ramp by default */
  	snd_soc_write(codec, SGTL5000_CHIP_ADCDAC_CTRL,


Well, I don't like this brute-force approach and that's why I'm looking 
for a more intelligent one.

Kind regards,
Nikolay