[PATCH 1/4] ASoC: tlv320aic3x: Optimize PLL programming in aic3x_set_bias_level

[PATCH 1/4] ASoC: tlv320aic3x: Optimize PLL programming in aic3x_set_bias_level

[Jarkko Nikula]

There is only need to enable/disable once the PLL when the bias is going
between on, prepare, standby and off states.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
---
 sound/soc/codecs/tlv320aic3x.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index b317586..94dc707 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -1069,7 +1069,8 @@ static int aic3x_set_bias_level(struct snd_soc_codec *codec,
 	case SND_SOC_BIAS_ON:
 		break;
 	case SND_SOC_BIAS_PREPARE:
-		if (aic3x->master) {
+		if (codec->bias_level == SND_SOC_BIAS_STANDBY &&
+		    aic3x->master) {
 			/* enable pll */
 			reg = aic3x_read_reg_cache(codec, AIC3X_PLL_PROGA_REG);
 			aic3x_write(codec, AIC3X_PLL_PROGA_REG,
@@ -1077,15 +1078,16 @@ static int aic3x_set_bias_level(struct snd_soc_codec *codec,
 		}
 		break;
 	case SND_SOC_BIAS_STANDBY:
-		/* fall through and disable pll */
-	case SND_SOC_BIAS_OFF:
-		if (aic3x->master) {
+		if (codec->bias_level == SND_SOC_BIAS_PREPARE &&
+		    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:
+		break;
 	}
 	codec->bias_level = level;
 
-- 
1.7.1

[PATCH 2/4] ASoC: tlv320aic3x: Add runtime regulator control to aic3x_set_bias_level

[Jarkko Nikula]

Now all the regulators are disabled when entering into SND_SOC_BIAS_OFF
and enabled when coming back to SND_SOC_BIAS_STANDBY state. Currently this
runtime control happens only with suspend/resume as this patch does not
change the default idle behavior.

This patch manages all the regulators and reset since it seems that register
sync is needed even if only analog supplies AVDD and DRVDD are disabled.
This was noted when the system was running with idle behavior changed and
IOVDD and DVDD were on.

It is not known are all the registers needed to sync or only some subset of
them. Therefore patch plays safe and does always full shutdown/power-up.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
---
 sound/soc/codecs/tlv320aic3x.c |   66 +++++++++++++++++++++++++++++++--------
 1 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 94dc707..c549b0f 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -64,6 +64,7 @@ static const char *aic3x_supply_names[AIC3X_NUM_SUPPLIES] = {
 /* codec private data */
 struct aic3x_priv {
 	struct regulator_bulk_data supplies[AIC3X_NUM_SUPPLIES];
+	int power;
 	enum snd_soc_control_type control_type;
 	struct aic3x_setup_data *setup;
 	void *control_data;
@@ -141,6 +142,7 @@ static inline void aic3x_write_reg_cache(struct snd_soc_codec *codec,
 static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 		       unsigned int value)
 {
+	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
 	u8 data[2];
 
 	/* data is
@@ -151,7 +153,8 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 	data[1] = value & 0xff;
 
 	aic3x_write_reg_cache(codec, data[0], data[1]);
-	if (codec->hw_write(codec->control_data, data, 2) == 2)
+	if (!aic3x->power ||
+	    codec->hw_write(codec->control_data, data, 2) == 2)
 		return 0;
 	else
 		return -EIO;
@@ -163,11 +166,17 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 static int aic3x_read(struct snd_soc_codec *codec, unsigned int reg,
 		      u8 *value)
 {
+	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
 	*value = reg & 0xff;
 
-	value[0] = i2c_smbus_read_byte_data(codec->control_data, value[0]);
+	if (aic3x->power) {
+		value[0] = i2c_smbus_read_byte_data(codec->control_data,
+						    value[0]);
+		aic3x_write_reg_cache(codec, reg, *value);
+	} else {
+		value[0] = aic3x_read_reg_cache(codec, reg);
+	}
 
-	aic3x_write_reg_cache(codec, reg, *value);
 	return 0;
 }
 
@@ -1059,6 +1068,41 @@ static int aic3x_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
+static int aic3x_set_power(struct snd_soc_codec *codec, int power)
+{
+	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
+	int i, ret;
+	u8 data[2];
+	u8 *cache = codec->reg_cache;
+
+	if (power) {
+		ret = regulator_bulk_enable(ARRAY_SIZE(aic3x->supplies),
+					    aic3x->supplies);
+		if (ret)
+			goto out;
+		if (aic3x->gpio_reset >= 0) {
+			udelay(1);
+			gpio_set_value(aic3x->gpio_reset, 1);
+		}
+		aic3x->power = 1;
+
+		/* Sync reg_cache with the hardware */
+		for (i = 0; i < ARRAY_SIZE(aic3x_reg); i++) {
+			data[0] = i;
+			data[1] = cache[i];
+			codec->hw_write(codec->control_data, data, 2);
+		}
+	} else {
+		aic3x->power = 0;
+		if (aic3x->gpio_reset >= 0)
+			gpio_set_value(aic3x->gpio_reset, 0);
+		ret = regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies),
+					     aic3x->supplies);
+	}
+out:
+	return ret;
+}
+
 static int aic3x_set_bias_level(struct snd_soc_codec *codec,
 				enum snd_soc_bias_level level)
 {
@@ -1078,6 +1122,8 @@ static int aic3x_set_bias_level(struct snd_soc_codec *codec,
 		}
 		break;
 	case SND_SOC_BIAS_STANDBY:
+		if (!aic3x->power)
+			aic3x_set_power(codec, 1);
 		if (codec->bias_level == SND_SOC_BIAS_PREPARE &&
 		    aic3x->master) {
 			/* disable pll */
@@ -1087,6 +1133,8 @@ static int aic3x_set_bias_level(struct snd_soc_codec *codec,
 		}
 		break;
 	case SND_SOC_BIAS_OFF:
+		if (aic3x->power)
+			aic3x_set_power(codec, 0);
 		break;
 	}
 	codec->bias_level = level;
@@ -1186,17 +1234,6 @@ static int aic3x_suspend(struct snd_soc_codec *codec, pm_message_t state)
 
 static int aic3x_resume(struct snd_soc_codec *codec)
 {
-	int i;
-	u8 data[2];
-	u8 *cache = codec->reg_cache;
-
-	/* Sync reg_cache with the hardware */
-	for (i = 0; i < ARRAY_SIZE(aic3x_reg); i++) {
-		data[0] = i;
-		data[1] = cache[i];
-		codec->hw_write(codec->control_data, data, 2);
-	}
-
 	aic3x_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
 
 	return 0;
@@ -1415,6 +1452,7 @@ static int aic3x_i2c_probe(struct i2c_client *i2c,
 		udelay(1);
 		gpio_set_value(aic3x->gpio_reset, 1);
 	}
+	aic3x->power = 1;
 
 	ret = snd_soc_register_codec(&i2c->dev,
 			&soc_codec_dev_aic3x, &aic3x_dai, 1);
-- 
1.7.1

Re: [PATCH 2/4] ASoC: tlv320aic3x: Add runtime regulator control to aic3x_set_bias_level

[Mark Brown]

On Fri, Sep 10, 2010 at 02:23:30PM +0300, Jarkko Nikula wrote:

> This patch manages all the regulators and reset since it seems that register
> sync is needed even if only analog supplies AVDD and DRVDD are disabled.
> This was noted when the system was running with idle behavior changed and
> IOVDD and DVDD were on.

This is normal, chips normally require all the core supplies to be
enabled to take them out of reset.

> @@ -151,7 +153,8 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
>  	data[1] = value & 0xff;
>  
>  	aic3x_write_reg_cache(codec, data[0], data[1]);
> -	if (codec->hw_write(codec->control_data, data, 2) == 2)
> +	if (!aic3x->power ||
> +	    codec->hw_write(codec->control_data, data, 2) == 2)
>  		return 0;
>  	else
>  		return -EIO;

If you were using the soc-core stuff you'd be able to use the cache_only
flag in the CODEC to do this.  It might make sense to still use the flag
so that when someone converts the driver to use soc-cache this doesn't
get missed.

>  static int aic3x_read(struct snd_soc_codec *codec, unsigned int reg,
>  		      u8 *value)
>  {
> +	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
>  	*value = reg & 0xff;
>  
> -	value[0] = i2c_smbus_read_byte_data(codec->control_data, value[0]);
> +	if (aic3x->power) {
> +		value[0] = i2c_smbus_read_byte_data(codec->control_data,
> +						    value[0]);
> +		aic3x_write_reg_cache(codec, reg, *value);
> +	} else {
> +		value[0] = aic3x_read_reg_cache(codec, reg);
> +	}
>  
> -	aic3x_write_reg_cache(codec, reg, *value);
>  	return 0;
>  }

This seems fishy - surely the read should be being satisfied from cache
all the time if it can be, and if the register does need to be read from
the hardware due to being volatile then it's an error to read it while
powered off?

Re: [PATCH 2/4] ASoC: tlv320aic3x: Add runtime regulator control to aic3x_set_bias_level

[Jarkko Nikula]

On Fri, 10 Sep 2010 12:47:17 +0100
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Fri, Sep 10, 2010 at 02:23:30PM +0300, Jarkko Nikula wrote:
> 
> > This patch manages all the regulators and reset since it seems that register
> > sync is needed even if only analog supplies AVDD and DRVDD are disabled.
> > This was noted when the system was running with idle behavior changed and
> > IOVDD and DVDD were on.
> 
> This is normal, chips normally require all the core supplies to be
> enabled to take them out of reset.
> 
Some chips were programmable with digital IO supply only. I'll leave
this comment here to show that this chip has been noted to require all
of them :-)

> > @@ -151,7 +153,8 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
> >  	data[1] = value & 0xff;
> >  
> >  	aic3x_write_reg_cache(codec, data[0], data[1]);
> > -	if (codec->hw_write(codec->control_data, data, 2) == 2)
> > +	if (!aic3x->power ||
> > +	    codec->hw_write(codec->control_data, data, 2) == 2)
> >  		return 0;
> >  	else
> >  		return -EIO;
> 
> If you were using the soc-core stuff you'd be able to use the cache_only
> flag in the CODEC to do this.  It might make sense to still use the flag
> so that when someone converts the driver to use soc-cache this doesn't
> get missed.
> 
Makes sense, I'll change this.

> >  static int aic3x_read(struct snd_soc_codec *codec, unsigned int reg,
> >  		      u8 *value)
> >  {
> > +	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
> >  	*value = reg & 0xff;
> >  
> > -	value[0] = i2c_smbus_read_byte_data(codec->control_data, value[0]);
> > +	if (aic3x->power) {
> > +		value[0] = i2c_smbus_read_byte_data(codec->control_data,
> > +						    value[0]);
> > +		aic3x_write_reg_cache(codec, reg, *value);
> > +	} else {
> > +		value[0] = aic3x_read_reg_cache(codec, reg);
> > +	}
> >  
> > -	aic3x_write_reg_cache(codec, reg, *value);
> >  	return 0;
> >  }
> 
> This seems fishy - surely the read should be being satisfied from cache
> all the time if it can be, and if the register does need to be read from
> the hardware due to being volatile then it's an error to read it while
> powered off?

Good question. I didn't look at this. Actually it seems there is a need
to add some function to indicate if headset & button detection or gpio
readings are required. We cannot shutdown if those are needed.


-- 
Jarkko

[PATCH 3/4] ASoC: tlv320aic3x: Use regulator notifiers for optimizing the cache sync

[Jarkko Nikula]

There is no need to reset the codec and perform cache sync if none of the
supply regulators were not disabled. Patch registers a notifier callback for
each supply and callback then sets a flag to indicate when cache sync is
required.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
---
Mark, struct aic3x_disable_nb was created for getting pointer to aic3x easily.
Probably same idea could be applied to wm8962 as well?
---
 sound/soc/codecs/tlv320aic3x.c |   69 +++++++++++++++++++++++++++++++++++----
 1 files changed, 62 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index c549b0f..d269ccf 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -61,9 +61,18 @@ static const char *aic3x_supply_names[AIC3X_NUM_SUPPLIES] = {
 	"DRVDD",	/* ADC Analog and Output Driver Voltage */
 };
 
+struct aic3x_priv;
+
+struct aic3x_disable_nb {
+	struct notifier_block nb;
+	struct aic3x_priv *aic3x;
+};
+
 /* codec private data */
 struct aic3x_priv {
+	struct snd_soc_codec *codec;
 	struct regulator_bulk_data supplies[AIC3X_NUM_SUPPLIES];
+	struct aic3x_disable_nb disable_nb[AIC3X_NUM_SUPPLIES];
 	int power;
 	enum snd_soc_control_type control_type;
 	struct aic3x_setup_data *setup;
@@ -142,7 +151,6 @@ static inline void aic3x_write_reg_cache(struct snd_soc_codec *codec,
 static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 		       unsigned int value)
 {
-	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
 	u8 data[2];
 
 	/* data is
@@ -153,7 +161,7 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 	data[1] = value & 0xff;
 
 	aic3x_write_reg_cache(codec, data[0], data[1]);
-	if (!aic3x->power ||
+	if (codec->cache_sync ||
 	    codec->hw_write(codec->control_data, data, 2) == 2)
 		return 0;
 	else
@@ -166,10 +174,9 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 static int aic3x_read(struct snd_soc_codec *codec, unsigned int reg,
 		      u8 *value)
 {
-	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
 	*value = reg & 0xff;
 
-	if (aic3x->power) {
+	if (!codec->cache_sync) {
 		value[0] = i2c_smbus_read_byte_data(codec->control_data,
 						    value[0]);
 		aic3x_write_reg_cache(codec, reg, *value);
@@ -1068,6 +1075,27 @@ static int aic3x_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
+static int aic3x_regulator_event(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	struct aic3x_disable_nb *disable_nb =
+		container_of(nb, struct aic3x_disable_nb, nb);
+	struct aic3x_priv *aic3x = disable_nb->aic3x;
+
+	if (!aic3x->codec)
+		return 0;
+
+	/*
+	 * put codec to reset and require cache sync as at least one of the
+	 * supplies was disabled
+	 */
+	if (aic3x->gpio_reset >= 0)
+		gpio_set_value(aic3x->gpio_reset, 0);
+	aic3x->codec->cache_sync = 1;
+
+	return 0;
+}
+
 static int aic3x_set_power(struct snd_soc_codec *codec, int power)
 {
 	struct aic3x_priv *aic3x = snd_soc_codec_get_drvdata(codec);
@@ -1080,11 +1108,18 @@ static int aic3x_set_power(struct snd_soc_codec *codec, int power)
 					    aic3x->supplies);
 		if (ret)
 			goto out;
+		aic3x->power = 1;
+		/*
+		 * reset release and cache sync is necessary only if some
+		 * supply was off
+		 */
+		if (!codec->cache_sync)
+			goto out;
+
 		if (aic3x->gpio_reset >= 0) {
 			udelay(1);
 			gpio_set_value(aic3x->gpio_reset, 1);
 		}
-		aic3x->power = 1;
 
 		/* Sync reg_cache with the hardware */
 		for (i = 0; i < ARRAY_SIZE(aic3x_reg); i++) {
@@ -1092,10 +1127,9 @@ static int aic3x_set_power(struct snd_soc_codec *codec, int power)
 			data[1] = cache[i];
 			codec->hw_write(codec->control_data, data, 2);
 		}
+		codec->cache_sync = 0;
 	} else {
 		aic3x->power = 0;
-		if (aic3x->gpio_reset >= 0)
-			gpio_set_value(aic3x->gpio_reset, 0);
 		ret = regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies),
 					     aic3x->supplies);
 	}
@@ -1337,6 +1371,7 @@ static int aic3x_probe(struct snd_soc_codec *codec)
 
 	codec->hw_write = (hw_write_t) i2c_master_send;
 	codec->control_data = aic3x->control_data;
+	aic3x->codec = codec;
 
 	aic3x_init(codec);
 
@@ -1440,6 +1475,18 @@ static int aic3x_i2c_probe(struct i2c_client *i2c,
 		dev_err(&i2c->dev, "Failed to request supplies: %d\n", ret);
 		goto err_get;
 	}
+	for (i = 0; i < ARRAY_SIZE(aic3x->supplies); i++) {
+		aic3x->disable_nb[i].nb.notifier_call = aic3x_regulator_event;
+		aic3x->disable_nb[i].aic3x = aic3x;
+		ret = regulator_register_notifier(aic3x->supplies[i].consumer,
+						  &aic3x->disable_nb[i].nb);
+		if (ret) {
+			dev_err(&i2c->dev,
+				"Failed to request regulator notifier: %d\n",
+				 ret);
+			goto err_notif;
+		}
+	}
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(aic3x->supplies),
 				    aic3x->supplies);
@@ -1461,6 +1508,10 @@ static int aic3x_i2c_probe(struct i2c_client *i2c,
 	return ret;
 
 err_enable:
+err_notif:
+	while (i--)
+		regulator_unregister_notifier(aic3x->supplies[i].consumer,
+					      &aic3x->disable_nb[i].nb);
 	regulator_bulk_free(ARRAY_SIZE(aic3x->supplies), aic3x->supplies);
 err_get:
 	if (aic3x->gpio_reset >= 0)
@@ -1473,12 +1524,16 @@ err_gpio:
 static int aic3x_i2c_remove(struct i2c_client *client)
 {
 	struct aic3x_priv *aic3x = i2c_get_clientdata(client);
+	int i;
 
 	if (aic3x->gpio_reset >= 0) {
 		gpio_set_value(aic3x->gpio_reset, 0);
 		gpio_free(aic3x->gpio_reset);
 	}
 	regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies), aic3x->supplies);
+	for (i = 0; i < ARRAY_SIZE(aic3x->supplies); i++)
+		regulator_unregister_notifier(aic3x->supplies[i].consumer,
+					      &aic3x->disable_nb[i].nb);
 	regulator_bulk_free(ARRAY_SIZE(aic3x->supplies), aic3x->supplies);
 
 	snd_soc_unregister_codec(&client->dev);
-- 
1.7.1

Re: [PATCH 3/4] ASoC: tlv320aic3x: Use regulator notifiers for optimizing the cache sync

[Mark Brown]

On Fri, Sep 10, 2010 at 02:23:31PM +0300, Jarkko Nikula wrote:

> Mark, struct aic3x_disable_nb was created for getting pointer to aic3x easily.
> Probably same idea could be applied to wm8962 as well?

Probably.  TBH I'd rather fix this in the notifier API - either way it's
pretty nasty.

> @@ -153,7 +161,7 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
>  	data[1] = value & 0xff;
>  
>  	aic3x_write_reg_cache(codec, data[0], data[1]);
> -	if (!aic3x->power ||
> +	if (codec->cache_sync ||
>  	    codec->hw_write(codec->control_data, data, 2) == 2)
>  		return 0;
>  	else

This isn't the expected use of cache_sync, the idea is that it is a flag
indicating that a cache sync is required - this will happen when writes
are held while the regulators are disabled but the regualators haven't
actually been powered down.  This can be nice since we end up not
needing to do I2C I/O during bulk configuration at startup, I'm hoping
that we may be able to exploit this even more in the future.

Re: [PATCH 3/4] ASoC: tlv320aic3x: Use regulator notifiers for optimizing the cache sync

[Jarkko Nikula]

On Fri, 10 Sep 2010 12:58:08 +0100
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Fri, Sep 10, 2010 at 02:23:31PM +0300, Jarkko Nikula wrote:
> 
> > Mark, struct aic3x_disable_nb was created for getting pointer to aic3x easily.
> > Probably same idea could be applied to wm8962 as well?
> 
> Probably.  TBH I'd rather fix this in the notifier API - either way it's
> pretty nasty.
> 
Yep. I read this that I can still use this idea in v2 :-)

I noticed that I managed to forget to add test for event in
aic3x_regulator_event so I need to resend.

> > @@ -153,7 +161,7 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
> >  	data[1] = value & 0xff;
> >  
> >  	aic3x_write_reg_cache(codec, data[0], data[1]);
> > -	if (!aic3x->power ||
> > +	if (codec->cache_sync ||
> >  	    codec->hw_write(codec->control_data, data, 2) == 2)
> >  		return 0;
> >  	else
> 
> This isn't the expected use of cache_sync, the idea is that it is a flag
> indicating that a cache sync is required - this will happen when writes
> are held while the regulators are disabled but the regualators haven't
> actually been powered down.  This can be nice since we end up not
> needing to do I2C I/O during bulk configuration at startup, I'm hoping
> that we may be able to exploit this even more in the future.

I'll change this to cache_only and set both flags in
aic3x_regulator_event so that core can take use of them. My idea was to
cover both regulators not disabled case and CONFIG_REGULATOR not set
case and picked up this flag as it was set.


-- 
Jarkko

[PATCH 4/4] ASoC: tlv320aic3x: Let the codec hit SND_SOC_BIAS_OFF when idle

[Jarkko Nikula]

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
---
 sound/soc/codecs/tlv320aic3x.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index d269ccf..291111d 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -1359,9 +1359,6 @@ static int aic3x_init(struct snd_soc_codec *codec)
 		aic3x_write(codec, CLASSD_CTRL, 0);
 	}
 
-	/* off, with power on */
-	aic3x_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
-
 	return 0;
 }
 
@@ -1383,6 +1380,9 @@ static int aic3x_probe(struct snd_soc_codec *codec)
 			    (aic3x->setup->gpio_func[1] & 0xf) << 4);
 	}
 
