Re: [PATCH v3 03/11] power: sequencing: Add pwrseq_power_is_controllable() API

[Loic Poulain]

On Thu, Jul 16, 2026 at 4:22 PM Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote:
>
> On Fri, Jul 10, 2026 at 06:13:55AM -0700, Bartosz Golaszewski wrote:
> > On Fri, 10 Jul 2026 11:57:29 +0200, Loic Poulain
> > <loic.poulain@xxxxxxxxxxxxxxxx> said:
> > > On some boards a power sequencing target has no host-controllable enable
> > > for its function, for instance when the enable line is not wired up to a
> > > GPIO and is hardwired to an always-on level. The pcie-m2 "uart" target is
> > > one such example: when the M.2 connector does not route the W_DISABLE2#
> > > signal to a host GPIO, its enable/disable are no-ops and the host cannot
> > > gate the Bluetooth function at all or exclusively.
> > >
> > > Add a generic pwrseq_power_is_controllable() helper. It reports whether the
> > > target's final unit provides a host-controllable dedicated power actuator.
> > > The unit can implement a new optional per-unit is_controllable() callback,
> > > reporting whether that actuator is effective on this instance (for example
> > > depending on GPIO presence). If the unit does not provide the callback, it
> > > is assumed to be controllable.
> > >
> > > Note this only describes the target's own enable actuator. It does not
> > > imply that a power-off reaches an electrical OFF state as a target may
> > > have multiple consumers. Also, this does not restrict consumers from
> > > calling pwrseq_power_off() either, which remains valid to drop a vote
> > > on shared unit resources/dependencies.
> > >
> >
> > Thanks. In general it looks good, though I would extend the last paragraph by
> > mentioning that the fact that the target's final unit doesn't guarantee power
> > control for the host, doesn't mean that the power is not controlled for its
> > dependencies. This is what you basically said but it may not be clear to
> > someone new to this API what a "vote on shared unit resources" exactly means.
> >
> > > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/power/sequencing/core.c | 39 +++++++++++++++++++++++++++++++++++++++
> > >  include/linux/pwrseq/consumer.h |  7 +++++++
> > >  include/linux/pwrseq/provider.h |  9 +++++++++
> > >  3 files changed, 55 insertions(+)
> > >
> > > diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> > > index 02f42da915985339d3de507fc36dd158b0035a99..35df55312a71e9dfd3f24a8199b539746466af36 100644
> > > --- a/drivers/power/sequencing/core.c
> > > +++ b/drivers/power/sequencing/core.c
> > > @@ -72,6 +72,8 @@ static DECLARE_RWSEM(pwrseq_sem);
> > >   *          this unit.
> > >   * @disable: Callback running the part of the power-off sequence provided
> > >   *           by this unit.
> > > + * @is_controllable: Optional callback reporting whether this unit's
> > > + *                   enable/disable actually control power.
> > >   * @enable_count: Current number of users that enabled this unit. May be the
> > >   *                consumer of the power sequencer or other units that depend
> > >   *                on this one.
> > > @@ -83,6 +85,7 @@ struct pwrseq_unit {
> > >     struct list_head deps;
> > >     pwrseq_power_state_func enable;
> > >     pwrseq_power_state_func disable;
> > > +   pwrseq_is_controllable_func is_controllable;
> >
> > Is there any reason not to put it in struct pwrseq_target? That would avoid
> > needless duplication of NULL pointers across all units, right?
> >
> > >     unsigned int enable_count;
> > >  };
> > >
> > > @@ -104,6 +107,7 @@ static struct pwrseq_unit *pwrseq_unit_new(const struct pwrseq_unit_data *data)
> > >     INIT_LIST_HEAD(&unit->deps);
> > >     unit->enable = data->enable;
> > >     unit->disable = data->disable;
> > > +   unit->is_controllable = data->is_controllable;
> > >
> > >     return unit;
> > >  }
> > > @@ -991,6 +995,41 @@ struct device *pwrseq_to_device(struct pwrseq_desc *desc)
> > >  }
> > >  EXPORT_SYMBOL_GPL(pwrseq_to_device);
> > >
> > > +/**
> > > + * pwrseq_power_is_controllable() - Check whether the target provides a
> > > + *                                  host-controllable power actuator.
> > > + * @desc: Descriptor referencing the power sequencer.
> > > + *
> > > + * Some power sequencing targets provide no host-controllable enable for their
> > > + * function on a given board, for instance when the enable line is not wired up
> > > + * and is instead hardwired to an always-on level. For such targets a call to
> > > + * pwrseq_power_off() is still allowed, so that the consumer can drop its vote
> > > + * on the (possibly shared) resources, but the host cannot gate the function
> > > + * on its own.
> > > + *
> > > + * Returns:
> > > + * True if the target provides a host-controllable power actuator, false
> > > + * otherwise. Also returns false if @desc is NULL.
> > > + */
> > > +bool pwrseq_power_is_controllable(struct pwrseq_desc *desc)
> >
> > I think, we should call it simply pwrseq_is_controllable(). I was thinking
> > about pwrseq_target_is_controllable() but it's redundant: a pwrseq handle is
> > already associated with a concrete target. When you say "power" it suggests
> > a concrete thing that's "controllable" but it may be a ragulator, it may be
> > a GPIO or reset. I'd just go with pwrseq_is_controllable().
> >
>
> But what does pwrseq_is_controllable() mean to the clients? What is
> controllable? pwrseq itself is controllable? If so, then what does
> pwrseq_power_{on/off} provide?
>
> pwrseq_power_is_controllable() also suffers from the same ambiguity. The goal
> of this API is to tell the clients whether a specific GPIO or some signal is
> available that enables/disables the radio on the device. But that is not same as
> controlling power to the sequencer, which is controlled by the regulator.

By "controllable" it means: can the host toggle this target's own
power by itself to gate the function? Or is it out of the host's
direct control, which doesn't imply pwrseq_power_off() has no
'influence' on the power state.

> But TBH, I don't have any suggestion for the generic API name :/

Well, unless we come up with a better description/naming, I'll stick
with Bartosz's proposed name for now, and we can revisit the naming
later if a better one comes up.

Regards,
Loic