Message-Id: <8517D6F0-C1A2-4E38-8E62-57DCCD5E58D4@collabora.com>
Date: Mon, 19 May 2025 07:52:38 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3] rust: regulator: add a bare minimum regulator
 abstraction

Hi Alex,

I still don’t understand your use case 100% :/

> 
> I just mean the cases where users will want to enable and disable the
> regulator more frequently than just enabling it at probe time.

This is already possible through kernel::types::Either. 

i.e.: the current design - or the proposed typestate one - can already switch
back and forth between Regulator and EnabledRegulator. Using Either makes it
just work, because you can change the variant at runtime without hassle. This
lets you consume self in an ergonomic way.

By the way, the reason I'm pushing back slightly here is because you seem
(IIUC) to be trying to reintroduce the pattern we had to move away from in v1.

i.e.: we explicitly had to move away from trying to match enables and disables
in Rust, because it was hard to get this right.

The current design is a simplification that apparently works, because at best
you have +1 on the count and that is encoded in the type itself, so there is
nothing to actually "track" or "balance" within a given instance. Multiple
calls to _get() or _enable() on the same instance are simply forbidden.

Can you add some pseudocode that shows how this doesn't work (or is otherwise
unergonomic) in Nova? I think it will make your point clearer.

> 
>> 
>>> 
>>> It has been proposed earlier to use a typestate, and this would indeed
>>> provide several benefits, the first one being the ability to have shared
>>> impl blocks (and shared documentation) between the enabled and disabled
>>> states for methods like set/get_voltage().
>>> 
>>> But the key benefit I see is that it could also address the
>>> aforementioned dynamic management problem through the introduction of a
>>> third state.
>>> 
>>> Alongside the `Enabled` and `Disabled` states, there would be a third
>>> state (`Dynamic`?) in which the regulator could either be enabled or
>>> disabled. This `Dynamic` state is the only one providing `enable` and
>>> `disable` methods (as well as `is_enabled`) to change its operational
>>> state without affecting its type.
>> 
>> Dynamic is just "Regulator" in the current version of this patch. There is no
>> "Disabled" because there is no guarantee that someone else won't enable the
>> regulator, trivially breaking this invariant at any moment.
> 
> There is a core difference, which is that in your version of
> `Regulator`, `enable` takes ownership of `self` and returns a different
> type, whereas `Dynamic` would take `&mut self` and change its internal
> state, like the C API does.

I see now, but consuming self is something we're trying after considering the
&mut self approach, which did not work very well in v1.

> 
>> 
>> The only thing we can guarantee is "Enabled", through our own call to
>> "regulator_enable()".
>> 
>> In fact, for the typestate solution, I was thinking about "UnknownState" and
>> "Enabled", or any nomenclature along these lines.
>> 
>>> 
>>> All three states then implement `set_voltage` and `get_voltage` through
>>> a common impl block, that could be extended with other methods from the
>>> C API that are independent of the state, as needed.
>>> 
>>> To handle typestate transitions:
>>> 
>>> - The `Disabled` and `Dynamic` states provide a `try_into_enabled()`
>>> method to transition the regulator to the `Enabled` state.
>> 
>> Why not “enable()” as we currently have?
> 
> `enable()` to me sounds like a method that mutates `self` in-place,
> whereas your version consumes it and turns it into a different type.
> Such methods are typically named `into_*`, or `try_into_*` when they can
> fail.
> 
> Actually, this could even be a `TryInto` implementation, but in this
> particular case having methods with the target state in their names may
> result in clearer code and allow the reader to model the transition
> graph more easily.
> 
>> 
>> If we go with the "Dynamic" nomenclature, and we agree that there's no
>> "Disabled",  then we can implement "pub fn enable(self) -> Regulator<Enabled>",
>> for "Dynamic", which is what we currently have, but with other names.
> 
> Not if we want to provide the behavior of the C consumer API, which
> requires multiple calls to `regulator_enable()` to be matched by an equal
> number of calls to `regulator_disable()`, which could be useful to some
> drivers (lest they reimplement their own counter).

This is explicitly not supported, because (given the current code) why should it be?

If you want a given regulator to be enabled, just make sure you have
Regulator<Enabled> in your kernel::types::Either container.

You don't need a counter either: Regulator<Enabled> has a count of one, and
when that goes out of scope, it's decremented.

> 
>> 
>> Also, I personally dislike this term: it's hard to tell what Regulator<Dynamic>
>> means on a first glance.
> 
> Yeah I'm not a fan of it and I'm sure there are better alternatives.
> Maybe `Controlled`?
> 
>> 
>>> - The `Enabled` and `Dynamic` states provide `try_into_disabled()`.
>>> - `Enabled` and `Disabled` also provide `into_dynamic()` (which cannot
>>> fail).
>>> 
>>> Essentially, the `Enabled` and `Disabled` states simply enforce an
>>> additional operational state invariant on the underlying regulator, and
>>> do not provide methods to change it.
>>> 
>>> The `Dynamic` state would be the default for `Regulator`, so by just
>>> using `Regulator`, the user gets an interface that works very similarly
>>> to the C API it abstracts, making it intuitive to those familiar with
>>> it.
>> 
>> This is already how it works: Regulator is the default and EnabledRegulator
>> adds an extra invariant.
> 
> Hopefully the above clarified the differences.
> 
>> 
>>> 
>>> But for cases where the regulator is known to always be in a specific
>>> state like `Enabled`, one can use `Regulator<Enabled>` and simplify
>>> their code.
>>> 
>>> This should retain the compile-time safety that your version proposed
>>> for common cases, while still exposing the flexibility of the C API when
>>> needed.
>>> 
>> 
>> Yeah, I agree.
> 
> I think the concept looks better if you consider the generic parameter
> of `Regulator` not as a state, but as a marker of guaranteed invariants
> and allowed transitions.
> 
> - `Regulator<Enabled>` guarantees that the regulator is enabled on the
>  consumer side. It is also disabled when the object is dropped. Many
>  drivers will just do `Regulator::<Enabled>::get()` at probe time (the
>  equivalent of `regulator_get_enable()`), store the object into the
>  device's state, and forget about it.
> 
> - `Regulator<Disabled>` guarantees that the regulator is not enabled on
>  the consumer side. It could be useful to have for drivers that use
>  distinct types to store their state depending on their power status:
>  the powered-on type would store a `Regulator<Enabled>`, the
>  powered-off type a `Regulator<Disabled>`. That way you cannot even
>  write code transitioning between the states if you omit the regulator
>  - which I think is really neat.
> 

I thought we had agreed that there is no “Disabled”, even in C?

> These two should cover a large percentage of consumer needs, but for
> those that need more fine-grained control we should also have one or two
> policies that follow the C API a bit closer, e.g.:
> 
> - `Regulator<Controlled>` (or just `Regulator`): user is granted an API
>  very close to the C one, and is responsible for balancing calls to
>  `enable()` and `disable()`. Regulator remains in the state it was in
>  when dropped.
> 
> - `Regulator<Switch>`: calls to `enable()` and `disable()` do not need
>  to be balanced, and the regulator always transitions to one state if
>  it was in the other. Regulator gets automatically disabled on drop.
>  This provides a simpler, safer alternative to `Controlled`.
> 
> Note that I am not advocating for including these two policies
> specifically, these are just examples. My main point is that we should
> also provide a way to change the regulator's enabled state without
> requiring a change of type ; which policy(es) to adopt will depend on
> which restrictions we conclude are adequate to place on the C API, if
> any.

Can you expand a bit on the issues of changing types? Again, some pseudocode
will probably help a lot :)

— Daniel

Powered by blists - more mailing lists