[PATCH] rust: clk: use the type-state pattern

[PATCH] 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.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
Hi Michael, Stephen, Viresh and others. I believe that we should get this
change in before more drivers start depending on clocks. It uses the same
rationale as Regulator<T>, and that was the result of some extensive
discussion, so I really believe that it is a good fit for clocks as well,
in the sense that it correctly tracks all refcounts, decrement them on drop
as needed and also provides a guarantee on the underlying state of the
clock via the type system. For instance, it is now possible to express via
the type system that a Clk is enabled, i.e.: Clk<Enabled>.

On top of that, it can constrain operations to a given state. For instance,
the current patch only offers get_rate() and set_rate() for the
Clk<Enabled> state. We can also offer it to all states, or a subset of
states as appropriate.
---
 drivers/cpufreq/rcpufreq_dt.rs |   2 +-
 rust/kernel/clk.rs             | 358 ++++++++++++++++++++++++++---------------
 rust/kernel/cpufreq.rs         |   8 +-
 3 files changed, 232 insertions(+), 136 deletions(-)

diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index 94ed81644fe1c3f8a0240dede2299102a2118e73..f5dee7aba95c5c150fe75d6e2ffc2adf61c41c60 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -45,7 +45,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/rust/kernel/clk.rs b/rust/kernel/clk.rs
index fbcea31dbccadc29ae1db12cd77beda67a5665ee..6c8edb665c608386a36f28a2c9ec39604b5bc20d 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -85,12 +85,73 @@ mod common_clk {
         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`].
     ///
-    /// ## Examples
+    /// 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 [`Enabled`] 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::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
     /// use kernel::device::Device;
     /// use kernel::error::Result;
     ///
     /// fn configure_clk(dev: &Device) -> Result {
-    ///     let clk = Clk::get(dev, Some(c_str!("apb_clk")))?;
+    ///     // 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,104 +200,172 @@ 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(dev: &Device, 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
+    ///     })?;
+    ///
+    ///     let clk: Clk<Unprepared> = 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>,
+    }
 
-    impl Clk {
+    impl Clk<Unprepared> {
         /// Gets [`Clk`] corresponding to a [`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, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
             let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
 
             // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
-            //
+            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, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
+            let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
+
+            // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
+            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 [`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, name: Option<&CStr>) -> Result<Clk<Prepared>> {
+            Clk::<Unprepared>::get(dev, name)?
+                .prepare()
+                .map_err(|error| error.error)
         }
 
-        /// Prepare the clock.
+        /// Attempts to convert the [`Clk`] to an [`Unprepared`] state.
         ///
-        /// Equivalent to the kernel's [`clk_prepare`] API.
-        ///
-        /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
+        /// Equivalent to the kernel's [`clk_unprepare`] API.
         #[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 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(clk.as_raw()) }
+
+            Clk {
+                inner: clk.inner,
+                _phantom: PhantomData,
+            }
         }
 
-        /// Unprepare the clock.
+        /// Attempts to convert the [`Clk`] to an [`Enabled`] state.
         ///
-        /// Equivalent to the kernel's [`clk_unprepare`] API.
+        /// Equivalent to the kernel's [`clk_enable`] API.
         ///
-        /// [`clk_unprepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_unprepare
+        /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
         #[inline]
-        pub fn unprepare(&self) {
-            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
-            // [`clk_unprepare`].
-            unsafe { bindings::clk_unprepare(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),
+                })
         }
+    }
 
-        /// Prepare and enable the clock.
+    impl Clk<Enabled> {
+        /// Gets [`Clk`] corresponding to a [`Device`] and a connection id and
+        /// then prepares and enables it.
         ///
-        /// Equivalent to calling [`Clk::prepare`] followed by [`Clk::enable`].
+        /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
+        /// followed by [`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 get(dev: &Device, name: Option<&CStr>) -> Result<Clk<Enabled>> {
+            Clk::<Prepared>::get(dev, name)?
+                .enable()
+                .map_err(|error| error.error)
         }
 
-        /// Disable and unprepare the clock.
-        ///
-        /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
         #[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()) };
+        /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]
+        /// state.
+        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.
@@ -245,83 +393,27 @@ 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_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;
-
-        fn deref(&self) -> &Clk {
-            &self.0
+    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()) };
+            }
         }
     }
 }
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index b0a9c6182aec6027f85d65a492595e8f815ae6b9..8eef0ffeb60276951737726af3c43c04d7f8dff3 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -551,8 +551,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,
+        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)
     }

---
base-commit: bc27a30feb4932493af841a14fe80d752e049f95
change-id: 20250729-clk-type-state-1f988ec3843a

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

