[PATCH v2 0/2] Clk improvements for 6.18

[PATCH v2 0/2] Clk improvements for 6.18

[Daniel Almeida]

This series contains a few improvements that will be soon needed by
drivers:

Patch 1 implements Send and Sync for Clocks, as a raw pointer precludes
the compiler from automatically implementing these traits. This will
lead to an otherwise unnecessary unsafe implementation of Send and Sync
for all drivers that use kernel::clk::Clk. It was included in this
series as it would otherwise conflict with patch 2.

Patch 2 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.

---
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

---
Alice Ryhl (1):
      rust: clk: implement Send and Sync

Daniel Almeida (1):
      rust: clk: use the type-state pattern

 drivers/cpufreq/rcpufreq_dt.rs |   2 +-
 rust/kernel/clk.rs             | 405 ++++++++++++++++++++++++++++-------------
 rust/kernel/cpufreq.rs         |   8 +-
 3 files changed, 283 insertions(+), 132 deletions(-)
---
base-commit: 76eeb9b8de9880ca38696b2fb56ac45ac0a25c6c
change-id: 20250909-clk-type-state-c01aa7dd551d

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>

[PATCH v2 1/2] rust: clk: implement Send and Sync

[Daniel Almeida]

From: Alice Ryhl <aliceryhl@google.com>

These traits are required for drivers to embed the Clk type in their own
data structures because driver data structures are usually required to
be Send. See e.g. [1] for the kind of workaround that drivers currently
need due to lacking this annotation.

