[PATCH 1/4] dt-bindings: leds: document property for LED triggers

[PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

There was a long discussion on describing LED trigger sources in DT.
Few solutions were posted but neither was clear & flexible enough. It's
really hard to design DT bindings for a LED node that will allow
describing any kinds of triggers.

Finally Jacek suggested a different design. It involved using separated
DT node for each trigger.

This documentation follows that idea. It really simplifies DT bindings
and allows clear support for different trigger types. With this solution
every type can have its own specific properties.

Please note an example will be added later with the first supported
trigger bindings. The point is to have nodes like:
foo-trigger {
	trigger-type = "foo";
	...
};

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 Documentation/devicetree/bindings/leds/common.txt   |  3 +++
 Documentation/devicetree/bindings/leds/triggers.txt | 13 +++++++++++++
 2 files changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/triggers.txt

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 696be5792625..0bc91556a47a 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -49,6 +49,9 @@ Optional properties for child nodes:
 - panic-indicator : This property specifies that the LED should be used,
 		    if at all possible, as a panic indicator.
 
+- triggers : List of nodes of triggers that should control this LED state. For
+	     more details see triggers.txt.
+
 Required properties for flash LED child nodes:
 - flash-max-microamp : Maximum flash LED supply current in microamperes.
 - flash-max-timeout-us : Maximum timeout in microseconds after which the flash
diff --git a/Documentation/devicetree/bindings/leds/triggers.txt b/Documentation/devicetree/bindings/leds/triggers.txt
new file mode 100644
index 000000000000..a1fbf3a75d67
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/triggers.txt
@@ -0,0 +1,13 @@
+Common trigger properties.
+
+Triggers describe the way LEDs should be controlled, e.g. under what conditions
+they should be turned on or off. Depending on a trigger type various events can
+be used to determine a LED state. Some triggers can be hardware independent
+(e.g. time based), some can react to a specific hardware events.
+
+Common properties:
+- trigger-type : Type of a trigger. Choosing a trigger determines what kind of
+		 events will be used to control LED. See specific trigger
+		 documentation for more details.
+
+More properties can be available depending on the chosen trigger type.
-- 
2.11.0

[PATCH 2/4] leds: triggers: add early support for trigger-type DT property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This adds support for getting DT node pointed in "triggers" property
and reading its "trigger-type". This will allow:
1) Activating proper trigger as specified in DT
2) Accessing DT info in specific triggers

Please note there is a validation of specified trigger type. This
requires documenting every supported value instead of blindly mapping
them into Linux triggers.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/leds/led-triggers.c | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h        |  1 +
 2 files changed, 42 insertions(+)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 572ede298d68..83897e0d6b76 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -16,6 +16,7 @@
 #include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/device.h>
+#include <linux/of.h>
 #include <linux/timer.h>
 #include <linux/rwsem.h>
 #include <linux/leds.h>
@@ -157,11 +158,48 @@ void led_trigger_remove(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_trigger_remove);
 
+static void led_trigger_of_read_trigger(struct led_classdev *led_cdev)
+{
+	struct device *dev = led_cdev->dev;
+	struct device_node *led_np = dev_of_node(dev);
+	struct of_phandle_args args;
+	const char *trigger_type;
+	int err;
+
+	if (!led_np)
+		return;
+
+	/* Currently we support only 1 trigger at a time */
+	err = of_parse_phandle_with_args(led_np, "triggers", NULL, 0, &args);
+	if (err)
+		return;
+
+	err = of_property_read_string(args.np, "trigger-type", &trigger_type);
+	if (err) {
+		dev_err(dev, "Failed to read trigger type\n");
+		goto err_node_put;
+	}
+
+	/* Check if trigger specified in DT is supported */
+	if (1) /* TODO */
+		goto err_node_put;
+
+	led_cdev->default_trigger = trigger_type;
+	led_cdev->trigger_node = args.np;
+
+	return;
+
+err_node_put:
+	of_node_put(args.np);
+}
+
 int led_trigger_set_default(struct led_classdev *led_cdev)
 {
 	struct led_trigger *trig;
 	int ret = -EPROBE_DEFER;
 
+	led_trigger_of_read_trigger(led_cdev);
+
 	if (!led_cdev->default_trigger)
 		return 0;
 
@@ -252,6 +290,9 @@ void led_trigger_unregister(struct led_trigger *trig)
 		up_write(&led_cdev->trigger_lock);
 	}
 	up_read(&leds_list_lock);
