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(-)
--
2.41.0
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.11
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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);
--
2.41.0
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.5
Signed-off-by: Johan Hovold <[email protected]>
---
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 },
--
2.41.0
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.5
Reviewed-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(-)
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;
--
2.41.0
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]>
---
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:
--
2.41.0
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
Cc: Srinivas Kandagatla <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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,
--
2.41.0
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: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: [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...
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: [email protected] # 6.0
> Cc: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> 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 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: [email protected] # 6.5
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> 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 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: [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...
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 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.5
> Reviewed-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(-)
>
> 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 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: [email protected] # 5.11
> Cc: Srinivas Kandagatla <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> 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 = 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 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: [email protected] # 6.0
> > Cc: Srinivas Kandagatla <[email protected]>
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
> > 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
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 <[email protected]>
> ---
lgtm,
Reviewed-by: Srinivas Kandagatla <[email protected]>
--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;
> 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 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 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: [email protected] # 6.5
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
> > 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
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: [email protected] # 6.5
> > Reviewed-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(-)
> >
> > 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
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: [email protected] # 5.11
> > Cc: Srinivas Kandagatla <[email protected]>
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
> > 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