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

[Markus Probst]

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. :)
> 
> 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.
How would this work with handling incoming data?

For example, once the power button on the NAS device is pressed, the
serdev device would receive a `0x30` byte.

Regmap seems like it can only do read and write after it has been
requested. No event handling.

Thanks
- Markus Probst

> 
> 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(())
> > +    }
> > +}

Attachment: signature.asc
Description: This is a digitally signed message part