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

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

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)?;
> 
> 	    ...
> 	}