[PATCH v3 0/7] Introduce Synology Microp driver

[PATCH v3 0/7] Introduce Synology Microp driver

[Markus Probst via B4 Relay]

Apparently my email provider refused to send the whole patch series,
because of a 100 receiver per hour limit to protect against spam
(according to the error message). Please ignore the previous submission.

Synology uses a microcontroller in their NAS devices connected to a
serial port to control certain LEDs, fan speeds, a beeper, to handle
proper shutdown and restart, buttons and fan failures.

This patch series depends on the rust led abstraction [1] and the rust
serdev abstraction [2].

This only adds the core driver and led driver.
The following rust abstractions would be required, to implement the
remaining features:
- hwmon (include/linux/hwmon.h)
- input (include/linux/input.h)
- sysoff handler + hardware protection shutdown (include/linux/reboot.h)

[1] https://lore.kernel.org/rust-for-linux/20260207-rust_leds-v12-0-fdb518417b75@posteo.de/
[2] https://lore.kernel.org/rust-for-linux/20260313-rust_serdev-v3-0-c9a3af214f7f@posteo.de/

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Changes in v3:
- remove `default n` from Kconfig entry, as n is the default already.
- select RUST_SERIAL_DEV_BUS_ABSTRACTIONS in Kconfig
- add mfd rust abstraction
- split core and led parts into their own driver. It should now be considered a
  MFD device.
- split led part of dt binding into its own file
- Link to v2: https://lore.kernel.org/r/20260308-synology_microp_initial-v2-0-9389963f31c5@posteo.de

Changes in v2:
- fix missing tabs in MAINTAINERS file
- remove word binding from patch subject
- add missing signed-off-by
- add missing help entry in Kconfig
- add missing spdx license headers
- remove no-check{,-cpu}-fan properties from the dt-bindings and replace
  them with the check_fan module parameter
- use patternProperties for leds in dt-bindings
- license dt-binding as GPL-2.0-only OR BSD-2-Clause
- move driver from staging tree into mfd tree and mark it as work in
  progress inside Kconfig
- only register alert and usb led if fwnode is present
- Link to v1: https://lore.kernel.org/r/20260306-synology_microp_initial-v1-0-fcffede6448c@posteo.de

---
Markus Probst (7):
      rust: Add `parent_unchecked` function to `Device`
      rust: add basic mfd abstractions
      acpi: add acpi_of_match_device_ids
      mfd: match acpi devices against PRP0001
      dt-bindings: mfd: Add synology,microp device
      mfd: Add synology microp core driver
      leds: add synology microp led driver

 .../devicetree/bindings/leds/synology,microp.yaml  |  40 +++
 .../devicetree/bindings/mfd/synology,microp.yaml   |  51 ++++
 MAINTAINERS                                        |  14 +
 drivers/acpi/bus.c                                 |   7 +
 drivers/leds/Kconfig                               |  11 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds_synology_microp.rs               | 303 +++++++++++++++++++++
 drivers/mfd/Kconfig                                |  11 +
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/mfd-core.c                             |  27 ++
 drivers/mfd/synology_microp.rs                     |  46 ++++
 include/acpi/acpi_bus.h                            |   2 +
 rust/bindings/bindings_helper.h                    |   1 +
 rust/kernel/device.rs                              |  25 +-
 rust/kernel/i2c.rs                                 |   7 +
 rust/kernel/lib.rs                                 |   1 +
 rust/kernel/mfd.rs                                 | 114 ++++++++
 rust/kernel/pci.rs                                 |   7 +
 rust/kernel/platform.rs                            |   7 +
 rust/kernel/serdev.rs                              |   6 +
 rust/kernel/usb.rs                                 |   7 +
 21 files changed, 688 insertions(+), 2 deletions(-)
---
base-commit: 3daa4f5dc6cc1ac1ab2f95b5b4c16bc5fb87f48f
change-id: 20260306-synology_microp_initial-0f7dac7b7496
prerequisite-change-id: 20251217-rust_serdev-ee5481e9085c:v3
prerequisite-patch-id: 52b17274481cc770c257d8f95335293eca32a2c5
prerequisite-patch-id: eec47e5051640d08bcd34a9670b98804449cad52
prerequisite-patch-id: f24b68c71c3f69371e8ac0251efca0a023b31cc4
prerequisite-patch-id: 3dfc1f7e5ecd3e0dd65d676aeb16f55260847b25
prerequisite-change-id: 20251114-rust_leds-a959f7c2f7f9:v12
prerequisite-patch-id: 42c445ef6981e3a3740dbaaf307f4b810042e46f
prerequisite-patch-id: 90c7b200cca722a592353885e21af069101c4e09
prerequisite-patch-id: c664a52faa3d47000d252eb7603c9c08382e868a

[PATCH v3 1/7] rust: Add `parent_unchecked` function to `Device`

[Markus Probst via B4 Relay]

From: Markus Probst <markus.probst@posteo.de>

This allows mfd sub-devices to access the bus device of their parent.

The existing safe `parent` function has been made public for
consistency.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 rust/kernel/device.rs | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 94e0548e7687..a53fb8a388e8 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -340,8 +340,7 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
     }
 
     /// Returns a reference to the parent device, if any.
-    #[cfg_attr(not(CONFIG_AUXILIARY_BUS), expect(dead_code))]
-    pub(crate) fn parent(&self) -> Option<&Device> {
+    pub fn parent(&self) -> Option<&Device> {
         // SAFETY:
         // - By the type invariant `self.as_raw()` is always valid.
         // - The parent device is only ever set at device creation.
@@ -358,6 +357,28 @@ pub(crate) fn parent(&self) -> Option<&Device> {
         }
     }
 
+    /// Returns a reference to the parent device as bus device.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the device has a parent, that is contained in `T`.
+    pub unsafe fn parent_unchecked<T: AsBusDevice<Ctx>>(&self) -> &T {
+        // SAFETY:
+        // - By the type invariant `self.as_raw()` is always valid.
+        // - The parent device is only ever set at device creation.
+        let parent_raw = unsafe { (*self.as_raw()).parent };
+
+        // SAFETY:
+        // - The safety requirements guarantee that the device has a parent, thus `parent_raw`
+        //   must be a pointer to a valid `struct device`.
+        // - `parent_raw` is valid for the lifetime of `self`, since a `struct device` holds a
+        //   reference count of its parent.
+        let parent = unsafe { Device::from_raw(parent_raw) };
+
+        // SAFETY: The safety requirements guarantee that the parent device is contained in `T`.
+        unsafe { T::from_device(parent) }
+    }
+
     /// Convert a raw C `struct device` pointer to a `&'a Device`.
     ///
     /// # Safety

-- 
2.52.0

[PATCH v3 2/7] rust: add basic mfd abstractions

[Markus Probst via B4 Relay]

From: Markus Probst <markus.probst@posteo.de>

Implement the basic mfd rust abstractions required to register mfd
sub-devices, including:
- `mfd::Cell` - a safe wrapper for `struct mfd_cell`
- `mfd::CellAcpiMatch` - a safe wrapper for `struct mfd_cell_acpi_match`
- `const MFD_CELLS: Option<&'static [mfd::Cell]>` in each bus device,
  except auxiliary

The mfd sub-device registration will be done after a successful call to
probe in each bus device, except auxiliary. This guarantees that
`mfd_add_devices` will only be run at most once per device. It also
ensures that the sub-devices will be probed after the drvdata of the
device has been set.

In order to register mfd sub-devices for a device, the driver needs to
set `const MFD_CELLS` in their Driver trait implementation to Some.
A build_assert guarantees that this can only be set to Some, if
CONFIG_MFD_CORE is enabled.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 MAINTAINERS                     |   6 +++
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/i2c.rs              |   7 +++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/mfd.rs              | 114 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/pci.rs              |   7 +++
 rust/kernel/platform.rs         |   7 +++
 rust/kernel/serdev.rs           |   6 +++
 rust/kernel/usb.rs              |   7 +++
 9 files changed, 156 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 749d63ca18fa..fa49e40836ab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18082,6 +18082,12 @@ F:	drivers/mfd/
 F:	include/dt-bindings/mfd/
 F:	include/linux/mfd/
 
+MULTIFUNCTION DEVICES (MFD) [RUST]
+M:	Markus Probst <markus.probst@posteo.de>
+S:	Maintained
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git
+F:	rust/kernel/mfd.rs
+
 MULTIMEDIA CARD (MMC) ETC. OVER SPI
 S:	Orphan
 F:	drivers/mmc/host/mmc_spi.c
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index f597fe3352f5..b7c17d1d9ece 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -65,6 +65,7 @@
 #include <linux/jump_label.h>
 #include <linux/led-class-multicolor.h>
 #include <linux/mdio.h>
+#include <linux/mfd/core.h>
 #include <linux/mm.h>
 #include <linux/miscdevice.h>
 #include <linux/of_device.h>
diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index bb5b830f48c3..e733b651d878 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -14,6 +14,7 @@
     devres::Devres,
     driver,
     error::*,
+    mfd,
     of,
     prelude::*,
     types::{
@@ -167,6 +168,9 @@ extern "C" fn probe_callback(idev: *mut bindings::i2c_client) -> kernel::ffi::c_
             let data = T::probe(idev, info);
 
             idev.as_ref().set_drvdata(data)?;
+
+            idev.as_ref().mfd_add_devices(T::MFD_CELLS)?;
+
             Ok(0)
         })
     }
