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

[Viresh Kumar]

On 03-03-25, 11:16, Miguel Ojeda wrote:
> 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?

Actually Daneil did suggest to make this "Struct Hertz(c_ulong)", but then I
looked at rust/kernel/time.rs:

pub type Jiffies = crate::ffi::c_ulong;

And I thought this is probably what everyone would have agreed to and did it
this way.

> > +    /// 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?

Something like this (from print.rs) ?

/// [`pr_debug`]: https://docs.kernel.org/core-api/printk-basics.html#c.pr_debug

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

Yes, I was using this under `cfg` earlier, but removed that recently after
testing this without CONFIG_HAVE_CLK. clk.h provides wrappers for cases where
the config option isn't available.

-- 
viresh