[patch] tlv320aic3x: disable ADC/DAC while changing clock

[patch] tlv320aic3x: disable ADC/DAC while changing clock

[Daniel Glöckner]

While using a tlv320aic3101 as a clock master, it often happened that the
clock lines got stuck while changing the sampling rate. In a discussion
with TI technical support it was suggested to shut down the ADC and DAC
while there are changes being made to the clock.

Following that rule resolves the problem, but merely writing the
registers to switch the ADC/DAC off is not enough. One needs to poll the
power status registers to wait until they have completely shut down.

The aforementioned rule implies that we can not disable the PLL in
stand-by bias level.

Signed-off-by: Daniel Glöckner <dg@emlix.com>
---
 sound/soc/codecs/tlv320aic3x.c |   89 ++++++++++++++++++++++++++++------------
 sound/soc/codecs/tlv320aic3x.h |    8 ++++
 2 files changed, 71 insertions(+), 26 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index aea0cb7..4c44022 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -740,6 +740,57 @@ static int aic3x_add_widgets(struct snd_soc_codec *codec)
 	return 0;
 }
 
+static u8 aic3x_power_codec(struct snd_soc_codec *codec, u8 new)
+{
+	u8 val, old;
+
+	val = aic3x_read_reg_cache(codec, LINE1L_2_LADC_CTRL);
+	old = (val & LADC_PWR_ON) ? 1 : 0;
+	if ((old ^ new) & 1) {
+		val ^= LADC_PWR_ON;
+		aic3x_write(codec, LINE1L_2_LADC_CTRL, val);
+	}
+
+	val = aic3x_read_reg_cache(codec, LINE1R_2_RADC_CTRL);
+	old += (val & RADC_PWR_ON) ? 2 : 0;
+	if ((old ^ new) & 2) {
+		val ^= RADC_PWR_ON;
+		aic3x_write(codec, LINE1R_2_RADC_CTRL, val);
+	}
+
+	val = aic3x_read_reg_cache(codec, DAC_PWR);
+	old += (val & LDAC_PWR_ON) ? 4 : 0;
+	old += (val & RDAC_PWR_ON) ? 8 : 0;
+	if ((old ^ new) & 4)
+		val ^= LDAC_PWR_ON;
+	if ((old ^ new) & 8)
+		val ^= RDAC_PWR_ON;
+	if ((old ^ new) & 12)
+		aic3x_write(codec, DAC_PWR, val);
+
+	return old;
+}
+
+static u8 aic3x_power_off_wait(struct snd_soc_codec *codec)
+{
+	int timeout = 1000;
+	u8 old, data;
+	old = aic3x_power_codec(codec, 0);
+
+	do {
+		aic3x_read(codec, ADC_FLAGS, &data);
+	} while ((data & (LADC_PWR_STATUS | RADC_PWR_STATUS)) && --timeout > 0);
+
+	do {
+		aic3x_read(codec, MODULE_PWR_STATUS, &data);
+	} while ((data & (LDAC_PWR_STATUS | RDAC_PWR_STATUS)) && --timeout > 0);
+
+	if (timeout < 0)
+		printk(KERN_WARNING "aic3x: timeout waiting for ADC/DAC to shut down");
+
+	return old;
+}
+
 static int aic3x_hw_params(struct snd_pcm_substream *substream,
 			   struct snd_pcm_hw_params *params,
 			   struct snd_soc_dai *dai)
@@ -749,9 +800,11 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 	int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
-	u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
+	u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1, state_saved;
 	u16 pll_d = 1;
 
+	state_saved = aic3x_power_off_wait(codec);
+
 	/* select data word length */
 	data =
 	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
@@ -805,8 +858,10 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	data |= (data << 4);
 	aic3x_write(codec, AIC3X_SAMPLE_RATE_SEL_REG, data);
 
-	if (bypass_pll)
+	if (bypass_pll) {
+		aic3x_power_codec(codec, state_saved);
 		return 0;
+	}
 
 	/* Use PLL
 	 * find an apropriate setup for j, d, r and p by iterating over
@@ -847,6 +902,7 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 
 	if (last_clk == 0) {
 		printk(KERN_ERR "%s(): unable to setup PLL\n", __func__);
+		aic3x_power_codec(codec, state_saved);
 		return -EINVAL;
 	}
 
@@ -857,7 +913,11 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGC_REG, (pll_d >> 6) << PLLD_MSB_SHIFT);
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
+	data = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
+	aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p << PLLP_SHIFT)
+						     | PLL_ENABLE);
 
+	aic3x_power_codec(codec, state_saved);
 	return 0;
 }
 
@@ -951,37 +1011,14 @@ static int aic3x_set_bias_level(struct snd_soc_codec *codec,
 
 	switch (level) {
 	case SND_SOC_BIAS_ON:
-		/* all power is driven by DAPM system */
-		if (aic3x->master) {
-			/* enable pll */
-			reg = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
-			aic3x_write(codec, AIC3X_PLL_PROGA_REG,
-				    reg | PLL_ENABLE);
-		}
 		break;
 	case SND_SOC_BIAS_PREPARE:
 		break;
 	case SND_SOC_BIAS_STANDBY:
