linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] rust: regulator: improve the ergonomics of Rust regulators
@ 2025-08-29 21:11 Daniel Almeida
  2025-08-29 21:11 ` [PATCH 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
  2025-08-29 21:11 ` [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API Daniel Almeida
  0 siblings, 2 replies; 9+ messages in thread
From: Daniel Almeida @ 2025-08-29 21:11 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, Danilo Krummrich
  Cc: linux-kernel, rust-for-linux, Daniel Almeida

This small series comes after some extensive discussion on a few minor
changes that can improve the current Rust regulator API.

Patch 1 removes Regulator<Dynamic>, as we have now established that
there is no usecase that can't use the safer Regulator<Enabled> and
Regulator<Disabled> APIs instead.

Patch 2 makes "devm_regulator_enable_get" and
"devm_regulator_enable_get_optional" available in Rust. This comes after
realizing that a lot of drivers simply care about whether regulators are
enabled for as long as the device is bound.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
Daniel Almeida (2):
      rust: regulator: remove Regulator<Dynamic>
      rust: regulator: add devm_regulator_get_enable API

 rust/helpers/regulator.c |  10 ++++
 rust/kernel/regulator.rs | 130 +++++++++++++++--------------------------------
 2 files changed, 52 insertions(+), 88 deletions(-)
---
base-commit: 1b237f190eb3d36f52dffe07a40b5eb210280e00
change-id: 20250829-regulator-remove-dynamic-f1a6b8c0c1b0

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] rust: regulator: remove Regulator<Dynamic>
  2025-08-29 21:11 [PATCH 0/2] rust: regulator: improve the ergonomics of Rust regulators Daniel Almeida
@ 2025-08-29 21:11 ` Daniel Almeida
  2025-08-30  5:21   ` Alexandre Courbot
                     ` (2 more replies)
  2025-08-29 21:11 ` [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API Daniel Almeida
  1 sibling, 3 replies; 9+ messages in thread
From: Daniel Almeida @ 2025-08-29 21:11 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, Danilo Krummrich
  Cc: linux-kernel, rust-for-linux, Daniel Almeida

After some experimenting and further discussion, it is starting to look
like Regulator<Dynamic> might be a footgun. It turns out that one can
get the same behavior by correctly using just Regulator<Enabled> and
Regulator<Disabled>, so there is no need to directly expose the manual
refcounting ability of Regulator<Dynamic> to clients.

Remove it while we do not have any other users.

Suggested-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/regulator.rs | 88 +-----------------------------------------------
 1 file changed, 1 insertion(+), 87 deletions(-)

diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
index 65f3a125348f2d821898188b4ac6a0b593f18bf2..60993373f4d911f4f0cbec2510f0c67efa24a51b 100644
--- a/rust/kernel/regulator.rs
+++ b/rust/kernel/regulator.rs
@@ -30,7 +30,6 @@ pub trait Sealed {}
 
     impl Sealed for super::Enabled {}
     impl Sealed for super::Disabled {}
-    impl Sealed for super::Dynamic {}
 }
 
 /// A trait representing the different states a [`Regulator`] can be in.
@@ -50,13 +49,6 @@ pub trait RegulatorState: private::Sealed + 'static {
 /// own an `enable` reference count, but the regulator may still be on.
 pub struct Disabled;
 
-/// A state that models the C API. The [`Regulator`] can be either enabled or
-/// disabled, and the user is in control of the reference count. This is also
-/// the default state.
-///
-/// Use [`Regulator::is_enabled`] to check the regulator's current state.
-pub struct Dynamic;
-
 impl RegulatorState for Enabled {
     const DISABLE_ON_DROP: bool = true;
 }
@@ -65,14 +57,9 @@ impl RegulatorState for Disabled {
     const DISABLE_ON_DROP: bool = false;
 }
 
-impl RegulatorState for Dynamic {
-    const DISABLE_ON_DROP: bool = false;
-}
-
 /// A trait that abstracts the ability to check if a [`Regulator`] is enabled.
 pub trait IsEnabled: RegulatorState {}
 impl IsEnabled for Disabled {}