Re: [PATCH] rust: clk: use the type-state pattern

[Viresh Kumar]

On 29-07-25, 18:38, Daniel Almeida wrote:
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
>      /// 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`].
>      ///
> -    /// ## Examples
> +    /// 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 [`Enabled`] state is associated with a
> +    /// single count of `clk_prepare` and `clk_enable()`.

You have mentioned the `Enabled` state twice. Also clk_prepare() ?

No objections from my side. Thanks.

-- 
viresh

Re: [PATCH] rust: clk: use the type-state pattern

[Daniel Sedlak]

Hi Daniel,

On 7/29/25 11:38 PM, Daniel Almeida wrote:
> +    mod private {
> +        pub trait Sealed {}
> +
> +        impl Sealed for super::Unprepared {}
> +        impl Sealed for super::Prepared {}
> +        impl Sealed for super::Enabled {}
> +    }

I just noticed we have plenty of Sealed traits scattered across rust/ 
folder. Do you think we would benefit from unifying it to a single 
location to prevent duplication?

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

I would put a private member into the structs so the user of this API 
cannot construct it themself without using your abstractions.

	pub struct Unprepared(());
	pub struct Prepared(());
	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> {

Nit: IMO we mostly use the `where` variant instead of the colon.

	pub struct Error<State>
	where State: ClkState

But does it make sense to put the bounds on the structs? Shouldn't be 
enough but but the bounds on the impl block?

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

Thanks!
Daniel

Re: [PATCH] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Tue Jul 29, 2025 at 11:38 PM CEST, Daniel Almeida wrote:
> 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.

That's a great improvement, thanks! Some questions / comments below.

>      /// 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`].
>      ///
> -    /// ## Examples
> +    /// 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 [`Enabled`] 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::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
>      /// use kernel::device::Device;
>      /// use kernel::error::Result;
>      ///
>      /// fn configure_clk(dev: &Device) -> Result {
> -    ///     let clk = Clk::get(dev, Some(c_str!("apb_clk")))?;
> +    ///     // 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")))?;

Given that this is a driver API, why do we allow obtaining and configuring
clocks of any device, i.e. also unbound devices?

I think Clk::<T>::get() should take a &Device<Bound> instead.

> -    ///     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,104 +200,172 @@ 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(dev: &Device, clk: Clk<Enabled>) -> Result {

You don't need the dev argument here.

> +    ///     // The states can be traversed "in the reverse order" as well:
> +    ///     let clk: Clk<Prepared> = clk.disable().map_err(|error| {
> +    ///         error.error
> +    ///     })?;
> +    ///
> +    ///     let clk: Clk<Unprepared> = clk.unprepare();

I know you want to showcase the type state, yet I don't know if we should
explicitly declare the type if not necessary. People will likely just copy
things. Maybe a comment is better to emphasize it?

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

<snip>

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

Nice! I like this cleanup. However, don't you still need to call clk_put() to
drop the reference count?

Also, given that this is a device resource, don't we want to take it away from
drivers once the corresponding device has been unbound, i.e. use Devres?

>          }
>      }
>  }

Re: [PATCH] rust: clk: use the type-state pattern

[Benno Lossin]

On Wed Jul 30, 2025 at 9:29 AM CEST, Daniel Sedlak wrote:
> On 7/29/25 11:38 PM, Daniel Almeida wrote:
>> +    mod private {
>> +        pub trait Sealed {}
>> +
>> +        impl Sealed for super::Unprepared {}
>> +        impl Sealed for super::Prepared {}
>> +        impl Sealed for super::Enabled {}
>> +    }
>
> I just noticed we have plenty of Sealed traits scattered across rust/ 
> folder. Do you think we would benefit from unifying it to a single 
> location to prevent duplication?

I don't think we can merge the various `Sealed` traits we have. They are
implemented for different types and ensure that only those selected
types are allowed when `T: Sealed`. If we merge them, then that is no
longer the case.

We essentially would like to have a `#[sealed]` attribute that we can
put on a trait to avoid the `mod private { pub trait Sealed }` dance.
(so a trait that cannot be implemented outside of the module declaring
it)

---
Cheers,
Benno

