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

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

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

On Fri, 2026-09-04 at 14:32 +0100, Gary Guo wrote:
> On Fri Sep 4, 2026 at 2:15 PM BST, Markus Probst wrote:
> > 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.
> 
> Right, I missed that. In that case I think you can just remove `Bus` completely
> from the callback?
Yes.

> 
> Do you have a user that needs this info? BTW, it would also help to include a
> link to a potential user in the cover letter so people can see how the API is
> supposed to be used. This is especially useful for API design reviews.
Primarily
https://lore.kernel.org/rust-for-linux/20260724-synology_microp_initial-v18-0-fb2f49f10e77@posteo.de/

.
But I also have another rust i2c driver, which would use the led
abstraction:

https://codeberg.org/0xIO32/linux/src/branch/synology_disk_leds

Still needs changes before it can be submitted.


> 
> > > 
> > > 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.
> 
> An option is to provide `Deref`. Then you could even have
> `self: &Device<'bound, Self>` in callbacks.
Didn't know we had `feature(arbitrary_self_types)` enabled.
I like this idea.

> 
> That said, you might want to eventually support type-erased `Device` types to
> support consumer of LED class devices. So I'm unsure if we want to provide data
> accessors on class devices (maybe eventually device'll be split into two types?)
Sounds good, but I won't implement any consumer functions until they
are needed.

Thanks
- Markus Probst

> 
> Best,
> Gary

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