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

[PATCH v3] 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 v3:
- Rebased on rust-next
- Added examples to showcase the API
- Fixed some rendering issues in the docs
- Exposed {get|set}_voltage for both Regulator and EnabledRegulator
- Derived Clone, Copy, PartialEq and Eq for Microvolt
- Link to v2: https://lore.kernel.org/r/20250326-topics-tyr-regulator-v2-1-c0ea6a861be6@collabora.com

Resend v2:
  - cc Regulator maintainers
Changes from v1:
  - Rebased on rust-next
  - Split the design into two types as suggested by Alice Ryhl.
  - Modify the docs to highlight how users can use kernel::types::Either
    or an enum to enable and disable the regulator at runtime.
  - Link to v1: https://lore.kernel.org/rust-for-linux/20250219162517.278362-1-daniel.almeida@collabora.com/
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/regulator.rs        | 211 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ab37e1d35c70d52e69b754bf855bc19911d156d8..e14cce03338ef5f6a09a23fd41ca47b8c913fa65 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -31,6 +31,7 @@
 #include <linux/poll.h>
 #include <linux/property.h>
 #include <linux/refcount.h>
+#include <linux/regulator/consumer.h>
 #include <linux/sched.h>
 #include <linux/security.h>
 #include <linux/slab.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 28007be98fbad0e875d7e5345e164e2af2c5da32..c8fd7e4e036e9e5b6958acf0dcfa952b916a3d48 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -86,6 +86,8 @@
 pub mod prelude;
 pub mod print;
 pub mod rbtree;
+#[cfg(CONFIG_REGULATOR)]
+pub mod regulator;
 pub mod revocable;
 pub mod security;
 pub mod seq_file;
diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
new file mode 100644
index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
--- /dev/null
+++ b/rust/kernel/regulator.rs
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Regulator abstractions, providing a standard kernel interface to control
+//! voltage and current regulators.
+//!
+//! The intention is to allow systems to dynamically control regulator power
+//! output in order to save power and prolong battery life. This applies to both
+//! voltage regulators (where voltage output is controllable) and current sinks
+//! (where current limit is controllable).
+//!
+//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
+//!
+//! Regulators are modeled in Rust with two types: [`Regulator`] and
+//! [`EnabledRegulator`].
+//!
+//! The transition between these types is done by calling
+//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
+//!
+//! Use an enum or [`kernel::types::Either`] to gracefully transition between
+//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
+//! otherwise.
+//!
+//! 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::{mem::ManuallyDrop, ptr::NonNull};
+
+/// A `struct regulator` abstraction.
+///
+/// # Examples
+///
+/// Enabling a regulator:
+///
+/// ```
+/// # use kernel::prelude::*;
+/// # use kernel::c_str;
+/// # use kernel::device::Device;
+/// # use kernel::regulator::{Microvolt, Regulator, EnabledRegulator};
+/// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
+///    // Obtain a reference to a (fictitious) regulator.
+///    let regulator: Regulator = Regulator::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
+///    // `EnabledRegulator`.
+///    let regulator: EnabledRegulator = regulator.enable()?;
+///
+///    // 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>(())
+/// }
+///```
+///
+/// Disabling a regulator:
+///
+///```
+/// # use kernel::prelude::*;
+/// # use kernel::c_str;
+/// # use kernel::device::Device;
+/// # use kernel::regulator::{Microvolt, Regulator, EnabledRegulator};
+/// fn disable(dev: &Device, regulator: EnabledRegulator) -> Result {
+///    // We can also disable an enabled regulator without reliquinshing our
+///    // refcount:
+///    let regulator: Regulator = regulator.disable()?;
+///
+///    // The refcount will be decremented when `regulator` is dropped.
+///    drop(regulator);
+///    // ...
+///    # Ok::<(), Error>(())
+/// }
+/// ```
+///
+/// # Invariants
+///
+/// - [`Regulator`] 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).
+/// - Each instance of [`Regulator`] is associated with a single count of
+///   [`regulator_get()`](https://docs.kernel.org/driver-api/regulator.html#c.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 })
+    }
+
+    /// 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(Error::from_errno(voltage))
+        } else {
+            Ok(Microvolt(voltage))
+        }
+    }
+}
+
+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 [`Regulator`] 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()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_enable)
+///   that was obtained from the [`Regulator`] instance once it was enabled.
+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 {
+        self.inner.set_voltage(min_uv, max_uv)
+    }
+
+    /// Gets the current voltage of the regulator.
+    pub fn get_voltage(&self) -> Result<Microvolt> {
+        self.inner.get_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)]
+#[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 v3] rust: regulator: add a bare minimum regulator abstraction

[Benno Lossin]

On Tue May 13, 2025 at 5:44 PM CEST, Daniel Almeida wrote:
> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
> --- /dev/null
> +++ b/rust/kernel/regulator.rs
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Regulator abstractions, providing a standard kernel interface to control
> +//! voltage and current regulators.
> +//!
> +//! The intention is to allow systems to dynamically control regulator power
> +//! output in order to save power and prolong battery life. This applies to both
> +//! voltage regulators (where voltage output is controllable) and current sinks
> +//! (where current limit is controllable).
> +//!
> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
> +//!
> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
> +//! [`EnabledRegulator`].

Would it make sense to store this in a generic variable acting as a type
state instead of using two different names? So:

    pub struct Regulator<State: RegulatorState> { /* ... */ }

    pub trait RegulatorState: private::Sealed {}

    pub struct Enabled;
    pub struct Disabled;

    impl RegulatorState for Enabled {}
    impl RegulatorState for Disabled {}

And then one would use `Regulator<Enabled>` and `Regulator<Disabled>`.

> +//!
> +//! 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.
> +//!
> +//! 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::{mem::ManuallyDrop, ptr::NonNull};
> +
> +/// A `struct regulator` abstraction.
> +///
> +/// # Examples
> +///
> +/// Enabling a regulator:
> +///
> +/// ```
> +/// # use kernel::prelude::*;
> +/// # use kernel::c_str;
> +/// # use kernel::device::Device;
> +/// # use kernel::regulator::{Microvolt, Regulator, EnabledRegulator};
> +/// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
> +///    // Obtain a reference to a (fictitious) regulator.
> +///    let regulator: Regulator = Regulator::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
> +///    // `EnabledRegulator`.
> +///    let regulator: EnabledRegulator = regulator.enable()?;
> +///
> +///    // 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.

Where would you normally store an enabled regulator to keep it alive?
Maybe adjust the example to do just that?

> +///    drop(regulator);
> +///    // ...
> +///    # Ok::<(), Error>(())
> +/// }
> +///```
> +///
> +/// Disabling a regulator:
> +///
> +///```
> +/// # use kernel::prelude::*;
> +/// # use kernel::c_str;
> +/// # use kernel::device::Device;
> +/// # use kernel::regulator::{Microvolt, Regulator, EnabledRegulator};
> +/// fn disable(dev: &Device, regulator: EnabledRegulator) -> Result {
> +///    // We can also disable an enabled regulator without reliquinshing our
> +///    // refcount:
> +///    let regulator: Regulator = regulator.disable()?;
> +///
> +///    // The refcount will be decremented when `regulator` is dropped.
> +///    drop(regulator);
> +///    // ...
> +///    # Ok::<(), Error>(())
> +/// }
> +/// ```
> +///
> +/// # Invariants
> +///
> +/// - [`Regulator`] 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).

This should be "`inner` is a pointer obtained from
[`regulator_get()`](...)".

> +/// - Each instance of [`Regulator`] is associated with a single count of
> +///   [`regulator_get()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_get).

This is redundant, so we should remove it.

> +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 })
> +    }
> +
> +    /// 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(Error::from_errno(voltage))
> +        } else {
> +            Ok(Microvolt(voltage))
> +        }
> +    }
> +}
> +
> +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 [`Regulator`] that is known to be enabled.
> +///
> +/// # Invariants
> +///
> +/// - [`EnabledRegulator`] is a valid regulator that has been enabled.

This isn't fully clear what it's supposed to mean to me. Maybe mention
the `regulator_enable` function?

> +/// - Each instance of [`EnabledRegulator`] is associated with a single count
> +///   of [`regulator_enable()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_enable)
> +///   that was obtained from the [`Regulator`] instance once it was enabled.

Ah I see you mention it here, then what is the bullet point above about?

Also, please refer to `inner` instead of [`EnabledRegulator`].

> +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);

Why don't we drop the refcount if the `regulator_disable` call fails?

> +
> +        // 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 {
> +        self.inner.set_voltage(min_uv, max_uv)
> +    }
> +
> +    /// Gets the current voltage of the regulator.
> +    pub fn get_voltage(&self) -> Result<Microvolt> {
> +        self.inner.get_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()) };

Same here, what happens to the refcount?

---
Cheers,
Benno

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

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

[Mark Brown]

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

On Tue, May 13, 2025 at 10:01:05PM +0200, Benno Lossin wrote:
> On Tue May 13, 2025 at 5:44 PM CEST, Daniel Almeida wrote:

> > +/// A [`Regulator`] that is known to be enabled.
> > +///
> > +/// # Invariants
> > +///
> > +/// - [`EnabledRegulator`] is a valid regulator that has been enabled.

> This isn't fully clear what it's supposed to mean to me. Maybe mention
> the `regulator_enable` function?

I suspect this is adequately clear to someone with the domain specific
knowledge required to be using the API.

> > +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);

> Why don't we drop the refcount if the `regulator_disable` call fails?

If you fail to disable the regulator then the underlying C code won't
drop it's reference count.

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

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

[Mark Brown]

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

On Tue, May 13, 2025 at 12:44:08PM -0300, 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.

Please also add the new file to the MAINTAINERS entry for the regulator
API, otherwise this looks fine from a regulator API point of view.

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

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

[Benno Lossin]

On Wed May 14, 2025 at 9:46 AM CEST, Mark Brown wrote:
> On Tue, May 13, 2025 at 10:01:05PM +0200, Benno Lossin wrote:
>> On Tue May 13, 2025 at 5:44 PM CEST, Daniel Almeida wrote:
>
>> > +/// A [`Regulator`] that is known to be enabled.
>> > +///
>> > +/// # Invariants
>> > +///
>> > +/// - [`EnabledRegulator`] is a valid regulator that has been enabled.
>
>> This isn't fully clear what it's supposed to mean to me. Maybe mention
>> the `regulator_enable` function?
>
> I suspect this is adequately clear to someone with the domain specific
> knowledge required to be using the API.

I still think it's useful to name the exact function that is meant by
"enabled".

>> > +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);
>
>> Why don't we drop the refcount if the `regulator_disable` call fails?
>
> If you fail to disable the regulator then the underlying C code won't
> drop it's reference count.

So if it fails, the regulator should stay alive indefinitely? Would be
useful to explain that in the comment above the `ManuallyDrop`.

---
Cheers,
Benno

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

[Mark Brown]

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

On Wed, May 14, 2025 at 11:37:46AM +0200, Benno Lossin wrote:
> On Wed May 14, 2025 at 9:46 AM CEST, Mark Brown wrote:
> > On Tue, May 13, 2025 at 10:01:05PM +0200, Benno Lossin wrote:

> >> This isn't fully clear what it's supposed to mean to me. Maybe mention
> >> the `regulator_enable` function?

> > I suspect this is adequately clear to someone with the domain specific
> > knowledge required to be using the API.

> I still think it's useful to name the exact function that is meant by
> "enabled".

It's not clear to me that it's helpful to have to refer to the C API, as
opposed to just being free standing.

> >> Why don't we drop the refcount if the `regulator_disable` call fails?

> > If you fail to disable the regulator then the underlying C code won't
> > drop it's reference count.

> So if it fails, the regulator should stay alive indefinitely? Would be
> useful to explain that in the comment above the `ManuallyDrop`.

Practically speaking if the regulator disable fails the system is having
an extremely bad time and the actual state of the regulator is not clear.
Users might want to try some attempt at retrying, one of which could
possibly succeed in future, but realistically if this happens there's
something fairly catastrophic going on.  Some critical users might want
to care and have a good idea what makes sense for them, but probably the
majority of users of the API aren't going to have a good strategy here.

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

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

[Benno Lossin]

On Wed May 14, 2025 at 12:16 PM CEST, Mark Brown wrote:
> On Wed, May 14, 2025 at 11:37:46AM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 9:46 AM CEST, Mark Brown wrote:
>> > On Tue, May 13, 2025 at 10:01:05PM +0200, Benno Lossin wrote:
>
>> >> This isn't fully clear what it's supposed to mean to me. Maybe mention
>> >> the `regulator_enable` function?
>
>> > I suspect this is adequately clear to someone with the domain specific
>> > knowledge required to be using the API.
>
>> I still think it's useful to name the exact function that is meant by
>> "enabled".
>
> It's not clear to me that it's helpful to have to refer to the C API, as
> opposed to just being free standing.

To me it would be much more clear if the function were named.

>> >> Why don't we drop the refcount if the `regulator_disable` call fails?
>
>> > If you fail to disable the regulator then the underlying C code won't
>> > drop it's reference count.
>
>> So if it fails, the regulator should stay alive indefinitely? Would be
>> useful to explain that in the comment above the `ManuallyDrop`.
>
> Practically speaking if the regulator disable fails the system is having
> an extremely bad time and the actual state of the regulator is not clear.
> Users might want to try some attempt at retrying, one of which could
> possibly succeed in future, but realistically if this happens there's
> something fairly catastrophic going on.  Some critical users might want
> to care and have a good idea what makes sense for them, but probably the
> majority of users of the API aren't going to have a good strategy here.

Makes sense. So does `regulator_disable` take ownership of the refcount?
If yes, then just put that in the comment above the `ManuallyDrop` & in
the `Drop` impl of `EnabledRegulator`.

---
Cheers,
Benno

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

[Mark Brown]

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

On Wed, May 14, 2025 at 12:31:38PM +0200, Benno Lossin wrote:
> On Wed May 14, 2025 at 12:16 PM CEST, Mark Brown wrote:

> > Practically speaking if the regulator disable fails the system is having
> > an extremely bad time and the actual state of the regulator is not clear.
> > Users might want to try some attempt at retrying, one of which could
> > possibly succeed in future, but realistically if this happens there's
> > something fairly catastrophic going on.  Some critical users might want
> > to care and have a good idea what makes sense for them, but probably the
> > majority of users of the API aren't going to have a good strategy here.

