diff mbox series

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

Message ID 20240118165811.13672-2-johan+linaro@kernel.org (mailing list archive)
State Superseded
Headers show
Series ASoC: qcom: volume fixes and codec cleanups | expand

Commit Message

Johan Hovold Jan. 18, 2024, 4:58 p.m. UTC
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(-)

Comments

Mark Brown Jan. 18, 2024, 5:24 p.m. UTC | #1
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...
Srinivas Kandagatla Jan. 19, 2024, 7:14 a.m. UTC | #2
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,
Johan Hovold Jan. 19, 2024, 7:34 a.m. UTC | #3
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
Johan Hovold Jan. 19, 2024, 7:49 a.m. UTC | #4
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
diff mbox series

Patch

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,