Re: [PATCH v3 5/7] dt-bindings: mfd: Add synology,microp device

[Markus Probst]

On Fri, 2026-03-13 at 20:37 +0100, Krzysztof Kozlowski wrote:
> On 13/03/2026 20:03, Markus Probst via B4 Relay wrote:
> > From: Markus Probst <markus.probst@xxxxxxxxx>
> > 
> > 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.
> > 
> > Signed-off-by: Markus Probst <markus.probst@xxxxxxxxx>
> > ---
> 
> You keep sending the same without responding to review.
> 
> NAK
All review comments have been resolved to my knowledge, but here a
formal reply to all of them.

> A nit, subject: drop second/last, redundant "binding for". The
> "dt-bindings" prefix is already stating that these are bindings.
Has been removed from the patch subject.

> > +description: |
> Do not need '|' unless you need to preserve formatting.
It got removed in v2.
In the current patch revision v3, it is needed because it has ":" in
the description (to ensure it does not get interpreted as property).
Thus it has been readded.

> > +properties:
> > +  compatible:
> > +    enum:
> > +      - synology,microp
> Missing blank line. Look at other bindings how to write one.
Blank line has been added.

> > +  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.
>
> ... and could be simpler with patternProperties and regex
It has been minified using patternProperties.

> > +  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.
The 2 properties have been removed entirely. Thus those comments are
not relevant anymore.

> > +    uart {
>
> Drop, unuesed
Has been dropped.

> > +      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).
node name has been changed to mcu.

> You we have tools which save you review time. Most important, save
> maintainers/reviewers time from giving feedback on obvious mistakes.
> You
> must use these tools, otherwise maintainers get grumpy by wasting
their
> time.

> Please run scripts/checkpatch.pl on the patches and fix reported
> warnings. After that, run also 'scripts/checkpatch.pl --strict' on
the
> patches and (probably) fix more warnings. Some warnings can be
ignored,
> especially from --strict run, but the code here looks like it needs a
> fix. Feel free to get in touch if the warning is not clear.
with the exception of
- help paragraph having less than 4 lines in Kconfig (not necessary in
this case)
- of_device_id not being const (it has to be)
- "added, moved or deleted file(s), does MAINTAINERS need updating?"
(file will be added in a following patch)
there are no warnings left.

> This is not an "MFD" device.
It now uses the MFD APIs. By the definiton of @Lee (assuming I
understood it correctly), this device should now qualify as "MFD"
device.

> > +
> > +    mcu {
>
> Please read previous comments.

You are likly trying to refer to this comment from you:
> Depending what this is. MCU is generic purpose unit where you load
your
> different FW for different purposes and you have here specific - to
> handle certain aspects of this entire machine. This looks like EC, so
> should be called embedded-controller and placed in that directory.
Synology uses Microchip PIC for this purpose. On a Synology DS215j, it
uses a "Microchip PIC16F1829". At least to me, this looks like a
general purpose microcontroller with firmware from synology flashed
onto it. Therefore it is a MCU.

If I did miss any relevant comments, let me know.
(Replies on replies on review comments have not been included here).

> 
> Best regards,
> Krzysztof

Thanks
- Markus Probst

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