[PATCH v4] rust: regulator: add a bare minimum regulator abstraction

[PATCH v4] 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 in v4:
- Rewrote the abstraction to use typestates as per the suggestions by
  Benno and Alex.
- Introduced the `Dynamic` state.
- Added more examples.
- Fixed some broken docs.
- Link to v3: https://lore.kernel.org/r/20250513-topics-tyr-regulator-v3-1-4cc2704dfec6@collabora.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        | 385 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 388 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..338fc653c32b49e630ecb6a320ac26aea973cd64
--- /dev/null
+++ b/rust/kernel/regulator.rs
@@ -0,0 +1,385 @@
+// 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 a collection of states. Each state may
+//! enforce a given invariant, and they may convert between each other where applicable.
+//!
+//! See [`Voltage and current regulator API`]("https://docs.kernel.org/driver-api/regulator.html")
+//! for more information.
+
+use crate::{
+    bindings,
+    device::Device,
+    error::{from_err_ptr, to_result, Result},
+    prelude::*,
+};
+
+use core::{marker::PhantomData, ptr::NonNull};
+
+mod private {
+    pub trait Sealed {}
+
+    impl Sealed for super::Enabled {}
+    impl Sealed for super::Disabled {}
+    impl Sealed for super::Dynamic {}
+}
+
+/// A trait representing the different states a [`Regulator`] can be in.
+pub trait RegulatorState: private::Sealed {}
+
+/// A state where the [`Regulator`] is known to be enabled.
+pub struct Enabled;
+
+/// A state where this [`Regulator`] handle has not specifically asked for the
+/// underlying regulator to be enabled. This means that this reference does not
+/// own an `enable` reference count, but the regulator may still be on.
+pub struct Disabled;
+
+/// A state that models the C API. The [`Regulator`] can be either enabled or
+/// disabled, and the user is in control of the reference count. This is also
+/// the default state.
+///
+/// Use [`Regulator::is_enabled`] to check the regulator's current state.
+pub struct Dynamic;
+
+impl RegulatorState for Enabled {}
+impl RegulatorState for Disabled {}
+impl RegulatorState for Dynamic {}
+
+/// A trait that abstracts the conversion of a [`Regulator`] to an enabled state.
+pub trait TryIntoEnabled: RegulatorState {}
+impl TryIntoEnabled for Disabled {}
+
+/// A trait that abstracts the conversion of a [`Regulator`] to a disabled state.
+pub trait TryIntoDisabled: RegulatorState {}
+impl TryIntoDisabled for Enabled {}
+
+/// A trait that abstracts the ability to check if a [`Regulator`] is enabled.
+pub trait IsEnabled: RegulatorState {}
+impl IsEnabled for Disabled {}
+impl IsEnabled for Dynamic {}
+
+/// An error that can occur when trying to convert a [`Regulator`] between states.
+pub struct Error<State: RegulatorState + 'static> {
+    /// The error that occurred.
+    pub error: kernel::error::Error,
+    /// The regulator that caused the error, so that the operation may be retried.
+    pub regulator: Regulator<State>,
+}
+
+/// A `struct regulator` abstraction.
+///
+/// # Examples
+///
+/// Enabling a regulator:
+///
+/// This example uses [`Regulator<Enabled>`], which is suitable for drivers that
+/// enable a regulator at probe time and leave them on until the device is
+/// removed or otherwise shutdown.
+///
+/// These users can store [`Regulator<Enabled>`] directly in their driver's
+/// private data struct.
+///
+/// ```
+/// # use kernel::prelude::*;
+/// # use kernel::c_str;
+/// # use kernel::device::Device;
+/// # use kernel::regulator::{Microvolt, Regulator, Disabled, Enabled};
+/// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
+///    // Obtain a reference to a (fictitious) regulator.
+///    let regulator: Regulator<Disabled> = Regulator::<Disabled>::get(dev, c_str!("vcc"))?;
+///
+///    // The voltage can be set before enabling the regulator if needed, e.g.:
+///    regulator.set_voltage(min_uv, max_uv)?;
+///
+///    // The same applies for `get_voltage()`, i.e.:
+///    let voltage: Microvolt = regulator.get_voltage()?;
+///
+///    // Enables the regulator, consuming the previous value.
+///    //
+///    // From now on, the regulator is known to be enabled because of the type
+///    // `Enabled`.
+///    //
+///    // If this operation fails, the `Error` will contain the regulator
+///    // reference, so that the operation may be retried.
+///    let regulator: Regulator<Enabled> = regulator.try_into_enabled().map_err(|error| error.error)?;
+///
+///    // The voltage can also be set after enabling the regulator, e.g.:
+///    regulator.set_voltage(min_uv, max_uv)?;
+///
+///    // The same applies for `get_voltage()`, i.e.:
+///    let voltage: Microvolt = regulator.get_voltage()?;
+///
+///    // Dropping an enabled regulator will disable it. The refcount will be
+///    // decremented.
+///    drop(regulator);
+///    // ...
+///    # Ok::<(), Error>(())
+/// }
+///```
+///
+/// A more concise shortcut is available for enabling a regulator. This is
+/// equivalent to `regulator_get_enable()`:
+///
+/// ```
+/// # use kernel::prelude::*;
+/// # use kernel::c_str;
+/// # use kernel::device::Device;
+/// # use kernel::regulator::{Microvolt, Regulator, Enabled};
+/// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
+///    // Obtain a reference to a (fictitious) regulator and enable it.
+///    let regulator: Regulator<Enabled> = Regulator::<Enabled>::get(dev, c_str!("vcc"))?;
+///
+///    // Dropping an enabled regulator will disable it. The refcount will be
+///    // decremented.
+///    drop(regulator);
+///    // ...
+///    # Ok::<(), Error>(())
+/// }
+/// ```
+///
+/// Disabling a regulator:
+///
+///```
+/// # use kernel::prelude::*;
+/// # use kernel::device::Device;
+/// # use kernel::regulator::{Regulator, Enabled, Disabled};
+/// fn disable(dev: &Device, regulator: Regulator<Enabled>) -> Result {
+///    // We can also disable an enabled regulator without reliquinshing our
+///    // refcount:
+///    //
+///    // If this operation fails, the `Error` will contain the regulator
+///    // reference, so that the operation may be retried.
+///    let regulator: Regulator<Disabled> = regulator.try_into_disabled().map_err(|error| error.error)?;
+///
+///    // The refcount will be decremented when `regulator` is dropped.
+///    drop(regulator);
+///    // ...
+///    # Ok::<(), Error>(())
+/// }
+/// ```
+///
+///
+/// Using `Regulator<Dynamic>`:
+///
+/// This example mimics the behavior of the C API, where the user is in
+/// control of the enabled reference count. This is useful for drivers that
+/// might call enable and disable to manage the `enable` reference count at
+/// runtime, perhaps as a result of `open()` and `close()` calls or whatever
+/// other driver-specific or subsystem-specific hooks.
+///
+/// ```
+/// # use kernel::prelude::*;
+/// # use kernel::c_str;
+/// # use kernel::device::Device;
+/// # use kernel::regulator::{Regulator, Dynamic};
+///
+/// struct PrivateData {
+///     regulator: Regulator<Dynamic>,
+/// }
+///
+/// // A fictictious probe function that obtains a regulator and sets it up.
+/// fn probe(dev: &Device, data: &mut PrivateData) -> Result<PrivateData> {
+///    // Obtain a reference to a (fictitious) regulator.
+///    let mut regulator = Regulator::<Dynamic>::get(dev, c_str!("vcc"))?;
+///    // Enable the regulator. The type is still `Regulator<Dynamic>`.
+///    regulator.enable()?;
+///
+///   Ok(PrivateData {
+///       regulator,
+///   })
+/// }
+///
+/// // A fictictious function that indicates that the device is going to be used.
+/// fn open(dev: &Device, data: &mut PrivateData) -> Result {
+///     // Increase the `enabled` reference count.
+///     data.regulator.enable()?;
+///     Ok(())
+/// }
+///
+/// fn close(dev: &Device, data: &mut PrivateData) -> Result {
+///    // Decrease the `enabled` reference count.
+///    data.regulator.disable()?;
+///     Ok(())
+/// }
+///
+/// fn remove(dev: &Device, data: PrivateData) -> Result {
+///     // `PrivateData` is dropped here, which will drop the
+///     // `Regulator<Dynamic>` in turn.
+///     //
+///     // The reference that was obtained by `regulator_get()` will be
+///     // released, but it is up to the user to make sure that the number of calls
+///     // to `enable()` and `disabled()` are balanced before this point.
+///     Ok(())
+/// }
+///
+///
+/// ```
+/// # Invariants
+///
+/// - `inner` is a non-null wrapper over a pointer to a `struct
+///   regulator` obtained from [`regulator_get()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_get).
+pub struct Regulator<State = Dynamic>
+where
+    State: RegulatorState + 'static,
+{
+    inner: NonNull<bindings::regulator>,
+    _phantom: PhantomData<State>,
+}
+
+impl<T: RegulatorState + 'static> Regulator<T> {
+    /// Sets the voltage for the regulator.
+    ///
+    /// This can be used to ensure that the device powers up cleanly.
+    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.inner.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.inner.as_ptr()) };
+        if voltage < 0 {
+            Err(kernel::error::Error::from_errno(voltage))
+        } else {
+            Ok(Microvolt(voltage))
+        }
+    }
+
+    fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
+        // 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,
+            _phantom: PhantomData,
+        })
+    }
+
+    fn enable_internal(&mut self) -> Result {
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) })
+    }
+
+    fn disable_internal(&mut self) -> Result {
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) })
+    }
+}
+
+impl Regulator<Disabled> {
+    /// Obtains a [`Regulator`] instance from the system.
+    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
+        Regulator::get_internal(dev, name)
+    }
+}
+
+impl Regulator<Enabled> {
+    /// Obtains a [`Regulator`] instance from the system and enables it.
+    ///
+    /// This is equivalent to calling `regulator_get_enable()` in the C API.
+    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
+        Regulator::<Disabled>::get_internal(dev, name)?
+            .try_into_enabled()
+            .map_err(|error| error.error)
+    }
+}
+
+impl Regulator<Dynamic> {
+    /// Obtains a [`Regulator`] instance from the system. The current state of
+    /// the regulator is unknown and it is up to the user to manage the enabled
+    /// reference count.
+    ///
+    /// This closely mimics the behavior of the C API and can be used to
+    /// dynamically manage the enabled reference count at runtime.
+    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
+        Regulator::get_internal(dev, name)
+    }
+
+    /// Increases the `enabled` reference count.
+    pub fn enable(&mut self) -> Result {
+        self.enable_internal()
+    }
+
+    /// Decreases the `enabled` reference count.
+    pub fn disable(&mut self) -> Result {
+        self.disable_internal()
+    }
+}
+
+impl<T: TryIntoEnabled> Regulator<T> {
+    /// Attempts to convert the regulator to an enabled state.
+    pub fn try_into_enabled(mut self) -> Result<Regulator<Enabled>, Error<T>> {
+        self.enable_internal()
+            .map(|()| Regulator {
+                inner: self.inner,
+                _phantom: PhantomData,
+            })
+            .map_err(|error| Error {
+                error,
+                regulator: self,
+            })
+    }
+}
+
+impl<T: TryIntoDisabled> Regulator<T> {
+    /// Attempts to convert the regulator to a disabled state.
+    pub fn try_into_disabled(mut self) -> Result<Regulator<Disabled>, Error<T>> {
+        self.disable_internal()
+            .map(|()| Regulator {
+                inner: self.inner,
+                _phantom: PhantomData,
+            })
+            .map_err(|error| Error {
+                error,
+                regulator: self,
+            })
+    }
+}
+
+impl<T: IsEnabled> Regulator<T> {
+    /// Checks if the regulator is enabled.
+    pub fn is_enabled(&self) -> bool {
+        // SAFETY: Safe as per the type invariants of `Regulator`.
+        unsafe { bindings::regulator_is_enabled(self.inner.as_ptr()) != 0 }
+    }
+}
+
+impl<T: RegulatorState + 'static> Drop for Regulator<T> {
+    fn drop(&mut self) {
+        if core::any::TypeId::of::<T>() == core::any::TypeId::of::<Enabled>() {
+            // SAFETY: By the type invariants, we know that `self` owns a
+            // reference on the enabled refcount, so it is safe to relinquish it
+            // now.
+            unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
+        }
+        // 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 voltage in microvolts.
+///
+/// The explicit type is used to avoid confusion with other multiples of the
+/// volt, which can be desastrous.
+#[repr(transparent)]
+#[derive(Copy, Clone, PartialEq, Eq)]
+pub struct Microvolt(pub i32);

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

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

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

[Miguel Ojeda]

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

On Mon, Jun 9, 2025 at 5:34 PM Daniel Almeida
<daniel.almeida@collabora.com> 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.

For future versions, or at apply time, please see the attached diff
for some documentation fixes (mostly) -- please check with
`--ignore-space-change`, since some indentation fixes can hide other
changes.

Most should be self-explanatory, but if there is something that you
think should not be done, please let me know of course.

Thanks!

Cheers,
Miguel

[-- Attachment #2: docfixes.diff --]
[-- Type: text/x-patch, Size: 9209 bytes --]

diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
index 338fc653c32b..0baf763a098e 100644
--- a/rust/kernel/regulator.rs
+++ b/rust/kernel/regulator.rs
@@ -13,7 +13,7 @@
 //! Regulators are modeled in Rust with a collection of states. Each state may
 //! enforce a given invariant, and they may convert between each other where applicable.
 //!
-//! See [`Voltage and current regulator API`]("https://docs.kernel.org/driver-api/regulator.html")
+//! See [Voltage and current regulator API](https://docs.kernel.org/driver-api/regulator.html)
 //! for more information.
 
 use crate::{
@@ -72,6 +72,7 @@ impl IsEnabled for Dynamic {}
 pub struct Error<State: RegulatorState + 'static> {
     /// The error that occurred.
     pub error: kernel::error::Error,
+
     /// The regulator that caused the error, so that the operation may be retried.
     pub regulator: Regulator<State>,
 }
@@ -80,7 +81,7 @@ pub struct Error<State: RegulatorState + 'static> {
 ///
 /// # Examples
 ///
-/// Enabling a regulator:
+/// ## Enabling a regulator
 ///
 /// This example uses [`Regulator<Enabled>`], which is suitable for drivers that
 /// enable a regulator at probe time and leave them on until the device is
@@ -95,37 +96,40 @@ pub struct Error<State: RegulatorState + 'static> {
 /// # use kernel::device::Device;
 /// # use kernel::regulator::{Microvolt, Regulator, Disabled, Enabled};
 /// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
-///    // Obtain a reference to a (fictitious) regulator.
-///    let regulator: Regulator<Disabled> = Regulator::<Disabled>::get(dev, c_str!("vcc"))?;
-///
-///    // The voltage can be set before enabling the regulator if needed, e.g.:
-///    regulator.set_voltage(min_uv, max_uv)?;
-///
-///    // The same applies for `get_voltage()`, i.e.:
-///    let voltage: Microvolt = regulator.get_voltage()?;
-///
-///    // Enables the regulator, consuming the previous value.
-///    //
-///    // From now on, the regulator is known to be enabled because of the type
-///    // `Enabled`.
-///    //
-///    // If this operation fails, the `Error` will contain the regulator
-///    // reference, so that the operation may be retried.
-///    let regulator: Regulator<Enabled> = regulator.try_into_enabled().map_err(|error| error.error)?;
-///
-///    // The voltage can also be set after enabling the regulator, e.g.:
-///    regulator.set_voltage(min_uv, max_uv)?;
-///
-///    // The same applies for `get_voltage()`, i.e.:
-///    let voltage: Microvolt = regulator.get_voltage()?;
-///
-///    // Dropping an enabled regulator will disable it. The refcount will be
-///    // decremented.
-///    drop(regulator);
-///    // ...
-///    # Ok::<(), Error>(())
+///     // Obtain a reference to a (fictitious) regulator.
+///     let regulator: Regulator<Disabled> = Regulator::<Disabled>::get(dev, c_str!("vcc"))?;
+///
+///     // The voltage can be set before enabling the regulator if needed, e.g.:
+///     regulator.set_voltage(min_uv, max_uv)?;
+///
+///     // The same applies for `get_voltage()`, i.e.:
+///     let voltage: Microvolt = regulator.get_voltage()?;
+///
+///     // Enables the regulator, consuming the previous value.
+///     //
+///     // From now on, the regulator is known to be enabled because of the type
+///     // `Enabled`.
+///     //
+///     // If this operation fails, the `Error` will contain the regulator
+///     // reference, so that the operation may be retried.
+///     let regulator: Regulator<Enabled> =
+///         regulator.try_into_enabled().map_err(|error| error.error)?;
+///
+///     // The voltage can also be set after enabling the regulator, e.g.:
+///     regulator.set_voltage(min_uv, max_uv)?;
+///
+///     // The same applies for `get_voltage()`, i.e.:
+///     let voltage: Microvolt = regulator.get_voltage()?;
+///
+///     // Dropping an enabled regulator will disable it. The refcount will be
+///     // decremented.
+///     drop(regulator);
+///
+///     // ...
+///
+///     Ok(())
 /// }
-///```
+/// ```
 ///
 /// A more concise shortcut is available for enabling a regulator. This is
 /// equivalent to `regulator_get_enable()`:
@@ -136,40 +140,44 @@ pub struct Error<State: RegulatorState + 'static> {
 /// # use kernel::device::Device;
 /// # use kernel::regulator::{Microvolt, Regulator, Enabled};
 /// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
-///    // Obtain a reference to a (fictitious) regulator and enable it.
-///    let regulator: Regulator<Enabled> = Regulator::<Enabled>::get(dev, c_str!("vcc"))?;
-///
-///    // Dropping an enabled regulator will disable it. The refcount will be
-///    // decremented.
-///    drop(regulator);
-///    // ...
-///    # Ok::<(), Error>(())
+///     // Obtain a reference to a (fictitious) regulator and enable it.
+///     let regulator: Regulator<Enabled> = Regulator::<Enabled>::get(dev, c_str!("vcc"))?;
+///
+///     // Dropping an enabled regulator will disable it. The refcount will be
+///     // decremented.
+///     drop(regulator);
+///
+///     // ...
+///
+///     Ok(())
 /// }
 /// ```
 ///
-/// Disabling a regulator:
+/// ## Disabling a regulator
 ///
-///```
+/// ```
 /// # use kernel::prelude::*;
 /// # use kernel::device::Device;
 /// # use kernel::regulator::{Regulator, Enabled, Disabled};
 /// fn disable(dev: &Device, regulator: Regulator<Enabled>) -> Result {
-///    // We can also disable an enabled regulator without reliquinshing our
-///    // refcount:
-///    //
-///    // If this operation fails, the `Error` will contain the regulator
-///    // reference, so that the operation may be retried.
-///    let regulator: Regulator<Disabled> = regulator.try_into_disabled().map_err(|error| error.error)?;
-///
-///    // The refcount will be decremented when `regulator` is dropped.
-///    drop(regulator);
-///    // ...
-///    # Ok::<(), Error>(())
+///     // We can also disable an enabled regulator without reliquinshing our
+///     // refcount:
+///     //
+///     // If this operation fails, the `Error` will contain the regulator
+///     // reference, so that the operation may be retried.
+///     let regulator: Regulator<Disabled> =
+///         regulator.try_into_disabled().map_err(|error| error.error)?;
+///
+///     // The refcount will be decremented when `regulator` is dropped.
+///     drop(regulator);
+///
+///     // ...
+///
+///     Ok(())
 /// }
 /// ```
 ///
-///
-/// Using `Regulator<Dynamic>`:
+/// ## Using [`Regulator<Dynamic>`]
 ///
 /// This example mimics the behavior of the C API, where the user is in
 /// control of the enabled reference count. This is useful for drivers that
@@ -182,33 +190,33 @@ pub struct Error<State: RegulatorState + 'static> {
 /// # use kernel::c_str;
 /// # use kernel::device::Device;
 /// # use kernel::regulator::{Regulator, Dynamic};
-///
 /// struct PrivateData {
 ///     regulator: Regulator<Dynamic>,
 /// }
 ///
 /// // A fictictious probe function that obtains a regulator and sets it up.
 /// fn probe(dev: &Device, data: &mut PrivateData) -> Result<PrivateData> {
-///    // Obtain a reference to a (fictitious) regulator.
-///    let mut regulator = Regulator::<Dynamic>::get(dev, c_str!("vcc"))?;
-///    // Enable the regulator. The type is still `Regulator<Dynamic>`.
-///    regulator.enable()?;
-///
-///   Ok(PrivateData {
-///       regulator,
-///   })
+///     // Obtain a reference to a (fictitious) regulator.
+///     let mut regulator = Regulator::<Dynamic>::get(dev, c_str!("vcc"))?;
+///
+///     // Enable the regulator. The type is still `Regulator<Dynamic>`.
+///     regulator.enable()?;
+///
+///     Ok(PrivateData { regulator })
 /// }
 ///
 /// // A fictictious function that indicates that the device is going to be used.
 /// fn open(dev: &Device, data: &mut PrivateData) -> Result {
 ///     // Increase the `enabled` reference count.
 ///     data.regulator.enable()?;
+///
 ///     Ok(())
 /// }
 ///
 /// fn close(dev: &Device, data: &mut PrivateData) -> Result {
-///    // Decrease the `enabled` reference count.
-///    data.regulator.disable()?;
+///     // Decrease the `enabled` reference count.
+///     data.regulator.disable()?;
+///
 ///     Ok(())
 /// }
 ///
@@ -221,13 +229,14 @@ pub struct Error<State: RegulatorState + 'static> {
 ///     // to `enable()` and `disabled()` are balanced before this point.
 ///     Ok(())
 /// }
-///
-///
 /// ```
+///
 /// # Invariants
 ///
 /// - `inner` is a non-null wrapper over a pointer to a `struct
-///   regulator` obtained from [`regulator_get()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_get).
+///   regulator` obtained from [`regulator_get()`].
+///
+/// [`regulator_get()`]: https://docs.kernel.org/driver-api/regulator.html#c.regulator_get
 pub struct Regulator<State = Dynamic>
 where
     State: RegulatorState + 'static,
@@ -379,7 +388,7 @@ fn drop(&mut self) {
 /// A voltage in microvolts.
 ///
 /// The explicit type is used to avoid confusion with other multiples of the
-/// volt, which can be desastrous.
+/// volt, which can be disastrous.
 #[repr(transparent)]
 #[derive(Copy, Clone, PartialEq, Eq)]
 pub struct Microvolt(pub i32);

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

[Daniel Almeida]

Hi Miguel,

Sure, thank you!

By the way, are you using some lint to generate this?

> On 9 Jun 2025, at 13:23, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Mon, Jun 9, 2025 at 5:34 PM Daniel Almeida
> <daniel.almeida@collabora.com> 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.
> 
> For future versions, or at apply time, please see the attached diff
> for some documentation fixes (mostly) -- please check with
> `--ignore-space-change`, since some indentation fixes can hide other
> changes.
> 
> Most should be self-explanatory, but if there is something that you
> think should not be done, please let me know of course.
> 
> Thanks!
> 
> Cheers,
> Miguel
> <docfixes.diff>


Also,

> -/// Enabling a regulator:
> +/// ## Enabling a regulator
>  ///

Did you try to render this? I tried it before submitting, but it did not render
correctly after the first time it was used, i.e.: only the first subsection was
rendered correctly and the others remained as-is.

— Daniel

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

[Miguel Ojeda]

On Mon, Jun 9, 2025 at 6:31 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> By the way, are you using some lint to generate this?

No, just my eyes :)

But I try to report upstream things that I think could be automated so
that eventually we need less time to catch this sort of thing, e.g. I
was just opening this one inspired by this patch:

    https://github.com/rust-lang/rust-clippy/issues/15023

Plus these other two, which do not happen here, but are related:

    https://github.com/rust-lang/rust-clippy/issues/15024
    https://github.com/rust-lang/rust-clippy/issues/15025

Some of our suggestions did get implemented in the past by others
even, so that was great.

> Did you try to render this? I tried it before submitting, but it did not render
> correctly after the first time it was used, i.e.: only the first subsection was
> rendered correctly and the others remained as-is.

Yeah, I did -- it works fine for me.

I even checked that it generates a higher level HTML `<hN>` tag vs.
the main one, as expected in Markdown (I checked that because the font
was fairly similar in size, so it was hard to see).

Do you see something different?

Cheers,
Miguel

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

[Alexandre Courbot]

Hi Daniel,

To no surprise, I like this iteration. :) A few comments below after a
first pass.

On Tue Jun 10, 2025 at 12:32 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@collabora.com>
> ---
> Changes in v4:
> - Rewrote the abstraction to use typestates as per the suggestions by
>   Benno and Alex.
> - Introduced the `Dynamic` state.
> - Added more examples.
> - Fixed some broken docs.
> - Link to v3: https://lore.kernel.org/r/20250513-topics-tyr-regulator-v3-1-4cc2704dfec6@collabora.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        | 385 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 388 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..338fc653c32b49e630ecb6a320ac26aea973cd64
> --- /dev/null
> +++ b/rust/kernel/regulator.rs
> @@ -0,0 +1,385 @@
> +// 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 a collection of states. Each state may
> +//! enforce a given invariant, and they may convert between each other where applicable.
> +//!
> +//! See [`Voltage and current regulator API`]("https://docs.kernel.org/driver-api/regulator.html")
> +//! for more information.
> +
> +use crate::{
> +    bindings,
> +    device::Device,
> +    error::{from_err_ptr, to_result, Result},
> +    prelude::*,
> +};
> +
> +use core::{marker::PhantomData, ptr::NonNull};
> +
> +mod private {
> +    pub trait Sealed {}
> +
> +    impl Sealed for super::Enabled {}
> +    impl Sealed for super::Disabled {}
> +    impl Sealed for super::Dynamic {}
> +}
> +
> +/// A trait representing the different states a [`Regulator`] can be in.
> +pub trait RegulatorState: private::Sealed {}
> +
> +/// A state where the [`Regulator`] is known to be enabled.
> +pub struct Enabled;
> +
> +/// A state where this [`Regulator`] handle has not specifically asked for the
> +/// underlying regulator to be enabled. This means that this reference does not
> +/// own an `enable` reference count, but the regulator may still be on.
> +pub struct Disabled;
> +
> +/// A state that models the C API. The [`Regulator`] can be either enabled or
> +/// disabled, and the user is in control of the reference count. This is also
> +/// the default state.
> +///
> +/// Use [`Regulator::is_enabled`] to check the regulator's current state.
> +pub struct Dynamic;
> +
> +impl RegulatorState for Enabled {}
> +impl RegulatorState for Disabled {}
> +impl RegulatorState for Dynamic {}
> +
> +/// A trait that abstracts the conversion of a [`Regulator`] to an enabled state.
> +pub trait TryIntoEnabled: RegulatorState {}
> +impl TryIntoEnabled for Disabled {}

Since `TryIntoEnabled` is only implemented for `Disabled`, it doesn't
look like we need a trait to factorize code. Why not just implement
`try_into_enabled` directly in the `Disabled` state?

> +
> +/// A trait that abstracts the conversion of a [`Regulator`] to a disabled state.
> +pub trait TryIntoDisabled: RegulatorState {}
> +impl TryIntoDisabled for Enabled {}

Same remark.

> +
> +/// A trait that abstracts the ability to check if a [`Regulator`] is enabled.
> +pub trait IsEnabled: RegulatorState {}
> +impl IsEnabled for Disabled {}
> +impl IsEnabled for Dynamic {}
> +
> +/// An error that can occur when trying to convert a [`Regulator`] between states.
> +pub struct Error<State: RegulatorState + 'static> {
> +    /// The error that occurred.
> +    pub error: kernel::error::Error,
> +    /// The regulator that caused the error, so that the operation may be retried.
> +    pub regulator: Regulator<State>,
> +}
> +
> +/// A `struct regulator` abstraction.
> +///
> +/// # Examples
> +///
> +/// Enabling a regulator:
> +///
> +/// This example uses [`Regulator<Enabled>`], which is suitable for drivers that
> +/// enable a regulator at probe time and leave them on until the device is
> +/// removed or otherwise shutdown.
> +///
> +/// These users can store [`Regulator<Enabled>`] directly in their driver's
> +/// private data struct.
> +///
> +/// ```
> +/// # use kernel::prelude::*;
> +/// # use kernel::c_str;
> +/// # use kernel::device::Device;
> +/// # use kernel::regulator::{Microvolt, Regulator, Disabled, Enabled};
> +/// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
> +///    // Obtain a reference to a (fictitious) regulator.
> +///    let regulator: Regulator<Disabled> = Regulator::<Disabled>::get(dev, c_str!("vcc"))?;
> +///
> +///    // The voltage can be set before enabling the regulator if needed, e.g.:
> +///    regulator.set_voltage(min_uv, max_uv)?;
> +///
> +///    // The same applies for `get_voltage()`, i.e.:
> +///    let voltage: Microvolt = regulator.get_voltage()?;
> +///
> +///    // Enables the regulator, consuming the previous value.
> +///    //
> +///    // From now on, the regulator is known to be enabled because of the type
> +///    // `Enabled`.
> +///    //
> +///    // If this operation fails, the `Error` will contain the regulator
> +///    // reference, so that the operation may be retried.
> +///    let regulator: Regulator<Enabled> = regulator.try_into_enabled().map_err(|error| error.error)?;
> +///
> +///    // The voltage can also be set after enabling the regulator, e.g.:
> +///    regulator.set_voltage(min_uv, max_uv)?;
> +///
> +///    // The same applies for `get_voltage()`, i.e.:
> +///    let voltage: Microvolt = regulator.get_voltage()?;
> +///
> +///    // Dropping an enabled regulator will disable it. The refcount will be
> +///    // decremented.
> +///    drop(regulator);
> +///    // ...
> +///    # Ok::<(), Error>(())
> +/// }
> +///```
> +///
> +/// A more concise shortcut is available for enabling a regulator. This is
> +/// equivalent to `regulator_get_enable()`:
> +///
> +/// ```
> +/// # use kernel::prelude::*;
> +/// # use kernel::c_str;
> +/// # use kernel::device::Device;
> +/// # use kernel::regulator::{Microvolt, Regulator, Enabled};
> +/// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
> +///    // Obtain a reference to a (fictitious) regulator and enable it.
> +///    let regulator: Regulator<Enabled> = Regulator::<Enabled>::get(dev, c_str!("vcc"))?;
> +///
> +///    // Dropping an enabled regulator will disable it. The refcount will be
> +///    // decremented.
> +///    drop(regulator);
> +///    // ...
> +///    # Ok::<(), Error>(())
> +/// }
> +/// ```
> +///
> +/// Disabling a regulator:
> +///
> +///```
> +/// # use kernel::prelude::*;
> +/// # use kernel::device::Device;
> +/// # use kernel::regulator::{Regulator, Enabled, Disabled};
> +/// fn disable(dev: &Device, regulator: Regulator<Enabled>) -> Result {
> +///    // We can also disable an enabled regulator without reliquinshing our
> +///    // refcount:
> +///    //
> +///    // If this operation fails, the `Error` will contain the regulator
> +///    // reference, so that the operation may be retried.
> +///    let regulator: Regulator<Disabled> = regulator.try_into_disabled().map_err(|error| error.error)?;
> +///
> +///    // The refcount will be decremented when `regulator` is dropped.
> +///    drop(regulator);
> +///    // ...
> +///    # Ok::<(), Error>(())
> +/// }
> +/// ```
> +///
> +///
> +/// Using `Regulator<Dynamic>`:
> +///
> +/// This example mimics the behavior of the C API, where the user is in
> +/// control of the enabled reference count. This is useful for drivers that
> +/// might call enable and disable to manage the `enable` reference count at
> +/// runtime, perhaps as a result of `open()` and `close()` calls or whatever
> +/// other driver-specific or subsystem-specific hooks.
> +///
> +/// ```
> +/// # use kernel::prelude::*;
> +/// # use kernel::c_str;
> +/// # use kernel::device::Device;
> +/// # use kernel::regulator::{Regulator, Dynamic};
> +///
> +/// struct PrivateData {
> +///     regulator: Regulator<Dynamic>,
> +/// }
> +///
> +/// // A fictictious probe function that obtains a regulator and sets it up.
> +/// fn probe(dev: &Device, data: &mut PrivateData) -> Result<PrivateData> {
> +///    // Obtain a reference to a (fictitious) regulator.
> +///    let mut regulator = Regulator::<Dynamic>::get(dev, c_str!("vcc"))?;
> +///    // Enable the regulator. The type is still `Regulator<Dynamic>`.
> +///    regulator.enable()?;
> +///
> +///   Ok(PrivateData {
> +///       regulator,
> +///   })
> +/// }
> +///
> +/// // A fictictious function that indicates that the device is going to be used.
> +/// fn open(dev: &Device, data: &mut PrivateData) -> Result {
> +///     // Increase the `enabled` reference count.
> +///     data.regulator.enable()?;
> +///     Ok(())
> +/// }
> +///
> +/// fn close(dev: &Device, data: &mut PrivateData) -> Result {
> +///    // Decrease the `enabled` reference count.
> +///    data.regulator.disable()?;
> +///     Ok(())
> +/// }
> +///
> +/// fn remove(dev: &Device, data: PrivateData) -> Result {
> +///     // `PrivateData` is dropped here, which will drop the
> +///     // `Regulator<Dynamic>` in turn.
> +///     //
> +///     // The reference that was obtained by `regulator_get()` will be
> +///     // released, but it is up to the user to make sure that the number of calls
> +///     // to `enable()` and `disabled()` are balanced before this point.
> +///     Ok(())
> +/// }
> +///
> +///
> +/// ```
> +/// # Invariants
> +///
> +/// - `inner` is a non-null wrapper over a pointer to a `struct
> +///   regulator` obtained from [`regulator_get()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_get).
> +pub struct Regulator<State = Dynamic>
> +where
> +    State: RegulatorState + 'static,
> +{
> +    inner: NonNull<bindings::regulator>,
> +    _phantom: PhantomData<State>,
> +}
> +
> +impl<T: RegulatorState + 'static> Regulator<T> {
> +    /// Sets the voltage for the regulator.
> +    ///
> +    /// This can be used to ensure that the device powers up cleanly.
> +    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.inner.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.inner.as_ptr()) };
> +        if voltage < 0 {
> +            Err(kernel::error::Error::from_errno(voltage))
> +        } else {
> +            Ok(Microvolt(voltage))
> +        }
> +    }
> +
> +    fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
> +        // 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,
> +            _phantom: PhantomData,
> +        })
> +    }
> +
> +    fn enable_internal(&mut self) -> Result {
> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) })
> +    }
> +
> +    fn disable_internal(&mut self) -> Result {
> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) })
> +    }
> +}
> +
> +impl Regulator<Disabled> {
> +    /// Obtains a [`Regulator`] instance from the system.
> +    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
> +        Regulator::get_internal(dev, name)
> +    }
> +}
> +
> +impl Regulator<Enabled> {
> +    /// Obtains a [`Regulator`] instance from the system and enables it.
> +    ///
> +    /// This is equivalent to calling `regulator_get_enable()` in the C API.
> +    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
> +        Regulator::<Disabled>::get_internal(dev, name)?
> +            .try_into_enabled()
> +            .map_err(|error| error.error)
> +    }
> +}
> +
> +impl Regulator<Dynamic> {
> +    /// Obtains a [`Regulator`] instance from the system. The current state of
> +    /// the regulator is unknown and it is up to the user to manage the enabled
> +    /// reference count.
> +    ///
> +    /// This closely mimics the behavior of the C API and can be used to
> +    /// dynamically manage the enabled reference count at runtime.
> +    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
> +        Regulator::get_internal(dev, name)
> +    }
> +
> +    /// Increases the `enabled` reference count.
> +    pub fn enable(&mut self) -> Result {
> +        self.enable_internal()
> +    }
> +
> +    /// Decreases the `enabled` reference count.
> +    pub fn disable(&mut self) -> Result {
> +        self.disable_internal()
> +    }
> +}
> +
> +impl<T: TryIntoEnabled> Regulator<T> {
> +    /// Attempts to convert the regulator to an enabled state.
> +    pub fn try_into_enabled(mut self) -> Result<Regulator<Enabled>, Error<T>> {
> +        self.enable_internal()
> +            .map(|()| Regulator {
> +                inner: self.inner,
> +                _phantom: PhantomData,
> +            })
> +            .map_err(|error| Error {
> +                error,
> +                regulator: self,
> +            })
> +    }
> +}
> +
> +impl<T: TryIntoDisabled> Regulator<T> {
> +    /// Attempts to convert the regulator to a disabled state.
> +    pub fn try_into_disabled(mut self) -> Result<Regulator<Disabled>, Error<T>> {
> +        self.disable_internal()
> +            .map(|()| Regulator {
> +                inner: self.inner,
> +                _phantom: PhantomData,
> +            })
> +            .map_err(|error| Error {
> +                error,
> +                regulator: self,
> +            })
> +    }
> +}
> +
> +impl<T: IsEnabled> Regulator<T> {
> +    /// Checks if the regulator is enabled.
> +    pub fn is_enabled(&self) -> bool {
> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        unsafe { bindings::regulator_is_enabled(self.inner.as_ptr()) != 0 }
> +    }
> +}
> +
> +impl<T: RegulatorState + 'static> Drop for Regulator<T> {
> +    fn drop(&mut self) {
> +        if core::any::TypeId::of::<T>() == core::any::TypeId::of::<Enabled>() {
> +            // SAFETY: By the type invariants, we know that `self` owns a
> +            // reference on the enabled refcount, so it is safe to relinquish it
> +            // now.
> +            unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
> +        }

Do we want to keep enabled dynamic regulators enabled? IIUC that's what
the C API does, so doing the same is ok, but let's at least mention that
fact in the documentation.

> +        // 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 voltage in microvolts.
> +///
> +/// The explicit type is used to avoid confusion with other multiples of the
> +/// volt, which can be desastrous.
> +#[repr(transparent)]
> +#[derive(Copy, Clone, PartialEq, Eq)]
> +pub struct Microvolt(pub i32);

This type actually contains a voltage, but is named after the unit it
stores. A bit like if `Duration` was named `Nanoseconds`. How about just
naming it `Voltage` and give it `from_microvolts` and `as_microvolts`
methods? We might not need to use other units, but at least it doesn't
close that option.

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

[Mark Brown]

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

On Sun, Jun 15, 2025 at 10:31:59PM +0900, Alexandre Courbot wrote:
> On Tue Jun 10, 2025 at 12:32 AM JST, Daniel Almeida wrote:

> > +impl<T: RegulatorState + 'static> Drop for Regulator<T> {
> > +    fn drop(&mut self) {
> > +        if core::any::TypeId::of::<T>() == core::any::TypeId::of::<Enabled>() {
> > +            // SAFETY: By the type invariants, we know that `self` owns a
> > +            // reference on the enabled refcount, so it is safe to relinquish it
> > +            // now.
> > +            unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
> > +        }

> Do we want to keep enabled dynamic regulators enabled? IIUC that's what
> the C API does, so doing the same is ok, but let's at least mention that
> fact in the documentation.

I'm not sure what you mean by a "dynamic regulator" here but as
previously outlined we currently take a very conservative approach and
don't do things without being explicitly told to do so since getting
things wrong can result in physical damage to the system.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

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

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

[Daniel Almeida]

Mark,

The current patch implements different strategies to handle regulators. By
‘dynamic regulator’ he means the one that implements the current C
behavior.

> I'm not sure what you mean by a "dynamic regulator" here but as
> previously outlined we currently take a very conservative approach and
> don't do things without being explicitly told to do so since getting
> things wrong can result in physical damage to the system.
> 
> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.

Ok, no worries, this current version of the binding is already implemented as
you said, so we won’t have any problems there.

— Daniel

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

[Daniel Almeida]

Hi Alex,

>> +
>> +/// A voltage in microvolts.
>> +///
>> +/// The explicit type is used to avoid confusion with other multiples of the
>> +/// volt, which can be desastrous.
>> +#[repr(transparent)]
>> +#[derive(Copy, Clone, PartialEq, Eq)]
>> +pub struct Microvolt(pub i32);
> 
> This type actually contains a voltage, but is named after the unit it
> stores. A bit like if `Duration` was named `Nanoseconds`. How about just
> naming it `Voltage` and give it `from_microvolts` and `as_microvolts`
> methods? We might not need to use other units, but at least it doesn't
> close that option.

I think that not accepting anything other than microvolts is by design, and
that changing this to `Voltage` would defeat the purpose, which is to make it
crystal clear that the unit is microvolts.

Also, the places where this type is used take microvolts, so I don't see why we
should add this indirection, which would require a convertion back and forth
from volt.

By the way, I went ahead and sent a new version [0] without this change.

— Daniel

[0] https://lore.kernel.org/rust-for-linux/20250623-topics-tyr-regulator-v5-0-99069658cb54@collabora.com/T/#t