| Message ID | 20230705125723.40464-1-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c03226ba15fe3c42d13907ec7d8536396602557b |
| Headers | show |
| Series | ASoC: codecs: wcd938x: fix dB range for HPHL and HPHR | expand |
On Wed, 05 Jul 2023 13:57:23 +0100, Srinivas Kandagatla wrote: > dB range for HPHL and HPHR gains are from +6dB to -30dB in steps of > 1.5dB with register values range from 0 to 24. > > Current code maps these dB ranges incorrectly, fix them to allow proper > volume setting. > > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: codecs: wcd938x: fix dB range for HPHL and HPHR commit: c03226ba15fe3c42d13907ec7d8536396602557b All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Wed, Jul 05, 2023 at 01:57:23PM +0100, Srinivas Kandagatla wrote: > dB range for HPHL and HPHR gains are from +6dB to -30dB in steps of > 1.5dB with register values range from 0 to 24. > > Current code maps these dB ranges incorrectly, fix them to allow proper > volume setting. > > Fixes: e8ba1e05bdc0("ASoC: codecs: wcd938x: add basic controls") > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/codecs/wcd938x.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c > index faa15a5ed2c8..3a3360711f8f 100644 > --- a/sound/soc/codecs/wcd938x.c > +++ b/sound/soc/codecs/wcd938x.c > @@ -210,7 +210,7 @@ struct wcd938x_priv { > }; > > static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(ear_pa_gain, 600, -1800); > -static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(line_gain, 600, -3000); > +static const DECLARE_TLV_DB_SCALE(line_gain, -3000, 150, -3000); This looks wrong, and indeed that forth argument appears to be a mute flag. I guess that one should have been 0 (false) here? Headphone output also appears to be way too loud by default with this patch (alone) applied. Perhaps it's just the default mixer settings need to be updated to match? It looks like you're inverting the scale above. Perhaps that's intended, but some more details in the commit message as to what was wrong and what you intended to do would have been good. Johan
On Fri, Jul 07, 2023 at 09:35:44AM +0200, Johan Hovold wrote: > On Wed, Jul 05, 2023 at 01:57:23PM +0100, Srinivas Kandagatla wrote: > > +static const DECLARE_TLV_DB_SCALE(line_gain, -3000, 150, -3000); > This looks wrong, and indeed that forth argument appears to be a mute > flag. I guess that one should have been 0 (false) here? Yes, it is - it's for flagging if the lowest value is mute (many devices integrate mute into a volume control). > Headphone output also appears to be way too loud by default with this > patch (alone) applied. Perhaps it's just the default mixer settings need > to be updated to match? Some of the discussion on IRC suggested that this might be the case.
On 07/07/2023 08:35, Johan Hovold wrote: > On Wed, Jul 05, 2023 at 01:57:23PM +0100, Srinivas Kandagatla wrote: >> dB range for HPHL and HPHR gains are from +6dB to -30dB in steps of >> 1.5dB with register values range from 0 to 24. >> >> Current code maps these dB ranges incorrectly, fix them to allow proper >> volume setting. >> >> Fixes: e8ba1e05bdc0("ASoC: codecs: wcd938x: add basic controls") >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> sound/soc/codecs/wcd938x.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c >> index faa15a5ed2c8..3a3360711f8f 100644 >> --- a/sound/soc/codecs/wcd938x.c >> +++ b/sound/soc/codecs/wcd938x.c >> @@ -210,7 +210,7 @@ struct wcd938x_priv { >> }; >> >> static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(ear_pa_gain, 600, -1800); >> -static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(line_gain, 600, -3000); >> +static const DECLARE_TLV_DB_SCALE(line_gain, -3000, 150, -3000); > > This looks wrong, and indeed that forth argument appears to be a mute > flag. I guess that one should have been 0 (false) here? yes, this should be true instead of a mute dB value. > > Headphone output also appears to be way too loud by default with this > patch (alone) applied. Perhaps it's just the default mixer settings need > to be updated to match? > > It looks like you're inverting the scale above. Perhaps that's intended, yes, the highest value corresponds to lowest dB which is why its inverted. > but some more details in the commit message as to what was wrong and > what you intended to do would have been good. HPHR/HPHL Volume control is broken on this codec. current UCM uses digital volume control for x13s which needs to be moved to Analog volume control. I have this change https://termbin.com/mpp9 in UCM which I plan to send out once I test and fix other paths as well. --srini > > Johan
On Fri, 07 Jul 2023 14:54:31 +0200, Srinivas Kandagatla wrote: > > On 07/07/2023 08:35, Johan Hovold wrote: > > On Wed, Jul 05, 2023 at 01:57:23PM +0100, Srinivas Kandagatla wrote: > >> dB range for HPHL and HPHR gains are from +6dB to -30dB in steps of > >> 1.5dB with register values range from 0 to 24. > >> > >> Current code maps these dB ranges incorrectly, fix them to allow proper > >> volume setting. > >> > >> Fixes: e8ba1e05bdc0("ASoC: codecs: wcd938x: add basic controls") > >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > >> --- > >> sound/soc/codecs/wcd938x.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c > >> index faa15a5ed2c8..3a3360711f8f 100644 > >> --- a/sound/soc/codecs/wcd938x.c > >> +++ b/sound/soc/codecs/wcd938x.c > >> @@ -210,7 +210,7 @@ struct wcd938x_priv { > >> }; > >> static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(ear_pa_gain, 600, > >> -1800); > >> -static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(line_gain, 600, -3000); > >> +static const DECLARE_TLV_DB_SCALE(line_gain, -3000, 150, -3000); > > > > This looks wrong, and indeed that forth argument appears to be a mute > > flag. I guess that one should have been 0 (false) here? > > yes, this should be true instead of a mute dB value. > > > > > Headphone output also appears to be way too loud by default with this > > patch (alone) applied. Perhaps it's just the default mixer settings need > > to be updated to match? > > > > It looks like you're inverting the scale above. Perhaps that's intended, > > yes, the highest value corresponds to lowest dB which is why its inverted. Ouch, that's a bad design choice... Takashi
On Fri, Jul 07, 2023 at 03:20:10PM +0200, Takashi Iwai wrote: > Srinivas Kandagatla wrote: > > yes, the highest value corresponds to lowest dB which is why its inverted. > Ouch, that's a bad design choice... It's moderately common - typically in these cases the control is described in the datasheet as an attenuation control rather than a gain, and this usually corresponds to the physical implementation being only able to make signals smaller relative to the reference.
On Fri, 07 Jul 2023 15:22:45 +0200, Mark Brown wrote: > > On Fri, Jul 07, 2023 at 03:20:10PM +0200, Takashi Iwai wrote: > > Srinivas Kandagatla wrote: > > > > yes, the highest value corresponds to lowest dB which is why its inverted. > > > Ouch, that's a bad design choice... > > It's moderately common - typically in these cases the control is > described in the datasheet as an attenuation control rather than a gain, > and this usually corresponds to the physical implementation being only > able to make signals smaller relative to the reference. Yeah, I see the use case. The problem is, however, that we're using the very same dB info for both gain and attenuation. That means, application has no idea how to interpret those dB values -- to be added or to be subtracted. We should have defined a new TLV type for attenuation to differentiate, and define the TLV macro to give proper min/max. Takashi
On Fri, Jul 07, 2023 at 03:30:48PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > It's moderately common - typically in these cases the control is > > described in the datasheet as an attenuation control rather than a gain, > > and this usually corresponds to the physical implementation being only > > able to make signals smaller relative to the reference. > Yeah, I see the use case. The problem is, however, that we're using > the very same dB info for both gain and attenuation. That means, > application has no idea how to interpret those dB values -- to be > added or to be subtracted. > We should have defined a new TLV type for attenuation to > differentiate, and define the TLV macro to give proper min/max. The ASoC generic control stuff supports inverting the value prior to presentation to userspace so it's masked there (instead of writing the number userspace sees to the register we subtract the number from the maximum value and write that to the register), pulling that up further to the ALSA core might be nice I guess?
On Fri, Jul 07, 2023 at 01:54:31PM +0100, Srinivas Kandagatla wrote: > On 07/07/2023 08:35, Johan Hovold wrote: > > On Wed, Jul 05, 2023 at 01:57:23PM +0100, Srinivas Kandagatla wrote: > >> static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(ear_pa_gain, 600, -1800); > >> -static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(line_gain, 600, -3000); > >> +static const DECLARE_TLV_DB_SCALE(line_gain, -3000, 150, -3000); > > > > This looks wrong, and indeed that forth argument appears to be a mute > > flag. I guess that one should have been 0 (false) here? > > yes, this should be true instead of a mute dB value. Ok, so mute is supported. Then that argument can just be changed to "1" as a cleanup to follow the current convention. > > Headphone output also appears to be way too loud by default with this > > patch (alone) applied. Perhaps it's just the default mixer settings need > > to be updated to match? > > > > It looks like you're inverting the scale above. Perhaps that's intended, > > yes, the highest value corresponds to lowest dB which is why its inverted. Got it, thanks. > > but some more details in the commit message as to what was wrong and > > what you intended to do would have been good. > > HPHR/HPHL Volume control is broken on this codec. > current UCM uses digital volume control for x13s which needs to be moved > to Analog volume control. > I have this change https://termbin.com/mpp9 in UCM which I plan to send > out once I test and fix other paths as well. With those UCM changes the headphone volume appears to be restored even if pavucontrol now sets the "base" marker at 80% rather than 20% volume on the X13s (which is much too loud here). Audio quality seem fine and I'm not hearing any distortion at 20% volume as some people were complaining about (even if I haven't really used the headphones myself before). Sounds like you had a similar fix for the speaker distortion coming soon too, looking forward to that one. Johan
On Fri, 07 Jul 2023 15:35:29 +0200, Mark Brown wrote: > > On Fri, Jul 07, 2023 at 03:30:48PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > It's moderately common - typically in these cases the control is > > > described in the datasheet as an attenuation control rather than a gain, > > > and this usually corresponds to the physical implementation being only > > > able to make signals smaller relative to the reference. > > > Yeah, I see the use case. The problem is, however, that we're using > > the very same dB info for both gain and attenuation. That means, > > application has no idea how to interpret those dB values -- to be > > added or to be subtracted. > > > We should have defined a new TLV type for attenuation to > > differentiate, and define the TLV macro to give proper min/max. > > The ASoC generic control stuff supports inverting the value prior to > presentation to userspace so it's masked there (instead of writing the > number userspace sees to the register we subtract the number from the > maximum value and write that to the register), pulling that up further > to the ALSA core might be nice I guess? I believe yes. Though, I'm still not sure how we can improve the mismatch of dB min/max. The dB values of those inverted controls reflect the result of subtraction, no? Takashi
On Fri, Jul 07, 2023 at 03:47:47PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > The ASoC generic control stuff supports inverting the value prior to > > presentation to userspace so it's masked there (instead of writing the > > number userspace sees to the register we subtract the number from the > > maximum value and write that to the register), pulling that up further > > to the ALSA core might be nice I guess? > I believe yes. Though, I'm still not sure how we can improve the > mismatch of dB min/max. The dB values of those inverted controls > reflect the result of subtraction, no? Yes, the dB scale presented to userspace is reversed relative to the ordering in the registers.
On Fri, 07 Jul 2023 17:06:24 +0200, Mark Brown wrote: > > On Fri, Jul 07, 2023 at 03:47:47PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > The ASoC generic control stuff supports inverting the value prior to > > > presentation to userspace so it's masked there (instead of writing the > > > number userspace sees to the register we subtract the number from the > > > maximum value and write that to the register), pulling that up further > > > to the ALSA core might be nice I guess? > > > I believe yes. Though, I'm still not sure how we can improve the > > mismatch of dB min/max. The dB values of those inverted controls > > reflect the result of subtraction, no? > > Yes, the dB scale presented to userspace is reversed relative to the > ordering in the registers. Right, the TLV min/max corresponds to the control values, and they don't mean the raw register values. BTW, this thread made me wonder whether it makes sense to give some sanity checks (maybe with CONFIG_SND_DEBUG) in ALSA core. e.g. read_tlv_buf() in sound/core/control.c can perform some tests before actually passing to user-space. Takashi
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c index faa15a5ed2c8..3a3360711f8f 100644 --- a/sound/soc/codecs/wcd938x.c +++ b/sound/soc/codecs/wcd938x.c @@ -210,7 +210,7 @@ struct wcd938x_priv { }; static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(ear_pa_gain, 600, -1800); -static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(line_gain, 600, -3000); +static const DECLARE_TLV_DB_SCALE(line_gain, -3000, 150, -3000); static const SNDRV_CTL_TLVD_DECLARE_DB_MINMAX(analog_gain, 0, 3000); struct wcd938x_mbhc_zdet_param { @@ -2655,8 +2655,8 @@ static const struct snd_kcontrol_new wcd938x_snd_controls[] = { wcd938x_get_swr_port, wcd938x_set_swr_port), SOC_SINGLE_EXT("DSD_R Switch", WCD938X_DSD_R, 0, 1, 0, wcd938x_get_swr_port, wcd938x_set_swr_port), - SOC_SINGLE_TLV("HPHL Volume", WCD938X_HPH_L_EN, 0, 0x18, 0, line_gain), - SOC_SINGLE_TLV("HPHR Volume", WCD938X_HPH_R_EN, 0, 0x18, 0, line_gain), + SOC_SINGLE_TLV("HPHL Volume", WCD938X_HPH_L_EN, 0, 0x18, 1, line_gain), + SOC_SINGLE_TLV("HPHR Volume", WCD938X_HPH_R_EN, 0, 0x18, 1, line_gain), WCD938X_EAR_PA_GAIN_TLV("EAR_PA Volume", WCD938X_ANA_EAR_COMPANDER_CTL, 2, 0x10, 0, ear_pa_gain), SOC_SINGLE_EXT("ADC1 Switch", WCD938X_ADC1, 1, 1, 0,
dB range for HPHL and HPHR gains are from +6dB to -30dB in steps of 1.5dB with register values range from 0 to 24. Current code maps these dB ranges incorrectly, fix them to allow proper volume setting. Fixes: e8ba1e05bdc0("ASoC: codecs: wcd938x: add basic controls") Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- sound/soc/codecs/wcd938x.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)