[PATCH v3 0/5] leds: trigger: Add pattern initialization from Device Tree

[PATCH v3 0/5] leds: trigger: Add pattern initialization from Device Tree

[Krzysztof Kozlowski]

Hi,

Changes since v2:
1. Drop Jacek's patches and my "led: triggers: Initialize
   LED_INIT_DEFAULT_TRIGGER if trigger is brought after class".
2. Follow Rob's advices about bindings - use "led-pattern" property
   and generalize usage of it into to three triggers.
3. New patches (2/5, 4/5 and 5/5).

Changes since v1:
1. Rebase on Jacek's patches.
2. Add patch 3/5 for fixup of Jacek's solution.
3. Drop first two patches from the series (applied).
4. Patch 5/5: Use LED_INIT_DEFAULT_TRIGGER (suggested by Jacek Anaszewski).
5. Patch 5/5: Return-on-error and log warning (suggested by Pavel Machek).


Best regards,
Krzysztof


Krzysztof Kozlowski (5):
  dt-bindings: leds: Add pattern initialization from Device Tree
  leds: Add helper for getting default pattern from Device Tree
  leds: trigger: pattern: Add pattern initialization from Device Tree
  leds: trigger: oneshot: Add initialization from Device Tree
  leds: trigger: timer: Add initialization from Device Tree

 Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++
 drivers/leds/led-class.c                          | 25 ++++++++++++++++
 drivers/leds/trigger/ledtrig-oneshot.c            | 31 +++++++++++++++++--
 drivers/leds/trigger/ledtrig-pattern.c            | 25 ++++++++++++++++
 drivers/leds/trigger/ledtrig-timer.c              | 28 ++++++++++++++++++
 include/linux/leds.h                              |  3 ++
 6 files changed, 146 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v3 1/5] dt-bindings: leds: Add pattern initialization from Device Tree

[Krzysztof Kozlowski]

Document new linux,trigger-pattern property for initialization of LED
pattern trigger.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index aa1399814a2a..3daccd4ea8a3 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -37,6 +37,42 @@ Optional properties for child nodes:
      "ide-disk" - LED indicates IDE disk activity (deprecated),
                   in new implementations use "disk-activity"
      "timer" - LED flashes at a fixed, configurable rate