> Makes sense. So does `regulator_disable` take ownership of the refcount?
> If yes, then just put that in the comment above the `ManuallyDrop` & in
> the `Drop` impl of `EnabledRegulator`.

In the C API the disable operation just fails and it's treated as though
you hadn't done anything from a refcounting point of view.

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

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

[Benno Lossin]

On Wed May 14, 2025 at 1:50 PM CEST, Mark Brown wrote:
> On Wed, May 14, 2025 at 12:31:38PM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 12:16 PM CEST, Mark Brown wrote:
>> > Practically speaking if the regulator disable fails the system is having
>> > an extremely bad time and the actual state of the regulator is not clear.
>> > Users might want to try some attempt at retrying, one of which could
>> > possibly succeed in future, but realistically if this happens there's
>> > something fairly catastrophic going on.  Some critical users might want
>> > to care and have a good idea what makes sense for them, but probably the
>> > majority of users of the API aren't going to have a good strategy here.
>
>> Makes sense. So does `regulator_disable` take ownership of the refcount?
>> If yes, then just put that in the comment above the `ManuallyDrop` & in
>> the `Drop` impl of `EnabledRegulator`.
>
> In the C API the disable operation just fails and it's treated as though
> you hadn't done anything from a refcounting point of view.

But if it succeeds it takes ownership? The function `regulator_disable`
is also used in the `Drop` impl of the `EnabledRegulator`, so it better
give up the refcount, otherwise we would leak it.

---
Cheers,
Benno

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

[Mark Brown]

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

On Wed, May 14, 2025 at 02:23:04PM +0200, Benno Lossin wrote:
> On Wed May 14, 2025 at 1:50 PM CEST, Mark Brown wrote:

> > In the C API the disable operation just fails and it's treated as though
> > you hadn't done anything from a refcounting point of view.

> But if it succeeds it takes ownership? The function `regulator_disable`
> is also used in the `Drop` impl of the `EnabledRegulator`, so it better
> give up the refcount, otherwise we would leak it.

I can't understand what you are saying at all, sorry.  What does "take
ownership" mean, and what is the "it" here?  We are talking about the
case where regulator_disable() fails here, that means it didn't do what
it was asked to do.

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

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

[Daniel Almeida]

Hi Benno,

> On 13 May 2025, at 17:01, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Tue May 13, 2025 at 5:44 PM CEST, Daniel Almeida wrote:
>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
>> --- /dev/null
>> +++ b/rust/kernel/regulator.rs
>> @@ -0,0 +1,211 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Regulator abstractions, providing a standard kernel interface to control
>> +//! voltage and current regulators.
>> +//!
>> +//! The intention is to allow systems to dynamically control regulator power
>> +//! output in order to save power and prolong battery life. This applies to both
>> +//! voltage regulators (where voltage output is controllable) and current sinks
>> +//! (where current limit is controllable).
>> +//!
>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>> +//!
>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>> +//! [`EnabledRegulator`].
> 
> Would it make sense to store this in a generic variable acting as a type
> state instead of using two different names? So:
> 
>    pub struct Regulator<State: RegulatorState> { /* ... */ }
> 
>    pub trait RegulatorState: private::Sealed {}
> 
>    pub struct Enabled;
>    pub struct Disabled;
> 
>    impl RegulatorState for Enabled {}
>    impl RegulatorState for Disabled {}
> 
> And then one would use `Regulator<Enabled>` and `Regulator<Disabled>`.

This seems like just another way of doing the same thing.

I have nothing against a typestate, it's an elegant solution really, but so is
the current one. I'd say let's keep what we have unless there is something
objectively better about a typestatethat makes it worthy to change this.

> 
>> +//!
>> +//! 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.
>> +//!
>> +//! 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::{mem::ManuallyDrop, ptr::NonNull};
>> +
>> +/// A `struct regulator` abstraction.
>> +///
>> +/// # Examples
>> +///
>> +/// Enabling a regulator:
>> +///
>> +/// ```
>> +/// # use kernel::prelude::*;
>> +/// # use kernel::c_str;
>> +/// # use kernel::device::Device;
>> +/// # use kernel::regulator::{Microvolt, Regulator, EnabledRegulator};
>> +/// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
>> +///    // Obtain a reference to a (fictitious) regulator.
>> +///    let regulator: Regulator = Regulator::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
>> +///    // `EnabledRegulator`.
>> +///    let regulator: EnabledRegulator = regulator.enable()?;
>> +///
>> +///    // 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.
> 
> Where would you normally store an enabled regulator to keep it alive?
> Maybe adjust the example to do just that?

In your driver’s private data.

> 
>> +///    drop(regulator);
>> +///    // ...
>> +///    # Ok::<(), Error>(())
>> +/// }
>> +///```
>> +///
>> +/// Disabling a regulator:
>> +///
>> +///```
>> +/// # use kernel::prelude::*;
>> +/// # use kernel::c_str;
>> +/// # use kernel::device::Device;
>> +/// # use kernel::regulator::{Microvolt, Regulator, EnabledRegulator};
>> +/// fn disable(dev: &Device, regulator: EnabledRegulator) -> Result {
>> +///    // We can also disable an enabled regulator without reliquinshing our
>> +///    // refcount:
>> +///    let regulator: Regulator = regulator.disable()?;
>> +///
>> +///    // The refcount will be decremented when `regulator` is dropped.
>> +///    drop(regulator);
>> +///    // ...
>> +///    # Ok::<(), Error>(())
>> +/// }
>> +/// ```
>> +///
>> +/// # Invariants
>> +///
>> +/// - [`Regulator`] 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).
> 
> This should be "`inner` is a pointer obtained from
> [`regulator_get()`](...)".
> 
>> +/// - Each instance of [`Regulator`] is associated with a single count of
>> +///   [`regulator_get()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_get).
> 
> This is redundant, so we should remove it.

Why is this redundant? It says that we are associated with a *single* refcount.

> 
>> +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 })
>> +    }
>> +
>> +    /// 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(Error::from_errno(voltage))
>> +        } else {
>> +            Ok(Microvolt(voltage))
>> +        }
>> +    }
>> +}
>> +
>> +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 [`Regulator`] that is known to be enabled.
>> +///
>> +/// # Invariants
>> +///
>> +/// - [`EnabledRegulator`] is a valid regulator that has been enabled.
> 
> This isn't fully clear what it's supposed to mean to me. Maybe mention
> the `regulator_enable` function?
> 
>> +/// - Each instance of [`EnabledRegulator`] is associated with a single count
>> +///   of [`regulator_enable()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_enable)
>> +///   that was obtained from the [`Regulator`] instance once it was enabled.
> 
> Ah I see you mention it here, then what is the bullet point above about?

Again, the word _single_ is what is important here.

> 
> Also, please refer to `inner` instead of [`EnabledRegulator`].
> 
>> +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);
> 
> Why don't we drop the refcount if the `regulator_disable` call fails?

I am operating under the assumption that regulator_enable() and
regulator_disable() do not touch the reference count. Note that we do not
acquire a new reference when we build EnabledRegulator in Regulator::enable(),
we merely move our instance of Regulator into EnabledRegulator.

disable() takes EnabledRegulator by value in order to convert it to Regulator.
If we let the destructor run, that will decrement the refcount as it calls
inner.drop(), so that is why the ManuallyDrop is there in the first place.  

Now if disable() fails, perhaps we should somehow return `self` to the caller.
That would let them retry the operation, even if it's unlikely to be of any
help, as Mark said. In this sense, I agree that there is a leak that I
overlooked.

On a second note, Benno, do you have a clean way to return both kernel::Error
and `self` here? I assume that introducing a separate error type just for this
function is overkill.


>> +
>> +        // 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 {
>> +        self.inner.set_voltage(min_uv, max_uv)
>> +    }
>> +
>> +    /// Gets the current voltage of the regulator.
>> +    pub fn get_voltage(&self) -> Result<Microvolt> {
>> +        self.inner.get_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()) };
> 
> Same here, what happens to the refcount?

It remains the same, we never acquired one when we enabled, so we are relying
on inner.drop() to decrement it.

> 
> ---
> Cheers,
> Benno
> 
>> +    }
>> +}
>> +
>> +/// 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

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

[Benno Lossin]

On Wed May 14, 2025 at 3:01 PM CEST, Daniel Almeida wrote:
>> On 13 May 2025, at 17:01, Benno Lossin <lossin@kernel.org> wrote:
>> On Tue May 13, 2025 at 5:44 PM CEST, Daniel Almeida wrote:
>>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
>>> --- /dev/null
>>> +++ b/rust/kernel/regulator.rs
>>> @@ -0,0 +1,211 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>> +//! voltage and current regulators.
>>> +//!
>>> +//! The intention is to allow systems to dynamically control regulator power
>>> +//! output in order to save power and prolong battery life. This applies to both
>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>> +//! (where current limit is controllable).
>>> +//!
>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>> +//!
>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>> +//! [`EnabledRegulator`].
>> 
>> Would it make sense to store this in a generic variable acting as a type
>> state instead of using two different names? So:
>> 
>>    pub struct Regulator<State: RegulatorState> { /* ... */ }
>> 
>>    pub trait RegulatorState: private::Sealed {}
>> 
>>    pub struct Enabled;
>>    pub struct Disabled;
>> 
>>    impl RegulatorState for Enabled {}
>>    impl RegulatorState for Disabled {}
>> 
>> And then one would use `Regulator<Enabled>` and `Regulator<Disabled>`.
>
> This seems like just another way of doing the same thing.
>
> I have nothing against a typestate, it's an elegant solution really, but so is
> the current one. I'd say let's keep what we have unless there is something
> objectively better about a typestatethat makes it worthy to change this.

I'd say it's cleaner and we already have some APIs that utilize type
states, so I'd prefer we use that where it makes sense.

>>> +/// # Invariants
>>> +///
>>> +/// - [`Regulator`] 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).
>> 
>> This should be "`inner` is a pointer obtained from
>> [`regulator_get()`](...)".
>> 
>>> +/// - Each instance of [`Regulator`] is associated with a single count of
>>> +///   [`regulator_get()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_get).
>> 
>> This is redundant, so we should remove it.
>
> Why is this redundant? It says that we are associated with a *single* refcount.

I'd fold it into the previous bullet point. To me they aren't really
disjoint.

>>> +/// A [`Regulator`] that is known to be enabled.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// - [`EnabledRegulator`] is a valid regulator that has been enabled.
>> 
>> This isn't fully clear what it's supposed to mean to me. Maybe mention
>> the `regulator_enable` function?
>> 
>>> +/// - Each instance of [`EnabledRegulator`] is associated with a single count
>>> +///   of [`regulator_enable()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_enable)
>>> +///   that was obtained from the [`Regulator`] instance once it was enabled.
>> 
>> Ah I see you mention it here, then what is the bullet point above about?
>
> Again, the word _single_ is what is important here.

I don't see the point in stating it in two separate bullet points.

>>> +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);
>> 
>> Why don't we drop the refcount if the `regulator_disable` call fails?
>
> I am operating under the assumption that regulator_enable() and
> regulator_disable() do not touch the reference count. Note that we do not
> acquire a new reference when we build EnabledRegulator in Regulator::enable(),
> we merely move our instance of Regulator into EnabledRegulator.
>
> disable() takes EnabledRegulator by value in order to convert it to Regulator.
> If we let the destructor run, that will decrement the refcount as it calls
> inner.drop(), so that is why the ManuallyDrop is there in the first place.  
>
> Now if disable() fails, perhaps we should somehow return `self` to the caller.
> That would let them retry the operation, even if it's unlikely to be of any
> help, as Mark said. In this sense, I agree that there is a leak that I
> overlooked.

I see two different options on what to do about the leak:

(1) implement `disable` in terms of a new function `try_disable`
    returning `Result<Regulator, DisableError>` where `DisableError`
    gives access to the `EnabledRegulator`.

(2) document that the `disable` function leaks the refcount if an error
    is returned.

I have a slight preference for (1), but if Mark & you decide that it's
not necessary, since users of regulators won't need it any time soon,
then go with (2).

> On a second note, Benno, do you have a clean way to return both kernel::Error
> and `self` here? I assume that introducing a separate error type just for this
> function is overkill.

It's pretty common in Rust to have a custom error type for those cases.
I think we should also do it here, Alice already used the pattern in the
alloc module [1].

[1]: https://lore.kernel.org/all/20250502-vec-methods-v5-3-06d20ad9366f@google.com

>>> +
>>> +        // 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 {
>>> +        self.inner.set_voltage(min_uv, max_uv)
>>> +    }
>>> +
>>> +    /// Gets the current voltage of the regulator.
>>> +    pub fn get_voltage(&self) -> Result<Microvolt> {
>>> +        self.inner.get_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()) };
>> 
>> Same here, what happens to the refcount?
>
> It remains the same, we never acquired one when we enabled, so we are relying
> on inner.drop() to decrement it.

Ah I overlooked that. Then it's fine.

---
Cheers,
Benno

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

[Benno Lossin]

On Wed May 14, 2025 at 2:48 PM CEST, Mark Brown wrote:
> On Wed, May 14, 2025 at 02:23:04PM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 1:50 PM CEST, Mark Brown wrote:
>
>> > In the C API the disable operation just fails and it's treated as though
>> > you hadn't done anything from a refcounting point of view.
>
>> But if it succeeds it takes ownership? The function `regulator_disable`
>> is also used in the `Drop` impl of the `EnabledRegulator`, so it better
>> give up the refcount, otherwise we would leak it.
>
> I can't understand what you are saying at all, sorry.  What does "take
> ownership" mean, and what is the "it" here?  We are talking about the
> case where regulator_disable() fails here, that means it didn't do what
> it was asked to do.

My confusion got cleared by what Daniel wrote:

> I am operating under the assumption that regulator_enable() and
> regulator_disable() do not touch the reference count. Note that we do not
> acquire a new reference when we build EnabledRegulator in Regulator::enable(),
> we merely move our instance of Regulator into EnabledRegulator.

and

>>> +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()) };
>>
>> Same here, what happens to the refcount?
>
> It remains the same, we never acquired one when we enabled, so we are relying
> on inner.drop() to decrement it.

I'll try to explain what my initial question regardless, maybe that'll
clear up your confusion :)

My initial question was whether `regulator_disable` would drop the
refcount (in the case of success and/or in the case of failure). Since
if it failed in the quoted code:

> +    /// 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()) });
So this call would fail                           ^^^^^^^^^^^^^^^^^

