[PATCH 0/2] Cleanup in rockchip_sai.c

[PATCH 0/2] Cleanup in rockchip_sai.c

[Pei Xiao]

1.Simplify the condition logic in
2.Use helper function devm_clk_get_enabled()

Pei Xiao (2):
  ASOC: rochchip: Simplify the condition logic in rockchip_sai_xfer_stop
  ASOC: rockchip: Use helper function devm_clk_get_enabled()

 sound/soc/rockchip/rockchip_sai.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

-- 
2.25.1

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

[Pei Xiao]

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

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

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

            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.

Kind regards,
Nicolas Frattaroli

Re: [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@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
>
>

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

[Nicolas Frattaroli]

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

[PATCH 2/2] ASOC: rockchip: Use helper function devm_clk_get_enabled()

[Pei Xiao]

Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
replaced by devm_clk_get_enabled() when driver enables the clocks for the
whole lifetime of the device. Moreover, it is no longer necessary to
unprepare and disable the clocks explicitly.

Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 sound/soc/rockchip/rockchip_sai.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_sai.c b/sound/soc/rockchip/rockchip_sai.c
index 79b04770da1c..2ec675708681 100644
--- a/sound/soc/rockchip/rockchip_sai.c
+++ b/sound/soc/rockchip/rockchip_sai.c
@@ -1448,16 +1448,12 @@ static int rockchip_sai_probe(struct platform_device *pdev)
 				     "Failed to get mclk\n");
 	}
 
-	sai->hclk = devm_clk_get(&pdev->dev, "hclk");
+	sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
 	if (IS_ERR(sai->hclk)) {
 		return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
 				     "Failed to get hclk\n");
 	}
 
-	ret = clk_prepare_enable(sai->hclk);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
-
 	regmap_read(sai->regmap, SAI_VERSION, &sai->version);
 
 	ret = rockchip_sai_init_dai(sai, res, &dai);
@@ -1512,8 +1508,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
 	if (pm_runtime_put(&pdev->dev))
 		rockchip_sai_runtime_suspend(&pdev->dev);
 err_disable_hclk:
-	clk_disable_unprepare(sai->hclk);
-
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH 2/2] ASOC: rockchip: Use helper function devm_clk_get_enabled()

[Nicolas Frattaroli]

On Wednesday, 4 June 2025 05:13:30 Central European Summer Time Pei Xiao wrote:
> Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
> and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
> replaced by devm_clk_get_enabled() when driver enables the clocks for the
> whole lifetime of the device. Moreover, it is no longer necessary to
> unprepare and disable the clocks explicitly.
> 
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  sound/soc/rockchip/rockchip_sai.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/sound/soc/rockchip/rockchip_sai.c b/sound/soc/rockchip/rockchip_sai.c
> index 79b04770da1c..2ec675708681 100644
> --- a/sound/soc/rockchip/rockchip_sai.c
> +++ b/sound/soc/rockchip/rockchip_sai.c
> @@ -1448,16 +1448,12 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>  				     "Failed to get mclk\n");
>  	}
>  
> -	sai->hclk = devm_clk_get(&pdev->dev, "hclk");
> +	sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
>  	if (IS_ERR(sai->hclk)) {
>  		return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
>  				     "Failed to get hclk\n");
>  	}
>  
> -	ret = clk_prepare_enable(sai->hclk);
> -	if (ret)
> -		return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
> -
>  	regmap_read(sai->regmap, SAI_VERSION, &sai->version);
>  
>  	ret = rockchip_sai_init_dai(sai, res, &dai);
> @@ -1512,8 +1508,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>  	if (pm_runtime_put(&pdev->dev))
>  		rockchip_sai_runtime_suspend(&pdev->dev);
>  err_disable_hclk:
> -	clk_disable_unprepare(sai->hclk);
> -
>  	return ret;
>  }
>  
> 

Please get rid of the err_disable_hclk label, and change the

    goto err_disable_hclk;

in the resume failure condition to a 

    return ret;

Other than that, patch tested to be working fine.

Kind regards,
Nicolas Frattaroli

Re: [PATCH 2/2] ASOC: rockchip: Use helper function devm_clk_get_enabled()

[Pei Xiao]

