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

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

[Krzysztof Kozlowski]

Hi,

I rebased on top of Jacek's patches and, as I could not find them on git kernel
tree, I included them here. Also with small fixup.

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


Jacek Anaszewski (2):
  led: triggers: Break the for loop after default trigger is found
  led: triggers: Add LED_INIT_DEFAULT_TRIGGER flag

Krzysztof Kozlowski (3):
  led: triggers: Initialize LED_INIT_DEFAULT_TRIGGER if trigger is
    brought after class
  dt-bindings: leds: Add pattern initialization from Device Tree
  leds: trigger: Add pattern initialization from Device Tree

 Documentation/devicetree/bindings/leds/common.txt |  7 +++++
 drivers/leds/led-triggers.c                       | 10 +++++--
 drivers/leds/trigger/ledtrig-pattern.c            | 32 +++++++++++++++++++++++
 include/linux/leds.h                              |  1 +
 4 files changed, 48 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] led: triggers: Break the for loop after default trigger is found

[Krzysztof Kozlowski]

From: Jacek Anaszewski <jacek.anaszewski@gmail.com>

It is of no avail to continue iterating through registered
triggers in the led_trigger_set_default() after the trigger to set
has been found. Add "break" statement to fix this omission.

Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/leds/led-triggers.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 17d73db1456e..52b12e601ebe 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -200,8 +200,10 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
 	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))
+		if (!strcmp(led_cdev->default_trigger, trig->name)) {
 			led_trigger_set(led_cdev, trig);
+			break;
+		}
 	}
 	up_write(&led_cdev->trigger_lock);
 	up_read(&triggers_list_lock);
-- 
2.7.4

[PATCH v2 2/5] led: triggers: Add LED_INIT_DEFAULT_TRIGGER flag

[Krzysztof Kozlowski]

From: Jacek Anaszewski <jacek.anaszewski@gmail.com>

Add the flag LED_INIT_DEFAULT_TRIGGER for indicating that trigger
being set is a default trigger for the LED class device, and
thus it should be initialized with settings provided in the fwnode.

Set the flag in the led_trigger_set_default(). It is expected to be
cleared in the activate() op of a trigger after trigger fwnode
initialization data is parsed and applied. This should happen only
once after LED class device registration, to allow leaving triggers
in the idle state on re-apply and let the users apply their own
settings without being interfered with the default ones.

Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/leds/led-triggers.c | 2 ++
 include/linux/leds.h        | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 52b12e601ebe..9421222ca7a0 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -201,10 +201,12 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
 	down_write(&led_cdev->trigger_lock);
 	list_for_each_entry(trig, &trigger_list, next_trig) {
 		if (!strcmp(led_cdev->default_trigger, trig->name)) {
+			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
 			led_trigger_set(led_cdev, trig);
 			break;
 		}
 	}
+
 	up_write(&led_cdev->trigger_lock);
 	up_read(&triggers_list_lock);
 }
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 580cbaef789a..5263f87e1d2c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -51,6 +51,7 @@ struct led_classdev {
 #define LED_PANIC_INDICATOR	BIT(20)
 #define LED_BRIGHT_HW_CHANGED	BIT(21)
 #define LED_RETAIN_AT_SHUTDOWN	BIT(22)
+#define LED_INIT_DEFAULT_TRIGGER BIT(23)
 
 	/* set_brightness_work / blink_timer flags, atomic, private. */
 	unsigned long		work_flags;
-- 
2.7.4

[PATCH v2 3/5] led: triggers: Initialize LED_INIT_DEFAULT_TRIGGER if trigger is brought after class

[Krzysztof Kozlowski]

Trigger driver can be initialized after the LED class device driver.  In
such case led_trigger_set_default() won't be called and flag
LED_INIT_DEFAULT_TRIGGER should be set from led_trigger_register().

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/leds/led-triggers.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 9421222ca7a0..c67b044ae0af 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -252,8 +252,10 @@ int led_trigger_register(struct led_trigger *trig)
 	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))