> +
> +        res.map(|()| Regulator {
> +            inner: regulator.inner.inner,
> +        })
> +    }

Then the `.map` call below the `regulator_disable` call would not take
ownership of the `inner.inner` value of the `regulator` variable, since
the `res` variable would be the `Err` variant of the `Result` enum (the
closure supplied to the `map` function only operates on the `Ok`
variant). This would mean that the refcount is not decremented, but the
`Regulator` wouldn't be available to the caller any more, thus leaking
the refcount.

Now if the `regulator_disable` function took ownership of the refcount
(ie decrement it, or store the regulator somewhere else that would own
that refcount), then this would be fine.

See my reply to Daniel as for what I suggest to do about the refcount
leak.

---
Cheers,
Benno

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

[Daniel Almeida]

> On 14 May 2025, at 10:57, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Wed May 14, 2025 at 3:01 PM CEST, Daniel Almeida wrote:
>>> On 13 May 2025, at 17:01, Benno Lossin <lossin@kernel.org> wrote:
>>> On Tue May 13, 2025 at 5:44 PM CEST, Daniel Almeida wrote:
>>>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>>>> new file mode 100644
>>>> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
>>>> --- /dev/null
>>>> +++ b/rust/kernel/regulator.rs
>>>> @@ -0,0 +1,211 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>>> +//! voltage and current regulators.
>>>> +//!
>>>> +//! The intention is to allow systems to dynamically control regulator power
>>>> +//! output in order to save power and prolong battery life. This applies to both
>>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>>> +//! (where current limit is controllable).
>>>> +//!
>>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>>> +//!
>>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>>> +//! [`EnabledRegulator`].
>>> 
>>> Would it make sense to store this in a generic variable acting as a type
>>> state instead of using two different names? So:
>>> 
>>>   pub struct Regulator<State: RegulatorState> { /* ... */ }
>>> 
>>>   pub trait RegulatorState: private::Sealed {}
>>> 
>>>   pub struct Enabled;
>>>   pub struct Disabled;
>>> 
>>>   impl RegulatorState for Enabled {}
>>>   impl RegulatorState for Disabled {}
>>> 
>>> And then one would use `Regulator<Enabled>` and `Regulator<Disabled>`.
>> 
>> This seems like just another way of doing the same thing.
>> 
>> I have nothing against a typestate, it's an elegant solution really, but so is
>> the current one. I'd say let's keep what we have unless there is something
>> objectively better about a typestatethat makes it worthy to change this.
> 
> I'd say it's cleaner and we already have some APIs that utilize type
> states, so I'd prefer we use that where it makes sense.
> 

By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
It just tells the system that you don't care about it being enabled anymore. It can
still remain on if there are other users.

This means that Regulator<Disabled> is a misnomer.

Also, the current solution relies on Regulator being a member of
EnabledRegulator to keep the refcounts sane. I wonder how that is going to work
now that Regulator<Disabled> is obviously not a member of Regulator<Enabled>, i.e.:

impl Drop for Regulator<Enabled> {
 fn drop(&mut self) {
  regulator_disable();  
  
  // We now have to call this explicitly, because no one else will call it for
  // us.
  regulator_put();
 }
}

impl Drop for Regulator<Disabled> {
 fn drop(&mut self) {
  // We now have to repeat this in both destructors.
  regulator_put();  
 }
}

Just to confirm: is that what you have in mind?

— Daniel

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

[Benno Lossin]

On Wed May 14, 2025 at 4:40 PM CEST, Daniel Almeida wrote:
>> On 14 May 2025, at 10:57, Benno Lossin <lossin@kernel.org> wrote:
>> On Wed May 14, 2025 at 3:01 PM CEST, Daniel Almeida wrote:
>>>> On 13 May 2025, at 17:01, Benno Lossin <lossin@kernel.org> wrote:
>>>> On Tue May 13, 2025 at 5:44 PM CEST, Daniel Almeida wrote:
>>>>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>>>>> new file mode 100644
>>>>> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
>>>>> --- /dev/null
>>>>> +++ b/rust/kernel/regulator.rs
>>>>> @@ -0,0 +1,211 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>>>> +//! voltage and current regulators.
>>>>> +//!
>>>>> +//! The intention is to allow systems to dynamically control regulator power
>>>>> +//! output in order to save power and prolong battery life. This applies to both
>>>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>>>> +//! (where current limit is controllable).
>>>>> +//!
>>>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>>>> +//!
>>>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>>>> +//! [`EnabledRegulator`].
>>>> 
>>>> Would it make sense to store this in a generic variable acting as a type
>>>> state instead of using two different names? So:
>>>> 
>>>>   pub struct Regulator<State: RegulatorState> { /* ... */ }
>>>> 
>>>>   pub trait RegulatorState: private::Sealed {}
>>>> 
>>>>   pub struct Enabled;
>>>>   pub struct Disabled;
>>>> 
>>>>   impl RegulatorState for Enabled {}
>>>>   impl RegulatorState for Disabled {}
>>>> 
>>>> And then one would use `Regulator<Enabled>` and `Regulator<Disabled>`.
>>> 
>>> This seems like just another way of doing the same thing.
>>> 
>>> I have nothing against a typestate, it's an elegant solution really, but so is
>>> the current one. I'd say let's keep what we have unless there is something
>>> objectively better about a typestatethat makes it worthy to change this.
>> 
>> I'd say it's cleaner and we already have some APIs that utilize type
>> states, so I'd prefer we use that where it makes sense.
>> 
>
> By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
> It just tells the system that you don't care about it being enabled anymore. It can
> still remain on if there are other users.

Hmm, so a `struct regulator` might already be enabled and calling
`regulator_enable` doesn't do anything?

> This means that Regulator<Disabled> is a misnomer.

Yeah.

> Also, the current solution relies on Regulator being a member of
> EnabledRegulator to keep the refcounts sane. I wonder how that is going to work
> now that Regulator<Disabled> is obviously not a member of Regulator<Enabled>, i.e.:
>
> impl Drop for Regulator<Enabled> {
>  fn drop(&mut self) {
>   regulator_disable();  
>   
>   // We now have to call this explicitly, because no one else will call it for
>   // us.
>   regulator_put();
>  }
> }
>
> impl Drop for Regulator<Disabled> {
>  fn drop(&mut self) {
>   // We now have to repeat this in both destructors.
>   regulator_put();  
>  }
> }
>
> Just to confirm: is that what you have in mind?

You can't specialize the `Drop` impl, you'd have to do this:

    impl<State: RegulatorState> Drop for Regulator<State> {
        fn drop(&mut self) {
            if State::ENABLED {
                regulator_disable();
            }
            regulator_put();
        }
    }

I still think it's an improvement though.

---
Cheers,
Benno

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

[Mark Brown]

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

On Wed, May 14, 2025 at 11:40:15AM -0300, Daniel Almeida wrote:

> By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
> It just tells the system that you don't care about it being enabled anymore. It can
> still remain on if there are other users.

Or just if there's no physical control of the regulator, or no
permission to control it.  It's logically disabled from the point of
view of the consumer - the consumer can't assume that the regulator will
be enabled unless it holds a reference on the regulator.

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

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

[Mark Brown]

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

On Wed, May 14, 2025 at 05:38:40PM +0200, Benno Lossin wrote:
> On Wed May 14, 2025 at 4:40 PM CEST, Daniel Almeida wrote:

> > By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
> > It just tells the system that you don't care about it being enabled anymore. It can
> > still remain on if there are other users.

> Hmm, so a `struct regulator` might already be enabled and calling
> `regulator_enable` doesn't do anything?

It takes a reference to the regulator.  This may or may not result in a
change in an underlying physical regulator.

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

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

[Benno Lossin]

On Wed May 14, 2025 at 5:50 PM CEST, Mark Brown wrote:
> On Wed, May 14, 2025 at 05:38:40PM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 4:40 PM CEST, Daniel Almeida wrote:
>> > By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
>> > It just tells the system that you don't care about it being enabled anymore. It can
>> > still remain on if there are other users.
>
>> Hmm, so a `struct regulator` might already be enabled and calling
>> `regulator_enable` doesn't do anything?
>
> It takes a reference to the regulator.  This may or may not result in a
> change in an underlying physical regulator.

Gotcha. So calling `regulator_enable` twice on the same regulator is
fine?

If that is the case -- and after re-reading the functions exposed on
both types `EnabledRegulator` and `Regulator` -- I am confused why we
even need two different type states? Both expose the same functions
(except `enable` and `disable`) and I don't otherwise see the purpose of
having two types.

---
Cheers,
Benno

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

[Mark Brown]

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

On Wed, May 14, 2025 at 06:05:11PM +0200, Benno Lossin wrote:

> Gotcha. So calling `regulator_enable` twice on the same regulator is
> fine?

Yes, absolutely.  You might potentially see this with a multi-function
device where multiple functions on the device need the same supply in
order to help reduce coordination requirements.

> If that is the case -- and after re-reading the functions exposed on
> both types `EnabledRegulator` and `Regulator` -- I am confused why we
> even need two different type states? Both expose the same functions
> (except `enable` and `disable`) and I don't otherwise see the purpose of
> having two types.

IIUC the point is to allow Rust's type system to keep track of the
reference on the regulator, otherwise the user code has to keep track of
the number of enables it's done like it currently does in C code.

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

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

[Daniel Almeida]

> On 14 May 2025, at 12:50, Mark Brown <broonie@kernel.org> wrote:
> 
> On Wed, May 14, 2025 at 05:38:40PM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 4:40 PM CEST, Daniel Almeida wrote:
> 
>>> By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
>>> It just tells the system that you don't care about it being enabled anymore. It can
>>> still remain on if there are other users.
> 
>> Hmm, so a `struct regulator` might already be enabled and calling
>> `regulator_enable` doesn't do anything?
> 
> It takes a reference to the regulator.  This may or may not result in a
> change in an underlying physical regulator.


I assume these are two different reference counts, right? One for
regulator_get()/regulator_put(), and one for
regulator_enable()/regulator_disable().

Looking at regulator_dev, I can see both "use_count" and "open_count" for
example.

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

[Daniel Almeida]

> On 14 May 2025, at 13:05, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Wed May 14, 2025 at 5:50 PM CEST, Mark Brown wrote:
>> On Wed, May 14, 2025 at 05:38:40PM +0200, Benno Lossin wrote:
>>> On Wed May 14, 2025 at 4:40 PM CEST, Daniel Almeida wrote:
>>>> By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
>>>> It just tells the system that you don't care about it being enabled anymore. It can
>>>> still remain on if there are other users.
>> 
>>> Hmm, so a `struct regulator` might already be enabled and calling
>>> `regulator_enable` doesn't do anything?
>> 
>> It takes a reference to the regulator.  This may or may not result in a
>> change in an underlying physical regulator.
> 
> Gotcha. So calling `regulator_enable` twice on the same regulator is
> fine?
> 
> If that is the case -- and after re-reading the functions exposed on
> both types `EnabledRegulator` and `Regulator` -- I am confused why we
> even need two different type states? Both expose the same functions
> (except `enable` and `disable`) and I don't otherwise see the purpose of
> having two types.
> 
> ---
> Cheers,
> Benno
> 


As Mark said:

> IIUC the point is to allow Rust's type system to keep track of the
> reference on the regulator, otherwise the user code has to keep track of
> the number of enables it's done like it currently does in C code.

So this all started because keeping track of the enables was rather clunky. See
v1 [0].

[0] https://lore.kernel.org/rust-for-linux/20250219162517.278362-1-daniel.almeida@collabora.com/

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

[Benno Lossin]

On Wed May 14, 2025 at 6:19 PM CEST, Daniel Almeida wrote:
>> On 14 May 2025, at 13:05, Benno Lossin <lossin@kernel.org> wrote:
>> On Wed May 14, 2025 at 5:50 PM CEST, Mark Brown wrote:
>>> On Wed, May 14, 2025 at 05:38:40PM +0200, Benno Lossin wrote:
>>>> On Wed May 14, 2025 at 4:40 PM CEST, Daniel Almeida wrote:
>>>>> By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
>>>>> It just tells the system that you don't care about it being enabled anymore. It can
>>>>> still remain on if there are other users.
>>> 
>>>> Hmm, so a `struct regulator` might already be enabled and calling
>>>> `regulator_enable` doesn't do anything?
>>> 
>>> It takes a reference to the regulator.  This may or may not result in a
>>> change in an underlying physical regulator.
>> 
>> Gotcha. So calling `regulator_enable` twice on the same regulator is
>> fine?
>> 
>> If that is the case -- and after re-reading the functions exposed on
>> both types `EnabledRegulator` and `Regulator` -- I am confused why we
>> even need two different type states? Both expose the same functions
>> (except `enable` and `disable`) and I don't otherwise see the purpose of
>> having two types.
>> 
>> ---
>> Cheers,
>> Benno
>> 
>
>
> As Mark said:
>
>> IIUC the point is to allow Rust's type system to keep track of the
>> reference on the regulator, otherwise the user code has to keep track of
>> the number of enables it's done like it currently does in C code.
>
> So this all started because keeping track of the enables was rather clunky. See
> v1 [0].
>
> [0] https://lore.kernel.org/rust-for-linux/20250219162517.278362-1-daniel.almeida@collabora.com/

Ahh thanks for the pointer this makes much more sense now. Yeah that's a
reason to have two types.

Please document this properly. A good reference is
`rust/kernel/fs/file.rs`. It also deals with different refcounts and
ownership, so that might give you some idea for how to write the
comments around changes in ownership. Also have a general comment about
the two different refcounts in a `struct regulator` on the two regulator
wrappers.

---
Cheers,
Benno

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

[Mark Brown]

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

On Wed, May 14, 2025 at 01:10:52PM -0300, Daniel Almeida wrote:
> > On 14 May 2025, at 12:50, Mark Brown <broonie@kernel.org> wrote:

> >> Hmm, so a `struct regulator` might already be enabled and calling
> >> `regulator_enable` doesn't do anything?

> > It takes a reference to the regulator.  This may or may not result in a
> > change in an underlying physical regulator.

> I assume these are two different reference counts, right? One for
> regulator_get()/regulator_put(), and one for
> regulator_enable()/regulator_disable().

> Looking at regulator_dev, I can see both "use_count" and "open_count" for
> example.

Yes, there are separate counts for gets and enables.

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

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

[Alexandre Courbot]

Hi Daniel,

