Re: [PATCH v8 2/2] platform: Add initial synology microp driver

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

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

On Tue, 2026-04-21 at 14:59 +0300, Ilpo Järvinen wrote:
> On Mon, 20 Apr 2026, Markus Probst wrote:
> 
> > Add a initial synology microp driver, written in Rust.
> > The driver targets a microcontroller found in Synology NAS devices. It
> > currently only supports controlling of the power led, status led, alert
> > led and usb led. Other components such as fan control or handling
> > on-device buttons will be added once the required rust abstractions are
> > there.
> > 
> > This driver can be used both on arm and x86, thus it goes into the root
> > directory of drivers/platform.
> > 
> > Tested successfully on a Synology DS923+.
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  MAINTAINERS                                        |   6 +
> >  drivers/platform/Kconfig                           |   2 +
> >  drivers/platform/Makefile                          |   1 +
> >  drivers/platform/synology_microp/Kconfig           |  13 +
> >  drivers/platform/synology_microp/Makefile          |   3 +
> >  drivers/platform/synology_microp/TODO              |   7 +
> >  drivers/platform/synology_microp/command.rs        |  54 ++++
> >  drivers/platform/synology_microp/led.rs            | 281 +++++++++++++++++++++
> >  drivers/platform/synology_microp/model.rs          |  49 ++++
> >  .../platform/synology_microp/synology_microp.rs    | 110 ++++++++
> >  10 files changed, 526 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c1c686846cdd..49f08290eed0 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -25555,6 +25555,12 @@ 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>
> 
> You should probably add:
> 
> L:	platform-driver-x86@vger.kernel.org
> 
> Through which tree the patches to this driver are generally expected to be 
> picked up?
I suppose platform-drivers-x86. The driver itself can be used both on
x86 and arm64. Although I also have seen Synology devices with PowerPC
(no device with PowerPC is supported in the driver yet). 

> 
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/embedded-controller/synology,ds1825p-microp.yaml
> > +F:	drivers/platform/synology_microp/
> > +
> >  SYNOPSYS ARC ARCHITECTURE
> >  M:	Vineet Gupta <vgupta@kernel.org>
> >  L:	linux-snps-arc@lists.infradead.org
> > diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
> > index 312788f249c9..996050566a4a 100644
> > --- a/drivers/platform/Kconfig
> > +++ b/drivers/platform/Kconfig
> > @@ -22,3 +22,5 @@ source "drivers/platform/arm64/Kconfig"
> >  source "drivers/platform/raspberrypi/Kconfig"
> >  
> >  source "drivers/platform/wmi/Kconfig"
> > +
> > +source "drivers/platform/synology_microp/Kconfig"
> > diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
> > index fa322e7f8716..2381872e9133 100644
> > --- a/drivers/platform/Makefile
> > +++ b/drivers/platform/Makefile
> > @@ -15,3 +15,4 @@ obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
> >  obj-$(CONFIG_ARM64_PLATFORM_DEVICES)	+= arm64/
> >  obj-$(CONFIG_BCM2835_VCHIQ)	+= raspberrypi/
> >  obj-$(CONFIG_ACPI_WMI)		+= wmi/
> > +obj-$(CONFIG_SYNOLOGY_MICROP)	+= synology_microp/
> > diff --git a/drivers/platform/synology_microp/Kconfig b/drivers/platform/synology_microp/Kconfig
> > new file mode 100644
> > index 000000000000..7c4d8f2808f0
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +config SYNOLOGY_MICROP
> > +	tristate "Synology Microp driver"
> > +	depends on LEDS_CLASS && LEDS_CLASS_MULTICOLOR
> > +	depends on RUST_SERIAL_DEV_BUS_ABSTRACTIONS
> > +	help
> > +	  Enable support for the MCU found in Synology NAS devices.
> > +
> > +	  This is needed to properly shutdown and reboot the device, as well as
> > +	  additional functionality like fan and LED control.
> > +
> > +	  This driver is work in progress and may not be fully functional.
> > diff --git a/drivers/platform/synology_microp/Makefile b/drivers/platform/synology_microp/Makefile
> > new file mode 100644
> > index 000000000000..63585ccf76e4
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +obj-y += synology_microp.o
> > diff --git a/drivers/platform/synology_microp/TODO b/drivers/platform/synology_microp/TODO
> > new file mode 100644
> > index 000000000000..1961a33115db
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/TODO
> > @@ -0,0 +1,7 @@
> > +TODO:
> > +- add missing components:
> > +  - handle on-device buttons (Power, Factory reset, "USB Copy")
> > +  - handle fan failure
> > +  - beeper
> > +  - fan speed control
> > +  - correctly perform device power-off and restart on Synology devices
> 
> Is this TODO list really needed within the kernel distribution?
Not really. Although it indicates the current state of the driver.

