Re: [PATCH v2 1/1] rust: refactor to_result to return the original value

[Daniel Almeida <daniel.almeida@collabora.com>]

Onur,

> On 9 Sep 2025, at 14:43, Onur Özkan <work@onurozkan.dev> wrote:
> 
> On Tue, 9 Sep 2025 19:17:56 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
>> On Tue, Sep 9, 2025 at 7:01 PM Onur Özkan <work@onurozkan.dev> wrote:
>>> 
>>> This patch only fixes the callers that broke after the changes on
>>> `to_result`. I haven't included all the improvements made possible
>>> by the new design since
>> 
>> I think Daniel asked in the previous version what you mean by "callers
>> that broke" here -- it is a bit confusing, since it seems this is a
>> fix (and thus needs to be prioritized).
>> 
>> Is that the case?
>> 
>> Thanks!
>> 
>> Cheers,
>> Miguel
> 
> What I meant is that the change on `to_result` signature introduced a
> breaking change so I had to update its callers accordingly.
> 
> The fix I mentioned in this version is a different matter.
> 
> Before the rebase, the regulator module had a get_voltage function like
> this:
> 
> let voltage = unsafe {...};
> 
> if voltage < 0 {
>     Err(...)
> } else {
>     Ok(Voltage::from_microvolts(voltage))
> }
> 
> But on the regulator/for-next branch, a patch was applied that changed
> it to:
> 
> let voltage = unsafe {...};
> to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
> 
> That change was incompatible with v1 (due to the different signature of
> to_result), which fails to build with my patch. This version (v2)
> fixes the issue introduced in v1.

Fixes what issue? What is the actual problem being addressed here?

It looks like a mere change from

to_result(…) and,
to_result(…).map()

To:

to_result(…)?;
Ok(())

and

-        let voltage = unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) };
-
-        to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
+        to_result(unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) })
+            .map(Voltage::from_microvolts)
    }


> 
> Sorry for the confusion, I hope it's more clear now.
> 
> Thanks,
> Onur
> 

Your last regulator patch was minor, correct, and was picked up (merged). It
cleared up an if/else, so that was an improvement.

I now see yet another change, doing apparently the same thing (correct me if
I’m wrong) in a slightly different way, in a patch that now has your previous regulator
patch as a dependency.

So my question is, why do we need this?

diff --git a/rust/kernel/regulator.rsb/rust/kernel/regulator.rs
index 34bb24ec8d4d..a5f357bda6e9 100644
--- a/rust/kernel/regulator.rs
+++ b/rust/kernel/regulator.rs
@@ -260,15 +260,15 @@ pub fn set_voltage(&self, min_voltage: Voltage, max_voltage: Voltage) -> Result
                min_voltage.as_microvolts(),
                max_voltage.as_microvolts(),
            )
-        })
+        })?;
+        Ok(())
    }

    /// Gets the current voltage of the regulator.
    pub fn get_voltage(&self) -> Result<Voltage> {
        // SAFETY: Safe as per the type invariants of `Regulator`.
-        let voltage = unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) };
-
-        to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
+        to_result(unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) })
+            .map(Voltage::from_microvolts)
    }

    fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
@@ -288,12 +288,14 @@ fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {

    fn enable_internal(&self) -> Result {
        // SAFETY: Safe as per the type invariants of `Regulator`.
-        to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) })
+        to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) })?;
+        Ok(())
    }

    fn disable_internal(&self) -> Result {
        // SAFETY: Safe as per the type invariants of `Regulator`.
-        to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) })
+        to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) })?;
+        Ok(())
    }


— Daniel