-		/*
-		 * all power is driven by DAPM system,
-		 * so output power is safe if bypass was set
-		 */
-		if (aic3x->master) {
-			/* disable pll */
-			reg = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
-			aic3x_write(codec, AIC3X_PLL_PROGA_REG,
-				    reg & ~PLL_ENABLE);
-		}
 		break;
 	case SND_SOC_BIAS_OFF:
 		/* force all power off */
-		reg = aic3x_read_reg_cache(codec, LINE1L_2_LADC_CTRL);
-		aic3x_write(codec, LINE1L_2_LADC_CTRL, reg & ~LADC_PWR_ON);
-		reg = aic3x_read_reg_cache(codec, LINE1R_2_RADC_CTRL);
-		aic3x_write(codec, LINE1R_2_RADC_CTRL, reg & ~RADC_PWR_ON);
-
-		reg = aic3x_read_reg_cache(codec, DAC_PWR);
-		aic3x_write(codec, DAC_PWR, reg & ~(LDAC_PWR_ON | RDAC_PWR_ON));
+		aic3x_power_off_wait(codec);
 
 		reg = aic3x_read_reg_cache(codec, HPLOUT_CTRL);
 		aic3x_write(codec, HPLOUT_CTRL, reg & ~HPLOUT_PWR_ON);
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index ac827e5..0b4fedf 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -69,6 +69,8 @@
 #define RAGC_CTRL_B			30
 #define RAGC_CTRL_C			31
 
+/* ADC Flag register */
+#define ADC_FLAGS			34
 /* DAC Power and Left High Power Output control registers */
 #define DAC_PWR				37
 #define HPLCOM_CFG			37
@@ -126,6 +128,8 @@
 #define DACR1_2_LLOPM_VOL		85
 #define LLOPM_CTRL			86
 #define RLOPM_CTRL			93
+/* Module Power status register */
+#define MODULE_PWR_STATUS		94
 /* GPIO/IRQ registers */
 #define AIC3X_STICKY_IRQ_FLAGS_REG	96
 #define AIC3X_RT_IRQ_FLAGS_REG		97
@@ -191,6 +195,10 @@
 #define MONOLOPM_PWR_ON		0x01
 #define LLOPM_PWR_ON		0x01
 #define RLOPM_PWR_ON	0x01
+#define LADC_PWR_STATUS		0x40
+#define RADC_PWR_STATUS		0x04
+#define LDAC_PWR_STATUS		0x80
+#define RDAC_PWR_STATUS		0x40
 
 #define INVERT_VOL(val)   (0x7f - val)
 
-- 
1.6.2.107.ge47ee

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

Re: [patch] tlv320aic3x: disable ADC/DAC while changing clock

[Mark Brown]

On Thu, Mar 26, 2009 at 02:21:46PM +0100, Daniel Gl??ckner wrote:
> While using a tlv320aic3101 as a clock master, it often happened that the
> clock lines got stuck while changing the sampling rate. In a discussion
> with TI technical support it was suggested to shut down the ADC and DAC
> while there are changes being made to the clock.

> Following that rule resolves the problem, but merely writing the
> registers to switch the ADC/DAC off is not enough. One needs to poll the
> power status registers to wait until they have completely shut down.

> The aforementioned rule implies that we can not disable the PLL in
> stand-by bias level.

Could you go into more detail on why you believe that this is the case?
The DACs and ADCs won't be operational when the bias is held at standby
which means that at most standby needs to wait for them to go idle.

In any case, it's looking more and more like the driver should just
implement a set_pll() operaton here...

