[PATCH v6 0/2] Add a bare-minimum Regulator abstraction

Daniel Almeida posted 2 patches 3 months, 1 week ago
There is a newer version of this series
MAINTAINERS                     |   1 +
rust/bindings/bindings_helper.h |   1 +
rust/kernel/lib.rs              |   2 +
rust/kernel/regulator.rs        | 392 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 396 insertions(+)
[PATCH v6 0/2] Add a bare-minimum Regulator abstraction
Posted by Daniel Almeida 3 months, 1 week ago
---
Changes in v6:
- Use ManuallyDrop<T> to avoid running the destructor in
  try_into_enabled() and try_into_disabled(). This is the same strategy
  that was being used successfully in the pre-typestate version of this
  patch
- Link to v5: https://lore.kernel.org/r/20250623-topics-tyr-regulator-v5-0-99069658cb54@collabora.com

Changes in v5:
- Remove TryIntoEnabled and TryIntoDisabled traits (they were only
  implemented for a single type anyways)
- Added regulator.rs to VOLTAGE AND CURRENT REGULATOR FRAMEWORK
- Applied the diff from Miguel Ojeda to format the docs
- Link to v4: https://lore.kernel.org/r/20250609-topics-tyr-regulator-v4-1-b4fdcf1385a7@collabora.com

Changes in v4:
- Rewrote the abstraction to use typestates as per the suggestions by
  Benno and Alex.
- Introduced the `Dynamic` state.
- Added more examples.
- Fixed some broken docs.
- Link to v3: https://lore.kernel.org/r/20250513-topics-tyr-regulator-v3-1-4cc2704dfec6@collabora.com

Changes in v3:
- Rebased on rust-next
- Added examples to showcase the API
- Fixed some rendering issues in the docs
- Exposed {get|set}_voltage for both Regulator and EnabledRegulator
- Derived Clone, Copy, PartialEq and Eq for Microvolt
- Link to v2: https://lore.kernel.org/r/20250326-topics-tyr-regulator-v2-1-c0ea6a861be6@collabora.com

Resend v2:
  - cc Regulator maintainers
Changes from v1:
  - Rebased on rust-next
  - Split the design into two types as suggested by Alice Ryhl.
  - Modify the docs to highlight how users can use kernel::types::Either
    or an enum to enable and disable the regulator at runtime.
  - Link to v1: https://lore.kernel.org/rust-for-linux/20250219162517.278362-1-daniel.almeida@collabora.com/

---
Daniel Almeida (2):
      rust: regulator: add a bare minimum regulator abstraction
      MAINAINTERS: add regulator.rs to the regulator API entry

 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/regulator.rs        | 392 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 396 insertions(+)
---
base-commit: edc5e6e019c99b529b3d1f2801d5cce9924ae79b
change-id: 20250326-topics-tyr-regulator-e8b98f6860d7

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>
Re: [PATCH v6 0/2] Add a bare-minimum Regulator abstraction
Posted by Daniel Almeida 3 months, 1 week ago
Mark,

Is it ok for the abstraction to only be built when CONFIG_REGULATOR=y? This
means that any Rust drivers using it have to depend on CONFIG_REGULATOR too.

I thought this was acceptable, but apparently that is not the case? See this
comment from Rob Herring [0].

-- Daniel

[0] https://lore.kernel.org/rust-for-linux/a1b3561d-f5de-4474-85ef-1525a6c36bc5@arm.com/T/#mdf9d4005ee99929d0009ccc988efbc0789164b6d
Re: [PATCH v6 0/2] Add a bare-minimum Regulator abstraction
Posted by Mark Brown 3 months, 1 week ago
On Tue, Jul 01, 2025 at 12:28:37PM -0300, Daniel Almeida wrote:

> Is it ok for the abstraction to only be built when CONFIG_REGULATOR=y? This
> means that any Rust drivers using it have to depend on CONFIG_REGULATOR too.

> I thought this was acceptable, but apparently that is not the case? See this
> comment from Rob Herring [0].

The regulator API stubs itself out when disabled, but given that this is
just wrappers it's not clear what the tasteful thing would be here - it
should do the right thing because it will itself be built in terms of
the C stubs.  I don't know if Rust can sensibly stub things, or if
there's much percentage in that for a thin wrapper which does basically
nothing itself.
Re: [PATCH v6 0/2] Add a bare-minimum Regulator abstraction
Posted by Daniel Almeida 3 months, 1 week ago

> On 1 Jul 2025, at 12:34, Mark Brown <broonie@kernel.org> wrote:
> 
> On Tue, Jul 01, 2025 at 12:28:37PM -0300, Daniel Almeida wrote:
> 
>> Is it ok for the abstraction to only be built when CONFIG_REGULATOR=y? This
>> means that any Rust drivers using it have to depend on CONFIG_REGULATOR too.
> 
>> I thought this was acceptable, but apparently that is not the case? See this
>> comment from Rob Herring [0].
> 
> The regulator API stubs itself out when disabled, but given that this is
> just wrappers it's not clear what the tasteful thing would be here - it
> should do the right thing because it will itself be built in terms of
> the C stubs.  I don't know if Rust can sensibly stub things, or if
> there's much percentage in that for a thin wrapper which does basically
> nothing itself.

Well, if all functions in the regulator API have stubs, then perhaps a trivial
solution would be removing this #[cfg] from here:

+#[cfg(CONFIG_REGULATOR)] <---
+pub mod regulator;

This would build the abstractions unconditionally, and it would transparently
use the C stubs when calling the C functions.

Waiting for more comments from the other RFL folks.

-- Daniel
Re: [PATCH v6 0/2] Add a bare-minimum Regulator abstraction
Posted by Miguel Ojeda 3 months, 1 week ago
On Tue, Jul 1, 2025 at 6:10 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Well, if all functions in the regulator API have stubs, then perhaps a trivial
> solution would be removing this #[cfg] from here:
>
> +#[cfg(CONFIG_REGULATOR)] <---
> +pub mod regulator;
>
> This would build the abstractions unconditionally, and it would transparently
> use the C stubs when calling the C functions.
>
> Waiting for more comments from the other RFL folks.

If C typically provides an API with stubs because users should not
generally care whether it is enabled or not, then you should try to
mimic that if it makes sense.

There have been similar discussions for e.g. debugfs which you may
want to take a look at:

    https://lore.kernel.org/rust-for-linux/2025043021-plaza-grip-2916@gregkh/

where the callers didn't even want to get a `Result` if not enabled,
and as a result of that, ended up like e.g.:

    https://lore.kernel.org/rust-for-linux/20250627-debugfs-rust-v8-1-c6526e413d40@google.com/

In that case, they ended up with a few `cfg`s, e.g. to optionally have
a field in a struct, but in your case it may be simpler.

I hope that helps.

Cheers,
Miguel