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

[Danilo Krummrich]

On Sun Mar 15, 2026 at 4:15 PM CET, Markus Probst wrote:
> 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.

That's orthogonal, directly accessing the struct serdev doesn't help with this
either.

Isn't this handled through IRQs, i.e. you device issues an IRQ and then you read
from the serial bus?

(I'm asking since such chips can usually be connected via different busses, e.g.
serial and I2C. And with I2C the slave can't issue a transfer by itself.)

Other MFD drivers register their own IRQ chip for this. I.e. one would register
an IRQ chip in the MFD driver and pass it to the sub-devices created through
mfd_add_devices(). Then the sub-device receives an IRQ and reads the regmap.

Now, if you don't have IRQs at all and the only event you get is through
receive_buf() (which implies that the chip is only compatible with a serial bus)
this technically still works, but might be a bit overkill.

In this case, maybe a monolithic driver would even be better; no idea where it
would live though.

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