+
+	if (led_cdev->trigger_node)
+		of_node_put(led_cdev->trigger_node);
 }
 EXPORT_SYMBOL_GPL(led_trigger_unregister);
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 97ff66e3e8c4..bb6dcb9da4a9 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -106,6 +106,7 @@ struct led_classdev {
 
 	struct led_trigger	*trigger;
 	struct list_head	 trig_list;
+	struct device_node	*trigger_node;
 	void			*trigger_data;
 	/* true if activated - deactivate routine uses it to do cleanup */
 	bool			activated;
-- 
2.11.0

[PATCH 3/4] dt-bindings: leds: document binding for LED timer trigger

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Timer trigger is one of the simplest, hardware independent triggers. It
just blinks LED with a given intervals.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../devicetree/bindings/leds/triggers-timer.txt        | 18 ++++++++++++++++++
 Documentation/devicetree/bindings/leds/triggers.txt    | 17 +++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/triggers-timer.txt

diff --git a/Documentation/devicetree/bindings/leds/triggers-timer.txt b/Documentation/devicetree/bindings/leds/triggers-timer.txt
new file mode 100644
index 000000000000..0e6c81d6f81b
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/triggers-timer.txt
@@ -0,0 +1,18 @@
+Timer LED trigger properties.
+
+Timer trigger enables and disables LED with a given time intervals. It's example
+usage may be a simple indication that system is alive & working (to some point
+at least).
+
+Properties:
+- trigger-type : Must be "timer".
+- delay-on : Amount of time (in milliseconds) LED stays enabled.
+- delay-off : Amount of time (in milliseconds) LED stays disabled.
+
+Examples:
+
+timer-trigger {
+	trigger-type = "timer";
+	delay-on = <300>;
+	delay-off = <1000>;
+};
diff --git a/Documentation/devicetree/bindings/leds/triggers.txt b/Documentation/devicetree/bindings/leds/triggers.txt
index a1fbf3a75d67..8d8daa027bee 100644
--- a/Documentation/devicetree/bindings/leds/triggers.txt
+++ b/Documentation/devicetree/bindings/leds/triggers.txt
@@ -11,3 +11,20 @@ Common properties:
 		 documentation for more details.
 
 More properties can be available depending on the chosen trigger type.
+
+Examples:
+
+timer_trigger: timer-trigger {
+	trigger-type = "timer";
+	...
+};
+
+gpio-leds {
+	compatible = "gpio-leds";
+
+	system-status {
+		label = "Status";
+		gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
+		triggers = <&timer_trigger>;
+	};
+};
-- 
2.11.0

[PATCH 4/4] leds: triggers: support timer trigger DT bindings

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

Timer trigger has 2 parameters: delay on and delay off times. This patch
allows specifying "timer" trigger in DT including both time values.

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
 drivers/leds/led-triggers.c          |  2 +-
 drivers/leds/trigger/ledtrig-timer.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 83897e0d6b76..c53c20d676cd 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -181,7 +181,7 @@ static void led_trigger_of_read_trigger(struct led_classdev *led_cdev)
 	}
 
 	/* Check if trigger specified in DT is supported */
-	if (1) /* TODO */
+	if (strcmp(trigger_type, "timer"))
 		goto err_node_put;
 
 	led_cdev->default_trigger = trigger_type;
diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
index 8d09327b5719..98b58469d705 100644
--- a/drivers/leds/trigger/ledtrig-timer.c
+++ b/drivers/leds/trigger/ledtrig-timer.c
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/ctype.h>
 #include <linux/leds.h>
+#include <linux/of.h>
 
 static ssize_t led_delay_on_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
