Message-Id: <DFSMRQFIYQPO.1A38Y84XZ1GZO@garyguo.net>
Date: Mon, 19 Jan 2026 14:20:43 +0000
From: "Gary Guo" <gary@...yguo.net>
To: "Daniel Almeida" <daniel.almeida@...labora.com>, "Rafael J. Wysocki"
 <rafael@...nel.org>, "Viresh Kumar" <viresh.kumar@...aro.org>, "Danilo
 Krummrich" <dakr@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>, "Maarten
 Lankhorst" <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard"
 <mripard@...nel.org>, "Thomas Zimmermann" <tzimmermann@...e.de>, "David
 Airlie" <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Drew
 Fustini" <fustini@...nel.org>, "Guo Ren" <guoren@...nel.org>, "Fu Wei"
 <wefu@...hat.com>, Uwe Kleine-König <ukleinek@...nel.org>,
 "Michael Turquette" <mturquette@...libre.com>, "Stephen Boyd"
 <sboyd@...nel.org>, "Miguel Ojeda" <ojeda@...nel.org>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor
 Gross" <tmgross@...ch.edu>
Cc: <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <dri-devel@...ts.freedesktop.org>, <linux-riscv@...ts.infradead.org>,
 <linux-pwm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

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@...labora.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

Powered by blists - more mailing lists