Link: https://lore.kernel.org/rust-for-linux/20250812-tyr-v2-1-9e0f3dc9da95@collabora.com/ [1]
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/clk.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index 1e6c8c42fb3a321951e275101848b35e1ae5c2a8..0a290202da69669d670ddad2b6762a1d5f1d912e 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -129,6 +129,13 @@ mod common_clk {
     #[repr(transparent)]
     pub struct Clk(*mut bindings::clk);
 
+    // SAFETY: It is safe to call `clk_put` on another thread than where `clk_get` was called.
+    unsafe impl Send for Clk {}
+
+    // 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 {}
+
     impl Clk {
         /// Gets [`Clk`] corresponding to a [`Device`] and a connection id.
         ///

-- 
2.51.0

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Boqun Feng]

On Wed, Sep 10, 2025 at 02:28:27PM -0300, Daniel Almeida wrote:
> From: Alice Ryhl <aliceryhl@google.com>
> 
> These traits are required for drivers to embed the Clk type in their own
> data structures because driver data structures are usually required to
> be Send. See e.g. [1] for the kind of workaround that drivers currently
> need due to lacking this annotation.
> 
> Link: https://lore.kernel.org/rust-for-linux/20250812-tyr-v2-1-9e0f3dc9da95@collabora.com/ [1]
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

This tag list looks a bit weird to me. Why is there a SoB from you
before Alice's SoB? At least for the usage I'm familiar with, outside
the case of Co-developed-bys, multiple SoBs is used for recording how
the patches are routed. For example, if I have a patch that has my SoB
and I send it to you, you queue in your tree and then send out to other
maintainers for merging, in general you would put your SoB after mine in
that case. But I don't think that's case here? Alice's patch has only
her SoB:

	https://lore.kernel.org/rust-for-linux/20250904-clk-send-sync-v1-1-48d023320eb8@google.com/

What's the intention of the SoB tag here?

Otherwise the patch looks good to me. If we get the tag list resolved,
feel free to add:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> ---
>  rust/kernel/clk.rs | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index 1e6c8c42fb3a321951e275101848b35e1ae5c2a8..0a290202da69669d670ddad2b6762a1d5f1d912e 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs
> @@ -129,6 +129,13 @@ mod common_clk {
>      #[repr(transparent)]
>      pub struct Clk(*mut bindings::clk);
>  
> +    // SAFETY: It is safe to call `clk_put` on another thread than where `clk_get` was called.
> +    unsafe impl Send for Clk {}
> +
> +    // 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 {}
> +
>      impl Clk {
>          /// Gets [`Clk`] corresponding to a [`Device`] and a connection id.
>          ///
> 
> -- 
> 2.51.0
> 

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Daniel Almeida]

Hi Boqun,

> On 10 Sep 2025, at 14:49, Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> On Wed, Sep 10, 2025 at 02:28:27PM -0300, Daniel Almeida wrote:
>> From: Alice Ryhl <aliceryhl@google.com>
>> 
>> These traits are required for drivers to embed the Clk type in their own
>> data structures because driver data structures are usually required to
>> be Send. See e.g. [1] for the kind of workaround that drivers currently
>> need due to lacking this annotation.
>> 
>> Link: https://lore.kernel.org/rust-for-linux/20250812-tyr-v2-1-9e0f3dc9da95@collabora.com/ [1]
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> This tag list looks a bit weird to me. Why is there a SoB from you
> before Alice's SoB? At least for the usage I'm familiar with, outside
> the case of Co-developed-bys, multiple SoBs is used for recording how
> the patches are routed. For example, if I have a patch that has my SoB
> and I send it to you, you queue in your tree and then send out to other
> maintainers for merging, in general you would put your SoB after mine in
> that case. But I don't think that's case here? Alice's patch has only
> her SoB:
> 
> https://lore.kernel.org/rust-for-linux/20250904-clk-send-sync-v1-1-48d023320eb8@google.com/
> 
> What's the intention of the SoB tag here?
> 
> Otherwise the patch looks good to me. If we get the tag list resolved,
> feel free to add:
> 
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> 
> Regards,
> Boqun
> 

You have to include your SOB when submitting patches from others.

This is something I tend to forget often, so I made sure it was there. The
order may be indeed off though.

— Daniel

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Boqun Feng]

On Wed, Sep 10, 2025 at 03:47:30PM -0300, Daniel Almeida wrote:
> Hi Boqun,
> 
> > On 10 Sep 2025, at 14:49, Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> > On Wed, Sep 10, 2025 at 02:28:27PM -0300, Daniel Almeida wrote:
> >> From: Alice Ryhl <aliceryhl@google.com>
> >> 
> >> These traits are required for drivers to embed the Clk type in their own
> >> data structures because driver data structures are usually required to
> >> be Send. See e.g. [1] for the kind of workaround that drivers currently
> >> need due to lacking this annotation.
> >> 
> >> Link: https://lore.kernel.org/rust-for-linux/20250812-tyr-v2-1-9e0f3dc9da95@collabora.com/ [1]
> >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> >> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> >> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> >> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> > 
> > This tag list looks a bit weird to me. Why is there a SoB from you
> > before Alice's SoB? At least for the usage I'm familiar with, outside
> > the case of Co-developed-bys, multiple SoBs is used for recording how
> > the patches are routed. For example, if I have a patch that has my SoB
> > and I send it to you, you queue in your tree and then send out to other
> > maintainers for merging, in general you would put your SoB after mine in
> > that case. But I don't think that's case here? Alice's patch has only
> > her SoB:
> > 
> > https://lore.kernel.org/rust-for-linux/20250904-clk-send-sync-v1-1-48d023320eb8@google.com/
> > 
> > What's the intention of the SoB tag here?
> > 
> > Otherwise the patch looks good to me. If we get the tag list resolved,
> > feel free to add:
> > 
> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > 
> > Regards,
> > Boqun
> > 
> 
> You have to include your SOB when submitting patches from others.
> 
> This is something I tend to forget often, so I made sure it was there. The
> order may be indeed off though.
> 

I mean you can just use `b4 shazam -s` to apply the patch and add your
SoB at the end ;-)

Regards,
Boqun

> - Daniel
> 

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Stephen Boyd]

Quoting Daniel Almeida (2025-09-10 11:47:30)
> Hi Boqun,
> 
> > On 10 Sep 2025, at 14:49, Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> > On Wed, Sep 10, 2025 at 02:28:27PM -0300, Daniel Almeida wrote:
> >> From: Alice Ryhl <aliceryhl@google.com>
> >> 
> >> These traits are required for drivers to embed the Clk type in their own
> >> data structures because driver data structures are usually required to
> >> be Send. See e.g. [1] for the kind of workaround that drivers currently
> >> need due to lacking this annotation.
> >> 
> >> Link: https://lore.kernel.org/rust-for-linux/20250812-tyr-v2-1-9e0f3dc9da95@collabora.com/ [1]
> >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> >> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> >> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> >> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> > 
> > This tag list looks a bit weird to me. Why is there a SoB from you
> > before Alice's SoB? At least for the usage I'm familiar with, outside
> > the case of Co-developed-bys, multiple SoBs is used for recording how
> > the patches are routed. For example, if I have a patch that has my SoB
> > and I send it to you, you queue in your tree and then send out to other
> > maintainers for merging, in general you would put your SoB after mine in
> > that case. But I don't think that's case here? Alice's patch has only
> > her SoB:
> > 
> > https://lore.kernel.org/rust-for-linux/20250904-clk-send-sync-v1-1-48d023320eb8@google.com/
> > 
> > What's the intention of the SoB tag here?
> > 
> > Otherwise the patch looks good to me. If we get the tag list resolved,
> > feel free to add:
> > 
> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > 
> > Regards,
> > Boqun
> > 
> 
> You have to include your SOB when submitting patches from others.
> 
> This is something I tend to forget often, so I made sure it was there. The
> order may be indeed off though.

