rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] rust: regulator: improve the ergonomics of Rust regulators
@ 2025-09-08 23:10 Daniel Almeida
  2025-09-08 23:10 ` [PATCH v2 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Daniel Almeida @ 2025-09-08 23:10 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, Alexandre Courbot

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>
---
Changes in v2:
- Picked up tags
- Rebased on regulator/for-next
- Renamed enable() and enable_optional() to devm_enable() and
  devm_enable_optional().
- Renamed patch 2/2 to pick up the above change
- Added more docs and links where applicable (thanks, Alex)

- Link to v1: https://lore.kernel.org/r/20250829-regulator-remove-dynamic-v1-0-deb59205e8e9@collabora.com

---
Daniel Almeida (2):
      rust: regulator: remove Regulator<Dynamic>
      rust: regulator: add devm_enable and devm_enable_optional

 rust/helpers/regulator.c |  10 ++++
 rust/kernel/regulator.rs | 146 +++++++++++++++++++----------------------------
 2 files changed, 68 insertions(+), 88 deletions(-)
---
base-commit: 59e8e7b7f2206d7097e43266722b625715720dfa
change-id: 20250829-regulator-remove-dynamic-f1a6b8c0c1b0

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


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

* [PATCH v2 1/2] rust: regulator: remove Regulator<Dynamic>
  2025-09-08 23:10 [PATCH v2 0/2] rust: regulator: improve the ergonomics of Rust regulators Daniel Almeida
@ 2025-09-08 23:10 ` Daniel Almeida
  2025-09-08 23:10 ` [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional Daniel Almeida
  2025-09-11 23:23 ` (subset) [PATCH v2 0/2] rust: regulator: improve the ergonomics of Rust regulators Mark Brown
  2 siblings, 0 replies; 23+ messages in thread
