Re: [PATCH v3 7/7] leds: add synology microp led driver

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

[-- Attachment #1: Type: text/plain, Size: 3905 bytes --]

On Fri, 2026-03-13 at 22:00 +0100, Danilo Krummrich wrote:
> On Fri Mar 13, 2026 at 8:03 PM CET, Markus Probst via B4 Relay wrote:
> > +impl Command {
> > +    fn write(self, dev: &platform::Device<Bound>) -> Result {
> > +        // SAFETY: Since we have no of and no acpi match table, we assume this is a mfd sub-device
> > +        // and our parent is a serial device bus device, bound to the synology microp core driver.
> > +        let parent = unsafe { dev.as_ref().parent_unchecked::<serdev::Device<Bound>>() };
> 
> Despite being accurate description, "assume" is not what you want to read for a
> safety justification. :)
Apparently this is how all C mfd sub-devices I have seen yet do it. Not
directly using the parent device, but assuming there is a parent
device, and accessing the drvdata of that parent device with most of
the time to little checking.

Some examples:

drivers/leds/leds-lm3533.c:
- assuming there is a parent device
- assuming the drvdata of the parent device has the type `lm3533_led`
- It does check however if drvdata is set.

drivers/leds/leds-upboard.c:
- assuming there is a parent device
- assuming drvdata of the parent device is set
- assuming drvdata of the parent device has the type `upboard_fpga`

> 
> We don't want to directly access the serial device from this driver. Instead,
> there should be an abstraction layer of the resource you are accessing.
> 
> If this would be I2C or SPI you would request the regmap of the parent at this
> point, e.g.
> 
> 	dev.parent().regmap("led_registers")
> 
> Now, this is a serial device, but regmap still works perfectly fine for this
> case. It even allows you to ensure from the MFD driver to restrict the LED
> driver of sending commands that are not LED specific by exposing a LED specific
> regmap. Additionally, if you need additional locking etc. it can all be done
> within the regmap implementation, so you entirely avoid custom APIs.
> 
> I'm not sure how common regmap is for serial devices to be honest, but
> apparently there are drivers doing this and I don't really see a reason against
> it.
> 
> For instance, there is drivers/iio/imu/bno055/, which is a chip that works on
> both serial and I2C busses and fully abstracts this fact with regmap.
> 
> In Rust a regmap will probably become a backend of the generic I/O
> infrastructure we are working on, which will also allow you to use the
> register!() infrastructure, etc.
> 
> register!() and some other generic I/O improvements will land this cycle, I/O
> projections are more likely to land next cycle.
> 
> > +        parent.write_all(
> > +            match self {
> > +                Self::Power(State::On) => &[0x34],
> > +                Self::Power(State::Blink) => &[0x35],
> > +                Self::Power(State::Off) => &[0x36],
> > +
> > +                Self::Status(_, State::Off) => &[0x37],
> > +                Self::Status(StatusLedColor::Green, State::On) => &[0x38],
> > +                Self::Status(StatusLedColor::Green, State::Blink) => &[0x39],
> > +                Self::Status(StatusLedColor::Orange, State::On) => &[0x3A],
> > +                Self::Status(StatusLedColor::Orange, State::Blink) => &[0x3B],
> > +
> > +                Self::Alert(State::On) => &[0x4C, 0x41, 0x31],
> > +                Self::Alert(State::Blink) => &[0x4C, 0x41, 0x32],
> > +                Self::Alert(State::Off) => &[0x4C, 0x41, 0x33],
> > +
> > +                Self::Usb(State::On) => &[0x40],
> > +                Self::Usb(State::Blink) => &[0x41],
> > +                Self::Usb(State::Off) => &[0x42],
> > +            },
> > +            serdev::Timeout::Max,
> > +        )?;
> > +        Ok(())
> > +    }
> > +}

But this looks like a better solution (the same would probably apply to
the existing C drivers).

Thanks
- Markus Probst

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]