+     "pattern" - LED alters the brightness for the specified duration with one
+                 software timer (requires "led-pattern" property)
+
+- led-pattern : String with default pattern for certain triggers. Each trigger
+                may parse this string differently:
+                - one-shot : two numbers specifying delay on and delay off,
+                - timer : two numbers specifying delay on and delay off,
+                - pattern : The pattern is given by a series of tuples, of
+                  brightness and duration (ms).  The LED is expected to traverse
+                  the series and each brightness value for the specified
+                  duration.  Duration of 0 means brightness should immediately
+                  change to new value.
+
+                  1. For gradual dimming, the dimming interval now is set as 50
+                  milliseconds. So the tuple with duration less than dimming
+                  interval (50ms) is treated as a step change of brightness,
+                  i.e. the subsequent brightness will be applied without adding
+                  intervening dimming intervals.
+
+                  The gradual dimming format of the software pattern values should be:
+                  "brightness_1 duration_1 brightness_2 duration_2 brightness_3
+                  duration_3 ...".
+                  For example "0 1000 255 2000" will make the LED go gradually
+                  from zero-intensity to max (255) intensity in 1000
+                  milliseconds, then back to zero intensity in 2000
+                  milliseconds.
+
+                  2. To make the LED go instantly from one brightness value to
+                  another, pattern should use zero-time lengths (the brightness
+                  must be same as the previous tuple's). So the format should be:
+                  "brightness_1 duration_1 brightness_1 0 brightness_2
+                  duration_2 brightness_2 0 ...".
+                  For example "0 1000 0 0 255 2000 255 0" will make the LED
+                  stay off for one second, then stay at max brightness for two
+                  seconds.
+
 
 - led-max-microamp : Maximum LED supply current in microamperes. This property
                      can be made mandatory for the board configurations
-- 
2.7.4

[PATCH v3 2/5] leds: Add helper for getting default pattern from Device Tree

[Krzysztof Kozlowski]

Multiple LED triggers might need to access default pattern so add a
helper for that.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---

New patch in the series

---
 drivers/leds/led-class.c | 25 +++++++++++++++++++++++++
 include/linux/leds.h     |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e3487b373..44b95e6480f6 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -17,6 +17,7 @@
 #include <linux/leds.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
@@ -244,6 +245,30 @@ static int led_classdev_next_name(const char *init_name, char *name,
 }
 
 /**
+ * led_classdev_get_default_pattern - return default pattern
+ *
+ * @led_cdev: the led_classdev structure for this device
+ *
+ * Return: Null terminated string with default pattern from DeviceTree or NULL
+ */
+const char *led_classdev_get_default_pattern(struct led_classdev *led_cdev)
+{
+	struct device_node *np = dev_of_node(led_cdev->dev);
+	const char *pattern;
+
+	if (!np)
+		return NULL;
+
+	if (of_property_read_string(np, "led-pattern", &pattern))
+		return NULL;
+
+	if (!strlen(pattern))
+		return NULL;
+
+	return pattern;
+}
+
+/**
  * of_led_classdev_register - register a new object of led_classdev class.
  *
  * @parent: parent of LED device
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 5263f87e1d2c..9da2bfa183ea 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -129,6 +129,9 @@ struct led_classdev {
 	struct mutex		led_access;
 };
 
+extern const char *
+led_classdev_get_default_pattern(struct led_classdev *led_cdev);
+
 extern int of_led_classdev_register(struct device *parent,
 				    struct device_node *np,
 				    struct led_classdev *led_cdev);
-- 
2.7.4

[PATCH v3 3/5] leds: trigger: pattern: Add pattern initialization from Device Tree

[Krzysztof Kozlowski]

Allow initialization of pattern used in pattern trigger from Device Tree
property.

This is especially useful for embedded systems where the pattern trigger
would be used to indicate the process of boot status in a nice,
user-friendly blinking way.  This initialization pattern will be used
till user-space is brought up and sets its own pattern, indicating the
boot status is for example finished.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/leds/trigger/ledtrig-pattern.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
index 1870cf87afe1..5be7bbe13a2c 100644
--- a/drivers/leds/trigger/ledtrig-pattern.c
+++ b/drivers/leds/trigger/ledtrig-pattern.c
@@ -331,6 +331,22 @@ static const struct attribute_group *pattern_trig_groups[] = {
 	NULL,
 };
 
+static void pattern_init(struct led_classdev *led_cdev)
+{
+	const char *pattern;
+	int err;
+
+	pattern = led_classdev_get_default_pattern(led_cdev);
+	if (!pattern)
+		return;
+
+	err = pattern_trig_store_patterns(led_cdev, pattern, strlen(pattern),
+					  false);
+	if (err)
+		dev_warn(led_cdev->dev,
+			 "Pattern initialization failed with error %d\n", err);
+}
+
 static int pattern_trig_activate(struct led_classdev *led_cdev)
 {
 	struct pattern_trig_data *data;
@@ -354,6 +370,15 @@ static int pattern_trig_activate(struct led_classdev *led_cdev)
 	timer_setup(&data->timer, pattern_trig_timer_function, 0);
 	led_cdev->activated = true;
 
+	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
+		pattern_init(led_cdev);
+		/*
+		 * Mark as initialized even on pattern_init() error because
+		 * any consecutive call to it would produce the same error.
+		 */
+		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
+	}
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH v3 4/5] leds: trigger: oneshot: Add initialization from Device Tree

[Krzysztof Kozlowski]

Allow initialization of delays used in oneshot trigger from Device
Tree property.

This is especially useful for embedded systems where the trigger might
be used early, before bringing up user-space.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---

New patch in the series
---
 drivers/leds/trigger/ledtrig-oneshot.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-oneshot.c b/drivers/leds/trigger/ledtrig-oneshot.c