From: Daniel Almeida @ 2025-09-08 23:10 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, Alexandre Courbot

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>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
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 34bb24ec8d4d436437d92a344c61fc3a46de0b5d..5ea2307f02df4a10c1c8c07b3b8c134d13519b69 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 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: &PrivateData) -> Result {
-///     // Increase the `enabled` reference count.
-///     data.regulator.enable()?;
-///
-///     Ok(())
-/// }
-///
-/// fn close(dev: &Device, data: &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,
 {
@@ -351,28 +287,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(&self) -> Result {
-        self.enable_internal()
-    }
-
-    /// Decreases the `enabled` reference count.
-    pub fn disable(&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] 23+ messages in thread

* [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-08 23:10 [PATCH v2 0/2] rust: regulator: improve the ergonomics of Rust regulators Daniel Almeida
  2025-09-08 23:10 ` [PATCH v2 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
@ 2025-09-08 23:10 ` Daniel Almeida
  2025-09-09  2:29   ` Alexandre Courbot
                     ` (3 more replies)
  2025-09-11 23:23 ` (subset) [PATCH v2 0/2] rust: regulator: improve the ergonomics of Rust regulators Mark Brown
  2 siblings, 4 replies; 23+ messages in thread
From: Daniel Almeida @ 2025-09-08 23:10 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 67 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 5ea2307f02df4a10c1c8c07b3b8c134d13519b69..d1c8c7308cdd9ae398883ddac52ff093b97764cd 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,39 @@ pub struct Error<State: RegulatorState> {
     pub regulator: Regulator<State>,
 }
 
+/// Enables a regulator whose lifetime is tied to the lifetime of `dev` through
+/// [`devres`].
+///
+/// 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.
+///
+/// [`devres`]: https://docs.kernel.org/driver-api/driver-model/devres.html
+pub fn devm_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 [`devm_enable`], but calls `devm_regulator_get_enable_optional`
+/// instead.
+///
+/// This enables a regulator whose lifetime is tied to the lifetime of `dev`
+/// through [`devres`], but does not print a message nor provides a dummy if the
+/// regulator is not found.
+///
+/// This calls `regulator_disable()` and `regulator_put()` automatically on
+/// driver detach.
+///
+/// [`devres`]: https://docs.kernel.org/driver-api/driver-model/devres.html
+pub fn devm_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 +179,29 @@ 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 [`devm_enable`] or [`devm_enable_optional`].
+/// This should be the default use-case unless they need more fine-grained
+/// control over the regulator's state.
+///
+/// [`devm_enable`]: crate::regulator::devm_enable
+/// [`devm_optional`]: crate::regulator::devm_enable_optional
+///
+/// ```
+/// # 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::devm_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] 23+ messages in thread

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-08 23:10 ` [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional Daniel Almeida
@ 2025-09-09  2:29   ` Alexandre Courbot
  2025-09-09  6:57   ` Boqun Feng
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 23+ messages in thread
From: Alexandre Courbot @ 2025-09-09  2:29 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 Tue Sep 9, 2025 at 8:10 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>
> ---

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

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-08 23:10 ` [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional Daniel Almeida
  2025-09-09  2:29   ` Alexandre Courbot
@ 2025-09-09  6:57   ` Boqun Feng
  2025-09-09 15:04     ` Daniel Almeida
  2025-09-09  7:15   ` Danilo Krummrich
  2025-09-10  5:26   ` Boqun Feng
  3 siblings, 1 reply; 23+ messages in thread
From: Boqun Feng @ 2025-09-09  6:57 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux

On Mon, Sep 08, 2025 at 08:10:28PM -0300, 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 67 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);
> +}
> +

These two functions are already EXPORT_SYMBOL_GPL(), so you won't need
to add rust_helper for them. Creating rust_helper_*() for them will just
export additional symbols.

Regards,
Boqun

>  #endif
> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> index 5ea2307f02df4a10c1c8c07b3b8c134d13519b69..d1c8c7308cdd9ae398883ddac52ff093b97764cd 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,39 @@ pub struct Error<State: RegulatorState> {
>      pub regulator: Regulator<State>,
>  }
>  
> +/// Enables a regulator whose lifetime is tied to the lifetime of `dev` through
> +/// [`devres`].
> +///
> +/// 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.
> +///
> +/// [`devres`]: https://docs.kernel.org/driver-api/driver-model/devres.html
> +pub fn devm_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 [`devm_enable`], but calls `devm_regulator_get_enable_optional`
> +/// instead.
> +///
> +/// This enables a regulator whose lifetime is tied to the lifetime of `dev`
> +/// through [`devres`], but does not print a message nor provides a dummy if the
> +/// regulator is not found.
> +///
> +/// This calls `regulator_disable()` and `regulator_put()` automatically on
> +/// driver detach.
> +///
> +/// [`devres`]: https://docs.kernel.org/driver-api/driver-model/devres.html
> +pub fn devm_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 +179,29 @@ 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 [`devm_enable`] or [`devm_enable_optional`].
> +/// This should be the default use-case unless they need more fine-grained
> +/// control over the regulator's state.
> +///
> +/// [`devm_enable`]: crate::regulator::devm_enable
> +/// [`devm_optional`]: crate::regulator::devm_enable_optional
> +///
> +/// ```
> +/// # 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::devm_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	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-08 23:10 ` [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional Daniel Almeida
  2025-09-09  2:29   ` Alexandre Courbot
  2025-09-09  6:57   ` Boqun Feng
@ 2025-09-09  7:15   ` Danilo Krummrich
  2025-09-10  5:26   ` Boqun Feng
  3 siblings, 0 replies; 23+ messages in thread
From: Danilo Krummrich @ 2025-09-09  7:15 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 Tue Sep 9, 2025 at 1:10 AM CEST, Daniel Almeida wrote:
> +/// Enables a regulator whose lifetime is tied to the lifetime of `dev` through
> +/// [`devres`].
> +///
> +/// 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.

Preferred over what? :)

I'd link to the alternative Regulator API.

> +///
> +/// [`devres`]: https://docs.kernel.org/driver-api/driver-model/devres.html
> +pub fn devm_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 [`devm_enable`], but calls `devm_regulator_get_enable_optional`
> +/// instead.
> +///
> +/// This enables a regulator whose lifetime is tied to the lifetime of `dev`
> +/// through [`devres`], but does not print a message nor provides a dummy if the
> +/// regulator is not found.
> +///
> +/// This calls `regulator_disable()` and `regulator_put()` automatically on
> +/// driver detach.
> +///
> +/// [`devres`]: https://docs.kernel.org/driver-api/driver-model/devres.html
> +pub fn devm_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 +179,29 @@ pub struct Error<State: RegulatorState> {
>  /// }
>  /// ```
>  ///
> +/// If a driver only cares about the regulator being on for as long it is bound

s/on/enabled/

> +/// to a device, then it should use [`devm_enable`] or [`devm_enable_optional`].
> +/// This should be the default use-case unless they need more fine-grained
> +/// control over the regulator's state.

"unless more fine-grained control over the regulator's state is required"

With those nits addressed and the unnecessary helpers removed,

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

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09  6:57   ` Boqun Feng
@ 2025-09-09 15:04     ` Daniel Almeida
  2025-09-09 15:38       ` Boqun Feng
  0 siblings, 1 reply; 23+ messages in thread
From: Daniel Almeida @ 2025-09-09 15:04 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux

Hi Boqun, thanks for chiming in!

> On 9 Sep 2025, at 03:57, Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> On Mon, Sep 08, 2025 at 08:10:28PM -0300, 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 67 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);
>> +}
>> +
> 
> These two functions are already EXPORT_SYMBOL_GPL(), so you won't need
> to add rust_helper for them. Creating rust_helper_*() for them will just
> export additional symbols.

These are inlined (stubbed) if CONFIG_REGULATOR is not set, so we need the
helpers to get around that, IIUC.

In fact, doing the change you proposed will result in Intel’s bot
complaining. Same for all other functions defined in the helper C file.

— Daniel


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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 15:04     ` Daniel Almeida
@ 2025-09-09 15:38       ` Boqun Feng
  2025-09-09 15:58         ` Miguel Ojeda
                           ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Boqun Feng @ 2025-09-09 15:38 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux

On Tue, Sep 09, 2025 at 12:04:35PM -0300, Daniel Almeida wrote:
> Hi Boqun, thanks for chiming in!
> 
> > On 9 Sep 2025, at 03:57, Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> > On Mon, Sep 08, 2025 at 08:10:28PM -0300, 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++-
> >> 2 files changed, 67 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);
> >> +}
> >> +
> > 
> > These two functions are already EXPORT_SYMBOL_GPL(), so you won't need
> > to add rust_helper for them. Creating rust_helper_*() for them will just
> > export additional symbols.
> 
> These are inlined (stubbed) if CONFIG_REGULATOR is not set, so we need the
> helpers to get around that, IIUC.
> 

Well, then the question is why we want to compiler regulator.rs if
CONFIG_REGULATOR=n? Shouldn't we do:

#[cfg(CONFIG_REGULATOR)]
pub mod regulator

in rust/kernel/lib.rs?

> In fact, doing the change you proposed will result in Intel´s bot
> complaining. Same for all other functions defined in the helper C file.
> 

With above, we probably can remove the whole helper file for regulator.

Regards,
Boqun

> - Daniel
> 

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 15:38       ` Boqun Feng
@ 2025-09-09 15:58         ` Miguel Ojeda
  2025-09-09 16:27           ` Boqun Feng
  2025-09-09 16:12         ` Daniel Almeida
  2025-09-09 16:17         ` Mark Brown
  2 siblings, 1 reply; 23+ messages in thread
From: Miguel Ojeda @ 2025-09-09 15:58 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Daniel Almeida, Liam Girdwood, Mark Brown, Miguel Ojeda,
	Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
	linux-kernel, rust-for-linux

On Tue, Sep 9, 2025 at 5:38 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Well, then the question is why we want to compiler regulator.rs if
> CONFIG_REGULATOR=n? Shouldn't we do:
>
> #[cfg(CONFIG_REGULATOR)]
> pub mod regulator
>
> in rust/kernel/lib.rs?

It depends on what the C API does -- if the C API is supposed to
"always work" (even if it is no-op or if it returns errors) even when
disabled (so that callers are easier), then we likely need to
replicate that on our side.

We had a similar discussion for DebugFS.

Cheers,
Miguel

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 15:38       ` Boqun Feng
  2025-09-09 15:58         ` Miguel Ojeda
@ 2025-09-09 16:12         ` Daniel Almeida
  2025-09-09 16:40           ` Boqun Feng
  2025-09-09 16:17         ` Mark Brown
  2 siblings, 1 reply; 23+ messages in thread
From: Daniel Almeida @ 2025-09-09 16:12 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux



> On 9 Sep 2025, at 12:38, Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> On Tue, Sep 09, 2025 at 12:04:35PM -0300, Daniel Almeida wrote:
>> Hi Boqun, thanks for chiming in!
>> 
>>> On 9 Sep 2025, at 03:57, Boqun Feng <boqun.feng@gmail.com> wrote:
>>> 
>>> On Mon, Sep 08, 2025 at 08:10:28PM -0300, 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++-
>>>> 2 files changed, 67 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);
>>>> +}
>>>> +
>>> 
>>> These two functions are already EXPORT_SYMBOL_GPL(), so you won't need
>>> to add rust_helper for them. Creating rust_helper_*() for them will just
>>> export additional symbols.
>> 
>> These are inlined (stubbed) if CONFIG_REGULATOR is not set, so we need the
>> helpers to get around that, IIUC.
>> 
> 
> Well, then the question is why we want to compiler regulator.rs if
> CONFIG_REGULATOR=n? Shouldn't we do:

Yes, we do want to compile regulator .rs. There’s been prior discussion on
this (see [0] below, but also [1]).

> 
> #[cfg(CONFIG_REGULATOR)]
> pub mod regulator
> 
> in rust/kernel/lib.rs?

This was the original approach, but this is incorrect behavior (again, see
[0]). The right thing to do is to call the stubs if CONFIG_REGULATOR is not
set, not make the regulator API unavailable.

> 
>> In fact, doing the change you proposed will result in Intel´s bot
>> complaining. Same for all other functions defined in the helper C file.
>> 
> 
> With above, we probably can remove the whole helper file for regulator.

Given the above, we cannot remove the file, and we must also add the devm
helpers as they are in the current patch.

— Daniel

[0] https://lore.kernel.org/rust-for-linux/a1b3561d-f5de-4474-85ef-1525a6c36bc5@arm.com/T/#mdf9d4005ee99929d0009ccc988efbc0789164b6d

[1] https://lore.kernel.org/rust-for-linux/25e9a9b6-4d81-4731-98fa-add40ccd4aab@sirena.org.uk/





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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 15:38       ` Boqun Feng
  2025-09-09 15:58         ` Miguel Ojeda
  2025-09-09 16:12         ` Daniel Almeida
@ 2025-09-09 16:17         ` Mark Brown
  2025-09-09 16:29           ` Danilo Krummrich
  2 siblings, 1 reply; 23+ messages in thread
From: Mark Brown @ 2025-09-09 16:17 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Daniel Almeida, Liam Girdwood, Miguel Ojeda, Alex Gaynor,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, Danilo Krummrich, linux-kernel,
	rust-for-linux

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

On Tue, Sep 09, 2025 at 08:38:27AM -0700, Boqun Feng wrote:

> Well, then the question is why we want to compiler regulator.rs if
> CONFIG_REGULATOR=n? Shouldn't we do:

> #[cfg(CONFIG_REGULATOR)]
> pub mod regulator

> in rust/kernel/lib.rs?

If we do that then every single user needs to also add ifdefs for the
regulator API which is not exactly wonderful usability.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 15:58         ` Miguel Ojeda
@ 2025-09-09 16:27           ` Boqun Feng
  2025-09-09 17:11             ` Mark Brown
  0 siblings, 1 reply; 23+ messages in thread