> +static u8 aic3x_power_codec(struct snd_soc_codec *codec, u8 new)
> +{

This really needs some comments explaining what it's doing; it's not
really clear what effect it's trying to achieve or how it interacts with
DAPM.

> @@ -749,9 +800,11 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
>  	struct snd_soc_codec *codec = socdev->codec;
>  	struct aic3x_priv *aic3x = codec->private_data;
>  	int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
> -	u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
> +	u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1, state_saved;
>  	u16 pll_d = 1;
>  
> +	state_saved = aic3x_power_off_wait(codec);
> +

This appears to unconditionally power off the DAC and ADC when setting
parameters - I'd expect that to cause issues with simultaneous record
and playback, even for completely symmetric configurations where there's
no actual change in configuration being made.  It'd be better to check
to see if a change is being requested while the device is operational
and refuse to reconfigure if that is the case.

>  	if (last_clk == 0) {
>  		printk(KERN_ERR "%s(): unable to setup PLL\n", __func__);
> +		aic3x_power_codec(codec, state_saved);
>  		return -EINVAL;

This looks wrong - if the configuration is known broken then I'd expect
that it'd be safer to leave things powered off.

Re: [patch] tlv320aic3x: disable ADC/DAC while changing clock

[Daniel Glöckner]

On 03/26/2009 02:45 PM, Mark Brown wrote:
> On Thu, Mar 26, 2009 at 02:21:46PM +0100, Daniel Gl??ckner wrote:
>> The aforementioned rule implies that we can not disable the PLL in
>> stand-by bias level.
> 
> Could you go into more detail on why you believe that this is the case?
> The DACs and ADCs won't be operational when the bias is held at standby
> which means that at most standby needs to wait for them to go idle.

This is based on the fact that soc_pcm_prepare calls
snd_soc_dapm_stream_event(..., SND_SOC_DAPM_STREAM_START) before
snd_soc_dapm_set_bias_level(..., SND_SOC_BIAS_ON).

In addition there is a block of code in tlv320aic3x.c that explicitly disables
the ADC/DAC in SND_SOC_BIAS_OFF. It may be superfluous, though.

>> +static u8 aic3x_power_codec(struct snd_soc_codec *codec, u8 new)
>> +{
> 
> This really needs some comments explaining what it's doing; it's not
> really clear what effect it's trying to achieve or how it interacts with
> DAPM.

This function takes a bitmask of the desired power state of the ADC/DAC blocks
and configures the device accordingly. It returns a bitmask of the previous
power state, suitable for input.

  Daniel


-- 
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax -11, Bahnhofsallee 1b, 37081 Göttingen, Germany
Geschäftsführung: Dr. Uwe Kracke, Dr. Cord Seele, Ust-IdNr.: DE 205 198 055
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160

emlix - your embedded linux partner

Re: [patch] tlv320aic3x: disable ADC/DAC while changing clock

[Mark Brown]

On Mon, Mar 30, 2009 at 02:48:09PM +0200, Daniel Gl?ckner wrote:
> On 03/26/2009 02:45 PM, Mark Brown wrote:

> > Could you go into more detail on why you believe that this is the case?
> > The DACs and ADCs won't be operational when the bias is held at standby
> > which means that at most standby needs to wait for them to go idle.

> This is based on the fact that soc_pcm_prepare calls
> snd_soc_dapm_stream_event(..., SND_SOC_DAPM_STREAM_START) before
> snd_soc_dapm_set_bias_level(..., SND_SOC_BIAS_ON).

So enable the PLL on BIAS_PREPARE?  That's probably more correct anyway.

> In addition there is a block of code in tlv320aic3x.c that explicitly disables
> the ADC/DAC in SND_SOC_BIAS_OFF. It may be superfluous, though.

Yes, all that code is redundant - all the widgets will be powered down
before the bias is removed.

> >> +static u8 aic3x_power_codec(struct snd_soc_codec *codec, u8 new)
> >> +{

> > This really needs some comments explaining what it's doing; it's not
> > really clear what effect it's trying to achieve or how it interacts with
> > DAPM.

> This function takes a bitmask of the desired power state of the ADC/DAC blocks
> and configures the device accordingly. It returns a bitmask of the previous
> power state, suitable for input.

"Suitable for input"?  Like I say, there need to be way more comments in
the code.

[PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Daniel Glöckner]

While using a tlv320aic3101 as a clock master, it often happened that the
clock lines got stuck while changing the sampling rate. In a discussion
with TI technical support it was suggested to shut down the ADC and DAC
while there are changes being made to the clock.

Following that rule resolves the problem, but merely writing the
registers to switch the ADC/DAC off is not enough. One needs to poll the
power status registers to wait until they have completely shut down.

The aforementioned rule implies that we can not disable the PLL in
stand-by bias level.

Changes compared to v1 of this patch:

Enabling/disabling of the PLL has been reinstated in another bias level
and is done only if the PLL is required. Waiting for ADC/DAC shutdown
has been added at those points.

Redundant code disabling dapm widgets in bias level off has been removed.

Changing of hw parameters is allowed only while the stream in the other
direction is not running. snd_pcm_hw_params() takes care of our stream
not running.

Comments have been added to describe the helper functions.

Signed-off-by: Daniel Glöckner <dg@emlix.com>
---
 sound/soc/codecs/tlv320aic3x.c |  160 +++++++++++++++++++++++++++++----------
 sound/soc/codecs/tlv320aic3x.h |    8 ++
 2 files changed, 127 insertions(+), 41 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index ab099f4..3422700 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -55,6 +55,9 @@
 struct aic3x_priv {
 	unsigned int sysclk;
 	int master;
+	unsigned int rate;
+	unsigned int format;
+	char running[SNDRV_PCM_STREAM_LAST + 1];
 };
 
 /*
@@ -756,6 +759,67 @@ static int aic3x_add_widgets(struct snd_soc_codec *codec)
 	return 0;
 }
 
+/*
+ * Enables or disables the left/right ADC/DAC according to new.
+ * Bits in new:
+ *	0 left ADC
+ *	1 right ADC
+ *	2 left DAC
+ *	4 right DAC
+ * A set bit enables the component.
+ * Returns the previous state of those components encoded in the same way.
+ */
+static u8 aic3x_power_adc_dac(struct snd_soc_codec *codec, u8 new)
+{
+	u8 val, old;
+
+	val = aic3x_read_reg_cache(codec, LINE1L_2_LADC_CTRL);
+	old = (val & LADC_PWR_ON) ? 1 : 0;
+	if ((old ^ new) & 1) {
+		val ^= LADC_PWR_ON;
+		aic3x_write(codec, LINE1L_2_LADC_CTRL, val);
+	}
+
+	val = aic3x_read_reg_cache(codec, LINE1R_2_RADC_CTRL);
+	old += (val & RADC_PWR_ON) ? 2 : 0;
+	if ((old ^ new) & 2) {
+		val ^= RADC_PWR_ON;
+		aic3x_write(codec, LINE1R_2_RADC_CTRL, val);
+	}
+
+	val = aic3x_read_reg_cache(codec, DAC_PWR);
+	old += (val & LDAC_PWR_ON) ? 4 : 0;
+	old += (val & RDAC_PWR_ON) ? 8 : 0;
+	if ((old ^ new) & 4)
+		val ^= LDAC_PWR_ON;
+	if ((old ^ new) & 8)
+		val ^= RDAC_PWR_ON;
+	if ((old ^ new) & 12)
+		aic3x_write(codec, DAC_PWR, val);
+
+	return old;
+}
+
+/*
+ * Polls the power status registers until ADC and DAC are both off or timeout
+ */
+static void aic3x_wait_adc_dac_off(struct snd_soc_codec *codec)
+{
+	int timeout = 1000;
+	u8 data;
+	do {
+		aic3x_read(codec, ADC_FLAGS, &data);
+	} while ((data & (LADC_PWR_STATUS | RADC_PWR_STATUS)) && --timeout > 0);
+
+	do {
+		aic3x_read(codec, MODULE_PWR_STATUS, &data);
+	} while ((data & (LDAC_PWR_STATUS | RDAC_PWR_STATUS)) && --timeout > 0);
+
+	if (timeout < 0)
+		printk(KERN_WARNING "aic3x: timeout waiting for ADC/DAC"
+				    " to shut down");
+}
+
 static int aic3x_hw_params(struct snd_pcm_substream *substream,
 			   struct snd_pcm_hw_params *params,
 			   struct snd_soc_dai *dai)
@@ -765,9 +829,23 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_codec *codec = socdev->card->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 	int codec_clk = 0, bypass_pll = 0, fsref, last_clk = 0;
-	u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1;
+	u8 data, r, p, pll_q, pll_p = 1, pll_r = 1, pll_j = 1, old_state;
 	u16 pll_d = 1;
 
+	if (aic3x->format == params_format(params) &&
+	    aic3x->rate == params_rate(params))
+		return 0;
+
+	if (aic3x->running[substream->stream ^ 1])
+		return -EINVAL;
+
+	aic3x->format = params_format(params);
+	aic3x->rate = params_rate(params);
+
+	/* switch off the ADC/DAC while changing parameters */
+	old_state = aic3x_power_adc_dac(codec, 0);
+	aic3x_wait_adc_dac_off(codec);
+
 	/* select data word length */
 	data =
 	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
@@ -821,8 +899,10 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	data |= (data << 4);
 	aic3x_write(codec, AIC3X_SAMPLE_RATE_SEL_REG, data);
 
-	if (bypass_pll)
+	if (bypass_pll) {
+		aic3x_power_adc_dac(codec, old_state);
 		return 0;
+	}
 
 	/* Use PLL
 	 * find an apropriate setup for j, d, r and p by iterating over
@@ -863,17 +943,23 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 
 	if (last_clk == 0) {
 		printk(KERN_ERR "%s(): unable to setup PLL\n", __func__);
+		aic3x->rate = 0;
 		return -EINVAL;
 	}
 
 	data = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
-	aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p << PLLP_SHIFT));
+	if (codec->bias_level == SND_SOC_BIAS_ON ||
+	    codec->bias_level == SND_SOC_BIAS_PREPARE)
+		data |= PLL_ENABLE;
+
 	aic3x_write(codec, AIC3X_OVRF_STATUS_AND_PLLR_REG, pll_r << PLLR_SHIFT);
 	aic3x_write(codec, AIC3X_PLL_PROGB_REG, pll_j << PLLJ_SHIFT);
 	aic3x_write(codec, AIC3X_PLL_PROGC_REG, (pll_d >> 6) << PLLD_MSB_SHIFT);
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
+	aic3x_write(codec, AIC3X_PLL_PROGA_REG, data | (pll_p << PLLP_SHIFT));
 
+	aic3x_power_adc_dac(codec, old_state);
 	return 0;
 }
 
@@ -959,70 +1045,60 @@ static int aic3x_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
+static int aic3x_trigger(struct snd_pcm_substream *substream, int cmd,
+			 struct snd_soc_dai *codec_dai)
+{
+	struct snd_soc_codec *codec = codec_dai->codec;
+	struct aic3x_priv *aic3x = codec->private_data;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		aic3x->running[substream->stream] = 1;
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		aic3x->running[substream->stream] = 0;
+	}
+	return 0;
+}
+
 static int aic3x_set_bias_level(struct snd_soc_codec *codec,
 				enum snd_soc_bias_level level)
 {
-	struct aic3x_priv *aic3x = codec->private_data;
 	u8 reg;
 
 	switch (level) {
 	case SND_SOC_BIAS_ON:
+		break;
+	case SND_SOC_BIAS_PREPARE:
 		/* all power is driven by DAPM system */
-		if (aic3x->master) {
+		if ((aic3x_read_reg_cache(codec, AIC3X_GPIOB_REG) & 1)
+				== CODEC_CLKIN_PLLDIV) {
 			/* enable pll */
 			reg = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
+			aic3x_wait_adc_dac_off(codec);
 			aic3x_write(codec, AIC3X_PLL_PROGA_REG,
 				    reg | PLL_ENABLE);
 		}
 		break;
-	case SND_SOC_BIAS_PREPARE:
-		break;
 	case SND_SOC_BIAS_STANDBY:
 		/*
 		 * all power is driven by DAPM system,
 		 * so output power is safe if bypass was set
 		 */
-		if (aic3x->master) {
+		if ((aic3x_read_reg_cache(codec, AIC3X_GPIOB_REG) & 1)
+				 == CODEC_CLKIN_PLLDIV) {
 			/* disable pll */
 			reg = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
+			aic3x_wait_adc_dac_off(codec);
 			aic3x_write(codec, AIC3X_PLL_PROGA_REG,
 				    reg & ~PLL_ENABLE);
 		}
 		break;
 	case SND_SOC_BIAS_OFF:
-		/* force all power off */
-		reg = aic3x_read_reg_cache(codec, LINE1L_2_LADC_CTRL);
-		aic3x_write(codec, LINE1L_2_LADC_CTRL, reg & ~LADC_PWR_ON);
-		reg = aic3x_read_reg_cache(codec, LINE1R_2_RADC_CTRL);
-		aic3x_write(codec, LINE1R_2_RADC_CTRL, reg & ~RADC_PWR_ON);
-
-		reg = aic3x_read_reg_cache(codec, DAC_PWR);
-		aic3x_write(codec, DAC_PWR, reg & ~(LDAC_PWR_ON | RDAC_PWR_ON));
-
-		reg = aic3x_read_reg_cache(codec, HPLOUT_CTRL);
-		aic3x_write(codec, HPLOUT_CTRL, reg & ~HPLOUT_PWR_ON);
-		reg = aic3x_read_reg_cache(codec, HPROUT_CTRL);
-		aic3x_write(codec, HPROUT_CTRL, reg & ~HPROUT_PWR_ON);
-
-		reg = aic3x_read_reg_cache(codec, HPLCOM_CTRL);
-		aic3x_write(codec, HPLCOM_CTRL, reg & ~HPLCOM_PWR_ON);
-		reg = aic3x_read_reg_cache(codec, HPRCOM_CTRL);
-		aic3x_write(codec, HPRCOM_CTRL, reg & ~HPRCOM_PWR_ON);
-
-		reg = aic3x_read_reg_cache(codec, MONOLOPM_CTRL);
-		aic3x_write(codec, MONOLOPM_CTRL, reg & ~MONOLOPM_PWR_ON);
-
-		reg = aic3x_read_reg_cache(codec, LLOPM_CTRL);
-		aic3x_write(codec, LLOPM_CTRL, reg & ~LLOPM_PWR_ON);
-		reg = aic3x_read_reg_cache(codec, RLOPM_CTRL);
-		aic3x_write(codec, RLOPM_CTRL, reg & ~RLOPM_PWR_ON);
-
-		if (aic3x->master) {
-			/* disable pll */
-			reg = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
-			aic3x_write(codec, AIC3X_PLL_PROGA_REG,
-				    reg & ~PLL_ENABLE);
-		}
 		break;
 	}
 	codec->bias_level = level;
@@ -1093,6 +1169,7 @@ static struct snd_soc_dai_ops aic3x_dai_ops = {
 	.digital_mute	= aic3x_mute,
 	.set_sysclk	= aic3x_set_dai_sysclk,
 	.set_fmt	= aic3x_set_dai_fmt,
+	.trigger	= aic3x_trigger,
 };
 
 struct snd_soc_dai aic3x_dai = {
@@ -1383,6 +1460,7 @@ static int aic3x_probe(struct platform_device *pdev)
 		kfree(codec);
 		return -ENOMEM;
 	}
+	aic3x->format = SNDRV_PCM_FORMAT_LAST + 1;
 
 	codec->private_data = aic3x;
 	socdev->card->codec = codec;
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index ac827e5..0b4fedf 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -69,6 +69,8 @@
 #define RAGC_CTRL_B			30
 #define RAGC_CTRL_C			31
 
+/* ADC Flag register */
+#define ADC_FLAGS			34
 /* DAC Power and Left High Power Output control registers */
 #define DAC_PWR				37
 #define HPLCOM_CFG			37
@@ -126,6 +128,8 @@
 #define DACR1_2_LLOPM_VOL		85
 #define LLOPM_CTRL			86
 #define RLOPM_CTRL			93
+/* Module Power status register */
+#define MODULE_PWR_STATUS		94
 /* GPIO/IRQ registers */
 #define AIC3X_STICKY_IRQ_FLAGS_REG	96
 #define AIC3X_RT_IRQ_FLAGS_REG		97
@@ -191,6 +195,10 @@
 #define MONOLOPM_PWR_ON		0x01
 #define LLOPM_PWR_ON		0x01
 #define RLOPM_PWR_ON	0x01
+#define LADC_PWR_STATUS		0x40
+#define RADC_PWR_STATUS		0x04
+#define LDAC_PWR_STATUS		0x80
+#define RDAC_PWR_STATUS		0x40
 
 #define INVERT_VOL(val)   (0x7f - val)
 
-- 
1.6.1.3

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

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Mark Brown]

On Wed, Apr 01, 2009 at 12:56:55PM +0200, Daniel Gl??ckner wrote:

> +static int aic3x_trigger(struct snd_pcm_substream *substream, int cmd,
> +                        struct snd_soc_dai *codec_dai)
> +{
> +       struct snd_soc_codec *codec = codec_dai->codec;
> +       struct aic3x_priv *aic3x = codec->private_data;
> +
> +       switch (cmd) {
> +       case SNDRV_PCM_TRIGGER_START:
> +       case SNDRV_PCM_TRIGGER_RESUME:
> +       case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +               aic3x->running[substream->stream] = 1;
> +               break;

Hrm, does the chip support asymmetric configurations for playback and
capture?

> +/*
> + * Enables or disables the left/right ADC/DAC according to new.
> + * Bits in new:
> + *	0 left ADC
> + *	1 right ADC
> + *	2 left DAC
> + *	4 right DAC
> + * A set bit enables the component.
> + * Returns the previous state of those components encoded in the same way.
> + */

Is this really required?  I can't see it being a good idea to bounce the
power of the DAC or ADC while they are live since that will impact the
audio stream noticably.  It looks like it'd be better to do a check
which refuses to do the reconfiguration when it's not possible.

It's especially suspicious since the power of the DAC and ADC is managed
via DAPM; the only current user should be safe since it saves then
restores the state but it does raise alarm bells.

>  	case SND_SOC_BIAS_OFF:
> -		/* force all power off */
> -		reg = aic3x_read_reg_cache(codec, LINE1L_2_LADC_CTRL);
> -		aic3x_write(codec, LINE1L_2_LADC_CTRL, reg & ~LADC_PWR_ON);

This should be split into a separate patch; it's a separate change.

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Daniel Glöckner]

On 04/01/2009 02:10 PM, Mark Brown wrote:
> On Wed, Apr 01, 2009 at 12:56:55PM +0200, Daniel Glöckner wrote:
> 
>> +static int aic3x_trigger(struct snd_pcm_substream *substream, int cmd,
>> +                        struct snd_soc_dai *codec_dai)
>> +{
>> +       struct snd_soc_codec *codec = codec_dai->codec;
>> +       struct aic3x_priv *aic3x = codec->private_data;
>> +
>> +       switch (cmd) {
>> +       case SNDRV_PCM_TRIGGER_START:
>> +       case SNDRV_PCM_TRIGGER_RESUME:
>> +       case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +               aic3x->running[substream->stream] = 1;
>> +               break;
> 
> Hrm, does the chip support asymmetric configurations for playback and
> capture?

No, why do you ask?

>> +/*
>> + * Enables or disables the left/right ADC/DAC according to new.
>> + * Bits in new:
>> + *	0 left ADC
>> + *	1 right ADC
>> + *	2 left DAC
>> + *	4 right DAC
>> + * A set bit enables the component.
>> + * Returns the previous state of those components encoded in the same way.
>> + */
> 
> Is this really required?  I can't see it being a good idea to bounce the
> power of the DAC or ADC while they are live since that will impact the
> audio stream noticably.  It looks like it'd be better to do a check
> which refuses to do the reconfiguration when it's not possible.

Powering off is required in case the stream has been closed previously but the
close_delayed_work has not yet been run. That was the case where I observed the
problems this patch tries to fix.

> It's especially suspicious since the power of the DAC and ADC is managed
> via DAPM; the only current user should be safe since it saves then
> restores the state but it does raise alarm bells.

How about making it a function that always disables the four components and
referring to snd_soc_dapm_sync to restore the state?

In this version of the patch I removed the line that restores the state when
setting the sampling rate failed in-between. Is it safe to assume that DAPM will
not enable the ADC/DAC on other occasions before a valid sampling rate has been
configured? One could reorder aic3x_hw_params to detect the error before the
first registers are written..

>>  	case SND_SOC_BIAS_OFF:
>> -		/* force all power off */
>> -		reg = aic3x_read_reg_cache(codec, LINE1L_2_LADC_CTRL);
>> -		aic3x_write(codec, LINE1L_2_LADC_CTRL, reg & ~LADC_PWR_ON);
> 
> This should be split into a separate patch; it's a separate change.

Ok

  Daniel

-- 
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax -11, Bahnhofsallee 1b, 37081 Göttingen, Germany
Geschäftsführung: Dr. Uwe Kracke, Dr. Cord Seele, Ust-IdNr.: DE 205 198 055
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160

emlix - your embedded linux partner

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Mark Brown]

On Wed, Apr 01, 2009 at 04:06:05PM +0200, Daniel Gl?ckner wrote:
> On 04/01/2009 02:10 PM, Mark Brown wrote:

> > Hrm, does the chip support asymmetric configurations for playback and
> > capture?

> No, why do you ask?

In that case rather than using a trigger to check if the device is
running it should be possible to use constraints to prevent
reconfiguration of the device when it's not supported - this is more
friendly to apps since they know they won't be allowed to change the
configuration.

> >> + * Enables or disables the left/right ADC/DAC according to new.
> >> + * Bits in new:

> > Is this really required?  I can't see it being a good idea to bounce the
> > power of the DAC or ADC while they are live since that will impact the
> > audio stream noticably.  It looks like it'd be better to do a check
> > which refuses to do the reconfiguration when it's not possible.

> Powering off is required in case the stream has been closed previously but the
m close_delayed_work has not yet been run. That was the case where I observed the
> problems this patch tries to fix.

Eew, right.  This should go in the comments.

> > It's especially suspicious since the power of the DAC and ADC is managed
> > via DAPM; the only current user should be safe since it saves then
> > restores the state but it does raise alarm bells.

> How about making it a function that always disables the four components and
> referring to snd_soc_dapm_sync to restore the state?

Hrm.  For the ADC that's probably OK but powering off the DAC does risk
being audible in the output; that does depend on the hardware, though.
It should do the right thing, I think, but checking for audio artefacts
would be advisable.

It'd probably also help to only do reconfiguration if actually required
rather than always powering down the ADCs and DACs.

> In this version of the patch I removed the line that restores the state when
> setting the sampling rate failed in-between. Is it safe to assume that DAPM will
> not enable the ADC/DAC on other occasions before a valid sampling rate has been

You should always get a chance to do a configuration before the DAC or
ADC is enabled, yes.

> configured? One could reorder aic3x_hw_params to detect the error before the
> first registers are written..

Yes.

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Daniel Glöckner]

On 04/01/2009 05:24 PM, Mark Brown wrote:
> In that case rather than using a trigger to check if the device is
> running it should be possible to use constraints to prevent
> reconfiguration of the device when it's not supported - this is more
> friendly to apps since they know they won't be allowed to change the
> configuration.

It was my impression that it is up to the pcm part to install these
constraints as this is where SNDRV_PCM_INFO_JOINT_DUPLEX can be put into
the snd_pcm_hardware structure.

>>> It's especially suspicious since the power of the DAC and ADC is managed
>>> via DAPM; the only current user should be safe since it saves then
>>> restores the state but it does raise alarm bells.
> 
>> How about making it a function that always disables the four components and
>> referring to snd_soc_dapm_sync to restore the state?
> 
> Hrm.  For the ADC that's probably OK but powering off the DAC does risk
> being audible in the output; that does depend on the hardware, though.
> It should do the right thing, I think, but checking for audio artefacts
> would be advisable.

I can't hear any artefacts with my cheap headphones.
The delayed close is audible, though.

> It'd probably also help to only do reconfiguration if actually required
> rather than always powering down the ADCs and DACs.

That's what is done in these lines at the very first in aic3x_hw_params:

+	if (aic3x->format == params_format(params) &&
+	    aic3x->rate == params_rate(params))
+		return 0;


>> One could reorder aic3x_hw_params to detect the error before the
>> first registers are written..
> 
> Yes.

I'll make it three patches then:
- one to remove the redundant actions on bias level off
- one to reorder hw_params
- and one to fix the bug

  Daniel

-- 
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax -11, Bahnhofsallee 1b, 37081 Göttingen, Germany
Geschäftsführung: Dr. Uwe Kracke, Dr. Cord Seele, Ust-IdNr.: DE 205 198 055
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160

emlix - your embedded linux partner

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Mark Brown]

On Wed, Apr 01, 2009 at 06:21:19PM +0200, Daniel Gl?ckner wrote:
> On 04/01/2009 05:24 PM, Mark Brown wrote:
> > In that case rather than using a trigger to check if the device is
> > running it should be possible to use constraints to prevent
> > reconfiguration of the device when it's not supported - this is more
> > friendly to apps since they know they won't be allowed to change the
> > configuration.

> It was my impression that it is up to the pcm part to install these
> constraints as this is where SNDRV_PCM_INFO_JOINT_DUPLEX can be put into
> the snd_pcm_hardware structure.

JOINT_DUPLEX is just a hint to the application about how the card will
behave - you still need to enforce the actual constraints you have in
the driver.  In any case, the PCM driver shouldn't be setting that
unless it has such a constraint itself since it should work with the
widest possible range of systems.

> > Hrm.  For the ADC that's probably OK but powering off the DAC does risk
> > being audible in the output; that does depend on the hardware, though.
> > It should do the right thing, I think, but checking for audio artefacts
> > would be advisable.

> I can't hear any artefacts with my cheap headphones.
> The delayed close is audible, though.

Probably due to something else in the output path powerdown (eg, the
output stages).  If it happens without the change to power down the DAC
it's a separate issue :)

> - and one to fix the bug

This sounds like a good plan.  Please put something explaining the
failure scenario in the comments so anyone pulling the PLL configuration
out for machine drivers doesn't get suprised by this.

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Daniel Glöckner]

On 04/01/2009 06:48 PM, Mark Brown wrote:
> On Wed, Apr 01, 2009 at 06:21:19PM +0200, Daniel Glöckner wrote:
>> It was my impression that it is up to the pcm part to install these
>> constraints as this is where SNDRV_PCM_INFO_JOINT_DUPLEX can be put into
>> the snd_pcm_hardware structure.
> 
> JOINT_DUPLEX is just a hint to the application about how the card will
> behave - you still need to enforce the actual constraints you have in
> the driver.  In any case, the PCM driver shouldn't be setting that
> unless it has such a constraint itself since it should work with the
> widest possible range of systems.

Then there needs to be a way for codecs and machines to force this bit if
the PCM driver didn't set it.

IMHO joint duplex is a hack that works only by chance.
Constraints that depend on the configuration of the first stream on open of the
second stream are no longer valid when the first stream is closed.
It is also not possible to add constraints when the first stream has been opened
but not yet configured.

  Daniel

-- 
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax -11, Bahnhofsallee 1b, 37081 Göttingen, Germany
Geschäftsführung: Dr. Uwe Kracke, Dr. Cord Seele, Ust-IdNr.: DE 205 198 055
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160

emlix - your embedded linux partner

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Mark Brown]

On Wed, Apr 01, 2009 at 07:16:15PM +0200, Daniel Gl?ckner wrote:
> On 04/01/2009 06:48 PM, Mark Brown wrote:

> > JOINT_DUPLEX is just a hint to the application about how the card will

> Then there needs to be a way for codecs and machines to force this bit if
> the PCM driver didn't set it.

Ideally, yes, but it's not really important if it's set or not since
it's the constraints that actually matter.  Like I say, it's just a
hint.

> IMHO joint duplex is a hack that works only by chance.
> Constraints that depend on the configuration of the first stream on open of the
> second stream are no longer valid when the first stream is closed.
> It is also not possible to add constraints when the first stream has been opened
> but not yet configured.

There's several existing drivers which do this (the fsl-ssi driver or
the wm8903 spring to mind) - I'm planning to provide a helper for it
shortly since it's quite a common requirement.  There are some races
there but they are fairly narrow and the ALSA APIs don't allow them to
be closed down; error handling should still be done.

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Jarkko Nikula]

