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

[Markus Probst]

On Tue, 2026-04-21 at 21:10 +0300, Ilpo Järvinen wrote:
> On Tue, 21 Apr 2026, Markus Probst wrote:
> 
> > 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@xxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxx>
> > > 
> > > You should probably add:
> > > 
> > > L:	platform-driver-x86@xxxxxxxxxxxxxxx
> > > 
> > > Through which tree the patches to this driver are generally expected to be 
> > > picked up?
> > 
> > I suppose platform-drivers-x86.
> 
> Okay (with the platform drivers maintainer hat on). Just don't expect me 
> to have deep Rust knowledge.
> 
> > 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). 
> 
> In practice platform drivers scope has already expanded beyond x86 so the 
> platform-drivers-x86 list naming is just a historic artifact.
Does this also include the drivers/platform/x86 folder?

Because of the multiple architectures, I put it into the root, i. e.
drivers/platform/synology_microp/

Is this fine or should I move it into drivers/platform/x86 ?

> 
> > > > +S:	Maintained
> > > > +F:	Documentation/devicetree/bindings/embedded-controller/synology,ds1825p-microp.yaml
> > > > +F:	drivers/platform/synology_microp/
> > > > +
> > > >  SYNOPSYS ARC ARCHITECTURE
> > > >  M:	Vineet Gupta <vgupta@xxxxxxxxxx>
> > > >  L:	linux-snps-arc@xxxxxxxxxxxxxxxxxxx
> 
> > > > 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.
> 
> Lets not include the TODO file then.
> 
> > > > +/// 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.
> 
> I don't have opinion on this with my limited Rust knowledge (it just 
> stuck to my eye how non-specific that original one looked). If Rust 
> can do things even better as Miguel seems to imply, please look at those 
> directions.
Delta stores it in nano seconds, so it will require an additional
`.as_millis()` call on use. I assume rust will optimize that out, so it
will be fine. I will use the `Delta` type like Miguel suggested in the
next revision.

Thanks
- Markus Probst

Attachment: signature.asc
Description: This is a digitally signed message part