ASoC: codecs: wcd938x: fix dB range for HPHL and HPHR

  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(-)
  

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