* [PATCH v4 0/3] Clk improvements
@ 2026-06-18 3:46 ` Daniel Almeida
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Almeida @ 2026-06-18 3:46 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Michal Wilczynski,
Brian Masney, Boqun Feng, Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
This series contains a few improvements that simplifies clock handling for
drivers.
Patch 1 implements the same typestate pattern that has been used
successfully for Regulators. This is needed because otherwise drivers
will be responsible for unpreparing and disabling clocks themselves and
ultimately handling the reference counts on their own. This is
undesirable. The patch automatically encodes this information using the
type system so that no misuse can occur.
Patch 2 makes things more convenient by offering devres-managed APIs. This
lets drivers set clock parameters once and forget about lifetime
management.
Patch 3 converts clk.rs to the newer kernel-vertical style in order to make
future changes easier.
Alice Ryhl's "rust: clk: implement Send and Sync" series, which this used to
depend on, has since been merged, so this series now applies directly on
clk-next.
---
Changes in v4:
- Rebased onto clk-next. Alice Ryhl's "rust: clk: implement Send and Sync"
series is now merged upstream, so it is no longer carried as a dependency.
- Fixed the build with CONFIG_CPUFREQ_DT_RUST=y. The generic DT cpufreq
driver only has the (unbound) per-CPU device, so it cannot hand a
&Device<Bound> to Policy::set_clk(). Added a pub(crate) Clk::get_unbound()
for the few in-tree abstractions that operate on a device outside a bind
scope; set_clk() now takes &Device and uses it. Clk::get()/get_optional()
and the devm_* helpers still require &Device<Bound>.
- Added impl From<Error<State>> for kernel::error::Error, so the fallible
state transitions can be used with `?` (and chained) instead of
.map_err(|e| e.error).
- Added Clk::<Prepared>::with_enabled(), which runs a closure with the clock
temporarily enabled, scoping the Enabled state without giving up the
prepared clock.
- Documented how to change a clock's state at runtime via an enum, for
drivers that enable/disable across resume/suspend.
- Link to v3: https://lore.kernel.org/r/20260107-clk-type-state-v3-0-77d3e3ee59c2@collabora.com
Changes in v3:
- Rebased on top of 6.19-rc4
- Dropped patch 1 (from Alice), added her series as a dependency instead
- Fixed Tyr, PWM_TH1520 drivers
- Changed clk.rs imports to kernel-vertical style
- Added support get_optional shortcut for Prepared and Enabled (i.e.:
Clk::<Enabled>::get_optional())
- Fixed misplaced #[inline] tag
Thanks, Danilo {
- Moved the devres changes into its own patch
- Require &Device<Bound> for all functions where a &Device is used
- Account for con_in in SAFETY comments where applicable
- Added backticks
}
- Link to v2: https://lore.kernel.org/r/20250910-clk-type-state-v2-0-1b97c11bb631@collabora.com
Changes in v2:
- Added Alice's patch as patch 1, since it is a dependency.
- Added devm helpers (like we did for Regulator<T>)
- Fixed missing clk_put() call in Drop (Danilo)
- Fixed missing parenthesis and wrong docs (Viresh)
- Removed extra "dev" parameter from "shutdown" example (Danilo)
- Removed useless type annotation from example (Danilo)
- Link to v1: https://lore.kernel.org/rust-for-linux/20250729-clk-type-state-v1-1-896b53816f7b@collabora.com/#r
To: "Rafael J. Wysocki" <rafael@kernel.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
To: Danilo Krummrich <dakr@kernel.org>
To: Alice Ryhl <aliceryhl@google.com>
To: Daniel Almeida <daniel.almeida@collabora.com>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Drew Fustini <fustini@kernel.org>
To: Guo Ren <guoren@kernel.org>
To: Fu Wei <wefu@redhat.com>
To: Michal Wilczynski <m.wilczynski@samsung.com>
To: Uwe Kleine-König <ukleinek@kernel.org>
To: Michael Turquette <mturquette@baylibre.com>
To: Stephen Boyd <sboyd@kernel.org>
To: Brian Masney <bmasney@redhat.com>
To: Miguel Ojeda <ojeda@kernel.org>
To: Boqun Feng <boqun@kernel.org>
To: Gary Guo <gary@garyguo.net>
To: Björn Roy Baron <bjorn3_gh@protonmail.com>
To: Benno Lossin <lossin@kernel.org>
To: Andreas Hindborg <a.hindborg@kernel.org>
To: Trevor Gross <tmgross@umich.edu>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-riscv@lists.infradead.org
Cc: linux-pwm@vger.kernel.org
Cc: linux-clk@vger.kernel.org
Cc: rust-for-linux@vger.kernel.org
Cc: Boris Brezillon <boris.brezillon@collabora.com>
---
Daniel Almeida (3):
rust: clk: use the type-state pattern
rust: clk: add devres-managed clks
rust: clk: use 'kernel vertical style' for imports
drivers/cpufreq/rcpufreq_dt.rs | 2 +-
drivers/gpu/drm/tyr/driver.rs | 31 +--
drivers/pwm/pwm_th1520.rs | 17 +-
rust/kernel/clk.rs | 595 +++++++++++++++++++++++++++++++----------
rust/kernel/cpufreq.rs | 8 +-
5 files changed, 477 insertions(+), 176 deletions(-)
---
base-commit: 8d03cc42a42b1a3f1535757f65bfb74a9753953f
change-id: 20250909-clk-type-state-c01aa7dd551d
Best regards,
--
Daniel Almeida <daniel.almeida@collabora.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 0/3] Clk improvements
@ 2026-06-18 3:46 ` Daniel Almeida
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Almeida @ 2026-06-18 3:46 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Michal Wilczynski,
Brian Masney, Boqun Feng, Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
This series contains a few improvements that simplifies clock handling for
drivers.
Patch 1 implements the same typestate pattern that has been used
successfully for Regulators. This is needed because otherwise drivers
will be responsible for unpreparing and disabling clocks themselves and
ultimately handling the reference counts on their own. This is
undesirable. The patch automatically encodes this information using the
type system so that no misuse can occur.
Patch 2 makes things more convenient by offering devres-managed APIs. This
lets drivers set clock parameters once and forget about lifetime
management.
Patch 3 converts clk.rs to the newer kernel-vertical style in order to make
future changes easier.
Alice Ryhl's "rust: clk: implement Send and Sync" series, which this used to
depend on, has since been merged, so this series now applies directly on
clk-next.
---
Changes in v4:
- Rebased onto clk-next. Alice Ryhl's "rust: clk: implement Send and Sync"
series is now merged upstream, so it is no longer carried as a dependency.
- Fixed the build with CONFIG_CPUFREQ_DT_RUST=y. The generic DT cpufreq
driver only has the (unbound) per-CPU device, so it cannot hand a
&Device<Bound> to Policy::set_clk(). Added a pub(crate) Clk::get_unbound()
for the few in-tree abstractions that operate on a device outside a bind
scope; set_clk() now takes &Device and uses it. Clk::get()/get_optional()
and the devm_* helpers still require &Device<Bound>.
- Added impl From<Error<State>> for kernel::error::Error, so the fallible
state transitions can be used with `?` (and chained) instead of
.map_err(|e| e.error).
- Added Clk::<Prepared>::with_enabled(), which runs a closure with the clock
temporarily enabled, scoping the Enabled state without giving up the
prepared clock.
- Documented how to change a clock's state at runtime via an enum, for
drivers that enable/disable across resume/suspend.
- Link to v3: https://lore.kernel.org/r/20260107-clk-type-state-v3-0-77d3e3ee59c2@collabora.com
Changes in v3:
- Rebased on top of 6.19-rc4
- Dropped patch 1 (from Alice), added her series as a dependency instead
- Fixed Tyr, PWM_TH1520 drivers
- Changed clk.rs imports to kernel-vertical style
- Added support get_optional shortcut for Prepared and Enabled (i.e.:
Clk::<Enabled>::get_optional())
- Fixed misplaced #[inline] tag
Thanks, Danilo {
- Moved the devres changes into its own patch
- Require &Device<Bound> for all functions where a &Device is used
- Account for con_in in SAFETY comments where applicable
- Added backticks
}
- Link to v2: https://lore.kernel.org/r/20250910-clk-type-state-v2-0-1b97c11bb631@collabora.com
Changes in v2:
- Added Alice's patch as patch 1, since it is a dependency.
- Added devm helpers (like we did for Regulator<T>)
- Fixed missing clk_put() call in Drop (Danilo)
- Fixed missing parenthesis and wrong docs (Viresh)
- Removed extra "dev" parameter from "shutdown" example (Danilo)
- Removed useless type annotation from example (Danilo)
- Link to v1: https://lore.kernel.org/rust-for-linux/20250729-clk-type-state-v1-1-896b53816f7b@collabora.com/#r
To: "Rafael J. Wysocki" <rafael@kernel.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
To: Danilo Krummrich <dakr@kernel.org>
To: Alice Ryhl <aliceryhl@google.com>
To: Daniel Almeida <daniel.almeida@collabora.com>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Drew Fustini <fustini@kernel.org>
To: Guo Ren <guoren@kernel.org>
To: Fu Wei <wefu@redhat.com>
To: Michal Wilczynski <m.wilczynski@samsung.com>
To: Uwe Kleine-König <ukleinek@kernel.org>
To: Michael Turquette <mturquette@baylibre.com>
To: Stephen Boyd <sboyd@kernel.org>
To: Brian Masney <bmasney@redhat.com>
To: Miguel Ojeda <ojeda@kernel.org>
To: Boqun Feng <boqun@kernel.org>
To: Gary Guo <gary@garyguo.net>
To: Björn Roy Baron <bjorn3_gh@protonmail.com>
To: Benno Lossin <lossin@kernel.org>
To: Andreas Hindborg <a.hindborg@kernel.org>
To: Trevor Gross <tmgross@umich.edu>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-riscv@lists.infradead.org
Cc: linux-pwm@vger.kernel.org
Cc: linux-clk@vger.kernel.org
Cc: rust-for-linux@vger.kernel.org
Cc: Boris Brezillon <boris.brezillon@collabora.com>
---
Daniel Almeida (3):
rust: clk: use the type-state pattern
rust: clk: add devres-managed clks
rust: clk: use 'kernel vertical style' for imports
drivers/cpufreq/rcpufreq_dt.rs | 2 +-
drivers/gpu/drm/tyr/driver.rs | 31 +--
drivers/pwm/pwm_th1520.rs | 17 +-
rust/kernel/clk.rs | 595 +++++++++++++++++++++++++++++++----------
rust/kernel/cpufreq.rs | 8 +-
5 files changed, 477 insertions(+), 176 deletions(-)
---
base-commit: 8d03cc42a42b1a3f1535757f65bfb74a9753953f
change-id: 20250909-clk-type-state-c01aa7dd551d
Best regards,
--
Daniel Almeida <daniel.almeida@collabora.com>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 1/3] rust: clk: use the type-state pattern
2026-06-18 3:46 ` Daniel Almeida
@ 2026-06-18 3:46 ` Daniel Almeida
-1 siblings, 0 replies; 18+ messages in thread
From: Daniel Almeida @ 2026-06-18 3:46 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Michal Wilczynski,
Brian Masney, Boqun Feng, Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
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 | 512 ++++++++++++++++++++++++++++++-----------
rust/kernel/cpufreq.rs | 8 +-
5 files changed, 396 insertions(+), 174 deletions(-)
diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index f17bf64c22e2..9d2ec7df4bac 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -40,7 +40,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 279710b36a10..a2230aebfea2 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -3,7 +3,7 @@
use kernel::{
clk::{
Clk,
- OptionalClk, //
+ Enabled, //
},
device::{
Bound,
@@ -49,7 +49,7 @@ pub(crate) struct TyrPlatformDriverData {
_device: ARef<TyrDrmDevice>,
}
-#[pin_data(PinnedDrop)]
+#[pin_data]
pub(crate) struct TyrDrmDeviceData {
pub(crate) pdev: ARef<platform::Device>,
@@ -97,13 +97,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"core"))?;
- let stacks_clk = OptionalClk::get(pdev.as_ref(), Some(c"stacks"))?;
- let coregroup_clk = OptionalClk::get(pdev.as_ref(), Some(c"coregroup"))?;
-
- core_clk.prepare_enable()?;
- stacks_clk.prepare_enable()?;
- coregroup_clk.prepare_enable()?;
+ let core_clk = Clk::<Enabled>::get(pdev.as_ref(), Some(c"core"))?;
+ let stacks_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c"stacks"))?;
+ let coregroup_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c"coregroup"))?;
let mali_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c"mali")?;
let sram_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c"sram")?;
@@ -150,17 +146,6 @@ impl PinnedDrop for TyrPlatformDriverData {
fn drop(self: Pin<&mut Self>) {}
}
-#[pinned_drop]
-impl PinnedDrop for TyrDrmDeviceData {
- 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 {
@@ -186,9 +171,9 @@ impl drm::Driver for TyrDrmDriver {
#[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 ddd44a5ce497..343a1178c5e7 100644
--- a/drivers/pwm/pwm_th1520.rs
+++ b/drivers/pwm/pwm_th1520.rs
@@ -22,7 +22,7 @@
use core::ops::Deref;
use kernel::{
- clk::Clk,
+ clk::{Clk, Enabled},
device::{Bound, Core, Device},
devres,
io::{
@@ -89,11 +89,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 {
@@ -298,13 +298,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!(
@@ -325,9 +318,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 7abbd0767d8c..a62e4c7e252e 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -80,17 +80,103 @@ 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>,
+ }
+
+ impl<State: ClkState> From<Error<State>> for kernel::error::Error {
+ /// Discards the [`Clk`] and keeps only the error code.
+ ///
+ /// This makes the fallible state transitions usable with the `?`
+ /// operator when the caller does not need to retry the operation on the
+ /// original [`Clk`], e.g.:
+ ///
+ /// ```
+ /// use kernel::clk::{Clk, Enabled, Unprepared};
+ /// use kernel::device::{Bound, Device};
+ /// use kernel::error::Result;
+ ///
+ /// fn get_enabled(dev: &Device<Bound>) -> Result<Clk<Enabled>> {
+ /// let clk = Clk::<Unprepared>::get(dev, Some(c"apb_clk"))?
+ /// .prepare()?
+ /// .enable()?;
+ /// Ok(clk)
+ /// }
+ /// ```
+ #[inline]
+ fn from(err: Error<State>) -> Self {
+ err.error
+ }
+ }
/// 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,19 +185,37 @@ 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::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"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"apb_clk"))?;
+ ///
+ /// // Any other state is also possible, e.g.:
+ /// let clk: Clk<Prepared> = Clk::<Prepared>::get(dev, Some(c"apb_clk"))?;
///
- /// clk.prepare_enable()?;
+ /// // Later:
+ /// //
+ /// // `?` works directly thanks to `From<Error<State>>`; the failed
+ /// // `Clk` is dropped on error. Match on the returned `Error<State>`
+ /// // instead (its `clk` field is the original `Clk`) if you want to
+ /// // retry the operation.
+ /// let clk: Clk<Enabled> = clk.enable()?;
///
/// let expected_rate = Hertz::from_ghz(1);
///
@@ -119,111 +223,300 @@ 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()?;
+ ///
+ /// // This is of type `Clk<Unprepared>`.
+ /// let clk = clk.unprepare();
+ ///
/// Ok(())
/// }
/// ```
///
+ /// Drivers that need to change a clock's state at runtime (for example to
+ /// enable it on resume and disable it on suspend) can keep it in an enum
+ /// and move between the variants:
+ ///
+ /// ```
+ /// use kernel::clk::{Clk, Enabled, Prepared};
+ /// use kernel::error::Result;
+ ///
+ /// enum DeviceClk {
+ /// Suspended(Clk<Prepared>),
+ /// Resumed(Clk<Enabled>),
+ /// }
+ ///
+ /// impl DeviceClk {
+ /// fn resume(self) -> Result<Self> {
+ /// Ok(match self {
+ /// DeviceClk::Suspended(clk) => DeviceClk::Resumed(clk.enable()?),
+ /// resumed => resumed,
+ /// })
+ /// }
+ ///
+ /// fn suspend(self) -> Result<Self> {
+ /// Ok(match self {
+ /// DeviceClk::Resumed(clk) => DeviceClk::Suspended(clk.disable()?),
+ /// suspended => suspended,
+ /// })
+ /// }
+ /// }
+ /// ```
+ ///
/// [`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>> {
+ Self::get_unbound(dev, name)
+ }
+
+ /// Gets [`Clk`] corresponding to a [`Device`] and a connection id,
+ /// without requiring the device to be bound.
+ ///
+ /// This is sound because [`clk_get`] and [`clk_put`] do not depend on the
+ /// device being bound to a driver. It is `pub(crate)` because a driver
+ /// should obtain its clocks through a bound device (see [`Clk::get`]); it
+ /// is meant for the few in-tree abstractions that operate on a device
+ /// outside a bind scope, such as the generic DT cpufreq driver.
+ ///
+ /// [`clk_get`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_get
+ #[inline]
+ pub(crate) fn get_unbound(dev: &Device, 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.
+ impl Clk<Prepared> {
+ /// Obtains a [`Clk`] from a bound [`Device`] and a connection id and
+ /// prepares it.
///
- /// Equivalent to the kernel's [`clk_disable`] API.
+ /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
+ #[inline]
+ pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Prepared>> {
+ Clk::<Unprepared>::get(dev, name)?
+ .prepare()
+ .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<Prepared>> {
+ Clk::<Unprepared>::get_optional(dev, name)?
+ .prepare()
+ .map_err(|error| error.error)
+ }
+
+ /// Attempts to convert the [`Clk`] to an [`Unprepared`] state.
///
- /// [`clk_disable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_disable
+ /// Equivalent to the kernel's [`clk_unprepare`] API.
#[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 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(clk.as_raw()) }
+
+ Clk {
+ inner: clk.inner,
+ _phantom: PhantomData,
+ }
}
- /// Prepare the clock.
+ /// Attempts to convert the [`Clk`] to an [`Enabled`] state.
///
- /// Equivalent to the kernel's [`clk_prepare`] API.
+ /// Equivalent to the kernel's [`clk_enable`] API.
///
- /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
+ /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
#[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 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),
+ })
}
- /// Unprepare the clock.
+ /// Runs `cb` with the clock temporarily enabled.
///
- /// Equivalent to the kernel's [`clk_unprepare`] API.
+ /// The clock is enabled before `cb` runs and disabled again afterwards,
+ /// so the [`Enabled`] state is scoped to the closure and the [`Clk`]
+ /// remains [`Prepared`]. This is convenient for drivers that only need
+ /// the clock running for a short, well-defined section (e.g. while
+ /// touching registers) without giving up ownership of the prepared
+ /// clock or threading it through an intermediate state, e.g.:
+ ///
+ /// ```
+ /// use kernel::clk::{Clk, Enabled, Hertz, Prepared};
+ /// use kernel::error::Result;
///
- /// [`clk_unprepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_unprepare
+ /// fn read_rate(clk: &Clk<Prepared>) -> Result<Hertz> {
+ /// clk.with_enabled(|clk: &Clk<Enabled>| clk.rate())
+ /// }
+ /// ```
+ ///
+ /// Equivalent to a balanced [`clk_enable`]/[`clk_disable`] pair around
+ /// `cb`.
+ ///
+ /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
+ /// [`clk_disable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_disable
#[inline]
- pub fn unprepare(&self) {
- // SAFETY: By the type invariants, self.as_raw() is a valid argument for
- // [`clk_unprepare`].
- unsafe { bindings::clk_unprepare(self.as_raw()) };
+ pub fn with_enabled<R>(&self, cb: impl FnOnce(&Clk<Enabled>) -> R) -> Result<R> {
+ // SAFETY: By the type invariants, `self.as_raw()` is a valid argument for
+ // [`clk_enable`].
+ to_result(unsafe { bindings::clk_enable(self.as_raw()) })?;
+
+ // Borrow the same clock as `Clk<Enabled>` for the duration of `cb`.
+ // It must not be dropped, as that would run `clk_disable`/`clk_put`
+ // against counts owned by `self`; the matching `clk_disable` below
+ // balances the `clk_enable` above instead.
+ //
+ // INVARIANT: The clock is enabled for the lifetime of `enabled`.
+ let enabled = ManuallyDrop::new(Clk::<Enabled> {
+ inner: self.inner,
+ _phantom: PhantomData,
+ });
+
+ let ret = cb(&enabled);
+
+ // SAFETY: The `clk_enable` above succeeded, so this balances it.
+ // `cb` only had a shared reference, so the enable count is unchanged.
+ unsafe { bindings::clk_disable(self.as_raw()) };
+
+ Ok(ret)
}
+ }
- /// Prepare and enable 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::prepare`] followed by [`Clk::enable`].
+ /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
+ /// followed by [`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 get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
+ Clk::<Prepared>::get(dev, name)?
+ .enable()
+ .map_err(|error| error.error)
}
- /// Disable and unprepare the clock.
- ///
- /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
+ /// 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 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_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.
@@ -251,82 +544,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::clk::{OptionalClk, Hertz};
- /// use kernel::device::Device;
- /// use kernel::error::Result;
- ///
- /// fn configure_clk(dev: &Device) -> Result {
- /// let clk = OptionalClk::get(dev, Some(c"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 d8d26870bea2..e837bb1010e0 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -553,8 +553,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,
+ name: Option<&CStr>,
+ ) -> Result<Clk<crate::clk::Unprepared>> {
+ let clk = Clk::<crate::clk::Unprepared>::get_unbound(dev, name)?;
self.as_mut_ref().clk = clk.as_raw();
Ok(clk)
}
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 1/3] rust: clk: use the type-state pattern
@ 2026-06-18 3:46 ` Daniel Almeida
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Almeida @ 2026-06-18 3:46 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Michal Wilczynski,
Brian Masney, Boqun Feng, Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
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 | 512 ++++++++++++++++++++++++++++++-----------
rust/kernel/cpufreq.rs | 8 +-
5 files changed, 396 insertions(+), 174 deletions(-)
diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index f17bf64c22e2..9d2ec7df4bac 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -40,7 +40,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 279710b36a10..a2230aebfea2 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -3,7 +3,7 @@
use kernel::{
clk::{
Clk,
- OptionalClk, //
+ Enabled, //
},
device::{
Bound,
@@ -49,7 +49,7 @@ pub(crate) struct TyrPlatformDriverData {
_device: ARef<TyrDrmDevice>,
}
-#[pin_data(PinnedDrop)]
+#[pin_data]
pub(crate) struct TyrDrmDeviceData {
pub(crate) pdev: ARef<platform::Device>,
@@ -97,13 +97,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"core"))?;
- let stacks_clk = OptionalClk::get(pdev.as_ref(), Some(c"stacks"))?;
- let coregroup_clk = OptionalClk::get(pdev.as_ref(), Some(c"coregroup"))?;
-
- core_clk.prepare_enable()?;
- stacks_clk.prepare_enable()?;
- coregroup_clk.prepare_enable()?;
+ let core_clk = Clk::<Enabled>::get(pdev.as_ref(), Some(c"core"))?;
+ let stacks_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c"stacks"))?;
+ let coregroup_clk = Clk::<Enabled>::get_optional(pdev.as_ref(), Some(c"coregroup"))?;
let mali_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c"mali")?;
let sram_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c"sram")?;
@@ -150,17 +146,6 @@ impl PinnedDrop for TyrPlatformDriverData {
fn drop(self: Pin<&mut Self>) {}
}
-#[pinned_drop]
-impl PinnedDrop for TyrDrmDeviceData {
- 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 {
@@ -186,9 +171,9 @@ impl drm::Driver for TyrDrmDriver {
#[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 ddd44a5ce497..343a1178c5e7 100644
--- a/drivers/pwm/pwm_th1520.rs
+++ b/drivers/pwm/pwm_th1520.rs
@@ -22,7 +22,7 @@
use core::ops::Deref;
use kernel::{
- clk::Clk,
+ clk::{Clk, Enabled},
device::{Bound, Core, Device},
devres,
io::{
@@ -89,11 +89,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 {
@@ -298,13 +298,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!(
@@ -325,9 +318,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 7abbd0767d8c..a62e4c7e252e 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -80,17 +80,103 @@ 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>,
+ }
+
+ impl<State: ClkState> From<Error<State>> for kernel::error::Error {
+ /// Discards the [`Clk`] and keeps only the error code.
+ ///
+ /// This makes the fallible state transitions usable with the `?`
+ /// operator when the caller does not need to retry the operation on the
+ /// original [`Clk`], e.g.:
+ ///
+ /// ```
+ /// use kernel::clk::{Clk, Enabled, Unprepared};
+ /// use kernel::device::{Bound, Device};
+ /// use kernel::error::Result;
+ ///
+ /// fn get_enabled(dev: &Device<Bound>) -> Result<Clk<Enabled>> {
+ /// let clk = Clk::<Unprepared>::get(dev, Some(c"apb_clk"))?
+ /// .prepare()?
+ /// .enable()?;
+ /// Ok(clk)
+ /// }
+ /// ```
+ #[inline]
+ fn from(err: Error<State>) -> Self {
+ err.error
+ }
+ }
/// 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,19 +185,37 @@ 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::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"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"apb_clk"))?;
+ ///
+ /// // Any other state is also possible, e.g.:
+ /// let clk: Clk<Prepared> = Clk::<Prepared>::get(dev, Some(c"apb_clk"))?;
///
- /// clk.prepare_enable()?;
+ /// // Later:
+ /// //
+ /// // `?` works directly thanks to `From<Error<State>>`; the failed
+ /// // `Clk` is dropped on error. Match on the returned `Error<State>`
+ /// // instead (its `clk` field is the original `Clk`) if you want to
+ /// // retry the operation.
+ /// let clk: Clk<Enabled> = clk.enable()?;
///
/// let expected_rate = Hertz::from_ghz(1);
///
@@ -119,111 +223,300 @@ 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()?;
+ ///
+ /// // This is of type `Clk<Unprepared>`.
+ /// let clk = clk.unprepare();
+ ///
/// Ok(())
/// }
/// ```
///
+ /// Drivers that need to change a clock's state at runtime (for example to
+ /// enable it on resume and disable it on suspend) can keep it in an enum
+ /// and move between the variants:
+ ///
+ /// ```
+ /// use kernel::clk::{Clk, Enabled, Prepared};
+ /// use kernel::error::Result;
+ ///
+ /// enum DeviceClk {
+ /// Suspended(Clk<Prepared>),
+ /// Resumed(Clk<Enabled>),
+ /// }
+ ///
+ /// impl DeviceClk {
+ /// fn resume(self) -> Result<Self> {
+ /// Ok(match self {
+ /// DeviceClk::Suspended(clk) => DeviceClk::Resumed(clk.enable()?),
+ /// resumed => resumed,
+ /// })
+ /// }
+ ///
+ /// fn suspend(self) -> Result<Self> {
+ /// Ok(match self {
+ /// DeviceClk::Resumed(clk) => DeviceClk::Suspended(clk.disable()?),
+ /// suspended => suspended,
+ /// })
+ /// }
+ /// }
+ /// ```
+ ///
/// [`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>> {
+ Self::get_unbound(dev, name)
+ }
+
+ /// Gets [`Clk`] corresponding to a [`Device`] and a connection id,
+ /// without requiring the device to be bound.
+ ///
+ /// This is sound because [`clk_get`] and [`clk_put`] do not depend on the
+ /// device being bound to a driver. It is `pub(crate)` because a driver
+ /// should obtain its clocks through a bound device (see [`Clk::get`]); it
+ /// is meant for the few in-tree abstractions that operate on a device
+ /// outside a bind scope, such as the generic DT cpufreq driver.
+ ///
+ /// [`clk_get`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_get
+ #[inline]
+ pub(crate) fn get_unbound(dev: &Device, 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.
+ impl Clk<Prepared> {
+ /// Obtains a [`Clk`] from a bound [`Device`] and a connection id and
+ /// prepares it.
///
- /// Equivalent to the kernel's [`clk_disable`] API.
+ /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
+ #[inline]
+ pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Prepared>> {
+ Clk::<Unprepared>::get(dev, name)?
+ .prepare()
+ .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<Prepared>> {
+ Clk::<Unprepared>::get_optional(dev, name)?
+ .prepare()
+ .map_err(|error| error.error)
+ }
+
+ /// Attempts to convert the [`Clk`] to an [`Unprepared`] state.
///
- /// [`clk_disable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_disable
+ /// Equivalent to the kernel's [`clk_unprepare`] API.
#[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 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(clk.as_raw()) }
+
+ Clk {
+ inner: clk.inner,
+ _phantom: PhantomData,
+ }
}
- /// Prepare the clock.
+ /// Attempts to convert the [`Clk`] to an [`Enabled`] state.
///
- /// Equivalent to the kernel's [`clk_prepare`] API.
+ /// Equivalent to the kernel's [`clk_enable`] API.
///
- /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
+ /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
#[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 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),
+ })
}
- /// Unprepare the clock.
+ /// Runs `cb` with the clock temporarily enabled.
///
- /// Equivalent to the kernel's [`clk_unprepare`] API.
+ /// The clock is enabled before `cb` runs and disabled again afterwards,
+ /// so the [`Enabled`] state is scoped to the closure and the [`Clk`]
+ /// remains [`Prepared`]. This is convenient for drivers that only need
+ /// the clock running for a short, well-defined section (e.g. while
+ /// touching registers) without giving up ownership of the prepared
+ /// clock or threading it through an intermediate state, e.g.:
+ ///
+ /// ```
+ /// use kernel::clk::{Clk, Enabled, Hertz, Prepared};
+ /// use kernel::error::Result;
///
- /// [`clk_unprepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_unprepare
+ /// fn read_rate(clk: &Clk<Prepared>) -> Result<Hertz> {
+ /// clk.with_enabled(|clk: &Clk<Enabled>| clk.rate())
+ /// }
+ /// ```
+ ///
+ /// Equivalent to a balanced [`clk_enable`]/[`clk_disable`] pair around
+ /// `cb`.
+ ///
+ /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
+ /// [`clk_disable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_disable
#[inline]
- pub fn unprepare(&self) {
- // SAFETY: By the type invariants, self.as_raw() is a valid argument for
- // [`clk_unprepare`].
- unsafe { bindings::clk_unprepare(self.as_raw()) };
+ pub fn with_enabled<R>(&self, cb: impl FnOnce(&Clk<Enabled>) -> R) -> Result<R> {
+ // SAFETY: By the type invariants, `self.as_raw()` is a valid argument for
+ // [`clk_enable`].
+ to_result(unsafe { bindings::clk_enable(self.as_raw()) })?;
+
+ // Borrow the same clock as `Clk<Enabled>` for the duration of `cb`.
+ // It must not be dropped, as that would run `clk_disable`/`clk_put`
+ // against counts owned by `self`; the matching `clk_disable` below
+ // balances the `clk_enable` above instead.
+ //
+ // INVARIANT: The clock is enabled for the lifetime of `enabled`.
+ let enabled = ManuallyDrop::new(Clk::<Enabled> {
+ inner: self.inner,
+ _phantom: PhantomData,
+ });
+
+ let ret = cb(&enabled);
+
+ // SAFETY: The `clk_enable` above succeeded, so this balances it.
+ // `cb` only had a shared reference, so the enable count is unchanged.
+ unsafe { bindings::clk_disable(self.as_raw()) };
+
+ Ok(ret)
}
+ }
- /// Prepare and enable 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::prepare`] followed by [`Clk::enable`].
+ /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
+ /// followed by [`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 get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
+ Clk::<Prepared>::get(dev, name)?
+ .enable()
+ .map_err(|error| error.error)
}
- /// Disable and unprepare the clock.
- ///
- /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
+ /// 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 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_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.
@@ -251,82 +544,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::clk::{OptionalClk, Hertz};
- /// use kernel::device::Device;
- /// use kernel::error::Result;
- ///
- /// fn configure_clk(dev: &Device) -> Result {
- /// let clk = OptionalClk::get(dev, Some(c"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 d8d26870bea2..e837bb1010e0 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -553,8 +553,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,
+ name: Option<&CStr>,
+ ) -> Result<Clk<crate::clk::Unprepared>> {
+ let clk = Clk::<crate::clk::Unprepared>::get_unbound(dev, name)?;
self.as_mut_ref().clk = clk.as_raw();
Ok(clk)
}
--
2.54.0
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 2/3] rust: clk: add devres-managed clks
2026-06-18 3:46 ` Daniel Almeida
@ 2026-06-18 3:46 ` Daniel Almeida
-1 siblings, 0 replies; 18+ messages in thread
From: Daniel Almeida @ 2026-06-18 3:46 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Michal Wilczynski,
Brian Masney, Boqun Feng, Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
The clk API allows fine-grained control, but some drivers might be
more interested in a "set and forget" API.
Expand the current API to support this. The clock will automatically be
disabled, unprepared and freed when the device is unbound from the bus
without further intervention by the driver.
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
rust/kernel/clk.rs | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index a62e4c7e252e..692ee88ca772 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -95,6 +95,49 @@ impl Sealed for super::Prepared {}
impl Sealed for super::Enabled {}
}
+ /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
+ ///
+ /// [`devres`]: crate::devres::Devres
+ pub fn devm_enable(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
+ let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+ // SAFETY: It is safe to call [`devm_clk_get_enabled`] with a valid
+ // device pointer.
+ from_err_ptr(unsafe { bindings::devm_clk_get_enabled(dev.as_raw(), name) })?;
+ Ok(())
+ }
+
+ /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
+ ///
+ /// This does not print any error messages if the clock is not found.
+ ///
+ /// [`devres`]: crate::devres::Devres
+ pub fn devm_enable_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
+ let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+ // SAFETY: It is safe to call [`devm_clk_get_optional_enabled`] with a
+ // valid device pointer.
+ from_err_ptr(unsafe { bindings::devm_clk_get_optional_enabled(dev.as_raw(), name) })?;
+ Ok(())
+ }
+
+ /// Same as [`devm_enable_optional`], but also sets the rate.
+ pub fn devm_enable_optional_with_rate(
+ dev: &Device<Bound>,
+ name: Option<&CStr>,
+ rate: Hertz,
+ ) -> Result {
+ let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+ // SAFETY: It is safe to call
+ // [`devm_clk_get_optional_enabled_with_rate`] with a valid device
+ // pointer.
+ from_err_ptr(unsafe {
+ bindings::devm_clk_get_optional_enabled_with_rate(dev.as_raw(), name, rate.as_hz())
+ })?;
+ Ok(())
+ }
+
/// 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.
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 2/3] rust: clk: add devres-managed clks
@ 2026-06-18 3:46 ` Daniel Almeida
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Almeida @ 2026-06-18 3:46 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Michal Wilczynski,
Brian Masney, Boqun Feng, Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
The clk API allows fine-grained control, but some drivers might be
more interested in a "set and forget" API.
Expand the current API to support this. The clock will automatically be
disabled, unprepared and freed when the device is unbound from the bus
without further intervention by the driver.
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
rust/kernel/clk.rs | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index a62e4c7e252e..692ee88ca772 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -95,6 +95,49 @@ impl Sealed for super::Prepared {}
impl Sealed for super::Enabled {}
}
+ /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
+ ///
+ /// [`devres`]: crate::devres::Devres
+ pub fn devm_enable(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
+ let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+ // SAFETY: It is safe to call [`devm_clk_get_enabled`] with a valid
+ // device pointer.
+ from_err_ptr(unsafe { bindings::devm_clk_get_enabled(dev.as_raw(), name) })?;
+ Ok(())
+ }
+
+ /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
+ ///
+ /// This does not print any error messages if the clock is not found.
+ ///
+ /// [`devres`]: crate::devres::Devres
+ pub fn devm_enable_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
+ let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+ // SAFETY: It is safe to call [`devm_clk_get_optional_enabled`] with a
+ // valid device pointer.
+ from_err_ptr(unsafe { bindings::devm_clk_get_optional_enabled(dev.as_raw(), name) })?;
+ Ok(())
+ }
+
+ /// Same as [`devm_enable_optional`], but also sets the rate.
+ pub fn devm_enable_optional_with_rate(
+ dev: &Device<Bound>,
+ name: Option<&CStr>,
+ rate: Hertz,
+ ) -> Result {
+ let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
+
+ // SAFETY: It is safe to call
+ // [`devm_clk_get_optional_enabled_with_rate`] with a valid device
+ // pointer.
+ from_err_ptr(unsafe {
+ bindings::devm_clk_get_optional_enabled_with_rate(dev.as_raw(), name, rate.as_hz())
+ })?;
+ Ok(())
+ }
+
/// 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.
--
2.54.0
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 3/3] rust: clk: use 'kernel vertical style' for imports
2026-06-18 3:46 ` Daniel Almeida
@ 2026-06-18 3:46 ` Daniel Almeida
-1 siblings, 0 replies; 18+ messages in thread
From: Daniel Almeida @ 2026-06-18 3:46 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Michal Wilczynski,
Brian Masney, Boqun Feng, Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
Convert all imports to use the new import style. This will make it easier
to land new changes in the future.
No change of functionality implied.
Link: https://docs.kernel.org/rust/coding-guidelines.html#imports
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
rust/kernel/clk.rs | 56 ++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 46 insertions(+), 10 deletions(-)
diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index 692ee88ca772..1412a2f0aedf 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -80,12 +80,23 @@ fn from(freq: Hertz) -> Self {
mod common_clk {
use super::Hertz;
use crate::{
- device::{Bound, Device},
- error::{from_err_ptr, to_result, Result},
- prelude::*,
+ device::{
+ Bound,
+ Device, //
+ },
+ error::{
+ from_err_ptr,
+ to_result,
+ Result, //
+ },
+ prelude::*, //
};
- use core::{marker::PhantomData, mem::ManuallyDrop, ptr};
+ use core::{
+ marker::PhantomData,
+ mem::ManuallyDrop,
+ ptr, //
+ };
mod private {
pub trait Sealed {}
@@ -189,8 +200,15 @@ impl<State: ClkState> From<Error<State>> for kernel::error::Error {
/// original [`Clk`], e.g.:
///
/// ```
- /// use kernel::clk::{Clk, Enabled, Unprepared};
- /// use kernel::device::{Bound, Device};
+ /// use kernel::clk::{
+ /// Clk,
+ /// Enabled,
+ /// Unprepared, //
+ /// };
+ /// use kernel::device::{
+ /// Bound,
+ /// Device, //
+ /// };
/// use kernel::error::Result;
///
/// fn get_enabled(dev: &Device<Bound>) -> Result<Clk<Enabled>> {
@@ -240,8 +258,17 @@ fn from(err: Error<State>) -> Self {
/// The following example demonstrates how to obtain and configure a clock for a device.
///
/// ```
- /// use kernel::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
- /// use kernel::device::{Bound, Device};
+ /// use kernel::clk::{
+ /// Clk,
+ /// Enabled,
+ /// Hertz,
+ /// Prepared,
+ /// Unprepared, //
+ /// };
+ /// use kernel::device::{
+ /// Bound,
+ /// Device, //
+ /// };
/// use kernel::error::Result;
///
/// fn configure_clk(dev: &Device<Bound>) -> Result {
@@ -287,7 +314,11 @@ fn from(err: Error<State>) -> Self {
/// and move between the variants:
///
/// ```
- /// use kernel::clk::{Clk, Enabled, Prepared};
+ /// use kernel::clk::{
+ /// Clk,
+ /// Enabled,
+ /// Prepared, //
+ /// };
/// use kernel::error::Result;
///
/// enum DeviceClk {
@@ -481,7 +512,12 @@ pub fn enable(self) -> Result<Clk<Enabled>, Error<Prepared>> {
/// clock or threading it through an intermediate state, e.g.:
///
/// ```
- /// use kernel::clk::{Clk, Enabled, Hertz, Prepared};
+ /// use kernel::clk::{
+ /// Clk,
+ /// Enabled,
+ /// Hertz,
+ /// Prepared, //
+ /// };
/// use kernel::error::Result;
///
/// fn read_rate(clk: &Clk<Prepared>) -> Result<Hertz> {
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 3/3] rust: clk: use 'kernel vertical style' for imports
@ 2026-06-18 3:46 ` Daniel Almeida
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Almeida @ 2026-06-18 3:46 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Michal Wilczynski,
Brian Masney, Boqun Feng, Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
Convert all imports to use the new import style. This will make it easier
to land new changes in the future.
No change of functionality implied.
Link: https://docs.kernel.org/rust/coding-guidelines.html#imports
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
rust/kernel/clk.rs | 56 ++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 46 insertions(+), 10 deletions(-)
diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index 692ee88ca772..1412a2f0aedf 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -80,12 +80,23 @@ fn from(freq: Hertz) -> Self {
mod common_clk {
use super::Hertz;
use crate::{
- device::{Bound, Device},
- error::{from_err_ptr, to_result, Result},
- prelude::*,
+ device::{
+ Bound,
+ Device, //
+ },
+ error::{
+ from_err_ptr,
+ to_result,
+ Result, //
+ },
+ prelude::*, //
};
- use core::{marker::PhantomData, mem::ManuallyDrop, ptr};
+ use core::{
+ marker::PhantomData,
+ mem::ManuallyDrop,
+ ptr, //
+ };
mod private {
pub trait Sealed {}
@@ -189,8 +200,15 @@ impl<State: ClkState> From<Error<State>> for kernel::error::Error {
/// original [`Clk`], e.g.:
///
/// ```
- /// use kernel::clk::{Clk, Enabled, Unprepared};
- /// use kernel::device::{Bound, Device};
+ /// use kernel::clk::{
+ /// Clk,
+ /// Enabled,
+ /// Unprepared, //
+ /// };
+ /// use kernel::device::{
+ /// Bound,
+ /// Device, //
+ /// };
/// use kernel::error::Result;
///
/// fn get_enabled(dev: &Device<Bound>) -> Result<Clk<Enabled>> {
@@ -240,8 +258,17 @@ fn from(err: Error<State>) -> Self {
/// The following example demonstrates how to obtain and configure a clock for a device.
///
/// ```
- /// use kernel::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
- /// use kernel::device::{Bound, Device};
+ /// use kernel::clk::{
+ /// Clk,
+ /// Enabled,
+ /// Hertz,
+ /// Prepared,
+ /// Unprepared, //
+ /// };
+ /// use kernel::device::{
+ /// Bound,
+ /// Device, //
+ /// };
/// use kernel::error::Result;
///
/// fn configure_clk(dev: &Device<Bound>) -> Result {
@@ -287,7 +314,11 @@ fn from(err: Error<State>) -> Self {
/// and move between the variants:
///
/// ```
- /// use kernel::clk::{Clk, Enabled, Prepared};
+ /// use kernel::clk::{
+ /// Clk,
+ /// Enabled,
+ /// Prepared, //
+ /// };
/// use kernel::error::Result;
///
/// enum DeviceClk {
@@ -481,7 +512,12 @@ pub fn enable(self) -> Result<Clk<Enabled>, Error<Prepared>> {
/// clock or threading it through an intermediate state, e.g.:
///
/// ```
- /// use kernel::clk::{Clk, Enabled, Hertz, Prepared};
+ /// use kernel::clk::{
+ /// Clk,
+ /// Enabled,
+ /// Hertz,
+ /// Prepared, //
+ /// };
/// use kernel::error::Result;
///
/// fn read_rate(clk: &Clk<Prepared>) -> Result<Hertz> {
--
2.54.0
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] rust: clk: use the type-state pattern
2026-06-18 3:46 ` Daniel Almeida
(?)
@ 2026-06-18 3:56 ` sashiko-bot
-1 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-06-18 3:56 UTC (permalink / raw)
To: Daniel Almeida; +Cc: dri-devel, ojeda
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The clock rate configuration methods (`rate()` and `set_rate()`) have been restricted exclusively to `Clk<Enabled>`, breaking support for clocks with `CLK_SET_RATE_GATE` and preventing pre-enablement configuration.
- [Low] Missing `// INVARIANT:` comments when constructing `Clk` instances during state transitions, and safety comments incorrectly refer to a non-existent `self.0` field.
- [Low] `Clk::<Enabled>::disable()` incorrectly returns a `Result` despite being infallible, while `unprepare()` correctly returns the struct directly.
- [Low] Typo in the `# Invariants` documentation section regarding the `Prepared` and `Enabled` states.
--
commit c6dd3e4cdd99489c0a4f27adce8b4f6ceef42c51
Author: Daniel Almeida <daniel.almeida@collabora.com>
rust: clk: use the type-state pattern
This commit switches the clock abstraction to use the type-state pattern
to enforce valid state transitions and track reference counts automatically.
Link: https://crates.io/crates/sealed [1]
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index 7abbd0767d8cf..a62e4c7e252e3 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs
[ ... ]
> + /// 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()`.
[Severity: Low]
Is the last part of this sentence a typo? It attributes the dual count
requirement to the Prepared state again instead of the Enabled state.
[ ... ]
> + 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,
> + })
[Severity: Low]
Should there be an '// INVARIANT:' comment when constructing this new Clk,
similar to the initialization in get_unbound()?
Also, does the safety comment reference 'self.0' which no longer exists
after the struct was refactored to use named fields? The same stale reference
and missing invariant comment appears in unprepare(), enable(), and disable().
[ ... ]
> + 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,
> + })
> + }
[Severity: Low]
Since clk_disable() is infallible, does this method need to return a Result?
The unprepare() method returns the struct directly, which avoids unnecessary
error handling for the caller.
[ ... ]
> + impl Clk<Enabled> {
[ ... ]
> pub fn set_rate(&self, rate: Hertz) -> Result {
> // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> // [`clk_set_rate`].
> to_result(unsafe { bindings::clk_set_rate(self.as_raw(), rate.as_hz()) })
> }
> }
[Severity: High]
Does restricting set_rate() and rate() to the Enabled state prevent configuring
clocks that have the CLK_SET_RATE_GATE flag?
The C API requires clocks with this flag to be disabled before changing their
rate. Additionally, is it still possible to safely configure a clock's rate
before enabling it, which is often required to avoid running hardware at
incorrect frequencies?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-clk-type-state-v4-0-8be082786080@collabora.com?part=1
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] rust: clk: add devres-managed clks
2026-06-18 3:46 ` Daniel Almeida
(?)
@ 2026-06-18 3:57 ` sashiko-bot
-1 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-06-18 3:57 UTC (permalink / raw)
To: Daniel Almeida; +Cc: dri-devel, ojeda
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] Missing `#[inline]` annotations on small abstraction functions.
Pre-existing issues:
- [High] Error path in `devm_clk_get_optional_enabled_with_rate` triggers unbalanced `clk_disable_unprepare`.
--
commit 6968c407fbf832032a4c2355e55f45330a6b1049
Author: Daniel Almeida <daniel.almeida@collabora.com>
rust: clk: add devres-managed clks
The clk API allows fine-grained control, but some drivers might be
more interested in a "set and forget" API. Expand the current API to
support this by automatically disabling, unpreparing and freeing the
clock when the device is unbound from the bus.
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index a62e4c7e252e3..692ee88ca772a 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs
> @@ -95,6 +95,49 @@ impl Sealed for super::Prepared {}
> impl Sealed for super::Enabled {}
> }
>
> + /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
> + ///
> + /// [`devres`]: crate::devres::Devres
> + pub fn devm_enable(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
[Severity: Low]
Should this small forwarding function be annotated with #[inline]?
According to the Rust subsystem guidelines, functions in abstractions
that are small or simply forward to a binding call require this annotation.
> + let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
> +
> + // SAFETY: It is safe to call [`devm_clk_get_enabled`] with a valid
> + // device pointer.
> + from_err_ptr(unsafe { bindings::devm_clk_get_enabled(dev.as_raw(), name) })?;
> + Ok(())
> + }
> +
> + /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
> + ///
> + /// This does not print any error messages if the clock is not found.
> + ///
> + /// [`devres`]: crate::devres::Devres
> + pub fn devm_enable_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result {
[Severity: Low]
Does this function also need an #[inline] annotation since it just
forwards to a C binding?
> + let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
> +
> + // SAFETY: It is safe to call [`devm_clk_get_optional_enabled`] with a
> + // valid device pointer.
> + from_err_ptr(unsafe { bindings::devm_clk_get_optional_enabled(dev.as_raw(), name) })?;
> + Ok(())
> + }
> +
> + /// Same as [`devm_enable_optional`], but also sets the rate.
> + pub fn devm_enable_optional_with_rate(
> + dev: &Device<Bound>,
> + name: Option<&CStr>,
> + rate: Hertz,
> + ) -> Result {
[Severity: Low]
Should this small wrapper function be annotated with #[inline] to comply
with the Rust subsystem guidelines for abstractions?
> + let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
> +
> + // SAFETY: It is safe to call
> + // [`devm_clk_get_optional_enabled_with_rate`] with a valid device
> + // pointer.
> + from_err_ptr(unsafe {
> + bindings::devm_clk_get_optional_enabled_with_rate(dev.as_raw(), name, rate.as_hz())
[Severity: High]
This is a pre-existing issue, but does the underlying C function
devm_clk_get_optional_enabled_with_rate() have an unbalanced
clk_disable_unprepare() on its error path?
Looking at drivers/clk/clk-devres.c:devm_clk_get_optional_enabled_with_rate(),
it registers a devres entry with clk_disable_unprepare() as the cleanup
action.
If clk_set_rate() or clk_prepare_enable() fails, the error path executes
devm_clk_put(), which releases the devres entry and immediately invokes the
cleanup action:
drivers/clk/clk-devres.c:devm_clk_get_optional_enabled_with_rate() {
...
out_put_clk:
devm_clk_put(dev, clk);
return ERR_PTR(ret);
}
Because the clock was never successfully prepared or enabled, won't this
underflow the clock's enable/prepare counts? For a shared clock, could this
prematurely disable the clock for other devices?
> + })?;
> + Ok(())
> + }
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-clk-type-state-v4-0-8be082786080@collabora.com?part=2
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 3/3] rust: clk: use 'kernel vertical style' for imports
2026-06-18 3:46 ` Daniel Almeida
@ 2026-06-18 7:58 ` Onur Özkan
-1 siblings, 0 replies; 18+ messages in thread
From: Onur Özkan @ 2026-06-18 7:58 UTC (permalink / raw)
To: Daniel Almeida
Cc: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Michal Wilczynski, Brian Masney,
Boqun Feng, linux-pm, linux-kernel, dri-devel, linux-riscv,
linux-pwm, linux-clk, rust-for-linux, Boris Brezillon,
Onur Özkan
On Thu, 18 Jun 2026 00:46:37 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
> Convert all imports to use the new import style. This will make it easier
> to land new changes in the future.
>
> No change of functionality implied.
>
> Link: https://docs.kernel.org/rust/coding-guidelines.html#imports
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Onur Özkan <work@onurozkan.dev>
> ---
> rust/kernel/clk.rs | 56 ++++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index 692ee88ca772..1412a2f0aedf 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs
> @@ -80,12 +80,23 @@ fn from(freq: Hertz) -> Self {
> mod common_clk {
> use super::Hertz;
> use crate::{
> - device::{Bound, Device},
> - error::{from_err_ptr, to_result, Result},
> - prelude::*,
> + device::{
> + Bound,
> + Device, //
> + },
> + error::{
> + from_err_ptr,
> + to_result,
> + Result, //
> + },
> + prelude::*, //
> };
>
> - use core::{marker::PhantomData, mem::ManuallyDrop, ptr};
> + use core::{
> + marker::PhantomData,
> + mem::ManuallyDrop,
> + ptr, //
> + };
>
> mod private {
> pub trait Sealed {}
> @@ -189,8 +200,15 @@ impl<State: ClkState> From<Error<State>> for kernel::error::Error {
> /// original [`Clk`], e.g.:
> ///
> /// ```
> - /// use kernel::clk::{Clk, Enabled, Unprepared};
> - /// use kernel::device::{Bound, Device};
> + /// use kernel::clk::{
> + /// Clk,
> + /// Enabled,
> + /// Unprepared, //
> + /// };
> + /// use kernel::device::{
> + /// Bound,
> + /// Device, //
> + /// };
> /// use kernel::error::Result;
> ///
> /// fn get_enabled(dev: &Device<Bound>) -> Result<Clk<Enabled>> {
> @@ -240,8 +258,17 @@ fn from(err: Error<State>) -> Self {
> /// The following example demonstrates how to obtain and configure a clock for a device.
> ///
> /// ```
> - /// use kernel::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
> - /// use kernel::device::{Bound, Device};
> + /// use kernel::clk::{
> + /// Clk,
> + /// Enabled,
> + /// Hertz,
> + /// Prepared,
> + /// Unprepared, //
> + /// };
> + /// use kernel::device::{
> + /// Bound,
> + /// Device, //
> + /// };
> /// use kernel::error::Result;
> ///
> /// fn configure_clk(dev: &Device<Bound>) -> Result {
> @@ -287,7 +314,11 @@ fn from(err: Error<State>) -> Self {
> /// and move between the variants:
> ///
> /// ```
> - /// use kernel::clk::{Clk, Enabled, Prepared};
> + /// use kernel::clk::{
> + /// Clk,
> + /// Enabled,
> + /// Prepared, //
> + /// };
> /// use kernel::error::Result;
> ///
> /// enum DeviceClk {
> @@ -481,7 +512,12 @@ pub fn enable(self) -> Result<Clk<Enabled>, Error<Prepared>> {
> /// clock or threading it through an intermediate state, e.g.:
> ///
> /// ```
> - /// use kernel::clk::{Clk, Enabled, Hertz, Prepared};
> + /// use kernel::clk::{
> + /// Clk,
> + /// Enabled,
> + /// Hertz,
> + /// Prepared, //
> + /// };
> /// use kernel::error::Result;
> ///
> /// fn read_rate(clk: &Clk<Prepared>) -> Result<Hertz> {
>
> --
> 2.54.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 3/3] rust: clk: use 'kernel vertical style' for imports
@ 2026-06-18 7:58 ` Onur Özkan
0 siblings, 0 replies; 18+ messages in thread
From: Onur Özkan @ 2026-06-18 7:58 UTC (permalink / raw)
To: Daniel Almeida
Cc: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Michal Wilczynski, Brian Masney,
Boqun Feng, linux-pm, linux-kernel, dri-devel, linux-riscv,
linux-pwm, linux-clk, rust-for-linux, Boris Brezillon,
Onur Özkan
On Thu, 18 Jun 2026 00:46:37 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
> Convert all imports to use the new import style. This will make it easier
> to land new changes in the future.
>
> No change of functionality implied.
>
> Link: https://docs.kernel.org/rust/coding-guidelines.html#imports
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Onur Özkan <work@onurozkan.dev>
> ---
> rust/kernel/clk.rs | 56 ++++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index 692ee88ca772..1412a2f0aedf 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs
> @@ -80,12 +80,23 @@ fn from(freq: Hertz) -> Self {
> mod common_clk {
> use super::Hertz;
> use crate::{
> - device::{Bound, Device},
> - error::{from_err_ptr, to_result, Result},
> - prelude::*,
> + device::{
> + Bound,
> + Device, //
> + },
> + error::{
> + from_err_ptr,
> + to_result,
> + Result, //
> + },
> + prelude::*, //
> };
>
> - use core::{marker::PhantomData, mem::ManuallyDrop, ptr};
> + use core::{
> + marker::PhantomData,
> + mem::ManuallyDrop,
> + ptr, //
> + };
>
> mod private {
> pub trait Sealed {}
> @@ -189,8 +200,15 @@ impl<State: ClkState> From<Error<State>> for kernel::error::Error {
> /// original [`Clk`], e.g.:
> ///
> /// ```
> - /// use kernel::clk::{Clk, Enabled, Unprepared};
> - /// use kernel::device::{Bound, Device};
> + /// use kernel::clk::{
> + /// Clk,
> + /// Enabled,
> + /// Unprepared, //
> + /// };
> + /// use kernel::device::{
> + /// Bound,
> + /// Device, //
> + /// };
> /// use kernel::error::Result;
> ///
> /// fn get_enabled(dev: &Device<Bound>) -> Result<Clk<Enabled>> {
> @@ -240,8 +258,17 @@ fn from(err: Error<State>) -> Self {
> /// The following example demonstrates how to obtain and configure a clock for a device.
> ///
> /// ```
> - /// use kernel::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
> - /// use kernel::device::{Bound, Device};
> + /// use kernel::clk::{
> + /// Clk,
> + /// Enabled,
> + /// Hertz,
> + /// Prepared,
> + /// Unprepared, //
> + /// };
> + /// use kernel::device::{
> + /// Bound,
> + /// Device, //
> + /// };
> /// use kernel::error::Result;
> ///
> /// fn configure_clk(dev: &Device<Bound>) -> Result {
> @@ -287,7 +314,11 @@ fn from(err: Error<State>) -> Self {
> /// and move between the variants:
> ///
> /// ```
> - /// use kernel::clk::{Clk, Enabled, Prepared};
> + /// use kernel::clk::{
> + /// Clk,
> + /// Enabled,
> + /// Prepared, //
> + /// };
> /// use kernel::error::Result;
> ///
> /// enum DeviceClk {
> @@ -481,7 +512,12 @@ pub fn enable(self) -> Result<Clk<Enabled>, Error<Prepared>> {
> /// clock or threading it through an intermediate state, e.g.:
> ///
> /// ```
> - /// use kernel::clk::{Clk, Enabled, Hertz, Prepared};
> + /// use kernel::clk::{
> + /// Clk,
> + /// Enabled,
> + /// Hertz,
> + /// Prepared, //
> + /// };
> /// use kernel::error::Result;
> ///
> /// fn read_rate(clk: &Clk<Prepared>) -> Result<Hertz> {
>
> --
> 2.54.0
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] rust: clk: use the type-state pattern
2026-06-18 3:46 ` Daniel Almeida
@ 2026-06-18 8:10 ` Onur Özkan
-1 siblings, 0 replies; 18+ messages in thread
From: Onur Özkan @ 2026-06-18 8:10 UTC (permalink / raw)
To: Daniel Almeida
Cc: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Michal Wilczynski, Brian Masney,
Boqun Feng, linux-pm, linux-kernel, dri-devel, linux-riscv,
linux-pwm, linux-clk, rust-for-linux, Boris Brezillon
On Thu, 18 Jun 2026 00:46:35 -0300
Daniel Almeida <daniel.almeida@collabora.com> 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 | 512 ++++++++++++++++++++++++++++++-----------
> rust/kernel/cpufreq.rs | 8 +-
> 5 files changed, 396 insertions(+), 174 deletions(-)
>
> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
> index f17bf64c22e2..9d2ec7df4bac 100644
> --- a/drivers/cpufreq/rcpufreq_dt.rs
> +++ b/drivers/cpufreq/rcpufreq_dt.rs
> @@ -40,7 +40,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 279710b36a10..a2230aebfea2 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -3,7 +3,7 @@
> use kernel::{
> clk::{
> Clk,
> - OptionalClk, //
> + Enabled, //
> },
> device::{
> Bound,
> @@ -49,7 +49,7 @@ pub(crate) struct TyrPlatformDriverData {
[...]
> - /// Disable and unprepare the clock.
> - ///
> - /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
> + /// 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 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_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`]
nit: I wouldn't use the word "Attempts" for an infallible function.
> + /// state.
> + #[inline]
> + pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
This is an infallible function, you can return Clk<Prepared> directly.
Thanks,
Onur
> + // 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.
> @@ -251,82 +544,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
> + // [`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 d8d26870bea2..e837bb1010e0 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -553,8 +553,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,
> + name: Option<&CStr>,
> + ) -> Result<Clk<crate::clk::Unprepared>> {
> + let clk = Clk::<crate::clk::Unprepared>::get_unbound(dev, name)?;
> self.as_mut_ref().clk = clk.as_raw();
> Ok(clk)
> }
>
> --
> 2.54.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] rust: clk: use the type-state pattern
@ 2026-06-18 8:10 ` Onur Özkan
0 siblings, 0 replies; 18+ messages in thread
From: Onur Özkan @ 2026-06-18 8:10 UTC (permalink / raw)
To: Daniel Almeida
Cc: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Michal Wilczynski, Brian Masney,
Boqun Feng, linux-pm, linux-kernel, dri-devel, linux-riscv,
linux-pwm, linux-clk, rust-for-linux, Boris Brezillon
On Thu, 18 Jun 2026 00:46:35 -0300
Daniel Almeida <daniel.almeida@collabora.com> 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 | 512 ++++++++++++++++++++++++++++++-----------
> rust/kernel/cpufreq.rs | 8 +-
> 5 files changed, 396 insertions(+), 174 deletions(-)
>
> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
> index f17bf64c22e2..9d2ec7df4bac 100644
> --- a/drivers/cpufreq/rcpufreq_dt.rs
> +++ b/drivers/cpufreq/rcpufreq_dt.rs
> @@ -40,7 +40,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 279710b36a10..a2230aebfea2 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -3,7 +3,7 @@
> use kernel::{
> clk::{
> Clk,
> - OptionalClk, //
> + Enabled, //
> },
> device::{
> Bound,
> @@ -49,7 +49,7 @@ pub(crate) struct TyrPlatformDriverData {
[...]
> - /// Disable and unprepare the clock.
> - ///
> - /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
> + /// 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 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_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`]
nit: I wouldn't use the word "Attempts" for an infallible function.
> + /// state.
> + #[inline]
> + pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
This is an infallible function, you can return Clk<Prepared> directly.
Thanks,
Onur
> + // 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.
> @@ -251,82 +544,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
> + // [`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 d8d26870bea2..e837bb1010e0 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -553,8 +553,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,
> + name: Option<&CStr>,
> + ) -> Result<Clk<crate::clk::Unprepared>> {
> + let clk = Clk::<crate::clk::Unprepared>::get_unbound(dev, name)?;
> self.as_mut_ref().clk = clk.as_raw();
> Ok(clk)
> }
>
> --
> 2.54.0
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 3/3] rust: clk: use 'kernel vertical style' for imports
2026-06-18 3:46 ` Daniel Almeida
@ 2026-06-18 10:40 ` Miguel Ojeda
-1 siblings, 0 replies; 18+ messages in thread
From: Miguel Ojeda @ 2026-06-18 10:40 UTC (permalink / raw)
To: Daniel Almeida
Cc: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Michal Wilczynski, Brian Masney,
Boqun Feng, linux-pm, linux-kernel, dri-devel, linux-riscv,
linux-pwm, linux-clk, rust-for-linux, Boris Brezillon
On Thu, Jun 18, 2026 at 5:48 AM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> + Result, //
Since you import the prelude, I think you can take the change to
remove e.g. this one.
> /// use kernel::error::Result;
> /// use kernel::error::Result;
> /// use kernel::error::Result;
> /// use kernel::error::Result;
Similarly, the prelude is imported in doctests by default.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 3/3] rust: clk: use 'kernel vertical style' for imports
@ 2026-06-18 10:40 ` Miguel Ojeda
0 siblings, 0 replies; 18+ messages in thread
From: Miguel Ojeda @ 2026-06-18 10:40 UTC (permalink / raw)
To: Daniel Almeida
Cc: Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich, Alice Ryhl,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Michal Wilczynski, Brian Masney,
Boqun Feng, linux-pm, linux-kernel, dri-devel, linux-riscv,
linux-pwm, linux-clk, rust-for-linux, Boris Brezillon
On Thu, Jun 18, 2026 at 5:48 AM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> + Result, //
Since you import the prelude, I think you can take the change to
remove e.g. this one.
> /// use kernel::error::Result;
> /// use kernel::error::Result;
> /// use kernel::error::Result;
> /// use kernel::error::Result;
Similarly, the prelude is imported in doctests by default.
Cheers,
Miguel
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] rust: clk: use the type-state pattern
2026-06-18 3:46 ` Daniel Almeida
@ 2026-06-28 20:15 ` Maurice
-1 siblings, 0 replies; 18+ messages in thread
From: Maurice @ 2026-06-28 20:15 UTC (permalink / raw)
To: Daniel Almeida, Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich,
Alice Ryhl, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Michal Wilczynski, Brian Masney,
Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
On Thu Jun 18, 2026 at 5:46 AM CEST, Daniel Almeida wrote:
> + /// 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<Prepared>> {
> + Clk::<Unprepared>::get_optional(dev, name)?
> + .prepare()
> + .map_err(|error| error.error)
> + }
> +
> + /// Attempts to convert the [`Clk`] to an [`Unprepared`] state.
> ///
> - /// [`clk_disable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_disable
> + /// Equivalent to the kernel's [`clk_unprepare`] API.
Missing definition of `clk_unprepare`.
Best,
Maurice
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] rust: clk: use the type-state pattern
@ 2026-06-28 20:15 ` Maurice
0 siblings, 0 replies; 18+ messages in thread
From: Maurice @ 2026-06-28 20:15 UTC (permalink / raw)
To: Daniel Almeida, Rafael J. Wysocki, Viresh Kumar, Danilo Krummrich,
Alice Ryhl, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter, Drew Fustini, Guo Ren, Fu Wei,
Uwe Kleine-König, Michael Turquette, Stephen Boyd,
Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Michal Wilczynski, Brian Masney,
Boqun Feng
Cc: linux-pm, linux-kernel, dri-devel, linux-riscv, linux-pwm,
linux-clk, rust-for-linux, Boris Brezillon
On Thu Jun 18, 2026 at 5:46 AM CEST, Daniel Almeida wrote:
> + /// 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<Prepared>> {
> + Clk::<Unprepared>::get_optional(dev, name)?
> + .prepare()
> + .map_err(|error| error.error)
> + }
> +
> + /// Attempts to convert the [`Clk`] to an [`Unprepared`] state.
> ///
> - /// [`clk_disable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_disable
> + /// Equivalent to the kernel's [`clk_unprepare`] API.
Missing definition of `clk_unprepare`.
Best,
Maurice
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2026-06-28 20:15 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-18 3:46 [PATCH v4 0/3] Clk improvements Daniel Almeida
2026-06-18 3:46 ` Daniel Almeida
2026-06-18 3:46 ` [PATCH v4 1/3] rust: clk: use the type-state pattern Daniel Almeida
2026-06-18 3:46 ` Daniel Almeida
2026-06-18 3:56 ` sashiko-bot
2026-06-18 8:10 ` Onur Özkan
2026-06-18 8:10 ` Onur Özkan
2026-06-28 20:15 ` Maurice
2026-06-28 20:15 ` Maurice
2026-06-18 3:46 ` [PATCH v4 2/3] rust: clk: add devres-managed clks Daniel Almeida
2026-06-18 3:46 ` Daniel Almeida
2026-06-18 3:57 ` sashiko-bot
2026-06-18 3:46 ` [PATCH v4 3/3] rust: clk: use 'kernel vertical style' for imports Daniel Almeida
2026-06-18 3:46 ` Daniel Almeida
2026-06-18 7:58 ` Onur Özkan
2026-06-18 7:58 ` Onur Özkan
2026-06-18 10:40 ` Miguel Ojeda
2026-06-18 10:40 ` Miguel Ojeda
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.