On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
> Add a bare minimum regulator abstraction to be used by Rust drivers.
> This abstraction adds a small subset of the regulator API, which is
> thought to be sufficient for the drivers we have now.
>
> Regulators provide the power needed by many hardware blocks and thus are
> likely to be needed by a lot of drivers.
>
> It was tested on rk3588, where it was used to power up the "mali"
> regulator in order to power up the GPU.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@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        | 211 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 214 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index ab37e1d35c70d52e69b754bf855bc19911d156d8..e14cce03338ef5f6a09a23fd41ca47b8c913fa65 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -31,6 +31,7 @@
>  #include <linux/poll.h>
>  #include <linux/property.h>
>  #include <linux/refcount.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/sched.h>
>  #include <linux/security.h>
>  #include <linux/slab.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 28007be98fbad0e875d7e5345e164e2af2c5da32..c8fd7e4e036e9e5b6958acf0dcfa952b916a3d48 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -86,6 +86,8 @@
>  pub mod prelude;
>  pub mod print;
>  pub mod rbtree;
> +#[cfg(CONFIG_REGULATOR)]
> +pub mod regulator;
>  pub mod revocable;
>  pub mod security;
>  pub mod seq_file;
> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
> --- /dev/null
> +++ b/rust/kernel/regulator.rs
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Regulator abstractions, providing a standard kernel interface to control
> +//! voltage and current regulators.
> +//!
> +//! The intention is to allow systems to dynamically control regulator power
> +//! output in order to save power and prolong battery life. This applies to both
> +//! voltage regulators (where voltage output is controllable) and current sinks
> +//! (where current limit is controllable).
> +//!
> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
> +//!
> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
> +//! [`EnabledRegulator`].
> +//!
> +//! The transition between these types is done by calling
> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
> +//!
> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
> +//! otherwise.

Having the enabled or disabled state baked into the type is indeed
valuable for drivers that just need to acquire and enable a regulator at
probe time. However, there are also more dynamic use cases and I don't
think the burden of managing this aspect - by either performing a manual
match to call any method (even the shared ones), or implementing custom
dispatch types (which will lead to many similar ad-hoc implementations)
- should fall on the user. Thus I strongly suggest that this module
provides a solution for this as well.

It has been proposed earlier to use a typestate, and this would indeed
provide several benefits, the first one being the ability to have shared
impl blocks (and shared documentation) between the enabled and disabled
states for methods like set/get_voltage().

But the key benefit I see is that it could also address the
aforementioned dynamic management problem through the introduction of a
third state.

Alongside the `Enabled` and `Disabled` states, there would be a third
state (`Dynamic`?) in which the regulator could either be enabled or
disabled. This `Dynamic` state is the only one providing `enable` and
`disable` methods (as well as `is_enabled`) to change its operational
state without affecting its type.

All three states then implement `set_voltage` and `get_voltage` through
a common impl block, that could be extended with other methods from the
C API that are independent of the state, as needed.

To handle typestate transitions:

- The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
  method to transition the regulator to the `Enabled` state.
