[PATCH leds + devicetree v2 0/2] Parse DT property `trigger-sources` for netdev LED trigger

[PATCH leds + devicetree v2 0/2] Parse DT property `trigger-sources` for netdev LED trigger

[Marek Behún]

Hi,

this is v2.

Changes since v1:
- fixed problems with v1 reported by kernel test robot
- created helper functions of_led_count_trigger_sources and
  of_led_get_trigger_source

Below is description from v1:

The `trigger-sources` LED DT property is currently only implemented
for ledtrig-usbport.

Lets implement it for the netdev LED trigger.

In this proposal the specific netdev LED trigger mode is determined
from the `function` LED DT property.

Example:
  eth0: ethernet@30000 {
    compatible = "xyz";
    #trigger-source-cells = <0>;
  };

  led {
    color = <LED_COLOR_ID_GREEN>;
    function = LED_FUNCTION_LINK;
    trigger-sources = <&eth0>;
  };

When led is registered, the netdev trigger is automatically activated
and set to light the LED on if eth0 is linked.

Please let me know if this binding is OK, or if the binding should
instead of the `function` property determine the trigger settings from
arguments of the `trigger-sources` property :
  led {
    color = <LED_COLOR_ID_GREEN>;
    trigger-sources = <&eth0 (NETDEV_ATTR_LINK | NETDEV_ATTR_RX)>;
  };

I prefer the first binding, since we already have the `function`
property. Multiple modes can be achieved by string array, but this is
not yet implemented:
  led {
    color = <LED_COLOR_ID_GREEN>;
    function = LED_FUNCTION_LINK, LED_FUNCTION_ACTIVITY;
    trigger-sources = <&eth0>;
  };

Marek

Marek Behún (2):
  leds: trigger: add trigger sources validating method and helper
    functions
  leds: trigger: netdev: parse `trigger-sources` from device tree

 drivers/leds/led-triggers.c           | 68 ++++++++++++++++++++---
 drivers/leds/trigger/ledtrig-netdev.c | 80 ++++++++++++++++++++++++++-
 include/dt-bindings/leds/common.h     |  1 +
 include/linux/leds.h                  | 25 +++++++++
 4 files changed, 165 insertions(+), 9 deletions(-)

-- 
2.26.2

[PATCH leds + devicetree v2 1/2] leds: trigger: add trigger sources validating method and helper functions

[Marek Behún]

Currently we use the `linux,default-trigger` device tree property of a
LED to define the default trigger which should be activated for a LED.

But the LED device tree binding also documents the `trigger-sources`
property, which specifies the source device which should be triggering
the LED.

The `trigger-sources` property is currently implemented only in
drivers/usb/core/ledtrig-usbport.c.

Lets add a method to struct led_trigger which, if implemented, can check
whether this trigger should be enabled as default. This check shall be
done by checking whether the specified `trigger-sources` refers to a
device compatible with the trigger. For this two new helper functions,
of_led_count_trigger_sources and of_led_get_trigger_source, are
implemented.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 drivers/leds/led-triggers.c | 68 ++++++++++++++++++++++++++++++++-----
 include/linux/leds.h        | 25 ++++++++++++++
 2 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 91da90cfb11d9..a93c1b6a94282 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -243,18 +243,30 @@ void led_trigger_remove(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_trigger_remove);
 
