Re: [PATCH v4 2/2] PM: runtime: Add Example Driver Patterns section

[Brian Norris]

Hi Rafael,

On Fri, Oct 16, 2026 at 08:05:12PM +0200, Rafael J. Wysocki (Intel) wrote:
> On Fri, Oct 16, 2026 at 7:42 PM Rafael J. Wysocki (Intel)
> <rafael@xxxxxxxxxx> wrote:
> > On Thu, Oct 15, 2026 at 10:55 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
> > > On Thu, Oct 15, 2026 at 10:11:04PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > On Wed, Oct 14, 2026 at 10:15 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
> > > > > On Wed, Oct 14, 2026 at 09:07:54PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > > On Thu, Oct 1, 2026 at 5:51 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
> > > > > > >
> 
> [cut]

[ Thanks for the earlier clarification about PM domain. It helps me
  understand where you're coming from. ]

> > > I kinda think the point of most of this power domain architecture is
> > > that a typical driver just doesn't have to worry about it at all (in
> > > probe(), and if you use runtime PM properly), so I'd prefer not
> > > mentioning it in the Basic example. But sure.
> >
> > Well, your basic example needs to be put in a certain context because
> > it may not be generally applicable everywhere.
> >
> > I think that you should make it clear what the driver's expectations
> > regarding the physical state of the device at the probe time are and
> > the general rule is that it is better to leave the device in the same
> > state in which it is expected to be found next time.

Ack, will do to the best of my ability.

> To be somewhat more precise, if the driver expects to find the device
> completely off and the runtime PM callbacks are set up to make it
> operational, runtime PM can be used to power it up in probe:
> 
> /* Assume the runtime PM status to be "suspended". */
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
> 
> but then in remove or on probe errors the "power off" state needs to
> be restored manually:
> 
> pm_runtime_disable(dev);
> /* Allow the parent and suppliers (if any) to suspend. */
> pm_runtime_set_suspended(dev);
> device_specific_power_off(dev);

That's ... precisely the second example, "Probe with Hardware Powered
Off". It's couched more in my non-ACPI background, but it's the same.
And it mostly avoids pseudocode, so it should be a bit more clear than
the "Basic Probe" we've been talking about.

But there, I still propose that the main distinction is about what work
needs done in the driver in question. The PM domain is still mostly
irrelevant to the driver writer, except perhaps in the language I use
about "on", "off", "operational", etc. And yes, sometimes that means the
work needed to "become operational" is minimal/empty; but that seems
like an obvious reduction from "do what's needed to power on" (nothing)
and "undo what you did on the way up" (nothing).

> However, if the driver expects to find the device operational (for
> example, because it assumes the ACPI PM domain to be there), it may
> just update the runtime PM status before enabling runtime PM:
> 
> /* Assume the runtime PM status to be "suspended". */
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> 
> but then in remove or on probe errors it may just leave the device in
> its current physical state (whatever it is) and assume somebody else
> to take care of turning it off subsequently:
> 
> pm_runtime_disable(dev);
> /* Allow the parent and suppliers (if any) to suspend. */
> pm_runtime_set_suspended(dev);
> 
> These are two distinct cases that are rather hard to cover in one
> simple example.

To me, this case is barely different than my existing "Basic Probe"
case, except that the "/* Power on and initialize hardware registers... */"
(or "foo_initialize()") is empty, and so the setup and teardown becomes
smaller.

So I still think my first 2 example cases are perfectly sufficient to
cover what you're talking about. I can just improve a bit of the
language and clarity w.r.t. the things you've mentioned. For example, I
can clarify that the driver might need to power on resources or itself,
or the PM domain might have done that for you -- regardless, one needs
to tear down what they set up (which is nothing but the
pm_runtime_set_[status](), in some cases).

Brian