- The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
- `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
  fail).

Essentially, the `Enabled` and `Disabled` states simply enforce an
additional operational state invariant on the underlying regulator, and
do not provide methods to change it.

The `Dynamic` state would be the default for `Regulator`, so by just
using `Regulator`, the user gets an interface that works very similarly
to the C API it abstracts, making it intuitive to those familiar with
it.

But for cases where the regulator is known to always be in a specific
state like `Enabled`, one can use `Regulator<Enabled>` and simplify
their code.

This should retain the compile-time safety that your version proposed
for common cases, while still exposing the flexibility of the C API when
needed.

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

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

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

[Benno Lossin]

On Sun May 18, 2025 at 4:28 AM CEST, Alexandre Courbot wrote:
> On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
>> +//! Regulator abstractions, providing a standard kernel interface to control
>> +//! voltage and current regulators.
>> +//!
>> +//! The intention is to allow systems to dynamically control regulator power
>> +//! output in order to save power and prolong battery life. This applies to both
>> +//! voltage regulators (where voltage output is controllable) and current sinks
>> +//! (where current limit is controllable).
>> +//!
>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>> +//!
>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>> +//! [`EnabledRegulator`].
>> +//!
>> +//! The transition between these types is done by calling
>> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
>> +//!
>> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
>> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
>> +//! otherwise.
>
> Having the enabled or disabled state baked into the type is indeed
> valuable for drivers that just need to acquire and enable a regulator at
> probe time. However, there are also more dynamic use cases and I don't
> think the burden of managing this aspect - by either performing a manual
> match to call any method (even the shared ones), or implementing custom
> dispatch types (which will lead to many similar ad-hoc implementations)
> - should fall on the user. Thus I strongly suggest that this module
> provides a solution for this as well.
>
> It has been proposed earlier to use a typestate, and this would indeed
> provide several benefits, the first one being the ability to have shared
> impl blocks (and shared documentation) between the enabled and disabled
> states for methods like set/get_voltage().
>
> But the key benefit I see is that it could also address the
> aforementioned dynamic management problem through the introduction of a
> third state.
>
> Alongside the `Enabled` and `Disabled` states, there would be a third
> state (`Dynamic`?) in which the regulator could either be enabled or
> disabled. This `Dynamic` state is the only one providing `enable` and
> `disable` methods (as well as `is_enabled`) to change its operational
> state without affecting its type.
>
> All three states then implement `set_voltage` and `get_voltage` through
> a common impl block, that could be extended with other methods from the
> C API that are independent of the state, as needed.
>
> To handle typestate transitions:
>
> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>   method to transition the regulator to the `Enabled` state.
> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>   fail).
>
> Essentially, the `Enabled` and `Disabled` states simply enforce an
> additional operational state invariant on the underlying regulator, and
> do not provide methods to change it.
>
> The `Dynamic` state would be the default for `Regulator`, so by just
> using `Regulator`, the user gets an interface that works very similarly
> to the C API it abstracts, making it intuitive to those familiar with
> it.

How will the `Dynamic` typestate track the enable refcount? AFAIK one
has to drop all enable refcounts before removing the regulator. Also
what happens when I call `disable` without any enable calls before?

---
Cheers,
Benno

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

[Alexandre Courbot]

On Sun May 18, 2025 at 4:19 PM JST, Benno Lossin wrote:
> On Sun May 18, 2025 at 4:28 AM CEST, Alexandre Courbot wrote:
>> On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>> +//! voltage and current regulators.
>>> +//!
>>> +//! The intention is to allow systems to dynamically control regulator power
>>> +//! output in order to save power and prolong battery life. This applies to both
>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>> +//! (where current limit is controllable).
>>> +//!
>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>> +//!
>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>> +//! [`EnabledRegulator`].
>>> +//!
>>> +//! The transition between these types is done by calling
>>> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
>>> +//!
>>> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
>>> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
>>> +//! otherwise.
>>
>> Having the enabled or disabled state baked into the type is indeed
>> valuable for drivers that just need to acquire and enable a regulator at
>> probe time. However, there are also more dynamic use cases and I don't
>> think the burden of managing this aspect - by either performing a manual
>> match to call any method (even the shared ones), or implementing custom
>> dispatch types (which will lead to many similar ad-hoc implementations)
>> - should fall on the user. Thus I strongly suggest that this module
>> provides a solution for this as well.
>>
>> It has been proposed earlier to use a typestate, and this would indeed
>> provide several benefits, the first one being the ability to have shared
>> impl blocks (and shared documentation) between the enabled and disabled
>> states for methods like set/get_voltage().
>>
>> But the key benefit I see is that it could also address the
>> aforementioned dynamic management problem through the introduction of a
>> third state.
>>
>> Alongside the `Enabled` and `Disabled` states, there would be a third
>> state (`Dynamic`?) in which the regulator could either be enabled or
>> disabled. This `Dynamic` state is the only one providing `enable` and
>> `disable` methods (as well as `is_enabled`) to change its operational
>> state without affecting its type.
>>
>> All three states then implement `set_voltage` and `get_voltage` through
>> a common impl block, that could be extended with other methods from the
>> C API that are independent of the state, as needed.
>>
>> To handle typestate transitions:
>>
>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>   method to transition the regulator to the `Enabled` state.
>> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
>> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>>   fail).
>>
>> Essentially, the `Enabled` and `Disabled` states simply enforce an
>> additional operational state invariant on the underlying regulator, and
>> do not provide methods to change it.
>>
>> The `Dynamic` state would be the default for `Regulator`, so by just
>> using `Regulator`, the user gets an interface that works very similarly
>> to the C API it abstracts, making it intuitive to those familiar with
>> it.
>
> How will the `Dynamic` typestate track the enable refcount? AFAIK one
> has to drop all enable refcounts before removing the regulator.

I guess a choice has to be made about whether to just proxy the C API
as-is (where an unbalanced number of enable/disable calls can result in
a dropped regulator still being enabled), or whether to clamp the number
of times a Rust consumer can enable a regulator to 0 and 1 and disable
an enabled regulator in the destructor.

The initial proposal does such clamping by design, but I also suspect
the C API behave like it does for good reasons (which I am not familiar
enough to be aware of unfortunately).

> Also what happens when I call `disable` without any enable calls
> before?

The C API displays a warning an returns -EIO, which sounds like a
reasonable behavior for Rust to proxy.

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

[Alexandre Courbot]

On Sun May 18, 2025 at 5:14 PM JST, Alexandre Courbot wrote:
> On Sun May 18, 2025 at 4:19 PM JST, Benno Lossin wrote:
>> On Sun May 18, 2025 at 4:28 AM CEST, Alexandre Courbot wrote:
>>> On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
>>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>>> +//! voltage and current regulators.
>>>> +//!
>>>> +//! The intention is to allow systems to dynamically control regulator power
>>>> +//! output in order to save power and prolong battery life. This applies to both
>>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>>> +//! (where current limit is controllable).
>>>> +//!
>>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>>> +//!
>>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>>> +//! [`EnabledRegulator`].
>>>> +//!
>>>> +//! The transition between these types is done by calling
>>>> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
>>>> +//!
>>>> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
>>>> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
>>>> +//! otherwise.
>>>
>>> Having the enabled or disabled state baked into the type is indeed
>>> valuable for drivers that just need to acquire and enable a regulator at
>>> probe time. However, there are also more dynamic use cases and I don't
>>> think the burden of managing this aspect - by either performing a manual
>>> match to call any method (even the shared ones), or implementing custom
>>> dispatch types (which will lead to many similar ad-hoc implementations)
>>> - should fall on the user. Thus I strongly suggest that this module
>>> provides a solution for this as well.
>>>
>>> It has been proposed earlier to use a typestate, and this would indeed
>>> provide several benefits, the first one being the ability to have shared
>>> impl blocks (and shared documentation) between the enabled and disabled
>>> states for methods like set/get_voltage().
>>>
>>> But the key benefit I see is that it could also address the
>>> aforementioned dynamic management problem through the introduction of a
>>> third state.
>>>
>>> Alongside the `Enabled` and `Disabled` states, there would be a third
>>> state (`Dynamic`?) in which the regulator could either be enabled or
>>> disabled. This `Dynamic` state is the only one providing `enable` and
>>> `disable` methods (as well as `is_enabled`) to change its operational
>>> state without affecting its type.
>>>
>>> All three states then implement `set_voltage` and `get_voltage` through
>>> a common impl block, that could be extended with other methods from the
>>> C API that are independent of the state, as needed.
>>>
>>> To handle typestate transitions:
>>>
>>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>>   method to transition the regulator to the `Enabled` state.
>>> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
>>> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>>>   fail).
>>>
>>> Essentially, the `Enabled` and `Disabled` states simply enforce an
>>> additional operational state invariant on the underlying regulator, and
>>> do not provide methods to change it.
>>>
>>> The `Dynamic` state would be the default for `Regulator`, so by just
>>> using `Regulator`, the user gets an interface that works very similarly
>>> to the C API it abstracts, making it intuitive to those familiar with
>>> it.
>>
>> How will the `Dynamic` typestate track the enable refcount? AFAIK one
>> has to drop all enable refcounts before removing the regulator.
>
> I guess a choice has to be made about whether to just proxy the C API
> as-is (where an unbalanced number of enable/disable calls can result in
> a dropped regulator still being enabled), or whether to clamp the number
> of times a Rust consumer can enable a regulator to 0 and 1 and disable
> an enabled regulator in the destructor.
>
> The initial proposal does such clamping by design, but I also suspect
> the C API behave like it does for good reasons (which I am not familiar
> enough to be aware of unfortunately).

Well after thinking a bit more about it, it is clear that is does that
because a single consumer may need to ensure a regulator is on across
multiple internal states. I suspect we will have Rust drivers complex
enough to benefit from this behavior sometime soon.

So I'd say the `Dynamic` state should probably mirror the C API as
closely as possible and not try to outsmart the user. The
`Enabled`/`Disabled` typestates will cover the simpler use-cases
perfectly well and ensure a well-controlled enable count.

I guess this also means transitions to/from `Dynamic` and the other
states will have to be limited to the ones where we can clearly infer
the enable count. That's probably ok anyway because I can't think of a
reason to switch from one pattern to the other for the same regulator.
Maybe we don't even need these transitions at all?

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

[Benno Lossin]

On Sun May 18, 2025 at 10:30 AM CEST, Alexandre Courbot wrote:
> On Sun May 18, 2025 at 5:14 PM JST, Alexandre Courbot wrote:
>> On Sun May 18, 2025 at 4:19 PM JST, Benno Lossin wrote:
>>> On Sun May 18, 2025 at 4:28 AM CEST, Alexandre Courbot wrote:
>>>> On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
>>>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>>>> +//! voltage and current regulators.
>>>>> +//!
>>>>> +//! The intention is to allow systems to dynamically control regulator power
>>>>> +//! output in order to save power and prolong battery life. This applies to both
>>>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>>>> +//! (where current limit is controllable).
>>>>> +//!
>>>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>>>> +//!
>>>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>>>> +//! [`EnabledRegulator`].
>>>>> +//!
>>>>> +//! The transition between these types is done by calling
>>>>> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
>>>>> +//!
>>>>> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
>>>>> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
>>>>> +//! otherwise.
>>>>
>>>> Having the enabled or disabled state baked into the type is indeed
>>>> valuable for drivers that just need to acquire and enable a regulator at
>>>> probe time. However, there are also more dynamic use cases and I don't
>>>> think the burden of managing this aspect - by either performing a manual
>>>> match to call any method (even the shared ones), or implementing custom
>>>> dispatch types (which will lead to many similar ad-hoc implementations)
>>>> - should fall on the user. Thus I strongly suggest that this module
>>>> provides a solution for this as well.
>>>>
>>>> It has been proposed earlier to use a typestate, and this would indeed
>>>> provide several benefits, the first one being the ability to have shared
>>>> impl blocks (and shared documentation) between the enabled and disabled
>>>> states for methods like set/get_voltage().
>>>>
>>>> But the key benefit I see is that it could also address the
>>>> aforementioned dynamic management problem through the introduction of a
>>>> third state.
>>>>
>>>> Alongside the `Enabled` and `Disabled` states, there would be a third
>>>> state (`Dynamic`?) in which the regulator could either be enabled or
>>>> disabled. This `Dynamic` state is the only one providing `enable` and
>>>> `disable` methods (as well as `is_enabled`) to change its operational
>>>> state without affecting its type.
>>>>
>>>> All three states then implement `set_voltage` and `get_voltage` through
>>>> a common impl block, that could be extended with other methods from the
>>>> C API that are independent of the state, as needed.
>>>>
>>>> To handle typestate transitions:
>>>>
>>>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>>>   method to transition the regulator to the `Enabled` state.
>>>> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
>>>> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>>>>   fail).
>>>>
>>>> Essentially, the `Enabled` and `Disabled` states simply enforce an
>>>> additional operational state invariant on the underlying regulator, and
>>>> do not provide methods to change it.
>>>>
>>>> The `Dynamic` state would be the default for `Regulator`, so by just
>>>> using `Regulator`, the user gets an interface that works very similarly
>>>> to the C API it abstracts, making it intuitive to those familiar with
>>>> it.
>>>
>>> How will the `Dynamic` typestate track the enable refcount? AFAIK one
>>> has to drop all enable refcounts before removing the regulator.
>>
>> I guess a choice has to be made about whether to just proxy the C API
>> as-is (where an unbalanced number of enable/disable calls can result in
>> a dropped regulator still being enabled), or whether to clamp the number
>> of times a Rust consumer can enable a regulator to 0 and 1 and disable
>> an enabled regulator in the destructor.
>>
>> The initial proposal does such clamping by design, but I also suspect
>> the C API behave like it does for good reasons (which I am not familiar
>> enough to be aware of unfortunately).
>
> Well after thinking a bit more about it, it is clear that is does that
> because a single consumer may need to ensure a regulator is on across
> multiple internal states. I suspect we will have Rust drivers complex
> enough to benefit from this behavior sometime soon.
>
> So I'd say the `Dynamic` state should probably mirror the C API as
> closely as possible and not try to outsmart the user. The
> `Enabled`/`Disabled` typestates will cover the simpler use-cases
> perfectly well and ensure a well-controlled enable count.

So just let users ensure that they always match each `enable` call with
a `disable` call in the `Dynamic` typestate?

That is ok, if no memory issues can arise from forgetting to do so,
otherwise those functions need to be `unsafe`. Also we should clearly
document that the `Enabled`/`Disabled` typestates should be preferred if
possible.

---
Cheers,
Benno

> I guess this also means transitions to/from `Dynamic` and the other
> states will have to be limited to the ones where we can clearly infer
> the enable count. That's probably ok anyway because I can't think of a
> reason to switch from one pattern to the other for the same regulator.
> Maybe we don't even need these transitions at all?

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

[Alexandre Courbot]

On Sun May 18, 2025 at 6:57 PM JST, Benno Lossin wrote:
> On Sun May 18, 2025 at 10:30 AM CEST, Alexandre Courbot wrote:
>> On Sun May 18, 2025 at 5:14 PM JST, Alexandre Courbot wrote:
>>> On Sun May 18, 2025 at 4:19 PM JST, Benno Lossin wrote:
>>>> On Sun May 18, 2025 at 4:28 AM CEST, Alexandre Courbot wrote:
>>>>> On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
>>>>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>>>>> +//! voltage and current regulators.
>>>>>> +//!
>>>>>> +//! The intention is to allow systems to dynamically control regulator power
>>>>>> +//! output in order to save power and prolong battery life. This applies to both
>>>>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>>>>> +//! (where current limit is controllable).
>>>>>> +//!
>>>>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>>>>> +//!
>>>>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>>>>> +//! [`EnabledRegulator`].
>>>>>> +//!
>>>>>> +//! The transition between these types is done by calling
>>>>>> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
>>>>>> +//!
>>>>>> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
>>>>>> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
>>>>>> +//! otherwise.
>>>>>
>>>>> Having the enabled or disabled state baked into the type is indeed
>>>>> valuable for drivers that just need to acquire and enable a regulator at
>>>>> probe time. However, there are also more dynamic use cases and I don't
>>>>> think the burden of managing this aspect - by either performing a manual
>>>>> match to call any method (even the shared ones), or implementing custom
>>>>> dispatch types (which will lead to many similar ad-hoc implementations)
>>>>> - should fall on the user. Thus I strongly suggest that this module
>>>>> provides a solution for this as well.
>>>>>
>>>>> It has been proposed earlier to use a typestate, and this would indeed
>>>>> provide several benefits, the first one being the ability to have shared
>>>>> impl blocks (and shared documentation) between the enabled and disabled
>>>>> states for methods like set/get_voltage().
>>>>>
>>>>> But the key benefit I see is that it could also address the
>>>>> aforementioned dynamic management problem through the introduction of a
>>>>> third state.
>>>>>
>>>>> Alongside the `Enabled` and `Disabled` states, there would be a third
>>>>> state (`Dynamic`?) in which the regulator could either be enabled or
>>>>> disabled. This `Dynamic` state is the only one providing `enable` and
>>>>> `disable` methods (as well as `is_enabled`) to change its operational
>>>>> state without affecting its type.
>>>>>
>>>>> All three states then implement `set_voltage` and `get_voltage` through
>>>>> a common impl block, that could be extended with other methods from the
>>>>> C API that are independent of the state, as needed.
>>>>>
>>>>> To handle typestate transitions:
>>>>>
>>>>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>>>>   method to transition the regulator to the `Enabled` state.
>>>>> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
>>>>> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>>>>>   fail).
>>>>>
>>>>> Essentially, the `Enabled` and `Disabled` states simply enforce an
>>>>> additional operational state invariant on the underlying regulator, and
>>>>> do not provide methods to change it.
>>>>>
>>>>> The `Dynamic` state would be the default for `Regulator`, so by just
>>>>> using `Regulator`, the user gets an interface that works very similarly
>>>>> to the C API it abstracts, making it intuitive to those familiar with
>>>>> it.
>>>>
>>>> How will the `Dynamic` typestate track the enable refcount? AFAIK one
>>>> has to drop all enable refcounts before removing the regulator.
>>>
>>> I guess a choice has to be made about whether to just proxy the C API
>>> as-is (where an unbalanced number of enable/disable calls can result in
>>> a dropped regulator still being enabled), or whether to clamp the number
>>> of times a Rust consumer can enable a regulator to 0 and 1 and disable
>>> an enabled regulator in the destructor.
>>>
>>> The initial proposal does such clamping by design, but I also suspect
>>> the C API behave like it does for good reasons (which I am not familiar
>>> enough to be aware of unfortunately).
>>
>> Well after thinking a bit more about it, it is clear that is does that
>> because a single consumer may need to ensure a regulator is on across
>> multiple internal states. I suspect we will have Rust drivers complex
>> enough to benefit from this behavior sometime soon.
>>
>> So I'd say the `Dynamic` state should probably mirror the C API as
>> closely as possible and not try to outsmart the user. The
>> `Enabled`/`Disabled` typestates will cover the simpler use-cases
>> perfectly well and ensure a well-controlled enable count.
>
> So just let users ensure that they always match each `enable` call with
> a `disable` call in the `Dynamic` typestate?
>
> That is ok, if no memory issues can arise from forgetting to do so,
> otherwise those functions need to be `unsafe`.

There shouldn't be any, the only side effect would be that the regulator
stays enabled when it shouldn't.

It's also easy to implement more behaviors using more states. For
instance, `Dynamic` just proxies the C API. But if we also think it its
useful to have a regulator which use count is clamped to 0 and 1, you
could have another state that includes a boolean (instead of being empty
lke the others) to track whether the regulator is enabled or not, and an
`enable` method that only calls the C `regulator_enable` if that boolean
is not already true. That way you remove the need to mirror the calls to
enable and disable, while only paying the memory overhead for doing so
when you explicitly state you want this behavior.

But maybe I'm overthinking it. It would be interesting to hear to core
developers' opinion on whether the C API can/should be restricted and
guide the design from that.

> Also we should clearly document that the `Enabled`/`Disabled`
> typestates should be preferred if possible.

Definitely, build-time guarantees should be used whenever possible.

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

[Mark Brown]

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

On Sun, May 18, 2025 at 11:28:01AM +0900, Alexandre Courbot wrote:

> Alongside the `Enabled` and `Disabled` states, there would be a third
> state (`Dynamic`?) in which the regulator could either be enabled or
> disabled. This `Dynamic` state is the only one providing `enable` and
> `disable` methods (as well as `is_enabled`) to change its operational
> state without affecting its type.

Note that checking is_enabled() is a red flag, it's only there for
bootstrapping purposes as drivers are probing where there's some
different sequence might be needed - the use cases are quite limited,
most drivers shold just enable the regulator and initialise the
device.

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

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

[Mark Brown]

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

On Sun, May 18, 2025 at 05:14:41PM +0900, Alexandre Courbot wrote:

> The initial proposal does such clamping by design, but I also suspect
> the C API behave like it does for good reasons (which I am not familiar
> enough to be aware of unfortunately).

It's so that if you have multiple logical users within the device (eg,
an interrupt handler and code for normal operation) they can work
independently of each other.  You could also request the regulator
multiple times but that's often not idiomatic.

Originally we didn't actually refcount within the individual consumers
at all and only refcounted on the underlying regulator, the per consumer
reference count is mainly there for debugging purposes.

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

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

[Alexandre Courbot]

On Sun May 18, 2025 at 9:17 PM JST, Mark Brown wrote:
> On Sun, May 18, 2025 at 11:28:01AM +0900, Alexandre Courbot wrote:
>
>> Alongside the `Enabled` and `Disabled` states, there would be a third
>> state (`Dynamic`?) in which the regulator could either be enabled or
>> disabled. This `Dynamic` state is the only one providing `enable` and
>> `disable` methods (as well as `is_enabled`) to change its operational
>> state without affecting its type.
>
> Note that checking is_enabled() is a red flag, it's only there for
> bootstrapping purposes as drivers are probing where there's some
> different sequence might be needed - the use cases are quite limited,
> most drivers shold just enable the regulator and initialise the
> device.

What things that are possible with the C API do you think should *not*
ever be done? That's typically around these kind of restrictions that
Rust abstractions should be designed, so you cannot end up in any
undesired state no matter what sequence of methods you call.

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

[Alexandre Courbot]

On Sun May 18, 2025 at 9:20 PM JST, Mark Brown wrote:
> On Sun, May 18, 2025 at 05:14:41PM +0900, Alexandre Courbot wrote:
>
>> The initial proposal does such clamping by design, but I also suspect
>> the C API behave like it does for good reasons (which I am not familiar
>> enough to be aware of unfortunately).
>
> It's so that if you have multiple logical users within the device (eg,
> an interrupt handler and code for normal operation) they can work
> independently of each other.  You could also request the regulator
> multiple times but that's often not idiomatic.

I guess this means that we want to preserve this use-case with Rust as
well?

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

[Benno Lossin]

On Sun May 18, 2025 at 2:20 PM CEST, Mark Brown wrote:
> On Sun, May 18, 2025 at 05:14:41PM +0900, Alexandre Courbot wrote:
>
>> The initial proposal does such clamping by design, but I also suspect
>> the C API behave like it does for good reasons (which I am not familiar
>> enough to be aware of unfortunately).
>
> It's so that if you have multiple logical users within the device (eg,
> an interrupt handler and code for normal operation) they can work
> independently of each other.  You could also request the regulator
> multiple times but that's often not idiomatic.
>
> Originally we didn't actually refcount within the individual consumers
> at all and only refcounted on the underlying regulator, the per consumer
> reference count is mainly there for debugging purposes.

I'm not sure if I understand correctly, so I'll just try to echo it and
see if it's correct :)

The `enable`/`disable` functions change a refcount on the underlying
regulator that tracks if the regulator actually is enabled/disabled.
Asking the hardware to enable or disable a regulator can fail, but if we
already know that it is enabled, only the refcount is incremented.
    It's okay to leak this enabled-refcount, since when the regulators
actual refcount (so the one adjusted by `_get` & `_put`) hits zero, we
can also disable the regulator. So the enabled-refcount is essentially a
weak refcount that only does something while the regulator exists.

In that case, we can use any API for enabling/disabling, since all of
them will be safe. Just a question of which gives the required
expressiveness and makes misusing it hard.

---
Cheers,
Benno

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

[Benno Lossin]

On Sun May 18, 2025 at 1:12 PM CEST, Alexandre Courbot wrote:
> On Sun May 18, 2025 at 6:57 PM JST, Benno Lossin wrote:
>> So just let users ensure that they always match each `enable` call with
>> a `disable` call in the `Dynamic` typestate?
>>
>> That is ok, if no memory issues can arise from forgetting to do so,
>> otherwise those functions need to be `unsafe`.
>
> There shouldn't be any, the only side effect would be that the regulator
> stays enabled when it shouldn't.
>
> It's also easy to implement more behaviors using more states. For
> instance, `Dynamic` just proxies the C API. But if we also think it its
> useful to have a regulator which use count is clamped to 0 and 1, you
> could have another state that includes a boolean (instead of being empty
> lke the others) to track whether the regulator is enabled or not, and an
> `enable` method that only calls the C `regulator_enable` if that boolean
> is not already true. That way you remove the need to mirror the calls to
> enable and disable, while only paying the memory overhead for doing so
> when you explicitly state you want this behavior.

Aren't we then duplicating the refcount from the C side?

---
Cheers,
Benno

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

[Daniel Almeida]

Hi Alex, thanks for looking at this!

> On 17 May 2025, at 23:28, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> Hi Daniel,
> 
> On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
>> Add a bare minimum regulator abstraction to be used by Rust drivers.
>> This abstraction adds a small subset of the regulator API, which is
>> thought to be sufficient for the drivers we have now.
>> 
>> Regulators provide the power needed by many hardware blocks and thus are
>> likely to be needed by a lot of drivers.
>> 
>> It was tested on rk3588, where it was used to power up the "mali"
>> regulator in order to power up the GPU.
>> 
>> Signed-off-by: Daniel Almeida <daniel.almeida@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        | 211 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 214 insertions(+)
>> 
>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>> index ab37e1d35c70d52e69b754bf855bc19911d156d8..e14cce03338ef5f6a09a23fd41ca47b8c913fa65 100644
>> --- a/rust/bindings/bindings_helper.h
>> +++ b/rust/bindings/bindings_helper.h
>> @@ -31,6 +31,7 @@
>> #include <linux/poll.h>
>> #include <linux/property.h>
>> #include <linux/refcount.h>
>> +#include <linux/regulator/consumer.h>
>> #include <linux/sched.h>
>> #include <linux/security.h>
>> #include <linux/slab.h>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 28007be98fbad0e875d7e5345e164e2af2c5da32..c8fd7e4e036e9e5b6958acf0dcfa952b916a3d48 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -86,6 +86,8 @@
>> pub mod prelude;
>> pub mod print;
>> pub mod rbtree;
>> +#[cfg(CONFIG_REGULATOR)]
>> +pub mod regulator;
>> pub mod revocable;
>> pub mod security;
>> pub mod seq_file;
>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
>> --- /dev/null
>> +++ b/rust/kernel/regulator.rs
>> @@ -0,0 +1,211 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Regulator abstractions, providing a standard kernel interface to control
>> +//! voltage and current regulators.
>> +//!
>> +//! The intention is to allow systems to dynamically control regulator power
>> +//! output in order to save power and prolong battery life. This applies to both
>> +//! voltage regulators (where voltage output is controllable) and current sinks
>> +//! (where current limit is controllable).
>> +//!
>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>> +//!
>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>> +//! [`EnabledRegulator`].
>> +//!
>> +//! The transition between these types is done by calling
>> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
>> +//!
>> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
>> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
>> +//! otherwise.
> 
> Having the enabled or disabled state baked into the type is indeed
> valuable for drivers that just need to acquire and enable a regulator at
> probe time. However, there are also more dynamic use cases and I don't
> think the burden of managing this aspect - by either performing a manual
> match to call any method (even the shared ones), or implementing custom
> dispatch types (which will lead to many similar ad-hoc implementations)
> - should fall on the user. Thus I strongly suggest that this module
> provides a solution for this as well.

I am not sure I understand what you mean by “dynamic use cases”.

Can you expand a bit on that?

> 
> It has been proposed earlier to use a typestate, and this would indeed
> provide several benefits, the first one being the ability to have shared
> impl blocks (and shared documentation) between the enabled and disabled
> states for methods like set/get_voltage().
> 
> But the key benefit I see is that it could also address the
> aforementioned dynamic management problem through the introduction of a
> third state.
> 
> Alongside the `Enabled` and `Disabled` states, there would be a third
> state (`Dynamic`?) in which the regulator could either be enabled or
> disabled. This `Dynamic` state is the only one providing `enable` and
> `disable` methods (as well as `is_enabled`) to change its operational
> state without affecting its type.

Dynamic is just "Regulator" in the current version of this patch. There is no
"Disabled" because there is no guarantee that someone else won't enable the
regulator, trivially breaking this invariant at any moment.

The only thing we can guarantee is "Enabled", through our own call to
"regulator_enable()".

In fact, for the typestate solution, I was thinking about "UnknownState" and
"Enabled", or any nomenclature along these lines.

> 
> All three states then implement `set_voltage` and `get_voltage` through
> a common impl block, that could be extended with other methods from the
> C API that are independent of the state, as needed.
> 
> To handle typestate transitions:
> 
> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>  method to transition the regulator to the `Enabled` state.

Why not “enable()” as we currently have?

If we go with the "Dynamic" nomenclature, and we agree that there's no
"Disabled",  then we can implement "pub fn enable(self) -> Regulator<Enabled>",
for "Dynamic", which is what we currently have, but with other names.

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

> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>  fail).
> 
> Essentially, the `Enabled` and `Disabled` states simply enforce an
> additional operational state invariant on the underlying regulator, and
> do not provide methods to change it.
> 
> The `Dynamic` state would be the default for `Regulator`, so by just
> using `Regulator`, the user gets an interface that works very similarly
> to the C API it abstracts, making it intuitive to those familiar with
> it.

This is already how it works: Regulator is the default and EnabledRegulator
adds an extra invariant.

> 
> But for cases where the regulator is known to always be in a specific
> state like `Enabled`, one can use `Regulator<Enabled>` and simplify
> their code.
> 
> This should retain the compile-time safety that your version proposed
> for common cases, while still exposing the flexibility of the C API when
> needed.
> 

Yeah, I agree.

> <snip>
>> +impl EnabledRegulator {
>> +    fn as_ptr(&self) -> *mut bindings::regulator {
>> +        self.inner.inner.as_ptr()
>> +    }
>> +
>> +    /// Disables the regulator.
>> +    pub fn disable(self) -> Result<Regulator> {
> 
> This has been mentioned, but I think you will want to return `self` in
> case of failure. Granted, most users won't do anything with it and fail,
> but this is allowed by the C API and we shouldn't restrict it.

That is true.

— Daniel

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

[Alexandre Courbot]

On Sun May 18, 2025 at 11:05 PM JST, Benno Lossin wrote:
> On Sun May 18, 2025 at 1:12 PM CEST, Alexandre Courbot wrote:
>> On Sun May 18, 2025 at 6:57 PM JST, Benno Lossin wrote:
>>> So just let users ensure that they always match each `enable` call with
>>> a `disable` call in the `Dynamic` typestate?
>>>
>>> That is ok, if no memory issues can arise from forgetting to do so,
>>> otherwise those functions need to be `unsafe`.
>>
>> There shouldn't be any, the only side effect would be that the regulator
>> stays enabled when it shouldn't.
>>
>> It's also easy to implement more behaviors using more states. For
>> instance, `Dynamic` just proxies the C API. But if we also think it its
>> useful to have a regulator which use count is clamped to 0 and 1, you
>> could have another state that includes a boolean (instead of being empty
>> lke the others) to track whether the regulator is enabled or not, and an
>> `enable` method that only calls the C `regulator_enable` if that boolean
>> is not already true. That way you remove the need to mirror the calls to
>> enable and disable, while only paying the memory overhead for doing so
>> when you explicitly state you want this behavior.
>
> Aren't we then duplicating the refcount from the C side?

Not exactly, we are just tracking whether the consumer has already
called `regulator_enable` or not. Unfortunately `regulator_is_enabled`
appears to track the enabled state of the regulator device (not the
consumer reference), so it cannot be used for that.

Maybe a better way would be to have a Rust helper that returns the
`enable_count` of the regulator and check that instead. And to be clear
I am not saying this one is the policy we should follow - I just want to
illustrate the fact that the typestate pattern allows us to implement
different kinds of behaviors.

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

[Alexandre Courbot]

Hi Daniel,

On Mon May 19, 2025 at 12:11 AM JST, Daniel Almeida wrote:
> Hi Alex, thanks for looking at this!
>
>> On 17 May 2025, at 23:28, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> 
>> Hi Daniel,
>> 
>> On Wed May 14, 2025 at 12:44 AM JST, Daniel Almeida wrote:
>>> Add a bare minimum regulator abstraction to be used by Rust drivers.
>>> This abstraction adds a small subset of the regulator API, which is
>>> thought to be sufficient for the drivers we have now.
>>> 
>>> Regulators provide the power needed by many hardware blocks and thus are
>>> likely to be needed by a lot of drivers.
>>> 
>>> It was tested on rk3588, where it was used to power up the "mali"
>>> regulator in order to power up the GPU.
>>> 
>>> Signed-off-by: Daniel Almeida <daniel.almeida@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        | 211 ++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 214 insertions(+)
>>> 
>>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>>> index ab37e1d35c70d52e69b754bf855bc19911d156d8..e14cce03338ef5f6a09a23fd41ca47b8c913fa65 100644
>>> --- a/rust/bindings/bindings_helper.h
>>> +++ b/rust/bindings/bindings_helper.h
>>> @@ -31,6 +31,7 @@
>>> #include <linux/poll.h>
>>> #include <linux/property.h>
>>> #include <linux/refcount.h>
>>> +#include <linux/regulator/consumer.h>
>>> #include <linux/sched.h>
>>> #include <linux/security.h>
>>> #include <linux/slab.h>
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 28007be98fbad0e875d7e5345e164e2af2c5da32..c8fd7e4e036e9e5b6958acf0dcfa952b916a3d48 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -86,6 +86,8 @@
>>> pub mod prelude;
>>> pub mod print;
>>> pub mod rbtree;
>>> +#[cfg(CONFIG_REGULATOR)]
>>> +pub mod regulator;
>>> pub mod revocable;
>>> pub mod security;
>>> pub mod seq_file;
>>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
>>> --- /dev/null
>>> +++ b/rust/kernel/regulator.rs
>>> @@ -0,0 +1,211 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>> +//! voltage and current regulators.
>>> +//!
>>> +//! The intention is to allow systems to dynamically control regulator power
>>> +//! output in order to save power and prolong battery life. This applies to both
>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>> +//! (where current limit is controllable).
>>> +//!
>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>> +//!
>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>> +//! [`EnabledRegulator`].
>>> +//!
>>> +//! The transition between these types is done by calling
>>> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
>>> +//!
>>> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
>>> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
>>> +//! otherwise.
>> 
>> Having the enabled or disabled state baked into the type is indeed
>> valuable for drivers that just need to acquire and enable a regulator at
>> probe time. However, there are also more dynamic use cases and I don't
>> think the burden of managing this aspect - by either performing a manual
>> match to call any method (even the shared ones), or implementing custom
>> dispatch types (which will lead to many similar ad-hoc implementations)
>> - should fall on the user. Thus I strongly suggest that this module
>> provides a solution for this as well.
>
> I am not sure I understand what you mean by “dynamic use cases”.
>
> Can you expand a bit on that?

I just mean the cases where users will want to enable and disable the
regulator more frequently than just enabling it at probe time.

>
>> 
>> It has been proposed earlier to use a typestate, and this would indeed
>> provide several benefits, the first one being the ability to have shared
>> impl blocks (and shared documentation) between the enabled and disabled
>> states for methods like set/get_voltage().
>> 
>> But the key benefit I see is that it could also address the
>> aforementioned dynamic management problem through the introduction of a
>> third state.
>> 
>> Alongside the `Enabled` and `Disabled` states, there would be a third
>> state (`Dynamic`?) in which the regulator could either be enabled or
>> disabled. This `Dynamic` state is the only one providing `enable` and
>> `disable` methods (as well as `is_enabled`) to change its operational
>> state without affecting its type.
>
> Dynamic is just "Regulator" in the current version of this patch. There is no
> "Disabled" because there is no guarantee that someone else won't enable the
> regulator, trivially breaking this invariant at any moment.

There is a core difference, which is that in your version of
`Regulator`, `enable` takes ownership of `self` and returns a different
type, whereas `Dynamic` would take `&mut self` and change its internal
state, like the C API does.

>
> The only thing we can guarantee is "Enabled", through our own call to
> "regulator_enable()".
>
> In fact, for the typestate solution, I was thinking about "UnknownState" and
> "Enabled", or any nomenclature along these lines.
>
>> 
>> All three states then implement `set_voltage` and `get_voltage` through
>> a common impl block, that could be extended with other methods from the
>> C API that are independent of the state, as needed.
>> 
>> To handle typestate transitions:
>> 
>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>  method to transition the regulator to the `Enabled` state.
>
> Why not “enable()” as we currently have?

`enable()` to me sounds like a method that mutates `self` in-place,
whereas your version consumes it and turns it into a different type.
Such methods are typically named `into_*`, or `try_into_*` when they can
fail.

Actually, this could even be a `TryInto` implementation, but in this
particular case having methods with the target state in their names may
result in clearer code and allow the reader to model the transition
graph more easily.

>
> If we go with the "Dynamic" nomenclature, and we agree that there's no
> "Disabled",  then we can implement "pub fn enable(self) -> Regulator<Enabled>",
> for "Dynamic", which is what we currently have, but with other names.

Not if we want to provide the behavior of the C consumer API, which
requires multiple calls to `regulator_enable()` to be matched by an equal
number of calls to `regulator_disable()`, which could be useful to some
drivers (lest they reimplement their own counter).

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

Yeah I'm not a fan of it and I'm sure there are better alternatives.
Maybe `Controlled`?

>
>> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
>> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>>  fail).
>> 
>> Essentially, the `Enabled` and `Disabled` states simply enforce an
>> additional operational state invariant on the underlying regulator, and
>> do not provide methods to change it.
>> 
>> The `Dynamic` state would be the default for `Regulator`, so by just
>> using `Regulator`, the user gets an interface that works very similarly
>> to the C API it abstracts, making it intuitive to those familiar with
>> it.
>
> This is already how it works: Regulator is the default and EnabledRegulator
> adds an extra invariant.

Hopefully the above clarified the differences.

>
>> 
>> But for cases where the regulator is known to always be in a specific
>> state like `Enabled`, one can use `Regulator<Enabled>` and simplify
>> their code.
>> 
>> This should retain the compile-time safety that your version proposed
>> for common cases, while still exposing the flexibility of the C API when
>> needed.
>> 
>
> Yeah, I agree.

I think the concept looks better if you consider the generic parameter
of `Regulator` not as a state, but as a marker of guaranteed invariants
and allowed transitions.

- `Regulator<Enabled>` guarantees that the regulator is enabled on the
  consumer side. It is also disabled when the object is dropped. Many
  drivers will just do `Regulator::<Enabled>::get()` at probe time (the
  equivalent of `regulator_get_enable()`), store the object into the
  device's state, and forget about it.

- `Regulator<Disabled>` guarantees that the regulator is not enabled on
  the consumer side. It could be useful to have for drivers that use
  distinct types to store their state depending on their power status:
  the powered-on type would store a `Regulator<Enabled>`, the
  powered-off type a `Regulator<Disabled>`. That way you cannot even
  write code transitioning between the states if you omit the regulator
  - which I think is really neat.

These two should cover a large percentage of consumer needs, but for
those that need more fine-grained control we should also have one or two
policies that follow the C API a bit closer, e.g.:

- `Regulator<Controlled>` (or just `Regulator`): user is granted an API
  very close to the C one, and is responsible for balancing calls to
  `enable()` and `disable()`. Regulator remains in the state it was in
  when dropped.

- `Regulator<Switch>`: calls to `enable()` and `disable()` do not need
  to be balanced, and the regulator always transitions to one state if
  it was in the other. Regulator gets automatically disabled on drop.
  This provides a simpler, safer alternative to `Controlled`.

Note that I am not advocating for including these two policies
specifically, these are just examples. My main point is that we should
also provide a way to change the regulator's enabled state without
requiring a change of type ; which policy(es) to adopt will depend on
which restrictions we conclude are adequate to place on the C API, if
any.

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

[Mark Brown]

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

On Sun, May 18, 2025 at 09:49:00PM +0900, Alexandre Courbot wrote:

> What things that are possible with the C API do you think should *not*
> ever be done? That's typically around these kind of restrictions that
> Rust abstractions should be designed, so you cannot end up in any
> undesired state no matter what sequence of methods you call.

There's nothing that should *never* be used, but there's a bunch of
things like _is_enabled() and _get_optional() where the uses are
specialist and people are far too enthusiastic about using them.

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

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

[Mark Brown]

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

On Sun, May 18, 2025 at 09:51:33PM +0900, Alexandre Courbot wrote:
> On Sun May 18, 2025 at 9:20 PM JST, Mark Brown wrote:

> > It's so that if you have multiple logical users within the device (eg,
> > an interrupt handler and code for normal operation) they can work
> > independently of each other.  You could also request the regulator
> > multiple times but that's often not idiomatic.

> I guess this means that we want to preserve this use-case with Rust as
> well?

Perhaps.  It might be that the multiple requests approach is more
idiomatic in rust than it is un C.

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

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

[Mark Brown]

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

On Sun, May 18, 2025 at 04:04:24PM +0200, Benno Lossin wrote:

> I'm not sure if I understand correctly, so I'll just try to echo it and
> see if it's correct :)

> The `enable`/`disable` functions change a refcount on the underlying
> regulator that tracks if the regulator actually is enabled/disabled.
> Asking the hardware to enable or disable a regulator can fail, but if we
> already know that it is enabled, only the refcount is incremented.

Yes.

>     It's okay to leak this enabled-refcount, since when the regulators
> actual refcount (so the one adjusted by `_get` & `_put`) hits zero, we
> can also disable the regulator. So the enabled-refcount is essentially a
> weak refcount that only does something while the regulator exists.

No.  You should not leak any refcount, the per consumer refcount
duplicates what's being done for the regulator as a whole, one should
never be incremented or decremented without the other (but there may be
multiple consumers to choose from).

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

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

[Daniel Almeida]

Hi Alex,

I still don’t understand your use case 100% :/

> 
> I just mean the cases where users will want to enable and disable the
> regulator more frequently than just enabling it at probe time.

This is already possible through kernel::types::Either. 

i.e.: the current design - or the proposed typestate one - can already switch
back and forth between Regulator and EnabledRegulator. Using Either makes it
just work, because you can change the variant at runtime without hassle. This
lets you consume self in an ergonomic way.

By the way, the reason I'm pushing back slightly here is because you seem
(IIUC) to be trying to reintroduce the pattern we had to move away from in v1.

i.e.: we explicitly had to move away from trying to match enables and disables
in Rust, because it was hard to get this right.

The current design is a simplification that apparently works, because at best
you have +1 on the count and that is encoded in the type itself, so there is
nothing to actually "track" or "balance" within a given instance. Multiple
calls to _get() or _enable() on the same instance are simply forbidden.

Can you add some pseudocode that shows how this doesn't work (or is otherwise
unergonomic) in Nova? I think it will make your point clearer.

> 
>> 
>>> 
>>> It has been proposed earlier to use a typestate, and this would indeed
>>> provide several benefits, the first one being the ability to have shared
>>> impl blocks (and shared documentation) between the enabled and disabled
>>> states for methods like set/get_voltage().
>>> 
>>> But the key benefit I see is that it could also address the
>>> aforementioned dynamic management problem through the introduction of a
>>> third state.
>>> 
>>> Alongside the `Enabled` and `Disabled` states, there would be a third
>>> state (`Dynamic`?) in which the regulator could either be enabled or
>>> disabled. This `Dynamic` state is the only one providing `enable` and
>>> `disable` methods (as well as `is_enabled`) to change its operational
>>> state without affecting its type.
>> 
>> Dynamic is just "Regulator" in the current version of this patch. There is no
>> "Disabled" because there is no guarantee that someone else won't enable the
>> regulator, trivially breaking this invariant at any moment.
> 
> There is a core difference, which is that in your version of
> `Regulator`, `enable` takes ownership of `self` and returns a different
> type, whereas `Dynamic` would take `&mut self` and change its internal
> state, like the C API does.

I see now, but consuming self is something we're trying after considering the
&mut self approach, which did not work very well in v1.

> 
>> 
>> The only thing we can guarantee is "Enabled", through our own call to
>> "regulator_enable()".
>> 
>> In fact, for the typestate solution, I was thinking about "UnknownState" and
>> "Enabled", or any nomenclature along these lines.
>> 
>>> 
>>> All three states then implement `set_voltage` and `get_voltage` through
>>> a common impl block, that could be extended with other methods from the
>>> C API that are independent of the state, as needed.
>>> 
>>> To handle typestate transitions:
>>> 
>>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>> method to transition the regulator to the `Enabled` state.
>> 
>> Why not “enable()” as we currently have?
> 
> `enable()` to me sounds like a method that mutates `self` in-place,
> whereas your version consumes it and turns it into a different type.
> Such methods are typically named `into_*`, or `try_into_*` when they can
> fail.
> 
> Actually, this could even be a `TryInto` implementation, but in this
> particular case having methods with the target state in their names may
> result in clearer code and allow the reader to model the transition
> graph more easily.
> 
>> 
>> If we go with the "Dynamic" nomenclature, and we agree that there's no
>> "Disabled",  then we can implement "pub fn enable(self) -> Regulator<Enabled>",
>> for "Dynamic", which is what we currently have, but with other names.
> 
> Not if we want to provide the behavior of the C consumer API, which
> requires multiple calls to `regulator_enable()` to be matched by an equal
> number of calls to `regulator_disable()`, which could be useful to some
> drivers (lest they reimplement their own counter).

This is explicitly not supported, because (given the current code) why should it be?

If you want a given regulator to be enabled, just make sure you have
Regulator<Enabled> in your kernel::types::Either container.

You don't need a counter either: Regulator<Enabled> has a count of one, and
when that goes out of scope, it's decremented.

> 
>> 
>> Also, I personally dislike this term: it's hard to tell what Regulator<Dynamic>
>> means on a first glance.
> 
> Yeah I'm not a fan of it and I'm sure there are better alternatives.
> Maybe `Controlled`?
> 
>> 
>>> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
>>> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>>> fail).
>>> 
>>> Essentially, the `Enabled` and `Disabled` states simply enforce an
>>> additional operational state invariant on the underlying regulator, and
>>> do not provide methods to change it.
>>> 
>>> The `Dynamic` state would be the default for `Regulator`, so by just
>>> using `Regulator`, the user gets an interface that works very similarly
>>> to the C API it abstracts, making it intuitive to those familiar with
>>> it.
>> 
>> This is already how it works: Regulator is the default and EnabledRegulator
>> adds an extra invariant.
> 
> Hopefully the above clarified the differences.
> 
>> 
>>> 
>>> But for cases where the regulator is known to always be in a specific
>>> state like `Enabled`, one can use `Regulator<Enabled>` and simplify
>>> their code.
>>> 
>>> This should retain the compile-time safety that your version proposed
>>> for common cases, while still exposing the flexibility of the C API when
>>> needed.
>>> 
>> 
>> Yeah, I agree.
> 
> I think the concept looks better if you consider the generic parameter
> of `Regulator` not as a state, but as a marker of guaranteed invariants
> and allowed transitions.
> 
> - `Regulator<Enabled>` guarantees that the regulator is enabled on the
>  consumer side. It is also disabled when the object is dropped. Many
>  drivers will just do `Regulator::<Enabled>::get()` at probe time (the
>  equivalent of `regulator_get_enable()`), store the object into the
>  device's state, and forget about it.
> 
> - `Regulator<Disabled>` guarantees that the regulator is not enabled on
>  the consumer side. It could be useful to have for drivers that use
>  distinct types to store their state depending on their power status:
>  the powered-on type would store a `Regulator<Enabled>`, the
>  powered-off type a `Regulator<Disabled>`. That way you cannot even
>  write code transitioning between the states if you omit the regulator
>  - which I think is really neat.
> 

I thought we had agreed that there is no “Disabled”, even in C?

> These two should cover a large percentage of consumer needs, but for
> those that need more fine-grained control we should also have one or two
> policies that follow the C API a bit closer, e.g.:
> 
> - `Regulator<Controlled>` (or just `Regulator`): user is granted an API
>  very close to the C one, and is responsible for balancing calls to
>  `enable()` and `disable()`. Regulator remains in the state it was in
>  when dropped.
> 
> - `Regulator<Switch>`: calls to `enable()` and `disable()` do not need
>  to be balanced, and the regulator always transitions to one state if
>  it was in the other. Regulator gets automatically disabled on drop.
>  This provides a simpler, safer alternative to `Controlled`.
> 
> Note that I am not advocating for including these two policies
> specifically, these are just examples. My main point is that we should
> also provide a way to change the regulator's enabled state without
> requiring a change of type ; which policy(es) to adopt will depend on
> which restrictions we conclude are adequate to place on the C API, if
> any.

Can you expand a bit on the issues of changing types? Again, some pseudocode
will probably help a lot :)

— Daniel

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

[Daniel Almeida]

Minor correction:


> This is explicitly not supported, because (given the current code) why should it be?
> 
> If you want a given regulator to be enabled, just make sure you have
> Regulator<Enabled> in your kernel::types::Either container.
> 
> You don't need a counter either: Regulator<Enabled> has a count of one, and
> when that goes out of scope, it's decremented.

Regulator<Enabled> has a +1 on the count. The actual underlying count is unknown.

— Daniel

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

[Benno Lossin]

On Mon May 19, 2025 at 11:56 AM CEST, Mark Brown wrote:
> On Sun, May 18, 2025 at 04:04:24PM +0200, Benno Lossin wrote:
>
>> I'm not sure if I understand correctly, so I'll just try to echo it and
>> see if it's correct :)
>
>> The `enable`/`disable` functions change a refcount on the underlying
>> regulator that tracks if the regulator actually is enabled/disabled.
>> Asking the hardware to enable or disable a regulator can fail, but if we
>> already know that it is enabled, only the refcount is incremented.
>
> Yes.
>
>>     It's okay to leak this enabled-refcount, since when the regulators
>> actual refcount (so the one adjusted by `_get` & `_put`) hits zero, we
>> can also disable the regulator. So the enabled-refcount is essentially a
>> weak refcount that only does something while the regulator exists.
>
> No.  You should not leak any refcount, the per consumer refcount
> duplicates what's being done for the regulator as a whole, one should
> never be incremented or decremented without the other (but there may be
> multiple consumers to choose from).

What stops the last `regulator_put` to also call `regulator_disable` a
correct number of times?

What are the kinds of problems that one could encounter when not calling
`regulator_disable` before `regulator_put` or if `regulator_enable` was
never called to begin with?
    I'm asking, because if the answer is "memory bugs", then we'll need
to make the abstraction such that users cannot misuse the enable/disable
calls (or make those calls `unsafe`).

---
Cheers,
Benno

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

[Mark Brown]

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

On Mon, May 19, 2025 at 01:25:56PM +0200, Benno Lossin wrote:
> On Mon May 19, 2025 at 11:56 AM CEST, Mark Brown wrote:

> > No.  You should not leak any refcount, the per consumer refcount
> > duplicates what's being done for the regulator as a whole, one should
> > never be incremented or decremented without the other (but there may be
> > multiple consumers to choose from).

> What stops the last `regulator_put` to also call `regulator_disable` a
> correct number of times?

We obviously could but the regulator API defaults to not doing anything
unless explicitly told to since getting things wrong can physically
damage the system.  We've no idea if just disabling the regulator would
be safe at this point so we just don't touch anything and complain about
it.

> What are the kinds of problems that one could encounter when not calling
> `regulator_disable` before `regulator_put` or if `regulator_enable` was
> never called to begin with?

If you don't disable the regulator you've just leaked a reference which
is obviously a problem.

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

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

[Benno Lossin]

On Mon May 19, 2025 at 12:52 PM CEST, Daniel Almeida wrote:
>> I just mean the cases where users will want to enable and disable the
>> regulator more frequently than just enabling it at probe time.
>
> This is already possible through kernel::types::Either. 
>
> i.e.: the current design - or the proposed typestate one - can already switch
> back and forth between Regulator and EnabledRegulator. Using Either makes it
> just work, because you can change the variant at runtime without hassle. This
> lets you consume self in an ergonomic way.

Have you tried to write such a use-case using `Either`? My personal
experience with `Either` was pretty horrible, since you always have to
match on it before you can do anything to the values. It's not really
ergonomic.

I think we should remove it, as it also doesn't have any users at the
moment. Anyone that needs it should define a custom enum for their
use-case.

And effectively an `Either<Regulator, EnabledRegulator>` is just a
`Regulator<Switch>` in Alexandre's proposal if I understood it
correctly.

> By the way, the reason I'm pushing back slightly here is because you seem
> (IIUC) to be trying to reintroduce the pattern we had to move away from in v1.
>
> i.e.: we explicitly had to move away from trying to match enables and disables
> in Rust, because it was hard to get this right.
>
> The current design is a simplification that apparently works, because at best
> you have +1 on the count and that is encoded in the type itself, so there is
> nothing to actually "track" or "balance" within a given instance. Multiple
> calls to _get() or _enable() on the same instance are simply forbidden.
>
> Can you add some pseudocode that shows how this doesn't work (or is otherwise
> unergonomic) in Nova? I think it will make your point clearer.

+1 on actual code examples :)

---
Cheers,
Benno

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

[Miguel Ojeda]

On Mon, May 19, 2025 at 1:54 PM Benno Lossin <lossin@kernel.org> wrote:
>
> I think we should remove it, as it also doesn't have any users at the
> moment. Anyone that needs it should define a custom enum for their
> use-case.

Yeah, it has a similar feeling to `bool` parameters, i.e. whenever an
`Either` is needed, it is likely clearer for readers to have a custom
type instead.

Cheers,
Miguel

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

[Benno Lossin]

On Mon May 19, 2025 at 1:46 PM CEST, Mark Brown wrote:
> On Mon, May 19, 2025 at 01:25:56PM +0200, Benno Lossin wrote:
>> On Mon May 19, 2025 at 11:56 AM CEST, Mark Brown wrote:
>
>> > No.  You should not leak any refcount, the per consumer refcount
>> > duplicates what's being done for the regulator as a whole, one should
>> > never be incremented or decremented without the other (but there may be
>> > multiple consumers to choose from).
>
>> What stops the last `regulator_put` to also call `regulator_disable` a
>> correct number of times?
>
> We obviously could but the regulator API defaults to not doing anything
> unless explicitly told to since getting things wrong can physically
> damage the system.  We've no idea if just disabling the regulator would
> be safe at this point so we just don't touch anything and complain about
> it.

Gotcha.

>> What are the kinds of problems that one could encounter when not calling
>> `regulator_disable` before `regulator_put` or if `regulator_enable` was
>> never called to begin with?
>
> If you don't disable the regulator you've just leaked a reference which
> is obviously a problem.

For sure. But I'm trying to figure out if this is a safety-related issue
or not. Safety in Rust has a rather specific meaning that can be
summarized with "no UB". So since the C side does nothing if the user
screwed up the refcounts, it lets me to believe that we don't have any
safety related issues when forgetting to call `regulator_disable`.

Of course we still should strive for an API that makes that impossible
or at least very hard, but we don't need to make the API `unsafe` or
have to take special care. (At least if I understood correctly)

---
Cheers,
Benno

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

[Mark Brown]

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

On Mon, May 19, 2025 at 02:30:05PM +0200, Benno Lossin wrote:
> On Mon May 19, 2025 at 1:46 PM CEST, Mark Brown wrote:

> > If you don't disable the regulator you've just leaked a reference which
> > is obviously a problem.

> For sure. But I'm trying to figure out if this is a safety-related issue
> or not. Safety in Rust has a rather specific meaning that can be
> summarized with "no UB". So since the C side does nothing if the user
> screwed up the refcounts, it lets me to believe that we don't have any
> safety related issues when forgetting to call `regulator_disable`.

> Of course we still should strive for an API that makes that impossible
> or at least very hard, but we don't need to make the API `unsafe` or
> have to take special care. (At least if I understood correctly)

Yes, it's relatively unlikely that it would lead to any undefined
behaviour.  There is an API for crashing through the refcounts and
disabling if we detect some emergency, but that's very extreme.

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

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

[Alexandre Courbot]

Hi Daniel,

On Mon May 19, 2025 at 7:52 PM JST, Daniel Almeida wrote:
> Hi Alex,
>
> I still don’t understand your use case 100% :/

I should clarify that I don't have a use-case for this (yet), nor do I
foresee that Nova will need to use regulators ; it's just a drive-by
review as I am a bit familiar with the regulator consumer API thanks to
past work.

>
>> 
>> I just mean the cases where users will want to enable and disable the
>> regulator more frequently than just enabling it at probe time.
>
> This is already possible through kernel::types::Either. 
>
> i.e.: the current design - or the proposed typestate one - can already switch
> back and forth between Regulator and EnabledRegulator. Using Either makes it
> just work, because you can change the variant at runtime without hassle. This
> lets you consume self in an ergonomic way.
>
> By the way, the reason I'm pushing back slightly here is because you seem
> (IIUC) to be trying to reintroduce the pattern we had to move away from in v1.
>
> i.e.: we explicitly had to move away from trying to match enables and disables
> in Rust, because it was hard to get this right.
>
> The current design is a simplification that apparently works, because at best
> you have +1 on the count and that is encoded in the type itself, so there is
> nothing to actually "track" or "balance" within a given instance. Multiple
> calls to _get() or _enable() on the same instance are simply forbidden.
>
> Can you add some pseudocode that shows how this doesn't work (or is otherwise
> unergonomic) in Nova? I think it will make your point clearer.

So let's say you have this in your device struct:

  regulator: Either<Regulator, EnabledRegulator>,

And you want to set the voltage on it. Now you need to do:

  match &self.regulator {
    Either::Left(regulator) => regulator.set_voltage(...)?,
    Either::Right(regulator) => regulator.set_voltage(...)?,
  }

And you need to do that for every single method that is available on
both types. It's unergonomic and cumbersome.

Conversely, if you have:

  regulator: Regulator<Switch>,

then it's simply a matter of doing

  self.regulator.set_voltage(...)?;

It's more code. More code means more bugs. :) And it's going to be quite
a common pattern, so I think we should make it convenient.

Also it looks like `Either` is going away, which means each user will
now have to implement their own enum type.

>
>> 
>>> 
>>>> 
>>>> It has been proposed earlier to use a typestate, and this would indeed
>>>> provide several benefits, the first one being the ability to have shared
>>>> impl blocks (and shared documentation) between the enabled and disabled
>>>> states for methods like set/get_voltage().
>>>> 
>>>> But the key benefit I see is that it could also address the
>>>> aforementioned dynamic management problem through the introduction of a
>>>> third state.
>>>> 
>>>> Alongside the `Enabled` and `Disabled` states, there would be a third
>>>> state (`Dynamic`?) in which the regulator could either be enabled or
>>>> disabled. This `Dynamic` state is the only one providing `enable` and
>>>> `disable` methods (as well as `is_enabled`) to change its operational
>>>> state without affecting its type.
>>> 
>>> Dynamic is just "Regulator" in the current version of this patch. There is no
>>> "Disabled" because there is no guarantee that someone else won't enable the
>>> regulator, trivially breaking this invariant at any moment.
>> 
>> There is a core difference, which is that in your version of
>> `Regulator`, `enable` takes ownership of `self` and returns a different
>> type, whereas `Dynamic` would take `&mut self` and change its internal
>> state, like the C API does.
>
> I see now, but consuming self is something we're trying after considering the
> &mut self approach, which did not work very well in v1.

Just took a look at the comments on v1 since I arrived late in the
discussion.

I think both approaches have scenarios where they apply. It's a bit like
Rust's borrow checker: whenever possible, you want the rules to be
enforced at compile-time, but sometimes the access patterns become too
complex so you resort to RefCell to enforce these rules dynamically.

So please don't get me wrong, I think having a dedicated type for an
enabled regulator is great design, and its use should be preferred
whenever possible ; most drivers just want to get and enable a regulator
at probe time, and `Regulator::<Enabled>::get()` would be perfect for
that.

But if we only provide that then we also put restrictions on what the C
API allows, and users with more complex use-cases will pay the price by
rewriting code that already exists (a bit more on that later).

The following comment was made on v1:

> It's possible there's a need to split simple and complex consumer APIs
> in Rust?

And it sounds sensible to me.
>
>> 
>>> 
>>> The only thing we can guarantee is "Enabled", through our own call to
>>> "regulator_enable()".
>>> 
>>> In fact, for the typestate solution, I was thinking about "UnknownState" and
>>> "Enabled", or any nomenclature along these lines.
>>> 
>>>> 
>>>> All three states then implement `set_voltage` and `get_voltage` through
>>>> a common impl block, that could be extended with other methods from the
>>>> C API that are independent of the state, as needed.
>>>> 
>>>> To handle typestate transitions:
>>>> 
>>>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>>> method to transition the regulator to the `Enabled` state.
>>> 
>>> Why not “enable()” as we currently have?
>> 
>> `enable()` to me sounds like a method that mutates `self` in-place,
>> whereas your version consumes it and turns it into a different type.
>> Such methods are typically named `into_*`, or `try_into_*` when they can
>> fail.
>> 
>> Actually, this could even be a `TryInto` implementation, but in this
>> particular case having methods with the target state in their names may
>> result in clearer code and allow the reader to model the transition
>> graph more easily.
>> 
>>> 
>>> If we go with the "Dynamic" nomenclature, and we agree that there's no
>>> "Disabled",  then we can implement "pub fn enable(self) -> Regulator<Enabled>",
>>> for "Dynamic", which is what we currently have, but with other names.
>> 
>> Not if we want to provide the behavior of the C consumer API, which
>> requires multiple calls to `regulator_enable()` to be matched by an equal
>> number of calls to `regulator_disable()`, which could be useful to some
>> drivers (lest they reimplement their own counter).
>
> This is explicitly not supported, because (given the current code) why should it be?
>
> If you want a given regulator to be enabled, just make sure you have
> Regulator<Enabled> in your kernel::types::Either container.
>
> You don't need a counter either: Regulator<Enabled> has a count of one, and
> when that goes out of scope, it's decremented.

That works very well for simpler scenarios, but won't cover everything
optimally.

A common pattern is a driver (sensors come to mind, and maybe codecs)
that acquire a regulator at probe time, and enable it every time
user-space opens the device (and, conversely, disable it as the user
session closes). The driver wants the regulator to be enabled if there
is at least one user-space session opened, and only disable it when all
user-space sessions are closed. With the proposed scheme, a Rust driver
would have to keep track of the opened sessions count and disable the
regulator itself when it reaches zero - effectively duplicating what the
C API would happily do for it, with the potential of bugs being
introduced in these extra lines of code.

There was also a discussion stating that it is safer (for the hardware)
to leave a regulator on than it is to switch it off when, due to a bug,
it is released while its enable count is not zero, and I think that's a
behavior we should allow if the user expresses a need for it. Safety is
not only memory safety :) (and in this case, it is not affected anyway).

>> - `Regulator<Disabled>` guarantees that the regulator is not enabled on
>>  the consumer side. It could be useful to have for drivers that use
>>  distinct types to store their state depending on their power status:
>>  the powered-on type would store a `Regulator<Enabled>`, the
>>  powered-off type a `Regulator<Disabled>`. That way you cannot even
>>  write code transitioning between the states if you omit the regulator
>>  - which I think is really neat.
>> 
>
> I thought we had agreed that there is no “Disabled”, even in C?

The relevant function is called `regulator_disable()`, and it may or may
not actually disable the regulator, but the consumer behaves as if it
were. I think we should keep the same nomenclature for consistency.

>> These two should cover a large percentage of consumer needs, but for
>> those that need more fine-grained control we should also have one or two
>> policies that follow the C API a bit closer, e.g.:
>> 
>> - `Regulator<Controlled>` (or just `Regulator`): user is granted an API
>>  very close to the C one, and is responsible for balancing calls to
>>  `enable()` and `disable()`. Regulator remains in the state it was in
>>  when dropped.
>> 
>> - `Regulator<Switch>`: calls to `enable()` and `disable()` do not need
>>  to be balanced, and the regulator always transitions to one state if
>>  it was in the other. Regulator gets automatically disabled on drop.
>>  This provides a simpler, safer alternative to `Controlled`.
>> 
>> Note that I am not advocating for including these two policies
>> specifically, these are just examples. My main point is that we should
>> also provide a way to change the regulator's enabled state without
>> requiring a change of type ; which policy(es) to adopt will depend on
>> which restrictions we conclude are adequate to place on the C API, if
>> any.
>
> Can you expand a bit on the issues of changing types? Again, some pseudocode
> will probably help a lot :)

See my example using `Either` above. :) I hope to be forgiven for the
user-space sessions example as I believe it is easy to model.

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

[Alexandre Courbot]

On Mon May 19, 2025 at 8:54 PM JST, Benno Lossin wrote:
> On Mon May 19, 2025 at 12:52 PM CEST, Daniel Almeida wrote:
>>> I just mean the cases where users will want to enable and disable the
>>> regulator more frequently than just enabling it at probe time.
>>
>> This is already possible through kernel::types::Either. 
>>
>> i.e.: the current design - or the proposed typestate one - can already switch
>> back and forth between Regulator and EnabledRegulator. Using Either makes it
>> just work, because you can change the variant at runtime without hassle. This
>> lets you consume self in an ergonomic way.
>
> Have you tried to write such a use-case using `Either`? My personal
> experience with `Either` was pretty horrible, since you always have to
> match on it before you can do anything to the values. It's not really
> ergonomic.
>
> I think we should remove it, as it also doesn't have any users at the
> moment. Anyone that needs it should define a custom enum for their
> use-case.
>
> And effectively an `Either<Regulator, EnabledRegulator>` is just a
> `Regulator<Switch>` in Alexandre's proposal if I understood it
> correctly.

Exactly. And btw, there is no reason to block the merging of a simple
version with just enabled and disabled types while we discuss the rest,
as long as it is implemented as a typestate. Adding more ways to control
the enabled status just involves adding new types to be given as
arguments to `Regulator<>` and their respective `impl` blocks, so it can
be done incrementally on top of that base, which I believe everybody
agrees is sound.

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

[Benno Lossin]

On Mon May 19, 2025 at 4:43 PM CEST, Alexandre Courbot wrote:
> On Mon May 19, 2025 at 8:54 PM JST, Benno Lossin wrote:
>> On Mon May 19, 2025 at 12:52 PM CEST, Daniel Almeida wrote:
>>>> I just mean the cases where users will want to enable and disable the
>>>> regulator more frequently than just enabling it at probe time.
>>>
>>> This is already possible through kernel::types::Either. 
>>>
>>> i.e.: the current design - or the proposed typestate one - can already switch
>>> back and forth between Regulator and EnabledRegulator. Using Either makes it
>>> just work, because you can change the variant at runtime without hassle. This
>>> lets you consume self in an ergonomic way.
>>
>> Have you tried to write such a use-case using `Either`? My personal
>> experience with `Either` was pretty horrible, since you always have to
>> match on it before you can do anything to the values. It's not really
>> ergonomic.
>>
>> I think we should remove it, as it also doesn't have any users at the
>> moment. Anyone that needs it should define a custom enum for their
>> use-case.
>>
>> And effectively an `Either<Regulator, EnabledRegulator>` is just a
>> `Regulator<Switch>` in Alexandre's proposal if I understood it
>> correctly.
>
> Exactly. And btw, there is no reason to block the merging of a simple
> version with just enabled and disabled types while we discuss the rest,
> as long as it is implemented as a typestate. Adding more ways to control
> the enabled status just involves adding new types to be given as
> arguments to `Regulator<>` and their respective `impl` blocks, so it can
> be done incrementally on top of that base, which I believe everybody
> agrees is sound.

Yeah, so I think it would be best if we changed to the typestate design.

---
Cheers,
Benno