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
