Re: [PATCH v4 1/2] dt-bindings: embedded-controller: Add synology,microp device

[Rob Herring]

On Thu, Mar 26, 2026 at 8:02 AM Markus Probst <markus.probst@xxxxxxxxx> wrote:
>
> On Wed, 2026-03-25 at 17:07 -0500, Rob Herring wrote:
> > On Sat, Mar 21, 2026 at 01:02:22PM +0000, Markus Probst wrote:
> > > On Sat, 2026-03-21 at 13:32 +0100, Krzysztof Kozlowski wrote:
> > > > On 21/03/2026 13:17, Markus Probst wrote:
> > > > > On Sat, 2026-03-21 at 11:21 +0100, Krzysztof Kozlowski wrote:
> > > > > > On Fri, Mar 20, 2026 at 11:09:53PM +0100, Markus Probst wrote:
> > > > > > > +
> > > > > > > +examples:
> > > > > > > +  - |
> > > > > > > +    #include <dt-bindings/leds/common.h>
> > > > > > > +
> > > > > > > +    embedded-controller {
> > > > > > > +      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;
> > > > > > > +      };
> > > > > >
> > > > > > Where are other leds? Binding mentions 4.
> > > > > >
> > > > > Status and Power leds exist on every Synology NAS model I am aware of.
> > > > > But there are models which have additionally a usb or alert led. The
> > > > > device nodes for those leds should only be present, if they exist
> > > > > physically on the device.
> > > >
> > > > Then help me to understand - are these different models?
> > > Yes, even with different CPU architectures.
> > > How much the "microp" device differs is not clear, but the
> > > communication protocol is the same.
> > > >
> > > > EC is not a generic purpose component and is tightly coupled with the
> > > > actual board it is being present on. Unless exactly same board is used
> > > > in different models (unlikely) then the compatible defines the LEDs and
> > > > they are not needed in DT.
> > > So for instance "synology,ds923p-microp", "synology,ds723p-microp" etc.
> > > ?
> > >
> > > I can do that, but that would be many.
> >
> > How many is many?
> Estimated 300.

Okay, that's a lot and probably safe to say there are not 300
variations of the EC.

> As a side note: I only have 1 model I can test the driver with.
> >
> > > Having it generic seems more flexible.
> >
> > Is there firmware for these ECs? If so is it the same or different
> > firmware for each device? If the former or the functionality is really
> > trivial, then I'd be more comfortable with 1 or a few compatibles.
> The firmware is not public and the exact differences between them isn't
> documented. The communication protocol is the same though.
>
> >
> > Generic means you'll need to add quirk properties when there is some
> > difference the OS needs to handle which we'll reject. So stuck with one
> > compatible and no way to distinguish different ECs is anything but
> > flexible.
> Describing the physical leds that are present on the NAS device are not
> quirk properties, at least in my definition.

That's not what I mean. I mean things like this other device needs
some different timing for power-on/reset or delays between accesses or
some LED control is inverted or some protocol difference... Could be
about anything. The key thing is you have specific enough information
(compatible) to start with that you can handle any issue that comes up
*without* changing the DT.

As you said, you only have 1 device. Make the binding specific to that
1 device. If the next one that comes along can reuse the binding as
it, then great. Nothing to do. If it can't, then it gets its own new
compatible. Strictly speaking we would add a new compatible for each
device, but it's a judgement call that there aren't going to be
differences to handle. In this case, there likely aren't 300 versions
of h/w, the functionality is simple enough, and the functionality is
entirely optional (just a guess). But that's all really your argument
to make.

Rob