Semantics for _SX controls
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:
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
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
On Wed, Jun 01, 2022 at 06:14:47PM +0100, Srinivas Kandagatla wrote:
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.
Sounds good, thanks for checking.
On 6/1/22 10:24, Mark Brown wrote:
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.
The Cirrus parts have volume fields with a signed value. Typically this is a large negative attenuation and a smaller amount of gain (-102dB / +12dB for example). The goal for the control is to map this to a continuous range and in a way that understands the signed bit field.
From my review, all of the Cirrus drivers are providing the minimum and number of gain settings to SOC_SINGLE_SX_TLV as Charles described, with the exception of cs53l30. The cs53l30 driver uses the minimum (negative) and maximum (positive) values of the bit field.
At the very least, cs53l30 should be patched. For the other drivers, using a macro that is more explicit about the signed bit field and removes the max/#-steps confusion would be an improvement. Something similar to SOC_DOUBLE_R_S_TLV could work.
Thanks, David
On Wed, Jun 01, 2022 at 02:47:28PM -0500, David Rhodes wrote:
At the very least, cs53l30 should be patched. For the other drivers, using a
OK, great - thanks for checking.
macro that is more explicit about the signed bit field and removes the max/#-steps confusion would be an improvement. Something similar to SOC_DOUBLE_R_S_TLV could work.
I think as a first step it's best to refactor the current info() callback to be clearer then refactor further from there when it's clear what we're doing. I do think explicitly representing the sign bit as you suggest is probably sensible, if you or someone else with hardware could do those changes that'd be safer than me since I don't have any relevant hardware.
participants (3)
-
David Rhodes -
Mark Brown -
Srinivas Kandagatla