+			    !strcmp(led_cdev->default_trigger, trig->name)) {
+			led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
 			led_trigger_set(led_cdev, trig);
+		}
 		up_write(&led_cdev->trigger_lock);
 	}
 	up_read(&leds_list_lock);
-- 
2.7.4

[PATCH v2 4/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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index aa1399814a2a..4c65e854bb91 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -37,6 +37,13 @@ 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 (see
+                 Documentation/ABI/testing/sysfs-class-led-trigger-pattern)
+
+- linux,trigger-pattern : Default pattern for "pattern" trigger (see
+                          Documentation/ABI/testing/sysfs-class-led-trigger-pattern
+                          for description of format)
 
 - led-max-microamp : Maximum LED supply current in microamperes. This property
                      can be made mandatory for the board configurations
-- 
2.7.4

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

[Rob Herring]

On Mon, Dec 10, 2018 at 3:30 AM Krzysztof Kozlowski <krzk@kernel.org> 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index aa1399814a2a..4c65e854bb91 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -37,6 +37,13 @@ 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 (see
> +                 Documentation/ABI/testing/sysfs-class-led-trigger-pattern)
> +
> +- linux,trigger-pattern : Default pattern for "pattern" trigger (see
> +                          Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> +                          for description of format)

Binding documents should generally not refer to kernel docs and need
to stand on their own.

I don't think this needs to be linux specific. I'd just call it
'led-pattern'. I don't think this should be tied to the trigger types.
Isn't possible you would want to tie a pattern to different triggers.
IOW, when event X happens, run the pattern. And you could have
one-shot versus continuous.

Rob

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

[Krzysztof Kozlowski]

On Tue, 11 Dec 2018 at 18:23, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Dec 10, 2018 at 3:30 AM Krzysztof Kozlowski <krzk@kernel.org> 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 | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> > index aa1399814a2a..4c65e854bb91 100644
> > --- a/Documentation/devicetree/bindings/leds/common.txt
> > +++ b/Documentation/devicetree/bindings/leds/common.txt
> > @@ -37,6 +37,13 @@ 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 (see
> > +                 Documentation/ABI/testing/sysfs-class-led-trigger-pattern)
> > +
> > +- linux,trigger-pattern : Default pattern for "pattern" trigger (see
> > +                          Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> > +                          for description of format)
>
> Binding documents should generally not refer to kernel docs and need
> to stand on their own.

I'll copy then the description of pattern.

> I don't think this needs to be linux specific. I'd just call it
> 'led-pattern'. I don't think this should be tied to the trigger types.
> Isn't possible you would want to tie a pattern to different triggers.
> IOW, when event X happens, run the pattern. And you could have
> one-shot versus continuous.

In general pattern has trigger specific format. For pattern-trigger it
would look like "0 1000 255 1000". For timer trigger it would be just
a single number (period for blinks). Quite similar for the oneshot
trigger. I can change it to "led-pattern" (or maybe
"led-default-pattern" to match "linux,default-trigger" property?) and
document that the pattern itself is trigger specific

Best regards,
Krzysztof

[PATCH v2 5/5] leds: trigger: 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 | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
index 1870cf87afe1..1e7672eba2b8 100644
--- a/drivers/leds/trigger/ledtrig-pattern.c
+++ b/drivers/leds/trigger/ledtrig-pattern.c
@@ -11,6 +11,7 @@
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/timer.h>
 
@@ -331,6 +332,28 @@ static const struct attribute_group *pattern_trig_groups[] = {
 	NULL,
 };
 
+static void pattern_init(struct led_classdev *led_cdev)
+{
+	struct device_node *np = dev_of_node(led_cdev->dev);
+	const char *pattern;
+	int err;
+
+	if (!np)
+		return;
+
+	if (of_property_read_string(np, "linux,trigger-pattern", &pattern))
+		return;
+
+	if (!strlen(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 +377,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

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

[Jacek Anaszewski]

Hi Krzysztof,

Thank you for the update.

On 12/10/18 10:29 AM, Krzysztof Kozlowski wrote:
> Hi,
> 
> I rebased on top of Jacek's patches and, as I could not find them on git kernel
> tree, I included them here. Also with small fixup.
> 
> Changes since v1:
> 1. Rebase on Jacek's patches.

Made following amendments to 2/5:

- removed extra empty line
- fixed language in the commit message as proposed by Pavel

> 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)
Applied to the for-next branch of linux-leds.git, thanks.

Pavel - I'm still awaiting your acks, just wanted to have
the patch set in linux-next quicker, taking into account that
merge window is imminent.

Best regards,
Jacek Anaszewski

> Jacek Anaszewski (2):
>    led: triggers: Break the for loop after default trigger is found
>    led: triggers: Add LED_INIT_DEFAULT_TRIGGER flag
> 
> Krzysztof Kozlowski (3):
>    led: triggers: Initialize LED_INIT_DEFAULT_TRIGGER if trigger is
>      brought after class
>    dt-bindings: leds: Add pattern initialization from Device Tree
>    leds: trigger: Add pattern initialization from Device Tree
> 
>   Documentation/devicetree/bindings/leds/common.txt |  7 +++++
>   drivers/leds/led-triggers.c                       | 10 +++++--
>   drivers/leds/trigger/ledtrig-pattern.c            | 32 +++++++++++++++++++++++
>   include/linux/leds.h                              |  1 +
>   4 files changed, 48 insertions(+), 2 deletions(-)
> 

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

[Pavel Machek]

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

On Mon 2018-12-10 22:03:18, Jacek Anaszewski wrote:
> Hi Krzysztof,
> 
> Thank you for the update.
> 
> On 12/10/18 10:29 AM, Krzysztof Kozlowski wrote:
> >Hi,
> >
> >I rebased on top of Jacek's patches and, as I could not find them on git kernel
> >tree, I included them here. Also with small fixup.
> >
> >Changes since v1:
> >1. Rebase on Jacek's patches.
> 
> Made following amendments to 2/5:
> 
> - removed extra empty line
> - fixed language in the commit message as proposed by Pavel
> 
> >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)
> Applied to the for-next branch of linux-leds.git, thanks.
> 
> Pavel - I'm still awaiting your acks, just wanted to have
> the patch set in linux-next quicker, taking into account that
> merge window is imminent.

Ok, let me take a look. But this really should have Rob's acknowledge,
so...

								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 v2 0/5] leds: trigger: Add pattern initialization from Device Tree

[Jacek Anaszewski]

On 12/12/18 1:16 PM, Pavel Machek wrote:
> On Mon 2018-12-10 22:03:18, Jacek Anaszewski wrote:
>> Hi Krzysztof,
>>
>> Thank you for the update.
>>
>> On 12/10/18 10:29 AM, Krzysztof Kozlowski wrote:
>>> Hi,
>>>
>>> I rebased on top of Jacek's patches and, as I could not find them on git kernel
>>> tree, I included them here. Also with small fixup.
>>>
>>> Changes since v1:
>>> 1. Rebase on Jacek's patches.
>>
>> Made following amendments to 2/5:
>>
>> - removed extra empty line
>> - fixed language in the commit message as proposed by Pavel
>>
>>> 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)
>> Applied to the for-next branch of linux-leds.git, thanks.
>>
>> Pavel - I'm still awaiting your acks, just wanted to have
>> the patch set in linux-next quicker, taking into account that
>> merge window is imminent.
> 
> Ok, let me take a look. But this really should have Rob's acknowledge,
> so...

Dropped 4/5 and 5/5 as they are now superseded by v3, that appears
not to be the final too.

-- 
Best regards,
Jacek Anaszewski