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 {
+    // 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.
+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
+/// unless they need more fine-grained control over the regulator's state.
+///
+/// ```
+/// # use kernel::prelude::*;
+/// # use kernel::c_str;
+/// # use kernel::device::{Bound, Device};
+/// # use kernel::regulator;
+/// fn enable(dev: &Device<Bound>) -> Result {
+///     // Obtain a reference to a (fictitious) regulator and enable it. This
+///     // call only returns whether the operation succeeded.
+///     regulator::enable(dev, c_str!("vcc"))?;
+///
+///     // The regulator will be disabled and put when `dev` is unbound.
+///     Ok(())
+/// }
+/// ```
+///
 /// ## Disabling a regulator
 ///
 /// ```

-- 
2.51.0

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.

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

> +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 :)).

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

On Sat Aug 30, 2025 at 3:13 PM CEST, Daniel Almeida wrote:
>> 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:
>>> +/// 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".

Technically, it does acquire a reference count, we just don't return the
corresponding regulator object to the caller, but leave the reference count to
devres.

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

If we want to be extra correct, it should be devm_get_enable(). But the fact
that devres holds a reference count is an implementation detail not relevant to
the caller.

Hence, I think devm_enable() (and devm_enable_optional()) is enough. But we
should make it obvious that it's devres managed, i.e. "devm".