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.0On 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>
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
>
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
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
>
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.
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?
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).
> 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/
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? > [...]
> 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
> 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? >> > [...]
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
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
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).
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
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
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?
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>
© 2016 - 2025 Red Hat, Inc.