[PATCH v3 0/3] Clk improvements

[PATCH v3 0/3] Clk improvements

[Daniel Almeida]

This series contains a few improvements that simplifies clock handling for
drivers.

Patch 1 implements the same typestate pattern that has been used
successfully for Regulators. This is needed because otherwise drivers
will be responsible for unpreparing and disabling clocks themselves and
ultimately handling the reference counts on their own. This is
undesirable. The patch automatically encodes this information using the
type system so that no misuse can occur.

Patch 2 makes things more convenient by offering devres-managed APIs. This
lets drivers set clock parameters once and forget about lifetime
management.

Patch 3 converts clk.rs to the newer kernel-vertical style in order to make
future changes easier.

This depends on Alice Ryhl's series [0].

[0]: https://lore.kernel.org/rust-for-linux/20251218-clk-send-sync-v3-0-e48b2e2f1eac@google.com/

---
Changes in v3:
- Rebased on top of 6.19-rc4
- Dropped patch 1 (from Alice), added her series as a dependency instead
- Fixed Tyr, PWM_TH1520 drivers
- Changed clk.rs imports to kernel-vertical style
- Added support get_optional shortcut for Prepared and Enabled (i.e.:
  Clk::<Enabled>::get_optional())
- Fixed misplaced #[inline] tag

Thanks, Danilo {
  - Moved the devres changes into its own patch
  - Require &Device<Bound> for all functions where a &Device is used
  - Account for con_in in SAFETY comments where applicable
  - Added backticks
}

- Link to v2: https://lore.kernel.org/r/20250910-clk-type-state-v2-0-1b97c11bb631@collabora.com

Changes in v2:
- Added Alice's patch as patch 1, since it is a dependency.
- Added devm helpers (like we did for Regulator<T>)
- Fixed missing clk_put() call in Drop (Danilo)
- Fixed missing parenthesis and wrong docs (Viresh)
- Removed extra "dev" parameter from "shutdown" example (Danilo)
- Removed useless type annotation from example (Danilo)
- Link to v1: https://lore.kernel.org/rust-for-linux/20250729-clk-type-state-v1-1-896b53816f7b@collabora.com/#r

---
Daniel Almeida (3):
      rust: clk: use the type-state pattern
      rust: clk: add devres-managed clks
      rust: clk: use 'kernel vertical style' for imports

 drivers/cpufreq/rcpufreq_dt.rs |   2 +-
 drivers/gpu/drm/tyr/driver.rs  |  31 +--
 drivers/pwm/pwm_th1520.rs      |  17 +-
 rust/kernel/clk.rs             | 466 +++++++++++++++++++++++++++++------------
 rust/kernel/cpufreq.rs         |   8 +-
 5 files changed, 346 insertions(+), 178 deletions(-)
---
base-commit: 9ace4753a5202b02191d54e9fdf7f9e3d02b85eb
change-id: 20250909-clk-type-state-c01aa7dd551d
prerequisite-change-id: 20250904-clk-send-sync-3cfa7f4e1ce2:v3
prerequisite-patch-id: 13476f9e7e7c3bdbcab42912948953743381b1e0
prerequisite-patch-id: 8f91583384bb4516afcac66d21ac08b3982747b2
prerequisite-patch-id: b2ad5ecbd9a395b622bc04f891b5bb276f6f6b16

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

[PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

The current Clk abstraction can still be improved on the following issues:

a) It only keeps track of a count to clk_get(), which means that users have
to manually call disable() and unprepare(), or a variation of those, like
disable_unprepare().

b) It allows repeated calls to prepare() or enable(), but it keeps no track
of how often these were called, i.e., it's currently legal to write the
following:

clk.prepare();
clk.prepare();
clk.enable();
clk.enable();

And nothing gets undone on drop().

c) It adds a OptionalClk type that is probably not needed. There is no
"struct optional_clk" in C and we should probably not add one.

d) It does not let a user express the state of the clk through the
type system. For example, there is currently no way to encode that a Clk is
enabled via the type system alone.

In light of the Regulator abstraction that was recently merged, switch this
abstraction to use the type-state pattern instead. It solves both a) and b)
by establishing a number of states and the valid ways to transition between
them. It also automatically undoes any call to clk_get(), clk_prepare() and
clk_enable() as applicable on drop(), so users do not have to do anything
special before Clk goes out of scope.

It solves c) by removing the OptionalClk type, which is now simply encoded
as a Clk whose inner pointer is NULL.

It solves d) by directly encoding the state of the Clk into the type, e.g.:
Clk<Enabled> is now known to be a Clk that is enabled.

The INVARIANTS section for Clk is expanded to highlight the relationship
between the states and the respective reference counts that are owned by
each of them.

The examples are expanded to highlight how a user can transition between
states, as well as highlight some of the shortcuts built into the API.

The current implementation is also more flexible, in the sense that it
allows for more states to be added in the future. This lets us implement
different strategies for handling clocks, including one that mimics the
current API, allowing for multiple calls to prepare() and enable().

The users (cpufreq.rs/ rcpufreq_dt.rs) were updated by this patch (and not
a separate one) to reflect the new changes. This is needed, because
otherwise this patch would break the build.

Link: https://crates.io/crates/sealed [1]
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 drivers/cpufreq/rcpufreq_dt.rs |   2 +-
 drivers/gpu/drm/tyr/driver.rs  |  31 +---
 drivers/pwm/pwm_th1520.rs      |  17 +-
 rust/kernel/clk.rs             | 399 +++++++++++++++++++++++++++--------------
 rust/kernel/cpufreq.rs         |   8 +-
 5 files changed, 281 insertions(+), 176 deletions(-)

diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index 31e07f0279db..f1bd7d71ed54 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -41,7 +41,7 @@ struct CPUFreqDTDevice {
     freq_table: opp::FreqTable,
     _mask: CpumaskVar,
     _token: Option<opp::ConfigToken>,
-    _clk: Clk,
+    _clk: Clk<kernel::clk::Unprepared>,
 }
 
 #[derive(Default)]
diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index 09711fb7fe0b..5692def25621 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -2,7 +2,7 @@
 
 use kernel::c_str;
 use kernel::clk::Clk;
-use kernel::clk::OptionalClk;
+use kernel::clk::Enabled;
 use kernel::device::Bound;
 use kernel::device::Core;
 use kernel::device::Device;
@@ -37,7 +37,7 @@ pub(crate) struct TyrDriver {
     device: ARef<TyrDevice>,
 }
 
-#[pin_data(PinnedDrop)]
+#[pin_data]
 pub(crate) struct TyrData {
     pub(crate) pdev: ARef<platform::Device>,
 
@@ -92,13 +92,9 @@ fn probe(
         pdev: &platform::Device<Core>,
         _info: Option<&Self::IdInfo>,
     ) -> impl PinInit<Self, Error> {
-        let core_clk = Clk::get(pdev.as_ref(), Some(c_str!("core")))?;
-        let stacks_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("stacks")))?;
-        let coregroup_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("coregroup")))?;
-
-        core_clk.prepare_enable()?;
-        stacks_clk.prepare_enable()?;
-        coregroup_clk.prepare_enable()?;
+        let core_clk = Clk::<Enabled>::get(pdev.as_ref(), Some(c_str!("core")))?;
+        let stacks_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c_str!("stacks")))?;
+        let coregroup_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c_str!("coregroup")))?;
 
         let mali_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("mali"))?;
         let sram_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("sram"))?;
@@ -145,17 +141,6 @@ impl PinnedDrop for TyrDriver {
     fn drop(self: Pin<&mut Self>) {}
 }
 
