From: Markus Probst <markus.probst@posteo.de>
Add a driver to control the power, status, alert and usb LEDs on
Synology NAS devices. This will be bound to a mfd sub-device, registered
by the synology microp core driver.
Tested successfully on a Synology DS923+.
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
MAINTAINERS | 1 +
drivers/leds/Kconfig | 11 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds_synology_microp.rs | 303 +++++++++++++++++++++++++++++++++++
4 files changed, 316 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 32932ecab9cf..1d9240055d29 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25544,6 +25544,7 @@ M: Markus Probst <markus.probst@posteo.de>
S: Maintained
F: Documentation/devicetree/bindings/leds/synology,microp.yaml
F: Documentation/devicetree/bindings/mfd/synology,microp.yaml
+F: drivers/leds/leds_synology_microp.rs
F: drivers/mfd/synology_microp.rs
SYNOPSYS ARC ARCHITECTURE
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 597d7a79c988..9a9609b924fe 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -1029,6 +1029,17 @@ config LEDS_ACER_A500
This option enables support for the Power Button LED of
Acer Iconia Tab A500.
+config LEDS_SYNOLOGY_MICROP
+ tristate "Synology Microp led driver"
+ depends on MFD_SYNOLOGY_MICROP
+ depends on RUST
+ depends on LEDS_CLASS && LEDS_CLASS_MULTICOLOR
+ help
+ Enable support for the MCU found in Synology NAS devices.
+
+ This is needed to control the power, status, alert and usb leds on the
+ NAS device.
+
source "drivers/leds/blink/Kconfig"
comment "Flash and Torch LED drivers"
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 8fdb45d5b439..200101eb26d5 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -89,6 +89,7 @@ obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o
obj-$(CONFIG_LEDS_ST1202) += leds-st1202.o
obj-$(CONFIG_LEDS_SUN50I_A100) += leds-sun50i-a100.o
obj-$(CONFIG_LEDS_SUNFIRE) += leds-sunfire.o
+obj-$(CONFIG_LEDS_SYNOLOGY_MICROP) += leds_synology_microp.o
obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
obj-$(CONFIG_LEDS_TCA6507) += leds-tca6507.o
obj-$(CONFIG_LEDS_TI_LMU_COMMON) += leds-ti-lmu-common.o
diff --git a/drivers/leds/leds_synology_microp.rs b/drivers/leds/leds_synology_microp.rs
new file mode 100644
index 000000000000..d2e94e992981
--- /dev/null
+++ b/drivers/leds/leds_synology_microp.rs
@@ -0,0 +1,303 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Synology Microp led driver
+
+use kernel::{
+ device::Bound,
+ devres::{self, Devres},
+ led::{
+ self,
+ LedOps,
+ MultiColorSubLed, //
+ },
+ new_mutex,
+ platform,
+ prelude::*,
+ serdev,
+ sync::Mutex, //
+};
+use pin_init::pin_init_scope;
+
+kernel::module_platform_driver! {
+ type: SynologyMicropLedDriver,
+ name: "synology_microp_leds",
+ authors: ["Markus Probst <markus.probst@posteo.de>"],
+ description: "Synology Microp led driver",
+ license: "GPL v2",
+}
+
+#[pin_data]
+struct SynologyMicropLedDriver {
+ #[pin]
+ status: Devres<led::MultiColorDevice<StatusLedHandler>>,
+ #[pin]
+ power: Devres<led::Device<LedHandler>>,
+}
+
+impl platform::Driver for SynologyMicropLedDriver {
+ type IdInfo = ();
+
+ fn probe(
+ dev: &platform::Device<kernel::device::Core>,
+ _id_info: Option<&Self::IdInfo>,
+ ) -> impl PinInit<Self, Error> {
+ pin_init_scope(move || {
+ let fwnode = dev.as_ref().fwnode().ok_or(EINVAL)?;
+
+ if let Some(alert_fwnode) = fwnode.get_child_by_name(c"alert-led") {
+ devres::register(
+ dev.as_ref(),
+ led::DeviceBuilder::new()
+ .fwnode(Some(alert_fwnode))
+ .devicename(c"synology-microp/alert-led")
+ .color(led::Color::Orange)
+ .build(
+ &**dev,
+ try_pin_init!(LedHandler {
+ blink <- new_mutex!(false),
+ command: Command::Alert,
+ }),
+ ),
+ GFP_KERNEL,
+ )?;
+ }
+
+ if let Some(alert_fwnode) = fwnode.get_child_by_name(c"usb-led") {
+ devres::register(
+ dev.as_ref(),
+ led::DeviceBuilder::new()
+ .fwnode(Some(alert_fwnode))
+ .devicename(c"synology-microp/usb-led")
+ .color(led::Color::Green)
+ .build(
+ &**dev,
+ try_pin_init!(LedHandler {
+ blink <- new_mutex!(false),
+ command: Command::Usb,
+ }),
+ ),
+ GFP_KERNEL,
+ )?;
+ }
+
+ Ok(try_pin_init!(Self {
+ status <- led::DeviceBuilder::new()
+ .fwnode(Some(fwnode.get_child_by_name(c"status-led").ok_or(EINVAL)?))
+ .devicename(c"synology-microp/status-led")
+ .color(led::Color::Multi)
+ .build_multicolor(
+ &**dev,
+ try_pin_init!(StatusLedHandler {
+ blink <- new_mutex!(false),
+ }),
+ StatusLedHandler::SUBLEDS,
+ ),
+ power <- led::DeviceBuilder::new()
+ .fwnode(Some(fwnode.get_child_by_name(c"power-led").ok_or(EINVAL)?))
+ .devicename(c"synology-microp/power-led")
+ .color(led::Color::Blue)
+ .default_trigger(c"timer")
+ .build(
+ &**dev,
+ try_pin_init!(LedHandler {
+ blink <- new_mutex!(true),
+ command: Command::Power,
+ }),
+ ),
+ }))
+ })
+ }
+}
+
+enum StatusLedColor {
+ Green,
+ Orange,
+}
+
+enum State {
+ On,
+ Blink,
+ Off,
+}
+
+enum Command {
+ Power(State),
+ Status(StatusLedColor, State),
+ Alert(State),
+ Usb(State),
+}
+
+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>>() };
+ 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(())
+ }
+}
+
+#[pin_data]
+struct LedHandler {
+ #[pin]
+ blink: Mutex<bool>,
+ command: fn(State) -> Command,
+}
+
+#[vtable]
+impl LedOps for LedHandler {
+ type Bus = platform::Device<Bound>;
+ type Mode = led::Normal;
+ const BLOCKING: bool = true;
+ const MAX_BRIGHTNESS: u32 = 1;
+
+ fn brightness_set(
+ &self,
+ dev: &Self::Bus,
+ _classdev: &led::Device<Self>,
+ brightness: u32,
+ ) -> Result<()> {
+ let mut blink = self.blink.lock();
+ (self.command)(if brightness == 0 {
+ *blink = false;
+ State::Off
+ } else if *blink {
+ State::Blink
+ } else {
+ State::On
+ })
+ .write(dev)?;
+
+ Ok(())
+ }
+
+ fn blink_set(
+ &self,
+ dev: &Self::Bus,
+ _classdev: &led::Device<Self>,
+ delay_on: &mut usize,
+ delay_off: &mut usize,
+ ) -> Result<()> {
+ let mut blink = self.blink.lock();
+
+ (self.command)(if *delay_on == 0 && *delay_off != 0 {
+ State::Off
+ } else if *delay_on != 0 && *delay_off == 0 {
+ State::On
+ } else {
+ *blink = true;
+ *delay_on = 167;
+ *delay_off = 167;
+
+ State::Blink
+ })
+ .write(dev)
+ }
+}
+
+#[pin_data]
+struct StatusLedHandler {
+ #[pin]
+ blink: Mutex<bool>,
+}
+
+impl StatusLedHandler {
+ const SUBLEDS: &[MultiColorSubLed] = &[
+ MultiColorSubLed::new(led::Color::Green).initial_intensity(1),
+ MultiColorSubLed::new(led::Color::Orange),
+ ];
+}
+
+#[vtable]
+impl LedOps for StatusLedHandler {
+ type Bus = platform::Device<Bound>;
+ type Mode = led::MultiColor;
+ const BLOCKING: bool = true;
+ const MAX_BRIGHTNESS: u32 = 1;
+
+ fn brightness_set(
+ &self,
+ dev: &Self::Bus,
+ classdev: &led::MultiColorDevice<Self>,
+ brightness: u32,
+ ) -> Result<()> {
+ let mut blink = self.blink.lock();
+ if brightness == 0 {
+ *blink = false;
+ }
+
+ let (color, subled_brightness) = if classdev.subleds()[1].intensity == 0 {
+ (StatusLedColor::Green, classdev.subleds()[0].brightness)
+ } else {
+ (StatusLedColor::Orange, classdev.subleds()[1].brightness)
+ };
+
+ Command::Status(
+ color,
+ if subled_brightness == 0 {
+ State::Off
+ } else if *blink {
+ State::Blink
+ } else {
+ State::On
+ },
+ )
+ .write(dev)
+ }
+
+ fn blink_set(
+ &self,
+ dev: &Self::Bus,
+ classdev: &led::MultiColorDevice<Self>,
+ delay_on: &mut usize,
+ delay_off: &mut usize,
+ ) -> Result<()> {
+ let mut blink = self.blink.lock();
+ *blink = true;
+
+ let (color, subled_intensity) = if classdev.subleds()[1].intensity == 0 {
+ (StatusLedColor::Green, classdev.subleds()[0].intensity)
+ } else {
+ (StatusLedColor::Orange, classdev.subleds()[1].intensity)
+ };
+ Command::Status(
+ color,
+ if *delay_on == 0 && *delay_off != 0 {
+ *blink = false;
+ State::Off
+ } else if subled_intensity == 0 {
+ State::Off
+ } else if *delay_on != 0 && *delay_off == 0 {
+ *blink = false;
+ State::On
+ } else {
+ *delay_on = 167;
+ *delay_off = 167;
+
+ State::Blink
+ },
+ )
+ .write(dev)
+ }
+}
--
2.52.0On 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(())
> + }
> +}
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(())
> > + }
> > +}
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(())
>> > + }
>> > +}
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(())
> > > > + }
> > > > +}
On Sun Mar 15, 2026 at 7:47 PM CET, Markus Probst wrote: > On Sun, 2026-03-15 at 19:20 +0100, Danilo Krummrich wrote: >> 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? Not really virtual, there are a lot of MFD drivers that register their own IRQ chip to forward only relevant IRQs to the sub-device. What you say should work as well, but as mentioned below, I feel like that's overkill. > Could you provide an example driver in the tree? One example would be drivers/mfd/palmas.c, but there should be many more. >> 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. I think you are confusing the IRQ of the serial bus controller with a device IRQ. The serial bus controller the device is connected to has an IRQ itself, but what I mean is a device IRQ line. This is very common for devices on busses such as I2C, SPI, etc., as they have master/slave semantics. I.e. the device issues an IRQ and the kernel reads a register subsequently. UART does not force master/slave sematics on a bus level though. That's why I asked whether the device is UART only, or if it supports other busses as well. > 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. I mean, it's not really that the kernel exposes registers to the device. The device just uses the fact that the UART is not a master/slave bus and pushes a single byte to the kernel to signal that a button has been pressed. So, it's still "IRQ semantics". (But I see that on abstract level one could argue in this direction.) TBH, I think that the combination of this chip supporting multiple functions and being connected through UART, where the device pushes bytes through the UART to signal events is a bit of an edge case. As mentioned, if it would be connected through I2C instead, it would be simple: forward the IRQ and use a regmap, and you can do it entirely with generic infrastructure and no custom APIs, which in the end is the idea of MFD. I.e. you can describe the whole sub-device with a struct mfd_cell. And while we could technically "emulate" this, it remains to be odd and has unnecessary overhead. I've seen one other case in the kernel, which is drivers/mfd/rave-sp.c. But this driver doesn't use MFD infrastructure at all and just goes for a custom API, which clearly defeats the purpose of MFD in the first place. I.e. it shouldn't even live under drivers/mfd/. Greg already mentioned the auxiliary bus, which for a custom API clearly is the better choice. But to be honest, the more I hear about this device, the more I feel like a monolithic driver is all that's needed, as everything else sounds like overhead that doesn't really provide any value. I.e. if we can't (easily) use mfd cells and would need a custom API, then why even split it up at all, given that splitting it up would probably the most complicated part of the whole driver. Greg, what do you think? *me runs away*
On Sun, Mar 15, 2026 at 08:41:06PM +0100, Danilo Krummrich wrote: > I.e. if we can't (easily) use mfd cells and would need a custom API, then why > even split it up at all, given that splitting it up would probably the most > complicated part of the whole driver. > > Greg, what do you think? I think this has yet to be proven to be a kernel driver at all at this point, and not just a userspace daemon that listens to the serial port and then does what is needed from there :) Or, if someone can prove that the operations on this serial data stream actually do require it to be in the kernel (which I have yet to see a list of what this connection does, did I miss it?) then a single driver, under the drivers/platform section of the kernel tree makes sense. thanks, greg k-h
On Mon, 2026-03-16 at 07:33 +0100, Greg Kroah-Hartman wrote: > On Sun, Mar 15, 2026 at 08:41:06PM +0100, Danilo Krummrich wrote: > > I.e. if we can't (easily) use mfd cells and would need a custom API, then why > > even split it up at all, given that splitting it up would probably the most > > complicated part of the whole driver. > > > > Greg, what do you think? > > I think this has yet to be proven to be a kernel driver at all at this > point, and not just a userspace daemon that listens to the serial port > and then does what is needed from there :) > > Or, if someone can prove that the operations on this serial data stream > actually do require it to be in the kernel (which I have yet to see a > list of what this connection does, did I miss it?) then a single driver, > under the drivers/platform section of the kernel tree makes sense. The sysoff component is strictly necessary for poweroff and reboot. On ARM64 Synology NAS devices it is needed so the device actually powers off after calling `syscall(SYS_reboot, LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, LINUX_REBOOT_CMD_POWER_OFF, NULL);` . Otherwise it would stay on. Same applies to reboot. On x86 it isn't clearly documented what sending the poweroff and reboot command to the microp device exactly does, so this is based on observations. It should be sent before issuing a poweroff or reboot via ACPI Sleep. On reboot it resets various device states, so fan speeds go to default, leds turn off etc., so it behaves like a coldboot. On poweroff it will mark it as graceful shutdown (i. e. the device won't turn automatically on, because it thinks a power-loss happend). For the other components: - leds - hwmon - input It could theoratically be implemented in userspace. A userspace daemon could theoratically control the fan speeds directly, issue a systemd shutdown on power button press, control the leds directly etc. But honestly, I don't understand why this is an argument. With that argument drivers/leds, drivers/hwmon and drivers/input would not even exist, because everything could be implemented in userspace via - I2C: /dev/i2c-* (drivers/i2c/i2c-dev.c) - MMIO: /dev/ioport and /dev/mem (drivers/char/mem.c) - GPIO: /sys/class/gpio (drivers/gpio/gpiolib-sysfs.c) - SPI: /dev/spidev* (drivers/spi/spidev.c) - PCI: /sys/class/pci_bus/ (drivers/pci/pci-sysfs.c) - Serial: /dev/ttyS* and likely almost any other bus device too. Generally speaking, the kernel and its drivers is the layer between hardware and software. It provides the hardware abstractions as userspace interfaces. So any software on the same cpu architecture can work with any hardware, as long as there is a kernel driver. In the case of this driver, it means - *any* led daemon can control the leds - *any* fan control daemon can control the fan speed and frequency - *any* monitoring software can view the provided sensors - *any* init system can react to the power button - *any* process can request a reboot or shutdown . I think this is the expected behaviour. Thanks - Markus Probst > > thanks, > > greg k-h
On Mon, Mar 16, 2026 at 01:43:44PM +0000, Markus Probst wrote: > On Mon, 2026-03-16 at 07:33 +0100, Greg Kroah-Hartman wrote: > > On Sun, Mar 15, 2026 at 08:41:06PM +0100, Danilo Krummrich wrote: > > > I.e. if we can't (easily) use mfd cells and would need a custom API, then why > > > even split it up at all, given that splitting it up would probably the most > > > complicated part of the whole driver. > > > > > > Greg, what do you think? > > > > I think this has yet to be proven to be a kernel driver at all at this > > point, and not just a userspace daemon that listens to the serial port > > and then does what is needed from there :) > > > > Or, if someone can prove that the operations on this serial data stream > > actually do require it to be in the kernel (which I have yet to see a > > list of what this connection does, did I miss it?) then a single driver, > > under the drivers/platform section of the kernel tree makes sense. > The sysoff component is strictly necessary for poweroff and reboot. > > On ARM64 Synology NAS devices it is needed so the device actually > powers off after calling > `syscall(SYS_reboot, LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, > LINUX_REBOOT_CMD_POWER_OFF, NULL);` > . Otherwise it would stay on. > Same applies to reboot. So that means a write of a set of bytes to the serial port will cause the machine to reboot or shutdown? > On x86 it isn't clearly documented what sending the poweroff and reboot > command to the microp device exactly does, so this is based on > observations. It should be sent before issuing a poweroff or reboot via > ACPI Sleep. On reboot it resets various device states, so fan speeds go > to default, leds turn off etc., so it behaves like a coldboot. > On poweroff it will mark it as graceful shutdown (i. e. the device > won't turn automatically on, because it thinks a power-loss happend). > > For the other components: > - leds > - hwmon > - input For "input" what exactly does the input device show up as? A power button? Something else? For hwmon, that makes sense to have a kernel driver. For leds, that depends on what you want to do with the led, as in the end you are just controlling it from userspace anyway :) > It could theoratically be implemented in userspace. A userspace daemon > could theoratically control the fan speeds directly, issue a systemd > shutdown on power button press, control the leds directly etc. > > But honestly, I don't understand why this is an argument. > With that argument drivers/leds, drivers/hwmon and drivers/input would > not even exist, because everything could be implemented in userspace > via > - I2C: /dev/i2c-* (drivers/i2c/i2c-dev.c) > - MMIO: /dev/ioport and /dev/mem (drivers/char/mem.c) > - GPIO: /sys/class/gpio (drivers/gpio/gpiolib-sysfs.c) > - SPI: /dev/spidev* (drivers/spi/spidev.c) > - PCI: /sys/class/pci_bus/ (drivers/pci/pci-sysfs.c) > - Serial: /dev/ttyS* > and likely almost any other bus device too. > > Generally speaking, the kernel and its drivers is the layer between > hardware and software. It provides the hardware abstractions as > userspace interfaces. So any software on the same cpu architecture can > work with any hardware, as long as there is a kernel driver. Yes, I kind of know what drivers and classes do and why they are needed, that's not the point here. :) > In the case of this driver, it means > - *any* led daemon can control the leds > - *any* fan control daemon can control the fan speed and frequency > - *any* monitoring software can view the provided sensors > - *any* init system can react to the power button > - *any* process can request a reboot or shutdown > . > I think this is the expected behaviour. Ok great, then make a single driver that handles all of this, like other drivers/platform/ drivers do today, and all should be fine. thanks, greg k-h
On Mon, 2026-03-16 at 14:58 +0100, Greg Kroah-Hartman wrote: > On Mon, Mar 16, 2026 at 01:43:44PM +0000, Markus Probst wrote: > > On Mon, 2026-03-16 at 07:33 +0100, Greg Kroah-Hartman wrote: > > > On Sun, Mar 15, 2026 at 08:41:06PM +0100, Danilo Krummrich wrote: > > > > I.e. if we can't (easily) use mfd cells and would need a custom API, then why > > > > even split it up at all, given that splitting it up would probably the most > > > > complicated part of the whole driver. > > > > > > > > Greg, what do you think? > > > > > > I think this has yet to be proven to be a kernel driver at all at this > > > point, and not just a userspace daemon that listens to the serial port > > > and then does what is needed from there :) > > > > > > Or, if someone can prove that the operations on this serial data stream > > > actually do require it to be in the kernel (which I have yet to see a > > > list of what this connection does, did I miss it?) then a single driver, > > > under the drivers/platform section of the kernel tree makes sense. > > The sysoff component is strictly necessary for poweroff and reboot. > > > > On ARM64 Synology NAS devices it is needed so the device actually > > powers off after calling > > `syscall(SYS_reboot, LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, > > LINUX_REBOOT_CMD_POWER_OFF, NULL);` > > . Otherwise it would stay on. > > Same applies to reboot. > > So that means a write of a set of bytes to the serial port will cause > the machine to reboot or shutdown? Yes. > > > On x86 it isn't clearly documented what sending the poweroff and reboot > > command to the microp device exactly does, so this is based on > > observations. It should be sent before issuing a poweroff or reboot via > > ACPI Sleep. On reboot it resets various device states, so fan speeds go > > to default, leds turn off etc., so it behaves like a coldboot. > > On poweroff it will mark it as graceful shutdown (i. e. the device > > won't turn automatically on, because it thinks a power-loss happend). > > > > For the other components: > > - leds > > - hwmon > > - input > > For "input" what exactly does the input device show up as? A power > button? Something else? Well there are a few other things besides a power button: - beeper (short and long variant) - factory reset button - "USB Copy" button. Not present on all devices (including not present on my testing device). It is meant for copying data from or to a usb stick. Both of these buttons don't have a keycode in include/uapi/linux/input- event-codes.h, which is suprising given how many devices have a factory reset button. So I am not sure if I can handle them in this driver. From what I can tell, there isn't actually a input device type I could specify on registering a input device. But it would be considered a misc input device. > > For hwmon, that makes sense to have a kernel driver. > > For leds, that depends on what you want to do with the led, as in the > end you are just controlling it from userspace anyway :) > > > It could theoratically be implemented in userspace. A userspace daemon > > could theoratically control the fan speeds directly, issue a systemd > > shutdown on power button press, control the leds directly etc. > > > > But honestly, I don't understand why this is an argument. > > With that argument drivers/leds, drivers/hwmon and drivers/input would > > not even exist, because everything could be implemented in userspace > > via > > - I2C: /dev/i2c-* (drivers/i2c/i2c-dev.c) > > - MMIO: /dev/ioport and /dev/mem (drivers/char/mem.c) > > - GPIO: /sys/class/gpio (drivers/gpio/gpiolib-sysfs.c) > > - SPI: /dev/spidev* (drivers/spi/spidev.c) > > - PCI: /sys/class/pci_bus/ (drivers/pci/pci-sysfs.c) > > - Serial: /dev/ttyS* > > and likely almost any other bus device too. > > > > Generally speaking, the kernel and its drivers is the layer between > > hardware and software. It provides the hardware abstractions as > > userspace interfaces. So any software on the same cpu architecture can > > work with any hardware, as long as there is a kernel driver. > > Yes, I kind of know what drivers and classes do and why they are needed, > that's not the point here. :) > > > In the case of this driver, it means > > - *any* led daemon can control the leds > > - *any* fan control daemon can control the fan speed and frequency > > - *any* monitoring software can view the provided sensors > > - *any* init system can react to the power button > > - *any* process can request a reboot or shutdown > > . > > I think this is the expected behaviour. > > Ok great, then make a single driver that handles all of this, like other > drivers/platform/ drivers do today, and all should be fine. Great, I will. Thanks - Markus Probst > > thanks, > > greg k-h
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
© 2016 - 2026 Red Hat, Inc.