[PATCH 1/4] leds: pwm: Simplify with resource-managed devm_led_classdev_register()

[PATCH 1/4] leds: pwm: Simplify with resource-managed devm_led_classdev_register()

[Krzysztof Kozlowski]

Simplify the exit path with resource-managed version of registering LED
class device.  The code should be functionally the same, except that on
device removal the led_pwm_priv->num_leds is not decremented to zero
(which should not have any effect as device is going away).

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/leds/leds-pwm.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 5d3faae51d59..fcb3e87a9887 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -74,12 +74,6 @@ static inline size_t sizeof_pwm_leds_priv(int num_leds)
 		      (sizeof(struct led_pwm_data) * num_leds);
 }
 
-static void led_pwm_cleanup(struct led_pwm_priv *priv)
-{
-	while (priv->num_leds--)
-		led_classdev_unregister(&priv->leds[priv->num_leds].cdev);
-}
-
 static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 		       struct led_pwm *led, struct device_node *child)
 {
@@ -120,7 +114,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	if (!led_data->period && (led->pwm_period_ns > 0))
 		led_data->period = led->pwm_period_ns;
 
-	ret = led_classdev_register(dev, &led_data->cdev);
+	ret = devm_led_classdev_register(dev, &led_data->cdev);
 	if (ret == 0) {
 		priv->num_leds++;
 		led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
@@ -191,25 +185,14 @@ static int led_pwm_probe(struct platform_device *pdev)
 		ret = led_pwm_create_of(&pdev->dev, priv);
 	}
 
-	if (ret) {
-		led_pwm_cleanup(priv);
+	if (ret)
 		return ret;
-	}
 
 	platform_set_drvdata(pdev, priv);
 
 	return 0;
 }
 
-static int led_pwm_remove(struct platform_device *pdev)
-{
-	struct led_pwm_priv *priv = platform_get_drvdata(pdev);
-
-	led_pwm_cleanup(priv);
-
-	return 0;
-}
-
 static const struct of_device_id of_pwm_leds_match[] = {
 	{ .compatible = "pwm-leds", },
 	{},
@@ -218,7 +201,6 @@ MODULE_DEVICE_TABLE(of, of_pwm_leds_match);
 
 static struct platform_driver led_pwm_driver = {
 	.probe		= led_pwm_probe,
-	.remove		= led_pwm_remove,
 	.driver		= {
 		.name	= "leds_pwm",
 		.of_match_table = of_pwm_leds_match,
-- 
2.7.4

[PATCH 2/4] leds: pwm: Use OF variant of LED registering function

[Krzysztof Kozlowski]

The PWM leds can be instantiated from Device Tree so pass the
respective device node to LED core.  This provides the LED system with
proper device node and exposes it through uevent.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/leds/leds-pwm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index fcb3e87a9887..af08bcdc4fd8 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -114,7 +114,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	if (!led_data->period && (led->pwm_period_ns > 0))
 		led_data->period = led->pwm_period_ns;
 
-	ret = devm_led_classdev_register(dev, &led_data->cdev);
+	ret = devm_of_led_classdev_register(dev, child, &led_data->cdev);
 	if (ret == 0) {
 		priv->num_leds++;
 		led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
-- 
2.7.4

[PATCH 3/4] 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

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

diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
index 1870cf87afe1..96309d3bc43c 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,21 @@ 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;
+
+	if (!np)
+		return;
+
+	if (!of_property_read_string(np, "linux,trigger-pattern", &pattern)) {
+		if (strlen(pattern))
+			pattern_trig_store_patterns(led_cdev, pattern,
+						    strlen(pattern), false);
+	}
+}
+
 static int pattern_trig_activate(struct led_classdev *led_cdev)
 {
 	struct pattern_trig_data *data;
@@ -354,6 +370,8 @@ static int pattern_trig_activate(struct led_classdev *led_cdev)
 	timer_setup(&data->timer, pattern_trig_timer_function, 0);
 	led_cdev->activated = true;
 
+	pattern_init(led_cdev);
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 4/4] leds: trigger: Add pattern initialization from Device Tree

[Jacek Anaszewski]

Hi Krzysztof,

Thank you for the patch set.

Applied 1/4 and 2/4.

I'll hold on merging 3/4 until we sort out the issues
I have with this one. Please refer to my comment below.

On 12/7/18 1:32 PM, Krzysztof Kozlowski wrote:
> 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 | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
> index 1870cf87afe1..96309d3bc43c 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,21 @@ 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;
> +
> +	if (!np)
> +		return;
> +
> +	if (!of_property_read_string(np, "linux,trigger-pattern", &pattern)) {
> +		if (strlen(pattern))
> +			pattern_trig_store_patterns(led_cdev, pattern,
> +						    strlen(pattern), false);
> +	}
> +}
> +
>   static int pattern_trig_activate(struct led_classdev *led_cdev)
>   {
>   	struct pattern_trig_data *data;
> @@ -354,6 +370,8 @@ static int pattern_trig_activate(struct led_classdev *led_cdev)
>   	timer_setup(&data->timer, pattern_trig_timer_function, 0);
>   	led_cdev->activated = true;
>   
> +	pattern_init(led_cdev);

It means that the pattern defined in DT would be applied
whenever the pattern trigger is set for a LED class device,
whereas it should happen only on LED class device initialization,
if linux,default-trigger is set to "pattern".

What we would need for making that work is a generic support for
parsing trigger specific initialization data in the
of_led_classdev_register().

> +
>   	return 0;
>   }
>   
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 4/4] leds: trigger: Add pattern initialization from Device Tree

[Jacek Anaszewski]

On 12/8/18 7:44 PM, Jacek Anaszewski wrote:
> Hi Krzysztof,
> 
> Thank you for the patch set.
> 
> Applied 1/4 and 2/4.
> 
> I'll hold on merging 3/4 until we sort out the issues
> I have with this one. Please refer to my comment below.
> 
> On 12/7/18 1:32 PM, Krzysztof Kozlowski wrote:
>> 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 | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/leds/trigger/ledtrig-pattern.c 
>> b/drivers/leds/trigger/ledtrig-pattern.c
>> index 1870cf87afe1..96309d3bc43c 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,21 @@ 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;
>> +
>> +    if (!np)
>> +        return;
>> +
>> +    if (!of_property_read_string(np, "linux,trigger-pattern", 
>> &pattern)) {
>> +        if (strlen(pattern))
>> +            pattern_trig_store_patterns(led_cdev, pattern,
>> +                            strlen(pattern), false);
>> +    }
>> +}
>> +
>>   static int pattern_trig_activate(struct led_classdev *led_cdev)
>>   {
>>       struct pattern_trig_data *data;
>> @@ -354,6 +370,8 @@ static int pattern_trig_activate(struct 
>> led_classdev *led_cdev)
>>       timer_setup(&data->timer, pattern_trig_timer_function, 0);
>>       led_cdev->activated = true;
>> +    pattern_init(led_cdev);

With my recent patches it would suffice to replace above line with:

if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
	pattern_init(led_cdev);
	led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
}

