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

[Markus Probst]

On Mon, 2026-02-02 at 15:41 +0000, Gary Guo wrote:
> 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.
With into_raw(), the reference count of fwnode would not decrement if
the registration fails.

Thanks
- Markus Probst

> 
> 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) }
> > +    }
> > +}

Attachment: signature.asc
Description: This is a digitally signed message part