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

Boris Brezillon
2026-02-02T16:10:45Z

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