| Message ID | 20190313202615.22883-5-linux@rasmusvillemoes.dk |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | None | expand |
Hello, On Wed, Mar 13, 2019 at 09:26:15PM +0100, Rasmus Villemoes wrote: > It can be quite convenient to initialize a netdev-triggered LED with a > device name and setting the rx,tx,link properties from device tree, > instead of having to do that in an init script in userspace. Well, you'd do this in an udev rule instead of an init script. > My main motivation for this is to be able to switch away from the > deprecated CONFIG_CAN_LEDS, so add an example based on that and add a > pointer in the net/can/Kconfig file. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > .../devicetree/bindings/leds/common.txt | 17 ++++++++++ > drivers/leds/trigger/ledtrig-netdev.c | 31 +++++++++++++++++++ > drivers/net/can/Kconfig | 3 +- > 3 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 7cb88460a47c..4f3a97e73417 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -43,6 +43,23 @@ Optional properties for child nodes: > Documentation/ABI/testing/sysfs-class-led-trigger-netdev) > to reflect the state and activity of a net device. > > + The optional child node netdev can be used to > + configure initial values for the link, rx, tx and > + device_name properties. For example, setting > + linux,default-trigger = "netdev" and adding the child > + node > + > + netdev { > + rx; > + tx; > + link; > + device-name = "can0"; Maybe make this: device = <&can0>; ? > + }; > + > + can be used to replace 'linux,default-trigger = > + "can0-rxtx"' that relies on the deprecated > + CONFIG_CAN_LEDS. Mentioning "CONFIG_CAN_LEDS" is wrong for a binding document that is supposed to be independent from the kernel. > - led-pattern : Array of integers with default pattern for certain triggers. > Each trigger may parse this property differently: > - one-shot : two numbers specifying delay on and delay off (in ms), > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c > index 55153a7e8433..1f7c86df1e91 100644 > --- a/drivers/leds/trigger/ledtrig-netdev.c > +++ b/drivers/leds/trigger/ledtrig-netdev.c > @@ -20,6 +20,7 @@ > #include <linux/list.h> > #include <linux/module.h> > #include <linux/netdevice.h> > +#include <linux/of.h> > #include <linux/spinlock.h> > #include <linux/timer.h> > #include "../leds.h" > @@ -395,6 +396,35 @@ static void netdev_trig_work(struct work_struct *work) > (atomic_read(&trigger_data->interval)*2)); > } > > +static void netdev_trig_of_init(struct led_classdev *led_cdev, > + struct led_netdev_data *trigger_data) > +{ > + struct device_node *np = led_cdev->dev->of_node; > + const char *device_name; > + > + if (!np) > + return; > + np = of_get_child_by_name(np, "netdev"); > + if (!np) > + return; > + > + if (of_property_read_bool(np, "link")) > + __set_bit(NETDEV_LED_LINK, &trigger_data->mode); > + if (of_property_read_bool(np, "tx")) > + __set_bit(NETDEV_LED_TX, &trigger_data->mode); > + if (of_property_read_bool(np, "rx")) > + __set_bit(NETDEV_LED_RX, &trigger_data->mode); > + if (!of_property_read_string(np, "device-name", &device_name)) { > + unsigned len = strlen(device_name); > + > + if (len < IFNAMSIZ) > + set_device(trigger_data, device_name, len); if set_device contained the length check you'd have it in only one place. > + } > + set_baseline_state(trigger_data); > + > + of_node_put(np); > +} > + > static int netdev_trig_activate(struct led_classdev *led_cdev) > { > struct led_netdev_data *trigger_data; > @@ -418,6 +448,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev) > trigger_data->mode = 0; > atomic_set(&trigger_data->interval, msecs_to_jiffies(50)); > trigger_data->last_activity = 0; > + netdev_trig_of_init(led_cdev, trigger_data); > > led_set_trigger_data(led_cdev, trigger_data); > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > index e0f0ad7a550a..91703a96b636 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -77,7 +77,8 @@ config CAN_LEDS > # everything that this driver is doing. This is marked as broken > # because it uses stuff that is intended to be changed or removed. > # Please consider switching to the netdev trigger and confirm it > - # fulfills your needs instead of fixing this driver. > + # fulfills your needs instead of fixing this driver. See e.g. > + # Documentation/devicetree/bindings/leds/common.txt > depends on BROKEN > select LEDS_TRIGGERS > ---help--- This hunk can better live in the patch adding to Documentation/devicetree/bindings/leds/common.txt or a separate patch. Best regards Uwe
On 14/03/2019 10.36, Uwe Kleine-König wrote: > Hello, > > On Wed, Mar 13, 2019 at 09:26:15PM +0100, Rasmus Villemoes wrote: >> It can be quite convenient to initialize a netdev-triggered LED with a >> device name and setting the rx,tx,link properties from device tree, >> instead of having to do that in an init script in userspace. > > Well, you'd do this in an udev rule instead of an init script. Perhaps, if my userspace had udev. But even then, if I have to modify my userspace in order to upgrade the kernel (which is what I'm trying to avoid, for a number of reasons), I'd probably still do it in a seven-line init script instead of trying to wrap my head around udev/mdev rule writing. Initializing the netdev trigger from DT data is certainly in line with what is possible for certain other triggers via the led-pattern property. Just out of curiosity, do you have a udev rule (template) for this? >> >> + The optional child node netdev can be used to >> + configure initial values for the link, rx, tx and >> + device_name properties. For example, setting >> + linux,default-trigger = "netdev" and adding the child >> + node >> + >> + netdev { >> + rx; >> + tx; >> + link; >> + device-name = "can0"; > > Maybe make this: > > device = <&can0>; Huh? I don't think there's any guarantee that the netdevice has a DT node/phandle, and even if it did, how would the init code map from that phandle to the string it should fill into ->device_name? Since DT natively has bools and strings, it's much more natural to just use those types which map nicely to the sysfs properties. > ? > >> + }; >> + >> + can be used to replace 'linux,default-trigger = >> + "can0-rxtx"' that relies on the deprecated >> + CONFIG_CAN_LEDS. > > Mentioning "CONFIG_CAN_LEDS" is wrong for a binding document that is > supposed to be independent from the kernel. Well, this is in relation to the already linux-specific linux,default-trigger binding. But I can certainly reword this to a simple example that doesn't mention what it replaces and why, and just move the mentioning of CONFIG_CAN_LEDS to the commit message. >> - led-pattern : Array of integers with default pattern for certain triggers. >> Each trigger may parse this property differently: >> - one-shot : two numbers specifying delay on and delay off (in ms), >> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c >> index 55153a7e8433..1f7c86df1e91 100644 >> --- a/drivers/leds/trigger/ledtrig-netdev.c >> +++ b/drivers/leds/trigger/ledtrig-netdev.c >> @@ -20,6 +20,7 @@ >> #include <linux/list.h> >> #include <linux/module.h> >> #include <linux/netdevice.h> >> +#include <linux/of.h> >> #include <linux/spinlock.h> >> #include <linux/timer.h> >> #include "../leds.h" >> @@ -395,6 +396,35 @@ static void netdev_trig_work(struct work_struct *work) >> (atomic_read(&trigger_data->interval)*2)); >> } >> >> +static void netdev_trig_of_init(struct led_classdev *led_cdev, >> + struct led_netdev_data *trigger_data) >> +{ >> + struct device_node *np = led_cdev->dev->of_node; >> + const char *device_name; >> + >> + if (!np) >> + return; >> + np = of_get_child_by_name(np, "netdev"); >> + if (!np) >> + return; >> + >> + if (of_property_read_bool(np, "link")) >> + __set_bit(NETDEV_LED_LINK, &trigger_data->mode); >> + if (of_property_read_bool(np, "tx")) >> + __set_bit(NETDEV_LED_TX, &trigger_data->mode); >> + if (of_property_read_bool(np, "rx")) >> + __set_bit(NETDEV_LED_RX, &trigger_data->mode); >> + if (!of_property_read_string(np, "device-name", &device_name)) { >> + unsigned len = strlen(device_name); >> + >> + if (len < IFNAMSIZ) >> + set_device(trigger_data, device_name, len); > > if set_device contained the length check you'd have it in only one > place. I considered that, but rejected it because it complicates the existing user of set_device() (where the call happens under a lock; from the _of_init function, nobody else knows about the trigger_data instance yet). But on second thought, I think you're right that it's better done in that one place. So I'll refactor. >> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig >> index e0f0ad7a550a..91703a96b636 100644 >> --- a/drivers/net/can/Kconfig >> +++ b/drivers/net/can/Kconfig >> @@ -77,7 +77,8 @@ config CAN_LEDS >> # everything that this driver is doing. This is marked as broken >> # because it uses stuff that is intended to be changed or removed. >> # Please consider switching to the netdev trigger and confirm it >> - # fulfills your needs instead of fixing this driver. >> + # fulfills your needs instead of fixing this driver. See e.g. >> + # Documentation/devicetree/bindings/leds/common.txt >> depends on BROKEN >> select LEDS_TRIGGERS >> ---help--- > > This hunk can better live in the patch adding to > Documentation/devicetree/bindings/leds/common.txt or a separate patch. OK, will move to separate patch. Rasmus
On Wed 2019-03-13 21:26:15, Rasmus Villemoes wrote: > It can be quite convenient to initialize a netdev-triggered LED with a > device name and setting the rx,tx,link properties from device tree, > instead of having to do that in an init script in userspace. > > My main motivation for this is to be able to switch away from the > deprecated CONFIG_CAN_LEDS, so add an example based on that and add a > pointer in the net/can/Kconfig file. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > .../devicetree/bindings/leds/common.txt | 17 ++++++++++ > drivers/leds/trigger/ledtrig-netdev.c | 31 +++++++++++++++++++ > drivers/net/can/Kconfig | 3 +- > 3 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 7cb88460a47c..4f3a97e73417 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -43,6 +43,23 @@ Optional properties for child nodes: > Documentation/ABI/testing/sysfs-class-led-trigger-netdev) > to reflect the state and activity of a net device. > > + The optional child node netdev can be used to > + configure initial values for the link, rx, tx and > + device_name properties. For example, setting > + linux,default-trigger = "netdev" and adding the child > + node > + > + netdev { > + rx; > + tx; > + link; > + device-name = "can0"; > + }; > + > + can be used to replace 'linux,default-trigger = > + "can0-rxtx"' that relies on the deprecated > + CONFIG_CAN_LEDS. I'm sorry, but no, not like this. I see it works for you, only having single can device, but it would quickly break with two of them and two ethernets are rather common. So this will need to be device = <&phandle_of_ethernet_device> or something like that. There may be example with usb port triggers. Thanks, Pavel
Hi Rasmus, Thank you for the patch set. I have one comment below. On 3/13/19 9:26 PM, Rasmus Villemoes wrote: > It can be quite convenient to initialize a netdev-triggered LED with a > device name and setting the rx,tx,link properties from device tree, > instead of having to do that in an init script in userspace. > > My main motivation for this is to be able to switch away from the > deprecated CONFIG_CAN_LEDS, so add an example based on that and add a > pointer in the net/can/Kconfig file. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > .../devicetree/bindings/leds/common.txt | 17 ++++++++++ > drivers/leds/trigger/ledtrig-netdev.c | 31 +++++++++++++++++++ > drivers/net/can/Kconfig | 3 +- > 3 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 7cb88460a47c..4f3a97e73417 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -43,6 +43,23 @@ Optional properties for child nodes: > Documentation/ABI/testing/sysfs-class-led-trigger-netdev) > to reflect the state and activity of a net device. > > + The optional child node netdev can be used to > + configure initial values for the link, rx, tx and > + device_name properties. For example, setting > + linux,default-trigger = "netdev" and adding the child > + node > + > + netdev { > + rx; > + tx; > + link; > + device-name = "can0"; > + }; > + > + can be used to replace 'linux,default-trigger = > + "can0-rxtx"' that relies on the deprecated > + CONFIG_CAN_LEDS. > + > - led-pattern : Array of integers with default pattern for certain triggers. > Each trigger may parse this property differently: > - one-shot : two numbers specifying delay on and delay off (in ms), > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c > index 55153a7e8433..1f7c86df1e91 100644 > --- a/drivers/leds/trigger/ledtrig-netdev.c > +++ b/drivers/leds/trigger/ledtrig-netdev.c > @@ -20,6 +20,7 @@ > #include <linux/list.h> > #include <linux/module.h> > #include <linux/netdevice.h> > +#include <linux/of.h> > #include <linux/spinlock.h> > #include <linux/timer.h> > #include "../leds.h" > @@ -395,6 +396,35 @@ static void netdev_trig_work(struct work_struct *work) > (atomic_read(&trigger_data->interval)*2)); > } > > +static void netdev_trig_of_init(struct led_classdev *led_cdev, > + struct led_netdev_data *trigger_data) > +{ > + struct device_node *np = led_cdev->dev->of_node; > + const char *device_name; > + > + if (!np) > + return; > + np = of_get_child_by_name(np, "netdev"); > + if (!np) > + return; > + > + if (of_property_read_bool(np, "link")) > + __set_bit(NETDEV_LED_LINK, &trigger_data->mode); > + if (of_property_read_bool(np, "tx")) > + __set_bit(NETDEV_LED_TX, &trigger_data->mode); > + if (of_property_read_bool(np, "rx")) > + __set_bit(NETDEV_LED_RX, &trigger_data->mode); > + if (!of_property_read_string(np, "device-name", &device_name)) { > + unsigned len = strlen(device_name); > + > + if (len < IFNAMSIZ) > + set_device(trigger_data, device_name, len); > + } > + set_baseline_state(trigger_data); > + > + of_node_put(np); > +} > + > static int netdev_trig_activate(struct led_classdev *led_cdev) > { > struct led_netdev_data *trigger_data; > @@ -418,6 +448,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev) > trigger_data->mode = 0; > atomic_set(&trigger_data->interval, msecs_to_jiffies(50)); > trigger_data->last_activity = 0; > + netdev_trig_of_init(led_cdev, trigger_data); You want to execute this only if recently introduced flag LED_INIT_DEFAULT_TRIGGER is set. Please compare how drivers/leds/trigger/ledtrig-pattern.c uses it. > > led_set_trigger_data(led_cdev, trigger_data); > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > index e0f0ad7a550a..91703a96b636 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -77,7 +77,8 @@ config CAN_LEDS > # everything that this driver is doing. This is marked as broken > # because it uses stuff that is intended to be changed or removed. > # Please consider switching to the netdev trigger and confirm it > - # fulfills your needs instead of fixing this driver. > + # fulfills your needs instead of fixing this driver. See e.g. > + # Documentation/devicetree/bindings/leds/common.txt > depends on BROKEN > select LEDS_TRIGGERS > ---help--- >
On 14/03/2019 11.29, Pavel Machek wrote: > On Wed 2019-03-13 21:26:15, Rasmus Villemoes wrote: >> It can be quite convenient to initialize a netdev-triggered LED with a >> device name and setting the rx,tx,link properties from device tree, >> instead of having to do that in an init script in userspace. >> >> + The optional child node netdev can be used to >> + configure initial values for the link, rx, tx and >> + device_name properties. For example, setting >> + linux,default-trigger = "netdev" and adding the child >> + node >> + >> + netdev { >> + rx; >> + tx; >> + link; >> + device-name = "can0"; >> + }; >> + >> + can be used to replace 'linux,default-trigger = >> + "can0-rxtx"' that relies on the deprecated >> + CONFIG_CAN_LEDS. > > I'm sorry, but no, not like this. I see it works for you, only having > single can device, but it would quickly break with two of them and two > ethernets are rather common. > > So this will need to be device = <&phandle_of_ethernet_device> or > something like that. There may be example with usb port triggers. Huh? I have two CAN devices, and there are two LEDs on the front panel labeled CAN-A and CAN-B; my device tree nodes for that are cana { label = "cana:green:activity"; gpios = <&gpio0 10 0>; default-state = "off"; linux,default-trigger = "netdev"; netdev { rx; tx; link; device-name = "can0"; }; }; canb { label = "canb:green:activity"; gpios = <&gpio0 5 0>; default-state = "off"; linux,default-trigger = "netdev"; netdev { rx; tx; link; device-name = "can1"; }; }; and this works just fine. The only change from the old DT is the addition of the netdev nodes and changing linux,default-trigger from "can0-rxtx", "can1-rxtx" to both "netdev". Rasmus
On Thu 2019-03-14 12:26:38, Rasmus Villemoes wrote: > On 14/03/2019 11.29, Pavel Machek wrote: > > On Wed 2019-03-13 21:26:15, Rasmus Villemoes wrote: > >> It can be quite convenient to initialize a netdev-triggered LED with a > >> device name and setting the rx,tx,link properties from device tree, > >> instead of having to do that in an init script in userspace. > >> > >> + The optional child node netdev can be used to > >> + configure initial values for the link, rx, tx and > >> + device_name properties. For example, setting > >> + linux,default-trigger = "netdev" and adding the child > >> + node > >> + > >> + netdev { > >> + rx; > >> + tx; > >> + link; > >> + device-name = "can0"; > >> + }; > >> + > >> + can be used to replace 'linux,default-trigger = > >> + "can0-rxtx"' that relies on the deprecated > >> + CONFIG_CAN_LEDS. > > > > I'm sorry, but no, not like this. I see it works for you, only having > > single can device, but it would quickly break with two of them and two > > ethernets are rather common. > > > > So this will need to be device = <&phandle_of_ethernet_device> or > > something like that. There may be example with usb port triggers. > > Huh? I have two CAN devices, and there are two LEDs on the front panel > labeled CAN-A and CAN-B; my device tree nodes for that are ... > and this works just fine. The only change from the old DT is the > addition of the netdev nodes and changing linux,default-trigger from > "can0-rxtx", "can1-rxtx" to both "netdev". Yeah and now insert the modules for the can devices in different order... May not happen in your case but will be fairly common for ethernets. Pavel
On 14/03/2019 13.00, Pavel Machek wrote: linux,default-trigger from > > Yeah and now insert the modules for the can devices in different > order... May not happen in your case but will be fairly common for > ethernets. Unpredictable device names is not a problem I'm trying to solve, nor one that actually occurs on embedded systems. And even if one has such a system, one is likely running udev or similar to rename devices to something sane and predictable, in which case a fixed string _still_ works fine. Or one uses udev rules to munge the netdev trigger sysfs files, and has applications dealing with devices appearing as banana457. OK, so there's of_find_net_device_by_node, so a phandle solution might also be possible, and might work just as well for my case. But I still think initializing with a fixed string is the simplest and sanest thing to do. Rasmus
On Thu 2019-03-14 14:19:42, Rasmus Villemoes wrote: > On 14/03/2019 13.00, Pavel Machek wrote: > linux,default-trigger from > > > > Yeah and now insert the modules for the can devices in different > > order... May not happen in your case but will be fairly common for > > ethernets. > > Unpredictable device names is not a problem I'm trying to solve, nor one > that actually occurs on embedded systems. And even if one has such a > system, one is likely running udev or similar to rename devices to > something sane and predictable, in which case a fixed string _still_ > works fine. Or one uses udev rules to munge the netdev trigger sysfs > files, and has applications dealing with devices appearing as banana457. > > OK, so there's of_find_net_device_by_node, so a phandle solution might > also be possible, and might work just as well for my case. But I still > think initializing with a fixed string is the simplest and sanest thing > to do. Simplest, yes. Mergeable, no. Sorry. Udev will not help with renaming -- it runs after kernel boot, while udev parsing is usually done before userland starts. Plus, device tree should really describe hardware, and be usable with other operating systems. Of course, there are exceptions, but "can0" is unneccessarily linux-specific. Pavel
On 17/03/2019 20.11, Pavel Machek wrote: > On Thu 2019-03-14 14:19:42, Rasmus Villemoes wrote: >> On 14/03/2019 13.00, Pavel Machek wrote: >> linux,default-trigger from >>> > Udev will not help with renaming -- it runs after kernel boot, while > udev parsing is usually done before userland starts. This is simply plain wrong. The netdev trigger explicitly tracks a netdevice _by name_, and has netdev notifier in place to get notified should a netdevice by that name ever appear. > Plus, device tree should really describe hardware, and be usable with > other operating systems. Of course, there are exceptions, but "can0" > is unneccessarily linux-specific. And "linux,netdev-trigger" is not linux-specific? And why are you so hung up on the can example, it would work just as fine for usb0 or whatever. Anyway, to whoever is responsible for merging this stuff, please at most apply patch 1, factoring out the string-to-netdev handling doesn't make much sense without the final configure-from-dt stuff. Rasmus
diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 7cb88460a47c..4f3a97e73417 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -43,6 +43,23 @@ Optional properties for child nodes: Documentation/ABI/testing/sysfs-class-led-trigger-netdev) to reflect the state and activity of a net device. + The optional child node netdev can be used to + configure initial values for the link, rx, tx and + device_name properties. For example, setting + linux,default-trigger = "netdev" and adding the child + node + + netdev { + rx; + tx; + link; + device-name = "can0"; + }; + + can be used to replace 'linux,default-trigger = + "can0-rxtx"' that relies on the deprecated + CONFIG_CAN_LEDS. + - led-pattern : Array of integers with default pattern for certain triggers. Each trigger may parse this property differently: - one-shot : two numbers specifying delay on and delay off (in ms), diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c index 55153a7e8433..1f7c86df1e91 100644 --- a/drivers/leds/trigger/ledtrig-netdev.c +++ b/drivers/leds/trigger/ledtrig-netdev.c @@ -20,6 +20,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/netdevice.h> +#include <linux/of.h> #include <linux/spinlock.h> #include <linux/timer.h> #include "../leds.h" @@ -395,6 +396,35 @@ static void netdev_trig_work(struct work_struct *work) (atomic_read(&trigger_data->interval)*2)); } +static void netdev_trig_of_init(struct led_classdev *led_cdev, + struct led_netdev_data *trigger_data) +{ + struct device_node *np = led_cdev->dev->of_node; + const char *device_name; + + if (!np) + return; + np = of_get_child_by_name(np, "netdev"); + if (!np) + return; + + if (of_property_read_bool(np, "link")) + __set_bit(NETDEV_LED_LINK, &trigger_data->mode); + if (of_property_read_bool(np, "tx")) + __set_bit(NETDEV_LED_TX, &trigger_data->mode); + if (of_property_read_bool(np, "rx")) + __set_bit(NETDEV_LED_RX, &trigger_data->mode); + if (!of_property_read_string(np, "device-name", &device_name)) { + unsigned len = strlen(device_name); + + if (len < IFNAMSIZ) + set_device(trigger_data, device_name, len); + } + set_baseline_state(trigger_data); + + of_node_put(np); +} + static int netdev_trig_activate(struct led_classdev *led_cdev) { struct led_netdev_data *trigger_data; @@ -418,6 +448,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev) trigger_data->mode = 0; atomic_set(&trigger_data->interval, msecs_to_jiffies(50)); trigger_data->last_activity = 0; + netdev_trig_of_init(led_cdev, trigger_data); led_set_trigger_data(led_cdev, trigger_data); diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index e0f0ad7a550a..91703a96b636 100644 --- a/drivers/net/can/Kconfig +++ b/drivers/net/can/Kconfig @@ -77,7 +77,8 @@ config CAN_LEDS # everything that this driver is doing. This is marked as broken # because it uses stuff that is intended to be changed or removed. # Please consider switching to the netdev trigger and confirm it - # fulfills your needs instead of fixing this driver. + # fulfills your needs instead of fixing this driver. See e.g. + # Documentation/devicetree/bindings/leds/common.txt depends on BROKEN select LEDS_TRIGGERS ---help---
It can be quite convenient to initialize a netdev-triggered LED with a device name and setting the rx,tx,link properties from device tree, instead of having to do that in an init script in userspace. My main motivation for this is to be able to switch away from the deprecated CONFIG_CAN_LEDS, so add an example based on that and add a pointer in the net/can/Kconfig file. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- .../devicetree/bindings/leds/common.txt | 17 ++++++++++ drivers/leds/trigger/ledtrig-netdev.c | 31 +++++++++++++++++++ drivers/net/can/Kconfig | 3 +- 3 files changed, 50 insertions(+), 1 deletion(-)