| Message ID | 20240118165811.13672-2-johan+linaro@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: qcom: volume fixes and codec cleanups | expand |
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 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 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 --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,
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(-)