-impl IsEnabled for Dynamic {}
 
 /// An error that can occur when trying to convert a [`Regulator`] between states.
 pub struct Error<State: RegulatorState> {
@@ -183,64 +170,13 @@ pub struct Error<State: RegulatorState> {
 /// }
 /// ```
 ///
-/// ## Using [`Regulator<Dynamic>`]
-///
-/// This example mimics the behavior of the C API, where the user is in
-/// control of the enabled reference count. This is useful for drivers that
-/// might call enable and disable to manage the `enable` reference count at
-/// runtime, perhaps as a result of `open()` and `close()` calls or whatever
-/// other driver-specific or subsystem-specific hooks.
-///
-/// ```
-/// # use kernel::prelude::*;
-/// # use kernel::c_str;
-/// # use kernel::device::Device;
-/// # use kernel::regulator::{Regulator, Dynamic};
-/// struct PrivateData {
-///     regulator: Regulator<Dynamic>,
-/// }
-///
-/// // A fictictious probe function that obtains a regulator and sets it up.
-/// fn probe(dev: &Device) -> Result<PrivateData> {
-///     // Obtain a reference to a (fictitious) regulator.
-///     let mut regulator = Regulator::<Dynamic>::get(dev, c_str!("vcc"))?;
-///
-///     Ok(PrivateData { regulator })
-/// }
-///
-/// // A fictictious function that indicates that the device is going to be used.
-/// fn open(dev: &Device, data: &mut PrivateData) -> Result {
-///     // Increase the `enabled` reference count.
-///     data.regulator.enable()?;
-///
-///     Ok(())
-/// }
-///
-/// fn close(dev: &Device, data: &mut PrivateData) -> Result {
-///     // Decrease the `enabled` reference count.
-///     data.regulator.disable()?;
-///
-///     Ok(())
-/// }
-///
-/// fn remove(dev: &Device, data: PrivateData) -> Result {
-///     // `PrivateData` is dropped here, which will drop the
-///     // `Regulator<Dynamic>` in turn.
-///     //
-///     // The reference that was obtained by `regulator_get()` will be
-///     // released, but it is up to the user to make sure that the number of calls
-///     // to `enable()` and `disabled()` are balanced before this point.
-///     Ok(())
-/// }
-/// ```
-///
 /// # Invariants
 ///
 /// - `inner` is a non-null wrapper over a pointer to a `struct
 ///   regulator` obtained from [`regulator_get()`].
 ///
 /// [`regulator_get()`]: https://docs.kernel.org/driver-api/regulator.html#c.regulator_get
-pub struct Regulator<State = Dynamic>
+pub struct Regulator<State>
 where
     State: RegulatorState,
 {
@@ -354,28 +290,6 @@ pub fn try_into_disabled(self) -> Result<Regulator<Disabled>, Error<Enabled>> {
     }
 }
 
-impl Regulator<Dynamic> {
-    /// Obtains a [`Regulator`] instance from the system. The current state of
-    /// the regulator is unknown and it is up to the user to manage the enabled
-    /// reference count.
-    ///
-    /// This closely mimics the behavior of the C API and can be used to
-    /// dynamically manage the enabled reference count at runtime.
-    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
-        Regulator::get_internal(dev, name)
-    }
-
-    /// Increases the `enabled` reference count.
-    pub fn enable(&mut self) -> Result {
-        self.enable_internal()
-    }
-
-    /// Decreases the `enabled` reference count.
-    pub fn disable(&mut self) -> Result {
-        self.disable_internal()
-    }
-}
-
 impl<T: IsEnabled> Regulator<T> {
     /// Checks if the regulator is enabled.
     pub fn is_enabled(&self) -> bool {

-- 
2.51.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API
  2025-08-29 21:11 [PATCH 0/2] rust: regulator: improve the ergonomics of Rust regulators Daniel Almeida
  2025-08-29 21:11 ` [PATCH 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
@ 2025-08-29 21:11 ` Daniel Almeida
  2025-08-30  5:20   ` Alexandre Courbot
  1 sibling, 1 reply; 9+ messages in thread
From: Daniel Almeida @ 2025-08-29 21:11 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, Danilo Krummrich
  Cc: linux-kernel, rust-for-linux, Daniel Almeida

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API
  2025-08-29 21:11 ` [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API Daniel Almeida
@ 2025-08-30  5:20   ` Alexandre Courbot
  2025-08-30 13:13     ` Daniel Almeida
  0 siblings, 1 reply; 9+ messages in thread
From: Alexandre Courbot @ 2025-08-30  5:20 UTC (permalink / raw)
  To: Daniel Almeida, Liam Girdwood, Mark Brown, Miguel Ojeda,
	Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Danilo Krummrich
  Cc: linux-kernel, rust-for-linux

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] rust: regulator: remove Regulator<Dynamic>
  2025-08-29 21:11 ` [PATCH 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
@ 2025-08-30  5:21   ` Alexandre Courbot
  2025-08-30 13:33   ` Danilo Krummrich
  2025-09-02  7:53   ` Alice Ryhl
  2 siblings, 0 replies; 9+ messages in thread
From: Alexandre Courbot @ 2025-08-30  5:21 UTC (permalink / raw)
  To: Daniel Almeida, Liam Girdwood, Mark Brown, Miguel Ojeda,
	Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Danilo Krummrich
  Cc: linux-kernel, rust-for-linux

On Sat Aug 30, 2025 at 6:11 AM JST, Daniel Almeida wrote:
> After some experimenting and further discussion, it is starting to look
> like Regulator<Dynamic> might be a footgun. It turns out that one can
> get the same behavior by correctly using just Regulator<Enabled> and
> Regulator<Disabled>, so there is no need to directly expose the manual
> refcounting ability of Regulator<Dynamic> to clients.
>
> Remove it while we do not have any other users.
>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API
  2025-08-30  5:20   ` Alexandre Courbot
@ 2025-08-30 13:13     ` Daniel Almeida
  2025-08-30 13:38       ` Danilo Krummrich
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Almeida @ 2025-08-30 13:13 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, Danilo Krummrich, linux-kernel,
	rust-for-linux

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




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] rust: regulator: remove Regulator<Dynamic>
  2025-08-29 21:11 ` [PATCH 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
  2025-08-30  5:21   ` Alexandre Courbot
@ 2025-08-30 13:33   ` Danilo Krummrich
  2025-09-02  7:53   ` Alice Ryhl
  2 siblings, 0 replies; 9+ messages in thread
From: Danilo Krummrich @ 2025-08-30 13:33 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, linux-kernel, rust-for-linux

On Fri Aug 29, 2025 at 11:11 PM CEST, Daniel Almeida wrote:
> After some experimenting and further discussion, it is starting to look
> like Regulator<Dynamic> might be a footgun. It turns out that one can
> get the same behavior by correctly using just Regulator<Enabled> and
> Regulator<Disabled>, so there is no need to directly expose the manual
> refcounting ability of Regulator<Dynamic> to clients.
>
> Remove it while we do not have any other users.
>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API
  2025-08-30 13:13     ` Daniel Almeida
@ 2025-08-30 13:38       ` Danilo Krummrich
  0 siblings, 0 replies; 9+ messages in thread
From: Danilo Krummrich @ 2025-08-30 13:38 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Alexandre Courbot, Liam Girdwood, Mark Brown, Miguel Ojeda,
	Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	linux-kernel, rust-for-linux

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] rust: regulator: remove Regulator<Dynamic>
  2025-08-29 21:11 ` [PATCH 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
  2025-08-30  5:21   ` Alexandre Courbot
  2025-08-30 13:33   ` Danilo Krummrich
@ 2025-09-02  7:53   ` Alice Ryhl
  2 siblings, 0 replies; 9+ messages in thread
From: Alice Ryhl @ 2025-09-02  7:53 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux

On Fri, Aug 29, 2025 at 06:11:31PM -0300, Daniel Almeida wrote:
> After some experimenting and further discussion, it is starting to look
> like Regulator<Dynamic> might be a footgun. It turns out that one can
> get the same behavior by correctly using just Regulator<Enabled> and
> Regulator<Disabled>, so there is no need to directly expose the manual
> refcounting ability of Regulator<Dynamic> to clients.
> 
> Remove it while we do not have any other users.
> 
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-09-02  7:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-29 21:11 [PATCH 0/2] rust: regulator: improve the ergonomics of Rust regulators Daniel Almeida
2025-08-29 21:11 ` [PATCH 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
2025-08-30  5:21   ` Alexandre Courbot
2025-08-30 13:33   ` Danilo Krummrich
2025-09-02  7:53   ` Alice Ryhl
2025-08-29 21:11 ` [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API Daniel Almeida
2025-08-30  5:20   ` Alexandre Courbot
2025-08-30 13:13     ` Daniel Almeida
2025-08-30 13:38       ` Danilo Krummrich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).