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

[Christian Marangi <ansuelsmth@gmail.com>]

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!

-- 
	Ansuel