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

[Alice Ryhl <aliceryhl@google.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 definitely do not want a guard type.

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

> I am sure we can find ways around that, but a simple `bool` here seems to  
> fix this problem.

What I meant to suggest is two types:

	pub struct DisabledRegulator {
	    inner: NonNull<bindings::regulator>,
	}
	pub struct EnabledRegulator {
	    inner: DisabledRegulator,
	}

	impl Drop for EnabledRegulator {
	    fn drop(&mut self) {
	        unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
		// Destructor of DisabledRegulator runs now since it's a
		// field of EnabledRegulator.
	    }
	}
	impl Drop for DisabledRegulator {
	    fn drop(&mut self) {
	        unsafe { bindings::regulator_put(self.inner.as_ptr()) };
	    }
	}

This gives the right behavior. For devices where the regulator is always
going to be enabled, you just store an EnabledRegulator. If you need to
dynamically switch between them, you store an enum like Boqun suggested.

Alice