Re: [PATCH v5 03/16] power: sequencing: Add pwrseq_power_is_on()

[Bartosz Golaszewski]

On Thu, 16 Jul 2026 06:59:01 +0200, Chen-Yu Tsai <wenst@xxxxxxxxxxxx> said:
> On Wed, Jul 15, 2026 at 8:10 PM Bartosz Golaszewski <brgl@xxxxxxxxxx> wrote:
>>
>> On Wed, 15 Jul 2026 11:29:02 +0200, Andy Shevchenko
>> <andriy.shevchenko@xxxxxxxxxxxxxxx> said:
>> > On Wed, Jul 15, 2026 at 04:53:33PM +0800, Chen-Yu Tsai wrote:
>> >> The power sequencing consumer API already does power on state tracking
>> >> internally. Expose the state to consumers through pwrseq_power_is_on()
>> >> so that they don't have to reimplement it locally.
>> >
>> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>> >
>> > ...
>> >
>> >> +/**
>> >> + * pwrseq_power_is_on() - Queries the last requested state of the power sequencer.
>> >> + * @desc: Descriptor referencing the power sequencer.
>> >> + *
>> >> + * This returns the last requested state of the power sequencer.
>> >> + *
>> >> + * Returns:
>> >> + * On success, 1 for on or desc is NULL (optional) and 0 for off;
>> >> + * negative error number on failure.
>> >
>> > I would rephrase it a bit.
>> >
>> >  * On success, 1 for on and 0 for off; negative error number on failure.
>> >  * If desc is NULL (means optional) return 1.
>> >
>> > And this rises a question: why 1? Shouldn't it be some "unknown" state?
>> > (But since Bart Acked this, this doesn't prevent the patch to go, you
>> >  got my tag above.)
>> >
>>
>> No, this is good feedback. Maybe we should prefer an enum like so:
>>
>> enum pwrseq_state {
>>         PWRSEQ_POWER_UNKNOWN,
>>         PWRSEQ_POWER_ON,
>>         PWRSEQ_POWER_OFF
>> };
>
> Well I think the crux of it is whether the API considers the descriptor
> optional. The power on/off functions suggest that is the case.
>

True. That may be an inconsistency of the API contract on my part. There's no
such thing as an optional variant of pwrseq_get() so perhaps pwrseq_power_on()
and pwrseq_power_off() should be converted to returning -EINVAL on NULL
descriptor.

> I modeled this on reset_control_status(). I believe the rationale is that
> if the reset control or pwrseq is optional, then either there is some
> other mechanism to deal with the reset (GPIO) or pwrseq (GPIO + regulator),
> or it's always on and operational.
>
> In such cases and the rare case where one has to query the state, the
> queries are possibly chained together and I think it makes sense to
> assume the "operational" state as the fallback.
>
> If this returns an enum, then I think the previous approach of returning
> an error value for an invalid descriptor is better. One doesn't need to
> learn a new set of return values.
>
> I'm fine either way. I would just move the checks over to the consumer
> side. But I think the principles guiding the overall API design is
> something you need to provide:
>
>   - Is the descriptor optional? What should the action functions
>     (power on / off , or query status) return when NULL is given?
>   - Is the subsystem optional? Should the stub functions besides
>     pwrseq_get() return 0 instead of -ENOSYS?
>
> Right now the API feels like the descriptor is optional but the
> subsystem is not. One has to add IS_ENABLED(CONFIG_POWER_SEQUENCING)
> around the consumer API to make it an optional feature.
>

There's one more use-case where the power state trully can be *unknown*.
Please look at the proposed new function from Loic[1]. Here we deal with
targets over which linux has no direct power control even though it may still
control some of its dependencies. This is where an UNKNOWN state may make
sense.

This is where I also floated an idea of renaming pwrseq_power_on/off() to
pwrseq_vote_on/off() for better clarity of their purpose.

Thanks,
Bartosz

[1] https://lore.kernel.org/all/20260710-monza-wireless-v3-3-46253587af64@xxxxxxxxxxxxxxxx/