Re: [PATCH] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Wed Jul 30, 2025 at 9:29 AM CEST, Daniel Sedlak wrote:
> On 7/29/25 11:38 PM, Daniel Almeida wrote:
>> +    /// An error that can occur when trying to convert a [`Clk`] between states.
>> +    pub struct Error<State: ClkState> {
>
> Nit: IMO we mostly use the `where` variant instead of the colon.
>
> 	pub struct Error<State>
> 	where State: ClkState

For such simple struct definitions with just a single trait bound it's not very
common to use a where clause. I think the definition is fine as it is.

Re: [PATCH] rust: clk: use the type-state pattern

[Daniel Almeida]

> On 30 Jul 2025, at 05:03, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Tue Jul 29, 2025 at 11:38 PM CEST, Daniel Almeida wrote:
>> 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.
> 
> That's a great improvement, thanks! Some questions / comments below.
> 
>>     /// 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`].
>>     ///
>> -    /// ## Examples
>> +    /// 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 [`Enabled`] 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::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
>>     /// use kernel::device::Device;
>>     /// use kernel::error::Result;
>>     ///
>>     /// fn configure_clk(dev: &Device) -> Result {
>> -    ///     let clk = Clk::get(dev, Some(c_str!("apb_clk")))?;
>> +    ///     // 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")))?;
> 
> Given that this is a driver API, why do we allow obtaining and configuring
> clocks of any device, i.e. also unbound devices?
> 
> I think Clk::<T>::get() should take a &Device<Bound> instead.

Ah, this was a question I had, but that I forgot to mention here.

The same can probably be said of the regulator series? i.e.:

impl Regulator<Disabled> {
    /// Obtains a [`Regulator`] instance from the system.
    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
        Regulator::get_internal(dev, name)
    }

> 
>> -    ///     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,104 +200,172 @@ 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(dev: &Device, clk: Clk<Enabled>) -> Result {
> 
> You don't need the dev argument here.
> 
>> +    ///     // The states can be traversed "in the reverse order" as well:
>> +    ///     let clk: Clk<Prepared> = clk.disable().map_err(|error| {
>> +    ///         error.error
>> +    ///     })?;
>> +    ///
>> +    ///     let clk: Clk<Unprepared> = clk.unprepare();
> 
> I know you want to showcase the type state, yet I don't know if we should
> explicitly declare the type if not necessary. People will likely just copy
> things. Maybe a comment is better to emphasize it?

Ok

> 
>> +    ///
>>     ///     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>,
>> +    }
> 
> <snip>
> 
>> +    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()) };
>> +            }
> 
> Nice! I like this cleanup. However, don't you still need to call clk_put() to
> drop the reference count?

Right, clk_put() was totally forgotten.

> 
> Also, given that this is a device resource, don't we want to take it away from
> drivers once the corresponding device has been unbound, i.e. use Devres?

Do you mean to have the get() functions return Devres<Clk>?

Also, is this applicable for Regulator as well?

> 
>>         }
>>     }
>> }
> 

— Daniel

Re: [PATCH] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Wed Jul 30, 2025 at 2:13 PM CEST, Daniel Almeida wrote:
>> On 30 Jul 2025, at 05:03, Danilo Krummrich <dakr@kernel.org> wrote:
>> On Tue Jul 29, 2025 at 11:38 PM CEST, Daniel Almeida wrote:
>>>     /// fn configure_clk(dev: &Device) -> Result {
>>> -    ///     let clk = Clk::get(dev, Some(c_str!("apb_clk")))?;
>>> +    ///     // 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")))?;
>> 
>> Given that this is a driver API, why do we allow obtaining and configuring
>> clocks of any device, i.e. also unbound devices?
>> 
>> I think Clk::<T>::get() should take a &Device<Bound> instead.
>
> Ah, this was a question I had, but that I forgot to mention here.
>
> The same can probably be said of the regulator series? i.e.:
>
> impl Regulator<Disabled> {
>     /// Obtains a [`Regulator`] instance from the system.
>     pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
>         Regulator::get_internal(dev, name)
>     }

Yes, that's a device resource as well. We should only give it out to drivers
when they're actually bound to the device.

>> 
>>> -    ///     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,104 +200,172 @@ 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(dev: &Device, clk: Clk<Enabled>) -> Result {
>> 
>> You don't need the dev argument here.
>> 
>>> +    ///     // The states can be traversed "in the reverse order" as well:
>>> +    ///     let clk: Clk<Prepared> = clk.disable().map_err(|error| {
>>> +    ///         error.error
>>> +    ///     })?;
>>> +    ///
>>> +    ///     let clk: Clk<Unprepared> = clk.unprepare();
>> 
>> I know you want to showcase the type state, yet I don't know if we should
>> explicitly declare the type if not necessary. People will likely just copy
>> things. Maybe a comment is better to emphasize it?
>
> Ok
>
>> 
>>> +    ///
>>>     ///     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>,
>>> +    }
>> 
>> <snip>
>> 
>>> +    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()) };
>>> +            }
>> 
>> Nice! I like this cleanup. However, don't you still need to call clk_put() to
>> drop the reference count?
>
> Right, clk_put() was totally forgotten.
>
>> 
>> Also, given that this is a device resource, don't we want to take it away from
>> drivers once the corresponding device has been unbound, i.e. use Devres?
>
> Do you mean to have the get() functions return Devres<Clk>?
>
> Also, is this applicable for Regulator as well?

