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::InitData` - data set 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 | 333 +++++++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
2 files changed, 334 insertions(+)
create mode 100644 rust/kernel/led.rs
diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
new file mode 100644
index 000000000000..4261c2f5a903
--- /dev/null
+++ b/rust/kernel/led.rs
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the leds driver model.
+//!
+//! C header: [`include/linux/leds.h`](srctree/include/linux/leds.h)
+
+use core::{marker::PhantomData, pin::Pin, ptr::NonNull};
+
+use pin_init::{pin_data, pinned_drop, PinInit};
+
+use crate::{
+ build_error, container_of,
+ device::{self, property::FwNode, Bound},
+ devres::Devres,
+ error::{from_result, to_result, Error, Result, VTABLE_DEFAULT_ERROR},
+ macros::vtable,
+ str::CStr,
+ try_pin_init,
+ types::{ARef, Opaque},
+};
+
+/// The led class device representation.
+///
+/// This structure represents the Rust abstraction for a C `struct led_classdev`.
+#[pin_data(PinnedDrop)]
+pub struct Device<T: LedOps> {
+ ops: T,
+ #[pin]
+ classdev: Opaque<bindings::led_classdev>,
+}
+
+/// The led init data representation.
+///
+/// This structure represents the Rust abstraction for a C `struct led_init_data`.
+#[derive(Default)]
+pub struct InitData<'a> {
+ fwnode: Option<&'a FwNode>,
+ default_label: Option<&'a CStr>,
+ devicename: Option<&'a CStr>,
+ devname_mandatory: bool,
+}
+
+impl InitData<'static> {
+ /// Creates a new [`InitData`]
+ pub fn new() -> Self {
+ Self::default()
+ }
+}
+
+impl<'a> InitData<'a> {
+ /// Sets the firmware node
+ pub fn fwnode<'b, 'c>(self, fwnode: &'b FwNode) -> InitData<'c>
+ where
+ 'a: 'c,
+ 'b: 'c,
+ {
+ InitData {
+ fwnode: Some(fwnode),
+ ..self
+ }
+ }
+
+ /// Sets a default label
+ pub fn default_label<'b, 'c>(self, label: &'b CStr) -> InitData<'c>
+ where
+ 'a: 'c,
+ 'b: 'c,
+ {
+ InitData {
+ default_label: Some(label),
+ ..self
+ }
+ }
+
+ /// Sets the device name
+ pub fn devicename<'b, 'c>(self, devicename: &'b CStr) -> InitData<'c>
+ where
+ 'a: 'c,
+ 'b: 'c,
+ {
+ InitData {
+ devicename: Some(devicename),
+ ..self
+ }
+ }
+
+ /// Sets if a device name is mandatory
+ pub fn devicename_mandatory(self, mandatory: bool) -> Self {
+ Self {
+ devname_mandatory: mandatory,
+
+ ..self
+ }
+ }
+}
+
+/// Trait defining the operations for a LED driver.
+///
+/// # Examples
+///
+///```
+/// # use kernel::{c_str, led, device, devres::Devres, prelude::*, error::Result, macros::vtable};
+/// # use core::pin::Pin;
+///
+/// struct MyLedOps;
+///
+///
+/// #[vtable]
+/// impl led::LedOps for MyLedOps {
+/// const BLOCKING: bool = false;
+/// const MAX_BRIGHTNESS: u32 = 255;
+///
+/// fn brightness_set(&self, _brightness: u32) -> Result<()> {
+/// // Set the brightness for the led here
+/// Ok(())
+/// }
+/// }
+///
+/// fn register_my_led(
+/// parent: &device::Device<device::Bound>,
+/// ) -> Result<Pin<KBox<Devres<led::Device<MyLedOps>>>>> {
+/// KBox::pin_init(led::Device::new(
+/// parent,
+/// led::InitData::new()
+/// .default_label(c_str!("my_led")),
+/// MyLedOps,
+/// ), GFP_KERNEL)
+/// }
+///```
+/// Led drivers must implement this trait in order to register and handle a [`Device`].
+#[vtable]
+pub trait LedOps: Send + 'static + Sized {
+ /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must not sleep
+ /// and perform the operation immediately.
+ const BLOCKING: bool;
+ /// The max brightness level
+ const MAX_BRIGHTNESS: u32;
+
+ /// Sets the brightness level.
+ ///
+ /// See also [`LedOps::BLOCKING`]
+ fn brightness_set(&self, brightness: u32) -> Result<()>;
+
+ /// Gets the current brightness level.
+ fn brightness_get(&self) -> u32 {
+ 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(&self, _delay_on: &mut usize, _delay_off: &mut usize) -> Result<()> {
+ build_error!(VTABLE_DEFAULT_ERROR)
+ }
+}
+
+// SAFETY: A `led::Device` can be unregistered from any thread.
+unsafe impl<T: LedOps + Send> Send for Device<T> {}
+
+// SAFETY: `led::Device` can be shared among threads because all methods of `led::Device`
+// are thread safe.
+unsafe impl<T: LedOps + Sync> Sync for Device<T> {}
+
+impl<T: LedOps> Device<T> {
+ /// Registers a new led classdev.
+ ///
+ /// The [`Device`] will be unregistered on drop.
+ pub fn new<'a>(
+ parent: &'a device::Device<Bound>,
+ init_data: InitData<'a>,
+ ops: T,
+ ) -> impl PinInit<Devres<Self>, Error> + 'a {
+ Devres::new(
+ parent,
+ try_pin_init!(Self {
+ ops,
+ classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
+ // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
+ // `led_classdev` gets fully initialized in-place by
+ // `led_classdev_register_ext` including `mutex` and `list_head`.
+ unsafe { ptr.write(bindings::led_classdev {
+ max_brightness: T::MAX_BRIGHTNESS,
+ brightness_set: T::BLOCKING
+ .then_some(Adapter::<T>::brightness_set_callback),
+ brightness_set_blocking: (!T::BLOCKING)
+ .then_some(Adapter::<T>::brightness_set_blocking_callback),
+ brightness_get: T::HAS_BRIGHTNESS_GET
+ .then_some(Adapter::<T>::brightness_get_callback),
+ blink_set: T::HAS_BLINK_SET
+ .then_some(Adapter::<T>::blink_set_callback),
+ .. bindings::led_classdev::default()
+ }) };
+
+ let fwnode = init_data.fwnode.map(ARef::from);
+
+ let mut init_data = bindings::led_init_data {
+ fwnode: fwnode.as_ref()
+ .map_or(core::ptr::null_mut(), |fwnode| 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,
+ };
+
+ // SAFETY:
+ // - `parent.as_raw()` 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.as_raw(), ptr, &mut init_data)
+ })?;
+
+ core::mem::forget(fwnode); // keep the reference count incremented
+
+ Ok::<_, Error>(())
+ }),
+ }),
+ )
+ }
+
+ /// # Safety
+ /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+ /// `led::Device`.
+ unsafe fn from_raw<'a>(led_cdev: *mut bindings::led_classdev) -> &'a Self {
+ // SAFETY: The function's contract guarantees that `led_cdev` points to a `led_classdev`
+ // field embedded within a valid `led::Device`. `container_of!` can therefore
+ // safely calculate the address of the containing struct.
+ unsafe { &*container_of!(Opaque::cast_from(led_cdev), Self, classdev) }
+ }
+}
+
+struct Adapter<T: LedOps> {
+ _p: PhantomData<T>,
+}
+
+impl<T: LedOps> Adapter<T> {
+ /// # Safety
+ /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+ /// `led::Device`.
+ /// This function is called on setting the brightness of a led.
+ unsafe extern "C" fn brightness_set_callback(
+ led_cdev: *mut bindings::led_classdev,
+ brightness: u32,
+ ) {
+ // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
+ // `led_classdev` embedded within a `led::Device`.
+ let _ = unsafe { Device::<T>::from_raw(led_cdev) }
+ .ops
+ .brightness_set(brightness);
+ }
+
+ /// # Safety
+ /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+ /// `led::Device`.
+ /// This function is called on setting the brightness of a led immediately.
+ unsafe extern "C" fn brightness_set_blocking_callback(
+ led_cdev: *mut bindings::led_classdev,
+ brightness: u32,
+ ) -> i32 {
+ from_result(|| {
+ // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
+ // `led_classdev` embedded within a `led::Device`.
+ unsafe { Device::<T>::from_raw(led_cdev) }
+ .ops
+ .brightness_set(brightness)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+ /// `led::Device`.
+ /// This function is called on getting the brightness of a led.
+ unsafe extern "C" fn brightness_get_callback(led_cdev: *mut bindings::led_classdev) -> u32 {
+ // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
+ // `led_classdev` embedded within a `led::Device`.
+ unsafe { Device::<T>::from_raw(led_cdev) }
+ .ops
+ .brightness_get()
+ }
+
+ /// # Safety
+ /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+ /// `led::Device`.
+ /// `delay_on` and `delay_off` must be valid pointers to `usize` and have
+ /// exclusive access for the period of this function.
+ /// This function is called on enabling hardware accelerated blinking.
+ unsafe extern "C" fn blink_set_callback(
+ led_cdev: *mut bindings::led_classdev,
+ delay_on: *mut usize,
+ delay_off: *mut usize,
+ ) -> i32 {
+ from_result(|| {
+ // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
+ // `led_classdev` embedded within a `led::Device`.
+ unsafe { Device::<T>::from_raw(led_cdev) }.ops.blink_set(
+ // SAFETY: The function's contract guarantees that `delay_on` points to a `usize`
+ // and is exclusive for the period of this function.
+ unsafe { &mut *delay_on },
+ // SAFETY: The function's contract guarantees that `delay_off` points to a `usize`
+ // and is exclusive for the period of this function.
+ unsafe { &mut *delay_off },
+ )?;
+ Ok(0)
+ })
+ }
+}
+
+#[pinned_drop]
+impl<T: LedOps> PinnedDrop for Device<T> {
+ fn drop(self: Pin<&mut Self>) {
+ let raw = self.classdev.get();
+ // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
+ // valid `struct led_classdev`.
+ let dev: &device::Device = unsafe { device::Device::from_raw((*raw).dev) };
+
+ let _fwnode = dev
+ .fwnode()
+ // SAFETY: the reference count of `fwnode` has previously been
+ // incremented in `led::Device::new`.
+ .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });
+
+ // SAFETY: The existence of `self` guarantees that `self.classdev` has previously been
+ // successfully registered with `led_classdev_register_ext`.
+ unsafe { bindings::led_classdev_unregister(self.classdev.get()) };
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e5247f584ad2..f42c60da21ae 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -97,6 +97,7 @@
pub mod jump_label;
#[cfg(CONFIG_KUNIT)]
pub mod kunit;
+pub mod led;
pub mod list;
pub mod miscdevice;
pub mod mm;
--
2.49.1On Sun, Oct 12, 2025 at 02:52:39PM +0000, 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::InitData` - data set for the led class device
>
> * `led::Device` - a safe wrapper around `led_classdev`
>
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> +pub trait LedOps: Send + 'static + Sized {
> + /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must not sleep
> + /// and perform the operation immediately.
> + const BLOCKING: bool;
> + /// The max brightness level
> + const MAX_BRIGHTNESS: u32;
> +
> + /// Sets the brightness level.
> + ///
> + /// See also [`LedOps::BLOCKING`]
> + fn brightness_set(&self, brightness: u32) -> Result<()>;
> +
> + /// Gets the current brightness level.
> + fn brightness_get(&self) -> u32 {
> + 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(&self, _delay_on: &mut usize, _delay_off: &mut usize) -> Result<()> {
> + build_error!(VTABLE_DEFAULT_ERROR)
> + }
These functions should probably take a &Device<Bound> argument so that
they can use methods that require a bound device (such as IO).
> +impl<T: LedOps> Device<T> {
> + /// Registers a new led classdev.
> + ///
> + /// The [`Device`] will be unregistered on drop.
> + pub fn new<'a>(
> + parent: &'a device::Device<Bound>,
> + init_data: InitData<'a>,
> + ops: T,
> + ) -> impl PinInit<Devres<Self>, Error> + 'a {
> + Devres::new(
> + parent,
> + try_pin_init!(Self {
> + ops,
> + classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
> + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
> + // `led_classdev` gets fully initialized in-place by
> + // `led_classdev_register_ext` including `mutex` and `list_head`.
> + unsafe { ptr.write(bindings::led_classdev {
> + max_brightness: T::MAX_BRIGHTNESS,
> + brightness_set: T::BLOCKING
> + .then_some(Adapter::<T>::brightness_set_callback),
> + brightness_set_blocking: (!T::BLOCKING)
> + .then_some(Adapter::<T>::brightness_set_blocking_callback),
> + brightness_get: T::HAS_BRIGHTNESS_GET
> + .then_some(Adapter::<T>::brightness_get_callback),
> + blink_set: T::HAS_BLINK_SET
> + .then_some(Adapter::<T>::blink_set_callback),
> + .. bindings::led_classdev::default()
> + }) };
This doesn't look like something rustfmt would output? Could you run
rustfmt if you haven't already. If it doesn't do anything, then please
format it outside of the macro and move it back in.
Alice
On Mon, 2025-10-13 at 18:34 +0000, Alice Ryhl wrote:
> On Sun, Oct 12, 2025 at 02:52:39PM +0000, 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::InitData` - data set for the led class device
> >
> > * `led::Device` - a safe wrapper around `led_classdev`
> >
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
>
> > +pub trait LedOps: Send + 'static + Sized {
> > + /// If set true, [`LedOps::brightness_set`] and
> > [`LedOps::blink_set`] must not sleep
> > + /// and perform the operation immediately.
> > + const BLOCKING: bool;
> > + /// The max brightness level
> > + const MAX_BRIGHTNESS: u32;
> > +
> > + /// Sets the brightness level.
> > + ///
> > + /// See also [`LedOps::BLOCKING`]
> > + fn brightness_set(&self, brightness: u32) -> Result<()>;
> > +
> > + /// Gets the current brightness level.
> > + fn brightness_get(&self) -> u32 {
> > + 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(&self, _delay_on: &mut usize, _delay_off: &mut
> > usize) -> Result<()> {
> > + build_error!(VTABLE_DEFAULT_ERROR)
> > + }
>
> These functions should probably take a &Device<Bound> argument so
> that
> they can use methods that require a bound device (such as IO).
How about instead something like
mod device {
unsafe trait Container<Ctx: DeviceContext>: AsRef<Device<Ctx>> {
const Offset: usize;
unsafe fn from_device(dev: &Device<Ctx>) -> &Self {
<implementation here>
}
}
unsafe impl Device<Ctx> for Container<Ctx> {
const Offset: usize = 0;
}
}
And instead of passing &Device<Bound> to the functions, we should add a
type parameter to LedOps, e.g.:
trait LedOps<T: device::Container<device::Bound>> {
...
fn brightness_set(&self, dev: &T, brightness: u32) -> Result<()>;
...
}
impl<T: LedOps<E>, E: device::Container<device::Bound>> Device<T> {
pub fn new<'a>(
parent: &'a E,
init_data: InitData<'a>,
ops: T,
) -> impl PinInit<Devres<Self>, Error> + 'a {
...
}
...
}
In the example of i2c (or any other container for `struct device`), we
implement the device::Container trait:
mod i2c {
unsafe impl device::Container for I2cClient {
const Offset: usize = offset_of!(bindings::i2c_client, dev);
}
}
This allows the LedOps function to use any functions from the I2cClient
or any other device container which may be used (removing the need to
store it inside the LedOps implementations struct). It still allows
Device<Bound> to be used, as it also would implement device::Container.
Thanks
- Markus Probst
>
> > +impl<T: LedOps> Device<T> {
> > + /// Registers a new led classdev.
> > + ///
> > + /// The [`Device`] will be unregistered on drop.
> > + pub fn new<'a>(
> > + parent: &'a device::Device<Bound>,
> > + init_data: InitData<'a>,
> > + ops: T,
> > + ) -> impl PinInit<Devres<Self>, Error> + 'a {
> > + Devres::new(
> > + parent,
> > + try_pin_init!(Self {
> > + ops,
> > + classdev <- Opaque::try_ffi_init(|ptr: *mut
> > bindings::led_classdev| {
> > + // SAFETY: `try_ffi_init` guarantees that
> > `ptr` is valid for write.
> > + // `led_classdev` gets fully initialized in-
> > place by
> > + // `led_classdev_register_ext` including
> > `mutex` and `list_head`.
> > + unsafe { ptr.write(bindings::led_classdev {
> > + max_brightness: T::MAX_BRIGHTNESS,
> > + brightness_set: T::BLOCKING
> > +
> > .then_some(Adapter::<T>::brightness_set_callback),
> > + brightness_set_blocking: (!T::BLOCKING)
> > +
> > .then_some(Adapter::<T>::brightness_set_blocking_callback),
> > + brightness_get: T::HAS_BRIGHTNESS_GET
> > +
> > .then_some(Adapter::<T>::brightness_get_callback),
> > + blink_set: T::HAS_BLINK_SET
> > +
> > .then_some(Adapter::<T>::blink_set_callback),
> > + .. bindings::led_classdev::default()
> > + }) };
>
> This doesn't look like something rustfmt would output? Could you run
> rustfmt if you haven't already. If it doesn't do anything, then
> please
> format it outside of the macro and move it back in.
>
> Alice
On Wed Oct 15, 2025 at 3:44 PM CEST, Markus Probst wrote:
> On Mon, 2025-10-13 at 18:34 +0000, Alice RyhlV wrote:
>> On Sun, Oct 12, 2025 at 02:52:39PM +0000, 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::InitData` - data set for the led class device
>> >
>> > * `led::Device` - a safe wrapper around `led_classdev`
>> >
>> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
>>
>> > +pub trait LedOps: Send + 'static + Sized {
>> > + /// If set true, [`LedOps::brightness_set`] and
>> > [`LedOps::blink_set`] must not sleep
>> > + /// and perform the operation immediately.
>> > + const BLOCKING: bool;
>> > + /// The max brightness level
>> > + const MAX_BRIGHTNESS: u32;
>> > +
>> > + /// Sets the brightness level.
>> > + ///
>> > + /// See also [`LedOps::BLOCKING`]
>> > + fn brightness_set(&self, brightness: u32) -> Result<()>;
>> > +
>> > + /// Gets the current brightness level.
>> > + fn brightness_get(&self) -> u32 {
>> > + 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(&self, _delay_on: &mut usize, _delay_off: &mut
>> > usize) -> Result<()> {
>> > + build_error!(VTABLE_DEFAULT_ERROR)
>> > + }
>>
>> These functions should probably take a &Device<Bound> argument so
>> that
>> they can use methods that require a bound device (such as IO).
> How about instead something like
>
> mod device {
>
> unsafe trait Container<Ctx: DeviceContext>: AsRef<Device<Ctx>> {
> const Offset: usize;
>
> unsafe fn from_device(dev: &Device<Ctx>) -> &Self {
> <implementation here>
> }
> }
>
> unsafe impl Device<Ctx> for Container<Ctx> {
> const Offset: usize = 0;
> }
>
> }
>
> And instead of passing &Device<Bound> to the functions, we should add a
> type parameter to LedOps, e.g.:
>
> trait LedOps<T: device::Container<device::Bound>> {
>
> ...
>
> fn brightness_set(&self, dev: &T, brightness: u32) -> Result<()>;
>
> ...
>
> }
>
> impl<T: LedOps<E>, E: device::Container<device::Bound>> Device<T> {
>
> pub fn new<'a>(
> parent: &'a E,
> init_data: InitData<'a>,
> ops: T,
> ) -> impl PinInit<Devres<Self>, Error> + 'a {
> ...
> }
>
> ...
>
> }
>
> In the example of i2c (or any other container for `struct device`), we
> implement the device::Container trait:
>
> mod i2c {
>
> unsafe impl device::Container for I2cClient {
> const Offset: usize = offset_of!(bindings::i2c_client, dev);
> }
>
> }
> This allows the LedOps function to use any functions from the I2cClient
> or any other device container which may be used (removing the need to
> store it inside the LedOps implementations struct). It still allows
> Device<Bound> to be used, as it also would implement device::Container.
I had a similar idea in the past, but it has some disadvantages:
(1) You have to implement the upcast from a generic device to a bus device for
every bus device.
(2) You have to store a Box<dyn T> in the Rust LED class device; the C struct
device can't carry the fat pointer.
The alternative would be to provide a safe method for bus devices to upgrade to
a bound device by presenting its corresponding &Device<Bound> base device, for
instance:
impl I2cClient {
pub fn into_bound<'a>(&'a self, &'a Device<Bound>) -> Result<&'a I2cClient<Bound>> {
// Fails if the presented `&Device<Bound` is not the base device of `self`.
...
}
}
The advantage is that this can easily be implemented with a macro for all bus
devices.
There is a slight downside in ergonomics due to the into_bound() call though:
fn brightness_set(&self, parent: &Device<Bound>, brightness: u32) -> Result {
let i2c: &I2cClient<Bound> = self.i2c.into_bound(parent)?;
...
}
On Wed, 2025-10-15 at 16:52 +0200, Danilo Krummrich wrote:
> On Wed Oct 15, 2025 at 3:44 PM CEST, Markus Probst wrote:
> > On Mon, 2025-10-13 at 18:34 +0000, Alice RyhlV wrote:
> > > On Sun, Oct 12, 2025 at 02:52:39PM +0000, 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::InitData` - data set for the led class device
> > > >
> > > > * `led::Device` - a safe wrapper around `led_classdev`
> > > >
> > > > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > >
> > > > +pub trait LedOps: Send + 'static + Sized {
> > > > + /// If set true, [`LedOps::brightness_set`] and
> > > > [`LedOps::blink_set`] must not sleep
> > > > + /// and perform the operation immediately.
> > > > + const BLOCKING: bool;
> > > > + /// The max brightness level
> > > > + const MAX_BRIGHTNESS: u32;
> > > > +
> > > > + /// Sets the brightness level.
> > > > + ///
> > > > + /// See also [`LedOps::BLOCKING`]
> > > > + fn brightness_set(&self, brightness: u32) -> Result<()>;
> > > > +
> > > > + /// Gets the current brightness level.
> > > > + fn brightness_get(&self) -> u32 {
> > > > + 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(&self, _delay_on: &mut usize, _delay_off:
> > > > &mut
> > > > usize) -> Result<()> {
> > > > + build_error!(VTABLE_DEFAULT_ERROR)
> > > > + }
> > >
> > > These functions should probably take a &Device<Bound> argument so
> > > that
> > > they can use methods that require a bound device (such as IO).
> > How about instead something like
> >
> > mod device {
> >
> > unsafe trait Container<Ctx: DeviceContext>: AsRef<Device<Ctx>> {
> > const Offset: usize;
> >
> > unsafe fn from_device(dev: &Device<Ctx>) -> &Self {
> > <implementation here>
> > }
> > }
> >
> > unsafe impl Device<Ctx> for Container<Ctx> {
> > const Offset: usize = 0;
> > }
> >
> > }
> >
> > And instead of passing &Device<Bound> to the functions, we should
> > add a
> > type parameter to LedOps, e.g.:
> >
> > trait LedOps<T: device::Container<device::Bound>> {
> >
> > ...
> >
> > fn brightness_set(&self, dev: &T, brightness: u32) -> Result<()>;
> >
> > ...
> >
> > }
> >
> > impl<T: LedOps<E>, E: device::Container<device::Bound>> Device<T> {
> >
> > pub fn new<'a>(
> > parent: &'a E,
> > init_data: InitData<'a>,
> > ops: T,
> > ) -> impl PinInit<Devres<Self>, Error> + 'a {
> > ...
> > }
> >
> > ...
> >
> > }
> >
> > In the example of i2c (or any other container for `struct device`),
> > we
> > implement the device::Container trait:
> >
> > mod i2c {
> >
> > unsafe impl device::Container for I2cClient {
> > const Offset: usize = offset_of!(bindings::i2c_client, dev);
> > }
> >
> > }
> > This allows the LedOps function to use any functions from the
> > I2cClient
> > or any other device container which may be used (removing the need
> > to
> > store it inside the LedOps implementations struct). It still allows
> > Device<Bound> to be used, as it also would implement
> > device::Container.
>
> I had a similar idea in the past, but it has some disadvantages:
>
> (1) You have to implement the upcast from a generic device to a bus
> device for
> every bus device.
Not necessarily every, like I said `container::Device` itself also
would implement `device::Container` (still allowing &Device<Bound>).
>
> (2) You have to store a Box<dyn T> in the Rust LED class device;
> the C struct
> device can't carry the fat pointer.
Why carry a fat pointer (assuming you mean pointers to unsized types)?
We already know the address of it with the `struct led_classdev`-
>`parent` field, we just need to substract the offset from `<T as
Container>::Offset`, and we have the address of the device container
(like `struct i2c_client`). No Box needed.
Thanks
- Markus Probst
>
> The alternative would be to provide a safe method for bus devices to
> upgrade to
> a bound device by presenting its corresponding &Device<Bound> base
> device, for
> instance:
>
> impl I2cClient {
> pub fn into_bound<'a>(&'a self, &'a Device<Bound>) ->
> Result<&'a I2cClient<Bound>> {
> // Fails if the presented `&Device<Bound` is not the
> base device of `self`.
> ...
> }
> }
>
> The advantage is that this can easily be implemented with a macro for
> all bus
> devices.
>
> There is a slight downside in ergonomics due to the into_bound() call
> though:
>
> fn brightness_set(&self, parent: &Device<Bound>, brightness:
> u32) -> Result {
> let i2c: &I2cClient<Bound> =
> self.i2c.into_bound(parent)?;
>
> ...
> }
On Wed Oct 15, 2025 at 5:02 PM CEST, Markus Probst wrote: > Not necessarily every, like I said `container::Device` itself also > would implement `device::Container` (still allowing &Device<Bound>). Sure, but eventually we'll need all bus devices to implement it because you want to be able to use this class device on any bus. > We already know the address of it with the `struct led_classdev`- >`parent` field, we just need to substract the offset from `<T as > Container>::Offset`, and we have the address of the device container > (like `struct i2c_client`). No Box needed. Yeah, that should work. What I still don't like about it (and didn't like back then when I thought about something similar) is that this provides a publicly accessible unsafe way to upcast from a generic device to a bus device. However, I do see the ergonomic advantage for drivers. Even though I'd say it's minor, it is indeed nice that the class device callbacks can already carry the actual parent bus device. So, I'm fine giving this a shot. If go for an implementation, please name the trait something along the lines of device::IntoBusDevice, device::Container is too generic. Only bus devices have a DeviceContext.
On Mon Oct 13, 2025 at 8:34 PM CEST, Alice Ryhl wrote:
> On Sun, Oct 12, 2025 at 02:52:39PM +0000, 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::InitData` - data set for the led class device
>>
>> * `led::Device` - a safe wrapper around `led_classdev`
>>
>> Signed-off-by: Markus Probst <markus.probst@posteo.de>
>
>> +pub trait LedOps: Send + 'static + Sized {
>> + /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must not sleep
>> + /// and perform the operation immediately.
>> + const BLOCKING: bool;
>> + /// The max brightness level
>> + const MAX_BRIGHTNESS: u32;
>> +
>> + /// Sets the brightness level.
>> + ///
>> + /// See also [`LedOps::BLOCKING`]
>> + fn brightness_set(&self, brightness: u32) -> Result<()>;
>> +
>> + /// Gets the current brightness level.
>> + fn brightness_get(&self) -> u32 {
>> + 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(&self, _delay_on: &mut usize, _delay_off: &mut usize) -> Result<()> {
>> + build_error!(VTABLE_DEFAULT_ERROR)
>> + }
>
> These functions should probably take a &Device<Bound> argument so that
> they can use methods that require a bound device (such as IO).
Indeed!
@Markus: Note that this guarantee is given by the LED device registration being
lifetime controlled by devres, while led_classdev_unregister() is synchronized
against those callbacks.
For the latter, please double check that this is actually the case -- I'm not
familiar with the LED subsystem, I'm reviewing from driver-core perspective. But
from a quick look it should be the case. :)
On Mon, 2025-10-13 at 22:11 +0200, Danilo Krummrich wrote:
> On Mon Oct 13, 2025 at 8:34 PM CEST, Alice Ryhl wrote:
> > On Sun, Oct 12, 2025 at 02:52:39PM +0000, 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::InitData` - data set for the led class device
> > >
> > > * `led::Device` - a safe wrapper around `led_classdev`
> > >
> > > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> >
> > > +pub trait LedOps: Send + 'static + Sized {
> > > + /// If set true, [`LedOps::brightness_set`] and
> > > [`LedOps::blink_set`] must not sleep
> > > + /// and perform the operation immediately.
> > > + const BLOCKING: bool;
> > > + /// The max brightness level
> > > + const MAX_BRIGHTNESS: u32;
> > > +
> > > + /// Sets the brightness level.
> > > + ///
> > > + /// See also [`LedOps::BLOCKING`]
> > > + fn brightness_set(&self, brightness: u32) -> Result<()>;
> > > +
> > > + /// Gets the current brightness level.
> > > + fn brightness_get(&self) -> u32 {
> > > + 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(&self, _delay_on: &mut usize, _delay_off: &mut
> > > usize) -> Result<()> {
> > > + build_error!(VTABLE_DEFAULT_ERROR)
> > > + }
> >
> > These functions should probably take a &Device<Bound> argument so
> > that
> > they can use methods that require a bound device (such as IO).
>
> Indeed!
>
> @Markus: Note that this guarantee is given by the LED device
> registration being
> lifetime controlled by devres, while led_classdev_unregister() is
> synchronized
> against those callbacks.
>
> For the latter, please double check that this is actually the case --
> I'm not
> familiar with the LED subsystem, I'm reviewing from driver-core
> perspective. But
> from a quick look it should be the case. :)
There is also the led_classdev->dev device, but I assume you mean the
parent of the led classdev (the one being passed on register)?
Thanks
- Markus Probst
© 2016 - 2025 Red Hat, Inc.