Re: lets settle the LED `function` property regarding the netdev trigger

[Marek Behún]

On Sun, 3 Oct 2021 20:56:23 +0200
Andrew Lunn <andrew@xxxxxxx> wrote:

> On Fri, Oct 01, 2021 at 02:36:01PM +0200, Marek Behún wrote:
> > Hello Pavel, Jacek, Rob and others,
> > 
> > I'd like to settle DT binding for the LED function property regarding
> > the netdev LED trigger.
> > 
> > Currently we have, in include/dt-bindings/leds/common.h, the following
> > functions defined that could be interpreted as request to enable netdev
> > trigger on given LEDs:
> >   activity
> >   lan
> >   rx tx
> >   wan
> >   wlan
> > 
> > The "activity" function was originally meant to imply the CPU
> > activity trigger, while "rx" and "tx" are AFAIK meant as UART indicators
> > (tty LED trigger), see
> > https://lore.kernel.org/linux-leds/20190609190803.14815-27-jacek.anaszewski@xxxxxxxxx/
> > 
> > The netdev trigger supports different settings:
> > - indicate link
> > - blink on rx, blink on tx, blink on both
> > 
> > The current scheme does not allow for implying these.
> > 
> > I therefore propose that when a LED has a network device handle in the
> > trigger-sources property, the "rx", "tx" and "activity" functions
> > should also imply netdev trigger (with the corresponding setting).
> > A "link" function should be added, also implying netdev trigger.
> > 
> > What about if a LED is meant by the device vendor to indicate both link
> > (on) and activity (blink)?
> > The function property is currently a string. This could be changed to
> > array of strings, and then we can have
> >   function = "link", "activity";
> > Since the function property is also used for composing LED classdev
> > names, I think only the first member should be used for that.
> > 
> > This would allow for ethernet LEDs with names
> >   ethphy-0:green:link
> >   ethphy-0:yellow:activity
> > to be controlled by netdev trigger in a specific setting without the
> > need to set the trigger in /sys/class/leds.  
> 
> Hi Marek
> 
> There is no real standardization here. Which means PHYs differ a lot
> in what they can do. We need to strike a balance between over
> simplifying and only supporting a very small set of PHY LED features,
> and allowing great flexibility and having each PHY implement its own
> specific features and having little in common.
> 
> I think your current proposal is currently on the too simple side.
> 
> One common feature is that there are multiple modes for indicating
> link, which take into account the link speed. Look at for example
> include/dt-bindings/net/microchip-lan78xx.h
> 
> #define LAN78XX_LINK_ACTIVITY           0
> #define LAN78XX_LINK_1000_ACTIVITY      1
> #define LAN78XX_LINK_100_ACTIVITY       2
> #define LAN78XX_LINK_10_ACTIVITY        3
> #define LAN78XX_LINK_100_1000_ACTIVITY  4
> #define LAN78XX_LINK_10_1000_ACTIVITY   5
> #define LAN78XX_LINK_10_100_ACTIVITY    6
> 
> And:
> 
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10	0x0010
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK100	0x0020
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10X	0x0030
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK1000	0x0040
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10_0	0x0050
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK100X	0x0060
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10XX	0x0070
> 
> Marvell PHYs have similar LINK modes which can either be one specific
> speed, or a combination of speeds.
> 
> This is a common enough feature, and a frequently used feature, we
> need to support it. We also need to forward looking. We should not
> limit ourselves to 10/100/1G. We have 3 PHY drivers which support
> 2.5G, 5G and 10G. 25G and 40G are standardized so are likely to come
> along at some point.
> 
> One way we could support this is:
> 
> function = "link100", "link1G", "activity";
> 
> for LAN78XX_LINK_100_1000_ACTIVITY, etc.
> 
>     Andrew

Hello Andrew,

I am aware of this, and in fact am working on a proposal for an
extension of netdev LED extension, to support the different link
modes. (And also to support for multi-color LEDs.)

But I am not entirely sure whether these different link modes should be
also definable via device-tree. Are there devices with ethernet LEDs
dedicated for a specific speed? (i.e. the manufacturer says in the
documentation of the device, or perhaps on the device's case, that this
LED shows 100M/1000M link, and that other LED is shows 10M link?)
If so, that this should be specified in the devicetree, IMO. But are
such devices common?

And what about multi-color LEDs? There are ethernet ports where one LED
is red-green, and so can generate red, green, and yellow color. Should
device tree also define which color indicates which mode?

Marek