-#[pinned_drop]
-impl PinnedDrop for TyrData {
-    fn drop(self: Pin<&mut Self>) {
-        // TODO: the type-state pattern for Clks will fix this.
-        let clks = self.clks.lock();
-        clks.core.disable_unprepare();
-        clks.stacks.disable_unprepare();
-        clks.coregroup.disable_unprepare();
-    }
-}
-
 // We need to retain the name "panthor" to achieve drop-in compatibility with
 // the C driver in the userspace stack.
 const INFO: drm::DriverInfo = drm::DriverInfo {
@@ -181,9 +166,9 @@ impl drm::Driver for TyrDriver {
 
 #[pin_data]
 struct Clocks {
-    core: Clk,
-    stacks: OptionalClk,
-    coregroup: OptionalClk,
+    core: Clk<Enabled>,
+    stacks: Clk<Enabled>,
+    coregroup: Clk<Enabled>,
 }
 
 #[pin_data]
diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
index 043dc4dbc623..f4d03b988533 100644
--- a/drivers/pwm/pwm_th1520.rs
+++ b/drivers/pwm/pwm_th1520.rs
@@ -23,7 +23,7 @@
 use core::ops::Deref;
 use kernel::{
     c_str,
-    clk::Clk,
+    clk::{Clk, Enabled},
     device::{Bound, Core, Device},
     devres,
     io::mem::IoMem,
@@ -90,11 +90,11 @@ struct Th1520WfHw {
 }
 
 /// The driver's private data struct. It holds all necessary devres managed resources.
-#[pin_data(PinnedDrop)]
+#[pin_data]
 struct Th1520PwmDriverData {
     #[pin]
     iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
-    clk: Clk,
+    clk: Clk<Enabled>,
 }
 
 impl pwm::PwmOps for Th1520PwmDriverData {
@@ -299,13 +299,6 @@ fn write_waveform(
     }
 }
 
-#[pinned_drop]
-impl PinnedDrop for Th1520PwmDriverData {
-    fn drop(self: Pin<&mut Self>) {
-        self.clk.disable_unprepare();
-    }
-}
-
 struct Th1520PwmPlatformDriver;
 
 kernel::of_device_table!(
@@ -326,9 +319,7 @@ fn probe(
         let dev = pdev.as_ref();
         let request = pdev.io_request_by_index(0).ok_or(ENODEV)?;
 
-        let clk = Clk::get(dev, None)?;
-
-        clk.prepare_enable()?;
+        let clk = Clk::<Enabled>::get(dev, None)?;
 
         // TODO: Get exclusive ownership of the clock to prevent rate changes.
         // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available.
diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index d192fbd97861..6323b40dc7ba 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -80,17 +80,78 @@ fn from(freq: Hertz) -> Self {
 mod common_clk {
     use super::Hertz;
     use crate::{
-        device::Device,
+        device::{Bound, Device},
         error::{from_err_ptr, to_result, Result},
         prelude::*,
     };
 
-    use core::{ops::Deref, ptr};
+    use core::{marker::PhantomData, mem::ManuallyDrop, ptr};
+
+    mod private {
+        pub trait Sealed {}
+
+        impl Sealed for super::Unprepared {}
+        impl Sealed for super::Prepared {}
+        impl Sealed for super::Enabled {}
+    }
+
+    /// A trait representing the different states that a [`Clk`] can be in.
+    pub trait ClkState: private::Sealed {
+        /// Whether the clock should be disabled when dropped.
+        const DISABLE_ON_DROP: bool;
+
+        /// Whether the clock should be unprepared when dropped.
+        const UNPREPARE_ON_DROP: bool;
+    }
+
+    /// A state where the [`Clk`] is not prepared and not enabled.
+    pub struct Unprepared;
+
+    /// A state where the [`Clk`] is prepared but not enabled.
+    pub struct Prepared;
+
+    /// A state where the [`Clk`] is both prepared and enabled.
+    pub struct Enabled;
+
+    impl ClkState for Unprepared {
+        const DISABLE_ON_DROP: bool = false;
+        const UNPREPARE_ON_DROP: bool = false;
+    }
+
+    impl ClkState for Prepared {
+        const DISABLE_ON_DROP: bool = false;
+        const UNPREPARE_ON_DROP: bool = true;
+    }
+
+    impl ClkState for Enabled {
+        const DISABLE_ON_DROP: bool = true;
+        const UNPREPARE_ON_DROP: bool = true;
+    }
+
+    /// An error that can occur when trying to convert a [`Clk`] between states.
+    pub struct Error<State: ClkState> {
+        /// The error that occurred.
+        pub error: kernel::error::Error,
+
+        /// The [`Clk`] that caused the error, so that the operation may be
+        /// retried.
+        pub clk: Clk<State>,
+    }
 
     /// A reference-counted clock.
     ///
     /// Rust abstraction for the C [`struct clk`].
     ///
+    /// A [`Clk`] instance represents a clock that can be in one of several
+    /// states: [`Unprepared`], [`Prepared`], or [`Enabled`].
+    ///
+    /// No action needs to be taken when a [`Clk`] is dropped. The calls to
+    /// `clk_unprepare()` and `clk_disable()` will be placed as applicable.
+    ///
+    /// An optional [`Clk`] is treated just like a regular [`Clk`], but its
+    /// inner `struct clk` pointer is `NULL`. This interfaces correctly with the
+    /// C API and also exposes all the methods of a regular [`Clk`] to users.
+    ///
     /// # Invariants
     ///
     /// A [`Clk`] instance holds either a pointer to a valid [`struct clk`] created by the C
@@ -99,20 +160,39 @@ mod common_clk {
     /// Instances of this type are reference-counted. Calling [`Clk::get`] ensures that the
     /// allocation remains valid for the lifetime of the [`Clk`].
     ///
+    /// The [`Prepared`] state is associated with a single count of
+    /// `clk_prepare()`, and the [`Enabled`] state is associated with a single
+    /// count of `clk_enable()`, and the [`Prepared`] state is associated with a
+    /// single count of `clk_prepare()` and `clk_enable()`.
+    ///
+    /// All states are associated with a single count of `clk_get()`.
+    ///
     /// # Examples
     ///
     /// The following example demonstrates how to obtain and configure a clock for a device.
     ///
     /// ```
     /// use kernel::c_str;
-    /// use kernel::clk::{Clk, Hertz};
-    /// use kernel::device::Device;
+    /// use kernel::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
+    /// use kernel::device::{Bound, Device};
     /// use kernel::error::Result;
     ///
-    /// fn configure_clk(dev: &Device) -> Result {
-    ///     let clk = Clk::get(dev, Some(c_str!("apb_clk")))?;
+    /// fn configure_clk(dev: &Device<Bound>) -> Result {
+    ///     // The fastest way is to use a version of `Clk::get` for the desired
+    ///     // state, i.e.:
+    ///     let clk: Clk<Enabled> = Clk::<Enabled>::get(dev, Some(c_str!("apb_clk")))?;
     ///
-    ///     clk.prepare_enable()?;
+    ///     // Any other state is also possible, e.g.:
+    ///     let clk: Clk<Prepared> = Clk::<Prepared>::get(dev, Some(c_str!("apb_clk")))?;
+    ///
+    ///     // Later:
+    ///     let clk: Clk<Enabled> = clk.enable().map_err(|error| {
+    ///         error.error
+    ///     })?;
+    ///
+    ///     // Note that error.clk is the original `clk` if the operation
+    ///     // failed. It is provided as a convenience so that the operation may be
+    ///     // retried in case of errors.
     ///
     ///     let expected_rate = Hertz::from_ghz(1);
     ///
@@ -120,111 +200,208 @@ mod common_clk {
     ///         clk.set_rate(expected_rate)?;
     ///     }
     ///
-    ///     clk.disable_unprepare();
+    ///     // Nothing is needed here. The drop implementation will undo any
+    ///     // operations as appropriate.
+    ///     Ok(())
+    /// }
+    ///
+    /// fn shutdown(clk: Clk<Enabled>) -> Result {
+    ///     // The states can be traversed "in the reverse order" as well:
+    ///     let clk: Clk<Prepared> = clk.disable().map_err(|error| {
+    ///         error.error
+    ///     })?;
+    ///
+    ///     // This is of type `Clk<Unprepared>`.
+    ///     let clk = clk.unprepare();
+    ///
     ///     Ok(())
     /// }
     /// ```
     ///
     /// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
     #[repr(transparent)]
-    pub struct Clk(*mut bindings::clk);
+    pub struct Clk<T: ClkState> {
+        inner: *mut bindings::clk,
+        _phantom: core::marker::PhantomData<T>,
+    }
 
     // SAFETY: It is safe to call `clk_put` on another thread than where `clk_get` was called.
-    unsafe impl Send for Clk {}
+    unsafe impl<T: ClkState> Send for Clk<T> {}
 
     // SAFETY: It is safe to call any combination of the `&self` methods in parallel, as the
     // methods are synchronized internally.
-    unsafe impl Sync for Clk {}
+    unsafe impl<T: ClkState> Sync for Clk<T> {}
 
-    impl Clk {
-        /// Gets [`Clk`] corresponding to a [`Device`] and a connection id.
+    impl Clk<Unprepared> {
+        /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection
+        /// id.
         ///
         /// Equivalent to the kernel's [`clk_get`] API.
         ///
         /// [`clk_get`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_get
-        pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
+        #[inline]
+        pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
             let con_id = name.map_or(ptr::null(), |n| n.as_char_ptr());
 
-            // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
-            //
+            // SAFETY: It is safe to call [`clk_get`] for a valid device pointer
+            // and any `con_id`, including NULL.
+            let inner = from_err_ptr(unsafe { bindings::clk_get(dev.as_raw(), con_id) })?;
+
             // INVARIANT: The reference-count is decremented when [`Clk`] goes out of scope.
-            Ok(Self(from_err_ptr(unsafe {
-                bindings::clk_get(dev.as_raw(), con_id)
-            })?))
+            Ok(Self {
+                inner,
+                _phantom: PhantomData,
+            })
         }
 
-        /// Obtain the raw [`struct clk`] pointer.
+        /// Behaves the same as [`Self::get`], except when there is no clock
+        /// producer. In this case, instead of returning [`ENOENT`], it returns
+        /// a dummy [`Clk`].
         #[inline]
-        pub fn as_raw(&self) -> *mut bindings::clk {
-            self.0
+        pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
+            let con_id = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+            // SAFETY: It is safe to call [`clk_get`] for a valid device pointer
+            // and any `con_id`, including NULL.
+            let inner = from_err_ptr(unsafe { bindings::clk_get_optional(dev.as_raw(), con_id) })?;
+
+            // INVARIANT: The reference-count is decremented when [`Clk`] goes out of scope.
+            Ok(Self {
+                inner,
+                _phantom: PhantomData,
+            })
         }
 
-        /// Enable the clock.
+        /// Attempts to convert the [`Clk`] to a [`Prepared`] state.
         ///
-        /// Equivalent to the kernel's [`clk_enable`] API.
+        /// Equivalent to the kernel's [`clk_prepare`] API.
         ///
-        /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
+        /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
         #[inline]
-        pub fn enable(&self) -> Result {
-            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
-            // [`clk_enable`].
-            to_result(unsafe { bindings::clk_enable(self.as_raw()) })
+        pub fn prepare(self) -> Result<Clk<Prepared>, Error<Unprepared>> {
+            // We will be transferring the ownership of our `clk_get()` count to
+            // `Clk<Prepared>`.
+            let clk = ManuallyDrop::new(self);
+
+            // SAFETY: By the type invariants, `self.0` is a valid argument for
+            // [`clk_prepare`].
+            to_result(unsafe { bindings::clk_prepare(clk.as_raw()) })
+                .map(|()| Clk {
+                    inner: clk.inner,
+                    _phantom: PhantomData,
+                })
+                .map_err(|error| Error {
+                    error,
+                    clk: ManuallyDrop::into_inner(clk),
+                })
         }
+    }
 
-        /// Disable the clock.
-        ///
-        /// Equivalent to the kernel's [`clk_disable`] API.
+    impl Clk<Prepared> {
+        /// Obtains a [`Clk`] from a bound [`Device`] and a connection id and
+        /// prepares it.
         ///
-        /// [`clk_disable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_disable
+        /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
         #[inline]
-        pub fn disable(&self) {
-            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
-            // [`clk_disable`].
-            unsafe { bindings::clk_disable(self.as_raw()) };
+        pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Prepared>> {
+            Clk::<Unprepared>::get(dev, name)?
+                .prepare()
+                .map_err(|error| error.error)
         }
 
-        /// Prepare the clock.
-        ///
-        /// Equivalent to the kernel's [`clk_prepare`] API.
-        ///
-        /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
+        /// Behaves the same as [`Self::get`], except when there is no clock
+        /// producer. In this case, instead of returning [`ENOENT`], it returns
+        /// a dummy [`Clk`].
         #[inline]
-        pub fn prepare(&self) -> Result {
-            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
-            // [`clk_prepare`].
-            to_result(unsafe { bindings::clk_prepare(self.as_raw()) })
+        pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Prepared>> {
+            Clk::<Unprepared>::get_optional(dev, name)?
+                .prepare()
+                .map_err(|error| error.error)
         }
 
-        /// Unprepare the clock.
+        /// Attempts to convert the [`Clk`] to an [`Unprepared`] state.
         ///
         /// Equivalent to the kernel's [`clk_unprepare`] API.
-        ///
-        /// [`clk_unprepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_unprepare
         #[inline]
-        pub fn unprepare(&self) {
-            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
+        pub fn unprepare(self) -> Clk<Unprepared> {
+            // We will be transferring the ownership of our `clk_get()` count to
+            // `Clk<Unprepared>`.
+            let clk = ManuallyDrop::new(self);
+
+            // SAFETY: By the type invariants, `self.0` is a valid argument for
             // [`clk_unprepare`].
-            unsafe { bindings::clk_unprepare(self.as_raw()) };
+            unsafe { bindings::clk_unprepare(clk.as_raw()) }
+
+            Clk {
+                inner: clk.inner,
+                _phantom: PhantomData,
+            }
         }
 
-        /// Prepare and enable the clock.
+        /// Attempts to convert the [`Clk`] to an [`Enabled`] state.
+        ///
+        /// Equivalent to the kernel's [`clk_enable`] API.
         ///
-        /// Equivalent to calling [`Clk::prepare`] followed by [`Clk::enable`].
+        /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
         #[inline]
-        pub fn prepare_enable(&self) -> Result {
-            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
-            // [`clk_prepare_enable`].
-            to_result(unsafe { bindings::clk_prepare_enable(self.as_raw()) })
+        pub fn enable(self) -> Result<Clk<Enabled>, Error<Prepared>> {
+            // We will be transferring the ownership of our `clk_get()` and
+            // `clk_prepare()` counts to `Clk<Enabled>`.
+            let clk = ManuallyDrop::new(self);
+
+            // SAFETY: By the type invariants, `self.0` is a valid argument for
+            // [`clk_enable`].
+            to_result(unsafe { bindings::clk_enable(clk.as_raw()) })
+                .map(|()| Clk {
+                    inner: clk.inner,
+                    _phantom: PhantomData,
+                })
+                .map_err(|error| Error {
+                    error,
+                    clk: ManuallyDrop::into_inner(clk),
+                })
         }
+    }
 
-        /// Disable and unprepare the clock.
+    impl Clk<Enabled> {
+        /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection id
+        /// and then prepares and enables it.
         ///
-        /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
+        /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
+        /// followed by [`Clk::enable`].
         #[inline]
-        pub fn disable_unprepare(&self) {
-            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
-            // [`clk_disable_unprepare`].
-            unsafe { bindings::clk_disable_unprepare(self.as_raw()) };
+        pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
+            Clk::<Prepared>::get(dev, name)?
+                .enable()
+                .map_err(|error| error.error)
+        }
+
+        /// Behaves the same as [`Self::get`], except when there is no clock
+        /// producer. In this case, instead of returning [`ENOENT`], it returns
+        /// a dummy [`Clk`].
+        #[inline]
+        pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
+            Clk::<Prepared>::get_optional(dev, name)?
+                .enable()
+                .map_err(|error| error.error)
+        }
+
+        /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]
+        /// state.
+        #[inline]
+        pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
+            // We will be transferring the ownership of our `clk_get()` and
+            // `clk_enable()` counts to `Clk<Prepared>`.
+            let clk = ManuallyDrop::new(self);
+
+            // SAFETY: By the type invariants, `self.0` is a valid argument for
+            // [`clk_disable`].
+            unsafe { bindings::clk_disable(clk.as_raw()) };
+
+            Ok(Clk {
+                inner: clk.inner,
+                _phantom: PhantomData,
+            })
         }
 
         /// Get clock's rate.
@@ -252,83 +429,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
         }
     }
 
-    impl Drop for Clk {
-        fn drop(&mut self) {
-            // SAFETY: By the type invariants, self.as_raw() is a valid argument for [`clk_put`].
-            unsafe { bindings::clk_put(self.as_raw()) };
-        }
-    }
-
-    /// A reference-counted optional clock.
-    ///
-    /// A lightweight wrapper around an optional [`Clk`]. An [`OptionalClk`] represents a [`Clk`]
-    /// that a driver can function without but may improve performance or enable additional
-    /// features when available.
-    ///
-    /// # Invariants
-    ///
-    /// An [`OptionalClk`] instance encapsulates a [`Clk`] with either a valid [`struct clk`] or
-    /// `NULL` pointer.
-    ///
-    /// Instances of this type are reference-counted. Calling [`OptionalClk::get`] ensures that the
-    /// allocation remains valid for the lifetime of the [`OptionalClk`].
-    ///
-    /// # Examples
-    ///
-    /// The following example demonstrates how to obtain and configure an optional clock for a
-    /// device. The code functions correctly whether or not the clock is available.
-    ///
-    /// ```
-    /// use kernel::c_str;
-    /// use kernel::clk::{OptionalClk, Hertz};
-    /// use kernel::device::Device;
-    /// use kernel::error::Result;
-    ///
-    /// fn configure_clk(dev: &Device) -> Result {
-    ///     let clk = OptionalClk::get(dev, Some(c_str!("apb_clk")))?;
-    ///
-    ///     clk.prepare_enable()?;
-    ///
-    ///     let expected_rate = Hertz::from_ghz(1);
-    ///
-    ///     if clk.rate() != expected_rate {
-    ///         clk.set_rate(expected_rate)?;
-    ///     }
-    ///
-    ///     clk.disable_unprepare();
-    ///     Ok(())
-    /// }
-    /// ```
-    ///
-    /// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
-    pub struct OptionalClk(Clk);
-
-    impl OptionalClk {
-        /// Gets [`OptionalClk`] corresponding to a [`Device`] and a connection id.
-        ///
-        /// Equivalent to the kernel's [`clk_get_optional`] API.
-        ///
-        /// [`clk_get_optional`]:
-        /// https://docs.kernel.org/core-api/kernel-api.html#c.clk_get_optional
-        pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
-            let con_id = name.map_or(ptr::null(), |n| n.as_char_ptr());
-
-            // SAFETY: It is safe to call [`clk_get_optional`] for a valid device pointer.
-            //
-            // INVARIANT: The reference-count is decremented when [`OptionalClk`] goes out of
-            // scope.
-            Ok(Self(Clk(from_err_ptr(unsafe {
-                bindings::clk_get_optional(dev.as_raw(), con_id)
-            })?)))
+    impl<T: ClkState> Clk<T> {
+        /// Obtain the raw [`struct clk`] pointer.
+        #[inline]
+        pub fn as_raw(&self) -> *mut bindings::clk {
+            self.inner
         }
     }
 
-    // Make [`OptionalClk`] behave like [`Clk`].
-    impl Deref for OptionalClk {
-        type Target = Clk;
+    impl<T: ClkState> Drop for Clk<T> {
+        fn drop(&mut self) {
+            if T::DISABLE_ON_DROP {
+                // SAFETY: By the type invariants, self.as_raw() is a valid argument for
+                // [`clk_disable`].
+                unsafe { bindings::clk_disable(self.as_raw()) };
+            }
+
+            if T::UNPREPARE_ON_DROP {
+                // SAFETY: By the type invariants, self.as_raw() is a valid argument for
+                // [`clk_unprepare`].
+                unsafe { bindings::clk_unprepare(self.as_raw()) };
+            }
 
-        fn deref(&self) -> &Clk {
-            &self.0
+            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
+            // [`clk_put`].
+            unsafe { bindings::clk_put(self.as_raw()) };
         }
     }
 }
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index f968fbd22890..d51c697cad9e 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -552,8 +552,12 @@ pub fn cpus(&mut self) -> &mut cpumask::Cpumask {
     /// The caller must guarantee that the returned [`Clk`] is not dropped while it is getting used
     /// by the C code.
     #[cfg(CONFIG_COMMON_CLK)]
-    pub unsafe fn set_clk(&mut self, dev: &Device, name: Option<&CStr>) -> Result<Clk> {
-        let clk = Clk::get(dev, name)?;
+    pub unsafe fn set_clk(
+        &mut self,
+        dev: &Device<Bound>,
+        name: Option<&CStr>,
+    ) -> Result<Clk<crate::clk::Unprepared>> {
+        let clk = Clk::<crate::clk::Unprepared>::get(dev, name)?;
         self.as_mut_ref().clk = clk.as_raw();
         Ok(clk)
     }

-- 
2.52.0

[PATCH v3 2/3] rust: clk: add devres-managed clks

[Daniel Almeida]

The clk API allows fine-grained control, but some drivers might be
more interested in a "set and forget" API.

Expand the current API to support this. The clock will automatically be
disabled, unprepared and freed when the device is unbound from the bus
without further intervention by the driver.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/clk.rs | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index 6323b40dc7ba..e840e7c20af7 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -95,6 +95,49 @@ impl Sealed for super::Prepared {}
         impl Sealed for super::Enabled {}
     }
 
+    /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
+    ///
+    /// [`devres`]: crate::devres::Devres
+    pub fn devm_enable(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
+        let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+        // SAFETY: It is safe to call [`devm_clk_get_enabled`] with a valid
+        // device pointer.
+        from_err_ptr(unsafe { bindings::devm_clk_get_enabled(dev.as_raw(), name) })?;
+        Ok(())
+    }
+
+    /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
+    ///
+    /// This does not print any error messages if the clock is not found.
+    ///
+    /// [`devres`]: crate::devres::Devres
+    pub fn devm_enable_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
+        let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+        // SAFETY: It is safe to call [`devm_clk_get_optional_enabled`] with a
+        // valid device pointer.
+        from_err_ptr(unsafe { bindings::devm_clk_get_optional_enabled(dev.as_raw(), name) })?;
+        Ok(())
+    }
+
+    /// Same as [`devm_enable_optional`], but also sets the rate.
+    pub fn devm_enable_optional_with_rate(
+        dev: &Device,
+        name: Option<&CStr>,
+        rate: Hertz,
+    ) -> Result {
+        let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+        // SAFETY: It is safe to call
+        // [`devm_clk_get_optional_enabled_with_rate`] with a valid device
+        // pointer.
+        from_err_ptr(unsafe {
+            bindings::devm_clk_get_optional_enabled_with_rate(dev.as_raw(), name, rate.as_hz())
+        })?;
+        Ok(())
+    }
+
     /// A trait representing the different states that a [`Clk`] can be in.
     pub trait ClkState: private::Sealed {
         /// Whether the clock should be disabled when dropped.

-- 
2.52.0

[PATCH v3 3/3] rust: clk: use 'kernel vertical style' for imports

[Daniel Almeida]

Convert all imports to use the new import style. This will make it easier
to land new changes in the future.

No change of functionality implied.

Link: https://docs.kernel.org/rust/coding-guidelines.html#imports
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/clk.rs | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index e840e7c20af7..73a2b51414a1 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -80,12 +80,23 @@ fn from(freq: Hertz) -> Self {
 mod common_clk {
     use super::Hertz;
     use crate::{
-        device::{Bound, Device},
-        error::{from_err_ptr, to_result, Result},
-        prelude::*,
+        device::{
+            Bound,
+            Device, //
+        },
+        error::{
+            from_err_ptr,
+            to_result,
+            Result, //
+        },
+        prelude::*, //
     };
 
-    use core::{marker::PhantomData, mem::ManuallyDrop, ptr};
+    use core::{
+        marker::PhantomData,
+        mem::ManuallyDrop,
+        ptr, //
+    };
 
     mod private {
         pub trait Sealed {}
@@ -216,8 +227,17 @@ pub struct Error<State: ClkState> {
     ///
     /// ```
     /// use kernel::c_str;
-    /// use kernel::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
-    /// use kernel::device::{Bound, Device};
+    /// use kernel::clk::{
+    ///     Clk,
+    ///     Enabled,
+    ///     Hertz,
+    ///     Prepared,
+    ///     Unprepared, //
+    /// };
+    /// use kernel::device::{
+    ///     Bound,
+    ///     Device, //
+    /// };
     /// use kernel::error::Result;
     ///
     /// fn configure_clk(dev: &Device<Bound>) -> Result {

-- 
2.52.0

Re: [PATCH v3 3/3] rust: clk: use 'kernel vertical style' for imports

[Maxime Ripard]

On Wed, 7 Jan 2026 12:09:54 -0300, Daniel Almeida wrote:
> Convert all imports to use the new import style. This will make it easier
> to land new changes in the future.
> 
> No change of functionality implied.
> 
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

Hi Daniel,

On Wed, Jan 07, 2026 at 12:09:52PM -0300, Daniel Almeida wrote:
> The current Clk abstraction can still be improved on the following issues:
> 
> a) It only keeps track of a count to clk_get(), which means that users have
> to manually call disable() and unprepare(), or a variation of those, like
> disable_unprepare().
> 
> b) It allows repeated calls to prepare() or enable(), but it keeps no track
> of how often these were called, i.e., it's currently legal to write the
> following:
> 
> clk.prepare();
> clk.prepare();
> clk.enable();
> clk.enable();
> 
> And nothing gets undone on drop().
> 
> c) It adds a OptionalClk type that is probably not needed. There is no
> "struct optional_clk" in C and we should probably not add one.
> 
> d) It does not let a user express the state of the clk through the
> type system. For example, there is currently no way to encode that a Clk is
> enabled via the type system alone.
> 
> In light of the Regulator abstraction that was recently merged, switch this
> abstraction to use the type-state pattern instead. It solves both a) and b)
> by establishing a number of states and the valid ways to transition between
> them. It also automatically undoes any call to clk_get(), clk_prepare() and
> clk_enable() as applicable on drop(), so users do not have to do anything
> special before Clk goes out of scope.
> 
> It solves c) by removing the OptionalClk type, which is now simply encoded
> as a Clk whose inner pointer is NULL.
> 
> It solves d) by directly encoding the state of the Clk into the type, e.g.:
> Clk<Enabled> is now known to be a Clk that is enabled.
> 
> The INVARIANTS section for Clk is expanded to highlight the relationship
> between the states and the respective reference counts that are owned by
> each of them.
> 
> The examples are expanded to highlight how a user can transition between
> states, as well as highlight some of the shortcuts built into the API.
> 
> The current implementation is also more flexible, in the sense that it
> allows for more states to be added in the future. This lets us implement
> different strategies for handling clocks, including one that mimics the
> current API, allowing for multiple calls to prepare() and enable().
> 
> The users (cpufreq.rs/ rcpufreq_dt.rs) were updated by this patch (and not
> a separate one) to reflect the new changes. This is needed, because
> otherwise this patch would break the build.
> 
> Link: https://crates.io/crates/sealed [1]
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

I don't know the typestate pattern that well, but I wonder if we don't
paint ourselves into a corner by introducing it.

While it's pretty common to get your clock from the get go into a state,
and then don't modify it (like what devm_clk_get_enabled provides for
example), and the typestate pattern indeed works great for those, we
also have a significant number of drivers that will have a finer-grained
control over the clock enablement for PM.

For example, it's quite typical to have (at least) one clock for the bus
interface that drives the register, and one that drives the main
component logic. The former needs to be enabled only when you're
accessing the registers (and can be abstracted with
regmap_mmio_attach_clk for example), and the latter needs to be enabled
only when the device actually starts operating.

You have a similar thing for the prepare vs enable thing. The difference
between the two is that enable can be called into atomic context but
prepare can't.

So for drivers that would care about this, you would create your device
with an unprepared clock, and then at various times during the driver
lifetime, you would mutate that state.

AFAIU, encoding the state of the clock into the Clk type (and thus
forcing the structure that holds it) prevents that mutation. If not, we
should make it clearer (by expanding the doc maybe?) how such a pattern
can be supported.

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Miguel Ojeda]

On Thu, Jan 8, 2026 at 9:07 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> AFAIU, encoding the state of the clock into the Clk type (and thus
> forcing the structure that holds it) prevents that mutation. If not, we
> should make it clearer (by expanding the doc maybe?) how such a pattern
> can be supported.

One possibility to consider in cases like this is whether supporting
both cases differently makes sense, i.e. one for that covers
easily/safely/... the usual "80%" of cases, and another "advanced" one
(possibly unsafe etc.) for the rest.

While it may be a bit more to maintain, it may pay itself off by
making it easier to review the easy ones if the majority only need
that etc.

Cheers,
Miguel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

Hi Maxime :)

> 
> I don't know the typestate pattern that well, but I wonder if we don't
> paint ourselves into a corner by introducing it.
> 
> While it's pretty common to get your clock from the get go into a state,
> and then don't modify it (like what devm_clk_get_enabled provides for
> example), and the typestate pattern indeed works great for those, we

Minor correction, devm_clk_get_enabled is not handled by the typestate
pattern. The next patch does include this function for convenience, but
you get a Result<()>. The typestate pattern is used when you want more
control.

> also have a significant number of drivers that will have a finer-grained
> control over the clock enablement for PM.
> 
> For example, it's quite typical to have (at least) one clock for the bus
> interface that drives the register, and one that drives the main
> component logic. The former needs to be enabled only when you're
> accessing the registers (and can be abstracted with
> regmap_mmio_attach_clk for example), and the latter needs to be enabled
> only when the device actually starts operating.
> 
> You have a similar thing for the prepare vs enable thing. The difference
> between the two is that enable can be called into atomic context but
> prepare can't.
> 
> So for drivers that would care about this, you would create your device
> with an unprepared clock, and then at various times during the driver
> lifetime, you would mutate that state.
> 
> AFAIU, encoding the state of the clock into the Clk type (and thus
> forcing the structure that holds it) prevents that mutation. If not, we
> should make it clearer (by expanding the doc maybe?) how such a pattern
> can be supported.
> 
> Maxime

IIUC, your main point seems to be about mutating the state at runtime? This is
possible with this code. You can just have an enum, for example:

enum MyClocks {
	Unprepared(Clk<Unprepared>),
        Prepared(Clk<Prepared>),
	Enabled(Clk<Enabled>), 
}

In fact, I specifically wanted to ensure that this was possible when writing
these patches, as it’s needed by drivers. If you want to, I can cover that in
the examples, no worries.

Same for Regulator<T>, by the way.

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

> On 8 Jan 2026, at 10:57, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Thu, Jan 8, 2026 at 9:07 AM Maxime Ripard <mripard@kernel.org> wrote:
>> 
>> AFAIU, encoding the state of the clock into the Clk type (and thus
>> forcing the structure that holds it) prevents that mutation. If not, we
>> should make it clearer (by expanding the doc maybe?) how such a pattern
>> can be supported.
> 
> One possibility to consider in cases like this is whether supporting
> both cases differently makes sense, i.e. one for that covers
> easily/safely/... the usual "80%" of cases, and another "advanced" one
> (possibly unsafe etc.) for the rest.
> 
> While it may be a bit more to maintain, it may pay itself off by
> making it easier to review the easy ones if the majority only need
> that etc.
> 
> Cheers,
> Miguel

This is already the case. The devm_* stuff in the next patch covers a lot of
simpler drivers (who might simply want to enable the clk, perhaps with a given
rate), while the typestate pattern allows for more control when needed. There
will be users for both, IMHO.

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
> Hi Maxime :)
> 
> > 
> > I don't know the typestate pattern that well, but I wonder if we don't
> > paint ourselves into a corner by introducing it.
> > 
> > While it's pretty common to get your clock from the get go into a state,
> > and then don't modify it (like what devm_clk_get_enabled provides for
> > example), and the typestate pattern indeed works great for those, we
> 
> Minor correction, devm_clk_get_enabled is not handled by the typestate
> pattern. The next patch does include this function for convenience, but
> you get a Result<()>. The typestate pattern is used when you want more
> control.
>
> > also have a significant number of drivers that will have a finer-grained
> > control over the clock enablement for PM.
> > 
> > For example, it's quite typical to have (at least) one clock for the bus
> > interface that drives the register, and one that drives the main
> > component logic. The former needs to be enabled only when you're
> > accessing the registers (and can be abstracted with
> > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > only when the device actually starts operating.
> > 
> > You have a similar thing for the prepare vs enable thing. The difference
> > between the two is that enable can be called into atomic context but
> > prepare can't.
> > 
> > So for drivers that would care about this, you would create your device
> > with an unprepared clock, and then at various times during the driver
> > lifetime, you would mutate that state.
> > 
> > AFAIU, encoding the state of the clock into the Clk type (and thus
> > forcing the structure that holds it) prevents that mutation. If not, we
> > should make it clearer (by expanding the doc maybe?) how such a pattern
> > can be supported.
> > 
> > Maxime
> 
> IIUC, your main point seems to be about mutating the state at runtime? This is
> possible with this code. You can just have an enum, for example:
> 
> enum MyClocks {
> 	Unprepared(Clk<Unprepared>),
>         Prepared(Clk<Prepared>),
> 	Enabled(Clk<Enabled>), 
> }
> 
> In fact, I specifically wanted to ensure that this was possible when writing
> these patches, as it’s needed by drivers. If you want to, I can cover that in
> the examples, no worries.

Yes, that would be great. I do wonder though if it wouldn't make sense
to turn it the other way around. It creates a fair share of boilerplate
for a number of drivers. Can't we keep Clk the way it is as a
lower-level type, and crate a ManagedClk (or whatever name you prefer)
that drivers can use, and would be returned by higher-level helpers, if
they so choose?

That way, we do have the typestate API for whoever wants to, without
creating too much boilerplate for everybody else.

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

> On 19 Jan 2026, at 07:45, Maxime Ripard <mripard@kernel.org> wrote:
> 
> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>> Hi Maxime :)
>> 
>>> 
>>> I don't know the typestate pattern that well, but I wonder if we don't
>>> paint ourselves into a corner by introducing it.
>>> 
>>> While it's pretty common to get your clock from the get go into a state,
>>> and then don't modify it (like what devm_clk_get_enabled provides for
>>> example), and the typestate pattern indeed works great for those, we
>> 
>> Minor correction, devm_clk_get_enabled is not handled by the typestate
>> pattern. The next patch does include this function for convenience, but
>> you get a Result<()>. The typestate pattern is used when you want more
>> control.
>> 
>>> also have a significant number of drivers that will have a finer-grained
>>> control over the clock enablement for PM.
>>> 
>>> For example, it's quite typical to have (at least) one clock for the bus
>>> interface that drives the register, and one that drives the main
>>> component logic. The former needs to be enabled only when you're
>>> accessing the registers (and can be abstracted with
>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>> only when the device actually starts operating.
>>> 
>>> You have a similar thing for the prepare vs enable thing. The difference
>>> between the two is that enable can be called into atomic context but
>>> prepare can't.
>>> 
>>> So for drivers that would care about this, you would create your device
>>> with an unprepared clock, and then at various times during the driver
>>> lifetime, you would mutate that state.
>>> 
>>> AFAIU, encoding the state of the clock into the Clk type (and thus
>>> forcing the structure that holds it) prevents that mutation. If not, we
>>> should make it clearer (by expanding the doc maybe?) how such a pattern
>>> can be supported.
>>> 
>>> Maxime
>> 
>> IIUC, your main point seems to be about mutating the state at runtime? This is
>> possible with this code. You can just have an enum, for example:
>> 
>> enum MyClocks {
>> Unprepared(Clk<Unprepared>),
>>        Prepared(Clk<Prepared>),
>> Enabled(Clk<Enabled>), 
>> }
>> 
>> In fact, I specifically wanted to ensure that this was possible when writing
>> these patches, as it’s needed by drivers. If you want to, I can cover that in
>> the examples, no worries.
> 
> Yes, that would be great. I do wonder though if it wouldn't make sense
> to turn it the other way around. It creates a fair share of boilerplate
> for a number of drivers. Can't we keep Clk the way it is as a
> lower-level type, and crate a ManagedClk (or whatever name you prefer)
> that drivers can use, and would be returned by higher-level helpers, if
> they so choose?

The problem with keeping it the way it is that you’re back to manual
prepare/unprepare and enable/disable, as the type-state is what’s enforcing
the correct order of calls. This is also the case when the type is dropped.

In fact, one of the aims of this patch is to get rid of the current Clk type
before we have more users. The current series fixes this by enforcing a sane
order of operations. Most importantly, it enforces that the refcounts to get(),
enable() and etc are handled correctly using the type system.

It rids us of this problem, which is possible today:

clk.enable();
clk.prepare();
clk.prepare();
clk.disable_unprepare();
clk.set_rate();


> 
> That way, we do have the typestate API for whoever wants to, without
> creating too much boilerplate for everybody else.


But that's how it works in this series. The typestate pattern is opt-in. If you
need to "set and forget" there's the devm API that's introduced by the next
patch. I can expose more devm_* APIs if you want.

I'm not sure the boilerplate is significant, by the way. You can just do:


Clk::<Enabled>::get();


As a starting point, and have the enum thing (which is also simple) _if_ you
need to manually enable/disable at runtime. Most of the time, you will only
need to mention the type state once, like I did in the call above, and then
the type system will figure out the rest when transitions take place.

What boilerplate did you have in mind?

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Alice Ryhl]

On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
> > > For example, it's quite typical to have (at least) one clock for the bus
> > > interface that drives the register, and one that drives the main
> > > component logic. The former needs to be enabled only when you're
> > > accessing the registers (and can be abstracted with
> > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > > only when the device actually starts operating.
> > > 
> > > You have a similar thing for the prepare vs enable thing. The difference
> > > between the two is that enable can be called into atomic context but
> > > prepare can't.
> > > 
> > > So for drivers that would care about this, you would create your device
> > > with an unprepared clock, and then at various times during the driver
> > > lifetime, you would mutate that state.

The case where you're doing it only while accessing registers is
interesting, because that means the Enable bit may be owned by a local
variable. We may imagine an:

    let enabled = self.prepared_clk.enable_scoped();
    ... use registers
    drop(enabled);

Now ... this doesn't quite work with the current API - the current
Enabled stated owns both a prepare and enable count, but the above keeps
the prepare count in `self` and the enabled count in a local variable.
But it could be done with a fourth state, or by a closure method:

    self.prepared_clk.with_enabled(|| {
        ... use registers
    });

All of this would work with an immutable variable of type Clk<Prepared>.

> > > AFAIU, encoding the state of the clock into the Clk type (and thus
> > > forcing the structure that holds it) prevents that mutation. If not, we
> > > should make it clearer (by expanding the doc maybe?) how such a pattern
> > > can be supported.
> > > 
> > > Maxime
> > 
> > IIUC, your main point seems to be about mutating the state at runtime? This is
> > possible with this code. You can just have an enum, for example:
> > 
> > enum MyClocks {
> >     Unprepared(Clk<Unprepared>),
> >     Prepared(Clk<Prepared>),
> >     Enabled(Clk<Enabled>), 
> > }

I believe you need an extra state if the state is not bound to the scope
of a function:

enum MyClocks {
    Unprepared(Clk<Unprepared>),
    Prepared(Clk<Prepared>),
    Enabled(Clk<Enabled>), 
    Transitioning,
}

since mem::replace() needs a new value before you can take ownership of
the existing Clk value.

> > In fact, I specifically wanted to ensure that this was possible when writing
> > these patches, as it’s needed by drivers. If you want to, I can cover that in
> > the examples, no worries.
> 
> Yes, that would be great. I do wonder though if it wouldn't make sense
> to turn it the other way around. It creates a fair share of boilerplate
> for a number of drivers. Can't we keep Clk the way it is as a
> lower-level type, and crate a ManagedClk (or whatever name you prefer)
> that drivers can use, and would be returned by higher-level helpers, if
> they so choose?
> 
> That way, we do have the typestate API for whoever wants to, without
> creating too much boilerplate for everybody else.

I think that if you still want an API where you just call enable/disable
directly on it with no protection against unbalanced calls, then that
should be the special API. Probably called RawClk and functions marked
unsafe. Unbalanced calls seem really dangerous and use should not be
encouraged.

The current type-state based API is the least-boilerplate option for
drivers that exist today.

Alice

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>>>> For example, it's quite typical to have (at least) one clock for the bus
>>>> interface that drives the register, and one that drives the main
>>>> component logic. The former needs to be enabled only when you're
>>>> accessing the registers (and can be abstracted with
>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>>> only when the device actually starts operating.
>>>> 
>>>> You have a similar thing for the prepare vs enable thing. The difference
>>>> between the two is that enable can be called into atomic context but
>>>> prepare can't.
>>>> 
>>>> So for drivers that would care about this, you would create your device
>>>> with an unprepared clock, and then at various times during the driver
>>>> lifetime, you would mutate that state.
> 
> The case where you're doing it only while accessing registers is
> interesting, because that means the Enable bit may be owned by a local
> variable. We may imagine an:
> 
>    let enabled = self.prepared_clk.enable_scoped();
>    ... use registers
>    drop(enabled);


Not sure I understand. You can get a Clk<Enabled>, do what you need, and then
consume Clk<Enabled> to go back to Clk<Prepared>. I think I added this, but if
I didn’t, it’s a trivial thing to do.

> 
> Now ... this doesn't quite work with the current API - the current
> Enabled stated owns both a prepare and enable count, but the above keeps
> the prepare count in `self` and the enabled count in a local variable.
> But it could be done with a fourth state, or by a closure method:
> 
>    self.prepared_clk.with_enabled(|| {
>        ... use registers
>    });
> 
> All of this would work with an immutable variable of type Clk<Prepared>.
> 
>>>> AFAIU, encoding the state of the clock into the Clk type (and thus
>>>> forcing the structure that holds it) prevents that mutation. If not, we
>>>> should make it clearer (by expanding the doc maybe?) how such a pattern
>>>> can be supported.
>>>> 
>>>> Maxime
>>> 
>>> IIUC, your main point seems to be about mutating the state at runtime? This is
>>> possible with this code. You can just have an enum, for example:
>>> 
>>> enum MyClocks {
>>>    Unprepared(Clk<Unprepared>),
>>>    Prepared(Clk<Prepared>),
>>>    Enabled(Clk<Enabled>), 
>>> }
> 
> I believe you need an extra state if the state is not bound to the scope
> of a function:
> 
> enum MyClocks {
>    Unprepared(Clk<Unprepared>),
>    Prepared(Clk<Prepared>),
>    Enabled(Clk<Enabled>), 
>    Transitioning,
> }
> 
> since mem::replace() needs a new value before you can take ownership of
> the existing Clk value.

Right, I need to update the docs to account for this, as they imply that you
can do this with only two states.

> 
>>> In fact, I specifically wanted to ensure that this was possible when writing
>>> these patches, as it’s needed by drivers. If you want to, I can cover that in
>>> the examples, no worries.
>> 
>> Yes, that would be great. I do wonder though if it wouldn't make sense
>> to turn it the other way around. It creates a fair share of boilerplate
>> for a number of drivers. Can't we keep Clk the way it is as a
>> lower-level type, and crate a ManagedClk (or whatever name you prefer)
>> that drivers can use, and would be returned by higher-level helpers, if
>> they so choose?
>> 
>> That way, we do have the typestate API for whoever wants to, without
>> creating too much boilerplate for everybody else.
> 
> I think that if you still want an API where you just call enable/disable
> directly on it with no protection against unbalanced calls, then that
> should be the special API. Probably called RawClk and functions marked
> unsafe. Unbalanced calls seem really dangerous and use should not be
> encouraged.

I think we should discourage RawClk if at all possible. But if the consensus
is that we *really* need this easily-abused thing, I can provide a follow-up.

> 
> The current type-state based API is the least-boilerplate option for
> drivers that exist today.
> 
> Alice

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Gary Guo]

On Mon Jan 19, 2026 at 12:35 PM GMT, Alice Ryhl wrote:
> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>> > > For example, it's quite typical to have (at least) one clock for the bus
>> > > interface that drives the register, and one that drives the main
>> > > component logic. The former needs to be enabled only when you're
>> > > accessing the registers (and can be abstracted with
>> > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
>> > > only when the device actually starts operating.
>> > > 
>> > > You have a similar thing for the prepare vs enable thing. The difference
>> > > between the two is that enable can be called into atomic context but
>> > > prepare can't.
>> > > 
>> > > So for drivers that would care about this, you would create your device
>> > > with an unprepared clock, and then at various times during the driver
>> > > lifetime, you would mutate that state.
>
> The case where you're doing it only while accessing registers is
> interesting, because that means the Enable bit may be owned by a local
> variable. We may imagine an:
>
>     let enabled = self.prepared_clk.enable_scoped();
>     ... use registers
>     drop(enabled);
>
> Now ... this doesn't quite work with the current API - the current
> Enabled stated owns both a prepare and enable count, but the above keeps
> the prepare count in `self` and the enabled count in a local variable.
> But it could be done with a fourth state, or by a closure method:
>
>     self.prepared_clk.with_enabled(|| {
>         ... use registers
>     });
>
> All of this would work with an immutable variable of type Clk<Prepared>.
>
>> > > AFAIU, encoding the state of the clock into the Clk type (and thus
>> > > forcing the structure that holds it) prevents that mutation. If not, we
>> > > should make it clearer (by expanding the doc maybe?) how such a pattern
>> > > can be supported.
>> > > 
>> > > Maxime
>> > 
>> > IIUC, your main point seems to be about mutating the state at runtime? This is
>> > possible with this code. You can just have an enum, for example:
>> > 
>> > enum MyClocks {
>> >     Unprepared(Clk<Unprepared>),
>> >     Prepared(Clk<Prepared>),
>> >     Enabled(Clk<Enabled>), 
>> > }
>
> I believe you need an extra state if the state is not bound to the scope
> of a function:
>
> enum MyClocks {
>     Unprepared(Clk<Unprepared>),
>     Prepared(Clk<Prepared>),
>     Enabled(Clk<Enabled>), 
>     Transitioning,
> }
>
> since mem::replace() needs a new value before you can take ownership of
> the existing Clk value.

We can provide `replace_with` in the kernel, it's fine as we don't have
unwinding panics.

Best,
Gary

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote:
>> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote:
>> I think that if you still want an API where you just call enable/disable
>> directly on it with no protection against unbalanced calls, then that
>> should be the special API. Probably called RawClk and functions marked
>> unsafe. Unbalanced calls seem really dangerous and use should not be
>> encouraged.

+1; and unless there is a use-case that requires otherwise, it should not even
be possible to do this at all -- at least for driver code.

> I think we should discourage RawClk if at all possible. But if the consensus
> is that we *really* need this easily-abused thing, I can provide a follow-up.

I think we should only do this if there are use-case with no alternative, so far
there haven't been any AFAIK.

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote:
> On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote:
> >> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote:
> >> I think that if you still want an API where you just call enable/disable
> >> directly on it with no protection against unbalanced calls, then that
> >> should be the special API. Probably called RawClk and functions marked
> >> unsafe. Unbalanced calls seem really dangerous and use should not be
> >> encouraged.
> 
> +1; and unless there is a use-case that requires otherwise, it should not even
> be possible to do this at all -- at least for driver code.

I mean, it's great, it's safe, etc. but it's also suboptimal from a PM
perspective on many platforms. It's totally fine to provide nice, safe,
ergonomic wrappers for the drivers that don't care (or can't, really),
but treating a legitimate optimisation as something we should consider
impossible to do is just weird to me.

> > I think we should discourage RawClk if at all possible. But if the consensus
> > is that we *really* need this easily-abused thing, I can provide a follow-up.
> 
> I think we should only do this if there are use-case with no alternative, so far
> there haven't been any AFAIK.

I don't really care about which alternative we come up with, but look at
devm_regmap_init_mmio_clk for example. It is a valid use-case that
already exists today, and has had for more than a decade at this point.

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Gary Guo]

On Wed Jan 7, 2026 at 3:09 PM GMT, Daniel Almeida wrote:
> The current Clk abstraction can still be improved on the following issues:
>
> a) It only keeps track of a count to clk_get(), which means that users have
> to manually call disable() and unprepare(), or a variation of those, like
> disable_unprepare().
>
> b) It allows repeated calls to prepare() or enable(), but it keeps no track
> of how often these were called, i.e., it's currently legal to write the
> following:
>
> clk.prepare();
> clk.prepare();
> clk.enable();
> clk.enable();
>
> And nothing gets undone on drop().
>
> c) It adds a OptionalClk type that is probably not needed. There is no
> "struct optional_clk" in C and we should probably not add one.
>
> d) It does not let a user express the state of the clk through the
> type system. For example, there is currently no way to encode that a Clk is
> enabled via the type system alone.
>
> In light of the Regulator abstraction that was recently merged, switch this
> abstraction to use the type-state pattern instead. It solves both a) and b)
> by establishing a number of states and the valid ways to transition between
> them. It also automatically undoes any call to clk_get(), clk_prepare() and
> clk_enable() as applicable on drop(), so users do not have to do anything
> special before Clk goes out of scope.
>
> It solves c) by removing the OptionalClk type, which is now simply encoded
> as a Clk whose inner pointer is NULL.
>
> It solves d) by directly encoding the state of the Clk into the type, e.g.:
> Clk<Enabled> is now known to be a Clk that is enabled.
>
> The INVARIANTS section for Clk is expanded to highlight the relationship
> between the states and the respective reference counts that are owned by
> each of them.
>
> The examples are expanded to highlight how a user can transition between
> states, as well as highlight some of the shortcuts built into the API.
>
> The current implementation is also more flexible, in the sense that it
> allows for more states to be added in the future. This lets us implement
> different strategies for handling clocks, including one that mimics the
> current API, allowing for multiple calls to prepare() and enable().
>
> The users (cpufreq.rs/ rcpufreq_dt.rs) were updated by this patch (and not
> a separate one) to reflect the new changes. This is needed, because
> otherwise this patch would break the build.
>
> Link: https://crates.io/crates/sealed [1]
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  drivers/cpufreq/rcpufreq_dt.rs |   2 +-
>  drivers/gpu/drm/tyr/driver.rs  |  31 +---
>  drivers/pwm/pwm_th1520.rs      |  17 +-
>  rust/kernel/clk.rs             | 399 +++++++++++++++++++++++++++--------------
>  rust/kernel/cpufreq.rs         |   8 +-
>  5 files changed, 281 insertions(+), 176 deletions(-)
>
> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
> index 31e07f0279db..f1bd7d71ed54 100644
> --- a/drivers/cpufreq/rcpufreq_dt.rs
> +++ b/drivers/cpufreq/rcpufreq_dt.rs
> @@ -41,7 +41,7 @@ struct CPUFreqDTDevice {
>      freq_table: opp::FreqTable,
>      _mask: CpumaskVar,
>      _token: Option<opp::ConfigToken>,
> -    _clk: Clk,
> +    _clk: Clk<kernel::clk::Unprepared>,
>  }
>  
>  #[derive(Default)]
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 09711fb7fe0b..5692def25621 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -2,7 +2,7 @@
>  
>  use kernel::c_str;
>  use kernel::clk::Clk;
> -use kernel::clk::OptionalClk;
> +use kernel::clk::Enabled;
>  use kernel::device::Bound;
>  use kernel::device::Core;
>  use kernel::device::Device;
> @@ -37,7 +37,7 @@ pub(crate) struct TyrDriver {
>      device: ARef<TyrDevice>,
>  }
>  
> -#[pin_data(PinnedDrop)]
> +#[pin_data]
>  pub(crate) struct TyrData {
>      pub(crate) pdev: ARef<platform::Device>,
>  
> @@ -92,13 +92,9 @@ fn probe(
>          pdev: &platform::Device<Core>,
>          _info: Option<&Self::IdInfo>,
>      ) -> impl PinInit<Self, Error> {
> -        let core_clk = Clk::get(pdev.as_ref(), Some(c_str!("core")))?;
> -        let stacks_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("stacks")))?;
> -        let coregroup_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("coregroup")))?;
> -
> -        core_clk.prepare_enable()?;
> -        stacks_clk.prepare_enable()?;
> -        coregroup_clk.prepare_enable()?;
> +        let core_clk = Clk::<Enabled>::get(pdev.as_ref(), Some(c_str!("core")))?;