Yes the order is wrong. The first SoB should be the commit author.

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Daniel Almeida]

Hi Stephen,

[…]

>> 
>> You have to include your SOB when submitting patches from others.
>> 
>> This is something I tend to forget often, so I made sure it was there. The
>> order may be indeed off though.
> 
> Yes the order is wrong. The first SoB should be the commit author.

Do you want me to send a new version, or do you prefer fixing this before applying?

For trivial changes some maintainers prefer the latter to reduce the amount of noise.

— Daniel

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Stephen Boyd]

Quoting Daniel Almeida (2025-09-20 03:33:07)
> Hi Stephen,
> 
> […]
> 
> >> 
> >> You have to include your SOB when submitting patches from others.
> >> 
> >> This is something I tend to forget often, so I made sure it was there. The
> >> order may be indeed off though.
> > 
> > Yes the order is wrong. The first SoB should be the commit author.
> 
> Do you want me to send a new version, or do you prefer fixing this before applying?
> 
> For trivial changes some maintainers prefer the latter to reduce the amount of noise.
> 

You can send another round. I'm waiting to see if anyone will review the
second patch. Maybe sending another round will prompt a review.

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Alice Ryhl]

On Sat, Sep 20, 2025 at 7:06 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Daniel Almeida (2025-09-10 11:47:30)
> > Hi Boqun,
> >
> > > On 10 Sep 2025, at 14:49, Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Wed, Sep 10, 2025 at 02:28:27PM -0300, Daniel Almeida wrote:
> > >> From: Alice Ryhl <aliceryhl@google.com>
> > >>
> > >> These traits are required for drivers to embed the Clk type in their own
> > >> data structures because driver data structures are usually required to
> > >> be Send. See e.g. [1] for the kind of workaround that drivers currently
> > >> need due to lacking this annotation.
> > >>
> > >> Link: https://lore.kernel.org/rust-for-linux/20250812-tyr-v2-1-9e0f3dc9da95@collabora.com/ [1]
> > >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> > >> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > >> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
> > >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > >> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> > >
> > > This tag list looks a bit weird to me. Why is there a SoB from you
> > > before Alice's SoB? At least for the usage I'm familiar with, outside
> > > the case of Co-developed-bys, multiple SoBs is used for recording how
> > > the patches are routed. For example, if I have a patch that has my SoB
> > > and I send it to you, you queue in your tree and then send out to other
> > > maintainers for merging, in general you would put your SoB after mine in
> > > that case. But I don't think that's case here? Alice's patch has only
> > > her SoB:
> > >
> > > https://lore.kernel.org/rust-for-linux/20250904-clk-send-sync-v1-1-48d023320eb8@google.com/
> > >
> > > What's the intention of the SoB tag here?
> > >
> > > Otherwise the patch looks good to me. If we get the tag list resolved,
> > > feel free to add:
> > >
> > > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > >
> > > Regards,
> > > Boqun
> > >
> >
> > You have to include your SOB when submitting patches from others.
> >
> > This is something I tend to forget often, so I made sure it was there. The
> > order may be indeed off though.
>
> Yes the order is wrong. The first SoB should be the commit author.

One optoin is to just land the original patch:
https://lore.kernel.org/all/20250904-clk-send-sync-v1-1-48d023320eb8@google.com/

Alice

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Daniel Almeida]

