Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel,
On 27 July 2018 at 16:36, Pavel Machek wrote:
> Hi!
>
>> > This should be a bit better. I attempted to compile it with your
>> > driver, but whether it works is an open question.
>>
>> Sorry for late reply. I've compiled and tested this version on my
>> platform, the hardware pattern can work with one small fix as below.
>>
>> if (led_cdev->pattern_set && !led_cdev->pattern_set(led_cdev,
>> data->steps, data->nsteps)) {
>> return;
>> }
>>
>> But I saw there are lots coding style issues and something can be
>> improved in this patch, so will you send out one clean patch (I will
>> help to test the hardware pattern support)? Or I can help to improve
>> this code and try to upstream it again?
>
> If you could do the upstreaming, that would be great.
OK, I will improve the code and test the software/hardware pattern,
and upstream it again with keeping the original authorization.
> I tried to get hardware accelerated LED to work on N900, but that
> hardware is rather complex, so it would take me some time...
--
Baolin Wang
Best Regards
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel,
On 27 July 2018 at 16:36, Pavel Machek wrote:
> Hi!
>
>> > This should be a bit better. I attempted to compile it with your
>> > driver, but whether it works is an open question.
>>
>> Sorry for late reply. I've compiled and tested this version on my
>> platform, the hardware pattern can work with one small fix as below.
>>
>> if (led_cdev->pattern_set && !led_cdev->pattern_set(led_cdev,
>> data->steps, data->nsteps)) {
>> return;
>> }
>>
>> But I saw there are lots coding style issues and something can be
>> improved in this patch, so will you send out one clean patch (I will
>> help to test the hardware pattern support)? Or I can help to improve
>> this code and try to upstream it again?
>
> If you could do the upstreaming, that would be great.
OK, I will improve the code and test the software/hardware pattern,
and upstream it again with keeping the original authorization.
> I tried to get hardware accelerated LED to work on N900, but that
> hardware is rather complex, so it would take me some time...
--
Baolin Wang
Best Regards
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> > This should be a bit better. I attempted to compile it with your
> > driver, but whether it works is an open question.
>
> Sorry for late reply. I've compiled and tested this version on my
> platform, the hardware pattern can work with one small fix as below.
>
> if (led_cdev->pattern_set && !led_cdev->pattern_set(led_cdev,
> data->steps, data->nsteps)) {
> return;
> }
>
> But I saw there are lots coding style issues and something can be
> improved in this patch, so will you send out one clean patch (I will
> help to test the hardware pattern support)? Or I can help to improve
> this code and try to upstream it again?
If you could do the upstreaming, that would be great.
I tried to get hardware accelerated LED to work on N900, but that
hardware is rather complex, so it would take me some time...
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> > This should be a bit better. I attempted to compile it with your
> > driver, but whether it works is an open question.
>
> Sorry for late reply. I've compiled and tested this version on my
> platform, the hardware pattern can work with one small fix as below.
>
> if (led_cdev->pattern_set && !led_cdev->pattern_set(led_cdev,
> data->steps, data->nsteps)) {
> return;
> }
>
> But I saw there are lots coding style issues and something can be
> improved in this patch, so will you send out one clean patch (I will
> help to test the hardware pattern support)? Or I can help to improve
> this code and try to upstream it again?
If you could do the upstreaming, that would be great.
I tried to get hardware accelerated LED to work on N900, but that
hardware is rather complex, so it would take me some time...
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel,
On 24 July 2018 at 19:41, Pavel Machek wrote:
> Hi!
>
>> >> > >Please keep in mind that this is ABI documentation for the pattern file
>> >> > >to be exposed by LED core, and not by the pattern trigger, that, as we
>> >> > >agreed, will be implemented later. In this case, I'd go for
>> >> >
>> >> > Gosh, I got completely distracted by the recent discussion about
>> >> > pattern synchronization.
>> >> >
>> >> > So, to recap, we need to decide if we are taking Baolin's solution
>> >> > or we're opting for implementing pattern trigger.
>> >> >
>> >> > If we choose the latter, then we will also need some software
>> >> > pattern engine in the trigger, to be applied as a software pattern
>> >> > fallback for the devices without hardware pattern support.
>> >> > It will certainly delay the contribution process, provided that Baolin
>> >> > would find time for this work at all.
>> >>
>> >> I'd recommend the latter. Yes, software pattern as a fallback would be
>> >> nice, but I have that code already... let me get it back to running
>> >> state, and figure out where to add interface for "hardware
>> >> acceleration". I'd like to have same interface to userland, whether
>> >> pattern can be done by hardware or by software.
>> >
>> > For the record, I'd like something like this. (Software pattern should
>> > work. Hardware pattern... needs more work).
>>
>> Thanks for showing your thoughts. But I failed to compile your code,
>> would you like to send out formal patches (Or only including software
>> pattern, I will help to add hardware pattern part and do some testing
>> with our driver)? Thanks.
>
> This should be a bit better. I attempted to compile it with your
> driver, but whether it works is an open question.
Sorry for late reply. I've compiled and tested this version on my
platform, the hardware pattern can work with one small fix as below.
if (led_cdev->pattern_set && !led_cdev->pattern_set(led_cdev,
data->steps, data->nsteps)) {
return;
}
But I saw there are lots coding style issues and something can be
improved in this patch, so will you send out one clean patch (I will
help to test the hardware pattern support)? Or I can help to improve
this code and try to upstream it again?
> Signed-off-by: Pavel Machek
>
>
> diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
> index 9d9b7aa..898f92d 100644
> --- a/drivers/leds/leds-sc27xx-bltc.c
> +++ b/drivers/leds/leds-sc27xx-bltc.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> /* PMIC global control register definition */
> @@ -32,8 +33,13 @@
> #define SC27XX_DUTY_MASK GENMASK(15, 0)
> #define SC27XX_MOD_MASKGENMASK(7, 0)
>
> +#define SC27XX_CURVE_SHIFT 8
> +#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
> +#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
> +
> #define SC27XX_LEDS_OFFSET 0x10
> #define SC27XX_LEDS_MAX3
> +#define SC27XX_LEDS_PATTERN_CNT4
>
> struct sc27xx_led {
> char name[LED_MAX_NAME_SIZE];
> @@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev,
> enum led_brightness value)
> return err;
> }
>
> +static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
> +{
> + struct sc27xx_led *leds = to_sc27xx_led(ldev);
> + struct regmap *regmap = leds->priv->regmap;
> + u32 base = sc27xx_led_get_offset(leds);
> + u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
> + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
> + int err;
> +
> + mutex_lock(>priv->lock);
> +
> + /* Reset the rise, high, fall and low time to zero. */
> + regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
> + regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
> +
> + err = regmap_update_bits(regmap, ctrl_base,
> + (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
> +
> + mutex_unlock(>priv->lock);
> +
> + return err;
> +}
> +
> +static int sc27xx_led_pattern_set(struct led_classdev *ldev,
> + struct led_pattern *pattern,
> + int len)
> +{
> + struct sc27xx_led *leds = to_sc27xx_led(ldev);
> + u32 base = sc27xx_led_get_offset(leds);
> + u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
> + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
> + struct regmap *regmap = leds->priv->regmap;
> + int err;
> +
> + /*
> +* Must contain 4 patterns to configure the rise time, high time, fall
> +* time and low time to enable the breathing mode.
> +*/
> + if (len != SC27XX_LEDS_PATTERN_CNT)
> + return -EINVAL;
> +
> + mutex_lock(>priv->lock);
> +
> + err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
> +SC27XX_CURVE_L_MASK, pattern[0].delta_t);
> + if (err)
> + goto out;
> +
> +
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel,
On 24 July 2018 at 19:41, Pavel Machek wrote:
> Hi!
>
>> >> > >Please keep in mind that this is ABI documentation for the pattern file
>> >> > >to be exposed by LED core, and not by the pattern trigger, that, as we
>> >> > >agreed, will be implemented later. In this case, I'd go for
>> >> >
>> >> > Gosh, I got completely distracted by the recent discussion about
>> >> > pattern synchronization.
>> >> >
>> >> > So, to recap, we need to decide if we are taking Baolin's solution
>> >> > or we're opting for implementing pattern trigger.
>> >> >
>> >> > If we choose the latter, then we will also need some software
>> >> > pattern engine in the trigger, to be applied as a software pattern
>> >> > fallback for the devices without hardware pattern support.
>> >> > It will certainly delay the contribution process, provided that Baolin
>> >> > would find time for this work at all.
>> >>
>> >> I'd recommend the latter. Yes, software pattern as a fallback would be
>> >> nice, but I have that code already... let me get it back to running
>> >> state, and figure out where to add interface for "hardware
>> >> acceleration". I'd like to have same interface to userland, whether
>> >> pattern can be done by hardware or by software.
>> >
>> > For the record, I'd like something like this. (Software pattern should
>> > work. Hardware pattern... needs more work).
>>
>> Thanks for showing your thoughts. But I failed to compile your code,
>> would you like to send out formal patches (Or only including software
>> pattern, I will help to add hardware pattern part and do some testing
>> with our driver)? Thanks.
>
> This should be a bit better. I attempted to compile it with your
> driver, but whether it works is an open question.
Sorry for late reply. I've compiled and tested this version on my
platform, the hardware pattern can work with one small fix as below.
if (led_cdev->pattern_set && !led_cdev->pattern_set(led_cdev,
data->steps, data->nsteps)) {
return;
}
But I saw there are lots coding style issues and something can be
improved in this patch, so will you send out one clean patch (I will
help to test the hardware pattern support)? Or I can help to improve
this code and try to upstream it again?
> Signed-off-by: Pavel Machek
>
>
> diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
> index 9d9b7aa..898f92d 100644
> --- a/drivers/leds/leds-sc27xx-bltc.c
> +++ b/drivers/leds/leds-sc27xx-bltc.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> /* PMIC global control register definition */
> @@ -32,8 +33,13 @@
> #define SC27XX_DUTY_MASK GENMASK(15, 0)
> #define SC27XX_MOD_MASKGENMASK(7, 0)
>
> +#define SC27XX_CURVE_SHIFT 8
> +#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
> +#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
> +
> #define SC27XX_LEDS_OFFSET 0x10
> #define SC27XX_LEDS_MAX3
> +#define SC27XX_LEDS_PATTERN_CNT4
>
> struct sc27xx_led {
> char name[LED_MAX_NAME_SIZE];
> @@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev,
> enum led_brightness value)
> return err;
> }
>
> +static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
> +{
> + struct sc27xx_led *leds = to_sc27xx_led(ldev);
> + struct regmap *regmap = leds->priv->regmap;
> + u32 base = sc27xx_led_get_offset(leds);
> + u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
> + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
> + int err;
> +
> + mutex_lock(>priv->lock);
> +
> + /* Reset the rise, high, fall and low time to zero. */
> + regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
> + regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
> +
> + err = regmap_update_bits(regmap, ctrl_base,
> + (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
> +
> + mutex_unlock(>priv->lock);
> +
> + return err;
> +}
> +
> +static int sc27xx_led_pattern_set(struct led_classdev *ldev,
> + struct led_pattern *pattern,
> + int len)
> +{
> + struct sc27xx_led *leds = to_sc27xx_led(ldev);
> + u32 base = sc27xx_led_get_offset(leds);
> + u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
> + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
> + struct regmap *regmap = leds->priv->regmap;
> + int err;
> +
> + /*
> +* Must contain 4 patterns to configure the rise time, high time, fall
> +* time and low time to enable the breathing mode.
> +*/
> + if (len != SC27XX_LEDS_PATTERN_CNT)
> + return -EINVAL;
> +
> + mutex_lock(>priv->lock);
> +
> + err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
> +SC27XX_CURVE_L_MASK, pattern[0].delta_t);
> + if (err)
> + goto out;
> +
> +
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> Thanks for showing your thoughts. But I failed to compile your code,
> would you like to send out formal patches (Or only including software
> pattern, I will help to add hardware pattern part and do some testing
> with our driver)? Thanks.
Random thoughts:
I am not sure "struct led_pattern" makes sense. Maybe array of
integers would be enough? Should we have structure with whole array
and length of the pattern?
struct led_pattern {
int len;
struct led_pattern steps[];
}
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> Thanks for showing your thoughts. But I failed to compile your code,
> would you like to send out formal patches (Or only including software
> pattern, I will help to add hardware pattern part and do some testing
> with our driver)? Thanks.
Random thoughts:
I am not sure "struct led_pattern" makes sense. Maybe array of
integers would be enough? Should we have structure with whole array
and length of the pattern?
struct led_pattern {
int len;
struct led_pattern steps[];
}
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> >> > >Please keep in mind that this is ABI documentation for the pattern file
> >> > >to be exposed by LED core, and not by the pattern trigger, that, as we
> >> > >agreed, will be implemented later. In this case, I'd go for
> >> >
> >> > Gosh, I got completely distracted by the recent discussion about
> >> > pattern synchronization.
> >> >
> >> > So, to recap, we need to decide if we are taking Baolin's solution
> >> > or we're opting for implementing pattern trigger.
> >> >
> >> > If we choose the latter, then we will also need some software
> >> > pattern engine in the trigger, to be applied as a software pattern
> >> > fallback for the devices without hardware pattern support.
> >> > It will certainly delay the contribution process, provided that Baolin
> >> > would find time for this work at all.
> >>
> >> I'd recommend the latter. Yes, software pattern as a fallback would be
> >> nice, but I have that code already... let me get it back to running
> >> state, and figure out where to add interface for "hardware
> >> acceleration". I'd like to have same interface to userland, whether
> >> pattern can be done by hardware or by software.
> >
> > For the record, I'd like something like this. (Software pattern should
> > work. Hardware pattern... needs more work).
>
> Thanks for showing your thoughts. But I failed to compile your code,
> would you like to send out formal patches (Or only including software
> pattern, I will help to add hardware pattern part and do some testing
> with our driver)? Thanks.
This should be a bit better. I attempted to compile it with your
driver, but whether it works is an open question.
Signed-off-by: Pavel Machek
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..898f92d 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
/* PMIC global control register definition */
@@ -32,8 +33,13 @@
#define SC27XX_DUTY_MASK GENMASK(15, 0)
#define SC27XX_MOD_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_SHIFT 8
+#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
+
#define SC27XX_LEDS_OFFSET 0x10
#define SC27XX_LEDS_MAX3
+#define SC27XX_LEDS_PATTERN_CNT4
struct sc27xx_led {
char name[LED_MAX_NAME_SIZE];
@@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum
led_brightness value)
return err;
}
+static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ struct regmap *regmap = leds->priv->regmap;
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ int err;
+
+ mutex_lock(>priv->lock);
+
+ /* Reset the rise, high, fall and low time to zero. */
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
+
+ err = regmap_update_bits(regmap, ctrl_base,
+ (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
+
+ mutex_unlock(>priv->lock);
+
+ return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+ struct led_pattern *pattern,
+ int len)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ struct regmap *regmap = leds->priv->regmap;
+ int err;
+
+ /*
+* Must contain 4 patterns to configure the rise time, high time, fall
+* time and low time to enable the breathing mode.
+*/
+ if (len != SC27XX_LEDS_PATTERN_CNT)
+ return -EINVAL;
+
+ mutex_lock(>priv->lock);
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_L_MASK, pattern[0].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_L_MASK, pattern[1].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_H_MASK,
+pattern[2].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_H_MASK,
+pattern[3].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_DUTY,
+
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> >> > >Please keep in mind that this is ABI documentation for the pattern file
> >> > >to be exposed by LED core, and not by the pattern trigger, that, as we
> >> > >agreed, will be implemented later. In this case, I'd go for
> >> >
> >> > Gosh, I got completely distracted by the recent discussion about
> >> > pattern synchronization.
> >> >
> >> > So, to recap, we need to decide if we are taking Baolin's solution
> >> > or we're opting for implementing pattern trigger.
> >> >
> >> > If we choose the latter, then we will also need some software
> >> > pattern engine in the trigger, to be applied as a software pattern
> >> > fallback for the devices without hardware pattern support.
> >> > It will certainly delay the contribution process, provided that Baolin
> >> > would find time for this work at all.
> >>
> >> I'd recommend the latter. Yes, software pattern as a fallback would be
> >> nice, but I have that code already... let me get it back to running
> >> state, and figure out where to add interface for "hardware
> >> acceleration". I'd like to have same interface to userland, whether
> >> pattern can be done by hardware or by software.
> >
> > For the record, I'd like something like this. (Software pattern should
> > work. Hardware pattern... needs more work).
>
> Thanks for showing your thoughts. But I failed to compile your code,
> would you like to send out formal patches (Or only including software
> pattern, I will help to add hardware pattern part and do some testing
> with our driver)? Thanks.
This should be a bit better. I attempted to compile it with your
driver, but whether it works is an open question.
Signed-off-by: Pavel Machek
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..898f92d 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
/* PMIC global control register definition */
@@ -32,8 +33,13 @@
#define SC27XX_DUTY_MASK GENMASK(15, 0)
#define SC27XX_MOD_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_SHIFT 8
+#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
+
#define SC27XX_LEDS_OFFSET 0x10
#define SC27XX_LEDS_MAX3
+#define SC27XX_LEDS_PATTERN_CNT4
struct sc27xx_led {
char name[LED_MAX_NAME_SIZE];
@@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum
led_brightness value)
return err;
}
+static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ struct regmap *regmap = leds->priv->regmap;
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ int err;
+
+ mutex_lock(>priv->lock);
+
+ /* Reset the rise, high, fall and low time to zero. */
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
+
+ err = regmap_update_bits(regmap, ctrl_base,
+ (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
+
+ mutex_unlock(>priv->lock);
+
+ return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+ struct led_pattern *pattern,
+ int len)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ struct regmap *regmap = leds->priv->regmap;
+ int err;
+
+ /*
+* Must contain 4 patterns to configure the rise time, high time, fall
+* time and low time to enable the breathing mode.
+*/
+ if (len != SC27XX_LEDS_PATTERN_CNT)
+ return -EINVAL;
+
+ mutex_lock(>priv->lock);
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_L_MASK, pattern[0].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_L_MASK, pattern[1].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_H_MASK,
+pattern[2].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_H_MASK,
+pattern[3].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_DUTY,
+
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Fri 20 Jul 12:11 PDT 2018, Jacek Anaszewski wrote:
> Hi David,
>
> On 07/18/2018 07:00 PM, David Lechner wrote:
> >
> >
> > On 7/18/18 7:08 AM, Pavel Machek wrote:
> > > On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
> > > > On 18 July 2018 at 15:56, Pavel Machek wrote:
> > > > > Hi!
> > > > >
> > > > > > > > > > I believe I meant "changing patterns
> > > > > > > > > > from kernel in response to events
> > > > > > > > > > is probably overkill"... or something like that.
> > > > > > > > >
> > > > > > > > > Anyway -- to clean up the confusion -- I'd like to see
> > > > > > > > >
> > > > > > > > > echo pattern > trigger
> > > > > > > > > echo "1 2 3 4 5 6 7 8" > somewhere
> > > > > > > >
> > > > > > > > s/somewhere/pattern/
> > > > > > > >
> > > > > > > > pattern trigger should create "pattern" file
> > > > > > > > similarly how ledtrig-timer
> > > > > > > > creates delay_{on|off} files.
> > > > >
> > > > > Yes, that sounds reasonable. v5 still says
> > > > >
> > > > > + Writing non-empty string to this file will
> > > > > activate the pattern,
> > > > > + and empty string will disable the pattern.
> > > > >
> > > > > I'd deactivate the pattern by simply writing something else to the
> > > > > trigger file.
> > > >
> > > > For the case we met in patch 2, it is not related with trigger things.
> > > > We just set some series of tuples including brightness and duration
> > > > (ms) to the hardware to enable the breath mode of the LED, we did not
> > > > trigger anything. So it is weird to write something to trigger file to
> > > > deactive the pattern.
> > >
> > > Confused. I thought that "breathing mode" would be handled similar way
> > > to hardware blinking: userland selects pattern trigger, pattern file
> > > appears (similar way to delay_on/delay_off files with blinking), he
> > > configures it, hardware brightness follows the pattern ("breathing
> > > mode"). If pattern is no longer required, echo none > trigger stops
> > > it.
> > > Pavel
> > >
> >
> > I was confused too when I first read this thread. But after reviewing
> > v5, it is clear that this is _not_ a trigger (and it should not be a
> > trigger). This is basically the equivalent the brightness attribute -
> > except that now the brightness changes over time instead of being a
> > constant value.
>
> Pattern trigger would be just more flexible version of existing
> ledtrig-timer.c, which also changes brightness over time.
>
> Trigger, by definition, is a kernel based source of LED events,
> so timer trigger falls under this definition, same way as pattern
> trigger would.
>
> What may cause confusion is the possibility of exploiting hardware
> capabilities, in case the requested settings fit in.
> ledtrig-timer fathom the hardware capabilities using blink_set op,
> and pattern trigger would use pattern_set op for that purpose.
>
For the use cases I had in mind it's perfectly fine to describe this as
a trigger.
But that said, I rather quickly started playing around with associating
patterns to trigger events; e.g. having a continuous pulse associated
with the bluetooth "power" trigger or defining a smooth rampdown for the
mmc activity trigger.
> > This way, the pattern can be used in conjunction with triggers.
> >
> > For example, one might want to set the _pattern_ to something like a
> > "breathe" pattern and set the _trigger_ to the power supply charging
> > full trigger. This way, the LED will be off until the battery is full
> > and then the LED will "breath" when the battery is full.
>
> AFAICS you comprehend "pattern trigger" notion as a LED trigger that
> activates pattern functionality. I'd say that you'd need a specialized
> "battery" trigger for that purpose, that instead of calling
> led_set_brightness_nosleep() would schedule call to
> led_trigger_set(led_cdev "pattern"), assuming that pattern trigger
> would be implemented as I explained above.
>
Presumably the battery logic has a number of states; low batter
discharging, low battery charging, full battery, levels in-between.
Defining the levels for these and an appropriate UX seems like a job for
something with a little bit of configurability and product specific
logic. The transitions here would need to reconfigure the pattern, so it
doesn't seem unreasonable for it to also set the pattern trigger.
I'm not sure this logic does belong in the kernel, but I think that
question is of less importance for the design of this.
Regards,
Bjorn
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Fri 20 Jul 12:11 PDT 2018, Jacek Anaszewski wrote:
> Hi David,
>
> On 07/18/2018 07:00 PM, David Lechner wrote:
> >
> >
> > On 7/18/18 7:08 AM, Pavel Machek wrote:
> > > On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
> > > > On 18 July 2018 at 15:56, Pavel Machek wrote:
> > > > > Hi!
> > > > >
> > > > > > > > > > I believe I meant "changing patterns
> > > > > > > > > > from kernel in response to events
> > > > > > > > > > is probably overkill"... or something like that.
> > > > > > > > >
> > > > > > > > > Anyway -- to clean up the confusion -- I'd like to see
> > > > > > > > >
> > > > > > > > > echo pattern > trigger
> > > > > > > > > echo "1 2 3 4 5 6 7 8" > somewhere
> > > > > > > >
> > > > > > > > s/somewhere/pattern/
> > > > > > > >
> > > > > > > > pattern trigger should create "pattern" file
> > > > > > > > similarly how ledtrig-timer
> > > > > > > > creates delay_{on|off} files.
> > > > >
> > > > > Yes, that sounds reasonable. v5 still says
> > > > >
> > > > > + Writing non-empty string to this file will
> > > > > activate the pattern,
> > > > > + and empty string will disable the pattern.
> > > > >
> > > > > I'd deactivate the pattern by simply writing something else to the
> > > > > trigger file.
> > > >
> > > > For the case we met in patch 2, it is not related with trigger things.
> > > > We just set some series of tuples including brightness and duration
> > > > (ms) to the hardware to enable the breath mode of the LED, we did not
> > > > trigger anything. So it is weird to write something to trigger file to
> > > > deactive the pattern.
> > >
> > > Confused. I thought that "breathing mode" would be handled similar way
> > > to hardware blinking: userland selects pattern trigger, pattern file
> > > appears (similar way to delay_on/delay_off files with blinking), he
> > > configures it, hardware brightness follows the pattern ("breathing
> > > mode"). If pattern is no longer required, echo none > trigger stops
> > > it.
> > > Pavel
> > >
> >
> > I was confused too when I first read this thread. But after reviewing
> > v5, it is clear that this is _not_ a trigger (and it should not be a
> > trigger). This is basically the equivalent the brightness attribute -
> > except that now the brightness changes over time instead of being a
> > constant value.
>
> Pattern trigger would be just more flexible version of existing
> ledtrig-timer.c, which also changes brightness over time.
>
> Trigger, by definition, is a kernel based source of LED events,
> so timer trigger falls under this definition, same way as pattern
> trigger would.
>
> What may cause confusion is the possibility of exploiting hardware
> capabilities, in case the requested settings fit in.
> ledtrig-timer fathom the hardware capabilities using blink_set op,
> and pattern trigger would use pattern_set op for that purpose.
>
For the use cases I had in mind it's perfectly fine to describe this as
a trigger.
But that said, I rather quickly started playing around with associating
patterns to trigger events; e.g. having a continuous pulse associated
with the bluetooth "power" trigger or defining a smooth rampdown for the
mmc activity trigger.
> > This way, the pattern can be used in conjunction with triggers.
> >
> > For example, one might want to set the _pattern_ to something like a
> > "breathe" pattern and set the _trigger_ to the power supply charging
> > full trigger. This way, the LED will be off until the battery is full
> > and then the LED will "breath" when the battery is full.
>
> AFAICS you comprehend "pattern trigger" notion as a LED trigger that
> activates pattern functionality. I'd say that you'd need a specialized
> "battery" trigger for that purpose, that instead of calling
> led_set_brightness_nosleep() would schedule call to
> led_trigger_set(led_cdev "pattern"), assuming that pattern trigger
> would be implemented as I explained above.
>
Presumably the battery logic has a number of states; low batter
discharging, low battery charging, full battery, levels in-between.
Defining the levels for these and an appropriate UX seems like a job for
something with a little bit of configurability and product specific
logic. The transitions here would need to reconfigure the pattern, so it
doesn't seem unreasonable for it to also set the pattern trigger.
I'm not sure this logic does belong in the kernel, but I think that
question is of less importance for the design of this.
Regards,
Bjorn
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Mon 16 Jul 14:56 PDT 2018, Pavel Machek wrote:
> Hi!
>
> > >>>echo pattern > trigger
> > >>>echo "1 2 3 4 5 6 7 8" > somewhere
> > >>
> > >>s/somewhere/pattern/
> > >>
> > >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> > >>creates delay_{on|off} files.
> > >>
> > >
> > >I don't think this is the best way. For example, if you want more than one
> > >LED to have the same pattern, then the patterns will not be synchronized
> > >between the LEDs. The same things happens now with many of the existing
> > >triggers. For example, if I have two LEDs side-by-side using the heartbeat
> > >trigger, they may blink at the same time or they may not, which is not
> > >very nice. I think we can make something better.
>
> Yes, synchronization would be nice -- it is really a must for RGB LEDs
> -- but I believe it is too much to ask from Baolin at the moment.
>
In my work on the Qualcomm LPG (which I should finish up and resend) I
described the RGB as a single LED, with the color configured by Pavel's
suggested HSV interface. This single LED would be given one pattern.
This works fine for the typical use cases we've seen so far, but would
not be enough to describe transitions between colors.
I believe that a reasonable solution to this would be to extend the
pattern to allow each value in the list to contain data for more than
one channel - something that should be reasonable to add on top of this
suggestion.
> > It is virtually impossible to enforce synchronous blinking for the
> > LEDs driven by different hardware due to:
> >
> > - different hardware means via which brightness is set (MMIO, I2C, SPI,
> > PWM and other pulsed fashion based protocols),
> > - the need for deferring brightness setting to a workqueue task to
> > allow for setting LED brightness from atomic context,
> > - contention on locks
>
> I disagree here. Yes, it would be hard to synchronize blinking down to
> microseconds, but it would be easy to get synchronization right down
> to miliseconds and humans will not be able to tell the difference.
>
I like the HSV approach for exposing multi color LEDs and we should be
able to provide a "virtual" LED that groups some number of LEDs into
one, to represent this use case when the hardware doesn't do directly.
In such cases it would work quite weel to use the proposed pattern
interface for setting the pattern of this virtual LED and having it
cascade the pattern into the individual LEDs and then start them at
about the same time...
> > For the LEDs driven by the same chip it would make more sense
> > to allow for synchronization, but it can be achieved on driver
> > level, with help of some subsystem level interface to indicate
> > which LEDs should be synchronized.
> >
> > However, when we start to pretend that we can synchronize the
> > devices, we must answer how accurate we can be. The accuracy
> > will decrease as blink frequency rises. We'd need to define
> > reliability limit.
>
> We don't need _that_ ammount of overengineering. We just need to
> synchronize them well enough :-).
>
> > We've had few attempts of approaching the subject of synchronized
> > blinking but none of them proved to be good enough to be merged.
>
> I'm sure interested person could do something like that in less than
> two weeks fulltime... It is not rocket science, just a lot of work in
> kernel...
>
With the Qualcomm LPG driver I expect the hardware description to group
the channels of a RGB and as such the driver will synchronize the
pattern execution when the output is enabled.
> But patterns are few years overdue and I believe we should not delay
> them any further.
>
Sorry for not prioritizing reworking this patch based on your suggestion
of describing it as a trigger, I still think this sounds reasonable.
Regards,
Bjorn
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Mon 16 Jul 14:56 PDT 2018, Pavel Machek wrote:
> Hi!
>
> > >>>echo pattern > trigger
> > >>>echo "1 2 3 4 5 6 7 8" > somewhere
> > >>
> > >>s/somewhere/pattern/
> > >>
> > >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> > >>creates delay_{on|off} files.
> > >>
> > >
> > >I don't think this is the best way. For example, if you want more than one
> > >LED to have the same pattern, then the patterns will not be synchronized
> > >between the LEDs. The same things happens now with many of the existing
> > >triggers. For example, if I have two LEDs side-by-side using the heartbeat
> > >trigger, they may blink at the same time or they may not, which is not
> > >very nice. I think we can make something better.
>
> Yes, synchronization would be nice -- it is really a must for RGB LEDs
> -- but I believe it is too much to ask from Baolin at the moment.
>
In my work on the Qualcomm LPG (which I should finish up and resend) I
described the RGB as a single LED, with the color configured by Pavel's
suggested HSV interface. This single LED would be given one pattern.
This works fine for the typical use cases we've seen so far, but would
not be enough to describe transitions between colors.
I believe that a reasonable solution to this would be to extend the
pattern to allow each value in the list to contain data for more than
one channel - something that should be reasonable to add on top of this
suggestion.
> > It is virtually impossible to enforce synchronous blinking for the
> > LEDs driven by different hardware due to:
> >
> > - different hardware means via which brightness is set (MMIO, I2C, SPI,
> > PWM and other pulsed fashion based protocols),
> > - the need for deferring brightness setting to a workqueue task to
> > allow for setting LED brightness from atomic context,
> > - contention on locks
>
> I disagree here. Yes, it would be hard to synchronize blinking down to
> microseconds, but it would be easy to get synchronization right down
> to miliseconds and humans will not be able to tell the difference.
>
I like the HSV approach for exposing multi color LEDs and we should be
able to provide a "virtual" LED that groups some number of LEDs into
one, to represent this use case when the hardware doesn't do directly.
In such cases it would work quite weel to use the proposed pattern
interface for setting the pattern of this virtual LED and having it
cascade the pattern into the individual LEDs and then start them at
about the same time...
> > For the LEDs driven by the same chip it would make more sense
> > to allow for synchronization, but it can be achieved on driver
> > level, with help of some subsystem level interface to indicate
> > which LEDs should be synchronized.
> >
> > However, when we start to pretend that we can synchronize the
> > devices, we must answer how accurate we can be. The accuracy
> > will decrease as blink frequency rises. We'd need to define
> > reliability limit.
>
> We don't need _that_ ammount of overengineering. We just need to
> synchronize them well enough :-).
>
> > We've had few attempts of approaching the subject of synchronized
> > blinking but none of them proved to be good enough to be merged.
>
> I'm sure interested person could do something like that in less than
> two weeks fulltime... It is not rocket science, just a lot of work in
> kernel...
>
With the Qualcomm LPG driver I expect the hardware description to group
the channels of a RGB and as such the driver will synchronize the
pattern execution when the output is enabled.
> But patterns are few years overdue and I believe we should not delay
> them any further.
>
Sorry for not prioritizing reworking this patch based on your suggestion
of describing it as a trigger, I still think this sounds reasonable.
Regards,
Bjorn
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Sun 15 Jul 18:00 PDT 2018, David Lechner wrote:
> On 07/15/2018 07:22 AM, Jacek Anaszewski wrote:
> > On 07/15/2018 12:39 AM, Pavel Machek wrote:
> > > On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
> > > > On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
> > > > > Hi Pavel,
> > > > >
> > > > > On 07/14/2018 11:20 PM, Pavel Machek wrote:
> > > > > > Hi!
> > > > > >
> > > > > > > > It also drew my attention to the issue of desired pattern sysfs
> > > > > > > > interface semantics on uninitialized pattern. In your
> > > > > > > > implementation
> > > > > > > > user seems to be unable to determine if the pattern is activated
> > > > > > > > or not. We should define the semantics for this use case and
> > > > > > > > describe it in the documentation. Possibly pattern could
> > > > > > > > return alone new line character then.
> > > > > >
> > > > > > Let me take a step back: we have triggers.. like LED blinking.
> > > > > >
> > > > > > How is that going to interact with patterns? We probably want the
> > > > > > patterns to be ignored in that case...?
> > > > > >
> > > > > > Which suggest to me that we should treat patterns as a trigger. I
> > > > > > believe we do something similar with blinking already.
> > > > > >
> > > > > > Then it is easy to determine if pattern is active, and pattern
> > > > > > vs. trigger issue is solved automatically.
> > > > >
> > > > > I'm all for it. I proposed this approach during the previous
> > > > > discussions related to possible pattern interface implementations,
> > > > > but you seemed not to be so enthusiastic in [0].
> > > > >
> > > > > [0] https://lkml.org/lkml/2017/4/7/350
> > > >
> > > > Hmm. Reading my own email now, I can't decipher it.
> > > >
> > > > I believe I meant "changing patterns from kernel in response to events
> > > > is probably overkill"... or something like that.
> > >
> > > Anyway -- to clean up the confusion -- I'd like to see
> > >
> > > echo pattern > trigger
> > > echo "1 2 3 4 5 6 7 8" > somewhere
> >
> > s/somewhere/pattern/
> >
> > pattern trigger should create "pattern" file similarly how ledtrig-timer
> > creates delay_{on|off} files.
> >
>
> I don't think this is the best way. For example, if you want more than one
> LED to have the same pattern, then the patterns will not be synchronized
> between the LEDs. The same things happens now with many of the existing
> triggers. For example, if I have two LEDs side-by-side using the heartbeat
> trigger, they may blink at the same time or they may not, which is not
> very nice. I think we can make something better.
>
> Perhaps a way to do this would be to use configfs to create a pattern
> trigger that can be shared by multiple LEDs. Like this:
>
> mkdir /sys/kernel/config/leds/triggers/my-nice-pattern
> echo "1 2 3 4" > /sys/kernel/config/leds/triggers/my-nice-pattern/pattern
> echo my-nice-pattern > /sys/class/leds/led0/trigger
> echo my-nice-pattern > /sys/class/leds/led1/trigger
>
In the case where you describe this as two different LEDs (to Linux) and
you rely on the two enable-calls to happen fairly quickly I think you
can just as well specify the same pattern in two independent triggers.
This also helps by not providing the illusion of there being any
synchronization between them.
Regards,
Bjorn
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Sun 15 Jul 18:00 PDT 2018, David Lechner wrote:
> On 07/15/2018 07:22 AM, Jacek Anaszewski wrote:
> > On 07/15/2018 12:39 AM, Pavel Machek wrote:
> > > On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
> > > > On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
> > > > > Hi Pavel,
> > > > >
> > > > > On 07/14/2018 11:20 PM, Pavel Machek wrote:
> > > > > > Hi!
> > > > > >
> > > > > > > > It also drew my attention to the issue of desired pattern sysfs
> > > > > > > > interface semantics on uninitialized pattern. In your
> > > > > > > > implementation
> > > > > > > > user seems to be unable to determine if the pattern is activated
> > > > > > > > or not. We should define the semantics for this use case and
> > > > > > > > describe it in the documentation. Possibly pattern could
> > > > > > > > return alone new line character then.
> > > > > >
> > > > > > Let me take a step back: we have triggers.. like LED blinking.
> > > > > >
> > > > > > How is that going to interact with patterns? We probably want the
> > > > > > patterns to be ignored in that case...?
> > > > > >
> > > > > > Which suggest to me that we should treat patterns as a trigger. I
> > > > > > believe we do something similar with blinking already.
> > > > > >
> > > > > > Then it is easy to determine if pattern is active, and pattern
> > > > > > vs. trigger issue is solved automatically.
> > > > >
> > > > > I'm all for it. I proposed this approach during the previous
> > > > > discussions related to possible pattern interface implementations,
> > > > > but you seemed not to be so enthusiastic in [0].
> > > > >
> > > > > [0] https://lkml.org/lkml/2017/4/7/350
> > > >
> > > > Hmm. Reading my own email now, I can't decipher it.
> > > >
> > > > I believe I meant "changing patterns from kernel in response to events
> > > > is probably overkill"... or something like that.
> > >
> > > Anyway -- to clean up the confusion -- I'd like to see
> > >
> > > echo pattern > trigger
> > > echo "1 2 3 4 5 6 7 8" > somewhere
> >
> > s/somewhere/pattern/
> >
> > pattern trigger should create "pattern" file similarly how ledtrig-timer
> > creates delay_{on|off} files.
> >
>
> I don't think this is the best way. For example, if you want more than one
> LED to have the same pattern, then the patterns will not be synchronized
> between the LEDs. The same things happens now with many of the existing
> triggers. For example, if I have two LEDs side-by-side using the heartbeat
> trigger, they may blink at the same time or they may not, which is not
> very nice. I think we can make something better.
>
> Perhaps a way to do this would be to use configfs to create a pattern
> trigger that can be shared by multiple LEDs. Like this:
>
> mkdir /sys/kernel/config/leds/triggers/my-nice-pattern
> echo "1 2 3 4" > /sys/kernel/config/leds/triggers/my-nice-pattern/pattern
> echo my-nice-pattern > /sys/class/leds/led0/trigger
> echo my-nice-pattern > /sys/class/leds/led1/trigger
>
In the case where you describe this as two different LEDs (to Linux) and
you rely on the two enable-calls to happen fairly quickly I think you
can just as well specify the same pattern in two independent triggers.
This also helps by not providing the illusion of there being any
synchronization between them.
Regards,
Bjorn
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel,
On 20 July 2018 at 04:20, Pavel Machek wrote:
> Hi!
>
>> > >Please keep in mind that this is ABI documentation for the pattern file
>> > >to be exposed by LED core, and not by the pattern trigger, that, as we
>> > >agreed, will be implemented later. In this case, I'd go for
>> >
>> > Gosh, I got completely distracted by the recent discussion about
>> > pattern synchronization.
>> >
>> > So, to recap, we need to decide if we are taking Baolin's solution
>> > or we're opting for implementing pattern trigger.
>> >
>> > If we choose the latter, then we will also need some software
>> > pattern engine in the trigger, to be applied as a software pattern
>> > fallback for the devices without hardware pattern support.
>> > It will certainly delay the contribution process, provided that Baolin
>> > would find time for this work at all.
>>
>> I'd recommend the latter. Yes, software pattern as a fallback would be
>> nice, but I have that code already... let me get it back to running
>> state, and figure out where to add interface for "hardware
>> acceleration". I'd like to have same interface to userland, whether
>> pattern can be done by hardware or by software.
>
> For the record, I'd like something like this. (Software pattern should
> work. Hardware pattern... needs more work).
Thanks for showing your thoughts. But I failed to compile your code,
would you like to send out formal patches (Or only including software
pattern, I will help to add hardware pattern part and do some testing
with our driver)? Thanks.
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ede4fa0..8cf5962 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -51,6 +51,8 @@ static void led_timer_function(struct timer_list *t)
> unsigned long brightness;
> unsigned long delay;
>
> + /* FIXME spin_lock(led_cdev->lock); protecting led_cdev->flags? */
> +
> if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
> led_set_brightness_nosleep(led_cdev, LED_OFF);
> clear_bit(LED_BLINK_SW, _cdev->work_flags);
> diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
> index 9d9b7aa..898f92d 100644
> --- a/drivers/leds/leds-sc27xx-bltc.c
> +++ b/drivers/leds/leds-sc27xx-bltc.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> /* PMIC global control register definition */
> @@ -32,8 +33,13 @@
> #define SC27XX_DUTY_MASK GENMASK(15, 0)
> #define SC27XX_MOD_MASKGENMASK(7, 0)
>
> +#define SC27XX_CURVE_SHIFT 8
> +#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
> +#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
> +
> #define SC27XX_LEDS_OFFSET 0x10
> #define SC27XX_LEDS_MAX3
> +#define SC27XX_LEDS_PATTERN_CNT4
>
> struct sc27xx_led {
> char name[LED_MAX_NAME_SIZE];
> @@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev,
> enum led_brightness value)
> return err;
> }
>
> +static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
> +{
> + struct sc27xx_led *leds = to_sc27xx_led(ldev);
> + struct regmap *regmap = leds->priv->regmap;
> + u32 base = sc27xx_led_get_offset(leds);
> + u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
> + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
> + int err;
> +
> + mutex_lock(>priv->lock);
> +
> + /* Reset the rise, high, fall and low time to zero. */
> + regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
> + regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
> +
> + err = regmap_update_bits(regmap, ctrl_base,
> + (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
> +
> + mutex_unlock(>priv->lock);
> +
> + return err;
> +}
> +
> +static int sc27xx_led_pattern_set(struct led_classdev *ldev,
> + struct led_pattern *pattern,
> + int len)
> +{
> + struct sc27xx_led *leds = to_sc27xx_led(ldev);
> + u32 base = sc27xx_led_get_offset(leds);
> + u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
> + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
> + struct regmap *regmap = leds->priv->regmap;
> + int err;
> +
> + /*
> +* Must contain 4 patterns to configure the rise time, high time, fall
> +* time and low time to enable the breathing mode.
> +*/
> + if (len != SC27XX_LEDS_PATTERN_CNT)
> + return -EINVAL;
> +
> + mutex_lock(>priv->lock);
> +
> + err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
> +SC27XX_CURVE_L_MASK, pattern[0].delta_t);
> + if (err)
> + goto out;
> +
> + err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
> +SC27XX_CURVE_L_MASK, pattern[1].delta_t);
> + if
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel,
On 20 July 2018 at 04:20, Pavel Machek wrote:
> Hi!
>
>> > >Please keep in mind that this is ABI documentation for the pattern file
>> > >to be exposed by LED core, and not by the pattern trigger, that, as we
>> > >agreed, will be implemented later. In this case, I'd go for
>> >
>> > Gosh, I got completely distracted by the recent discussion about
>> > pattern synchronization.
>> >
>> > So, to recap, we need to decide if we are taking Baolin's solution
>> > or we're opting for implementing pattern trigger.
>> >
>> > If we choose the latter, then we will also need some software
>> > pattern engine in the trigger, to be applied as a software pattern
>> > fallback for the devices without hardware pattern support.
>> > It will certainly delay the contribution process, provided that Baolin
>> > would find time for this work at all.
>>
>> I'd recommend the latter. Yes, software pattern as a fallback would be
>> nice, but I have that code already... let me get it back to running
>> state, and figure out where to add interface for "hardware
>> acceleration". I'd like to have same interface to userland, whether
>> pattern can be done by hardware or by software.
>
> For the record, I'd like something like this. (Software pattern should
> work. Hardware pattern... needs more work).
Thanks for showing your thoughts. But I failed to compile your code,
would you like to send out formal patches (Or only including software
pattern, I will help to add hardware pattern part and do some testing
with our driver)? Thanks.
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ede4fa0..8cf5962 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -51,6 +51,8 @@ static void led_timer_function(struct timer_list *t)
> unsigned long brightness;
> unsigned long delay;
>
> + /* FIXME spin_lock(led_cdev->lock); protecting led_cdev->flags? */
> +
> if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
> led_set_brightness_nosleep(led_cdev, LED_OFF);
> clear_bit(LED_BLINK_SW, _cdev->work_flags);
> diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
> index 9d9b7aa..898f92d 100644
> --- a/drivers/leds/leds-sc27xx-bltc.c
> +++ b/drivers/leds/leds-sc27xx-bltc.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> /* PMIC global control register definition */
> @@ -32,8 +33,13 @@
> #define SC27XX_DUTY_MASK GENMASK(15, 0)
> #define SC27XX_MOD_MASKGENMASK(7, 0)
>
> +#define SC27XX_CURVE_SHIFT 8
> +#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
> +#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
> +
> #define SC27XX_LEDS_OFFSET 0x10
> #define SC27XX_LEDS_MAX3
> +#define SC27XX_LEDS_PATTERN_CNT4
>
> struct sc27xx_led {
> char name[LED_MAX_NAME_SIZE];
> @@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev,
> enum led_brightness value)
> return err;
> }
>
> +static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
> +{
> + struct sc27xx_led *leds = to_sc27xx_led(ldev);
> + struct regmap *regmap = leds->priv->regmap;
> + u32 base = sc27xx_led_get_offset(leds);
> + u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
> + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
> + int err;
> +
> + mutex_lock(>priv->lock);
> +
> + /* Reset the rise, high, fall and low time to zero. */
> + regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
> + regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
> +
> + err = regmap_update_bits(regmap, ctrl_base,
> + (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
> +
> + mutex_unlock(>priv->lock);
> +
> + return err;
> +}
> +
> +static int sc27xx_led_pattern_set(struct led_classdev *ldev,
> + struct led_pattern *pattern,
> + int len)
> +{
> + struct sc27xx_led *leds = to_sc27xx_led(ldev);
> + u32 base = sc27xx_led_get_offset(leds);
> + u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
> + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
> + struct regmap *regmap = leds->priv->regmap;
> + int err;
> +
> + /*
> +* Must contain 4 patterns to configure the rise time, high time, fall
> +* time and low time to enable the breathing mode.
> +*/
> + if (len != SC27XX_LEDS_PATTERN_CNT)
> + return -EINVAL;
> +
> + mutex_lock(>priv->lock);
> +
> + err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
> +SC27XX_CURVE_L_MASK, pattern[0].delta_t);
> + if (err)
> + goto out;
> +
> + err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
> +SC27XX_CURVE_L_MASK, pattern[1].delta_t);
> + if
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi David,
On 07/18/2018 07:00 PM, David Lechner wrote:
On 7/18/18 7:08 AM, Pavel Machek wrote:
On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
On 18 July 2018 at 15:56, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to
events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how
ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate
the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
For the case we met in patch 2, it is not related with trigger things.
We just set some series of tuples including brightness and duration
(ms) to the hardware to enable the breath mode of the LED, we did not
trigger anything. So it is weird to write something to trigger file to
deactive the pattern.
Confused. I thought that "breathing mode" would be handled similar way
to hardware blinking: userland selects pattern trigger, pattern file
appears (similar way to delay_on/delay_off files with blinking), he
configures it, hardware brightness follows the pattern ("breathing
mode"). If pattern is no longer required, echo none > trigger stops
it.
Pavel
I was confused too when I first read this thread. But after reviewing
v5, it is clear that this is _not_ a trigger (and it should not be a
trigger). This is basically the equivalent the brightness attribute -
except that now the brightness changes over time instead of being a
constant value.
Pattern trigger would be just more flexible version of existing
ledtrig-timer.c, which also changes brightness over time.
Trigger, by definition, is a kernel based source of LED events,
so timer trigger falls under this definition, same way as pattern
trigger would.
What may cause confusion is the possibility of exploiting hardware
capabilities, in case the requested settings fit in.
ledtrig-timer fathom the hardware capabilities using blink_set op,
and pattern trigger would use pattern_set op for that purpose.
This way, the pattern can be used in conjunction with
triggers.
For example, one might want to set the _pattern_ to something like a
"breathe" pattern and set the _trigger_ to the power supply charging
full trigger. This way, the LED will be off until the battery is full
and then the LED will "breath" when the battery is full.
AFAICS you comprehend "pattern trigger" notion as a LED trigger that
activates pattern functionality. I'd say that you'd need a specialized
"battery" trigger for that purpose, that instead of calling
led_set_brightness_nosleep() would schedule call to
led_trigger_set(led_cdev "pattern"), assuming that pattern trigger
would be implemented as I explained above.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi David,
On 07/18/2018 07:00 PM, David Lechner wrote:
On 7/18/18 7:08 AM, Pavel Machek wrote:
On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
On 18 July 2018 at 15:56, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to
events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how
ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate
the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
For the case we met in patch 2, it is not related with trigger things.
We just set some series of tuples including brightness and duration
(ms) to the hardware to enable the breath mode of the LED, we did not
trigger anything. So it is weird to write something to trigger file to
deactive the pattern.
Confused. I thought that "breathing mode" would be handled similar way
to hardware blinking: userland selects pattern trigger, pattern file
appears (similar way to delay_on/delay_off files with blinking), he
configures it, hardware brightness follows the pattern ("breathing
mode"). If pattern is no longer required, echo none > trigger stops
it.
Pavel
I was confused too when I first read this thread. But after reviewing
v5, it is clear that this is _not_ a trigger (and it should not be a
trigger). This is basically the equivalent the brightness attribute -
except that now the brightness changes over time instead of being a
constant value.
Pattern trigger would be just more flexible version of existing
ledtrig-timer.c, which also changes brightness over time.
Trigger, by definition, is a kernel based source of LED events,
so timer trigger falls under this definition, same way as pattern
trigger would.
What may cause confusion is the possibility of exploiting hardware
capabilities, in case the requested settings fit in.
ledtrig-timer fathom the hardware capabilities using blink_set op,
and pattern trigger would use pattern_set op for that purpose.
This way, the pattern can be used in conjunction with
triggers.
For example, one might want to set the _pattern_ to something like a
"breathe" pattern and set the _trigger_ to the power supply charging
full trigger. This way, the LED will be off until the battery is full
and then the LED will "breath" when the battery is full.
AFAICS you comprehend "pattern trigger" notion as a LED trigger that
activates pattern functionality. I'd say that you'd need a specialized
"battery" trigger for that purpose, that instead of calling
led_set_brightness_nosleep() would schedule call to
led_trigger_set(led_cdev "pattern"), assuming that pattern trigger
would be implemented as I explained above.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel,
On 07/19/2018 10:20 PM, Pavel Machek wrote:
Hi!
Please keep in mind that this is ABI documentation for the pattern file
to be exposed by LED core, and not by the pattern trigger, that, as we
agreed, will be implemented later. In this case, I'd go for
Gosh, I got completely distracted by the recent discussion about
pattern synchronization.
So, to recap, we need to decide if we are taking Baolin's solution
or we're opting for implementing pattern trigger.
If we choose the latter, then we will also need some software
pattern engine in the trigger, to be applied as a software pattern
fallback for the devices without hardware pattern support.
It will certainly delay the contribution process, provided that Baolin
would find time for this work at all.
I'd recommend the latter. Yes, software pattern as a fallback would be
nice, but I have that code already... let me get it back to running
state, and figure out where to add interface for "hardware
acceleration". I'd like to have same interface to userland, whether
pattern can be done by hardware or by software.
For the record, I'd like something like this. (Software pattern should
work. Hardware pattern... needs more work).
Thank you for the patch. I'll be able to comment on it in two weeks.
I'll be offline during that time.
Best regards,
Jacek Anaszewski
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index ede4fa0..8cf5962 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -51,6 +51,8 @@ static void led_timer_function(struct timer_list *t)
unsigned long brightness;
unsigned long delay;
+ /* FIXME spin_lock(led_cdev->lock); protecting led_cdev->flags? */
+
if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
led_set_brightness_nosleep(led_cdev, LED_OFF);
clear_bit(LED_BLINK_SW, _cdev->work_flags);
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..898f92d 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
/* PMIC global control register definition */
@@ -32,8 +33,13 @@
#define SC27XX_DUTY_MASK GENMASK(15, 0)
#define SC27XX_MOD_MASK GENMASK(7, 0)
+#define SC27XX_CURVE_SHIFT 8
+#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
+
#define SC27XX_LEDS_OFFSET0x10
#define SC27XX_LEDS_MAX 3
+#define SC27XX_LEDS_PATTERN_CNT4
struct sc27xx_led {
char name[LED_MAX_NAME_SIZE];
@@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum
led_brightness value)
return err;
}
+static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ struct regmap *regmap = leds->priv->regmap;
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ int err;
+
+ mutex_lock(>priv->lock);
+
+ /* Reset the rise, high, fall and low time to zero. */
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
+
+ err = regmap_update_bits(regmap, ctrl_base,
+ (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
+
+ mutex_unlock(>priv->lock);
+
+ return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+ struct led_pattern *pattern,
+ int len)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ struct regmap *regmap = leds->priv->regmap;
+ int err;
+
+ /*
+* Must contain 4 patterns to configure the rise time, high time, fall
+* time and low time to enable the breathing mode.
+*/
+ if (len != SC27XX_LEDS_PATTERN_CNT)
+ return -EINVAL;
+
+ mutex_lock(>priv->lock);
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_L_MASK, pattern[0].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_L_MASK, pattern[1].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_H_MASK,
+pattern[2].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel,
On 07/19/2018 10:20 PM, Pavel Machek wrote:
Hi!
Please keep in mind that this is ABI documentation for the pattern file
to be exposed by LED core, and not by the pattern trigger, that, as we
agreed, will be implemented later. In this case, I'd go for
Gosh, I got completely distracted by the recent discussion about
pattern synchronization.
So, to recap, we need to decide if we are taking Baolin's solution
or we're opting for implementing pattern trigger.
If we choose the latter, then we will also need some software
pattern engine in the trigger, to be applied as a software pattern
fallback for the devices without hardware pattern support.
It will certainly delay the contribution process, provided that Baolin
would find time for this work at all.
I'd recommend the latter. Yes, software pattern as a fallback would be
nice, but I have that code already... let me get it back to running
state, and figure out where to add interface for "hardware
acceleration". I'd like to have same interface to userland, whether
pattern can be done by hardware or by software.
For the record, I'd like something like this. (Software pattern should
work. Hardware pattern... needs more work).
Thank you for the patch. I'll be able to comment on it in two weeks.
I'll be offline during that time.
Best regards,
Jacek Anaszewski
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index ede4fa0..8cf5962 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -51,6 +51,8 @@ static void led_timer_function(struct timer_list *t)
unsigned long brightness;
unsigned long delay;
+ /* FIXME spin_lock(led_cdev->lock); protecting led_cdev->flags? */
+
if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
led_set_brightness_nosleep(led_cdev, LED_OFF);
clear_bit(LED_BLINK_SW, _cdev->work_flags);
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..898f92d 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
/* PMIC global control register definition */
@@ -32,8 +33,13 @@
#define SC27XX_DUTY_MASK GENMASK(15, 0)
#define SC27XX_MOD_MASK GENMASK(7, 0)
+#define SC27XX_CURVE_SHIFT 8
+#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
+
#define SC27XX_LEDS_OFFSET0x10
#define SC27XX_LEDS_MAX 3
+#define SC27XX_LEDS_PATTERN_CNT4
struct sc27xx_led {
char name[LED_MAX_NAME_SIZE];
@@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum
led_brightness value)
return err;
}
+static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ struct regmap *regmap = leds->priv->regmap;
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ int err;
+
+ mutex_lock(>priv->lock);
+
+ /* Reset the rise, high, fall and low time to zero. */
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
+
+ err = regmap_update_bits(regmap, ctrl_base,
+ (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
+
+ mutex_unlock(>priv->lock);
+
+ return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+ struct led_pattern *pattern,
+ int len)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ struct regmap *regmap = leds->priv->regmap;
+ int err;
+
+ /*
+* Must contain 4 patterns to configure the rise time, high time, fall
+* time and low time to enable the breathing mode.
+*/
+ if (len != SC27XX_LEDS_PATTERN_CNT)
+ return -EINVAL;
+
+ mutex_lock(>priv->lock);
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_L_MASK, pattern[0].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_L_MASK, pattern[1].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_H_MASK,
+pattern[2].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> > >Please keep in mind that this is ABI documentation for the pattern file
> > >to be exposed by LED core, and not by the pattern trigger, that, as we
> > >agreed, will be implemented later. In this case, I'd go for
> >
> > Gosh, I got completely distracted by the recent discussion about
> > pattern synchronization.
> >
> > So, to recap, we need to decide if we are taking Baolin's solution
> > or we're opting for implementing pattern trigger.
> >
> > If we choose the latter, then we will also need some software
> > pattern engine in the trigger, to be applied as a software pattern
> > fallback for the devices without hardware pattern support.
> > It will certainly delay the contribution process, provided that Baolin
> > would find time for this work at all.
>
> I'd recommend the latter. Yes, software pattern as a fallback would be
> nice, but I have that code already... let me get it back to running
> state, and figure out where to add interface for "hardware
> acceleration". I'd like to have same interface to userland, whether
> pattern can be done by hardware or by software.
For the record, I'd like something like this. (Software pattern should
work. Hardware pattern... needs more work).
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index ede4fa0..8cf5962 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -51,6 +51,8 @@ static void led_timer_function(struct timer_list *t)
unsigned long brightness;
unsigned long delay;
+ /* FIXME spin_lock(led_cdev->lock); protecting led_cdev->flags? */
+
if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
led_set_brightness_nosleep(led_cdev, LED_OFF);
clear_bit(LED_BLINK_SW, _cdev->work_flags);
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..898f92d 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
/* PMIC global control register definition */
@@ -32,8 +33,13 @@
#define SC27XX_DUTY_MASK GENMASK(15, 0)
#define SC27XX_MOD_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_SHIFT 8
+#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
+
#define SC27XX_LEDS_OFFSET 0x10
#define SC27XX_LEDS_MAX3
+#define SC27XX_LEDS_PATTERN_CNT4
struct sc27xx_led {
char name[LED_MAX_NAME_SIZE];
@@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum
led_brightness value)
return err;
}
+static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ struct regmap *regmap = leds->priv->regmap;
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ int err;
+
+ mutex_lock(>priv->lock);
+
+ /* Reset the rise, high, fall and low time to zero. */
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
+
+ err = regmap_update_bits(regmap, ctrl_base,
+ (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
+
+ mutex_unlock(>priv->lock);
+
+ return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+ struct led_pattern *pattern,
+ int len)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ struct regmap *regmap = leds->priv->regmap;
+ int err;
+
+ /*
+* Must contain 4 patterns to configure the rise time, high time, fall
+* time and low time to enable the breathing mode.
+*/
+ if (len != SC27XX_LEDS_PATTERN_CNT)
+ return -EINVAL;
+
+ mutex_lock(>priv->lock);
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_L_MASK, pattern[0].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_L_MASK, pattern[1].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_H_MASK,
+pattern[2].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_H_MASK,
+pattern[3].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> > >Please keep in mind that this is ABI documentation for the pattern file
> > >to be exposed by LED core, and not by the pattern trigger, that, as we
> > >agreed, will be implemented later. In this case, I'd go for
> >
> > Gosh, I got completely distracted by the recent discussion about
> > pattern synchronization.
> >
> > So, to recap, we need to decide if we are taking Baolin's solution
> > or we're opting for implementing pattern trigger.
> >
> > If we choose the latter, then we will also need some software
> > pattern engine in the trigger, to be applied as a software pattern
> > fallback for the devices without hardware pattern support.
> > It will certainly delay the contribution process, provided that Baolin
> > would find time for this work at all.
>
> I'd recommend the latter. Yes, software pattern as a fallback would be
> nice, but I have that code already... let me get it back to running
> state, and figure out where to add interface for "hardware
> acceleration". I'd like to have same interface to userland, whether
> pattern can be done by hardware or by software.
For the record, I'd like something like this. (Software pattern should
work. Hardware pattern... needs more work).
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index ede4fa0..8cf5962 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -51,6 +51,8 @@ static void led_timer_function(struct timer_list *t)
unsigned long brightness;
unsigned long delay;
+ /* FIXME spin_lock(led_cdev->lock); protecting led_cdev->flags? */
+
if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
led_set_brightness_nosleep(led_cdev, LED_OFF);
clear_bit(LED_BLINK_SW, _cdev->work_flags);
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..898f92d 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
/* PMIC global control register definition */
@@ -32,8 +33,13 @@
#define SC27XX_DUTY_MASK GENMASK(15, 0)
#define SC27XX_MOD_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_SHIFT 8
+#define SC27XX_CURVE_L_MASKGENMASK(7, 0)
+#define SC27XX_CURVE_H_MASKGENMASK(15, 8)
+
#define SC27XX_LEDS_OFFSET 0x10
#define SC27XX_LEDS_MAX3
+#define SC27XX_LEDS_PATTERN_CNT4
struct sc27xx_led {
char name[LED_MAX_NAME_SIZE];
@@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum
led_brightness value)
return err;
}
+static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ struct regmap *regmap = leds->priv->regmap;
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ int err;
+
+ mutex_lock(>priv->lock);
+
+ /* Reset the rise, high, fall and low time to zero. */
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
+
+ err = regmap_update_bits(regmap, ctrl_base,
+ (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
+
+ mutex_unlock(>priv->lock);
+
+ return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+ struct led_pattern *pattern,
+ int len)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ struct regmap *regmap = leds->priv->regmap;
+ int err;
+
+ /*
+* Must contain 4 patterns to configure the rise time, high time, fall
+* time and low time to enable the breathing mode.
+*/
+ if (len != SC27XX_LEDS_PATTERN_CNT)
+ return -EINVAL;
+
+ mutex_lock(>priv->lock);
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_L_MASK, pattern[0].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_L_MASK, pattern[1].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+SC27XX_CURVE_H_MASK,
+pattern[2].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+SC27XX_CURVE_H_MASK,
+pattern[3].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi! > >Please keep in mind that this is ABI documentation for the pattern file > >to be exposed by LED core, and not by the pattern trigger, that, as we > >agreed, will be implemented later. In this case, I'd go for > > Gosh, I got completely distracted by the recent discussion about > pattern synchronization. > > So, to recap, we need to decide if we are taking Baolin's solution > or we're opting for implementing pattern trigger. > > If we choose the latter, then we will also need some software > pattern engine in the trigger, to be applied as a software pattern > fallback for the devices without hardware pattern support. > It will certainly delay the contribution process, provided that Baolin > would find time for this work at all. I'd recommend the latter. Yes, software pattern as a fallback would be nice, but I have that code already... let me get it back to running state, and figure out where to add interface for "hardware acceleration". I'd like to have same interface to userland, whether pattern can be done by hardware or by software. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi! > >Please keep in mind that this is ABI documentation for the pattern file > >to be exposed by LED core, and not by the pattern trigger, that, as we > >agreed, will be implemented later. In this case, I'd go for > > Gosh, I got completely distracted by the recent discussion about > pattern synchronization. > > So, to recap, we need to decide if we are taking Baolin's solution > or we're opting for implementing pattern trigger. > > If we choose the latter, then we will also need some software > pattern engine in the trigger, to be applied as a software pattern > fallback for the devices without hardware pattern support. > It will certainly delay the contribution process, provided that Baolin > would find time for this work at all. I'd recommend the latter. Yes, software pattern as a fallback would be nice, but I have that code already... let me get it back to running state, and figure out where to add interface for "hardware acceleration". I'd like to have same interface to userland, whether pattern can be done by hardware or by software. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/18/2018 02:22 PM, Jacek Anaszewski wrote:
On 07/18/2018 08:54 PM, Jacek Anaszewski wrote:
On 07/18/2018 09:56 AM, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
Please keep in mind that this is ABI documentation for the pattern file
to be exposed by LED core, and not by the pattern trigger, that, as we
agreed, will be implemented later. In this case, I'd go for
Gosh, I got completely distracted by the recent discussion about
pattern synchronization.
So, to recap, we need to decide if we are taking Baolin's solution
or we're opting for implementing pattern trigger.
I think we should take Baolin's solution.
If we choose the latter, then we will also need some software
pattern engine in the trigger, to be applied as a software pattern
fallback for the devices without hardware pattern support.
It will certainly delay the contribution process, provided that Baolin
would find time for this work at all.
I'd just take v5 based solution for now (with improved semantics
of disabling pattern - in this case my reasoning from the message
I'm replying to is still valid),
"echo 0 > brightness" as a command disabling pattern. The same operation
disables triggers, so later transition to using pattern trigger will be
seamless for userspace.
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/18/2018 02:22 PM, Jacek Anaszewski wrote:
On 07/18/2018 08:54 PM, Jacek Anaszewski wrote:
On 07/18/2018 09:56 AM, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
Please keep in mind that this is ABI documentation for the pattern file
to be exposed by LED core, and not by the pattern trigger, that, as we
agreed, will be implemented later. In this case, I'd go for
Gosh, I got completely distracted by the recent discussion about
pattern synchronization.
So, to recap, we need to decide if we are taking Baolin's solution
or we're opting for implementing pattern trigger.
I think we should take Baolin's solution.
If we choose the latter, then we will also need some software
pattern engine in the trigger, to be applied as a software pattern
fallback for the devices without hardware pattern support.
It will certainly delay the contribution process, provided that Baolin
would find time for this work at all.
I'd just take v5 based solution for now (with improved semantics
of disabling pattern - in this case my reasoning from the message
I'm replying to is still valid),
"echo 0 > brightness" as a command disabling pattern. The same operation
disables triggers, so later transition to using pattern trigger will be
seamless for userspace.
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/18/2018 08:54 PM, Jacek Anaszewski wrote:
On 07/18/2018 09:56 AM, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to
events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how
ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate
the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
Please keep in mind that this is ABI documentation for the pattern file
to be exposed by LED core, and not by the pattern trigger, that, as we
agreed, will be implemented later. In this case, I'd go for
Gosh, I got completely distracted by the recent discussion about
pattern synchronization.
So, to recap, we need to decide if we are taking Baolin's solution
or we're opting for implementing pattern trigger.
If we choose the latter, then we will also need some software
pattern engine in the trigger, to be applied as a software pattern
fallback for the devices without hardware pattern support.
It will certainly delay the contribution process, provided that Baolin
would find time for this work at all.
I'd just take v5 based solution for now (with improved semantics
of disabling pattern - in this case my reasoning from the message
I'm replying to is still valid),
"echo 0 > brightness" as a command disabling pattern. The same operation
disables triggers, so later transition to using pattern trigger will be
seamless for userspace.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/18/2018 08:54 PM, Jacek Anaszewski wrote:
On 07/18/2018 09:56 AM, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to
events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how
ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate
the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
Please keep in mind that this is ABI documentation for the pattern file
to be exposed by LED core, and not by the pattern trigger, that, as we
agreed, will be implemented later. In this case, I'd go for
Gosh, I got completely distracted by the recent discussion about
pattern synchronization.
So, to recap, we need to decide if we are taking Baolin's solution
or we're opting for implementing pattern trigger.
If we choose the latter, then we will also need some software
pattern engine in the trigger, to be applied as a software pattern
fallback for the devices without hardware pattern support.
It will certainly delay the contribution process, provided that Baolin
would find time for this work at all.
I'd just take v5 based solution for now (with improved semantics
of disabling pattern - in this case my reasoning from the message
I'm replying to is still valid),
"echo 0 > brightness" as a command disabling pattern. The same operation
disables triggers, so later transition to using pattern trigger will be
seamless for userspace.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/18/2018 09:56 AM, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
Please keep in mind that this is ABI documentation for the pattern file
to be exposed by LED core, and not by the pattern trigger, that, as we
agreed, will be implemented later. In this case, I'd go for
"echo 0 > brightness" as a command disabling pattern. The same operation
disables triggers, so later transition to using pattern trigger will be
seamless for userspace.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/18/2018 09:56 AM, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
Please keep in mind that this is ABI documentation for the pattern file
to be exposed by LED core, and not by the pattern trigger, that, as we
agreed, will be implemented later. In this case, I'd go for
"echo 0 > brightness" as a command disabling pattern. The same operation
disables triggers, so later transition to using pattern trigger will be
seamless for userspace.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 7/18/18 7:08 AM, Pavel Machek wrote:
On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
On 18 July 2018 at 15:56, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
For the case we met in patch 2, it is not related with trigger things.
We just set some series of tuples including brightness and duration
(ms) to the hardware to enable the breath mode of the LED, we did not
trigger anything. So it is weird to write something to trigger file to
deactive the pattern.
Confused. I thought that "breathing mode" would be handled similar way
to hardware blinking: userland selects pattern trigger, pattern file
appears (similar way to delay_on/delay_off files with blinking), he
configures it, hardware brightness follows the pattern ("breathing
mode"). If pattern is no longer required, echo none > trigger stops
it.
Pavel
I was confused too when I first read this thread. But after reviewing
v5, it is clear that this is _not_ a trigger (and it should not be a
trigger). This is basically the equivalent the brightness attribute -
except that now the brightness changes over time instead of being a
constant value. This way, the pattern can be used in conjunction with
triggers.
For example, one might want to set the _pattern_ to something like a
"breathe" pattern and set the _trigger_ to the power supply charging
full trigger. This way, the LED will be off until the battery is full
and then the LED will "breath" when the battery is full.
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 7/18/18 7:08 AM, Pavel Machek wrote:
On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
On 18 July 2018 at 15:56, Pavel Machek wrote:
Hi!
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
For the case we met in patch 2, it is not related with trigger things.
We just set some series of tuples including brightness and duration
(ms) to the hardware to enable the breath mode of the LED, we did not
trigger anything. So it is weird to write something to trigger file to
deactive the pattern.
Confused. I thought that "breathing mode" would be handled similar way
to hardware blinking: userland selects pattern trigger, pattern file
appears (similar way to delay_on/delay_off files with blinking), he
configures it, hardware brightness follows the pattern ("breathing
mode"). If pattern is no longer required, echo none > trigger stops
it.
Pavel
I was confused too when I first read this thread. But after reviewing
v5, it is clear that this is _not_ a trigger (and it should not be a
trigger). This is basically the equivalent the brightness attribute -
except that now the brightness changes over time instead of being a
constant value. This way, the pattern can be used in conjunction with
triggers.
For example, one might want to set the _pattern_ to something like a
"breathe" pattern and set the _trigger_ to the power supply charging
full trigger. This way, the LED will be off until the battery is full
and then the LED will "breath" when the battery is full.
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
> On 18 July 2018 at 15:56, Pavel Machek wrote:
> > Hi!
> >
> >> I believe I meant "changing patterns from kernel in response to events
> >> is probably overkill"... or something like that.
> >> >>>
> >> >>>Anyway -- to clean up the confusion -- I'd like to see
> >> >>>
> >> >>>echo pattern > trigger
> >> >>>echo "1 2 3 4 5 6 7 8" > somewhere
> >> >>
> >> >>s/somewhere/pattern/
> >> >>
> >> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> >> >>creates delay_{on|off} files.
> >
> > Yes, that sounds reasonable. v5 still says
> >
> > + Writing non-empty string to this file will activate the
> > pattern,
> > + and empty string will disable the pattern.
> >
> > I'd deactivate the pattern by simply writing something else to the
> > trigger file.
>
> For the case we met in patch 2, it is not related with trigger things.
> We just set some series of tuples including brightness and duration
> (ms) to the hardware to enable the breath mode of the LED, we did not
> trigger anything. So it is weird to write something to trigger file to
> deactive the pattern.
Confused. I thought that "breathing mode" would be handled similar way
to hardware blinking: userland selects pattern trigger, pattern file
appears (similar way to delay_on/delay_off files with blinking), he
configures it, hardware brightness follows the pattern ("breathing
mode"). If pattern is no longer required, echo none > trigger stops
it.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
> On 18 July 2018 at 15:56, Pavel Machek wrote:
> > Hi!
> >
> >> I believe I meant "changing patterns from kernel in response to events
> >> is probably overkill"... or something like that.
> >> >>>
> >> >>>Anyway -- to clean up the confusion -- I'd like to see
> >> >>>
> >> >>>echo pattern > trigger
> >> >>>echo "1 2 3 4 5 6 7 8" > somewhere
> >> >>
> >> >>s/somewhere/pattern/
> >> >>
> >> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> >> >>creates delay_{on|off} files.
> >
> > Yes, that sounds reasonable. v5 still says
> >
> > + Writing non-empty string to this file will activate the
> > pattern,
> > + and empty string will disable the pattern.
> >
> > I'd deactivate the pattern by simply writing something else to the
> > trigger file.
>
> For the case we met in patch 2, it is not related with trigger things.
> We just set some series of tuples including brightness and duration
> (ms) to the hardware to enable the breath mode of the LED, we did not
> trigger anything. So it is weird to write something to trigger file to
> deactive the pattern.
Confused. I thought that "breathing mode" would be handled similar way
to hardware blinking: userland selects pattern trigger, pattern file
appears (similar way to delay_on/delay_off files with blinking), he
configures it, hardware brightness follows the pattern ("breathing
mode"). If pattern is no longer required, echo none > trigger stops
it.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 18 July 2018 at 15:56, Pavel Machek wrote:
> Hi!
>
>> I believe I meant "changing patterns from kernel in response to events
>> is probably overkill"... or something like that.
>> >>>
>> >>>Anyway -- to clean up the confusion -- I'd like to see
>> >>>
>> >>>echo pattern > trigger
>> >>>echo "1 2 3 4 5 6 7 8" > somewhere
>> >>
>> >>s/somewhere/pattern/
>> >>
>> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
>> >>creates delay_{on|off} files.
>
> Yes, that sounds reasonable. v5 still says
>
> + Writing non-empty string to this file will activate the
> pattern,
> + and empty string will disable the pattern.
>
> I'd deactivate the pattern by simply writing something else to the
> trigger file.
For the case we met in patch 2, it is not related with trigger things.
We just set some series of tuples including brightness and duration
(ms) to the hardware to enable the breath mode of the LED, we did not
trigger anything. So it is weird to write something to trigger file to
deactive the pattern.
--
Baolin Wang
Best Regards
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 18 July 2018 at 15:56, Pavel Machek wrote:
> Hi!
>
>> I believe I meant "changing patterns from kernel in response to events
>> is probably overkill"... or something like that.
>> >>>
>> >>>Anyway -- to clean up the confusion -- I'd like to see
>> >>>
>> >>>echo pattern > trigger
>> >>>echo "1 2 3 4 5 6 7 8" > somewhere
>> >>
>> >>s/somewhere/pattern/
>> >>
>> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
>> >>creates delay_{on|off} files.
>
> Yes, that sounds reasonable. v5 still says
>
> + Writing non-empty string to this file will activate the
> pattern,
> + and empty string will disable the pattern.
>
> I'd deactivate the pattern by simply writing something else to the
> trigger file.
For the case we met in patch 2, it is not related with trigger things.
We just set some series of tuples including brightness and duration
(ms) to the hardware to enable the breath mode of the LED, we did not
trigger anything. So it is weird to write something to trigger file to
deactive the pattern.
--
Baolin Wang
Best Regards
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> I believe I meant "changing patterns from kernel in response to events
> is probably overkill"... or something like that.
> >>>
> >>>Anyway -- to clean up the confusion -- I'd like to see
> >>>
> >>>echo pattern > trigger
> >>>echo "1 2 3 4 5 6 7 8" > somewhere
> >>
> >>s/somewhere/pattern/
> >>
> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> >>creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> I believe I meant "changing patterns from kernel in response to events
> is probably overkill"... or something like that.
> >>>
> >>>Anyway -- to clean up the confusion -- I'd like to see
> >>>
> >>>echo pattern > trigger
> >>>echo "1 2 3 4 5 6 7 8" > somewhere
> >>
> >>s/somewhere/pattern/
> >>
> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> >>creates delay_{on|off} files.
Yes, that sounds reasonable. v5 still says
+ Writing non-empty string to this file will activate the pattern,
+ and empty string will disable the pattern.
I'd deactivate the pattern by simply writing something else to the
trigger file.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi! > >>- different hardware means via which brightness is set (MMIO, I2C, SPI, > >> PWM and other pulsed fashion based protocols), > >>- the need for deferring brightness setting to a workqueue task to > >> allow for setting LED brightness from atomic context, > >>- contention on locks > > > >I disagree here. Yes, it would be hard to synchronize blinking down to > >microseconds, but it would be easy to get synchronization right down > >to miliseconds and humans will not be able to tell the difference. > > There have been problems with blink interval stability close to > 1ms, and thus there were some attempts of employing hr timers, > which in turn introduced a new class of issues related to > system performance etc. Yeah, well. This is LED subsystem. Noone should program blink intervals at 1 msec. > >>For the LEDs driven by the same chip it would make more sense > >>to allow for synchronization, but it can be achieved on driver > >>level, with help of some subsystem level interface to indicate > >>which LEDs should be synchronized. > >> > >>However, when we start to pretend that we can synchronize the > >>devices, we must answer how accurate we can be. The accuracy > >>will decrease as blink frequency rises. We'd need to define > >>reliability limit. > > > >We don't need _that_ ammount of overengineering. We just need to > >synchronize them well enough :-). > > Well, it would be disappointing for the users to realize that > they don't get the effect advertised by the ABI documentation. Linux is always best-effort, w.r.t. timing. And we can do well enough that user will not see anything bad on "normal" systems. > >>We've had few attempts of approaching the subject of synchronized > >>blinking but none of them proved to be good enough to be merged. > > > >I'm sure interested person could do something like that in less than > >two weeks fulltime... It is not rocket science, just a lot of work in > >kernel... > > > >But patterns are few years overdue and I believe we should not delay > >them any further. > > > >So... I guess I agree with Jacek in the end :-). > > How about taking Baolin's patches as of v5? Later, provided that > the pattern trigger yet to be implemented will create pattern file > on activation, we'll need to initialize default-trigger DT property, > to keep the interface unchanged. I have yet to look at the v5 of patches. But I agree that we do not need to design synchronization at this moment. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi! > >>- different hardware means via which brightness is set (MMIO, I2C, SPI, > >> PWM and other pulsed fashion based protocols), > >>- the need for deferring brightness setting to a workqueue task to > >> allow for setting LED brightness from atomic context, > >>- contention on locks > > > >I disagree here. Yes, it would be hard to synchronize blinking down to > >microseconds, but it would be easy to get synchronization right down > >to miliseconds and humans will not be able to tell the difference. > > There have been problems with blink interval stability close to > 1ms, and thus there were some attempts of employing hr timers, > which in turn introduced a new class of issues related to > system performance etc. Yeah, well. This is LED subsystem. Noone should program blink intervals at 1 msec. > >>For the LEDs driven by the same chip it would make more sense > >>to allow for synchronization, but it can be achieved on driver > >>level, with help of some subsystem level interface to indicate > >>which LEDs should be synchronized. > >> > >>However, when we start to pretend that we can synchronize the > >>devices, we must answer how accurate we can be. The accuracy > >>will decrease as blink frequency rises. We'd need to define > >>reliability limit. > > > >We don't need _that_ ammount of overengineering. We just need to > >synchronize them well enough :-). > > Well, it would be disappointing for the users to realize that > they don't get the effect advertised by the ABI documentation. Linux is always best-effort, w.r.t. timing. And we can do well enough that user will not see anything bad on "normal" systems. > >>We've had few attempts of approaching the subject of synchronized > >>blinking but none of them proved to be good enough to be merged. > > > >I'm sure interested person could do something like that in less than > >two weeks fulltime... It is not rocket science, just a lot of work in > >kernel... > > > >But patterns are few years overdue and I believe we should not delay > >them any further. > > > >So... I guess I agree with Jacek in the end :-). > > How about taking Baolin's patches as of v5? Later, provided that > the pattern trigger yet to be implemented will create pattern file > on activation, we'll need to initialize default-trigger DT property, > to keep the interface unchanged. I have yet to look at the v5 of patches. But I agree that we do not need to design synchronization at this moment. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/16/2018 11:56 PM, Pavel Machek wrote:
Hi!
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
I don't think this is the best way. For example, if you want more than one
LED to have the same pattern, then the patterns will not be synchronized
between the LEDs. The same things happens now with many of the existing
triggers. For example, if I have two LEDs side-by-side using the heartbeat
trigger, they may blink at the same time or they may not, which is not
very nice. I think we can make something better.
Yes, synchronization would be nice -- it is really a must for RGB LEDs
-- but I believe it is too much to ask from Baolin at the moment.
It is virtually impossible to enforce synchronous blinking for the
LEDs driven by different hardware due to:
- different hardware means via which brightness is set (MMIO, I2C, SPI,
PWM and other pulsed fashion based protocols),
- the need for deferring brightness setting to a workqueue task to
allow for setting LED brightness from atomic context,
- contention on locks
I disagree here. Yes, it would be hard to synchronize blinking down to
microseconds, but it would be easy to get synchronization right down
to miliseconds and humans will not be able to tell the difference.
There have been problems with blink interval stability close to
1ms, and thus there were some attempts of employing hr timers,
which in turn introduced a new class of issues related to
system performance etc.
For the LEDs driven by the same chip it would make more sense
to allow for synchronization, but it can be achieved on driver
level, with help of some subsystem level interface to indicate
which LEDs should be synchronized.
However, when we start to pretend that we can synchronize the
devices, we must answer how accurate we can be. The accuracy
will decrease as blink frequency rises. We'd need to define
reliability limit.
We don't need _that_ ammount of overengineering. We just need to
synchronize them well enough :-).
Well, it would be disappointing for the users to realize that
they don't get the effect advertised by the ABI documentation.
We've had few attempts of approaching the subject of synchronized
blinking but none of them proved to be good enough to be merged.
I'm sure interested person could do something like that in less than
two weeks fulltime... It is not rocket science, just a lot of work in
kernel...
But patterns are few years overdue and I believe we should not delay
them any further.
So... I guess I agree with Jacek in the end :-).
How about taking Baolin's patches as of v5? Later, provided that
the pattern trigger yet to be implemented will create pattern file
on activation, we'll need to initialize default-trigger DT property,
to keep the interface unchanged.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/16/2018 11:56 PM, Pavel Machek wrote:
Hi!
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
I don't think this is the best way. For example, if you want more than one
LED to have the same pattern, then the patterns will not be synchronized
between the LEDs. The same things happens now with many of the existing
triggers. For example, if I have two LEDs side-by-side using the heartbeat
trigger, they may blink at the same time or they may not, which is not
very nice. I think we can make something better.
Yes, synchronization would be nice -- it is really a must for RGB LEDs
-- but I believe it is too much to ask from Baolin at the moment.
It is virtually impossible to enforce synchronous blinking for the
LEDs driven by different hardware due to:
- different hardware means via which brightness is set (MMIO, I2C, SPI,
PWM and other pulsed fashion based protocols),
- the need for deferring brightness setting to a workqueue task to
allow for setting LED brightness from atomic context,
- contention on locks
I disagree here. Yes, it would be hard to synchronize blinking down to
microseconds, but it would be easy to get synchronization right down
to miliseconds and humans will not be able to tell the difference.
There have been problems with blink interval stability close to
1ms, and thus there were some attempts of employing hr timers,
which in turn introduced a new class of issues related to
system performance etc.
For the LEDs driven by the same chip it would make more sense
to allow for synchronization, but it can be achieved on driver
level, with help of some subsystem level interface to indicate
which LEDs should be synchronized.
However, when we start to pretend that we can synchronize the
devices, we must answer how accurate we can be. The accuracy
will decrease as blink frequency rises. We'd need to define
reliability limit.
We don't need _that_ ammount of overengineering. We just need to
synchronize them well enough :-).
Well, it would be disappointing for the users to realize that
they don't get the effect advertised by the ABI documentation.
We've had few attempts of approaching the subject of synchronized
blinking but none of them proved to be good enough to be merged.
I'm sure interested person could do something like that in less than
two weeks fulltime... It is not rocket science, just a lot of work in
kernel...
But patterns are few years overdue and I believe we should not delay
them any further.
So... I guess I agree with Jacek in the end :-).
How about taking Baolin's patches as of v5? Later, provided that
the pattern trigger yet to be implemented will create pattern file
on activation, we'll need to initialize default-trigger DT property,
to keep the interface unchanged.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> >>>echo pattern > trigger
> >>>echo "1 2 3 4 5 6 7 8" > somewhere
> >>
> >>s/somewhere/pattern/
> >>
> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> >>creates delay_{on|off} files.
> >>
> >
> >I don't think this is the best way. For example, if you want more than one
> >LED to have the same pattern, then the patterns will not be synchronized
> >between the LEDs. The same things happens now with many of the existing
> >triggers. For example, if I have two LEDs side-by-side using the heartbeat
> >trigger, they may blink at the same time or they may not, which is not
> >very nice. I think we can make something better.
Yes, synchronization would be nice -- it is really a must for RGB LEDs
-- but I believe it is too much to ask from Baolin at the moment.
> It is virtually impossible to enforce synchronous blinking for the
> LEDs driven by different hardware due to:
>
> - different hardware means via which brightness is set (MMIO, I2C, SPI,
> PWM and other pulsed fashion based protocols),
> - the need for deferring brightness setting to a workqueue task to
> allow for setting LED brightness from atomic context,
> - contention on locks
I disagree here. Yes, it would be hard to synchronize blinking down to
microseconds, but it would be easy to get synchronization right down
to miliseconds and humans will not be able to tell the difference.
> For the LEDs driven by the same chip it would make more sense
> to allow for synchronization, but it can be achieved on driver
> level, with help of some subsystem level interface to indicate
> which LEDs should be synchronized.
>
> However, when we start to pretend that we can synchronize the
> devices, we must answer how accurate we can be. The accuracy
> will decrease as blink frequency rises. We'd need to define
> reliability limit.
We don't need _that_ ammount of overengineering. We just need to
synchronize them well enough :-).
> We've had few attempts of approaching the subject of synchronized
> blinking but none of them proved to be good enough to be merged.
I'm sure interested person could do something like that in less than
two weeks fulltime... It is not rocket science, just a lot of work in
kernel...
But patterns are few years overdue and I believe we should not delay
them any further.
So... I guess I agree with Jacek in the end :-).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi!
> >>>echo pattern > trigger
> >>>echo "1 2 3 4 5 6 7 8" > somewhere
> >>
> >>s/somewhere/pattern/
> >>
> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> >>creates delay_{on|off} files.
> >>
> >
> >I don't think this is the best way. For example, if you want more than one
> >LED to have the same pattern, then the patterns will not be synchronized
> >between the LEDs. The same things happens now with many of the existing
> >triggers. For example, if I have two LEDs side-by-side using the heartbeat
> >trigger, they may blink at the same time or they may not, which is not
> >very nice. I think we can make something better.
Yes, synchronization would be nice -- it is really a must for RGB LEDs
-- but I believe it is too much to ask from Baolin at the moment.
> It is virtually impossible to enforce synchronous blinking for the
> LEDs driven by different hardware due to:
>
> - different hardware means via which brightness is set (MMIO, I2C, SPI,
> PWM and other pulsed fashion based protocols),
> - the need for deferring brightness setting to a workqueue task to
> allow for setting LED brightness from atomic context,
> - contention on locks
I disagree here. Yes, it would be hard to synchronize blinking down to
microseconds, but it would be easy to get synchronization right down
to miliseconds and humans will not be able to tell the difference.
> For the LEDs driven by the same chip it would make more sense
> to allow for synchronization, but it can be achieved on driver
> level, with help of some subsystem level interface to indicate
> which LEDs should be synchronized.
>
> However, when we start to pretend that we can synchronize the
> devices, we must answer how accurate we can be. The accuracy
> will decrease as blink frequency rises. We'd need to define
> reliability limit.
We don't need _that_ ammount of overengineering. We just need to
synchronize them well enough :-).
> We've had few attempts of approaching the subject of synchronized
> blinking but none of them proved to be good enough to be merged.
I'm sure interested person could do something like that in less than
two weeks fulltime... It is not rocket science, just a lot of work in
kernel...
But patterns are few years overdue and I believe we should not delay
them any further.
So... I guess I agree with Jacek in the end :-).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi David,
On 07/16/2018 03:00 AM, David Lechner wrote:
On 07/15/2018 07:22 AM, Jacek Anaszewski wrote:
On 07/15/2018 12:39 AM, Pavel Machek wrote:
On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
Hi Pavel,
On 07/14/2018 11:20 PM, Pavel Machek wrote:
Hi!
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your
implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
Let me take a step back: we have triggers.. like LED blinking.
How is that going to interact with patterns? We probably want the
patterns to be ignored in that case...?
Which suggest to me that we should treat patterns as a trigger. I
believe we do something similar with blinking already.
Then it is easy to determine if pattern is active, and pattern
vs. trigger issue is solved automatically.
I'm all for it. I proposed this approach during the previous
discussions related to possible pattern interface implementations,
but you seemed not to be so enthusiastic in [0].
[0] https://lkml.org/lkml/2017/4/7/350
Hmm. Reading my own email now, I can't decipher it.
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
I don't think this is the best way. For example, if you want more than one
LED to have the same pattern, then the patterns will not be synchronized
between the LEDs. The same things happens now with many of the existing
triggers. For example, if I have two LEDs side-by-side using the heartbeat
trigger, they may blink at the same time or they may not, which is not
very nice. I think we can make something better.
It is virtually impossible to enforce synchronous blinking for the
LEDs driven by different hardware due to:
- different hardware means via which brightness is set (MMIO, I2C, SPI,
PWM and other pulsed fashion based protocols),
- the need for deferring brightness setting to a workqueue task to
allow for setting LED brightness from atomic context,
- contention on locks
For the LEDs driven by the same chip it would make more sense
to allow for synchronization, but it can be achieved on driver
level, with help of some subsystem level interface to indicate
which LEDs should be synchronized.
However, when we start to pretend that we can synchronize the
devices, we must answer how accurate we can be. The accuracy
will decrease as blink frequency rises. We'd need to define
reliability limit.
For different devices, this limit will be different, and it will also
depend on the CPU speed.
We've had few attempts of approaching the subject of synchronized
blinking but none of them proved to be good enough to be merged.
Frankly speaking I doubt it is good task for the system like Linux.
Perhaps a way to do this would be to use configfs to create a pattern
trigger that can be shared by multiple LEDs. Like this:
mkdir /sys/kernel/config/leds/triggers/my-nice-pattern
echo "1 2 3 4" >
/sys/kernel/config/leds/triggers/my-nice-pattern/pattern
echo my-nice-pattern > /sys/class/leds/led0/trigger
echo my-nice-pattern > /sys/class/leds/led1/trigger
Please CC me on any future revisions of this series. I would like to
test it.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi David,
On 07/16/2018 03:00 AM, David Lechner wrote:
On 07/15/2018 07:22 AM, Jacek Anaszewski wrote:
On 07/15/2018 12:39 AM, Pavel Machek wrote:
On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
Hi Pavel,
On 07/14/2018 11:20 PM, Pavel Machek wrote:
Hi!
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your
implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
Let me take a step back: we have triggers.. like LED blinking.
How is that going to interact with patterns? We probably want the
patterns to be ignored in that case...?
Which suggest to me that we should treat patterns as a trigger. I
believe we do something similar with blinking already.
Then it is easy to determine if pattern is active, and pattern
vs. trigger issue is solved automatically.
I'm all for it. I proposed this approach during the previous
discussions related to possible pattern interface implementations,
but you seemed not to be so enthusiastic in [0].
[0] https://lkml.org/lkml/2017/4/7/350
Hmm. Reading my own email now, I can't decipher it.
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
I don't think this is the best way. For example, if you want more than one
LED to have the same pattern, then the patterns will not be synchronized
between the LEDs. The same things happens now with many of the existing
triggers. For example, if I have two LEDs side-by-side using the heartbeat
trigger, they may blink at the same time or they may not, which is not
very nice. I think we can make something better.
It is virtually impossible to enforce synchronous blinking for the
LEDs driven by different hardware due to:
- different hardware means via which brightness is set (MMIO, I2C, SPI,
PWM and other pulsed fashion based protocols),
- the need for deferring brightness setting to a workqueue task to
allow for setting LED brightness from atomic context,
- contention on locks
For the LEDs driven by the same chip it would make more sense
to allow for synchronization, but it can be achieved on driver
level, with help of some subsystem level interface to indicate
which LEDs should be synchronized.
However, when we start to pretend that we can synchronize the
devices, we must answer how accurate we can be. The accuracy
will decrease as blink frequency rises. We'd need to define
reliability limit.
For different devices, this limit will be different, and it will also
depend on the CPU speed.
We've had few attempts of approaching the subject of synchronized
blinking but none of them proved to be good enough to be merged.
Frankly speaking I doubt it is good task for the system like Linux.
Perhaps a way to do this would be to use configfs to create a pattern
trigger that can be shared by multiple LEDs. Like this:
mkdir /sys/kernel/config/leds/triggers/my-nice-pattern
echo "1 2 3 4" >
/sys/kernel/config/leds/triggers/my-nice-pattern/pattern
echo my-nice-pattern > /sys/class/leds/led0/trigger
echo my-nice-pattern > /sys/class/leds/led1/trigger
Please CC me on any future revisions of this series. I would like to
test it.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 15 July 2018 at 20:22, Jacek Anaszewski wrote:
> On 07/15/2018 12:39 AM, Pavel Machek wrote:
>>
>> On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
>>>
>>> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
Hi Pavel,
On 07/14/2018 11:20 PM, Pavel Machek wrote:
>
> Hi!
>
>>> It also drew my attention to the issue of desired pattern sysfs
>>> interface semantics on uninitialized pattern. In your implementation
>>> user seems to be unable to determine if the pattern is activated
>>> or not. We should define the semantics for this use case and
>>> describe it in the documentation. Possibly pattern could
>>> return alone new line character then.
>
>
> Let me take a step back: we have triggers.. like LED blinking.
>
> How is that going to interact with patterns? We probably want the
> patterns to be ignored in that case...?
>
> Which suggest to me that we should treat patterns as a trigger. I
> believe we do something similar with blinking already.
>
> Then it is easy to determine if pattern is active, and pattern
> vs. trigger issue is solved automatically.
I'm all for it. I proposed this approach during the previous
discussions related to possible pattern interface implementations,
but you seemed not to be so enthusiastic in [0].
[0] https://lkml.org/lkml/2017/4/7/350
>>>
>>>
>>> Hmm. Reading my own email now, I can't decipher it.
>>>
>>> I believe I meant "changing patterns from kernel in response to events
>>> is probably overkill"... or something like that.
>>
>>
>> Anyway -- to clean up the confusion -- I'd like to see
>>
>> echo pattern > trigger
>> echo "1 2 3 4 5 6 7 8" > somewhere
>
>
> s/somewhere/pattern/
>
> pattern trigger should create "pattern" file similarly how ledtrig-timer
> creates delay_{on|off} files.
Yes. Anyway, I will submit V5 patchset with addressing previous
comments, but did not include pattern trigger issue.
--
Baolin Wang
Best Regards
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 15 July 2018 at 20:22, Jacek Anaszewski wrote:
> On 07/15/2018 12:39 AM, Pavel Machek wrote:
>>
>> On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
>>>
>>> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
Hi Pavel,
On 07/14/2018 11:20 PM, Pavel Machek wrote:
>
> Hi!
>
>>> It also drew my attention to the issue of desired pattern sysfs
>>> interface semantics on uninitialized pattern. In your implementation
>>> user seems to be unable to determine if the pattern is activated
>>> or not. We should define the semantics for this use case and
>>> describe it in the documentation. Possibly pattern could
>>> return alone new line character then.
>
>
> Let me take a step back: we have triggers.. like LED blinking.
>
> How is that going to interact with patterns? We probably want the
> patterns to be ignored in that case...?
>
> Which suggest to me that we should treat patterns as a trigger. I
> believe we do something similar with blinking already.
>
> Then it is easy to determine if pattern is active, and pattern
> vs. trigger issue is solved automatically.
I'm all for it. I proposed this approach during the previous
discussions related to possible pattern interface implementations,
but you seemed not to be so enthusiastic in [0].
[0] https://lkml.org/lkml/2017/4/7/350
>>>
>>>
>>> Hmm. Reading my own email now, I can't decipher it.
>>>
>>> I believe I meant "changing patterns from kernel in response to events
>>> is probably overkill"... or something like that.
>>
>>
>> Anyway -- to clean up the confusion -- I'd like to see
>>
>> echo pattern > trigger
>> echo "1 2 3 4 5 6 7 8" > somewhere
>
>
> s/somewhere/pattern/
>
> pattern trigger should create "pattern" file similarly how ledtrig-timer
> creates delay_{on|off} files.
Yes. Anyway, I will submit V5 patchset with addressing previous
comments, but did not include pattern trigger issue.
--
Baolin Wang
Best Regards
Re: Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/15/2018 07:22 AM, Jacek Anaszewski wrote:
On 07/15/2018 12:39 AM, Pavel Machek wrote:
On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
Hi Pavel,
On 07/14/2018 11:20 PM, Pavel Machek wrote:
Hi!
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
Let me take a step back: we have triggers.. like LED blinking.
How is that going to interact with patterns? We probably want the
patterns to be ignored in that case...?
Which suggest to me that we should treat patterns as a trigger. I
believe we do something similar with blinking already.
Then it is easy to determine if pattern is active, and pattern
vs. trigger issue is solved automatically.
I'm all for it. I proposed this approach during the previous
discussions related to possible pattern interface implementations,
but you seemed not to be so enthusiastic in [0].
[0] https://lkml.org/lkml/2017/4/7/350
Hmm. Reading my own email now, I can't decipher it.
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
I don't think this is the best way. For example, if you want more than one
LED to have the same pattern, then the patterns will not be synchronized
between the LEDs. The same things happens now with many of the existing
triggers. For example, if I have two LEDs side-by-side using the heartbeat
trigger, they may blink at the same time or they may not, which is not
very nice. I think we can make something better.
Perhaps a way to do this would be to use configfs to create a pattern
trigger that can be shared by multiple LEDs. Like this:
mkdir /sys/kernel/config/leds/triggers/my-nice-pattern
echo "1 2 3 4" > /sys/kernel/config/leds/triggers/my-nice-pattern/pattern
echo my-nice-pattern > /sys/class/leds/led0/trigger
echo my-nice-pattern > /sys/class/leds/led1/trigger
Please CC me on any future revisions of this series. I would like to test it.
Re: Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/15/2018 07:22 AM, Jacek Anaszewski wrote:
On 07/15/2018 12:39 AM, Pavel Machek wrote:
On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
Hi Pavel,
On 07/14/2018 11:20 PM, Pavel Machek wrote:
Hi!
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
Let me take a step back: we have triggers.. like LED blinking.
How is that going to interact with patterns? We probably want the
patterns to be ignored in that case...?
Which suggest to me that we should treat patterns as a trigger. I
believe we do something similar with blinking already.
Then it is easy to determine if pattern is active, and pattern
vs. trigger issue is solved automatically.
I'm all for it. I proposed this approach during the previous
discussions related to possible pattern interface implementations,
but you seemed not to be so enthusiastic in [0].
[0] https://lkml.org/lkml/2017/4/7/350
Hmm. Reading my own email now, I can't decipher it.
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
I don't think this is the best way. For example, if you want more than one
LED to have the same pattern, then the patterns will not be synchronized
between the LEDs. The same things happens now with many of the existing
triggers. For example, if I have two LEDs side-by-side using the heartbeat
trigger, they may blink at the same time or they may not, which is not
very nice. I think we can make something better.
Perhaps a way to do this would be to use configfs to create a pattern
trigger that can be shared by multiple LEDs. Like this:
mkdir /sys/kernel/config/leds/triggers/my-nice-pattern
echo "1 2 3 4" > /sys/kernel/config/leds/triggers/my-nice-pattern/pattern
echo my-nice-pattern > /sys/class/leds/led0/trigger
echo my-nice-pattern > /sys/class/leds/led1/trigger
Please CC me on any future revisions of this series. I would like to test it.
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/15/2018 12:39 AM, Pavel Machek wrote:
On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
Hi Pavel,
On 07/14/2018 11:20 PM, Pavel Machek wrote:
Hi!
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
Let me take a step back: we have triggers.. like LED blinking.
How is that going to interact with patterns? We probably want the
patterns to be ignored in that case...?
Which suggest to me that we should treat patterns as a trigger. I
believe we do something similar with blinking already.
Then it is easy to determine if pattern is active, and pattern
vs. trigger issue is solved automatically.
I'm all for it. I proposed this approach during the previous
discussions related to possible pattern interface implementations,
but you seemed not to be so enthusiastic in [0].
[0] https://lkml.org/lkml/2017/4/7/350
Hmm. Reading my own email now, I can't decipher it.
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On 07/15/2018 12:39 AM, Pavel Machek wrote:
On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
Hi Pavel,
On 07/14/2018 11:20 PM, Pavel Machek wrote:
Hi!
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
Let me take a step back: we have triggers.. like LED blinking.
How is that going to interact with patterns? We probably want the
patterns to be ignored in that case...?
Which suggest to me that we should treat patterns as a trigger. I
believe we do something similar with blinking already.
Then it is easy to determine if pattern is active, and pattern
vs. trigger issue is solved automatically.
I'm all for it. I proposed this approach during the previous
discussions related to possible pattern interface implementations,
but you seemed not to be so enthusiastic in [0].
[0] https://lkml.org/lkml/2017/4/7/350
Hmm. Reading my own email now, I can't decipher it.
I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.
Anyway -- to clean up the confusion -- I'd like to see
echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere
s/somewhere/pattern/
pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Sun 2018-07-15 00:29:25, Pavel Machek wrote: > On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: > > Hi Pavel, > > > > On 07/14/2018 11:20 PM, Pavel Machek wrote: > > >Hi! > > > > > >>>It also drew my attention to the issue of desired pattern sysfs > > >>>interface semantics on uninitialized pattern. In your implementation > > >>>user seems to be unable to determine if the pattern is activated > > >>>or not. We should define the semantics for this use case and > > >>>describe it in the documentation. Possibly pattern could > > >>>return alone new line character then. > > > > > >Let me take a step back: we have triggers.. like LED blinking. > > > > > >How is that going to interact with patterns? We probably want the > > >patterns to be ignored in that case...? > > > > > >Which suggest to me that we should treat patterns as a trigger. I > > >believe we do something similar with blinking already. > > > > > >Then it is easy to determine if pattern is active, and pattern > > >vs. trigger issue is solved automatically. > > > > I'm all for it. I proposed this approach during the previous > > discussions related to possible pattern interface implementations, > > but you seemed not to be so enthusiastic in [0]. > > > > [0] https://lkml.org/lkml/2017/4/7/350 > > Hmm. Reading my own email now, I can't decipher it. > > I believe I meant "changing patterns from kernel in response to events > is probably overkill"... or something like that. Anyway -- to clean up the confusion -- I'd like to see echo pattern > trigger echo "1 2 3 4 5 6 7 8" > somewhere to activate pattern, and echo none > trigger to stop it. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Sun 2018-07-15 00:29:25, Pavel Machek wrote: > On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: > > Hi Pavel, > > > > On 07/14/2018 11:20 PM, Pavel Machek wrote: > > >Hi! > > > > > >>>It also drew my attention to the issue of desired pattern sysfs > > >>>interface semantics on uninitialized pattern. In your implementation > > >>>user seems to be unable to determine if the pattern is activated > > >>>or not. We should define the semantics for this use case and > > >>>describe it in the documentation. Possibly pattern could > > >>>return alone new line character then. > > > > > >Let me take a step back: we have triggers.. like LED blinking. > > > > > >How is that going to interact with patterns? We probably want the > > >patterns to be ignored in that case...? > > > > > >Which suggest to me that we should treat patterns as a trigger. I > > >believe we do something similar with blinking already. > > > > > >Then it is easy to determine if pattern is active, and pattern > > >vs. trigger issue is solved automatically. > > > > I'm all for it. I proposed this approach during the previous > > discussions related to possible pattern interface implementations, > > but you seemed not to be so enthusiastic in [0]. > > > > [0] https://lkml.org/lkml/2017/4/7/350 > > Hmm. Reading my own email now, I can't decipher it. > > I believe I meant "changing patterns from kernel in response to events > is probably overkill"... or something like that. Anyway -- to clean up the confusion -- I'd like to see echo pattern > trigger echo "1 2 3 4 5 6 7 8" > somewhere to activate pattern, and echo none > trigger to stop it. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: > Hi Pavel, > > On 07/14/2018 11:20 PM, Pavel Machek wrote: > >Hi! > > > >>>It also drew my attention to the issue of desired pattern sysfs > >>>interface semantics on uninitialized pattern. In your implementation > >>>user seems to be unable to determine if the pattern is activated > >>>or not. We should define the semantics for this use case and > >>>describe it in the documentation. Possibly pattern could > >>>return alone new line character then. > > > >Let me take a step back: we have triggers.. like LED blinking. > > > >How is that going to interact with patterns? We probably want the > >patterns to be ignored in that case...? > > > >Which suggest to me that we should treat patterns as a trigger. I > >believe we do something similar with blinking already. > > > >Then it is easy to determine if pattern is active, and pattern > >vs. trigger issue is solved automatically. > > I'm all for it. I proposed this approach during the previous > discussions related to possible pattern interface implementations, > but you seemed not to be so enthusiastic in [0]. > > [0] https://lkml.org/lkml/2017/4/7/350 Hmm. Reading my own email now, I can't decipher it. I believe I meant "changing patterns from kernel in response to events is probably overkill"... or something like that. Sorry about confusion, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: > Hi Pavel, > > On 07/14/2018 11:20 PM, Pavel Machek wrote: > >Hi! > > > >>>It also drew my attention to the issue of desired pattern sysfs > >>>interface semantics on uninitialized pattern. In your implementation > >>>user seems to be unable to determine if the pattern is activated > >>>or not. We should define the semantics for this use case and > >>>describe it in the documentation. Possibly pattern could > >>>return alone new line character then. > > > >Let me take a step back: we have triggers.. like LED blinking. > > > >How is that going to interact with patterns? We probably want the > >patterns to be ignored in that case...? > > > >Which suggest to me that we should treat patterns as a trigger. I > >believe we do something similar with blinking already. > > > >Then it is easy to determine if pattern is active, and pattern > >vs. trigger issue is solved automatically. > > I'm all for it. I proposed this approach during the previous > discussions related to possible pattern interface implementations, > but you seemed not to be so enthusiastic in [0]. > > [0] https://lkml.org/lkml/2017/4/7/350 Hmm. Reading my own email now, I can't decipher it. I believe I meant "changing patterns from kernel in response to events is probably overkill"... or something like that. Sorry about confusion, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel, On 07/14/2018 11:20 PM, Pavel Machek wrote: Hi! It also drew my attention to the issue of desired pattern sysfs interface semantics on uninitialized pattern. In your implementation user seems to be unable to determine if the pattern is activated or not. We should define the semantics for this use case and describe it in the documentation. Possibly pattern could return alone new line character then. Let me take a step back: we have triggers.. like LED blinking. How is that going to interact with patterns? We probably want the patterns to be ignored in that case...? Which suggest to me that we should treat patterns as a trigger. I believe we do something similar with blinking already. Then it is easy to determine if pattern is active, and pattern vs. trigger issue is solved automatically. I'm all for it. I proposed this approach during the previous discussions related to possible pattern interface implementations, but you seemed not to be so enthusiastic in [0]. [0] https://lkml.org/lkml/2017/4/7/350 -- Best regards, Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Pavel, On 07/14/2018 11:20 PM, Pavel Machek wrote: Hi! It also drew my attention to the issue of desired pattern sysfs interface semantics on uninitialized pattern. In your implementation user seems to be unable to determine if the pattern is activated or not. We should define the semantics for this use case and describe it in the documentation. Possibly pattern could return alone new line character then. Let me take a step back: we have triggers.. like LED blinking. How is that going to interact with patterns? We probably want the patterns to be ignored in that case...? Which suggest to me that we should treat patterns as a trigger. I believe we do something similar with blinking already. Then it is easy to determine if pattern is active, and pattern vs. trigger issue is solved automatically. I'm all for it. I proposed this approach during the previous discussions related to possible pattern interface implementations, but you seemed not to be so enthusiastic in [0]. [0] https://lkml.org/lkml/2017/4/7/350 -- Best regards, Jacek Anaszewski
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi! > > It also drew my attention to the issue of desired pattern sysfs > > interface semantics on uninitialized pattern. In your implementation > > user seems to be unable to determine if the pattern is activated > > or not. We should define the semantics for this use case and > > describe it in the documentation. Possibly pattern could > > return alone new line character then. Let me take a step back: we have triggers.. like LED blinking. How is that going to interact with patterns? We probably want the patterns to be ignored in that case...? Which suggest to me that we should treat patterns as a trigger. I believe we do something similar with blinking already. Then it is easy to determine if pattern is active, and pattern vs. trigger issue is solved automatically. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi! > > It also drew my attention to the issue of desired pattern sysfs > > interface semantics on uninitialized pattern. In your implementation > > user seems to be unable to determine if the pattern is activated > > or not. We should define the semantics for this use case and > > describe it in the documentation. Possibly pattern could > > return alone new line character then. Let me take a step back: we have triggers.. like LED blinking. How is that going to interact with patterns? We probably want the patterns to be ignored in that case...? Which suggest to me that we should treat patterns as a trigger. I believe we do something similar with blinking already. Then it is easy to determine if pattern is active, and pattern vs. trigger issue is solved automatically. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Jacek,
On 13 July 2018 at 05:41, Jacek Anaszewski wrote:
> Hi Baolin,
>
>
> On 07/12/2018 02:24 PM, Baolin Wang wrote:
>>
>> Hi Jacek,
>>
>> On 12 July 2018 at 05:10, Jacek Anaszewski
>> wrote:
>>>
>>> Hi Baolin.
>>>
>>>
>>> On 07/11/2018 01:02 PM, Baolin Wang wrote:
Hi Jacek and Pavel,
On 29 June 2018 at 13:03, Baolin Wang wrote:
>
>
> From: Bjorn Andersson
>
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
>
> This adds a new optional operator that LED class drivers can implement
> if they support such functionality as well as a new device attribute to
> configure the pattern for a given LED.
>
> [Baolin Wang did some minor improvements.]
>
> Signed-off-by: Bjorn Andersson
> Signed-off-by: Baolin Wang
> ---
> Changes from v2:
>- Change kernel version to 4.19.
>- Force user to return error pointer if failed to issue
> pattern_get().
>- Use strstrip() to trim trailing newline.
>- Other optimization.
>
> Changes from v1:
>- Add some comments suggested by Pavel.
>- Change 'delta_t' can be 0.
>
> Note: I removed the pattern repeat check and will get the repeat number
> by adding
> one extra file named 'pattern_repeat' according to previous discussion.
> ---
Do you have any comments for this version patch set? Thanks.
>>>
>>>
>>>
>>> I tried modifying uleds.c driver to support pattern ops, but
>>> I'm getting segfault when doing "cat pattern". I didn't give
>>> it serious testing and analysis - will do it at weekend.
>>>
>>> It also drew my attention to the issue of desired pattern sysfs
>>> interface semantics on uninitialized pattern. In your implementation
>>> user seems to be unable to determine if the pattern is activated
>>> or not. We should define the semantics for this use case and
>>> describe it in the documentation. Possibly pattern could
>>> return alone new line character then.
>>
>>
>> I am not sure I get your points correctly. If user writes values to
>> pattern interface which means we activated the pattern.
>> If I am wrong, could you elaborate on the issue you concerned? Thanks.
>
>
> Now I see, that writing empty string disables the pattern, right?
> It should be explicitly stated in the pattern file documentation.
Yes, you are right. OK, I will add some documentation for this. Thanks.
>>> This is the code snippet I've used for testing pattern interface:
>>>
>>> static struct led_pattern ptrn[10];
>>> static int ptrn_len;
>>>
>>> static int uled_pattern_clear(struct led_classdev *ldev)
>>> {
>>> return 0;
>>> }
>>>
>>> static int uled_pattern_set(struct led_classdev *ldev,
>>>struct led_pattern *pattern,
>>>int len)
>>> {
>>> int i;
>>>
>>> for (i = 0; i < len; i++) {
>>> ptrn[i].brightness = pattern[i].brightness;
>>> ptrn[i].delta_t = pattern[i].delta_t;
>>> }
>>>
>>> ptrn_len = len;
>>>
>>> return 0;
>>> }
>>>
>>> static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
>>>int *len)
>>> {
>>> int i;
>>>
>>> for (i = 0; i < ptrn_len; i++) {
>>> ptrn[i].brightness = 3;
>>> ptrn[i].delta_t = 5;
>>> }
>>>
>>> *len = ptrn_len;
>>>
>>> return ptrn;
>>>
>>> }
>>
>>
>> The reason you met segfault when doing "cat pattern" is you should not
>> return one static pattern array, since in pattern_show() it will help
>> to free the pattern memory, could you change to return one pattern
>> pointer with dynamic allocation like my patch 2?
>
>
> Thanks for pointing this out.
>
>
>Documentation/ABI/testing/sysfs-class-led | 17 +
>drivers/leds/led-class.c | 119
> +
>include/linux/leds.h | 19 +
>3 files changed, 155 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led
> b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..e01ac55 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,20 @@ Description:
> gpio and backlight triggers. In case of the backlight
> trigger,
> it is useful when driving a LED which is intended to
> indicate
> a device in a standby like state.
> +
> +What: /sys/class/leds//pattern
> +Date: June 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern for the LED, for LED hardware that support
> + altering the brightness as a
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Jacek,
On 13 July 2018 at 05:41, Jacek Anaszewski wrote:
> Hi Baolin,
>
>
> On 07/12/2018 02:24 PM, Baolin Wang wrote:
>>
>> Hi Jacek,
>>
>> On 12 July 2018 at 05:10, Jacek Anaszewski
>> wrote:
>>>
>>> Hi Baolin.
>>>
>>>
>>> On 07/11/2018 01:02 PM, Baolin Wang wrote:
Hi Jacek and Pavel,
On 29 June 2018 at 13:03, Baolin Wang wrote:
>
>
> From: Bjorn Andersson
>
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
>
> This adds a new optional operator that LED class drivers can implement
> if they support such functionality as well as a new device attribute to
> configure the pattern for a given LED.
>
> [Baolin Wang did some minor improvements.]
>
> Signed-off-by: Bjorn Andersson
> Signed-off-by: Baolin Wang
> ---
> Changes from v2:
>- Change kernel version to 4.19.
>- Force user to return error pointer if failed to issue
> pattern_get().
>- Use strstrip() to trim trailing newline.
>- Other optimization.
>
> Changes from v1:
>- Add some comments suggested by Pavel.
>- Change 'delta_t' can be 0.
>
> Note: I removed the pattern repeat check and will get the repeat number
> by adding
> one extra file named 'pattern_repeat' according to previous discussion.
> ---
Do you have any comments for this version patch set? Thanks.
>>>
>>>
>>>
>>> I tried modifying uleds.c driver to support pattern ops, but
>>> I'm getting segfault when doing "cat pattern". I didn't give
>>> it serious testing and analysis - will do it at weekend.
>>>
>>> It also drew my attention to the issue of desired pattern sysfs
>>> interface semantics on uninitialized pattern. In your implementation
>>> user seems to be unable to determine if the pattern is activated
>>> or not. We should define the semantics for this use case and
>>> describe it in the documentation. Possibly pattern could
>>> return alone new line character then.
>>
>>
>> I am not sure I get your points correctly. If user writes values to
>> pattern interface which means we activated the pattern.
>> If I am wrong, could you elaborate on the issue you concerned? Thanks.
>
>
> Now I see, that writing empty string disables the pattern, right?
> It should be explicitly stated in the pattern file documentation.
Yes, you are right. OK, I will add some documentation for this. Thanks.
>>> This is the code snippet I've used for testing pattern interface:
>>>
>>> static struct led_pattern ptrn[10];
>>> static int ptrn_len;
>>>
>>> static int uled_pattern_clear(struct led_classdev *ldev)
>>> {
>>> return 0;
>>> }
>>>
>>> static int uled_pattern_set(struct led_classdev *ldev,
>>>struct led_pattern *pattern,
>>>int len)
>>> {
>>> int i;
>>>
>>> for (i = 0; i < len; i++) {
>>> ptrn[i].brightness = pattern[i].brightness;
>>> ptrn[i].delta_t = pattern[i].delta_t;
>>> }
>>>
>>> ptrn_len = len;
>>>
>>> return 0;
>>> }
>>>
>>> static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
>>>int *len)
>>> {
>>> int i;
>>>
>>> for (i = 0; i < ptrn_len; i++) {
>>> ptrn[i].brightness = 3;
>>> ptrn[i].delta_t = 5;
>>> }
>>>
>>> *len = ptrn_len;
>>>
>>> return ptrn;
>>>
>>> }
>>
>>
>> The reason you met segfault when doing "cat pattern" is you should not
>> return one static pattern array, since in pattern_show() it will help
>> to free the pattern memory, could you change to return one pattern
>> pointer with dynamic allocation like my patch 2?
>
>
> Thanks for pointing this out.
>
>
>Documentation/ABI/testing/sysfs-class-led | 17 +
>drivers/leds/led-class.c | 119
> +
>include/linux/leds.h | 19 +
>3 files changed, 155 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led
> b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..e01ac55 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,20 @@ Description:
> gpio and backlight triggers. In case of the backlight
> trigger,
> it is useful when driving a LED which is intended to
> indicate
> a device in a standby like state.
> +
> +What: /sys/class/leds//pattern
> +Date: June 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern for the LED, for LED hardware that support
> + altering the brightness as a
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Baolin,
On 07/12/2018 02:24 PM, Baolin Wang wrote:
Hi Jacek,
On 12 July 2018 at 05:10, Jacek Anaszewski wrote:
Hi Baolin.
On 07/11/2018 01:02 PM, Baolin Wang wrote:
Hi Jacek and Pavel,
On 29 June 2018 at 13:03, Baolin Wang wrote:
From: Bjorn Andersson
Some LED controllers have support for autonomously controlling
brightness over time, according to some preprogrammed pattern or
function.
This adds a new optional operator that LED class drivers can implement
if they support such functionality as well as a new device attribute to
configure the pattern for a given LED.
[Baolin Wang did some minor improvements.]
Signed-off-by: Bjorn Andersson
Signed-off-by: Baolin Wang
---
Changes from v2:
- Change kernel version to 4.19.
- Force user to return error pointer if failed to issue pattern_get().
- Use strstrip() to trim trailing newline.
- Other optimization.
Changes from v1:
- Add some comments suggested by Pavel.
- Change 'delta_t' can be 0.
Note: I removed the pattern repeat check and will get the repeat number
by adding
one extra file named 'pattern_repeat' according to previous discussion.
---
Do you have any comments for this version patch set? Thanks.
I tried modifying uleds.c driver to support pattern ops, but
I'm getting segfault when doing "cat pattern". I didn't give
it serious testing and analysis - will do it at weekend.
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
I am not sure I get your points correctly. If user writes values to
pattern interface which means we activated the pattern.
If I am wrong, could you elaborate on the issue you concerned? Thanks.
Now I see, that writing empty string disables the pattern, right?
It should be explicitly stated in the pattern file documentation.
This is the code snippet I've used for testing pattern interface:
static struct led_pattern ptrn[10];
static int ptrn_len;
static int uled_pattern_clear(struct led_classdev *ldev)
{
return 0;
}
static int uled_pattern_set(struct led_classdev *ldev,
struct led_pattern *pattern,
int len)
{
int i;
for (i = 0; i < len; i++) {
ptrn[i].brightness = pattern[i].brightness;
ptrn[i].delta_t = pattern[i].delta_t;
}
ptrn_len = len;
return 0;
}
static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
int *len)
{
int i;
for (i = 0; i < ptrn_len; i++) {
ptrn[i].brightness = 3;
ptrn[i].delta_t = 5;
}
*len = ptrn_len;
return ptrn;
}
The reason you met segfault when doing "cat pattern" is you should not
return one static pattern array, since in pattern_show() it will help
to free the pattern memory, could you change to return one pattern
pointer with dynamic allocation like my patch 2?
Thanks for pointing this out.
Documentation/ABI/testing/sysfs-class-led | 17 +
drivers/leds/led-class.c | 119
+
include/linux/leds.h | 19 +
3 files changed, 155 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-led
b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..e01ac55 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,20 @@ Description:
gpio and backlight triggers. In case of the backlight
trigger,
it is useful when driving a LED which is intended to
indicate
a device in a standby like state.
+
+What: /sys/class/leds//pattern
+Date: June 2018
+KernelVersion: 4.19
+Description:
+ Specify a pattern for the LED, for LED hardware that support
+ altering the brightness as a function of time.
+
+ 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.
+
+ As LED hardware might have different capabilities and precision
+ the requested pattern might be slighly adjusted by the driver
+ and the resulting pattern of such operation should be returned
+ when this file is read.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..8a685a2 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device
*dev,
}
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Baolin,
On 07/12/2018 02:24 PM, Baolin Wang wrote:
Hi Jacek,
On 12 July 2018 at 05:10, Jacek Anaszewski wrote:
Hi Baolin.
On 07/11/2018 01:02 PM, Baolin Wang wrote:
Hi Jacek and Pavel,
On 29 June 2018 at 13:03, Baolin Wang wrote:
From: Bjorn Andersson
Some LED controllers have support for autonomously controlling
brightness over time, according to some preprogrammed pattern or
function.
This adds a new optional operator that LED class drivers can implement
if they support such functionality as well as a new device attribute to
configure the pattern for a given LED.
[Baolin Wang did some minor improvements.]
Signed-off-by: Bjorn Andersson
Signed-off-by: Baolin Wang
---
Changes from v2:
- Change kernel version to 4.19.
- Force user to return error pointer if failed to issue pattern_get().
- Use strstrip() to trim trailing newline.
- Other optimization.
Changes from v1:
- Add some comments suggested by Pavel.
- Change 'delta_t' can be 0.
Note: I removed the pattern repeat check and will get the repeat number
by adding
one extra file named 'pattern_repeat' according to previous discussion.
---
Do you have any comments for this version patch set? Thanks.
I tried modifying uleds.c driver to support pattern ops, but
I'm getting segfault when doing "cat pattern". I didn't give
it serious testing and analysis - will do it at weekend.
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
I am not sure I get your points correctly. If user writes values to
pattern interface which means we activated the pattern.
If I am wrong, could you elaborate on the issue you concerned? Thanks.
Now I see, that writing empty string disables the pattern, right?
It should be explicitly stated in the pattern file documentation.
This is the code snippet I've used for testing pattern interface:
static struct led_pattern ptrn[10];
static int ptrn_len;
static int uled_pattern_clear(struct led_classdev *ldev)
{
return 0;
}
static int uled_pattern_set(struct led_classdev *ldev,
struct led_pattern *pattern,
int len)
{
int i;
for (i = 0; i < len; i++) {
ptrn[i].brightness = pattern[i].brightness;
ptrn[i].delta_t = pattern[i].delta_t;
}
ptrn_len = len;
return 0;
}
static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
int *len)
{
int i;
for (i = 0; i < ptrn_len; i++) {
ptrn[i].brightness = 3;
ptrn[i].delta_t = 5;
}
*len = ptrn_len;
return ptrn;
}
The reason you met segfault when doing "cat pattern" is you should not
return one static pattern array, since in pattern_show() it will help
to free the pattern memory, could you change to return one pattern
pointer with dynamic allocation like my patch 2?
Thanks for pointing this out.
Documentation/ABI/testing/sysfs-class-led | 17 +
drivers/leds/led-class.c | 119
+
include/linux/leds.h | 19 +
3 files changed, 155 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-led
b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..e01ac55 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,20 @@ Description:
gpio and backlight triggers. In case of the backlight
trigger,
it is useful when driving a LED which is intended to
indicate
a device in a standby like state.
+
+What: /sys/class/leds//pattern
+Date: June 2018
+KernelVersion: 4.19
+Description:
+ Specify a pattern for the LED, for LED hardware that support
+ altering the brightness as a function of time.
+
+ 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.
+
+ As LED hardware might have different capabilities and precision
+ the requested pattern might be slighly adjusted by the driver
+ and the resulting pattern of such operation should be returned
+ when this file is read.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..8a685a2 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device
*dev,
}
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Jacek,
On 12 July 2018 at 05:10, Jacek Anaszewski wrote:
> Hi Baolin.
>
>
> On 07/11/2018 01:02 PM, Baolin Wang wrote:
>>
>> Hi Jacek and Pavel,
>>
>> On 29 June 2018 at 13:03, Baolin Wang wrote:
>>>
>>> From: Bjorn Andersson
>>>
>>> Some LED controllers have support for autonomously controlling
>>> brightness over time, according to some preprogrammed pattern or
>>> function.
>>>
>>> This adds a new optional operator that LED class drivers can implement
>>> if they support such functionality as well as a new device attribute to
>>> configure the pattern for a given LED.
>>>
>>> [Baolin Wang did some minor improvements.]
>>>
>>> Signed-off-by: Bjorn Andersson
>>> Signed-off-by: Baolin Wang
>>> ---
>>> Changes from v2:
>>> - Change kernel version to 4.19.
>>> - Force user to return error pointer if failed to issue pattern_get().
>>> - Use strstrip() to trim trailing newline.
>>> - Other optimization.
>>>
>>> Changes from v1:
>>> - Add some comments suggested by Pavel.
>>> - Change 'delta_t' can be 0.
>>>
>>> Note: I removed the pattern repeat check and will get the repeat number
>>> by adding
>>> one extra file named 'pattern_repeat' according to previous discussion.
>>> ---
>>
>>
>> Do you have any comments for this version patch set? Thanks.
>
>
> I tried modifying uleds.c driver to support pattern ops, but
> I'm getting segfault when doing "cat pattern". I didn't give
> it serious testing and analysis - will do it at weekend.
>
> It also drew my attention to the issue of desired pattern sysfs
> interface semantics on uninitialized pattern. In your implementation
> user seems to be unable to determine if the pattern is activated
> or not. We should define the semantics for this use case and
> describe it in the documentation. Possibly pattern could
> return alone new line character then.
I am not sure I get your points correctly. If user writes values to
pattern interface which means we activated the pattern.
If I am wrong, could you elaborate on the issue you concerned? Thanks.
>
> This is the code snippet I've used for testing pattern interface:
>
> static struct led_pattern ptrn[10];
> static int ptrn_len;
>
> static int uled_pattern_clear(struct led_classdev *ldev)
> {
> return 0;
> }
>
> static int uled_pattern_set(struct led_classdev *ldev,
> struct led_pattern *pattern,
> int len)
> {
> int i;
>
> for (i = 0; i < len; i++) {
> ptrn[i].brightness = pattern[i].brightness;
> ptrn[i].delta_t = pattern[i].delta_t;
> }
>
> ptrn_len = len;
>
> return 0;
> }
>
> static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
> int *len)
> {
> int i;
>
> for (i = 0; i < ptrn_len; i++) {
> ptrn[i].brightness = 3;
> ptrn[i].delta_t = 5;
> }
>
> *len = ptrn_len;
>
> return ptrn;
>
> }
The reason you met segfault when doing "cat pattern" is you should not
return one static pattern array, since in pattern_show() it will help
to free the pattern memory, could you change to return one pattern
pointer with dynamic allocation like my patch 2?
>>
>>> Documentation/ABI/testing/sysfs-class-led | 17 +
>>> drivers/leds/led-class.c | 119
>>> +
>>> include/linux/leds.h | 19 +
>>> 3 files changed, 155 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>>> b/Documentation/ABI/testing/sysfs-class-led
>>> index 5f67f7a..e01ac55 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>> @@ -61,3 +61,20 @@ Description:
>>> gpio and backlight triggers. In case of the backlight
>>> trigger,
>>> it is useful when driving a LED which is intended to
>>> indicate
>>> a device in a standby like state.
>>> +
>>> +What: /sys/class/leds//pattern
>>> +Date: June 2018
>>> +KernelVersion: 4.19
>>> +Description:
>>> + Specify a pattern for the LED, for LED hardware that support
>>> + altering the brightness as a function of time.
>>> +
>>> + 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.
>>> +
>>> + As LED hardware might have different capabilities and precision
>>> + the requested pattern might be slighly adjusted by the driver
>>> + and the resulting pattern of such operation should be returned
>>> + when this file is read.
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 3c7e348..8a685a2 100644
>>> ---
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Jacek,
On 12 July 2018 at 05:10, Jacek Anaszewski wrote:
> Hi Baolin.
>
>
> On 07/11/2018 01:02 PM, Baolin Wang wrote:
>>
>> Hi Jacek and Pavel,
>>
>> On 29 June 2018 at 13:03, Baolin Wang wrote:
>>>
>>> From: Bjorn Andersson
>>>
>>> Some LED controllers have support for autonomously controlling
>>> brightness over time, according to some preprogrammed pattern or
>>> function.
>>>
>>> This adds a new optional operator that LED class drivers can implement
>>> if they support such functionality as well as a new device attribute to
>>> configure the pattern for a given LED.
>>>
>>> [Baolin Wang did some minor improvements.]
>>>
>>> Signed-off-by: Bjorn Andersson
>>> Signed-off-by: Baolin Wang
>>> ---
>>> Changes from v2:
>>> - Change kernel version to 4.19.
>>> - Force user to return error pointer if failed to issue pattern_get().
>>> - Use strstrip() to trim trailing newline.
>>> - Other optimization.
>>>
>>> Changes from v1:
>>> - Add some comments suggested by Pavel.
>>> - Change 'delta_t' can be 0.
>>>
>>> Note: I removed the pattern repeat check and will get the repeat number
>>> by adding
>>> one extra file named 'pattern_repeat' according to previous discussion.
>>> ---
>>
>>
>> Do you have any comments for this version patch set? Thanks.
>
>
> I tried modifying uleds.c driver to support pattern ops, but
> I'm getting segfault when doing "cat pattern". I didn't give
> it serious testing and analysis - will do it at weekend.
>
> It also drew my attention to the issue of desired pattern sysfs
> interface semantics on uninitialized pattern. In your implementation
> user seems to be unable to determine if the pattern is activated
> or not. We should define the semantics for this use case and
> describe it in the documentation. Possibly pattern could
> return alone new line character then.
I am not sure I get your points correctly. If user writes values to
pattern interface which means we activated the pattern.
If I am wrong, could you elaborate on the issue you concerned? Thanks.
>
> This is the code snippet I've used for testing pattern interface:
>
> static struct led_pattern ptrn[10];
> static int ptrn_len;
>
> static int uled_pattern_clear(struct led_classdev *ldev)
> {
> return 0;
> }
>
> static int uled_pattern_set(struct led_classdev *ldev,
> struct led_pattern *pattern,
> int len)
> {
> int i;
>
> for (i = 0; i < len; i++) {
> ptrn[i].brightness = pattern[i].brightness;
> ptrn[i].delta_t = pattern[i].delta_t;
> }
>
> ptrn_len = len;
>
> return 0;
> }
>
> static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
> int *len)
> {
> int i;
>
> for (i = 0; i < ptrn_len; i++) {
> ptrn[i].brightness = 3;
> ptrn[i].delta_t = 5;
> }
>
> *len = ptrn_len;
>
> return ptrn;
>
> }
The reason you met segfault when doing "cat pattern" is you should not
return one static pattern array, since in pattern_show() it will help
to free the pattern memory, could you change to return one pattern
pointer with dynamic allocation like my patch 2?
>>
>>> Documentation/ABI/testing/sysfs-class-led | 17 +
>>> drivers/leds/led-class.c | 119
>>> +
>>> include/linux/leds.h | 19 +
>>> 3 files changed, 155 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>>> b/Documentation/ABI/testing/sysfs-class-led
>>> index 5f67f7a..e01ac55 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>> @@ -61,3 +61,20 @@ Description:
>>> gpio and backlight triggers. In case of the backlight
>>> trigger,
>>> it is useful when driving a LED which is intended to
>>> indicate
>>> a device in a standby like state.
>>> +
>>> +What: /sys/class/leds//pattern
>>> +Date: June 2018
>>> +KernelVersion: 4.19
>>> +Description:
>>> + Specify a pattern for the LED, for LED hardware that support
>>> + altering the brightness as a function of time.
>>> +
>>> + 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.
>>> +
>>> + As LED hardware might have different capabilities and precision
>>> + the requested pattern might be slighly adjusted by the driver
>>> + and the resulting pattern of such operation should be returned
>>> + when this file is read.
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 3c7e348..8a685a2 100644
>>> ---
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Baolin.
On 07/11/2018 01:02 PM, Baolin Wang wrote:
Hi Jacek and Pavel,
On 29 June 2018 at 13:03, Baolin Wang wrote:
From: Bjorn Andersson
Some LED controllers have support for autonomously controlling
brightness over time, according to some preprogrammed pattern or
function.
This adds a new optional operator that LED class drivers can implement
if they support such functionality as well as a new device attribute to
configure the pattern for a given LED.
[Baolin Wang did some minor improvements.]
Signed-off-by: Bjorn Andersson
Signed-off-by: Baolin Wang
---
Changes from v2:
- Change kernel version to 4.19.
- Force user to return error pointer if failed to issue pattern_get().
- Use strstrip() to trim trailing newline.
- Other optimization.
Changes from v1:
- Add some comments suggested by Pavel.
- Change 'delta_t' can be 0.
Note: I removed the pattern repeat check and will get the repeat number by
adding
one extra file named 'pattern_repeat' according to previous discussion.
---
Do you have any comments for this version patch set? Thanks.
I tried modifying uleds.c driver to support pattern ops, but
I'm getting segfault when doing "cat pattern". I didn't give
it serious testing and analysis - will do it at weekend.
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
This is the code snippet I've used for testing pattern interface:
static struct led_pattern ptrn[10];
static int ptrn_len;
static int uled_pattern_clear(struct led_classdev *ldev)
{
return 0;
}
static int uled_pattern_set(struct led_classdev *ldev,
struct led_pattern *pattern,
int len)
{
int i;
for (i = 0; i < len; i++) {
ptrn[i].brightness = pattern[i].brightness;
ptrn[i].delta_t = pattern[i].delta_t;
}
ptrn_len = len;
return 0;
}
static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
int *len)
{
int i;
for (i = 0; i < ptrn_len; i++) {
ptrn[i].brightness = 3;
ptrn[i].delta_t = 5;
}
*len = ptrn_len;
return ptrn;
}
Documentation/ABI/testing/sysfs-class-led | 17 +
drivers/leds/led-class.c | 119 +
include/linux/leds.h | 19 +
3 files changed, 155 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-led
b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..e01ac55 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,20 @@ Description:
gpio and backlight triggers. In case of the backlight trigger,
it is useful when driving a LED which is intended to indicate
a device in a standby like state.
+
+What: /sys/class/leds//pattern
+Date: June 2018
+KernelVersion: 4.19
+Description:
+ Specify a pattern for the LED, for LED hardware that support
+ altering the brightness as a function of time.
+
+ 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.
+
+ As LED hardware might have different capabilities and precision
+ the requested pattern might be slighly adjusted by the driver
+ and the resulting pattern of such operation should be returned
+ when this file is read.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..8a685a2 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device *dev,
}
static DEVICE_ATTR_RO(max_brightness);
+static ssize_t pattern_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_pattern *pattern;
+ size_t offset = 0;
+ int count, n, i;
+
+ if (!led_cdev->pattern_get)
+ return -EOPNOTSUPP;
Please check this in led_classdev_register() and fail
if pattern_set is initialized, but pattern_get or pattern_clear not.
+ pattern = led_cdev->pattern_get(led_cdev, );
+ if (IS_ERR(pattern))
+ return PTR_ERR(pattern);
+
+ for (i = 0; i < count; i++) {
+ n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
+
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Baolin.
On 07/11/2018 01:02 PM, Baolin Wang wrote:
Hi Jacek and Pavel,
On 29 June 2018 at 13:03, Baolin Wang wrote:
From: Bjorn Andersson
Some LED controllers have support for autonomously controlling
brightness over time, according to some preprogrammed pattern or
function.
This adds a new optional operator that LED class drivers can implement
if they support such functionality as well as a new device attribute to
configure the pattern for a given LED.
[Baolin Wang did some minor improvements.]
Signed-off-by: Bjorn Andersson
Signed-off-by: Baolin Wang
---
Changes from v2:
- Change kernel version to 4.19.
- Force user to return error pointer if failed to issue pattern_get().
- Use strstrip() to trim trailing newline.
- Other optimization.
Changes from v1:
- Add some comments suggested by Pavel.
- Change 'delta_t' can be 0.
Note: I removed the pattern repeat check and will get the repeat number by
adding
one extra file named 'pattern_repeat' according to previous discussion.
---
Do you have any comments for this version patch set? Thanks.
I tried modifying uleds.c driver to support pattern ops, but
I'm getting segfault when doing "cat pattern". I didn't give
it serious testing and analysis - will do it at weekend.
It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.
This is the code snippet I've used for testing pattern interface:
static struct led_pattern ptrn[10];
static int ptrn_len;
static int uled_pattern_clear(struct led_classdev *ldev)
{
return 0;
}
static int uled_pattern_set(struct led_classdev *ldev,
struct led_pattern *pattern,
int len)
{
int i;
for (i = 0; i < len; i++) {
ptrn[i].brightness = pattern[i].brightness;
ptrn[i].delta_t = pattern[i].delta_t;
}
ptrn_len = len;
return 0;
}
static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
int *len)
{
int i;
for (i = 0; i < ptrn_len; i++) {
ptrn[i].brightness = 3;
ptrn[i].delta_t = 5;
}
*len = ptrn_len;
return ptrn;
}
Documentation/ABI/testing/sysfs-class-led | 17 +
drivers/leds/led-class.c | 119 +
include/linux/leds.h | 19 +
3 files changed, 155 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-led
b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..e01ac55 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,20 @@ Description:
gpio and backlight triggers. In case of the backlight trigger,
it is useful when driving a LED which is intended to indicate
a device in a standby like state.
+
+What: /sys/class/leds//pattern
+Date: June 2018
+KernelVersion: 4.19
+Description:
+ Specify a pattern for the LED, for LED hardware that support
+ altering the brightness as a function of time.
+
+ 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.
+
+ As LED hardware might have different capabilities and precision
+ the requested pattern might be slighly adjusted by the driver
+ and the resulting pattern of such operation should be returned
+ when this file is read.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..8a685a2 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device *dev,
}
static DEVICE_ATTR_RO(max_brightness);
+static ssize_t pattern_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_pattern *pattern;
+ size_t offset = 0;
+ int count, n, i;
+
+ if (!led_cdev->pattern_get)
+ return -EOPNOTSUPP;
Please check this in led_classdev_register() and fail
if pattern_set is initialized, but pattern_get or pattern_clear not.
+ pattern = led_cdev->pattern_get(led_cdev, );
+ if (IS_ERR(pattern))
+ return PTR_ERR(pattern);
+
+ for (i = 0; i < count; i++) {
+ n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
+
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Jacek and Pavel,
On 29 June 2018 at 13:03, Baolin Wang wrote:
> From: Bjorn Andersson
>
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
>
> This adds a new optional operator that LED class drivers can implement
> if they support such functionality as well as a new device attribute to
> configure the pattern for a given LED.
>
> [Baolin Wang did some minor improvements.]
>
> Signed-off-by: Bjorn Andersson
> Signed-off-by: Baolin Wang
> ---
> Changes from v2:
> - Change kernel version to 4.19.
> - Force user to return error pointer if failed to issue pattern_get().
> - Use strstrip() to trim trailing newline.
> - Other optimization.
>
> Changes from v1:
> - Add some comments suggested by Pavel.
> - Change 'delta_t' can be 0.
>
> Note: I removed the pattern repeat check and will get the repeat number by
> adding
> one extra file named 'pattern_repeat' according to previous discussion.
> ---
Do you have any comments for this version patch set? Thanks.
> Documentation/ABI/testing/sysfs-class-led | 17 +
> drivers/leds/led-class.c | 119
> +
> include/linux/leds.h | 19 +
> 3 files changed, 155 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led
> b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..e01ac55 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,20 @@ Description:
> gpio and backlight triggers. In case of the backlight trigger,
> it is useful when driving a LED which is intended to indicate
> a device in a standby like state.
> +
> +What: /sys/class/leds//pattern
> +Date: June 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern for the LED, for LED hardware that support
> + altering the brightness as a function of time.
> +
> + 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.
> +
> + As LED hardware might have different capabilities and precision
> + the requested pattern might be slighly adjusted by the driver
> + and the resulting pattern of such operation should be returned
> + when this file is read.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3c7e348..8a685a2 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(max_brightness);
>
> +static ssize_t pattern_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_pattern *pattern;
> + size_t offset = 0;
> + int count, n, i;
> +
> + if (!led_cdev->pattern_get)
> + return -EOPNOTSUPP;
> +
> + pattern = led_cdev->pattern_get(led_cdev, );
> + if (IS_ERR(pattern))
> + return PTR_ERR(pattern);
> +
> + for (i = 0; i < count; i++) {
> + n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
> +pattern[i].brightness, pattern[i].delta_t);
> +
> + if (offset + n >= PAGE_SIZE)
> + goto err_nospc;
> +
> + offset += n;
> + }
> +
> + buf[offset - 1] = '\n';
> +
> + kfree(pattern);
> + return offset;
> +
> +err_nospc:
> + kfree(pattern);
> + return -ENOSPC;
> +}
> +
> +static ssize_t pattern_store(struct device *dev,
> +struct device_attribute *attr,
> +const char *buf, size_t size)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_pattern *pattern = NULL;
> + unsigned long val;
> + char *sbegin;
> + char *elem;
> + char *s;
> + int ret, len = 0;
> + bool odd = true;
> +
> + sbegin = kstrndup(buf, size, GFP_KERNEL);
> + if (!sbegin)
> + return -ENOMEM;
> +
> + /*
> +* Trim trailing newline, if the remaining string is empty,
> +* clear the pattern.
> +*/
> + s = strstrip(sbegin);
> + if (!*s) {
> + ret = led_cdev->pattern_clear(led_cdev);
> + goto out;
> + }
> +
> + pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
> + if (!pattern) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + /* Parse out the brightness & delta_t touples */
> + while ((elem = strsep(, " ")) != NULL) {
> +
Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
Hi Jacek and Pavel,
On 29 June 2018 at 13:03, Baolin Wang wrote:
> From: Bjorn Andersson
>
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
>
> This adds a new optional operator that LED class drivers can implement
> if they support such functionality as well as a new device attribute to
> configure the pattern for a given LED.
>
> [Baolin Wang did some minor improvements.]
>
> Signed-off-by: Bjorn Andersson
> Signed-off-by: Baolin Wang
> ---
> Changes from v2:
> - Change kernel version to 4.19.
> - Force user to return error pointer if failed to issue pattern_get().
> - Use strstrip() to trim trailing newline.
> - Other optimization.
>
> Changes from v1:
> - Add some comments suggested by Pavel.
> - Change 'delta_t' can be 0.
>
> Note: I removed the pattern repeat check and will get the repeat number by
> adding
> one extra file named 'pattern_repeat' according to previous discussion.
> ---
Do you have any comments for this version patch set? Thanks.
> Documentation/ABI/testing/sysfs-class-led | 17 +
> drivers/leds/led-class.c | 119
> +
> include/linux/leds.h | 19 +
> 3 files changed, 155 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led
> b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..e01ac55 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,20 @@ Description:
> gpio and backlight triggers. In case of the backlight trigger,
> it is useful when driving a LED which is intended to indicate
> a device in a standby like state.
> +
> +What: /sys/class/leds//pattern
> +Date: June 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern for the LED, for LED hardware that support
> + altering the brightness as a function of time.
> +
> + 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.
> +
> + As LED hardware might have different capabilities and precision
> + the requested pattern might be slighly adjusted by the driver
> + and the resulting pattern of such operation should be returned
> + when this file is read.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3c7e348..8a685a2 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(max_brightness);
>
> +static ssize_t pattern_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_pattern *pattern;
> + size_t offset = 0;
> + int count, n, i;
> +
> + if (!led_cdev->pattern_get)
> + return -EOPNOTSUPP;
> +
> + pattern = led_cdev->pattern_get(led_cdev, );
> + if (IS_ERR(pattern))
> + return PTR_ERR(pattern);
> +
> + for (i = 0; i < count; i++) {
> + n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
> +pattern[i].brightness, pattern[i].delta_t);
> +
> + if (offset + n >= PAGE_SIZE)
> + goto err_nospc;
> +
> + offset += n;
> + }
> +
> + buf[offset - 1] = '\n';
> +
> + kfree(pattern);
> + return offset;
> +
> +err_nospc:
> + kfree(pattern);
> + return -ENOSPC;
> +}
> +
> +static ssize_t pattern_store(struct device *dev,
> +struct device_attribute *attr,
> +const char *buf, size_t size)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_pattern *pattern = NULL;
> + unsigned long val;
> + char *sbegin;
> + char *elem;
> + char *s;
> + int ret, len = 0;
> + bool odd = true;
> +
> + sbegin = kstrndup(buf, size, GFP_KERNEL);
> + if (!sbegin)
> + return -ENOMEM;
> +
> + /*
> +* Trim trailing newline, if the remaining string is empty,
> +* clear the pattern.
> +*/
> + s = strstrip(sbegin);
> + if (!*s) {
> + ret = led_cdev->pattern_clear(led_cdev);
> + goto out;
> + }
> +
> + pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
> + if (!pattern) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + /* Parse out the brightness & delta_t touples */
> + while ((elem = strsep(, " ")) != NULL) {
> +