Ah, more turbofish.. I'd really want to avoid them if possible.

Any disadvantage on just ask the user to chain `.get().prepare_enable()?`? This
way it is also clear that some action is performed.

Alternatively, I think function names that mimick C API is also fine, e.g.
`Clk::get_enabled`.

> +        let stacks_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c_str!("stacks")))?;
> +        let coregroup_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c_str!("coregroup")))?;
>  
>          let mali_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("mali"))?;
>          let sram_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("sram"))?;
> @@ -145,17 +141,6 @@ impl PinnedDrop for TyrDriver {
>      fn drop(self: Pin<&mut Self>) {}
>  }
>  
> -#[pinned_drop]
> -impl PinnedDrop for TyrData {
> -    fn drop(self: Pin<&mut Self>) {
> -        // TODO: the type-state pattern for Clks will fix this.
> -        let clks = self.clks.lock();
> -        clks.core.disable_unprepare();
> -        clks.stacks.disable_unprepare();
> -        clks.coregroup.disable_unprepare();
> -    }
> -}
> -
>  // We need to retain the name "panthor" to achieve drop-in compatibility with
>  // the C driver in the userspace stack.
>  const INFO: drm::DriverInfo = drm::DriverInfo {
> @@ -181,9 +166,9 @@ impl drm::Driver for TyrDriver {
>  
>  #[pin_data]
>  struct Clocks {
> -    core: Clk,
> -    stacks: OptionalClk,
> -    coregroup: OptionalClk,
> +    core: Clk<Enabled>,
> +    stacks: Clk<Enabled>,
> +    coregroup: Clk<Enabled>,
>  }
>  
>  #[pin_data]
> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> index 043dc4dbc623..f4d03b988533 100644
> --- a/drivers/pwm/pwm_th1520.rs
> +++ b/drivers/pwm/pwm_th1520.rs
> @@ -23,7 +23,7 @@
>  use core::ops::Deref;
>  use kernel::{
>      c_str,
> -    clk::Clk,
> +    clk::{Clk, Enabled},
>      device::{Bound, Core, Device},
>      devres,
>      io::mem::IoMem,
> @@ -90,11 +90,11 @@ struct Th1520WfHw {
>  }
>  
>  /// The driver's private data struct. It holds all necessary devres managed resources.
> -#[pin_data(PinnedDrop)]
> +#[pin_data]
>  struct Th1520PwmDriverData {
>      #[pin]
>      iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
> -    clk: Clk,
> +    clk: Clk<Enabled>,
>  }
>  
>  impl pwm::PwmOps for Th1520PwmDriverData {
> @@ -299,13 +299,6 @@ fn write_waveform(
>      }
>  }
>  
> -#[pinned_drop]
> -impl PinnedDrop for Th1520PwmDriverData {
> -    fn drop(self: Pin<&mut Self>) {
> -        self.clk.disable_unprepare();
> -    }
> -}
> -
>  struct Th1520PwmPlatformDriver;
>  
>  kernel::of_device_table!(
> @@ -326,9 +319,7 @@ fn probe(
>          let dev = pdev.as_ref();
>          let request = pdev.io_request_by_index(0).ok_or(ENODEV)?;
>  
> -        let clk = Clk::get(dev, None)?;
> -
> -        clk.prepare_enable()?;
> +        let clk = Clk::<Enabled>::get(dev, None)?;
>  
>          // TODO: Get exclusive ownership of the clock to prevent rate changes.
>          // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available.
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index d192fbd97861..6323b40dc7ba 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs
> @@ -80,17 +80,78 @@ fn from(freq: Hertz) -> Self {
>  mod common_clk {
>      use super::Hertz;
>      use crate::{
> -        device::Device,
> +        device::{Bound, Device},
>          error::{from_err_ptr, to_result, Result},
>          prelude::*,
>      };
>  
> -    use core::{ops::Deref, ptr};
> +    use core::{marker::PhantomData, mem::ManuallyDrop, ptr};
> +
> +    mod private {
> +        pub trait Sealed {}
> +
> +        impl Sealed for super::Unprepared {}
> +        impl Sealed for super::Prepared {}
> +        impl Sealed for super::Enabled {}
> +    }

I guess it's time for me to work on a `#[sealed]` macro...

> +
> +    /// A trait representing the different states that a [`Clk`] can be in.
> +    pub trait ClkState: private::Sealed {
> +        /// Whether the clock should be disabled when dropped.
> +        const DISABLE_ON_DROP: bool;
> +
> +        /// Whether the clock should be unprepared when dropped.
> +        const UNPREPARE_ON_DROP: bool;
> +    }
> +
> +    /// A state where the [`Clk`] is not prepared and not enabled.

Do we want to make it explicit that it's "not known to be prepared or
enabled"?

> +    pub struct Unprepared;
> +
> +    /// A state where the [`Clk`] is prepared but not enabled.
> +    pub struct Prepared;
> +
> +    /// A state where the [`Clk`] is both prepared and enabled.
> +    pub struct Enabled;
> +
> +    impl ClkState for Unprepared {
> +        const DISABLE_ON_DROP: bool = false;
> +        const UNPREPARE_ON_DROP: bool = false;
> +    }
> +
> +    impl ClkState for Prepared {
> +        const DISABLE_ON_DROP: bool = false;
> +        const UNPREPARE_ON_DROP: bool = true;
> +    }
> +
> +    impl ClkState for Enabled {
> +        const DISABLE_ON_DROP: bool = true;
> +        const UNPREPARE_ON_DROP: bool = true;
> +    }
> +
> +    /// An error that can occur when trying to convert a [`Clk`] between states.
> +    pub struct Error<State: ClkState> {
> +        /// The error that occurred.
> +        pub error: kernel::error::Error,
> +
> +        /// The [`Clk`] that caused the error, so that the operation may be
> +        /// retried.
> +        pub clk: Clk<State>,
> +    }

I wonder if it makes sense to add a general `ErrorWith` type for errors that
carries error code + data.

Best,
Gary

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Gary Guo]

On Mon Jan 19, 2026 at 10:45 AM GMT, Maxime Ripard wrote:
> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>> Hi Maxime :)
>> 
>> > 
>> > I don't know the typestate pattern that well, but I wonder if we don't
>> > paint ourselves into a corner by introducing it.
>> > 
>> > While it's pretty common to get your clock from the get go into a state,
>> > and then don't modify it (like what devm_clk_get_enabled provides for
>> > example), and the typestate pattern indeed works great for those, we
>> 
>> Minor correction, devm_clk_get_enabled is not handled by the typestate
>> pattern. The next patch does include this function for convenience, but
>> you get a Result<()>. The typestate pattern is used when you want more
>> control.
>>
>> > also have a significant number of drivers that will have a finer-grained
>> > control over the clock enablement for PM.
>> > 
>> > For example, it's quite typical to have (at least) one clock for the bus
>> > interface that drives the register, and one that drives the main
>> > component logic. The former needs to be enabled only when you're
>> > accessing the registers (and can be abstracted with
>> > regmap_mmio_attach_clk for example), and the latter needs to be enabled
>> > only when the device actually starts operating.
>> > 
>> > You have a similar thing for the prepare vs enable thing. The difference
>> > between the two is that enable can be called into atomic context but
>> > prepare can't.
>> > 
>> > So for drivers that would care about this, you would create your device
>> > with an unprepared clock, and then at various times during the driver
>> > lifetime, you would mutate that state.
>> > 
>> > AFAIU, encoding the state of the clock into the Clk type (and thus
>> > forcing the structure that holds it) prevents that mutation. If not, we
>> > should make it clearer (by expanding the doc maybe?) how such a pattern
>> > can be supported.
>> > 
>> > Maxime
>> 
>> IIUC, your main point seems to be about mutating the state at runtime? This is
>> possible with this code. You can just have an enum, for example:
>> 
>> enum MyClocks {
>> 	Unprepared(Clk<Unprepared>),
>>         Prepared(Clk<Prepared>),
>> 	Enabled(Clk<Enabled>), 
>> }
>> 
>> In fact, I specifically wanted to ensure that this was possible when writing
>> these patches, as it’s needed by drivers. If you want to, I can cover that in
>> the examples, no worries.
>
> Yes, that would be great. I do wonder though if it wouldn't make sense
> to turn it the other way around. It creates a fair share of boilerplate
> for a number of drivers. Can't we keep Clk the way it is as a
> lower-level type, and crate a ManagedClk (or whatever name you prefer)
> that drivers can use, and would be returned by higher-level helpers, if
> they so choose?
>
> That way, we do have the typestate API for whoever wants to, without
> creating too much boilerplate for everybody else.

One solution is to have a new typestate `Dynamic` which opts to track things
using variables.

struct Dynamic {
    enabled: bool,
    prepared: bool,
}

trait ClkState {
    // Change to methods
    fn disable_on_drop(&self) -> bool;
}

struct Clk<State> {
    ...
    // Keep an instance, which is zero-sized for everything except `Dynamic`
    state: State,
}

this way we can have runtime-checked state conversions.

Best,
Gary

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Mon, Jan 19, 2026 at 12:35:21PM +0000, Alice Ryhl wrote:
> > > In fact, I specifically wanted to ensure that this was possible when writing
> > > these patches, as it’s needed by drivers. If you want to, I can cover that in
> > > the examples, no worries.
> > 
> > Yes, that would be great. I do wonder though if it wouldn't make sense
> > to turn it the other way around. It creates a fair share of boilerplate
> > for a number of drivers. Can't we keep Clk the way it is as a
> > lower-level type, and crate a ManagedClk (or whatever name you prefer)
> > that drivers can use, and would be returned by higher-level helpers, if
> > they so choose?
> > 
> > That way, we do have the typestate API for whoever wants to, without
> > creating too much boilerplate for everybody else.
> 
> I think that if you still want an API where you just call enable/disable
> directly on it with no protection against unbalanced calls, then that
> should be the special API. Probably called RawClk and functions marked
> unsafe. Unbalanced calls seem really dangerous and use should not be
> encouraged.

Sure, that makes sense to me.

> The current type-state based API is the least-boilerplate option for
> drivers that exist today.

I wasn't saying that we should add boilerplate to the typestate API
either. I was saying that the non-typestated API was common enough that
we probably didn't want boilerplate for it either if possible.

Maxime

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

Re: [PATCH v3 2/3] rust: clk: add devres-managed clks

[Gary Guo]

On Wed Jan 7, 2026 at 3:09 PM GMT, Daniel Almeida wrote:
> The clk API allows fine-grained control, but some drivers might be
> more interested in a "set and forget" API.
>
> Expand the current API to support this. The clock will automatically be
> disabled, unprepared and freed when the device is unbound from the bus
> without further intervention by the driver.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/kernel/clk.rs | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index 6323b40dc7ba..e840e7c20af7 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs
> @@ -95,6 +95,49 @@ impl Sealed for super::Prepared {}
>          impl Sealed for super::Enabled {}
>      }
>  
> +    /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
> +    ///
> +    /// [`devres`]: crate::devres::Devres
> +    pub fn devm_enable(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
> +        let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
> +
> +        // SAFETY: It is safe to call [`devm_clk_get_enabled`] with a valid
> +        // device pointer.
> +        from_err_ptr(unsafe { bindings::devm_clk_get_enabled(dev.as_raw(), name) })?;
> +        Ok(())
> +    }

Would it make sense to have them as assoc functions instead of standalone?

Regardless, the code looks correct to me.

Reviewed-by: Gary Guo <gary@garyguo.net>

Best,
Gary

> +
> +    /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
> +    ///
> +    /// This does not print any error messages if the clock is not found.
> +    ///
> +    /// [`devres`]: crate::devres::Devres
> +    pub fn devm_enable_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
> +        let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
> +
> +        // SAFETY: It is safe to call [`devm_clk_get_optional_enabled`] with a
> +        // valid device pointer.
> +        from_err_ptr(unsafe { bindings::devm_clk_get_optional_enabled(dev.as_raw(), name) })?;
> +        Ok(())
> +    }
> +
> +    /// Same as [`devm_enable_optional`], but also sets the rate.
> +    pub fn devm_enable_optional_with_rate(
> +        dev: &Device,
> +        name: Option<&CStr>,
> +        rate: Hertz,
> +    ) -> Result {
> +        let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
> +
> +        // SAFETY: It is safe to call
> +        // [`devm_clk_get_optional_enabled_with_rate`] with a valid device
> +        // pointer.
> +        from_err_ptr(unsafe {
> +            bindings::devm_clk_get_optional_enabled_with_rate(dev.as_raw(), name, rate.as_hz())
> +        })?;
> +        Ok(())
> +    }
> +
>      /// A trait representing the different states that a [`Clk`] can be in.
>      pub trait ClkState: private::Sealed {
>          /// Whether the clock should be disabled when dropped.

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Mon Jan 19, 2026 at 3:18 PM CET, Maxime Ripard wrote:
> On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote:
>> On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote:
>> >> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote:
>> >> I think that if you still want an API where you just call enable/disable
>> >> directly on it with no protection against unbalanced calls, then that
>> >> should be the special API. Probably called RawClk and functions marked
>> >> unsafe. Unbalanced calls seem really dangerous and use should not be
>> >> encouraged.
>> 
>> +1; and unless there is a use-case that requires otherwise, it should not even
>> be possible to do this at all -- at least for driver code.
>
> I mean, it's great, it's safe, etc. but it's also suboptimal from a PM
> perspective on many platforms. It's totally fine to provide nice, safe,
> ergonomic wrappers for the drivers that don't care (or can't, really),
> but treating a legitimate optimisation as something we should consider
> impossible to do is just weird to me.

I said that an unsafe API with potentially unbalanced calls is something we
should clearly avoid for drivers. This is *not* equivalent to "treating a
legitimate optimisation as something we should consider impossible".

If we discover use-cases where the current API doesn't work well, we can
invenstigate further.

>> > I think we should discourage RawClk if at all possible. But if the consensus
>> > is that we *really* need this easily-abused thing, I can provide a follow-up.
>> 
>> I think we should only do this if there are use-case with no alternative, so far
>> there haven't been any AFAIK.
>
> I don't really care about which alternative we come up with, but look at
> devm_regmap_init_mmio_clk for example. It is a valid use-case that
> already exists today, and has had for more than a decade at this point.

I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference
count of the clock and prepares it when called and unprepares the clk in drops
its reference in regmap_mmio_free_context() called from the devres callback.

That something we can easily do with the current API, no?

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Miguel Ojeda]

On Mon, Jan 19, 2026 at 3:20 PM Gary Guo <gary@garyguo.net> wrote:
>
> I guess it's time for me to work on a `#[sealed]` macro...

...and perhaps pinging upstream for the language feature... :) I added
https://github.com/rust-lang/rust/issues/105077 to our usual list of
wishes at https://github.com/Rust-for-Linux/linux/issues/354.

