[PATCH 1/2] ASOC: rochchip: Simplify the condition logic in rockchip_sai_xfer_stop

[Pei Xiao]

在 2025/6/5 01:23, Nicolas Frattaroli 写道:
> On Wednesday, 4 June 2025 05:13:29 Central European Summer Time Pei Xiao wrote:
>> cocci warning:
>> ./sound/soc/rockchip/rockchip_sai.c:387:8-10:
>> 	WARNING: possible condition with no effect (if == else)
>>
>> Simplify the condition logic in rockchip_sai_xfer_stop() by removing the
>> redundant SNDRV_PCM_STREAM_PLAYBACK branch. The modified logic now:
>> 1. For stream < 0: handles both playback and capture
>> 2. For all other cases (both PLAYBACK and CAPTURE):
>>    sets playback = true and capture = false
>>
>> Signed-off-by: Pei Xiao <xiaopei01 at kylinos.cn>
>> ---
>>  sound/soc/rockchip/rockchip_sai.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/sound/soc/rockchip/rockchip_sai.c b/sound/soc/rockchip/rockchip_sai.c
>> index 602f1ddfad00..79b04770da1c 100644
>> --- a/sound/soc/rockchip/rockchip_sai.c
>> +++ b/sound/soc/rockchip/rockchip_sai.c
>> @@ -384,9 +384,6 @@ static void rockchip_sai_xfer_stop(struct rk_sai_dev *sai, int stream)
>>  	if (stream < 0) {
>>  		playback = true;
>>  		capture = true;
>> -	} else if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> -		playback = true;
>> -		capture = false;
>>  	} else {
>>  		playback = true;
>>  		capture = false;
>>
> You can probably get rid of the locals playback and capture altogether:
>
>     static void rockchip_sai_xfer_stop(struct rk_sai_dev *sai, int stream)
>     {
>         unsigned int msk, val, clr;
>
>         msk = SAI_XFER_TXS_MASK;
>         val = SAI_XFER_TXS_DIS;
>         clr = SAI_CLR_TXC;
>         
>         if (stream < 0) {
>             msk |= SAI_XFER_RXS_MASK;
>             val |= SAI_XFER_RXS_DIS;
>             clr |= SAI_CLR_RXC;
>         }
>
>         regmap_update_bits(sai->regmap, SAI_XFER, msk, val);
>         rockchip_sai_poll_stream_idle(sai, true, stream < 0);
>
>         rockchip_sai_clear(sai, clr);
>     }
>
> but this in general makes me suspicious of the intent of the code in
> the first place. Playback always being true and capture only being
> true if playback is also true seems odd. Checking the callsites of

Yes,it's very odd to me too.So I send this patch to ask for your advice.

> this function confirms my suspicions that while this fixes the cocci
> warning, it just means the code is now intentionally broken.
>
> This here may be closer to the original intent:
>
>     static void rockchip_sai_xfer_stop(struct rk_sai_dev *sai, int stream)
>     {
>             unsigned int msk = 0, val = 0, clr = 0;
>             bool capture = stream == SNDRV_PCM_STREAM_CAPTURE || stream < 0;
>             /* could be <= 0 but we don't want to depend on enum values */
>             bool playback = stream == SNDRV_PCM_STREAM_PLAYBACK || stream < 0;


bool invalid =  stream < 0; 

bool capture =  stream == SNDRV_PCM_STREAM_CAPTURE || invalid; 

bool playback = stream == SNDRV_PCM_STREAM_PLAYBACK || invalid;

Would this modification be acceptable? 

It could shorten each line since the stream value only needs to be evaluated once.

>             if (playback) {
>                     msk |= SAI_XFER_TXS_MASK;
>                     val |= SAI_XFER_TXS_DIS;
>                     clr |= SAI_CLR_TXC;
>             }
>
>             if (capture) {
>                     msk |= SAI_XFER_RXS_MASK;
>                     val |= SAI_XFER_RXS_DIS;
>                     clr |= SAI_CLR_RXC;
>             }
>
>             regmap_update_bits(sai->regmap, SAI_XFER, msk, val);
>             rockchip_sai_poll_stream_idle(sai, playback, capture);
>
>             rockchip_sai_clear(sai, clr);
>     }
>
>
> Please let me know whether this looks right to you.

thanks！

Pei.

> Kind regards,
> Nicolas Frattaroli
>
>