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

[Miguel Ojeda]

On Mon, Mar 3, 2025 at 11:00 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> +/// Frequency unit.
> +pub type Hertz = crate::ffi::c_ulong;

Do we want this to be an alias or would it make sense to take the
chance to make this a newtype?

> +/// A simple implementation of `struct clk` from the C code.

In general, please try to provide some documentation and/or examples
where possible/reasonable.

> +    /// Gets clock corresponding to a device and a connection id and returns `Clk`.

Please use intra-doc links wherever they may work.

> +    /// Clock enable.

Should these be e.g. "Enable the clock." or similar?

Moreover, I see quite a lot of documentation about some of these
functions in the C side. I think we should not regress on that. Should
we link to the C docs, too?

> +    pub fn enable(&self) -> Result<()> {

Can this simply use `Result`?

> +pub mod clk;

Just to double check, do we need any `cfg`? I see some functions exist
even without e.g. `CONFIG_COMMON_CLK`, but I wanted to ask if you
tried to build it without it enabled.

Thanks!

Cheers,
Miguel