Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Jon Lin <jon.lin@rock-chips.com>]

On 2024/8/27 10:59, Brian Norris wrote:
> On Mon, Aug 26, 2024 at 6:33 PM Jon Lin <jon.lin@rock-chips.com> wrote:
>> On 2024/8/27 6:27, Brian Norris wrote:
>>> It seems like you'd really be served well by
>>> pm_runtime_force_{suspend,resume}() here, and in fact, that's what this
>>> driver used to use before the breaking change (commit
>>> e882575efc77). Why aren't you just going back to using it? (This is the
>>> kind of thing I might expect in your commit message -- reasoning as to
>>> why you're doing what you're doing.)
>>>
>>> And in fact, I already submitted a patch that resolves the above problem
>>> and does exactly that:
>>>
>>> https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/
>>> [PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling
>>>
>>> Do you see any problem with it?
>>>
>>
>> I have reviewed your submission and although the code has been
>> simplified, the execution efficiency has decreased. So although it is a
>> commonly used processing solution for SPI Upstream, I still hope to
>> retain a more efficiency approach as I submitted.
> 
> What do you mean by "efficiency"? You mean because there's
> indirection, via the PM runtime framework? If so, I doubt that's a
> priority for this piece of functionality -- simplicity is more
> important than a function call or two when talking about system
> suspend.
> 

Your code is fine, and I have tested it locally. The interface 
simplification is indeed in line with the direction of community 
development, and your solution can be used as a solution.

> Additionally, simplicity has additional benefits -- it heads off
> questions that your more complex code doesn't address. For example,
> are runtime PM and system PM mutually exclusive? Do we have to
> coordinate with any pending autosuspend? (Reading through
> https://docs.kernel.org/power/runtime_pm.html, I believe these are not
> actually concerns, but it's really not obvious and takes a bit of
> reading.) But your patch makes it more likely that runtime and system
> PM states get out of sync.
> 
> Anyway, if the patches really are equivalent, I suppose it can be the
> maintainer's choice as to which is preferable.
> 
> Brian
>