[alsa-devel] [PATCH 0/4] ASoC: tlv320dac33: Support for turning codec off
Hello,
The following series adds support for turning the tlv320dac33 codec off in BIAS_STANDBY (and obviously in BIAS_OFF).
Since the driver already had mechanics for supporting turned off codec in register writes, the needed changes are mainly in the BIAS/stream handling.
As part of the series further optimization has been done to codec initialization, resume.
The following scenarios has been tested, and verified (I think these are covering all cases): 1. Analog bypass caused BIAS_STANDBY -> BIAS_ON We need to power on the codec, and do the chip init, but we does not need to execute the playback related configuration 2. Playback caused BIAS_STANDBY -> BIAS_ON We need to power on the codec, and do the chip init, and also we need to execute the playback related configuration. 3. Playback start, while Analog bypass is on (BIAS_ON -> BIAS_ON) We need to execute the playback related configuration. The codec is already on. 4. Analog bypass enable, while playback (BIAS_ON -> BIAS_ON) Nothing need to be done. 5. Playback start withing soc power down timeout (BIAS_ON -> BIAS_ON) We need to execute the playback related configuration. The codec is still on. 6. Analog bypass enable withing soc power down timeout (BIAS_ON -> BIAS_ON) Nothing need to be done.
This series applies on top of Liam's: git://git.kernel.org/pub/scm/linux/kernel/git/lrg/asoc-2.6.git for-2.6.35 branch
--- Peter Ujfalusi (4): ASoC: tlv320dac33: Optimize power up, and restore ASoC: tlv320dac33: Revised module loading, and DAC33 ID read ASoC: tlv320dac33: Manage a pointer for snd_pcm_substream in private structure ASoC: tlv320dac33: Support for turning off the codec in BIAS_STANDBY
sound/soc/codecs/tlv320dac33.c | 236 +++++++++++++++++++++++----------------- 1 files changed, 137 insertions(+), 99 deletions(-)
On power up we only need to initialize the codec, and restore only registers, which are not in either in DAPM nor in the playback start sequence. These are mostly gain related registers.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tlv320dac33.c | 111 ++++++++++++++++------------------------ 1 files changed, 44 insertions(+), 67 deletions(-)
diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c index 54b2a05..41cad6c 100644 --- a/sound/soc/codecs/tlv320dac33.c +++ b/sound/soc/codecs/tlv320dac33.c @@ -284,45 +284,54 @@ static int dac33_write16(struct snd_soc_codec *codec, unsigned int reg, return ret; }
-static void dac33_restore_regs(struct snd_soc_codec *codec) +static void dac33_init_chip(struct snd_soc_codec *codec) { struct tlv320dac33_priv *dac33 = snd_soc_codec_get_drvdata(codec); - u8 *cache = codec->reg_cache; - u8 data[2]; - int i, ret;
- if (!dac33->chip_power) + if (unlikely(!dac33->chip_power)) return;
- for (i = DAC33_PWR_CTRL; i <= DAC33_INTP_CTRL_B; i++) { - data[0] = i; - data[1] = cache[i]; - /* Skip the read only registers */ - if ((i >= DAC33_INT_OSC_STATUS && - i <= DAC33_INT_OSC_FREQ_RAT_READ_B) || - (i >= DAC33_FIFO_WPTR_MSB && i <= DAC33_FIFO_IRQ_FLAG) || - i == DAC33_DAC_STATUS_FLAGS || - i == DAC33_SRC_EST_REF_CLK_RATIO_A || - i == DAC33_SRC_EST_REF_CLK_RATIO_B) - continue; - ret = codec->hw_write(codec->control_data, data, 2); - if (ret != 2) - dev_err(codec->dev, "Write failed (%d)\n", ret); - } - for (i = DAC33_LDAC_PWR_CTRL; i <= DAC33_LINEL_TO_LLO_VOL; i++) { - data[0] = i; - data[1] = cache[i]; - ret = codec->hw_write(codec->control_data, data, 2); - if (ret != 2) - dev_err(codec->dev, "Write failed (%d)\n", ret); - } - for (i = DAC33_LINER_TO_RLO_VOL; i <= DAC33_OSC_TRIM; i++) { - data[0] = i; - data[1] = cache[i]; - ret = codec->hw_write(codec->control_data, data, 2); - if (ret != 2) - dev_err(codec->dev, "Write failed (%d)\n", ret); - } + /* 44-46: DAC Control Registers */ + /* A : DAC sample rate Fsref/1.5 */ + dac33_write(codec, DAC33_DAC_CTRL_A, DAC33_DACRATE(0)); + /* B : DAC src=normal, not muted */ + dac33_write(codec, DAC33_DAC_CTRL_B, DAC33_DACSRCR_RIGHT | + DAC33_DACSRCL_LEFT); + /* C : (defaults) */ + dac33_write(codec, DAC33_DAC_CTRL_C, 0x00); + + /* 64-65 : L&R DAC power control + Line In -> OUT 1V/V Gain, DAC -> OUT 4V/V Gain*/ + dac33_write(codec, DAC33_LDAC_PWR_CTRL, DAC33_LROUT_GAIN(2)); + dac33_write(codec, DAC33_RDAC_PWR_CTRL, DAC33_LROUT_GAIN(2)); + + /* 73 : volume soft stepping control, + clock source = internal osc (?) */ + dac33_write(codec, DAC33_ANA_VOL_SOFT_STEP_CTRL, DAC33_VOLCLKEN); + + /* 66 : LOP/LOM Modes */ + dac33_write(codec, DAC33_OUT_AMP_CM_CTRL, 0xff); + + /* 68 : LOM inverted from LOP */ + dac33_write(codec, DAC33_OUT_AMP_CTRL, (3<<2)); + + dac33_write(codec, DAC33_PWR_CTRL, DAC33_PDNALLB); + + /* Restore only selected registers (gains mostly) */ + dac33_write(codec, DAC33_LDAC_DIG_VOL_CTRL, + dac33_read_reg_cache(codec, DAC33_LDAC_DIG_VOL_CTRL)); + dac33_write(codec, DAC33_RDAC_DIG_VOL_CTRL, + dac33_read_reg_cache(codec, DAC33_RDAC_DIG_VOL_CTRL)); + + dac33_write(codec, DAC33_LDAC_DIG_VOL_CTRL, + dac33_read_reg_cache(codec, DAC33_LDAC_DIG_VOL_CTRL)); + dac33_write(codec, DAC33_RDAC_DIG_VOL_CTRL, + dac33_read_reg_cache(codec, DAC33_RDAC_DIG_VOL_CTRL)); + + dac33_write(codec, DAC33_LINEL_TO_LLO_VOL, + dac33_read_reg_cache(codec, DAC33_LINEL_TO_LLO_VOL)); + dac33_write(codec, DAC33_LINER_TO_RLO_VOL, + dac33_read_reg_cache(codec, DAC33_LINER_TO_RLO_VOL)); }
static inline void dac33_soft_power(struct snd_soc_codec *codec, int power) @@ -358,8 +367,7 @@ static int dac33_hard_power(struct snd_soc_codec *codec, int power)
dac33->chip_power = 1;
- /* Restore registers */ - dac33_restore_regs(codec); + dac33_init_chip(codec);
dac33_soft_power(codec, 1); } else { @@ -1269,35 +1277,6 @@ static int dac33_set_dai_fmt(struct snd_soc_dai *codec_dai, return 0; }
-static void dac33_init_chip(struct snd_soc_codec *codec) -{ - /* 44-46: DAC Control Registers */ - /* A : DAC sample rate Fsref/1.5 */ - dac33_write(codec, DAC33_DAC_CTRL_A, DAC33_DACRATE(0)); - /* B : DAC src=normal, not muted */ - dac33_write(codec, DAC33_DAC_CTRL_B, DAC33_DACSRCR_RIGHT | - DAC33_DACSRCL_LEFT); - /* C : (defaults) */ - dac33_write(codec, DAC33_DAC_CTRL_C, 0x00); - - /* 64-65 : L&R DAC power control - Line In -> OUT 1V/V Gain, DAC -> OUT 4V/V Gain*/ - dac33_write(codec, DAC33_LDAC_PWR_CTRL, DAC33_LROUT_GAIN(2)); - dac33_write(codec, DAC33_RDAC_PWR_CTRL, DAC33_LROUT_GAIN(2)); - - /* 73 : volume soft stepping control, - clock source = internal osc (?) */ - dac33_write(codec, DAC33_ANA_VOL_SOFT_STEP_CTRL, DAC33_VOLCLKEN); - - /* 66 : LOP/LOM Modes */ - dac33_write(codec, DAC33_OUT_AMP_CM_CTRL, 0xff); - - /* 68 : LOM inverted from LOP */ - dac33_write(codec, DAC33_OUT_AMP_CTRL, (3<<2)); - - dac33_write(codec, DAC33_PWR_CTRL, DAC33_PDNALLB); -} - static int dac33_soc_probe(struct platform_device *pdev) { struct snd_soc_device *socdev = platform_get_drvdata(pdev); @@ -1313,8 +1292,6 @@ static int dac33_soc_probe(struct platform_device *pdev)
/* Power up the codec */ dac33_hard_power(codec, 1); - /* Set default configuration */ - dac33_init_chip(codec);
/* register pcms */ ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1);
On Friday 30 April 2010 10:31:52 Ujfalusi Peter (Nokia-D/Tampere) wrote:
On power up we only need to initialize the codec, and restore only registers, which are not in either in DAPM nor in the playback start sequence. These are mostly gain related registers.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com
...
- /* Restore only selected registers (gains mostly) */
- dac33_write(codec, DAC33_LDAC_DIG_VOL_CTRL,
dac33_read_reg_cache(codec, DAC33_LDAC_DIG_VOL_CTRL));- dac33_write(codec, DAC33_RDAC_DIG_VOL_CTRL,
dac33_read_reg_cache(codec, DAC33_RDAC_DIG_VOL_CTRL));- dac33_write(codec, DAC33_LDAC_DIG_VOL_CTRL,
dac33_read_reg_cache(codec, DAC33_LDAC_DIG_VOL_CTRL));- dac33_write(codec, DAC33_RDAC_DIG_VOL_CTRL,
dac33_read_reg_cache(codec, DAC33_RDAC_DIG_VOL_CTRL));
Hmm, restoring the same registers twice... I'll remove the duplication in the v2 series (which I'm sure will come)
- dac33_write(codec, DAC33_LINEL_TO_LLO_VOL,
dac33_read_reg_cache(codec, DAC33_LINEL_TO_LLO_VOL));- dac33_write(codec, DAC33_LINER_TO_RLO_VOL,
dac33_read_reg_cache(codec, DAC33_LINER_TO_RLO_VOL));}
Optimize the way how tlv320dac33 is powered uppon module and soc initialization. Also read the DAC33 ID registers, and update the reg_cache to reflect it.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tlv320dac33.c | 37 ++++++++++++++++++------------------- 1 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c index 41cad6c..7d01759 100644 --- a/sound/soc/codecs/tlv320dac33.c +++ b/sound/soc/codecs/tlv320dac33.c @@ -334,6 +334,15 @@ static void dac33_init_chip(struct snd_soc_codec *codec) dac33_read_reg_cache(codec, DAC33_LINER_TO_RLO_VOL)); }
+static inline void dac33_read_id(struct snd_soc_codec *codec) +{ + u8 reg; + + dac33_read(codec, DAC33_DEVICE_ID_MSB, ®); + dac33_read(codec, DAC33_DEVICE_ID_LSB, ®); + dac33_read(codec, DAC33_DEVICE_REV_ID, ®); +} + static inline void dac33_soft_power(struct snd_soc_codec *codec, int power) { u8 reg; @@ -1290,9 +1299,6 @@ static int dac33_soc_probe(struct platform_device *pdev) socdev->card->codec = codec; dac33 = snd_soc_codec_get_drvdata(codec);
- /* Power up the codec */ - dac33_hard_power(codec, 1); - /* register pcms */ ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1); if (ret < 0) { @@ -1312,9 +1318,6 @@ static int dac33_soc_probe(struct platform_device *pdev) /* power on device */ dac33_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
- /* Bias level configuration has enabled regulator an extra time */ - regulator_bulk_disable(ARRAY_SIZE(dac33->supplies), dac33->supplies); - return 0;
pcm_err: @@ -1464,8 +1467,6 @@ static int __devinit dac33_i2c_probe(struct i2c_client *client, goto error_gpio; } gpio_direction_output(dac33->power_gpio, 0); - } else { - dac33->chip_power = 1; }
/* Check if the IRQ number is valid and request it */ @@ -1503,12 +1504,14 @@ static int __devinit dac33_i2c_probe(struct i2c_client *client, goto err_get; }
- ret = regulator_bulk_enable(ARRAY_SIZE(dac33->supplies), - dac33->supplies); + /* Read the tlv320dac33 ID registers */ + ret = dac33_hard_power(codec, 1); if (ret != 0) { - dev_err(codec->dev, "Failed to enable supplies: %d\n", ret); - goto err_enable; + dev_err(codec->dev, "Failed to power up codec: %d\n", ret); + goto error_codec; } + dac33_read_id(codec); + dac33_hard_power(codec, 0);
ret = snd_soc_register_codec(codec); if (ret != 0) { @@ -1523,14 +1526,9 @@ static int __devinit dac33_i2c_probe(struct i2c_client *client, goto error_codec; }
- /* Shut down the codec for now */ - dac33_hard_power(codec, 0); - return ret;
error_codec: - regulator_bulk_disable(ARRAY_SIZE(dac33->supplies), dac33->supplies); -err_enable: regulator_bulk_free(ARRAY_SIZE(dac33->supplies), dac33->supplies); err_get: if (dac33->irq >= 0) { @@ -1554,14 +1552,15 @@ static int __devexit dac33_i2c_remove(struct i2c_client *client) struct tlv320dac33_priv *dac33;
dac33 = i2c_get_clientdata(client); - dac33_hard_power(&dac33->codec, 0); + + if (unlikely(dac33->chip_power)) + dac33_hard_power(&dac33->codec, 0);
if (dac33->power_gpio >= 0) gpio_free(dac33->power_gpio); if (dac33->irq >= 0) free_irq(dac33->irq, &dac33->codec);
- regulator_bulk_disable(ARRAY_SIZE(dac33->supplies), dac33->supplies); regulator_bulk_free(ARRAY_SIZE(dac33->supplies), dac33->supplies);
destroy_workqueue(dac33->dac33_wq);
As a preparation for supporting codec to be turned off, when we are in BIAS_STANDBY.
The substream must be easily available in other places than pcm_* callbacks.
Manage a pointer in _startup, and _shutdown for this.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tlv320dac33.c | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c index 7d01759..6edad36 100644 --- a/sound/soc/codecs/tlv320dac33.c +++ b/sound/soc/codecs/tlv320dac33.c @@ -91,6 +91,7 @@ struct tlv320dac33_priv { struct work_struct work; struct snd_soc_codec codec; struct regulator_bulk_data supplies[DAC33_NUM_SUPPLIES]; + struct snd_pcm_substream *substream; int power_gpio; int chip_power; int irq; @@ -725,6 +726,31 @@ static void dac33_oscwait(struct snd_soc_codec *codec) "internal oscillator calibration failed\n"); }
+static int dac33_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_device *socdev = rtd->socdev; + struct snd_soc_codec *codec = socdev->card->codec; + struct tlv320dac33_priv *dac33 = snd_soc_codec_get_drvdata(codec); + + /* Stream started, save the substream pointer */ + dac33->substream = substream; + + return 0; +} + +static void dac33_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_device *socdev = rtd->socdev; + struct snd_soc_codec *codec = socdev->card->codec; + struct tlv320dac33_priv *dac33 = snd_soc_codec_get_drvdata(codec); + + dac33->substream = NULL; +} + static int dac33_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) @@ -1372,6 +1398,8 @@ EXPORT_SYMBOL_GPL(soc_codec_dev_tlv320dac33); #define DAC33_FORMATS SNDRV_PCM_FMTBIT_S16_LE
static struct snd_soc_dai_ops dac33_dai_ops = { + .startup = dac33_startup, + .shutdown = dac33_shutdown, .hw_params = dac33_hw_params, .prepare = dac33_pcm_prepare, .trigger = dac33_pcm_trigger,
When the codec is in STANDBY we can actually turn it off. When the codec is off, than the associated regulator can be also turned off (if the number of users on the regulator is 0).
After initialization, the codec remains in power off, it is only turned on for reading the ID registers (also testing the regulators).
The codec power is enabled, when the codec is moving from BIAS_STANDBY to BIAS_ON (via the BIAS_PREPARE state). The codec is turned off, when it hits BIAS_STANDBY or BIAS_OFF.
There are few scenarios, which has to be taken care:: 1. Analog bypass caused BIAS_STANDBY -> BIAS_ON We need to power on the codec, and do the chip init, but we does not need to execute the playback related configuration 2. Playback caused BIAS_STANDBY -> BIAS_ON We need to power on the codec, and do the chip init, and also we need to execute the playback related configuration. 3. Playback start, while Analog bypass is on (BIAS_ON -> BIAS_ON) We need to execute the playback related configuration. The codec is already on. 4. Analog bypass enable, while playback (BIAS_ON -> BIAS_ON) Nothing need to be done. 5. Playback start withing soc power down timeout (BIAS_ON -> BIAS_ON) We need to execute the playback related configuration. The codec is still on.
Since the power up, and the codec init is optimized, the added overhead in stream start is minimal.
Withing this patch, the hard_power function is now only doing what it supposed to: only handle the powers, and GPIO reset line. The codec initialization and state restore has been moved out.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tlv320dac33.c | 62 +++++++++++++++++++++++++++++++--------- 1 files changed, 48 insertions(+), 14 deletions(-)
diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c index 6edad36..d513f86 100644 --- a/sound/soc/codecs/tlv320dac33.c +++ b/sound/soc/codecs/tlv320dac33.c @@ -62,6 +62,8 @@ (rate / (1000000 / us))
+static int dac33_prepare_chip(struct snd_pcm_substream *substream); + static struct snd_soc_codec *tlv320dac33_codec;
enum dac33_state { @@ -360,9 +362,17 @@ static inline void dac33_soft_power(struct snd_soc_codec *codec, int power) static int dac33_hard_power(struct snd_soc_codec *codec, int power) { struct tlv320dac33_priv *dac33 = snd_soc_codec_get_drvdata(codec); - int ret; + int ret = 0;
mutex_lock(&dac33->mutex); + + /* Safety check */ + if (unlikely(power == dac33->chip_power)) { + dev_warn(codec->dev, "Trying to set the same power state: %s\n", + power ? "ON" : "OFF"); + goto exit; + } + if (power) { ret = regulator_bulk_enable(ARRAY_SIZE(dac33->supplies), dac33->supplies); @@ -376,10 +386,6 @@ static int dac33_hard_power(struct snd_soc_codec *codec, int power) gpio_set_value(dac33->power_gpio, 1);
dac33->chip_power = 1; - - dac33_init_chip(codec); - - dac33_soft_power(codec, 1); } else { dac33_soft_power(codec, 0); if (dac33->power_gpio >= 0) @@ -562,6 +568,7 @@ static int dac33_add_widgets(struct snd_soc_codec *codec) static int dac33_set_bias_level(struct snd_soc_codec *codec, enum snd_soc_bias_level level) { + struct tlv320dac33_priv *dac33 = snd_soc_codec_get_drvdata(codec); int ret;
switch (level) { @@ -569,21 +576,36 @@ static int dac33_set_bias_level(struct snd_soc_codec *codec, dac33_soft_power(codec, 1); break; case SND_SOC_BIAS_PREPARE: + if (codec->bias_level == SND_SOC_BIAS_STANDBY) { + /* Coming from STANDBY, power up, and init the chip */ + ret = dac33_hard_power(codec, 1); + if (ret != 0) + return ret; + + dac33_init_chip(codec); + /* + * If we have a stream started do the postponded + * configuration here + */ + if (dac33->substream) + dac33_prepare_chip(dac33->substream); + } break; case SND_SOC_BIAS_STANDBY: - if (codec->bias_level == SND_SOC_BIAS_OFF) { - ret = dac33_hard_power(codec, 1); + if (codec->bias_level != SND_SOC_BIAS_OFF) { + /* Not coming from OFF, switch off codec */ + ret = dac33_hard_power(codec, 0); if (ret != 0) return ret; } - - dac33_soft_power(codec, 0); break; case SND_SOC_BIAS_OFF: - ret = dac33_hard_power(codec, 0); - if (ret != 0) - return ret; - + if (codec->bias_level != SND_SOC_BIAS_STANDBY) { + /* Not coming from STANDBY, switch off codec */ + ret = dac33_hard_power(codec, 0); + if (ret != 0) + return ret; + } break; } codec->bias_level = level; @@ -834,6 +856,16 @@ static int dac33_prepare_chip(struct snd_pcm_substream *substream) }
mutex_lock(&dac33->mutex); + + if (!dac33->chip_power) { + /* + * Chip is not powered yet. + * Do the init in the dac33_set_bias_level later. + */ + mutex_unlock(&dac33->mutex); + return 0; + } + dac33_soft_power(codec, 0); dac33_soft_power(codec, 1);
@@ -1341,7 +1373,7 @@ static int dac33_soc_probe(struct platform_device *pdev)
dac33_add_widgets(codec);
- /* power on device */ + /* Set codec BIAS */ dac33_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
return 0; @@ -1380,6 +1412,8 @@ static int dac33_soc_resume(struct platform_device *pdev) struct snd_soc_codec *codec = socdev->card->codec;
dac33_set_bias_level(codec, SND_SOC_BIAS_STANDBY); + if (codec->suspend_bias_level == SND_SOC_BIAS_ON) + dac33_set_bias_level(codec, SND_SOC_BIAS_PREPARE); dac33_set_bias_level(codec, codec->suspend_bias_level);
return 0;
On Fri, Apr 30, 2010 at 10:31:55AM +0300, Peter Ujfalusi wrote:
When the codec is in STANDBY we can actually turn it off. When the codec is off, than the associated regulator can be also turned off (if the number of users on the regulator is 0).
You can just set idle_bias_off in the CODEC and then the core will push you down into _BIAS_OFF.
There are few scenarios, which has to be taken care::
- Analog bypass caused BIAS_STANDBY -> BIAS_ON We need to power on the codec, and do the chip init, but we does not need to execute the playback related configuration
Moving the playback related configuration into events on the DAC widgets (or probably a supply connected to the DAC widgets) seems like a good move for a lot of these scenarios? The core will then take care of ensuring that the startup sequence for the playback is called for you and the states can do what they're supposed to more directly.
On Friday 30 April 2010 11:41:00 ext Mark Brown wrote:
On Fri, Apr 30, 2010 at 10:31:55AM +0300, Peter Ujfalusi wrote:
When the codec is in STANDBY we can actually turn it off. When the codec is off, than the associated regulator can be also turned off (if the number of users on the regulator is 0).
You can just set idle_bias_off in the CODEC and then the core will push you down into _BIAS_OFF.
Good idea. Needed some code movement, but works fine. Thanks.
There are few scenarios, which has to be taken care::
Analog bypass caused BIAS_STANDBY -> BIAS_ON
We need to power on the codec, and do the chip init, but we does not need to execute the playback related configuration
Moving the playback related configuration into events on the DAC widgets (or probably a supply connected to the DAC widgets) seems like a good move for a lot of these scenarios? The core will then take care of ensuring that the startup sequence for the playback is called for you and the states can do what they're supposed to more directly.
This is not working. Actually it works, if we come from BIAS_OFF, but... If I restart the playback fast (within asoc timeout for BIAS change), than the widget will not get event (since it is still powered). This means that I can not do the needed reconfiguration for the tlv320dac33 -> audio breaks.
I will keep the current logic, but move it a bit with the idle_bias_off change.
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, Apr 30, 2010 at 12:45:38PM +0300, Peter Ujfalusi wrote:
On Friday 30 April 2010 11:41:00 ext Mark Brown wrote:
Moving the playback related configuration into events on the DAC widgets (or probably a supply connected to the DAC widgets) seems like a good move for a lot of these scenarios? The core will then take care of ensuring that the startup sequence for the playback is called for you and the states can do what they're supposed to more directly.
This is not working. Actually it works, if we come from BIAS_OFF, but... If I restart the playback fast (within asoc timeout for BIAS change), than the widget will not get event (since it is still powered). This means that I can not do the needed reconfiguration for the tlv320dac33 -> audio breaks.
I will keep the current logic, but move it a bit with the idle_bias_off change.
Hrm, you need to do this any time playback is started? Then just use the hooks in the normal audio stream bringup/teardown surely? It's possible that I'm missing something as a result of your list of use cases but I'd expect this to flow fairly naturally from the normal call flow.
On Friday 30 April 2010 12:56:37 ext Mark Brown wrote:
I will keep the current logic, but move it a bit with the idle_bias_off change.
Hrm, you need to do this any time playback is started?
Yes, otherwise the codec might get confused.
Then just use the hooks in the normal audio stream bringup/teardown surely? It's possible that I'm missing something as a result of your list of use cases but I'd expect this to flow fairly naturally from the normal call flow.
The thing is, that I want to handle the chip power in one place, and dac33_set_bias_level is a really good place for that.
Now, when the codec was in off, and I start the playback: [ 31.705444] dac33_prepare_chip (0) // called from pcm_prepare [ 31.708953] Bias level: STANDBY [ 31.729278] dac33_hard_power: 1 [ 31.726074] Bias level: PREPARE ... [ 31.833801] dac33_prepare_chip (1) // From set_bias_level ... DAPM switching ... [ 32.075775] Bias level: ON ... Workqueue kicks the codec (in FIFO enabled mode)
At the time when pcm_prepare is called the codec is still in OFF. So I just postponed the dac33_prepare_chip call for later, when the codec switches BIAS level. Than I enable the power and if there is a stream, than I do the preparation. Note: the BIAS level change is still within the pcm_prepare call chain...
But, if the codec is still in _ON, and the stream is restarted, than the codec can be re-configured within the pcm_prepare call, since it is on.
In this way I don't need to do any additional housekeeping while managing the power of the codec.
On Fri, Apr 30, 2010 at 01:10:14PM +0300, Peter Ujfalusi wrote:
On Friday 30 April 2010 12:56:37 ext Mark Brown wrote:
Then just use the hooks in the normal audio stream bringup/teardown surely? It's possible that I'm missing something as a result of your list of use cases but I'd expect this to flow fairly naturally from the normal call flow.
The thing is, that I want to handle the chip power in one place, and dac33_set_bias_level is a really good place for that.
Sure, but it shouldn't need to be worrying about playback at all.
At the time when pcm_prepare is called the codec is still in OFF. So I just postponed the dac33_prepare_chip call for later, when the codec switches BIAS level. Than I enable the power and if there is a stream, than I do the preparation. Note: the BIAS level change is still within the pcm_prepare call chain...
Surely a much more straightforward solution to this is just to add a post-DAPM prepare() callback to the DAI ops? It seems like a perfectly reasonable thing to have that callback and it means you can rely on the existing mechanisms having taken care of the power for you.
In this way I don't need to do any additional housekeeping while managing the power of the codec.
My point here is that it seems like you need to do more housekeeping than you should :)
On Friday 30 April 2010 13:23:10 ext Mark Brown wrote:
On Fri, Apr 30, 2010 at 01:10:14PM +0300, Peter Ujfalusi wrote:
On Friday 30 April 2010 12:56:37 ext Mark Brown wrote:
Then just use the hooks in the normal audio stream bringup/teardown surely? It's possible that I'm missing something as a result of your list of use cases but I'd expect this to flow fairly naturally from the normal call flow.
The thing is, that I want to handle the chip power in one place, and dac33_set_bias_level is a really good place for that.
Sure, but it shouldn't need to be worrying about playback at all.
Valid point ;)
At the time when pcm_prepare is called the codec is still in OFF. So I just postponed the dac33_prepare_chip call for later, when the codec switches BIAS level. Than I enable the power and if there is a stream, than I do the preparation. Note: the BIAS level change is still within the pcm_prepare call chain...
Surely a much more straightforward solution to this is just to add a post-DAPM prepare() callback to the DAI ops? It seems like a perfectly reasonable thing to have that callback and it means you can rely on the existing mechanisms having taken care of the power for you.
Hmm, right... Well. This is not going to work, I think. I need to keep the dac33_pcm_prepare level of configuration for cases, when the codec is in ON and a playback is starting (and if the codec is not ON, than respond it for later, when it is switch on), right? I _need_ to do the things, which is done in the dac33_prepare_chip function every time, when a stream is starting :(
Now, if I use DAPM_SUPPLY attached to the DAC: If the codec has been brought up because the loopback is enabled, than the dac33_prepare_chip will be called twice: once from dac33_pcm_prepare, and then from the SUPPLY event. This might be also the case if I use the post-DAPM prepare (or pre)
I do agree, that it is not really nice to have playback related thing in the dac33_set_bias_level, but so far I think that is the only way to avoid additional hassle (which means more places to have error, problems).
In this way I don't need to do any additional housekeeping while managing the power of the codec.
My point here is that it seems like you need to do more housekeeping than you should :)
HeHe :) My problem with that, is the additional house keeping needs more code, which usually means more place, where some corner case is not handled correctly. Well, more code == more place for bugs ;)
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, Apr 30, 2010 at 01:55:25PM +0300, Peter Ujfalusi wrote:
I need to keep the dac33_pcm_prepare level of configuration for cases, when the codec is in ON and a playback is starting (and if the codec is not ON, than respond it for later, when it is switch on), right? I _need_ to do the things, which is done in the dac33_prepare_chip function every time, when a stream is starting :(
Yes, absolutely - and if we add a post-DAPM prepare callback then you can just do that there without worrying about power in the driver.
I do agree, that it is not really nice to have playback related thing in the dac33_set_bias_level, but so far I think that is the only way to avoid additional hassle (which means more places to have error, problems).
Like I say, I think the addition of a post-DAPM callback on prepare ought to do the job just as well.
On Friday 30 April 2010 13:55:25 Ujfalusi Peter (Nokia-D/Tampere) wrote:
Now, if I use DAPM_SUPPLY attached to the DAC: If the codec has been brought up because the loopback is enabled, than the dac33_prepare_chip will be called twice: once from dac33_pcm_prepare, and then from the SUPPLY event. This might be also the case if I use the post-DAPM prepare (or pre)
And I admit that you are actually right (and I was wrong) :D
By adding SND_SOC_DAPM_PRE widget to a codec, and move the former content of dac33_pcm_prepare function to PRE_PMU event works like charm.
It is called all the time, when the stream starts/restarts. So I'll get rid of the dac33_pcm_prepare, and clean up the dac33_set_bias_level, and resubmit the series.
Thank you Mark for guiding me to the correct direction!
participants (2)
-
Mark Brown -
Peter Ujfalusi