Yes, drivers shouldn't be able to mess with device specific resources once they
are unbound from the device.

Re: [PATCH] rust: clk: use the type-state pattern

[Daniel Almeida]

Hi Daniel,

> On 30 Jul 2025, at 04:29, Daniel Sedlak <daniel@sedlak.dev> wrote:
> 
> Hi Daniel,
> 
> On 7/29/25 11:38 PM, Daniel Almeida wrote:
>> +    mod private {
>> +        pub trait Sealed {}
>> +
>> +        impl Sealed for super::Unprepared {}
>> +        impl Sealed for super::Prepared {}
>> +        impl Sealed for super::Enabled {}
>> +    }
> 
> I just noticed we have plenty of Sealed traits scattered across rust/ folder. Do you think we would benefit from unifying it to a single location to prevent duplication?

Benno replied to this below.

> 
>> +
>> +    /// 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;
> 
> I would put a private member into the structs so the user of this API cannot construct it themself without using your abstractions.
> 
> pub struct Unprepared(());
> pub struct Prepared(());
> pub struct Enabled(());


I don’t think it’s a problem if they construct these. They can
construct e.g.: Unprepared, Prepared, etc, but not Clk<Unprepared>, or
Clk<Prepared> and others without going through the right API.

> 
>> +
>> +    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> {
> 
> Nit: IMO we mostly use the `where` variant instead of the colon.
> 
> pub struct Error<State>
> where State: ClkState
> 
> But does it make sense to put the bounds on the structs? Shouldn't be enough but but the bounds on the impl block?

This does have some implications in general, but in practical terms, I don’t think it
matters here.

Also, the bound on Clk<T> is somewhat warranted even though we could probably
get away with having it only on the impl block. It correctly encodes that a
Clk<T> should only _ever_ exist if T is one of the states.

From the above, it follows that Error<T> also needs the bound on the struct,
otherwise this will likely not compile.

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

Re: [PATCH] rust: clk: use the type-state pattern

[Daniel Almeida]

Hi Viresh,

> On 30 Jul 2025, at 03:23, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> On 29-07-25, 18:38, Daniel Almeida wrote:
>> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
>>     /// 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`].
>>     ///
>> -    /// ## Examples
>> +    /// 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 [`Enabled`] state is associated with a
>> +    /// single count of `clk_prepare` and `clk_enable()`.
> 
> You have mentioned the `Enabled` state twice. Also clk_prepare() ?

Ah, thanks for catching that.

What’s the issue with clk_prepare? You mean that the parenthesis are missing?

> 
> No objections from my side. Thanks.
> 
> -- 
> viresh

— Daniel

Re: [PATCH] rust: clk: use the type-state pattern

[Daniel Almeida]

[…]

> We essentially would like to have a `#[sealed]` attribute that we can
> put on a trait to avoid the `mod private { pub trait Sealed }` dance.
> (so a trait that cannot be implemented outside of the module declaring
> it)
> 
> ---
> Cheers,
> Benno

This is not exactly what you said, but how about a declarative macro? e.g.:

macro_rules! sealed {
    ($($ty:ident),* $(,)?) => {
        mod private {
            pub trait Sealed {}
            $(impl Sealed for super::$ty {})*
        }
        use private::Sealed;
    };
}

sealed!(Unprepared, Prepared, Enabled)

Note that I am just brainstorming the general idea here, I did not test it yet.

— Daniel

Re: [PATCH] rust: clk: use the type-state pattern

[Daniel Sedlak]

On 7/30/25 2:59 PM, Daniel Almeida wrote:
> […]
> 
>> We essentially would like to have a `#[sealed]` attribute that we can
>> put on a trait to avoid the `mod private { pub trait Sealed }` dance.
>> (so a trait that cannot be implemented outside of the module declaring
>> it)
>>
>> ---
>> Cheers,
>> Benno
> 
> This is not exactly what you said, but how about a declarative macro? e.g.:
> 
> macro_rules! sealed {
>      ($($ty:ident),* $(,)?) => {
>          mod private {
>              pub trait Sealed {}
>              $(impl Sealed for super::$ty {})*
>          }
>          use private::Sealed;
>      };
> }
> 
> sealed!(Unprepared, Prepared, Enabled)
> 
> Note that I am just brainstorming the general idea here, I did not test it yet.

I think that API-wise it would be better to have a proc-macro #[sealed], 
something similar to [1], as it may provide better error messages, when 
used incorrectly. So the outcome could look like.

	#[sealed]
	pub trait ClkState {
	…
	}

And then

	#[sealed]
	impl ClkState for XXX {
	…
	}

If you are interested, I can try to look into that.

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

Thanks!
Daniel

Re: [PATCH] rust: clk: use the type-state pattern

[Daniel Almeida]

> On 30 Jul 2025, at 10:27, Daniel Sedlak <daniel@sedlak.dev> wrote:
> 
> 
> On 7/30/25 2:59 PM, Daniel Almeida wrote:
>> […]
>>> We essentially would like to have a `#[sealed]` attribute that we can
>>> put on a trait to avoid the `mod private { pub trait Sealed }` dance.
>>> (so a trait that cannot be implemented outside of the module declaring
>>> it)
>>> 
>>> ---
>>> Cheers,
>>> Benno
>> This is not exactly what you said, but how about a declarative macro? e.g.:
>> macro_rules! sealed {
>>     ($($ty:ident),* $(,)?) => {
>>         mod private {
>>             pub trait Sealed {}
>>             $(impl Sealed for super::$ty {})*
>>         }
>>         use private::Sealed;
>>     };
>> }
>> sealed!(Unprepared, Prepared, Enabled)
>> Note that I am just brainstorming the general idea here, I did not test it yet.
> 
> I think that API-wise it would be better to have a proc-macro #[sealed], something similar to [1], as it may provide better error messages, when used incorrectly. So the outcome could look like.
> 
> #[sealed]
> pub trait ClkState {
> …
> }
> 
> And then
> 
> #[sealed]
> impl ClkState for XXX {
> …
> }
> 
> If you are interested, I can try to look into that.
> 
> Link: https://crates.io/crates/sealed [1]
> 
> Thanks!
> Daniel

If you have the bandwidth, that would be nice indeed! :)

