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

[Sander Vanheule]

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