Do you think using the linked crate in the commit message would make
sense? It seems to be a couple pages of code, using `syn` 2.0, which
should be fine now.

Another option is a "good first issue" of medium difficulty if you wish.

> I wonder if it makes sense to add a general `ErrorWith` type for errors that
> carries error code + data.

Sounds reasonable...

Cheers,
Miguel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Gary Guo]

On Mon Jan 19, 2026 at 3:22 PM GMT, Miguel Ojeda wrote:
> On Mon, Jan 19, 2026 at 3:20 PM Gary Guo <gary@garyguo.net> wrote:
>>
>> I guess it's time for me to work on a `#[sealed]` macro...
>
> ...and perhaps pinging upstream for the language feature... :) I added
> https://github.com/rust-lang/rust/issues/105077 to our usual list of
> wishes at https://github.com/Rust-for-Linux/linux/issues/354.
>
> Do you think using the linked crate in the commit message would make
> sense? It seems to be a couple pages of code, using `syn` 2.0, which
> should be fine now.

Which crate are you talking about? I can't find a linked crate in the issue.

Best,
Gary

>
> Another option is a "good first issue" of medium difficulty if you wish.
>
>> I wonder if it makes sense to add a general `ErrorWith` type for errors that
>> carries error code + data.
>
> Sounds reasonable...
>
> Cheers,
> Miguel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

>> Yes, that would be great. I do wonder though if it wouldn't make sense
>> to turn it the other way around. It creates a fair share of boilerplate
>> for a number of drivers. Can't we keep Clk the way it is as a
>> lower-level type, and crate a ManagedClk (or whatever name you prefer)
>> that drivers can use, and would be returned by higher-level helpers, if
>> they so choose?
>> 
>> That way, we do have the typestate API for whoever wants to, without
>> creating too much boilerplate for everybody else.
> 
> One solution is to have a new typestate `Dynamic` which opts to track things
> using variables.
> 
> struct Dynamic {
>    enabled: bool,
>    prepared: bool,
> }
> 
> trait ClkState {
>    // Change to methods
>    fn disable_on_drop(&self) -> bool;
> }
> 
> struct Clk<State> {
>    ...
>    // Keep an instance, which is zero-sized for everything except `Dynamic`
>    state: State,
> }
> 
> this way we can have runtime-checked state conversions.
> 
> Best,
> Gary

There used to be a Dynamic state in the past in a similar setting. That was removed after some thorough discussion. I’d say we should refrain from going back to this. Specially considering that the current design works fine.

 I can remove the turbofish if you want, even though I think they’re useful so that we have the same API for all states.

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Miguel Ojeda]

On Mon, Jan 19, 2026 at 4:36 PM Gary Guo <gary@garyguo.net> wrote:
>
> Which crate are you talking about? I can't find a linked crate in the issue.

The commit message (i.e. not the issue) has an (unused) link with the
`sealed` crate:

Link: https://crates.io/crates/sealed [1]

Cheers,
Miguel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Gary Guo]

On Mon Jan 19, 2026 at 3:46 PM GMT, Miguel Ojeda wrote:
> On Mon, Jan 19, 2026 at 4:36 PM Gary Guo <gary@garyguo.net> wrote:
>>
>> Which crate are you talking about? I can't find a linked crate in the issue.
>
> The commit message (i.e. not the issue) has an (unused) link with the
> `sealed` crate:
>
> Link: https://crates.io/crates/sealed [1]

Yeah, some thing similar. Probably an even simpler version with only things that
we need.

Best,
Gary

>
> Cheers,
> Miguel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Mon, Jan 19, 2026 at 03:37:17PM +0100, Danilo Krummrich wrote:
> On Mon Jan 19, 2026 at 3:18 PM CET, Maxime Ripard wrote:
> > On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote:
> >> On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote:
> >> >> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote:
> >> >> I think that if you still want an API where you just call enable/disable
> >> >> directly on it with no protection against unbalanced calls, then that
> >> >> should be the special API. Probably called RawClk and functions marked
> >> >> unsafe. Unbalanced calls seem really dangerous and use should not be
> >> >> encouraged.
> >> 
> >> +1; and unless there is a use-case that requires otherwise, it should not even
> >> be possible to do this at all -- at least for driver code.
> >
> > I mean, it's great, it's safe, etc. but it's also suboptimal from a PM
> > perspective on many platforms. It's totally fine to provide nice, safe,
> > ergonomic wrappers for the drivers that don't care (or can't, really),
> > but treating a legitimate optimisation as something we should consider
> > impossible to do is just weird to me.
> 
> I said that an unsafe API with potentially unbalanced calls is something we
> should clearly avoid for drivers. This is *not* equivalent to "treating a
> legitimate optimisation as something we should consider impossible".
> 
> If we discover use-cases where the current API doesn't work well, we can
> invenstigate further.

I'm not sure I'm following what you're saying, sorry. I've pointed out
such a use-case already.

> >> > I think we should discourage RawClk if at all possible. But if the consensus
> >> > is that we *really* need this easily-abused thing, I can provide a follow-up.
> >> 
> >> I think we should only do this if there are use-case with no alternative, so far
> >> there haven't been any AFAIK.
> >
> > I don't really care about which alternative we come up with, but look at
> > devm_regmap_init_mmio_clk for example. It is a valid use-case that
> > already exists today, and has had for more than a decade at this point.
> 
> I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference
> count of the clock and prepares it when called and unprepares the clk in drops
> its reference in regmap_mmio_free_context() called from the devres callback.
> 
> That something we can easily do with the current API, no?

The current one, yes. Doing that in the API suggested here would involve
some boilerplate in all those drivers they don't have right now.

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

> On 22 Jan 2026, at 10:44, Maxime Ripard <mripard@kernel.org> wrote:
> 
> On Mon, Jan 19, 2026 at 03:37:17PM +0100, Danilo Krummrich wrote:
>> On Mon Jan 19, 2026 at 3:18 PM CET, Maxime Ripard wrote:
>>> On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote:
>>>> On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote:
>>>>>> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote:
>>>>>> I think that if you still want an API where you just call enable/disable
>>>>>> directly on it with no protection against unbalanced calls, then that
>>>>>> should be the special API. Probably called RawClk and functions marked
>>>>>> unsafe. Unbalanced calls seem really dangerous and use should not be
>>>>>> encouraged.
>>>> 
>>>> +1; and unless there is a use-case that requires otherwise, it should not even
>>>> be possible to do this at all -- at least for driver code.
>>> 
>>> I mean, it's great, it's safe, etc. but it's also suboptimal from a PM
>>> perspective on many platforms. It's totally fine to provide nice, safe,
>>> ergonomic wrappers for the drivers that don't care (or can't, really),
>>> but treating a legitimate optimisation as something we should consider
>>> impossible to do is just weird to me.
>> 
>> I said that an unsafe API with potentially unbalanced calls is something we
>> should clearly avoid for drivers. This is *not* equivalent to "treating a
>> legitimate optimisation as something we should consider impossible".
>> 
>> If we discover use-cases where the current API doesn't work well, we can
>> invenstigate further.
> 
> I'm not sure I'm following what you're saying, sorry. I've pointed out
> such a use-case already.
> 
>>>>> I think we should discourage RawClk if at all possible. But if the consensus
>>>>> is that we *really* need this easily-abused thing, I can provide a follow-up.
>>>> 
>>>> I think we should only do this if there are use-case with no alternative, so far
>>>> there haven't been any AFAIK.
>>> 
>>> I don't really care about which alternative we come up with, but look at
>>> devm_regmap_init_mmio_clk for example. It is a valid use-case that
>>> already exists today, and has had for more than a decade at this point.
>> 
>> I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference
>> count of the clock and prepares it when called and unprepares the clk in drops
>> its reference in regmap_mmio_free_context() called from the devres callback.
>> 
>> That something we can easily do with the current API, no?
> 
> The current one, yes. Doing that in the API suggested here would involve
> some boilerplate in all those drivers they don't have right now.
> 
> Maxime

Maxime, I know you’ve already pointed out a use-case, but I think the
confusion stems from why you seem to think that the current solution cannot
cater to the API you mentioned in a clean way. You seem to imply that there
will be a lot of boilerplate involved, but we (or I) cannot see this. Perhaps
it would help if you highlighted how exactly the type state solution would be
verbose using some pseudocode. I guess that would make your point clearer for
us.

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Thu Jan 22, 2026 at 2:44 PM CET, Maxime Ripard wrote:
> On Mon, Jan 19, 2026 at 03:37:17PM +0100, Danilo Krummrich wrote:
>> I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference
>> count of the clock and prepares it when called and unprepares the clk in drops
>> its reference in regmap_mmio_free_context() called from the devres callback.
>> 
>> That something we can easily do with the current API, no?
>
> The current one, yes. Doing that in the API suggested here would involve
> some boilerplate in all those drivers they don't have right now.

No, I did mean the API suggested here.

If you would implement something like devm_regmap_init_mmio_clk() in Rust with
this API, you'd have some object like

	struct RegmapResource<T: Backend, R: Resource> {
	    map: Regmap<T>,
	    res: R,
	}

and a concrete instance could have the following type

	RegmapResource<MmIo, Clk<Prepared>>

So, eventually you could have:

	fn devm_regmap_init_mmio_clk(dev: &Device<Bound>,  name: &CStr, ...) -> ... {
	    let clk: Clk<Prepared> = Clk::get(dev, name)?;
	    let regmap = RegmapResource::new(..., clk);

	    Devres::new(dev, regmap)
	}

Of course, we would never design the API in this way, as we have generic I/O
backends and register abstractions, and we'd also not have
devm_regmap_init_mmio_clk() as a constructor for a RegmapResource type, but you
get the idea.

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Mon, 19 Jan 2026 14:20:43 +0000
"Gary Guo" <gary@garyguo.net> wrote:

> On Wed Jan 7, 2026 at 3:09 PM GMT, Daniel Almeida wrote:
> > The current Clk abstraction can still be improved on the following issues:
> >
> > a) It only keeps track of a count to clk_get(), which means that users have
> > to manually call disable() and unprepare(), or a variation of those, like
> > disable_unprepare().
> >
> > b) It allows repeated calls to prepare() or enable(), but it keeps no track
> > of how often these were called, i.e., it's currently legal to write the
> > following:
> >
> > clk.prepare();
> > clk.prepare();
> > clk.enable();
> > clk.enable();
> >
> > And nothing gets undone on drop().
> >
> > c) It adds a OptionalClk type that is probably not needed. There is no
> > "struct optional_clk" in C and we should probably not add one.
> >
> > d) It does not let a user express the state of the clk through the
> > type system. For example, there is currently no way to encode that a Clk is
> > enabled via the type system alone.
> >
> > In light of the Regulator abstraction that was recently merged, switch this
> > abstraction to use the type-state pattern instead. It solves both a) and b)
> > by establishing a number of states and the valid ways to transition between
> > them. It also automatically undoes any call to clk_get(), clk_prepare() and
> > clk_enable() as applicable on drop(), so users do not have to do anything
> > special before Clk goes out of scope.
> >
> > It solves c) by removing the OptionalClk type, which is now simply encoded
> > as a Clk whose inner pointer is NULL.
> >
> > It solves d) by directly encoding the state of the Clk into the type, e.g.:
> > Clk<Enabled> is now known to be a Clk that is enabled.
> >
> > The INVARIANTS section for Clk is expanded to highlight the relationship
> > between the states and the respective reference counts that are owned by
> > each of them.
> >
> > The examples are expanded to highlight how a user can transition between
> > states, as well as highlight some of the shortcuts built into the API.
> >
> > The current implementation is also more flexible, in the sense that it
> > allows for more states to be added in the future. This lets us implement
> > different strategies for handling clocks, including one that mimics the
> > current API, allowing for multiple calls to prepare() and enable().
> >
> > The users (cpufreq.rs/ rcpufreq_dt.rs) were updated by this patch (and not
> > a separate one) to reflect the new changes. This is needed, because
> > otherwise this patch would break the build.
> >
> > Link: https://crates.io/crates/sealed [1]
> > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> > ---
> >  drivers/cpufreq/rcpufreq_dt.rs |   2 +-
> >  drivers/gpu/drm/tyr/driver.rs  |  31 +---
> >  drivers/pwm/pwm_th1520.rs      |  17 +-
> >  rust/kernel/clk.rs             | 399 +++++++++++++++++++++++++++--------------
> >  rust/kernel/cpufreq.rs         |   8 +-
> >  5 files changed, 281 insertions(+), 176 deletions(-)
> >
> > diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
> > index 31e07f0279db..f1bd7d71ed54 100644
> > --- a/drivers/cpufreq/rcpufreq_dt.rs
> > +++ b/drivers/cpufreq/rcpufreq_dt.rs
> > @@ -41,7 +41,7 @@ struct CPUFreqDTDevice {
> >      freq_table: opp::FreqTable,
> >      _mask: CpumaskVar,
> >      _token: Option<opp::ConfigToken>,
> > -    _clk: Clk,
> > +    _clk: Clk<kernel::clk::Unprepared>,
> >  }
> >  
> >  #[derive(Default)]
> > diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> > index 09711fb7fe0b..5692def25621 100644
> > --- a/drivers/gpu/drm/tyr/driver.rs
> > +++ b/drivers/gpu/drm/tyr/driver.rs
> > @@ -2,7 +2,7 @@
> >  
> >  use kernel::c_str;
> >  use kernel::clk::Clk;
> > -use kernel::clk::OptionalClk;
> > +use kernel::clk::Enabled;
> >  use kernel::device::Bound;
> >  use kernel::device::Core;
> >  use kernel::device::Device;
> > @@ -37,7 +37,7 @@ pub(crate) struct TyrDriver {
> >      device: ARef<TyrDevice>,
> >  }
> >  
> > -#[pin_data(PinnedDrop)]
> > +#[pin_data]
> >  pub(crate) struct TyrData {
> >      pub(crate) pdev: ARef<platform::Device>,
> >  
> > @@ -92,13 +92,9 @@ fn probe(
> >          pdev: &platform::Device<Core>,
> >          _info: Option<&Self::IdInfo>,
> >      ) -> impl PinInit<Self, Error> {
> > -        let core_clk = Clk::get(pdev.as_ref(), Some(c_str!("core")))?;
> > -        let stacks_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("stacks")))?;
> > -        let coregroup_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("coregroup")))?;
> > -
> > -        core_clk.prepare_enable()?;
> > -        stacks_clk.prepare_enable()?;
> > -        coregroup_clk.prepare_enable()?;
> > +        let core_clk = Clk::<Enabled>::get(pdev.as_ref(), Some(c_str!("core")))?;  
> 
> Ah, more turbofish.. I'd really want to avoid them if possible.
> 
> Any disadvantage on just ask the user to chain `.get().prepare_enable()?`? This
> way it is also clear that some action is performed.

I've just disc

> 
> Alternatively, I think function names that mimick C API is also fine, e.g.
> `Clk::get_enabled`.
> 
> > +        let stacks_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c_str!("stacks")))?;
> > +        let coregroup_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c_str!("coregroup")))?;
> >  
> >          let mali_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("mali"))?;
> >          let sram_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c_str!("sram"))?;
> > @@ -145,17 +141,6 @@ impl PinnedDrop for TyrDriver {
> >      fn drop(self: Pin<&mut Self>) {}
> >  }
> >  
> > -#[pinned_drop]
> > -impl PinnedDrop for TyrData {
> > -    fn drop(self: Pin<&mut Self>) {
> > -        // TODO: the type-state pattern for Clks will fix this.
> > -        let clks = self.clks.lock();
> > -        clks.core.disable_unprepare();
> > -        clks.stacks.disable_unprepare();
> > -        clks.coregroup.disable_unprepare();
> > -    }
> > -}
> > -
> >  // We need to retain the name "panthor" to achieve drop-in compatibility with
> >  // the C driver in the userspace stack.
> >  const INFO: drm::DriverInfo = drm::DriverInfo {
> > @@ -181,9 +166,9 @@ impl drm::Driver for TyrDriver {
> >  
> >  #[pin_data]
> >  struct Clocks {
> > -    core: Clk,
> > -    stacks: OptionalClk,
> > -    coregroup: OptionalClk,
> > +    core: Clk<Enabled>,
> > +    stacks: Clk<Enabled>,
> > +    coregroup: Clk<Enabled>,
> >  }
> >  
> >  #[pin_data]
> > diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs
> > index 043dc4dbc623..f4d03b988533 100644
> > --- a/drivers/pwm/pwm_th1520.rs
> > +++ b/drivers/pwm/pwm_th1520.rs
> > @@ -23,7 +23,7 @@
> >  use core::ops::Deref;
> >  use kernel::{
> >      c_str,
> > -    clk::Clk,
> > +    clk::{Clk, Enabled},
> >      device::{Bound, Core, Device},
> >      devres,
> >      io::mem::IoMem,
> > @@ -90,11 +90,11 @@ struct Th1520WfHw {
> >  }
> >  
> >  /// The driver's private data struct. It holds all necessary devres managed resources.
> > -#[pin_data(PinnedDrop)]
> > +#[pin_data]
> >  struct Th1520PwmDriverData {
> >      #[pin]
> >      iomem: devres::Devres<IoMem<TH1520_PWM_REG_SIZE>>,
> > -    clk: Clk,
> > +    clk: Clk<Enabled>,
> >  }
> >  
> >  impl pwm::PwmOps for Th1520PwmDriverData {
> > @@ -299,13 +299,6 @@ fn write_waveform(
> >      }
> >  }
> >  
> > -#[pinned_drop]
> > -impl PinnedDrop for Th1520PwmDriverData {
> > -    fn drop(self: Pin<&mut Self>) {
> > -        self.clk.disable_unprepare();
> > -    }
> > -}
> > -
> >  struct Th1520PwmPlatformDriver;
> >  
> >  kernel::of_device_table!(
> > @@ -326,9 +319,7 @@ fn probe(
> >          let dev = pdev.as_ref();
> >          let request = pdev.io_request_by_index(0).ok_or(ENODEV)?;
> >  
> > -        let clk = Clk::get(dev, None)?;
> > -
> > -        clk.prepare_enable()?;
> > +        let clk = Clk::<Enabled>::get(dev, None)?;
> >  
> >          // TODO: Get exclusive ownership of the clock to prevent rate changes.
> >          // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available.
> > diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> > index d192fbd97861..6323b40dc7ba 100644
> > --- a/rust/kernel/clk.rs
> > +++ b/rust/kernel/clk.rs
> > @@ -80,17 +80,78 @@ fn from(freq: Hertz) -> Self {
> >  mod common_clk {
> >      use super::Hertz;
> >      use crate::{
> > -        device::Device,
> > +        device::{Bound, Device},
> >          error::{from_err_ptr, to_result, Result},
> >          prelude::*,
> >      };
> >  
> > -    use core::{ops::Deref, ptr};
> > +    use core::{marker::PhantomData, mem::ManuallyDrop, ptr};
> > +
> > +    mod private {
> > +        pub trait Sealed {}
> > +
> > +        impl Sealed for super::Unprepared {}
> > +        impl Sealed for super::Prepared {}
> > +        impl Sealed for super::Enabled {}
> > +    }  
> 
> I guess it's time for me to work on a `#[sealed]` macro...
> 
> > +
> > +    /// A trait representing the different states that a [`Clk`] can be in.
> > +    pub trait ClkState: private::Sealed {
> > +        /// Whether the clock should be disabled when dropped.
> > +        const DISABLE_ON_DROP: bool;
> > +
> > +        /// Whether the clock should be unprepared when dropped.
> > +        const UNPREPARE_ON_DROP: bool;
> > +    }
> > +
> > +    /// A state where the [`Clk`] is not prepared and not enabled.  
> 
> Do we want to make it explicit that it's "not known to be prepared or
> enabled"?
> 
> > +    pub struct Unprepared;
> > +
> > +    /// A state where the [`Clk`] is prepared but not enabled.
> > +    pub struct Prepared;
> > +
> > +    /// A state where the [`Clk`] is both prepared and enabled.
> > +    pub struct Enabled;
> > +
> > +    impl ClkState for Unprepared {
> > +        const DISABLE_ON_DROP: bool = false;
> > +        const UNPREPARE_ON_DROP: bool = false;
> > +    }
> > +
> > +    impl ClkState for Prepared {
> > +        const DISABLE_ON_DROP: bool = false;
> > +        const UNPREPARE_ON_DROP: bool = true;
> > +    }
> > +
> > +    impl ClkState for Enabled {
> > +        const DISABLE_ON_DROP: bool = true;
> > +        const UNPREPARE_ON_DROP: bool = true;
> > +    }
> > +
> > +    /// An error that can occur when trying to convert a [`Clk`] between states.
> > +    pub struct Error<State: ClkState> {
> > +        /// The error that occurred.
> > +        pub error: kernel::error::Error,
> > +
> > +        /// The [`Clk`] that caused the error, so that the operation may be
> > +        /// retried.
> > +        pub clk: Clk<State>,
> > +    }  
> 
> I wonder if it makes sense to add a general `ErrorWith` type for errors that
> carries error code + data.
> 
> Best,
> Gary

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

Hello Daniel,

On Mon, 2 Feb 2026 17:10:38 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> > > -#[pin_data(PinnedDrop)]
> > > +#[pin_data]
> > >  pub(crate) struct TyrData {
> > >      pub(crate) pdev: ARef<platform::Device>,
> > >  
> > > @@ -92,13 +92,9 @@ fn probe(
> > >          pdev: &platform::Device<Core>,
> > >          _info: Option<&Self::IdInfo>,
> > >      ) -> impl PinInit<Self, Error> {
> > > -        let core_clk = Clk::get(pdev.as_ref(), Some(c_str!("core")))?;
> > > -        let stacks_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("stacks")))?;
> > > -        let coregroup_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("coregroup")))?;
> > > -
> > > -        core_clk.prepare_enable()?;
> > > -        stacks_clk.prepare_enable()?;
> > > -        coregroup_clk.prepare_enable()?;
> > > +        let core_clk = Clk::<Enabled>::get(pdev.as_ref(), Some(c_str!("core")))?;    
> > 
> > Ah, more turbofish.. I'd really want to avoid them if possible.
> > 
> > Any disadvantage on just ask the user to chain `.get().prepare_enable()?`? This
> > way it is also clear that some action is performed.  
> 
> I've just disc

Sorry, I've hit the reply button before I had finished writing my
answer. So I was about to say that I had started writing something
similar without knowing this series existed, and I feel like we'd don't
really need those prepare_enable() shortcuts that exist in C. We might
has well just go:

	Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;

and have the following variant-specofoc functions

- Clk<Unprepared>::get[_optional]() (no get on Prepared and Enabled
  variants)
- Clk<Unprepared>::prepare()
- Clk<Prepared>::{enable,unprepare}()
- Clk<Enabled>::{disable}()

Regards,

Boris

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

Hello Daniel,

On Wed, 07 Jan 2026 12:09:52 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> -        /// Disable and unprepare the clock.
> +    impl Clk<Enabled> {
> +        /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection id
> +        /// and then prepares and enables it.
>          ///
> -        /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
> +        /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
> +        /// followed by [`Clk::enable`].
>          #[inline]
> -        pub fn disable_unprepare(&self) {
> -            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> -            // [`clk_disable_unprepare`].
> -            unsafe { bindings::clk_disable_unprepare(self.as_raw()) };
> +        pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
> +            Clk::<Prepared>::get(dev, name)?
> +                .enable()
> +                .map_err(|error| error.error)
> +        }
> +
> +        /// Behaves the same as [`Self::get`], except when there is no clock
> +        /// producer. In this case, instead of returning [`ENOENT`], it returns
> +        /// a dummy [`Clk`].
> +        #[inline]
> +        pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
> +            Clk::<Prepared>::get_optional(dev, name)?
> +                .enable()
> +                .map_err(|error| error.error)
> +        }
> +
> +        /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]
> +        /// state.
> +        #[inline]
> +        pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
> +            // We will be transferring the ownership of our `clk_get()` and
> +            // `clk_enable()` counts to `Clk<Prepared>`.
> +            let clk = ManuallyDrop::new(self);
> +
> +            // SAFETY: By the type invariants, `self.0` is a valid argument for
> +            // [`clk_disable`].
> +            unsafe { bindings::clk_disable(clk.as_raw()) };
> +
> +            Ok(Clk {
> +                inner: clk.inner,
> +                _phantom: PhantomData,
> +            })
>          }
>  
>          /// Get clock's rate.

Dunno if this has been mentioned already, but I belive the rate
getter/setter should be in the generic implementation. Indeed, it's
quite common for clock users to change the rate when the clk is
disabled to avoid unstable transitional state. The usual pattern for
that is:

	- clk_set_parent(my_clk, secondary_parent)
	- clk_disable[_unprepare](primary_parent) // (usually a PLL)
	- clk_set_rate(primary_parent)
	- clk[_prepare]_enable(primary_parent)
	- clk_set_parent(my_clk, primary_parent)

The case where the clk rate is changed while the clk is active is also
valid (usually fine when it's just a divider that's changed, because
there's no stabilization period).

> @@ -252,83 +429,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
>          }
>      }

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Tue, 3 Feb 2026 10:09:13 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Hello Daniel,
> 
> On Mon, 2 Feb 2026 17:10:38 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > > > -#[pin_data(PinnedDrop)]
> > > > +#[pin_data]
> > > >  pub(crate) struct TyrData {
> > > >      pub(crate) pdev: ARef<platform::Device>,
> > > >  
> > > > @@ -92,13 +92,9 @@ fn probe(
> > > >          pdev: &platform::Device<Core>,
> > > >          _info: Option<&Self::IdInfo>,
> > > >      ) -> impl PinInit<Self, Error> {
> > > > -        let core_clk = Clk::get(pdev.as_ref(), Some(c_str!("core")))?;
> > > > -        let stacks_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("stacks")))?;
> > > > -        let coregroup_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("coregroup")))?;
> > > > -
> > > > -        core_clk.prepare_enable()?;
> > > > -        stacks_clk.prepare_enable()?;
> > > > -        coregroup_clk.prepare_enable()?;
> > > > +        let core_clk = Clk::<Enabled>::get(pdev.as_ref(), Some(c_str!("core")))?;      
> > > 
> > > Ah, more turbofish.. I'd really want to avoid them if possible.
> > > 
> > > Any disadvantage on just ask the user to chain `.get().prepare_enable()?`? This
> > > way it is also clear that some action is performed.    
> > 
> > I've just disc  
> 
> Sorry, I've hit the reply button before I had finished writing my
> answer. So I was about to say that I had started writing something
> similar without knowing this series existed, and I feel like we'd don't
> really need those prepare_enable() shortcuts that exist in C. We might
> has well just go:
> 
> 	Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;