@@ -71,10 +72,25 @@ static ssize_t led_delay_off_store(struct device *dev,
 static DEVICE_ATTR(delay_on, 0644, led_delay_on_show, led_delay_on_store);
 static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
 
+static void ledtrig_timer_of_read(struct led_classdev *led_cdev)
+{
+	struct device_node *np = led_cdev->trigger_node;
+
+	if (!np)
+		return;
+
+	of_property_read_u32(np, "delay-on",
+			     (u32 *)&led_cdev->blink_delay_on);
+	of_property_read_u32(np, "delay-off",
+			     (u32 *)&led_cdev->blink_delay_off);
+}
+
 static void timer_trig_activate(struct led_classdev *led_cdev)
 {
 	int rc;
 
+	ledtrig_timer_of_read(led_cdev);
+
 	led_cdev->trigger_data = NULL;
 
 	rc = device_create_file(led_cdev->dev, &dev_attr_delay_on);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Jacek Anaszewski]

Hi Rafał,

Thanks for continuing this effort.

I think that it would be simpler if we could initially see
a complete sample dts implementation containing all required DT
nodes. The example could contain timer trigger as well as usb-port
trigger specific bindings.

I suppose that we should see DT nodes containing #list-cells
properties that define the quantity of phandle arguments.

It seems that this approach allows for defining a list of elements
with variable number of arguments, i.e. what you were initially
asking for.


Best regards,
Jacek Anaszewski

On 02/28/2017 01:04 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> 
> There was a long discussion on describing LED trigger sources in DT.
> Few solutions were posted but neither was clear & flexible enough. It's
> really hard to design DT bindings for a LED node that will allow
> describing any kinds of triggers.
> 
> Finally Jacek suggested a different design. It involved using separated
> DT node for each trigger.
> 
> This documentation follows that idea. It really simplifies DT bindings
> and allows clear support for different trigger types. With this solution
> every type can have its own specific properties.
> 
> Please note an example will be added later with the first supported
> trigger bindings. The point is to have nodes like:
> foo-trigger {
> 	trigger-type = "foo";
> 	...
> };
> 
> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/leds/common.txt   |  3 +++
>  Documentation/devicetree/bindings/leds/triggers.txt | 13 +++++++++++++
>  2 files changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/triggers.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 696be5792625..0bc91556a47a 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -49,6 +49,9 @@ Optional properties for child nodes:
>  - panic-indicator : This property specifies that the LED should be used,
>  		    if at all possible, as a panic indicator.
>  
> +- triggers : List of nodes of triggers that should control this LED state. For
> +	     more details see triggers.txt.
> +
>  Required properties for flash LED child nodes:
>  - flash-max-microamp : Maximum flash LED supply current in microamperes.
>  - flash-max-timeout-us : Maximum timeout in microseconds after which the flash
> diff --git a/Documentation/devicetree/bindings/leds/triggers.txt b/Documentation/devicetree/bindings/leds/triggers.txt
> new file mode 100644
> index 000000000000..a1fbf3a75d67
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/triggers.txt
> @@ -0,0 +1,13 @@
> +Common trigger properties.
> +
> +Triggers describe the way LEDs should be controlled, e.g. under what conditions
> +they should be turned on or off. Depending on a trigger type various events can
> +be used to determine a LED state. Some triggers can be hardware independent
> +(e.g. time based), some can react to a specific hardware events.
> +
> +Common properties:
> +- trigger-type : Type of a trigger. Choosing a trigger determines what kind of
> +		 events will be used to control LED. See specific trigger
> +		 documentation for more details.
> +
> +More properties can be available depending on the chosen trigger type.
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Rafał Miłecki]

On 02/28/2017 10:38 PM, Jacek Anaszewski wrote:
> I think that it would be simpler if we could initially see
> a complete sample dts implementation containing all required DT
> nodes. The example could contain timer trigger as well as usb-port
> trigger specific bindings.

Please take a look at attached patch. I used it on Tenda AC9 with:

usb_trigger: usb-trigger {
	trigger-type = "usbport";
	ports = <&ohci_port1>, <&ehci_port1>;
};

usb {
	label = "bcm53xx:blue:usb";
	gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
	triggers = <&usb_trigger>;
};


> I suppose that we should see DT nodes containing #list-cells
> properties that define the quantity of phandle arguments.
>
> It seems that this approach allows for defining a list of elements
> with variable number of arguments, i.e. what you were initially
> asking for.

Are you sure we need #list-cells? Can't we simply use
of_count_phandle_with_args(np, "triggers", NULL);
?


From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <rafal@milecki.pl>
Date: Mon, 3 Oct 2016 16:49:05 +0200
Subject: [PATCH] usb: core: read USB ports from DT in the usbport LED trigger
  driver
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
  .../devicetree/bindings/leds/triggers-usbport.txt  | 19 ++++++++
  drivers/leds/led-triggers.c                        |  3 +-
  drivers/usb/core/ledtrig-usbport.c                 | 56 ++++++++++++++++++++++
  3 files changed, 77 insertions(+), 1 deletion(-)
  create mode 100644 Documentation/devicetree/bindings/leds/triggers-usbport.txt

diff --git a/Documentation/devicetree/bindings/leds/triggers-usbport.txt b/Documentation/devicetree/bindings/leds/triggers-usbport.txt
new file mode 100644
index 000000000000..10e55122cded
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/triggers-usbport.txt
@@ -0,0 +1,19 @@
+USB port LED trigger properties.
+
+USB port trigger can be used for signalling to the user a presence of USB
+device(s) in given ports. It's possible to specify USB ports in DT by providing
+their references.
+
+Properties:
+- trigger-type : Must be "usbport".
+- ports : List of USB ports related to this LED. Some devices may have one USB
+	  LED for all ports, other may have few of them (e.g. USB version
+	  specific). It's used by usbport trigger for reading a list of ports
+	  that should cause LED to turn on whenver device get connected.
+
+Examples:
+
+usbport-trigger {
+	trigger-type = "usbport";
+	ports = <&ohci_port1>, <&ehci_port1>;
+};
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index c53c20d676cd..1d8bda9755ca 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -181,7 +181,8 @@ static void led_trigger_of_read_trigger(struct led_classdev *led_cdev)
  	}

  	/* Check if trigger specified in DT is supported */
