[PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control

[PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control

[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: 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,
-- 
2.41.0

Re: [PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

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...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control

[Srinivas Kandagatla]

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,

Re: [PATCH v3 1/5] ASoC: codecs: wsa883x: fix PA volume control

[Johan Hovold]

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

[PATCH v3 2/5] ASoC: codecs: wsa883x: lower default PA gain

[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: 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 },
-- 
2.41.0

Re: [PATCH v3 2/5] ASoC: codecs: wsa883x: lower default PA gain

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/5] ASoC: codecs: wsa883x: lower default PA gain

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/5] ASoC: codecs: wsa883x: lower default PA gain

[Srinivas Kandagatla]

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 },

Re: [PATCH v3 2/5] ASoC: codecs: wsa883x: lower default PA gain

[Johan Hovold]

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

[PATCH v3 3/5] ASoC: qcom: sc8280xp: limit speaker volumes

[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: 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;
-- 
2.41.0

Re: [PATCH v3 3/5] ASoC: qcom: sc8280xp: limit speaker volumes

[Srinivas Kandagatla]

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;

Re: [PATCH v3 3/5] ASoC: qcom: sc8280xp: limit speaker volumes

[Johan Hovold]

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

[PATCH v3 4/5] ASoC: codecs: lpass-wsa-macro: fix compander volume hack

[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: 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);
-- 
2.41.0

Re: [PATCH v3 4/5] ASoC: codecs: lpass-wsa-macro: fix compander volume hack

[Srinivas Kandagatla]

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 = 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

[Johan Hovold]

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