[v7,11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example

  Add LEDs definition example for qca8k using the offload trigger as the
default trigger and add all the supported offload triggers by the
switch.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../devicetree/bindings/net/dsa/qca8k.yaml    | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)
  

On Thu, Dec 15, 2022 at 12:54:38AM +0100, Christian Marangi wrote:
> Add LEDs definition example for qca8k using the offload trigger as the
> default trigger and add all the supported offload triggers by the
> switch.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/dsa/qca8k.yaml    | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> index 978162df51f7..4090cf65c41c 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> @@ -65,6 +65,8 @@ properties:
>                   internal mdio access is used.
>                   With the legacy mapping the reg corresponding to the internal
>                   mdio is the switch reg with an offset of -1.
> +                 Each phy have at least 3 LEDs connected and can be declared
> +                 using the standard LEDs structure.
>  
>  patternProperties:
>    "^(ethernet-)?ports$":
> @@ -202,6 +204,7 @@ examples:
>      };
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/leds/common.h>
>  
>      mdio {
>          #address-cells = <1>;
> @@ -284,6 +287,27 @@ examples:
>  
>                  internal_phy_port1: ethernet-phy@0 {
>                      reg = <0>;
> +
> +                    leds {
> +                        #address-cells = <1>;
> +                        #size-cells = <0>;
> +
> +                        led@0 {
> +                            reg = <0>;
> +                            color = <LED_COLOR_ID_WHITE>;
> +                            function = LED_FUNCTION_LAN;
> +                            function-enumerator = <1>;
> +                            linux,default-trigger = "netdev";

'function' should replace this. Don't encourage more users. 

Also, 'netdev' is not documented which leaves me wondering why there's 
no warning? Either this patch didn't apply or there's a problem in the 
schema that's not checking this node.

> +                        };
> +
> +                        led@1 {
> +                            reg = <1>;
> +                            color = <LED_COLOR_ID_AMBER>;
> +                            function = LED_FUNCTION_LAN;
> +                            function-enumerator = <1>;

Typo? These are supposed to be unique. Can't you use 'reg' in your case?


> +                            linux,default-trigger = "netdev";
> +                        };
> +                    };
>                  };
>  
>                  internal_phy_port2: ethernet-phy@1 {
> -- 
> 2.37.2
> 
>
  

On Tue, Dec 20, 2022 at 11:39:58AM -0600, Rob Herring wrote:
> On Thu, Dec 15, 2022 at 12:54:38AM +0100, Christian Marangi wrote:
> > Add LEDs definition example for qca8k using the offload trigger as the
> > default trigger and add all the supported offload triggers by the
> > switch.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../devicetree/bindings/net/dsa/qca8k.yaml    | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > index 978162df51f7..4090cf65c41c 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > @@ -65,6 +65,8 @@ properties:
> >                   internal mdio access is used.
> >                   With the legacy mapping the reg corresponding to the internal
> >                   mdio is the switch reg with an offset of -1.
> > +                 Each phy have at least 3 LEDs connected and can be declared
> > +                 using the standard LEDs structure.
> >  
> >  patternProperties:
> >    "^(ethernet-)?ports$":
> > @@ -202,6 +204,7 @@ examples:
> >      };
> >    - |
> >      #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/leds/common.h>
> >  
> >      mdio {
> >          #address-cells = <1>;
> > @@ -284,6 +287,27 @@ examples:
> >  
> >                  internal_phy_port1: ethernet-phy@0 {
> >                      reg = <0>;
> > +
> > +                    leds {
> > +                        #address-cells = <1>;
> > +                        #size-cells = <0>;
> > +
> > +                        led@0 {
> > +                            reg = <0>;
> > +                            color = <LED_COLOR_ID_WHITE>;
> > +                            function = LED_FUNCTION_LAN;
> > +                            function-enumerator = <1>;
> > +                            linux,default-trigger = "netdev";
> 
> 'function' should replace this. Don't encourage more users. 
> 
> Also, 'netdev' is not documented which leaves me wondering why there's 
> no warning? Either this patch didn't apply or there's a problem in the 
> schema that's not checking this node.

It is probably the usual limitation that the tools require a
compatible, where as the kernel does not.

> > +                        };
> > +
> > +                        led@1 {
> > +                            reg = <1>;
> > +                            color = <LED_COLOR_ID_AMBER>;
> > +                            function = LED_FUNCTION_LAN;
> > +                            function-enumerator = <1>;
> 
> Typo? These are supposed to be unique. Can't you use 'reg' in your case?

reg in this context is the address of the PHY on the MDIO bus. This is
an Ethernet switch, so has many PHYs, each with its own address.

   Andrew
  

On Thu, Dec 15, 2022 at 12:54:38AM +0100, Christian Marangi wrote:
> Add LEDs definition example for qca8k using the offload trigger as the
> default trigger and add all the supported offload triggers by the
> switch.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../devicetree/bindings/net/dsa/qca8k.yaml    | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> index 978162df51f7..4090cf65c41c 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> @@ -65,6 +65,8 @@ properties:
>                   internal mdio access is used.
>                   With the legacy mapping the reg corresponding to the internal
>                   mdio is the switch reg with an offset of -1.
> +                 Each phy have at least 3 LEDs connected and can be declared
> +                 using the standard LEDs structure.
>  
>  patternProperties:
>    "^(ethernet-)?ports$":
> @@ -202,6 +204,7 @@ examples:
>      };
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/leds/common.h>
>  
>      mdio {
>          #address-cells = <1>;
> @@ -284,6 +287,27 @@ examples:
>  
>                  internal_phy_port1: ethernet-phy@0 {
>                      reg = <0>;
> +
> +                    leds {
> +                        #address-cells = <1>;
> +                        #size-cells = <0>;
> +
> +                        led@0 {
> +                            reg = <0>;
> +                            color = <LED_COLOR_ID_WHITE>;
> +                            function = LED_FUNCTION_LAN;
> +                            function-enumerator = <1>;
> +                            linux,default-trigger = "netdev";
> +                        };
> +
> +                        led@1 {
> +                            reg = <1>;
> +                            color = <LED_COLOR_ID_AMBER>;
> +                            function = LED_FUNCTION_LAN;
> +                            function-enumerator = <1>;
> +                            linux,default-trigger = "netdev";
> +                        };
> +                    };
>                  };

I don't see anything here which is specific to the QCA8K. I really
hope the same binding should work for any PHY which has LEDs. So
please move this into ethernet-phy.yaml.

       Andrew
  

> > > +                        };
> > > +
> > > +                        led@1 {
> > > +                            reg = <1>;
> > > +                            color = <LED_COLOR_ID_AMBER>;
> > > +                            function = LED_FUNCTION_LAN;
> > > +                            function-enumerator = <1>;
> > 
> > Typo? These are supposed to be unique. Can't you use 'reg' in your case?
> 
> reg in this context is the address of the PHY on the MDIO bus. This is
> an Ethernet switch, so has many PHYs, each with its own address.

Actually, i'm wrong about that. reg in this context is the LED number
of the PHY. Typically there are 2 or 3 LEDs per PHY.

There is no reason the properties need to be unique. Often the LEDs
have 8 or 16 functions, identical for each LED, but with different
reset defaults so they show different things.

   Andrew
  

On Wed, Dec 21, 2022 at 02:41:50AM +0100, Andrew Lunn wrote:
> > > > +                        };
> > > > +
> > > > +                        led@1 {
> > > > +                            reg = <1>;
> > > > +                            color = <LED_COLOR_ID_AMBER>;
> > > > +                            function = LED_FUNCTION_LAN;
> > > > +                            function-enumerator = <1>;
> > > 
> > > Typo? These are supposed to be unique. Can't you use 'reg' in your case?
> > 
> > reg in this context is the address of the PHY on the MDIO bus. This is
> > an Ethernet switch, so has many PHYs, each with its own address.
> 
> Actually, i'm wrong about that. reg in this context is the LED number
> of the PHY. Typically there are 2 or 3 LEDs per PHY.
> 
> There is no reason the properties need to be unique. Often the LEDs
> have 8 or 16 functions, identical for each LED, but with different
> reset defaults so they show different things.
> 

Are we taking about reg or function-enumerator?

For reg it's really specific to the driver... My idea was that since a
single phy can have multiple leds attached, reg will represent the led
number.

This is an example of the dt implemented on a real device.

		mdio {
			#address-cells = <1>;
			#size-cells = <0>;

			phy_port1: phy@0 {
				reg = <0>;

				leds {
					#address-cells = <1>;
					#size-cells = <0>;

					lan1_led@0 {
						reg = <0>;
						color = <LED_COLOR_ID_WHITE>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <1>;
						linux,default-trigger = "netdev";
					};

					lan1_led@1 {
						reg = <1>;
						color = <LED_COLOR_ID_AMBER>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <1>;
						linux,default-trigger = "netdev";
					};
				};
			};

			phy_port2: phy@1 {
				reg = <1>;

				leds {
					#address-cells = <1>;
					#size-cells = <0>;


					lan2_led@0 {
						reg = <0>;
						color = <LED_COLOR_ID_WHITE>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <2>;
						linux,default-trigger = "netdev";
					};

					lan2_led@1 {
						reg = <1>;
						color = <LED_COLOR_ID_AMBER>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <2>;
						linux,default-trigger = "netdev";
					};
				};
			};

			phy_port3: phy@2 {
				reg = <2>;

				leds {
					#address-cells = <1>;
					#size-cells = <0>;

					lan3_led@0 {
						reg = <0>;
						color = <LED_COLOR_ID_WHITE>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <3>;
						linux,default-trigger = "netdev";
					};

					lan3_led@1 {
						reg = <1>;
						color = <LED_COLOR_ID_AMBER>;
						function = LED_FUNCTION_LAN;
						function-enumerator = <3>;
						linux,default-trigger = "netdev";
					};
				};
			};

In the following implementation. Each port have 2 leds attached (out of
3) one white and one amber. The driver parse the reg and calculate the
offset to set the correct option with the regs by also checking the phy
number.

An alternative way would be set the reg to be the global led number in
the switch and deatch the phy from the calculation.

Something like
port 0 led 0 = reg 0
port 0 led 1 = reg 1
port 1 led 0 = reg 2
port 1 led 1 = reg 3
...

Using the function-enumerator can be problematic since ideally someone
would declare a dedicated function for wan led.

I'm very open to discuss and improve/fix this!
  

> An alternative way would be set the reg to be the global led number in
> the switch and deatch the phy from the calculation.
> 
> Something like
> port 0 led 0 = reg 0
> port 0 led 1 = reg 1
> port 1 led 0 = reg 2
> port 1 led 1 = reg 3

I would not do this. It will make LED controllers embedded in switches
different to LEDs controllers embedded in PHYs. Ideally we want them
identical. One binding to rule them all.

	   Andrew
  

On Wed, Dec 21, 2022 at 6:55 AM Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Wed, Dec 21, 2022 at 02:41:50AM +0100, Andrew Lunn wrote:
> > > > > +                        };
> > > > > +
> > > > > +                        led@1 {
> > > > > +                            reg = <1>;
> > > > > +                            color = <LED_COLOR_ID_AMBER>;
> > > > > +                            function = LED_FUNCTION_LAN;
> > > > > +                            function-enumerator = <1>;
> > > >
> > > > Typo? These are supposed to be unique. Can't you use 'reg' in your case?
> > >
> > > reg in this context is the address of the PHY on the MDIO bus. This is
> > > an Ethernet switch, so has many PHYs, each with its own address.
> >
> > Actually, i'm wrong about that. reg in this context is the LED number
> > of the PHY. Typically there are 2 or 3 LEDs per PHY.
> >
> > There is no reason the properties need to be unique. Often the LEDs
> > have 8 or 16 functions, identical for each LED, but with different
> > reset defaults so they show different things.
> >
>
> Are we taking about reg or function-enumerator?
>
> For reg it's really specific to the driver... My idea was that since a
> single phy can have multiple leds attached, reg will represent the led
> number.
>
> This is an example of the dt implemented on a real device.
>
>                 mdio {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>
>                         phy_port1: phy@0 {
>                                 reg = <0>;
>
>                                 leds {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>
>                                         lan1_led@0 {
>                                                 reg = <0>;
>                                                 color = <LED_COLOR_ID_WHITE>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <1>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>
>                                         lan1_led@1 {
>                                                 reg = <1>;
>                                                 color = <LED_COLOR_ID_AMBER>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <1>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>                                 };
>                         };
>
>                         phy_port2: phy@1 {
>                                 reg = <1>;
>
>                                 leds {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>
>
>                                         lan2_led@0 {
>                                                 reg = <0>;
>                                                 color = <LED_COLOR_ID_WHITE>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <2>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>
>                                         lan2_led@1 {
>                                                 reg = <1>;
>                                                 color = <LED_COLOR_ID_AMBER>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <2>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>                                 };
>                         };
>
>                         phy_port3: phy@2 {
>                                 reg = <2>;
>
>                                 leds {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
>
>                                         lan3_led@0 {
>                                                 reg = <0>;
>                                                 color = <LED_COLOR_ID_WHITE>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <3>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>
>                                         lan3_led@1 {
>                                                 reg = <1>;
>                                                 color = <LED_COLOR_ID_AMBER>;
>                                                 function = LED_FUNCTION_LAN;
>                                                 function-enumerator = <3>;
>                                                 linux,default-trigger = "netdev";
>                                         };
>                                 };
>                         };
>
> In the following implementation. Each port have 2 leds attached (out of
> 3) one white and one amber. The driver parse the reg and calculate the
> offset to set the correct option with the regs by also checking the phy
> number.

Okay, the full example makes more sense. But I still thought
'function-enumerator' values should be globally unique within a value
of 'function'. Maybe Jacek has an opinion on this?

You are using it to distinguish phys/ports, but there's already enough
information in the DT to do that. You have the parent nodes and I
assume you have port numbers under 'ethernet-ports'. For each port,
get the phy node and then get the LEDs.

> An alternative way would be set the reg to be the global led number in
> the switch and deatch the phy from the calculation.
>
> Something like
> port 0 led 0 = reg 0
> port 0 led 1 = reg 1
> port 1 led 0 = reg 2
> port 1 led 1 = reg 3
> ...

No.

Rob
  

Hi Christian,

On Wed, 2022-12-21 at 13:54 +0100, Christian Marangi wrote:
> For reg it's really specific to the driver... My idea was that since a
> single phy can have multiple leds attached, reg will represent the led
> number.
> 
> This is an example of the dt implemented on a real device.
> 
>                 mdio {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> 
>                         phy_port1: phy@0 {
>                                 reg = <0>;
> 
>                                 leds {
>                                         #address-cells = <1>;
>                                         #size-cells = <0>;
[...]
>                                 };
>                         };
[...]
>                 };
> 
> In the following implementation. Each port have 2 leds attached (out of
> 3) one white and one amber. The driver parse the reg and calculate the
> offset to set the correct option with the regs by also checking the phy
> number.

With switch silicon allowing user control of the LEDs, vendors can (and will)
use the switch's LED peripheral to drive other LEDs (or worse). E.g. on a Cisco
SG220-26 switch, using a Realtek RTL8382 SoC, the LEDs associated with some
unused switch ports are used to display a global device status. My concern here
is that one would have to specify switch ports, that aren't connected to
anything, just to describe those non-ethernet LEDs.

Would an alternative with a 'trigger-sources' property pointing to the right phy
be an option? The trade-off I see would be that extra port info has to be
provided on a separate LED controller, which your example can avoid thanks to
the phy's reg property.

Building on your example this may become:

       switch {
           mdio {
                #address-cells = <1>;
                #size-cells = <0>;
                
                switch_phy0: phy@0 {
                    reg = <0>;
                    #trigger-source-cells = <1>;
                };
            };

            leds {
                #address-cells = <2>;
                #size-cells = <0>;

                /* First port, first LED */
                /* Port status, can be offloaded */
                led@0.0 {
                    reg = <0 0>;
                    trigger-sources = <&switch_phy0 (NET_LINK | NET_SPEED_1000)>;
                    function = color = <LED_COLOR_ID_WHITE>;
                    function = LED_FUNCTION_LAN;
                    function-enumerator = <1>;
                    linux,default-trigger = "netdev";
                };

                /* First port, first LED */
                /* Port status, can be offloaded */
                led@0.1 {
                    reg = <0 1>;
                    trigger-sources = <&switch_phy0 (NET_LINK | NET_SPEED_100 | NET_SPEED_10)>;
                    function = color = <LED_COLOR_ID_AMBER>;
                    function = LED_FUNCTION_LAN;
                    function-enumerator = <1>;
                    linux,default-trigger = "netdev";
                };

                /* Last port (not used in hardware), first LED */
                /* Device status, software controlled */
                led@7.0 {
                    reg = <7 0>;
                    function = color = <LED_COLOR_ID_AMBER>;
                    function = LED_FUNCTION_STATUS;
                    linux,default-trigger = "default-on";
                };
            };
        };


To be a bit less verbose, the &switch_mdio node might serve as trigger provider
with a single cell, but the above would allow only defined phy-s to be
referenced.

The trigger-source cells could be used for a more fine grained control of what
should be offloaded (link up/down, Rx/Tx activity, link speed, ...). Although
this selectivity is most likely runtime configurable, this could serve as a
description of static device labeling (e.g. "LINK/ACT 1000").

Switching to the implementation and driver side, the 'trigger-sources' property
could be used by the netdev trigger to determine if a status LED can be
offloaded. The netdev trigger could just hide the whole hardware/software
control aspect then. Much like how the timer trigger always offloads if an
implementation is provided, even when offloading is less flexible than the
software implementation of the timer trigger.


Best,
Sander
  

> > This is an example of the dt implemented on a real device.
> > 
> >                 mdio {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> > 
> >                         phy_port1: phy@0 {
> >                                 reg = <0>;
> > 
> >                                 leds {
> >                                         #address-cells = <1>;
> >                                         #size-cells = <0>;
> [...]
> >                                 };
> >                         };
> [...]
> >                 };
> > 
> > In the following implementation. Each port have 2 leds attached (out of
> > 3) one white and one amber. The driver parse the reg and calculate the
> > offset to set the correct option with the regs by also checking the phy
> > number.
> 
> With switch silicon allowing user control of the LEDs, vendors can (and will)
> use the switch's LED peripheral to drive other LEDs (or worse). E.g. on a Cisco
> SG220-26 switch, using a Realtek RTL8382 SoC, the LEDs associated with some
> unused switch ports are used to display a global device status. My concern here
> is that one would have to specify switch ports, that aren't connected to
> anything, just to describe those non-ethernet LEDs.

Note that the binding is adding properties to the PHY nodes, not the
switch port nodes. Is this how the RTL8382 works? Marvell Switches
have LED registers which are not in the PHY register space.

But the point is, the PHYs will probe if listed. They don't have to
have a MAC pointing to them with a phandle. So the phydev will exist,
and that should be enough to get the LED class device registered. If
there is basic on/off support, that should be enough for you to attach
the Morse code panic trigger, the heartbeat handler, or any other LED
trigger.

	Andrew
  

Hi Andrew,

On Sun, 2023-01-29 at 23:02 +0100, Andrew Lunn wrote:
> > > This is an example of the dt implemented on a real device.
> > > 
> > >                 mdio {
> > >                         #address-cells = <1>;
> > >                         #size-cells = <0>;
> > > 
> > >                         phy_port1: phy@0 {
> > >                                 reg = <0>;
> > > 
> > >                                 leds {
> > >                                         #address-cells = <1>;
> > >                                         #size-cells = <0>;
> > [...]
> > >                                 };
> > >                         };
> > [...]
> > >                 };
> > > 
> > > In the following implementation. Each port have 2 leds attached (out of
> > > 3) one white and one amber. The driver parse the reg and calculate the
> > > offset to set the correct option with the regs by also checking the phy
> > > number.
> > 
> > With switch silicon allowing user control of the LEDs, vendors can (and
> > will)
> > use the switch's LED peripheral to drive other LEDs (or worse). E.g. on a
> > Cisco
> > SG220-26 switch, using a Realtek RTL8382 SoC, the LEDs associated with some
> > unused switch ports are used to display a global device status. My concern
> > here
> > is that one would have to specify switch ports, that aren't connected to
> > anything, just to describe those non-ethernet LEDs.
> 
> Note that the binding is adding properties to the PHY nodes, not the
> switch port nodes. Is this how the RTL8382 works? Marvell Switches
> have LED registers which are not in the PHY register space.

Thanks for the quick clarification. Because you mention this, I realised that
the RTL8382's LED controller is actually not in the PHYs. These SoCs use
external PHYs, which may have their own, independent, LED controllers. For
example the RTL8212D [1].

[1]
https://datasheet.lcsc.com/lcsc/2203252253_Realtek-Semicon-RTL8218D-CG_C2901898.pdf

> 
> But the point is, the PHYs will probe if listed. They don't have to
> have a MAC pointing to them with a phandle. So the phydev will exist,
> and that should be enough to get the LED class device registered. If
> there is basic on/off support, that should be enough for you to attach
> the Morse code panic trigger, the heartbeat handler, or any other LED
> trigger.

OK, this makes sense for (external) PHYs which need to be probed anyway to have
access to the LEDs.

Looking at the RTL8212D's datasheet (Table 11, p. 24), it appears to be possible
to assign an LED to any of the eight PHYs. Perhaps to allow more freedom in the
board layout. Maybe I'm just not seeing it, but I don't think the example with
an 'leds' node under a PHY contains enough information to perform such a non-
trivial mapping. On the other hand, I'm not sure where else that info might go.
Maybe a 'trigger-sources' property cross-referencing another PHY in the same
package?

Best,
Sander
  

> Thanks for the quick clarification. Because you mention this, I realised that
> the RTL8382's LED controller is actually not in the PHYs. These SoCs use
> external PHYs, which may have their own, independent, LED controllers. For
> example the RTL8212D [1].
> 
> [1]
> https://datasheet.lcsc.com/lcsc/2203252253_Realtek-Semicon-RTL8218D-CG_C2901898.pdf
> 
> > 
> > But the point is, the PHYs will probe if listed. They don't have to
> > have a MAC pointing to them with a phandle. So the phydev will exist,
> > and that should be enough to get the LED class device registered. If
> > there is basic on/off support, that should be enough for you to attach
> > the Morse code panic trigger, the heartbeat handler, or any other LED
> > trigger.
> 
> OK, this makes sense for (external) PHYs which need to be probed anyway to have
> access to the LEDs.
> 
> Looking at the RTL8212D's datasheet (Table 11, p. 24), it appears to be possible
> to assign an LED to any of the eight PHYs. Perhaps to allow more freedom in the
> board layout. Maybe I'm just not seeing it, but I don't think the example with
> an 'leds' node under a PHY contains enough information to perform such a non-
> trivial mapping. On the other hand, I'm not sure where else that info might go.

The binding is defining all the generic properties need for generic
PHY LED. For most PHYs, it is probably sufficient. However, there is
nothing stopping you from adding PHY specific properties. So for
example, for each PHY LED you could have a property which maps it to
a LED00-LED35.

So propose a binding for the RTL8218D with whatever extra properties
you think are needed, and it will be reviewed in the normal way.

    Andrew