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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

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

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.

> > > +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/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.

-- 
 i.