Re: [PATCH v9 2/3] rust: leds: split generic and normal led classdev abstractions up

[Markus Probst]

On Thu, 2025-11-20 at 10:54 +0000, Alice Ryhl wrote:
> On Wed, Nov 19, 2025 at 02:11:24PM +0000, Markus Probst wrote:
> > Move code specific to normal led class devices into a separate file and
> > introduce the `led::Mode` trait to allow for other types of led class
> > devices.
> > 
> > Signed-off-by: Markus Probst <markus.probst@xxxxxxxxx>
> 
> So it seems like the goal of this trait is to support both normal led
> and multicolor led under the same code. However, it seems the traits
> involved with this are pretty complex.
> 
> My primary feedback here is: please consider if we can avoid these
> complex traits. How much duplication would it really take to just have
> two Device structs and two LedOps traits? I think a few pieces of
> duplication would be far better than what this patch does.
I counted over 221 lines of code for each classdev type (not counting
LedOps). If we want to support `led_classdev_flash` later down the line
(if a Rust driver would need it), that would be over 663 lines of code.

It doesn't look too complex to me, but thats maybe because I wrote it?

> 
> In fact, I'm not sure you even need two LedOps traits if you did that;
> it seems like you could even get away with reusing the trait for both
> cases.
Currently it only uses one LedOps trait. The implementation in the
multicolor needs access to `Device::subleds()`. `Device` gets passed as
a reference to every LedOps function. If there would be two `Device`
structs, I am not sure how we can preserve one LedOps trait. Possibly a
type declaration on `Mode`? (assuming we still want the `Mode` trait)

Thanks
- Markus Probst

> 
> Alice