@@ -328,6 +332,9 @@ pub trait Driver: Send {
     /// The table of ACPI device ids supported by the driver.
     const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None;
 
+    /// The mfd cells for mfd devices.
+    const MFD_CELLS: Option<&'static [mfd::Cell]> = None;
+
     /// I2C driver probe.
     ///
     /// Called when a new i2c client is added or discovered.
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 311fdf984b87..bacc54ca6aea 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -114,6 +114,7 @@
 pub mod led;
 pub mod list;
 pub mod maple_tree;
+pub mod mfd;
 pub mod miscdevice;
 pub mod mm;
 pub mod module_param;
diff --git a/rust/kernel/mfd.rs b/rust/kernel/mfd.rs
new file mode 100644
index 000000000000..6c47d9211bf2
--- /dev/null
+++ b/rust/kernel/mfd.rs
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the mfd subsystem.
+//!
+//! C header: [`include/linux/mfd/core.h`](srctree/include/linux/mfd/core.h)
+
+use core::{mem::MaybeUninit, ptr};
+
+use crate::{
+    device::{
+        CoreInternal,
+        Device, //
+    },
+    error::to_result,
+    prelude::*, //
+};
+
+/// A mfd cell.
+///
+/// # Invariants
+///
+/// A [`Cell`] instance represents a valid `struct mfd_cell`.
+#[repr(transparent)]
+pub struct Cell(bindings::mfd_cell);
+
+impl Cell {
+    /// Creates a new mfd cell.
+    pub const fn new(name: &'static CStr) -> Self {
+        Self(bindings::mfd_cell {
+            name: name.as_ptr().cast::<u8>(),
+
+            // SAFETY: Always safe to call.
+            // This is the const equivalent to `bindings::mfd_cell::default()`.
+            ..unsafe { MaybeUninit::zeroed().assume_init() }
+        })
+    }
+
+    /// Sets `of_compatible` and optionally `of_reg` and `use_of_reg` on the mfd cell.
+    pub const fn of(self, compatible: &'static CStr, reg: Option<u64>) -> Self {
+        Self(bindings::mfd_cell {
+            of_compatible: compatible.as_ptr().cast::<u8>(),
+            // TODO: Use `unwrap_or` once stabilized in const fn.
+            of_reg: if let Some(reg) = reg { reg } else { 0 },
+            use_of_reg: reg.is_some(),
+
+            ..self.0
+        })
+    }
+
+    /// Sets `acpi_match` on the mfd cell.
+    pub const fn acpi(self, acpi_match: &'static CellAcpiMatch) -> Self {
+        Self(bindings::mfd_cell {
+            acpi_match: &raw const acpi_match.0,
+
+            ..self.0
+        })
+    }
+}
+
+/// A mfd cell acpi match entry.
+///
+/// # Invariants
+///
+/// A [`CellAcpiMatch`] instance represents a valid `struct mfd_cell_acpi_match`.
+#[repr(transparent)]
+pub struct CellAcpiMatch(bindings::mfd_cell_acpi_match);
+
+impl CellAcpiMatch {
+    /// Creates a new mfd cell acpi match entry, using a ACPI PNP ID.
+    pub const fn pnpid(pnpid: &'static CStr) -> Self {
+        Self(bindings::mfd_cell_acpi_match {
+            pnpid: pnpid.as_ptr().cast::<u8>(),
+            adr: 0,
+        })
+    }
+
+    /// Creates a new mfd cell acpi match entry, using a ACPI ADR.
+    pub const fn adr(adr: u64) -> Self {
+        Self(bindings::mfd_cell_acpi_match {
+            pnpid: ptr::null(),
+            adr,
+        })
+    }
+}
+
+impl Device<CoreInternal> {
+    /// Registers child mfd devices.
+    // Always inline to optimize out error path of `build_assert`.
+    #[inline(always)]
+    pub(crate) fn mfd_add_devices(&self, cells: Option<&'static [Cell]>) -> Result {
+        if let Some(cells) = cells {
+            build_assert!(cfg!(CONFIG_MFD_CORE));
+
+            // SAFETY:
+            // - `self.as_raw()` is guaranteed to be a pointer to a valid `device`.
+            // - `cells.as_ptr()` is a guaranteed to be a pointer to a valid `mfd_cell` array
+            //   with the length of `cells.len()`.
+            to_result(unsafe {
+                bindings::devm_mfd_add_devices(
+                    self.as_raw(),
+                    bindings::PLATFORM_DEVID_AUTO,
+                    // CAST: `Cell` is a transparent wrapper of `mfd_cell`.
+                    cells.as_ptr().cast::<bindings::mfd_cell>(),
+                    i32::try_from(cells.len())?,
+                    ptr::null_mut(),
+                    0,
+                    ptr::null_mut(),
+                )
+            })?;
+        }
+
+        Ok(())
+    }
+}
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index af74ddff6114..6c4cf6cf970b 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -17,6 +17,7 @@
         from_result,
         to_result, //
     },
+    mfd,
     prelude::*,
     str::CStr,
     types::Opaque,
@@ -116,6 +117,9 @@ extern "C" fn probe_callback(
             let data = T::probe(pdev, info);
 
             pdev.as_ref().set_drvdata(data)?;
+
+            pdev.as_ref().mfd_add_devices(T::MFD_CELLS)?;
+
             Ok(0)
         })
     }
@@ -303,6 +307,9 @@ pub trait Driver: Send {
     /// The table of device ids supported by the driver.
     const ID_TABLE: IdTable<Self::IdInfo>;
 
+    /// The mfd cells for mfd devices.
+    const MFD_CELLS: Option<&'static [mfd::Cell]> = None;
+
     /// PCI driver probe.
     ///
     /// Called when a new pci device is added or discovered. Implementers should
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 8917d4ee499f..e2bcf8ef093c 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -25,6 +25,7 @@
         self,
         IrqRequest, //
     },
+    mfd,
     of,
     prelude::*,
     types::Opaque,
@@ -104,6 +105,9 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
             let data = T::probe(pdev, info);
 
             pdev.as_ref().set_drvdata(data)?;
+
+            pdev.as_ref().mfd_add_devices(T::MFD_CELLS)?;
+
             Ok(0)
         })
     }
@@ -218,6 +222,9 @@ pub trait Driver: Send {
     /// The table of ACPI device ids supported by the driver.
     const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None;
 
+    /// The mfd cells for mfd devices.
+    const MFD_CELLS: Option<&'static [mfd::Cell]> = None;
+
     /// Platform driver probe.
     ///
     /// Called when a new platform device is added or discovered.
diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index d9fea4bd4439..6e702c734ded 100644
--- a/rust/kernel/serdev.rs
+++ b/rust/kernel/serdev.rs
@@ -14,6 +14,7 @@
         to_result,
         VTABLE_DEFAULT_ERROR, //
     },
+    mfd,
     of,
     prelude::*,
     sync::Completion,
@@ -180,6 +181,8 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
 
             private_data.probe_complete.complete_all();
 
+            sdev.as_ref().mfd_add_devices(T::MFD_CELLS)?;
+
             result.map(|()| 0)
         })
     }
