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

Gary Guo
2026-01-19T14:20:48Z

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