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

[Manivannan Sadhasivam <mani@kernel.org>]

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@oss.qualcomm.com> 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@oss.qualcomm.com>
> > ---
> >  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.

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

- Mani

-- 
மணிவண்ணன் சதாசிவம்