| Message ID | 20250403124247.7313-3-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: codecs: lpass-wsa: fix VI capture setup. | expand |
On Thu, Apr 03, 2025 at 01:42:47PM +0100, srinivas.kandagatla@linaro.org wrote: > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > > Existing code only configures one of WSA_MACRO_TX0 or WSA_MACRO_TX1 > paths eventhough we enable both of them. Fix this bug by adding proper > checks and rearranging some of the common code to able to allow setting > both TX0 and TX1 paths > > Without this patch only one channel gets enabled in VI path instead of 2 > channels. End result would be 1 channel recording instead of 2. Could you please rearrange the code to make the patch more obvious? > > Fixes: 2c4066e5d428 ("ASoC: codecs: lpass-wsa-macro: add dapm widgets and route") > Cc: stable@vger.kernel.org > Co-developed-by: Manikantan R <quic_manrav@quicinc.com> > Signed-off-by: Manikantan R <quic_manrav@quicinc.com> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > sound/soc/codecs/lpass-wsa-macro.c | 112 +++++++++++++++++------------ > 1 file changed, 68 insertions(+), 44 deletions(-) > > diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c > index ac119847bc22..c9e7f185f2bc 100644 > --- a/sound/soc/codecs/lpass-wsa-macro.c > +++ b/sound/soc/codecs/lpass-wsa-macro.c > @@ -1469,46 +1469,11 @@ static int wsa_macro_mclk_event(struct snd_soc_dapm_widget *w, > return 0; > } > > -static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, > - struct snd_kcontrol *kcontrol, > - int event) > -{ > - struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); > - struct wsa_macro *wsa = snd_soc_component_get_drvdata(component); > - u32 tx_reg0, tx_reg1; > - u32 rate_val; > > - switch (wsa->pcm_rate_vi) { > - case 8000: > - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; > - break; > - case 16000: > - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_16K; > - break; > - case 24000: > - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_24K; > - break; > - case 32000: > - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_32K; > - break; > - case 48000: > - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_48K; > - break; > - default: > - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; > - break; > - } This can go to the wsa_macro_enable_disable_vi_sense(). > - > - if (test_bit(WSA_MACRO_TX0, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { > - tx_reg0 = CDC_WSA_TX0_SPKR_PROT_PATH_CTL; > - tx_reg1 = CDC_WSA_TX1_SPKR_PROT_PATH_CTL; > - } else if (test_bit(WSA_MACRO_TX1, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { > - tx_reg0 = CDC_WSA_TX2_SPKR_PROT_PATH_CTL; > - tx_reg1 = CDC_WSA_TX3_SPKR_PROT_PATH_CTL; > - } > - > - switch (event) { > - case SND_SOC_DAPM_POST_PMU: > +static void wsa_macro_enable_disable_vi_sense(struct snd_soc_component *component, bool enable, > + u32 tx_reg0, u32 tx_reg1, u32 val) > +{ > + if (enable) { > /* Enable V&I sensing */ > snd_soc_component_update_bits(component, tx_reg0, > CDC_WSA_TX_SPKR_PROT_RESET_MASK, > @@ -1518,10 +1483,10 @@ static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, > CDC_WSA_TX_SPKR_PROT_RESET); > snd_soc_component_update_bits(component, tx_reg0, > CDC_WSA_TX_SPKR_PROT_PCM_RATE_MASK, > - rate_val); > + val); No need for extra renames, they complicate reviewing. > snd_soc_component_update_bits(component, tx_reg1, > CDC_WSA_TX_SPKR_PROT_PCM_RATE_MASK, > - rate_val); > + val); > snd_soc_component_update_bits(component, tx_reg0, > CDC_WSA_TX_SPKR_PROT_CLK_EN_MASK, > CDC_WSA_TX_SPKR_PROT_CLK_ENABLE); > @@ -1534,9 +1499,7 @@ static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, > snd_soc_component_update_bits(component, tx_reg1, > CDC_WSA_TX_SPKR_PROT_RESET_MASK, > CDC_WSA_TX_SPKR_PROT_NO_RESET); > - break; > - case SND_SOC_DAPM_POST_PMD: > - /* Disable V&I sensing */ > + } else { > snd_soc_component_update_bits(component, tx_reg0, > CDC_WSA_TX_SPKR_PROT_RESET_MASK, > CDC_WSA_TX_SPKR_PROT_RESET); > @@ -1549,6 +1512,67 @@ static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, > snd_soc_component_update_bits(component, tx_reg1, > CDC_WSA_TX_SPKR_PROT_CLK_EN_MASK, > CDC_WSA_TX_SPKR_PROT_CLK_DISABLE); > + } > +} > + > +static void wsa_macro_enable_disable_vi_feedback(struct snd_soc_component *component, > + bool enable, u32 rate) > +{ > + struct wsa_macro *wsa = snd_soc_component_get_drvdata(component); > + u32 tx_reg0, tx_reg1; > + > + if (test_bit(WSA_MACRO_TX0, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { > + tx_reg0 = CDC_WSA_TX0_SPKR_PROT_PATH_CTL; > + tx_reg1 = CDC_WSA_TX1_SPKR_PROT_PATH_CTL; > + wsa_macro_enable_disable_vi_sense(component, enable, tx_reg0, tx_reg1, rate); As you are refactoring this piece of code, do you need tx_reg0 / tx_reg1 variables? Can you inline them instead? > + } > + > + if (test_bit(WSA_MACRO_TX1, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { > + tx_reg0 = CDC_WSA_TX2_SPKR_PROT_PATH_CTL; > + tx_reg1 = CDC_WSA_TX3_SPKR_PROT_PATH_CTL; > + wsa_macro_enable_disable_vi_sense(component, enable, tx_reg0, tx_reg1, rate); > + > + } > + Extra empty line. > +} > + > +static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, > + struct snd_kcontrol *kcontrol, > + int event) > +{ > + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); > + struct wsa_macro *wsa = snd_soc_component_get_drvdata(component); > + u32 rate_val; > + > + switch (wsa->pcm_rate_vi) { > + case 8000: > + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; > + break; > + case 16000: > + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_16K; > + break; > + case 24000: > + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_24K; > + break; > + case 32000: > + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_32K; > + break; > + case 48000: > + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_48K; > + break; > + default: > + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; > + break; > + } > + > + switch (event) { > + case SND_SOC_DAPM_POST_PMU: > + /* Enable V&I sensing */ > + wsa_macro_enable_disable_vi_feedback(component, true, rate_val); > + break; > + case SND_SOC_DAPM_POST_PMD: > + /* Disable V&I sensing */ > + wsa_macro_enable_disable_vi_feedback(component, false, rate_val); > break; > } > > -- > 2.39.5 >
On 03/04/2025 15:35, Dmitry Baryshkov wrote: > On Thu, Apr 03, 2025 at 01:42:47PM +0100, srinivas.kandagatla@linaro.org wrote: >> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> >> Existing code only configures one of WSA_MACRO_TX0 or WSA_MACRO_TX1 >> paths eventhough we enable both of them. Fix this bug by adding proper >> checks and rearranging some of the common code to able to allow setting >> both TX0 and TX1 paths >> >> Without this patch only one channel gets enabled in VI path instead of 2 >> channels. End result would be 1 channel recording instead of 2. > > Could you please rearrange the code to make the patch more obvious? Will try it in next version. > >> >> Fixes: 2c4066e5d428 ("ASoC: codecs: lpass-wsa-macro: add dapm widgets and route") >> Cc: stable@vger.kernel.org >> Co-developed-by: Manikantan R <quic_manrav@quicinc.com> >> Signed-off-by: Manikantan R <quic_manrav@quicinc.com> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> sound/soc/codecs/lpass-wsa-macro.c | 112 +++++++++++++++++------------ >> 1 file changed, 68 insertions(+), 44 deletions(-) >> >> diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c >> index ac119847bc22..c9e7f185f2bc 100644 >> --- a/sound/soc/codecs/lpass-wsa-macro.c >> +++ b/sound/soc/codecs/lpass-wsa-macro.c >> @@ -1469,46 +1469,11 @@ static int wsa_macro_mclk_event(struct snd_soc_dapm_widget *w, >> return 0; >> } >> >> -static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, >> - struct snd_kcontrol *kcontrol, >> - int event) >> -{ >> - struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); >> - struct wsa_macro *wsa = snd_soc_component_get_drvdata(component); >> - u32 tx_reg0, tx_reg1; >> - u32 rate_val; >> >> - switch (wsa->pcm_rate_vi) { >> - case 8000: >> - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; >> - break; >> - case 16000: >> - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_16K; >> - break; >> - case 24000: >> - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_24K; >> - break; >> - case 32000: >> - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_32K; >> - break; >> - case 48000: >> - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_48K; >> - break; >> - default: >> - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; >> - break; >> - } > > This can go to the wsa_macro_enable_disable_vi_sense(). Not sure I undestood this, v4 looks much clearner without this hunk. > >> - >> - if (test_bit(WSA_MACRO_TX0, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { >> - tx_reg0 = CDC_WSA_TX0_SPKR_PROT_PATH_CTL; >> - tx_reg1 = CDC_WSA_TX1_SPKR_PROT_PATH_CTL; >> - } else if (test_bit(WSA_MACRO_TX1, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { >> - tx_reg0 = CDC_WSA_TX2_SPKR_PROT_PATH_CTL; >> - tx_reg1 = CDC_WSA_TX3_SPKR_PROT_PATH_CTL; >> - } >> - >> - switch (event) { >> - case SND_SOC_DAPM_POST_PMU: >> +static void wsa_macro_enable_disable_vi_sense(struct snd_soc_component *component, bool enable, >> + u32 tx_reg0, u32 tx_reg1, u32 val) >> +{ >> + if (enable) { >> /* Enable V&I sensing */ >> snd_soc_component_update_bits(component, tx_reg0, >> CDC_WSA_TX_SPKR_PROT_RESET_MASK, >> @@ -1518,10 +1483,10 @@ static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, >> CDC_WSA_TX_SPKR_PROT_RESET); >> snd_soc_component_update_bits(component, tx_reg0, >> CDC_WSA_TX_SPKR_PROT_PCM_RATE_MASK, >> - rate_val); >> + val); > > No need for extra renames, they complicate reviewing. > >> snd_soc_component_update_bits(component, tx_reg1, >> CDC_WSA_TX_SPKR_PROT_PCM_RATE_MASK, >> - rate_val); >> + val); >> snd_soc_component_update_bits(component, tx_reg0, >> CDC_WSA_TX_SPKR_PROT_CLK_EN_MASK, >> CDC_WSA_TX_SPKR_PROT_CLK_ENABLE); >> @@ -1534,9 +1499,7 @@ static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, >> snd_soc_component_update_bits(component, tx_reg1, >> CDC_WSA_TX_SPKR_PROT_RESET_MASK, >> CDC_WSA_TX_SPKR_PROT_NO_RESET); >> - break; >> - case SND_SOC_DAPM_POST_PMD: >> - /* Disable V&I sensing */ >> + } else { >> snd_soc_component_update_bits(component, tx_reg0, >> CDC_WSA_TX_SPKR_PROT_RESET_MASK, >> CDC_WSA_TX_SPKR_PROT_RESET); >> @@ -1549,6 +1512,67 @@ static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, >> snd_soc_component_update_bits(component, tx_reg1, >> CDC_WSA_TX_SPKR_PROT_CLK_EN_MASK, >> CDC_WSA_TX_SPKR_PROT_CLK_DISABLE); >> + } >> +} >> + >> +static void wsa_macro_enable_disable_vi_feedback(struct snd_soc_component *component, >> + bool enable, u32 rate) >> +{ >> + struct wsa_macro *wsa = snd_soc_component_get_drvdata(component); >> + u32 tx_reg0, tx_reg1; >> + >> + if (test_bit(WSA_MACRO_TX0, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { >> + tx_reg0 = CDC_WSA_TX0_SPKR_PROT_PATH_CTL; >> + tx_reg1 = CDC_WSA_TX1_SPKR_PROT_PATH_CTL; >> + wsa_macro_enable_disable_vi_sense(component, enable, tx_reg0, tx_reg1, rate); > > As you are refactoring this piece of code, do you need tx_reg0 / tx_reg1 > variables? Can you inline them instead? Ack. > >> + } >> + >> + if (test_bit(WSA_MACRO_TX1, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { >> + tx_reg0 = CDC_WSA_TX2_SPKR_PROT_PATH_CTL; >> + tx_reg1 = CDC_WSA_TX3_SPKR_PROT_PATH_CTL; >> + wsa_macro_enable_disable_vi_sense(component, enable, tx_reg0, tx_reg1, rate); >> + >> + } >> + > > Extra empty line. Ack > >> +} >> + >> +static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, >> + struct snd_kcontrol *kcontrol, >> + int event) >> +{ >> + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); >> + struct wsa_macro *wsa = snd_soc_component_get_drvdata(component); >> + u32 rate_val; >> + >> + switch (wsa->pcm_rate_vi) { >> + case 8000: >> + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; >> + break; >> + case 16000: >> + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_16K; >> + break; >> + case 24000: >> + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_24K; >> + break; >> + case 32000: >> + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_32K; >> + break; >> + case 48000: >> + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_48K; >> + break; >> + default: >> + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; >> + break; >> + } >> + >> + switch (event) { >> + case SND_SOC_DAPM_POST_PMU: >> + /* Enable V&I sensing */ >> + wsa_macro_enable_disable_vi_feedback(component, true, rate_val); >> + break; >> + case SND_SOC_DAPM_POST_PMD: >> + /* Disable V&I sensing */ >> + wsa_macro_enable_disable_vi_feedback(component, false, rate_val); >> break; >> } >> >> -- >> 2.39.5 >> >
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c index ac119847bc22..c9e7f185f2bc 100644 --- a/sound/soc/codecs/lpass-wsa-macro.c +++ b/sound/soc/codecs/lpass-wsa-macro.c @@ -1469,46 +1469,11 @@ static int wsa_macro_mclk_event(struct snd_soc_dapm_widget *w, return 0; } -static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, - struct snd_kcontrol *kcontrol, - int event) -{ - struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); - struct wsa_macro *wsa = snd_soc_component_get_drvdata(component); - u32 tx_reg0, tx_reg1; - u32 rate_val; - switch (wsa->pcm_rate_vi) { - case 8000: - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; - break; - case 16000: - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_16K; - break; - case 24000: - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_24K; - break; - case 32000: - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_32K; - break; - case 48000: - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_48K; - break; - default: - rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; - break; - } - - if (test_bit(WSA_MACRO_TX0, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { - tx_reg0 = CDC_WSA_TX0_SPKR_PROT_PATH_CTL; - tx_reg1 = CDC_WSA_TX1_SPKR_PROT_PATH_CTL; - } else if (test_bit(WSA_MACRO_TX1, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { - tx_reg0 = CDC_WSA_TX2_SPKR_PROT_PATH_CTL; - tx_reg1 = CDC_WSA_TX3_SPKR_PROT_PATH_CTL; - } - - switch (event) { - case SND_SOC_DAPM_POST_PMU: +static void wsa_macro_enable_disable_vi_sense(struct snd_soc_component *component, bool enable, + u32 tx_reg0, u32 tx_reg1, u32 val) +{ + if (enable) { /* Enable V&I sensing */ snd_soc_component_update_bits(component, tx_reg0, CDC_WSA_TX_SPKR_PROT_RESET_MASK, @@ -1518,10 +1483,10 @@ static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, CDC_WSA_TX_SPKR_PROT_RESET); snd_soc_component_update_bits(component, tx_reg0, CDC_WSA_TX_SPKR_PROT_PCM_RATE_MASK, - rate_val); + val); snd_soc_component_update_bits(component, tx_reg1, CDC_WSA_TX_SPKR_PROT_PCM_RATE_MASK, - rate_val); + val); snd_soc_component_update_bits(component, tx_reg0, CDC_WSA_TX_SPKR_PROT_CLK_EN_MASK, CDC_WSA_TX_SPKR_PROT_CLK_ENABLE); @@ -1534,9 +1499,7 @@ static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, snd_soc_component_update_bits(component, tx_reg1, CDC_WSA_TX_SPKR_PROT_RESET_MASK, CDC_WSA_TX_SPKR_PROT_NO_RESET); - break; - case SND_SOC_DAPM_POST_PMD: - /* Disable V&I sensing */ + } else { snd_soc_component_update_bits(component, tx_reg0, CDC_WSA_TX_SPKR_PROT_RESET_MASK, CDC_WSA_TX_SPKR_PROT_RESET); @@ -1549,6 +1512,67 @@ static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, snd_soc_component_update_bits(component, tx_reg1, CDC_WSA_TX_SPKR_PROT_CLK_EN_MASK, CDC_WSA_TX_SPKR_PROT_CLK_DISABLE); + } +} + +static void wsa_macro_enable_disable_vi_feedback(struct snd_soc_component *component, + bool enable, u32 rate) +{ + struct wsa_macro *wsa = snd_soc_component_get_drvdata(component); + u32 tx_reg0, tx_reg1; + + if (test_bit(WSA_MACRO_TX0, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { + tx_reg0 = CDC_WSA_TX0_SPKR_PROT_PATH_CTL; + tx_reg1 = CDC_WSA_TX1_SPKR_PROT_PATH_CTL; + wsa_macro_enable_disable_vi_sense(component, enable, tx_reg0, tx_reg1, rate); + } + + if (test_bit(WSA_MACRO_TX1, &wsa->active_ch_mask[WSA_MACRO_AIF_VI])) { + tx_reg0 = CDC_WSA_TX2_SPKR_PROT_PATH_CTL; + tx_reg1 = CDC_WSA_TX3_SPKR_PROT_PATH_CTL; + wsa_macro_enable_disable_vi_sense(component, enable, tx_reg0, tx_reg1, rate); + + } + +} + +static int wsa_macro_enable_vi_feedback(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, + int event) +{ + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); + struct wsa_macro *wsa = snd_soc_component_get_drvdata(component); + u32 rate_val; + + switch (wsa->pcm_rate_vi) { + case 8000: + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; + break; + case 16000: + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_16K; + break; + case 24000: + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_24K; + break; + case 32000: + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_32K; + break; + case 48000: + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_48K; + break; + default: + rate_val = CDC_WSA_TX_SPKR_PROT_PCM_RATE_8K; + break; + } + + switch (event) { + case SND_SOC_DAPM_POST_PMU: + /* Enable V&I sensing */ + wsa_macro_enable_disable_vi_feedback(component, true, rate_val); + break; + case SND_SOC_DAPM_POST_PMD: + /* Disable V&I sensing */ + wsa_macro_enable_disable_vi_feedback(component, false, rate_val); break; }