— Daniel

Re: [PATCH] rust: clk: use the type-state pattern

[Alice Ryhl]

On Wed, Jul 30, 2025 at 3:27 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
>
> On 7/30/25 2:59 PM, Daniel Almeida wrote:
> > […]
> >
> >> We essentially would like to have a `#[sealed]` attribute that we can
> >> put on a trait to avoid the `mod private { pub trait Sealed }` dance.
> >> (so a trait that cannot be implemented outside of the module declaring
> >> it)
> >>
> >> ---
> >> Cheers,
> >> Benno
> >
> > This is not exactly what you said, but how about a declarative macro? e.g.:
> >
> > macro_rules! sealed {
> >      ($($ty:ident),* $(,)?) => {
> >          mod private {
> >              pub trait Sealed {}
> >              $(impl Sealed for super::$ty {})*
> >          }
> >          use private::Sealed;
> >      };
> > }
> >
> > sealed!(Unprepared, Prepared, Enabled)
> >
> > Note that I am just brainstorming the general idea here, I did not test it yet.
>
> I think that API-wise it would be better to have a proc-macro #[sealed],
> something similar to [1], as it may provide better error messages, when
> used incorrectly. So the outcome could look like.
>
>         #[sealed]
>         pub trait ClkState {
>         …
>         }
>
> And then
>
>         #[sealed]
>         impl ClkState for XXX {
>         …
>         }
>
> If you are interested, I can try to look into that.
>
> Link: https://crates.io/crates/sealed [1]

It seems a bit much to have macros for everything.

Re: [PATCH] rust: clk: use the type-state pattern

[Daniel Almeida]

Hi Alice,


> It seems a bit much to have macros for everything.

I understand your point of view, but at this point the “mod private”
boilerplate is growing by the day.

It would at least make reviews slightly easier, because reviewers don't have to
ask themselves whether the submitter implemented the pattern correctly.

IMHO, of course.

— Daniel

Re: [PATCH] rust: clk: use the type-state pattern

[Daniel Almeida]

[…]

>         }
> 
> -        /// 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`].

I just noticed that some comments still refer to the old “self.0” field, but that doesn’t exist anymore.

I’ll fix that in v2.

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

— Daniel

Re: [PATCH] rust: clk: use the type-state pattern

[Viresh Kumar]

On 30-07-25, 09:27, Daniel Almeida wrote:
> What’s the issue with clk_prepare? You mean that the parenthesis are missing?

Yes.

-- 
viresh