[PATCH 0/4] ASoC: tlv320dac33: Support for turning codec off

[PATCH 0/4] ASoC: tlv320dac33: Support for turning codec off

[Peter Ujfalusi]

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(-)

[PATCH 1/4] ASoC: tlv320dac33: Optimize power up, and restore

[Peter Ujfalusi]

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);
-- 
1.7.0.4

[PATCH 2/4] ASoC: tlv320dac33: Revised module loading, and DAC33 ID read

[Peter Ujfalusi]

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, &reg);
+	dac33_read(codec, DAC33_DEVICE_ID_LSB, &reg);
+	dac33_read(codec, DAC33_DEVICE_REV_ID, &reg);
+}
+
 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);
-- 
1.7.0.4

[PATCH 3/4] ASoC: tlv320dac33: Manage a pointer for snd_pcm_substream in private structure

[Peter Ujfalusi]

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,
-- 
1.7.0.4

[PATCH 4/4] ASoC: tlv320dac33: Support for turning off the codec in BIAS_STANDBY

[Peter Ujfalusi]

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;
-- 
1.7.0.4

Re: [PATCH 1/4] ASoC: tlv320dac33: Optimize power up, and restore

[Peter Ujfalusi]

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));
>  }
> 

-- 
Péter

Re: [PATCH 4/4] ASoC: tlv320dac33: Support for turning off the codec in BIAS_STANDBY

[Mark Brown]

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::
> 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

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.

Re: [PATCH 4/4] ASoC: tlv320dac33: Support for turning off the codec in BIAS_STANDBY

[Peter Ujfalusi]

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::
> > 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
> 
> 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

-- 
Péter

Re: [PATCH 4/4] ASoC: tlv320dac33: Support for turning off the codec in BIAS_STANDBY

[Mark Brown]

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.

Re: [PATCH 4/4] ASoC: tlv320dac33: Support for turning off the codec in BIAS_STANDBY

[Peter Ujfalusi]

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.

-- 
Péter

Re: [PATCH 4/4] ASoC: tlv320dac33: Support for?turning off the codec in BIAS_STANDBY

[Mark Brown]

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 :)

Re: [PATCH 4/4] ASoC: tlv320dac33: Support for turning off the codec in BIAS_STANDBY

[Peter Ujfalusi]

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

-- 
Péter

Re: [PATCH 4/4] ASoC: tlv320dac33: Support for turning off the codec in BIAS_STANDBY

[Mark Brown]

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.

Re: [PATCH 4/4] ASoC: tlv320dac33: Support for turning off the codec in BIAS_STANDBY

[Peter Ujfalusi]

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!

-- 
Péter