[PATCH] ASoC: tlv320aic3x: add AGC, MIC_BIAS

[PATCH] ASoC: tlv320aic3x: add AGC, MIC_BIAS

[Prchal Jiří]

This patch adds settings for TLV320AIC3106 of AGC, MIC_BIAS, De-emphasis.
Also added default settings of output pop-up reduction, short circuit protection and mix both channels to mono_out to 
get maximum level.

Signed-off-by: Jiri Prchal <jiri.prchal@aksignal.cz>

--- /home/prchal/arm/fw-cdu/linux/linux-3.5-rc3/sound/soc/codecs/tlv320aic3x.c
+++ /home/prchal/arm/fw-cdu/linux/linux-3.5-rc3/sound/soc/codecs/tlv320aic3x.c.new
@@ -194,6 +194,11 @@
  static const char *aic3x_linein_mode_mux[] = { "single-ended", "differential" };
  static const char *aic3x_adc_hpf[] =
      { "Disabled", "0.0045xFs", "0.0125xFs", "0.025xFs" };
+static const char *aic3x_agc_level[] =
+    { "-5.5dB", "-8dB", "-10dB", "-12dB", "-14dB", "-17dB", "-20dB", "-24dB" };
+static const char *aic3x_agc_attack[] = { "8ms", "11ms", "16ms", "20ms" };
+static const char *aic3x_agc_decay[] = { "100ms", "200ms", "400ms", "500ms" };
+static const char *aic3x_mic_bias[] = { "off", "2V", "2.5V", "AVDD" };

  #define LDAC_ENUM	0
  #define RDAC_ENUM	1
@@ -206,6 +211,13 @@
  #define LINE2L_ENUM	8
  #define LINE2R_ENUM	9
  #define ADC_HPF_ENUM	10
+#define LAGC_LEV_ENUM	11
+#define RAGC_LEV_ENUM	12
+#define LAGC_ATT_ENUM	13
+#define RAGC_ATT_ENUM	14
+#define LAGC_DEC_ENUM	15
+#define RAGC_DEC_ENUM	16
+#define MIC_BIAS_ENUM	17

  static const struct soc_enum aic3x_enum[] = {
  	SOC_ENUM_SINGLE(DAC_LINE_MUX, 6, 3, aic3x_left_dac_mux),
@@ -219,6 +231,13 @@
  	SOC_ENUM_SINGLE(LINE2L_2_LADC_CTRL, 7, 2, aic3x_linein_mode_mux),
  	SOC_ENUM_SINGLE(LINE2R_2_RADC_CTRL, 7, 2, aic3x_linein_mode_mux),
  	SOC_ENUM_DOUBLE(AIC3X_CODEC_DFILT_CTRL, 6, 4, 4, aic3x_adc_hpf),
+	SOC_ENUM_SINGLE(LAGC_CTRL_A, 4, 8, aic3x_agc_level),
+	SOC_ENUM_SINGLE(RAGC_CTRL_A, 4, 8, aic3x_agc_level),
+	SOC_ENUM_SINGLE(LAGC_CTRL_A, 2, 4, aic3x_agc_attack),
+	SOC_ENUM_SINGLE(RAGC_CTRL_A, 2, 4, aic3x_agc_attack),
+	SOC_ENUM_SINGLE(LAGC_CTRL_A, 0, 4, aic3x_agc_decay),
+	SOC_ENUM_SINGLE(RAGC_CTRL_A, 0, 4, aic3x_agc_decay),
+	SOC_ENUM_SINGLE(MICBIAS_CTRL, 6, 4, aic3x_mic_bias),
  };

  /*
@@ -345,6 +364,15 @@
  	 * adjust PGA to max value when ADC is on and will never go back.
  	*/
  	SOC_DOUBLE_R("AGC Switch", LAGC_CTRL_A, RAGC_CTRL_A, 7, 0x01, 0),
+	SOC_ENUM("Left AGC Target level", aic3x_enum[LAGC_LEV_ENUM]),
+	SOC_ENUM("Right AGC Target level", aic3x_enum[RAGC_LEV_ENUM]),
+	SOC_ENUM("Left AGC Attack time", aic3x_enum[LAGC_ATT_ENUM]),
+	SOC_ENUM("Right AGC Attack time", aic3x_enum[RAGC_ATT_ENUM]),
+	SOC_ENUM("Left AGC Decay time", aic3x_enum[LAGC_DEC_ENUM]),
+	SOC_ENUM("Right AGC Decay time", aic3x_enum[RAGC_DEC_ENUM]),
+
+	/* De-emphasis */
+	SOC_DOUBLE("De-emphasis Switch", AIC3X_CODEC_DFILT_CTRL, 2, 0, 0x01, 0),

  	/* Input */
  	SOC_DOUBLE_R_TLV("PGA Capture Volume", LADC_VOL, RADC_VOL,
@@ -352,6 +380,8 @@
  	SOC_DOUBLE_R("PGA Capture Switch", LADC_VOL, RADC_VOL, 7, 0x01, 1),

  	SOC_ENUM("ADC HPF Cut-off", aic3x_enum[ADC_HPF_ENUM]),
+
+	SOC_ENUM("Mic Bias", aic3x_enum[MIC_BIAS_ENUM]),
  };

  /*
@@ -1261,6 +1291,13 @@
  	snd_soc_write(codec, AIC3X_PAGE_SELECT, PAGE0_SELECT);
  	snd_soc_write(codec, AIC3X_RESET, SOFT_RESET);

+	/* set to avoid artifacts on the audio output during power-on/off */
+	snd_soc_write(codec, AIC3X_HEADSET_DETECT_CTRL_B, 0x80); /*ac-coupled*/
+	snd_soc_write(codec, HPOUT_POP_REDUCTION, 0x4e); /* 10 + 4 ms, reference*/
+
+	/* short circuit protection */
+	snd_soc_write(codec, HPRCOM_CFG, 0x04);
+
  	/* DAC default volume and mute */
  	snd_soc_write(codec, LDAC_VOL, DEFAULT_VOL | MUTE_ON);
  	snd_soc_write(codec, RDAC_VOL, DEFAULT_VOL | MUTE_ON);
