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")))?;
+ 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 {}
+ }
+
+ /// 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.
+ 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>,
+ }
/// A reference-counted clock.
///
/// Rust abstraction for the C [`struct clk`].
///
+ /// A [`Clk`] instance represents a clock that can be in one of several
+ /// states: [`Unprepared`], [`Prepared`], or [`Enabled`].
+ ///
+ /// No action needs to be taken when a [`Clk`] is dropped. The calls to
+ /// `clk_unprepare()` and `clk_disable()` will be placed as applicable.
+ ///
+ /// An optional [`Clk`] is treated just like a regular [`Clk`], but its
+ /// inner `struct clk` pointer is `NULL`. This interfaces correctly with the
+ /// C API and also exposes all the methods of a regular [`Clk`] to users.
+ ///
/// # Invariants
///
/// A [`Clk`] instance holds either a pointer to a valid [`struct clk`] created by the C
@@ -99,20 +160,39 @@ mod common_clk {
/// Instances of this type are reference-counted. Calling [`Clk::get`] ensures that the
/// allocation remains valid for the lifetime of the [`Clk`].
///
+ /// The [`Prepared`] state is associated with a single count of
+ /// `clk_prepare()`, and the [`Enabled`] state is associated with a single
+ /// count of `clk_enable()`, and the [`Prepared`] state is associated with a
+ /// single count of `clk_prepare()` and `clk_enable()`.
+ ///
+ /// All states are associated with a single count of `clk_get()`.
+ ///
/// # Examples
///
/// The following example demonstrates how to obtain and configure a clock for a device.
///
/// ```
/// use kernel::c_str;
- /// use kernel::clk::{Clk, Hertz};
- /// use kernel::device::Device;
+ /// use kernel::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
+ /// use kernel::device::{Bound, Device};
/// use kernel::error::Result;
///
- /// fn configure_clk(dev: &Device) -> Result {
- /// let clk = Clk::get(dev, Some(c_str!("apb_clk")))?;
+ /// fn configure_clk(dev: &Device<Bound>) -> Result {
+ /// // The fastest way is to use a version of `Clk::get` for the desired
+ /// // state, i.e.:
+ /// let clk: Clk<Enabled> = Clk::<Enabled>::get(dev, Some(c_str!("apb_clk")))?;
///
- /// clk.prepare_enable()?;
+ /// // Any other state is also possible, e.g.:
+ /// let clk: Clk<Prepared> = Clk::<Prepared>::get(dev, Some(c_str!("apb_clk")))?;
+ ///
+ /// // Later:
+ /// let clk: Clk<Enabled> = clk.enable().map_err(|error| {
+ /// error.error
+ /// })?;
+ ///
+ /// // Note that error.clk is the original `clk` if the operation
+ /// // failed. It is provided as a convenience so that the operation may be
+ /// // retried in case of errors.
///
/// let expected_rate = Hertz::from_ghz(1);
///
@@ -120,111 +200,208 @@ mod common_clk {
/// clk.set_rate(expected_rate)?;
/// }
///
- /// clk.disable_unprepare();
+ /// // Nothing is needed here. The drop implementation will undo any
+ /// // operations as appropriate.
+ /// Ok(())
+ /// }
+ ///
+ /// fn shutdown(clk: Clk<Enabled>) -> Result {
+ /// // The states can be traversed "in the reverse order" as well:
+ /// let clk: Clk<Prepared> = clk.disable().map_err(|error| {
+ /// error.error
+ /// })?;
+ ///
+ /// // This is of type `Clk<Unprepared>`.
+ /// let clk = clk.unprepare();
+ ///
/// Ok(())
/// }
/// ```
///
/// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
#[repr(transparent)]
- pub struct Clk(*mut bindings::clk);
+ pub struct Clk<T: ClkState> {
+ inner: *mut bindings::clk,
+ _phantom: core::marker::PhantomData<T>,
+ }
// SAFETY: It is safe to call `clk_put` on another thread than where `clk_get` was called.
- unsafe impl Send for Clk {}
+ unsafe impl<T: ClkState> Send for Clk<T> {}
// SAFETY: It is safe to call any combination of the `&self` methods in parallel, as the
// methods are synchronized internally.
- unsafe impl Sync for Clk {}
+ unsafe impl<T: ClkState> Sync for Clk<T> {}
- impl Clk {
- /// Gets [`Clk`] corresponding to a [`Device`] and a connection id.
+ impl Clk<Unprepared> {
+ /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection
+ /// id.
///
/// Equivalent to the kernel's [`clk_get`] API.
///
/// [`clk_get`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_get
- pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
+ #[inline]
+ pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
let con_id = name.map_or(ptr::null(), |n| n.as_char_ptr());
- // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
- //
+ // SAFETY: It is safe to call [`clk_get`] for a valid device pointer
+ // and any `con_id`, including NULL.
+ let inner = from_err_ptr(unsafe { bindings::clk_get(dev.as_raw(), con_id) })?;
+
// INVARIANT: The reference-count is decremented when [`Clk`] goes out of scope.
- Ok(Self(from_err_ptr(unsafe {
- bindings::clk_get(dev.as_raw(), con_id)
- })?))
+ Ok(Self {
+ inner,
+ _phantom: PhantomData,
+ })
}
- /// Obtain the raw [`struct clk`] pointer.
+ /// Behaves the same as [`Self::get`], except when there is no clock
+ /// producer. In this case, instead of returning [`ENOENT`], it returns
+ /// a dummy [`Clk`].
#[inline]
- pub fn as_raw(&self) -> *mut bindings::clk {
- self.0
+ pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
+ let con_id = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+ // SAFETY: It is safe to call [`clk_get`] for a valid device pointer
+ // and any `con_id`, including NULL.
+ let inner = from_err_ptr(unsafe { bindings::clk_get_optional(dev.as_raw(), con_id) })?;
+
+ // INVARIANT: The reference-count is decremented when [`Clk`] goes out of scope.
+ Ok(Self {
+ inner,
+ _phantom: PhantomData,
+ })
}
- /// Enable the clock.
+ /// Attempts to convert the [`Clk`] to a [`Prepared`] state.
///
- /// Equivalent to the kernel's [`clk_enable`] API.
+ /// Equivalent to the kernel's [`clk_prepare`] API.
///
- /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
+ /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
#[inline]
- pub fn enable(&self) -> Result {
- // SAFETY: By the type invariants, self.as_raw() is a valid argument for
- // [`clk_enable`].
- to_result(unsafe { bindings::clk_enable(self.as_raw()) })
+ pub fn prepare(self) -> Result<Clk<Prepared>, Error<Unprepared>> {
+ // We will be transferring the ownership of our `clk_get()` count to
+ // `Clk<Prepared>`.
+ let clk = ManuallyDrop::new(self);
+
+ // SAFETY: By the type invariants, `self.0` is a valid argument for
+ // [`clk_prepare`].
+ to_result(unsafe { bindings::clk_prepare(clk.as_raw()) })
+ .map(|()| Clk {
+ inner: clk.inner,
+ _phantom: PhantomData,
+ })
+ .map_err(|error| Error {
+ error,
+ clk: ManuallyDrop::into_inner(clk),
+ })
}
+ }
- /// Disable the clock.
- ///
- /// Equivalent to the kernel's [`clk_disable`] API.
+ impl Clk<Prepared> {
+ /// Obtains a [`Clk`] from a bound [`Device`] and a connection id and
+ /// prepares it.
///
- /// [`clk_disable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_disable
+ /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
#[inline]
- pub fn disable(&self) {
- // SAFETY: By the type invariants, self.as_raw() is a valid argument for
- // [`clk_disable`].
- unsafe { bindings::clk_disable(self.as_raw()) };
+ pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Prepared>> {
+ Clk::<Unprepared>::get(dev, name)?
+ .prepare()
+ .map_err(|error| error.error)
}
- /// Prepare the clock.
- ///
- /// Equivalent to the kernel's [`clk_prepare`] API.
- ///
- /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
+ /// Behaves the same as [`Self::get`], except when there is no clock
+ /// producer. In this case, instead of returning [`ENOENT`], it returns
+ /// a dummy [`Clk`].
#[inline]
- pub fn prepare(&self) -> Result {
- // SAFETY: By the type invariants, self.as_raw() is a valid argument for
- // [`clk_prepare`].
- to_result(unsafe { bindings::clk_prepare(self.as_raw()) })
+ pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Prepared>> {
+ Clk::<Unprepared>::get_optional(dev, name)?
+ .prepare()
+ .map_err(|error| error.error)
}
- /// Unprepare the clock.
+ /// Attempts to convert the [`Clk`] to an [`Unprepared`] state.
///
/// Equivalent to the kernel's [`clk_unprepare`] API.
- ///
- /// [`clk_unprepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_unprepare
#[inline]
- pub fn unprepare(&self) {
- // SAFETY: By the type invariants, self.as_raw() is a valid argument for
+ pub fn unprepare(self) -> Clk<Unprepared> {
+ // We will be transferring the ownership of our `clk_get()` count to
+ // `Clk<Unprepared>`.
+ let clk = ManuallyDrop::new(self);
+
+ // SAFETY: By the type invariants, `self.0` is a valid argument for
// [`clk_unprepare`].
- unsafe { bindings::clk_unprepare(self.as_raw()) };
+ unsafe { bindings::clk_unprepare(clk.as_raw()) }
+
+ Clk {
+ inner: clk.inner,
+ _phantom: PhantomData,
+ }
}
- /// Prepare and enable the clock.
+ /// Attempts to convert the [`Clk`] to an [`Enabled`] state.
+ ///
+ /// Equivalent to the kernel's [`clk_enable`] API.
///
- /// Equivalent to calling [`Clk::prepare`] followed by [`Clk::enable`].
+ /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
#[inline]
- pub fn prepare_enable(&self) -> Result {
- // SAFETY: By the type invariants, self.as_raw() is a valid argument for
- // [`clk_prepare_enable`].
- to_result(unsafe { bindings::clk_prepare_enable(self.as_raw()) })
+ pub fn enable(self) -> Result<Clk<Enabled>, Error<Prepared>> {
+ // We will be transferring the ownership of our `clk_get()` and
+ // `clk_prepare()` counts to `Clk<Enabled>`.
+ let clk = ManuallyDrop::new(self);
+
+ // SAFETY: By the type invariants, `self.0` is a valid argument for
+ // [`clk_enable`].
+ to_result(unsafe { bindings::clk_enable(clk.as_raw()) })
+ .map(|()| Clk {
+ inner: clk.inner,
+ _phantom: PhantomData,
+ })
+ .map_err(|error| Error {
+ error,
+ clk: ManuallyDrop::into_inner(clk),
+ })
}
+ }
- /// Disable and unprepare the clock.
+ impl Clk<Enabled> {
+ /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection id
+ /// and then prepares and enables it.
///
- /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
+ /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
+ /// followed by [`Clk::enable`].
#[inline]
- pub fn disable_unprepare(&self) {
- // SAFETY: By the type invariants, self.as_raw() is a valid argument for
- // [`clk_disable_unprepare`].
- unsafe { bindings::clk_disable_unprepare(self.as_raw()) };
+ pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
+ Clk::<Prepared>::get(dev, name)?
+ .enable()
+ .map_err(|error| error.error)
+ }
+
+ /// Behaves the same as [`Self::get`], except when there is no clock
+ /// producer. In this case, instead of returning [`ENOENT`], it returns
+ /// a dummy [`Clk`].
+ #[inline]
+ pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
+ Clk::<Prepared>::get_optional(dev, name)?
+ .enable()
+ .map_err(|error| error.error)
+ }
+
+ /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]
+ /// state.
+ #[inline]
+ pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
+ // We will be transferring the ownership of our `clk_get()` and
+ // `clk_enable()` counts to `Clk<Prepared>`.
+ let clk = ManuallyDrop::new(self);
+
+ // SAFETY: By the type invariants, `self.0` is a valid argument for
+ // [`clk_disable`].
+ unsafe { bindings::clk_disable(clk.as_raw()) };
+
+ Ok(Clk {
+ inner: clk.inner,
+ _phantom: PhantomData,
+ })
}
/// Get clock's rate.
@@ -252,83 +429,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
}
}
- impl Drop for Clk {
- fn drop(&mut self) {
- // SAFETY: By the type invariants, self.as_raw() is a valid argument for [`clk_put`].
- unsafe { bindings::clk_put(self.as_raw()) };
- }
- }
-
- /// A reference-counted optional clock.
- ///
- /// A lightweight wrapper around an optional [`Clk`]. An [`OptionalClk`] represents a [`Clk`]
- /// that a driver can function without but may improve performance or enable additional
- /// features when available.
- ///
- /// # Invariants
- ///
- /// An [`OptionalClk`] instance encapsulates a [`Clk`] with either a valid [`struct clk`] or
- /// `NULL` pointer.
- ///
- /// Instances of this type are reference-counted. Calling [`OptionalClk::get`] ensures that the
- /// allocation remains valid for the lifetime of the [`OptionalClk`].
- ///
- /// # Examples
- ///
- /// The following example demonstrates how to obtain and configure an optional clock for a
- /// device. The code functions correctly whether or not the clock is available.
- ///
- /// ```
- /// use kernel::c_str;
- /// use kernel::clk::{OptionalClk, Hertz};
- /// use kernel::device::Device;
- /// use kernel::error::Result;
- ///
- /// fn configure_clk(dev: &Device) -> Result {
- /// let clk = OptionalClk::get(dev, Some(c_str!("apb_clk")))?;
- ///
- /// clk.prepare_enable()?;
- ///
- /// let expected_rate = Hertz::from_ghz(1);
- ///
- /// if clk.rate() != expected_rate {
- /// clk.set_rate(expected_rate)?;
- /// }
- ///
- /// clk.disable_unprepare();
- /// Ok(())
- /// }
- /// ```
- ///
- /// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
- pub struct OptionalClk(Clk);
-
- impl OptionalClk {
- /// Gets [`OptionalClk`] corresponding to a [`Device`] and a connection id.
- ///
- /// Equivalent to the kernel's [`clk_get_optional`] API.
- ///
- /// [`clk_get_optional`]:
- /// https://docs.kernel.org/core-api/kernel-api.html#c.clk_get_optional
- pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
- let con_id = name.map_or(ptr::null(), |n| n.as_char_ptr());
-
- // SAFETY: It is safe to call [`clk_get_optional`] for a valid device pointer.
- //
- // INVARIANT: The reference-count is decremented when [`OptionalClk`] goes out of
- // scope.
- Ok(Self(Clk(from_err_ptr(unsafe {
- bindings::clk_get_optional(dev.as_raw(), con_id)
- })?)))
+ impl<T: ClkState> Clk<T> {
+ /// Obtain the raw [`struct clk`] pointer.
+ #[inline]
+ pub fn as_raw(&self) -> *mut bindings::clk {
+ self.inner
}
}
- // Make [`OptionalClk`] behave like [`Clk`].
- impl Deref for OptionalClk {
- type Target = Clk;
+ impl<T: ClkState> Drop for Clk<T> {
+ fn drop(&mut self) {
+ if T::DISABLE_ON_DROP {
+ // SAFETY: By the type invariants, self.as_raw() is a valid argument for
+ // [`clk_disable`].
+ unsafe { bindings::clk_disable(self.as_raw()) };
+ }
+
+ if T::UNPREPARE_ON_DROP {
+ // SAFETY: By the type invariants, self.as_raw() is a valid argument for
+ // [`clk_unprepare`].
+ unsafe { bindings::clk_unprepare(self.as_raw()) };
+ }
- fn deref(&self) -> &Clk {
- &self.0
+ // SAFETY: By the type invariants, self.as_raw() is a valid argument for
+ // [`clk_put`].
+ unsafe { bindings::clk_put(self.as_raw()) };
}
}
}
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index f968fbd22890..d51c697cad9e 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -552,8 +552,12 @@ pub fn cpus(&mut self) -> &mut cpumask::Cpumask {
/// The caller must guarantee that the returned [`Clk`] is not dropped while it is getting used
/// by the C code.
#[cfg(CONFIG_COMMON_CLK)]
- pub unsafe fn set_clk(&mut self, dev: &Device, name: Option<&CStr>) -> Result<Clk> {
- let clk = Clk::get(dev, name)?;
+ pub unsafe fn set_clk(
+ &mut self,
+ dev: &Device<Bound>,
+ name: Option<&CStr>,
+ ) -> Result<Clk<crate::clk::Unprepared>> {
+ let clk = Clk::<crate::clk::Unprepared>::get(dev, name)?;
self.as_mut_ref().clk = clk.as_raw();
Ok(clk)
}
--
2.52.0Hello Daniel,
On Wed, 07 Jan 2026 12:09:52 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
> - /// Disable and unprepare the clock.
> + impl Clk<Enabled> {
> + /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection id
> + /// and then prepares and enables it.
> ///
> - /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
> + /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
> + /// followed by [`Clk::enable`].
> #[inline]
> - pub fn disable_unprepare(&self) {
> - // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> - // [`clk_disable_unprepare`].
> - unsafe { bindings::clk_disable_unprepare(self.as_raw()) };
> + pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
> + Clk::<Prepared>::get(dev, name)?
> + .enable()
> + .map_err(|error| error.error)
> + }
> +
> + /// Behaves the same as [`Self::get`], except when there is no clock
> + /// producer. In this case, instead of returning [`ENOENT`], it returns
> + /// a dummy [`Clk`].
> + #[inline]
> + pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
> + Clk::<Prepared>::get_optional(dev, name)?
> + .enable()
> + .map_err(|error| error.error)
> + }
> +
> + /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]
> + /// state.
> + #[inline]
> + pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
> + // We will be transferring the ownership of our `clk_get()` and
> + // `clk_enable()` counts to `Clk<Prepared>`.
> + let clk = ManuallyDrop::new(self);
> +
> + // SAFETY: By the type invariants, `self.0` is a valid argument for
> + // [`clk_disable`].
> + unsafe { bindings::clk_disable(clk.as_raw()) };
> +
> + Ok(Clk {
> + inner: clk.inner,
> + _phantom: PhantomData,
> + })
> }
>
> /// Get clock's rate.
Dunno if this has been mentioned already, but I belive the rate
getter/setter should be in the generic implementation. Indeed, it's
quite common for clock users to change the rate when the clk is
disabled to avoid unstable transitional state. The usual pattern for
that is:
- clk_set_parent(my_clk, secondary_parent)
- clk_disable[_unprepare](primary_parent) // (usually a PLL)
- clk_set_rate(primary_parent)
- clk[_prepare]_enable(primary_parent)
- clk_set_parent(my_clk, primary_parent)
The case where the clk rate is changed while the clk is active is also
valid (usually fine when it's just a divider that's changed, because
there's no stabilization period).
> @@ -252,83 +429,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
> }
> }
Hi Boris,
> On 3 Feb 2026, at 06:17, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
> Hello Daniel,
>
> On Wed, 07 Jan 2026 12:09:52 -0300
> Daniel Almeida <daniel.almeida@collabora.com> wrote:
>
>> - /// Disable and unprepare the clock.
>> + impl Clk<Enabled> {
>> + /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection id
>> + /// and then prepares and enables it.
>> ///
>> - /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
>> + /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
>> + /// followed by [`Clk::enable`].
>> #[inline]
>> - pub fn disable_unprepare(&self) {
>> - // SAFETY: By the type invariants, self.as_raw() is a valid argument for
>> - // [`clk_disable_unprepare`].
>> - unsafe { bindings::clk_disable_unprepare(self.as_raw()) };
>> + pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
>> + Clk::<Prepared>::get(dev, name)?
>> + .enable()
>> + .map_err(|error| error.error)
>> + }
>> +
>> + /// Behaves the same as [`Self::get`], except when there is no clock
>> + /// producer. In this case, instead of returning [`ENOENT`], it returns
>> + /// a dummy [`Clk`].
>> + #[inline]
>> + pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
>> + Clk::<Prepared>::get_optional(dev, name)?
>> + .enable()
>> + .map_err(|error| error.error)
>> + }
>> +
>> + /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]
>> + /// state.
>> + #[inline]
>> + pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
>> + // We will be transferring the ownership of our `clk_get()` and
>> + // `clk_enable()` counts to `Clk<Prepared>`.
>> + let clk = ManuallyDrop::new(self);
>> +
>> + // SAFETY: By the type invariants, `self.0` is a valid argument for
>> + // [`clk_disable`].
>> + unsafe { bindings::clk_disable(clk.as_raw()) };
>> +
>> + Ok(Clk {
>> + inner: clk.inner,
>> + _phantom: PhantomData,
>> + })
>> }
>>
>> /// Get clock's rate.
>
> Dunno if this has been mentioned already, but I belive the rate
> getter/setter should be in the generic implementation. Indeed, it's
> quite common for clock users to change the rate when the clk is
> disabled to avoid unstable transitional state. The usual pattern for
> that is:
>
> - clk_set_parent(my_clk, secondary_parent)
> - clk_disable[_unprepare](primary_parent) // (usually a PLL)
> - clk_set_rate(primary_parent)
> - clk[_prepare]_enable(primary_parent)
> - clk_set_parent(my_clk, primary_parent)
>
> The case where the clk rate is changed while the clk is active is also
> valid (usually fine when it's just a divider that's changed, because
> there's no stabilization period).
>
>> @@ -252,83 +429,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
>> }
>> }
>
I’m ok with this. I just assumed that these operations were only valid on enabled clks.
Will change this in the next version.
— Daniel
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
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
Hello Daniel,
On Mon, 2 Feb 2026 17:10:38 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> > > -#[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
Sorry, I've hit the reply button before I had finished writing my
answer. So I was about to say that I had started writing something
similar without knowing this series existed, and I feel like we'd don't
really need those prepare_enable() shortcuts that exist in C. We might
has well just go:
Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;
and have the following variant-specofoc functions
- Clk<Unprepared>::get[_optional]() (no get on Prepared and Enabled
variants)
- Clk<Unprepared>::prepare()
- Clk<Prepared>::{enable,unprepare}()
- Clk<Enabled>::{disable}()
Regards,
Boris
> On 3 Feb 2026, at 06:09, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
> Hello Daniel,
>
> On Mon, 2 Feb 2026 17:10:38 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
>>>> -#[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
>
> Sorry, I've hit the reply button before I had finished writing my
> answer. So I was about to say that I had started writing something
> similar without knowing this series existed, and I feel like we'd don't
> really need those prepare_enable() shortcuts that exist in C. We might
> has well just go:
>
> Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;
>
> and have the following variant-specofoc functions
>
> - Clk<Unprepared>::get[_optional]() (no get on Prepared and Enabled
> variants)
> - Clk<Unprepared>::prepare()
> - Clk<Prepared>::{enable,unprepare}()
> - Clk<Enabled>::{disable}()
>
> Regards,
>
> Boris
>
>
I don’t understand how is this better than the turbofish we currently have.
In other words, how is this:
Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;
Better than this:
Clk::<Enabled>::get(/*…*/);
— Daniel
On Tue, 3 Feb 2026 10:37:15 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
> > On 3 Feb 2026, at 06:09, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >
> > Hello Daniel,
> >
> > On Mon, 2 Feb 2026 17:10:38 +0100
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >
> >>>> -#[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
> >
> > Sorry, I've hit the reply button before I had finished writing my
> > answer. So I was about to say that I had started writing something
> > similar without knowing this series existed, and I feel like we'd don't
> > really need those prepare_enable() shortcuts that exist in C. We might
> > has well just go:
> >
> > Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;
> >
> > and have the following variant-specofoc functions
> >
> > - Clk<Unprepared>::get[_optional]() (no get on Prepared and Enabled
> > variants)
> > - Clk<Unprepared>::prepare()
> > - Clk<Prepared>::{enable,unprepare}()
> > - Clk<Enabled>::{disable}()
> >
> > Regards,
> >
> > Boris
> >
> >
>
>
> I don’t understand how is this better than the turbofish we currently have.
>
> In other words, how is this:
>
> Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;
>
> Better than this:
>
> Clk::<Enabled>::get(/*…*/);
For one, it doesn't force you to expose multiple functions in the
implementation (::get[_optional]() is only present in the Unprepared
impl variant, no shortcut to combine state transition, ...) which means
less code to maintain overall. But I also prefer the fact this clearly
reflects the state transitions that exist to get an Enabled clk (first
you get an Unprepared clk that you have to prepare and enable to turn
that into an Enabled clk). That's a matter of taste of course, just
saying that if we get rid of the turbofish syntax like Gary suggested
at some point, I think I'd find it clearer to also just expose the
transitions between two consecutive states, and let the caller go
through all the steps.
On Tue, 3 Feb 2026 10:09:13 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> Hello Daniel,
>
> On Mon, 2 Feb 2026 17:10:38 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
> > > > -#[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
>
> Sorry, I've hit the reply button before I had finished writing my
> answer. So I was about to say that I had started writing something
> similar without knowing this series existed, and I feel like we'd don't
> really need those prepare_enable() shortcuts that exist in C. We might
> has well just go:
>
> Clk::get(dev, Some(c_str!("core"))).prepare()?.enable()?;
If we want this pattern to work, we also need:
impl<State: ClkState> From<Error<State>> for kernel::error::Error {
fn from(e: Error<State>) -> kernel::error::Error {
e.error
}
}
>
> and have the following variant-specofoc functions
>
> - Clk<Unprepared>::get[_optional]() (no get on Prepared and Enabled
> variants)
> - Clk<Unprepared>::prepare()
> - Clk<Prepared>::{enable,unprepare}()
> - Clk<Enabled>::{disable}()
>
> Regards,
>
> Boris
>
On Mon, Jan 19, 2026 at 3:20 PM Gary Guo <gary@garyguo.net> wrote: > > I guess it's time for me to work on a `#[sealed]` macro... ...and perhaps pinging upstream for the language feature... :) I added https://github.com/rust-lang/rust/issues/105077 to our usual list of wishes at https://github.com/Rust-for-Linux/linux/issues/354. Do you think using the linked crate in the commit message would make sense? It seems to be a couple pages of code, using `syn` 2.0, which should be fine now. Another option is a "good first issue" of medium difficulty if you wish. > I wonder if it makes sense to add a general `ErrorWith` type for errors that > carries error code + data. Sounds reasonable... Cheers, Miguel
On Mon Jan 19, 2026 at 3:22 PM GMT, Miguel Ojeda wrote: > On Mon, Jan 19, 2026 at 3:20 PM Gary Guo <gary@garyguo.net> wrote: >> >> I guess it's time for me to work on a `#[sealed]` macro... > > ...and perhaps pinging upstream for the language feature... :) I added > https://github.com/rust-lang/rust/issues/105077 to our usual list of > wishes at https://github.com/Rust-for-Linux/linux/issues/354. > > Do you think using the linked crate in the commit message would make > sense? It seems to be a couple pages of code, using `syn` 2.0, which > should be fine now. Which crate are you talking about? I can't find a linked crate in the issue. Best, Gary > > Another option is a "good first issue" of medium difficulty if you wish. > >> I wonder if it makes sense to add a general `ErrorWith` type for errors that >> carries error code + data. > > Sounds reasonable... > > Cheers, > Miguel
On Mon, Jan 19, 2026 at 4:36 PM Gary Guo <gary@garyguo.net> wrote: > > Which crate are you talking about? I can't find a linked crate in the issue. The commit message (i.e. not the issue) has an (unused) link with the `sealed` crate: Link: https://crates.io/crates/sealed [1] Cheers, Miguel
On Mon Jan 19, 2026 at 3:46 PM GMT, Miguel Ojeda wrote: > On Mon, Jan 19, 2026 at 4:36 PM Gary Guo <gary@garyguo.net> wrote: >> >> Which crate are you talking about? I can't find a linked crate in the issue. > > The commit message (i.e. not the issue) has an (unused) link with the > `sealed` crate: > > Link: https://crates.io/crates/sealed [1] Yeah, some thing similar. Probably an even simpler version with only things that we need. Best, Gary > > Cheers, > Miguel
Hi Daniel, On Wed, Jan 07, 2026 at 12:09:52PM -0300, 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> I don't know the typestate pattern that well, but I wonder if we don't paint ourselves into a corner by introducing it. While it's pretty common to get your clock from the get go into a state, and then don't modify it (like what devm_clk_get_enabled provides for example), and the typestate pattern indeed works great for those, we also have a significant number of drivers that will have a finer-grained control over the clock enablement for PM. For example, it's quite typical to have (at least) one clock for the bus interface that drives the register, and one that drives the main component logic. The former needs to be enabled only when you're accessing the registers (and can be abstracted with regmap_mmio_attach_clk for example), and the latter needs to be enabled only when the device actually starts operating. You have a similar thing for the prepare vs enable thing. The difference between the two is that enable can be called into atomic context but prepare can't. So for drivers that would care about this, you would create your device with an unprepared clock, and then at various times during the driver lifetime, you would mutate that state. AFAIU, encoding the state of the clock into the Clk type (and thus forcing the structure that holds it) prevents that mutation. If not, we should make it clearer (by expanding the doc maybe?) how such a pattern can be supported. Maxime
Hi Maxime :)
>
> I don't know the typestate pattern that well, but I wonder if we don't
> paint ourselves into a corner by introducing it.
>
> While it's pretty common to get your clock from the get go into a state,
> and then don't modify it (like what devm_clk_get_enabled provides for
> example), and the typestate pattern indeed works great for those, we
Minor correction, devm_clk_get_enabled is not handled by the typestate
pattern. The next patch does include this function for convenience, but
you get a Result<()>. The typestate pattern is used when you want more
control.
> also have a significant number of drivers that will have a finer-grained
> control over the clock enablement for PM.
>
> For example, it's quite typical to have (at least) one clock for the bus
> interface that drives the register, and one that drives the main
> component logic. The former needs to be enabled only when you're
> accessing the registers (and can be abstracted with
> regmap_mmio_attach_clk for example), and the latter needs to be enabled
> only when the device actually starts operating.
>
> You have a similar thing for the prepare vs enable thing. The difference
> between the two is that enable can be called into atomic context but
> prepare can't.
>
> So for drivers that would care about this, you would create your device
> with an unprepared clock, and then at various times during the driver
> lifetime, you would mutate that state.
>
> AFAIU, encoding the state of the clock into the Clk type (and thus
> forcing the structure that holds it) prevents that mutation. If not, we
> should make it clearer (by expanding the doc maybe?) how such a pattern
> can be supported.
>
> Maxime
IIUC, your main point seems to be about mutating the state at runtime? This is
possible with this code. You can just have an enum, for example:
enum MyClocks {
Unprepared(Clk<Unprepared>),
Prepared(Clk<Prepared>),
Enabled(Clk<Enabled>),
}
In fact, I specifically wanted to ensure that this was possible when writing
these patches, as it’s needed by drivers. If you want to, I can cover that in
the examples, no worries.
Same for Regulator<T>, by the way.
— Daniel
On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
> Hi Maxime :)
>
> >
> > I don't know the typestate pattern that well, but I wonder if we don't
> > paint ourselves into a corner by introducing it.
> >
> > While it's pretty common to get your clock from the get go into a state,
> > and then don't modify it (like what devm_clk_get_enabled provides for
> > example), and the typestate pattern indeed works great for those, we
>
> Minor correction, devm_clk_get_enabled is not handled by the typestate
> pattern. The next patch does include this function for convenience, but
> you get a Result<()>. The typestate pattern is used when you want more
> control.
>
> > also have a significant number of drivers that will have a finer-grained
> > control over the clock enablement for PM.
> >
> > For example, it's quite typical to have (at least) one clock for the bus
> > interface that drives the register, and one that drives the main
> > component logic. The former needs to be enabled only when you're
> > accessing the registers (and can be abstracted with
> > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > only when the device actually starts operating.
> >
> > You have a similar thing for the prepare vs enable thing. The difference
> > between the two is that enable can be called into atomic context but
> > prepare can't.
> >
> > So for drivers that would care about this, you would create your device
> > with an unprepared clock, and then at various times during the driver
> > lifetime, you would mutate that state.
> >
> > AFAIU, encoding the state of the clock into the Clk type (and thus
> > forcing the structure that holds it) prevents that mutation. If not, we
> > should make it clearer (by expanding the doc maybe?) how such a pattern
> > can be supported.
> >
> > Maxime
>
> IIUC, your main point seems to be about mutating the state at runtime? This is
> possible with this code. You can just have an enum, for example:
>
> enum MyClocks {
> Unprepared(Clk<Unprepared>),
> Prepared(Clk<Prepared>),
> Enabled(Clk<Enabled>),
> }
>
> In fact, I specifically wanted to ensure that this was possible when writing
> these patches, as it’s needed by drivers. If you want to, I can cover that in
> the examples, no worries.
Yes, that would be great. I do wonder though if it wouldn't make sense
to turn it the other way around. It creates a fair share of boilerplate
for a number of drivers. Can't we keep Clk the way it is as a
lower-level type, and crate a ManagedClk (or whatever name you prefer)
that drivers can use, and would be returned by higher-level helpers, if
they so choose?
That way, we do have the typestate API for whoever wants to, without
creating too much boilerplate for everybody else.
Maxime
On Mon Jan 19, 2026 at 10:45 AM GMT, Maxime Ripard wrote:
> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>> Hi Maxime :)
>>
>> >
>> > I don't know the typestate pattern that well, but I wonder if we don't
>> > paint ourselves into a corner by introducing it.
>> >
>> > While it's pretty common to get your clock from the get go into a state,
>> > and then don't modify it (like what devm_clk_get_enabled provides for
>> > example), and the typestate pattern indeed works great for those, we
>>
>> Minor correction, devm_clk_get_enabled is not handled by the typestate
>> pattern. The next patch does include this function for convenience, but
>> you get a Result<()>. The typestate pattern is used when you want more
>> control.
>>
>> > also have a significant number of drivers that will have a finer-grained
>> > control over the clock enablement for PM.
>> >
>> > For example, it's quite typical to have (at least) one clock for the bus
>> > interface that drives the register, and one that drives the main
>> > component logic. The former needs to be enabled only when you're
>> > accessing the registers (and can be abstracted with
>> > regmap_mmio_attach_clk for example), and the latter needs to be enabled
>> > only when the device actually starts operating.
>> >
>> > You have a similar thing for the prepare vs enable thing. The difference
>> > between the two is that enable can be called into atomic context but
>> > prepare can't.
>> >
>> > So for drivers that would care about this, you would create your device
>> > with an unprepared clock, and then at various times during the driver
>> > lifetime, you would mutate that state.
>> >
>> > AFAIU, encoding the state of the clock into the Clk type (and thus
>> > forcing the structure that holds it) prevents that mutation. If not, we
>> > should make it clearer (by expanding the doc maybe?) how such a pattern
>> > can be supported.
>> >
>> > Maxime
>>
>> IIUC, your main point seems to be about mutating the state at runtime? This is
>> possible with this code. You can just have an enum, for example:
>>
>> enum MyClocks {
>> Unprepared(Clk<Unprepared>),
>> Prepared(Clk<Prepared>),
>> Enabled(Clk<Enabled>),
>> }
>>
>> In fact, I specifically wanted to ensure that this was possible when writing
>> these patches, as it’s needed by drivers. If you want to, I can cover that in
>> the examples, no worries.
>
> Yes, that would be great. I do wonder though if it wouldn't make sense
> to turn it the other way around. It creates a fair share of boilerplate
> for a number of drivers. Can't we keep Clk the way it is as a
> lower-level type, and crate a ManagedClk (or whatever name you prefer)
> that drivers can use, and would be returned by higher-level helpers, if
> they so choose?
>
> That way, we do have the typestate API for whoever wants to, without
> creating too much boilerplate for everybody else.
One solution is to have a new typestate `Dynamic` which opts to track things
using variables.
struct Dynamic {
enabled: bool,
prepared: bool,
}
trait ClkState {
// Change to methods
fn disable_on_drop(&self) -> bool;
}
struct Clk<State> {
...
// Keep an instance, which is zero-sized for everything except `Dynamic`
state: State,
}
this way we can have runtime-checked state conversions.
Best,
Gary
>> Yes, that would be great. I do wonder though if it wouldn't make sense
>> to turn it the other way around. It creates a fair share of boilerplate
>> for a number of drivers. Can't we keep Clk the way it is as a
>> lower-level type, and crate a ManagedClk (or whatever name you prefer)
>> that drivers can use, and would be returned by higher-level helpers, if
>> they so choose?
>>
>> That way, we do have the typestate API for whoever wants to, without
>> creating too much boilerplate for everybody else.
>
> One solution is to have a new typestate `Dynamic` which opts to track things
> using variables.
>
> struct Dynamic {
> enabled: bool,
> prepared: bool,
> }
>
> trait ClkState {
> // Change to methods
> fn disable_on_drop(&self) -> bool;
> }
>
> struct Clk<State> {
> ...
> // Keep an instance, which is zero-sized for everything except `Dynamic`
> state: State,
> }
>
> this way we can have runtime-checked state conversions.
>
> Best,
> Gary
There used to be a Dynamic state in the past in a similar setting. That was removed after some thorough discussion. I’d say we should refrain from going back to this. Specially considering that the current design works fine.
I can remove the turbofish if you want, even though I think they’re useful so that we have the same API for all states.
— Daniel
On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
> > > For example, it's quite typical to have (at least) one clock for the bus
> > > interface that drives the register, and one that drives the main
> > > component logic. The former needs to be enabled only when you're
> > > accessing the registers (and can be abstracted with
> > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > > only when the device actually starts operating.
> > >
> > > You have a similar thing for the prepare vs enable thing. The difference
> > > between the two is that enable can be called into atomic context but
> > > prepare can't.
> > >
> > > So for drivers that would care about this, you would create your device
> > > with an unprepared clock, and then at various times during the driver
> > > lifetime, you would mutate that state.
The case where you're doing it only while accessing registers is
interesting, because that means the Enable bit may be owned by a local
variable. We may imagine an:
let enabled = self.prepared_clk.enable_scoped();
... use registers
drop(enabled);
Now ... this doesn't quite work with the current API - the current
Enabled stated owns both a prepare and enable count, but the above keeps
the prepare count in `self` and the enabled count in a local variable.
But it could be done with a fourth state, or by a closure method:
self.prepared_clk.with_enabled(|| {
... use registers
});
All of this would work with an immutable variable of type Clk<Prepared>.
> > > AFAIU, encoding the state of the clock into the Clk type (and thus
> > > forcing the structure that holds it) prevents that mutation. If not, we
> > > should make it clearer (by expanding the doc maybe?) how such a pattern
> > > can be supported.
> > >
> > > Maxime
> >
> > IIUC, your main point seems to be about mutating the state at runtime? This is
> > possible with this code. You can just have an enum, for example:
> >
> > enum MyClocks {
> > Unprepared(Clk<Unprepared>),
> > Prepared(Clk<Prepared>),
> > Enabled(Clk<Enabled>),
> > }
I believe you need an extra state if the state is not bound to the scope
of a function:
enum MyClocks {
Unprepared(Clk<Unprepared>),
Prepared(Clk<Prepared>),
Enabled(Clk<Enabled>),
Transitioning,
}
since mem::replace() needs a new value before you can take ownership of
the existing Clk value.
> > In fact, I specifically wanted to ensure that this was possible when writing
> > these patches, as it’s needed by drivers. If you want to, I can cover that in
> > the examples, no worries.
>
> Yes, that would be great. I do wonder though if it wouldn't make sense
> to turn it the other way around. It creates a fair share of boilerplate
> for a number of drivers. Can't we keep Clk the way it is as a
> lower-level type, and crate a ManagedClk (or whatever name you prefer)
> that drivers can use, and would be returned by higher-level helpers, if
> they so choose?
>
> That way, we do have the typestate API for whoever wants to, without
> creating too much boilerplate for everybody else.
I think that if you still want an API where you just call enable/disable
directly on it with no protection against unbalanced calls, then that
should be the special API. Probably called RawClk and functions marked
unsafe. Unbalanced calls seem really dangerous and use should not be
encouraged.
The current type-state based API is the least-boilerplate option for
drivers that exist today.
Alice
On Mon, 19 Jan 2026 12:35:21 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
> > On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
> > > > For example, it's quite typical to have (at least) one clock for the bus
> > > > interface that drives the register, and one that drives the main
> > > > component logic. The former needs to be enabled only when you're
> > > > accessing the registers (and can be abstracted with
> > > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > > > only when the device actually starts operating.
> > > >
> > > > You have a similar thing for the prepare vs enable thing. The difference
> > > > between the two is that enable can be called into atomic context but
> > > > prepare can't.
> > > >
> > > > So for drivers that would care about this, you would create your device
> > > > with an unprepared clock, and then at various times during the driver
> > > > lifetime, you would mutate that state.
>
> The case where you're doing it only while accessing registers is
> interesting, because that means the Enable bit may be owned by a local
> variable. We may imagine an:
>
> let enabled = self.prepared_clk.enable_scoped();
> ... use registers
> drop(enabled);
>
> Now ... this doesn't quite work with the current API - the current
> Enabled stated owns both a prepare and enable count, but the above keeps
> the prepare count in `self` and the enabled count in a local variable.
> But it could be done with a fourth state, or by a closure method:
>
> self.prepared_clk.with_enabled(|| {
> ... use registers
> });
>
> All of this would work with an immutable variable of type Clk<Prepared>.
Hm, maybe it'd make sense to implement Clone so we can have a temporary
clk variable that has its own prepare/enable refs and releases them
as it goes out of scope. This implies wrapping *mut bindings::clk in an
Arc<> because bindings::clk is not ARef, but should be relatively easy
to do. Posting the quick experiment I did with this approach, in case
you're interested [1]
[1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837
Hi Boris,
> On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
> On Mon, 19 Jan 2026 12:35:21 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>>>>> For example, it's quite typical to have (at least) one clock for the bus
>>>>> interface that drives the register, and one that drives the main
>>>>> component logic. The former needs to be enabled only when you're
>>>>> accessing the registers (and can be abstracted with
>>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>>>> only when the device actually starts operating.
>>>>>
>>>>> You have a similar thing for the prepare vs enable thing. The difference
>>>>> between the two is that enable can be called into atomic context but
>>>>> prepare can't.
>>>>>
>>>>> So for drivers that would care about this, you would create your device
>>>>> with an unprepared clock, and then at various times during the driver
>>>>> lifetime, you would mutate that state.
>>
>> The case where you're doing it only while accessing registers is
>> interesting, because that means the Enable bit may be owned by a local
>> variable. We may imagine an:
>>
>> let enabled = self.prepared_clk.enable_scoped();
>> ... use registers
>> drop(enabled);
>>
>> Now ... this doesn't quite work with the current API - the current
>> Enabled stated owns both a prepare and enable count, but the above keeps
>> the prepare count in `self` and the enabled count in a local variable.
>> But it could be done with a fourth state, or by a closure method:
>>
>> self.prepared_clk.with_enabled(|| {
>> ... use registers
>> });
>>
>> All of this would work with an immutable variable of type Clk<Prepared>.
>
> Hm, maybe it'd make sense to implement Clone so we can have a temporary
> clk variable that has its own prepare/enable refs and releases them
> as it goes out of scope. This implies wrapping *mut bindings::clk in an
> Arc<> because bindings::clk is not ARef, but should be relatively easy
> to do. Posting the quick experiment I did with this approach, in case
> you're interested [1]
>
> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837
The problem with what you have suggested is that the previous state is not
consumed if you can clone it, and consuming the previous state is a pretty key
element in ensuring you cannot misuse it. For example, here:
let enabled_clk = prepared_clk.clone().enable()?;
// do stuff
// enabled_clk goes out of scope and releases the enable
// ref it had
prepared_clk is still alive. Now, this may not be the end of the world in this
particular case, but for API consistency, I'd say we should probably avoid this
behavior.
I see that Alice suggested a closure approach. IMHO, we should use that
instead.
— Daniel
On Tue, 3 Feb 2026 10:33:34 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
> Hi Boris,
>
> > On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >
> > On Mon, 19 Jan 2026 12:35:21 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> >> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
> >>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
> >>>>> For example, it's quite typical to have (at least) one clock for the bus
> >>>>> interface that drives the register, and one that drives the main
> >>>>> component logic. The former needs to be enabled only when you're
> >>>>> accessing the registers (and can be abstracted with
> >>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
> >>>>> only when the device actually starts operating.
> >>>>>
> >>>>> You have a similar thing for the prepare vs enable thing. The difference
> >>>>> between the two is that enable can be called into atomic context but
> >>>>> prepare can't.
> >>>>>
> >>>>> So for drivers that would care about this, you would create your device
> >>>>> with an unprepared clock, and then at various times during the driver
> >>>>> lifetime, you would mutate that state.
> >>
> >> The case where you're doing it only while accessing registers is
> >> interesting, because that means the Enable bit may be owned by a local
> >> variable. We may imagine an:
> >>
> >> let enabled = self.prepared_clk.enable_scoped();
> >> ... use registers
> >> drop(enabled);
> >>
> >> Now ... this doesn't quite work with the current API - the current
> >> Enabled stated owns both a prepare and enable count, but the above keeps
> >> the prepare count in `self` and the enabled count in a local variable.
> >> But it could be done with a fourth state, or by a closure method:
> >>
> >> self.prepared_clk.with_enabled(|| {
> >> ... use registers
> >> });
> >>
> >> All of this would work with an immutable variable of type Clk<Prepared>.
> >
> > Hm, maybe it'd make sense to implement Clone so we can have a temporary
> > clk variable that has its own prepare/enable refs and releases them
> > as it goes out of scope. This implies wrapping *mut bindings::clk in an
> > Arc<> because bindings::clk is not ARef, but should be relatively easy
> > to do. Posting the quick experiment I did with this approach, in case
> > you're interested [1]
> >
> > [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837
>
> The problem with what you have suggested is that the previous state is not
> consumed if you can clone it, and consuming the previous state is a pretty key
> element in ensuring you cannot misuse it. For example, here:
>
> let enabled_clk = prepared_clk.clone().enable()?;
> // do stuff
> // enabled_clk goes out of scope and releases the enable
> // ref it had
>
> prepared_clk is still alive.
That was intentional in this example. Think about a prepared_clk that's
stored in some driver-internal object, because you want to keep the clk
prepared at all times between the probe() and unbind(). Then you have
some sections where you want to briefly enable the clk to access
registers, and immediately disable it when you're done. The clone()
here guarantees that the initial prepared_clk stays valid.
If you were to disable, unprepare and put the clk when enabled_clk goes
out of scope, you'd go
let enabled_clk = prepared_clk.enable()?;
and that would still work, it's just not the same use-case.
> Now, this may not be the end of the world in this
> particular case, but for API consistency, I'd say we should probably avoid this
> behavior.
>
> I see that Alice suggested a closure approach. IMHO, we should use that
> instead.
The closure, while being useful for the above local clk-enablement
example, doesn't allow for passing some Clk<Enabled> guard around, like
you would do with a lock Guard, and I believe that's a useful thing to
have.
<snip>
>>
>> The problem with what you have suggested is that the previous state is not
>> consumed if you can clone it, and consuming the previous state is a pretty key
>> element in ensuring you cannot misuse it. For example, here:
>>
>> let enabled_clk = prepared_clk.clone().enable()?;
>> // do stuff
>> // enabled_clk goes out of scope and releases the enable
>> // ref it had
>>
>> prepared_clk is still alive.
>
> That was intentional in this example. Think about a prepared_clk that's
> stored in some driver-internal object, because you want to keep the clk
> prepared at all times between the probe() and unbind(). Then you have
> some sections where you want to briefly enable the clk to access
> registers, and immediately disable it when you're done. The clone()
> here guarantees that the initial prepared_clk stays valid.
>
> If you were to disable, unprepare and put the clk when enabled_clk goes
> out of scope, you'd go
>
> let enabled_clk = prepared_clk.enable()?;
>
> and that would still work, it's just not the same use-case.
>
Ok, let’s have clone() then.
>> Now, this may not be the end of the world in this
>> particular case, but for API consistency, I'd say we should probably avoid this
>> behavior.
>>
>> I see that Alice suggested a closure approach. IMHO, we should use that
>> instead.
>
> The closure, while being useful for the above local clk-enablement
> example, doesn't allow for passing some Clk<Enabled> guard around, like
> you would do with a lock Guard, and I believe that's a useful thing to
> have.
Wdym? You’d still get a &Clk<Enabled> that you can pass around, i.e.:
self.prepared_clk.with_enabled(|clk: &Clk<Enabled> | {
... use registers, pass &Clk<Enabled> as needed
});
This is now not about clone() vs not clone(), but more about limiting the scope of the
Enabled state, which would cater to the use-case you mentioned IIUC.
— Daniel
On Tue Feb 3, 2026 at 4:28 PM GMT, Daniel Almeida wrote:
> <snip>
>
>>>
>>> The problem with what you have suggested is that the previous state is not
>>> consumed if you can clone it, and consuming the previous state is a pretty key
>>> element in ensuring you cannot misuse it. For example, here:
>>>
>>> let enabled_clk = prepared_clk.clone().enable()?;
>>> // do stuff
>>> // enabled_clk goes out of scope and releases the enable
>>> // ref it had
>>>
>>> prepared_clk is still alive.
>>
>> That was intentional in this example. Think about a prepared_clk that's
>> stored in some driver-internal object, because you want to keep the clk
>> prepared at all times between the probe() and unbind(). Then you have
>> some sections where you want to briefly enable the clk to access
>> registers, and immediately disable it when you're done. The clone()
>> here guarantees that the initial prepared_clk stays valid.
>>
>> If you were to disable, unprepare and put the clk when enabled_clk goes
>> out of scope, you'd go
>
>>
>> let enabled_clk = prepared_clk.enable()?;
>>
>> and that would still work, it's just not the same use-case.
>>
>
> Ok, let’s have clone() then.
>
>
>>> Now, this may not be the end of the world in this
>>> particular case, but for API consistency, I'd say we should probably avoid this
>>> behavior.
>>>
>>> I see that Alice suggested a closure approach. IMHO, we should use that
>>> instead.
>>
>> The closure, while being useful for the above local clk-enablement
>> example, doesn't allow for passing some Clk<Enabled> guard around, like
>> you would do with a lock Guard, and I believe that's a useful thing to
>> have.
>
>
> Wdym? You’d still get a &Clk<Enabled> that you can pass around, i.e.:
>
> self.prepared_clk.with_enabled(|clk: &Clk<Enabled> | {
> ... use registers, pass &Clk<Enabled> as needed
> });
>
> This is now not about clone() vs not clone(), but more about limiting the scope of the
> Enabled state, which would cater to the use-case you mentioned IIUC.
I think it's fine to have all of these:
* `Clone` impl
* `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
* `with_enabled` that gives `&Clk<Enabled>`
This way, if you only want to enable in short time, you can do `with_enabled`.
If the closure callback wants to keep clock enabled for longer, it can just do
`.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
If the user just have a reference and want to enable the callback they can do
`prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
Best,
Gary
>
> — Daniel
> > I think it's fine to have all of these: > * `Clone` impl > * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>` > * `with_enabled` that gives `&Clk<Enabled>` > > This way, if you only want to enable in short time, you can do `with_enabled`. > If the closure callback wants to keep clock enabled for longer, it can just do > `.clone()` inside the closure and obtain an owned `Clk<Enabled>`. > > If the user just have a reference and want to enable the callback they can do > `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts? > > Best, > Gary I’m ok with what you proposed above. The only problem is that implementing clone() is done through an Arc<*mut bindings::clk> in Boris’ current design, so this requires an extra allocation. — Daniel
On Tue Feb 3, 2026 at 7:26 PM GMT, Daniel Almeida wrote: > >> >> I think it's fine to have all of these: >> * `Clone` impl >> * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>` >> * `with_enabled` that gives `&Clk<Enabled>` >> >> This way, if you only want to enable in short time, you can do `with_enabled`. >> If the closure callback wants to keep clock enabled for longer, it can just do >> `.clone()` inside the closure and obtain an owned `Clk<Enabled>`. >> >> If the user just have a reference and want to enable the callback they can do >> `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts? >> >> Best, >> Gary > > > I’m ok with what you proposed above. The only problem is that implementing > clone() is done through an Arc<*mut bindings::clk> in Boris’ current > design, so this requires an extra allocation. Hmm, that's a very good point. `struct clk` is already a reference into clk_core, so having to put another level of indirection over is not ideal. However, if we're going to keep C code unchanged and do a zero-cost abstraction on the Rust side, then we won't be able to have have multiple prepare/enable to the same `struct clk` with the current design. It feels like we can to do a trade-off and choose from: * Not be able to have multiple prepare/enable calls on the same `clk` (this can limit users that need dynamically enable/disable clocks, with the very limited exception that closure-callback is fine). * Do an extra allocation * Put lifetime on types that represent a prepared/enabled `Clk` * Change C to make `struct clk` refcounted. Best, Gary > > — Daniel
Hi Gary, Daniel,
On Tue, 03 Feb 2026 20:36:30 +0000
"Gary Guo" <gary@garyguo.net> wrote:
> On Tue Feb 3, 2026 at 7:26 PM GMT, Daniel Almeida wrote:
> >
> >>
> >> I think it's fine to have all of these:
> >> * `Clone` impl
> >> * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
> >> * `with_enabled` that gives `&Clk<Enabled>`
> >>
> >> This way, if you only want to enable in short time, you can do `with_enabled`.
> >> If the closure callback wants to keep clock enabled for longer, it can just do
> >> `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
> >>
> >> If the user just have a reference and want to enable the callback they can do
> >> `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
> >>
> >> Best,
> >> Gary
> >
> >
> > I’m ok with what you proposed above. The only problem is that implementing
> > clone() is done through an Arc<*mut bindings::clk> in Boris’ current
> > design, so this requires an extra allocation.
>
> Hmm, that's a very good point. `struct clk` is already a reference into
> clk_core, so having to put another level of indirection over is not ideal.
> However, if we're going to keep C code unchanged and do a zero-cost abstraction
> on the Rust side, then we won't be able to have have multiple prepare/enable to
> the same `struct clk` with the current design.
>
> It feels like we can to do a trade-off and choose from:
> 1. Not be able to have multiple prepare/enable calls on the same `clk` (this can
> limit users that need dynamically enable/disable clocks, with the very limited
> exception that closure-callback is fine).
> 2. Do an extra allocation
> 3. Put lifetime on types that represent a prepared/enabled `Clk`
> 4. Change C to make `struct clk` refcounted.
It probably comes to no surprise that I'd be more in favor of option 2
or 4. Maybe option 2 first, so we can get the user-facing API merged
without too much churn, and then we can see if the clk maintainers are
happy adding a refcnt to struct clk to optimize things.
If we really feel that the indirection/memory overhead is going to
hurt us, we can also start with option 1, and extend it to 2 and/or 4
(needed to add a Clone support) when it becomes evident we can't do
without it. But as I was saying in my previous reply to Daniel, I
expect the extra indirection/memory overhead to be negligible since:
1. clks are usually not {prepared,enabled}/{disabled,unprepared} in a
hot path
2. in the rare occasions where they might be ({dev,cpu}freq ?), this
clk state change is usually one operation in an ocean of other
slower operations (regulator reconfiguration, for instance, which
usually goes over a slow I2C bus, or a
relatively-faster-but-still-slow SPI one, at least when we compare
it to an IoMem access for in-SoCs clks). So overall, the clk state
change might account for a very small portion of the CPU cycles
spent in this bigger operation
3. if I focus solely on the clk aspect, and look at the existing
indirections in the clk framework (clk -> clk_core -> clk_{hw,ops} ->
clk_methods), I'd expect the Arc indirection to be just noise in
this pre-existing overhead
4. in term of memory, we're talking about 16 more bytes allocated per
Clk on a 64-bit architecture (refcount is an int, but the alignment
for the clk pointer forces 4 bytes of padding on most
architectures). On a 64 bit arch, struct clk is 72 bytes if my math
is correct, so that's a 22% overhead, compared to 11% overhead if
the refcount was in struct clk (or in a struct
refcounted_clk variant if we don't want C users to pay the price).
Not great, but not terrible either
So yeah my gut feeling is that we might be overthinking this extra
allocation/indirection issue. This being said, one thing I'd really like
to avoid is us being dragged into infinite discussions about a perfect
implementation causing the merging of these changes to be delayed and
other contributions being blocked on this (perfect is the enemy of
good). I mean, option #1 is already an improvement compared to the raw
functions we have at the moment, so if that's the middle-ground we
agree on, I'm happy to give it my R-b.
Regards,
Boris
On Wed, Feb 04, 2026 at 09:11:04AM +0100, Boris Brezillon wrote:
> Hi Gary, Daniel,
>
> On Tue, 03 Feb 2026 20:36:30 +0000
> "Gary Guo" <gary@garyguo.net> wrote:
>
> > On Tue Feb 3, 2026 at 7:26 PM GMT, Daniel Almeida wrote:
> > >
> > >>
> > >> I think it's fine to have all of these:
> > >> * `Clone` impl
> > >> * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
> > >> * `with_enabled` that gives `&Clk<Enabled>`
> > >>
> > >> This way, if you only want to enable in short time, you can do `with_enabled`.
> > >> If the closure callback wants to keep clock enabled for longer, it can just do
> > >> `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
> > >>
> > >> If the user just have a reference and want to enable the callback they can do
> > >> `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
> > >>
> > >> Best,
> > >> Gary
> > >
> > >
> > > I’m ok with what you proposed above. The only problem is that implementing
> > > clone() is done through an Arc<*mut bindings::clk> in Boris’ current
> > > design, so this requires an extra allocation.
> >
> > Hmm, that's a very good point. `struct clk` is already a reference into
> > clk_core, so having to put another level of indirection over is not ideal.
> > However, if we're going to keep C code unchanged and do a zero-cost abstraction
> > on the Rust side, then we won't be able to have have multiple prepare/enable to
> > the same `struct clk` with the current design.
> >
> > It feels like we can to do a trade-off and choose from:
> > 1. Not be able to have multiple prepare/enable calls on the same `clk` (this can
> > limit users that need dynamically enable/disable clocks, with the very limited
> > exception that closure-callback is fine).
> > 2. Do an extra allocation
> > 3. Put lifetime on types that represent a prepared/enabled `Clk`
> > 4. Change C to make `struct clk` refcounted.
>
> It probably comes to no surprise that I'd be more in favor of option 2
> or 4. Maybe option 2 first, so we can get the user-facing API merged
> without too much churn, and then we can see if the clk maintainers are
> happy adding a refcnt to struct clk to optimize things.
>
> If we really feel that the indirection/memory overhead is going to
> hurt us, we can also start with option 1, and extend it to 2 and/or 4
> (needed to add a Clone support) when it becomes evident we can't do
> without it. But as I was saying in my previous reply to Daniel, I
> expect the extra indirection/memory overhead to be negligible since:
>
> 1. clks are usually not {prepared,enabled}/{disabled,unprepared} in a
> hot path
> 2. in the rare occasions where they might be ({dev,cpu}freq ?), this
> clk state change is usually one operation in an ocean of other
> slower operations (regulator reconfiguration, for instance, which
> usually goes over a slow I2C bus, or a
> relatively-faster-but-still-slow SPI one, at least when we compare
> it to an IoMem access for in-SoCs clks). So overall, the clk state
> change might account for a very small portion of the CPU cycles
> spent in this bigger operation
I'm not even too worried about that one. devfreq and cpufreq will
typically adjust the clock rate while device is active, and thus the
clock is enabled. So we shouldn't have a prepared -> enabled transition
in the path that adjust the device / cpu rate, it would have happened
before.
Maxime
On Tue, 3 Feb 2026 16:26:22 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
> >
> > I think it's fine to have all of these:
> > * `Clone` impl
> > * `enable` which consumes `Clk<Prepared>` by value and spit out `Clk<Enabled>`
> > * `with_enabled` that gives `&Clk<Enabled>`
> >
> > This way, if you only want to enable in short time, you can do `with_enabled`.
> > If the closure callback wants to keep clock enabled for longer, it can just do
> > `.clone()` inside the closure and obtain an owned `Clk<Enabled>`.
> >
> > If the user just have a reference and want to enable the callback they can do
> > `prepared_clk.clone().enable()` which gives an owned `Clk<Enabled>`. Thoughts?
> >
> > Best,
> > Gary
>
>
> I’m ok with what you proposed above. The only problem is that implementing
> clone() is done through an Arc<*mut bindings::clk> in Boris’ current
> design,
It's actually Arc<RawClk> with
struct RawClk(*mut bindings::clk);
impl Drop for RawClk {
fn drop(&mut self) {
// SAFETY: By the type invariants, self.as_raw() is a valid argument for // [`clk_put`].
unsafe { bindings::clk_put(self.0) };
}
}
This is because struct clk is not refcounted, so cloning
implies wrapping this object in an Arc, and only calling
clk_put() when the Arc refcnt reaches zero.
> so this requires an extra allocation.
That's true. But the memory overhead should be pretty negligible,
and I don't think the extra indirection makes any noticeable
difference for an actual clk implementation (one that's not a NOP),
since we have indirections all over the place already (clk -> clk_hw,
clk_ops, ...). So I think I'd value ease of use over this small
perfs/mem-usage hit.
On Tue, 3 Feb 2026 13:28:15 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
> >> Now, this may not be the end of the world in this
> >> particular case, but for API consistency, I'd say we should probably avoid this
> >> behavior.
> >>
> >> I see that Alice suggested a closure approach. IMHO, we should use that
> >> instead.
> >
> > The closure, while being useful for the above local clk-enablement
> > example, doesn't allow for passing some Clk<Enabled> guard around, like
> > you would do with a lock Guard, and I believe that's a useful thing to
> > have.
>
>
> Wdym? You’d still get a &Clk<Enabled> that you can pass around, i.e.:
>
> self.prepared_clk.with_enabled(|clk: &Clk<Enabled> | {
> ... use registers, pass &Clk<Enabled> as needed
> });
>
> This is now not about clone() vs not clone(), but more about limiting the scope of the
> Enabled state, which would cater to the use-case you mentioned IIUC.
Fair enough. I guess it's more a matter of taste for that particular
case, and I'm not fundamentally opposed to adding this closure approach.
What the clone approach allows that's not doable with the closure
AFAICT, is something like:
let prep_clk = Clk::get(...)?.prepare()?;
let comp_a = MyComponentA {
prepared_clk: prep_clk.clone(),
}
let comp_b = MyComponentB {
enabled_clk: prep_clk.enable()?,
}
and have the guarantee that, as long as comp_a is alive the underlying
clk is at-least-Prepared, and as long as comp_b is alive the underlying
clk is Enabled.
On Tue Feb 3, 2026 at 1:33 PM GMT, Daniel Almeida wrote:
> Hi Boris,
>
>> On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>
>> On Mon, 19 Jan 2026 12:35:21 +0000
>> Alice Ryhl <aliceryhl@google.com> wrote:
>>
>>> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>>>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>>>>>> For example, it's quite typical to have (at least) one clock for the bus
>>>>>> interface that drives the register, and one that drives the main
>>>>>> component logic. The former needs to be enabled only when you're
>>>>>> accessing the registers (and can be abstracted with
>>>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>>>>> only when the device actually starts operating.
>>>>>>
>>>>>> You have a similar thing for the prepare vs enable thing. The difference
>>>>>> between the two is that enable can be called into atomic context but
>>>>>> prepare can't.
>>>>>>
>>>>>> So for drivers that would care about this, you would create your device
>>>>>> with an unprepared clock, and then at various times during the driver
>>>>>> lifetime, you would mutate that state.
>>>
>>> The case where you're doing it only while accessing registers is
>>> interesting, because that means the Enable bit may be owned by a local
>>> variable. We may imagine an:
>>>
>>> let enabled = self.prepared_clk.enable_scoped();
>>> ... use registers
>>> drop(enabled);
>>>
>>> Now ... this doesn't quite work with the current API - the current
>>> Enabled stated owns both a prepare and enable count, but the above keeps
>>> the prepare count in `self` and the enabled count in a local variable.
>>> But it could be done with a fourth state, or by a closure method:
>>>
>>> self.prepared_clk.with_enabled(|| {
>>> ... use registers
>>> });
>>>
>>> All of this would work with an immutable variable of type Clk<Prepared>.
>>
>> Hm, maybe it'd make sense to implement Clone so we can have a temporary
>> clk variable that has its own prepare/enable refs and releases them
>> as it goes out of scope. This implies wrapping *mut bindings::clk in an
>> Arc<> because bindings::clk is not ARef, but should be relatively easy
>> to do. Posting the quick experiment I did with this approach, in case
>> you're interested [1]
>>
>> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837
>
> The problem with what you have suggested is that the previous state is not
> consumed if you can clone it, and consuming the previous state is a pretty key
> element in ensuring you cannot misuse it. For example, here:
>
> let enabled_clk = prepared_clk.clone().enable()?;
> // do stuff
> // enabled_clk goes out of scope and releases the enable
> // ref it had
>
> prepared_clk is still alive. Now, this may not be the end of the world in this
> particular case, but for API consistency, I'd say we should probably avoid this
> behavior.
Is this an issue though? You cannot mistakenly own `Clk<Enabled>` while the clk
is not enabled, (and similarly for `Prepared`), and that should be sufficient.
Having `Clk<Prepared>` makes no guarantee on if the clk is enabled or not anyway
as you can have another user do `Clk<Unprepared>::get().enable()`.
The only guarantee is that the state the clk have is going to be greater than or
equal to the type state, so allowing cloning an intermediate state is no
problem.
Best,
Gary
>
> I see that Alice suggested a closure approach. IMHO, we should use that
> instead.
>
> — Daniel
> On 3 Feb 2026, at 10:42, Gary Guo <gary@garyguo.net> wrote:
>
> On Tue Feb 3, 2026 at 1:33 PM GMT, Daniel Almeida wrote:
>> Hi Boris,
>>
>>> On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>>
>>> On Mon, 19 Jan 2026 12:35:21 +0000
>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>
>>>> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>>>>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>>>>>>> For example, it's quite typical to have (at least) one clock for the bus
>>>>>>> interface that drives the register, and one that drives the main
>>>>>>> component logic. The former needs to be enabled only when you're
>>>>>>> accessing the registers (and can be abstracted with
>>>>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>>>>>> only when the device actually starts operating.
>>>>>>>
>>>>>>> You have a similar thing for the prepare vs enable thing. The difference
>>>>>>> between the two is that enable can be called into atomic context but
>>>>>>> prepare can't.
>>>>>>>
>>>>>>> So for drivers that would care about this, you would create your device
>>>>>>> with an unprepared clock, and then at various times during the driver
>>>>>>> lifetime, you would mutate that state.
>>>>
>>>> The case where you're doing it only while accessing registers is
>>>> interesting, because that means the Enable bit may be owned by a local
>>>> variable. We may imagine an:
>>>>
>>>> let enabled = self.prepared_clk.enable_scoped();
>>>> ... use registers
>>>> drop(enabled);
>>>>
>>>> Now ... this doesn't quite work with the current API - the current
>>>> Enabled stated owns both a prepare and enable count, but the above keeps
>>>> the prepare count in `self` and the enabled count in a local variable.
>>>> But it could be done with a fourth state, or by a closure method:
>>>>
>>>> self.prepared_clk.with_enabled(|| {
>>>> ... use registers
>>>> });
>>>>
>>>> All of this would work with an immutable variable of type Clk<Prepared>.
>>>
>>> Hm, maybe it'd make sense to implement Clone so we can have a temporary
>>> clk variable that has its own prepare/enable refs and releases them
>>> as it goes out of scope. This implies wrapping *mut bindings::clk in an
>>> Arc<> because bindings::clk is not ARef, but should be relatively easy
>>> to do. Posting the quick experiment I did with this approach, in case
>>> you're interested [1]
>>>
>>> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837
>>
>> The problem with what you have suggested is that the previous state is not
>> consumed if you can clone it, and consuming the previous state is a pretty key
>> element in ensuring you cannot misuse it. For example, here:
>>
>> let enabled_clk = prepared_clk.clone().enable()?;
>> // do stuff
>> // enabled_clk goes out of scope and releases the enable
>> // ref it had
>>
>> prepared_clk is still alive. Now, this may not be the end of the world in this
>> particular case, but for API consistency, I'd say we should probably avoid this
>> behavior.
>
> Is this an issue though? You cannot mistakenly own `Clk<Enabled>` while the clk
> is not enabled, (and similarly for `Prepared`), and that should be sufficient.
It is not an issue. However, I just find it a bit confusing. With a typestate, one
usually expects state transitions where a new state fully consumes the previous
one, and that assumption is “broken” in a way when you add clone().
>
> Having `Clk<Prepared>` makes no guarantee on if the clk is enabled or not anyway
> as you can have another user do `Clk<Unprepared>::get().enable()`.
Although you’re right here, I find this less confusing than clone(). You
have to explicitly craft a new Clk<Enabled>, where a clone() is a shorter way
to basically get around the “state transition” idea on an _existing_ Clk
reference.
This is a bit pedantic on my side though, so I have no problem in adding
clone() if it's the consensus of the majority.
>
> The only guarantee is that the state the clk have is going to be greater than or
> equal to the type state, so allowing cloning an intermediate state is no
> problem.
>
> Best,
> Gary
>
>>
>> I see that Alice suggested a closure approach. IMHO, we should use that
>> instead.
>>
>> — Daniel
Is there any pushback on the closure approach? If so, mind sharing why?
— Daniel
On Tue, 3 Feb 2026 10:55:05 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
> > On 3 Feb 2026, at 10:42, Gary Guo <gary@garyguo.net> wrote:
> >
> > On Tue Feb 3, 2026 at 1:33 PM GMT, Daniel Almeida wrote:
> >> Hi Boris,
> >>
> >>> On 3 Feb 2026, at 07:39, Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >>>
> >>> On Mon, 19 Jan 2026 12:35:21 +0000
> >>> Alice Ryhl <aliceryhl@google.com> wrote:
> >>>
> >>>> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
> >>>>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
> >>>>>>> For example, it's quite typical to have (at least) one clock for the bus
> >>>>>>> interface that drives the register, and one that drives the main
> >>>>>>> component logic. The former needs to be enabled only when you're
> >>>>>>> accessing the registers (and can be abstracted with
> >>>>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
> >>>>>>> only when the device actually starts operating.
> >>>>>>>
> >>>>>>> You have a similar thing for the prepare vs enable thing. The difference
> >>>>>>> between the two is that enable can be called into atomic context but
> >>>>>>> prepare can't.
> >>>>>>>
> >>>>>>> So for drivers that would care about this, you would create your device
> >>>>>>> with an unprepared clock, and then at various times during the driver
> >>>>>>> lifetime, you would mutate that state.
> >>>>
> >>>> The case where you're doing it only while accessing registers is
> >>>> interesting, because that means the Enable bit may be owned by a local
> >>>> variable. We may imagine an:
> >>>>
> >>>> let enabled = self.prepared_clk.enable_scoped();
> >>>> ... use registers
> >>>> drop(enabled);
> >>>>
> >>>> Now ... this doesn't quite work with the current API - the current
> >>>> Enabled stated owns both a prepare and enable count, but the above keeps
> >>>> the prepare count in `self` and the enabled count in a local variable.
> >>>> But it could be done with a fourth state, or by a closure method:
> >>>>
> >>>> self.prepared_clk.with_enabled(|| {
> >>>> ... use registers
> >>>> });
> >>>>
> >>>> All of this would work with an immutable variable of type Clk<Prepared>.
> >>>
> >>> Hm, maybe it'd make sense to implement Clone so we can have a temporary
> >>> clk variable that has its own prepare/enable refs and releases them
> >>> as it goes out of scope. This implies wrapping *mut bindings::clk in an
> >>> Arc<> because bindings::clk is not ARef, but should be relatively easy
> >>> to do. Posting the quick experiment I did with this approach, in case
> >>> you're interested [1]
> >>>
> >>> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/d5d04da4f4f6192b6e6760d5f861c69596c7d837
> >>
> >> The problem with what you have suggested is that the previous state is not
> >> consumed if you can clone it, and consuming the previous state is a pretty key
> >> element in ensuring you cannot misuse it. For example, here:
> >>
> >> let enabled_clk = prepared_clk.clone().enable()?;
> >> // do stuff
> >> // enabled_clk goes out of scope and releases the enable
> >> // ref it had
> >>
> >> prepared_clk is still alive. Now, this may not be the end of the world in this
> >> particular case, but for API consistency, I'd say we should probably avoid this
> >> behavior.
> >
> > Is this an issue though? You cannot mistakenly own `Clk<Enabled>` while the clk
> > is not enabled, (and similarly for `Prepared`), and that should be sufficient.
>
> It is not an issue. However, I just find it a bit confusing. With a typestate, one
> usually expects state transitions where a new state fully consumes the previous
> one, and that assumption is “broken” in a way when you add clone().
It's just the way clks work in practice: you having a Clk<Unprepared>
doesn't mean the underlying clk_hw (the C object) is in an unprepared
state, because some other users might point to the same clk_hw and have
it enabled already. What Clk<State> means here is that you have a local
view of a clk that's in at least this State. In order to guarantee that
the clk is at least OtherState, you'll have to transition your view to
this OtherState.
Clone here just means you're cloning a view of this clone in its
original view state. Then you're free to do whatever you want on this
new view. So is the original owner of the object you clone from.
>
> >
> > Having `Clk<Prepared>` makes no guarantee on if the clk is enabled or not anyway
> > as you can have another user do `Clk<Unprepared>::get().enable()`.
>
> Although you’re right here, I find this less confusing than clone(). You
> have to explicitly craft a new Clk<Enabled>, where a clone() is a shorter way
> to basically get around the “state transition” idea on an _existing_ Clk
> reference.
The idea behind the clone() is that you can transition from one state
to an higher state (prepared -> enabled for instance) for a shorter
period of time than the cloned clk lifetime. Something like that, for
instance:
let MyDevice {
prepared_clk: Clk::get(...)?.prepare()?,
}
implem MyDevice {
fn do_stuff(&self) {
let enabled_clk = self.prepared_clk.clone();
// do stuff that need to be guaranteed that clk
// is enabled
self.do_other_stuff(enabled_clk);
// the enabled_clk object is dropped, but the
// clk remains prepared because
// self.prepared_clk is still there
}
}
On Tue, 3 Feb 2026 11:39:02 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> On Mon, 19 Jan 2026 12:35:21 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
> > > On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
> > > > > For example, it's quite typical to have (at least) one clock for the bus
> > > > > interface that drives the register, and one that drives the main
> > > > > component logic. The former needs to be enabled only when you're
> > > > > accessing the registers (and can be abstracted with
> > > > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > > > > only when the device actually starts operating.
> > > > >
> > > > > You have a similar thing for the prepare vs enable thing. The difference
> > > > > between the two is that enable can be called into atomic context but
> > > > > prepare can't.
> > > > >
> > > > > So for drivers that would care about this, you would create your device
> > > > > with an unprepared clock, and then at various times during the driver
> > > > > lifetime, you would mutate that state.
> >
> > The case where you're doing it only while accessing registers is
> > interesting, because that means the Enable bit may be owned by a local
> > variable. We may imagine an:
> >
> > let enabled = self.prepared_clk.enable_scoped();
> > ... use registers
> > drop(enabled);
> >
> > Now ... this doesn't quite work with the current API - the current
> > Enabled stated owns both a prepare and enable count, but the above keeps
> > the prepare count in `self` and the enabled count in a local variable.
> > But it could be done with a fourth state, or by a closure method:
> >
> > self.prepared_clk.with_enabled(|| {
> > ... use registers
> > });
> >
> > All of this would work with an immutable variable of type Clk<Prepared>.
>
> Hm, maybe it'd make sense to implement Clone so we can have a temporary
> clk variable that has its own prepare/enable refs and releases them
> as it goes out of scope. This implies wrapping *mut bindings::clk in an
> Arc<> because bindings::clk is not ARef, but should be relatively easy
> to do. Posting the quick experiment I did with this approach, in case
> you're interested [1]
This time with a proper RawClk(*mut bindings::clk) wrapper, so we can
clk_put() called in RawClk::drop() instead of in Clk::drop().
[1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/6fa6cb72f14373b276c61d038bc2b16f49c78f74
On Tue, 3 Feb 2026 12:26:31 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> On Tue, 3 Feb 2026 11:39:02 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
> > On Mon, 19 Jan 2026 12:35:21 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > > On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
> > > > On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
> > > > > > For example, it's quite typical to have (at least) one clock for the bus
> > > > > > interface that drives the register, and one that drives the main
> > > > > > component logic. The former needs to be enabled only when you're
> > > > > > accessing the registers (and can be abstracted with
> > > > > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
> > > > > > only when the device actually starts operating.
> > > > > >
> > > > > > You have a similar thing for the prepare vs enable thing. The difference
> > > > > > between the two is that enable can be called into atomic context but
> > > > > > prepare can't.
> > > > > >
> > > > > > So for drivers that would care about this, you would create your device
> > > > > > with an unprepared clock, and then at various times during the driver
> > > > > > lifetime, you would mutate that state.
> > >
> > > The case where you're doing it only while accessing registers is
> > > interesting, because that means the Enable bit may be owned by a local
> > > variable. We may imagine an:
> > >
> > > let enabled = self.prepared_clk.enable_scoped();
> > > ... use registers
> > > drop(enabled);
> > >
> > > Now ... this doesn't quite work with the current API - the current
> > > Enabled stated owns both a prepare and enable count, but the above keeps
> > > the prepare count in `self` and the enabled count in a local variable.
> > > But it could be done with a fourth state, or by a closure method:
> > >
> > > self.prepared_clk.with_enabled(|| {
> > > ... use registers
> > > });
> > >
> > > All of this would work with an immutable variable of type Clk<Prepared>.
> >
> > Hm, maybe it'd make sense to implement Clone so we can have a temporary
> > clk variable that has its own prepare/enable refs and releases them
> > as it goes out of scope. This implies wrapping *mut bindings::clk in an
> > Arc<> because bindings::clk is not ARef, but should be relatively easy
> > to do. Posting the quick experiment I did with this approach, in case
> > you're interested [1]
>
> This time with a proper RawClk(*mut bindings::clk) wrapper, so we can
> clk_put() called in RawClk::drop() instead of in Clk::drop().
>
> [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/6fa6cb72f14373b276c61d038bc2b16f49c78f74
And I forgot to drop the ManuallyDrop in that one, but I bet you get the
idea.
On Mon, Jan 19, 2026 at 12:35:21PM +0000, Alice Ryhl wrote: > > > In fact, I specifically wanted to ensure that this was possible when writing > > > these patches, as it’s needed by drivers. If you want to, I can cover that in > > > the examples, no worries. > > > > Yes, that would be great. I do wonder though if it wouldn't make sense > > to turn it the other way around. It creates a fair share of boilerplate > > for a number of drivers. Can't we keep Clk the way it is as a > > lower-level type, and crate a ManagedClk (or whatever name you prefer) > > that drivers can use, and would be returned by higher-level helpers, if > > they so choose? > > > > That way, we do have the typestate API for whoever wants to, without > > creating too much boilerplate for everybody else. > > I think that if you still want an API where you just call enable/disable > directly on it with no protection against unbalanced calls, then that > should be the special API. Probably called RawClk and functions marked > unsafe. Unbalanced calls seem really dangerous and use should not be > encouraged. Sure, that makes sense to me. > The current type-state based API is the least-boilerplate option for > drivers that exist today. I wasn't saying that we should add boilerplate to the typestate API either. I was saying that the non-typestated API was common enough that we probably didn't want boilerplate for it either if possible. Maxime
On Mon Jan 19, 2026 at 12:35 PM GMT, Alice Ryhl wrote:
> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>> > > For example, it's quite typical to have (at least) one clock for the bus
>> > > interface that drives the register, and one that drives the main
>> > > component logic. The former needs to be enabled only when you're
>> > > accessing the registers (and can be abstracted with
>> > > regmap_mmio_attach_clk for example), and the latter needs to be enabled
>> > > only when the device actually starts operating.
>> > >
>> > > You have a similar thing for the prepare vs enable thing. The difference
>> > > between the two is that enable can be called into atomic context but
>> > > prepare can't.
>> > >
>> > > So for drivers that would care about this, you would create your device
>> > > with an unprepared clock, and then at various times during the driver
>> > > lifetime, you would mutate that state.
>
> The case where you're doing it only while accessing registers is
> interesting, because that means the Enable bit may be owned by a local
> variable. We may imagine an:
>
> let enabled = self.prepared_clk.enable_scoped();
> ... use registers
> drop(enabled);
>
> Now ... this doesn't quite work with the current API - the current
> Enabled stated owns both a prepare and enable count, but the above keeps
> the prepare count in `self` and the enabled count in a local variable.
> But it could be done with a fourth state, or by a closure method:
>
> self.prepared_clk.with_enabled(|| {
> ... use registers
> });
>
> All of this would work with an immutable variable of type Clk<Prepared>.
>
>> > > AFAIU, encoding the state of the clock into the Clk type (and thus
>> > > forcing the structure that holds it) prevents that mutation. If not, we
>> > > should make it clearer (by expanding the doc maybe?) how such a pattern
>> > > can be supported.
>> > >
>> > > Maxime
>> >
>> > IIUC, your main point seems to be about mutating the state at runtime? This is
>> > possible with this code. You can just have an enum, for example:
>> >
>> > enum MyClocks {
>> > Unprepared(Clk<Unprepared>),
>> > Prepared(Clk<Prepared>),
>> > Enabled(Clk<Enabled>),
>> > }
>
> I believe you need an extra state if the state is not bound to the scope
> of a function:
>
> enum MyClocks {
> Unprepared(Clk<Unprepared>),
> Prepared(Clk<Prepared>),
> Enabled(Clk<Enabled>),
> Transitioning,
> }
>
> since mem::replace() needs a new value before you can take ownership of
> the existing Clk value.
We can provide `replace_with` in the kernel, it's fine as we don't have
unwinding panics.
Best,
Gary
> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Mon, Jan 19, 2026 at 11:45:57AM +0100, Maxime Ripard wrote:
>> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>>>> For example, it's quite typical to have (at least) one clock for the bus
>>>> interface that drives the register, and one that drives the main
>>>> component logic. The former needs to be enabled only when you're
>>>> accessing the registers (and can be abstracted with
>>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>>> only when the device actually starts operating.
>>>>
>>>> You have a similar thing for the prepare vs enable thing. The difference
>>>> between the two is that enable can be called into atomic context but
>>>> prepare can't.
>>>>
>>>> So for drivers that would care about this, you would create your device
>>>> with an unprepared clock, and then at various times during the driver
>>>> lifetime, you would mutate that state.
>
> The case where you're doing it only while accessing registers is
> interesting, because that means the Enable bit may be owned by a local
> variable. We may imagine an:
>
> let enabled = self.prepared_clk.enable_scoped();
> ... use registers
> drop(enabled);
Not sure I understand. You can get a Clk<Enabled>, do what you need, and then
consume Clk<Enabled> to go back to Clk<Prepared>. I think I added this, but if
I didn’t, it’s a trivial thing to do.
>
> Now ... this doesn't quite work with the current API - the current
> Enabled stated owns both a prepare and enable count, but the above keeps
> the prepare count in `self` and the enabled count in a local variable.
> But it could be done with a fourth state, or by a closure method:
>
> self.prepared_clk.with_enabled(|| {
> ... use registers
> });
>
> All of this would work with an immutable variable of type Clk<Prepared>.
>
>>>> AFAIU, encoding the state of the clock into the Clk type (and thus
>>>> forcing the structure that holds it) prevents that mutation. If not, we
>>>> should make it clearer (by expanding the doc maybe?) how such a pattern
>>>> can be supported.
>>>>
>>>> Maxime
>>>
>>> IIUC, your main point seems to be about mutating the state at runtime? This is
>>> possible with this code. You can just have an enum, for example:
>>>
>>> enum MyClocks {
>>> Unprepared(Clk<Unprepared>),
>>> Prepared(Clk<Prepared>),
>>> Enabled(Clk<Enabled>),
>>> }
>
> I believe you need an extra state if the state is not bound to the scope
> of a function:
>
> enum MyClocks {
> Unprepared(Clk<Unprepared>),
> Prepared(Clk<Prepared>),
> Enabled(Clk<Enabled>),
> Transitioning,
> }
>
> since mem::replace() needs a new value before you can take ownership of
> the existing Clk value.
Right, I need to update the docs to account for this, as they imply that you
can do this with only two states.
>
>>> In fact, I specifically wanted to ensure that this was possible when writing
>>> these patches, as it’s needed by drivers. If you want to, I can cover that in
>>> the examples, no worries.
>>
>> Yes, that would be great. I do wonder though if it wouldn't make sense
>> to turn it the other way around. It creates a fair share of boilerplate
>> for a number of drivers. Can't we keep Clk the way it is as a
>> lower-level type, and crate a ManagedClk (or whatever name you prefer)
>> that drivers can use, and would be returned by higher-level helpers, if
>> they so choose?
>>
>> That way, we do have the typestate API for whoever wants to, without
>> creating too much boilerplate for everybody else.
>
> I think that if you still want an API where you just call enable/disable
> directly on it with no protection against unbalanced calls, then that
> should be the special API. Probably called RawClk and functions marked
> unsafe. Unbalanced calls seem really dangerous and use should not be
> encouraged.
I think we should discourage RawClk if at all possible. But if the consensus
is that we *really* need this easily-abused thing, I can provide a follow-up.
>
> The current type-state based API is the least-boilerplate option for
> drivers that exist today.
>
> Alice
On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote: >> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote: >> I think that if you still want an API where you just call enable/disable >> directly on it with no protection against unbalanced calls, then that >> should be the special API. Probably called RawClk and functions marked >> unsafe. Unbalanced calls seem really dangerous and use should not be >> encouraged. +1; and unless there is a use-case that requires otherwise, it should not even be possible to do this at all -- at least for driver code. > I think we should discourage RawClk if at all possible. But if the consensus > is that we *really* need this easily-abused thing, I can provide a follow-up. I think we should only do this if there are use-case with no alternative, so far there haven't been any AFAIK.
On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote: > On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote: > >> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote: > >> I think that if you still want an API where you just call enable/disable > >> directly on it with no protection against unbalanced calls, then that > >> should be the special API. Probably called RawClk and functions marked > >> unsafe. Unbalanced calls seem really dangerous and use should not be > >> encouraged. > > +1; and unless there is a use-case that requires otherwise, it should not even > be possible to do this at all -- at least for driver code. I mean, it's great, it's safe, etc. but it's also suboptimal from a PM perspective on many platforms. It's totally fine to provide nice, safe, ergonomic wrappers for the drivers that don't care (or can't, really), but treating a legitimate optimisation as something we should consider impossible to do is just weird to me. > > I think we should discourage RawClk if at all possible. But if the consensus > > is that we *really* need this easily-abused thing, I can provide a follow-up. > > I think we should only do this if there are use-case with no alternative, so far > there haven't been any AFAIK. I don't really care about which alternative we come up with, but look at devm_regmap_init_mmio_clk for example. It is a valid use-case that already exists today, and has had for more than a decade at this point. Maxime
On Mon Jan 19, 2026 at 3:18 PM CET, Maxime Ripard wrote: > On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote: >> On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote: >> >> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote: >> >> I think that if you still want an API where you just call enable/disable >> >> directly on it with no protection against unbalanced calls, then that >> >> should be the special API. Probably called RawClk and functions marked >> >> unsafe. Unbalanced calls seem really dangerous and use should not be >> >> encouraged. >> >> +1; and unless there is a use-case that requires otherwise, it should not even >> be possible to do this at all -- at least for driver code. > > I mean, it's great, it's safe, etc. but it's also suboptimal from a PM > perspective on many platforms. It's totally fine to provide nice, safe, > ergonomic wrappers for the drivers that don't care (or can't, really), > but treating a legitimate optimisation as something we should consider > impossible to do is just weird to me. I said that an unsafe API with potentially unbalanced calls is something we should clearly avoid for drivers. This is *not* equivalent to "treating a legitimate optimisation as something we should consider impossible". If we discover use-cases where the current API doesn't work well, we can invenstigate further. >> > I think we should discourage RawClk if at all possible. But if the consensus >> > is that we *really* need this easily-abused thing, I can provide a follow-up. >> >> I think we should only do this if there are use-case with no alternative, so far >> there haven't been any AFAIK. > > I don't really care about which alternative we come up with, but look at > devm_regmap_init_mmio_clk for example. It is a valid use-case that > already exists today, and has had for more than a decade at this point. I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference count of the clock and prepares it when called and unprepares the clk in drops its reference in regmap_mmio_free_context() called from the devres callback. That something we can easily do with the current API, no?
On Mon, Jan 19, 2026 at 03:37:17PM +0100, Danilo Krummrich wrote: > On Mon Jan 19, 2026 at 3:18 PM CET, Maxime Ripard wrote: > > On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote: > >> On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote: > >> >> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote: > >> >> I think that if you still want an API where you just call enable/disable > >> >> directly on it with no protection against unbalanced calls, then that > >> >> should be the special API. Probably called RawClk and functions marked > >> >> unsafe. Unbalanced calls seem really dangerous and use should not be > >> >> encouraged. > >> > >> +1; and unless there is a use-case that requires otherwise, it should not even > >> be possible to do this at all -- at least for driver code. > > > > I mean, it's great, it's safe, etc. but it's also suboptimal from a PM > > perspective on many platforms. It's totally fine to provide nice, safe, > > ergonomic wrappers for the drivers that don't care (or can't, really), > > but treating a legitimate optimisation as something we should consider > > impossible to do is just weird to me. > > I said that an unsafe API with potentially unbalanced calls is something we > should clearly avoid for drivers. This is *not* equivalent to "treating a > legitimate optimisation as something we should consider impossible". > > If we discover use-cases where the current API doesn't work well, we can > invenstigate further. I'm not sure I'm following what you're saying, sorry. I've pointed out such a use-case already. > >> > I think we should discourage RawClk if at all possible. But if the consensus > >> > is that we *really* need this easily-abused thing, I can provide a follow-up. > >> > >> I think we should only do this if there are use-case with no alternative, so far > >> there haven't been any AFAIK. > > > > I don't really care about which alternative we come up with, but look at > > devm_regmap_init_mmio_clk for example. It is a valid use-case that > > already exists today, and has had for more than a decade at this point. > > I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference > count of the clock and prepares it when called and unprepares the clk in drops > its reference in regmap_mmio_free_context() called from the devres callback. > > That something we can easily do with the current API, no? The current one, yes. Doing that in the API suggested here would involve some boilerplate in all those drivers they don't have right now. Maxime
On Thu Jan 22, 2026 at 2:44 PM CET, Maxime Ripard wrote:
> On Mon, Jan 19, 2026 at 03:37:17PM +0100, Danilo Krummrich wrote:
>> I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference
>> count of the clock and prepares it when called and unprepares the clk in drops
>> its reference in regmap_mmio_free_context() called from the devres callback.
>>
>> That something we can easily do with the current API, no?
>
> The current one, yes. Doing that in the API suggested here would involve
> some boilerplate in all those drivers they don't have right now.
No, I did mean the API suggested here.
If you would implement something like devm_regmap_init_mmio_clk() in Rust with
this API, you'd have some object like
struct RegmapResource<T: Backend, R: Resource> {
map: Regmap<T>,
res: R,
}
and a concrete instance could have the following type
RegmapResource<MmIo, Clk<Prepared>>
So, eventually you could have:
fn devm_regmap_init_mmio_clk(dev: &Device<Bound>, name: &CStr, ...) -> ... {
let clk: Clk<Prepared> = Clk::get(dev, name)?;
let regmap = RegmapResource::new(..., clk);
Devres::new(dev, regmap)
}
Of course, we would never design the API in this way, as we have generic I/O
backends and register abstractions, and we'd also not have
devm_regmap_init_mmio_clk() as a constructor for a RegmapResource type, but you
get the idea.
> On 22 Jan 2026, at 10:44, Maxime Ripard <mripard@kernel.org> wrote: > > On Mon, Jan 19, 2026 at 03:37:17PM +0100, Danilo Krummrich wrote: >> On Mon Jan 19, 2026 at 3:18 PM CET, Maxime Ripard wrote: >>> On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote: >>>> On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote: >>>>>> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote: >>>>>> I think that if you still want an API where you just call enable/disable >>>>>> directly on it with no protection against unbalanced calls, then that >>>>>> should be the special API. Probably called RawClk and functions marked >>>>>> unsafe. Unbalanced calls seem really dangerous and use should not be >>>>>> encouraged. >>>> >>>> +1; and unless there is a use-case that requires otherwise, it should not even >>>> be possible to do this at all -- at least for driver code. >>> >>> I mean, it's great, it's safe, etc. but it's also suboptimal from a PM >>> perspective on many platforms. It's totally fine to provide nice, safe, >>> ergonomic wrappers for the drivers that don't care (or can't, really), >>> but treating a legitimate optimisation as something we should consider >>> impossible to do is just weird to me. >> >> I said that an unsafe API with potentially unbalanced calls is something we >> should clearly avoid for drivers. This is *not* equivalent to "treating a >> legitimate optimisation as something we should consider impossible". >> >> If we discover use-cases where the current API doesn't work well, we can >> invenstigate further. > > I'm not sure I'm following what you're saying, sorry. I've pointed out > such a use-case already. > >>>>> I think we should discourage RawClk if at all possible. But if the consensus >>>>> is that we *really* need this easily-abused thing, I can provide a follow-up. >>>> >>>> I think we should only do this if there are use-case with no alternative, so far >>>> there haven't been any AFAIK. >>> >>> I don't really care about which alternative we come up with, but look at >>> devm_regmap_init_mmio_clk for example. It is a valid use-case that >>> already exists today, and has had for more than a decade at this point. >> >> I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference >> count of the clock and prepares it when called and unprepares the clk in drops >> its reference in regmap_mmio_free_context() called from the devres callback. >> >> That something we can easily do with the current API, no? > > The current one, yes. Doing that in the API suggested here would involve > some boilerplate in all those drivers they don't have right now. > > Maxime Maxime, I know you’ve already pointed out a use-case, but I think the confusion stems from why you seem to think that the current solution cannot cater to the API you mentioned in a clean way. You seem to imply that there will be a lot of boilerplate involved, but we (or I) cannot see this. Perhaps it would help if you highlighted how exactly the type state solution would be verbose using some pseudocode. I guess that would make your point clearer for us. — Daniel
On Thu, Jan 22, 2026 at 09:29:30PM -0300, Daniel Almeida wrote: > > > > On 22 Jan 2026, at 10:44, Maxime Ripard <mripard@kernel.org> wrote: > > > > On Mon, Jan 19, 2026 at 03:37:17PM +0100, Danilo Krummrich wrote: > >> On Mon Jan 19, 2026 at 3:18 PM CET, Maxime Ripard wrote: > >>> On Mon, Jan 19, 2026 at 02:13:48PM +0100, Danilo Krummrich wrote: > >>>> On Mon Jan 19, 2026 at 1:54 PM CET, Daniel Almeida wrote: > >>>>>> On 19 Jan 2026, at 09:35, Alice Ryhl <aliceryhl@google.com> wrote: > >>>>>> I think that if you still want an API where you just call enable/disable > >>>>>> directly on it with no protection against unbalanced calls, then that > >>>>>> should be the special API. Probably called RawClk and functions marked > >>>>>> unsafe. Unbalanced calls seem really dangerous and use should not be > >>>>>> encouraged. > >>>> > >>>> +1; and unless there is a use-case that requires otherwise, it should not even > >>>> be possible to do this at all -- at least for driver code. > >>> > >>> I mean, it's great, it's safe, etc. but it's also suboptimal from a PM > >>> perspective on many platforms. It's totally fine to provide nice, safe, > >>> ergonomic wrappers for the drivers that don't care (or can't, really), > >>> but treating a legitimate optimisation as something we should consider > >>> impossible to do is just weird to me. > >> > >> I said that an unsafe API with potentially unbalanced calls is something we > >> should clearly avoid for drivers. This is *not* equivalent to "treating a > >> legitimate optimisation as something we should consider impossible". > >> > >> If we discover use-cases where the current API doesn't work well, we can > >> invenstigate further. > > > > I'm not sure I'm following what you're saying, sorry. I've pointed out > > such a use-case already. > > > >>>>> I think we should discourage RawClk if at all possible. But if the consensus > >>>>> is that we *really* need this easily-abused thing, I can provide a follow-up. > >>>> > >>>> I think we should only do this if there are use-case with no alternative, so far > >>>> there haven't been any AFAIK. > >>> > >>> I don't really care about which alternative we come up with, but look at > >>> devm_regmap_init_mmio_clk for example. It is a valid use-case that > >>> already exists today, and has had for more than a decade at this point. > >> > >> I don't see the issue with devm_regmap_init_mmio_clk()? It takes a reference > >> count of the clock and prepares it when called and unprepares the clk in drops > >> its reference in regmap_mmio_free_context() called from the devres callback. > >> > >> That something we can easily do with the current API, no? > > > > The current one, yes. Doing that in the API suggested here would involve > > some boilerplate in all those drivers they don't have right now. > > > > Maxime > > Maxime, I know you’ve already pointed out a use-case, but I think the > confusion stems from why you seem to think that the current solution cannot > cater to the API you mentioned in a clean way. You seem to imply that there > will be a lot of boilerplate involved, but we (or I) cannot see this. Perhaps > it would help if you highlighted how exactly the type state solution would be > verbose using some pseudocode. I guess that would make your point clearer for > us. I'm probably missing something then, but let's assume you have a driver that wants its clock prepared and enabled in an hypothetical enable() callback, and disabled / unprepared in a disable() callback. From a PM management perspective, this usecase makes total sense, is a valid usecase, is widely used in the kernel, and is currently supported by both the C and Rust clk APIs. The only solution to this you suggested so far (I think?) to implement this on top of the new clk API you propose is to have a driver specific enum that would store each of the possible state transition. That's the boilerplate I'm talking about. If every driver wanting to implement that pattern has to make such an enum, with all the relevant traits implementation that might come with it, we go from an API where everything works at no-cost from a code-size perspective to a situation where every driver has to develop and maintain that enum. Maxime
> > I'm probably missing something then, but let's assume you have a driver > that wants its clock prepared and enabled in an hypothetical enable() > callback, and disabled / unprepared in a disable() callback. > > From a PM management perspective, this usecase makes total sense, is a > valid usecase, is widely used in the kernel, and is currently supported > by both the C and Rust clk APIs. > > The only solution to this you suggested so far (I think?) to implement > this on top of the new clk API you propose is to have a driver specific > enum that would store each of the possible state transition. Yes, you need an enum _if_ you want to model transitions at runtime. IIUC you only need two variants to implement the pattern you described. I do not consider this “boilerplate”, but rather a small cost to pay. I would understand if this was some elaborate pattern that had to be implemented by all drivers, but a two-variant enum is as straightforward as it gets. > > That's the boilerplate I'm talking about. If every driver wanting to > implement that pattern has to make such an enum, with all the relevant > traits implementation that might come with it, we go from an API where > everything works at no-cost from a code-size perspective to a situation > where every driver has to develop and maintain that enum. > > Maxime There are no "traits that come with it". It's just an enum, with two variants. > API where everything works at no-cost The previous API was far from “everything works”. It was fundamentally broken by design in multiple ways, i.e.: > 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(). IMHO, what we have here is an improvement that has been long overdue. — Daniel
On Wed, Feb 04, 2026 at 09:43:55AM -0300, Daniel Almeida wrote: > > I'm probably missing something then, but let's assume you have a driver > > that wants its clock prepared and enabled in an hypothetical enable() > > callback, and disabled / unprepared in a disable() callback. > > > > From a PM management perspective, this usecase makes total sense, is a > > valid usecase, is widely used in the kernel, and is currently supported > > by both the C and Rust clk APIs. > > > > The only solution to this you suggested so far (I think?) to implement > > this on top of the new clk API you propose is to have a driver specific > > enum that would store each of the possible state transition. > > Yes, you need an enum _if_ you want to model transitions at runtime. IIUC you > only need two variants to implement the pattern you described. I do not > consider this “boilerplate”, but rather a small cost to pay. A maintenance cost to pay by every driver is kind of the textbook definition of boilerplate to me. > I would understand if this was some elaborate pattern that had to be > implemented by all drivers, but a two-variant enum is as > straightforward as it gets. And yet, that framework has dozens of helpers that do not remove anything from drivers but a couple of lines. So surely its users must find value in reducing that boilerplate as much as possible. And you do implement some of them, so you must find value in that too. > > That's the boilerplate I'm talking about. If every driver wanting to > > implement that pattern has to make such an enum, with all the relevant > > traits implementation that might come with it, we go from an API where > > everything works at no-cost from a code-size perspective to a situation > > where every driver has to develop and maintain that enum. > > There are no "traits that come with it". It's just an enum, with two > variants. > > > API where everything works at no-cost > > The previous API was far from “everything works”. It was fundamentally > broken by design in multiple ways, i.e.: Out of context and not what I meant, but ok. > > 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(). > > IMHO, what we have here is an improvement that has been long overdue. Nothing is absolute. It is indeed an improvement on the refcounting side of things and general safety of the API for the general case. I don't think I ever questionned that. However, for the use-cases we've been discussing (and dozens of drivers implementing it), it also comes with a regression in the amount of code to create and maintain. They used to be able to only deal with the Clk structure, and now they can't anymore. You might find that neglible, you might have a plan to address that in the future, etc. and that's fine, but if you can't acknowledge that it's indeed happening, there's no point in me raising the issue and continuing the discussion. Maxime
> On 19 Jan 2026, at 07:45, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Thu, Jan 08, 2026 at 11:14:37AM -0300, Daniel Almeida wrote:
>> Hi Maxime :)
>>
>>>
>>> I don't know the typestate pattern that well, but I wonder if we don't
>>> paint ourselves into a corner by introducing it.
>>>
>>> While it's pretty common to get your clock from the get go into a state,
>>> and then don't modify it (like what devm_clk_get_enabled provides for
>>> example), and the typestate pattern indeed works great for those, we
>>
>> Minor correction, devm_clk_get_enabled is not handled by the typestate
>> pattern. The next patch does include this function for convenience, but
>> you get a Result<()>. The typestate pattern is used when you want more
>> control.
>>
>>> also have a significant number of drivers that will have a finer-grained
>>> control over the clock enablement for PM.
>>>
>>> For example, it's quite typical to have (at least) one clock for the bus
>>> interface that drives the register, and one that drives the main
>>> component logic. The former needs to be enabled only when you're
>>> accessing the registers (and can be abstracted with
>>> regmap_mmio_attach_clk for example), and the latter needs to be enabled
>>> only when the device actually starts operating.
>>>
>>> You have a similar thing for the prepare vs enable thing. The difference
>>> between the two is that enable can be called into atomic context but
>>> prepare can't.
>>>
>>> So for drivers that would care about this, you would create your device
>>> with an unprepared clock, and then at various times during the driver
>>> lifetime, you would mutate that state.
>>>
>>> AFAIU, encoding the state of the clock into the Clk type (and thus
>>> forcing the structure that holds it) prevents that mutation. If not, we
>>> should make it clearer (by expanding the doc maybe?) how such a pattern
>>> can be supported.
>>>
>>> Maxime
>>
>> IIUC, your main point seems to be about mutating the state at runtime? This is
>> possible with this code. You can just have an enum, for example:
>>
>> enum MyClocks {
>> Unprepared(Clk<Unprepared>),
>> Prepared(Clk<Prepared>),
>> Enabled(Clk<Enabled>),
>> }
>>
>> In fact, I specifically wanted to ensure that this was possible when writing
>> these patches, as it’s needed by drivers. If you want to, I can cover that in
>> the examples, no worries.
>
> Yes, that would be great. I do wonder though if it wouldn't make sense
> to turn it the other way around. It creates a fair share of boilerplate
> for a number of drivers. Can't we keep Clk the way it is as a
> lower-level type, and crate a ManagedClk (or whatever name you prefer)
> that drivers can use, and would be returned by higher-level helpers, if
> they so choose?
The problem with keeping it the way it is that you’re back to manual
prepare/unprepare and enable/disable, as the type-state is what’s enforcing
the correct order of calls. This is also the case when the type is dropped.
In fact, one of the aims of this patch is to get rid of the current Clk type
before we have more users. The current series fixes this by enforcing a sane
order of operations. Most importantly, it enforces that the refcounts to get(),
enable() and etc are handled correctly using the type system.
It rids us of this problem, which is possible today:
clk.enable();
clk.prepare();
clk.prepare();
clk.disable_unprepare();
clk.set_rate();
>
> That way, we do have the typestate API for whoever wants to, without
> creating too much boilerplate for everybody else.
But that's how it works in this series. The typestate pattern is opt-in. If you
need to "set and forget" there's the devm API that's introduced by the next
patch. I can expose more devm_* APIs if you want.
I'm not sure the boilerplate is significant, by the way. You can just do:
Clk::<Enabled>::get();
As a starting point, and have the enum thing (which is also simple) _if_ you
need to manually enable/disable at runtime. Most of the time, you will only
need to mention the type state once, like I did in the call above, and then
the type system will figure out the rest when transitions take place.
What boilerplate did you have in mind?
— Daniel
On Thu, Jan 8, 2026 at 9:07 AM Maxime Ripard <mripard@kernel.org> wrote: > > AFAIU, encoding the state of the clock into the Clk type (and thus > forcing the structure that holds it) prevents that mutation. If not, we > should make it clearer (by expanding the doc maybe?) how such a pattern > can be supported. One possibility to consider in cases like this is whether supporting both cases differently makes sense, i.e. one for that covers easily/safely/... the usual "80%" of cases, and another "advanced" one (possibly unsafe etc.) for the rest. While it may be a bit more to maintain, it may pay itself off by making it easier to review the easy ones if the majority only need that etc. Cheers, Miguel
> On 8 Jan 2026, at 10:57, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Thu, Jan 8, 2026 at 9:07 AM Maxime Ripard <mripard@kernel.org> wrote: >> >> AFAIU, encoding the state of the clock into the Clk type (and thus >> forcing the structure that holds it) prevents that mutation. If not, we >> should make it clearer (by expanding the doc maybe?) how such a pattern >> can be supported. > > One possibility to consider in cases like this is whether supporting > both cases differently makes sense, i.e. one for that covers > easily/safely/... the usual "80%" of cases, and another "advanced" one > (possibly unsafe etc.) for the rest. > > While it may be a bit more to maintain, it may pay itself off by > making it easier to review the easy ones if the majority only need > that etc. > > Cheers, > Miguel This is already the case. The devm_* stuff in the next patch covers a lot of simpler drivers (who might simply want to enable the clk, perhaps with a given rate), while the typestate pattern allows for more control when needed. There will be users for both, IMHO. — Daniel
© 2016 - 2026 Red Hat, Inc.