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

[Sangyun Kim <sangyun.kim@snu.ac.kr>]

On Tue, Apr 21, 2026 at 01:40:55 PM +0200, Uwe Kleine-König wrote:
>Hello Sangyun,

Hi Uwe,
Thanks for the review.

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

That makes sense. clk_get_rate() should indeed only be used on enabled
clocks, and your change is the simplest way to ensure correctness while
respecting the clk API constraints. I’m happy with squashing your diff
into my 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.

Keeping the clock enabled for the driver lifetime is acceptable for now
to fast-track the fix, and we can revisit potential optimizations in a
follow-up patch.

Thanks again for the suggestion.

>
>Best regards
>Uwe

Best regards,
Sangyun