ASoC: qcom: volume fixes and codec cleanups
Subsystems (4)
v3 cover ASoC: qcom: volume fixes and codec cleanups
To reduce the risk of speaker damage the PA gain needs to be limited on machines like the Lenovo Thinkpad X13s until we have active speaker protection in place.
Limit the gain to the current default setting provided by the UCM configuration which most user have so far been using (due to a bug in the configuration files which prevented hardware volume control [1]).
The wsa883x PA volume control also turned out to be broken, which meant that the default setting used by UCM configuration is actually the lowest level (-3 dB). With the codec driver fixed, hardware volume control also works as expected once we lift the PA volume limit.
Note that the new wsa884x driver most likely suffers from a similar bug, I'll send a fix for that once I've got that confirmed.
Included is also a related fix for the LPASS WSA macro driver, which was changing the digital gain setting behind the back of user space and which can result in excessive (or too low) digital gain.
There are further Qualcomm codec drivers that similarly appear to manipulate various gain settings, but on closer inspection it turns out that they only write back the current settings. Tests reveal that these writes are indeed needed for any prior updates to take effect (at least for the WSA and RX macros).
Johan
[1] https://github.com/alsa-project/alsa-ucm-conf/pull/382
Changes in v3
- fix the wsa883x PA volume control and update the machine limits accordingly
Changes in v2
- keep the volume register write on power-on in lpass-wsa-macro
- drop the other patches removing volume register writes on DAPM events
- only drop the constant-zero gain offsets in wcd9335
Johan Hovold (5):
ASoC: codecs: wsa883x: fix PA volume control
ASoC: codecs: wsa883x: lower default PA gain
ASoC: qcom: sc8280xp: limit speaker volumes
ASoC: codecs: lpass-wsa-macro: fix compander volume hack
ASoC: codecs: wcd9335: drop unused gain hack remnant
sound/soc/codecs/lpass-wsa-macro.c | 7 ------- sound/soc/codecs/wcd9335.c | 4 ---- sound/soc/codecs/wsa883x.c | 6 +++--- sound/soc/qcom/sc8280xp.c | 8 +++++--- 4 files changed, 8 insertions(+), 17 deletions(-)
Signature · 1 line
2.41.0
PATCH v3 1/5 ASoC: codecs: wsa883x: fix PA volume control
The PA gain can be set in steps of 1.5 dB from -3 dB to 18 dB, that is, in fifteen levels.
Fix the range of the PA volume control to avoid having the first sixteen levels all map to -3 dB.
Note that level 0 (-3 dB) does not mute the PA so the mute flag should also not be set.
--- sound/soc/codecs/wsa883x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
sound/soc/codecs/wsa883x.c +2 -2
@@ -1098,7 +1098,7 @@ static int wsa_dev_mode_put(struct snd_kcontrol *kcontrol,return 1 ; } -static const DECLARE_TLV_DB_SCALE(pa_gain, -300, 150, -300); +static const DECLARE_TLV_DB_SCALE(pa_gain, -300, 150, 0);static int wsa883x_get_swr_port (struct snd_kcontrol *kcontrol,struct snd_ctl_elem_value *ucontrol)
@@ -1239,7 +1239,7 @@ static const struct snd_soc_dapm_widget wsa883x_dapm_widgets[] = {static const struct snd_kcontrol_new wsa883x_snd_controls[] = {SOC_SINGLE_RANGE_TLV ("PA Volume" ,WSA883X_DRE_CTL_1 ,1 , -0x0 ,0x1f ,1 , pa_gain), +0x1 ,0xf ,1 , pa_gain),SOC_ENUM_EXT ("WSA MODE" , wsa_dev_mode_enum, wsa_dev_mode_get, wsa_dev_mode_put), SOC_SINGLE_EXT("COMP Offset", SND_SOC_NOPM, 0, 4, 0,
Signature · 1 line
2.41.0
Mark Brown
maintainerRe: [PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control
Quoted 11 lines · Johan Hovold
The PA gain can be set in steps of 1.5 dB from -3 dB to 18 dB, that is, in fifteen levels.
Fix the range of the PA volume control to avoid having the first sixteen levels all map to -3 dB.
Note that level 0 (-3 dB) does not mute the PA so the mute flag should also not be set.
Fixes cdb09e623143 ("ASoC: codecs: wsa883x: add control, dapm widgets and map")Cc [email protected] # 6.0
This will mean that any configuration saved with alsactl store will change effect, it might be better to just fix the TLV description and live with the unfortunate UX...
Re: [PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control
Quoted 10 lines · Mark Brown
Quoted 9 lines · Johan Hovold
The PA gain can be set in steps of 1.5 dB from -3 dB to 18 dB, that is, in fifteen levels.
Fix the range of the PA volume control to avoid having the first sixteen levels all map to -3 dB.
Note that level 0 (-3 dB) does not mute the PA so the mute flag should also not be set.
Quoted 6 lines
Quoted 2 lines
Fixes cdb09e623143 ("ASoC: codecs: wsa883x: add control, dapm widgets and map")Cc [email protected] # 6.0This will mean that any configuration saved with alsactl store will change effect, it might be better to just fix the TLV description and live with the unfortunate UX...
Indeed, but the machine limit set by this series will make that less of any issue. At least for mainline, all users of this codec use the same machine driver so will also be limited to -3 dB.
Johan
Re: [PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control
In reply to Johan HovoldQuoted 5 lines · Johan Hovold
The PA gain can be set in steps of 1.5 dB from -3 dB to 18 dB, that is, in fifteen levels.
Fix the range of the PA volume control to avoid having the first sixteen levels all map to -3 dB.
TBH, we really don't know what unsupported values map to w.r.t dB.
Quoted 26 lines
Note that level 0 (-3 dB) does not mute the PA so the mute flag should also not be set.
Fixes cdb09e623143 ("ASoC: codecs: wsa883x: add control, dapm widgets and map")Cc [email protected] # 6.0Cc Srinivas Kandagatla <[email protected]>Signed-off-by Johan Hovold <[email protected]>--- sound/soc/codecs/wsa883x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)sound/soc/codecs/wsa883x.c +1
-1@@ -1098,7 +1098,7 @@ static int wsa_dev_mode_put(struct snd_kcontrol *kcontrol,return 1 ; } -static const DECLARE_TLV_DB_SCALE(pa_gain, -300, 150, -300); +static const DECLARE_TLV_DB_SCALE(pa_gain, -300, 150, 0);static int wsa883x_get_swr_port (struct snd_kcontrol *kcontrol,struct snd_ctl_elem_value *ucontrol)
sound/soc/codecs/wsa883x.c static const struct snd_soc_dapm_widget wsa883x_dapm_widgets[] = { · line 1242
Quoted 5 lines
sound/soc/codecs/wsa883x.c +1
-1@@ -1239,7 +1239,7 @@ static const struct snd_soc_dapm_widget wsa883x_dapm_widgets[] = {static const struct snd_kcontrol_new wsa883x_snd_controls[] = {SOC_SINGLE_RANGE_TLV ("PA Volume" ,WSA883X_DRE_CTL_1 ,1 , -0x0 ,0x1f ,1 , pa_gain), +0x1 ,0xf ,1 , pa_gain),
gain field in register is Bit[5:1], so the max value of 0x1f is correct here. However the range of gains that it can actually support is only 0-15.
If we are artificially setting the max value of 0xf here, then somewhere we should ensure that Bit[5] is set to zero while programming the gain.
Whatever the mixer control is exposing is clearly reflecting what hardware is supporting.
--srini
Quoted 3 lines
sound/soc/codecs/wsa883x.c +0
-0SOC_ENUM_EXT ("WSA MODE" , wsa_dev_mode_enum, wsa_dev_mode_get, wsa_dev_mode_put), SOC_SINGLE_EXT("COMP Offset", SND_SOC_NOPM, 0, 4, 0,
Re: [PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control
Quoted 8 lines · Srinivas Kandagatla
Quoted 5 lines · Johan Hovold
The PA gain can be set in steps of 1.5 dB from -3 dB to 18 dB, that is, in fifteen levels.
Fix the range of the PA volume control to avoid having the first sixteen levels all map to -3 dB.
TBH, we really don't know what unsupported values map to w.r.t dB.
I've verified experimentally that all values in the range 0..16 map to the same lowest setting, and only at level 17 is there a perceivable difference in gain.
And the datasheet you have access to describes the range as -3 to 18 dB.
Quoted 34 lines
Quoted 25 lines
Note that level 0 (-3 dB) does not mute the PA so the mute flag should also not be set.
Fixes cdb09e623143 ("ASoC: codecs: wsa883x: add control, dapm widgets and map")Cc [email protected] # 6.0Cc Srinivas Kandagatla <[email protected]>Signed-off-by Johan Hovold <[email protected]>--- sound/soc/codecs/wsa883x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)sound/soc/codecs/wsa883x.c +1
-1@@ -1098,7 +1098,7 @@ static int wsa_dev_mode_put(struct snd_kcontrol *kcontrol,return 1 ; } -static const DECLARE_TLV_DB_SCALE(pa_gain, -300, 150, -300); +static const DECLARE_TLV_DB_SCALE(pa_gain, -300, 150, 0);static int wsa883x_get_swr_port (struct snd_kcontrol *kcontrol,struct snd_ctl_elem_value *ucontrol)sound/soc/codecs/wsa883x.c static const struct snd_soc_dapm_widget wsa883x_dapm_widgets[] = { · line 1242
Quoted 5 lines
sound/soc/codecs/wsa883x.c +1
-1@@ -1239,7 +1239,7 @@ static const struct snd_soc_dapm_widget wsa883x_dapm_widgets[] = {static const struct snd_kcontrol_new wsa883x_snd_controls[] = {SOC_SINGLE_RANGE_TLV ("PA Volume" ,WSA883X_DRE_CTL_1 ,1 , -0x0 ,0x1f ,1 , pa_gain), +0x1 ,0xf ,1 , pa_gain),gain field in register is Bit[5:1], so the max value of
0x1fis correct here. However the range of gains that it can actually support is only 0-15.
Quoted 2 lines
If we are artificially setting the max value of
0xfhere, then somewhere we should ensure that Bit[5] is set to zero while programming the gain.
Good point, but the reset value for that bit is 0 so we should be good here.
I'll also update patch 2/5 so that we explicitly set this register on probe in the unlikely event that something else has left that bit set before Linux boots (and the powerdown at probe isn't sufficient).
Quoted 2 lines
Whatever the mixer control is exposing is clearly reflecting what hardware is supporting.
No, not at all. The range exported to user space is all wrong and this breaks volume control in pulseaudio which expects the dB values to reflect the hardware.
If changing the range is a concern (as Mark mentioned), we at least have to fix the dB values.
And if this is something that may differ between the WSA883x variants currently handled by the driver then that needs to be taken into account too. I only have access to wsa8835 (and no docs, unlike you).
Johan
PATCH v3 2/5 ASoC: codecs: wsa883x: lower default PA gain
In reply to Johan HovoldThe default PA gain is set to a pretty high level of 15 dB. Initialise the register to the minimum -3 dB level instead.
This is specifically needed to allow machine drivers to use the lowest level as a volume limit.
--- sound/soc/codecs/wsa883x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/codecs/wsa883x.c +1 -1
@@ -722,7 +722,7 @@ static struct reg_default wsa883x_defaults[] = { { WSA883X_WAVG_PER_6_7, 0x88 }, { WSA883X_WAVG_STA, 0x00 }, { WSA883X_DRE_CTL_0, 0x70 }, - { WSA883X_DRE_CTL_1, 0x08 }, + { WSA883X_DRE_CTL_1, 0x1e }, { WSA883X_DRE_IDLE_DET_CTL, 0x1F }, { WSA883X_CLSH_CTL_0, 0x37 }, { WSA883X_CLSH_CTL_1, 0x81 },
Signature · 1 line
2.41.0
Mark Brown
maintainerRe: [PATCH v3 2/5] ASoC: codecs: wsa883x: lower default PA gain
sound/soc/codecs/wsa883x.c static struct reg_default wsa883x_defaults[] = { · line 725
Quoted 12 lines · Johan Hovold
The default PA gain is set to a pretty high level of 15 dB. Initialise the register to the minimum -3 dB level instead.
This is specifically needed to allow machine drivers to use the lowest level as a volume limit.
sound/soc/codecs/wsa883x.c +1
-1@@ -722,7 +722,7 @@ static struct reg_default wsa883x_defaults[] = { { WSA883X_WAVG_PER_6_7, 0x88 }, { WSA883X_WAVG_STA, 0x00 }, { WSA883X_DRE_CTL_0, 0x70 }, - { WSA883X_DRE_CTL_1, 0x08 }, + { WSA883X_DRE_CTL_1, 0x1e },
This is broken, the register defaults provided to regmap need to correspond to whatever the hardware default is since for example a register cache sync will not write back any default values (as they should already be there in the hardware). Anything like this would need to be done by writes during init.
Re: [PATCH v3 2/5] ASoC: codecs: wsa883x: lower default PA gain
Quoted 7 lines · Mark Brown
Quoted 5 lines · Johan Hovold
The default PA gain is set to a pretty high level of 15 dB. Initialise the register to the minimum -3 dB level instead.
This is specifically needed to allow machine drivers to use the lowest level as a volume limit.
Quoted 12 lines
sound/soc/codecs/wsa883x.c static struct reg_default wsa883x_defaults[] = { · line 725
Quoted 6 lines
sound/soc/codecs/wsa883x.c +1
-1@@ -722,7 +722,7 @@ static struct reg_default wsa883x_defaults[] = { { WSA883X_WAVG_PER_6_7, 0x88 }, { WSA883X_WAVG_STA, 0x00 }, { WSA883X_DRE_CTL_0, 0x70 }, - { WSA883X_DRE_CTL_1, 0x08 }, + { WSA883X_DRE_CTL_1, 0x1e },This is broken, the register defaults provided to regmap need to correspond to whatever the hardware default is since for example a register cache sync will not write back any default values (as they should already be there in the hardware). Anything like this would need to be done by writes during init.
Bah, thanks for catching that. For some reason this was enough to have the driver initialise the register at boot at least. I'll set it explicitly at probe instead.
Johan
Re: [PATCH v3 2/5] ASoC: codecs: wsa883x: lower default PA gain
In reply to Johan HovoldQuoted 17 lines · Johan Hovold
The default PA gain is set to a pretty high level of 15 dB. Initialise the register to the minimum -3 dB level instead.
This is specifically needed to allow machine drivers to use the lowest level as a volume limit.
Cc [email protected] # 6.5Signed-off-by Johan Hovold <[email protected]>--- sound/soc/codecs/wsa883x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)sound/soc/codecs/wsa883x.c +0
-0
sound/soc/codecs/wsa883x.c static struct reg_default wsa883x_defaults[] = { · line 725
Quoted 4 lines
sound/soc/codecs/wsa883x.c +0
-1@@ -722,7 +722,7 @@ static struct reg_default wsa883x_defaults[] = { { WSA883X_WAVG_PER_6_7, 0x88 }, { WSA883X_WAVG_STA, 0x00 }, { WSA883X_DRE_CTL_0, 0x70 }, - { WSA883X_DRE_CTL_1, 0x08 },
this is hw default value.
Quoted 4 lines
sound/soc/codecs/wsa883x.c +1
-0+ { WSA883X_DRE_CTL_1, 0x1e }, { WSA883X_DRE_IDLE_DET_CTL, 0x1F }, { WSA883X_CLSH_CTL_0, 0x37 }, { WSA883X_CLSH_CTL_1, 0x81 },
Re: [PATCH v3 2/5] ASoC: codecs: wsa883x: lower default PA gain
Quoted 15 lines · Srinivas Kandagatla
Quoted 12 lines · Johan Hovold
The default PA gain is set to a pretty high level of 15 dB. Initialise the register to the minimum -3 dB level instead.
This is specifically needed to allow machine drivers to use the lowest level as a volume limit.
Cc [email protected] # 6.5Signed-off-by Johan Hovold <[email protected]>--- sound/soc/codecs/wsa883x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 11 lines
sound/soc/codecs/wsa883x.c static struct reg_default wsa883x_defaults[] = { · line 725
Quoted 9 lines
sound/soc/codecs/wsa883x.c +0
-1@@ -722,7 +722,7 @@ static struct reg_default wsa883x_defaults[] = { { WSA883X_WAVG_PER_6_7, 0x88 }, { WSA883X_WAVG_STA, 0x00 }, { WSA883X_DRE_CTL_0, 0x70 }, - { WSA883X_DRE_CTL_1, 0x08 },this is hw default value.
Indeed. This was a last minute change when I noticed I could actually set the lowest limit in the machine driver after I offset it, but then the reset value was never updated. Didn't think this through.
Quoted 4 lines
sound/soc/codecs/wsa883x.c static struct reg_default wsa883x_defaults[] = { · line 728
Quoted 4 lines
sound/soc/codecs/wsa883x.c +1
-0+ { WSA883X_DRE_CTL_1, 0x1e }, { WSA883X_DRE_IDLE_DET_CTL, 0x1F }, { WSA883X_CLSH_CTL_0, 0x37 }, { WSA883X_CLSH_CTL_1, 0x81 },
Johan
PATCH v3 3/5 ASoC: qcom: sc8280xp: limit speaker volumes
In reply to Johan HovoldThe UCM configuration for the Lenovo ThinkPad X13s has up until now been setting the speaker PA volume to -3 dB when enabling the speakers, but this does not prevent the user from increasing the volume further.
Limit the PA volume to -3 dB in the machine driver to reduce the risk of speaker damage until we have active speaker protection in place.
Note that this will probably need to be generalised using machine-specific limits, but a common limit should do for now.
--- sound/soc/qcom/sc8280xp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
sound/soc/qcom/sc8280xp.c +5 -3
@@ -32,12 +32,14 @@ static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd)case WSA_CODEC_DMA_RX_0 :case WSA_CODEC_DMA_RX_1 :/* -* set limit of 0dB on Digital Volume for Speakers, -* this can prevent damage of speakers to some extent without -* active speaker protection +* Set limit of 0 dB on Digital Volume and -3 dB on PA Volume +* to reduce the risk of speaker damage until we have active +* speaker protection in place. */ snd_soc_limit_volume (card,"WSA_RX0 Digital Volume" ,84 );snd_soc_limit_volume (card,"WSA_RX1 Digital Volume" ,84 ); +snd_soc_limit_volume (card,"SpkrLeft PA Volume" ,1 ); +snd_soc_limit_volume (card,"SpkrRight PA Volume" ,1 );break ;default :break ;
Signature · 1 line
2.41.0
Re: [PATCH v3 3/5] ASoC: qcom: sc8280xp: limit speaker volumes
Quoted 22 lines · Johan Hovold
The UCM configuration for the Lenovo ThinkPad X13s has up until now been setting the speaker PA volume to -3 dB when enabling the speakers, but this does not prevent the user from increasing the volume further.
Limit the PA volume to -3 dB in the machine driver to reduce the risk of speaker damage until we have active speaker protection in place.
Note that this will probably need to be generalised using machine-specific limits, but a common limit should do for now.
Cc [email protected] # 6.5Reviewed-by Srinivas Kandagatla <[email protected]>Signed-off-by Johan Hovold <[email protected]>--- sound/soc/qcom/sc8280xp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)sound/soc/qcom/sc8280xp.c +0
-0
sound/soc/qcom/sc8280xp.c static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd) · line 37
Quoted 9 lines
sound/soc/qcom/sc8280xp.c +3
-3@@ -32,12 +32,14 @@ static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd)case WSA_CODEC_DMA_RX_0 :case WSA_CODEC_DMA_RX_1 :/* -* set limit of 0dB on Digital Volume for Speakers, -* this can prevent damage of speakers to some extent without -* active speaker protection +* Set limit of 0 dB on Digital Volume and -3 dB on PA Volume +* to reduce the risk of speaker damage until we have active +* speaker protection in place.
I would prefer a 0dB here instead of -3dB, this could become issue if we are testing speakers without any pluseaudio or any software amplification. ex: console
sound/soc/qcom/sc8280xp.c static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd)
Quoted 5 lines
sound/soc/qcom/sc8280xp.c +2
-0*/ snd_soc_limit_volume (card,"WSA_RX0 Digital Volume" ,84 );snd_soc_limit_volume (card,"WSA_RX1 Digital Volume" ,84 ); +snd_soc_limit_volume (card,"SpkrLeft PA Volume" ,1 ); +snd_soc_limit_volume (card,"SpkrRight PA Volume" ,1 )
It would be nice to consider using component->name_prefix here.
thanks,
srini
;
Quoted 3 lines
sound/soc/qcom/sc8280xp.c +0
-0break ;default :break ;
Re: [PATCH v3 3/5] ASoC: qcom: sc8280xp: limit speaker volumes
Quoted 30 lines · Srinivas Kandagatla
Quoted 27 lines · Johan Hovold
The UCM configuration for the Lenovo ThinkPad X13s has up until now been setting the speaker PA volume to -3 dB when enabling the speakers, but this does not prevent the user from increasing the volume further.
Limit the PA volume to -3 dB in the machine driver to reduce the risk of speaker damage until we have active speaker protection in place.
Note that this will probably need to be generalised using machine-specific limits, but a common limit should do for now.
Cc [email protected] # 6.5Reviewed-by Srinivas Kandagatla <[email protected]>Signed-off-by Johan Hovold <[email protected]>--- sound/soc/qcom/sc8280xp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)sound/soc/qcom/sc8280xp.c +0
-2@@ -32,12 +32,14 @@ static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd)case WSA_CODEC_DMA_RX_0 :case WSA_CODEC_DMA_RX_1 :/* -* set limit of 0dB on Digital Volume for Speakers, -* this can prevent damage of speakers to some extent without
Quoted 8 lines
sound/soc/qcom/sc8280xp.c static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd) · line 37
Quoted 4 lines
sound/soc/qcom/sc8280xp.c +3
-1-* active speaker protection +* Set limit of 0 dB on Digital Volume and -3 dB on PA Volume +* to reduce the risk of speaker damage until we have active +* speaker protection in place. I would prefer a 0dB here instead of -3dB, this could become issue if we are testing speakers without any pluseaudio or any software amplification. ex: console
I know you want that, but I'm not willing to be the one raising the default volume that people have been using so far and that you have (unknowingly) used in your tests to verify that you did not break your speakers.
Once you've run some more tests we can easily raise this limit.
I just want to make sure we have something safe in place ASAP now that people will soon be able to change the hardware volume control more easily (i.e. with the fixed UCM files).
Quoted 7 lines
sound/soc/qcom/sc8280xp.c static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd)
Quoted 5 lines
sound/soc/qcom/sc8280xp.c +2
-0*/ snd_soc_limit_volume (card,"WSA_RX0 Digital Volume" ,84 );snd_soc_limit_volume (card,"WSA_RX1 Digital Volume" ,84 ); +snd_soc_limit_volume (card,"SpkrLeft PA Volume" ,1 ); +snd_soc_limit_volume (card,"SpkrRight PA Volume" ,1 )It would be nice to consider using
component->name_prefixhere.
That can possibly be done later.
Johan
PATCH v3 4/5 ASoC: codecs: lpass-wsa-macro: fix compander volume hack
In reply to Johan HovoldThe LPASS WSA macro codec driver is updating the digital gain settings behind the back of user space on DAPM events if companding has been enabled.
As compander control is exported to user space, this can result in the digital gain setting being incremented (or decremented) every time the sound server is started and the codec suspended depending on what the UCM configuration looks like.
Soon enough playback will become distorted (or too quiet).
This is specifically a problem on the Lenovo ThinkPad X13s as this bypasses the limit for the digital gain setting that has been set by the machine driver.
Fix this by simply dropping the compander gain offset hack. If someone cares about modelling the impact of the compander setting this can possibly be done by exporting it as a volume control later.
Note that the volume registers still need to be written after enabling clocks in order for any prior updates to take effect.
--- sound/soc/codecs/lpass-wsa-macro.c | 7 ------- 1 file changed, 7 deletions(-)
sound/soc/codecs/lpass-wsa-macro.c +0 -7
@@ -1584,7 +1584,6 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w,u16 gain_reg;u16 reg;int val; -int offset_val =0 ;struct wsa_macro *wsa =snd_soc_component_get_drvdata (component);if (w->shift ==WSA_MACRO_COMP1 ) {
@@ -1623,10 +1622,8 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w,CDC_WSA_RX1_RX_PATH_MIX_SEC0 ,CDC_WSA_RX_PGA_HALF_DB_MASK , CDC_WSA_RX_PGA_HALF_DB_ENABLE); - offset_val =-2 ; } val =snd_soc_component_read (component, gain_reg); - val += offset_val;snd_soc_component_write (component, gain_reg, val);wsa_macro_config_ear_spkr_gain (component, wsa, event, gain_reg);
@@ -1654,10 +1651,6 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w,CDC_WSA_RX1_RX_PATH_MIX_SEC0 ,CDC_WSA_RX_PGA_HALF_DB_MASK , CDC_WSA_RX_PGA_HALF_DB_DISABLE); - offset_val =2 ; - val =snd_soc_component_read (component, gain_reg); - val += offset_val; -snd_soc_component_write (component, gain_reg, val); }wsa_macro_config_ear_spkr_gain (component, wsa, event, gain_reg);
Signature · 1 line
2.41.0
Re: [PATCH v3 4/5] ASoC: codecs: lpass-wsa-macro: fix compander volume hack
Quoted 35 lines · Johan Hovold
The LPASS WSA macro codec driver is updating the digital gain settings behind the back of user space on DAPM events if companding has been enabled.
As compander control is exported to user space, this can result in the digital gain setting being incremented (or decremented) every time the sound server is started and the codec suspended depending on what the UCM configuration looks like.
Soon enough playback will become distorted (or too quiet).
This is specifically a problem on the Lenovo ThinkPad X13s as this bypasses the limit for the digital gain setting that has been set by the machine driver.
Fix this by simply dropping the compander gain offset hack. If someone cares about modelling the impact of the compander setting this can possibly be done by exporting it as a volume control later.
Note that the volume registers still need to be written after enabling clocks in order for any prior updates to take effect.
Fixes 2c4066e5d428 ("ASoC: codecs: lpass-wsa-macro: add dapm widgets and route")Cc [email protected] # 5.11Cc Srinivas Kandagatla <[email protected]>Signed-off-by Johan Hovold <[email protected]>--- sound/soc/codecs/lpass-wsa-macro.c | 7 ------- 1 file changed, 7 deletions(-)sound/soc/codecs/lpass-wsa-macro.c +0
-0
sound/soc/codecs/lpass-wsa-macro.c static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w, · line 1587
Quoted 4 lines
sound/soc/codecs/lpass-wsa-macro.c +0
-1@@ -1584,7 +1584,6 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w,u16 gain_reg;u16 reg;int val; -int offset_val =0 ;
TBH, as discussed in my previous review we should just remove spkr_gain_offset and associated code path.
--srini
Quoted 25 lines
sound/soc/codecs/lpass-wsa-macro.c +0
-6struct wsa_macro *wsa =snd_soc_component_get_drvdata (component);if (w->shift ==WSA_MACRO_COMP1 ) {@@ -1623,10 +1622,8 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w,CDC_WSA_RX1_RX_PATH_MIX_SEC0 ,CDC_WSA_RX_PGA_HALF_DB_MASK , CDC_WSA_RX_PGA_HALF_DB_ENABLE); - offset_val =-2 ; } val =snd_soc_component_read (component, gain_reg); - val += offset_val;snd_soc_component_write (component, gain_reg, val);wsa_macro_config_ear_spkr_gain (component, wsa, event, gain_reg);@@ -1654,10 +1651,6 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w,CDC_WSA_RX1_RX_PATH_MIX_SEC0 ,CDC_WSA_RX_PGA_HALF_DB_MASK , CDC_WSA_RX_PGA_HALF_DB_DISABLE); - offset_val =2 ; - val =snd_soc_component_read (component, gain_reg); - val += offset_val; -snd_soc_component_write (component, gain_reg, val); }wsa_macro_config_ear_spkr_gain (component, wsa, event, gain_reg);
Re: [PATCH v3 4/5] ASoC: codecs: lpass-wsa-macro: fix compander volume hack
Quoted 31 lines · Srinivas Kandagatla
Quoted 30 lines · Johan Hovold
The LPASS WSA macro codec driver is updating the digital gain settings behind the back of user space on DAPM events if companding has been enabled.
As compander control is exported to user space, this can result in the digital gain setting being incremented (or decremented) every time the sound server is started and the codec suspended depending on what the UCM configuration looks like.
Soon enough playback will become distorted (or too quiet).
This is specifically a problem on the Lenovo ThinkPad X13s as this bypasses the limit for the digital gain setting that has been set by the machine driver.
Fix this by simply dropping the compander gain offset hack. If someone cares about modelling the impact of the compander setting this can possibly be done by exporting it as a volume control later.
Note that the volume registers still need to be written after enabling clocks in order for any prior updates to take effect.
Fixes 2c4066e5d428 ("ASoC: codecs: lpass-wsa-macro: add dapm widgets and route")Cc [email protected] # 5.11Cc Srinivas Kandagatla <[email protected]>Signed-off-by Johan Hovold <[email protected]>--- sound/soc/codecs/lpass-wsa-macro.c | 7 ------- 1 file changed, 7 deletions(-)
Quoted 12 lines
sound/soc/codecs/lpass-wsa-macro.c static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w, · line 1587
Quoted 9 lines
sound/soc/codecs/lpass-wsa-macro.c +0
-1@@ -1584,7 +1584,6 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w,u16 gain_reg;u16 reg;int val; -int offset_val =0 ;TBH, as discussed in my previous review we should just remove
spkr_gain_offsetand associated code path.
I don't understand what you are referring to. Are you talking about the "ear_spkr_gain" perhaps?
I left that hack in place for now, as it's not currently an issue. It could perhaps be removed later.
Johan
PATCH v3 5/5 ASoC: codecs: wcd9335: drop unused gain hack remnant
In reply to Johan HovoldThe vendor driver appears to be modifying the gain settings behind the back of user space but these hacks never made it upstream except for some essentially dead code that adds a constant zero to the current gain setting on DAPM events.
Note that the volume registers still need to be written after enabling clocks in order for any prior updates to take effect.
--- sound/soc/codecs/wcd9335.c | 4 ---- 1 file changed, 4 deletions(-)
sound/soc/codecs/wcd9335.c +0 -4
@@ -3033,7 +3033,6 @@ static int wcd9335_codec_enable_mix_path(struct snd_soc_dapm_widget *w, {struct snd_soc_component *comp =snd_soc_dapm_to_component (w->dapm);u16 gain_reg; -int offset_val =0 ;int val =0 ;switch (w->reg) {
@@ -3073,7 +3072,6 @@ static int wcd9335_codec_enable_mix_path(struct snd_soc_dapm_widget *w,switch (event) {case SND_SOC_DAPM_POST_PMU : val =snd_soc_component_read (comp, gain_reg); - val += offset_val;snd_soc_component_write (comp, gain_reg, val);break ;case SND_SOC_DAPM_POST_PMD :
@@ -3294,7 +3292,6 @@ static int wcd9335_codec_enable_interpolator(struct snd_soc_dapm_widget *w,u16 gain_reg;u16 reg;int val; -int offset_val =0 ;if (!(snd_soc_dapm_widget_name_cmp (w,"RX INT0 INTERP" ))) { reg =WCD9335_CDC_RX0_RX_PATH_CTL ;
@@ -3337,7 +3334,6 @@ static int wcd9335_codec_enable_interpolator(struct snd_soc_dapm_widget *w,case SND_SOC_DAPM_POST_PMU :wcd9335_config_compander (comp, w->shift, event); val =snd_soc_component_read (comp, gain_reg); - val += offset_val;snd_soc_component_write (comp, gain_reg, val);break ;case SND_SOC_DAPM_POST_PMD :
Signature · 1 line
2.41.0
Re: [PATCH v3 5/5] ASoC: codecs: wcd9335: drop unused gain hack remnant
Quoted 10 lines · Johan Hovold
The vendor driver appears to be modifying the gain settings behind the back of user space but these hacks never made it upstream except for some essentially dead code that adds a constant zero to the current gain setting on DAPM events.
Note that the volume registers still need to be written after enabling clocks in order for any prior updates to take effect.
Signed-off-by Johan Hovold <[email protected]>---
lgtm,
--srini
Quoted 39 lines
sound/soc/codecs/wcd9335.c | 4 ---- 1 file changed, 4 deletions(-)sound/soc/codecs/wcd9335.c +0
-4@@ -3033,7 +3033,6 @@ static int wcd9335_codec_enable_mix_path(struct snd_soc_dapm_widget *w, {struct snd_soc_component *comp =snd_soc_dapm_to_component (w->dapm);u16 gain_reg; -int offset_val =0 ;int val =0 ;switch (w->reg) {@@ -3073,7 +3072,6 @@ static int wcd9335_codec_enable_mix_path(struct snd_soc_dapm_widget *w,switch (event) {case SND_SOC_DAPM_POST_PMU : val =snd_soc_component_read (comp, gain_reg); - val += offset_val;snd_soc_component_write (comp, gain_reg, val);break ;case SND_SOC_DAPM_POST_PMD :@@ -3294,7 +3292,6 @@ static int wcd9335_codec_enable_interpolator(struct snd_soc_dapm_widget *w,u16 gain_reg;u16 reg;int val; -int offset_val =0 ;if (!(snd_soc_dapm_widget_name_cmp (w,"RX INT0 INTERP" ))) { reg =WCD9335_CDC_RX0_RX_PATH_CTL ;@@ -3337,7 +3334,6 @@ static int wcd9335_codec_enable_interpolator(struct snd_soc_dapm_widget *w,case SND_SOC_DAPM_POST_PMU :wcd9335_config_compander (comp, w->shift, event); val =snd_soc_component_read (comp, gain_reg); - val += offset_val;snd_soc_component_write (comp, gain_reg, val);break ;case SND_SOC_DAPM_POST_PMD :