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

[Gary Guo]

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@xxxxxxxxx>
>> > ---
>> >  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?

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.

>> 
>> 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.

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?)

Best,
Gary