Re: [PATCH V3 2/2] rust: Add initial clk abstractions

[Danilo Krummrich <dakr@kernel.org>]

On Tue, Mar 04, 2025 at 02:23:51PM +0530, Viresh Kumar wrote:
> +/// This structure represents the Rust abstraction for a C [`struct clk`].
> +///
> +/// # Invariants
> +///
> +/// A [`Clk`] instance always corresponds to a valid [`struct clk`] created by the C portion of the
> +/// kernel.
> +///
> +/// Instances of this type are reference-counted. Calling `get` ensures that the allocation remains
> +/// valid for the lifetime of the [`Clk`].
> +///
> +/// ## Example
> +///
> +/// 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::error::Result;
> +///
> +/// fn configure_clk(dev: &Device) -> Result {
> +///     let clk = Clk::get(dev, "apb_clk")?;
> +///
> +///     clk.prepare_enable()?;
> +///
> +///     let expected_rate = Hertz::new(1_000_000_000);
> +///
> +///     if clk.rate() != expected_rate {
> +///         clk.set_rate(expected_rate)?;
> +///     }
> +///
> +///     clk.disable_unprepare();
> +///     Ok(())
> +/// }
> +/// ```
> +///
> +/// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
> +#[repr(transparent)]
> +pub struct Clk(*mut bindings::clk);
> +
> +impl Clk {
> +    /// Gets `Clk` corresponding to a [`Device`] and a connection id.
> +    pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
> +        let con_id = if let Some(name) = name {
> +            name.as_ptr() as *const _
> +        } else {
> +            ptr::null()
> +        };
> +
> +        // SAFETY: It is safe to call `clk_get()` for a valid device pointer.
> +        Ok(Self(from_err_ptr(unsafe {
> +            bindings::clk_get(dev.as_raw(), con_id)
> +        })?))
> +    }
> +
> +    /// Obtain the raw `struct clk *`.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::clk {
> +        self.0
> +    }
> +
> +    /// Enable the clock.
> +    #[inline]
> +    pub fn enable(&self) -> Result {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.

You may want to add an invariant for this, i.e. something along the lines of
"Clk always holds either a pointer to a valid struct clk or a NULL pointer".

In this safety comment you can then say that by the type invariant of Clk
self.as_raw() is a valid argument for $func.

Not that your type invariant needs the NULL case, since OptionalClk may set Clk
to hold a NULL pointer.

I still think that a new type MaybeNull<T> would be nice to encapsulate this
invariant, but we can also wait until we get another use-case for it.