[PATCH 0/7] ASoC: qcom: volume fixes and codec cleanups
To reduce the risk of speaker damage the PA gain needs to be limited on machines like the Lenovo Thinkpad X13s until we have active speaker protection in place.
Limit the gain to the current default setting provided by the UCM configuration which most user have so far been been using (due to a bug in the configuration files which prevented hardware volume control [1]).
Included is also a related fix for the LPASS WSA macro driver, which was changing the digital gain setting behind the back of user space and which can result in excessive (or too low) digital gain.
There are further Qualcomm codec driver that appear to manipulate various gain settings, but on closer inspection this turned out to be effectively dead code which can be removed.
Johan
[1] https://github.com/alsa-project/alsa-ucm-conf/pull/382
Johan Hovold (7): ASoC: qcom: sc8280xp: limit speaker volumes ASoC: codecs: lpass-wsa-macro: fix compander volume hack ASoC: codecs: lpass-wsa-macro: drop dead mixer-path gain hack ASoC: codecs: lpass-rx-macro: drop dead mixer-path gain hack ASoC: codecs: wcd9335: drop dead gain hacks ASoC: codecs: wcd934x: drop dead gain hacks ASoC: codecs: msm8916-wcd-digital: drop dead gain hacks
sound/soc/codecs/lpass-rx-macro.c | 16 +--- sound/soc/codecs/lpass-wsa-macro.c | 19 +--- sound/soc/codecs/msm8916-wcd-digital.c | 26 +----- sound/soc/codecs/wcd9335.c | 115 ++++--------------------- sound/soc/codecs/wcd934x.c | 102 +++++----------------- sound/soc/qcom/sc8280xp.c | 8 +- 6 files changed, 53 insertions(+), 233 deletions(-)
The current UCM configuration sets the speaker PA volume to 15 dB when enabling the speakers but this does not prevent the user from increasing the volume further.
Limit the PA volume to 15 dB in the machine driver to reduce the risk of speaker damage until we have active speaker protection in place.
Note that this will probably need to be generalised using machine-specific limits, but a common limit should do for now.
Cc: stable@vger.kernel.org # 6.5 Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/qcom/sc8280xp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c index ed4bb551bfbb..aa43903421f5 100644 --- a/sound/soc/qcom/sc8280xp.c +++ b/sound/soc/qcom/sc8280xp.c @@ -32,12 +32,14 @@ static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd) case WSA_CODEC_DMA_RX_0: case WSA_CODEC_DMA_RX_1: /* - * set limit of 0dB on Digital Volume for Speakers, - * this can prevent damage of speakers to some extent without - * active speaker protection + * Set limit of 0 dB on Digital Volume and 15 dB on PA Volume + * to reduce the risk of speaker damage until we have active + * speaker protection in place. */ snd_soc_limit_volume(card, "WSA_RX0 Digital Volume", 84); snd_soc_limit_volume(card, "WSA_RX1 Digital Volume", 84); + snd_soc_limit_volume(card, "SpkrLeft PA Volume", 12); + snd_soc_limit_volume(card, "SpkrRight PA Volume", 12); break; default: break;
On 16/01/2024 09:38, Johan Hovold wrote:
The current UCM configuration sets the speaker PA volume to 15 dB when enabling the speakers but this does not prevent the user from increasing the volume further.
Limit the PA volume to 15 dB in the machine driver to reduce the risk of speaker damage until we have active speaker protection in place.
Note that this will probably need to be generalised using machine-specific limits, but a common limit should do for now.
Cc: stable@vger.kernel.org # 6.5 Signed-off-by: Johan Hovold johan+linaro@kernel.org
LGTM, We can get rid of this limit once we have Speaker protection inplace.
Reviewed-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
--srini
sound/soc/qcom/sc8280xp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c index ed4bb551bfbb..aa43903421f5 100644 --- a/sound/soc/qcom/sc8280xp.c +++ b/sound/soc/qcom/sc8280xp.c @@ -32,12 +32,14 @@ static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd) case WSA_CODEC_DMA_RX_0: case WSA_CODEC_DMA_RX_1: /*
* set limit of 0dB on Digital Volume for Speakers,* this can prevent damage of speakers to some extent without* active speaker protection
* Set limit of 0 dB on Digital Volume and 15 dB on PA Volume* to reduce the risk of speaker damage until we have active* speaker protection in place.snd_soc_limit_volume(card, "SpkrLeft PA Volume", 12);snd_soc_limit_volume(card, "SpkrRight PA Volume", 12);
On Tue, Jan 16, 2024 at 11:11:47AM +0000, Srinivas Kandagatla wrote:
On 16/01/2024 09:38, Johan Hovold wrote:
The current UCM configuration sets the speaker PA volume to 15 dB when enabling the speakers but this does not prevent the user from increasing the volume further.
Limit the PA volume to 15 dB in the machine driver to reduce the risk of speaker damage until we have active speaker protection in place.
LGTM, We can get rid of this limit once we have Speaker protection inplace.
There should be a userspace component for speaker protection so you'll need to limit things when that's not running.
The LPASS WSA macro codec driver is updating the digital gain settings behind the back of user space on DAPM events if companding has been enabled.
As compander control is exported to user space, this can result in the digital gain setting being incremented (or decremented) every time the sound server is started and the codec suspended depending on what the UCM configuration looks like.
Soon enough playback will become distorted (or too quiet).
This is specifically a problem on the Lenovo ThinkPad X13s as this bypasses the limit for the digital gain setting that has been set by the machine driver.
Fix this by simply dropping the compander gain hack. If someone cares about modelling the impact of the compander setting this can possibly be done by exporting it as a volume control later.
Fixes: 2c4066e5d428 ("ASoC: codecs: lpass-wsa-macro: add dapm widgets and route") Cc: stable@vger.kernel.org # 5.11 Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/codecs/lpass-wsa-macro.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c index 7e21cec3c2fb..7de221464d47 100644 --- a/sound/soc/codecs/lpass-wsa-macro.c +++ b/sound/soc/codecs/lpass-wsa-macro.c @@ -1583,8 +1583,6 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w, struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); u16 gain_reg; u16 reg; - int val; - int offset_val = 0; struct wsa_macro *wsa = snd_soc_component_get_drvdata(component);
if (w->shift == WSA_MACRO_COMP1) { @@ -1623,11 +1621,7 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w, CDC_WSA_RX1_RX_PATH_MIX_SEC0, CDC_WSA_RX_PGA_HALF_DB_MASK, CDC_WSA_RX_PGA_HALF_DB_ENABLE); - offset_val = -2; } - val = snd_soc_component_read(component, gain_reg); - val += offset_val; - snd_soc_component_write(component, gain_reg, val); wsa_macro_config_ear_spkr_gain(component, wsa, event, gain_reg); break; @@ -1654,10 +1648,6 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w, CDC_WSA_RX1_RX_PATH_MIX_SEC0, CDC_WSA_RX_PGA_HALF_DB_MASK, CDC_WSA_RX_PGA_HALF_DB_DISABLE); - offset_val = 2; - val = snd_soc_component_read(component, gain_reg); - val += offset_val; - snd_soc_component_write(component, gain_reg, val); } wsa_macro_config_ear_spkr_gain(component, wsa, event, gain_reg);
Thanks Johan for this patch,
On 16/01/2024 09:38, Johan Hovold wrote:
The LPASS WSA macro codec driver is updating the digital gain settings behind the back of user space on DAPM events if companding has been enabled.
As compander control is exported to user space, this can result in the digital gain setting being incremented (or decremented) every time the sound server is started and the codec suspended depending on what the UCM configuration looks like.
Soon enough playback will become distorted (or too quiet).
This is specifically a problem on the Lenovo ThinkPad X13s as this bypasses the limit for the digital gain setting that has been set by the machine driver.
Fix this by simply dropping the compander gain hack. If someone cares about modelling the impact of the compander setting this can possibly be done by exporting it as a volume control later.
This is not a hack, wsa codec requires gain to be programmed after the clk is enabled for that particular interpolator.
However I agree with you on programming the gain that is different to what user set it.
This is because of unimplemented or half baked implementation of half-db feature of gain control in this codec.
We can clean that half baked implementation for now and still continue to program the gain values after the clks are enabled.
lets remove spkr_gain_offset and associated code paths in this codec, which should fix the issue that you have reported and still do the right thing of programming gain after clks are enabled.
thanks, Srini
Fixes: 2c4066e5d428 ("ASoC: codecs: lpass-wsa-macro: add dapm widgets and route") Cc: stable@vger.kernel.org # 5.11 Cc: Srinivas Kandagatla srinivas.kandagatla@linaro.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
sound/soc/codecs/lpass-wsa-macro.c | 10 ---------- 1 file changed, 10 deletions(-)
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c index 7e21cec3c2fb..7de221464d47 100644 --- a/sound/soc/codecs/lpass-wsa-macro.c +++ b/sound/soc/codecs/lpass-wsa-macro.c @@ -1583,8 +1583,6 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w, struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); u16 gain_reg; u16 reg;
int val;
int offset_val = 0; struct wsa_macro *wsa = snd_soc_component_get_drvdata(component);
if (w->shift == WSA_MACRO_COMP1) {
@@ -1623,11 +1621,7 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w, CDC_WSA_RX1_RX_PATH_MIX_SEC0, CDC_WSA_RX_PGA_HALF_DB_MASK, CDC_WSA_RX_PGA_HALF_DB_ENABLE);
offset_val = -2;val = snd_soc_component_read(component, gain_reg);val += offset_val;snd_soc_component_write(component, gain_reg, val);@@ -1654,10 +1648,6 @@ static int wsa_macro_enable_interpolator(struct snd_soc_dapm_widget *w, CDC_WSA_RX1_RX_PATH_MIX_SEC0, CDC_WSA_RX_PGA_HALF_DB_MASK, CDC_WSA_RX_PGA_HALF_DB_DISABLE);
offset_val = 2;val = snd_soc_component_read(component, gain_reg);val += offset_val;snd_soc_component_write(component, gain_reg, val);
On Tue, Jan 16, 2024 at 11:10:21AM +0000, Srinivas Kandagatla wrote:
Thanks Johan for this patch,
On 16/01/2024 09:38, Johan Hovold wrote:
The LPASS WSA macro codec driver is updating the digital gain settings behind the back of user space on DAPM events if companding has been enabled.
As compander control is exported to user space, this can result in the digital gain setting being incremented (or decremented) every time the sound server is started and the codec suspended depending on what the UCM configuration looks like.
Soon enough playback will become distorted (or too quiet).
This is specifically a problem on the Lenovo ThinkPad X13s as this bypasses the limit for the digital gain setting that has been set by the machine driver.
Fix this by simply dropping the compander gain hack. If someone cares about modelling the impact of the compander setting this can possibly be done by exporting it as a volume control later.
This is not a hack, wsa codec requires gain to be programmed after the clk is enabled for that particular interpolator.
Ok, but then it's also broken as, as I mentioned off-list, these registers are cached so unless companding is enabled, the write on enable will have no effect.
However I agree with you on programming the gain that is different to what user set it.
This is because of unimplemented or half baked implementation of half-db feature of gain control in this codec.
We can clean that half baked implementation for now and still continue to program the gain values after the clks are enabled.
lets remove spkr_gain_offset and associated code paths in this codec, which should fix the issue that you have reported and still do the right thing of programming gain after clks are enabled.
Removing the offset which can alter the gain, will cause both of these writes to become no-ops as the registers are cached (e.g. just as for the follow-on codec cleanups). So then we might as well just remove this too.
How is the half-dB feature supposed to work?
And are you sure that you need to reprogram the gain value after enabling the clock? Everything seems to work without doing so.
Johan
On Tue, Jan 16, 2024 at 02:10:19PM +0100, Johan Hovold wrote:
On Tue, Jan 16, 2024 at 11:10:21AM +0000, Srinivas Kandagatla wrote:
This is not a hack, wsa codec requires gain to be programmed after the clk is enabled for that particular interpolator.
Ok, but then it's also broken as, as I mentioned off-list, these registers are cached so unless companding is enabled, the write on enable will have no effect.
I was obviously confused here, and indeed tests reveal that those writes are needed for any prior updates to take effect (e.g. volume changes while the codec is runtime suspended).
I've just sent a v2 here:
https://lore.kernel.org/lkml/20240117090331.31111-1-johan+linaro@kernel.org/
Johan
The vendor driver modifies the gain settings behind the back of user space but some of these hacks never made it upstream except for some essentially dead code that reads out the (cached) gain setting and writes it back again on DAPM events.
Drop the incomplete and pointless hack when enabling mixer paths.
Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/codecs/lpass-wsa-macro.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c index 7de221464d47..36a8f1980c6e 100644 --- a/sound/soc/codecs/lpass-wsa-macro.c +++ b/sound/soc/codecs/lpass-wsa-macro.c @@ -1220,27 +1220,20 @@ static int wsa_macro_enable_mix_path(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); - u16 path_reg, gain_reg; - int val; + u16 path_reg;
switch (w->shift) { case WSA_MACRO_RX_MIX0: path_reg = CDC_WSA_RX0_RX_PATH_MIX_CTL; - gain_reg = CDC_WSA_RX0_RX_VOL_MIX_CTL; break; case WSA_MACRO_RX_MIX1: path_reg = CDC_WSA_RX1_RX_PATH_MIX_CTL; - gain_reg = CDC_WSA_RX1_RX_VOL_MIX_CTL; break; default: return 0; }
switch (event) { - case SND_SOC_DAPM_POST_PMU: - val = snd_soc_component_read(component, gain_reg); - snd_soc_component_write(component, gain_reg, val); - break; case SND_SOC_DAPM_POST_PMD: snd_soc_component_update_bits(component, path_reg, CDC_WSA_RX_PATH_MIX_CLK_EN_MASK,
The vendor driver modifies the gain settings behind the back of user space but some of these hacks never made it upstream except for some essentially dead code that reads out the (cached) gain setting and writes it back again on DAPM events.
Drop the incomplete and pointless hack when enabling mixer paths.
Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/codecs/lpass-rx-macro.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/sound/soc/codecs/lpass-rx-macro.c b/sound/soc/codecs/lpass-rx-macro.c index f35187d69cac..b1ec41eed851 100644 --- a/sound/soc/codecs/lpass-rx-macro.c +++ b/sound/soc/codecs/lpass-rx-macro.c @@ -1956,10 +1956,9 @@ static int rx_macro_enable_main_path(struct snd_soc_dapm_widget *w, int event) { struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); - u16 gain_reg, reg; + u16 reg;
reg = CDC_RX_RXn_RX_PATH_CTL(w->shift); - gain_reg = CDC_RX_RXn_RX_VOL_CTL(w->shift);
switch (event) { case SND_SOC_DAPM_PRE_PMU: @@ -1969,10 +1968,6 @@ static int rx_macro_enable_main_path(struct snd_soc_dapm_widget *w, CDC_RX_PATH_CLK_EN_MASK, CDC_RX_PATH_CLK_ENABLE); break; - case SND_SOC_DAPM_POST_PMU: - snd_soc_component_write(component, gain_reg, - snd_soc_component_read(component, gain_reg)); - break; case SND_SOC_DAPM_POST_PMD: rx_macro_enable_interp_clk(component, event, w->shift); break; @@ -3031,16 +3026,13 @@ static const struct snd_soc_dapm_widget rx_macro_dapm_widgets[] = {
SND_SOC_DAPM_MUX_E("RX INT0_1 INTERP", SND_SOC_NOPM, INTERP_HPHL, 0, &rx_int0_1_interp_mux, rx_macro_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MUX_E("RX INT1_1 INTERP", SND_SOC_NOPM, INTERP_HPHR, 0, &rx_int1_1_interp_mux, rx_macro_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MUX_E("RX INT2_1 INTERP", SND_SOC_NOPM, INTERP_AUX, 0, &rx_int2_1_interp_mux, rx_macro_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
SND_SOC_DAPM_MUX("RX INT0_2 INTERP", SND_SOC_NOPM, 0, 0, &rx_int0_2_interp_mux),
The vendor driver appears to be modifying the gain settings behind the back of user space but these hacks never made it upstream except for some essentially dead code that reads out the (cached) gain setting and writes it back again on DAPM events.
Drop these incomplete and pointless hacks.
Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/codecs/wcd9335.c | 115 ++++++------------------------------- 1 file changed, 18 insertions(+), 97 deletions(-)
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 43c648efd0d9..cee17b309160 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -3028,61 +3028,6 @@ static int wcd9335_codec_enable_slim(struct snd_soc_dapm_widget *w, return 0; }
-static int wcd9335_codec_enable_mix_path(struct snd_soc_dapm_widget *w, - struct snd_kcontrol *kc, int event) -{ - struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm); - u16 gain_reg; - int offset_val = 0; - int val = 0; - - switch (w->reg) { - case WCD9335_CDC_RX0_RX_PATH_MIX_CTL: - gain_reg = WCD9335_CDC_RX0_RX_VOL_MIX_CTL; - break; - case WCD9335_CDC_RX1_RX_PATH_MIX_CTL: - gain_reg = WCD9335_CDC_RX1_RX_VOL_MIX_CTL; - break; - case WCD9335_CDC_RX2_RX_PATH_MIX_CTL: - gain_reg = WCD9335_CDC_RX2_RX_VOL_MIX_CTL; - break; - case WCD9335_CDC_RX3_RX_PATH_MIX_CTL: - gain_reg = WCD9335_CDC_RX3_RX_VOL_MIX_CTL; - break; - case WCD9335_CDC_RX4_RX_PATH_MIX_CTL: - gain_reg = WCD9335_CDC_RX4_RX_VOL_MIX_CTL; - break; - case WCD9335_CDC_RX5_RX_PATH_MIX_CTL: - gain_reg = WCD9335_CDC_RX5_RX_VOL_MIX_CTL; - break; - case WCD9335_CDC_RX6_RX_PATH_MIX_CTL: - gain_reg = WCD9335_CDC_RX6_RX_VOL_MIX_CTL; - break; - case WCD9335_CDC_RX7_RX_PATH_MIX_CTL: - gain_reg = WCD9335_CDC_RX7_RX_VOL_MIX_CTL; - break; - case WCD9335_CDC_RX8_RX_PATH_MIX_CTL: - gain_reg = WCD9335_CDC_RX8_RX_VOL_MIX_CTL; - break; - default: - dev_err(comp->dev, "%s: No gain register avail for %s\n", - __func__, w->name); - return 0; - } - - switch (event) { - case SND_SOC_DAPM_POST_PMU: - val = snd_soc_component_read(comp, gain_reg); - val += offset_val; - snd_soc_component_write(comp, gain_reg, val); - break; - case SND_SOC_DAPM_POST_PMD: - break; - } - - return 0; -} - static u16 wcd9335_interp_get_primary_reg(u16 reg, u16 *ind) { u16 prim_int_reg = WCD9335_CDC_RX0_RX_PATH_CTL; @@ -3291,38 +3236,26 @@ static int wcd9335_codec_enable_interpolator(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kc, int event) { struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm); - u16 gain_reg; u16 reg; - int val; - int offset_val = 0;
if (!(snd_soc_dapm_widget_name_cmp(w, "RX INT0 INTERP"))) { reg = WCD9335_CDC_RX0_RX_PATH_CTL; - gain_reg = WCD9335_CDC_RX0_RX_VOL_CTL; } else if (!(snd_soc_dapm_widget_name_cmp(w, "RX INT1 INTERP"))) { reg = WCD9335_CDC_RX1_RX_PATH_CTL; - gain_reg = WCD9335_CDC_RX1_RX_VOL_CTL; } else if (!(snd_soc_dapm_widget_name_cmp(w, "RX INT2 INTERP"))) { reg = WCD9335_CDC_RX2_RX_PATH_CTL; - gain_reg = WCD9335_CDC_RX2_RX_VOL_CTL; } else if (!(snd_soc_dapm_widget_name_cmp(w, "RX INT3 INTERP"))) { reg = WCD9335_CDC_RX3_RX_PATH_CTL; - gain_reg = WCD9335_CDC_RX3_RX_VOL_CTL; } else if (!(snd_soc_dapm_widget_name_cmp(w, "RX INT4 INTERP"))) { reg = WCD9335_CDC_RX4_RX_PATH_CTL; - gain_reg = WCD9335_CDC_RX4_RX_VOL_CTL; } else if (!(snd_soc_dapm_widget_name_cmp(w, "RX INT5 INTERP"))) { reg = WCD9335_CDC_RX5_RX_PATH_CTL; - gain_reg = WCD9335_CDC_RX5_RX_VOL_CTL; } else if (!(snd_soc_dapm_widget_name_cmp(w, "RX INT6 INTERP"))) { reg = WCD9335_CDC_RX6_RX_PATH_CTL; - gain_reg = WCD9335_CDC_RX6_RX_VOL_CTL; } else if (!(snd_soc_dapm_widget_name_cmp(w, "RX INT7 INTERP"))) { reg = WCD9335_CDC_RX7_RX_PATH_CTL; - gain_reg = WCD9335_CDC_RX7_RX_VOL_CTL; } else if (!(snd_soc_dapm_widget_name_cmp(w, "RX INT8 INTERP"))) { reg = WCD9335_CDC_RX8_RX_PATH_CTL; - gain_reg = WCD9335_CDC_RX8_RX_VOL_CTL; } else { dev_err(comp->dev, "%s: Interpolator reg not found\n", __func__); @@ -3336,9 +3269,6 @@ static int wcd9335_codec_enable_interpolator(struct snd_soc_dapm_widget *w, break; case SND_SOC_DAPM_POST_PMU: wcd9335_config_compander(comp, w->shift, event); - val = snd_soc_component_read(comp, gain_reg); - val += offset_val; - snd_soc_component_write(comp, gain_reg, val); break; case SND_SOC_DAPM_POST_PMD: wcd9335_config_compander(comp, w->shift, event); @@ -4367,33 +4297,24 @@ static const struct snd_soc_dapm_widget wcd9335_dapm_widgets[] = { SND_SOC_DAPM_MIXER("SLIM RX5", SND_SOC_NOPM, 0, 0, NULL, 0), SND_SOC_DAPM_MIXER("SLIM RX6", SND_SOC_NOPM, 0, 0, NULL, 0), SND_SOC_DAPM_MIXER("SLIM RX7", SND_SOC_NOPM, 0, 0, NULL, 0), - SND_SOC_DAPM_MUX_E("RX INT0_2 MUX", WCD9335_CDC_RX0_RX_PATH_MIX_CTL, - 5, 0, &rx_int0_2_mux, wcd9335_codec_enable_mix_path, - SND_SOC_DAPM_POST_PMU), - SND_SOC_DAPM_MUX_E("RX INT1_2 MUX", WCD9335_CDC_RX1_RX_PATH_MIX_CTL, - 5, 0, &rx_int1_2_mux, wcd9335_codec_enable_mix_path, - SND_SOC_DAPM_POST_PMU), - SND_SOC_DAPM_MUX_E("RX INT2_2 MUX", WCD9335_CDC_RX2_RX_PATH_MIX_CTL, - 5, 0, &rx_int2_2_mux, wcd9335_codec_enable_mix_path, - SND_SOC_DAPM_POST_PMU), - SND_SOC_DAPM_MUX_E("RX INT3_2 MUX", WCD9335_CDC_RX3_RX_PATH_MIX_CTL, - 5, 0, &rx_int3_2_mux, wcd9335_codec_enable_mix_path, - SND_SOC_DAPM_POST_PMU), - SND_SOC_DAPM_MUX_E("RX INT4_2 MUX", WCD9335_CDC_RX4_RX_PATH_MIX_CTL, - 5, 0, &rx_int4_2_mux, wcd9335_codec_enable_mix_path, - SND_SOC_DAPM_POST_PMU), - SND_SOC_DAPM_MUX_E("RX INT5_2 MUX", WCD9335_CDC_RX5_RX_PATH_MIX_CTL, - 5, 0, &rx_int5_2_mux, wcd9335_codec_enable_mix_path, - SND_SOC_DAPM_POST_PMU), - SND_SOC_DAPM_MUX_E("RX INT6_2 MUX", WCD9335_CDC_RX6_RX_PATH_MIX_CTL, - 5, 0, &rx_int6_2_mux, wcd9335_codec_enable_mix_path, - SND_SOC_DAPM_POST_PMU), - SND_SOC_DAPM_MUX_E("RX INT7_2 MUX", WCD9335_CDC_RX7_RX_PATH_MIX_CTL, - 5, 0, &rx_int7_2_mux, wcd9335_codec_enable_mix_path, - SND_SOC_DAPM_POST_PMU), - SND_SOC_DAPM_MUX_E("RX INT8_2 MUX", WCD9335_CDC_RX8_RX_PATH_MIX_CTL, - 5, 0, &rx_int8_2_mux, wcd9335_codec_enable_mix_path, - SND_SOC_DAPM_POST_PMU), + SND_SOC_DAPM_MUX("RX INT0_2 MUX", WCD9335_CDC_RX0_RX_PATH_MIX_CTL, + 5, 0, &rx_int0_2_mux), + SND_SOC_DAPM_MUX("RX INT1_2 MUX", WCD9335_CDC_RX1_RX_PATH_MIX_CTL, + 5, 0, &rx_int1_2_mux), + SND_SOC_DAPM_MUX("RX INT2_2 MUX", WCD9335_CDC_RX2_RX_PATH_MIX_CTL, + 5, 0, &rx_int2_2_mux), + SND_SOC_DAPM_MUX("RX INT3_2 MUX", WCD9335_CDC_RX3_RX_PATH_MIX_CTL, + 5, 0, &rx_int3_2_mux), + SND_SOC_DAPM_MUX("RX INT4_2 MUX", WCD9335_CDC_RX4_RX_PATH_MIX_CTL, + 5, 0, &rx_int4_2_mux), + SND_SOC_DAPM_MUX("RX INT5_2 MUX", WCD9335_CDC_RX5_RX_PATH_MIX_CTL, + 5, 0, &rx_int5_2_mux), + SND_SOC_DAPM_MUX("RX INT6_2 MUX", WCD9335_CDC_RX6_RX_PATH_MIX_CTL, + 5, 0, &rx_int6_2_mux), + SND_SOC_DAPM_MUX("RX INT7_2 MUX", WCD9335_CDC_RX7_RX_PATH_MIX_CTL, + 5, 0, &rx_int7_2_mux), + SND_SOC_DAPM_MUX("RX INT8_2 MUX", WCD9335_CDC_RX8_RX_PATH_MIX_CTL, + 5, 0, &rx_int8_2_mux), SND_SOC_DAPM_MUX("RX INT0_1 MIX1 INP0", SND_SOC_NOPM, 0, 0, &rx_int0_1_mix_inp0_mux), SND_SOC_DAPM_MUX("RX INT0_1 MIX1 INP1", SND_SOC_NOPM, 0, 0,
The vendor driver appears to be modifying the gain settings behind the back of user space but these hacks never made it upstream except for some essentially dead code that reads out the gain and writes it back again on DAPM events.
Drop these incomplete and pointless hacks.
Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/codecs/wcd934x.c | 102 ++++++++----------------------------- 1 file changed, 22 insertions(+), 80 deletions(-)
diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c index 1b6e376f3833..34e97d39a146 100644 --- a/sound/soc/codecs/wcd934x.c +++ b/sound/soc/codecs/wcd934x.c @@ -4357,12 +4357,8 @@ static int wcd934x_codec_enable_mix_path(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kc, int event) { struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm); - int offset_val = 0; - u16 gain_reg, mix_reg; - int val = 0; + u16 mix_reg;
- gain_reg = WCD934X_CDC_RX0_RX_VOL_MIX_CTL + - (w->shift * WCD934X_RX_PATH_CTL_OFFSET); mix_reg = WCD934X_CDC_RX0_RX_PATH_MIX_CTL + (w->shift * WCD934X_RX_PATH_CTL_OFFSET);
@@ -4373,12 +4369,6 @@ static int wcd934x_codec_enable_mix_path(struct snd_soc_dapm_widget *w, WCD934X_CDC_RX_MIX_CLK_EN_MASK, WCD934X_CDC_RX_MIX_CLK_ENABLE); break; - - case SND_SOC_DAPM_POST_PMU: - val = snd_soc_component_read(comp, gain_reg); - val += offset_val; - snd_soc_component_write(comp, gain_reg, val); - break; }
return 0; @@ -4418,26 +4408,6 @@ static int wcd934x_codec_set_iir_gain(struct snd_soc_dapm_widget *w, return 0; }
-static int wcd934x_codec_enable_main_path(struct snd_soc_dapm_widget *w, - struct snd_kcontrol *kcontrol, - int event) -{ - struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm); - u16 gain_reg; - - gain_reg = WCD934X_CDC_RX0_RX_VOL_CTL + (w->shift * - WCD934X_RX_PATH_CTL_OFFSET); - - switch (event) { - case SND_SOC_DAPM_POST_PMU: - snd_soc_component_write(comp, gain_reg, - snd_soc_component_read(comp, gain_reg)); - break; - } - - return 0; -} - static int wcd934x_codec_ear_dac_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kc, int event) { @@ -5238,32 +5208,25 @@ static const struct snd_soc_dapm_widget wcd934x_dapm_widgets[] = {
SND_SOC_DAPM_MUX_E("RX INT0_2 MUX", SND_SOC_NOPM, INTERP_EAR, 0, &rx_int0_2_mux, wcd934x_codec_enable_mix_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MUX_E("RX INT1_2 MUX", SND_SOC_NOPM, INTERP_HPHL, 0, &rx_int1_2_mux, wcd934x_codec_enable_mix_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MUX_E("RX INT2_2 MUX", SND_SOC_NOPM, INTERP_HPHR, 0, &rx_int2_2_mux, wcd934x_codec_enable_mix_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MUX_E("RX INT3_2 MUX", SND_SOC_NOPM, INTERP_LO1, 0, &rx_int3_2_mux, wcd934x_codec_enable_mix_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MUX_E("RX INT4_2 MUX", SND_SOC_NOPM, INTERP_LO2, 0, &rx_int4_2_mux, wcd934x_codec_enable_mix_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MUX_E("RX INT7_2 MUX", SND_SOC_NOPM, INTERP_SPKR1, 0, &rx_int7_2_mux, wcd934x_codec_enable_mix_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MUX_E("RX INT8_2 MUX", SND_SOC_NOPM, INTERP_SPKR2, 0, &rx_int8_2_mux, wcd934x_codec_enable_mix_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
SND_SOC_DAPM_MUX("RX INT0_1 MIX1 INP0", SND_SOC_NOPM, 0, 0, &rx_int0_1_mix_inp0_mux), @@ -5389,41 +5352,20 @@ static const struct snd_soc_dapm_widget wcd934x_dapm_widgets[] = { SND_SOC_DAPM_MUX("RX INT2 DEM MUX", SND_SOC_NOPM, 0, 0, &rx_int2_dem_inp_mux),
- SND_SOC_DAPM_MUX_E("RX INT0_1 INTERP", SND_SOC_NOPM, INTERP_EAR, 0, - &rx_int0_1_interp_mux, - wcd934x_codec_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), - SND_SOC_DAPM_MUX_E("RX INT1_1 INTERP", SND_SOC_NOPM, INTERP_HPHL, 0, - &rx_int1_1_interp_mux, - wcd934x_codec_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), - SND_SOC_DAPM_MUX_E("RX INT2_1 INTERP", SND_SOC_NOPM, INTERP_HPHR, 0, - &rx_int2_1_interp_mux, - wcd934x_codec_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), - SND_SOC_DAPM_MUX_E("RX INT3_1 INTERP", SND_SOC_NOPM, INTERP_LO1, 0, - &rx_int3_1_interp_mux, - wcd934x_codec_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), - SND_SOC_DAPM_MUX_E("RX INT4_1 INTERP", SND_SOC_NOPM, INTERP_LO2, 0, - &rx_int4_1_interp_mux, - wcd934x_codec_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), - SND_SOC_DAPM_MUX_E("RX INT7_1 INTERP", SND_SOC_NOPM, INTERP_SPKR1, 0, - &rx_int7_1_interp_mux, - wcd934x_codec_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), - SND_SOC_DAPM_MUX_E("RX INT8_1 INTERP", SND_SOC_NOPM, INTERP_SPKR2, 0, - &rx_int8_1_interp_mux, - wcd934x_codec_enable_main_path, - SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_MUX("RX INT0_1 INTERP", SND_SOC_NOPM, INTERP_EAR, 0, + &rx_int0_1_interp_mux), + SND_SOC_DAPM_MUX("RX INT1_1 INTERP", SND_SOC_NOPM, INTERP_HPHL, 0, + &rx_int1_1_interp_mux), + SND_SOC_DAPM_MUX("RX INT2_1 INTERP", SND_SOC_NOPM, INTERP_HPHR, 0, + &rx_int2_1_interp_mux), + SND_SOC_DAPM_MUX("RX INT3_1 INTERP", SND_SOC_NOPM, INTERP_LO1, 0, + &rx_int3_1_interp_mux), + SND_SOC_DAPM_MUX("RX INT4_1 INTERP", SND_SOC_NOPM, INTERP_LO2, 0, + &rx_int4_1_interp_mux), + SND_SOC_DAPM_MUX("RX INT7_1 INTERP", SND_SOC_NOPM, INTERP_SPKR1, 0, + &rx_int7_1_interp_mux), + SND_SOC_DAPM_MUX("RX INT8_1 INTERP", SND_SOC_NOPM, INTERP_SPKR2, 0, + &rx_int8_1_interp_mux),
SND_SOC_DAPM_MUX("RX INT0_2 INTERP", SND_SOC_NOPM, 0, 0, &rx_int0_2_interp_mux),
The vendor driver appears to be modifying the gain settings behind the back of user space but these hacks never made it upstream except for some essentially dead code that reads out the (cached) gain setting and writes it back again on DAPM events.
Drop these incomplete and pointless hacks.
Note that seemingly related 10 ms delay after enabling the interpolator is removed in the process.
Signed-off-by: Johan Hovold johan+linaro@kernel.org --- sound/soc/codecs/msm8916-wcd-digital.c | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-)
diff --git a/sound/soc/codecs/msm8916-wcd-digital.c b/sound/soc/codecs/msm8916-wcd-digital.c index 978c4d056e81..f73731aa4b58 100644 --- a/sound/soc/codecs/msm8916-wcd-digital.c +++ b/sound/soc/codecs/msm8916-wcd-digital.c @@ -228,17 +228,6 @@ struct msm8916_wcd_digital_priv { struct clk *ahbclk, *mclk; };
-static const unsigned long rx_gain_reg[] = { - LPASS_CDC_RX1_VOL_CTL_B2_CTL, - LPASS_CDC_RX2_VOL_CTL_B2_CTL, - LPASS_CDC_RX3_VOL_CTL_B2_CTL, -}; - -static const unsigned long tx_gain_reg[] = { - LPASS_CDC_TX1_VOL_CTL_GAIN, - LPASS_CDC_TX2_VOL_CTL_GAIN, -}; - static const char *const rx_mix1_text[] = { "ZERO", "IIR1", "IIR2", "RX1", "RX2", "RX3" }; @@ -580,12 +569,6 @@ static int msm8916_wcd_digital_enable_interpolator( struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
switch (event) { - case SND_SOC_DAPM_POST_PMU: - /* apply the digital gain after the interpolator is enabled */ - usleep_range(10000, 10100); - snd_soc_component_write(component, rx_gain_reg[w->shift], - snd_soc_component_read(component, rx_gain_reg[w->shift])); - break; case SND_SOC_DAPM_POST_PMD: snd_soc_component_update_bits(component, LPASS_CDC_CLK_RX_RESET_CTL, 1 << w->shift, 1 << w->shift); @@ -630,9 +613,6 @@ static int msm8916_wcd_digital_enable_dec(struct snd_soc_dapm_widget *w, snd_soc_component_update_bits(component, tx_mux_ctl_reg, TX_MUX_CTL_HPF_BP_SEL_MASK, TX_MUX_CTL_HPF_BP_SEL_NO_BYPASS); - /* apply the digital gain after the decimator is enabled */ - snd_soc_component_write(component, tx_gain_reg[w->shift], - snd_soc_component_read(component, tx_gain_reg[w->shift])); snd_soc_component_update_bits(component, tx_vol_ctl_reg, TX_VOL_CTL_CFG_MUTE_EN_MASK, 0); break; @@ -739,13 +719,13 @@ static const struct snd_soc_dapm_widget msm8916_wcd_digital_dapm_widgets[] = { /* Interpolator */ SND_SOC_DAPM_MIXER_E("RX1 INT", LPASS_CDC_CLK_RX_B1_CTL, 0, 0, NULL, 0, msm8916_wcd_digital_enable_interpolator, - SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MIXER_E("RX2 INT", LPASS_CDC_CLK_RX_B1_CTL, 1, 0, NULL, 0, msm8916_wcd_digital_enable_interpolator, - SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MIXER_E("RX3 INT", LPASS_CDC_CLK_RX_B1_CTL, 2, 0, NULL, 0, msm8916_wcd_digital_enable_interpolator, - SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_MUX("RX1 MIX1 INP1", SND_SOC_NOPM, 0, 0, &rx_mix1_inp1_mux), SND_SOC_DAPM_MUX("RX1 MIX1 INP2", SND_SOC_NOPM, 0, 0,
Hi Johan,
did you see any issue with this codec?
These are not really hacks, to make the gains effective it has to be applied in a particular order.
On 16/01/2024 09:39, Johan Hovold wrote:
/* apply the digital gain after the interpolator is enabled */
As you noticed in the comments, the gains have to be reprogrammed after interpolator and its clks are enabled.
usleep_range(10000, 10100);snd_soc_component_write(component, rx_gain_reg[w->shift],snd_soc_component_read(component, rx_gain_reg[w->shift]));break;@@ -630,9 +613,6 @@ static int msm8916_wcd_digital_enable_dec(struct snd_soc_dapm_widget *w, snd_soc_component_update_bits(component, tx_mux_ctl_reg, TX_MUX_CTL_HPF_BP_SEL_MASK, TX_MUX_CTL_HPF_BP_SEL_NO_BYPASS);
/* apply the digital gain after the decimator is enabled */
same here.
snd_soc_component_write(component, tx_gain_reg[w->shift],snd_soc_component_read(component, tx_gain_reg[w->shift]));
participants (4)
-
Johan Hovold -
Johan Hovold -
Mark Brown -
Srinivas Kandagatla