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

[Brian Norris <briannorris@xxxxxxxxxxxx>]

On Mon, Aug 26, 2024 at 6:33 PM Jon Lin <jon.lin@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxx/
> > [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.

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