-	if (strcmp(trigger_type, "timer"))
+	if (strcmp(trigger_type, "timer") &&
+	    strcmp(trigger_type, "usbport"))
  		goto err_node_put;

  	led_cdev->default_trigger = trigger_type;
diff --git a/drivers/usb/core/ledtrig-usbport.c b/drivers/usb/core/ledtrig-usbport.c
index 1713248ab15a..599f7e6b0ba8 100644
--- a/drivers/usb/core/ledtrig-usbport.c
+++ b/drivers/usb/core/ledtrig-usbport.c
@@ -11,8 +11,10 @@
  #include <linux/device.h>
  #include <linux/leds.h>
  #include <linux/module.h>
+#include <linux/of.h>
  #include <linux/slab.h>
  #include <linux/usb.h>
+#include <linux/usb/of.h>

  struct usbport_trig_data {
  	struct led_classdev *led_cdev;
@@ -123,6 +125,58 @@ static const struct attribute_group ports_group = {
   * Adding & removing ports
   ***************************************/

+/**
+ * usbport_trig_port_observed - Check if port should be observed
+ *
+ * Each LED may have list of related USB ports specified in a DT. This function
+ * reads them using ports property and sets a proper state.
+ */
+static bool usbport_trig_port_observed(struct usbport_trig_data *usbport_data,
+				       struct usb_device *usb_dev, int port1)
+{
+	struct device_node *led_np = usbport_data->led_cdev->trigger_node;
+	struct device *dev = usbport_data->led_cdev->dev;
+	struct of_phandle_args args;
+	struct device_node *port_np;
+	int count, i;
+
+	if (!led_np)
+		return false;
+
+	/* Get node of port being added */
+	port_np = usb_of_get_child_node(usb_dev->dev.of_node, port1);
+	if (!port_np)
+		return false;
+
+	/* Amount of ports this LED references */
+	count = of_count_phandle_with_args(led_np, "ports", NULL);
+	if (count < 0) {
+		dev_warn(dev, "Failed to get USB ports for %s\n",
+			 led_np->full_name);
+		return false;
+	}
+
+	/* Check if port is on this LED's list */
+	for (i = 0; i < count; i++) {
+		int err;
+
+		err = of_parse_phandle_with_args(led_np, "ports", NULL, i,
+						 &args);
+		if (err) {
+			dev_err(dev, "Failed to get USB port phandle at index %d: %d\n",
+				i, err);
+			continue;
+		}
+
+		of_node_put(args.np);
+
+		if (args.np == port_np)
+			return true;
+	}
+
+	return false;
+}
+
  static int usbport_trig_add_port(struct usbport_trig_data *usbport_data,
  				 struct usb_device *usb_dev,
  				 const char *hub_name, int portnum)
@@ -141,6 +195,7 @@ static int usbport_trig_add_port(struct usbport_trig_data *usbport_data,
  	port->data = usbport_data;
  	port->hub = usb_dev;
  	port->portnum = portnum;
+	port->observed = usbport_trig_port_observed(usbport_data, usb_dev, portnum);

  	len = strlen(hub_name) + 8;
  	port->port_name = kzalloc(len, GFP_KERNEL);
@@ -255,6 +310,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
  	if (err)
  		goto err_free;
  	usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports);
+	usbport_trig_update_count(usbport_data);

  	/* Notifications */
  	usbport_data->nb.notifier_call = usbport_trig_notify,
-- 
2.11.0

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Rob Herring]

On Tue, Feb 28, 2017 at 3:51 PM, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote:
>>
>> I think that it would be simpler if we could initially see
>> a complete sample dts implementation containing all required DT
>> nodes. The example could contain timer trigger as well as usb-port
>> trigger specific bindings.
>
>
> Please take a look at attached patch. I used it on Tenda AC9 with:

I'm not sure about this extra level of indirection. I don't see the need.

> usb_trigger: usb-trigger {
>         trigger-type = "usbport";

Why do we need to know the type? The trigger device knows what type it
is. All we should need to know here is what device(s) controls an LED.
The rest the kernel can figure out.

>         ports = <&ohci_port1>, <&ehci_port1>;
> };
>
> usb {
>         label = "bcm53xx:blue:usb";
>         gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
>         triggers = <&usb_trigger>;
> };

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Jacek Anaszewski]

