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

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

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

On Sun, 2026-03-15 at 19:20 +0100, Danilo Krummrich wrote:
> 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.
Before knowing about regmap, I just would have exposed a function once
needed, so the sub-device can register a function pointer with a unique
byte. Depending on what byte gets received, that function pointer is
called.

But now, this would still work but it bypasses regmap.

> 
> 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.
You mean registering a virtual IRQ and triggering it on data receival?

Could you provide an example driver in the tree?
> 
> 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.
There is a physical IRQ, but the serial device bus abstracts that so
the driver only has the receive_buf() function. The driver it self is
not aware of an IRQs.

Having like a reverse regmap would be great (in addition), in which the
mfd device is the one who calls write and the sub-device has to handle
it. But I don't think something like this exists in the kernel.

Thanks
- Markus Probst

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

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