在 2025/6/5 01:42, Nicolas Frattaroli 写道:
> On Wednesday, 4 June 2025 05:13:30 Central European Summer Time Pei Xiao wrote:
>> Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
>> and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
>> replaced by devm_clk_get_enabled() when driver enables the clocks for the
>> whole lifetime of the device. Moreover, it is no longer necessary to
>> unprepare and disable the clocks explicitly.
>>
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>>  sound/soc/rockchip/rockchip_sai.c | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/sound/soc/rockchip/rockchip_sai.c b/sound/soc/rockchip/rockchip_sai.c
>> index 79b04770da1c..2ec675708681 100644
>> --- a/sound/soc/rockchip/rockchip_sai.c
>> +++ b/sound/soc/rockchip/rockchip_sai.c
>> @@ -1448,16 +1448,12 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>>  				     "Failed to get mclk\n");
>>  	}
>>  
>> -	sai->hclk = devm_clk_get(&pdev->dev, "hclk");
>> +	sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
>>  	if (IS_ERR(sai->hclk)) {
>>  		return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
>>  				     "Failed to get hclk\n");
>>  	}
>>  
>> -	ret = clk_prepare_enable(sai->hclk);
>> -	if (ret)
>> -		return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
>> -
>>  	regmap_read(sai->regmap, SAI_VERSION, &sai->version);
>>  
>>  	ret = rockchip_sai_init_dai(sai, res, &dai);
>> @@ -1512,8 +1508,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>>  	if (pm_runtime_put(&pdev->dev))
>>  		rockchip_sai_runtime_suspend(&pdev->dev);
>>  err_disable_hclk:
>> -	clk_disable_unprepare(sai->hclk);
>> -
>>  	return ret;
>>  }
>>  
>>
> Please get rid of the err_disable_hclk label, and change the
>
>     goto err_disable_hclk;
>
> in the resume failure condition to a 
>
>     return ret;

ok,thanks!

> Other than that, patch tested to be working fine.
>
> Kind regards,
> Nicolas Frattaroli
>
>

Re: [PATCH 2/2] ASOC: rockchip: Use helper function devm_clk_get_enabled()

[Pei Xiao]

在 2025/6/5 01:42, Nicolas Frattaroli 写道:
> On Wednesday, 4 June 2025 05:13:30 Central European Summer Time Pei Xiao wrote:
>> Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
>> and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
>> replaced by devm_clk_get_enabled() when driver enables the clocks for the
>> whole lifetime of the device. Moreover, it is no longer necessary to
>> unprepare and disable the clocks explicitly.
>>
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>>  sound/soc/rockchip/rockchip_sai.c | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/sound/soc/rockchip/rockchip_sai.c b/sound/soc/rockchip/rockchip_sai.c
>> index 79b04770da1c..2ec675708681 100644
>> --- a/sound/soc/rockchip/rockchip_sai.c
>> +++ b/sound/soc/rockchip/rockchip_sai.c
>> @@ -1448,16 +1448,12 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>>  				     "Failed to get mclk\n");
>>  	}
>>  
>> -	sai->hclk = devm_clk_get(&pdev->dev, "hclk");
>> +	sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
>>  	if (IS_ERR(sai->hclk)) {
>>  		return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
>>  				     "Failed to get hclk\n");
>>  	}
>>  
>> -	ret = clk_prepare_enable(sai->hclk);
>> -	if (ret)
>> -		return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
>> -
>>  	regmap_read(sai->regmap, SAI_VERSION, &sai->version);
>>  
>>  	ret = rockchip_sai_init_dai(sai, res, &dai);
>> @@ -1512,8 +1508,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>>  	if (pm_runtime_put(&pdev->dev))
>>  		rockchip_sai_runtime_suspend(&pdev->dev);
>>  err_disable_hclk:
>> -	clk_disable_unprepare(sai->hclk);
>> -
>>  	return ret;
>>  }
>>  
>>
> Please get rid of the err_disable_hclk label, and change the
>
>     goto err_disable_hclk;
>
> in the resume failure condition to a 
>
>     return ret;
May I ask, could we use the dev_err_probe function instead?

return dev_err_probe(&pdev->dev, ret, "Failed to initialize DAI\n");


@@ -1441,28 +1441,22 @@ static int rockchip_sai_probe(struct platform_device *pdev)
                                     "Failed to get mclk\n");
        }
 
-       sai->hclk = devm_clk_get(&pdev->dev, "hclk");
+       sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
        if (IS_ERR(sai->hclk)) {
                return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
                                     "Failed to get hclk\n");
        }
 
