[4/4] leds: trigger: Add pattern initialization from Device Tree
diff mbox series

Message ID 1544185974-5932-4-git-send-email-krzk@kernel.org
State New
Headers show
Series
  • [1/4] leds: pwm: Simplify with resource-managed devm_led_classdev_register()
Related show

Commit Message

Krzysztof Kozlowski Dec. 7, 2018, 12:32 p.m. UTC
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(+)

Comments

Jacek Anaszewski Dec. 8, 2018, 6:44 p.m. UTC | #1
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;
>   }
>   
>
Pavel Machek Dec. 9, 2018, 8:17 a.m. UTC | #2
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
Jacek Anaszewski Dec. 9, 2018, 6:55 p.m. UTC | #3
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;
>>   }
>>
>
Krzysztof Kozlowski Dec. 10, 2018, 8:34 a.m. UTC | #4
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
Krzysztof Kozlowski Dec. 10, 2018, 8:35 a.m. UTC | #5
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

Patch
diff mbox series

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;
 }