Message-Id: <D9Z73XZUSYWO.R0P38ASITWR7@kernel.org>
Date: Sun, 18 May 2025 11:57:11 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Alexandre Courbot" <acourbot@...dia.com>, "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 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`. Also we should clearly
document that the `Enabled`/`Disabled` typestates should be preferred if
possible.

---
Cheers,
Benno

> 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