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

[Bartosz Golaszewski]

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().

> +{
> +	struct pwrseq_unit *unit;
> +

might_sleep();

> +	if (!desc)
> +		return false;
> +

Please follow the pattern in other functions where we guard against the
device's concurrent removal with pwrseq->rw_lock.

> +	unit = desc->target->unit;
> +
> +	if (!unit->enable && !unit->disable)
> +		return false;
> +
> +	if (!unit->is_controllable)
> +		return true;
> +
> +	return unit->is_controllable(desc->pwrseq);
> +}
> +EXPORT_SYMBOL_GPL(pwrseq_power_is_controllable);
> +
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>
>  struct pwrseq_debugfs_count_ctx {
> diff --git a/include/linux/pwrseq/consumer.h b/include/linux/pwrseq/consumer.h
> index 3c907c9e1885dc2958043a9a733fbe20bdf95f6e..ea2b87a521bceb7fb51e79c3b03fdb50f38bf94f 100644
> --- a/include/linux/pwrseq/consumer.h
> +++ b/include/linux/pwrseq/consumer.h
> @@ -25,6 +25,8 @@ int pwrseq_power_off(struct pwrseq_desc *desc);
>
>  struct device *pwrseq_to_device(struct pwrseq_desc *desc);
>
> +bool pwrseq_power_is_controllable(struct pwrseq_desc *desc);
> +
>  #else /* CONFIG_POWER_SEQUENCING */
>
>  static inline struct pwrseq_desc * __must_check
> @@ -58,6 +60,11 @@ static inline struct device *pwrseq_to_device(struct pwrseq_desc *desc)
>  	return NULL;
>  }
>
> +static inline bool pwrseq_power_is_controllable(struct pwrseq_desc *desc)
> +{
> +	return false;
> +}
> +
>  #endif /* CONFIG_POWER_SEQUENCING */
>
>  #endif /* __POWER_SEQUENCING_CONSUMER_H__ */
> diff --git a/include/linux/pwrseq/provider.h b/include/linux/pwrseq/provider.h
> index 33b3d2c2e39decafac6c6fca9254ad4329d90e94..42c7a37355869ecd0ae8e59b3a8c8716c7ff9ce8 100644
> --- a/include/linux/pwrseq/provider.h
> +++ b/include/linux/pwrseq/provider.h
> @@ -6,12 +6,15 @@
>  #ifndef __POWER_SEQUENCING_PROVIDER_H__
>  #define __POWER_SEQUENCING_PROVIDER_H__
>
> +#include <linux/types.h>
> +
>  struct device;
>  struct module;
>  struct pwrseq_device;
>
>  typedef int (*pwrseq_power_state_func)(struct pwrseq_device *);
>  typedef int (*pwrseq_match_func)(struct pwrseq_device *, struct device *);
> +typedef bool (*pwrseq_is_controllable_func)(struct pwrseq_device *);
>
>  #define PWRSEQ_NO_MATCH 0
>  #define PWRSEQ_MATCH_OK 1
> @@ -26,12 +29,18 @@ typedef int (*pwrseq_match_func)(struct pwrseq_device *, struct device *);
>   *          this unit.
>   * @disable: Callback running the part of the power-off sequence provided
>   *           by this unit.
> + * @is_controllable: Optional callback returning whether this unit's

s/returning/checking/

> + *                   enable/disable callbacks actually control power on this
> + *                   instance (for example when the controlling GPIO is wired

s/instance/unit/g

> + *                   up). If not provided, the unit's power is assumed to be
> + *                   always controllable.
>   */
>  struct pwrseq_unit_data {
>  	const char *name;
>  	const struct pwrseq_unit_data **deps;
>  	pwrseq_power_state_func enable;
>  	pwrseq_power_state_func disable;
> +	pwrseq_is_controllable_func is_controllable;
>  };
>
>  /**
>
> --
> 2.34.1
>
>

Bart