> On 20 Sep 2025, at 19:12, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Sat, Sep 20, 2025 at 7:06 AM Stephen Boyd <sboyd@kernel.org> wrote:
>> 
>> Quoting Daniel Almeida (2025-09-10 11:47:30)
>>> Hi Boqun,
>>> 
>>>> On 10 Sep 2025, at 14:49, Boqun Feng <boqun.feng@gmail.com> wrote:
>>>> 
>>>> On Wed, Sep 10, 2025 at 02:28:27PM -0300, Daniel Almeida wrote:
>>>>> From: Alice Ryhl <aliceryhl@google.com>
>>>>> 
>>>>> These traits are required for drivers to embed the Clk type in their own
>>>>> data structures because driver data structures are usually required to
>>>>> be Send. See e.g. [1] for the kind of workaround that drivers currently
>>>>> need due to lacking this annotation.
>>>>> 
>>>>> Link: https://lore.kernel.org/rust-for-linux/20250812-tyr-v2-1-9e0f3dc9da95@collabora.com/ [1]
>>>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>>>> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
>>>> 
>>>> This tag list looks a bit weird to me. Why is there a SoB from you
>>>> before Alice's SoB? At least for the usage I'm familiar with, outside
>>>> the case of Co-developed-bys, multiple SoBs is used for recording how
>>>> the patches are routed. For example, if I have a patch that has my SoB
>>>> and I send it to you, you queue in your tree and then send out to other
>>>> maintainers for merging, in general you would put your SoB after mine in
>>>> that case. But I don't think that's case here? Alice's patch has only
>>>> her SoB:
>>>> 
>>>> https://lore.kernel.org/rust-for-linux/20250904-clk-send-sync-v1-1-48d023320eb8@google.com/
>>>> 
>>>> What's the intention of the SoB tag here?
>>>> 
>>>> Otherwise the patch looks good to me. If we get the tag list resolved,
>>>> feel free to add:
>>>> 
>>>> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>>>> 
>>>> Regards,
>>>> Boqun
>>>> 
>>> 
>>> You have to include your SOB when submitting patches from others.
>>> 
>>> This is something I tend to forget often, so I made sure it was there. The
>>> order may be indeed off though.
>> 
>> Yes the order is wrong. The first SoB should be the commit author.
> 
> One optoin is to just land the original patch:
> https://lore.kernel.org/all/20250904-clk-send-sync-v1-1-48d023320eb8@google.com/
> 
> Alice

I guess this makes even more sense. I was hoping to land these two together,
but clearly this will not be possible for the time being as the second patch
has no r-b tags.

— Daniel

[PATCH v2 2/2] rust: clk: use the type-state pattern

[Daniel Almeida]

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 +-
 rust/kernel/clk.rs             | 412 +++++++++++++++++++++++++++--------------
 rust/kernel/cpufreq.rs         |   8 +-
 3 files changed, 283 insertions(+), 139 deletions(-)

diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index 7e1fbf9a091f74065f9e14e7e9b5195213642452..79b12d75f4e5df67b413624514bc297573b483e6 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -45,7 +45,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/rust/kernel/clk.rs b/rust/kernel/clk.rs
index 0a290202da69669d670ddad2b6762a1d5f1d912e..0c65b8c10390624e2528aa2d726216ad3df8274c 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -85,12 +85,116 @@ mod common_clk {
         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 {}
+    }
+
+    /// Obtains and enables a [`devres`]-managed [`Clk`] for a device.
+    ///
+    /// [`devres`]: crate::devres::Devres
+    pub fn devm_enable(dev: &Device, name: Option<&CStr>) -> Result {
+        let name = name.map_or(ptr::null(), |n| n.as_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 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, name: Option<&CStr>) -> Result {
+        let name = name.map_or(ptr::null(), |n| n.as_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,
+        name: Option<&CStr>,
+        rate: Hertz,
+    ) -> Result {
+        let name = name.map_or(ptr::null(), |n| n.as_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.
+        const DISABLE_ON_DROP: bool;
+
+        /// Whether the clock should be unprepared when dropped.
+        const UNPREPARE_ON_DROP: bool;
+    }
+
+    /// A state where the [`Clk`] is not prepared and not enabled.
+    pub struct Unprepared;
+
+    /// A state where the [`Clk`] is prepared but not enabled.
+    pub struct Prepared;
+
+    /// A state where the [`Clk`] is both prepared and enabled.
+    pub struct Enabled;
+
+    impl ClkState for Unprepared {
+        const DISABLE_ON_DROP: bool = false;
+        const UNPREPARE_ON_DROP: bool = false;
+    }
+
+    impl ClkState for Prepared {
+        const DISABLE_ON_DROP: bool = false;
+        const UNPREPARE_ON_DROP: bool = true;
+    }
+
+    impl ClkState for Enabled {
+        const DISABLE_ON_DROP: bool = true;
+        const UNPREPARE_ON_DROP: bool = true;
+    }
+
+    /// An error that can occur when trying to convert a [`Clk`] between states.
+    pub struct Error<State: ClkState> {
+        /// The error that occurred.
+        pub error: kernel::error::Error,
+
+        /// The [`Clk`] that caused the error, so that the operation may be
+        /// retried.
+        pub clk: Clk<State>,
+    }
 
     /// A reference-counted clock.
     ///
     /// Rust abstraction for the C [`struct clk`].
     ///
+    /// A [`Clk`] instance represents a clock that can be in one of several
+    /// states: [`Unprepared`], [`Prepared`], or [`Enabled`].
+    ///
+    /// No action needs to be taken when a [`Clk`] is dropped. The calls to
+    /// `clk_unprepare()` and `clk_disable()` will be placed as applicable.
+    ///
+    /// An optional [`Clk`] is treated just like a regular [`Clk`], but its
+    /// inner `struct clk` pointer is `NULL`. This interfaces correctly with the
+    /// C API and also exposes all the methods of a regular [`Clk`] to users.
+    ///
     /// # Invariants
     ///
     /// A [`Clk`] instance holds either a pointer to a valid [`struct clk`] created by the C
@@ -99,20 +203,39 @@ mod common_clk {
     /// Instances of this type are reference-counted. Calling [`Clk::get`] ensures that the
     /// allocation remains valid for the lifetime of the [`Clk`].
     ///
+    /// The [`Prepared`] state is associated with a single count of
+    /// `clk_prepare()`, and the [`Enabled`] state is associated with a single
+    /// count of `clk_enable()`, and the [`Prepared`] state is associated with a
+    /// single count of `clk_prepare()` and `clk_enable()`.
+    ///
+    /// All states are associated with a single count of `clk_get()`.
+    ///
     /// # Examples
     ///
     /// The following example demonstrates how to obtain and configure a clock for a device.
     ///
     /// ```
     /// use kernel::c_str;
-    /// use kernel::clk::{Clk, Hertz};
+    /// use kernel::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
     /// use kernel::device::Device;
     /// use kernel::error::Result;
     ///
     /// fn configure_clk(dev: &Device) -> Result {
-    ///     let clk = Clk::get(dev, Some(c_str!("apb_clk")))?;
+    ///     // The fastest way is to use a version of `Clk::get` for the desired
+    ///     // state, i.e.:
+    ///     let clk: Clk<Enabled> = Clk::<Enabled>::get(dev, Some(c_str!("apb_clk")))?;
+    ///
+    ///     // Any other state is also possible, e.g.:
+    ///     let clk: Clk<Prepared> = Clk::<Prepared>::get(dev, Some(c_str!("apb_clk")))?;
+    ///
+    ///     // Later:
+    ///     let clk: Clk<Enabled> = clk.enable().map_err(|error| {
+    ///         error.error
+    ///     })?;
     ///
-    ///     clk.prepare_enable()?;
+    ///     // Note that error.clk is the original `clk` if the operation
+    ///     // failed. It is provided as a convenience so that the operation may be
+    ///     // retried in case of errors.
     ///
     ///     let expected_rate = Hertz::from_ghz(1);
     ///
@@ -120,111 +243,173 @@ mod common_clk {
     ///         clk.set_rate(expected_rate)?;
     ///     }
     ///
-    ///     clk.disable_unprepare();
+    ///     // Nothing is needed here. The drop implementation will undo any
+    ///     // operations as appropriate.
+    ///     Ok(())
+    /// }
+    ///
+    /// fn shutdown(clk: Clk<Enabled>) -> Result {
+    ///     // The states can be traversed "in the reverse order" as well:
+    ///     let clk: Clk<Prepared> = clk.disable().map_err(|error| {
+    ///         error.error
+    ///     })?;
+    ///
+    ///     // This is of type `Clk<Unprepared>`.
+    ///     let clk = clk.unprepare();
+    ///
     ///     Ok(())
     /// }
     /// ```
     ///
     /// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
     #[repr(transparent)]
-    pub struct Clk(*mut bindings::clk);
-
-    // SAFETY: It is safe to call `clk_put` on another thread than where `clk_get` was called.
-    unsafe impl Send for Clk {}
-
-    // 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 {}
+    pub struct Clk<T: ClkState> {
+        inner: *mut bindings::clk,
+        _phantom: core::marker::PhantomData<T>,
+    }
 
-    impl Clk {
+    impl Clk<Unprepared> {
         /// Gets [`Clk`] corresponding to a [`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, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
             let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
 
             // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
-            //
+            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, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
+            let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
+
+            // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
+            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 [`Device`] and a connection id and prepares it.
         ///
-        /// Equivalent to the kernel's [`clk_disable`] API.
-        ///
-        /// [`clk_disable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_disable
+        /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
         #[inline]
-        pub fn disable(&self) {
-            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
-            // [`clk_disable`].
-            unsafe { bindings::clk_disable(self.as_raw()) };
+        pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Clk<Prepared>> {
+            Clk::<Unprepared>::get(dev, name)?
+                .prepare()
+                .map_err(|error| error.error)
         }
 
-        /// Prepare the clock.
-        ///
-        /// Equivalent to the kernel's [`clk_prepare`] API.
+        /// Attempts to convert the [`Clk`] to an [`Unprepared`] state.
         ///
-        /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
+        /// Equivalent to the kernel's [`clk_unprepare`] API.
         #[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 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,
+            }
         }
 
-        /// Unprepare the clock.
+        /// Attempts to convert the [`Clk`] to an [`Enabled`] state.
         ///
-        /// Equivalent to the kernel's [`clk_unprepare`] API.
+        /// Equivalent to the kernel's [`clk_enable`] API.
         ///
-        /// [`clk_unprepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_unprepare
+        /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
         #[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 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),
+                })
         }
+    }
 
-        /// Prepare and enable the clock.
+    impl Clk<Enabled> {
+        /// Gets [`Clk`] corresponding to a [`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, 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`].
         #[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()) };
+        /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]
+        /// state.
+        pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
+            // We will be transferring the ownership of our `clk_get()` and
+            // `clk_enable()` counts to `Clk<Prepared>`.
+            let clk = ManuallyDrop::new(self);
+
+            // SAFETY: By the type invariants, self.0 is a valid argument for [`clk_disable`].
+            unsafe { bindings::clk_disable(clk.as_raw()) };
+
+            Ok(Clk {
+                inner: clk.inner,
+                _phantom: PhantomData,
+            })
         }
 
         /// Get clock's rate.
@@ -252,85 +437,40 @@ 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()) };
+    impl<T: ClkState> Clk<T> {
+        /// Obtain the raw [`struct clk`] pointer.
+        #[inline]
+        pub fn as_raw(&self) -> *mut bindings::clk {
+            self.inner
         }
     }
 