@@ -339,6 +342,9 @@ pub trait Driver: Send {
     /// The table of ACPI device ids supported by the driver.
     const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None;
 
+    /// The mfd cells for mfd devices.
+    const MFD_CELLS: Option<&'static [mfd::Cell]> = None;
+
     /// Serial device bus device driver probe.
     ///
     /// Called when a new serial device bus device is added or discovered.
diff --git a/rust/kernel/usb.rs b/rust/kernel/usb.rs
index 0e1b9a88f4f1..a64ed6a530f1 100644
--- a/rust/kernel/usb.rs
+++ b/rust/kernel/usb.rs
@@ -17,6 +17,7 @@
         from_result,
         to_result, //
     },
+    mfd,
     prelude::*,
     types::{
         AlwaysRefCounted,
@@ -96,6 +97,9 @@ extern "C" fn probe_callback(
 
             let dev: &device::Device<device::CoreInternal> = intf.as_ref();
             dev.set_drvdata(data)?;
+
+            dev.mfd_add_devices(T::MFD_CELLS)?;
+
             Ok(0)
         })
     }
@@ -309,6 +313,9 @@ pub trait Driver {
     /// The table of device ids supported by the driver.
     const ID_TABLE: IdTable<Self::IdInfo>;
 
+    /// The mfd cells for mfd devices.
+    const MFD_CELLS: Option<&'static [mfd::Cell]> = None;
+
     /// USB driver probe.
     ///
     /// Called when a new USB interface is bound to this driver.

-- 
2.52.0

[PATCH v3 3/7] acpi: add acpi_of_match_device_ids

[Markus Probst via B4 Relay]

From: Markus Probst <markus.probst@posteo.de>

Add a function to match acpi devices against of_device_ids. This will be
used in the following commit ("mfd: match acpi devices against PRP0001")
to match mfd sub-devices against a of compatible string.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/acpi/bus.c      | 7 +++++++
 include/acpi/acpi_bus.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index f6707325f582..5ddcc56edc87 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1044,6 +1044,13 @@ int acpi_match_device_ids(struct acpi_device *device,
 }
 EXPORT_SYMBOL(acpi_match_device_ids);
 
+int acpi_of_match_device_ids(struct acpi_device *device,
+			  const struct of_device_id *ids)
+{
+	return __acpi_match_device(device, NULL, ids, NULL, NULL) ? 0 : -ENOENT;
+}
+EXPORT_SYMBOL(acpi_of_match_device_ids);
+
 bool acpi_driver_match_device(struct device *dev,
 			      const struct device_driver *drv)
 {
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index aad1a95e6863..0081b9e4aaee 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -677,6 +677,8 @@ void acpi_bus_trim(struct acpi_device *start);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
 int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids);
+int acpi_of_match_device_ids(struct acpi_device *device,
+			  const struct of_device_id *ids);
 void acpi_set_modalias(struct acpi_device *adev, const char *default_id,
 		       char *modalias, size_t len);
 

-- 
2.52.0

Re: [PATCH v3 3/7] acpi: add acpi_of_match_device_ids

[Rafael J. Wysocki]

On Fri, Mar 13, 2026 at 8:03 PM Markus Probst via B4 Relay
<devnull+markus.probst.posteo.de@kernel.org> wrote:
>
> From: Markus Probst <markus.probst@posteo.de>
>
> Add a function to match acpi devices against of_device_ids. This will be
> used in the following commit ("mfd: match acpi devices against PRP0001")
> to match mfd sub-devices against a of compatible string.

Please always spell ACPI in capitals in patch subjects, comments,
changelogs, etc.  It is not a regular word.

> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  drivers/acpi/bus.c      | 7 +++++++
>  include/acpi/acpi_bus.h | 2 ++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index f6707325f582..5ddcc56edc87 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1044,6 +1044,13 @@ int acpi_match_device_ids(struct acpi_device *device,
>  }
>  EXPORT_SYMBOL(acpi_match_device_ids);
>

Missing kerneldoc.

> +int acpi_of_match_device_ids(struct acpi_device *device,
> +                         const struct of_device_id *ids)
> +{
> +       return __acpi_match_device(device, NULL, ids, NULL, NULL) ? 0 : -ENOENT;
> +}
> +EXPORT_SYMBOL(acpi_of_match_device_ids);

Are you aware of the consensus that using PRP0001 in production
platform firmware will be regarded as invalid?

Because of that, it is not an option for a driver to avoid providing
ACPI match data on a platform that uses ACPI.

> +
>  bool acpi_driver_match_device(struct device *dev,
>                               const struct device_driver *drv)
>  {
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index aad1a95e6863..0081b9e4aaee 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -677,6 +677,8 @@ void acpi_bus_trim(struct acpi_device *start);
>  acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
>  int acpi_match_device_ids(struct acpi_device *device,
>                           const struct acpi_device_id *ids);
> +int acpi_of_match_device_ids(struct acpi_device *device,
> +                         const struct of_device_id *ids);
>  void acpi_set_modalias(struct acpi_device *adev, const char *default_id,
>                        char *modalias, size_t len);
>
>
> --
> 2.52.0
>
>

Re: [PATCH v3 3/7] acpi: add acpi_of_match_device_ids

[Markus Probst]

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

On Mon, 2026-03-23 at 20:57 +0100, Rafael J. Wysocki wrote:
> On Fri, Mar 13, 2026 at 8:03 PM Markus Probst via B4 Relay
> <devnull+markus.probst.posteo.de@kernel.org> wrote:
> > 
> > From: Markus Probst <markus.probst@posteo.de>
> > 
> > Add a function to match acpi devices against of_device_ids. This will be
> > used in the following commit ("mfd: match acpi devices against PRP0001")
> > to match mfd sub-devices against a of compatible string.
> 
> Please always spell ACPI in capitals in patch subjects, comments,
> changelogs, etc.  It is not a regular word.
Ok.
> 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  drivers/acpi/bus.c      | 7 +++++++
> >  include/acpi/acpi_bus.h | 2 ++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index f6707325f582..5ddcc56edc87 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -1044,6 +1044,13 @@ int acpi_match_device_ids(struct acpi_device *device,
> >  }
> >  EXPORT_SYMBOL(acpi_match_device_ids);
> > 
> 
> Missing kerneldoc.
The same amount of kerneldoc as `acpi_match_device_ids`, if I am not
mistaken.
> 
> > +int acpi_of_match_device_ids(struct acpi_device *device,
> > +                         const struct of_device_id *ids)
> > +{
> > +       return __acpi_match_device(device, NULL, ids, NULL, NULL) ? 0 : -ENOENT;
> > +}
> > +EXPORT_SYMBOL(acpi_of_match_device_ids);
> 
> Are you aware of the consensus that using PRP0001 in production
> platform firmware will be regarded as invalid?
> 
> Because of that, it is not an option for a driver to avoid providing
> ACPI match data on a platform that uses ACPI.
First of all, the driver that would have made use of it has been
restructed to not use mfd subdevices. It would not be affected anymore
through this patch set. Not sure if I should still send it as its own
patch series though.

The device of the driver has no ACPI ID allocated by the manufacturer,
as it is only used on a proprietary Linux OS (with their own modified
kernel).

The driver would have only been useful via device tree or an ACPI
Overlay. Obviously, I don't have a PNP or ACPI Vendor ID, so I can't
assign one. The parent/main driver does only have a of compatible id.
As it needs to use PRP0001 anyway on ACPI, I thought it makes more
sense to also use PRP0001 there instead of matching it with a _ADR
which is "a grey area in the ACPI specification".

Thanks
- Markus Probst

> 
> > +
> >  bool acpi_driver_match_device(struct device *dev,
> >                               const struct device_driver *drv)
> >  {
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index aad1a95e6863..0081b9e4aaee 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -677,6 +677,8 @@ void acpi_bus_trim(struct acpi_device *start);
> >  acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
> >  int acpi_match_device_ids(struct acpi_device *device,
> >                           const struct acpi_device_id *ids);
> > +int acpi_of_match_device_ids(struct acpi_device *device,
> > +                         const struct of_device_id *ids);
> >  void acpi_set_modalias(struct acpi_device *adev, const char *default_id,
> >                        char *modalias, size_t len);
> > 
> > 
> > --
> > 2.52.0
> > 
> > 

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

Re: [PATCH v3 3/7] acpi: add acpi_of_match_device_ids

[Rafael J. Wysocki]

On Tue, Mar 24, 2026 at 4:30 PM Markus Probst <markus.probst@posteo.de> wrote:
>
> On Mon, 2026-03-23 at 20:57 +0100, Rafael J. Wysocki wrote:
> > On Fri, Mar 13, 2026 at 8:03 PM Markus Probst via B4 Relay
> > <devnull+markus.probst.posteo.de@kernel.org> wrote:
> > >
> > > From: Markus Probst <markus.probst@posteo.de>
> > >
> > > Add a function to match acpi devices against of_device_ids. This will be
> > > used in the following commit ("mfd: match acpi devices against PRP0001")
> > > to match mfd sub-devices against a of compatible string.
> >
> > Please always spell ACPI in capitals in patch subjects, comments,
> > changelogs, etc.  It is not a regular word.
> Ok.
> >
> > > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > > ---
> > >  drivers/acpi/bus.c      | 7 +++++++
> > >  include/acpi/acpi_bus.h | 2 ++
> > >  2 files changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > index f6707325f582..5ddcc56edc87 100644
> > > --- a/drivers/acpi/bus.c
> > > +++ b/drivers/acpi/bus.c
> > > @@ -1044,6 +1044,13 @@ int acpi_match_device_ids(struct acpi_device *device,
> > >  }
> > >  EXPORT_SYMBOL(acpi_match_device_ids);
> > >
> >
> > Missing kerneldoc.
> The same amount of kerneldoc as `acpi_match_device_ids`, if I am not
> mistaken.
> >
> > > +int acpi_of_match_device_ids(struct acpi_device *device,
> > > +                         const struct of_device_id *ids)
> > > +{
> > > +       return __acpi_match_device(device, NULL, ids, NULL, NULL) ? 0 : -ENOENT;
> > > +}
> > > +EXPORT_SYMBOL(acpi_of_match_device_ids);
> >
> > Are you aware of the consensus that using PRP0001 in production
> > platform firmware will be regarded as invalid?
> >
> > Because of that, it is not an option for a driver to avoid providing
> > ACPI match data on a platform that uses ACPI.
> First of all, the driver that would have made use of it has been
> restructed to not use mfd subdevices. It would not be affected anymore
> through this patch set.

So what exactly would be affected by it?

> Not sure if I should still send it as its own patch series though.
>
> The device of the driver has no ACPI ID allocated by the manufacturer,
> as it is only used on a proprietary Linux OS (with their own modified
> kernel).

Do I understand correctly that there is an ACPI platform firmware on
the board, but it doesn't enumerate the given device properly (that
is, as an ACPI device object with a specific device ID)?

In which case there probably is a driver that can find that device
somehow (it has hardcoded resources or similar).

> The driver would have only been useful via device tree or an ACPI
> Overlay.

Do you mean a custom SSDT loaded via configfs or something else?

> Obviously, I don't have a PNP or ACPI Vendor ID, so I can't
> assign one. The parent/main driver does only have a of compatible id.
> As it needs to use PRP0001 anyway on ACPI, I thought it makes more
> sense to also use PRP0001 there instead of matching it with a _ADR
> which is "a grey area in the ACPI specification".

You can't match a device with _ADR.  By itself, _ADR doesn't provide
you with any information on the device in question, it only helps to
connect it to some information that can be collected by other means.
The role of it, at least in principle, is to allow some device objects
in the ACPI hierarchy to be associated with devices enumerated by
other means (like on a PCI bus).

The enumeration with the help of PRP0001 only works if there is a
device object in the ACPI hierarchy and its _HID is PRP0001 or its
_CID list contains PRP0001, there is a _DSD under it and a
"compatible" property is returned by that _DSD.  Who's going to
provide all of that for the given device?

Moreover, if the device has some resources that the kernel needs to
know about, there should be a _CRS under the device object in question
and the resources should be listed there.  Or how are the resources
going to be found otherwise?

Re: [PATCH v3 3/7] acpi: add acpi_of_match_device_ids

[Markus Probst]

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

On Tue, 2026-03-24 at 17:01 +0100, Rafael J. Wysocki wrote:
> On Tue, Mar 24, 2026 at 4:30 PM Markus Probst <markus.probst@posteo.de> wrote:
> > 
> > On Mon, 2026-03-23 at 20:57 +0100, Rafael J. Wysocki wrote:
> > > On Fri, Mar 13, 2026 at 8:03 PM Markus Probst via B4 Relay
> > > <devnull+markus.probst.posteo.de@kernel.org> wrote:
> > > > 
> > > > From: Markus Probst <markus.probst@posteo.de>
> > > > 
> > > > Add a function to match acpi devices against of_device_ids. This will be
> > > > used in the following commit ("mfd: match acpi devices against PRP0001")
> > > > to match mfd sub-devices against a of compatible string.
> > > 
> > > Please always spell ACPI in capitals in patch subjects, comments,
> > > changelogs, etc.  It is not a regular word.
> > Ok.
> > > 
> > > > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > > > ---
> > > >  drivers/acpi/bus.c      | 7 +++++++
> > > >  include/acpi/acpi_bus.h | 2 ++
> > > >  2 files changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > > index f6707325f582..5ddcc56edc87 100644
> > > > --- a/drivers/acpi/bus.c
> > > > +++ b/drivers/acpi/bus.c
> > > > @@ -1044,6 +1044,13 @@ int acpi_match_device_ids(struct acpi_device *device,
> > > >  }
> > > >  EXPORT_SYMBOL(acpi_match_device_ids);
> > > > 
> > > 
> > > Missing kerneldoc.
> > The same amount of kerneldoc as `acpi_match_device_ids`, if I am not
> > mistaken.
> > > 
> > > > +int acpi_of_match_device_ids(struct acpi_device *device,
> > > > +                         const struct of_device_id *ids)
> > > > +{
> > > > +       return __acpi_match_device(device, NULL, ids, NULL, NULL) ? 0 : -ENOENT;
> > > > +}
> > > > +EXPORT_SYMBOL(acpi_of_match_device_ids);
> > > 
> > > Are you aware of the consensus that using PRP0001 in production
> > > platform firmware will be regarded as invalid?
> > > 
> > > Because of that, it is not an option for a driver to avoid providing
> > > ACPI match data on a platform that uses ACPI.
> > First of all, the driver that would have made use of it has been
> > restructed to not use mfd subdevices. It would not be affected anymore
> > through this patch set.
> 
> So what exactly would be affected by it?
I won't have a use for myself anymore, but I still think the patch is
useful. Anyway,

MFD Devices without an assigned ACPI ID, if they are present on devices
with ACPI platform firmware.

> 
> > Not sure if I should still send it as its own patch series though.
That is why I asked this question (see 1. sentence in the paragraph
above).

> > 
> > The device of the driver has no ACPI ID allocated by the manufacturer,
> > as it is only used on a proprietary Linux OS (with their own modified
> > kernel).
> 
> Do I understand correctly that there is an ACPI platform firmware on
> the board, but it doesn't enumerate the given device properly (that
> is, as an ACPI device object with a specific device ID)?
There is only a serial device in the ACPI platform firmware.
The device connected to the bus isn't specified.
> 
> In which case there probably is a driver that can find that device
> somehow (it has hardcoded resources or similar).
Yes, that driver has `filp_open("/dev/ttyS1")` hardcoded.
> 
> > The driver would have only been useful via device tree or an ACPI
> > Overlay.
> 
> Do you mean a custom SSDT loaded via configfs or something else?
Yes, in my case via initrd.

> 
> > Obviously, I don't have a PNP or ACPI Vendor ID, so I can't
> > assign one. The parent/main driver does only have a of compatible id.
> > As it needs to use PRP0001 anyway on ACPI, I thought it makes more
> > sense to also use PRP0001 there instead of matching it with a _ADR
> > which is "a grey area in the ACPI specification".
> 
> You can't match a device with _ADR.  By itself, _ADR doesn't provide
> you with any information on the device in question, it only helps to
> connect it to some information that can be collected by other means.
> The role of it, at least in principle, is to allow some device objects
> in the ACPI hierarchy to be associated with devices enumerated by
> other means (like on a PCI bus).
This patch affects mfd devices. A bus device can via mfd register child
devices and those child devices will be matched to a fwnode if
available.

According to commit 98a3be44ffa67b812de7aa7aed9f2331edcfb1a5, there is
a board on the market with a sub-device that will be matched using _ADR
[1].

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=98a3be44ffa67b812de7aa7aed9f2331edcfb1a5

> 
> The enumeration with the help of PRP0001 only works if there is a
> device object in the ACPI hierarchy and its _HID is PRP0001 or its
> _CID list contains PRP0001, there is a _DSD under it and a
> "compatible" property is returned by that _DSD.  Who's going to
> provide all of that for the given device?
A ACPI Overlay would do that.

> 
> Moreover, if the device has some resources that the kernel needs to
> know about, there should be a _CRS under the device object in question
> and the resources should be listed there.  Or how are the resources
> going to be found otherwise?
Resources in mfd are usually handled by the parent device, not the mfd
child device. But yes, it would be using _CRS if any.

Thanks
- Markus Probst

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

Re: [PATCH v3 3/7] acpi: add acpi_of_match_device_ids

[Rafael J. Wysocki]

On Tue, Mar 24, 2026 at 5:26 PM Markus Probst <markus.probst@posteo.de> wrote:
>
> On Tue, 2026-03-24 at 17:01 +0100, Rafael J. Wysocki wrote:
> > On Tue, Mar 24, 2026 at 4:30 PM Markus Probst <markus.probst@posteo.de> wrote:
> > >
> > > On Mon, 2026-03-23 at 20:57 +0100, Rafael J. Wysocki wrote:
> > > > On Fri, Mar 13, 2026 at 8:03 PM Markus Probst via B4 Relay
> > > > <devnull+markus.probst.posteo.de@kernel.org> wrote:
> > > > >
> > > > > From: Markus Probst <markus.probst@posteo.de>
> > > > >
> > > > > Add a function to match acpi devices against of_device_ids. This will be
> > > > > used in the following commit ("mfd: match acpi devices against PRP0001")
> > > > > to match mfd sub-devices against a of compatible string.
> > > >
> > > > Please always spell ACPI in capitals in patch subjects, comments,
> > > > changelogs, etc.  It is not a regular word.
> > > Ok.
> > > >
> > > > > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > > > > ---
> > > > >  drivers/acpi/bus.c      | 7 +++++++
> > > > >  include/acpi/acpi_bus.h | 2 ++
> > > > >  2 files changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > > > > index f6707325f582..5ddcc56edc87 100644
> > > > > --- a/drivers/acpi/bus.c
> > > > > +++ b/drivers/acpi/bus.c
> > > > > @@ -1044,6 +1044,13 @@ int acpi_match_device_ids(struct acpi_device *device,
> > > > >  }
> > > > >  EXPORT_SYMBOL(acpi_match_device_ids);
> > > > >
> > > >
> > > > Missing kerneldoc.
> > > The same amount of kerneldoc as `acpi_match_device_ids`, if I am not
> > > mistaken.
> > > >
> > > > > +int acpi_of_match_device_ids(struct acpi_device *device,
> > > > > +                         const struct of_device_id *ids)
> > > > > +{
> > > > > +       return __acpi_match_device(device, NULL, ids, NULL, NULL) ? 0 : -ENOENT;
> > > > > +}
> > > > > +EXPORT_SYMBOL(acpi_of_match_device_ids);
> > > >
> > > > Are you aware of the consensus that using PRP0001 in production
> > > > platform firmware will be regarded as invalid?
> > > >
> > > > Because of that, it is not an option for a driver to avoid providing
> > > > ACPI match data on a platform that uses ACPI.
> > > First of all, the driver that would have made use of it has been
> > > restructed to not use mfd subdevices. It would not be affected anymore
> > > through this patch set.
> >
> > So what exactly would be affected by it?
> I won't have a use for myself anymore, but I still think the patch is
> useful. Anyway,
>
> MFD Devices without an assigned ACPI ID, if they are present on devices
> with ACPI platform firmware.
>
> >
> > > Not sure if I should still send it as its own patch series though.
> That is why I asked this question (see 1. sentence in the paragraph
> above).
>
> > >
> > > The device of the driver has no ACPI ID allocated by the manufacturer,
> > > as it is only used on a proprietary Linux OS (with their own modified
> > > kernel).
> >
> > Do I understand correctly that there is an ACPI platform firmware on
> > the board, but it doesn't enumerate the given device properly (that
> > is, as an ACPI device object with a specific device ID)?
> There is only a serial device in the ACPI platform firmware.
> The device connected to the bus isn't specified.
> >
> > In which case there probably is a driver that can find that device
> > somehow (it has hardcoded resources or similar).
> Yes, that driver has `filp_open("/dev/ttyS1")` hardcoded.
> >
> > > The driver would have only been useful via device tree or an ACPI
> > > Overlay.
> >
> > Do you mean a custom SSDT loaded via configfs or something else?
> Yes, in my case via initrd.
>
> >
> > > Obviously, I don't have a PNP or ACPI Vendor ID, so I can't
> > > assign one. The parent/main driver does only have a of compatible id.
> > > As it needs to use PRP0001 anyway on ACPI, I thought it makes more
> > > sense to also use PRP0001 there instead of matching it with a _ADR
> > > which is "a grey area in the ACPI specification".
> >
> > You can't match a device with _ADR.  By itself, _ADR doesn't provide
> > you with any information on the device in question, it only helps to
> > connect it to some information that can be collected by other means.
> > The role of it, at least in principle, is to allow some device objects
> > in the ACPI hierarchy to be associated with devices enumerated by
> > other means (like on a PCI bus).
>
> This patch affects mfd devices. A bus device can via mfd register child
> devices and those child devices will be matched to a fwnode if
> available.

