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

[Markus Probst]

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@xxxxxxxxx>
> > 
> > > +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