Move code specific to normal led class devices into a separate file and
introduce the `led::Mode` trait to allow for other types of led class
devices.
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
MAINTAINERS | 1 +
rust/kernel/device.rs | 2 +-
rust/kernel/led.rs | 128 +++++++++++++++++++++++++++++++++++-----------
rust/kernel/led/normal.rs | 39 ++++++++++++++
4 files changed, 139 insertions(+), 31 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 80cb030911b7..ca11b9064e3f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14118,6 +14118,7 @@ L: linux-leds@vger.kernel.org
L: rust-for-linux@vger.kernel.org
S: Maintained
F: rust/kernel/led.rs
+F: rust/kernel/led/
LEGO MINDSTORMS EV3
R: David Lechner <david@lechnology.com>
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 660cb2b48c07..d330248ff2aa 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -333,7 +333,7 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
}
/// Returns a reference to the parent device, if any.
- #[cfg_attr(not(CONFIG_AUXILIARY_BUS), expect(dead_code))]
+ #[cfg_attr(not(any(CONFIG_AUXILIARY_BUS, CONFIG_LEDS_CLASS)), expect(dead_code))]
pub(crate) fn parent(&self) -> Option<&Device> {
// SAFETY:
// - By the type invariant `self.as_raw()` is always valid.
diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
index fca55f02be8d..d51735322093 100644
--- a/rust/kernel/led.rs
+++ b/rust/kernel/led.rs
@@ -44,14 +44,18 @@
}, //
};
+mod normal;
+
+pub use normal::Normal;
+
/// The led class device representation.
///
-/// This structure represents the Rust abstraction for a C `struct led_classdev`.
+/// This structure represents the Rust abstraction for a led class device.
#[pin_data(PinnedDrop)]
pub struct Device<T: LedOps> {
ops: T,
#[pin]
- classdev: Opaque<bindings::led_classdev>,
+ classdev: Opaque<<T::Mode as private::Mode>::Type>,
}
/// The led init data representation.
@@ -153,6 +157,7 @@ pub fn color(self, color: Color) -> Self {
/// #[vtable]
/// impl led::LedOps for MyLedOps {
/// type Bus = platform::Device<device::Bound>;
+/// type Mode = led::Normal;
/// const BLOCKING: bool = false;
/// const MAX_BRIGHTNESS: u32 = 255;
///
@@ -184,6 +189,12 @@ pub trait LedOps: Send + 'static + Sized {
/// The bus device required by the implementation.
#[allow(private_bounds)]
type Bus: AsBusDevice<Bound>;
+
+ /// The led mode to use.
+ ///
+ /// See [`Mode`].
+ type Mode: Mode;
+
/// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must perform the
/// operation immediately. If set false, they must not sleep.
const BLOCKING: bool;
@@ -270,6 +281,42 @@ fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {
}
}
+/// The led mode.
+///
+/// Each led mode has its own led class device type with different capabilities.
+///
+/// See [`Normal`].
+pub trait Mode: private::Mode {}
+
+impl<T: private::Mode> Mode for T {}
+
+type RegisterFunc<T> =
+ unsafe extern "C" fn(*mut bindings::device, *mut T, *mut bindings::led_init_data) -> i32;
+
+type UnregisterFunc<T> = unsafe extern "C" fn(*mut T);
+
+mod private {
+ pub trait Mode {
+ type Type;
+ const REGISTER: super::RegisterFunc<Self::Type>;
+ const UNREGISTER: super::UnregisterFunc<Self::Type>;
+
+ /// # Safety
+ /// `raw` must be a valid pointer to [`Self::Type`].
+ unsafe fn device<'a>(raw: *mut Self::Type) -> &'a crate::device::Device;
+
+ /// # Safety
+ /// `led_cdev` must be a valid pointer to `led_classdev` embedded within [`Self::Type`].
+ unsafe fn from_classdev(led_cdev: *mut bindings::led_classdev) -> *mut Self::Type;
+
+ /// # Safety
+ /// `raw` must be a valid pointer to [`Self::Type`].
+ unsafe fn pre_brightness_set(_raw: *mut Self::Type, _brightness: u32) {}
+
+ fn release(_led_cdev: &mut Self::Type) {}
+ }
+}
+
// SAFETY: A `led::Device` can be unregistered from any thread.
unsafe impl<T: LedOps + Send> Send for Device<T> {}
@@ -278,24 +325,22 @@ unsafe impl<T: LedOps + Send> Send for Device<T> {}
unsafe impl<T: LedOps + Sync> Sync for Device<T> {}
impl<T: LedOps> Device<T> {
- /// Registers a new led classdev.
- ///
- /// The [`Device`] will be unregistered on drop.
- pub fn new<'a>(
+ fn __new<'a>(
parent: &'a T::Bus,
init_data: InitData<'a>,
ops: T,
+ func: impl FnOnce(bindings::led_classdev) -> Result<<T::Mode as private::Mode>::Type> + 'a,
) -> impl PinInit<Devres<Self>, Error> + 'a {
Devres::new(
parent.as_ref(),
try_pin_init!(Self {
ops,
- classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
+ classdev <- Opaque::try_ffi_init(|ptr: *mut <T::Mode as private::Mode>::Type| {
// SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
- // `led_classdev` gets fully initialized in-place by
- // `led_classdev_register_ext` including `mutex` and `list_head`.
+ // `T::Mode::Type` (and the embedded led_classdev) gets fully initialized
+ // in-place by `T::Mode::REGISTER` including `mutex` and `list_head`.
unsafe {
- ptr.write(bindings::led_classdev {
+ ptr.write((func)(bindings::led_classdev {
brightness_set: (!T::BLOCKING)
.then_some(Adapter::<T>::brightness_set_callback),
brightness_set_blocking: T::BLOCKING
@@ -309,7 +354,7 @@ pub fn new<'a>(
.map_or(core::ptr::null(), CStr::as_char_ptr),
color: init_data.color as u32,
..bindings::led_classdev::default()
- })
+ })?)
};
let mut init_data_raw = bindings::led_init_data {
@@ -326,11 +371,11 @@ pub fn new<'a>(
};
// SAFETY:
- // - `parent.as_raw()` is guaranteed to be a pointer to a valid `device`
- // or a null pointer.
- // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev`.
+ // - `parent.as_ref().as_raw()` is guaranteed to be a pointer to a valid
+ // `device`.
+ // - `ptr` is guaranteed to be a pointer to an initialized `T::Mode::Type`.
to_result(unsafe {
- bindings::led_classdev_register_ext(
+ (<T::Mode as private::Mode>::REGISTER)(
parent.as_ref().as_raw(),
ptr,
&mut init_data_raw,
@@ -350,15 +395,22 @@ pub fn new<'a>(
/// `led::Device`.
unsafe fn from_raw<'a>(led_cdev: *mut bindings::led_classdev) -> &'a Self {
// SAFETY: The function's contract guarantees that `led_cdev` points to a `led_classdev`
- // field embedded within a valid `led::Device`. `container_of!` can therefore
- // safely calculate the address of the containing struct.
- unsafe { &*container_of!(Opaque::cast_from(led_cdev), Self, classdev) }
+ // embedded within a `led::Device` and thus is embedded within `T::Mode::Type`.
+ let raw = unsafe { <T::Mode as private::Mode>::from_classdev(led_cdev) };
+
+ // SAFETY: The function's contract guarantees that `raw` points to a `led_classdev` field
+ // embedded within a valid `led::Device`. `container_of!` can therefore safely calculate
+ // the address of the containing struct.
+ unsafe { &*container_of!(Opaque::cast_from(raw), Self, classdev) }
}
fn parent(&self) -> &device::Device<Bound> {
- // SAFETY:
- // - `self.classdev.get()` is guaranteed to be a valid pointer to `led_classdev`.
- unsafe { device::Device::from_raw((*(*self.classdev.get()).dev).parent) }
+ // SAFETY: `self.classdev.get()` is guaranteed to be a valid pointer to `T::Mode::Type`.
+ let device = unsafe { <T::Mode as private::Mode>::device(self.classdev.get()) };
+ // SAFETY: `led::Device::__new` doesn't allow to register a class device without an parent.
+ let parent = unsafe { device.parent().unwrap_unchecked() };
+ // SAFETY: the existence of `self` guarantees that `parent` is bound to a driver.
+ unsafe { parent.as_bound() }
}
}
@@ -376,11 +428,17 @@ impl<T: LedOps> Adapter<T> {
brightness: u32,
) {
// SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
- // `led_classdev` embedded within a `led::Device`.
+ // `T::Mode::Type` embedded within a `led::Device`.
let classdev = unsafe { Device::<T>::from_raw(led_cdev) };
// SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
let parent = unsafe { T::Bus::from_device(classdev.parent()) };
+ // SAFETY: `classdev.classdev.get()` is guaranteed to be a valid pointer to a
+ // `T::Mode::Type`.
+ unsafe {
+ <T::Mode as private::Mode>::pre_brightness_set(classdev.classdev.get(), brightness);
+ }
+
let _ = classdev.ops.brightness_set(parent, classdev, brightness);
}
@@ -394,11 +452,17 @@ impl<T: LedOps> Adapter<T> {
) -> i32 {
from_result(|| {
// SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
- // `led_classdev` embedded within a `led::Device`.
+ // `T::Mode::Type` embedded within a `led::Device`.
let classdev = unsafe { Device::<T>::from_raw(led_cdev) };
// SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
let parent = unsafe { T::Bus::from_device(classdev.parent()) };
+ // SAFETY: `classdev.classdev.get()` is guaranteed to be a valid pointer to a
+ // `T::Mode::Type`.
+ unsafe {
+ <T::Mode as private::Mode>::pre_brightness_set(classdev.classdev.get(), brightness);
+ }
+
classdev.ops.brightness_set(parent, classdev, brightness)?;
Ok(0)
})
@@ -410,7 +474,7 @@ impl<T: LedOps> Adapter<T> {
/// This function is called on getting the brightness of a led.
unsafe extern "C" fn brightness_get_callback(led_cdev: *mut bindings::led_classdev) -> u32 {
// SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
- // `led_classdev` embedded within a `led::Device`.
+ // `T::Mode::Type` embedded within a `led::Device`.
let classdev = unsafe { Device::<T>::from_raw(led_cdev) };
// SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
let parent = unsafe { T::Bus::from_device(classdev.parent()) };
@@ -431,7 +495,7 @@ impl<T: LedOps> Adapter<T> {
) -> i32 {
from_result(|| {
// SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
- // `led_classdev` embedded within a `led::Device`.
+ // `T::Mode::Type` embedded within a `led::Device`.
let classdev = unsafe { Device::<T>::from_raw(led_cdev) };
// SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
let parent = unsafe { T::Bus::from_device(classdev.parent()) };
@@ -456,17 +520,21 @@ impl<T: LedOps> PinnedDrop for Device<T> {
fn drop(self: Pin<&mut Self>) {
let raw = self.classdev.get();
// SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
- // valid `struct led_classdev`.
- let dev: &device::Device = unsafe { device::Device::from_raw((*raw).dev) };
+ // valid `T::Mode::Type`.
+ let dev: &device::Device = unsafe { <T::Mode as private::Mode>::device(raw) };
let _fwnode = dev
.fwnode()
// SAFETY: the reference count of `fwnode` has previously been
- // incremented in `led::Device::new`.
+ // incremented in `led::Device::__new`.
.map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });
// SAFETY: The existence of `self` guarantees that `self.classdev` has previously been
- // successfully registered with `led_classdev_register_ext`.
- unsafe { bindings::led_classdev_unregister(self.classdev.get()) };
+ // successfully registered with `T::Mode::REGISTER`.
+ unsafe { (<T::Mode as private::Mode>::UNREGISTER)(raw) };
+
+ // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
+ // valid `T::Mode::Type`.
+ <T::Mode as private::Mode>::release(unsafe { &mut *raw });
}
}
diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
new file mode 100644
index 000000000000..13feeb3256f3
--- /dev/null
+++ b/rust/kernel/led/normal.rs
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Led mode for the `struct led_classdev`.
+//!
+//! C header: [`include/linux/leds.h`](srctree/include/linux/leds.h)
+
+use super::*;
+
+/// The led mode for the `struct led_classdev`. Leds with this mode can only have a fixed color.
+pub enum Normal {}
+
+impl private::Mode for Normal {
+ type Type = bindings::led_classdev;
+ const REGISTER: RegisterFunc<Self::Type> = bindings::led_classdev_register_ext;
+ const UNREGISTER: UnregisterFunc<Self::Type> = bindings::led_classdev_unregister;
+
+ unsafe fn device<'a>(raw: *mut Self::Type) -> &'a device::Device {
+ // SAFETY:
+ // - The function's contract guarantees that `raw` is a valid pointer to `led_classdev`.
+ unsafe { device::Device::from_raw((*raw).dev) }
+ }
+
+ unsafe fn from_classdev(led_cdev: *mut bindings::led_classdev) -> *mut Self::Type {
+ led_cdev
+ }
+}
+
+impl<T: LedOps<Mode = Normal>> Device<T> {
+ /// Registers a new led classdev.
+ ///
+ /// The [`Device`] will be unregistered on drop.
+ pub fn new<'a>(
+ parent: &'a T::Bus,
+ init_data: InitData<'a>,
+ ops: T,
+ ) -> impl PinInit<Devres<Self>, Error> + 'a {
+ Self::__new(parent, init_data, ops, Ok)
+ }
+}
--
2.51.0On Wed, Nov 19, 2025 at 02:11:24PM +0000, Markus Probst wrote: > Move code specific to normal led class devices into a separate file and > introduce the `led::Mode` trait to allow for other types of led class > devices. > > Signed-off-by: Markus Probst <markus.probst@posteo.de> So it seems like the goal of this trait is to support both normal led and multicolor led under the same code. However, it seems the traits involved with this are pretty complex. My primary feedback here is: please consider if we can avoid these complex traits. How much duplication would it really take to just have two Device structs and two LedOps traits? I think a few pieces of duplication would be far better than what this patch does. In fact, I'm not sure you even need two LedOps traits if you did that; it seems like you could even get away with reusing the trait for both cases. Alice
On Thu, 2025-11-20 at 10:54 +0000, Alice Ryhl wrote: > On Wed, Nov 19, 2025 at 02:11:24PM +0000, Markus Probst wrote: > > Move code specific to normal led class devices into a separate file and > > introduce the `led::Mode` trait to allow for other types of led class > > devices. > > > > Signed-off-by: Markus Probst <markus.probst@posteo.de> > > So it seems like the goal of this trait is to support both normal led > and multicolor led under the same code. However, it seems the traits > involved with this are pretty complex. > > My primary feedback here is: please consider if we can avoid these > complex traits. How much duplication would it really take to just have > two Device structs and two LedOps traits? I think a few pieces of > duplication would be far better than what this patch does. I counted over 221 lines of code for each classdev type (not counting LedOps). If we want to support `led_classdev_flash` later down the line (if a Rust driver would need it), that would be over 663 lines of code. It doesn't look too complex to me, but thats maybe because I wrote it? > > In fact, I'm not sure you even need two LedOps traits if you did that; > it seems like you could even get away with reusing the trait for both > cases. Currently it only uses one LedOps trait. The implementation in the multicolor needs access to `Device::subleds()`. `Device` gets passed as a reference to every LedOps function. If there would be two `Device` structs, I am not sure how we can preserve one LedOps trait. Possibly a type declaration on `Mode`? (assuming we still want the `Mode` trait) Thanks - Markus Probst > > Alice
© 2016 - 2025 Red Hat, Inc.