> 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. Hi Marek We are pushing the boundaries of the LED code here, doing things which have not been done before, as far as i know. So i expect some discussion about this. However, that discussion should not really affect this patchset, which is just adding plain boring software controlled LEDs. A quick recap about ledtrig-netdev. If you have a plain boring LED, you have: root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# ls brightness device max_brightness power subsystem trigger uevent You can turn the LED on with root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo 1 > brightness and turn it off with: root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo 0 > brightness You select the trigger via the trigger sysfs file: root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# cat trigger [none] kbd-scrolllock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock timer heartbeat netdev mmc0 root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo netdev > trigger root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# ls activity brightness device_name half_duplex link link_100 max_brightness rx trigger uevent available_mode device full_duplex interval link_10 link_1000 power subsystem tx When you select a trigger, that trigger can add additional sysfs files. For the netdev trigger we gain link, link_10, link_100, link_1000, rx & tx. Nothing special here, if you selected the timer trigger you get delay_off delay_on. The oneshot trigger has invert, delay_on, delay_off etc. You then configure the trigger via setting values in the sysfs files. If you want the LED to indicate if there is link, you would do: echo 1 > link The LED would then light up if the netdev has carrier. If you want link plus RX packets echo 1 > link echo 1 > rx The LED will then be on if there is link, and additionally blink if the netdev stats indicate received frames. For the netdev trigger, all the configuration values are boolean. So a simple way to represent this in DT would be boolean properties: netdev-link = <1>; netdev->rx = <1>; We probably want these properties name spaced, because we have oneshot delay_on and timer delay_on for example. The same sysfs name could have different types, bool vs milliseconds, etc. I would make it, that when the trigger is activated, the values are read from DT and used. There is currently no persistent state for triggers. If you where to swap to the timer trigger and then return to the netdev trigger, all state is lost, so i would re-read DT. Offloading to hardware should not make an difference here. All we are going to do is pass the current configuration to the LED and ask it, can you do this? If it says no, we keep blinking in software. If yes, we leave the blinking to the hardware. There is the open question of if DT should be used like this. It is not describing hardware, it is describing configuration of hardware. So it could well get rejected. You then need to configure it in software. Andrew
