Re: [PATCH v3] rust: regulator: add a bare minimum regulator abstraction

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

On Wed May 14, 2025 at 4:40 PM CEST, Daniel Almeida wrote:
>> On 14 May 2025, at 10:57, Benno Lossin <lossin@kernel.org> wrote:
>> On Wed May 14, 2025 at 3:01 PM CEST, Daniel Almeida wrote:
>>>> On 13 May 2025, at 17:01, Benno Lossin <lossin@kernel.org> wrote:
>>>> On Tue May 13, 2025 at 5:44 PM CEST, Daniel Almeida wrote:
>>>>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>>>>> new file mode 100644
>>>>> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
>>>>> --- /dev/null
>>>>> +++ b/rust/kernel/regulator.rs
>>>>> @@ -0,0 +1,211 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>>>> +//! voltage and current regulators.
>>>>> +//!
>>>>> +//! The intention is to allow systems to dynamically control regulator power
>>>>> +//! output in order to save power and prolong battery life. This applies to both
>>>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>>>> +//! (where current limit is controllable).
>>>>> +//!
>>>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>>>> +//!
>>>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>>>> +//! [`EnabledRegulator`].
>>>> 
>>>> Would it make sense to store this in a generic variable acting as a type
>>>> state instead of using two different names? So:
>>>> 
>>>>   pub struct Regulator<State: RegulatorState> { /* ... */ }
>>>> 
>>>>   pub trait RegulatorState: private::Sealed {}
>>>> 
>>>>   pub struct Enabled;
>>>>   pub struct Disabled;
>>>> 
>>>>   impl RegulatorState for Enabled {}
>>>>   impl RegulatorState for Disabled {}
>>>> 
>>>> And then one would use `Regulator<Enabled>` and `Regulator<Disabled>`.
>>> 
>>> This seems like just another way of doing the same thing.
>>> 
>>> I have nothing against a typestate, it's an elegant solution really, but so is
>>> the current one. I'd say let's keep what we have unless there is something
>>> objectively better about a typestatethat makes it worthy to change this.
>> 
>> I'd say it's cleaner and we already have some APIs that utilize type
>> states, so I'd prefer we use that where it makes sense.
>> 
>
> By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
> It just tells the system that you don't care about it being enabled anymore. It can
> still remain on if there are other users.

Hmm, so a `struct regulator` might already be enabled and calling
`regulator_enable` doesn't do anything?

> This means that Regulator<Disabled> is a misnomer.

Yeah.

> Also, the current solution relies on Regulator being a member of
> EnabledRegulator to keep the refcounts sane. I wonder how that is going to work
> now that Regulator<Disabled> is obviously not a member of Regulator<Enabled>, i.e.:
>
> impl Drop for Regulator<Enabled> {
>  fn drop(&mut self) {
>   regulator_disable();  
>   
>   // We now have to call this explicitly, because no one else will call it for
>   // us.
>   regulator_put();
>  }
> }
>
> impl Drop for Regulator<Disabled> {
>  fn drop(&mut self) {
>   // We now have to repeat this in both destructors.
>   regulator_put();  
>  }
> }
>
> Just to confirm: is that what you have in mind?

You can't specialize the `Drop` impl, you'd have to do this:

    impl<State: RegulatorState> Drop for Regulator<State> {
        fn drop(&mut self) {
            if State::ENABLED {
                regulator_disable();
            }
            regulator_put();
        }
    }

I still think it's an improvement though.

---
Cheers,
Benno