> 
> If you planning on add these features (relatively) soon yourself (perhaps 
> depending on when the rust infra required for these features becomes 
> available), the list would not be that useful for other developers at all.
Yes. Also I haven't seen anyone work on input, hwmon, reboot/sysoff
rust abstractions yet, so I will likely need to add those as well.

> 
> > diff --git a/drivers/platform/synology_microp/command.rs b/drivers/platform/synology_microp/command.rs
> > new file mode 100644
> > index 000000000000..430cb858e1c3
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/command.rs
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +use kernel::{
> > +    device::Bound,
> > +    error::Result,
> > +    serdev, //
> > +};
> > +
> > +use crate::led;
> > +
> > +#[expect(
> > +    clippy::enum_variant_names,
> > +    reason = "future variants will not end with Led"
> > +)]
> > +pub(crate) enum Command {
> > +    PowerLed(led::State),
> > +    StatusLed(led::StatusLedColor, led::State),
> > +    AlertLed(led::State),
> > +    UsbLed(led::State),
> > +    EsataLed(led::State),
> > +}
> > +
> > +impl Command {
> > +    pub(crate) fn write(self, dev: &serdev::Device<Bound>) -> Result {
> > +        dev.write_all(
> > +            match self {
> > +                Self::PowerLed(led::State::On) => &[0x34],
> > +                Self::PowerLed(led::State::Blink) => &[0x35],
> > +                Self::PowerLed(led::State::Off) => &[0x36],
> > +
> > +                Self::StatusLed(_, led::State::Off) => &[0x37],
> > +                Self::StatusLed(led::StatusLedColor::Green, led::State::On) => &[0x38],
> > +                Self::StatusLed(led::StatusLedColor::Green, led::State::Blink) => &[0x39],
> > +                Self::StatusLed(led::StatusLedColor::Orange, led::State::On) => &[0x3A],
> > +                Self::StatusLed(led::StatusLedColor::Orange, led::State::Blink) => &[0x3B],
> > +
> > +                Self::AlertLed(led::State::On) => &[0x4C, 0x41, 0x31],
> > +                Self::AlertLed(led::State::Blink) => &[0x4C, 0x41, 0x32],
> > +                Self::AlertLed(led::State::Off) => &[0x4C, 0x41, 0x33],
> > +
> > +                Self::UsbLed(led::State::On) => &[0x40],
> > +                Self::UsbLed(led::State::Blink) => &[0x41],
> > +                Self::UsbLed(led::State::Off) => &[0x42],
> > +
> > +                Self::EsataLed(led::State::On) => &[0x4C, 0x45, 0x31],
> > +                Self::EsataLed(led::State::Blink) => &[0x4C, 0x45, 0x32],
> > +                Self::EsataLed(led::State::Off) => &[0x4C, 0x45, 0x33],
> > +            },
> > +            serdev::Timeout::Max,
> > +        )?;
> > +        dev.wait_until_sent(serdev::Timeout::Max);
> > +        Ok(())
> > +    }
> > +}
> > diff --git a/drivers/platform/synology_microp/led.rs b/drivers/platform/synology_microp/led.rs
> > new file mode 100644
> > index 000000000000..f89998a7e6b4
> > --- /dev/null
> > +++ b/drivers/platform/synology_microp/led.rs
> > @@ -0,0 +1,281 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +use kernel::{
> > +    device::Bound,
> > +    devres::{
> > +        self,
> > +        Devres, //
> > +    },
> > +    led::{
> > +        self,
> > +        LedOps,
> > +        MultiColorSubLed, //
> > +    },
> > +    new_mutex,
> > +    prelude::*,
> > +    serdev,
> > +    str::CString,
> > +    sync::Mutex, //
> > +};
> > +use pin_init::pin_init_scope;
> > +
> > +use crate::{
> > +    command::Command,
> > +    model::Model, //
> > +};
> > +
> > +#[pin_data]
> > +pub(crate) struct Data {
> > +    #[pin]
> > +    status: Devres<led::MultiColorDevice<StatusLedHandler>>,
> > +    power_name: CString,
> > +    #[pin]
> > +    power: Devres<led::Device<LedHandler>>,
> > +}
> > +
> > +impl Data {
> > +    pub(super) fn register<'a>(
> > +        dev: &'a serdev::Device<Bound>,
> > +        model: &'a Model,
> > +    ) -> impl PinInit<Self, Error> + 'a {
> > +        pin_init_scope(move || {
> > +            if let Some(color) = model.led_alert {
> > +                let name = CString::try_from_fmt(fmt!("{}:alarm", color.as_c_str().to_str()?))?;
> > +                devres::register(
> > +                    dev.as_ref(),
> > +                    led::DeviceBuilder::new().color(color).name(&name).build(
> > +                        dev,
> > +                        try_pin_init!(LedHandler {
> > +                            blink <- new_mutex!(false),
> > +                            command: Command::AlertLed,
> > +                        }),
> > +                    ),
> > +                    GFP_KERNEL,
> > +                )?;
> > +            }
> > +
> > +            if model.led_usb_copy {
> > +                devres::register(
> > +                    dev.as_ref(),
> > +                    led::DeviceBuilder::new()
> > +                        .color(led::Color::Green)
> > +                        .name(c"green:usb")
> > +                        .build(
> > +                            dev,
> > +                            try_pin_init!(LedHandler {
> > +                                blink <- new_mutex!(false),
> > +                                command: Command::UsbLed,
> > +                            }),
> > +                        ),
> > +                    GFP_KERNEL,
> > +                )?;
> > +            }
> > +
> > +            if model.led_esata {
> > +                devres::register(
> > +                    dev.as_ref(),
> > +                    led::DeviceBuilder::new()
> > +                        .color(led::Color::Green)
> > +                        .name(c"green:esata")
> > +                        .build(
> > +                            dev,
> > +                            try_pin_init!(LedHandler {
> > +                                blink <- new_mutex!(false),
> > +                                command: Command::EsataLed,
> > +                            }),
> > +                        ),
> > +                    GFP_KERNEL,
> > +                )?;
> > +            }
> > +
> > +            Ok(try_pin_init!(Self {
> > +                status <- led::DeviceBuilder::new()
> > +                    .color(led::Color::Multi)
> > +                    .name(c"multicolor:status")
> > +                    .build_multicolor(
> > +                        dev,
> > +                        try_pin_init!(StatusLedHandler {
> > +                            blink <- new_mutex!(false),
> > +                        }),
> > +                        StatusLedHandler::SUBLEDS,
> > +                    ),
> > +                power_name: CString::try_from_fmt(fmt!(
> > +                    "{}:power",
> > +                    model.led_power.as_c_str().to_str()?
> > +                ))?,
> > +                power <- led::DeviceBuilder::new()
> > +                    .color(model.led_power)
> > +                    .name(power_name)
> > +                    .build(
> > +                        dev,
> > +                        try_pin_init!(LedHandler {
> > +                            blink <- new_mutex!(true),
> > +                            command: Command::PowerLed,
> > +                        }),
> > +                    ),
> > +            }))
> > +        })
> > +    }
> > +}
> > +
> > +#[derive(Copy, Clone)]
> > +pub(crate) enum StatusLedColor {
> > +    Green,
> > +    Orange,
> > +}
> > +
> > +#[derive(Copy, Clone)]
> > +pub(crate) enum State {
> > +    On,
> > +    Blink,
> > +    Off,
> > +}
> > +
> > +#[pin_data]
> > +struct LedHandler {
> > +    #[pin]
> > +    blink: Mutex<bool>,
> > +    command: fn(State) -> Command,
> > +}
> > +
> > +/// Blink delay measured using video recording on DS923+ for Power and Status Led.
> > +///
> > +/// We assume it is the same for all other leds and models.
> > +const BLINK_DELAY: usize = 167;
> 
> On C side time related consts are required to include the unit in their 
> name. Perhaps Rust code should also follow this convention?
How about `const BLINK_DELAY: Msecs` ? The unit would be implied
through the already existing type alias `kernel::time::Msecs` for u32.

Thanks
- Markus Probst

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