Re: [PATCH v6 2/2] dt-bindings: embedded-controller: Add synology microp devices

[Markus Probst]

On Mon, 2026-04-06 at 09:59 +0200, Krzysztof Kozlowski wrote:
> On Sun, Apr 05, 2026 at 07:36:29PM +0200, Markus Probst wrote:
> > 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.
> > 
> > This includes the following compatible ids:
> >  - synology,ds923p-microp
> >  - synology,ds918p-microp
> >  - synology,ds214play-microp
> >  - synology,ds225p-microp
> >  - synology,ds425p-microp
> >  - synology,ds710p-microp
> >  - synology,ds1010p-microp
> >  - synology,ds723p-microp
> >  - synology,ds1522p-microp
> >  - synology,rs422p-microp
> >  - synology,ds725p-microp
> >  - synology,ds118-microp
> >  - synology,ds124-microp
> >  - synology,ds223-microp
> >  - synology,ds223j-microp
> >  - synology,ds1823xsp-microp
> >  - synology,rs822p-microp
> >  - synology,rs1221p-microp
> >  - synology,rs1221rpp-microp
> >  - synology,ds925p-microp
> >  - synology,ds1525p-microp
> >  - synology,ds1825p-microp
> 
> Drop, we see this in the diff.
A prior review commit suggested I should add them [1].
So only synology,ds923p-microp in the Subject then?

[1]
https://lore.kernel.org/all/20260330-delicate-sassy-mayfly-ebcca7@quoll/

> 
> > 
> > Signed-off-by: Markus Probst <markus.probst@xxxxxxxxx>
> > ---
> >  .../synology,ds923p-microp.yaml                    | 112 +++++++++++++++++++++
> >  MAINTAINERS                                        |   1 +
> >  2 files changed, 113 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/embedded-controller/synology,ds923p-microp.yaml b/Documentation/devicetree/bindings/embedded-controller/synology,ds923p-microp.yaml
> > new file mode 100644
> > index 000000000000..4518e9b74be1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/embedded-controller/synology,ds923p-microp.yaml
> > @@ -0,0 +1,112 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/embedded-controller/synology,ds923p-microp.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Synology NAS on-board Microcontroller
> > +
> > +maintainers:
> > +  - Markus Probst <markus.probst@xxxxxxxxx>
> > +
> > +description: |
> > +  Synology Microp is a microcontroller found in Synology NAS devices.
> > +  It is connected to a serial port on the host device.
> > +
> > +  It is necessary to properly shutdown and reboot the NAS device and
> > +  provides additional functionality such as led control, fan speed control,
> > +  a beeper and buttons on the NAS device.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - synology,ds923p-microp
> > +      - synology,ds918p-microp
> > +      - synology,ds214play-microp
> > +      - synology,ds225p-microp
> > +      - synology,ds425p-microp
> > +      - synology,ds710p-microp
> > +      - synology,ds1010p-microp
> > +      - synology,ds723p-microp
> > +      - synology,ds1522p-microp
> > +      - synology,rs422p-microp
> > +      - synology,ds725p-microp
> > +      - synology,ds118-microp
> > +      - synology,ds124-microp
> > +      - synology,ds223-microp
> > +      - synology,ds223j-microp
> > +      - synology,ds1823xsp-microp
> > +      - synology,rs822p-microp
> > +      - synology,rs1221p-microp
> > +      - synology,rs1221rpp-microp
> > +      - synology,ds925p-microp
> > +      - synology,ds1525p-microp
> > +      - synology,ds1825p-microp
> 
> So we already talked about this and you were told to use compatibility.
> Your driver clearly states several of these are compatible, so I am
> confused that I do not see it expressed here.
The driver does not have all functionality implemented yet.

A few examples of differences not yet visible in the driver:
- synology,ds214play-microp is the only model in the current list to
have an cpu fan
- 4 of the models are arm based and need a different shutdown behaviour
- different amount of fans (already present in the binding via fan-
failure-gpios)

I could try to group them together, but Synology does not document the
exact difference between them.

As Rob mentioned [2], I need to be able to handle unexpected
differences without qurik properties.

[2]
https://lore.kernel.org/all/CAL_JsqJUVh1YnhmYYj4ara5BheaLOL1oayjtWNuPH53q1d4xXA@xxxxxxxxxxxxxx/

> 
> > +
> > +  fan-failure-gpios:
> > +    description: GPIOs needed to determine which fans stopped working on a fan failure event.
> > +    minItems: 2
> > +    maxItems: 3
> > +
> > +required:
> > +  - compatible
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - synology,ds214play-microp
> > +              - synology,ds225p-microp
> > +              - synology,ds710p-microp
> > +              - synology,ds723p-microp
> > +              - synology,ds725p-microp
> > +              - synology,ds118-microp
> > +              - synology,ds124-microp
> > +              - synology,ds223-microp
> > +              - synology,ds223j-microp
> > +              - synology,ds1823xsp-microp
> > +              - synology,rs822p-microp
> > +              - synology,rs1221p-microp
> > +              - synology,rs1221rpp-microp
> > +              - synology,ds1825p-microp
> > +    then:
> > +      properties:
> > +        fan-failure-gpios: false
> > +    else:
> > +      required:
> > +        - fan-failure-gpios
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    embedded-controller {
> > +      compatible = "synology,ds923p-microp";
> > +
> > +      fan-failure-gpios = <&gpio 68 GPIO_ACTIVE_HIGH>, <&gpio 69 GPIO_ACTIVE_HIGH>;
> 
> Keep only one example, they are basically the same. Difference in one
> property does not need a new example.
Ok, it seemed like a nice convenient way to automatically test the if
blocks.

Thanks
- Markus Probst

> 
> Best regards,
> Krzysztof

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