-    /// A reference-counted optional clock.
-    ///
-    /// A lightweight wrapper around an optional [`Clk`]. An [`OptionalClk`] represents a [`Clk`]
-    /// that a driver can function without but may improve performance or enable additional
-    /// features when available.
-    ///
-    /// # Invariants
-    ///
-    /// An [`OptionalClk`] instance encapsulates a [`Clk`] with either a valid [`struct clk`] or
-    /// `NULL` pointer.
-    ///
-    /// Instances of this type are reference-counted. Calling [`OptionalClk::get`] ensures that the
-    /// allocation remains valid for the lifetime of the [`OptionalClk`].
-    ///
-    /// # Examples
-    ///
-    /// The following example demonstrates how to obtain and configure an optional clock for a
-    /// device. The code functions correctly whether or not the clock is available.
-    ///
-    /// ```
-    /// use kernel::c_str;
-    /// use kernel::clk::{OptionalClk, Hertz};
-    /// use kernel::device::Device;
-    /// use kernel::error::Result;
-    ///
-    /// fn configure_clk(dev: &Device) -> Result {
-    ///     let clk = OptionalClk::get(dev, Some(c_str!("apb_clk")))?;
-    ///
-    ///     clk.prepare_enable()?;
-    ///
-    ///     let expected_rate = Hertz::from_ghz(1);
-    ///
-    ///     if clk.rate() != expected_rate {
-    ///         clk.set_rate(expected_rate)?;
-    ///     }
-    ///
-    ///     clk.disable_unprepare();
-    ///     Ok(())
-    /// }
-    /// ```
-    ///
-    /// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
-    pub struct OptionalClk(Clk);
-
-    impl OptionalClk {
-        /// Gets [`OptionalClk`] corresponding to a [`Device`] and a connection id.
-        ///
-        /// Equivalent to the kernel's [`clk_get_optional`] API.
-        ///
-        /// [`clk_get_optional`]:
-        /// https://docs.kernel.org/core-api/kernel-api.html#c.clk_get_optional
-        pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
-            let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
+    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()) };
+            }
 
-            // 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)
-            })?)))
+            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
+            // [`clk_put`].
+            unsafe { bindings::clk_put(self.as_raw()) };
         }
     }
 
