Re: [PATCH v24 1/4] rust: leds: add basic led classdev abstractions

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 4892 bytes --]

On Fri, 2026-09-04 at 14:03 +0100, Gary Guo wrote:
> On Thu Sep 3, 2026 at 12:01 AM BST, 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::DeviceBuilder` - the builder for the led class device
> > 
> > * `led::Device` - a safe wrapper around `led_classdev`
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  rust/kernel/led.rs        | 288 ++++++++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs        |   1 +
> >  3 files changed, 519 insertions(+)
> > 
> > [snip]
> > 
> > +/// 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<'bound>(
> > +///         &self,
> > +///         _dev: &'bound platform::Device<device::Bound>,
> > +///         _classdev: &led::Device<'bound, Self>,
> > +///         _brightness: u32
> > +///     ) -> Result<()> {
> > +///         // Set the brightness for the led here
> > +///         Ok(())
> > +///     }
> > +/// }
> > +/// ```
> > +/// Led drivers must implement this trait in order to register and handle a [`Device`].
> > +#[vtable]
> > +pub trait LedOps: Send + Sync + Sized {
> > +    /// The bus device required by the implementation.
> > +    #[allow(private_bounds)]
> > +    type Bus: AsBusDevice<Bound>;
> 
> Does LED class device has no private data that driver can use? This can be
> either a private pointer or extra allocation living at the end of the classdev
> struct.
On every callback `&self` is passed to the LedOps, which could be
considered the leds private data. It is currently stored in front of
the `led_classdev` struct.
> 
> It's usually a antipattern to get the bus device directly, especially that in
> Rust we do not allow anything other than callbacks to access data on bus
> devices.
> 
> Instead, the class device registration should provide a data initializer, and
> the callbacks would receive a pointer to the data instead. In cases that a
> device resource has to be referenced, it should be kept inside the private data
> by the driver themselves.
It should be possible to store a pointer to the bus device directly on
this data, thus I can remove it.

If I think about it, I could add a `led::Device::drvdata` function, so
it could be accessed from the drivers private data. Sync is a
requirement anyway.

Thanks
- Markus Probst

> 
> Best,
> Gary
> 
> > +
> > +    /// 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<'bound>(
> > +        &self,
> > +        dev: &'bound Self::Bus,
> > +        classdev: &Device<'bound, Self>,
> > +        brightness: u32,
> > +    ) -> Result<()>;
> > +
> > +    /// Gets the current brightness level.
> > +    fn brightness_get<'bound>(
> > +        &self,
> > +        dev: &'bound Self::Bus,
> > +        classdev: &Device<'bound, Self>,
> > +    ) -> Result<u32> {
> > +        let _ = (dev, classdev);
> > +        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<'bound>(
> > +        &self,
> > +        dev: &'bound Self::Bus,
> > +        classdev: &Device<'bound, Self>,
> > +        delay_on: &mut usize,
> > +        delay_off: &mut usize,
> > +    ) -> Result<()> {
> > +        let _ = (dev, classdev, delay_on, delay_off);
> > +        build_error!(VTABLE_DEFAULT_ERROR)
> > +    }
> > +}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]