> It means that the pattern defined in DT would be applied
> whenever the pattern trigger is set for a LED class device,
> whereas it should happen only on LED class device initialization,
> if linux,default-trigger is set to "pattern".
> 
> What we would need for making that work is a generic support for
> parsing trigger specific initialization data in the
> of_led_classdev_register().
> 
>> +
>>       return 0;
>>   }
>>
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 4/4] leds: trigger: Add pattern initialization from Device Tree

[Krzysztof Kozlowski]

On Sun, 9 Dec 2018 at 19:55, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
>
> On 12/8/18 7:44 PM, Jacek Anaszewski wrote:
> > Hi Krzysztof,
> >
> > Thank you for the patch set.
> >
> > Applied 1/4 and 2/4.
> >
> > I'll hold on merging 3/4 until we sort out the issues
> > I have with this one. Please refer to my comment below.
> >
> > On 12/7/18 1:32 PM, Krzysztof Kozlowski wrote:
> >> 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 | 18 ++++++++++++++++++
> >>   1 file changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/leds/trigger/ledtrig-pattern.c
> >> b/drivers/leds/trigger/ledtrig-pattern.c
> >> index 1870cf87afe1..96309d3bc43c 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,21 @@ 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;
> >> +
> >> +    if (!np)
> >> +        return;
> >> +
> >> +    if (!of_property_read_string(np, "linux,trigger-pattern",
> >> &pattern)) {
> >> +        if (strlen(pattern))
> >> +            pattern_trig_store_patterns(led_cdev, pattern,
> >> +                            strlen(pattern), false);
> >> +    }
> >> +}
> >> +
> >>   static int pattern_trig_activate(struct led_classdev *led_cdev)
> >>   {
> >>       struct pattern_trig_data *data;
> >> @@ -354,6 +370,8 @@ static int pattern_trig_activate(struct
> >> led_classdev *led_cdev)
> >>       timer_setup(&data->timer, pattern_trig_timer_function, 0);
> >>       led_cdev->activated = true;
> >> +    pattern_init(led_cdev);
>
> With my recent patches it would suffice to replace above line with:
>
> if (led_cdev->flags & LED_INIT_DEFAULT_TRIGGER) {
>         pattern_init(led_cdev);
>         led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
> }

Right, I see the problem. I'll rebase on top of it.

Best regards,
Krzysztof

Re: [PATCH 4/4] leds: trigger: Add pattern initialization from Device Tree

[Pavel Machek]

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

On Fri 2018-12-07 13:32:54, Krzysztof Kozlowski wrote:
> 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 | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
> index 1870cf87afe1..96309d3bc43c 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,21 @@ 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;
> +
> +	if (!np)
> +		return;
> +
> +	if (!of_property_read_string(np, "linux,trigger-pattern", &pattern)) {
> +		if (strlen(pattern))
> +			pattern_trig_store_patterns(led_cdev, pattern,
> +						    strlen(pattern), false);
> +	}
> +}

"  if (of_...())
      return;

   if (!strlen())
       return"

Check return value of pattern_trig_store_patterns().
									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 4/4] leds: trigger: Add pattern initialization from Device Tree

[Krzysztof Kozlowski]

On Sun, 9 Dec 2018 at 09:17, Pavel Machek <pavel@ucw.cz> wrote:
>
> On Fri 2018-12-07 13:32:54, Krzysztof Kozlowski wrote:
> > 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 | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
> > index 1870cf87afe1..96309d3bc43c 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,21 @@ 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;
> > +
> > +     if (!np)
> > +             return;
> > +
> > +     if (!of_property_read_string(np, "linux,trigger-pattern", &pattern)) {
> > +             if (strlen(pattern))
> > +                     pattern_trig_store_patterns(led_cdev, pattern,
> > +                                                 strlen(pattern), false);
> > +     }
> > +}
>
> "  if (of_...())
>       return;
>
>    if (!strlen())
>        return"

Sure.

>
> Check return value of pattern_trig_store_patterns().

i can add printing of error in such case but I am not sure whether it
is worth to fail entire pattern_trig_activate().

Best regards,
Krzysztof