Message-Id: <D9ZQUUA4FLXD.19MJI9HD48EMZ@nvidia.com>
Date: Mon, 19 May 2025 10:25:40 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "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>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3] rust: regulator: add a bare minimum regulator
 abstraction

Hi Daniel,

On Mon May 19, 2025 at 12:11 AM JST, Daniel Almeida wrote:
> Hi Alex, thanks for looking at this!
>
>> On 17 May 2025, at 23:28, Alexandre Courbot <acourbot@...dia.com> wrote:
>> 
>> Hi Daniel,
>> 
>> On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
>>> Add a bare minimum regulator abstraction to be used by Rust drivers.
>>> This abstraction adds a small subset of the regulator API, which is
>>> thought to be sufficient for the drivers we have now.
>>> 
>>> Regulators provide the power needed by many hardware blocks and thus are
>>> likely to be needed by a lot of drivers.
>>> 
>>> It was tested on rk3588, where it was used to power up the "mali"
>>> regulator in order to power up the GPU.
>>> 
>>> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
>>> ---
>>> Changes in v3:
>>> - Rebased on rust-next
>>> - Added examples to showcase the API
>>> - Fixed some rendering issues in the docs
>>> - Exposed {get|set}_voltage for both Regulator and EnabledRegulator
>>> - Derived Clone, Copy, PartialEq and Eq for Microvolt
>>> - Link to v2: https://lore.kernel.org/r/20250326-topics-tyr-regulator-v2-1-c0ea6a861be6@collabora.com
>>> 
>>> Resend v2:
>>>  - cc Regulator maintainers
>>> Changes from v1:
>>>  - Rebased on rust-next
>>>  - Split the design into two types as suggested by Alice Ryhl.
>>>  - Modify the docs to highlight how users can use kernel::types::Either
>>>    or an enum to enable and disable the regulator at runtime.
>>>  - Link to v1: https://lore.kernel.org/rust-for-linux/20250219162517.278362-1-daniel.almeida@collabora.com/
>>> ---
>>> rust/bindings/bindings_helper.h |   1 +
>>> rust/kernel/lib.rs              |   2 +
>>> rust/kernel/regulator.rs        | 211 ++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 214 insertions(+)
>>> 
>>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>>> index ab37e1d35c70d52e69b754bf855bc19911d156d8..e14cce03338ef5f6a09a23fd41ca47b8c913fa65 100644
>>> --- a/rust/bindings/bindings_helper.h
>>> +++ b/rust/bindings/bindings_helper.h
>>> @@ -31,6 +31,7 @@
>>> #include <linux/poll.h>
>>> #include <linux/property.h>
>>> #include <linux/refcount.h>
>>> +#include <linux/regulator/consumer.h>
>>> #include <linux/sched.h>
>>> #include <linux/security.h>
>>> #include <linux/slab.h>
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 28007be98fbad0e875d7e5345e164e2af2c5da32..c8fd7e4e036e9e5b6958acf0dcfa952b916a3d48 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -86,6 +86,8 @@
>>> pub mod prelude;
>>> pub mod print;
>>> pub mod rbtree;
>>> +#[cfg(CONFIG_REGULATOR)]
>>> +pub mod regulator;
>>> pub mod revocable;
>>> pub mod security;
>>> pub mod seq_file;
>>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
>>> --- /dev/null
>>> +++ b/rust/kernel/regulator.rs
>>> @@ -0,0 +1,211 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! 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.
>
> I am not sure I understand what you mean by “dynamic use cases”.
>
> Can you expand a bit on that?

I just mean the cases where users will want to enable and disable the
regulator more frequently than just enabling it at probe time.

>
>> 
>> 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.
>
> Dynamic is just "Regulator" in the current version of this patch. There is no
> "Disabled" because there is no guarantee that someone else won't enable the
> regulator, trivially breaking this invariant at any moment.

There is a core difference, which is that in your version of
`Regulator`, `enable` takes ownership of `self` and returns a different
type, whereas `Dynamic` would take `&mut self` and change its internal
state, like the C API does.

>
> The only thing we can guarantee is "Enabled", through our own call to
> "regulator_enable()".
>
> In fact, for the typestate solution, I was thinking about "UnknownState" and
> "Enabled", or any nomenclature along these lines.
>
>> 
>> 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.
>
> Why not “enable()” as we currently have?

`enable()` to me sounds like a method that mutates `self` in-place,
whereas your version consumes it and turns it into a different type.
Such methods are typically named `into_*`, or `try_into_*` when they can
fail.

Actually, this could even be a `TryInto` implementation, but in this
particular case having methods with the target state in their names may
result in clearer code and allow the reader to model the transition
graph more easily.

>
> If we go with the "Dynamic" nomenclature, and we agree that there's no
> "Disabled",  then we can implement "pub fn enable(self) -> Regulator<Enabled>",
> for "Dynamic", which is what we currently have, but with other names.

Not if we want to provide the behavior of the C consumer API, which
requires multiple calls to `regulator_enable()` to be matched by an equal
number of calls to `regulator_disable()`, which could be useful to some
drivers (lest they reimplement their own counter).

>
> Also, I personally dislike this term: it's hard to tell what Regulator<Dynamic>
> means on a first glance.

Yeah I'm not a fan of it and I'm sure there are better alternatives.
Maybe `Controlled`?

>
>> - 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.
>
> This is already how it works: Regulator is the default and EnabledRegulator
> adds an extra invariant.

Hopefully the above clarified the differences.

>
>> 
>> But for cases where the regulator is known to always be in a specific
>> state like `Enabled`, one can use `Regulator<Enabled>` and simplify
>> their code.
>> 
>> This should retain the compile-time safety that your version proposed
>> for common cases, while still exposing the flexibility of the C API when
>> needed.
>> 
>
> Yeah, I agree.

I think the concept looks better if you consider the generic parameter
of `Regulator` not as a state, but as a marker of guaranteed invariants
and allowed transitions.

- `Regulator<Enabled>` guarantees that the regulator is enabled on the
  consumer side. It is also disabled when the object is dropped. Many
  drivers will just do `Regulator::<Enabled>::get()` at probe time (the
  equivalent of `regulator_get_enable()`), store the object into the
  device's state, and forget about it.

- `Regulator<Disabled>` guarantees that the regulator is not enabled on
  the consumer side. It could be useful to have for drivers that use
  distinct types to store their state depending on their power status:
  the powered-on type would store a `Regulator<Enabled>`, the
  powered-off type a `Regulator<Disabled>`. That way you cannot even
  write code transitioning between the states if you omit the regulator
  - which I think is really neat.

These two should cover a large percentage of consumer needs, but for
those that need more fine-grained control we should also have one or two
policies that follow the C API a bit closer, e.g.:

- `Regulator<Controlled>` (or just `Regulator`): user is granted an API
  very close to the C one, and is responsible for balancing calls to
  `enable()` and `disable()`. Regulator remains in the state it was in
  when dropped.

- `Regulator<Switch>`: calls to `enable()` and `disable()` do not need
  to be balanced, and the regulator always transitions to one state if
  it was in the other. Regulator gets automatically disabled on drop.
  This provides a simpler, safer alternative to `Controlled`.

Note that I am not advocating for including these two policies
specifically, these are just examples. My main point is that we should
also provide a way to change the regulator's enabled state without
requiring a change of type ; which policy(es) to adopt will depend on
which restrictions we conclude are adequate to place on the C API, if
any.

Powered by blists - more mailing lists