That only works because the parent can be recognized and properly
enumerated, so it is parent-relative.

> According to commit 98a3be44ffa67b812de7aa7aed9f2331edcfb1a5, there is
> a board on the market with a sub-device that will be matched using _ADR
> [1].

With the help of a quirk though.

> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=98a3be44ffa67b812de7aa7aed9f2331edcfb1a5
>
> >
> > The enumeration with the help of PRP0001 only works if there is a
> > device object in the ACPI hierarchy and its _HID is PRP0001 or its
> > _CID list contains PRP0001, there is a _DSD under it and a
> > "compatible" property is returned by that _DSD.  Who's going to
> > provide all of that for the given device?
> A ACPI Overlay would do that.
>
> >
> > Moreover, if the device has some resources that the kernel needs to
> > know about, there should be a _CRS under the device object in question
> > and the resources should be listed there.  Or how are the resources
> > going to be found otherwise?
> Resources in mfd are usually handled by the parent device, not the mfd
> child device. But yes, it would be using _CRS if any.

So this is kind of a valid use case, but since you don't need it any
more, I'd rather not put it in without a clear need.

Also, it's not a huge deal for a vendor to allocate a proper ACPI
device ID for a piece of hardware.  It just involves some due
diligence.

[PATCH v3 4/7] mfd: match acpi devices against PRP0001

