| Message ID | 20180220221511.22861-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 20, 2018 at 11:15:04PM +0100, Hans de Goede wrote: > There is no need to set the LDO voltage to 1.2 volt each time we enter > standby, instead always leave it 1.2 volt on BIAS_OFF. Note we do a > snd_soc_codec_force_bias_level(BIAS_OFF) on probe, so this will configure > it correctly right from the start. That force on probe sounds like a problem... if this is being done once at startup it should be done in the probe function, not in the bias level configuration. Also, are you sure this is a good fix? If the bias voltage is being configured all the time does that perhaps indicate that for better performance or something it should have been being set to some other voltage when the device is in standby?
Hi, On 21-02-18 12:43, Mark Brown wrote: > On Tue, Feb 20, 2018 at 11:15:04PM +0100, Hans de Goede wrote: >> There is no need to set the LDO voltage to 1.2 volt each time we enter >> standby, instead always leave it 1.2 volt on BIAS_OFF. Note we do a >> snd_soc_codec_force_bias_level(BIAS_OFF) on probe, so this will configure >> it correctly right from the start. > > That force on probe sounds like a problem... if this is being done once > at startup it should be done in the probe function, not in the bias > level configuration. This is more like "we do a snd_soc_codec_force_bias_level(BIAS_OFF) on probe anyways and that already sets the level to 1.2 volt, so we don't need to do this explicitly at probe time". > Also, are you sure this is a good fix? If the bias voltage is being > configured all the time does that perhaps indicate that for better > performance or something it should have been being set to some other > voltage when the device is in standby? We switch the LDO off when in the bias level is BIAS_OFF and on at 1.2 volt when in standby or higher. Before this commit we would unconditionally write 0 to PWR_ANLG1 not only turning off all supplies, but additionally changing the LDO voltage control bits to 00, which requires reprogramming them when we go back to standby. The value of the LDO voltage controls bits when in BIAS_OFF does not matter as the LDO on/off bit is set to off, so the purpose of this commit is to replace the blindly setting of PWR_ANLG1 to 0 with setting all the bits in PWR_ANLG1 to 0, while leaving the 2 LDO voltage-control bits as-is, so that we don't need to reprogram these 2 bits when entering standby. Regards, Hans
On Wed, Feb 21, 2018 at 09:11:34PM +0100, Hans de Goede wrote: > On 21-02-18 12:43, Mark Brown wrote: > > On Tue, Feb 20, 2018 at 11:15:04PM +0100, Hans de Goede wrote: > > > There is no need to set the LDO voltage to 1.2 volt each time we enter > > > standby, instead always leave it 1.2 volt on BIAS_OFF. Note we do a > > > snd_soc_codec_force_bias_level(BIAS_OFF) on probe, so this will configure > > > it correctly right from the start. > > That force on probe sounds like a problem... if this is being done once > > at startup it should be done in the probe function, not in the bias > > level configuration. > This is more like "we do a snd_soc_codec_force_bias_level(BIAS_OFF) > on probe anyways and that already sets the level to 1.2 volt, so > we don't need to do this explicitly at probe time". What I'm saying is that if the code is written such that you need to do that _force_bias_level() then it's not very idiomatic which is a bit worrying - it might be safer to reorganize the code so that this isn't required any more. > > Also, are you sure this is a good fix? If the bias voltage is being > > configured all the time does that perhaps indicate that for better > > performance or something it should have been being set to some other > > voltage when the device is in standby? > We switch the LDO off when in the bias level is BIAS_OFF and on > at 1.2 volt when in standby or higher. Before this commit we would OK.
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index 52fb835ea584..4b0509f7e001 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -1513,8 +1513,6 @@ static int rt5651_set_dai_pll(struct snd_soc_dai *dai, int pll_id, int source, static int rt5651_set_bias_level(struct snd_soc_codec *codec, enum snd_soc_bias_level level) { - struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); - switch (level) { case SND_SOC_BIAS_STANDBY: if (snd_soc_codec_get_bias_level(codec) == SND_SOC_BIAS_OFF) { @@ -1527,9 +1525,6 @@ static int rt5651_set_bias_level(struct snd_soc_codec *codec, snd_soc_update_bits(codec, RT5651_PWR_ANLG1, RT5651_PWR_FV1 | RT5651_PWR_FV2, RT5651_PWR_FV1 | RT5651_PWR_FV2); - snd_soc_update_bits(codec, RT5651_PWR_ANLG1, - RT5651_PWR_LDO_DVO_MASK, - RT5651_PWR_LDO_DVO_1_2V); snd_soc_update_bits(codec, RT5651_D_MISC, 0x1, 0x1); if (snd_soc_read(codec, RT5651_PLL_MODE_1) & 0x9200) snd_soc_update_bits(codec, RT5651_D_MISC, @@ -1543,13 +1538,11 @@ static int rt5651_set_bias_level(struct snd_soc_codec *codec, snd_soc_write(codec, RT5651_PWR_DIG2, 0x0000); snd_soc_write(codec, RT5651_PWR_VOL, 0x0000); snd_soc_write(codec, RT5651_PWR_MIXER, 0x0000); - if (rt5651->pdata.jd_src) { - snd_soc_write(codec, RT5651_PWR_ANLG2, 0x0204); - snd_soc_write(codec, RT5651_PWR_ANLG1, 0x0002); - } else { - snd_soc_write(codec, RT5651_PWR_ANLG1, 0x0000); - snd_soc_write(codec, RT5651_PWR_ANLG2, 0x0000); - } + snd_soc_write(codec, RT5651_PWR_ANLG1, RT5651_PWR_LDO_DVO_1_2V); + /* Leave PLL1 and jack-detect power as is, all others off */ + snd_soc_update_bits(codec, RT5651_PWR_ANLG2, + ~(RT5651_PWR_PLL | RT5651_PWR_JD_M), + 0x0000); break; default:
There is no need to set the LDO voltage to 1.2 volt each time we enter standby, instead always leave it 1.2 volt on BIAS_OFF. Note we do a snd_soc_codec_force_bias_level(BIAS_OFF) on probe, so this will configure it correctly right from the start. For PWR_ANLG2 leave the RT5651_PWR_JD_M and PLL bits as is instead of having different code-paths for when we've jack-detect vs when we don't. Note that this also stops enabling the PLL bit when we've a jd_src and we are running of the RCCLK, this is intentional. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- sound/soc/codecs/rt5651.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)