linux-kernel - Re: [PATCH v3 2/2] ASoC: codecs:lpass-wsa-macro: Fix logic of enabling vi channels

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <pzyw3swuj4gqynxtp7kxbludyf6qq7fdfjaphw73tezzqocrda@i3f2knhbocme>
Date: Thu, 3 Apr 2025 17:35:36 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: srinivas.kandagatla@...aro.org
Cc: broonie@...nel.org, lgirdwood@...il.com, perex@...ex.cz, tiwai@...e.com,
        pierre-louis.bossart@...ux.dev, linux-sound@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, Manikantan R <quic_manrav@...cinc.com>
Subject: Re: [PATCH v3 2/2] ASoC: codecs:lpass-wsa-macro: Fix logic of
 enabling vi channels

On Thu, Apr 03, 2025 at 01:42:47PM +0100, srinivas.kandagatla@...aro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.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@...r.kernel.org
> Co-developed-by: Manikantan R <quic_manrav@...cinc.com>
> Signed-off-by: Manikantan R <quic_manrav@...cinc.com>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.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
> 

-- 
With best wishes
Dmitry