-    // Make [`OptionalClk`] behave like [`Clk`].
-    impl Deref for OptionalClk {
-        type Target = Clk;
+    // SAFETY: It is safe to call `clk_put` on another thread than where `clk_get` was called.
+    unsafe impl<T: ClkState> Send for Clk<T> {}
 
-        fn deref(&self) -> &Clk {
-            &self.0
-        }
-    }
+    // SAFETY: It is safe to call any combination of the `&self` methods in parallel, as the
+    // methods are synchronized internally.
+    unsafe impl<T: ClkState> Sync for Clk<T> {}
 }
 
 #[cfg(CONFIG_COMMON_CLK)]
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index afc15e72a7c37ac781f25a8a6edd804fa4c9658a..9b0d0e4198c7388af59e7b89ba8ac7b3f4fe1561 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(dev, name)?;
         self.as_mut_ref().clk = clk.as_raw();
         Ok(clk)
     }

-- 
2.51.0

Re: [PATCH v2 2/2] rust: clk: use the type-state pattern

[Daniel Almeida]

Hi everyone, gentle ping for reviewing this patch :)

> On 10 Sep 2025, at 19:28, 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().

^ this really sucks so we should probably take in this change.

— Daniel

Re: [PATCH v2 2/2] rust: clk: use the type-state pattern

[Danilo Krummrich]

