Re: [PATCH v3] rust: regulator: add a bare minimum regulator abstraction

["Alexandre Courbot" <acourbot@nvidia.com>]

On Sun May 18, 2025 at 5:14 PM JST, Alexandre Courbot wrote:
> On Sun May 18, 2025 at 4:19 PM JST, Benno Lossin wrote:
>> On Sun May 18, 2025 at 4:28 AM CEST, Alexandre Courbot wrote:
>>> On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
>>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>>> +//! voltage and current regulators.
>>>> +//!
>>>> +//! The intention is to allow systems to dynamically control regulator power
>>>> +//! output in order to save power and prolong battery life. This applies to both
>>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>>> +//! (where current limit is controllable).
>>>> +//!
>>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>>> +//!
>>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>>> +//! [`EnabledRegulator`].
>>>> +//!
>>>> +//! The transition between these types is done by calling
>>>> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
>>>> +//!
>>>> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
>>>> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
>>>> +//! otherwise.
>>>
>>> Having the enabled or disabled state baked into the type is indeed
>>> valuable for drivers that just need to acquire and enable a regulator at
>>> probe time. However, there are also more dynamic use cases and I don't
>>> think the burden of managing this aspect - by either performing a manual
>>> match to call any method (even the shared ones), or implementing custom
>>> dispatch types (which will lead to many similar ad-hoc implementations)
>>> - should fall on the user. Thus I strongly suggest that this module
>>> provides a solution for this as well.
>>>
>>> It has been proposed earlier to use a typestate, and this would indeed
>>> provide several benefits, the first one being the ability to have shared
>>> impl blocks (and shared documentation) between the enabled and disabled
>>> states for methods like set/get_voltage().
>>>
>>> But the key benefit I see is that it could also address the
>>> aforementioned dynamic management problem through the introduction of a
>>> third state.
>>>
>>> Alongside the `Enabled` and `Disabled` states, there would be a third
>>> state (`Dynamic`?) in which the regulator could either be enabled or
>>> disabled. This `Dynamic` state is the only one providing `enable` and
>>> `disable` methods (as well as `is_enabled`) to change its operational
>>> state without affecting its type.
>>>
>>> All three states then implement `set_voltage` and `get_voltage` through
>>> a common impl block, that could be extended with other methods from the
>>> C API that are independent of the state, as needed.
>>>
>>> To handle typestate transitions:
>>>
>>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>>   method to transition the regulator to the `Enabled` state.
>>> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
>>> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>>>   fail).
>>>
>>> Essentially, the `Enabled` and `Disabled` states simply enforce an
>>> additional operational state invariant on the underlying regulator, and
>>> do not provide methods to change it.
>>>
>>> The `Dynamic` state would be the default for `Regulator`, so by just
>>> using `Regulator`, the user gets an interface that works very similarly
>>> to the C API it abstracts, making it intuitive to those familiar with
>>> it.
>>
>> How will the `Dynamic` typestate track the enable refcount? AFAIK one
>> has to drop all enable refcounts before removing the regulator.
>
> I guess a choice has to be made about whether to just proxy the C API
> as-is (where an unbalanced number of enable/disable calls can result in
> a dropped regulator still being enabled), or whether to clamp the number
> of times a Rust consumer can enable a regulator to 0 and 1 and disable
> an enabled regulator in the destructor.
>
> The initial proposal does such clamping by design, but I also suspect
> the C API behave like it does for good reasons (which I am not familiar
> enough to be aware of unfortunately).

Well after thinking a bit more about it, it is clear that is does that
because a single consumer may need to ensure a regulator is on across
multiple internal states. I suspect we will have Rust drivers complex
enough to benefit from this behavior sometime soon.

So I'd say the `Dynamic` state should probably mirror the C API as
closely as possible and not try to outsmart the user. The
`Enabled`/`Disabled` typestates will cover the simpler use-cases
perfectly well and ensure a well-controlled enable count.

I guess this also means transitions to/from `Dynamic` and the other
states will have to be limited to the ones where we can clearly infer
the enable count. That's probably ok anyway because I can't think of a
reason to switch from one pattern to the other for the same regulator.
Maybe we don't even need these transitions at all?