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

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

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

On Mon, 2026-04-06 at 16:42 +0200, Krzysztof Kozlowski wrote:
> On 06/04/2026 16:22, Markus Probst wrote:
> > 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?
> 
> I do not see how this list resolves my comment. Really, explain my how
> listing part of binding answers WHY they are not compatible?
I might have misunderstood it then, my apologies for that.

> 
> 
> > 
> > [1]
> > https://lore.kernel.org/all/20260330-delicate-sassy-mayfly-ebcca7@quoll/
> > 
> > > 
> > > > 
> > > > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > > > ---
> > > >  .../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@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 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.
> 
> Either this drivers works or not. If it works, explain me how they are
> not compatible.
I will use my ds923+ as baseline here.

Known relevant differences that make them not compatible:
ds918p:
- no system current sensor
ds214play:
- has an cpu fan
- has only 1 fan
- no system current sensor
ds225p:
- has usb copy led and button
- has only 1 fan
- no system current sensor
ds415p:
- no system current sensor
ds710p:
- has esata led
- has only 1 fan
- no system current sensor
ds1010p:
- has alert led controlled via microp
- no system current sensor
ds723p
- has only 1 fan
ds725p:
- has only 1 fan
- no system current sensor
ds1522p: <no relevant known difference>
rs422p:
- power led is green
- has 3 fans
rs822p:
- power led is green
- supports fan rpm report via an adt7475 chip and therefore does not
have gpios for fan failure
- no system current sensor
ds118, ds124, ds223j:
- different shutdown behaviour
- has only 1 fan
- no system current sensor
ds223:
- different shutdown behaviour
- has usb copy led and button
- has only 1 fan
- no system current sensor
rs1221p, rs1221rpp:
- power led is green
- supports fan rpm report via an adt7475 chip and therefore does not
have gpios for fan failure
- no system current sensor
ds925p, ds1525p, ds1825p, ds1823xsp:
- supports fan rpm report via an adt7475 chip and therefore does not
have gpios for fan failure
- no system current sensor

Thanks
- Markus Probst

> 
> > 
> > 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.
> 
> I did not object that.
> 
> 
> Best regards,
> Krzysztof

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