Re: Semantics for _SX controls

[Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>]

On 01/06/2022 16:24, Mark Brown wrote:
> Hi,
>
> The fixes for input validation have highlighed a bunch of
> problems with the _SX controls.  These aren't widely used, it's
> just some Cirrus Logic and Qualcomm CODECs plus the obsolte
> ab8500 driver, but it has become apparent that at least some of
> the drivers were relying on the lack of validation we used to
> have to work.
>
> For the Cirrus case 34198710f55b5f ("ASoC: Add info callback for
> SX_TLV controls") says that the intended semantic is
>
>      Currently every CODEC implementing these controls specifies the minimum
>      as the non-zero value for the minimum and the maximum as the number of
>      gain settings available.
>
> which was from Charles at Cirrus so hopefully that's the semantic
> used by Cirrus drivers.  However Tan Nayir pointed at some out of
> tree patches from Qualcomm which change the info callback such
> that max is instead the maximum register value that can be
> written and reports that since we started validating written
> values properly they can no longer set some valid values in some
> controls on at least some Qualcomm devices (this was a wcd9340)
> which clearly indicates that there's some problems that need
> fixing.
>
> This isn't helped by the fact that snd_soc_info_volsw_sx() is
> implemented by calling snd_soc_info_volsw() and then applying an
> offset to the maximum value which makes things harder to follow -
> I think we should make snd_soc_info_volsw_sx() a standalone
> function, and we probably need some fixes in some combination of
> the shared functions and the drivers as well.
>
> Can people please check how the values in the current drivers
> correspond to what the actual bitfields in the registers have so
> we can get a handle on what's going on here?  I've pasted a quick
> grep for _SX controls below:

From Qualcomm codecs side of it usage of SOC_SINGLE_SX_TLV should be 
moved to SOC_SINGLE_S8_TLV() in sound/soc/codecs/msm8916-wcd-digital.c 
and sound/soc/codecs/wcd9335.c. using SOC_SINGLE_SX_TLV() will result in 
incorrect volume settings.

All of these gains have signed 7th bit in 8 bit register with gain range 
from -84dB to +40dB in 1dB steps.

Other WCD Codecs seems to have done correctly using S8_TLV.

I can send a patch to fix these.

Thanks,
Srini


> sound/soc/codecs/ab8500-codec.c:	SOC_SINGLE_XR_SX("ANC Warp Delay Shift",
> sound/soc/codecs/ab8500-codec.c:	SOC_SINGLE_XR_SX("ANC FIR Output Shift",
> sound/soc/codecs/ab8500-codec.c:	SOC_SINGLE_XR_SX("ANC IIR Output Shift",
> sound/soc/codecs/ab8500-codec.c:	SOC_SINGLE_XR_SX("ANC Warp Delay",
> sound/soc/codecs/cs35l33.c:	SOC_SINGLE_SX_TLV("DAC Volume", CS35L33_DIG_VOL_CTL,
> sound/soc/codecs/cs35l34.c:	SOC_SINGLE_SX_TLV("Digital Volume", CS35L34_AMP_DIG_VOL,
> sound/soc/codecs/cs35l35.c:	SOC_SINGLE_SX_TLV("Digital Audio Volume", CS35L35_AMP_DIG_VOL,
> sound/soc/codecs/cs35l35.c:	SOC_SINGLE_SX_TLV("Digital Advisory Volume", CS35L35_ADV_DIG_VOL,
> sound/soc/codecs/cs35l36.c:	SOC_SINGLE_SX_TLV("Digital PCM Volume", CS35L36_AMP_DIG_VOL_CTRL,
> sound/soc/codecs/cs35l41.c:	SOC_SINGLE_SX_TLV("Digital PCM Volume", CS35L41_AMP_DIG_VOL_CTRL,
> sound/soc/codecs/cs4265.c:	SOC_DOUBLE_R_SX_TLV("PGA Volume", CS4265_CHA_PGA_CTL,
> sound/soc/codecs/cs42l51.c:	SOC_DOUBLE_R_SX_TLV("PCM Playback Volume",
> sound/soc/codecs/cs42l51.c:	SOC_DOUBLE_R_SX_TLV("Analog Playback Volume",
> sound/soc/codecs/cs42l51.c:	SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume",
> sound/soc/codecs/cs42l51.c:	SOC_DOUBLE_R_SX_TLV("ADC Attenuator Volume",
> sound/soc/codecs/cs42l51.c:	SOC_DOUBLE_R_SX_TLV("PGA Volume",
> sound/soc/codecs/cs42l52.c:	SOC_DOUBLE_R_SX_TLV("Master Volume", CS42L52_MASTERA_VOL,
> sound/soc/codecs/cs42l52.c:	SOC_DOUBLE_R_SX_TLV("Headphone Volume", CS42L52_HPA_VOL,
> sound/soc/codecs/cs42l52.c:	SOC_DOUBLE_R_SX_TLV("Speaker Volume", CS42L52_SPKA_VOL,
> sound/soc/codecs/cs42l52.c:	SOC_DOUBLE_R_SX_TLV("Bypass Volume", CS42L52_PASSTHRUA_VOL,
> sound/soc/codecs/cs42l52.c:	SOC_DOUBLE_R_SX_TLV("ADC Volume", CS42L52_ADCA_VOL,
> sound/soc/codecs/cs42l52.c:	SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume",
> sound/soc/codecs/cs42l52.c:	SOC_DOUBLE_R_SX_TLV("PGA Volume", CS42L52_PGAA_CTL,
> sound/soc/codecs/cs42l52.c:	SOC_DOUBLE_R_SX_TLV("PCM Mixer Volume",
> sound/soc/codecs/cs42l52.c:	SOC_SINGLE_SX_TLV("Beep Volume", CS42L52_BEEP_VOL,
> sound/soc/codecs/cs42l56.c:	SOC_DOUBLE_R_SX_TLV("Master Volume", CS42L56_MASTER_A_VOLUME,
> sound/soc/codecs/cs42l56.c:	SOC_DOUBLE_R_SX_TLV("ADC Mixer Volume", CS42L56_ADCA_MIX_VOLUME,
> sound/soc/codecs/cs42l56.c:	SOC_DOUBLE_R_SX_TLV("PCM Mixer Volume", CS42L56_PCMA_MIX_VOLUME,
> sound/soc/codecs/cs42l56.c:	SOC_DOUBLE_R_SX_TLV("PGA Volume", CS42L56_PGAA_MUX_VOLUME,
> sound/soc/codecs/cs42l56.c:	SOC_DOUBLE_R_SX_TLV("Headphone Volume", CS42L56_HPA_VOLUME,
> sound/soc/codecs/cs42l56.c:	SOC_DOUBLE_R_SX_TLV("LineOut Volume", CS42L56_LOA_VOLUME,
> sound/soc/codecs/cs42l56.c:	SOC_SINGLE_SX_TLV("Beep Volume", CS42L56_BEEP_FREQ_OFFTIME,
> sound/soc/codecs/cs42l73.c:	SOC_DOUBLE_R_SX_TLV("Headphone Analog Playback Volume",
> sound/soc/codecs/cs42l73.c:	SOC_DOUBLE_R_SX_TLV("LineOut Analog Playback Volume", CS42L73_LOAAVOL,
> sound/soc/codecs/cs42l73.c:	SOC_DOUBLE_R_SX_TLV("Input PGA Analog Volume", CS42L73_MICAPREPGAAVOL,
> sound/soc/codecs/cs42l73.c:	SOC_DOUBLE_R_SX_TLV("Input Path Digital Volume", CS42L73_IPADVOL,
> sound/soc/codecs/cs42l73.c:	SOC_DOUBLE_R_SX_TLV("HL Digital Playback Volume",
> sound/soc/codecs/cs42l73.c:	SOC_SINGLE_SX_TLV("Speakerphone Digital Volume",
> sound/soc/codecs/cs42l73.c:	SOC_SINGLE_SX_TLV("Ear Speaker Digital Volume",
> sound/soc/codecs/cs53l30.c:	SOC_SINGLE_SX_TLV("ADC1A PGA Volume",
> sound/soc/codecs/cs53l30.c:	SOC_SINGLE_SX_TLV("ADC1B PGA Volume",
> sound/soc/codecs/cs53l30.c:	SOC_SINGLE_SX_TLV("ADC2A PGA Volume",
> sound/soc/codecs/cs53l30.c:	SOC_SINGLE_SX_TLV("ADC2B PGA Volume",
> sound/soc/codecs/cs53l30.c:	SOC_SINGLE_SX_TLV("ADC1A Digital Volume",
> sound/soc/codecs/cs53l30.c:	SOC_SINGLE_SX_TLV("ADC1B Digital Volume",
> sound/soc/codecs/cs53l30.c:	SOC_SINGLE_SX_TLV("ADC2A Digital Volume",
> sound/soc/codecs/cs53l30.c:	SOC_SINGLE_SX_TLV("ADC2B Digital Volume",
> sound/soc/codecs/msm8916-wcd-digital.c:	SOC_SINGLE_SX_TLV("IIR1 INP1 Volume", LPASS_CDC_IIR1_GAIN_B1_CTL,
> sound/soc/codecs/msm8916-wcd-digital.c:	SOC_SINGLE_SX_TLV("IIR1 INP2 Volume", LPASS_CDC_IIR1_GAIN_B2_CTL,
> sound/soc/codecs/msm8916-wcd-digital.c:	SOC_SINGLE_SX_TLV("IIR1 INP3 Volume", LPASS_CDC_IIR1_GAIN_B3_CTL,
> sound/soc/codecs/msm8916-wcd-digital.c:	SOC_SINGLE_SX_TLV("IIR1 INP4 Volume", LPASS_CDC_IIR1_GAIN_B4_CTL,
> sound/soc/codecs/msm8916-wcd-digital.c:	SOC_SINGLE_SX_TLV("IIR2 INP1 Volume", LPASS_CDC_IIR2_GAIN_B1_CTL,
> sound/soc/codecs/msm8916-wcd-digital.c:	SOC_SINGLE_SX_TLV("IIR2 INP2 Volume", LPASS_CDC_IIR2_GAIN_B2_CTL,
> sound/soc/codecs/msm8916-wcd-digital.c:	SOC_SINGLE_SX_TLV("IIR2 INP3 Volume", LPASS_CDC_IIR2_GAIN_B3_CTL,
> sound/soc/codecs/msm8916-wcd-digital.c:	SOC_SINGLE_SX_TLV("IIR2 INP4 Volume", LPASS_CDC_IIR2_GAIN_B4_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX0 Digital Volume", WCD9335_CDC_RX0_RX_VOL_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX1 Digital Volume", WCD9335_CDC_RX1_RX_VOL_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX2 Digital Volume", WCD9335_CDC_RX2_RX_VOL_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX3 Digital Volume", WCD9335_CDC_RX3_RX_VOL_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX4 Digital Volume", WCD9335_CDC_RX4_RX_VOL_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX5 Digital Volume", WCD9335_CDC_RX5_RX_VOL_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX6 Digital Volume", WCD9335_CDC_RX6_RX_VOL_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX7 Digital Volume", WCD9335_CDC_RX7_RX_VOL_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX8 Digital Volume", WCD9335_CDC_RX8_RX_VOL_CTL,
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX0 Mix Digital Volume",
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX1 Mix Digital Volume",
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX2 Mix Digital Volume",
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX3 Mix Digital Volume",
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX4 Mix Digital Volume",
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX5 Mix Digital Volume",
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX6 Mix Digital Volume",
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX7 Mix Digital Volume",
> sound/soc/codecs/wcd9335.c:	SOC_SINGLE_SX_TLV("RX8 Mix Digital Volume",
>
> Thanks,
> Mark