On 02/28/2017 11:12 PM, Rob Herring wrote:
> On Tue, Feb 28, 2017 at 3:51 PM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote:
>>>
>>> I think that it would be simpler if we could initially see
>>> a complete sample dts implementation containing all required DT
>>> nodes. The example could contain timer trigger as well as usb-port
>>> trigger specific bindings.
>>
>>
>> Please take a look at attached patch. I used it on Tenda AC9 with:
> 
> I'm not sure about this extra level of indirection. I don't see the need.
> 
>> usb_trigger: usb-trigger {
>>         trigger-type = "usbport";
> 
> Why do we need to know the type? The trigger device knows what type it
> is. All we should need to know here is what device(s) controls an LED.
> The rest the kernel can figure out.

The thing is that in the proposed approach the trigger is not necessary
a device. The trigger node is here only a container with initialization
data.

We could have e,g, two such nodes with different set of ports
(say usb1-trigger and usb2-trigger). Then if we had two LED nodes usb1
and usb2, the former could have its triggers property initialized
to &usb1-trigger and the latter to &usb2-trigger. Thanks to that both
LEDs after executing "echo usbport > triggers" would listen to events
from different set of usb ports.

Also, e.g. for the timer trigger we could define two separate DT nodes
with different delay intervals.

>>         ports = <&ohci_port1>, <&ehci_port1>;
>> };
>>
>> usb {
>>         label = "bcm53xx:blue:usb";
>>         gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
>>         triggers = <&usb_trigger>;
>> };
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Rob Herring]

On Wed, Mar 1, 2017 at 3:04 PM, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> On 02/28/2017 11:12 PM, Rob Herring wrote:
>> On Tue, Feb 28, 2017 at 3:51 PM, Rafał Miłecki <zajec5@gmail.com> wrote:
>>> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote:
>>>>
>>>> I think that it would be simpler if we could initially see
>>>> a complete sample dts implementation containing all required DT
>>>> nodes. The example could contain timer trigger as well as usb-port
>>>> trigger specific bindings.
>>>
>>>
>>> Please take a look at attached patch. I used it on Tenda AC9 with:
>>
>> I'm not sure about this extra level of indirection. I don't see the need.
>>
>>> usb_trigger: usb-trigger {
>>>         trigger-type = "usbport";
>>
>> Why do we need to know the type? The trigger device knows what type it
>> is. All we should need to know here is what device(s) controls an LED.
>> The rest the kernel can figure out.
>
> The thing is that in the proposed approach the trigger is not necessary
> a device. The trigger node is here only a container with initialization
> data.

That sounds like a questionable use of DT.

> We could have e,g, two such nodes with different set of ports
> (say usb1-trigger and usb2-trigger). Then if we had two LED nodes usb1
> and usb2, the former could have its triggers property initialized
> to &usb1-trigger and the latter to &usb2-trigger. Thanks to that both
> LEDs after executing "echo usbport > triggers" would listen to events
> from different set of usb ports.

This would still work if the trigger property points directly

> Also, e.g. for the timer trigger we could define two separate DT nodes
> with different delay intervals.

That sounds like user config to me.

>
>>>         ports = <&ohci_port1>, <&ehci_port1>;
>>> };
>>>
>>> usb {
>>>         label = "bcm53xx:blue:usb";
>>>         gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
>>>         triggers = <&usb_trigger>;
>>> };
>>
>
> --
> Best regards,
> Jacek Anaszewski

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Rafał Miłecki]

On 03/01/2017 10:04 PM, Jacek Anaszewski wrote:
> On 02/28/2017 11:12 PM, Rob Herring wrote:
>> On Tue, Feb 28, 2017 at 3:51 PM, Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote:
>>>>
>>>> I think that it would be simpler if we could initially see
>>>> a complete sample dts implementation containing all required DT
>>>> nodes. The example could contain timer trigger as well as usb-port
>>>> trigger specific bindings.
>>>
>>>
>>> Please take a look at attached patch. I used it on Tenda AC9 with:
>>
>> I'm not sure about this extra level of indirection. I don't see the need.
>>
>>> usb_trigger: usb-trigger {
>>>         trigger-type = "usbport";
>>
>> Why do we need to know the type? The trigger device knows what type it
>> is. All we should need to know here is what device(s) controls an LED.
>> The rest the kernel can figure out.
>
> The thing is that in the proposed approach the trigger is not necessary
> a device. The trigger node is here only a container with initialization
> data.
>
> We could have e,g, two such nodes with different set of ports
> (say usb1-trigger and usb2-trigger). Then if we had two LED nodes usb1
> and usb2, the former could have its triggers property initialized
> to &usb1-trigger and the latter to &usb2-trigger. Thanks to that both
> LEDs after executing "echo usbport > triggers" would listen to events
> from different set of usb ports.
>
> Also, e.g. for the timer trigger we could define two separate DT nodes
> with different delay intervals.

Hi Rob, what do you think about this?

Jacek is correct, we could have e.g.
timer-trigger-1 {
	trigger-type = "timer";
	delay-on = <200>;
	delay-off = <500>;
};
timer-trigger-2 {
	trigger-type = "timer";
	delay-on = <500>;
	delay-off = <1000>;
};

or something like:
usbport-2-0-trigger {
	trigger-type = "usbport";
	ports = <&ohci_port1>, <&ehci_port1>;
};
usbport-3-0-trigger {
	trigger-type = "usbport";
	ports = <&xhci_port1>;
};

Does it make more sense?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Rob Herring]

