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

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

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

This means that Regulator<Disabled> is a misnomer.

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?

— Daniel