Re: [PATCH V2 2/2] rust: Add basic bindings for clk APIs

[Danilo Krummrich <dakr@kernel.org>]

On Fri, Feb 21, 2025 at 12:03:39PM +0530, Viresh Kumar wrote:
> +/// A simple implementation of `struct clk` from the C code.
> +#[repr(transparent)]
> +pub struct Clk(*mut bindings::clk);

I remember that Stephen explained that NULL is valid value for struct clk. As a
consequence, all functions implemented for `Clk` have to consider this.

I wonder if it could make sense to have a transparent wrapper type
`MaybeNull<T>` (analogous to `NonNull<T>`) to make this fact more obvious for
cases like this?

> +
> +impl Clk {
> +    /// Creates `Clk` instance for a device and a connection id.
> +    pub fn new(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()`, on a device pointer earlier received from the C
> +        // code.
> +        Ok(Self(from_err_ptr(unsafe {
> +            bindings::clk_get(dev.as_raw(), con_id)
> +        })?))
> +    }
> +
> +    /// Obtain the raw `struct clk *`.
> +    pub fn as_raw(&self) -> *mut bindings::clk {
> +        self.0
> +    }
> +
> +    /// Clock enable.
> +    pub fn enable(&self) -> Result<()> {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // use it now.

This is not true.

1. There is no type invariant documented for `Clk`.
2. The pointer contained in an instance of `Clk` may be NULL, hence `self` does
   not necessarily own a reference.

The same applies for all other functions in this implementation.