[PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Daniel Almeida]

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@collabora.com>
---
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.
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/regulator.rs        | 127 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ccb988340df69c84a702fe39a09addcc2663aebe..374f48b5ce2a602b4d1a5791201514ed8a535844 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -30,6 +30,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 ba0f3b0297b27dbda6a7b5d9ef8fdb8b7e6463dc..5b3228e8c80b1eb33bf36929ce3671b982efaf4a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -72,6 +72,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..4ac9b6c537dff4cfc7f2f99d48aec3cecc3151e8
--- /dev/null
+++ b/rust/kernel/regulator.rs
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Regulator abstractions.
+//!
+//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
+//!
+//! Regulators are modeled 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.
+
+use crate::{
+    bindings,
+    device::Device,
+    error::{from_err_ptr, to_result, Result},
+    prelude::*,
+};
+
+use core::{mem::ManuallyDrop, ptr::NonNull};
+
+/// A `struct regulator` abstraction.
+///
+/// # Invariants
+///
+/// - [`Regulator`] is a non-null wrapper over a pointer to a `struct
+///   regulator` obtained from `regulator_get()`.
+/// - Each instance of [`Regulator`] is associated with a single count of `regulator_get()`.
+pub struct Regulator {
+    inner: NonNull<bindings::regulator>,
+}
+
+impl Regulator {
+    /// Obtains a [`Regulator`] instance from the system.
+    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
+        // SAFETY: It is safe to call `regulator_get()`, on a device pointer
+        // received from the C code.
+        let inner = from_err_ptr(unsafe { bindings::regulator_get(dev.as_raw(), name.as_ptr()) })?;
+
+        // SAFETY: We can safely trust `inner` to be a pointer to a valid
+        // regulator if `ERR_PTR` was not returned.
+        let inner = unsafe { NonNull::new_unchecked(inner) };
+
+        Ok(Self { inner })
+    }
+
+    /// Enables the regulator.
+    pub fn enable(self) -> Result<EnabledRegulator> {
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        let res = to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) });
+        res.map(|()| EnabledRegulator { inner: self })
+    }
+}
+
+impl Drop for Regulator {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference,
+        // so it is safe to relinquish it now.
+        unsafe { bindings::regulator_put(self.inner.as_ptr()) };
+    }
+}
+
+/// A `struct regulator` abstraction that is known to be enabled.
+///
+/// # Invariants
+///
+/// - [`EnabledRegulator`] is a valid regulator that has been enabled.
+/// - Each instance of [`EnabledRegulator`] is associated with a single count
+///   of `regulator_enable()`.
+pub struct EnabledRegulator {
+    inner: Regulator,
+}
+
+impl EnabledRegulator {
+    fn as_ptr(&self) -> *mut bindings::regulator {
+        self.inner.inner.as_ptr()
+    }
+
+    /// Disables the regulator.
+    pub fn disable(self) -> Result<Regulator> {
+        // Keep the count on `regulator_get()`.
+        let regulator = ManuallyDrop::new(self);
+
+        // SAFETY: Safe as per the type invariants of `Self`.
+        let res = to_result(unsafe { bindings::regulator_disable(regulator.as_ptr()) });
+
+        res.map(|()| Regulator {
+            inner: regulator.inner.inner,
+        })
+    }
+
+    /// Sets the voltage for the regulator.
+    pub fn set_voltage(&self, min_uv: Microvolt, max_uv: Microvolt) -> Result {
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        to_result(unsafe { bindings::regulator_set_voltage(self.as_ptr(), min_uv.0, max_uv.0) })
+    }
+
+    /// Gets the current voltage of the regulator.
+    pub fn get_voltage(&self) -> Result<Microvolt> {
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        let voltage = unsafe { bindings::regulator_get_voltage(self.as_ptr()) };
+        if voltage < 0 {
+            Err(Error::from_errno(voltage))
+        } else {
+            Ok(Microvolt(voltage))
+        }
+    }
+}
+
+impl Drop for EnabledRegulator {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference,
+        // so it is safe to relinquish it now.
+        unsafe { bindings::regulator_disable(self.as_ptr()) };
+    }
+}
+
+/// A voltage in microvolts.
+///
+/// The explicit type is used to avoid confusion with other multiples of the
+/// volt, which can be desastrous.
+#[repr(transparent)]
+pub struct Microvolt(pub i32);

---
base-commit: e6ea10d5dbe082c54add289b44f08c9fcfe658af
change-id: 20250326-topics-tyr-regulator-e8b98f6860d7

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2558 bytes --]