-       ret = clk_prepare_enable(sai->hclk);
-       if (ret)
-               return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
-
        regmap_read(sai->regmap, SAI_VERSION, &sai->version);
 
        ret = rockchip_sai_init_dai(sai, res, &dai);
        if (ret) {
-               dev_err(&pdev->dev, "Failed to initialize DAI: %d\n", ret);
-               goto err_disable_hclk;
+               return dev_err_probe(&pdev->dev, ret, "Failed to initialize DAI\n");
        }
 
        ret = rockchip_sai_parse_paths(sai, node);
        if (ret) {
-               dev_err(&pdev->dev, "Failed to parse paths: %d\n", ret);
-               goto err_disable_hclk;
+               return dev_err_probe(&pdev->dev, ret, "Failed to parse paths\n");
        }
 
        /*
@@ -1475,8 +1469,7 @@ static int rockchip_sai_probe(struct platform_device *pdev)
        pm_runtime_get_noresume(&pdev->dev);
        ret = rockchip_sai_runtime_resume(&pdev->dev);
        if (ret) {
-               dev_err(&pdev->dev, "Failed to resume device: %pe\n", ERR_PTR(ret));
-               goto err_disable_hclk;
+               return dev_err_probe(&pdev->dev, ret, "Failed to resume device\n");
        }
 
        ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
@@ -1504,8 +1497,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
        /* If we're !CONFIG_PM, we get -ENOSYS and disable manually */
        if (pm_runtime_put(&pdev->dev))
                rockchip_sai_runtime_suspend(&pdev->dev);
-err_disable_hclk:
-       clk_disable_unprepare(sai->hclk);
 
        return ret;
 }

thanks!

Pei.

> Other than that, patch tested to be working fine.
>
> Kind regards,
> Nicolas Frattaroli
>
>

Re: [PATCH 2/2] ASOC: rockchip: Use helper function devm_clk_get_enabled()

[Nicolas Frattaroli]

On Friday, 6 June 2025 05:27:13 Central European Summer Time Pei Xiao wrote:
> 
> 在 2025/6/5 01:42, Nicolas Frattaroli 写道:
> > On Wednesday, 4 June 2025 05:13:30 Central European Summer Time Pei Xiao wrote:
> >> Since commit 7ef9651e9792 ("clk: Provide new devm_clk helpers for prepared
> >> and enabled clocks"), devm_clk_get() and clk_prepare_enable() can now be
> >> replaced by devm_clk_get_enabled() when driver enables the clocks for the
> >> whole lifetime of the device. Moreover, it is no longer necessary to
> >> unprepare and disable the clocks explicitly.
> >>
> >> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> >> ---
> >>  sound/soc/rockchip/rockchip_sai.c | 8 +-------
> >>  1 file changed, 1 insertion(+), 7 deletions(-)
> >>
> >> diff --git a/sound/soc/rockchip/rockchip_sai.c b/sound/soc/rockchip/rockchip_sai.c
> >> index 79b04770da1c..2ec675708681 100644
> >> --- a/sound/soc/rockchip/rockchip_sai.c
> >> +++ b/sound/soc/rockchip/rockchip_sai.c
> >> @@ -1448,16 +1448,12 @@ static int rockchip_sai_probe(struct platform_device *pdev)
> >>  				     "Failed to get mclk\n");
> >>  	}
> >>  
> >> -	sai->hclk = devm_clk_get(&pdev->dev, "hclk");
> >> +	sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
> >>  	if (IS_ERR(sai->hclk)) {
> >>  		return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
> >>  				     "Failed to get hclk\n");
> >>  	}
> >>  
> >> -	ret = clk_prepare_enable(sai->hclk);
> >> -	if (ret)
> >> -		return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
> >> -
> >>  	regmap_read(sai->regmap, SAI_VERSION, &sai->version);
> >>  
> >>  	ret = rockchip_sai_init_dai(sai, res, &dai);
> >> @@ -1512,8 +1508,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
> >>  	if (pm_runtime_put(&pdev->dev))
> >>  		rockchip_sai_runtime_suspend(&pdev->dev);
> >>  err_disable_hclk:
> >> -	clk_disable_unprepare(sai->hclk);
> >> -
> >>  	return ret;
> >>  }
> >>  
> >>
> > Please get rid of the err_disable_hclk label, and change the
> >
> >     goto err_disable_hclk;
> >
> > in the resume failure condition to a 
> >
> >     return ret;
> May I ask, could we use the dev_err_probe function instead?
> 
> return dev_err_probe(&pdev->dev, ret, "Failed to initialize DAI\n");

