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

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:

quoted

On Fri, 2026-09-04 at 14:03 +0100, Gary Guo wrote:

quoted

On Thu Sep 3, 2026 at 12:01 AM BST, Markus Probst wrote:

quoted

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/ (local)

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

quoted

quoted

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