[PATCH] Various improvements on clock abstractions

[PATCH] Various improvements on clock abstractions

[onur-ozkan]

A few changes to improve the clock abstractions and make them a little
more idiomatic:

1. `impl Hertz` functions are now constant and compile-time evaluable.
2. `Hertz` conversions are now done with constant variables, which should
    make them more readable.
3. `con_id` is handled in a single line using `map_or` instead of using
    nested if-else blocks.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
---
 rust/kernel/clk.rs | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index 6041c6d07527..fbcea31dbcca 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -30,39 +30,43 @@
 pub struct Hertz(pub c_ulong);
 
 impl Hertz {
+    const KHZ_TO_HZ: c_ulong = 1_000;
+    const MHZ_TO_HZ: c_ulong = 1_000_000;
+    const GHZ_TO_HZ: c_ulong = 1_000_000_000;
+
     /// Create a new instance from kilohertz (kHz)
-    pub fn from_khz(khz: c_ulong) -> Self {
-        Self(khz * 1_000)
+    pub const fn from_khz(khz: c_ulong) -> Self {
+        Self(khz * Self::KHZ_TO_HZ)
     }
 
     /// Create a new instance from megahertz (MHz)
-    pub fn from_mhz(mhz: c_ulong) -> Self {
-        Self(mhz * 1_000_000)
+    pub const fn from_mhz(mhz: c_ulong) -> Self {
+        Self(mhz * Self::MHZ_TO_HZ)
     }
 
     /// Create a new instance from gigahertz (GHz)
-    pub fn from_ghz(ghz: c_ulong) -> Self {
-        Self(ghz * 1_000_000_000)
+    pub const fn from_ghz(ghz: c_ulong) -> Self {
+        Self(ghz * Self::GHZ_TO_HZ)
     }
 
     /// Get the frequency in hertz
-    pub fn as_hz(&self) -> c_ulong {
+    pub const fn as_hz(&self) -> c_ulong {
         self.0
     }
 
     /// Get the frequency in kilohertz
-    pub fn as_khz(&self) -> c_ulong {
-        self.0 / 1_000
+    pub const fn as_khz(&self) -> c_ulong {
+        self.0 / Self::KHZ_TO_HZ
     }
 
     /// Get the frequency in megahertz
-    pub fn as_mhz(&self) -> c_ulong {
-        self.0 / 1_000_000
+    pub const fn as_mhz(&self) -> c_ulong {
+        self.0 / Self::MHZ_TO_HZ
     }
 
     /// Get the frequency in gigahertz
-    pub fn as_ghz(&self) -> c_ulong {
-        self.0 / 1_000_000_000
+    pub const fn as_ghz(&self) -> c_ulong {
+        self.0 / Self::GHZ_TO_HZ
     }
 }
 
@@ -132,11 +136,7 @@ impl Clk {
         ///
         /// [`clk_get`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_get
         pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
-            let con_id = if let Some(name) = name {
-                name.as_ptr()
-            } else {
-                ptr::null()
-            };
+            let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
 
             // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
             //
@@ -304,11 +304,7 @@ impl OptionalClk {
         /// [`clk_get_optional`]:
         /// https://docs.kernel.org/core-api/kernel-api.html#c.clk_get_optional
         pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
-            let con_id = if let Some(name) = name {
-                name.as_ptr()
-            } else {
-                ptr::null()
-            };
+            let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
 
             // SAFETY: It is safe to call [`clk_get_optional`] for a valid device pointer.
             //
-- 
2.49.0

Re: [PATCH] Various improvements on clock abstractions

[Miguel Ojeda]

On Mon, Jun 16, 2025 at 10:01 PM onur-ozkan <work@onurozkan.dev> wrote:
>
> Signed-off-by: onur-ozkan <work@onurozkan.dev>

Should this be "Onur Özkan"? Apologies if not -- I am just asking
because the SoB typically uses the full name as usually written, and I
saw it in Zulip spelled differently.

Cc'ing Viresh, since he introduced the file.

On the changes themselves, the `const fn` one seems obvious.

The constants one is OK, though we could perhaps have a `units.h`
equivalent eventually.

The `map_or` is up to style, though I am not sure why you say "nested
if-else blocks" in the commit message, i.e. what is nested?

(By the way, this could (or not) have been split into different
patches -- it is up to the maintainers/reviewers, and it depends on
how complex a given patch is and other factors, but I thought I would
mention it just in case you thought it needed to be one.)

Thanks!

Cheers,
Miguel

Re: [PATCH] Various improvements on clock abstractions

[Miguel Ojeda]

On Tue, Jun 17, 2025 at 6:28 AM Onur Özkan <work@onurozkan.dev> wrote:
>
> Yes, it should be "Onur Özkan", sorry. Should I update that part and re-send the patch?

I would suggest to wait for other feedback, and then you can send a v2
if needed.

> where my patch converts this into a single straight line which I think makes it more idiomatic.

Up to the maintainers :) So far we have both styles around.

Thanks!

Cheers,
Miguel

Re: [PATCH] Various improvements on clock abstractions

[Viresh Kumar]

On 17-06-25, 08:55, Miguel Ojeda wrote:
> On Tue, Jun 17, 2025 at 6:28 AM Onur Özkan <work@onurozkan.dev> wrote:
> >
> > Yes, it should be "Onur Özkan", sorry. Should I update that part and re-send the patch?
> 
> I would suggest to wait for other feedback, and then you can send a v2
> if needed.
> 
> > where my patch converts this into a single straight line which I think makes it more idiomatic.
> 
> Up to the maintainers :) So far we have both styles around.

I am okay with all the changes, the commit log can be improved as you mentioned
earlier.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH] Various improvements on clock abstractions

[Onur]

On Thu, 19 Jun 2025 12:15:34 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 17-06-25, 08:55, Miguel Ojeda wrote:
> > On Tue, Jun 17, 2025 at 6:28 AM Onur Özkan <work@onurozkan.dev>
> > wrote:
> > >
> > > Yes, it should be "Onur Özkan", sorry. Should I update that part
> > > and re-send the patch?
> > 
> > I would suggest to wait for other feedback, and then you can send a
> > v2 if needed.
> > 
> > > where my patch converts this into a single straight line which I
> > > think makes it more idiomatic.
> > 
> > Up to the maintainers :) So far we have both styles around.
> 
> I am okay with all the changes, the commit log can be improved as you
> mentioned earlier.
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 

FWIW I split this patch into 3 parts and sent them separately as it
was suggested earlier by Alexandre.

Here are the patches I sent yesterday:

- https://lore.kernel.org/all/20250618092810.29370-1-work@onurozkan.dev/
- https://lore.kernel.org/all/20250618093508.16343-1-work@onurozkan.dev/
- https://lore.kernel.org/all/20250618091442.29104-1-work@onurozkan.dev/


Regards,
Onur

Re: [PATCH] Various improvements on clock abstractions

[Alexandre Courbot]

On Thu Jun 19, 2025 at 7:05 PM JST, Onur wrote:
> On Thu, 19 Jun 2025 12:15:34 +0530
> Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
>> On 17-06-25, 08:55, Miguel Ojeda wrote:
>> > On Tue, Jun 17, 2025 at 6:28 AM Onur Özkan <work@onurozkan.dev>
>> > wrote:
>> > >
>> > > Yes, it should be "Onur Özkan", sorry. Should I update that part
>> > > and re-send the patch?
>> > 
>> > I would suggest to wait for other feedback, and then you can send a
>> > v2 if needed.
>> > 
>> > > where my patch converts this into a single straight line which I
>> > > think makes it more idiomatic.
>> > 
>> > Up to the maintainers :) So far we have both styles around.
>> 
>> I am okay with all the changes, the commit log can be improved as you
>> mentioned earlier.
>> 
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> 
>
> FWIW I split this patch into 3 parts and sent them separately as it
> was suggested earlier by Alexandre.
>
> Here are the patches I sent yesterday:
>
> - https://lore.kernel.org/all/20250618092810.29370-1-work@onurozkan.dev/
> - https://lore.kernel.org/all/20250618093508.16343-1-work@onurozkan.dev/
> - https://lore.kernel.org/all/20250618091442.29104-1-work@onurozkan.dev/

These look good! Small nit for next time: since they are closely
related, please send them as a series to make review (and merging, as
they will conflict if not applied in the right order) easier.

Re: [PATCH] Various improvements on clock abstractions

[Alexandre Courbot]

On Tue Jun 17, 2025 at 5:01 AM JST, onur-ozkan wrote:
> A few changes to improve the clock abstractions and make them a little
> more idiomatic:
>
> 1. `impl Hertz` functions are now constant and compile-time evaluable.
> 2. `Hertz` conversions are now done with constant variables, which should

"constant variable" is an oxymoron. :) I think you just want to say
"constant" here.

>     make them more readable.
> 3. `con_id` is handled in a single line using `map_or` instead of using
>     nested if-else blocks.

Please split these 3 changes into 3 patches, I agree that they are
trivial but a patch should do a single thing. This makes review simpler
and allows to apply only part of the changes if e.g. one of them needs
further discussion or is rejected.

The changes in themselves look good though!

[PATCH] Various improvements on clock abstractions

[onur-ozkan]

A few changes to improve the clock abstractions and make them a little
more idiomatic:

1. `impl Hertz` functions are now constant and compile-time evaluable.
2. `Hertz` conversions are now done with constant variables, which should
    make them more readable.
3. `con_id` is handled in a single line using `map_or` instead of using
    nested if-else blocks.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
---
 rust/kernel/clk.rs | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
index 6041c6d07527..fbcea31dbcca 100644
--- a/rust/kernel/clk.rs
+++ b/rust/kernel/clk.rs
@@ -30,39 +30,43 @@
 pub struct Hertz(pub c_ulong);
 
 impl Hertz {
+    const KHZ_TO_HZ: c_ulong = 1_000;
+    const MHZ_TO_HZ: c_ulong = 1_000_000;
+    const GHZ_TO_HZ: c_ulong = 1_000_000_000;
+
     /// Create a new instance from kilohertz (kHz)
-    pub fn from_khz(khz: c_ulong) -> Self {
-        Self(khz * 1_000)
+    pub const fn from_khz(khz: c_ulong) -> Self {
+        Self(khz * Self::KHZ_TO_HZ)
     }
 
     /// Create a new instance from megahertz (MHz)
-    pub fn from_mhz(mhz: c_ulong) -> Self {
-        Self(mhz * 1_000_000)
+    pub const fn from_mhz(mhz: c_ulong) -> Self {
+        Self(mhz * Self::MHZ_TO_HZ)
     }
 
     /// Create a new instance from gigahertz (GHz)
-    pub fn from_ghz(ghz: c_ulong) -> Self {
-        Self(ghz * 1_000_000_000)
+    pub const fn from_ghz(ghz: c_ulong) -> Self {
+        Self(ghz * Self::GHZ_TO_HZ)
     }
 
     /// Get the frequency in hertz
-    pub fn as_hz(&self) -> c_ulong {
+    pub const fn as_hz(&self) -> c_ulong {
         self.0
     }
 
     /// Get the frequency in kilohertz
-    pub fn as_khz(&self) -> c_ulong {
-        self.0 / 1_000
+    pub const fn as_khz(&self) -> c_ulong {
+        self.0 / Self::KHZ_TO_HZ
     }
 
     /// Get the frequency in megahertz
-    pub fn as_mhz(&self) -> c_ulong {
-        self.0 / 1_000_000
+    pub const fn as_mhz(&self) -> c_ulong {
+        self.0 / Self::MHZ_TO_HZ
     }
 
     /// Get the frequency in gigahertz
-    pub fn as_ghz(&self) -> c_ulong {
-        self.0 / 1_000_000_000
+    pub const fn as_ghz(&self) -> c_ulong {
+        self.0 / Self::GHZ_TO_HZ
     }
 }
 
@@ -132,11 +136,7 @@ impl Clk {
         ///
         /// [`clk_get`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_get
         pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
-            let con_id = if let Some(name) = name {
-                name.as_ptr()
-            } else {
-                ptr::null()
-            };
+            let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
 
             // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
             //
@@ -304,11 +304,7 @@ impl OptionalClk {
         /// [`clk_get_optional`]:
         /// https://docs.kernel.org/core-api/kernel-api.html#c.clk_get_optional
         pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
-            let con_id = if let Some(name) = name {
-                name.as_ptr()
-            } else {
-                ptr::null()
-            };
+            let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
 
             // SAFETY: It is safe to call [`clk_get_optional`] for a valid device pointer.
             //
-- 
2.49.0