On Mon, Mar 06, 2017 at 07:06:32AM +0100, Rafał Miłecki wrote:
> On 03/01/2017 10:04 PM, Jacek Anaszewski wrote:
> > On 02/28/2017 11:12 PM, Rob Herring wrote:
> > > On Tue, Feb 28, 2017 at 3:51 PM, Rafał Miłecki <zajec5@gmail.com> wrote:
> > > > On 02/28/2017 10:38 PM, Jacek Anaszewski wrote:
> > > > > 
> > > > > I think that it would be simpler if we could initially see
> > > > > a complete sample dts implementation containing all required DT
> > > > > nodes. The example could contain timer trigger as well as usb-port
> > > > > trigger specific bindings.
> > > > 
> > > > 
> > > > Please take a look at attached patch. I used it on Tenda AC9 with:
> > > 
> > > I'm not sure about this extra level of indirection. I don't see the need.
> > > 
> > > > usb_trigger: usb-trigger {
> > > >         trigger-type = "usbport";
> > > 
> > > Why do we need to know the type? The trigger device knows what type it
> > > is. All we should need to know here is what device(s) controls an LED.
> > > The rest the kernel can figure out.
> > 
> > The thing is that in the proposed approach the trigger is not necessary
> > a device. The trigger node is here only a container with initialization
> > data.
> > 
> > We could have e,g, two such nodes with different set of ports
> > (say usb1-trigger and usb2-trigger). Then if we had two LED nodes usb1
> > and usb2, the former could have its triggers property initialized
> > to &usb1-trigger and the latter to &usb2-trigger. Thanks to that both
> > LEDs after executing "echo usbport > triggers" would listen to events
> > from different set of usb ports.
> > 
> > Also, e.g. for the timer trigger we could define two separate DT nodes
> > with different delay intervals.
> 
> Hi Rob, what do you think about this?
> 
> Jacek is correct, we could have e.g.
> timer-trigger-1 {
> 	trigger-type = "timer";
> 	delay-on = <200>;
> 	delay-off = <500>;
> };
> timer-trigger-2 {
> 	trigger-type = "timer";
> 	delay-on = <500>;
> 	delay-off = <1000>;
> };

This is just letting the Linux driver structure define/influence the 
binding. This could all be described with a single LED property like 
"led-blink-pattern". The fact that it gets implemented by the timer 
trigger is irrelevant to the binding.

Plus timer based triggering seems more like user controlled than 
platform or hardware defined. So trying to create a binding for all 
Linux triggers is a mistake. The only thing we need here is a way to 
associate an LED with a h/w device. 

> 
> or something like:
> usbport-2-0-trigger {
> 	trigger-type = "usbport";
> 	ports = <&ohci_port1>, <&ehci_port1>;
> };
> usbport-3-0-trigger {
> 	trigger-type = "usbport";
> 	ports = <&xhci_port1>;
> };
> 
> Does it make more sense?

No, for the reasons above.

Rob

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Jacek Anaszewski]

On 02/28/2017 10:51 PM, Rafał Miłecki wrote:
> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote:
>> I think that it would be simpler if we could initially see
>> a complete sample dts implementation containing all required DT
>> nodes. The example could contain timer trigger as well as usb-port
>> trigger specific bindings.
> 
> Please take a look at attached patch. I used it on Tenda AC9 with:
> 
> usb_trigger: usb-trigger {
>     trigger-type = "usbport";
>     ports = <&ohci_port1>, <&ehci_port1>;
> };
> 
> usb {
>     label = "bcm53xx:blue:usb";
>     gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
>     triggers = <&usb_trigger>;
> };

OK, I got it, thanks.

> 
>> I suppose that we should see DT nodes containing #list-cells
>> properties that define the quantity of phandle arguments.
>>
>> It seems that this approach allows for defining a list of elements
>> with variable number of arguments, i.e. what you were initially
>> asking for.
> 
> Are you sure we need #list-cells? Can't we simply use
> of_count_phandle_with_args(np, "triggers", NULL);
> ?

