Re: [PATCH v5 1/2] rust: regulator: add a bare minimum regulator abstraction

["Benno Lossin" <lossin@kernel.org>]

On Tue Jun 24, 2025 at 7:31 PM CEST, Daniel Almeida wrote:
> Hi,
>
>> +    /// Attempts to convert the regulator to an enabled state.
>> +    pub fn try_into_enabled(mut self) -> Result<Regulator<Enabled>, Error<Disabled>> {
>> +        self.enable_internal()
>> +            .map(|()| Regulator {
>> +                inner: self.inner,
>> +                _phantom: PhantomData,
>> +            })
>> +            .map_err(|error| Error {
>> +                error,
>> +                regulator: self,
>> +            })
>> +    }
>> +}
>> +
>> +impl Regulator<Enabled> {
>> +    /// Obtains a [`Regulator`] instance from the system and enables it.
>> +    ///
>> +    /// This is equivalent to calling `regulator_get_enable()` in the C API.
>> +    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
>> +        Regulator::<Disabled>::get_internal(dev, name)?
>> +            .try_into_enabled()
>> +            .map_err(|error| error.error)
>> +    }
>
> I just realized that this is a bug.
>
> The pre-typestate code was using ManuallyDrop<T> here, and it was forgotten on
> the newer versions. This means that the destructor for self runs here, which
> decreases the refcount by calling regulator_put().

Yeah that is correct.

> My proposed solution is:
>
>     /// Attempts to convert the regulator to an enabled state.
>     pub fn try_into_enabled(mut self) -> Result<Regulator<Enabled>, Error<Disabled>> {
>         // We will be transferring the ownership of our regulator_get() count to Regulator<Enabled>
>         let mut regulator = ManuallyDrop::new(self);
>
>         regulator.enable_internal()
>             .map(|()| Regulator {
>                 inner: regulator.inner,

This will only work if the type of `inner` implements `Copy`. I forgot
if it does.

>                 _phantom: PhantomData,
>             })
>             .map_err(|error| Error {
>                 error,
>                 regulator: ManuallyDrop::into_inner(regulator),
>             })
>     }
> }
>
>
>
> Alex, Benno, thoughts?

Looks correct.

---
Cheers,
Benno