[PATCH v6 0/2] Add a bare-minimum Regulator abstraction

[PATCH v6 0/2] Add a bare-minimum Regulator abstraction

[Daniel Almeida]

---
Changes in v6:
- Use ManuallyDrop<T> to avoid running the destructor in
  try_into_enabled() and try_into_disabled(). This is the same strategy
  that was being used successfully in the pre-typestate version of this
  patch
- Link to v5: https://lore.kernel.org/r/20250623-topics-tyr-regulator-v5-0-99069658cb54@collabora.com

Changes in v5:
- Remove TryIntoEnabled and TryIntoDisabled traits (they were only
  implemented for a single type anyways)
- Added regulator.rs to VOLTAGE AND CURRENT REGULATOR FRAMEWORK
- Applied the diff from Miguel Ojeda to format the docs
- Link to v4: https://lore.kernel.org/r/20250609-topics-tyr-regulator-v4-1-b4fdcf1385a7@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/

---
Daniel Almeida (2):
      rust: regulator: add a bare minimum regulator abstraction
      MAINAINTERS: add regulator.rs to the regulator API entry

 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/regulator.rs        | 392 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 396 insertions(+)
---
base-commit: edc5e6e019c99b529b3d1f2801d5cce9924ae79b
change-id: 20250326-topics-tyr-regulator-e8b98f6860d7

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

[PATCH v6 1/2] 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>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/regulator.rs        | 392 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 395 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..dbb4b5c8b06e05aa1e802e2c61c402dd997edfdb
--- /dev/null
+++ b/rust/kernel/regulator.rs
@@ -0,0 +1,392 @@
+// 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, mem::ManuallyDrop, 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 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(())
+/// }
+/// ```
+///
+/// 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(())
+/// }
+/// ```
+///
+/// ## 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(())
+/// }
+/// ```
+///
+/// ## 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()`].
+///
+/// [`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)
+    }
+
+    /// Attempts to convert the regulator to an enabled state.
+    pub fn try_into_enabled(mut self) -> Result<Regulator<Enabled>, Error<Disabled>> {
+        // We will be transferring the ownership of our `regulator_get()` count to
+        // `Regulator<Enabled>`.
+        let mut regulator = ManuallyDrop::new(self);
+
+        regulator
+            .enable_internal()
+            .map(|()| Regulator {
+                inner: regulator.inner,
+                _phantom: PhantomData,
+            })
+            .map_err(|error| Error {
+                error,
+                regulator: ManuallyDrop::into_inner(regulator),
+            })
+    }
+}
+
+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)
+    }
+
+    /// Attempts to convert the regulator to a disabled state.
+    pub fn try_into_disabled(mut self) -> Result<Regulator<Disabled>, Error<Enabled>> {
+        // We will be transferring the ownership of our `regulator_get()` count
+        // to `Regulator<Disabled>`.
+        let mut regulator = ManuallyDrop::new(self);
+
+        regulator
+            .disable_internal()
+            .map(|()| Regulator {
+                inner: regulator.inner,
+                _phantom: PhantomData,
+            })
+            .map_err(|error| Error {
+                error,
+                regulator: ManuallyDrop::into_inner(regulator),
+            })
+    }
+}
+
+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: 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 disastrous.
+#[repr(transparent)]
+#[derive(Copy, Clone, PartialEq, Eq)]
+pub struct Microvolt(pub i32);

-- 
2.50.0

[PATCH v6 2/2] MAINAINTERS: add regulator.rs to the regulator API entry

[Daniel Almeida]

Add this file to the regulator API entry as requested by Mark Brown.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f21f1dabb5fe1d31fca1a558473d92b90027088c..f8f9f571c19c8bc174eb8ac380de048dab9ecb99 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26000,6 +26000,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
 F:	Documentation/devicetree/bindings/regulator/
 F:	Documentation/power/regulator/
 F:	drivers/regulator/
