Re: [PATCH V12 13/15] rust: cpufreq: Extend abstractions for driver registration

[Viresh Kumar <viresh.kumar@linaro.org>]

On 19-05-25, 13:06, Danilo Krummrich wrote:
> Sorry, I didn't catch the above in my previous review -- fine for me if you do
> those improvements in a subsequent patch.

That's fine. Thanks a lot for reviewing the series.

-- 
viresh

diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index d0e60b7db81f..94ed81644fe1 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -152,30 +152,30 @@ fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
         )?)
     }
 
-    fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result<()> {
+    fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result {
         Ok(())
     }
 
-    fn online(_policy: &mut cpufreq::Policy) -> Result<()> {
+    fn online(_policy: &mut cpufreq::Policy) -> Result {
         // We did light-weight tear down earlier, nothing to do here.
         Ok(())
     }
 
-    fn offline(_policy: &mut cpufreq::Policy) -> Result<()> {
+    fn offline(_policy: &mut cpufreq::Policy) -> Result {
         // Preserve policy->data and don't free resources on light-weight
         // tear down.
         Ok(())
     }
 
-    fn suspend(policy: &mut cpufreq::Policy) -> Result<()> {
+    fn suspend(policy: &mut cpufreq::Policy) -> Result {
         policy.generic_suspend()
     }
 
-    fn verify(data: &mut cpufreq::PolicyData) -> Result<()> {
+    fn verify(data: &mut cpufreq::PolicyData) -> Result {
         data.generic_verify()
     }
 
-    fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result<()> {
+    fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result {
         let Some(data) = policy.data::<Self::PData>() else {
             return Err(ENOENT);
         };
@@ -188,7 +188,7 @@ fn get(policy: &mut cpufreq::Policy) -> Result<u32> {
         policy.generic_get()
     }
 
-    fn set_boost(_policy: &mut cpufreq::Policy, _state: i32) -> Result<()> {
+    fn set_boost(_policy: &mut cpufreq::Policy, _state: i32) -> Result {
         Ok(())
     }
 
@@ -213,10 +213,7 @@ fn probe(
         _id_info: Option<&Self::IdInfo>,
     ) -> Result<Pin<KBox<Self>>> {
         cpufreq::Registration::<CPUFreqDTDriver>::new_foreign_owned(pdev.as_ref())?;
-
-        let drvdata = KBox::new(Self {}, GFP_KERNEL)?;
-
-        Ok(drvdata.into())
+        Ok(KBox::new(Self {}, GFP_KERNEL)?.into())
     }
 }
 
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 826710c4f4b0..09b856bb297b 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -154,7 +154,7 @@ pub fn as_raw(&self) -> *mut bindings::cpufreq_policy_data {
 
     /// Wrapper for `cpufreq_generic_frequency_table_verify`.
     #[inline]
-    pub fn generic_verify(&self) -> Result<()> {
+    pub fn generic_verify(&self) -> Result {
         // SAFETY: By the type invariant, the pointer stored in `self` is valid.
         to_result(unsafe { bindings::cpufreq_generic_frequency_table_verify(self.as_raw()) })
     }
@@ -208,15 +208,16 @@ fn from(index: TableIndex) -> Self {
 /// ```
 /// use kernel::cpufreq::{Policy, TableIndex};
 ///
-/// fn show_freq(policy: &Policy) {
-///     let table = policy.freq_table().unwrap();
+/// fn show_freq(policy: &Policy) -> Result {
+///     let table = policy.freq_table()?;
 ///
 ///     // SAFETY: Index is a valid entry in the table.
 ///     let index = unsafe { TableIndex::new(0) };
 ///
-///     pr_info!("The frequency at index 0 is: {:?}\n", table.freq(index).unwrap());
+///     pr_info!("The frequency at index 0 is: {:?}\n", table.freq(index)?);
 ///     pr_info!("The flags at index 0 is: {}\n", table.flags(index));
 ///     pr_info!("The data at index 0 is: {}\n", table.data(index));
+///     Ok(())
 /// }
 /// ```
 #[repr(transparent)]
@@ -361,7 +362,7 @@ pub fn new() -> Self {
     }
 
     /// Adds a new entry to the table.
-    pub fn add(&mut self, freq: Hertz, flags: u32, driver_data: u32) -> Result<()> {
+    pub fn add(&mut self, freq: Hertz, flags: u32, driver_data: u32) -> Result {
         // Adds the new entry at the end of the vector.
         Ok(self.entries.push(
             bindings::cpufreq_frequency_table {
@@ -515,7 +516,7 @@ pub fn set_suspend_freq(&mut self, freq: Hertz) -> &mut Self {
 
     /// Provides a wrapper to the generic suspend routine.
     #[inline]
-    pub fn generic_suspend(&mut self) -> Result<()> {
+    pub fn generic_suspend(&mut self) -> Result {
         // SAFETY: By the type invariant, the pointer stored in `self` is valid.
         to_result(unsafe { bindings::cpufreq_generic_suspend(self.as_mut_ref()) })
     }
@@ -643,7 +644,7 @@ pub fn data<T: ForeignOwnable>(&mut self) -> Option<<T>::Borrowed<'_>> {
     /// # Errors
     ///
     /// Returns `EBUSY` if private data is already set.
-    fn set_data<T: ForeignOwnable>(&mut self, data: T) -> Result<()> {
+    fn set_data<T: ForeignOwnable>(&mut self, data: T) -> Result {
         if self.as_ref().driver_data.is_null() {
             // Transfer the ownership of the data to the foreign interface.
             self.as_mut_ref().driver_data = <T as ForeignOwnable>::into_foreign(data) as _;
@@ -736,27 +737,27 @@ pub trait Driver {
     fn init(policy: &mut Policy) -> Result<Self::PData>;
 
     /// Driver's `exit` callback.
-    fn exit(_policy: &mut Policy, _data: Option<Self::PData>) -> Result<()> {
+    fn exit(_policy: &mut Policy, _data: Option<Self::PData>) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Driver's `online` callback.
-    fn online(_policy: &mut Policy) -> Result<()> {
+    fn online(_policy: &mut Policy) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Driver's `offline` callback.
-    fn offline(_policy: &mut Policy) -> Result<()> {
+    fn offline(_policy: &mut Policy) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Driver's `suspend` callback.
-    fn suspend(_policy: &mut Policy) -> Result<()> {
+    fn suspend(_policy: &mut Policy) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Driver's `resume` callback.
-    fn resume(_policy: &mut Policy) -> Result<()> {
+    fn resume(_policy: &mut Policy) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
@@ -766,20 +767,20 @@ fn ready(_policy: &mut Policy) {
     }
 
     /// Driver's `verify` callback.
-    fn verify(data: &mut PolicyData) -> Result<()>;
+    fn verify(data: &mut PolicyData) -> Result;
 
     /// Driver's `setpolicy` callback.
-    fn setpolicy(_policy: &mut Policy) -> Result<()> {
+    fn setpolicy(_policy: &mut Policy) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Driver's `target` callback.
-    fn target(_policy: &mut Policy, _target_freq: u32, _relation: Relation) -> Result<()> {
+    fn target(_policy: &mut Policy, _target_freq: u32, _relation: Relation) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Driver's `target_index` callback.
-    fn target_index(_policy: &mut Policy, _index: TableIndex) -> Result<()> {
+    fn target_index(_policy: &mut Policy, _index: TableIndex) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
@@ -799,7 +800,7 @@ fn get_intermediate(_policy: &mut Policy, _index: TableIndex) -> u32 {
     }
 
     /// Driver's `target_intermediate` callback.
-    fn target_intermediate(_policy: &mut Policy, _index: TableIndex) -> Result<()> {
+    fn target_intermediate(_policy: &mut Policy, _index: TableIndex) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
@@ -814,12 +815,12 @@ fn update_limits(_policy: &mut Policy) {
     }
 
     /// Driver's `bios_limit` callback.
-    fn bios_limit(_policy: &mut Policy, _limit: &mut u32) -> Result<()> {
+    fn bios_limit(_policy: &mut Policy, _limit: &mut u32) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Driver's `set_boost` callback.
-    fn set_boost(_policy: &mut Policy, _state: i32) -> Result<()> {
+    fn set_boost(_policy: &mut Policy, _state: i32) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
@@ -837,43 +838,44 @@ fn register_em(_policy: &mut Policy) {
 ///
 /// ```
 /// use kernel::{
-///     cpu, cpufreq,
+///     cpufreq,
 ///     c_str,
-///     device::{Bound, Device},
+///     device::{Core, Device},
 ///     macros::vtable,
+///     of, platform,
 ///     sync::Arc,
 /// };
-/// struct FooDevice;
+/// struct SampleDevice;
 ///
 /// #[derive(Default)]
-/// struct FooDriver;
+/// struct SampleDriver;
 ///
 /// #[vtable]
-/// impl cpufreq::Driver for FooDriver {
-///     const NAME: &'static CStr = c_str!("cpufreq-foo");
+/// impl cpufreq::Driver for SampleDriver {
+///     const NAME: &'static CStr = c_str!("cpufreq-sample");
 ///     const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV;
 ///     const BOOST_ENABLED: bool = true;
 ///
-///     type PData = Arc<FooDevice>;
+///     type PData = Arc<SampleDevice>;
 ///
 ///     fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
 ///         // Initialize here
-///         Ok(Arc::new(FooDevice, GFP_KERNEL)?)
+///         Ok(Arc::new(SampleDevice, GFP_KERNEL)?)
 ///     }
 ///
-///     fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result<()> {
+///     fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result {
 ///         Ok(())
 ///     }
 ///
-///     fn suspend(policy: &mut cpufreq::Policy) -> Result<()> {
+///     fn suspend(policy: &mut cpufreq::Policy) -> Result {
 ///         policy.generic_suspend()
 ///     }
 ///
-///     fn verify(data: &mut cpufreq::PolicyData) -> Result<()> {
+///     fn verify(data: &mut cpufreq::PolicyData) -> Result {
 ///         data.generic_verify()
 ///     }
 ///
-///     fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result<()> {
+///     fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result {
 ///         // Update CPU frequency
 ///         Ok(())
 ///     }
@@ -883,8 +885,17 @@ fn register_em(_policy: &mut Policy) {
 ///     }
 /// }
 ///
-/// fn foo_probe(dev: &Device<Bound>) {
-///     cpufreq::Registration::<FooDriver>::new_foreign_owned(dev).unwrap();
+/// impl platform::Driver for SampleDriver {
+///     type IdInfo = ();
+///     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
+///
+///     fn probe(
+///         pdev: &platform::Device<Core>,
+///         _id_info: Option<&Self::IdInfo>,
+///     ) -> Result<Pin<KBox<Self>>> {
+///         cpufreq::Registration::<SampleDriver>::new_foreign_owned(pdev.as_ref())?;
+///         Ok(KBox::new(Self {}, GFP_KERNEL)?.into())
+///     }
 /// }
 /// ```
 #[repr(transparent)]
@@ -1035,7 +1046,7 @@ pub fn new() -> Result<Self> {
     ///
     /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
     /// device is detached.
-    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result<()> {
+    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
         Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
     }
 }
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index 1e5fd9887b3a..212555dacd45 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -292,7 +292,7 @@ pub enum SearchType {
 pub trait ConfigOps {
     /// This is typically used to scale clocks when transitioning between OPPs.
     #[inline]
-    fn config_clks(_dev: &Device, _table: &Table, _opp: &OPP, _scaling_down: bool) -> Result<()> {
+    fn config_clks(_dev: &Device, _table: &Table, _opp: &OPP, _scaling_down: bool) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 
@@ -304,7 +304,7 @@ fn config_regulators(
         _opp_new: &OPP,
         _data: *mut *mut bindings::regulator,
         _count: u32,
-    ) -> Result<()> {
+    ) -> Result {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 }
@@ -753,7 +753,7 @@ pub fn suspend_freq(&self) -> Hertz {
 
     /// Synchronizes regulators used by the [`Table`].
     #[inline]
-    pub fn sync_regulators(&self) -> Result<()> {
+    pub fn sync_regulators(&self) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe { bindings::dev_pm_opp_sync_regulators(self.dev.as_raw()) })
@@ -761,14 +761,14 @@ pub fn sync_regulators(&self) -> Result<()> {
 
     /// Gets sharing CPUs.
     #[inline]
-    pub fn sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result<()> {
+    pub fn sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe { bindings::dev_pm_opp_get_sharing_cpus(dev.as_raw(), cpumask.as_raw()) })
     }
 
     /// Sets sharing CPUs.
-    pub fn set_sharing_cpus(&mut self, cpumask: &mut Cpumask) -> Result<()> {
+    pub fn set_sharing_cpus(&mut self, cpumask: &mut Cpumask) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe {
@@ -786,7 +786,7 @@ pub fn set_sharing_cpus(&mut self, cpumask: &mut Cpumask) -> Result<()> {
     /// Gets sharing CPUs from device tree.
     #[cfg(CONFIG_OF)]
     #[inline]
-    pub fn of_sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result<()> {
+    pub fn of_sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe {
@@ -802,7 +802,7 @@ pub fn adjust_voltage(
         volt: MicroVolt,
         volt_min: MicroVolt,
         volt_max: MicroVolt,
-    ) -> Result<()> {
+    ) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe {
@@ -825,7 +825,7 @@ pub fn cpufreq_table(&mut self) -> Result<FreqTable> {
 
     /// Configures device with [`OPP`] matching the frequency value.
     #[inline]
-    pub fn set_rate(&self, freq: Hertz) -> Result<()> {
+    pub fn set_rate(&self, freq: Hertz) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe { bindings::dev_pm_opp_set_rate(self.dev.as_raw(), freq.into()) })
@@ -833,7 +833,7 @@ pub fn set_rate(&self, freq: Hertz) -> Result<()> {
 
     /// Configures device with [`OPP`].
     #[inline]
-    pub fn set_opp(&self, opp: &OPP) -> Result<()> {
+    pub fn set_opp(&self, opp: &OPP) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe { bindings::dev_pm_opp_set_opp(self.dev.as_raw(), opp.as_raw()) })
@@ -937,7 +937,7 @@ pub fn opp_from_bw(&self, mut bw: u32, index: i32, stype: SearchType) -> Result<
 
     /// Enables the [`OPP`].
     #[inline]
-    pub fn enable_opp(&self, freq: Hertz) -> Result<()> {
+    pub fn enable_opp(&self, freq: Hertz) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe { bindings::dev_pm_opp_enable(self.dev.as_raw(), freq.into()) })
@@ -945,7 +945,7 @@ pub fn enable_opp(&self, freq: Hertz) -> Result<()> {
 
     /// Disables the [`OPP`].
     #[inline]
-    pub fn disable_opp(&self, freq: Hertz) -> Result<()> {
+    pub fn disable_opp(&self, freq: Hertz) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe { bindings::dev_pm_opp_disable(self.dev.as_raw(), freq.into()) })
@@ -953,7 +953,7 @@ pub fn disable_opp(&self, freq: Hertz) -> Result<()> {
 
     /// Registers with the Energy model.
     #[cfg(CONFIG_OF)]
-    pub fn of_register_em(&mut self, cpumask: &mut Cpumask) -> Result<()> {
+    pub fn of_register_em(&mut self, cpumask: &mut Cpumask) -> Result {
         // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
         // requirements.
         to_result(unsafe {
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 27cc72d474f0..6ff34096d7ee 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -185,9 +185,9 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
 
     let info = ModuleInfo::parse(&mut it);
 
-    /* Rust does not allow hyphens in identifiers, use underscore instead */
-    let name_identifier = info.name.replace('-', "_");
-    let mut modinfo = ModInfoBuilder::new(name_identifier.as_ref());
+    // Rust does not allow hyphens in identifiers, use underscore instead.
+    let ident = info.name.replace('-', "_");
+    let mut modinfo = ModInfoBuilder::new(ident.as_ref());
     if let Some(author) = info.author {
         modinfo.emit("author", &author);
     }
@@ -312,15 +312,15 @@ mod __module_init {{
                     #[doc(hidden)]
                     #[link_section = \"{initcall_section}\"]
                     #[used]
-                    pub static __{name_identifier}_initcall: extern \"C\" fn() ->
-                        kernel::ffi::c_int = __{name_identifier}_init;
+                    pub static __{ident}_initcall: extern \"C\" fn() ->
+                        kernel::ffi::c_int = __{ident}_init;
 
                     #[cfg(not(MODULE))]
                     #[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)]
                     core::arch::global_asm!(
                         r#\".section \"{initcall_section}\", \"a\"
-                        __{name_identifier}_initcall:
-                            .long   __{name_identifier}_init - .
+                        __{ident}_initcall:
+                            .long   __{ident}_init - .
                             .previous
                         \"#
                     );
@@ -328,7 +328,7 @@ mod __module_init {{
                     #[cfg(not(MODULE))]
                     #[doc(hidden)]
                     #[no_mangle]
-                    pub extern \"C\" fn __{name_identifier}_init() -> kernel::ffi::c_int {{
+                    pub extern \"C\" fn __{ident}_init() -> kernel::ffi::c_int {{
                         // SAFETY: This function is inaccessible to the outside due to the double
                         // module wrapping it. It is called exactly once by the C side via its
                         // placement above in the initcall section.
@@ -338,12 +338,12 @@ mod __module_init {{
                     #[cfg(not(MODULE))]
                     #[doc(hidden)]
                     #[no_mangle]
-                    pub extern \"C\" fn __{name_identifier}_exit() {{
+                    pub extern \"C\" fn __{ident}_exit() {{
                         // SAFETY:
                         // - This function is inaccessible to the outside due to the double
                         //   module wrapping it. It is called exactly once by the C side via its
                         //   unique name,
-                        // - furthermore it is only called after `__{name_identifier}_init` has
+                        // - furthermore it is only called after `__{ident}_init` has
                         //   returned `0` (which delegates to `__init`).
                         unsafe {{ __exit() }}
                     }}
@@ -384,7 +384,7 @@ unsafe fn __exit() {{
         ",
         type_ = info.type_,
         name = info.name,
-        name_identifier = name_identifier,
+        ident = ident,
         modinfo = modinfo.buffer,
         initcall_section = ".initcall6.init"
     )