Re: [PATCH v4 2/2] rust: leds: add basic led classdev abstractions

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

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