On Mon, 19 Jan 2026 14:20:43 +0000 "Gary Guo" <[email protected]> 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 <[email protected]> > > --- > > 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
