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

[Boris Brezillon <boris.brezillon@collabora.com>]

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