Message-Id: <DBP8EWLCAE4B.34Y4FBSH5BTB6@kernel.org>
Date: Wed, 30 Jul 2025 10:03:48 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "Michael Turquette" <mturquette@...libre.com>, "Stephen Boyd"
 <sboyd@...nel.org>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Rafael J. Wysocki"
 <rafael@...nel.org>, "Viresh Kumar" <viresh.kumar@...aro.org>, "Alexandre
 Courbot" <acourbot@...dia.com>, <linux-clk@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-pm@...r.kernel.org>
Subject: Re: [PATCH] rust: clk: use the type-state pattern

On Tue Jul 29, 2025 at 11:38 PM CEST, Daniel Almeida wrote:
> 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.

That's a great improvement, thanks! Some questions / comments below.

>      /// A reference-counted clock.
>      ///
>      /// Rust abstraction for the C [`struct clk`].
>      ///
> +    /// A [`Clk`] instance represents a clock that can be in one of several
> +    /// states: [`Unprepared`], [`Prepared`], or [`Enabled`].
> +    ///
> +    /// No action needs to be taken when a [`Clk`] is dropped. The calls to
> +    /// `clk_unprepare()` and `clk_disable()` will be placed as applicable.
> +    ///
> +    /// An optional [`Clk`] is treated just like a regular [`Clk`], but its
> +    /// inner `struct clk` pointer is `NULL`. This interfaces correctly with the
> +    /// C API and also exposes all the methods of a regular [`Clk`] to users.
> +    ///
>      /// # Invariants
>      ///
>      /// A [`Clk`] instance holds either a pointer to a valid [`struct clk`] created by the C
> @@ -99,20 +160,39 @@ mod common_clk {
>      /// Instances of this type are reference-counted. Calling [`Clk::get`] ensures that the
>      /// allocation remains valid for the lifetime of the [`Clk`].
>      ///
> -    /// ## Examples
> +    /// 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 [`Enabled`] 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")))?;

Given that this is a driver API, why do we allow obtaining and configuring
clocks of any device, i.e. also unbound devices?

I think Clk::<T>::get() should take a &Device<Bound> instead.

> -    ///     clk.prepare_enable()?;
> +    ///     // Any other state is also possible, e.g.:
> +    ///     let clk: Clk<Prepared> = Clk::<Prepared>::get(dev, Some(c_str!("apb_clk")))?;
> +    ///
> +    ///     // Later:
> +    ///     let clk: Clk<Enabled> = clk.enable().map_err(|error| {
> +    ///         error.error
> +    ///     })?;
> +    ///
> +    ///     // Note that error.clk is the original `clk` if the operation
> +    ///     // failed. It is provided as a convenience so that the operation may be
> +    ///     // retried in case of errors.
>      ///
>      ///     let expected_rate = Hertz::from_ghz(1);
>      ///
> @@ -120,104 +200,172 @@ 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(dev: &Device, clk: Clk<Enabled>) -> Result {

You don't need the dev argument here.

> +    ///     // The states can be traversed "in the reverse order" as well:
> +    ///     let clk: Clk<Prepared> = clk.disable().map_err(|error| {
> +    ///         error.error
> +    ///     })?;
> +    ///
> +    ///     let clk: Clk<Unprepared> = clk.unprepare();

I know you want to showcase the type state, yet I don't know if we should
explicitly declare the type if not necessary. People will likely just copy
things. Maybe a comment is better to emphasize it?

> +    ///
>      ///     Ok(())
>      /// }
>      /// ```
>      ///
>      /// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
>      #[repr(transparent)]
> -    pub struct Clk(*mut bindings::clk);
> +    pub struct Clk<T: ClkState> {
> +        inner: *mut bindings::clk,
> +        _phantom: core::marker::PhantomData<T>,
> +    }

<snip>

> +    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()) };
> +            }

Nice! I like this cleanup. However, don't you still need to call clk_put() to
drop the reference count?

Also, given that this is a device resource, don't we want to take it away from
drivers once the corresponding device has been unbound, i.e. use Devres?

>          }
>      }
>  }

Powered by blists - more mailing lists