On Wed, Dec 21, 2022 at 6:55 AM Christian Marangi [off-list ref] wrote:

On Wed, Dec 21, 2022 at 02:41:50AM +0100, Andrew Lunn wrote:

quoted

quoted

quoted

quoted

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