[Markus Probst via B4 Relay]

From: Markus Probst <markus.probst@posteo.de>

PRP0001 can be used to provide DT-compatible device identification in
ACPI. If the mfd device is bound to a acpi fwnode and no acpi_match data
is provided by the driver, match against PRP0001 and the provided
`of_compatible` string.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/mfd/mfd-core.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 6be58eb5a746..29bf54664acf 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -39,6 +39,10 @@ struct match_ids_walk_data {
 	struct acpi_device_id *ids;
 	struct acpi_device *adev;
 };
+struct match_of_ids_walk_data {
+	const struct of_device_id *ids;
+	struct acpi_device *adev;
+};
 
 static int match_device_ids(struct acpi_device *adev, void *data)
 {
@@ -52,10 +56,23 @@ static int match_device_ids(struct acpi_device *adev, void *data)
 	return 0;
 }
 
+static int match_of_device_ids(struct acpi_device *adev, void *data)
+{
+	struct match_of_ids_walk_data *wd = data;
+
+	if (!acpi_of_match_device_ids(adev, wd->ids)) {
+		wd->adev = adev;
+		return 1;
+	}
+
+	return 0;
+}
+
 static void mfd_acpi_add_device(const struct mfd_cell *cell,
 				struct platform_device *pdev)
 {
 	const struct mfd_cell_acpi_match *match = cell->acpi_match;
+	const char *of_compatible = cell->of_compatible;
 	struct acpi_device *adev = NULL;
 	struct acpi_device *parent;
 
@@ -86,6 +103,16 @@ static void mfd_acpi_add_device(const struct mfd_cell *cell,
 		} else {
 			adev = acpi_find_child_device(parent, match->adr, false);
 		}
+	} else if (of_compatible) {
+		struct of_device_id ids[2] = {};
+		struct match_of_ids_walk_data wd = {
+			.adev = NULL,
+			.ids = ids,
+		};
+
+		strscpy(ids[0].compatible, of_compatible, sizeof(ids[0].compatible));
+		acpi_dev_for_each_child(parent, match_of_device_ids, &wd);
+		adev = wd.adev;
 	}
 
 	device_set_node(&pdev->dev, acpi_fwnode_handle(adev ?: parent));