+static bool trigger_is_default(struct led_classdev *led_cdev,
+			       struct led_trigger *trig)
+{
+	if (!trigger_relevant(led_cdev, trig))
+		return false;
+
+	if (led_cdev->default_trigger &&
+	    !strcmp(led_cdev->default_trigger, trig->name))
+		return true;
+
+	if (trig->has_valid_source && trig->has_valid_source(led_cdev))
+		return true;
+
+	return false;
+}
+
 void led_trigger_set_default(struct led_classdev *led_cdev)
 {
 	struct led_trigger *trig;
 
-	if (!led_cdev->default_trigger)
-		return;
-
 	down_read(&triggers_list_lock);
 	down_write(&led_cdev->trigger_lock);
 	list_for_each_entry(trig, &trigger_list, next_trig) {
-		if (!strcmp(led_cdev->default_trigger, trig->name) &&
-		    trigger_relevant(led_cdev, trig)) {
+		if (trigger_is_default(led_cdev, trig)) {
 			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
 			led_trigger_set(led_cdev, trig);
 			break;
@@ -306,9 +318,7 @@ int led_trigger_register(struct led_trigger *trig)
 	down_read(&leds_list_lock);
 	list_for_each_entry(led_cdev, &leds_list, node) {
 		down_write(&led_cdev->trigger_lock);
-		if (!led_cdev->trigger && led_cdev->default_trigger &&
-		    !strcmp(led_cdev->default_trigger, trig->name) &&
-		    trigger_relevant(led_cdev, trig)) {
+		if (!led_cdev->trigger && trigger_is_default(led_cdev, trig)) {
 			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
 			led_trigger_set(led_cdev, trig);
 		}
@@ -459,3 +469,45 @@ void led_trigger_unregister_simple(struct led_trigger *trig)
 	kfree(trig);
 }
 EXPORT_SYMBOL_GPL(led_trigger_unregister_simple);
+
+int of_led_count_trigger_sources(struct led_classdev *led_cdev)
+{
+	struct device_node *np;
+	int count;
+
+	np = dev_of_node(led_cdev->dev);
+	if (!np)
+		return 0;
+
+	count = of_count_phandle_with_args(np, "trigger-sources",
+					   "#trigger-source-cells");
+	if (count == -ENOENT)
+		return 0;
+	else if (count < 0)
+		dev_warn(led_cdev->dev,
+			 "Failed parsing trigger sources for %pOF!\n", np);
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(of_led_count_trigger_sources);
+
+int of_led_get_trigger_source(struct led_classdev *led_cdev, int index,
+			      struct of_phandle_args *args)
+{
+	struct device_node *np;
+	int err;
+
+	np = dev_of_node(led_cdev->dev);
+	if (!np)
+		return -ENOENT;
+
+	err = of_parse_phandle_with_args(np, "trigger-sources",
+					 "#trigger-source-cells", index, args);
+	if (err < 0 && err != -ENOENT)
+		dev_warn(led_cdev->dev,
+			 "Failed parsing trigger source index %i for %pOF!\n",
+			 index, np);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(of_led_get_trigger_source);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6a8d6409c993e..c03e0a4e0e45d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -13,6 +13,7 @@
 #include <linux/kernfs.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/rwsem.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
@@ -352,6 +353,14 @@ struct led_trigger {
 	int		(*activate)(struct led_classdev *led_cdev);
 	void		(*deactivate)(struct led_classdev *led_cdev);
 
+	/*
+	 * Check whether LED has defined valid source for this trigger.
+	 * If yes, this trigger should be set as default trigger for LED.
+	 * This should use of_led_count_trigger_sources and
+	 * of_led_get_trigger_source functions.
+	 */
+	bool		(*has_valid_source)(struct led_classdev *led_cdev);
+
 	/* LED-private triggers have this set */
 	struct led_hw_trigger_type *trigger_type;
 
@@ -394,6 +403,10 @@ void led_trigger_set_default(struct led_classdev *led_cdev);
 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger);
 void led_trigger_remove(struct led_classdev *led_cdev);
 
+int of_led_count_trigger_sources(struct led_classdev *led_cdev);
+int of_led_get_trigger_source(struct led_classdev *led_cdev, int index,
+			      struct of_phandle_args *args);
+
 static inline void led_set_trigger_data(struct led_classdev *led_cdev,
 					void *trigger_data)
 {
@@ -452,6 +465,18 @@ static inline int led_trigger_set(struct led_classdev *led_cdev,
 }
 
 static inline void led_trigger_remove(struct led_classdev *led_cdev) {}
+
+static inline int of_led_count_trigger_sources(struct led_classdev *led_cdev)
+{
+	return 0;
+}
+static inline int of_led_get_trigger_source(struct led_classdev *led_cdev,
+					    int index,
+					    struct of_phandle_args *args)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline void led_set_trigger_data(struct led_classdev *led_cdev) {}
 static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 {
-- 
2.26.2

Re: [PATCH leds + devicetree v2 1/2] leds: trigger: add trigger sources validating method and helper functions

[Marek Behun]

On Tue, 15 Sep 2020 17:26:15 +0200
Marek Behún <marek.behun@nic.cz> wrote:

> +	/*
> +	 * Check whether LED has defined valid source for this trigger.
> +	 * If yes, this trigger should be set as default trigger for LED.
> +	 * This should use of_led_count_trigger_sources and
> +	 * of_led_get_trigger_source functions.
> +	 */
> +	bool		(*has_valid_source)(struct led_classdev *led_cdev);

Hmm, the heartbeat and timer triggers do not need trigger sources. If
we want to deprecate linux,default-trigger device tree property for
these triggers also, this method should look to `function` DT property
for heartbeat and timer triggers.

In that case the name should be changed to something like
of_is_valid_default_for, or something like that.

Marek

Re: [PATCH leds + devicetree v2 1/2] leds: trigger: add trigger sources validating method and helper functions

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

Hi!

> Currently we use the `linux,default-trigger` device tree property of a
> LED to define the default trigger which should be activated for a LED.
> 
> But the LED device tree binding also documents the `trigger-sources`
> property, which specifies the source device which should be triggering
> the LED.
> 
> The `trigger-sources` property is currently implemented only in
> drivers/usb/core/ledtrig-usbport.c.
> 
> Lets add a method to struct led_trigger which, if implemented, can check
> whether this trigger should be enabled as default. This check shall be
> done by checking whether the specified `trigger-sources` refers to a
> device compatible with the trigger. For this two new helper functions,
> of_led_count_trigger_sources and of_led_get_trigger_source, are
> implemented.

> +int of_led_count_trigger_sources(struct led_classdev *led_cdev)
> +{
> +	struct device_node *np;
> +	int count;
> +
> +	np = dev_of_node(led_cdev->dev);
> +	if (!np)
> +		return 0;
> +
> +	count = of_count_phandle_with_args(np, "trigger-sources",
> +					   "#trigger-source-cells");
> +	if (count == -ENOENT)
> +		return 0;
> +	else if (count < 0)
> +		dev_warn(led_cdev->dev,
> +			 "Failed parsing trigger sources for %pOF!\n", np);
> +
> +	return count;
> +}

Will this need of_node_put() somewhere?

Best regards,
									Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree

[Marek Behún]

Allow setting netdev LED trigger as default when given LED DT node has
the `trigger-sources` property pointing to a node corresponding to a
network device.

The specific netdev trigger mode is determined from the `function` LED
property.

Example:
  eth0: ethernet@30000 {
    compatible = "xyz";
    #trigger-source-cells = <0>;
  };

  led {
    color = <LED_COLOR_ID_GREEN>;
    function = LED_FUNCTION_LINK;
    trigger-sources = <&eth0>;
  };

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 drivers/leds/trigger/ledtrig-netdev.c | 80 ++++++++++++++++++++++++++-
 include/dt-bindings/leds/common.h     |  1 +
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d830215..99fc2f0c68e12 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_net.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include "../leds.h"
@@ -389,6 +390,70 @@ static void netdev_trig_work(struct work_struct *work)
 			(atomic_read(&trigger_data->interval)*2));
 }
 
+static bool netdev_trig_of_parse(struct led_classdev *led_cdev,
+				 struct led_netdev_data *trigger_data)
+{
+	struct of_phandle_args args;
+	struct net_device *netdev;
+	struct device_node *np;
+	const char *function;
+	unsigned long mode;
+	int err;
+
+	/* netdev trigger can have only one source */
+	if (of_led_count_trigger_sources(led_cdev) != 1)
+		return false;
+
+	err = of_led_get_trigger_source(led_cdev, 0, &args);
+	if (err)
+		return false;
+
+	netdev = of_find_net_device_by_node(args.np);
+	if (!netdev)
+		return false;
+
+	np = dev_of_node(led_cdev->dev);
+	if (!np)
+		return false;
+
+	err = of_property_read_string(np, "function", &function);
+	if (err && err != -ENOENT) {
+		dev_warn(led_cdev->dev, "Failed parsing function for %pOF!\n",
+			 np);
+		return false;
+	} else if (err == -ENOENT) {
+		/* default function is link */
+		function = LED_FUNCTION_LINK;
+	}
+
+	mode = 0;
+	if (!strcmp(function, LED_FUNCTION_LINK)) {
+		set_bit(NETDEV_LED_LINK, &mode);
+	} else if (!strcmp(function, LED_FUNCTION_ACTIVITY)) {
+		set_bit(NETDEV_LED_TX, &mode);
+		set_bit(NETDEV_LED_RX, &mode);
+	} else if (!strcmp(function, LED_FUNCTION_RX)) {
+		set_bit(NETDEV_LED_RX, &mode);
+	} else if (!strcmp(function, LED_FUNCTION_TX)) {
+		set_bit(NETDEV_LED_TX, &mode);
+	} else {
+		dev_dbg(led_cdev->dev,
+			"Unsupported netdev trigger function for %pOF!\n", np);
+		return false;
+	}
+
+	if (trigger_data) {
+		dev_hold(netdev);
+		trigger_data->net_dev = netdev;
+		memcpy(trigger_data->device_name, netdev->name, IFNAMSIZ);
+		trigger_data->mode = mode;
+		if (netif_carrier_ok(netdev))
+			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+	}
+
+	return true;
+}
+
 static int netdev_trig_activate(struct led_classdev *led_cdev)
 {
 	struct led_netdev_data *trigger_data;
@@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	trigger_data->last_activity = 0;
 
 	led_set_trigger_data(led_cdev, trigger_data);
+	netdev_trig_of_parse(led_cdev, trigger_data);
 
 	rc = register_netdevice_notifier(&trigger_data->notifier);
-	if (rc)
+	if (rc) {
+		if (trigger_data->net_dev)
+			dev_put(trigger_data->net_dev);
 		kfree(trigger_data);
+	} else {
+		if (trigger_data->net_dev)
+			set_baseline_state(trigger_data);
+	}
 
 	return rc;
 }
@@ -436,10 +508,16 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
 	kfree(trigger_data);
 }
 
+static bool netdev_trig_has_valid_source(struct led_classdev *led_cdev)
+{
+	return netdev_trig_of_parse(led_cdev, NULL);
+}
+
 static struct led_trigger netdev_led_trigger = {
 	.name = "netdev",
 	.activate = netdev_trig_activate,
 	.deactivate = netdev_trig_deactivate,
+	.has_valid_source = netdev_trig_has_valid_source,
 	.groups = netdev_trig_groups,
 };
 
diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index 52b619d44ba25..c7f9d34d60206 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -77,6 +77,7 @@
 #define LED_FUNCTION_HEARTBEAT "heartbeat"
 #define LED_FUNCTION_INDICATOR "indicator"
 #define LED_FUNCTION_LAN "lan"
+#define LED_FUNCTION_LINK "link"
 #define LED_FUNCTION_MAIL "mail"
 #define LED_FUNCTION_MTD "mtd"
 #define LED_FUNCTION_PANIC "panic"
-- 
2.26.2

Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree

[Jacek Anaszewski]

Hi Marek,

On 9/15/20 5:26 PM, Marek Behún wrote:
> Allow setting netdev LED trigger as default when given LED DT node has
> the `trigger-sources` property pointing to a node corresponding to a
> network device.
> 
> The specific netdev trigger mode is determined from the `function` LED
> property.
> 
> Example:
>    eth0: ethernet@30000 {
>      compatible = "xyz";
>      #trigger-source-cells = <0>;
>    };
> 
>    led {
>      color = <LED_COLOR_ID_GREEN>;
>      function = LED_FUNCTION_LINK;
>      trigger-sources = <&eth0>;
>    };
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>   drivers/leds/trigger/ledtrig-netdev.c | 80 ++++++++++++++++++++++++++-
>   include/dt-bindings/leds/common.h     |  1 +
>   2 files changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index d5e774d830215..99fc2f0c68e12 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -20,6 +20,7 @@
[...]

>   static int netdev_trig_activate(struct led_classdev *led_cdev)
>   {
>   	struct led_netdev_data *trigger_data;
> @@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>   	trigger_data->last_activity = 0;
>   
>   	led_set_trigger_data(led_cdev, trigger_data);
> +	netdev_trig_of_parse(led_cdev, trigger_data);

Please be aware of LED_INIT_DEFAULT_TRIGGER flag - it would make
sense to use it here so as not to unnecessarily call
netdev_trig_of_parse(), which makes sense only if trigger will be
default, I presume.

See timer_trig_activate() in  drivers/leds/trigger/ledtrig-timer.c
for reference.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree

[Marek Behun]

On Tue, 15 Sep 2020 23:35:25 +0200
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> Hi Marek,
> 
> On 9/15/20 5:26 PM, Marek Behún wrote:
> > Allow setting netdev LED trigger as default when given LED DT node has
> > the `trigger-sources` property pointing to a node corresponding to a
> > network device.
> > 
> > The specific netdev trigger mode is determined from the `function` LED
> > property.
> > 
> > Example:
> >    eth0: ethernet@30000 {
> >      compatible = "xyz";
> >      #trigger-source-cells = <0>;
> >    };
> > 
> >    led {
> >      color = <LED_COLOR_ID_GREEN>;
> >      function = LED_FUNCTION_LINK;
> >      trigger-sources = <&eth0>;
> >    };
> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > ---
> >   drivers/leds/trigger/ledtrig-netdev.c | 80 ++++++++++++++++++++++++++-
> >   include/dt-bindings/leds/common.h     |  1 +
> >   2 files changed, 80 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index d5e774d830215..99fc2f0c68e12 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -20,6 +20,7 @@  
> [...]
> 
> >   static int netdev_trig_activate(struct led_classdev *led_cdev)
> >   {
> >   	struct led_netdev_data *trigger_data;
> > @@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
> >   	trigger_data->last_activity = 0;
> >   
> >   	led_set_trigger_data(led_cdev, trigger_data);
> > +	netdev_trig_of_parse(led_cdev, trigger_data);  
> 
> Please be aware of LED_INIT_DEFAULT_TRIGGER flag - it would make
> sense to use it here so as not to unnecessarily call
> netdev_trig_of_parse(), which makes sense only if trigger will be
> default, I presume.
> 
> See timer_trig_activate() in  drivers/leds/trigger/ledtrig-timer.c
> for reference.
> 

Hmmm. Jacek, all the triggers that work with the macro
LED_INIT_DEFAULT_TRIGGER are oneshot, timer and pattern.
If this macro is set, they all call pattern_init function where they
read led-pattern from fwnode.

But there is no device tree in Linux sources using this property.
In fact the command
  git grep led-pattern
yields only 2 files:
  Documentation/devicetree/bindings/leds/common.yaml
  drivers/leds/led-core.c

What is the purpose if no device tree uses this property? Is this used
from other fwnode sources, like acpi or efi?

The reason why I am asking this is that the `led-pattern` property in
device tree goes against the principle of device tree, that it
shouldn't set settings settable from userspace, only describe the
devices on the system and how they are connected to each other.

This is the same reason why multi-CPU DSA proposals which proposed to
set mappings between CPU ports and user ports were rejected...

Marek

Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree

[Jacek Anaszewski]

On 9/16/20 2:15 AM, Marek Behun wrote:
> On Tue, 15 Sep 2020 23:35:25 +0200
> Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> 
>> Hi Marek,
>>
>> On 9/15/20 5:26 PM, Marek Behún wrote:
>>> Allow setting netdev LED trigger as default when given LED DT node has
>>> the `trigger-sources` property pointing to a node corresponding to a
>>> network device.
>>>
>>> The specific netdev trigger mode is determined from the `function` LED
>>> property.
>>>
>>> Example:
>>>     eth0: ethernet@30000 {
>>>       compatible = "xyz";
>>>       #trigger-source-cells = <0>;
>>>     };
>>>
>>>     led {
>>>       color = <LED_COLOR_ID_GREEN>;
>>>       function = LED_FUNCTION_LINK;
>>>       trigger-sources = <&eth0>;
>>>     };
>>>
>>> Signed-off-by: Marek Behún <marek.behun@nic.cz>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>>    drivers/leds/trigger/ledtrig-netdev.c | 80 ++++++++++++++++++++++++++-
>>>    include/dt-bindings/leds/common.h     |  1 +
>>>    2 files changed, 80 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
>>> index d5e774d830215..99fc2f0c68e12 100644
>>> --- a/drivers/leds/trigger/ledtrig-netdev.c
>>> +++ b/drivers/leds/trigger/ledtrig-netdev.c
>>> @@ -20,6 +20,7 @@
>> [...]
>>
>>>    static int netdev_trig_activate(struct led_classdev *led_cdev)
>>>    {
>>>    	struct led_netdev_data *trigger_data;
>>> @@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>>>    	trigger_data->last_activity = 0;
>>>    
>>>    	led_set_trigger_data(led_cdev, trigger_data);
>>> +	netdev_trig_of_parse(led_cdev, trigger_data);
>>
>> Please be aware of LED_INIT_DEFAULT_TRIGGER flag - it would make
>> sense to use it here so as not to unnecessarily call
>> netdev_trig_of_parse(), which makes sense only if trigger will be
>> default, I presume.
>>
>> See timer_trig_activate() in  drivers/leds/trigger/ledtrig-timer.c
>> for reference.
>>
> 
> Hmmm. Jacek, all the triggers that work with the macro
> LED_INIT_DEFAULT_TRIGGER are oneshot, timer and pattern.
> If this macro is set, they all call pattern_init function where they
> read led-pattern from fwnode.

The fact that they all call pattern_init() does not mean that
the use of flag is limited only to this type of triggers.

It has been introduced to allow initialization of default trigger
with required parameters, but in the same time, not to enforce
the same initial parameters each next time the trigger is set
for the LED.

> 
> But there is no device tree in Linux sources using this property.
> In fact the command
>    git grep led-pattern
> yields only 2 files:
>    Documentation/devicetree/bindings/leds/common.yaml
>    drivers/leds/led-core.c
> 
> What is the purpose if no device tree uses this property? Is this used
> from other fwnode sources, like acpi or efi?

This is mainly useful for debugging purposes, probably that's why
it is not present in official dts files yet.

> The reason why I am asking this is that the `led-pattern` property in
> device tree goes against the principle of device tree, that it
> shouldn't set settings settable from userspace, only describe the
> devices on the system and how they are connected to each other.

If that was a hard principle then we wouldn't have properties like
linux,default-trigger. I have once asked Rob about that - see the reply
at the and of message [0].

[0] https://lore.kernel.org/linux-leds/20181025195444.GA12737@bogus/

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

Hi!

> Allow setting netdev LED trigger as default when given LED DT node has
> the `trigger-sources` property pointing to a node corresponding to a
> network device.
> 
> The specific netdev trigger mode is determined from the `function` LED
> property.

Sounds reasonable.

> +	netdev = of_find_net_device_by_node(args.np);
> +	if (!netdev)
> +		return false;
> +
> +	np = dev_of_node(led_cdev->dev);
> +	if (!np)
> +		return false;

Missing of_node_put?

> +++ b/include/dt-bindings/leds/common.h
> @@ -77,6 +77,7 @@
>  #define LED_FUNCTION_HEARTBEAT "heartbeat"
>  #define LED_FUNCTION_INDICATOR "indicator"
>  #define LED_FUNCTION_LAN "lan"
> +#define LED_FUNCTION_LINK "link"
>  #define LED_FUNCTION_MAIL "mail"
>  #define LED_FUNCTION_MTD "mtd"
>  #define LED_FUNCTION_PANIC "panic"

We have function "lan" already defined; "link" would do mostly same
thing. Should we use "lan"? Or should we delete "lan" and replace it
with "link"?

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH leds + devicetree v2 2/2] leds: trigger: netdev: parse `trigger-sources` from device tree

[Marek Behun]

On Wed, 25 Nov 2020 11:42:42 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > Allow setting netdev LED trigger as default when given LED DT node has
> > the `trigger-sources` property pointing to a node corresponding to a
> > network device.
> > 
> > The specific netdev trigger mode is determined from the `function` LED
> > property.  
> 
> Sounds reasonable.
> 
> > +	netdev = of_find_net_device_by_node(args.np);
> > +	if (!netdev)
> > +		return false;
> > +
> > +	np = dev_of_node(led_cdev->dev);
> > +	if (!np)
> > +		return false;  
> 
> Missing of_node_put?

I will look into this.

> 
> > +++ b/include/dt-bindings/leds/common.h
> > @@ -77,6 +77,7 @@
> >  #define LED_FUNCTION_HEARTBEAT "heartbeat"
> >  #define LED_FUNCTION_INDICATOR "indicator"
> >  #define LED_FUNCTION_LAN "lan"
> > +#define LED_FUNCTION_LINK "link"
> >  #define LED_FUNCTION_MAIL "mail"
> >  #define LED_FUNCTION_MTD "mtd"
> >  #define LED_FUNCTION_PANIC "panic"  
> 
> We have function "lan" already defined; "link" would do mostly same
> thing. Should we use "lan"? Or should we delete "lan" and replace it
> with "link"?

Removal of "lan" should depend on whether it is used somewhere...

But I think "link" is more sensible since:
- it can distinguish better from "activity" if "activity" should mean
  blinking on RX/TX
- the interface must not necessarily be a LAN (in the sense that on
  routers we have WAN and LAN and possibly other, user defined
  networks).

We could use "lan" though to mean "link" and "activity" together (ie.
LED on when interface linked, LED blinking on RX/TX).

We have to decide whether specifying more LED functions to one LED in
DT should look like:

  function = LED_FUNCTION_LINK, LED_FUNCTION_ACTIVITY;

or rather like

  function = LED_FUNCTION_LAN;

The problem with the second one is that we would need specific
functions for different compound modes (if we wanted to do that), eg:
  function = LED_FUNCTION_LAN_RX;

Marek

Re: [PATCH leds + devicetree v2 0/2] Parse DT property `trigger-sources` for netdev LED trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

Hi!

> In this proposal the specific netdev LED trigger mode is determined
> from the `function` LED DT property.
> 
> Example:
>   eth0: ethernet@30000 {
>     compatible = "xyz";
>     #trigger-source-cells = <0>;
>   };
> 
>   led {
>     color = <LED_COLOR_ID_GREEN>;
>     function = LED_FUNCTION_LINK;
>     trigger-sources = <&eth0>;
>   };

> 
> When led is registered, the netdev trigger is automatically activated
> and set to light the LED on if eth0 is linked.
> 
> Please let me know if this binding is OK, or if the binding should
> instead of the `function` property determine the trigger settings from
> arguments of the `trigger-sources` property :
>   led {
>     color = <LED_COLOR_ID_GREEN>;
>     trigger-sources = <&eth0 (NETDEV_ATTR_LINK | NETDEV_ATTR_RX)>;
>   };

So... Both interfaces look relatively sane.

I might preffer the second one. For development boards, the LEDs
really have no labels (etc), thus no function -- they are user LEDs 1
to 4. But we still may want to say "user LED one should have mmc0
trigger by default".

Of course, we might also want to simply say that the LED is really mmc LED...

> I prefer the first binding, since we already have the `function`
> property. Multiple modes can be achieved by string array, but this is
> not yet implemented:
>   led {
>     color = <LED_COLOR_ID_GREEN>;
>     function = LED_FUNCTION_LINK, LED_FUNCTION_ACTIVITY;
>     trigger-sources = <&eth0>;
>   };

I don't see how multiple functions would work.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]