Re: [PATCH v2] pwm: atmel-tcb: Cache clock rates and mark chip as atomic

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]

Hello Sangyun,

On Sun, Apr 19, 2026 at 05:08:38PM +0900, Sangyun Kim wrote:
> @@ -438,16 +441,33 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_gclk;
>  
> +	err = clk_rate_exclusive_get(tcbpwmc->clk);
> +	if (err)
> +		goto err_disable_clk;
> +
> +	err = clk_rate_exclusive_get(tcbpwmc->slow_clk);
> +	if (err)
> +		goto err_clk_unlock;
> +
> +	tcbpwmc->rate = clk_get_rate(tcbpwmc->clk);
> +	tcbpwmc->slow_rate = clk_get_rate(tcbpwmc->slow_clk);
> +

Only one concern left: clk_get_rate() should only be called on enabled
clocks. I don't know the architecture details and how expensive it is to
have .clk enabled (or if it's enabled anyhow).

If you're ok, I'd squash the following diff into your patch:

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index 1a2832f1ace2..3d30aeab507e 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -437,13 +437,17 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 	tcbpwmc->channel = channel;
 	tcbpwmc->width = config->counter_width;
 
-	err = clk_prepare_enable(tcbpwmc->slow_clk);
+	err = clk_prepare_enable(tcbpwmc->clk);
 	if (err)
 		goto err_gclk;
 
+	err = clk_prepare_enable(tcbpwmc->slow_clk);
+	if (err)
+		goto err_disable_clk;;
+
 	err = clk_rate_exclusive_get(tcbpwmc->clk);
 	if (err)
-		goto err_disable_clk;
+		goto err_disable_slow_clk;
 
 	err = clk_rate_exclusive_get(tcbpwmc->slow_clk);
 	if (err)
@@ -469,6 +473,9 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev)
 	clk_rate_exclusive_put(tcbpwmc->clk);
 
 err_disable_clk:
+	clk_disable_unprepare(tcbpwmc->clk);
+
+err_disable_slow_clk:
 	clk_disable_unprepare(tcbpwmc->slow_clk);
 
 err_gclk:
@@ -492,6 +499,7 @@ static void atmel_tcb_pwm_remove(struct platform_device *pdev)
 
 	clk_rate_exclusive_put(tcbpwmc->slow_clk);
 	clk_rate_exclusive_put(tcbpwmc->clk);
+	clk_disable_unprepare(tcbpwmc->clk);
 	clk_disable_unprepare(tcbpwmc->slow_clk);
 	clk_put(tcbpwmc->gclk);
 	clk_put(tcbpwmc->clk);


This has the downside that clk is kept enabled the whole driver
lifetime, but that's the easiest way to make your fix honor the clk API
constraints. This allows to fast-track the patch fixing the sleeping
function called from invalid context issue and the optimisation can then
be addressed with more time during the next development cycles.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]