If we want this pattern to work, we also need:

    impl<State: ClkState> From<Error<State>> for kernel::error::Error {
        fn from(e: Error<State>) -> kernel::error::Error {
            e.error
        }
    }

> 
> and have the following variant-specofoc functions
> 
> - Clk<Unprepared>::get[_optional]() (no get on Prepared and Enabled
>   variants)
> - Clk<Unprepared>::prepare()
> - Clk<Prepared>::{enable,unprepare}()
> - Clk<Enabled>::{disable}()
> 
> Regards,
> 
> Boris
> 

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Mon, 19 Jan 2026 12:35:21 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
> > On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:  
> > > > For example, it's quite typical to have (at least) one clock for the bus
> > > > interface that drives the register, and one that drives the main
> > > > component logic. The former needs to be enabled only when you're
> > > > accessing the registers (and can be abstracted with
> > > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > > > only when the device actually starts operating.
> > > > 
> > > > You have a similar thing for the prepare vs enable thing. The difference
> > > > between the two is that enable can be called into atomic context but
> > > > prepare can't.
> > > > 
> > > > So for drivers that would care about this, you would create your device
> > > > with an unprepared clock, and then at various times during the driver
> > > > lifetime, you would mutate that state.  
> 
> The case where you're doing it only while accessing registers is
> interesting, because that means the Enable bit may be owned by a local
> variable. We may imagine an:
> 
>     let enabled = self.prepared_clk.enable_scoped();
>     ... use registers
>     drop(enabled);
> 
> Now ... this doesn't quite work with the current API - the current
> Enabled stated owns both a prepare and enable count, but the above keeps
> the prepare count in `self` and the enabled count in a local variable.
> But it could be done with a fourth state, or by a closure method:
> 
>     self.prepared_clk.with_enabled(|| {
>         ... use registers
>     });
> 
> All of this would work with an immutable variable of type Clk<Prepared>.

Hm, maybe it'd make sense to implement Clone so we can have a temporary
clk variable that has its own prepare/enable refs and releases them
as it goes out of scope. This implies wrapping *mut bindings::clk in an
Arc<> because bindings::clk is not ARef, but should be relatively easy
to do. Posting the quick experiment I did with this approach, in case
you're interested [1]

[1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Tue, 3 Feb 2026 11:39:02 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Mon, 19 Jan 2026 12:35:21 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:  
> > > On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:    
> > > > > For example, it's quite typical to have (at least) one clock for the bus
> > > > > interface that drives the register, and one that drives the main
> > > > > component logic. The former needs to be enabled only when you're
> > > > > accessing the registers (and can be abstracted with
> > > > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > > > > only when the device actually starts operating.
> > > > > 
> > > > > You have a similar thing for the prepare vs enable thing. The difference
> > > > > between the two is that enable can be called into atomic context but
> > > > > prepare can't.
> > > > > 
> > > > > So for drivers that would care about this, you would create your device
> > > > > with an unprepared clock, and then at various times during the driver
> > > > > lifetime, you would mutate that state.    
> > 
> > The case where you're doing it only while accessing registers is
> > interesting, because that means the Enable bit may be owned by a local
> > variable. We may imagine an:
> > 
> >     let enabled = self.prepared_clk.enable_scoped();
> >     ... use registers
> >     drop(enabled);
> > 
> > Now ... this doesn't quite work with the current API - the current
> > Enabled stated owns both a prepare and enable count, but the above keeps
> > the prepare count in `self` and the enabled count in a local variable.
> > But it could be done with a fourth state, or by a closure method:
> > 
> >     self.prepared_clk.with_enabled(|| {
> >         ... use registers
> >     });
> > 
> > All of this would work with an immutable variable of type Clk<Prepared>.  
> 
> Hm, maybe it'd make sense to implement Clone so we can have a temporary
> clk variable that has its own prepare/enable refs and releases them
> as it goes out of scope. This implies wrapping *mut bindings::clk in an
> Arc<> because bindings::clk is not ARef, but should be relatively easy
> to do. Posting the quick experiment I did with this approach, in case
> you're interested [1]

This time with a proper RawClk(*mut bindings::clk) wrapper, so we can
clk_put() called in RawClk::drop() instead of in Clk::drop().

[1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/6fa6cb72f14373b276c61d038bc2b16f49c78f74

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

Hi Boris,

> On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> On Mon, 19 Jan 2026 12:35:21 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
>> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:  
>>>>> For example, it's quite typical to have (at least) one clock for the bus
>>>>> interface that drives the register, and one that drives the main
>>>>> component logic. The former needs to be enabled only when you're
>>>>> accessing the registers (and can be abstracted with
>>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>>>> only when the device actually starts operating.
>>>>> 
>>>>> You have a similar thing for the prepare vs enable thing. The difference
>>>>> between the two is that enable can be called into atomic context but
>>>>> prepare can't.
>>>>> 
>>>>> So for drivers that would care about this, you would create your device
>>>>> with an unprepared clock, and then at various times during the driver
>>>>> lifetime, you would mutate that state.  
>> 
>> The case where you're doing it only while accessing registers is
>> interesting, because that means the Enable bit may be owned by a local
>> variable. We may imagine an:
>> 
>>    let enabled = self.prepared_clk.enable_scoped();
>>    ... use registers
>>    drop(enabled);
>> 
>> Now ... this doesn't quite work with the current API - the current
>> Enabled stated owns both a prepare and enable count, but the above keeps
>> the prepare count in `self` and the enabled count in a local variable.
>> But it could be done with a fourth state, or by a closure method:
>> 
>>    self.prepared_clk.with_enabled(|| {
>>        ... use registers
>>    });
>> 
>> All of this would work with an immutable variable of type Clk<Prepared>.
> 
> Hm, maybe it'd make sense to implement Clone so we can have a temporary
> clk variable that has its own prepare/enable refs and releases them
> as it goes out of scope. This implies wrapping *mut bindings::clk in an
> Arc<> because bindings::clk is not ARef, but should be relatively easy
> to do. Posting the quick experiment I did with this approach, in case
> you're interested [1]
> 
> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837

The problem with what you have suggested is that the previous state is not
consumed if you can clone it, and consuming the previous state is a pretty key
element in ensuring you cannot misuse it. For example, here:

let enabled_clk = prepared_clk.clone().enable()?;
// do stuff
// enabled_clk goes out of scope and releases the enable
// ref it had

prepared_clk is still alive. Now, this may not be the end of the world in this
particular case, but for API consistency, I'd say we should probably avoid this
behavior.

I see that Alice suggested a closure approach. IMHO, we should use that
instead.

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

Hi Boris,

> On 3 Feb 2026, at 06:17, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> Hello Daniel,
> 
> On Wed, 07 Jan 2026 12:09:52 -0300
> Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
>> -        /// Disable and unprepare the clock.
>> +    impl Clk<Enabled> {
>> +        /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection id
>> +        /// and then prepares and enables it.
>>         ///
>> -        /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
>> +        /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
>> +        /// followed by [`Clk::enable`].
>>         #[inline]
>> -        pub fn disable_unprepare(&self) {
>> -            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
>> -            // [`clk_disable_unprepare`].
>> -            unsafe { bindings::clk_disable_unprepare(self.as_raw()) };
>> +        pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
>> +            Clk::<Prepared>::get(dev, name)?
>> +                .enable()
>> +                .map_err(|error| error.error)
>> +        }
>> +
>> +        /// Behaves the same as [`Self::get`], except when there is no clock
>> +        /// producer. In this case, instead of returning [`ENOENT`], it returns
>> +        /// a dummy [`Clk`].
>> +        #[inline]
>> +        pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
>> +            Clk::<Prepared>::get_optional(dev, name)?
>> +                .enable()
>> +                .map_err(|error| error.error)
>> +        }
>> +
>> +        /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]
>> +        /// state.
>> +        #[inline]
>> +        pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
>> +            // We will be transferring the ownership of our `clk_get()` and
>> +            // `clk_enable()` counts to `Clk<Prepared>`.
>> +            let clk = ManuallyDrop::new(self);
>> +
>> +            // SAFETY: By the type invariants, `self.0` is a valid argument for
>> +            // [`clk_disable`].
>> +            unsafe { bindings::clk_disable(clk.as_raw()) };
>> +
>> +            Ok(Clk {
>> +                inner: clk.inner,
>> +                _phantom: PhantomData,
>> +            })
>>         }
>> 
>>         /// Get clock's rate.
> 
> Dunno if this has been mentioned already, but I belive the rate
> getter/setter should be in the generic implementation. Indeed, it's
> quite common for clock users to change the rate when the clk is
> disabled to avoid unstable transitional state. The usual pattern for
> that is:
> 
> - clk_set_parent(my_clk, secondary_parent)
> - clk_disable[_unprepare](primary_parent) // (usually a PLL)
> - clk_set_rate(primary_parent)
> - clk[_prepare]_enable(primary_parent)
> - clk_set_parent(my_clk, primary_parent)
> 
> The case where the clk rate is changed while the clk is active is also
> valid (usually fine when it's just a divider that's changed, because
> there's no stabilization period).
> 
>> @@ -252,83 +429,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
>>         }
>>     }
> 


I’m ok with this. I just assumed that these operations were only valid on enabled clks.

Will change this in the next version.

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

> On 3 Feb 2026, at 06:09, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> Hello Daniel,
> 
> On Mon, 2 Feb 2026 17:10:38 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
>>>> -#[pin_data(PinnedDrop)]
>>>> +#[pin_data]
>>>> pub(crate) struct TyrData {
>>>>     pub(crate) pdev: ARef<platform::Device>,
>>>> 
>>>> @@ -92,13 +92,9 @@ fn probe(
>>>>         pdev: &platform::Device<Core>,
>>>>         _info: Option<&Self::IdInfo>,
>>>>     ) -> impl PinInit<Self, Error> {
>>>> -        let core_clk = Clk::get(pdev.as_ref(), Some(c_str!("core")))?;
>>>> -        let stacks_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("stacks")))?;
>>>> -        let coregroup_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("coregroup")))?;
>>>> -
>>>> -        core_clk.prepare_enable()?;
>>>> -        stacks_clk.prepare_enable()?;
>>>> -        coregroup_clk.prepare_enable()?;
>>>> +        let core_clk = Clk::<Enabled>::get(pdev.as_ref(), Some(c_str!("core")))?;    
>>> 
>>> Ah, more turbofish.. I'd really want to avoid them if possible.
>>> 
>>> Any disadvantage on just ask the user to chain `.get().prepare_enable()?`? This
>>> way it is also clear that some action is performed.  
>> 
>> I've just disc
> 
> Sorry, I've hit the reply button before I had finished writing my
> answer. So I was about to say that I had started writing something
> similar without knowing this series existed, and I feel like we'd don't
> really need those prepare_enable() shortcuts that exist in C. We might
> has well just go:
> 
> Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;
> 
> and have the following variant-specofoc functions
> 
> - Clk<Unprepared>::get[_optional]() (no get on Prepared and Enabled
>  variants)
> - Clk<Unprepared>::prepare()
> - Clk<Prepared>::{enable,unprepare}()
> - Clk<Enabled>::{disable}()
> 
> Regards,
> 
> Boris
> 
> 


I don’t understand how is this better than the turbofish we currently have.

In other words, how is this:

Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;

Better than this:

Clk::<Enabled>::get(/*…*/);

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Gary Guo]