Absolutely, dev_err_probe states the following in its documentation:

    Using this helper in your probe function is totally fine even if @err
    is known to never be -EPROBE_DEFER.

This means you can use it for every error case in the probe function.

> 
> 
> @@ -1441,28 +1441,22 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>                                      "Failed to get mclk\n");
>         }
>  
> -       sai->hclk = devm_clk_get(&pdev->dev, "hclk");
> +       sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
>         if (IS_ERR(sai->hclk)) {
>                 return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
>                                      "Failed to get hclk\n");
>         }
>  
> -       ret = clk_prepare_enable(sai->hclk);
> -       if (ret)
> -               return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
> -
>         regmap_read(sai->regmap, SAI_VERSION, &sai->version);
>  
>         ret = rockchip_sai_init_dai(sai, res, &dai);
>         if (ret) {
> -               dev_err(&pdev->dev, "Failed to initialize DAI: %d\n", ret);
> -               goto err_disable_hclk;
> +               return dev_err_probe(&pdev->dev, ret, "Failed to initialize DAI\n");
>         }

This would now be a one-line statement in the if branch, so checkpatch will
advise you to remove the redundant { } from the if, so that it becomes

    if (ret)
            return dev_err_probe(&pdev->dev, ret, "Failed to initialize DAI\n");

You can run `./scripts/checkpatch.pl` on either a git commit range or a
patch file, and it'll let you know. If you use b4, it'll run that
script with some recommended flags on all commits in your series with
`b4 prep --check`.

>  
>         ret = rockchip_sai_parse_paths(sai, node);
>         if (ret) {
> -               dev_err(&pdev->dev, "Failed to parse paths: %d\n", ret);
> -               goto err_disable_hclk;
> +               return dev_err_probe(&pdev->dev, ret, "Failed to parse paths\n");
>         }

Same here

>  
>         /*
> @@ -1475,8 +1469,7 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>         pm_runtime_get_noresume(&pdev->dev);
>         ret = rockchip_sai_runtime_resume(&pdev->dev);
>         if (ret) {
> -               dev_err(&pdev->dev, "Failed to resume device: %pe\n", ERR_PTR(ret));
> -               goto err_disable_hclk;
> +               return dev_err_probe(&pdev->dev, ret, "Failed to resume device\n");
>         }

Same here

>  
>         ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> @@ -1504,8 +1497,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>         /* If we're !CONFIG_PM, we get -ENOSYS and disable manually */
>         if (pm_runtime_put(&pdev->dev))
>                 rockchip_sai_runtime_suspend(&pdev->dev);
> -err_disable_hclk:
> -       clk_disable_unprepare(sai->hclk);
>  
>         return ret;
>  }
> 
> thanks!
> 
> Pei.

Kind regards,
Nicolas Frattaroli

> 
> > Other than that, patch tested to be working fine.
> >
> > Kind regards,
> > Nicolas Frattaroli
> >
> >
> 

Re: [PATCH 0/2] Cleanup in rockchip_sai.c

[Nicolas Frattaroli]

On Wednesday, 4 June 2025 05:13:28 Central European Summer Time Pei Xiao wrote:
> 1.Simplify the condition logic in
> 2.Use helper function devm_clk_get_enabled()
> 
> Pei Xiao (2):
>   ASOC: rochchip: Simplify the condition logic in rockchip_sai_xfer_stop
>   ASOC: rockchip: Use helper function devm_clk_get_enabled()
> 
>  sound/soc/rockchip/rockchip_sai.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> 

Hi,

for the v2 of this series, please To/Cc the maintainer that will actually
be merging the patches, not just me. You can get a full list of people to
include as addresses for your series with the ./scripts/get_maintainer.pl
script.

Ideally, you'll use a tool like b4[1] to make your life easier here and
do this for you. That way, Mark Brown won't yell at you as much, and the
responsible people have a higher chance of seeing the patches. The b4
tool has the `b4 prep --auto-to-cc` option for this, and it'll warn you
before `b4 send` if you haven't --auto-to-cc'd before sending. If you
are using your own SMTP server to send and not a b4 relay, you'll
probably want to `b4 send --no-sign`, but a dry run with
`b4 send -o some-dir/` is advisible beforehand so you can look over the
e-mails being generated.

Link: https://b4.docs.kernel.org/en/latest/contributor/overview.html [1]

Kind regards,
Nicolas Frattaroli