On Wed, Apr 1, 2009 at 3:10 PM, Mark Brown <broonie@sirena.org.uk> wrote:

> > +       switch (cmd) {
> > +       case SNDRV_PCM_TRIGGER_START:
> > +       case SNDRV_PCM_TRIGGER_RESUME:
> > +       case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > +               aic3x->running[substream->stream] = 1;
> > +               break;
>
> Hrm, does the chip support asymmetric configurations for playback and
> capture?
>
> My two cents: It supports different sampling rates for ADC and DAC but
I don't believe there is practical use or HW doing this. In this setup
there is separate word clock signal on GPIO1 for ADC.

Unfortunately I'm bit off from the actual issue here but I remember some
TI codec had requirement to keep DAC/ADC off while changing the
configuration...


Jarkko

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Mark Brown]

On Thu, Apr 02, 2009 at 10:39:12AM +0300, Jarkko Nikula wrote:

> > Hrm, does the chip support asymmetric configurations for playback and
> > capture?

> My two cents: It supports different sampling rates for ADC and DAC but
> I don't believe there is practical use or HW doing this. In this setup
> there is separate word clock signal on GPIO1 for ADC.

Hrm, that's fairly common for hardware - even with shared LRCLK many
devices will be able to support asymmetric rates providing there are
enough BCLKs to drive the data.  Presumably the only limit in the codec
itself is going to be that whatever the PLL is set for will be the
maximum.