index 95c9be4b6e7e..4e73a1c17a40 100644
--- a/drivers/leds/trigger/ledtrig-oneshot.c
+++ b/drivers/leds/trigger/ledtrig-oneshot.c
@@ -130,6 +130,27 @@ static struct attribute *oneshot_trig_attrs[] = {
 };
 ATTRIBUTE_GROUPS(oneshot_trig);
 
+static void pattern_init(struct led_classdev *led_cdev)
+{
+	const char *pattern;
+	unsigned long delay_on, delay_off;
+
+	pattern = led_classdev_get_default_pattern(led_cdev);
+	if (!pattern) {
+		led_cdev->blink_delay_on = DEFAULT_DELAY;
+		led_cdev->blink_delay_off = DEFAULT_DELAY;
+		return;
+	}
+
+	if (sscanf(pattern, "%lu %lu", &delay_on, &delay_off) == 2) {
+		led_cdev->blink_delay_on = delay_on;
+		led_cdev->blink_delay_off = delay_off;
+	} else {
+		dev_warn(led_cdev->dev,
+			 "Invalid value for delays pattern\n");
+	}
+}
+
 static int oneshot_trig_activate(struct led_classdev *led_cdev)
 {
 	struct oneshot_trig_data *oneshot_data;
@@ -140,8 +161,14 @@ static int oneshot_trig_activate(struct led_classdev *led_cdev)
 
 	led_set_trigger_data(led_cdev, oneshot_data);
 
-	led_cdev->blink_delay_on = DEFAULT_DELAY;
-	led_cdev->blink_delay_off = DEFAULT_DELAY;
+	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
+		pattern_init(led_cdev);
+		/*
+		 * Mark as initialized even on pattern_init() error because
+		 * any consecutive call to it would produce the same error.
+		 */
+		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
+	}
 
 	return 0;
 }
-- 
2.7.4

[PATCH v3 5/5] leds: trigger: timer: Add initialization from Device Tree

[Krzysztof Kozlowski]

Allow initialization of delays used in timer trigger from Device
Tree property.

This is especially useful for embedded systems where the trigger might
be used early, before bringing up user-space.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---

New patch in the series
---
 drivers/leds/trigger/ledtrig-timer.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
index 7c14983781ee..85a4b9fc3de0 100644
--- a/drivers/leds/trigger/ledtrig-timer.c
+++ b/drivers/leds/trigger/ledtrig-timer.c
@@ -77,8 +77,36 @@ static struct attribute *timer_trig_attrs[] = {
 };
 ATTRIBUTE_GROUPS(timer_trig);
 
+static void pattern_init(struct led_classdev *led_cdev)
+{
+	const char *pattern;
+	unsigned long delay_on, delay_off;
+
+	pattern = led_classdev_get_default_pattern(led_cdev);
+	if (!pattern)
+		return;
+
+	if (sscanf(pattern, "%lu %lu", &delay_on, &delay_off) == 2) {
+		led_blink_set(led_cdev, &delay_on, &delay_off);
+		led_cdev->blink_delay_on = delay_on;
+		led_cdev->blink_delay_off = delay_off;
+	} else {
+		dev_warn(led_cdev->dev,
+			 "Invalid value for delays pattern\n");
+	}
+}
+
 static int timer_trig_activate(struct led_classdev *led_cdev)
 {
+	if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
+		pattern_init(led_cdev);
+		/*
+		 * Mark as initialized even on pattern_init() error because
+		 * any consecutive call to it would produce the same error.
+		 */
+		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
+	}
+
 	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
 		      &led_cdev->blink_delay_off);
 
-- 
2.7.4

Re: [PATCH v3 2/5] leds: Add helper for getting default pattern from Device Tree

[Pavel Machek]

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

On Wed 2018-12-12 12:16:43, Krzysztof Kozlowski wrote:
> Multiple LED triggers might need to access default pattern so add a
> helper for that.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Acked-by: Pavel Machek <pavel@ucw.cz>

