Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled

[Kunihiko Hayashi <hayashi.kunihiko@socionext.com>]

Hi Andi,

Thank you for your comment.

On 2025/08/20 7:06, Andi Shyti wrote:
> Hi Kunihiko,
> 
> On Thu, Jul 24, 2025 at 01:22:10PM +0900, Kunihiko Hayashi wrote:
>> When removing the driver, enable the clocks once by calling
>> pm_runtime_get_sync(), and call pm_runtime_put_sync() to disable
>> the clocks.
> 
> can we rephrase this to something like:
> 
>    When the driver is removed, the clocks are first enabled by
>    calling pm_runtime_get_sync(), and then disabled with
>    pm_runtime_put_sync().
> 
> Does it work?

Yes, your description is better.

> 
>> If CONFIG_PM=y, clocks for this controller are disabled when it's in
>> the idle state. So the clocks are properly disabled when the driver
>> exits.
>>
>> Othewise, the clocks are always enabled and the PM functions have
>> no effect. Therefore, the driver exits without disabling the clocks.
>>
>>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>>      18
>>      # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/bind
>>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>>      20
>>      # echo 1214a000.i2c >
> /sys/bus/platform/drivers/i2c_designware/unbind
>>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>>      20
>>
>> To ensure that the clocks can be disabled correctly even without
>> CONFIG_PM=y, should add the following fixes:
>>
>> - Replace with pm_runtime_put_noidle(), which only decrements the
> runtime
>>    PM usage count.
>> - Call i2c_dw_prepare_clk(false) to explicitly disable the clocks.
>>
>> Fixes: 7272194ed391f ("i2c-designware: add minimal support for runtime
> PM")
> 
> This commit doesn't look quite right to me, although it's quite
> difficult to find the culprit, as the clk api's have changed a
> lot over time.
> 
> Do you think this is better:
> 
> Fixes: b33af11de236 ("i2c: designware: Do not require clock when SSCN and
> FFCN are provided")

Surely, the code that handles PM and clocks has changed some time,
so it's difficult to identify the commit with "Fixes" tag.
I think the first commit that called pm_runtime_put_sync() was the cause.

> It doesn't matter much as it won't apply in any of the two
> versions> 
> The Fixes tag should be added in both the patches, but there is
> no need to resend, I will apply them once we agree on the commit
> message.

Okay, I agree with you.

Thank you,

---
Best Regards
Kunihiko Hayashi