From: Boqun Feng @ 2025-09-09 16:27 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Daniel Almeida, Liam Girdwood, Mark Brown, Miguel Ojeda,
	Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
	linux-kernel, rust-for-linux

On Tue, Sep 09, 2025 at 05:58:34PM +0200, Miguel Ojeda wrote:
> On Tue, Sep 9, 2025 at 5:38 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Well, then the question is why we want to compiler regulator.rs if
> > CONFIG_REGULATOR=n? Shouldn't we do:
> >
> > #[cfg(CONFIG_REGULATOR)]
> > pub mod regulator
> >
> > in rust/kernel/lib.rs?
> 
> It depends on what the C API does -- if the C API is supposed to
> "always work" (even if it is no-op or if it returns errors) even when
> disabled (so that callers are easier), then we likely need to
> replicate that on our side.
> 

While I don't disagree with the rule in general, but I do want to say
the cost is the binary size (at least without inlining helpers), so I
think we are allowed to apply this rule case by case, i.e. even if a C
API is supposed to "always work" but Rust can avoid exposing the API at
some configurations.

I of course don't have a strong opinion on what the regulator subsystem
should do ;-)

Regards,
Boqun

> We had a similar discussion for DebugFS.
> 
> Cheers,
> Miguel

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 16:17         ` Mark Brown
@ 2025-09-09 16:29           ` Danilo Krummrich
  2025-09-09 17:10             ` Mark Brown
  0 siblings, 1 reply; 23+ messages in thread
From: Danilo Krummrich @ 2025-09-09 16:29 UTC (permalink / raw)
  To: Mark Brown
  Cc: Boqun Feng, Daniel Almeida, Liam Girdwood, Miguel Ojeda,
	Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, linux-kernel,
	rust-for-linux

On 9/9/25 6:17 PM, Mark Brown wrote:
> On Tue, Sep 09, 2025 at 08:38:27AM -0700, Boqun Feng wrote:
> 
>> Well, then the question is why we want to compiler regulator.rs if
>> CONFIG_REGULATOR=n? Shouldn't we do:
> 
>> #[cfg(CONFIG_REGULATOR)]
>> pub mod regulator
> 
>> in rust/kernel/lib.rs?
> 
> If we do that then every single user needs to also add ifdefs for the
> regulator API which is not exactly wonderful usability.

OOC, I assume users do not just depend on CONFIG_REGULATOR, as it depends on the
actual platform / board whether they require control over a regulator?

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 16:12         ` Daniel Almeida
@ 2025-09-09 16:40           ` Boqun Feng
  2025-09-09 17:02             ` Daniel Almeida
  2025-09-09 17:03             ` Daniel Almeida
  0 siblings, 2 replies; 23+ messages in thread
From: Boqun Feng @ 2025-09-09 16:40 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux

On Tue, Sep 09, 2025 at 01:12:51PM -0300, Daniel Almeida wrote:
> 
[...]
> >>> These two functions are already EXPORT_SYMBOL_GPL(), so you won't need
> >>> to add rust_helper for them. Creating rust_helper_*() for them will just
> >>> export additional symbols.
> >> 
> >> These are inlined (stubbed) if CONFIG_REGULATOR is not set, so we need the
> >> helpers to get around that, IIUC.
> >> 
> > 
> > Well, then the question is why we want to compiler regulator.rs if
> > CONFIG_REGULATOR=n? Shouldn't we do:
> 
> Yes, we do want to compile regulator .rs. There´s been prior discussion on
> this (see [0] below, but also [1]).
> 

Thanks for the pointers. Well I guess we really need helper inlining
badly ;-)

Regards,
Boqun

> > 
> > #[cfg(CONFIG_REGULATOR)]
> > pub mod regulator
> > 
> > in rust/kernel/lib.rs?
> 
[...]

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 16:40           ` Boqun Feng
@ 2025-09-09 17:02             ` Daniel Almeida
  2025-09-09 17:03             ` Daniel Almeida
  1 sibling, 0 replies; 23+ messages in thread