On Tue Feb 3, 2026 at 1:33 PM GMT, Daniel Almeida wrote:
> Hi Boris,
>
>> On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>> 
>> On Mon, 19 Jan 2026 12:35:21 +0000
>> Alice Ryhl <aliceryhl@google.com> wrote:
>> 
>>> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>>>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:  
>>>>>> For example, it's quite typical to have (at least) one clock for the bus
>>>>>> interface that drives the register, and one that drives the main
>>>>>> component logic. The former needs to be enabled only when you're
>>>>>> accessing the registers (and can be abstracted with
>>>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>>>>> only when the device actually starts operating.
>>>>>> 
>>>>>> You have a similar thing for the prepare vs enable thing. The difference
>>>>>> between the two is that enable can be called into atomic context but
>>>>>> prepare can't.
>>>>>> 
>>>>>> So for drivers that would care about this, you would create your device
>>>>>> with an unprepared clock, and then at various times during the driver
>>>>>> lifetime, you would mutate that state.  
>>> 
>>> The case where you're doing it only while accessing registers is
>>> interesting, because that means the Enable bit may be owned by a local
>>> variable. We may imagine an:
>>> 
>>>    let enabled = self.prepared_clk.enable_scoped();
>>>    ... use registers
>>>    drop(enabled);
>>> 
>>> Now ... this doesn't quite work with the current API - the current
>>> Enabled stated owns both a prepare and enable count, but the above keeps
>>> the prepare count in `self` and the enabled count in a local variable.
>>> But it could be done with a fourth state, or by a closure method:
>>> 
>>>    self.prepared_clk.with_enabled(|| {
>>>        ... use registers
>>>    });
>>> 
>>> All of this would work with an immutable variable of type Clk<Prepared>.
>> 
>> Hm, maybe it'd make sense to implement Clone so we can have a temporary
>> clk variable that has its own prepare/enable refs and releases them
>> as it goes out of scope. This implies wrapping *mut bindings::clk in an
>> Arc<> because bindings::clk is not ARef, but should be relatively easy
>> to do. Posting the quick experiment I did with this approach, in case
>> you're interested [1]
>> 
>> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837
>
> The problem with what you have suggested is that the previous state is not
> consumed if you can clone it, and consuming the previous state is a pretty key
> element in ensuring you cannot misuse it. For example, here:
>
> let enabled_clk = prepared_clk.clone().enable()?;
> // do stuff
> // enabled_clk goes out of scope and releases the enable
> // ref it had
>
> prepared_clk is still alive. Now, this may not be the end of the world in this
> particular case, but for API consistency, I'd say we should probably avoid this
> behavior.

Is this an issue though? You cannot mistakenly own `Clk<Enabled>` while the clk
is not enabled, (and similarly for `Prepared`), and that should be sufficient.

Having `Clk<Prepared>` makes no guarantee on if the clk is enabled or not anyway
as you can have another user do `Clk<Unprepared>::get().enable()`.

The only guarantee is that the state the clk have is going to be greater than or
equal to the type state, so allowing cloning an intermediate state is no
problem.

Best,
Gary

>
> I see that Alice suggested a closure approach. IMHO, we should use that
> instead.
>
> — Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

> On 3 Feb 2026, at 10:42, Gary Guo <gary@garyguo.net> wrote:
> 
> On Tue Feb 3, 2026 at 1:33 PM GMT, Daniel Almeida wrote:
>> Hi Boris,
>> 
>>> On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>> 
>>> On Mon, 19 Jan 2026 12:35:21 +0000
>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>> 
>>>> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>>>>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:  
>>>>>>> For example, it's quite typical to have (at least) one clock for the bus
>>>>>>> interface that drives the register, and one that drives the main
>>>>>>> component logic. The former needs to be enabled only when you're
>>>>>>> accessing the registers (and can be abstracted with
>>>>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>>>>>> only when the device actually starts operating.
>>>>>>> 
>>>>>>> You have a similar thing for the prepare vs enable thing. The difference
>>>>>>> between the two is that enable can be called into atomic context but
>>>>>>> prepare can't.
>>>>>>> 
>>>>>>> So for drivers that would care about this, you would create your device
>>>>>>> with an unprepared clock, and then at various times during the driver
>>>>>>> lifetime, you would mutate that state.  
>>>> 
>>>> The case where you're doing it only while accessing registers is
>>>> interesting, because that means the Enable bit may be owned by a local
>>>> variable. We may imagine an:
>>>> 
>>>>   let enabled = self.prepared_clk.enable_scoped();
>>>>   ... use registers
>>>>   drop(enabled);
>>>> 
>>>> Now ... this doesn't quite work with the current API - the current
>>>> Enabled stated owns both a prepare and enable count, but the above keeps
>>>> the prepare count in `self` and the enabled count in a local variable.
>>>> But it could be done with a fourth state, or by a closure method:
>>>> 
>>>>   self.prepared_clk.with_enabled(|| {
>>>>       ... use registers
>>>>   });
>>>> 
>>>> All of this would work with an immutable variable of type Clk<Prepared>.
>>> 
>>> Hm, maybe it'd make sense to implement Clone so we can have a temporary
>>> clk variable that has its own prepare/enable refs and releases them
>>> as it goes out of scope. This implies wrapping *mut bindings::clk in an
>>> Arc<> because bindings::clk is not ARef, but should be relatively easy
>>> to do. Posting the quick experiment I did with this approach, in case
>>> you're interested [1]
>>> 
>>> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837
>> 
>> The problem with what you have suggested is that the previous state is not
>> consumed if you can clone it, and consuming the previous state is a pretty key
>> element in ensuring you cannot misuse it. For example, here:
>> 
>> let enabled_clk = prepared_clk.clone().enable()?;
>> // do stuff
>> // enabled_clk goes out of scope and releases the enable
>> // ref it had
>> 
>> prepared_clk is still alive. Now, this may not be the end of the world in this
>> particular case, but for API consistency, I'd say we should probably avoid this
>> behavior.
> 
> Is this an issue though? You cannot mistakenly own `Clk<Enabled>` while the clk
> is not enabled, (and similarly for `Prepared`), and that should be sufficient.

It is not an issue. However, I just find it a bit confusing. With a typestate, one
usually expects state transitions where a new state fully consumes the previous
one, and that assumption is “broken” in a way when you add clone().

> 
> Having `Clk<Prepared>` makes no guarantee on if the clk is enabled or not anyway
> as you can have another user do `Clk<Unprepared>::get().enable()`.

Although you’re right here, I find this less confusing than clone(). You
have to explicitly craft a new Clk<Enabled>, where a clone() is a shorter way
to basically get around the “state transition” idea on an _existing_ Clk
reference.

This is a bit pedantic on my side though, so I have no problem in adding
clone() if it's the consensus of the majority.

> 
> The only guarantee is that the state the clk have is going to be greater than or
> equal to the type state, so allowing cloning an intermediate state is no
> problem.
> 
> Best,
> Gary
> 
>> 
>> I see that Alice suggested a closure approach. IMHO, we should use that
>> instead.
>> 
>> — Daniel


Is there any pushback on the closure approach? If so, mind sharing why?

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Tue, 3 Feb 2026 10:33:34 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> Hi Boris,
> 
> > On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > 
> > On Mon, 19 Jan 2026 12:35:21 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >   
> >> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:  
> >>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:    
> >>>>> For example, it's quite typical to have (at least) one clock for the bus
> >>>>> interface that drives the register, and one that drives the main
> >>>>> component logic. The former needs to be enabled only when you're
> >>>>> accessing the registers (and can be abstracted with
> >>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
> >>>>> only when the device actually starts operating.
> >>>>> 
> >>>>> You have a similar thing for the prepare vs enable thing. The difference
> >>>>> between the two is that enable can be called into atomic context but
> >>>>> prepare can't.
> >>>>> 
> >>>>> So for drivers that would care about this, you would create your device
> >>>>> with an unprepared clock, and then at various times during the driver
> >>>>> lifetime, you would mutate that state.    
> >> 
> >> The case where you're doing it only while accessing registers is
> >> interesting, because that means the Enable bit may be owned by a local
> >> variable. We may imagine an:
> >> 
> >>    let enabled = self.prepared_clk.enable_scoped();
> >>    ... use registers
> >>    drop(enabled);
> >> 
> >> Now ... this doesn't quite work with the current API - the current
> >> Enabled stated owns both a prepare and enable count, but the above keeps
> >> the prepare count in `self` and the enabled count in a local variable.
> >> But it could be done with a fourth state, or by a closure method:
> >> 
> >>    self.prepared_clk.with_enabled(|| {
> >>        ... use registers
> >>    });
> >> 
> >> All of this would work with an immutable variable of type Clk<Prepared>.  
> > 
> > Hm, maybe it'd make sense to implement Clone so we can have a temporary
> > clk variable that has its own prepare/enable refs and releases them
> > as it goes out of scope. This implies wrapping *mut bindings::clk in an
> > Arc<> because bindings::clk is not ARef, but should be relatively easy
> > to do. Posting the quick experiment I did with this approach, in case
> > you're interested [1]
> > 
> > [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837  
> 
> The problem with what you have suggested is that the previous state is not
> consumed if you can clone it, and consuming the previous state is a pretty key
> element in ensuring you cannot misuse it. For example, here:
> 
> let enabled_clk = prepared_clk.clone().enable()?;
> // do stuff
> // enabled_clk goes out of scope and releases the enable
> // ref it had
> 
> prepared_clk is still alive.

That was intentional in this example. Think about a prepared_clk that's
stored in some driver-internal object, because you want to keep the clk
prepared at all times between the probe() and unbind(). Then you have
some sections where you want to briefly enable the clk to access
registers, and immediately disable it when you're done. The clone()
here guarantees that the initial prepared_clk stays valid.

If you were to disable, unprepare and put the clk when enabled_clk goes
out of scope, you'd go

	let enabled_clk = prepared_clk.enable()?;

and that would still work, it's just not the same use-case.

> Now, this may not be the end of the world in this
> particular case, but for API consistency, I'd say we should probably avoid this
> behavior.
> 
> I see that Alice suggested a closure approach. IMHO, we should use that
> instead.

The closure, while being useful for the above local clk-enablement
example, doesn't allow for passing some Clk<Enabled> guard around, like
you would do with a lock Guard, and I believe that's a useful thing to
have.

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Tue, 3 Feb 2026 10:37:15 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> > On 3 Feb 2026, at 06:09, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > 
> > Hello Daniel,
> > 
> > On Mon, 2 Feb 2026 17:10:38 +0100
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> >>>> -#[pin_data(PinnedDrop)]
> >>>> +#[pin_data]
> >>>> pub(crate) struct TyrData {
> >>>>     pub(crate) pdev: ARef<platform::Device>,
> >>>> 
> >>>> @@ -92,13 +92,9 @@ fn probe(
> >>>>         pdev: &platform::Device<Core>,
> >>>>         _info: Option<&Self::IdInfo>,
> >>>>     ) -> impl PinInit<Self, Error> {
> >>>> -        let core_clk = Clk::get(pdev.as_ref(), Some(c_str!("core")))?;
> >>>> -        let stacks_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("stacks")))?;
> >>>> -        let coregroup_clk = OptionalClk::get(pdev.as_ref(), Some(c_str!("coregroup")))?;
> >>>> -
> >>>> -        core_clk.prepare_enable()?;
> >>>> -        stacks_clk.prepare_enable()?;
> >>>> -        coregroup_clk.prepare_enable()?;
> >>>> +        let core_clk = Clk::<Enabled>::get(pdev.as_ref(), Some(c_str!("core")))?;      
> >>> 
> >>> Ah, more turbofish.. I'd really want to avoid them if possible.
> >>> 
> >>> Any disadvantage on just ask the user to chain `.get().prepare_enable()?`? This
> >>> way it is also clear that some action is performed.    
> >> 
> >> I've just disc  
> > 
> > Sorry, I've hit the reply button before I had finished writing my
> > answer. So I was about to say that I had started writing something
> > similar without knowing this series existed, and I feel like we'd don't
> > really need those prepare_enable() shortcuts that exist in C. We might
> > has well just go:
> > 
> > Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;
> > 
> > and have the following variant-specofoc functions
> > 
> > - Clk<Unprepared>::get[_optional]() (no get on Prepared and Enabled
> >  variants)
> > - Clk<Unprepared>::prepare()
> > - Clk<Prepared>::{enable,unprepare}()
> > - Clk<Enabled>::{disable}()
> > 
> > Regards,
> > 
> > Boris
> > 
> >   
> 
> 
> I don’t understand how is this better than the turbofish we currently have.
> 
> In other words, how is this:
> 
> Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;
> 
> Better than this:
> 
> Clk::<Enabled>::get(/*…*/);

For one, it doesn't force you to expose multiple functions in the
implementation (::get[_optional]() is only present in the Unprepared
impl variant, no shortcut to combine state transition, ...) which means
less code to maintain overall. But I also prefer the fact this clearly
reflects the state transitions that exist to get an Enabled clk (first
you get an Unprepared clk that you have to prepare and enable to turn
that into an Enabled clk). That's a matter of taste of course, just
saying that if we get rid of the turbofish syntax like Gary suggested
at some point, I think I'd find it clearer to also just expose the
transitions between two consecutive states, and let the caller go
through all the steps.

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Tue, 3 Feb 2026 10:55:05 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> > On 3 Feb 2026, at 10:42, Gary Guo <gary@garyguo.net> wrote:
> > 
> > On Tue Feb 3, 2026 at 1:33 PM GMT, Daniel Almeida wrote:  
> >> Hi Boris,
> >>   
> >>> On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >>> 
> >>> On Mon, 19 Jan 2026 12:35:21 +0000
> >>> Alice Ryhl <aliceryhl@google.com> wrote:
> >>>   
> >>>> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:  
> >>>>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:    
> >>>>>>> For example, it's quite typical to have (at least) one clock for the bus
> >>>>>>> interface that drives the register, and one that drives the main
> >>>>>>> component logic. The former needs to be enabled only when you're
> >>>>>>> accessing the registers (and can be abstracted with
> >>>>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
> >>>>>>> only when the device actually starts operating.
> >>>>>>> 
> >>>>>>> You have a similar thing for the prepare vs enable thing. The difference
> >>>>>>> between the two is that enable can be called into atomic context but
> >>>>>>> prepare can't.
> >>>>>>> 
> >>>>>>> So for drivers that would care about this, you would create your device
> >>>>>>> with an unprepared clock, and then at various times during the driver
> >>>>>>> lifetime, you would mutate that state.    
> >>>> 
> >>>> The case where you're doing it only while accessing registers is
> >>>> interesting, because that means the Enable bit may be owned by a local
> >>>> variable. We may imagine an:
> >>>> 
> >>>>   let enabled = self.prepared_clk.enable_scoped();
> >>>>   ... use registers
> >>>>   drop(enabled);
> >>>> 
> >>>> Now ... this doesn't quite work with the current API - the current
> >>>> Enabled stated owns both a prepare and enable count, but the above keeps
> >>>> the prepare count in `self` and the enabled count in a local variable.
> >>>> But it could be done with a fourth state, or by a closure method:
> >>>> 
> >>>>   self.prepared_clk.with_enabled(|| {
> >>>>       ... use registers
> >>>>   });
> >>>> 
> >>>> All of this would work with an immutable variable of type Clk<Prepared>.  
> >>> 
> >>> Hm, maybe it'd make sense to implement Clone so we can have a temporary
> >>> clk variable that has its own prepare/enable refs and releases them
> >>> as it goes out of scope. This implies wrapping *mut bindings::clk in an
> >>> Arc<> because bindings::clk is not ARef, but should be relatively easy
> >>> to do. Posting the quick experiment I did with this approach, in case
> >>> you're interested [1]
> >>> 
> >>> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837  
> >> 
> >> The problem with what you have suggested is that the previous state is not
> >> consumed if you can clone it, and consuming the previous state is a pretty key
> >> element in ensuring you cannot misuse it. For example, here:
> >> 
> >> let enabled_clk = prepared_clk.clone().enable()?;
> >> // do stuff
> >> // enabled_clk goes out of scope and releases the enable
> >> // ref it had
> >> 
> >> prepared_clk is still alive. Now, this may not be the end of the world in this
> >> particular case, but for API consistency, I'd say we should probably avoid this
> >> behavior.  
> > 
> > Is this an issue though? You cannot mistakenly own `Clk<Enabled>` while the clk
> > is not enabled, (and similarly for `Prepared`), and that should be sufficient.  
> 
> It is not an issue. However, I just find it a bit confusing. With a typestate, one
> usually expects state transitions where a new state fully consumes the previous
> one, and that assumption is “broken” in a way when you add clone().

It's just the way clks work in practice: you having a Clk<Unprepared>
doesn't mean the underlying clk_hw (the C object) is in an unprepared
state, because some other users might point to the same clk_hw and have
it enabled already. What Clk<State> means here is that you have a local
view of a clk that's in at least this State. In order to guarantee that
the clk is at least OtherState, you'll have to transition your view to
this OtherState.

Clone here just means you're cloning a view of this clone in its
original view state. Then you're free to do whatever you want on this
new view. So is the original owner of the object you clone from.

> 
> > 
> > Having `Clk<Prepared>` makes no guarantee on if the clk is enabled or not anyway
> > as you can have another user do `Clk<Unprepared>::get().enable()`.  
> 
> Although you’re right here, I find this less confusing than clone(). You
> have to explicitly craft a new Clk<Enabled>, where a clone() is a shorter way
> to basically get around the “state transition” idea on an _existing_ Clk
> reference.

The idea behind the clone() is that you can transition from one state
to an higher state (prepared -> enabled for instance) for a shorter
period of time than the cloned clk lifetime. Something like that, for
instance:

	let MyDevice {
		prepared_clk: Clk::get(...)?.prepare()?,
	}


	implem MyDevice {
		fn do_stuff(&self) {
			let enabled_clk = self.prepared_clk.clone();

			// do stuff that need to be guaranteed that clk
			// is enabled
			self.do_other_stuff(enabled_clk);

			// the enabled_clk object is dropped, but the
			// clk remains prepared because
			// self.prepared_clk is still there
		}
	}

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Tue, 3 Feb 2026 12:26:31 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Tue, 3 Feb 2026 11:39:02 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Mon, 19 Jan 2026 12:35:21 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >   
> > > On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:    
> > > > On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:      
> > > > > > For example, it's quite typical to have (at least) one clock for the bus
> > > > > > interface that drives the register, and one that drives the main
> > > > > > component logic. The former needs to be enabled only when you're
> > > > > > accessing the registers (and can be abstracted with
> > > > > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > > > > > only when the device actually starts operating.
> > > > > > 
> > > > > > You have a similar thing for the prepare vs enable thing. The difference
> > > > > > between the two is that enable can be called into atomic context but
> > > > > > prepare can't.
> > > > > > 
> > > > > > So for drivers that would care about this, you would create your device
> > > > > > with an unprepared clock, and then at various times during the driver
> > > > > > lifetime, you would mutate that state.      
> > > 
> > > The case where you're doing it only while accessing registers is
> > > interesting, because that means the Enable bit may be owned by a local
> > > variable. We may imagine an:
> > > 
> > >     let enabled = self.prepared_clk.enable_scoped();
> > >     ... use registers
> > >     drop(enabled);
> > > 
> > > Now ... this doesn't quite work with the current API - the current
> > > Enabled stated owns both a prepare and enable count, but the above keeps
> > > the prepare count in `self` and the enabled count in a local variable.
> > > But it could be done with a fourth state, or by a closure method:
> > > 
> > >     self.prepared_clk.with_enabled(|| {
> > >         ... use registers
> > >     });
> > > 
> > > All of this would work with an immutable variable of type Clk<Prepared>.    
> > 
> > Hm, maybe it'd make sense to implement Clone so we can have a temporary
> > clk variable that has its own prepare/enable refs and releases them
> > as it goes out of scope. This implies wrapping *mut bindings::clk in an
> > Arc<> because bindings::clk is not ARef, but should be relatively easy
> > to do. Posting the quick experiment I did with this approach, in case
> > you're interested [1]  
> 
> This time with a proper RawClk(*mut bindings::clk) wrapper, so we can
> clk_put() called in RawClk::drop() instead of in Clk::drop().
> 
> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/6fa6cb72f14373b276c61d038bc2b16f49c78f74

And I forgot to drop the ManuallyDrop in that one, but I bet you get the
idea.

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

<snip>

>> 
>> The problem with what you have suggested is that the previous state is not
>> consumed if you can clone it, and consuming the previous state is a pretty key
>> element in ensuring you cannot misuse it. For example, here:
>> 
>> let enabled_clk = prepared_clk.clone().enable()?;
>> // do stuff
>> // enabled_clk goes out of scope and releases the enable
>> // ref it had
>> 
>> prepared_clk is still alive.
> 
> That was intentional in this example. Think about a prepared_clk that's
> stored in some driver-internal object, because you want to keep the clk
> prepared at all times between the probe() and unbind(). Then you have
> some sections where you want to briefly enable the clk to access
> registers, and immediately disable it when you're done. The clone()
> here guarantees that the initial prepared_clk stays valid.
> 
> If you were to disable, unprepare and put the clk when enabled_clk goes
> out of scope, you'd go

> 
> let enabled_clk = prepared_clk.enable()?;
> 
> and that would still work, it's just not the same use-case.
> 

Ok, let’s have clone() then.


>> Now, this may not be the end of the world in this
>> particular case, but for API consistency, I'd say we should probably avoid this
>> behavior.
>> 
>> I see that Alice suggested a closure approach. IMHO, we should use that
>> instead.
> 
> The closure, while being useful for the above local clk-enablement
> example, doesn't allow for passing some Clk<Enabled> guard around, like
> you would do with a lock Guard, and I believe that's a useful thing to
> have.


Wdym? You’d still get a &Clk<Enabled> that you can pass around, i.e.:

   self.prepared_clk.with_enabled(|clk: &Clk<Enabled> | {
       ... use registers, pass &Clk<Enabled> as needed
   });

This is now not about clone() vs not clone(), but more about limiting the scope of the
Enabled state, which would cater to the use-case you mentioned IIUC.

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Tue, 3 Feb 2026 13:28:15 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> >> Now, this may not be the end of the world in this
> >> particular case, but for API consistency, I'd say we should probably avoid this
> >> behavior.
> >> 
> >> I see that Alice suggested a closure approach. IMHO, we should use that
> >> instead.  
> > 
> > The closure, while being useful for the above local clk-enablement
> > example, doesn't allow for passing some Clk<Enabled> guard around, like
> > you would do with a lock Guard, and I believe that's a useful thing to
> > have.  
> 
> 
> Wdym? You’d still get a &Clk<Enabled> that you can pass around, i.e.:
> 
>    self.prepared_clk.with_enabled(|clk: &Clk<Enabled> | {
>        ... use registers, pass &Clk<Enabled> as needed
>    });
> 
> This is now not about clone() vs not clone(), but more about limiting the scope of the
> Enabled state, which would cater to the use-case you mentioned IIUC.

Fair enough. I guess it's more a matter of taste for that particular
case, and I'm not fundamentally opposed to adding this closure approach.

What the clone approach allows that's not doable with the closure
AFAICT, is something like:

	let prep_clk = Clk::get(...)?.prepare()?;

	let comp_a = MyComponentA {
		prepared_clk: prep_clk.clone(),
	}

	let comp_b = MyComponentB {
		enabled_clk: prep_clk.enable()?,
	}

and have the guarantee that, as long as comp_a is alive the underlying
clk is at-least-Prepared, and as long as comp_b is alive the underlying
clk is Enabled.

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Gary Guo]

On Tue Feb 3, 2026 at 4:28 PM GMT, Daniel Almeida wrote:
> <snip>
>
>>> 
>>> The problem with what you have suggested is that the previous state is not
>>> consumed if you can clone it, and consuming the previous state is a pretty key
>>> element in ensuring you cannot misuse it. For example, here:
>>> 
>>> let enabled_clk = prepared_clk.clone().enable()?;
>>> // do stuff
>>> // enabled_clk goes out of scope and releases the enable
>>> // ref it had
>>> 
>>> prepared_clk is still alive.
>> 
>> That was intentional in this example. Think about a prepared_clk that's
>> stored in some driver-internal object, because you want to keep the clk
>> prepared at all times between the probe() and unbind(). Then you have
>> some sections where you want to briefly enable the clk to access
>> registers, and immediately disable it when you're done. The clone()
>> here guarantees that the initial prepared_clk stays valid.
>> 
>> If you were to disable, unprepare and put the clk when enabled_clk goes
>> out of scope, you'd go
>
>> 
>> let enabled_clk = prepared_clk.enable()?;
>> 
>> and that would still work, it's just not the same use-case.
>> 
>
> Ok, let’s have clone() then.
>
>
>>> Now, this may not be the end of the world in this
>>> particular case, but for API consistency, I'd say we should probably avoid this
>>> behavior.
>>> 
>>> I see that Alice suggested a closure approach. IMHO, we should use that
>>> instead.
>> 
>> The closure, while being useful for the above local clk-enablement
>> example, doesn't allow for passing some Clk<Enabled> guard around, like
>> you would do with a lock Guard, and I believe that's a useful thing to
>> have.
>
>
> Wdym? You’d still get a &Clk<Enabled> that you can pass around, i.e.:
>
>    self.prepared_clk.with_enabled(|clk: &Clk<Enabled> | {
>        ... use registers, pass &Clk<Enabled> as needed
>    });
>
> This is now not about clone() vs not clone(), but more about limiting the scope of the
> Enabled state, which would cater to the use-case you mentioned IIUC.

I think it's fine to have all of these:
* `Clone` impl
* `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
* `with_enabled` that gives `&Clk<Enabled>`

This way, if you only want to enable in short time, you can do `with_enabled`.
If the closure callback wants to keep clock enabled for longer, it can just do
`.clone()` inside the closure and obtain an owned `Clk<Enabled>`.

If the user just have a reference and want to enable the callback they can do
`prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?

Best,
Gary

>
> — Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

> 
> I think it's fine to have all of these:
> * `Clone` impl
> * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
> * `with_enabled` that gives `&Clk<Enabled>`
> 
> This way, if you only want to enable in short time, you can do `with_enabled`.
> If the closure callback wants to keep clock enabled for longer, it can just do
> `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
> 
> If the user just have a reference and want to enable the callback they can do
> `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
> 
> Best,
> Gary


I’m ok with what you proposed above. The only problem is that implementing
clone() is done through an Arc<*mut bindings::clk>  in Boris’ current
design, so this requires an extra allocation.

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

On Tue, 3 Feb 2026 16:26:22 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> > 
> > I think it's fine to have all of these:
> > * `Clone` impl
> > * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
> > * `with_enabled` that gives `&Clk<Enabled>`
> > 
> > This way, if you only want to enable in short time, you can do `with_enabled`.
> > If the closure callback wants to keep clock enabled for longer, it can just do
> > `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
> > 
> > If the user just have a reference and want to enable the callback they can do
> > `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
> > 
> > Best,
> > Gary  
> 
> 
> I’m ok with what you proposed above. The only problem is that implementing
> clone() is done through an Arc<*mut bindings::clk>  in Boris’ current
> design,

It's actually Arc<RawClk> with

    struct RawClk(*mut bindings::clk);

    impl Drop for RawClk {
        fn drop(&mut self) {
            // SAFETY: By the type invariants, self.as_raw() is a valid argument for // [`clk_put`].
            unsafe { bindings::clk_put(self.0) };
        }
    }

This is because struct clk is not refcounted, so cloning
implies wrapping this object in an Arc, and only calling
clk_put() when the Arc refcnt reaches zero.

> so this requires an extra allocation.

That's true. But the memory overhead should be pretty negligible,
and I don't think the extra indirection makes any noticeable
difference for an actual clk implementation (one that's not a NOP),
since we have indirections all over the place already (clk -> clk_hw,
clk_ops, ...). So I think I'd value ease of use over this small
perfs/mem-usage hit.

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Gary Guo]

On Tue Feb 3, 2026 at 7:26 PM GMT, Daniel Almeida wrote:
>
>> 
>> I think it's fine to have all of these:
>> * `Clone` impl
>> * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
>> * `with_enabled` that gives `&Clk<Enabled>`
>> 
>> This way, if you only want to enable in short time, you can do `with_enabled`.
>> If the closure callback wants to keep clock enabled for longer, it can just do
>> `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
>> 
>> If the user just have a reference and want to enable the callback they can do
>> `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
>> 
>> Best,
>> Gary
>
>
> I’m ok with what you proposed above. The only problem is that implementing
> clone() is done through an Arc<*mut bindings::clk>  in Boris’ current
> design, so this requires an extra allocation.

Hmm, that's a very good point. `struct clk` is already a reference into
clk_core, so having to put another level of indirection over is not ideal.
However, if we're going to keep C code unchanged and do a zero-cost abstraction
on the Rust side, then we won't be able to have have multiple prepare/enable to
the same `struct clk` with the current design.

It feels like we can to do a trade-off and choose from:
* Not be able to have multiple prepare/enable calls on the same `clk` (this can
  limit users that need dynamically enable/disable clocks, with the very limited
  exception that closure-callback is fine).
* Do an extra allocation
* Put lifetime on types that represent a prepared/enabled `Clk`
* Change C to make `struct clk` refcounted.

Best,
Gary

>
> — Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

Hi Gary, Daniel,

On Tue, 03 Feb 2026 20:36:30 +0000
"Gary Guo" <gary@garyguo.net> wrote:

> On Tue Feb 3, 2026 at 7:26 PM GMT, Daniel Almeida wrote:
> >  
> >> 
> >> I think it's fine to have all of these:
> >> * `Clone` impl
> >> * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
> >> * `with_enabled` that gives `&Clk<Enabled>`
> >> 
> >> This way, if you only want to enable in short time, you can do `with_enabled`.
> >> If the closure callback wants to keep clock enabled for longer, it can just do
> >> `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
> >> 
> >> If the user just have a reference and want to enable the callback they can do
> >> `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
> >> 
> >> Best,
> >> Gary  
> >
> >
> > I’m ok with what you proposed above. The only problem is that implementing
> > clone() is done through an Arc<*mut bindings::clk>  in Boris’ current
> > design, so this requires an extra allocation.  
> 
> Hmm, that's a very good point. `struct clk` is already a reference into
> clk_core, so having to put another level of indirection over is not ideal.
> However, if we're going to keep C code unchanged and do a zero-cost abstraction
> on the Rust side, then we won't be able to have have multiple prepare/enable to
> the same `struct clk` with the current design.
> 
> It feels like we can to do a trade-off and choose from:
> 1. Not be able to have multiple prepare/enable calls on the same `clk` (this can
>    limit users that need dynamically enable/disable clocks, with the very limited
>    exception that closure-callback is fine).
> 2. Do an extra allocation
> 3. Put lifetime on types that represent a prepared/enabled `Clk`
> 4. Change C to make `struct clk` refcounted.

It probably comes to no surprise that I'd be more in favor of option 2
or 4. Maybe option 2 first, so we can get the user-facing API merged
without too much churn, and then we can see if the clk maintainers are
happy adding a refcnt to struct clk to optimize things.

If we really feel that the indirection/memory overhead is going to
hurt us, we can also start with option 1, and extend it to 2 and/or 4
(needed to add a Clone support) when it becomes evident we can't do
without it. But as I was saying in my previous reply to Daniel, I
expect the extra indirection/memory overhead to be negligible since:

1. clks are usually not {prepared,enabled}/{disabled,unprepared} in a
   hot path
2. in the rare occasions where they might be ({dev,cpu}freq ?), this
   clk state change is usually one operation in an ocean of other
   slower operations (regulator reconfiguration, for instance, which
   usually goes over a slow I2C bus, or a
   relatively-faster-but-still-slow SPI one, at least when we compare
   it to an IoMem access for in-SoCs clks). So overall, the clk state
   change might account for a very small portion of the CPU cycles
   spent in this bigger operation
3. if I focus solely on the clk aspect, and look at the existing
   indirections in the clk framework (clk -> clk_core -> clk_{hw,ops} ->
   clk_methods), I'd expect the Arc indirection to be just noise in
   this pre-existing overhead
4. in term of memory, we're talking about 16 more bytes allocated per
   Clk on a 64-bit architecture (refcount is an int, but the alignment
   for the clk pointer forces 4 bytes of padding on most
   architectures). On a 64 bit arch, struct clk is 72 bytes if my math
   is correct, so that's a 22% overhead, compared to 11% overhead if
   the refcount was in struct clk (or in a struct
   refcounted_clk variant if we don't want C users to pay the price).
   Not great, but not terrible either

So yeah my gut feeling is that we might be overthinking this extra
allocation/indirection issue. This being said, one thing I'd really like
to avoid is us being dragged into infinite discussions about a perfect
implementation causing the merging of these changes to be delayed and
other contributions being blocked on this (perfect is the enemy of
good). I mean, option #1 is already an improvement compared to the raw
functions we have at the moment, so if that's the middle-ground we
agree on, I'm happy to give it my R-b.

Regards,

Boris

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Thu, Jan 22, 2026 at 09:29:30PM -0300, Daniel Almeida wrote:
> 
> 
> > On 22 Jan 2026, at 10:44, Maxime Ripard <mripard@kernel.org> wrote:
> > 
> > On Mon, Jan 19, 2026 at 03:37:17PM +0100, Danilo Krummrich wrote:
> >> On Mon Jan 19, 2026 at 3:18 PM CET, Maxime Ripard wrote:
> >>> On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote:
> >>>> On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote:
> >>>>>> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote:
> >>>>>> I think that if you still want an API where you just call enable/disable
> >>>>>> directly on it with no protection against unbalanced calls, then that
> >>>>>> should be the special API. Probably called RawClk and functions marked
> >>>>>> unsafe. Unbalanced calls seem really dangerous and use should not be
> >>>>>> encouraged.
> >>>> 
> >>>> +1; and unless there is a use-case that requires otherwise, it should not even
> >>>> be possible to do this at all -- at least for driver code.
> >>> 
> >>> I mean, it's great, it's safe, etc. but it's also suboptimal from a PM
> >>> perspective on many platforms. It's totally fine to provide nice, safe,
> >>> ergonomic wrappers for the drivers that don't care (or can't, really),
> >>> but treating a legitimate optimisation as something we should consider
> >>> impossible to do is just weird to me.
> >> 
> >> I said that an unsafe API with potentially unbalanced calls is something we
> >> should clearly avoid for drivers. This is *not* equivalent to "treating a
> >> legitimate optimisation as something we should consider impossible".
> >> 
> >> If we discover use-cases where the current API doesn't work well, we can
> >> invenstigate further.
> > 
> > I'm not sure I'm following what you're saying, sorry. I've pointed out
> > such a use-case already.
> > 
> >>>>> I think we should discourage RawClk if at all possible. But if the consensus
> >>>>> is that we *really* need this easily-abused thing, I can provide a follow-up.
> >>>> 
> >>>> I think we should only do this if there are use-case with no alternative, so far
> >>>> there haven't been any AFAIK.
> >>> 
> >>> I don't really care about which alternative we come up with, but look at
> >>> devm_regmap_init_mmio_clk for example. It is a valid use-case that
> >>> already exists today, and has had for more than a decade at this point.
> >> 
> >> I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference
> >> count of the clock and prepares it when called and unprepares the clk in drops
> >> its reference in regmap_mmio_free_context() called from the devres callback.
> >> 
> >> That something we can easily do with the current API, no?
> > 
> > The current one, yes. Doing that in the API suggested here would involve
> > some boilerplate in all those drivers they don't have right now.
> > 
> > Maxime
> 
> Maxime, I know you’ve already pointed out a use-case, but I think the
> confusion stems from why you seem to think that the current solution cannot
> cater to the API you mentioned in a clean way. You seem to imply that there
> will be a lot of boilerplate involved, but we (or I) cannot see this. Perhaps
> it would help if you highlighted how exactly the type state solution would be
> verbose using some pseudocode. I guess that would make your point clearer for
> us.

I'm probably missing something then, but let's assume you have a driver
that wants its clock prepared and enabled in an hypothetical enable()
callback, and disabled / unprepared in a disable() callback.

From a PM management perspective, this usecase makes total sense, is a
valid usecase, is widely used in the kernel, and is currently supported
by both the C and Rust clk APIs.

The only solution to this you suggested so far (I think?) to implement
this on top of the new clk API you propose is to have a driver specific
enum that would store each of the possible state transition.

That's the boilerplate I'm talking about. If every driver wanting to
implement that pattern has to make such an enum, with all the relevant
traits implementation that might come with it, we go from an API where
everything works at no-cost from a code-size perspective to a situation
where every driver has to develop and maintain that enum.

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Wed, Feb 04, 2026 at 09:11:04AM +0100, Boris Brezillon wrote:
> Hi Gary, Daniel,
> 
> On Tue, 03 Feb 2026 20:36:30 +0000
> "Gary Guo" <gary@garyguo.net> wrote:
> 
> > On Tue Feb 3, 2026 at 7:26 PM GMT, Daniel Almeida wrote:
> > >  
> > >> 
> > >> I think it's fine to have all of these:
> > >> * `Clone` impl
> > >> * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
> > >> * `with_enabled` that gives `&Clk<Enabled>`
> > >> 
> > >> This way, if you only want to enable in short time, you can do `with_enabled`.
> > >> If the closure callback wants to keep clock enabled for longer, it can just do
> > >> `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
> > >> 
> > >> If the user just have a reference and want to enable the callback they can do
> > >> `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
> > >> 
> > >> Best,
> > >> Gary  
> > >
> > >
> > > I’m ok with what you proposed above. The only problem is that implementing
> > > clone() is done through an Arc<*mut bindings::clk>  in Boris’ current
> > > design, so this requires an extra allocation.  
> > 
> > Hmm, that's a very good point. `struct clk` is already a reference into
> > clk_core, so having to put another level of indirection over is not ideal.
> > However, if we're going to keep C code unchanged and do a zero-cost abstraction
> > on the Rust side, then we won't be able to have have multiple prepare/enable to
> > the same `struct clk` with the current design.
> > 
> > It feels like we can to do a trade-off and choose from:
> > 1. Not be able to have multiple prepare/enable calls on the same `clk` (this can
> >    limit users that need dynamically enable/disable clocks, with the very limited
> >    exception that closure-callback is fine).
> > 2. Do an extra allocation
> > 3. Put lifetime on types that represent a prepared/enabled `Clk`
> > 4. Change C to make `struct clk` refcounted.
> 
> It probably comes to no surprise that I'd be more in favor of option 2
> or 4. Maybe option 2 first, so we can get the user-facing API merged
> without too much churn, and then we can see if the clk maintainers are
> happy adding a refcnt to struct clk to optimize things.
> 
> If we really feel that the indirection/memory overhead is going to
> hurt us, we can also start with option 1, and extend it to 2 and/or 4
> (needed to add a Clone support) when it becomes evident we can't do
> without it. But as I was saying in my previous reply to Daniel, I
> expect the extra indirection/memory overhead to be negligible since:
> 
> 1. clks are usually not {prepared,enabled}/{disabled,unprepared} in a
>    hot path
> 2. in the rare occasions where they might be ({dev,cpu}freq ?), this
>    clk state change is usually one operation in an ocean of other
>    slower operations (regulator reconfiguration, for instance, which
>    usually goes over a slow I2C bus, or a
>    relatively-faster-but-still-slow SPI one, at least when we compare
>    it to an IoMem access for in-SoCs clks). So overall, the clk state
>    change might account for a very small portion of the CPU cycles
>    spent in this bigger operation

I'm not even too worried about that one. devfreq and cpufreq will
typically adjust the clock rate while device is active, and thus the
clock is enabled. So we shouldn't have a prepared -> enabled transition
in the path that adjust the device / cpu rate, it would have happened
before.

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Daniel Almeida]

> 
> I'm probably missing something then, but let's assume you have a driver
> that wants its clock prepared and enabled in an hypothetical enable()
> callback, and disabled / unprepared in a disable() callback.
> 
> From a PM management perspective, this usecase makes total sense, is a
> valid usecase, is widely used in the kernel, and is currently supported
> by both the C and Rust clk APIs.
> 
> The only solution to this you suggested so far (I think?) to implement
> this on top of the new clk API you propose is to have a driver specific
> enum that would store each of the possible state transition.

Yes, you need an enum _if_ you want to model transitions at runtime. IIUC you
only need two variants to implement the pattern you described. I do not
consider this  “boilerplate”, but rather a small cost to pay.

I would understand if this was some elaborate pattern that had to be
implemented by all drivers, but a two-variant enum is as straightforward as it
gets.


> 
> That's the boilerplate I'm talking about. If every driver wanting to
> implement that pattern has to make such an enum, with all the relevant
> traits implementation that might come with it, we go from an API where
> everything works at no-cost from a code-size perspective to a situation
> where every driver has to develop and maintain that enum.
> 
> Maxime

There are no "traits that come with it". It's just an enum, with two variants.

> API where everything works at no-cost

The previous API was far from “everything works”. It was fundamentally
broken by design in multiple ways, i.e.:

> a) It only keeps track of a count to clk_get(), which means that users have
> to manually call disable() and unprepare(), or a variation of those, like
> disable_unprepare().
> 
> b) It allows repeated calls to prepare() or enable(), but it keeps no track
> of how often these were called, i.e., it's currently legal to write the
> following:
> 
> clk.prepare();
> clk.prepare();
> clk.enable();
> clk.enable();
> 
> And nothing gets undone on drop().

IMHO, what we have here is an improvement that has been long overdue.

— Daniel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Wed, Feb 04, 2026 at 09:43:55AM -0300, Daniel Almeida wrote:
> > I'm probably missing something then, but let's assume you have a driver
> > that wants its clock prepared and enabled in an hypothetical enable()
> > callback, and disabled / unprepared in a disable() callback.
> > 
> > From a PM management perspective, this usecase makes total sense, is a
> > valid usecase, is widely used in the kernel, and is currently supported
> > by both the C and Rust clk APIs.
> > 
> > The only solution to this you suggested so far (I think?) to implement
> > this on top of the new clk API you propose is to have a driver specific
> > enum that would store each of the possible state transition.
> 
> Yes, you need an enum _if_ you want to model transitions at runtime. IIUC you
> only need two variants to implement the pattern you described. I do not
> consider this  “boilerplate”, but rather a small cost to pay.

A maintenance cost to pay by every driver is kind of the textbook
definition of boilerplate to me.

> I would understand if this was some elaborate pattern that had to be
> implemented by all drivers, but a two-variant enum is as
> straightforward as it gets.

And yet, that framework has dozens of helpers that do not remove
anything from drivers but a couple of lines. So surely its users must
find value in reducing that boilerplate as much as possible. And you do
implement some of them, so you must find value in that too.

> > That's the boilerplate I'm talking about. If every driver wanting to
> > implement that pattern has to make such an enum, with all the relevant
> > traits implementation that might come with it, we go from an API where
> > everything works at no-cost from a code-size perspective to a situation
> > where every driver has to develop and maintain that enum.
>
> There are no "traits that come with it". It's just an enum, with two
> variants.
> 
> > API where everything works at no-cost
> 
> The previous API was far from “everything works”. It was fundamentally
> broken by design in multiple ways, i.e.:

Out of context and not what I meant, but ok.

> > a) It only keeps track of a count to clk_get(), which means that users have
> > to manually call disable() and unprepare(), or a variation of those, like
> > disable_unprepare().
> > 
> > b) It allows repeated calls to prepare() or enable(), but it keeps no track
> > of how often these were called, i.e., it's currently legal to write the
> > following:
> > 
> > clk.prepare();
> > clk.prepare();
> > clk.enable();
> > clk.enable();
> > 
> > And nothing gets undone on drop().
> 
> IMHO, what we have here is an improvement that has been long overdue.