+	codec->idle_bias_off = 1;
+	aic3x_set_bias_level(codec, SND_SOC_BIAS_OFF);
+
 	snd_soc_add_controls(codec, aic3x_snd_controls,
 			     ARRAY_SIZE(aic3x_snd_controls));
 	if (aic3x->model == AIC3X_MODEL_3007)
@@ -1530,7 +1530,9 @@ static int aic3x_i2c_remove(struct i2c_client *client)
 		gpio_set_value(aic3x->gpio_reset, 0);
 		gpio_free(aic3x->gpio_reset);
 	}
-	regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies), aic3x->supplies);
+	if (aic3x->power)
+		regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies),
+				       aic3x->supplies);
 	for (i = 0; i < ARRAY_SIZE(aic3x->supplies); i++)
 		regulator_unregister_notifier(aic3x->supplies[i].consumer,
 					      &aic3x->disable_nb[i].nb);
-- 
1.7.1

Re: [PATCH 4/4] ASoC: tlv320aic3x: Let the codec hit SND_SOC_BIAS_OFF when idle

[Mark Brown]

On Fri, Sep 10, 2010 at 02:23:32PM +0300, Jarkko Nikula wrote:

> -	regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies), aic3x->supplies);
> +	if (aic3x->power)
> +		regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies),
> +				       aic3x->supplies);

