Re: [net-next PATCH v4 10/14] dt-bindings: net: dsa: qca8k: add LEDs definition example

[Christian Marangi <ansuelsmth@gmail.com>]

On Fri, Mar 17, 2023 at 09:14:10AM +0100, Marek Behún wrote:
> Hello Christian, also Rob Herring, Andrew Lunn and Pavel Machek,
> 
> On Fri, 17 Mar 2023 03:31:21 +0100
> Christian Marangi <ansuelsmth@gmail.com> wrote:
> 
> > Add LEDs definition example for qca8k Switch Family to describe how they
> > should be defined for a correct usage.
> > 
> > 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 389892592aac..2e9c14af0223 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml
> > @@ -18,6 +18,8 @@ description:
> >    PHY it is connected to. In this config, an internal mdio-bus is registered and
> >    the MDIO master is used for communication. Mixed external and internal
> >    mdio-bus configurations are not supported by the hardware.
> > +  Each phy has at least 3 LEDs connected and can be declared
> > +  using the standard LEDs structure.
> >  
> >  properties:
> >    compatible:
> > @@ -117,6 +119,7 @@ unevaluatedProperties: false
> >  examples:
> >    - |
> >      #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/leds/common.h>
> >  
> >      mdio {
> >          #address-cells = <1>;
> > @@ -226,6 +229,27 @@ examples:
> >                      label = "lan1";
> >                      phy-mode = "internal";
> >                      phy-handle = <&internal_phy_port1>;
> > +
> > +                    leds {
> > +                        #address-cells = <1>;
> > +                        #size-cells = <0>;
> > +
> > +                        led@0 {
> > +                            reg = <0>;
> > +                            color = <LED_COLOR_ID_WHITE>;
> > +                            function = LED_FUNCTION_LAN;
> > +                            function-enumerator = <1>;
> > +                            default-state = "keep";
> > +                        };
> > +
> > +                        led@1 {
> > +                            reg = <1>;
> > +                            color = <LED_COLOR_ID_AMBER>;
> > +                            function = LED_FUNCTION_LAN;
> > +                            function-enumerator = <1>;
> > +                            default-state = "keep";
> > +                        };
> > +                    };
> >                  };
> 
> I have nothing against this, but I would like to point out the
> existence of the trigger-sources DT property, and I would like to
> discuss how this property should be used by the LED subsystem to choose
> default behaviour of a LED.
> 
> Consider that we want to specify in device-tree that a PHY LED (or any
> other LED) should blink on network activity of the network device
> connected to this PHY (let's say the attached network device is eth0).
> (Why would we want to specify this in devicetree? Because currently the
>  drivers either keep the behaviour from boot or change it to something
>  specific that is not configurable.)
> 
> We could specify in DT something like:
>   eth0: ethernet-controller {
>     ...
>   }
> 
>   ethernet-phy {
>     leds {
>       led@0 {
>         reg = <0>;
>         color = <LED_COLOR_ID_GREEN>;
>         trigger-sources = <&eth0>;
>         function = LED_FUNCTION_ ?????? ;
>       }
>     }
>   }
> 
> The above example specifies that the LED has a trigger source (eth0),
> but we still need to specify the trigger itself (for example that
> the LED should blink on activity, or the different kinds of link). In my
> opinion, this should be specified by the function property, but this
> property is currently used in other way: it is filled in with something
> like "wan" or "lan" or "wlan", an information which, IMO,
> should instead come from the devicename part of the LED, not the
> function part.
> 
> Recall that the LED names are of the form
>   devicename:color:function
> where the devicename part is supposed to be something like mmc0 or
> sda1. With LEDs that are associated with network devices I think the
> corresponding name should be the name of the network device (like eth0),
> but there is the problem of network namespaces and also that network
> devices can be renamed :(.
> 
> So one option how to specify the behaviour of the LED to blink on
> activity would be to set
>   function = LED_FUNCTION_ACTIVITY;
> but this would conflict with how currently some devicetrees use "lan",
> "wlan" or "wan" as the function (which is IMO incorrect, as I said
> above).
> 
> Another option would be to ignore the function and instead use
> additional argument in the trigger-source property, something like
>   trigger-sources = <&eth0 TRIGGER_SOURCE_ACTIVITY>;
> 
> I would like to start a discussion on this and hear about your opinions,
> because I think that the trigger-sources and function properties were
> proposed in good faith, but currently the implementation and usage is a
> mess.
> 

I think we should continue and make this discussion when we start
implementing the hw contro for these LEDs to configure them in DT.

Currently we are implementing very basic support so everything will be
in sw.

Anyway just to give some ideas. Yes it sound a good idea to use the
trigger-sources binding. My idea would be that trigger needs to have
specific support for them. 
If this in mind netdev can be configured in DT and setup hw control to
offload blink with the required interface passed.

The current implementation still didn't include a way to configure the
blink in DT as the series are already a bit big... (currently we have 3:
- This series that already grow from 10 patch to 14
- A cleanup series for netdev trigger that is already 7 patch
- hw control that is another big boy with 12 patch
)

So our idea was to first implement the minor things and then polish and
improve it. (to make it easier to review)

But agree with you that it would be a nice idea to have a correct and
good implementation for trigger-sources.

-- 
	Ansuel