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

["Danilo Krummrich" <dakr@kernel.org>]

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

	    ...
	}