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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Thursday, 5 June 2025 03:57:54 Central European Summer Time Pei Xiao wrote:
> 
> 在 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@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; 

This isn't correct, -1 isn't passed as invalid but as both streams. This is
because it wants to pass something as an argument from the asoc core
directly in one code path (rockchip_sai_trigger to rockchip_sai_stop to
rockchip_sai_xfer_stop) but it also wants to clear both streams at once
in a different code path, and the enums aren't powers of two so it can't
just be flags bitwise-OR'd together.

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

This isn't really the right place for microoptimisations and the compiler is likely
doing this for us already anyway.

Kind regards,
Nicolas Frattaroli

> 
> >             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
> >
> >
>