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

["Onur Özkan" <work@onurozkan.dev>]

On Thu, 18 Jun 2026 00:46:35 -0300
Daniel Almeida <daniel.almeida@collabora.com> 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             | 512 ++++++++++++++++++++++++++++++-----------
>  rust/kernel/cpufreq.rs         |   8 +-
>  5 files changed, 396 insertions(+), 174 deletions(-)
> 
> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
> index f17bf64c22e2..9d2ec7df4bac 100644
> --- a/drivers/cpufreq/rcpufreq_dt.rs
> +++ b/drivers/cpufreq/rcpufreq_dt.rs
> @@ -40,7 +40,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 279710b36a10..a2230aebfea2 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -3,7 +3,7 @@
>  use kernel::{
>      clk::{
>          Clk,
> -        OptionalClk, //
> +        Enabled, //
>      },
>      device::{
>          Bound,
> @@ -49,7 +49,7 @@ pub(crate) struct TyrPlatformDriverData {

[...]

> -        /// Disable and unprepare the clock.
> -        ///
> -        /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
> +        /// 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 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_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`]

nit: I wouldn't use the word "Attempts" for an infallible function.

> +        /// state.
> +        #[inline]
> +        pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {

This is an infallible function, you can return Clk<Prepared> directly.

Thanks,
Onur

> +            // 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.
> @@ -251,82 +544,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
> +                // [`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 d8d26870bea2..e837bb1010e0 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -553,8 +553,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_unbound(dev, name)?;
>          self.as_mut_ref().clk = clk.as_raw();
>          Ok(clk)
>      }
> 
> -- 
> 2.54.0
>