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

[Daniel Almeida]

Hi Viresh,

> On 4 Mar 2025, at 05:53, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> 
> 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?
> 
> I have tried some improvements based on your (and Alice's comments), please see
> if it looks any better now.
> 
> -- 
> viresh
> 
> -------------------------8<-------------------------
> 
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> new file mode 100644
> index 000000000000..fc3cb0f5f332
> --- /dev/null
> +++ b/rust/kernel/clk.rs
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Clock abstractions.
> +//!
> +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
> +//!
> +//! Reference: <https://docs.kernel.org/driver-api/clk.html>
> +
> +use crate::{
> +    bindings,
> +    device::Device,
> +    error::{from_err_ptr, to_result, Result},
> +    ffi::c_ulong,
> +    prelude::*,
> +};
> +
> +use core::{ops::Deref, ptr};
> +
> +/// Frequency unit.
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +pub struct Hertz(c_ulong);

Maybe make self.0 pub too?

> +
> +impl Hertz {
> +    /// Creates a new `Hertz` value.
> +    pub fn new(freq: c_ulong) -> Self {
> +        Hertz(freq)

I don’t think we need a `new` function.  IMHO, the only thing that matters is
that the name Hertz shows up in the calling code, i.e.:

```
fn foo() {
    let clk = …;
    let some_val = …;
    clk.set_rate(Hertz(some_val)); // Ok: crystal clear this is Hertz
}
```

A  impl From<Hertz> for c_ulong would also be helpful, so that we don’t have to
manually define all the arithmetic operations on this.

```
fn foo() {
    let clk = …;
    let double = u32::from(clk.rate()) * 2;
    clk.set_rate(Hertz(double)); // Ok: crystal clear this is Hertz
}
```

I need more time to look at the rest of the patch, so feel free to carry on with the
feedback from others. Sorry for the delay!

— Daniel