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

[Danilo Krummrich]

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.

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