Nothing is absolute. It is indeed an improvement on the refcounting side
of things and general safety of the API for the general case. I don't
think I ever questionned that.

However, for the use-cases we've been discussing (and dozens of drivers
implementing it), it also comes with a regression in the amount of code
to create and maintain. They used to be able to only deal with the Clk
structure, and now they can't anymore.

You might find that neglible, you might have a plan to address that in
the future, etc. and that's fine, but if you can't acknowledge that it's
indeed happening, there's no point in me raising the issue and
continuing the discussion.

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Boris Brezillon]

Hi Maxime,

On Wed, 4 Feb 2026 15:34:29 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Wed, Feb 04, 2026 at 09:43:55AM -0300, Daniel Almeida wrote:
> > > I'm probably missing something then, but let's assume you have a driver
> > > that wants its clock prepared and enabled in an hypothetical enable()
> > > callback, and disabled / unprepared in a disable() callback.
> > > 
> > > From a PM management perspective, this usecase makes total sense, is a
> > > valid usecase, is widely used in the kernel, and is currently supported
> > > by both the C and Rust clk APIs.
> > > 
> > > The only solution to this you suggested so far (I think?) to implement
> > > this on top of the new clk API you propose is to have a driver specific
> > > enum that would store each of the possible state transition.  
> > 
> > Yes, you need an enum _if_ you want to model transitions at runtime. IIUC you
> > only need two variants to implement the pattern you described. I do not
> > consider this  “boilerplate”, but rather a small cost to pay.  
> 
> A maintenance cost to pay by every driver is kind of the textbook
> definition of boilerplate to me.
> 
> > I would understand if this was some elaborate pattern that had to be
> > implemented by all drivers, but a two-variant enum is as
> > straightforward as it gets.  
> 
> And yet, that framework has dozens of helpers that do not remove
> anything from drivers but a couple of lines. So surely its users must
> find value in reducing that boilerplate as much as possible. And you do
> implement some of them, so you must find value in that too.
> 
> > > That's the boilerplate I'm talking about. If every driver wanting to
> > > implement that pattern has to make such an enum, with all the relevant
> > > traits implementation that might come with it, we go from an API where
> > > everything works at no-cost from a code-size perspective to a situation
> > > where every driver has to develop and maintain that enum.  
> >
> > There are no "traits that come with it". It's just an enum, with two
> > variants.
> >   
> > > API where everything works at no-cost  
> > 
> > The previous API was far from “everything works”. It was fundamentally
> > broken by design in multiple ways, i.e.:  
> 
> Out of context and not what I meant, but ok.
> 
> > > a) It only keeps track of a count to clk_get(), which means that users have
> > > to manually call disable() and unprepare(), or a variation of those, like
> > > disable_unprepare().
> > > 
> > > b) It allows repeated calls to prepare() or enable(), but it keeps no track
> > > of how often these were called, i.e., it's currently legal to write the
> > > following:
> > > 
> > > clk.prepare();
> > > clk.prepare();
> > > clk.enable();
> > > clk.enable();
> > > 
> > > And nothing gets undone on drop().  
> > 
> > IMHO, what we have here is an improvement that has been long overdue.  
> 
> Nothing is absolute. It is indeed an improvement on the refcounting side
> of things and general safety of the API for the general case. I don't
> think I ever questionned that.
> 
> However, for the use-cases we've been discussing (and dozens of drivers
> implementing it), it also comes with a regression in the amount of code
> to create and maintain. They used to be able to only deal with the Clk
> structure, and now they can't anymore.
> 
> You might find that neglible, you might have a plan to address that in
> the future, etc. and that's fine, but if you can't acknowledge that it's
> indeed happening, there's no point in me raising the issue and
> continuing the discussion.


Okay, let's see if I can sum-up the use case you'd like to support. You
have some PM hooks, which I'm assuming are (or will be) written in
rust. It will probably take the form of some Device{Rpm,Pm} trait to
implement for your XxxDeviceData (Xxx being the bus under which is
device is) object (since I've only recently joined the R4L effort, I
wouldn't be surprised if what I'm describing already exists or is
currently being proposed/reviewed somewhere, so please excuse my
ignorance if that's the case :-)).

The way I see it, rather than having one enum per clk/regulator/xxx
where we keep track of each state individually, what we could have is a

trait DevicePm {
	type ResumedState;
	type SuspendedState;

	fn resume(&self, state: SuspendedState) -> Result<ResumedState, Error<SuspendedState>>;
	fn suspend(&self, state: SuspendedState) -> Result<SuspendedState, Error<ResumedState>>;
};

enum DeviceState<T: DevicePm> {
	Resumed(T::ResumedState),
	Suspended(T::SuspendedState),
};

and then in your driver:

MySuspendedDeviceResources {
	xxx_clk: Clk<Unprepared>,
};

MyResumedDeviceResources {
	xxx_clk: Clk<Enabled>,
};

implem DevicePm for MyDevice {
	type ResumedState = MyResumedDeviceResources;
	type SuspendedState = MySuspendedDeviceResources;

	fn resume(&self, state: SuspendedState) -> Result<ResumedState, Error<SuspendedState>> {
		// FIXME: error propagation not handled
		let enabled_clk = state.xxx_clk.clone().prepare()?.enable()?;

		Ok(ResumedState {
			xxx_clk: enabled_clk,
		});
	}

	fn suspend(&self, state: ResumedState) -> Result<SuspendedState, Error<ResumedState>> {
		// FIXME: error propagation not handled
		let unprep_clk = state.xxx_clk.clone().disable().unprepare();

		Ok(SuspendedState {
			xxx_clk: unprep_clk,
		});
	}
};

With this model, I don't think Daniel's refactor goes in the way of more
generalization at the core level, it's just expressed differently than
it would be if it was written in C. And I say that as someone who struggles
with his C developer bias every time I'm looking at or trying to write
rust code.

As others have said in this thread (Danilo and Gary), and after having
played myself with both approaches in Tyr, I do see this shift from manual
prepare/enable to an RAII approach as an improvement, so I hope we can
find a middle-ground where every one is happy.

Regards,

Boris

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Mon, Feb 09, 2026 at 10:50:47AM +0100, Boris Brezillon wrote:
> Hi Maxime,
> 
> On Wed, 4 Feb 2026 15:34:29 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Wed, Feb 04, 2026 at 09:43:55AM -0300, Daniel Almeida wrote:
> > > > I'm probably missing something then, but let's assume you have a driver
> > > > that wants its clock prepared and enabled in an hypothetical enable()
> > > > callback, and disabled / unprepared in a disable() callback.
> > > > 
> > > > From a PM management perspective, this usecase makes total sense, is a
> > > > valid usecase, is widely used in the kernel, and is currently supported
> > > > by both the C and Rust clk APIs.
> > > > 
> > > > The only solution to this you suggested so far (I think?) to implement
> > > > this on top of the new clk API you propose is to have a driver specific
> > > > enum that would store each of the possible state transition.  
> > > 
> > > Yes, you need an enum _if_ you want to model transitions at runtime. IIUC you
> > > only need two variants to implement the pattern you described. I do not
> > > consider this  “boilerplate”, but rather a small cost to pay.  
> > 
> > A maintenance cost to pay by every driver is kind of the textbook
> > definition of boilerplate to me.
> > 
> > > I would understand if this was some elaborate pattern that had to be
> > > implemented by all drivers, but a two-variant enum is as
> > > straightforward as it gets.  
> > 
> > And yet, that framework has dozens of helpers that do not remove
> > anything from drivers but a couple of lines. So surely its users must
> > find value in reducing that boilerplate as much as possible. And you do
> > implement some of them, so you must find value in that too.
> > 
> > > > That's the boilerplate I'm talking about. If every driver wanting to
> > > > implement that pattern has to make such an enum, with all the relevant
> > > > traits implementation that might come with it, we go from an API where
> > > > everything works at no-cost from a code-size perspective to a situation
> > > > where every driver has to develop and maintain that enum.  
> > >
> > > There are no "traits that come with it". It's just an enum, with two
> > > variants.
> > >   
> > > > API where everything works at no-cost  
> > > 
> > > The previous API was far from “everything works”. It was fundamentally
> > > broken by design in multiple ways, i.e.:  
> > 
> > Out of context and not what I meant, but ok.
> > 
> > > > a) It only keeps track of a count to clk_get(), which means that users have
> > > > to manually call disable() and unprepare(), or a variation of those, like
> > > > disable_unprepare().
> > > > 
> > > > b) It allows repeated calls to prepare() or enable(), but it keeps no track
> > > > of how often these were called, i.e., it's currently legal to write the
> > > > following:
> > > > 
> > > > clk.prepare();
> > > > clk.prepare();
> > > > clk.enable();
> > > > clk.enable();
> > > > 
> > > > And nothing gets undone on drop().  
> > > 
> > > IMHO, what we have here is an improvement that has been long overdue.  
> > 
> > Nothing is absolute. It is indeed an improvement on the refcounting side
> > of things and general safety of the API for the general case. I don't
> > think I ever questionned that.
> > 
> > However, for the use-cases we've been discussing (and dozens of drivers
> > implementing it), it also comes with a regression in the amount of code
> > to create and maintain. They used to be able to only deal with the Clk
> > structure, and now they can't anymore.
> > 
> > You might find that neglible, you might have a plan to address that in
> > the future, etc. and that's fine, but if you can't acknowledge that it's
> > indeed happening, there's no point in me raising the issue and
> > continuing the discussion.
> 
> 
> Okay, let's see if I can sum-up the use case you'd like to support. You
> have some PM hooks, which I'm assuming are (or will be) written in
> rust. It will probably take the form of some Device{Rpm,Pm} trait to
> implement for your XxxDeviceData (Xxx being the bus under which is
> device is) object (since I've only recently joined the R4L effort, I
> wouldn't be surprised if what I'm describing already exists or is
> currently being proposed/reviewed somewhere, so please excuse my
> ignorance if that's the case :-)).
> 
> The way I see it, rather than having one enum per clk/regulator/xxx
> where we keep track of each state individually, what we could have is a
> 
> trait DevicePm {
> 	type ResumedState;
> 	type SuspendedState;
> 
> 	fn resume(&self, state: SuspendedState) -> Result<ResumedState, Error<SuspendedState>>;
> 	fn suspend(&self, state: SuspendedState) -> Result<SuspendedState, Error<ResumedState>>;
> };
> 
> enum DeviceState<T: DevicePm> {
> 	Resumed(T::ResumedState),
> 	Suspended(T::SuspendedState),
> };
> 
> and then in your driver:
> 
> MySuspendedDeviceResources {
> 	xxx_clk: Clk<Unprepared>,
> };
> 
> MyResumedDeviceResources {
> 	xxx_clk: Clk<Enabled>,
> };
> 
> implem DevicePm for MyDevice {
> 	type ResumedState = MyResumedDeviceResources;
> 	type SuspendedState = MySuspendedDeviceResources;
> 
> 	fn resume(&self, state: SuspendedState) -> Result<ResumedState, Error<SuspendedState>> {
> 		// FIXME: error propagation not handled
> 		let enabled_clk = state.xxx_clk.clone().prepare()?.enable()?;
> 
> 		Ok(ResumedState {
> 			xxx_clk: enabled_clk,
> 		});
> 	}
> 
> 	fn suspend(&self, state: ResumedState) -> Result<SuspendedState, Error<ResumedState>> {
> 		// FIXME: error propagation not handled
> 		let unprep_clk = state.xxx_clk.clone().disable().unprepare();
> 
> 		Ok(SuspendedState {
> 			xxx_clk: unprep_clk,
> 		});
> 	}
> };

I'm not sure we need to associate this with the suspend/resume state either.

> With this model, I don't think Daniel's refactor goes in the way of more
> generalization at the core level, it's just expressed differently than
> it would be if it was written in C. And I say that as someone who struggles
> with his C developer bias every time I'm looking at or trying to write
> rust code.
>
> As others have said in this thread (Danilo and Gary), and after having
> played myself with both approaches in Tyr, I do see this shift from manual
> prepare/enable to an RAII approach as an improvement, so I hope we can
> find a middle-ground where every one is happy.

I do think we can find a compromise though. Miguel suggested for example
to make the current enable/prepare/disable/unprepare function unsafe,
and that's totally reasonable to me.

Then we can implement the "managed" clock version on that unsafe API,
and we would end up with a "raw", unsafe, version kind of equivalent to
the one we have today, and where callers would have to justify why their
usage of the API is actually safe, or the new, managed, variant that is
safe and can be easily used by most drivers.

And we can call these RawClk vs Clk, or Clk vs ManagedClk, or whatever.

How does that sound?

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Wed Feb 11, 2026 at 5:37 PM CET, Maxime Ripard wrote:
> I do think we can find a compromise though. Miguel suggested for example
> to make the current enable/prepare/disable/unprepare function unsafe,
> and that's totally reasonable to me.
>
> Then we can implement the "managed" clock version on that unsafe API,

What do you mean with "managed" clock? Do you mean devres managed? If so, I
don't think there is any reason to switch to the unsafe API to be able to
implement devres managed APIs (see also [1]).

[1] https://lore.kernel.org/all/DFVW9MS5YLON.CVJDBYQKJ0P6@kernel.org/

> and we would end up with a "raw", unsafe, version kind of equivalent to
> the one we have today, and where callers would have to justify why their
> usage of the API is actually safe, or the new, managed, variant that is
> safe and can be easily used by most drivers.
>
> And we can call these RawClk vs Clk, or Clk vs ManagedClk, or whatever.
>
> How does that sound?

What about we just wait until we have a user that really requires an unsafe API
for some reason? And if it never appears, even better. :)

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Wed, Feb 11, 2026 at 05:47:09PM +0100, Danilo Krummrich wrote:
> On Wed Feb 11, 2026 at 5:37 PM CET, Maxime Ripard wrote:
> > I do think we can find a compromise though. Miguel suggested for example
> > to make the current enable/prepare/disable/unprepare function unsafe,
> > and that's totally reasonable to me.
> >
> > Then we can implement the "managed" clock version on that unsafe API,
> 
> What do you mean with "managed" clock? Do you mean devres managed? If so, I
> don't think there is any reason to switch to the unsafe API to be able to
> implement devres managed APIs (see also [1]).
> 
> [1] https://lore.kernel.org/all/DFVW9MS5YLON.CVJDBYQKJ0P6@kernel.org/

By that, I mean what Daniel has been proposing to achieve with this series.

> > and we would end up with a "raw", unsafe, version kind of equivalent to
> > the one we have today, and where callers would have to justify why their
> > usage of the API is actually safe, or the new, managed, variant that is
> > safe and can be easily used by most drivers.
> >
> > And we can call these RawClk vs Clk, or Clk vs ManagedClk, or whatever.
> >
> > How does that sound?
> 
> What about we just wait until we have a user that really requires an unsafe API
> for some reason? And if it never appears, even better. :)

It works *today*.

