Re: [PATCH 2/2] rust: leds: add basic led classdev abstractions

["Danilo Krummrich" <dakr@kernel.org>]

On Thu Oct 9, 2025 at 7:07 PM CEST, Markus Probst wrote:
> +/// The led class device representation.
> +///
> +/// This structure represents the Rust abstraction for a C `struct led_classdev`.
> +#[pin_data(PinnedDrop)]
> +pub struct Device {
> +    handler: KBox<dyn HandlerImpl>,

The corresponding callbacks already exist in struct led_classdev, please use
them instead.

> +    #[pin]
> +    classdev: Opaque<bindings::led_classdev>,
> +}

<snip>

> +// SAFETY: A `led::Device` can be unregistered from any thread.
> +unsafe impl Send for Device {}
> +
> +// SAFETY: `led::Device` can be shared among threads because all methods of `led::Device`
> +// are thread safe.
> +unsafe impl Sync for Device {}
> +
> +impl Device {
> +    /// Registers a new led classdev.
> +    ///
> +    /// The [`Device`] will be unregistered and drop.
> +    pub fn new<'a, T: Handler + 'static>(
> +        parent: Option<&'a device::Device>,

This should not be an Option, all LED devices should have a parent (bus) device.

Also, it should be a &Device<Bound>, i.e. a device that is guaranteed to be
bound to a driver.

> +        init_data: InitData<'a>,
> +        handler: T,
> +    ) -> impl PinInit<Self, Error> + use<'a, T> {

This will not compile for all supported compiler versions. For now it just has
to be:

	impl PinInit<Self, Error> + 'a

Besides that, this should return impl PinInit<Devres<Self>, Error> + 'a instead.
This will allow you to let callbacks, such as blink_set(), to provide the parent
device as &Device<Bound>.

Please see also the PWM abstractions for reference [1]. There's one difference between
LED and PWM though. Unlike PWM (and most other class device implementations) LED
combines initialization and registration of the device, hence PWM is slightly
different in its implementation details, but semantically it's the same.

[1] https://lore.kernel.org/all/20250930-rust-next-pwm-working-fan-for-sending-v15-3-5661c3090877@samsung.com/

> +        try_pin_init!(Self {
> +            handler <- {
> +                let handler: KBox<dyn HandlerImpl> = KBox::<T>::new(handler, GFP_KERNEL)?;
> +                Ok::<_, Error>(handler)
> +            },
> +            classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
> +                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
> +                unsafe { ptr.write(bindings::led_classdev {

This looks very odd at a first glance, but it is indeed correct. It would be
good to add a comment that the parts of struct led_classdev that need to be
initialized in place (e.g. the struct mutex and struct list_head) are
initialized by led_classdev_register_ext() (which is not exactly straight
forward).

> +                    max_brightness: T::MAX_BRIGHTNESS,
> +                    brightness_set: T::BLOCKING.then_some(
> +                        brightness_set as unsafe extern "C" fn(*mut bindings::led_classdev, u32)
> +                    ),
> +                    brightness_set_blocking: (!T::BLOCKING).then_some(
> +                        brightness_set_blocking
> +                            as unsafe extern "C" fn(*mut bindings::led_classdev,u32) -> i32
> +                    ),
> +                    blink_set: T::BLINK.then_some(
> +                        blink_set
> +                            as unsafe extern "C" fn(*mut bindings::led_classdev, *mut usize,
> +                                                    *mut usize) -> i32
> +                    ),
> +                    .. bindings::led_classdev::default()
> +                }) };
> +
> +                let mut init_data = bindings::led_init_data {
> +                    fwnode: init_data.fwnode.map_or(core::ptr::null_mut(), FwNode::as_raw),
> +                    default_label: init_data.default_label
> +                                            .map_or(core::ptr::null(), CStr::as_char_ptr),
> +                    devicename: init_data.devicename.map_or(core::ptr::null(), CStr::as_char_ptr),
> +                    devname_mandatory: init_data.devname_mandatory,
> +                };
> +
> +                let parent = parent
> +                    .map_or(core::ptr::null_mut(), device::Device::as_raw);
> +
> +                // SAFETY:
> +                // - `parent` 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, ptr, &mut init_data)
> +                })
> +            }),
> +        })
> +    }
> +}