On Wed, Mar 26, 2025 at 03:39:33PM -0300, Daniel Almeida wrote:

This is flagged as a resend but appears to be the first copy I've
seen...

> 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 are modeled with two types: [`Regulator`] and
> +//! [`EnabledRegulator`].

It would be helpful to see what the client code actually looks like
here.

> +    /// Enables the regulator.
> +    pub fn enable(self) -> Result<EnabledRegulator> {
> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        let res = to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) });
> +        res.map(|()| EnabledRegulator { inner: self })
> +    }

I assume this map does soemthing to report errors?

> +impl EnabledRegulator {

> +    /// Disables the regulator.
> +    pub fn disable(self) -> Result<Regulator> {
> +        // Keep the count on `regulator_get()`.
> +        let regulator = ManuallyDrop::new(self);
> +
> +        // SAFETY: Safe as per the type invariants of `Self`.
> +        let res = to_result(unsafe { bindings::regulator_disable(regulator.as_ptr()) });
> +
> +        res.map(|()| Regulator {
> +            inner: regulator.inner.inner,
> +        })
> +    }

This looks like user code could manually call it which feels like asking
for trouble?

> +    /// Sets the voltage for the regulator.
> +    pub fn set_voltage(&self, min_uv: Microvolt, max_uv: Microvolt) -> Result {
> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        to_result(unsafe { bindings::regulator_set_voltage(self.as_ptr(), min_uv.0, max_uv.0) })
> +    }

set_voltage() is only implemented for enabled regulators.  It is
reasonable for a driver to want to set the voltage for a regulator prior
to enabling it in order to ensure that the device powers up cleanly,
there may be different requirements for power on from on and idle so the
constraints may not be enough to ensure that a device can power on
cleanly.

> +    /// Gets the current voltage of the regulator.
> +    pub fn get_voltage(&self) -> Result<Microvolt> {
> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        let voltage = unsafe { bindings::regulator_get_voltage(self.as_ptr()) };
> +        if voltage < 0 {
> +            Err(Error::from_errno(voltage))
> +        } else {
> +            Ok(Microvolt(voltage))
> +        }
> +    }

Same issue here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Daniel Almeida]

> On 26 Mar 2025, at 15:56, Mark Brown <broonie@kernel.org> wrote:
> 
> On Wed, Mar 26, 2025 at 03:39:33PM -0300, Daniel Almeida wrote:
> 
> This is flagged as a resend but appears to be the first copy I've
> seen...

Hi Mark, you were not on cc earlier this morning, which is why this is being
resent. I left a comment about this, but apparently b4 did not pick it up.

>> 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 are modeled with two types: [`Regulator`] and
>> +//! [`EnabledRegulator`].
> 
> It would be helpful to see what the client code actually looks like
> here.

Ack, I'll include examples in v3.

> 
>> +    /// Enables the regulator.
>> +    pub fn enable(self) -> Result<EnabledRegulator> {
>> +        // SAFETY: Safe as per the type invariants of `Regulator`.
>> +        let res = to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) });
>> +        res.map(|()| EnabledRegulator { inner: self })
>> +    }
> 
> I assume this map does soemthing to report errors?

map() returns the error to the caller, if any. Notice that the return type is
Result<EnabledRegulator>.

