Re: [PATCH v11 1/3] rust: leds: add basic led classdev abstractions

[Gary Guo]

On Mon Feb 2, 2026 at 1:52 PM GMT, Markus Probst wrote:
> Implement the core abstractions needed for led class devices, including:
>
> * `led::LedOps` - the trait for handling leds, including
>   `brightness_set`, `brightness_get` and `blink_set`
>
> * `led::InitData` - data set for the led class device
>
> * `led::Device` - a safe wrapper around `led_classdev`
>
> Signed-off-by: Markus Probst <markus.probst@xxxxxxxxx>
> ---
>  MAINTAINERS        |   7 +
>  rust/kernel/led.rs | 453 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs |   1 +
>  3 files changed, 461 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0efa8cc6775b..26765fecb9a9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14279,6 +14279,13 @@ F:	drivers/leds/
>  F:	include/dt-bindings/leds/
>  F:	include/linux/leds.h
>  
> +LED SUBSYSTEM [RUST]
> +M:	Markus Probst <markus.probst@xxxxxxxxx>
> +L:	linux-leds@xxxxxxxxxxxxxxx
> +L:	rust-for-linux@xxxxxxxxxxxxxxx
> +S:	Maintained
> +F:	rust/kernel/led.rs
> +
>  LEGO MINDSTORMS EV3
>  R:	David Lechner <david@xxxxxxxxxxxxxx>
>  S:	Maintained
> diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> new file mode 100644
> index 000000000000..9acb6946f3da
> --- /dev/null
> +++ b/rust/kernel/led.rs
> @@ -0,0 +1,453 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions for the leds driver model.
> +//!
> +//! C header: [`include/linux/leds.h`](srctree/include/linux/leds.h)
> +
> +use core::{
> +    marker::PhantomData,
> +    mem::transmute,
> +    ptr::NonNull, //
> +};
> +
> +use crate::{
> +    container_of,
> +    device::{
> +        self,
> +        property::FwNode,
> +        AsBusDevice,
> +        Bound, //
> +    },
> +    devres::Devres,
> +    error::{
> +        from_result,
> +        to_result,
> +        VTABLE_DEFAULT_ERROR, //
> +    },
> +    macros::vtable,
> +    prelude::*,
> +    str::CStrExt,
> +    types::{
> +        ARef,
> +        Opaque, //
> +    }, //
> +};
> +
> +/// The led class device representation.
> +///
> +/// This structure represents the Rust abstraction for a C `struct led_classdev`.
> +#[pin_data(PinnedDrop)]
> +pub struct Device<T: LedOps> {
> +    #[pin]
> +    ops: T,
> +    #[pin]
> +    classdev: Opaque<bindings::led_classdev>,
> +}
> +
> +/// The led init data representation.
> +///
> +/// This structure represents the Rust abstraction for a C `struct led_init_data` with additional
> +/// fields from `struct led_classdev`.
> +#[derive(Default)]
> +pub struct InitData<'a> {
> +    fwnode: Option<ARef<FwNode>>,
> +    devicename: Option<&'a CStr>,
> +    devname_mandatory: bool,
> +    initial_brightness: u32,
> +    default_trigger: Option<&'a CStr>,
> +    color: Color,
> +}

It appears to me that while this reflects on the C API, on the Rust side this is
more commonly known as the builder pattern.

I think this should properly be name `led::DeviceBuilder`, as it does more than
what `led_init_data` does on the C side (e.g. initial_brightness).

Perhaps the device creation can be part of this too, e.g.

    LedDeviceBuilder::new()
        .fwnode(...)
        .devicename(...)
        .initial_brightness(...)
        .build(parent, ops)

?