-- 
2.52.0

[PATCH v3 5/7] dt-bindings: mfd: Add synology,microp device

[Markus Probst via B4 Relay]

From: Markus Probst <markus.probst@posteo.de>

Add the Synology Microp devicetree bindings. Those devices are
microcontrollers found on Synology NAS devices. They are connected to a
serial port on the host device.

Those devices are used to control certain LEDs, fan speeds, a beeper, to
handle buttons, fan failures and to properly shutdown and reboot the
device.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 .../devicetree/bindings/leds/synology,microp.yaml  | 40 +++++++++++++++++
 .../devicetree/bindings/mfd/synology,microp.yaml   | 51 ++++++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/synology,microp.yaml b/Documentation/devicetree/bindings/leds/synology,microp.yaml
new file mode 100644
index 000000000000..f7bf32c240f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/synology,microp.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/synology,microp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synology NAS on-board Microcontroller LEDs
+
+maintainers:
+  - Markus Probst <markus.probst@posteo.de>
+
+description: |
+  This is part of device tree bindings for Synology NAS on-board
+  Microcontroller.
+
+  This device can manage up to 4 leds in Synology NAS devices:
+  - Power
+  - Status
+  - Alert
+  - USB
+
+  The Power and Status LEDs are available on every Synology NAS model,
+  therefore the associated child nodes are mandatory. If the NAS also has
+  a alert or usb led, the associated child nodes need to be added.
+
+properties:
+  compatible:
+    const: synology,microp-leds
+
+patternProperties:
+  "^(power|status|alert|usb)-led$":
+    $ref: /schemas/leds/common.yaml
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - power-led
+  - status-led
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/mfd/synology,microp.yaml b/Documentation/devicetree/bindings/mfd/synology,microp.yaml
new file mode 100644
index 000000000000..57bb986d24f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/synology,microp.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/synology,microp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synology NAS on-board Microcontroller
+
+maintainers:
+  - Markus Probst <markus.probst@posteo.de>
+
+description: |
+  Synology Microp is a microcontroller found in Synology NAS devices.
+  It is connected to a serial port on the host device.
+
+  It is a multifunction device with the following sub modules:
+  - LEDs
+
+properties:
+  compatible:
+    const: synology,microp
+
+  leds:
+    $ref: /schemas/leds/synology,microp.yaml
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    mcu {
+      compatible = "synology,microp";
+
+      leds {
+        compatible = "synology,microp-leds";
+
+        power-led {
+          color = <LED_COLOR_ID_BLUE>;
+          function = LED_FUNCTION_POWER;
+        };
+
+        status-led {
+          color = <LED_COLOR_ID_MULTI>;
+          function = LED_FUNCTION_STATUS;
+        };
+      };
+    };

-- 
2.52.0

Re: [PATCH v3 5/7] dt-bindings: mfd: Add synology,microp device

[Krzysztof Kozlowski]

On 13/03/2026 20:03, Markus Probst via B4 Relay wrote:
> From: Markus Probst <markus.probst@posteo.de>
> 
> Add the Synology Microp devicetree bindings. Those devices are
> microcontrollers found on Synology NAS devices. They are connected to a
> serial port on the host device.
> 
> Those devices are used to control certain LEDs, fan speeds, a beeper, to
> handle buttons, fan failures and to properly shutdown and reboot the
> device.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---

You keep sending the same without responding to review.

NAK

Best regards,
Krzysztof

Re: [PATCH v3 5/7] dt-bindings: mfd: Add synology,microp device

[Markus Probst]

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

On Fri, 2026-03-13 at 20:37 +0100, Krzysztof Kozlowski wrote:
> On 13/03/2026 20:03, Markus Probst via B4 Relay wrote:
> > From: Markus Probst <markus.probst@posteo.de>
> > 
> > Add the Synology Microp devicetree bindings. Those devices are
> > microcontrollers found on Synology NAS devices. They are connected to a
> > serial port on the host device.
> > 
> > Those devices are used to control certain LEDs, fan speeds, a beeper, to
> > handle buttons, fan failures and to properly shutdown and reboot the
> > device.
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> 
> You keep sending the same without responding to review.
> 
> NAK
All review comments have been resolved to my knowledge, but here a
formal reply to all of them.

> A nit, subject: drop second/last, redundant "binding for". The
> "dt-bindings" prefix is already stating that these are bindings.
Has been removed from the patch subject.

> > +description: |
> Do not need '|' unless you need to preserve formatting.
It got removed in v2.
In the current patch revision v3, it is needed because it has ":" in
the description (to ensure it does not get interpreted as property).
Thus it has been readded.

> > +properties:
> > +  compatible:
> > +    enum:
> > +      - synology,microp
> Missing blank line. Look at other bindings how to write one.
Blank line has been added.

> > +  power-led:
> > +    $ref: /schemas/leds/common.yaml
> > +    unevaluatedProperties: false
> > +  status-led:
> > +    $ref: /schemas/leds/common.yaml
> > +    unevaluatedProperties: false
> > +  alert-led:
> > +    $ref: /schemas/leds/common.yaml
> > +    unevaluatedProperties: false
> > +  usb-led:
> > +    $ref: /schemas/leds/common.yaml
> > +    unevaluatedProperties: false
> That's pretty unreadable code.
>
> ... and could be simpler with patternProperties and regex
It has been minified using patternProperties.

> > +  no-check-fan:
> Vendor prefix

> > +    type: boolean
> > +    description: |
> > +      Disable fan failure check.
>
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.

> > +
> > +      The fan failure event is triggered on the device, even if
the fan
> > +      has been intentionally set to a low speed. This property
prevents a
> > +      hardware protection shutdown if a fan failure event is
reported.
> > +  no-check-cpu-fan:
>
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
The 2 properties have been removed entirely. Thus those comments are
not relevant anymore.

