Re: [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Alex,

> On 30 Aug 2025, at 02:20, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> On Sat Aug 30, 2025 at 6:11 AM JST, Daniel Almeida wrote:
>> A lot of drivers only care about enabling the regulator for as long as
>> the underlying Device is bound. This can be easily observed due to the
>> extensive use of `devm_regulator_get_enable` and
>> `devm_regulator_get_enable_optional` throughout the kernel.
>> 
>> Therefore, make this helper available in Rust. Also add an example
>> noting how it should be the default API unless the driver needs more
>> fine-grained control over the regulator.
>> 
>> Suggested-by: Danilo Krummrich <dakr@kernel.org>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---
>> rust/helpers/regulator.c | 10 ++++++++++
>> rust/kernel/regulator.rs | 42 +++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 51 insertions(+), 1 deletion(-)
>> 
>> diff --git a/rust/helpers/regulator.c b/rust/helpers/regulator.c
>> index cd8b7ba648ee33dd14326c9242fb6c96ab8e32a7..11bc332443bd064f4b5afd350ffc045badff9076 100644
>> --- a/rust/helpers/regulator.c
>> +++ b/rust/helpers/regulator.c
>> @@ -40,4 +40,14 @@ int rust_helper_regulator_is_enabled(struct regulator *regulator)
>> return regulator_is_enabled(regulator);
>> }
>> 
>> +int rust_helper_devm_regulator_get_enable(struct device *dev, const char *id)
>> +{
>> + return devm_regulator_get_enable(dev, id);
>> +}
>> +
>> +int rust_helper_devm_regulator_get_enable_optional(struct device *dev, const char *id)
>> +{
>> + return devm_regulator_get_enable_optional(dev, id);
>> +}
>> +
>> #endif
>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>> index 60993373f4d911f4f0cbec2510f0c67efa24a51b..73d4c9b56dca9c676793d78e35e5758d18eef3e8 100644
>> --- a/rust/kernel/regulator.rs
>> +++ b/rust/kernel/regulator.rs
>> @@ -18,7 +18,7 @@
>> 
>> use crate::{
>>     bindings,
>> -    device::Device,
>> +    device::{Bound, Device},
>>     error::{from_err_ptr, to_result, Result},
>>     prelude::*,
>> };
>> @@ -70,6 +70,26 @@ pub struct Error<State: RegulatorState> {
>>     pub regulator: Regulator<State>,
>> }
>> 
>> +/// Enables a regulator whose lifetime is tied to the lifetime of `dev`.
>> +///
>> +/// This calls `regulator_disable()` and `regulator_put()` automatically on
>> +/// driver detach.
>> +///
>> +/// This API is identical to `devm_regulator_get_enable()`, and should be
>> +/// preferred if the caller only cares about the regulator being on.
>> +pub fn enable(dev: &Device<Bound>, name: &CStr) -> Result {
> 
> The name `enable` sounds like it just enables a regulator, which is a bit
> confusing IMHO. Maybe `get_enable` or `get_enable_for`? Not sure what
> would be idiomatic here.

So I thought about get_enabled, but I thought the "get" nomenclature was
confusing. For example, "get" acquires a refcount, but for the devm_ version
the refcount management is transparent. In this sense, I thought that just
"enable" would convey the idea better, i.e. "enable this and forget about any
lifetime management at all".

If you still think that using the "get" prefix is better, I can change it no
worries :)

> 
>> +    // SAFETY: `dev` is a valid and bound device, while `name` is a valid C
>> +    // string.
>> +    to_result(unsafe { bindings::devm_regulator_get_enable(dev.as_raw(), name.as_ptr()) })
>> +}
>> +
>> +/// Same as [`enable`], but calls `devm_regulator_get_enable_optional` instead.
> 
> Maybe explain in one sentence what `devm_regulator_get_enable_optional`
> as it might not be completely obvious.

Perhaps adding a link?

> 
>> +pub fn enable_optional(dev: &Device<Bound>, name: &CStr) -> Result {
>> +    // SAFETY: `dev` is a valid and bound device, while `name` is a valid C
>> +    // string.
>> +    to_result(unsafe { bindings::devm_regulator_get_enable_optional(dev.as_raw(), name.as_ptr()) })
>> +}
>> +
>> /// A `struct regulator` abstraction.
>> ///
>> /// # Examples
>> @@ -146,6 +166,26 @@ pub struct Error<State: RegulatorState> {
>> /// }
>> /// ```
>> ///
>> +/// If a driver only cares about the regulator being on for as long it is bound
>> +/// to a device, then it should use [`regulator::get_enabled`] or
>> +/// [`regulator::get_enabled_optional`]. This should be the default use-case
> 
> I suppose you mean `enable` and `enable_optional` instead of
> `get_enabled` and `get_enabled_optional` (although I personally would
> favor the latter :)).

Hmm, something happened here. I always make sure to run rustdoc before
submitting, and it did not error out even though this function does not exist.

In any case, my bad.

— Daniel