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

[Mark Brown]

On Wed, Feb 19, 2025 at 02:10:24PM -0300, Daniel Almeida wrote:

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

Having an enabled regulator object depend on a regulator object seems
like a goal rather than a problem, surely it's common to need such
relationships and there's an idiomatic way to do it?  It seems to be how
Rust does mutexes... 

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

That's true, unless you can make a type of enable that just fully takes
ownership of the regulator (which TBH people want, people really want a
devm_regulator_get_enable() C API which just gets and holds an enabled
regulator for the simple case where you don't actually ever manage the
power).  It's possible there's a need to split simple and complex
consumer APIs in Rust?

> 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

I don't understand the self reference thing?

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

Your second block of code doesn't look obviously painful there?

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

What you're describing here with creating one Regulator object per
enable sounds more like you want it to be an error to do multiple
enables on a single regulator.  Instead of reference counting or
silently ignoring duplicate enables it should error out on a duplicate
enable.

Attachment: signature.asc
Description: PGP signature