> 
> New patch in the series
> 
> ---
>  drivers/leds/led-class.c | 25 +++++++++++++++++++++++++
>  include/linux/leds.h     |  3 +++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3c7e3487b373..44b95e6480f6 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -17,6 +17,7 @@
>  #include <linux/leds.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/timer.h>
> @@ -244,6 +245,30 @@ static int led_classdev_next_name(const char *init_name, char *name,
>  }
>  
>  /**
> + * led_classdev_get_default_pattern - return default pattern
> + *
> + * @led_cdev: the led_classdev structure for this device
> + *
> + * Return: Null terminated string with default pattern from DeviceTree or NULL
> + */

Is it valid kerneldoc? Note that core still owns the string so that
caller is not expected to free it?

> +const char *led_classdev_get_default_pattern(struct led_classdev *led_cdev)
> +{
> +	struct device_node *np = dev_of_node(led_cdev->dev);
> +	const char *pattern;
> +
> +	if (!np)
> +		return NULL;
> +
> +	if (of_property_read_string(np, "led-pattern", &pattern))
> +		return NULL;
> +
> +	if (!strlen(pattern))
> +		return NULL;
> +
> +	return pattern;
> +}
> +
> +/**
>   * of_led_classdev_register - register a new object of led_classdev class.
>   *
>   * @parent: parent of LED device
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 5263f87e1d2c..9da2bfa183ea 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -129,6 +129,9 @@ struct led_classdev {
>  	struct mutex		led_access;
>  };
>  
> +extern const char *
> +led_classdev_get_default_pattern(struct led_classdev *led_cdev);
> +
>  extern int of_led_classdev_register(struct device *parent,
>  				    struct device_node *np,
>  				    struct led_classdev *led_cdev);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH v3 1/5] dt-bindings: leds: Add pattern initialization from Device Tree

[Pavel Machek]

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

On Wed 2018-12-12 12:16:42, Krzysztof Kozlowski wrote:
> Document new linux,trigger-pattern property for initialization of LED
> pattern trigger.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index aa1399814a2a..3daccd4ea8a3 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -37,6 +37,42 @@ Optional properties for child nodes:
>       "ide-disk" - LED indicates IDE disk activity (deprecated),
>                    in new implementations use "disk-activity"
>       "timer" - LED flashes at a fixed, configurable rate
> +     "pattern" - LED alters the brightness for the specified duration with one
> +                 software timer (requires "led-pattern" property)
> +
> +- led-pattern : String with default pattern for certain triggers. Each trigger
> +                may parse this string differently:
> +                - one-shot : two numbers specifying delay on and delay off,
> +                - timer : two numbers specifying delay on and delay
off,

Needs specifying that numbers are in miliseconds, at the very least.


> +                - pattern : The pattern is given by a series of tuples, of
> +                  brightness and duration (ms).  The LED is expected to traverse
> +                  the series and each brightness value for the specified
> +                  duration.  Duration of 0 means brightness should immediately
> +                  change to new value.
> +
> +                  1. For gradual dimming, the dimming interval now is set as 50
> +                  milliseconds. So the tuple with duration less than dimming
> +                  interval (50ms) is treated as a step change of brightness,
> +                  i.e. the subsequent brightness will be applied without adding
> +                  intervening dimming intervals.
> +
> +                  The gradual dimming format of the software pattern values should be:
> +                  "brightness_1 duration_1 brightness_2 duration_2 brightness_3
> +                  duration_3 ...".
> +                  For example "0 1000 255 2000" will make the LED go gradually
> +                  from zero-intensity to max (255) intensity in 1000
> +                  milliseconds, then back to zero intensity in 2000
> +                  milliseconds.
> +
> +                  2. To make the LED go instantly from one brightness value to
> +                  another, pattern should use zero-time lengths (the brightness
> +                  must be same as the previous tuple's). So the format should be:
> +                  "brightness_1 duration_1 brightness_1 0 brightness_2
> +                  duration_2 brightness_2 0 ...".
> +                  For example "0 1000 0 0 255 2000 255 0" will make the LED
> +                  stay off for one second, then stay at max brightness for two
> +                  seconds.
> +

No. Duplicated code is bad, and so is duplicated documentation. Maybe
the documentation should be in device tree part, but lets put it into
separate file, and reference it from the sysfs docs.

50msec for step change is really a implementation detail of Linux. May
be worth documenting for sysfs, but not for device tree
description. People should always specify 0 if they want step change.

Actually, no, I don't think this is quite suitable.

These are arrays of integers. They probably should not be specified as
strings in dts...?

(Plus, of course, timer and one-shot are subset of pattern, all we
need is to specify number of repeats...)

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH v3 2/5] leds: Add helper for getting default pattern from Device Tree

[Krzysztof Kozlowski]

On Wed, 12 Dec 2018 at 13:22, Pavel Machek <pavel@ucw.cz> wrote:
>
> On Wed 2018-12-12 12:16:43, Krzysztof Kozlowski wrote:
> > Multiple LED triggers might need to access default pattern so add a
> > helper for that.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
>
> >
> > New patch in the series
> >
> > ---
> >  drivers/leds/led-class.c | 25 +++++++++++++++++++++++++
> >  include/linux/leds.h     |  3 +++
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 3c7e3487b373..44b95e6480f6 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/leds.h>
> >  #include <linux/list.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/timer.h>
> > @@ -244,6 +245,30 @@ static int led_classdev_next_name(const char *init_name, char *name,
> >  }
> >
> >  /**
> > + * led_classdev_get_default_pattern - return default pattern
> > + *
> > + * @led_cdev: the led_classdev structure for this device
> > + *
> > + * Return: Null terminated string with default pattern from DeviceTree or NULL
> > + */
>
> Is it valid kerneldoc?