On Wed Sep 10, 2025 at 7:28 PM CEST, Daniel Almeida wrote:
> +    /// Obtains and enables a [`devres`]-managed [`Clk`] for a device.
> +    ///
> +    /// [`devres`]: crate::devres::Devres
> +    pub fn devm_enable(dev: &Device, name: Option<&CStr>) -> Result {
> +        let name = name.map_or(ptr::null(), |n| n.as_ptr());
> +
> +        // SAFETY: It is safe to call [`devm_clk_get_enabled`] with a valid
> +        // device pointer.

It's not, since this calls into devres it is only safe with a pointer to a bound
device, i.e. you need to require &Device<Bound>.

You also need to justify the CStr pointer in terms of being NULL and its
lifetime.

> +        from_err_ptr(unsafe { bindings::devm_clk_get_enabled(dev.as_raw(), name) })?;
> +        Ok(())
> +    }
> +
> +    /// Obtains and enables a [`devres`]-managed [`Clk`] for a 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, name: Option<&CStr>) -> Result {
> +        let name = name.map_or(ptr::null(), |n| n.as_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,
> +        name: Option<&CStr>,
> +        rate: Hertz,
> +    ) -> Result {
> +        let name = name.map_or(ptr::null(), |n| n.as_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(())
> +    }

I think those should be added in a separate patch.

> +    impl Clk<Unprepared> {
>          /// Gets [`Clk`] corresponding to a [`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, name: Option<&CStr>) -> Result<Clk<Unprepared>> {

Not related to your change, but I'm not sure we should allow drivers to mess
with clocks when they can't prove that they're still bound to the corresponding
device.

It's not introducing any safety issues or unsoundness, but it's not the correct
thing to do semantically.

>              let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
>  
>              // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
> -            //
> +            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, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
> +            let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
> +
> +            // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.

What about con_id?

> +            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.

I know you're consistent with other places, but this seems a odd. This doesn't
correspond to: "A [`Clk`] instance holds either a pointer to a valid [`struct
clk`] created by the C portion of the kernel or a NULL pointer."

> +            Ok(Self {
> +                inner,
> +                _phantom: PhantomData,
> +            })
>          }

<snip>

> +            // SAFETY: By the type invariants, self.as_raw() is a valid argument for

Missing backticks (also in a few other places).

> +            // [`clk_put`].
> +            unsafe { bindings::clk_put(self.as_raw()) };
>          }
>      }

[PATCH v2 0/2] Implement Send and Sync for clk

[Alice Ryhl]

I added a patch to remove the Send/Sync impl for the Tyr driver as well.
I think it's fine for the Tyr patch to land through whichever tree takes
the clk patch, as there should be no non-trivial merge conflicts. Or we
can also take the clk patch through drm-rust where tyr patches normally
go if preferred by clk maintainers.

Regarding Daniel's patch [1], I suggest that Daniel sends his type-state
change rebased on top of this series (without including the patches in
this series in his).

[1]: https://lore.kernel.org/rust-for-linux/20250910-clk-type-state-v2-2-1b97c11bb631@collabora.com/

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v2:
- Rebase on v6.18-rc1.
- Add patch to tyr driver.
- Link to v1: https://lore.kernel.org/r/20250904-clk-send-sync-v1-1-48d023320eb8@google.com

---
Alice Ryhl (2):
      rust: clk: implement Send and Sync
      tyr: remove impl Send/Sync for TyrData

 drivers/gpu/drm/tyr/driver.rs | 12 ------------
 rust/kernel/clk.rs            |  7 +++++++
 2 files changed, 7 insertions(+), 12 deletions(-)
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20250904-clk-send-sync-3cfa7f4e1ce2

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

[PATCH v2 1/2] rust: clk: implement Send and Sync

[Alice Ryhl]

These traits are required for drivers to embed the Clk type in their own
data structures because driver data structures are usually required to
be Send. Since the Clk type is thread-safe, implement the relevant
traits.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/clk.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index 1e6c8c42fb3a321951e275101848b35e1ae5c2a8..0a290202da69669d670ddad2b6762a1d5f1d912e 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -129,6 +129,13 @@ mod common_clk {
     #[repr(transparent)]
     pub struct Clk(*mut bindings::clk);
 
+    // SAFETY: It is safe to call `clk_put` on another thread than where `clk_get` was called.
+    unsafe impl Send for Clk {}
+
+    // 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 {}
+
     impl Clk {
         /// Gets [`Clk`] corresponding to a [`Device`] and a connection id.
         ///

-- 
2.51.0.915.g61a8936c21-goog

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Viresh Kumar]

On 20-10-25, 09:35, Alice Ryhl wrote:
> These traits are required for drivers to embed the Clk type in their own
> data structures because driver data structures are usually required to
> be Send. Since the Clk type is thread-safe, implement the relevant
> traits.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/clk.rs | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index 1e6c8c42fb3a321951e275101848b35e1ae5c2a8..0a290202da69669d670ddad2b6762a1d5f1d912e 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs
> @@ -129,6 +129,13 @@ mod common_clk {
>      #[repr(transparent)]
>      pub struct Clk(*mut bindings::clk);
>  
> +    // SAFETY: It is safe to call `clk_put` on another thread than where `clk_get` was called.
> +    unsafe impl Send for Clk {}
> +
> +    // 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 {}
> +
>      impl Clk {
>          /// Gets [`Clk`] corresponding to a [`Device`] and a connection id.
>          ///

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Alice Ryhl]

On Wed, Oct 22, 2025 at 09:21:38AM +0530, Viresh Kumar wrote:
> On 20-10-25, 09:35, Alice Ryhl wrote:
> > These traits are required for drivers to embed the Clk type in their own
> > data structures because driver data structures are usually required to
> > be Send. Since the Clk type is thread-safe, implement the relevant
> > traits.
> > 
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/clk.rs | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> > index 1e6c8c42fb3a321951e275101848b35e1ae5c2a8..0a290202da69669d670ddad2b6762a1d5f1d912e 100644
> > --- a/rust/kernel/clk.rs
> > +++ b/rust/kernel/clk.rs
> > @@ -129,6 +129,13 @@ mod common_clk {
> >      #[repr(transparent)]
> >      pub struct Clk(*mut bindings::clk);
> >  
> > +    // SAFETY: It is safe to call `clk_put` on another thread than where `clk_get` was called.
> > +    unsafe impl Send for Clk {}
> > +
> > +    // 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 {}
> > +
> >      impl Clk {
> >          /// Gets [`Clk`] corresponding to a [`Device`] and a connection id.
> >          ///
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

I'm guessing this means you want me to take it through drm-rust? See
what I put in the cover letter about choice of tree.

Alice

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Viresh Kumar]

On 22-10-25, 08:19, Alice Ryhl wrote:
> I'm guessing this means you want me to take it through drm-rust? See
> what I put in the cover letter about choice of tree.

I was expecting Stephen to pick it up directly.

-- 
viresh

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Alice Ryhl]

On Wed, Oct 22, 2025 at 03:03:26PM +0530, Viresh Kumar wrote:
> On 22-10-25, 08:19, Alice Ryhl wrote:
> > I'm guessing this means you want me to take it through drm-rust? See
> > what I put in the cover letter about choice of tree.
> 
> I was expecting Stephen to pick it up directly.

Ah, that's fine, thanks.

Alice

Re: [PATCH v2 1/2] rust: clk: implement Send and Sync

[Danilo Krummrich]

On 10/20/25 11:35 AM, Alice Ryhl wrote:
> These traits are required for drivers to embed the Clk type in their own
> data structures because driver data structures are usually required to
> be Send. Since the Clk type is thread-safe, implement the relevant
> traits.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Danilo Krummrich <dakr@kernel.org>