Message-Id: <498AB71C-58EF-487E-8D9B-C7C113862948@collabora.com>
Date: Sun, 18 May 2025 12:11:23 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alexandre Courbot <acourbot@...dia.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 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?

> 
> 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.

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?

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.

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

> - 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.

> 
> 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.

> <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.

That is true.

— Daniel

Powered by blists - more mailing lists