Yes, maybe kind a short but in general it follows style.
https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

> Note that core still owns the string so that
> caller is not expected to free it?

That should be obvious by returning "const char *". The
of_property_string() does not mention such case neither... I  can
document it but really "const char *" is self-documenting code.

> > +const char *led_classdev_get_default_pattern(struct led_classdev *led_cdev)
> > +{
> > +     struct device_node *np = dev_of_node(led_cdev->dev);
> > +     const char *pattern;
> > +
> > +     if (!np)
> > +             return NULL;
> > +
> > +     if (of_property_read_string(np, "led-pattern", &pattern))
> > +             return NULL;
> > +
> > +     if (!strlen(pattern))
> > +             return NULL;
> > +
> > +     return pattern;
> > +}

I missed here EXPORT_SYMBOL_GPL.

Best regards,
Krzysztof

> > +
> > +/**
> >   * of_led_classdev_register - register a new object of led_classdev class.
> >   *
> >   * @parent: parent of LED device
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index 5263f87e1d2c..9da2bfa183ea 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -129,6 +129,9 @@ struct led_classdev {
> >       struct mutex            led_access;
> >  };
> >
> > +extern const char *
> > +led_classdev_get_default_pattern(struct led_classdev *led_cdev);
> > +
> >  extern int of_led_classdev_register(struct device *parent,
> >                                   struct device_node *np,
> >                                   struct led_classdev *led_cdev);
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v3 1/5] dt-bindings: leds: Add pattern initialization from Device Tree

[Krzysztof Kozlowski]

On Wed, 12 Dec 2018 at 13:32, Pavel Machek <pavel@ucw.cz> wrote:
>
> On Wed 2018-12-12 12:16:42, Krzysztof Kozlowski wrote:
> > Document new linux,trigger-pattern property for initialization of LED
> > pattern trigger.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> > index aa1399814a2a..3daccd4ea8a3 100644
> > --- a/Documentation/devicetree/bindings/leds/common.txt
> > +++ b/Documentation/devicetree/bindings/leds/common.txt
> > @@ -37,6 +37,42 @@ Optional properties for child nodes:
> >       "ide-disk" - LED indicates IDE disk activity (deprecated),
> >                    in new implementations use "disk-activity"
> >       "timer" - LED flashes at a fixed, configurable rate
> > +     "pattern" - LED alters the brightness for the specified duration with one
> > +                 software timer (requires "led-pattern" property)
> > +
> > +- led-pattern : String with default pattern for certain triggers. Each trigger
> > +                may parse this string differently:
> > +                - one-shot : two numbers specifying delay on and delay off,
> > +                - timer : two numbers specifying delay on and delay
> off,
>
> Needs specifying that numbers are in miliseconds, at the very least.

Sure.

> > +                - pattern : The pattern is given by a series of tuples, of
> > +                  brightness and duration (ms).  The LED is expected to traverse
> > +                  the series and each brightness value for the specified
> > +                  duration.  Duration of 0 means brightness should immediately
> > +                  change to new value.
> > +
> > +                  1. For gradual dimming, the dimming interval now is set as 50
> > +                  milliseconds. So the tuple with duration less than dimming
> > +                  interval (50ms) is treated as a step change of brightness,
> > +                  i.e. the subsequent brightness will be applied without adding
> > +                  intervening dimming intervals.
> > +
> > +                  The gradual dimming format of the software pattern values should be:
> > +                  "brightness_1 duration_1 brightness_2 duration_2 brightness_3
> > +                  duration_3 ...".
> > +                  For example "0 1000 255 2000" will make the LED go gradually
> > +                  from zero-intensity to max (255) intensity in 1000
> > +                  milliseconds, then back to zero intensity in 2000
> > +                  milliseconds.
> > +
> > +                  2. To make the LED go instantly from one brightness value to
> > +                  another, pattern should use zero-time lengths (the brightness
> > +                  must be same as the previous tuple's). So the format should be:
> > +                  "brightness_1 duration_1 brightness_1 0 brightness_2
> > +                  duration_2 brightness_2 0 ...".
> > +                  For example "0 1000 0 0 255 2000 255 0" will make the LED
> > +                  stay off for one second, then stay at max brightness for two
> > +                  seconds.
> > +
>
> No. Duplicated code is bad, and so is duplicated documentation. Maybe
> the documentation should be in device tree part, but lets put it into
> separate file, and reference it from the sysfs docs.

I guess this would satisfy both needs - no external references from
bindings and no duplication.

> 50msec for step change is really a implementation detail of Linux. May
> be worth documenting for sysfs, but not for device tree
> description. People should always specify 0 if they want step change.
>
> Actually, no, I don't think this is quite suitable.
>
> These are arrays of integers. They probably should not be specified as
> strings in dts...?

Makes sense although this would limit the possible formats for other
triggers and LED drivers. Probably there are not many of such use
cases (pattern different than integer numbers) so maybe there is no
point to worry about it too much.

Best regards,
Krzysztof

>
> (Plus, of course, timer and one-shot are subset of pattern, all we
> need is to specify number of repeats...)
>
>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v3 1/5] dt-bindings: leds: Add pattern initialization from Device Tree

[Rob Herring]

On Wed, Dec 12, 2018 at 12:16:42PM +0100, Krzysztof Kozlowski wrote:
> Document new linux,trigger-pattern property for initialization of LED
> pattern trigger.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index aa1399814a2a..3daccd4ea8a3 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -37,6 +37,42 @@ Optional properties for child nodes:
>       "ide-disk" - LED indicates IDE disk activity (deprecated),
>                    in new implementations use "disk-activity"
>       "timer" - LED flashes at a fixed, configurable rate
> +     "pattern" - LED alters the brightness for the specified duration with one
> +                 software timer (requires "led-pattern" property)
> +
> +- led-pattern : String with default pattern for certain triggers. Each trigger
> +                may parse this string differently:
> +                - one-shot : two numbers specifying delay on and delay off,
> +                - timer : two numbers specifying delay on and delay off,

I misunderstood that these triggers applied to this. Is there any point 
to supporting these modes? Aren't they just a subset or simplification 
of a pattern?

Can we control the timer frequency from DT? The pattern could address 
that limitation.

> +                - pattern : The pattern is given by a series of tuples, of

Don't you need one-shot vs. repeat modes for patterns too?

I could imagine you'd want a pattern for any trigger. 

> +                  brightness and duration (ms).  The LED is expected to traverse
> +                  the series and each brightness value for the specified
> +                  duration.  Duration of 0 means brightness should immediately
> +                  change to new value.
> +
> +                  1. For gradual dimming, the dimming interval now is set as 50
> +                  milliseconds. So the tuple with duration less than dimming
> +                  interval (50ms) is treated as a step change of brightness,
> +                  i.e. the subsequent brightness will be applied without adding
> +                  intervening dimming intervals.
> +
> +                  The gradual dimming format of the software pattern values should be:
> +                  "brightness_1 duration_1 brightness_2 duration_2 brightness_3
> +                  duration_3 ...".
> +                  For example "0 1000 255 2000" will make the LED go gradually
> +                  from zero-intensity to max (255) intensity in 1000
> +                  milliseconds, then back to zero intensity in 2000
> +                  milliseconds.
> +
> +                  2. To make the LED go instantly from one brightness value to
> +                  another, pattern should use zero-time lengths (the brightness
> +                  must be same as the previous tuple's). So the format should be:
> +                  "brightness_1 duration_1 brightness_1 0 brightness_2
> +                  duration_2 brightness_2 0 ...".
> +                  For example "0 1000 0 0 255 2000 255 0" will make the LED
> +                  stay off for one second, then stay at max brightness for two
> +                  seconds.
> +
>  
>  - led-max-microamp : Maximum LED supply current in microamperes. This property
>                       can be made mandatory for the board configurations
> -- 
> 2.7.4
> 

Re: [PATCH v3 1/5] dt-bindings: leds: Add pattern initialization from Device Tree

[Krzysztof Kozlowski]

On Mon, 17 Dec 2018 at 23:40, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Dec 12, 2018 at 12:16:42PM +0100, Krzysztof Kozlowski wrote:
> > Document new linux,trigger-pattern property for initialization of LED
> > pattern trigger.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> > index aa1399814a2a..3daccd4ea8a3 100644
> > --- a/Documentation/devicetree/bindings/leds/common.txt
> > +++ b/Documentation/devicetree/bindings/leds/common.txt
> > @@ -37,6 +37,42 @@ Optional properties for child nodes:
> >       "ide-disk" - LED indicates IDE disk activity (deprecated),
> >                    in new implementations use "disk-activity"
> >       "timer" - LED flashes at a fixed, configurable rate
> > +     "pattern" - LED alters the brightness for the specified duration with one
> > +                 software timer (requires "led-pattern" property)
> > +
> > +- led-pattern : String with default pattern for certain triggers. Each trigger
> > +                may parse this string differently:
> > +                - one-shot : two numbers specifying delay on and delay off,
> > +                - timer : two numbers specifying delay on and delay off,
>
> I misunderstood that these triggers applied to this. Is there any point
> to supporting these modes?

I am not sure whether I explained this clearly enough, so let me be
more specific. The "led-pattern" is a string with pattern which will
be interpreted differently, based on LED trigger. One-shot and timer
triggers expect two numbers. Pattern trigger expects full pattern.

> Aren't they just a subset or simplification
> of a pattern?

Yes, they are.

>
> Can we control the timer frequency from DT? The pattern could address
> that limitation.

No, currently we cannot. This patch with pattern solves that issue.

>
> > +                - pattern : The pattern is given by a series of tuples, of
>
> Don't you need one-shot vs. repeat modes for patterns too?

Repeat mode is defined by trigger. One-shot is one time. Timer and
patter triggers are repeating.

>
> I could imagine you'd want a pattern for any trigger.

In general I think that only these three triggers require such
initialization through pattern. Other triggers - like heartbeat or
activity - have they own bindings (or do not need any).

Following Pavel's suggestion, I could convert it to array of integers
which still should be suitable for these and any future triggers
operating on numbers.

Best regards,
Krzysztof