Message-Id: <D9YXK1J1XO37.JVILKENRKYXD@nvidia.com>
Date: Sun, 18 May 2025 11:28:01 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "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

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.

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.

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.

<snip>
> +impl EnabledRegulator {
> +    fn as_ptr(&self) -> *mut bindings::regulator {
> +        self.inner.inner.as_ptr()
> +    }
> +
> +    /// Disables the regulator.
> +    pub fn disable(self) -> Result<Regulator> {

This has been mentioned, but I think you will want to return `self` in
case of failure. Granted, most users won't do anything with it and fail,
but this is allowed by the C API and we shouldn't restrict it.

Powered by blists - more mailing lists