I'm not sure, I just read the function documentation :-)
I haven't verified nor have I used this API.

> 
> From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> Date: Mon, 3 Oct 2016 16:49:05 +0200
> Subject: [PATCH] usb: core: read USB ports from DT in the usbport LED
> trigger
>  driver
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> ---
>  .../devicetree/bindings/leds/triggers-usbport.txt  | 19 ++++++++
>  drivers/leds/led-triggers.c                        |  3 +-
>  drivers/usb/core/ledtrig-usbport.c                 | 56
> ++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 1 deletion(-)
>  create mode 100644
> Documentation/devicetree/bindings/leds/triggers-usbport.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/triggers-usbport.txt
> b/Documentation/devicetree/bindings/leds/triggers-usbport.txt
> new file mode 100644
> index 000000000000..10e55122cded
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/triggers-usbport.txt
> @@ -0,0 +1,19 @@
> +USB port LED trigger properties.
> +
> +USB port trigger can be used for signalling to the user a presence of USB
> +device(s) in given ports. It's possible to specify USB ports in DT by
> providing
> +their references.
> +
> +Properties:
> +- trigger-type : Must be "usbport".
> +- ports : List of USB ports related to this LED. Some devices may have
> one USB
> +      LED for all ports, other may have few of them (e.g. USB version
> +      specific). It's used by usbport trigger for reading a list of ports
> +      that should cause LED to turn on whenver device get connected.
> +
> +Examples:
> +
> +usbport-trigger {
> +    trigger-type = "usbport";
> +    ports = <&ohci_port1>, <&ehci_port1>;
> +};
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index c53c20d676cd..1d8bda9755ca 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -181,7 +181,8 @@ static void led_trigger_of_read_trigger(struct
> led_classdev *led_cdev)
>      }
> 
>      /* Check if trigger specified in DT is supported */
> -    if (strcmp(trigger_type, "timer"))
> +    if (strcmp(trigger_type, "timer") &&
> +        strcmp(trigger_type, "usbport"))
>          goto err_node_put;
> 
>      led_cdev->default_trigger = trigger_type;
> diff --git a/drivers/usb/core/ledtrig-usbport.c
> b/drivers/usb/core/ledtrig-usbport.c
> index 1713248ab15a..599f7e6b0ba8 100644
> --- a/drivers/usb/core/ledtrig-usbport.c
> +++ b/drivers/usb/core/ledtrig-usbport.c
> @@ -11,8 +11,10 @@
>  #include <linux/device.h>
>  #include <linux/leds.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/slab.h>
>  #include <linux/usb.h>
> +#include <linux/usb/of.h>
> 
>  struct usbport_trig_data {
>      struct led_classdev *led_cdev;
> @@ -123,6 +125,58 @@ static const struct attribute_group ports_group = {
>   * Adding & removing ports
>   ***************************************/
> 
> +/**
> + * usbport_trig_port_observed - Check if port should be observed
> + *
> + * Each LED may have list of related USB ports specified in a DT. This
> function
> + * reads them using ports property and sets a proper state.
> + */
> +static bool usbport_trig_port_observed(struct usbport_trig_data
> *usbport_data,
> +                       struct usb_device *usb_dev, int port1)
> +{
> +    struct device_node *led_np = usbport_data->led_cdev->trigger_node;
> +    struct device *dev = usbport_data->led_cdev->dev;
> +    struct of_phandle_args args;
> +    struct device_node *port_np;
> +    int count, i;
> +
> +    if (!led_np)
> +        return false;
> +
> +    /* Get node of port being added */
> +    port_np = usb_of_get_child_node(usb_dev->dev.of_node, port1);
> +    if (!port_np)
> +        return false;
> +
> +    /* Amount of ports this LED references */
> +    count = of_count_phandle_with_args(led_np, "ports", NULL);
> +    if (count < 0) {
> +        dev_warn(dev, "Failed to get USB ports for %s\n",
> +             led_np->full_name);
> +        return false;
> +    }
> +
> +    /* Check if port is on this LED's list */
> +    for (i = 0; i < count; i++) {
> +        int err;
> +
> +        err = of_parse_phandle_with_args(led_np, "ports", NULL, i,
> +                         &args);
> +        if (err) {
> +            dev_err(dev, "Failed to get USB port phandle at index %d:
> %d\n",
> +                i, err);
> +            continue;
> +        }
> +
> +        of_node_put(args.np);
> +
> +        if (args.np == port_np)
> +            return true;
> +    }
> +
> +    return false;
> +}
> +
>  static int usbport_trig_add_port(struct usbport_trig_data *usbport_data,
>                   struct usb_device *usb_dev,
>                   const char *hub_name, int portnum)
> @@ -141,6 +195,7 @@ static int usbport_trig_add_port(struct
> usbport_trig_data *usbport_data,
>      port->data = usbport_data;
>      port->hub = usb_dev;
>      port->portnum = portnum;
> +    port->observed = usbport_trig_port_observed(usbport_data, usb_dev,
> portnum);
> 
>      len = strlen(hub_name) + 8;
>      port->port_name = kzalloc(len, GFP_KERNEL);
> @@ -255,6 +310,7 @@ static void usbport_trig_activate(struct
> led_classdev *led_cdev)
>      if (err)
>          goto err_free;
>      usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports);
> +    usbport_trig_update_count(usbport_data);
> 
>      /* Notifications */
>      usbport_data->nb.notifier_call = usbport_trig_notify,

-- 
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Rafał Miłecki]

On 03/01/2017 10:04 PM, Jacek Anaszewski wrote:
> On 02/28/2017 10:51 PM, Rafał Miłecki wrote:
>> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote:
>>> I think that it would be simpler if we could initially see
>>> a complete sample dts implementation containing all required DT
>>> nodes. The example could contain timer trigger as well as usb-port
>>> trigger specific bindings.
>>
>> Please take a look at attached patch. I used it on Tenda AC9 with:
>>
>> usb_trigger: usb-trigger {
>>     trigger-type = "usbport";
>>     ports = <&ohci_port1>, <&ehci_port1>;
>> };
>>
>> usb {
>>     label = "bcm53xx:blue:usb";
>>     gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
>>     triggers = <&usb_trigger>;
>> };
>
> OK, I got it, thanks.
>
>>
>>> I suppose that we should see DT nodes containing #list-cells
>>> properties that define the quantity of phandle arguments.
>>>
>>> It seems that this approach allows for defining a list of elements
>>> with variable number of arguments, i.e. what you were initially
>>> asking for.
>>
>> Are you sure we need #list-cells? Can't we simply use
>> of_count_phandle_with_args(np, "triggers", NULL);
>> ?
>
> I'm not sure, I just read the function documentation :-)
> I haven't verified nor have I used this API.

So we could use #list-cells to support something like:

usb {
	label = "bcm53xx:blue:usb";
	gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
	triggers = <&usb_trigger 1 2>, <&timer_trigger 3 4 5>;
};

but I don't see any need for this. Just specifying triggers like:
triggers = <&usb_trigger>, <&timer_trigger>, <&foo>;
should be always enough, especially with the new trigger nodes you suggested.

Re: [PATCH 1/4] dt-bindings: leds: document property for LED triggers

[Jacek Anaszewski]

On 03/06/2017 07:16 AM, Rafał Miłecki wrote:
> On 03/01/2017 10:04 PM, Jacek Anaszewski wrote:
>> On 02/28/2017 10:51 PM, Rafał Miłecki wrote:
>>> On 02/28/2017 10:38 PM, Jacek Anaszewski wrote:
>>>> I think that it would be simpler if we could initially see
>>>> a complete sample dts implementation containing all required DT
>>>> nodes. The example could contain timer trigger as well as usb-port
>>>> trigger specific bindings.
>>>
>>> Please take a look at attached patch. I used it on Tenda AC9 with:
>>>
>>> usb_trigger: usb-trigger {
>>>     trigger-type = "usbport";
>>>     ports = <&ohci_port1>, <&ehci_port1>;
>>> };
>>>
>>> usb {
>>>     label = "bcm53xx:blue:usb";
>>>     gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
>>>     triggers = <&usb_trigger>;
>>> };
>>
>> OK, I got it, thanks.
>>
>>>
>>>> I suppose that we should see DT nodes containing #list-cells
>>>> properties that define the quantity of phandle arguments.
>>>>
>>>> It seems that this approach allows for defining a list of elements
>>>> with variable number of arguments, i.e. what you were initially
>>>> asking for.
>>>
>>> Are you sure we need #list-cells? Can't we simply use
>>> of_count_phandle_with_args(np, "triggers", NULL);
>>> ?
>>
>> I'm not sure, I just read the function documentation :-)
>> I haven't verified nor have I used this API.
> 
> So we could use #list-cells to support something like:
> 
> usb {
>     label = "bcm53xx:blue:usb";
>     gpios = <&chipcommon 1 GPIO_ACTIVE_HIGH>;
>     triggers = <&usb_trigger 1 2>, <&timer_trigger 3 4 5>;
> };
> 
> but I don't see any need for this. Just specifying triggers like:
> triggers = <&usb_trigger>, <&timer_trigger>, <&foo>;
> should be always enough, especially with the new trigger nodes you
> suggested.

Indeed, it seems to be not needed for this approach.
I'm OK with this design.

-- 
Best regards,
Jacek Anaszewski