This looks suspicious - doesn't it mean that the enable/disables won't
be balanced any more?  I'd expect to either see the disable being
uncondtional or the power flag being updated.  I could be missing
something, though.

Re: [PATCH 4/4] ASoC: tlv320aic3x: Let the codec hit SND_SOC_BIAS_OFF when idle

[Jarkko Nikula]

On Fri, 10 Sep 2010 13:00:17 +0100
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Fri, Sep 10, 2010 at 02:23:32PM +0300, Jarkko Nikula wrote:
> 
> > -	regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies), aic3x->supplies);
> > +	if (aic3x->power)
> > +		regulator_bulk_disable(ARRAY_SIZE(aic3x->supplies),
> > +				       aic3x->supplies);
> 
> This looks suspicious - doesn't it mean that the enable/disables won't
> be balanced any more?  I'd expect to either see the disable being
> uncondtional or the power flag being updated.  I could be missing
> something, though.

I agree.

This is actually due the current implementation where the regulators are
enabled in aic3x_i2c_probe and disabled in aic3x_probe. So this disable
is needed only if the driver is removed without aic3x_probe being
called.

I'll cook up one patch more that moves regulator setup to aic3x_probe
as most of the codec drivers are doing. (side issue: it's hard to probe
a chip if voltages are missing but doing regulator setup in i2c probe is
not any better either).


-- 
Jarkko

Re: [PATCH 1/4] ASoC: tlv320aic3x: Optimize PLL programming in aic3x_set_bias_level

[Mark Brown]

On Fri, Sep 10, 2010 at 02:23:29PM +0300, Jarkko Nikula wrote:
> There is only need to enable/disable once the PLL when the bias is going
> between on, prepare, standby and off states.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH 1/4] ASoC: tlv320aic3x: Optimize PLL programming in aic3x_set_bias_level

[Liam Girdwood]

On Fri, 2010-09-10 at 12:36 +0100, Mark Brown wrote:
> On Fri, Sep 10, 2010 at 02:23:29PM +0300, Jarkko Nikula wrote:
> > There is only need to enable/disable once the PLL when the bias is going
> > between on, prepare, standby and off states.
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Applied.

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk