[alsa-devel] [PATCH 1/4] ASoC: tlv320aic3x: Optimize PLL programming in aic3x_set_bias_level
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;
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);
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)
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?
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)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.
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);
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.
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.
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);
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.
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).
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
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
participants (3)
-
Jarkko Nikula -
Liam Girdwood -
Mark Brown