And the "oh but driver is using the API" is kind of ironic in the
context of the Rust bindings which have globally been in that situation
for years. You can't argue it both ways.

Either way, I'm not sure what the point of that submission was if you
will just dismiss diverging opinions, including attempts to compromise.

Do whatever you want, but it's really hard to root for you some times.

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Alice Ryhl]

On Wed, Feb 11, 2026 at 5:37 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Mon, Feb 09, 2026 at 10:50:47AM +0100, Boris Brezillon wrote:
> > Hi Maxime,
> >
> > On Wed, 4 Feb 2026 15:34:29 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> >
> > > On Wed, Feb 04, 2026 at 09:43:55AM -0300, Daniel Almeida wrote:
> > > > > I'm probably missing something then, but let's assume you have a driver
> > > > > that wants its clock prepared and enabled in an hypothetical enable()
> > > > > callback, and disabled / unprepared in a disable() callback.
> > > > >
> > > > > From a PM management perspective, this usecase makes total sense, is a
> > > > > valid usecase, is widely used in the kernel, and is currently supported
> > > > > by both the C and Rust clk APIs.
> > > > >
> > > > > The only solution to this you suggested so far (I think?) to implement
> > > > > this on top of the new clk API you propose is to have a driver specific
> > > > > enum that would store each of the possible state transition.
> > > >
> > > > Yes, you need an enum _if_ you want to model transitions at runtime. IIUC you
> > > > only need two variants to implement the pattern you described. I do not
> > > > consider this  “boilerplate”, but rather a small cost to pay.
> > >
> > > A maintenance cost to pay by every driver is kind of the textbook
> > > definition of boilerplate to me.
> > >
> > > > I would understand if this was some elaborate pattern that had to be
> > > > implemented by all drivers, but a two-variant enum is as
> > > > straightforward as it gets.
> > >
> > > And yet, that framework has dozens of helpers that do not remove
> > > anything from drivers but a couple of lines. So surely its users must
> > > find value in reducing that boilerplate as much as possible. And you do
> > > implement some of them, so you must find value in that too.
> > >
> > > > > That's the boilerplate I'm talking about. If every driver wanting to
> > > > > implement that pattern has to make such an enum, with all the relevant
> > > > > traits implementation that might come with it, we go from an API where
> > > > > everything works at no-cost from a code-size perspective to a situation
> > > > > where every driver has to develop and maintain that enum.
> > > >
> > > > There are no "traits that come with it". It's just an enum, with two
> > > > variants.
> > > >
> > > > > API where everything works at no-cost
> > > >
> > > > The previous API was far from “everything works”. It was fundamentally
> > > > broken by design in multiple ways, i.e.:
> > >
> > > Out of context and not what I meant, but ok.
> > >
> > > > > a) It only keeps track of a count to clk_get(), which means that users have
> > > > > to manually call disable() and unprepare(), or a variation of those, like
> > > > > disable_unprepare().
> > > > >
> > > > > b) It allows repeated calls to prepare() or enable(), but it keeps no track
> > > > > of how often these were called, i.e., it's currently legal to write the
> > > > > following:
> > > > >
> > > > > clk.prepare();
> > > > > clk.prepare();
> > > > > clk.enable();
> > > > > clk.enable();
> > > > >
> > > > > And nothing gets undone on drop().
> > > >
> > > > IMHO, what we have here is an improvement that has been long overdue.
> > >
> > > Nothing is absolute. It is indeed an improvement on the refcounting side
> > > of things and general safety of the API for the general case. I don't
> > > think I ever questionned that.
> > >
> > > However, for the use-cases we've been discussing (and dozens of drivers
> > > implementing it), it also comes with a regression in the amount of code
> > > to create and maintain. They used to be able to only deal with the Clk
> > > structure, and now they can't anymore.
> > >
> > > You might find that neglible, you might have a plan to address that in
> > > the future, etc. and that's fine, but if you can't acknowledge that it's
> > > indeed happening, there's no point in me raising the issue and
> > > continuing the discussion.
> >
> >
> > Okay, let's see if I can sum-up the use case you'd like to support. You
> > have some PM hooks, which I'm assuming are (or will be) written in
> > rust. It will probably take the form of some Device{Rpm,Pm} trait to
> > implement for your XxxDeviceData (Xxx being the bus under which is
> > device is) object (since I've only recently joined the R4L effort, I
> > wouldn't be surprised if what I'm describing already exists or is
> > currently being proposed/reviewed somewhere, so please excuse my
> > ignorance if that's the case :-)).
> >
> > The way I see it, rather than having one enum per clk/regulator/xxx
> > where we keep track of each state individually, what we could have is a
> >
> > trait DevicePm {
> >       type ResumedState;
> >       type SuspendedState;
> >
> >       fn resume(&self, state: SuspendedState) -> Result<ResumedState, Error<SuspendedState>>;
> >       fn suspend(&self, state: SuspendedState) -> Result<SuspendedState, Error<ResumedState>>;
> > };
> >
> > enum DeviceState<T: DevicePm> {
> >       Resumed(T::ResumedState),
> >       Suspended(T::SuspendedState),
> > };
> >
> > and then in your driver:
> >
> > MySuspendedDeviceResources {
> >       xxx_clk: Clk<Unprepared>,
> > };
> >
> > MyResumedDeviceResources {
> >       xxx_clk: Clk<Enabled>,
> > };
> >
> > implem DevicePm for MyDevice {
> >       type ResumedState = MyResumedDeviceResources;
> >       type SuspendedState = MySuspendedDeviceResources;
> >
> >       fn resume(&self, state: SuspendedState) -> Result<ResumedState, Error<SuspendedState>> {
> >               // FIXME: error propagation not handled
> >               let enabled_clk = state.xxx_clk.clone().prepare()?.enable()?;
> >
> >               Ok(ResumedState {
> >                       xxx_clk: enabled_clk,
> >               });
> >       }
> >
> >       fn suspend(&self, state: ResumedState) -> Result<SuspendedState, Error<ResumedState>> {
> >               // FIXME: error propagation not handled
> >               let unprep_clk = state.xxx_clk.clone().disable().unprepare();
> >
> >               Ok(SuspendedState {
> >                       xxx_clk: unprep_clk,
> >               });
> >       }
> > };
>
> I'm not sure we need to associate this with the suspend/resume state either.
>
> > With this model, I don't think Daniel's refactor goes in the way of more
> > generalization at the core level, it's just expressed differently than
> > it would be if it was written in C. And I say that as someone who struggles
> > with his C developer bias every time I'm looking at or trying to write
> > rust code.
> >
> > As others have said in this thread (Danilo and Gary), and after having
> > played myself with both approaches in Tyr, I do see this shift from manual
> > prepare/enable to an RAII approach as an improvement, so I hope we can
> > find a middle-ground where every one is happy.
>
> I do think we can find a compromise though. Miguel suggested for example
> to make the current enable/prepare/disable/unprepare function unsafe,
> and that's totally reasonable to me.
>
> Then we can implement the "managed" clock version on that unsafe API,
> and we would end up with a "raw", unsafe, version kind of equivalent to
> the one we have today, and where callers would have to justify why their
> usage of the API is actually safe, or the new, managed, variant that is
> safe and can be easily used by most drivers.
>
> And we can call these RawClk vs Clk, or Clk vs ManagedClk, or whatever.
>
> How does that sound?

If you make the raw API unsafe, then that's okay but any use of an
unsafe API from a driver will receive very hard scrutiny. Yes, there
are occasionally good reasons to use unsafe from drivers, but the
entire point of this Rust exercise is to isolate unsafe code outside
of drivers as much as possible.

If Daniel's proposal is inconvenient for some drivers, it would be far
better to have a third API that is both safe and convenient for those
drivers.

Alice

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Alice Ryhl]

On Thu, Feb 12, 2026 at 8:59 AM Maxime Ripard <mripard@kernel.org> wrote:
> It works *today*.

The code we have today is both safe and unsound. That is a bug, and it
*must* be fixed.

> And the "oh but driver is using the API" is kind of ironic in the
> context of the Rust bindings which have globally been in that situation
> for years. You can't argue it both ways.

I don't really know what is meant by this. This API is for real Rust
drivers that have already started landing upstream. Sometimes we merge
APIs *before* their user lands, but not without a user. We generally
are quite consistent that Rust APIs must have a real Rust driver that
uses it. It's the usual no-dead-code rule in the kernel.

> Either way, I'm not sure what the point of that submission was if you
> will just dismiss diverging opinions, including attempts to compromise.

It is true that we are unwilling to compromise on the requirement that
APIs must be sound. It really does not matter how convenient the API
is when it's this easy to get it wrong and decrement the refcount when
you did not increment it. I'm sorry, but that's the way it is.

In your latest email, the suggestion came up to make the API unsafe.
That's more acceptable, but see my other email from half an hour ago.

Alice

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Thu Feb 12, 2026 at 8:59 AM CET, Maxime Ripard wrote:
> On Wed, Feb 11, 2026 at 05:47:09PM +0100, Danilo Krummrich wrote:
>> On Wed Feb 11, 2026 at 5:37 PM CET, Maxime Ripard wrote:
>> > I do think we can find a compromise though. Miguel suggested for example
>> > to make the current enable/prepare/disable/unprepare function unsafe,
>> > and that's totally reasonable to me.
>> >
>> > Then we can implement the "managed" clock version on that unsafe API,
>> 
>> What do you mean with "managed" clock? Do you mean devres managed? If so, I
>> don't think there is any reason to switch to the unsafe API to be able to
>> implement devres managed APIs (see also [1]).
>> 
>> [1] https://lore.kernel.org/all/DFVW9MS5YLON.CVJDBYQKJ0P6@kernel.org/
>
> By that, I mean what Daniel has been proposing to achieve with this series.
>
>> > and we would end up with a "raw", unsafe, version kind of equivalent to
>> > the one we have today, and where callers would have to justify why their
>> > usage of the API is actually safe, or the new, managed, variant that is
>> > safe and can be easily used by most drivers.
>> >
>> > And we can call these RawClk vs Clk, or Clk vs ManagedClk, or whatever.
>> >
>> > How does that sound?
>> 
>> What about we just wait until we have a user that really requires an unsafe API
>> for some reason? And if it never appears, even better. :)
>
> It works *today*.
>
> And the "oh but driver is using the API" is kind of ironic in the
> context of the Rust bindings which have globally been in that situation
> for years. You can't argue it both ways.

I can't remember ever advocating for merging code that does not have at least a
user in prospect.

> Either way, I'm not sure what the point of that submission was if you
> will just dismiss diverging opinions, including attempts to compromise.

Sorry -- I'm a bit confused here, since I did not submit this code.

I'm also not dismissing your opinion; I just have a different one.

In particular, I don't think we need an unsafe API until we see a concrete
example where the proposed safe API does not work (and no other safe API would
work either).

Framing a difference in opinion as "dismissing diverging opinions" doesn't feel
fair to me.

> Do whatever you want, but it's really hard to root for you some times.

I'm starting to wonder if the mail is addressed to me in the first place.

Thanks,
Danilo

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Miguel Ojeda]

On Thu, Feb 12, 2026 at 8:59 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> And the "oh but driver is using the API" is kind of ironic in the
> context of the Rust bindings which have globally been in that situation
> for years. You can't argue it both ways.

I don't think this is true, if I understand you correctly.

To give some context, when Rust was close to land in mainline, we were
explicitly asked to cut down what we had out-of-tree to a minimum of
APIs, which was fair. So I did exactly that, to a point where you
could essentially only declare a module for x86_64 and use `pr_info!`
in it (pretty much). I even considered dropping the `pr_info!` too...
:)

Then we started upstreaming bits piece by piece, getting agreement and
input from the relevant maintainers for each bit, and so on, which
took years for several reasons, one of which was precisely waiting for
actual users for APIs.

[ Others were getting maintainers engaged and involved for some of the
core APIs/subsystems, getting authors of the original code to submit
it upstream via patches was yet another, reworking and cleaning the
code with the new, better approaches we found meanwhile (like
pin-init), and so on and so forth. ]

Cheers,
Miguel

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

Hi Alice,

On Thu, Feb 12, 2026 at 09:16:51AM +0100, Alice Ryhl wrote:
> On Wed, Feb 11, 2026 at 5:37 PM Maxime Ripard <mripard@kernel.org> wrote:
> > On Mon, Feb 09, 2026 at 10:50:47AM +0100, Boris Brezillon wrote:
> > > On Wed, 4 Feb 2026 15:34:29 +0100
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > > On Wed, Feb 04, 2026 at 09:43:55AM -0300, Daniel Almeida wrote:
> > > > > > I'm probably missing something then, but let's assume you have a driver
> > > > > > that wants its clock prepared and enabled in an hypothetical enable()
> > > > > > callback, and disabled / unprepared in a disable() callback.
> > > > > >
> > > > > > From a PM management perspective, this usecase makes total sense, is a
> > > > > > valid usecase, is widely used in the kernel, and is currently supported
> > > > > > by both the C and Rust clk APIs.
> > > > > >
> > > > > > The only solution to this you suggested so far (I think?) to implement
> > > > > > this on top of the new clk API you propose is to have a driver specific
> > > > > > enum that would store each of the possible state transition.
> > > > >
> > > > > Yes, you need an enum _if_ you want to model transitions at runtime. IIUC you
> > > > > only need two variants to implement the pattern you described. I do not
> > > > > consider this  “boilerplate”, but rather a small cost to pay.
> > > >
> > > > A maintenance cost to pay by every driver is kind of the textbook
> > > > definition of boilerplate to me.
> > > >
> > > > > I would understand if this was some elaborate pattern that had to be
> > > > > implemented by all drivers, but a two-variant enum is as
> > > > > straightforward as it gets.
> > > >
> > > > And yet, that framework has dozens of helpers that do not remove
> > > > anything from drivers but a couple of lines. So surely its users must
> > > > find value in reducing that boilerplate as much as possible. And you do
> > > > implement some of them, so you must find value in that too.
> > > >
> > > > > > That's the boilerplate I'm talking about. If every driver wanting to
> > > > > > implement that pattern has to make such an enum, with all the relevant
> > > > > > traits implementation that might come with it, we go from an API where
> > > > > > everything works at no-cost from a code-size perspective to a situation
> > > > > > where every driver has to develop and maintain that enum.
> > > > >
> > > > > There are no "traits that come with it". It's just an enum, with two
> > > > > variants.
> > > > >
> > > > > > API where everything works at no-cost
> > > > >
> > > > > The previous API was far from “everything works”. It was fundamentally
> > > > > broken by design in multiple ways, i.e.:
> > > >
> > > > Out of context and not what I meant, but ok.
> > > >
> > > > > > a) It only keeps track of a count to clk_get(), which means that users have
> > > > > > to manually call disable() and unprepare(), or a variation of those, like
> > > > > > disable_unprepare().
> > > > > >
> > > > > > b) It allows repeated calls to prepare() or enable(), but it keeps no track
> > > > > > of how often these were called, i.e., it's currently legal to write the
> > > > > > following:
> > > > > >
> > > > > > clk.prepare();
> > > > > > clk.prepare();
> > > > > > clk.enable();
> > > > > > clk.enable();
> > > > > >
> > > > > > And nothing gets undone on drop().
> > > > >
> > > > > IMHO, what we have here is an improvement that has been long overdue.
> > > >
> > > > Nothing is absolute. It is indeed an improvement on the refcounting side
> > > > of things and general safety of the API for the general case. I don't
> > > > think I ever questionned that.
> > > >
> > > > However, for the use-cases we've been discussing (and dozens of drivers
> > > > implementing it), it also comes with a regression in the amount of code
> > > > to create and maintain. They used to be able to only deal with the Clk
> > > > structure, and now they can't anymore.
> > > >
> > > > You might find that neglible, you might have a plan to address that in
> > > > the future, etc. and that's fine, but if you can't acknowledge that it's
> > > > indeed happening, there's no point in me raising the issue and
> > > > continuing the discussion.
> > >
> > >
> > > Okay, let's see if I can sum-up the use case you'd like to support. You
> > > have some PM hooks, which I'm assuming are (or will be) written in
> > > rust. It will probably take the form of some Device{Rpm,Pm} trait to
> > > implement for your XxxDeviceData (Xxx being the bus under which is
> > > device is) object (since I've only recently joined the R4L effort, I
> > > wouldn't be surprised if what I'm describing already exists or is
> > > currently being proposed/reviewed somewhere, so please excuse my
> > > ignorance if that's the case :-)).
> > >
> > > The way I see it, rather than having one enum per clk/regulator/xxx
> > > where we keep track of each state individually, what we could have is a
> > >
> > > trait DevicePm {
> > >       type ResumedState;
> > >       type SuspendedState;
> > >
> > >       fn resume(&self, state: SuspendedState) -> Result<ResumedState, Error<SuspendedState>>;
> > >       fn suspend(&self, state: SuspendedState) -> Result<SuspendedState, Error<ResumedState>>;
> > > };
> > >
> > > enum DeviceState<T: DevicePm> {
> > >       Resumed(T::ResumedState),
> > >       Suspended(T::SuspendedState),
> > > };
> > >
> > > and then in your driver:
> > >
> > > MySuspendedDeviceResources {
> > >       xxx_clk: Clk<Unprepared>,
> > > };
> > >
> > > MyResumedDeviceResources {
> > >       xxx_clk: Clk<Enabled>,
> > > };
> > >
> > > implem DevicePm for MyDevice {
> > >       type ResumedState = MyResumedDeviceResources;
> > >       type SuspendedState = MySuspendedDeviceResources;
> > >
> > >       fn resume(&self, state: SuspendedState) -> Result<ResumedState, Error<SuspendedState>> {
> > >               // FIXME: error propagation not handled
> > >               let enabled_clk = state.xxx_clk.clone().prepare()?.enable()?;
> > >
> > >               Ok(ResumedState {
> > >                       xxx_clk: enabled_clk,
> > >               });
> > >       }
> > >
> > >       fn suspend(&self, state: ResumedState) -> Result<SuspendedState, Error<ResumedState>> {
> > >               // FIXME: error propagation not handled
> > >               let unprep_clk = state.xxx_clk.clone().disable().unprepare();
> > >
> > >               Ok(SuspendedState {
> > >                       xxx_clk: unprep_clk,
> > >               });
> > >       }
> > > };
> >
> > I'm not sure we need to associate this with the suspend/resume state either.
> >
> > > With this model, I don't think Daniel's refactor goes in the way of more
> > > generalization at the core level, it's just expressed differently than
> > > it would be if it was written in C. And I say that as someone who struggles
> > > with his C developer bias every time I'm looking at or trying to write
> > > rust code.
> > >
> > > As others have said in this thread (Danilo and Gary), and after having
> > > played myself with both approaches in Tyr, I do see this shift from manual
> > > prepare/enable to an RAII approach as an improvement, so I hope we can
> > > find a middle-ground where every one is happy.
> >
> > I do think we can find a compromise though. Miguel suggested for example
> > to make the current enable/prepare/disable/unprepare function unsafe,
> > and that's totally reasonable to me.
> >
> > Then we can implement the "managed" clock version on that unsafe API,
> > and we would end up with a "raw", unsafe, version kind of equivalent to
> > the one we have today, and where callers would have to justify why their
> > usage of the API is actually safe, or the new, managed, variant that is
> > safe and can be easily used by most drivers.
> >
> > And we can call these RawClk vs Clk, or Clk vs ManagedClk, or whatever.
> >
> > How does that sound?
> 
> If you make the raw API unsafe, then that's okay but any use of an
> unsafe API from a driver will receive very hard scrutiny.

And that's totally fair to me. If drivers want to have a more optimal
but potentially unsafe use of the API, then it should be flagged,
documented and scrutinized.

> Yes, there are occasionally good reasons to use unsafe from drivers,
> but the entire point of this Rust exercise is to isolate unsafe code
> outside of drivers as much as possible.

So, aside from the risk of fuckups, are you concerned about something
here? Do you expect that scrutiny to be constly on the maintenance side
of things?

I guess what I'm asking is: except for the obvious "review" cost
associated with it, why would that be a probleme if we have, say, a few
dozen drivers using that unsafe API?

> If Daniel's proposal is inconvenient for some drivers, it would be far
> better to have a third API that is both safe and convenient for those
> drivers.

Yeah, I guess I was expecting that one to come after we have a few
unsafe drivers using it and we do need to consolidate / make it safe.

Anyway, thanks for your answer,
Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Thu Feb 12, 2026 at 10:23 AM CET, Danilo Krummrich wrote:
> In particular, I don't think we need an unsafe API until we see a concrete
> example where the proposed safe API does not work (and no other safe API would
> work either).

One additional note for clarification, I'm not saying that I'm against an unsafe
API if it is necessary.

In fact, I have added an unsafe API for drivers myself with the dma::Device
trait [1]. This was not an easy decision and I discussed this back and forth
with a lot of people and, unfortunately, we had to come to the conclusion that
any attempt to make this safe from the Rust side of things would cause
unreasonable overhead and can't be considered an overall improvement.

But as I said, I am convinced that need clear evidence that an unsafe API is
actually needed and that there is no reasonable alternative.

[1] https://rust.docs.kernel.org/kernel/dma/trait.Device.html

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Alice Ryhl]

On Thu, Feb 12, 2026 at 2:38 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi Alice,
>
> On Thu, Feb 12, 2026 at 09:16:51AM +0100, Alice Ryhl wrote:
> > If you make the raw API unsafe, then that's okay but any use of an
> > unsafe API from a driver will receive very hard scrutiny.
>
> And that's totally fair to me. If drivers want to have a more optimal
> but potentially unsafe use of the API, then it should be flagged,
> documented and scrutinized.
>
> > Yes, there are occasionally good reasons to use unsafe from drivers,
> > but the entire point of this Rust exercise is to isolate unsafe code
> > outside of drivers as much as possible.
>
> So, aside from the risk of fuckups, are you concerned about something
> here? Do you expect that scrutiny to be constly on the maintenance side
> of things?

Well I do think it's mostly the risk of mistakes, yes.

> I guess what I'm asking is: except for the obvious "review" cost
> associated with it, why would that be a probleme if we have, say, a few
> dozen drivers using that unsafe API?

The reason I wouldn't really suggest the unsafe API is that, if I'm
the reviewer of said driver, then my review will be "just use a safe
API instead".

For example, if you really want explicit inc/dec calls for
convenience, we can make a safe API with a counter that returns an
error if you decrement more times than you have previously
incremented. Or if you prefer something else, I can think of several
other safe APIs.

> > If Daniel's proposal is inconvenient for some drivers, it would be far
> > better to have a third API that is both safe and convenient for those
> > drivers.
>
> Yeah, I guess I was expecting that one to come after we have a few
> unsafe drivers using it and we do need to consolidate / make it safe.

For most things, we implemented and used the safe API from the very
start. I'm not going to say there are never exceptions to that because
there *are* exceptions. But I don't really see why clk should be one
of them.

Alice

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Thu, Feb 12, 2026 at 03:02:09PM +0100, Alice Ryhl wrote:
> On Thu, Feb 12, 2026 at 2:38 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > If Daniel's proposal is inconvenient for some drivers, it would be far
> > > better to have a third API that is both safe and convenient for those
> > > drivers.
> >
> > Yeah, I guess I was expecting that one to come after we have a few
> > unsafe drivers using it and we do need to consolidate / make it safe.
> 
> For most things, we implemented and used the safe API from the very
> start. I'm not going to say there are never exceptions to that because
> there *are* exceptions. But I don't really see why clk should be one
> of them.

Ack. As long as we keep the door open for such an API, I'm ok with it then.

Maxime

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

Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

[Maxime Ripard]

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

On Thu, Feb 12, 2026 at 03:01:31PM +0100, Danilo Krummrich wrote:
> On Thu Feb 12, 2026 at 10:23 AM CET, Danilo Krummrich wrote:
> > In particular, I don't think we need an unsafe API until we see a concrete
> > example where the proposed safe API does not work (and no other safe API would
> > work either).
> 
> One additional note for clarification, I'm not saying that I'm against an unsafe
> API if it is necessary.
> 
> In fact, I have added an unsafe API for drivers myself with the dma::Device
> trait [1]. This was not an easy decision and I discussed this back and forth
> with a lot of people and, unfortunately, we had to come to the conclusion that
> any attempt to make this safe from the Rust side of things would cause
> unreasonable overhead and can't be considered an overall improvement.
> 
> But as I said, I am convinced that need clear evidence that an unsafe API is
> actually needed and that there is no reasonable alternative.
> 
> [1] https://rust.docs.kernel.org/kernel/dma/trait.Device.html

Ok, it works for me then.

Maxime

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