> +
> +impl InitData<'static> {
> +    /// Creates a new [`InitData`].
> +    pub fn new() -> Self {
> +        Self::default()
> +    }
> +}
> +
> +impl<'a> InitData<'a> {
> +    /// Sets the firmware node.
> +    pub fn fwnode(self, fwnode: Option<ARef<FwNode>>) -> Self {
> +        Self { fwnode, ..self }
> +    }
> +
> +    /// Sets the device name.
> +    pub fn devicename(self, devicename: &'a CStr) -> Self {
> +        Self {
> +            devicename: Some(devicename),
> +            ..self
> +        }
> +    }
> +
> +    /// Sets if a device name is mandatory.
> +    pub fn devicename_mandatory(self, mandatory: bool) -> Self {
> +        Self {
> +            devname_mandatory: mandatory,
> +            ..self
> +        }
> +    }
> +
> +    /// Sets the initial brightness value for the led.
> +    ///
> +    /// The default brightness is 0.
> +    /// If [`LedOps::brightness_get`] is implemented, this value will be ignored.
> +    pub fn initial_brightness(self, brightness: u32) -> Self {
> +        Self {
> +            initial_brightness: brightness,
> +            ..self
> +        }
> +    }
> +
> +    /// Set the default led trigger.
> +    ///
> +    /// This value can be overwritten by the "linux,default-trigger" fwnode property.
> +    pub fn default_trigger(self, trigger: &'a CStr) -> Self {
> +        Self {
> +            default_trigger: Some(trigger),
> +            ..self
> +        }
> +    }
> +
> +    /// Sets the color of the led.
> +    ///
> +    /// This value can be overwritten by the "color" fwnode property.
> +    pub fn color(self, color: Color) -> Self {
> +        Self { color, ..self }
> +    }
> +}
> +
> +/// Trait defining the operations for a LED driver.
> +///
> +/// # Examples
> +/// ```
> +/// use kernel::{
> +///      device,
> +///      devres::Devres,
> +///      led,
> +///      macros::vtable,
> +///      platform,
> +///      prelude::*, //
> +///  };
> +///
> +/// struct MyLedOps;
> +///
> +///
> +/// #[vtable]
> +/// impl led::LedOps for MyLedOps {
> +///     type Bus = platform::Device<device::Bound>;
> +///     const BLOCKING: bool = false;
> +///     const MAX_BRIGHTNESS: u32 = 255;
> +///
> +///     fn brightness_set(
> +///         &self,
> +///         _dev: &platform::Device<device::Bound>,
> +///         _classdev: &led::Device<Self>,
> +///         _brightness: u32
> +///     ) -> Result<()> {
> +///         // Set the brightness for the led here
> +///         Ok(())
> +///     }
> +/// }
> +///
> +/// fn register_my_led(
> +///     parent: &platform::Device<device::Bound>,
> +/// ) -> Result<Pin<KBox<Devres<led::Device<MyLedOps>>>>> {
> +///     KBox::pin_init(led::Device::new(
> +///         parent,
> +///         led::InitData::new(),
> +///         Ok(MyLedOps),
> +///     ), GFP_KERNEL)
> +/// }
> +/// ```
> +/// Led drivers must implement this trait in order to register and handle a [`Device`].
> +#[vtable]
> +pub trait LedOps: Send + 'static + Sized {
> +    /// The bus device required by the implementation.
> +    #[allow(private_bounds)]
> +    type Bus: AsBusDevice<Bound>;
> +    /// 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;
> +    /// The max brightness level.
> +    const MAX_BRIGHTNESS: u32;
> +
> +    /// Sets the brightness level.
> +    ///
> +    /// See also [`LedOps::BLOCKING`].
> +    fn brightness_set(
> +        &self,
> +        dev: &Self::Bus,
> +        classdev: &Device<Self>,
> +        brightness: u32,
> +    ) -> Result<()>;
> +
> +    /// Gets the current brightness level.
> +    fn brightness_get(&self, _dev: &Self::Bus, _classdev: &Device<Self>) -> u32 {
> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }
> +
> +    /// Activates hardware accelerated blinking.
> +    ///
> +    /// delays are in milliseconds. If both are zero, a sensible default should be chosen.
> +    /// The caller should adjust the timings in that case and if it can't match the values
> +    /// specified exactly. Setting the brightness to 0 will disable the hardware accelerated
> +    /// blinking.
> +    ///
> +    /// See also [`LedOps::BLOCKING`].
> +    fn blink_set(
> +        &self,
> +        _dev: &Self::Bus,
> +        _classdev: &Device<Self>,
> +        _delay_on: &mut usize,
> +        _delay_off: &mut usize,
> +    ) -> Result<()> {
> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }
> +}
> +
> +/// Led colors.
> +#[derive(Copy, Clone, Debug, Default)]
> +#[repr(u32)]
> +#[non_exhaustive]
> +#[expect(
> +    missing_docs,
> +    reason = "it shouldn't be necessary to document each color"
> +)]
> +pub enum Color {
> +    #[default]
> +    White = bindings::LED_COLOR_ID_WHITE,
> +    Red = bindings::LED_COLOR_ID_RED,
> +    Green = bindings::LED_COLOR_ID_GREEN,
> +    Blue = bindings::LED_COLOR_ID_BLUE,
> +    Amber = bindings::LED_COLOR_ID_AMBER,
> +    Violet = bindings::LED_COLOR_ID_VIOLET,
> +    Yellow = bindings::LED_COLOR_ID_YELLOW,
> +    Ir = bindings::LED_COLOR_ID_IR,
> +    Multi = bindings::LED_COLOR_ID_MULTI,
> +    Rgb = bindings::LED_COLOR_ID_RGB,
> +    Purple = bindings::LED_COLOR_ID_PURPLE,
> +    Orange = bindings::LED_COLOR_ID_ORANGE,
> +    Pink = bindings::LED_COLOR_ID_PINK,
> +    Cyan = bindings::LED_COLOR_ID_CYAN,
> +    Lime = bindings::LED_COLOR_ID_LIME,
> +}
> +
> +impl TryFrom<u32> for Color {
> +    type Error = Error;
> +
> +    fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {
> +        const _: () = {
> +            assert!(bindings::LED_COLOR_ID_MAX == 15);
> +        };

`static_assert!()` and move this out from the impl.

> +        if value < bindings::LED_COLOR_ID_MAX {
> +            // SAFETY:
> +            // - `Color` is represented as `u32`
> +            // - the const block above guarantees that no additional color has been added
> +            // - `value` is guaranteed to be in the color id range
> +            Ok(unsafe { transmute::<u32, Color>(value) })
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
> +}
> +
> +// SAFETY: A `led::Device` can be unregistered from any thread.
> +unsafe impl<T: LedOps + Send> Send for Device<T> {}
> +
> +// SAFETY: `led::Device` can be shared among threads because all methods of `led::Device`
> +// are thread safe.
> +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>(
> +        parent: &'a T::Bus,
> +        init_data: InitData<'a>,
> +        ops: impl PinInit<T, Error> + 'a,
> +    ) -> impl PinInit<Devres<Self>, Error> + 'a {
> +        Devres::new(
> +            parent.as_ref(),
> +            try_pin_init!(Self {
> +                ops <- ops,
> +                classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
> +                    // 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`.
> +                    unsafe {
> +                        ptr.write(bindings::led_classdev {
> +                            brightness_set: (!T::BLOCKING)
> +                                .then_some(Adapter::<T>::brightness_set_callback),
> +                            brightness_set_blocking: T::BLOCKING
> +                                .then_some(Adapter::<T>::brightness_set_blocking_callback),
> +                            brightness_get: T::HAS_BRIGHTNESS_GET
> +                                .then_some(Adapter::<T>::brightness_get_callback),
> +                            blink_set: T::HAS_BLINK_SET.then_some(Adapter::<T>::blink_set_callback),
> +                            max_brightness: T::MAX_BRIGHTNESS,
> +                            brightness: init_data.initial_brightness,
> +                            default_trigger: init_data
> +                                .default_trigger
> +                                .map_or(core::ptr::null(), CStrExt::as_char_ptr),
> +                            color: init_data.color as u32,
> +                            ..bindings::led_classdev::default()
> +                        })
> +                    };
> +
> +                    let mut init_data_raw = bindings::led_init_data {
> +                        fwnode: init_data
> +                            .fwnode
> +                            .as_ref()
> +                            .map_or(core::ptr::null_mut(), |fwnode| fwnode.as_raw()),

This should be `fwnode.into_raw()` which directly takes the ownership for
`ARef`, rather than `as_raw()` and forget the `ARef` later.

Best,
Gary

> +                        default_label: core::ptr::null(),
> +                        devicename: init_data
> +                            .devicename
> +                            .map_or(core::ptr::null(), CStrExt::as_char_ptr),
> +                        devname_mandatory: init_data.devname_mandatory,
> +                    };
> +
> +                    // 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`.
> +                    to_result(unsafe {
> +                        bindings::led_classdev_register_ext(
> +                            parent.as_ref().as_raw(),
> +                            ptr,
> +                            &raw mut init_data_raw,
> +                        )
> +                    })?;
> +
> +                    core::mem::forget(init_data.fwnode); // keep the reference count incremented
> +
> +                    Ok::<_, Error>(())
> +                }),
> +            }),
> +        )
> +    }
> +
> +    /// # Safety
> +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within 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) }
> +    }
> +
> +    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) }
> +    }
> +}