@@ -1274,8 +1311,9 @@
  	snd_soc_write(codec, DACL1_2_LLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
  	snd_soc_write(codec, DACR1_2_RLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
  	/* DAC to Mono Line Out default volume and route to Output mixer */
-	snd_soc_write(codec, DACL1_2_MONOLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
-	snd_soc_write(codec, DACR1_2_MONOLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
+	/* mix both channels with -6dB level */
+	snd_soc_write(codec, DACL1_2_MONOLOPM_VOL, (DEFAULT_VOL + 12) | ROUTE_ON);
+	snd_soc_write(codec, DACR1_2_MONOLOPM_VOL, (DEFAULT_VOL + 12) | ROUTE_ON);

  	/* unmute all outputs */
  	snd_soc_update_bits(codec, LLOPM_CTRL, UNMUTE, UNMUTE);
@@ -1292,6 +1330,12 @@
  	/* By default route Line1 to ADC PGA mixer */
  	snd_soc_write(codec, LINE1L_2_LADC_CTRL, 0x0);
  	snd_soc_write(codec, LINE1R_2_RADC_CTRL, 0x0);
+
+	/* AGC to -10dB, 20 / 500ms, no clip stepping, noise gate -90dB, hysteresis 3dB*/
+	snd_soc_write(codec, LAGC_CTRL_A, 0x2f);
+	snd_soc_write(codec, RAGC_CTRL_A, 0x2f);
+	snd_soc_write(codec, LAGC_CTRL_C, 0xbe);
+	snd_soc_write(codec, RAGC_CTRL_C, 0xbe);

  	/* PGA to HP Bypass default volume, disconnect from Output Mixer */
  	snd_soc_write(codec, PGAL_2_HPLOUT_VOL, DEFAULT_VOL);

Re: [PATCH] ASoC: tlv320aic3x: add AGC, MIC_BIAS

[Mark Brown]

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

On Tue, Jun 26, 2012 at 03:04:27PM +0200, Prchal Jiří wrote:

>  	SOC_DOUBLE_R("AGC Switch", LAGC_CTRL_A, RAGC_CTRL_A, 7, 0x01, 0),
> +	SOC_ENUM("Left AGC Target level", aic3x_enum[LAGC_LEV_ENUM]),
> +	SOC_ENUM("Right AGC Target level", aic3x_enum[RAGC_LEV_ENUM]),
> +	SOC_ENUM("Left AGC Attack time", aic3x_enum[LAGC_ATT_ENUM]),
> +	SOC_ENUM("Right AGC Attack time", aic3x_enum[RAGC_ATT_ENUM]),
> +	SOC_ENUM("Left AGC Decay time", aic3x_enum[LAGC_DEC_ENUM]),
> +	SOC_ENUM("Right AGC Decay time", aic3x_enum[RAGC_DEC_ENUM]),

Please move everything out of the big array or if you insist on having
the array then use the indexes in the assignments.  Otherwise the code
is just error prone and hard to read.

> +
> +	SOC_ENUM("Mic Bias", aic3x_enum[MIC_BIAS_ENUM]),

No, this should be platform data or done with a callback from the
machine driver.  It's very unlikely that it's sensible to vary at
runtime without coordination with other driver code.

> +	/* set to avoid artifacts on the audio output during power-on/off */
> +	snd_soc_write(codec, AIC3X_HEADSET_DETECT_CTRL_B, 0x80); /*ac-coupled*/
> +	snd_soc_write(codec, HPOUT_POP_REDUCTION, 0x4e); /* 10 + 4 ms, reference*/
> +
> +	/* short circuit protection */
> +	snd_soc_write(codec, HPRCOM_CFG, 0x04);
> +

This appears to be unrelated to the change.  What is it for?

>  	/* DAC to Mono Line Out default volume and route to Output mixer */
> -	snd_soc_write(codec, DACL1_2_MONOLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
> -	snd_soc_write(codec, DACR1_2_MONOLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
> +	/* mix both channels with -6dB level */
> +	snd_soc_write(codec, DACL1_2_MONOLOPM_VOL, (DEFAULT_VOL + 12) | ROUTE_ON);
> +	snd_soc_write(codec, DACR1_2_MONOLOPM_VOL, (DEFAULT_VOL + 12) | ROUTE_ON);

This is also an unrelated change to the chip defaults (which we were
overriding anyway when we shouldn't be...).  Code like this should just
be removed completely.

> +	/* AGC to -10dB, 20 / 500ms, no clip stepping, noise gate -90dB, hysteresis 3dB*/
> +	snd_soc_write(codec, LAGC_CTRL_A, 0x2f);
> +	snd_soc_write(codec, RAGC_CTRL_A, 0x2f);
> +	snd_soc_write(codec, LAGC_CTRL_C, 0xbe);
> +	snd_soc_write(codec, RAGC_CTRL_C, 0xbe);

Again, just use the chip defaults and let the user override.

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

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

Re: [PATCH] ASoC: tlv320aic3x: add AGC, MIC_BIAS

[Prchal Jiří]

Hi Mark,

Dne 26.6.2012 15:17, Mark Brown napsal(a):
> On Tue, Jun 26, 2012 at 03:04:27PM +0200, Prchal Jiří wrote:
>
>>   	SOC_DOUBLE_R("AGC Switch", LAGC_CTRL_A, RAGC_CTRL_A, 7, 0x01, 0),
>> +	SOC_ENUM("Left AGC Target level", aic3x_enum[LAGC_LEV_ENUM]),
>> +	SOC_ENUM("Right AGC Target level", aic3x_enum[RAGC_LEV_ENUM]),
>> +	SOC_ENUM("Left AGC Attack time", aic3x_enum[LAGC_ATT_ENUM]),
>> +	SOC_ENUM("Right AGC Attack time", aic3x_enum[RAGC_ATT_ENUM]),
>> +	SOC_ENUM("Left AGC Decay time", aic3x_enum[LAGC_DEC_ENUM]),
>> +	SOC_ENUM("Right AGC Decay time", aic3x_enum[RAGC_DEC_ENUM]),
>
> Please move everything out of the big array or if you insist on having
> the array then use the indexes in the assignments.  Otherwise the code
> is just error prone and hard to read.
I just add some elements to big array.
As I see in other codec drivers (e.g. wm9713) there are big arrays too.
I think it's more readable to use symbolic names instead of indexes.

>
>> +
>> +	SOC_ENUM("Mic Bias", aic3x_enum[MIC_BIAS_ENUM]),
>
> No, this should be platform data or done with a callback from the
> machine driver.  It's very unlikely that it's sensible to vary at
> runtime without coordination with other driver code.
I don't know how, can you, please, suggest me. There are three levels of voltage.

>
>> +	/* set to avoid artifacts on the audio output during power-on/off */
>> +	snd_soc_write(codec, AIC3X_HEADSET_DETECT_CTRL_B, 0x80); /*ac-coupled*/
>> +	snd_soc_write(codec, HPOUT_POP_REDUCTION, 0x4e); /* 10 + 4 ms, reference*/
>> +
>> +	/* short circuit protection */
>> +	snd_soc_write(codec, HPRCOM_CFG, 0x04);
>> +
>
> This appears to be unrelated to the change.  What is it for?
This sets default values and I think it would be nice to have defaultly reduced pops-up and short circuit protection.

>
>>   	/* DAC to Mono Line Out default volume and route to Output mixer */
>> -	snd_soc_write(codec, DACL1_2_MONOLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
>> -	snd_soc_write(codec, DACR1_2_MONOLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
>> +	/* mix both channels with -6dB level */
>> +	snd_soc_write(codec, DACL1_2_MONOLOPM_VOL, (DEFAULT_VOL + 12) | ROUTE_ON);
>> +	snd_soc_write(codec, DACR1_2_MONOLOPM_VOL, (DEFAULT_VOL + 12) | ROUTE_ON);
>
> This is also an unrelated change to the chip defaults (which we were
> overriding anyway when we shouldn't be...).  Code like this should just
> be removed completely.
No, correct mixing two channels to one is 1/2 + 1/2. 1/2 = -6dB.

>
>> +	/* AGC to -10dB, 20 / 500ms, no clip stepping, noise gate -90dB, hysteresis 3dB*/
>> +	snd_soc_write(codec, LAGC_CTRL_A, 0x2f);
>> +	snd_soc_write(codec, RAGC_CTRL_A, 0x2f);
>> +	snd_soc_write(codec, LAGC_CTRL_C, 0xbe);
>> +	snd_soc_write(codec, RAGC_CTRL_C, 0xbe);
>
> Again, just use the chip defaults and let the user override.
Originally the driver sets many defaults to hear playback, because after reset defaults won't hear anything and user 
need to set many switches and volumes. Maybe is not necessary to set AGC here.

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

Re: [PATCH] ASoC: tlv320aic3x: add AGC, MIC_BIAS

[Mark Brown]

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

On Wed, Jun 27, 2012 at 09:48:12AM +0200, Prchal Jiří wrote:

Please fix your mailer to word wrap within paragraphs, I've reflowed
your text for legibility.

> Dne 26.6.2012 15:17, Mark Brown napsal(a):
> >On Tue, Jun 26, 2012 at 03:04:27PM +0200, Prchal Jiří wrote:

> >Please move everything out of the big array or if you insist on having
> >the array then use the indexes in the assignments.  Otherwise the code
> >is just error prone and hard to read.

> I just add some elements to big array.
> As I see in other codec drivers (e.g. wm9713) there are big arrays too.
> I think it's more readable to use symbolic names instead of indexes.

I'm saying it's even better not to use an enormous array at all and you
should fix that up rather than extend it (especially given that you're
using a different style to the existing controls).  Only a few old
drivers still use the magic array indexes.

> >>+	SOC_ENUM("Mic Bias", aic3x_enum[MIC_BIAS_ENUM]),

> >No, this should be platform data or done with a callback from the
> >machine driver.  It's very unlikely that it's sensible to vary at
> >runtime without coordination with other driver code.

> I don't know how, can you, please, suggest me. There are three levels of voltage.

There are numerous examples of platform data in the kernel...

> >>+	/* set to avoid artifacts on the audio output during power-on/off */
> >>+	snd_soc_write(codec, AIC3X_HEADSET_DETECT_CTRL_B, 0x80); /*ac-coupled*/
> >>+	snd_soc_write(codec, HPOUT_POP_REDUCTION, 0x4e); /* 10 + 4 ms, reference*/

> >>+	/* short circuit protection */
> >>+	snd_soc_write(codec, HPRCOM_CFG, 0x04);

> >This appears to be unrelated to the change.  What is it for?

> This sets default values and I think it would be nice to have
> defaultly reduced pops-up and short circuit protection.

So it is as I said totally unrelated to the rest of your change and
should be sent separately.  As covered in SubmittingPatches don't make
multiple unrelated changes in a single commit, it is especially bad if
you don't even mention some of them.

> >>  	/* DAC to Mono Line Out default volume and route to Output mixer */
> >>-	snd_soc_write(codec, DACL1_2_MONOLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
> >>-	snd_soc_write(codec, DACR1_2_MONOLOPM_VOL, DEFAULT_VOL | ROUTE_ON);
> >>+	/* mix both channels with -6dB level */
> >>+	snd_soc_write(codec, DACL1_2_MONOLOPM_VOL, (DEFAULT_VOL + 12) | ROUTE_ON);
> >>+	snd_soc_write(codec, DACR1_2_MONOLOPM_VOL, (DEFAULT_VOL + 12) | ROUTE_ON);

> >This is also an unrelated change to the chip defaults (which we were
> >overriding anyway when we shouldn't be...).  Code like this should just
> >be removed completely.

> No, correct mixing two channels to one is 1/2 + 1/2. 1/2 = -6dB.

I'm not sure how that follows on from what I said?  To repeat, the code
you're changing shouldn't be here in the first place and what you're
doing is unrelated to the rest of your change.

> >>+	/* AGC to -10dB, 20 / 500ms, no clip stepping, noise gate -90dB, hysteresis 3dB*/
> >>+	snd_soc_write(codec, LAGC_CTRL_A, 0x2f);
> >>+	snd_soc_write(codec, RAGC_CTRL_A, 0x2f);
> >>+	snd_soc_write(codec, LAGC_CTRL_C, 0xbe);
> >>+	snd_soc_write(codec, RAGC_CTRL_C, 0xbe);

> >Again, just use the chip defaults and let the user override.

> Originally the driver sets many defaults to hear playback, because
> after reset defaults won't hear anything and user need to set many
> switches and volumes. Maybe is not necessary to set AGC here.

All this code in the driver should be removed, the driver should leave
the device in the default state for the most part.

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

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