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

[Krzysztof Kozlowski <krzk@kernel.org>]

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

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

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