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

["Onur Özkan" <work@onurozkan.dev>]

On Tue, 9 Sep 2025 15:25:55 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

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


If you look at the changes in rust/kernel/error.rs, you will see that
the `to_result` function has been modified and some of its callers were
updated because the function signature changed.

The regulator patch I sent earlier is unrelated to this one. They are
not doing the same thing. That patch aimed to make use of `to_result`
instead of handling errors manually. This patch on the other hand,
changes how `to_result` itself works and some of its callers need to be
updated accordingly just like in the other modules included here. The
reason for changing the `to_result` function is explained in the commit
description.

-Onur 


> 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