Message-Id: <D9Z4XGQ2QHXA.2H5X1NZ5IZECC@nvidia.com>
Date: Sun, 18 May 2025 17:14:41 +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 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).

> Also what happens when I call `disable` without any enable calls
> before?

The C API displays a warning an returns -EIO, which sounds like a
reasonable behavior for Rust to proxy.

Powered by blists - more mailing lists