Re: [PATCH 1/2] dt-bindings: mfd: Add binding for synology,microp devices

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

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

On Sat, 2026-03-07 at 11:20 +0100, Krzysztof Kozlowski wrote:
> On 06/03/2026 20:38, 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.
> 
> A nit, subject: drop second/last, redundant "binding for". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> > 
> > 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.
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  .../devicetree/bindings/mfd/synology,microp.yaml   | 75 ++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/synology,microp.yaml b/Documentation/devicetree/bindings/mfd/synology,microp.yaml
> > new file mode 100644
> > index 000000000000..0fcb0b750bf0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/synology,microp.yaml
> > @@ -0,0 +1,75 @@
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/synology,microp.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Synology NAS on-board Microcontroller
> > +
> > +maintainers:
> > +  - Markus Probst <markus.probst@posteo.de>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +  Synology devices contain a microcontroller on their device to control
> > +  certain leds, fan speeds, a beeper, to properly handle system shutdown
> > +  and reboot, buttons and fan failures.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - synology,microp
> 
> Missing blank line. Look at other bindings how to write one.
> 
> > +  power-led:
> > +    $ref: /schemas/leds/common.yaml
> > +    unevaluatedProperties: false
> > +  status-led:
> > +    $ref: /schemas/leds/common.yaml
> > +    unevaluatedProperties: false
> > +  alert-led:
> > +    $ref: /schemas/leds/common.yaml
> > +    unevaluatedProperties: false
> > +  usb-led:
> > +    $ref: /schemas/leds/common.yaml
> > +    unevaluatedProperties: false
> 
> That's pretty unreadable code.
> 
> > +  no-check-fan:
> 
> Vendor prefix
> 
> > +    type: boolean
> > +    description: |
> > +      Disable fan failure check.
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
> 
> > +
> > +      The fan failure event is triggered on the device, even if the fan
> > +      has been intentionally set to a low speed. This property prevents a
> > +      hardware protection shutdown if a fan failure event is reported.
> > +  no-check-cpu-fan:
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
> 
> > +    type: boolean
> > +    description: |
> > +      Disable cpu fan failure check.
> > +
> > +      The cpu fan failure event is triggered on the device, even if the cpu
> > +      fan has been intentionally set to a low speed. This property prevents
> > +      a hardware protection shutdown if a cpu fan failure event is
> > +      reported.
> > +
> > +required:
> > +  - compatible
> > +  - power-led
> > +  - status-led
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    uart {
> 
> Drop, unuesed
The device uses the serial device bus. Thus the device node needs to be
inside a uart device node. The uart node represents a serial device
port in that example.

This is also done the same way in
- Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml
- Documentation/devicetree/bindings/net/nfc/marvell,nci.yaml
- Documentation/devicetree/bindings/net/bluetooth/brcm,bluetooth.yaml

> 
> > +      microp {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> If you cannot find a name matching your device, please check in kernel
> sources for similar cases or you can grow the spec (via pull request to
> DT spec repo).
"mcu" seems the most fitting, altough it is not part of the list.
It it used here though:
- Documentation/devicetree/bindings/mfd/qnap,ts433-mcu.yaml

> 
> > +        compatible = "synology,microp";
> > +
> > +        power-led {
> > +          color = <LED_COLOR_ID_BLUE>;
> > +          function = LED_FUNCTION_POWER;
> > +        };
> > +
> > +        status-led {
> > +          color = <LED_COLOR_ID_MULTI>;
> > +          function = LED_FUNCTION_STATUS;
> > +        };
> > +      };
> > +    };
> > 
> 
> 
> Best regards,
> Krzysztof

Thanks
- Markus Probst

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