> 
>> +impl EnabledRegulator {
> 
>> +    /// Disables the regulator.
>> +    pub fn disable(self) -> Result<Regulator> {
>> +        // Keep the count on `regulator_get()`.
>> +        let regulator = ManuallyDrop::new(self);
>> +
>> +        // SAFETY: Safe as per the type invariants of `Self`.
>> +        let res = to_result(unsafe { bindings::regulator_disable(regulator.as_ptr()) });
>> +
>> +        res.map(|()| Regulator {
>> +            inner: regulator.inner.inner,
>> +        })
>> +    }
> 
> This looks like user code could manually call it which feels like asking
> for trouble?

Yes, user code can call this. My understanding is that drivers may want to
disable the regulator at runtime, possibly to save power when the device is
idle?

What trouble are you referring to?


>> +    /// Sets the voltage for the regulator.
>> +    pub fn set_voltage(&self, min_uv: Microvolt, max_uv: Microvolt) -> Result {
>> +        // SAFETY: Safe as per the type invariants of `Regulator`.
>> +        to_result(unsafe { bindings::regulator_set_voltage(self.as_ptr(), min_uv.0, max_uv.0) })
>> +    }
> 
> set_voltage() is only implemented for enabled regulators.  It is
> reasonable for a driver to want to set the voltage for a regulator prior
> to enabling it in order to ensure that the device powers up cleanly,
> there may be different requirements for power on from on and idle so the
> constraints may not be enough to ensure that a device can power on
> cleanly.
> 
>> +    /// Gets the current voltage of the regulator.
>> +    pub fn get_voltage(&self) -> Result<Microvolt> {
>> +        // SAFETY: Safe as per the type invariants of `Regulator`.
>> +        let voltage = unsafe { bindings::regulator_get_voltage(self.as_ptr()) };
>> +        if voltage < 0 {
>> +            Err(Error::from_errno(voltage))
>> +        } else {
>> +            Ok(Microvolt(voltage))
>> +        }
>> +    }
> 
> Same issue here.

Ok, we can move the implementation to Regulator then.

— Daniel

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On Wed, Mar 26, 2025 at 04:49:26PM -0300, Daniel Almeida wrote:
> > On 26 Mar 2025, at 15:56, Mark Brown <broonie@kernel.org> wrote:

> >> +    /// Disables the regulator.
> >> +    pub fn disable(self) -> Result<Regulator> {
> >> +        // Keep the count on `regulator_get()`.
> >> +        let regulator = ManuallyDrop::new(self);

> > This looks like user code could manually call it which feels like asking
> > for trouble?

> Yes, user code can call this. My understanding is that drivers may want to
> disable the regulator at runtime, possibly to save power when the device is
> idle?

> What trouble are you referring to?

My understanding was that the enable was done by transforming a
Regulator into an EnabledRegulator but if you can explicitly call
disable() on an EnabledRegulator without destroying it then you've got
an EnabledRegulator which isn't actually enabled.  Perhaps it's not
clear to me how the API should work?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Maud Spierings]

On 3/27/25 12:32, Mark Brown wrote:
> On Wed, Mar 26, 2025 at 04:49:26PM -0300, Daniel Almeida wrote:
>>> On 26 Mar 2025, at 15:56, Mark Brown <broonie@kernel.org> wrote:
> 
>>>> +    /// Disables the regulator.
>>>> +    pub fn disable(self) -> Result<Regulator> {
>>>> +        // Keep the count on `regulator_get()`.
>>>> +        let regulator = ManuallyDrop::new(self);
> 
>>> This looks like user code could manually call it which feels like asking
>>> for trouble?
> 
>> Yes, user code can call this. My understanding is that drivers may want to
>> disable the regulator at runtime, possibly to save power when the device is
>> idle?
> 
>> What trouble are you referring to?
> 
> My understanding was that the enable was done by transforming a
> Regulator into an EnabledRegulator but if you can explicitly call
> disable() on an EnabledRegulator without destroying it then you've got
> an EnabledRegulator which isn't actually enabled.  Perhaps it's not
> clear to me how the API should work?

 From my understanding, disable() takes ownership of self, it does not 
take a reference, so the EnabledRegulator is consumed and the Regulator 
is returned through the result. So EnabledRegulator will get dropped in 
this function which owns it.

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 758 bytes --]

On Thu, Mar 27, 2025 at 12:36:23PM +0100, Maud Spierings wrote:
> On 3/27/25 12:32, Mark Brown wrote:

> > My understanding was that the enable was done by transforming a
> > Regulator into an EnabledRegulator but if you can explicitly call
> > disable() on an EnabledRegulator without destroying it then you've got
> > an EnabledRegulator which isn't actually enabled.  Perhaps it's not
> > clear to me how the API should work?

> From my understanding, disable() takes ownership of self, it does not take a
> reference, so the EnabledRegulator is consumed and the Regulator is returned
> through the result. So EnabledRegulator will get dropped in this function
> which owns it.

Ah, OK - if the disable() takes ownership of the passed object that's
fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Daniel Almeida]

Hi Mark,