> > +    uart {
>
> Drop, unuesed
Has been dropped.

> > +      microp {

> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
>
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> If you cannot find a name matching your device, please check in
kernel
> sources for similar cases or you can grow the spec (via pull request
to
> DT spec repo).
node name has been changed to mcu.

> You we have tools which save you review time. Most important, save
> maintainers/reviewers time from giving feedback on obvious mistakes.
> You
> must use these tools, otherwise maintainers get grumpy by wasting
their
> time.

> Please run scripts/checkpatch.pl on the patches and fix reported
> warnings. After that, run also 'scripts/checkpatch.pl --strict' on
the
> patches and (probably) fix more warnings. Some warnings can be
ignored,
> especially from --strict run, but the code here looks like it needs a
> fix. Feel free to get in touch if the warning is not clear.
with the exception of
- help paragraph having less than 4 lines in Kconfig (not necessary in
this case)
- of_device_id not being const (it has to be)
- "added, moved or deleted file(s), does MAINTAINERS need updating?"
(file will be added in a following patch)
there are no warnings left.

> This is not an "MFD" device.
It now uses the MFD APIs. By the definiton of @Lee (assuming I
understood it correctly), this device should now qualify as "MFD"
device.

> > +
> > +    mcu {
>
> Please read previous comments.

You are likly trying to refer to this comment from you:
> Depending what this is. MCU is generic purpose unit where you load
your
> different FW for different purposes and you have here specific - to
> handle certain aspects of this entire machine. This looks like EC, so
> should be called embedded-controller and placed in that directory.
Synology uses Microchip PIC for this purpose. On a Synology DS215j, it
uses a "Microchip PIC16F1829". At least to me, this looks like a
general purpose microcontroller with firmware from synology flashed
onto it. Therefore it is a MCU.

If I did miss any relevant comments, let me know.
(Replies on replies on review comments have not been included here).

> 
> Best regards,
> Krzysztof

Thanks
- Markus Probst

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

Re: [PATCH v3 5/7] dt-bindings: mfd: Add synology,microp device

[Krzysztof Kozlowski]

On 13/03/2026 21:29, Markus Probst wrote:
> 
>> This is not an "MFD" device.
> It now uses the MFD APIs. By the definiton of @Lee (assuming I
> understood it correctly), this device should now qualify as "MFD"
> device.

No. Using Linux framework does not make this device MFD, since there is
no such term of hardware as MFD. Otherwise please explain or link to
verifiable external source describing what sort of device class is MFD,
because for sure this is not MFD how Wikipedia defines it.

> 
>>> +
>>> +    mcu {
>>
>> Please read previous comments.
> 
> You are likly trying to refer to this comment from you:
>> Depending what this is. MCU is generic purpose unit where you load
> your
>> different FW for different purposes and you have here specific - to
>> handle certain aspects of this entire machine. This looks like EC, so
>> should be called embedded-controller and placed in that directory.
> Synology uses Microchip PIC for this purpose. On a Synology DS215j, it
> uses a "Microchip PIC16F1829". At least to me, this looks like a

It does not matter what chip is used. Every component uses some sort of
chip.

> general purpose microcontroller with firmware from synology flashed
> onto it. Therefore it is a MCU.

Every chip is then an MCU with such logic. Every PMIC, every EC.

This is for me clearly embedded controller and that's where this should
be placed and called.

Best regards,
Krzysztof

Re: [PATCH v3 5/7] dt-bindings: mfd: Add synology,microp device

[Markus Probst]

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

On Sat, 2026-03-14 at 09:49 +0100, Krzysztof Kozlowski wrote:
> On 13/03/2026 21:29, Markus Probst wrote:
> > 
> > > This is not an "MFD" device.
> > It now uses the MFD APIs. By the definiton of @Lee (assuming I
> > understood it correctly), this device should now qualify as "MFD"
> > device.
> 
> No. Using Linux framework does not make this device MFD, since there is
> no such term of hardware as MFD. Otherwise please explain or link to
> verifiable external source describing what sort of device class is MFD
I assumed these comments would also apply for the dt bindings:
-
https://lore.kernel.org/rust-for-linux/DGYAFNSJ7576.1E0JZ2W499ZQ7@kernel.org/
-
https://lore.kernel.org/rust-for-linux/20260309151555.GU183676@google.com/

given that using linux MFD APIs also changes the structure of the dt
bindings with added sub-devices.

But it seems no?

> because for sure this is not MFD how Wikipedia defines it.

Wikipedia defines it as a synonym for a "multi-function
product/printer/peripheral"
https://en.wikipedia.org/wiki/Multifunction_device

> 
> > 
> > > > +
> > > > +    mcu {
> > > 
> > > Please read previous comments.
> > 
> > You are likly trying to refer to this comment from you:
> > > Depending what this is. MCU is generic purpose unit where you load
> > your
> > > different FW for different purposes and you have here specific - to
> > > handle certain aspects of this entire machine. This looks like EC, so
> > > should be called embedded-controller and placed in that directory.
> > Synology uses Microchip PIC for this purpose. On a Synology DS215j, it
> > uses a "Microchip PIC16F1829". At least to me, this looks like a
> 
> It does not matter what chip is used. Every component uses some sort of
> chip.
I would be interested in what does matter then.

I did not actually find an exact definition for what
Documentation/devicetree/bindings/mfd
and
Documentation/devicetree/bindings/embedded-controller
is for in the kernel tree or in the devicetree spec.

> 
> > general purpose microcontroller with firmware from synology flashed
> > onto it. Therefore it is a MCU.
> 
> Every chip is then an MCU with such logic. Every PMIC, every EC.
> 
> This is for me clearly embedded controller and that's where this should
> be placed and called.
In that case I will move it to
Documentation/devicetree/bindings/embedded-controller and update the
node name used in the example.

I will wait a bit for the other patches to be reviewed before sending a
next revision.

But I wonder how
Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
got in there then, given it is pretty similar to this device in the
functionality it provides.

> 
> Best regards,
> Krzysztof

Thanks
- Markus Probst

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

Re: [PATCH v3 5/7] dt-bindings: mfd: Add synology,microp device

[Krzysztof Kozlowski]

On 14/03/2026 13:31, Markus Probst wrote:
> On Sat, 2026-03-14 at 09:49 +0100, Krzysztof Kozlowski wrote:
>> On 13/03/2026 21:29, Markus Probst wrote:
>>>
>>>> This is not an "MFD" device.
>>> It now uses the MFD APIs. By the definiton of @Lee (assuming I
>>> understood it correctly), this device should now qualify as "MFD"
>>> device.
>>
>> No. Using Linux framework does not make this device MFD, since there is
>> no such term of hardware as MFD. Otherwise please explain or link to
>> verifiable external source describing what sort of device class is MFD
> I assumed these comments would also apply for the dt bindings:
> -
> https://lore.kernel.org/rust-for-linux/DGYAFNSJ7576.1E0JZ2W499ZQ7@kernel.org/
> -
> https://lore.kernel.org/rust-for-linux/20260309151555.GU183676@google.com/

I don't understand your question. We talk here about bindings, so why do
you ask if the comments are about bindings?

> 
> given that using linux MFD APIs also changes the structure of the dt
> bindings with added sub-devices.
> 
> But it seems no?

> 
>> because for sure this is not MFD how Wikipedia defines it.
> 
> Wikipedia defines it as a synonym for a "multi-function
> product/printer/peripheral"
> https://en.wikipedia.org/wiki/Multifunction_device

I know, not need to state obvious. And this is not a printer.



> 
>>
>>>
>>>>> +
>>>>> +    mcu {
>>>>
>>>> Please read previous comments.
>>>
>>> You are likly trying to refer to this comment from you:
>>>> Depending what this is. MCU is generic purpose unit where you load
>>> your
>>>> different FW for different purposes and you have here specific - to
>>>> handle certain aspects of this entire machine. This looks like EC, so
>>>> should be called embedded-controller and placed in that directory.
>>> Synology uses Microchip PIC for this purpose. On a Synology DS215j, it
>>> uses a "Microchip PIC16F1829". At least to me, this looks like a
>>
>> It does not matter what chip is used. Every component uses some sort of
>> chip.
> I would be interested in what does matter then.
> 
> I did not actually find an exact definition for what
> Documentation/devicetree/bindings/mfd

Because there is no such hardware as MFD.

> and
> Documentation/devicetree/bindings/embedded-controller
> is for in the kernel tree or in the devicetree spec.

Commit msg moving several devices there explained, no?


> 
>>
>>> general purpose microcontroller with firmware from synology flashed
>>> onto it. Therefore it is a MCU.
>>
>> Every chip is then an MCU with such logic. Every PMIC, every EC.
>>
>> This is for me clearly embedded controller and that's where this should
>> be placed and called.
> In that case I will move it to
> Documentation/devicetree/bindings/embedded-controller and update the
> node name used in the example.
> 
> I will wait a bit for the other patches to be reviewed before sending a
> next revision.
> 
> But I wonder how
> Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
> got in there then, given it is pretty similar to this device in the
> functionality it provides.

Great question. How do any bugs, mistakes, different judgments or
imperfectness got merged?  How is it possible that code for example is
reviewed but has a bug? Don't ever use arguments that something
somewhere happened, so you can do the same.

Not mentioning that if you even question this, you could at least look
at the history which would tell you if "embedded-controller" directory
existed that time or not.

Best regards,
Krzysztof

Re: [PATCH v3 5/7] dt-bindings: mfd: Add synology,microp device

[Markus Probst]

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

On Sat, 2026-03-14 at 14:59 +0100, Krzysztof Kozlowski wrote:
> On 14/03/2026 13:31, Markus Probst wrote:
> > On Sat, 2026-03-14 at 09:49 +0100, Krzysztof Kozlowski wrote:
> > > On 13/03/2026 21:29, Markus Probst wrote:
> > > > 
> > > > > This is not an "MFD" device.
> > > > It now uses the MFD APIs. By the definiton of @Lee (assuming I
> > > > understood it correctly), this device should now qualify as "MFD"
> > > > device.
> > > 
> > > No. Using Linux framework does not make this device MFD, since there is
> > > no such term of hardware as MFD. Otherwise please explain or link to
> > > verifiable external source describing what sort of device class is MFD
> > I assumed these comments would also apply for the dt bindings:
> > -
> > https://lore.kernel.org/rust-for-linux/DGYAFNSJ7576.1E0JZ2W499ZQ7@kernel.org/
> > -
> > https://lore.kernel.org/rust-for-linux/20260309151555.GU183676@google.com/
> 
> I don't understand your question. We talk here about bindings, so why do
> you ask if the comments are about bindings?
> 
> > 
> > given that using linux MFD APIs also changes the structure of the dt
> > bindings with added sub-devices.
> > 
> > But it seems no?
> 
> > 
> > > because for sure this is not MFD how Wikipedia defines it.
> > 
> > Wikipedia defines it as a synonym for a "multi-function
> > product/printer/peripheral"
> > https://en.wikipedia.org/wiki/Multifunction_device
> 
> I know, not need to state obvious. And this is not a printer.
> 
> 
> 
> > 
> > > 
> > > > 
> > > > > > +
> > > > > > +    mcu {
> > > > > 
> > > > > Please read previous comments.
> > > > 
> > > > You are likly trying to refer to this comment from you:
> > > > > Depending what this is. MCU is generic purpose unit where you load
> > > > your
> > > > > different FW for different purposes and you have here specific - to
> > > > > handle certain aspects of this entire machine. This looks like EC, so
> > > > > should be called embedded-controller and placed in that directory.
> > > > Synology uses Microchip PIC for this purpose. On a Synology DS215j, it
> > > > uses a "Microchip PIC16F1829". At least to me, this looks like a
> > > 
> > > It does not matter what chip is used. Every component uses some sort of
> > > chip.
> > I would be interested in what does matter then.
> > 
> > I did not actually find an exact definition for what
> > Documentation/devicetree/bindings/mfd
> 
> Because there is no such hardware as MFD.
> 
> > and
> > Documentation/devicetree/bindings/embedded-controller
> > is for in the kernel tree or in the devicetree spec.
> 
> Commit msg moving several devices there explained, no?
yes.

> 
> 
> > 
> > > 
> > > > general purpose microcontroller with firmware from synology flashed
> > > > onto it. Therefore it is a MCU.
> > > 
> > > Every chip is then an MCU with such logic. Every PMIC, every EC.
> > > 
> > > This is for me clearly embedded controller and that's where this should
> > > be placed and called.
> > In that case I will move it to
> > Documentation/devicetree/bindings/embedded-controller and update the
> > node name used in the example.
> > 
> > I will wait a bit for the other patches to be reviewed before sending a
> > next revision.
> > 
> > But I wonder how
> > Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
> > got in there then, given it is pretty similar to this device in the
> > functionality it provides.
> 
> Great question. How do any bugs, mistakes, different judgments or
> imperfectness got merged?  How is it possible that code for example is
> reviewed but has a bug? Don't ever use arguments that something
> somewhere happened, so you can do the same.
I was not trying to use this as an argument. As you can see above I
already agreed to put it into embedded-controller.

I was interested in knowing, if there was a difference between the two
devices I did not know of, as the bindings for this device go into
another directory despite the similarity (no need to answer it anymore,
see below).

But I see why that hasn't been clear.

> 
> Not mentioning that if you even question this, you could at least look
> at the history which would tell you if "embedded-controller" directory
> existed that time or not.
yes, that would have made my question entirely obsolete.

> 
> Best regards,
> Krzysztof

Thanks
- Markus Probst

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

[PATCH v3 6/7] mfd: Add synology microp core driver

[Markus Probst via B4 Relay]

From: Markus Probst <markus.probst@posteo.de>

Add a synology microp core driver, written in Rust.
The driver targets a microcontroller found in Synology NAS devices. It
only provides the base for sub-devices, like LEDs.

Tested successfully on a Synology DS923+.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 MAINTAINERS                    |  7 +++++++
 drivers/mfd/Kconfig            | 11 ++++++++++
 drivers/mfd/Makefile           |  2 ++
 drivers/mfd/synology_microp.rs | 46 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fa49e40836ab..32932ecab9cf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25539,6 +25539,13 @@ F:	drivers/dma-buf/sync_*
 F:	include/linux/sync_file.h
 F:	include/uapi/linux/sync_file.h
 
+SYNOLOGY MICROP DRIVER
+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/mfd/synology_microp.rs
+
 SYNOPSYS ARC ARCHITECTURE
 M:	Vineet Gupta <vgupta@kernel.org>
 L:	linux-snps-arc@lists.infradead.org
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 7192c9d1d268..8b8b391d1b47 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1320,6 +1320,17 @@ config MFD_SY7636A
 	  To enable support for building sub-devices as modules,
 	  choose M here.
 
+config MFD_SYNOLOGY_MICROP
+	tristate "Synology Microp core driver"
+	depends on RUST
+	select RUST_SERIAL_DEV_BUS_ABSTRACTIONS
+	select MFD_CORE
+	help
+	  Enable support for the MCU found in Synology NAS devices.
+
+	  This driver only provides the base for sub-devices. For additional
+	  functionality, you have to enable support for the sub-devices as well.
+
 config MFD_RDC321X
 	tristate "RDC R-321x southbridge"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e75e8045c28a..f754be7163cd 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -304,3 +304,5 @@ obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu_spi.o rsmu_core.o
 obj-$(CONFIG_MFD_UPBOARD_FPGA)	+= upboard-fpga.o
 
 obj-$(CONFIG_MFD_LOONGSON_SE)	+= loongson-se.o
+
+obj-$(CONFIG_MFD_SYNOLOGY_MICROP)	+= synology_microp.o
diff --git a/drivers/mfd/synology_microp.rs b/drivers/mfd/synology_microp.rs
new file mode 100644
index 000000000000..b9b5fff5c18d
--- /dev/null
+++ b/drivers/mfd/synology_microp.rs
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Synology Microp core driver
+
+use kernel::{
+    device,
+    mfd,
+    of,
+    prelude::*,
+    serdev, //
+};
+
+kernel::module_serdev_device_driver! {
+    type: SynologyMicropCoreDriver,
+    name: "synology_microp",
+    authors: ["Markus Probst <markus.probst@posteo.de>"],
+    description: "Synology Microp core driver",
+    license: "GPL v2",
+}
+
+struct SynologyMicropCoreDriver;
+
+kernel::of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <SynologyMicropCoreDriver as serdev::Driver>::IdInfo,
+    [(of::DeviceId::new(c"synology,microp"), ()),]
+);
+
+#[vtable]
+impl serdev::Driver for SynologyMicropCoreDriver {
+    type IdInfo = ();
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+    const MFD_CELLS: Option<&'static [mfd::Cell]> =
+        Some(&[mfd::Cell::new(c"synology_microp_leds").of(c"synology,microp-leds", None)]);
+
+    fn probe(
+        dev: &serdev::Device<device::Core>,
+        _id_info: Option<&Self::IdInfo>,
+    ) -> impl PinInit<Self, kernel::error::Error> {
+        let _ = dev.set_baudrate(9600);
+        dev.set_flow_control(false);
+        dev.set_parity(serdev::Parity::None)?;
+        Ok(Self)
+    }
+}

-- 
2.52.0

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

[Markus Probst via B4 Relay]

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.0

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

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

[Markus Probst]

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

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

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

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

[Markus Probst]

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

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

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

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

[Markus Probst]

[-- 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 --]

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

[Danilo Krummrich]

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*

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

[Greg Kroah-Hartman]

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

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

[Markus Probst]

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

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

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

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

[Greg Kroah-Hartman]

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

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

[Markus Probst]

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

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

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