Message-Id: <D9VATLUHDGU8.53I80TGVRV0J@kernel.org>
Date: Tue, 13 May 2025 22:01:05 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, "Boris Brezillon"
 <boris.brezillon@...labora.com>, "Sebastian Reichel"
 <sebastian.reichel@...labora.com>, "Liam Girdwood" <lgirdwood@...il.com>,
 "Mark Brown" <broonie@...nel.org>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3] rust: regulator: add a bare minimum regulator
 abstraction

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,

Powered by blists - more mailing lists