> On 27 Mar 2025, at 08:32, Mark Brown <broonie@kernel.org> wrote:
> 
> On Wed, Mar 26, 2025 at 04:49:26PM -0300, Daniel Almeida wrote:
>>> On 26 Mar 2025, at 15:56, Mark Brown <broonie@kernel.org> wrote:
> 
>>>> +    /// Disables the regulator.
>>>> +    pub fn disable(self) -> Result<Regulator> {
>>>> +        // Keep the count on `regulator_get()`.
>>>> +        let regulator = ManuallyDrop::new(self);
> 
>>> This looks like user code could manually call it which feels like asking
>>> for trouble?
> 
>> Yes, user code can call this. My understanding is that drivers may want to
>> disable the regulator at runtime, possibly to save power when the device is
>> idle?
> 
>> What trouble are you referring to?
> 
> My understanding was that the enable was done by transforming a
> Regulator into an EnabledRegulator but if you can explicitly call
> disable() on an EnabledRegulator without destroying it then you've got
> an EnabledRegulator which isn't actually enabled.  Perhaps it's not
> clear to me how the API should work?

No, you misunderstood a bit, but that’s on me, I should have included examples.

> +impl EnabledRegulator {

> +    /// Disables the regulator.
> +    pub fn disable(self) -> Result<Regulator> 

disable() consumes EnabledRegulator to return Regulator.

Any function that takes 'self' by value (i.e.: “self" instead of “&self” )
effectively kills it. So, in that sense, disable() performs a conversion
between the two types after calling regulator_disable().

— Daniel

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Danilo Krummrich]

On Thu, Mar 27, 2025 at 08:46:29AM -0300, Daniel Almeida wrote:
> Any function that takes 'self' by value (i.e.: “self" instead of “&self” )
> effectively kills it.

I'm sure Daniel didn't mean it that way, but to avoid confusion, I want to
clarify that a function that takes `self` as argument not necessarily results in
`self` being destroyed. It could be moved into some other structure, directly
returned by the same function etc.

It's just that if the functions lets `self` go out of scope, then it's
destroyed.

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Daniel Almeida]

> On 27 Mar 2025, at 08:50, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Thu, Mar 27, 2025 at 08:46:29AM -0300, Daniel Almeida wrote:
>> Any function that takes 'self' by value (i.e.: “self" instead of “&self” )
>> effectively kills it.
> 
> I'm sure Daniel didn't mean it that way, but to avoid confusion, I want to
> clarify that a function that takes `self` as argument not necessarily results in
> `self` being destroyed. It could be moved into some other structure, directly
> returned by the same function etc.
> 
> It's just that if the functions lets `self` go out of scope, then it's
> destroyed.
> 

True, I found it easier to explain the current code by simplifying the story a
bit to fit the function we are discussing, but what Danilo said is what is
actually correct.

— Daniel

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

Hi,

On Wed, Mar 26, 2025 at 03:39:33PM -0300, Daniel Almeida wrote:
> +    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
> +        // SAFETY: It is safe to call `regulator_get()`, on a device pointer
> +        // received from the C code.
> +        let inner = from_err_ptr(unsafe { bindings::regulator_get(dev.as_raw(), name.as_ptr()) })?;
> +
> +        // SAFETY: We can safely trust `inner` to be a pointer to a valid
> +        // regulator if `ERR_PTR` was not returned.
> +        let inner = unsafe { NonNull::new_unchecked(inner) };
> +
> +        Ok(Self { inner })
> +    }

I think it's worth discussing using regulator_get() VS
regulator_get_optional(). We somehow ended up with the C regulator
API being more or less orthogonal to other in-kernel C APIs (clocks,
gpio, reset, LED) with the _optional suffixed version returning
-ENODEV for a missing regulator (and thus needing explicit handling)
and the normal version creating a dummy regulator (and a warning).

Considering the Rust API is new, it would be possible to let the
Rust get() function call regulator_get_optional() instead and then
introduce something like get_or_dummy() to call the normal
regulator_get() C function.

I see reasons in favor and against this. I just want to make sure it
has been considered before the API is being used, which makes it a
lot harder to change.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On Thu, Mar 27, 2025 at 02:46:30PM +0100, Sebastian Reichel wrote:
> On Wed, Mar 26, 2025 at 03:39:33PM -0300, Daniel Almeida wrote:

> > +    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
> > +        // SAFETY: It is safe to call `regulator_get()`, on a device pointer
> > +        // received from the C code.
> > +        let inner = from_err_ptr(unsafe { bindings::regulator_get(dev.as_raw(), name.as_ptr()) })?;

> I think it's worth discussing using regulator_get() VS
> regulator_get_optional(). We somehow ended up with the C regulator
> API being more or less orthogonal to other in-kernel C APIs (clocks,
> gpio, reset, LED) with the _optional suffixed version returning
> -ENODEV for a missing regulator (and thus needing explicit handling)
> and the normal version creating a dummy regulator (and a warning).

regulator was first here...

> Considering the Rust API is new, it would be possible to let the
> Rust get() function call regulator_get_optional() instead and then
> introduce something like get_or_dummy() to call the normal
> regulator_get() C function.

> I see reasons in favor and against this. I just want to make sure it
> has been considered before the API is being used, which makes it a
> lot harder to change.

Unless rust somehow magically allows devices to work without power this
would just be broken.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]