Message-Id: <D9Z8PLIZGSJ6.254ICGG44E4PB@nvidia.com>
Date: Sun, 18 May 2025 20:12:29 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "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 6:57 PM JST, Benno Lossin wrote:
> On Sun May 18, 2025 at 10:30 AM CEST, Alexandre Courbot wrote:
>> 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.
>
> So just let users ensure that they always match each `enable` call with
> a `disable` call in the `Dynamic` typestate?
>
> That is ok, if no memory issues can arise from forgetting to do so,
> otherwise those functions need to be `unsafe`.

There shouldn't be any, the only side effect would be that the regulator
stays enabled when it shouldn't.

It's also easy to implement more behaviors using more states. For
instance, `Dynamic` just proxies the C API. But if we also think it its
useful to have a regulator which use count is clamped to 0 and 1, you
could have another state that includes a boolean (instead of being empty
lke the others) to track whether the regulator is enabled or not, and an
`enable` method that only calls the C `regulator_enable` if that boolean
is not already true. That way you remove the need to mirror the calls to
enable and disable, while only paying the memory overhead for doing so
when you explicitly state you want this behavior.

But maybe I'm overthinking it. It would be interesting to hear to core
developers' opinion on whether the C API can/should be restricted and
guide the design from that.

> Also we should clearly document that the `Enabled`/`Disabled`
> typestates should be preferred if possible.

Definitely, build-time guarantees should be used whenever possible.

Powered by blists - more mailing lists