[alsa-devel] [patch] tlv320aic3x: disable ADC/DAC while changing clock
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
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.
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
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.
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
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.
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
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.
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
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.
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
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.
+ 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
On Wed, Apr 1, 2009 at 3:10 PM, Mark Brown <broonie@sirena.org.uk> wrote: 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
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.
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
participants (3)
-
Daniel Glöckner -
Jarkko Nikula -
Mark Brown