From: Daniel Almeida @ 2025-09-09 17:02 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux



> On 9 Sep 2025, at 13:40, Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> On Tue, Sep 09, 2025 at 01:12:51PM -0300, Daniel Almeida wrote:
>> 
> [...]
>>>>> These two functions are already EXPORT_SYMBOL_GPL(), so you won't need
>>>>> to add rust_helper for them. Creating rust_helper_*() for them will just
>>>>> export additional symbols.
>>>> 
>>>> These are inlined (stubbed) if CONFIG_REGULATOR is not set, so we need the
>>>> helpers to get around that, IIUC.
>>>> 
>>> 
>>> Well, then the question is why we want to compiler regulator.rs if
>>> CONFIG_REGULATOR=n? Shouldn't we do:
>> 
>> Yes, we do want to compile regulator .rs. There´s been prior discussion on
>> this (see [0] below, but also [1]).
>> 
> 
> Thanks for the pointers. Well I guess we really need helper inlining
> badly ;-)
> 


A bit out of context now, but I don’t worry too much about binary size. I
don’t think there’s enough functions in helpers/*.c to justify that.

What I do worry about is the indirection that might get added on the hot paths,
like requiring one extra function call (i.e.: for rust_helper_*) for every mmio
read/write, for example. That might have non-negligible impacts on performance
IMHO.

So yes, I do agree with you that this should be fixed.



> Regards,
> Boqun
> 
>>> 
>>> #[cfg(CONFIG_REGULATOR)]
>>> pub mod regulator
>>> 
>>> in rust/kernel/lib.rs?
>> 
> [...]


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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 16:40           ` Boqun Feng
  2025-09-09 17:02             ` Daniel Almeida
@ 2025-09-09 17:03             ` Daniel Almeida
  1 sibling, 0 replies; 23+ messages in thread
From: Daniel Almeida @ 2025-09-09 17:03 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux



> On 9 Sep 2025, at 13:40, Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> On Tue, Sep 09, 2025 at 01:12:51PM -0300, Daniel Almeida wrote:
>> 
> [...]
>>>>> These two functions are already EXPORT_SYMBOL_GPL(), so you won't need
>>>>> to add rust_helper for them. Creating rust_helper_*() for them will just
>>>>> export additional symbols.
>>>> 
>>>> These are inlined (stubbed) if CONFIG_REGULATOR is not set, so we need the
>>>> helpers to get around that, IIUC.
>>>> 
>>> 
>>> Well, then the question is why we want to compiler regulator.rs if
>>> CONFIG_REGULATOR=n? Shouldn't we do:
>> 
>> Yes, we do want to compile regulator .rs. There´s been prior discussion on
>> this (see [0] below, but also [1]).
>> 
> 
> Thanks for the pointers. Well I guess we really need helper inlining
> badly ;-)
> 
> Regards,
> Boqun
> 
>>> 
>>> #[cfg(CONFIG_REGULATOR)]
>>> pub mod regulator
>>> 
>>> in rust/kernel/lib.rs?
>> 
> […]

Ah, by the way, that seemed to be your only comment. I will wait some more for
the discussion to die down and then send a new version addressing Danilo’s
points. Can I get your r-b, or is there anything else you’d like to see
changed?

— Daniel



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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 16:29           ` Danilo Krummrich
@ 2025-09-09 17:10             ` Mark Brown
  0 siblings, 0 replies; 23+ messages in thread
From: Mark Brown @ 2025-09-09 17:10 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Boqun Feng, Daniel Almeida, Liam Girdwood, Miguel Ojeda,
	Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, linux-kernel,
	rust-for-linux

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

On Tue, Sep 09, 2025 at 06:29:49PM +0200, Danilo Krummrich wrote:
> On 9/9/25 6:17 PM, Mark Brown wrote:

> > If we do that then every single user needs to also add ifdefs for the
> > regulator API which is not exactly wonderful usability.

> OOC, I assume users do not just depend on CONFIG_REGULATOR, as it depends on the
> actual platform / board whether they require control over a regulator?

Yes, and an awful lot of the time control over the regulator is just an
optimisation and not actually required for things to work (eg, doing
DVFS or disabling the supply when the device is idle to reduce power
consumption).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 16:27           ` Boqun Feng
@ 2025-09-09 17:11             ` Mark Brown
  2025-09-09 17:15               ` Boqun Feng
  2025-09-09 17:16               ` Miguel Ojeda
  0 siblings, 2 replies; 23+ messages in thread
From: Mark Brown @ 2025-09-09 17:11 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Miguel Ojeda, Daniel Almeida, Liam Girdwood, Miguel Ojeda,
	Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
	linux-kernel, rust-for-linux

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

On Tue, Sep 09, 2025 at 09:27:46AM -0700, Boqun Feng wrote:
> On Tue, Sep 09, 2025 at 05:58:34PM +0200, Miguel Ojeda wrote:

> > It depends on what the C API does -- if the C API is supposed to
> > "always work" (even if it is no-op or if it returns errors) even when
> > disabled (so that callers are easier), then we likely need to
> > replicate that on our side.

> While I don't disagree with the rule in general, but I do want to say
> the cost is the binary size (at least without inlining helpers), so I
> think we are allowed to apply this rule case by case, i.e. even if a C
> API is supposed to "always work" but Rust can avoid exposing the API at
> some configurations.

The C stubs are all inlined so should have zero impact on the resulting
binary (as are all equivalent stubs generally).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 17:11             ` Mark Brown
@ 2025-09-09 17:15               ` Boqun Feng
  2025-09-09 21:10                 ` Mark Brown
  2025-09-09 17:16               ` Miguel Ojeda
  1 sibling, 1 reply; 23+ messages in thread
From: Boqun Feng @ 2025-09-09 17:15 UTC (permalink / raw)
  To: Mark Brown
  Cc: Miguel Ojeda, Daniel Almeida, Liam Girdwood, Miguel Ojeda,
	Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
	linux-kernel, rust-for-linux

On Tue, Sep 09, 2025 at 06:11:24PM +0100, Mark Brown wrote:
> On Tue, Sep 09, 2025 at 09:27:46AM -0700, Boqun Feng wrote:
> > On Tue, Sep 09, 2025 at 05:58:34PM +0200, Miguel Ojeda wrote:
> 
> > > It depends on what the C API does -- if the C API is supposed to
> > > "always work" (even if it is no-op or if it returns errors) even when
> > > disabled (so that callers are easier), then we likely need to
> > > replicate that on our side.
> 
> > While I don't disagree with the rule in general, but I do want to say
> > the cost is the binary size (at least without inlining helpers), so I
> > think we are allowed to apply this rule case by case, i.e. even if a C
> > API is supposed to "always work" but Rust can avoid exposing the API at
> > some configurations.
> 
> The C stubs are all inlined so should have zero impact on the resulting
> binary (as are all equivalent stubs generally).

Yeah, but for a rust_helper, right now a function and an exported symbol
will be generated so that Rust can do FFI. That's the size impact I'm
talking about. And for regulator, we only have helpers if
CONFIG_REGULATOR=n.

Regards,
Boqun

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 17:11             ` Mark Brown
  2025-09-09 17:15               ` Boqun Feng
@ 2025-09-09 17:16               ` Miguel Ojeda
  1 sibling, 0 replies; 23+ messages in thread
From: Miguel Ojeda @ 2025-09-09 17:16 UTC (permalink / raw)
  To: Mark Brown
  Cc: Boqun Feng, Daniel Almeida, Liam Girdwood, Miguel Ojeda,
	Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
	linux-kernel, rust-for-linux

On Tue, Sep 9, 2025 at 7:11 PM Mark Brown <broonie@kernel.org> wrote:
>
> The C stubs are all inlined so should have zero impact on the resulting
> binary (as are all equivalent stubs generally).

I think Boqun may be referring to the Rust <-> C step.

Cheers,
Miguel

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-09 17:15               ` Boqun Feng
@ 2025-09-09 21:10                 ` Mark Brown
  0 siblings, 0 replies; 23+ messages in thread
From: Mark Brown @ 2025-09-09 21:10 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Miguel Ojeda, Daniel Almeida, Liam Girdwood, Miguel Ojeda,
	Alex Gaynor, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
	linux-kernel, rust-for-linux

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

On Tue, Sep 09, 2025 at 10:15:55AM -0700, Boqun Feng wrote:
> On Tue, Sep 09, 2025 at 06:11:24PM +0100, Mark Brown wrote:

> > The C stubs are all inlined so should have zero impact on the resulting
> > binary (as are all equivalent stubs generally).

> Yeah, but for a rust_helper, right now a function and an exported symbol
> will be generated so that Rust can do FFI. That's the size impact I'm
> talking about. And for regulator, we only have helpers if
> CONFIG_REGULATOR=n.

That seems like it should be solvable on the Rust side in a similar
manner to how it's solved on the C side?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional
  2025-09-08 23:10 ` [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional Daniel Almeida
                     ` (2 preceding siblings ...)
  2025-09-09  7:15   ` Danilo Krummrich
@ 2025-09-10  5:26   ` Boqun Feng
  3 siblings, 0 replies; 23+ messages in thread
From: Boqun Feng @ 2025-09-10  5:26 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Liam Girdwood, Mark Brown, Miguel Ojeda, Alex Gaynor, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux

On Mon, Sep 08, 2025 at 08:10:28PM -0300, 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 67 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 5ea2307f02df4a10c1c8c07b3b8c134d13519b69..d1c8c7308cdd9ae398883ddac52ff093b97764cd 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,39 @@ pub struct Error<State: RegulatorState> {
>      pub regulator: Regulator<State>,
>  }
>  
> +/// Enables a regulator whose lifetime is tied to the lifetime of `dev` through
> +/// [`devres`].

This description seems a bit wordy to me. How about "Obtains and
enables a [`devres`]-managed regulator for a device"? And if you want,
you could explain the `regulator_disable()` and `regulator_put()` in the
second paragraph.

The rest looks good to me. Feel free to add:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> +///
> +/// 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.
> +///
> +/// [`devres`]: https://docs.kernel.org/driver-api/driver-model/devres.html
> +pub fn devm_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 [`devm_enable`], but calls `devm_regulator_get_enable_optional`
> +/// instead.
> +///
> +/// This enables a regulator whose lifetime is tied to the lifetime of `dev`
> +/// through [`devres`], but does not print a message nor provides a dummy if the
> +/// regulator is not found.
> +///
> +/// This calls `regulator_disable()` and `regulator_put()` automatically on
> +/// driver detach.
> +///
> +/// [`devres`]: https://docs.kernel.org/driver-api/driver-model/devres.html
> +pub fn devm_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 +179,29 @@ 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 [`devm_enable`] or [`devm_enable_optional`].
> +/// This should be the default use-case unless they need more fine-grained
> +/// control over the regulator's state.
> +///
> +/// [`devm_enable`]: crate::regulator::devm_enable
> +/// [`devm_optional`]: crate::regulator::devm_enable_optional
> +///
> +/// ```
> +/// # 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::devm_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	[flat|nested] 23+ messages in thread

* Re: (subset) [PATCH v2 0/2] rust: regulator: improve the ergonomics of Rust regulators
  2025-09-08 23:10 [PATCH v2 0/2] rust: regulator: improve the ergonomics of Rust regulators Daniel Almeida
  2025-09-08 23:10 ` [PATCH v2 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
  2025-09-08 23:10 ` [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional Daniel Almeida
@ 2025-09-11 23:23 ` Mark Brown
  2 siblings, 0 replies; 23+ messages in thread
From: Mark Brown @ 2025-09-11 23:23 UTC (permalink / raw)
  To: Liam Girdwood, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Daniel Almeida
  Cc: linux-kernel, rust-for-linux, Alexandre Courbot

On Mon, 08 Sep 2025 20:10:26 -0300, Daniel Almeida wrote:
> 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.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/2] rust: regulator: remove Regulator<Dynamic>
      commit: b87ecbc54f22382ace1cf41645e8652a4ce44d52

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark


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

end of thread, other threads:[~2025-09-11 23:23 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-08 23:10 [PATCH v2 0/2] rust: regulator: improve the ergonomics of Rust regulators Daniel Almeida
2025-09-08 23:10 ` [PATCH v2 1/2] rust: regulator: remove Regulator<Dynamic> Daniel Almeida
2025-09-08 23:10 ` [PATCH v2 2/2] rust: regulator: add devm_enable and devm_enable_optional Daniel Almeida
2025-09-09  2:29   ` Alexandre Courbot
2025-09-09  6:57   ` Boqun Feng
2025-09-09 15:04     ` Daniel Almeida
2025-09-09 15:38       ` Boqun Feng
2025-09-09 15:58         ` Miguel Ojeda
2025-09-09 16:27           ` Boqun Feng
2025-09-09 17:11             ` Mark Brown
2025-09-09 17:15               ` Boqun Feng
2025-09-09 21:10                 ` Mark Brown
2025-09-09 17:16               ` Miguel Ojeda
2025-09-09 16:12         ` Daniel Almeida
2025-09-09 16:40           ` Boqun Feng
2025-09-09 17:02             ` Daniel Almeida
2025-09-09 17:03             ` Daniel Almeida
2025-09-09 16:17         ` Mark Brown
2025-09-09 16:29           ` Danilo Krummrich
2025-09-09 17:10             ` Mark Brown
2025-09-09  7:15   ` Danilo Krummrich
2025-09-10  5:26   ` Boqun Feng
2025-09-11 23:23 ` (subset) [PATCH v2 0/2] rust: regulator: improve the ergonomics of Rust regulators Mark Brown

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