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] [day] [month] [year] [list]
Message-ID: <69e32e9d-f263-4ed8-bd25-5e09dae9c20d@linaro.org>
Date: Thu, 3 Apr 2025 16:51:07 +0100
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
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 03/04/2025 15:35, Dmitry Baryshkov wrote:
> 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?
Will try it in next version.

> 
>>
>> 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().
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
>>
>