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