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

[Boqun Feng <boqun.feng@gmail.com>]

On Wed, Feb 19, 2025 at 02:10:24PM -0300, Daniel Almeida wrote:
> Hi Alice,
> 
> > On 19 Feb 2025, at 13:28, Alice Ryhl <aliceryhl@google.com> wrote:
> > 
> > I wonder if enabled vs disabled should be two different types?
> > 
> > Alice
> 
> I thought about having two types too, but I think it complicates the design.
> 
> 
> ```
> let foo: Regulator = Regulator::get(/*...*/)?;
> let foo_enabled: EnabledRegulator = foo.enable()?:
> ```
> 
> Let´s first agree that `Regulator::drop` is the right place to have `regulator_put`, since
> `Regulator::get()` acquired the reference in the first place.
> 
> This means that now, `EnabledRegulator` has to depend on `Regulator` somehow to ensure
> a proper drop order. Otherwise you might have an enabled regulator for which you don´t own
> the refcount. Furthermore, if Regulator drops while EnabledRegulator is alive, you get a splat.
> 
> In a driver, you now have to store both Regulator - for the refcount - and EnabledRegulator
> - as a way to tell the system you need that regulator to be active.
> 
> If EnabledRegulator is a guard type, this doesn´t work, as it creates a self-reference - on top
> of being extremely clunky.
> 
> You can then have EnabledRegulator consume Regulator, but this assumes that the regulator
> will be on all the time, which is not true. A simple pattern of
> 
> ```
> regulator_enable()
> do_fancy_stuff()
> regulator_disable()
> ```
> 
> Becomes a pain when one type consumes the other:
> 
> ```
> self.my_regulator.enable() // error, moves out of `&self`
> ``` 

You can introduce an enum:

	pub enum WaitForAGoodName {
		Regulator(Regulator),
		Enabled(EnableRegulator),
	}

for this case, and `my_regulator` is this type (or you can use
`kernel::types::Either`). The eventual code generation will probably use
a byte flag somewhere, but it only needs the byte flag for such a case.
In other cases, for example, the driver knows the regulator is always
enabled, you save the byte flag and the complexity of impl Regulator.

Regards,
Boqun

> 
> I am sure we can find ways around that, but a simple `bool` here seems to fix this problem.
> 
> Now you only have to store `Regulator`. If you need another part of your code to also keep
> the regulator enabled, you store a `Regulator` there and enable that as well. All calls to
> enable and disable will be automatically balanced for all instances of `Regulator` by
> virtue of the `enabled` bool as well.
> 
> - Daniel
>