+F:	rust/kernel/regulator.rs
 F:	include/dt-bindings/regulator/
 F:	include/linux/regulator/
 K:	regulator_get_optional

-- 
2.50.0

Re: [PATCH v6 1/2] rust: regulator: add a bare minimum regulator abstraction

[kernel test robot]

Hi Daniel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on edc5e6e019c99b529b3d1f2801d5cce9924ae79b]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Almeida/rust-regulator-add-a-bare-minimum-regulator-abstraction/20250628-011418
base:   edc5e6e019c99b529b3d1f2801d5cce9924ae79b
patch link:    https://lore.kernel.org/r/20250627-topics-tyr-regulator-v6-1-1d015219b454%40collabora.com
patch subject: [PATCH v6 1/2] rust: regulator: add a bare minimum regulator abstraction
config: x86_64-randconfig-008-20250629 (https://download.01.org/0day-ci/archive/20250629/202506292009.nbvCCbAr-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250629/202506292009.nbvCCbAr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506292009.nbvCCbAr-lkp@intel.com/

All warnings (new ones prefixed by >>):

   ***
   *** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
   *** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
   *** unless patched (like Debian's).
   ***   Your bindgen version:  0.65.1
   ***   Your libclang version: 20.1.7
   ***
   ***
   *** Please see Documentation/rust/quick-start.rst for details
   *** on how to set up the Rust support.
   ***
>> warning: variable does not need to be mutable
   --> rust/kernel/regulator.rs:295:29
   |
   295 |     pub fn try_into_enabled(mut self) -> Result<Regulator<Enabled>, Error<Disabled>> {
   |                             ----^^^^
   |                             |
   |                             help: remove this `mut`
   |
   = note: `#[warn(unused_mut)]` on by default
--
>> warning: variable does not need to be mutable
   --> rust/kernel/regulator.rs:324:30
   |
   324 |     pub fn try_into_disabled(mut self) -> Result<Regulator<Disabled>, Error<Enabled>> {
   |                              ----^^^^
   |                              |
   |                              help: remove this `mut`

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 0/2] Add a bare-minimum Regulator abstraction

[Daniel Almeida]

Mark,

Is it ok for the abstraction to only be built when CONFIG_REGULATOR=y? This
means that any Rust drivers using it have to depend on CONFIG_REGULATOR too.

I thought this was acceptable, but apparently that is not the case? See this
comment from Rob Herring [0].

-- Daniel

[0] https://lore.kernel.org/rust-for-linux/a1b3561d-f5de-4474-85ef-1525a6c36bc5@arm.com/T/#mdf9d4005ee99929d0009ccc988efbc0789164b6d

Re: [PATCH v6 0/2] Add a bare-minimum Regulator abstraction

[Mark Brown]

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

On Tue, Jul 01, 2025 at 12:28:37PM -0300, Daniel Almeida wrote:

> Is it ok for the abstraction to only be built when CONFIG_REGULATOR=y? This
> means that any Rust drivers using it have to depend on CONFIG_REGULATOR too.

> I thought this was acceptable, but apparently that is not the case? See this
> comment from Rob Herring [0].

The regulator API stubs itself out when disabled, but given that this is
just wrappers it's not clear what the tasteful thing would be here - it
should do the right thing because it will itself be built in terms of
the C stubs.  I don't know if Rust can sensibly stub things, or if
there's much percentage in that for a thin wrapper which does basically
nothing itself.

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

Re: [PATCH v6 0/2] Add a bare-minimum Regulator abstraction

[Daniel Almeida]

> On 1 Jul 2025, at 12:34, Mark Brown <broonie@kernel.org> wrote:
> 
> On Tue, Jul 01, 2025 at 12:28:37PM -0300, Daniel Almeida wrote:
> 
>> Is it ok for the abstraction to only be built when CONFIG_REGULATOR=y? This
>> means that any Rust drivers using it have to depend on CONFIG_REGULATOR too.
> 
>> I thought this was acceptable, but apparently that is not the case? See this
>> comment from Rob Herring [0].
> 
> The regulator API stubs itself out when disabled, but given that this is
> just wrappers it's not clear what the tasteful thing would be here - it
> should do the right thing because it will itself be built in terms of
> the C stubs.  I don't know if Rust can sensibly stub things, or if
> there's much percentage in that for a thin wrapper which does basically
> nothing itself.

Well, if all functions in the regulator API have stubs, then perhaps a trivial
solution would be removing this #[cfg] from here:

+#[cfg(CONFIG_REGULATOR)] <---
+pub mod regulator;

This would build the abstractions unconditionally, and it would transparently
use the C stubs when calling the C functions.

Waiting for more comments from the other RFL folks.

-- Daniel

Re: [PATCH v6 0/2] Add a bare-minimum Regulator abstraction

[Miguel Ojeda]

On Tue, Jul 1, 2025 at 6:10 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Well, if all functions in the regulator API have stubs, then perhaps a trivial
> solution would be removing this #[cfg] from here:
>
> +#[cfg(CONFIG_REGULATOR)] <---
> +pub mod regulator;
>
> This would build the abstractions unconditionally, and it would transparently
> use the C stubs when calling the C functions.
>
> Waiting for more comments from the other RFL folks.

If C typically provides an API with stubs because users should not
generally care whether it is enabled or not, then you should try to
mimic that if it makes sense.

There have been similar discussions for e.g. debugfs which you may
want to take a look at:

    https://lore.kernel.org/rust-for-linux/2025043021-plaza-grip-2916@gregkh/

where the callers didn't even want to get a `Result` if not enabled,
and as a result of that, ended up like e.g.:

    https://lore.kernel.org/rust-for-linux/20250627-debugfs-rust-v8-1-c6526e413d40@google.com/

In that case, they ended up with a few `cfg`s, e.g. to optionally have
a field in a struct, but in your case it may be simpler.

I hope that helps.

Cheers,
Miguel

Re: [PATCH v6 1/2] rust: regulator: add a bare minimum regulator abstraction

[Alice Ryhl]

On Fri, Jun 27, 2025 at 7:11 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.
>
> 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>

Overall looks reasonable to me.

> +/// 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 {}

Naming-wise, it's a bit weird that IsEnabled applies to everything
*but* enabled. And also, the is_enabled() method should probably exist
for only Dynamic anyway?

> +/// ## 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> {

The `data` argument is unused. It looks like it should be removed.

> +pub struct Regulator<State = Dynamic>
> +where
> +    State: RegulatorState + 'static,

You can add `: 'static` to the trait itself to avoid having it here.

> +impl<T: RegulatorState + 'static> Drop for Regulator<T> {
> +    fn drop(&mut self) {
> +        if core::any::TypeId::of::<T>() == core::any::TypeId::of::<Enabled>() {

I would avoid this kind of logic. Instead, you can add an
`disable_on_drop()` method or constant to the trait and check it here.

Alice

Re: [PATCH v6 1/2] rust: regulator: add a bare minimum regulator abstraction

[Daniel Almeida]

Hi Alice,

> On 2 Jul 2025, at 07:35, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Fri, Jun 27, 2025 at 7:11 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.
>> 
>> 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>
> 
> Overall looks reasonable to me.
> 
>> +/// 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 {}
> 
> Naming-wise, it's a bit weird that IsEnabled applies to everything
> *but* enabled. And also, the is_enabled() method should probably exist
> for only Dynamic anyway?

I think it's the other way around? Enabled doesn't need this impl precisely
because of the Enabled token. IOW:

Regulator<Enabled>::is_enabled() doesn't make sense.

>  And also, the is_enabled() method should probably exist for only Dynamic anyway?

Also no, because Regulator<Disabled> isn't necessarily disabled. It just was
not enabled by us, but might have been enabled by somebody else in the system.


[…]

> 
> 
>> +impl<T: RegulatorState + 'static> Drop for Regulator<T> {
>> +    fn drop(&mut self) {
>> +        if core::any::TypeId::of::<T>() == core::any::TypeId::of::<Enabled>() {
> 
> I would avoid this kind of logic. Instead, you can add an
> `disable_on_drop()` method or constant to the trait and check it here.
> 
> Alice
> 

Can you expand on this?

— Daniel

Re: [PATCH v6 1/2] rust: regulator: add a bare minimum regulator abstraction

[Alice Ryhl]

On Wed, Jul 2, 2025 at 3:03 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi Alice,
>
> > On 2 Jul 2025, at 07:35, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Fri, Jun 27, 2025 at 7:11 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.
> >>
> >> 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>
> >
> > Overall looks reasonable to me.
> >
> >> +/// 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 {}
> >
> > Naming-wise, it's a bit weird that IsEnabled applies to everything
> > *but* enabled. And also, the is_enabled() method should probably exist
> > for only Dynamic anyway?
>
> I think it's the other way around? Enabled doesn't need this impl precisely
> because of the Enabled token. IOW:
>
> Regulator<Enabled>::is_enabled() doesn't make sense.
>
> >  And also, the is_enabled() method should probably exist for only Dynamic anyway?
>
> Also no, because Regulator<Disabled> isn't necessarily disabled. It just was
> not enabled by us, but might have been enabled by somebody else in the system.

Okay.

> >> +impl<T: RegulatorState + 'static> Drop for Regulator<T> {
> >> +    fn drop(&mut self) {
> >> +        if core::any::TypeId::of::<T>() == core::any::TypeId::of::<Enabled>() {
> >
> > I would avoid this kind of logic. Instead, you can add an
> > `disable_on_drop()` method or constant to the trait and check it here.
> >
> > Alice
> >
>
> Can you expand on this?

Along these lines:

pub trait RegulatorState: 'static {
    const DISABLE_ON_DROP: bool;
}

impl RegulatorState for Enabled {
    const DISABLE_ON_DROP: bool = true;
}
impl RegulatorState for Disabled {
    const DISABLE_ON_DROP: bool = false;
}
impl RegulatorState for Dynamic {
    const DISABLE_ON_DROP: bool = false;
}

impl<T: RegulatorState> Drop for Regulator<T> {
    fn drop(&mut self) {
        if T::DISABLE_ON_DROP {
            unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
        }
        unsafe { bindings::regulator_put(self.inner.as_ptr()) };
    }
}

Re: [PATCH v6 1/2] rust: regulator: add a bare minimum regulator abstraction

[Daniel Almeida]

> 
>>>> +impl<T: RegulatorState + 'static> Drop for Regulator<T> {
>>>> +    fn drop(&mut self) {
>>>> +        if core::any::TypeId::of::<T>() == core::any::TypeId::of::<Enabled>() {
>>> 
>>> I would avoid this kind of logic. Instead, you can add an
>>> `disable_on_drop()` method or constant to the trait and check it here.
>>> 
>>> Alice
>>> 
>> 
>> Can you expand on this?
> 
> Along these lines:
> 
> pub trait RegulatorState: 'static {
>    const DISABLE_ON_DROP: bool;
> }
> 
> impl RegulatorState for Enabled {
>    const DISABLE_ON_DROP: bool = true;
> }
> impl RegulatorState for Disabled {
>    const DISABLE_ON_DROP: bool = false;
> }
> impl RegulatorState for Dynamic {
>    const DISABLE_ON_DROP: bool = false;
> }
> 
> impl<T: RegulatorState> Drop for Regulator<T> {
>    fn drop(&mut self) {
>        if T::DISABLE_ON_DROP {
>            unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
>        }
>        unsafe { bindings::regulator_put(self.inner.as_ptr()) };
>    }
> }

Ah, that indeed looks better, thanks.

— Daniel