Being a bit stricter than is required probaly won't hurt; someone can
always relax things later on if required.

> Unfortunately I'm bit off from the actual issue here but I remember some
> TI codec had requirement to keep DAC/ADC off while changing the
> configuration...

Yes, that's the issue that Daniel is fixing here - it's not possible to
change the clocking (or at least the PLL) while a DAC or ADC is active.

Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

[Daniel Glöckner]

On 04/02/2009 12:34 PM, Mark Brown wrote:
> On Thu, Apr 02, 2009 at 10:39:12AM +0300, Jarkko Nikula wrote:
>> My two cents: It supports different sampling rates for ADC and DAC but
>> I don't believe there is practical use or HW doing this. In this setup
>> there is separate word clock signal on GPIO1 for ADC.
> 
> Hrm, that's fairly common for hardware - even with shared LRCLK many
> devices will be able to support asymmetric rates providing there are
> enough BCLKs to drive the data.  Presumably the only limit in the codec
> itself is going to be that whatever the PLL is set for will be the
> maximum.

It's only the tlv320aic33 that has GPIO pins.
On the other supported devices the datasheet says on register 98:
"Reserved. Write only 0 to these bits"

While we're at it, there are other registers where we are already writing
values != 0 which are not allowed on aic31 and aic32.

  Daniel

-- 
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax -11, Bahnhofsallee 1b, 37081 Göttingen, Germany
Geschäftsführung: Dr. Uwe Kracke, Dr. Cord Seele, Ust-IdNr.: DE 205 198 055
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160

emlix - your embedded linux partner