[PATCH v3 0/5] 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(-)
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: stable@vger.kernel.org # 6.0 Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/codecs/wsa883x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c index cb83c569e18d..32983ca9afba 100644 --- a/sound/soc/codecs/wsa883x.c +++ b/sound/soc/codecs/wsa883x.c @@ -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,
On Thu, Jan 18, 2024 at 05:58:07PM +0100, Johan Hovold wrote:
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: stable@vger.kernel.org # 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...
On Thu, Jan 18, 2024 at 05:24:16PM +0000, Mark Brown wrote:
On Thu, Jan 18, 2024 at 05:58:07PM +0100, Johan Hovold wrote:
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: stable@vger.kernel.org # 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...
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
On 18/01/2024 16:58, Johan Hovold wrote:
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.
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: stable@vger.kernel.org # 6.0 Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
sound/soc/codecs/wsa883x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c index cb83c569e18d..32983ca9afba 100644 --- a/sound/soc/codecs/wsa883x.c +++ b/sound/soc/codecs/wsa883x.c @@ -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),
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
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,
On Fri, Jan 19, 2024 at 07:14:03AM +0000, Srinivas Kandagatla wrote:
On 18/01/2024 16:58, Johan Hovold wrote:
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.
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: stable@vger.kernel.org # 6.0 Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
sound/soc/codecs/wsa883x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c index cb83c569e18d..32983ca9afba 100644 --- a/sound/soc/codecs/wsa883x.c +++ b/sound/soc/codecs/wsa883x.c @@ -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),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.
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).
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
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: stable@vger.kernel.org # 6.5 Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/codecs/wsa883x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c index 32983ca9afba..8942c88dee09 100644 --- a/sound/soc/codecs/wsa883x.c +++ b/sound/soc/codecs/wsa883x.c @@ -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 },
On Thu, Jan 18, 2024 at 05:58:08PM +0100, Johan Hovold wrote:
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.
@@ -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.
On Thu, Jan 18, 2024 at 05:21:48PM +0000, Mark Brown wrote:
On Thu, Jan 18, 2024 at 05:58:08PM +0100, Johan Hovold wrote:
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.
@@ -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
On 18/01/2024 16:58, Johan Hovold wrote:
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: stable@vger.kernel.org # 6.5 Signed-off-by: Johan Hovold johan+linaro@kernel.org
sound/soc/codecs/wsa883x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c index 32983ca9afba..8942c88dee09 100644 --- a/sound/soc/codecs/wsa883x.c +++ b/sound/soc/codecs/wsa883x.c @@ -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.
- { WSA883X_DRE_CTL_1, 0x1e }, { WSA883X_DRE_IDLE_DET_CTL, 0x1F }, { WSA883X_CLSH_CTL_0, 0x37 }, { WSA883X_CLSH_CTL_1, 0x81 },
On Fri, Jan 19, 2024 at 07:15:33AM +0000, Srinivas Kandagatla wrote:
On 18/01/2024 16:58, Johan Hovold wrote:
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: stable@vger.kernel.org # 6.5 Signed-off-by: Johan Hovold johan+linaro@kernel.org
sound/soc/codecs/wsa883x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c index 32983ca9afba..8942c88dee09 100644 --- a/sound/soc/codecs/wsa883x.c +++ b/sound/soc/codecs/wsa883x.c @@ -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.
- { WSA883X_DRE_CTL_1, 0x1e }, { WSA883X_DRE_IDLE_DET_CTL, 0x1F }, { WSA883X_CLSH_CTL_0, 0x37 }, { WSA883X_CLSH_CTL_1, 0x81 },
Johan
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: stable@vger.kernel.org # 6.5 Reviewed-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/qcom/sc8280xp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c index ed4bb551bfbb..a19bfa354af8 100644 --- a/sound/soc/qcom/sc8280xp.c +++ b/sound/soc/qcom/sc8280xp.c @@ -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;
On 18/01/2024 16:58, Johan Hovold wrote:
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: stable@vger.kernel.org # 6.5 Reviewed-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
sound/soc/qcom/sc8280xp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c index ed4bb551bfbb..a19bfa354af8 100644 --- a/sound/soc/qcom/sc8280xp.c +++ b/sound/soc/qcom/sc8280xp.c @@ -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
*/ 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 ;
break;default: break;
On Fri, Jan 19, 2024 at 07:37:14AM +0000, Srinivas Kandagatla wrote:
On 18/01/2024 16:58, Johan Hovold wrote:
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: stable@vger.kernel.org # 6.5 Reviewed-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
sound/soc/qcom/sc8280xp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c index ed4bb551bfbb..a19bfa354af8 100644 --- a/sound/soc/qcom/sc8280xp.c +++ b/sound/soc/qcom/sc8280xp.c @@ -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
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).
*/ 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.
That can possibly be done later.
Johan
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: stable@vger.kernel.org # 5.11 Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/codecs/lpass-wsa-macro.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c index 7e21cec3c2fb..6ce309980cd1 100644 --- a/sound/soc/codecs/lpass-wsa-macro.c +++ b/sound/soc/codecs/lpass-wsa-macro.c @@ -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);
On 18/01/2024 16:58, Johan Hovold wrote:
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: stable@vger.kernel.org # 5.11 Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
sound/soc/codecs/lpass-wsa-macro.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c index 7e21cec3c2fb..6ce309980cd1 100644 --- a/sound/soc/codecs/lpass-wsa-macro.c +++ b/sound/soc/codecs/lpass-wsa-macro.c @@ -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
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 += offset_val;@@ -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);
On Fri, Jan 19, 2024 at 07:45:45AM +0000, Srinivas Kandagatla wrote:
On 18/01/2024 16:58, Johan Hovold wrote:
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: stable@vger.kernel.org # 5.11 Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
sound/soc/codecs/lpass-wsa-macro.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c index 7e21cec3c2fb..6ce309980cd1 100644 --- a/sound/soc/codecs/lpass-wsa-macro.c +++ b/sound/soc/codecs/lpass-wsa-macro.c @@ -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.
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
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 johan+linaro@kernel.org --- sound/soc/codecs/wcd9335.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 43c648efd0d9..deb15b95992d 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -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:
On 18/01/2024 16:58, Johan Hovold wrote:
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 johan+linaro@kernel.org
lgtm,
Reviewed-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
--srini
sound/soc/codecs/wcd9335.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 43c648efd0d9..deb15b95992d 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -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;@@ -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;
participants (4)
-
Johan Hovold -
Johan Hovold -
Mark Brown -
Srinivas Kandagatla