Message-Id: <D9Z59JWL4BTC.3DTN0LWCJX5AZ@nvidia.com>
Date: Sun, 18 May 2025 17:30:28 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alexandre Courbot" <acourbot@...dia.com>, "Benno Lossin"
 <lossin@...nel.org>, "Daniel Almeida" <daniel.almeida@...labora.com>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>,
 "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, "Boris Brezillon"
 <boris.brezillon@...labora.com>, "Sebastian Reichel"
 <sebastian.reichel@...labora.com>, "Liam Girdwood" <lgirdwood@...il.com>,
 "Mark Brown" <broonie@...nel.org>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3] rust: regulator: add a bare minimum regulator
 abstraction

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?

Powered by blists - more mailing lists