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: 5478 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

In the documentation it says

"
Nonstandard property names should specify a unique string prefix, such
as a stock ticker symbol, identifying the name of the company or
organization that defined the property.
"

I am not sure whether to use synology or linux as vendor prefix here.

The property itself was not defined by synology,

but the property also is not necessarily specific to linux.
The driver has to tell the device whether it should even check for fan
failures. The property wouldn't alter behaviour of the driver, but of
the device. The device itself would not check for fan failures anymore.

Thanks
- Markus Probst

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

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