Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 6 August 2018 at 19:41, Jacek Anaszewski wrote:
> Hi Baolin,
>
> On 08/06/2018 03:53 AM, Baolin Wang wrote:
>> Hi Jacek,
> [...]
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
@@ -0,0 +1,21 @@
+What:/sys/class/leds//pattern
+Date:August 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.
+
+ The format of the pattern values should be:
+ "brightness_1 duration_1 brightness_2 duration_2 brightness_3
+ duration_3 ...".
+
+What:/sys/class/leds//repeat
+Date:August 2018
+KernelVersion: 4.19
+Description:
+ Specify a pattern repeat number. 0 means repeat indefinitely.
>>>
>>> In current implementation this file on read returns the number
>>> of remaining repeat intervals. I'd add that to this description.
>>
>> I saw Pavel's comments that he did not suggest do this. So I will keep
>> the original description?
>
> Yes, please report always the original value.
Sure. Thanks.
>
> [...]
+static ssize_t pattern_trig_store_repeat(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct pattern_trig_data *data = led_cdev->trigger_data;
+ unsigned long res;
+ int err;
+
+ err = kstrtoul(buf, 10, );
+ if (err)
+ return err;
+
+ if (!led_cdev->pattern_set)
+ del_timer_sync(>timer);
>>>
>>> Is there a reason for not having this check under mutex?
>>
>> We will hold the mutex in pattern_trig_timer_function(), so if we do
>> del_timer_sync() under the mutex protection, we may meet dead-lock
>> issue. Moreover, the del_timer_sync() will make sure deactivating one
>> timer is safe.
>
> Ack.
>
> --
> Best regards,
> Jacek Anaszewski
--
Baolin Wang
Best Regards
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 6 August 2018 at 19:41, Jacek Anaszewski wrote:
> Hi Baolin,
>
> On 08/06/2018 03:53 AM, Baolin Wang wrote:
>> Hi Jacek,
> [...]
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
@@ -0,0 +1,21 @@
+What:/sys/class/leds//pattern
+Date:August 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.
+
+ The format of the pattern values should be:
+ "brightness_1 duration_1 brightness_2 duration_2 brightness_3
+ duration_3 ...".
+
+What:/sys/class/leds//repeat
+Date:August 2018
+KernelVersion: 4.19
+Description:
+ Specify a pattern repeat number. 0 means repeat indefinitely.
>>>
>>> In current implementation this file on read returns the number
>>> of remaining repeat intervals. I'd add that to this description.
>>
>> I saw Pavel's comments that he did not suggest do this. So I will keep
>> the original description?
>
> Yes, please report always the original value.
Sure. Thanks.
>
> [...]
+static ssize_t pattern_trig_store_repeat(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct pattern_trig_data *data = led_cdev->trigger_data;
+ unsigned long res;
+ int err;
+
+ err = kstrtoul(buf, 10, );
+ if (err)
+ return err;
+
+ if (!led_cdev->pattern_set)
+ del_timer_sync(>timer);
>>>
>>> Is there a reason for not having this check under mutex?
>>
>> We will hold the mutex in pattern_trig_timer_function(), so if we do
>> del_timer_sync() under the mutex protection, we may meet dead-lock
>> issue. Moreover, the del_timer_sync() will make sure deactivating one
>> timer is safe.
>
> Ack.
>
> --
> Best regards,
> Jacek Anaszewski
--
Baolin Wang
Best Regards
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
On 08/06/2018 03:53 AM, Baolin Wang wrote:
> Hi Jacek,
[...]
>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>> @@ -0,0 +1,21 @@
>>> +What:/sys/class/leds//pattern
>>> +Date:August 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.
>>> +
>>> + The format of the pattern values should be:
>>> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3
>>> + duration_3 ...".
>>> +
>>> +What:/sys/class/leds//repeat
>>> +Date:August 2018
>>> +KernelVersion: 4.19
>>> +Description:
>>> + Specify a pattern repeat number. 0 means repeat indefinitely.
>>
>> In current implementation this file on read returns the number
>> of remaining repeat intervals. I'd add that to this description.
>
> I saw Pavel's comments that he did not suggest do this. So I will keep
> the original description?
Yes, please report always the original value.
[...]
>>> +static ssize_t pattern_trig_store_repeat(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> + struct pattern_trig_data *data = led_cdev->trigger_data;
>>> + unsigned long res;
>>> + int err;
>>> +
>>> + err = kstrtoul(buf, 10, );
>>> + if (err)
>>> + return err;
>>> +
>>> + if (!led_cdev->pattern_set)
>>> + del_timer_sync(>timer);
>>
>> Is there a reason for not having this check under mutex?
>
> We will hold the mutex in pattern_trig_timer_function(), so if we do
> del_timer_sync() under the mutex protection, we may meet dead-lock
> issue. Moreover, the del_timer_sync() will make sure deactivating one
> timer is safe.
Ack.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
On 08/06/2018 03:53 AM, Baolin Wang wrote:
> Hi Jacek,
[...]
>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>> @@ -0,0 +1,21 @@
>>> +What:/sys/class/leds//pattern
>>> +Date:August 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.
>>> +
>>> + The format of the pattern values should be:
>>> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3
>>> + duration_3 ...".
>>> +
>>> +What:/sys/class/leds//repeat
>>> +Date:August 2018
>>> +KernelVersion: 4.19
>>> +Description:
>>> + Specify a pattern repeat number. 0 means repeat indefinitely.
>>
>> In current implementation this file on read returns the number
>> of remaining repeat intervals. I'd add that to this description.
>
> I saw Pavel's comments that he did not suggest do this. So I will keep
> the original description?
Yes, please report always the original value.
[...]
>>> +static ssize_t pattern_trig_store_repeat(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> + struct pattern_trig_data *data = led_cdev->trigger_data;
>>> + unsigned long res;
>>> + int err;
>>> +
>>> + err = kstrtoul(buf, 10, );
>>> + if (err)
>>> + return err;
>>> +
>>> + if (!led_cdev->pattern_set)
>>> + del_timer_sync(>timer);
>>
>> Is there a reason for not having this check under mutex?
>
> We will hold the mutex in pattern_trig_timer_function(), so if we do
> del_timer_sync() under the mutex protection, we may meet dead-lock
> issue. Moreover, the del_timer_sync() will make sure deactivating one
> timer is safe.
Ack.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
On 6 August 2018 at 16:06, Pavel Machek wrote: > On Mon 2018-08-06 15:33:12, Baolin Wang wrote: >> Hi Pavel, >> >> On 6 August 2018 at 15:09, Pavel Machek wrote: >> > Hi! >> > >> >> > In current implementation this file on read returns the number >> >> > of remaining repeat intervals. I'd add that to this description. >> >> >> >> I saw Pavel's comments that he did not suggest do this. So I will keep >> >> the original description? >> > >> > If that is accepted, the code should return originally written number. >> >> But I think it is more reasonable if user can know how many >> repetitions were left. So for your hardware case, can we add >> pattern_get() interface back to get the repetition number? > > Well, I'd say that we are mostly seeing "infinite" repetitions, or one > repetition. > > Yes, interface could be added to the lowlever driver -- but I'm not > sure hardware really supports getting that information; could be > tricky on N900 for example. > > I'd keep it simple for now. OK, I also like to keep it simple. I am fine the repeat file returns originally written number. If Jacek also agrees with this, then I will update new version. -- Baolin Wang Best Regards
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
On 6 August 2018 at 16:06, Pavel Machek wrote: > On Mon 2018-08-06 15:33:12, Baolin Wang wrote: >> Hi Pavel, >> >> On 6 August 2018 at 15:09, Pavel Machek wrote: >> > Hi! >> > >> >> > In current implementation this file on read returns the number >> >> > of remaining repeat intervals. I'd add that to this description. >> >> >> >> I saw Pavel's comments that he did not suggest do this. So I will keep >> >> the original description? >> > >> > If that is accepted, the code should return originally written number. >> >> But I think it is more reasonable if user can know how many >> repetitions were left. So for your hardware case, can we add >> pattern_get() interface back to get the repetition number? > > Well, I'd say that we are mostly seeing "infinite" repetitions, or one > repetition. > > Yes, interface could be added to the lowlever driver -- but I'm not > sure hardware really supports getting that information; could be > tricky on N900 for example. > > I'd keep it simple for now. OK, I also like to keep it simple. I am fine the repeat file returns originally written number. If Jacek also agrees with this, then I will update new version. -- Baolin Wang Best Regards
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
On Mon 2018-08-06 15:33:12, Baolin Wang wrote: > Hi Pavel, > > On 6 August 2018 at 15:09, Pavel Machek wrote: > > Hi! > > > >> > In current implementation this file on read returns the number > >> > of remaining repeat intervals. I'd add that to this description. > >> > >> I saw Pavel's comments that he did not suggest do this. So I will keep > >> the original description? > > > > If that is accepted, the code should return originally written number. > > But I think it is more reasonable if user can know how many > repetitions were left. So for your hardware case, can we add > pattern_get() interface back to get the repetition number? Well, I'd say that we are mostly seeing "infinite" repetitions, or one repetition. Yes, interface could be added to the lowlever driver -- but I'm not sure hardware really supports getting that information; could be tricky on N900 for example. I'd keep it simple for now. 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 v4 1/2] leds: core: Introduce LED pattern trigger
On Mon 2018-08-06 15:33:12, Baolin Wang wrote: > Hi Pavel, > > On 6 August 2018 at 15:09, Pavel Machek wrote: > > Hi! > > > >> > In current implementation this file on read returns the number > >> > of remaining repeat intervals. I'd add that to this description. > >> > >> I saw Pavel's comments that he did not suggest do this. So I will keep > >> the original description? > > > > If that is accepted, the code should return originally written number. > > But I think it is more reasonable if user can know how many > repetitions were left. So for your hardware case, can we add > pattern_get() interface back to get the repetition number? Well, I'd say that we are mostly seeing "infinite" repetitions, or one repetition. Yes, interface could be added to the lowlever driver -- but I'm not sure hardware really supports getting that information; could be tricky on N900 for example. I'd keep it simple for now. 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 v4 1/2] leds: core: Introduce LED pattern trigger
Hi Pavel, On 6 August 2018 at 15:09, Pavel Machek wrote: > Hi! > >> > In current implementation this file on read returns the number >> > of remaining repeat intervals. I'd add that to this description. >> >> I saw Pavel's comments that he did not suggest do this. So I will keep >> the original description? > > If that is accepted, the code should return originally written number. But I think it is more reasonable if user can know how many repetitions were left. So for your hardware case, can we add pattern_get() interface back to get the repetition number? -- Baolin Wang Best Regards
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
Hi Pavel, On 6 August 2018 at 15:09, Pavel Machek wrote: > Hi! > >> > In current implementation this file on read returns the number >> > of remaining repeat intervals. I'd add that to this description. >> >> I saw Pavel's comments that he did not suggest do this. So I will keep >> the original description? > > If that is accepted, the code should return originally written number. But I think it is more reasonable if user can know how many repetitions were left. So for your hardware case, can we add pattern_get() interface back to get the repetition number? -- Baolin Wang Best Regards
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
Hi! > > In current implementation this file on read returns the number > > of remaining repeat intervals. I'd add that to this description. > > I saw Pavel's comments that he did not suggest do this. So I will keep > the original description? If that is accepted, the code should return originally written number. 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 v4 1/2] leds: core: Introduce LED pattern trigger
Hi! > > In current implementation this file on read returns the number > > of remaining repeat intervals. I'd add that to this description. > > I saw Pavel's comments that he did not suggest do this. So I will keep > the original description? If that is accepted, the code should return originally written number. 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 v4 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 6 August 2018 at 00:10, Jacek Anaszewski wrote:
> Hi Baolin,
>
> Thank you for the fix.
>
> I've found few more details to improve, please take a look
> at my comments below.
>
> On 08/05/2018 06:04 AM, Baolin Wang wrote:
>> Some LED controllers have support for autonomously controlling
>> brightness over time, according to some preprogrammed pattern or
>> function.
>>
>> This patch adds pattern trigger that LED device can configure the
>> pattern and trigger it.
>>
>> Signed-off-by: Raphael Teysseyre
>> Signed-off-by: Baolin Wang
>> ---
>> Changes from v3:
>> - Reset pattern number to 0 if user provides incorrect pattern string.
>> - Support one pattern.
>>
>> Changes from v2:
>> - Remove hardware_pattern boolen.
>> - Chnage the pattern string format.
>>
>> Changes from v1:
>> - Use ATTRIBUTE_GROUPS() to define attributes.
>> - Introduce hardware_pattern flag to determine if software pattern
>> or hardware pattern.
>> - Re-implement pattern_trig_store_pattern() function.
>> - Remove pattern_get() interface.
>> - Improve comments.
>> - Other small optimization.
>> ---
>> .../ABI/testing/sysfs-class-led-trigger-pattern| 21 ++
>> drivers/leds/trigger/Kconfig | 7 +
>> drivers/leds/trigger/Makefile | 1 +
>> drivers/leds/trigger/ledtrig-pattern.c | 271
>> +
>> include/linux/leds.h | 16 ++
>> 5 files changed, 316 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> new file mode 100644
>> index 000..40afefe
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> @@ -0,0 +1,21 @@
>> +What:/sys/class/leds//pattern
>> +Date:August 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.
>> +
>> + The format of the pattern values should be:
>> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3
>> + duration_3 ...".
>> +
>> +What:/sys/class/leds//repeat
>> +Date:August 2018
>> +KernelVersion: 4.19
>> +Description:
>> + Specify a pattern repeat number. 0 means repeat indefinitely.
>
> In current implementation this file on read returns the number
> of remaining repeat intervals. I'd add that to this description.
I saw Pavel's comments that he did not suggest do this. So I will keep
the original description?
>
>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>> index 4018af7..b76fc3c 100644
>> --- a/drivers/leds/trigger/Kconfig
>> +++ b/drivers/leds/trigger/Kconfig
>> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV
>> This allows LEDs to be controlled by network device activity.
>> If unsure, say Y.
>>
>> +config LEDS_TRIGGER_PATTERN
>> + tristate "LED Pattern Trigger"
>> + help
>> + This allows LEDs to be controlled by a software or hardware pattern
>> + which is a series of tuples, of brightness and duration (ms).
>> + If unsure, say N
>> +
>> endif # LEDS_TRIGGERS
>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
>> index f3cfe19..9bcb64e 100644
>> --- a/drivers/leds/trigger/Makefile
>> +++ b/drivers/leds/trigger/Makefile
>> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+=
>> ledtrig-transient.o
>> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o
>> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
>> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o
>> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o
>> diff --git a/drivers/leds/trigger/ledtrig-pattern.c
>> b/drivers/leds/trigger/ledtrig-pattern.c
>> new file mode 100644
>> index 000..e5b90b7
>> --- /dev/null
>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>> @@ -0,0 +1,271 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/*
>> + * LED pattern trigger
>> + *
>> + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented
>> + * the first version, Baolin Wang simplified and improved the approach.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define MAX_PATTERNS 1024
>> +
>> +struct pattern_trig_data {
>> + struct led_classdev
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 6 August 2018 at 00:10, Jacek Anaszewski wrote:
> Hi Baolin,
>
> Thank you for the fix.
>
> I've found few more details to improve, please take a look
> at my comments below.
>
> On 08/05/2018 06:04 AM, Baolin Wang wrote:
>> Some LED controllers have support for autonomously controlling
>> brightness over time, according to some preprogrammed pattern or
>> function.
>>
>> This patch adds pattern trigger that LED device can configure the
>> pattern and trigger it.
>>
>> Signed-off-by: Raphael Teysseyre
>> Signed-off-by: Baolin Wang
>> ---
>> Changes from v3:
>> - Reset pattern number to 0 if user provides incorrect pattern string.
>> - Support one pattern.
>>
>> Changes from v2:
>> - Remove hardware_pattern boolen.
>> - Chnage the pattern string format.
>>
>> Changes from v1:
>> - Use ATTRIBUTE_GROUPS() to define attributes.
>> - Introduce hardware_pattern flag to determine if software pattern
>> or hardware pattern.
>> - Re-implement pattern_trig_store_pattern() function.
>> - Remove pattern_get() interface.
>> - Improve comments.
>> - Other small optimization.
>> ---
>> .../ABI/testing/sysfs-class-led-trigger-pattern| 21 ++
>> drivers/leds/trigger/Kconfig | 7 +
>> drivers/leds/trigger/Makefile | 1 +
>> drivers/leds/trigger/ledtrig-pattern.c | 271
>> +
>> include/linux/leds.h | 16 ++
>> 5 files changed, 316 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> new file mode 100644
>> index 000..40afefe
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> @@ -0,0 +1,21 @@
>> +What:/sys/class/leds//pattern
>> +Date:August 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.
>> +
>> + The format of the pattern values should be:
>> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3
>> + duration_3 ...".
>> +
>> +What:/sys/class/leds//repeat
>> +Date:August 2018
>> +KernelVersion: 4.19
>> +Description:
>> + Specify a pattern repeat number. 0 means repeat indefinitely.
>
> In current implementation this file on read returns the number
> of remaining repeat intervals. I'd add that to this description.
I saw Pavel's comments that he did not suggest do this. So I will keep
the original description?
>
>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>> index 4018af7..b76fc3c 100644
>> --- a/drivers/leds/trigger/Kconfig
>> +++ b/drivers/leds/trigger/Kconfig
>> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV
>> This allows LEDs to be controlled by network device activity.
>> If unsure, say Y.
>>
>> +config LEDS_TRIGGER_PATTERN
>> + tristate "LED Pattern Trigger"
>> + help
>> + This allows LEDs to be controlled by a software or hardware pattern
>> + which is a series of tuples, of brightness and duration (ms).
>> + If unsure, say N
>> +
>> endif # LEDS_TRIGGERS
>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
>> index f3cfe19..9bcb64e 100644
>> --- a/drivers/leds/trigger/Makefile
>> +++ b/drivers/leds/trigger/Makefile
>> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+=
>> ledtrig-transient.o
>> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o
>> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
>> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o
>> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o
>> diff --git a/drivers/leds/trigger/ledtrig-pattern.c
>> b/drivers/leds/trigger/ledtrig-pattern.c
>> new file mode 100644
>> index 000..e5b90b7
>> --- /dev/null
>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>> @@ -0,0 +1,271 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/*
>> + * LED pattern trigger
>> + *
>> + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented
>> + * the first version, Baolin Wang simplified and improved the approach.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define MAX_PATTERNS 1024
>> +
>> +struct pattern_trig_data {
>> + struct led_classdev
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
Hi! > > +What: /sys/class/leds//repeat > > +Date: August 2018 > > +KernelVersion: 4.19 > > +Description: > > + Specify a pattern repeat number. 0 means repeat indefinitely. > > In current implementation this file on read returns the number > of remaining repeat intervals. I'd add that to this description. Lets not do this. LEDs such as the one on N900 can do repeats automatically, in hardware. But in such case we may not be able to determine how many repetitions were done (and how many are left). 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 v4 1/2] leds: core: Introduce LED pattern trigger
Hi! > > +What: /sys/class/leds//repeat > > +Date: August 2018 > > +KernelVersion: 4.19 > > +Description: > > + Specify a pattern repeat number. 0 means repeat indefinitely. > > In current implementation this file on read returns the number > of remaining repeat intervals. I'd add that to this description. Lets not do this. LEDs such as the one on N900 can do repeats automatically, in hardware. But in such case we may not be able to determine how many repetitions were done (and how many are left). 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 v4 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
Thank you for the fix.
I've found few more details to improve, please take a look
at my comments below.
On 08/05/2018 06:04 AM, Baolin Wang wrote:
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
>
> This patch adds pattern trigger that LED device can configure the
> pattern and trigger it.
>
> Signed-off-by: Raphael Teysseyre
> Signed-off-by: Baolin Wang
> ---
> Changes from v3:
> - Reset pattern number to 0 if user provides incorrect pattern string.
> - Support one pattern.
>
> Changes from v2:
> - Remove hardware_pattern boolen.
> - Chnage the pattern string format.
>
> Changes from v1:
> - Use ATTRIBUTE_GROUPS() to define attributes.
> - Introduce hardware_pattern flag to determine if software pattern
> or hardware pattern.
> - Re-implement pattern_trig_store_pattern() function.
> - Remove pattern_get() interface.
> - Improve comments.
> - Other small optimization.
> ---
> .../ABI/testing/sysfs-class-led-trigger-pattern| 21 ++
> drivers/leds/trigger/Kconfig | 7 +
> drivers/leds/trigger/Makefile | 1 +
> drivers/leds/trigger/ledtrig-pattern.c | 271
> +
> include/linux/leds.h | 16 ++
> 5 files changed, 316 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> new file mode 100644
> index 000..40afefe
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> @@ -0,0 +1,21 @@
> +What:/sys/class/leds//pattern
> +Date:August 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.
> +
> + The format of the pattern values should be:
> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3
> + duration_3 ...".
> +
> +What:/sys/class/leds//repeat
> +Date:August 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern repeat number. 0 means repeat indefinitely.
In current implementation this file on read returns the number
of remaining repeat intervals. I'd add that to this description.
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 4018af7..b76fc3c 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV
> This allows LEDs to be controlled by network device activity.
> If unsure, say Y.
>
> +config LEDS_TRIGGER_PATTERN
> + tristate "LED Pattern Trigger"
> + help
> + This allows LEDs to be controlled by a software or hardware pattern
> + which is a series of tuples, of brightness and duration (ms).
> + If unsure, say N
> +
> endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index f3cfe19..9bcb64e 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+=
> ledtrig-transient.o
> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o
> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o
> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o
> diff --git a/drivers/leds/trigger/ledtrig-pattern.c
> b/drivers/leds/trigger/ledtrig-pattern.c
> new file mode 100644
> index 000..e5b90b7
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * LED pattern trigger
> + *
> + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented
> + * the first version, Baolin Wang simplified and improved the approach.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MAX_PATTERNS 1024
> +
> +struct pattern_trig_data {
> + struct led_classdev *led_cdev;
> + struct led_pattern patterns[MAX_PATTERNS];
> + struct led_pattern *curr;
> + struct led_pattern *next;
> + struct mutex lock;
> + u32 npatterns;
> + u32 repeat;
> + bool is_indefinite;
> + struct timer_list timer;
> +};
> +
> +static void
Re: [PATCH v4 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
Thank you for the fix.
I've found few more details to improve, please take a look
at my comments below.
On 08/05/2018 06:04 AM, Baolin Wang wrote:
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
>
> This patch adds pattern trigger that LED device can configure the
> pattern and trigger it.
>
> Signed-off-by: Raphael Teysseyre
> Signed-off-by: Baolin Wang
> ---
> Changes from v3:
> - Reset pattern number to 0 if user provides incorrect pattern string.
> - Support one pattern.
>
> Changes from v2:
> - Remove hardware_pattern boolen.
> - Chnage the pattern string format.
>
> Changes from v1:
> - Use ATTRIBUTE_GROUPS() to define attributes.
> - Introduce hardware_pattern flag to determine if software pattern
> or hardware pattern.
> - Re-implement pattern_trig_store_pattern() function.
> - Remove pattern_get() interface.
> - Improve comments.
> - Other small optimization.
> ---
> .../ABI/testing/sysfs-class-led-trigger-pattern| 21 ++
> drivers/leds/trigger/Kconfig | 7 +
> drivers/leds/trigger/Makefile | 1 +
> drivers/leds/trigger/ledtrig-pattern.c | 271
> +
> include/linux/leds.h | 16 ++
> 5 files changed, 316 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> new file mode 100644
> index 000..40afefe
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> @@ -0,0 +1,21 @@
> +What:/sys/class/leds//pattern
> +Date:August 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.
> +
> + The format of the pattern values should be:
> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3
> + duration_3 ...".
> +
> +What:/sys/class/leds//repeat
> +Date:August 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern repeat number. 0 means repeat indefinitely.
In current implementation this file on read returns the number
of remaining repeat intervals. I'd add that to this description.
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 4018af7..b76fc3c 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV
> This allows LEDs to be controlled by network device activity.
> If unsure, say Y.
>
> +config LEDS_TRIGGER_PATTERN
> + tristate "LED Pattern Trigger"
> + help
> + This allows LEDs to be controlled by a software or hardware pattern
> + which is a series of tuples, of brightness and duration (ms).
> + If unsure, say N
> +
> endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index f3cfe19..9bcb64e 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+=
> ledtrig-transient.o
> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o
> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o
> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o
> diff --git a/drivers/leds/trigger/ledtrig-pattern.c
> b/drivers/leds/trigger/ledtrig-pattern.c
> new file mode 100644
> index 000..e5b90b7
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * LED pattern trigger
> + *
> + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented
> + * the first version, Baolin Wang simplified and improved the approach.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MAX_PATTERNS 1024
> +
> +struct pattern_trig_data {
> + struct led_classdev *led_cdev;
> + struct led_pattern patterns[MAX_PATTERNS];
> + struct led_pattern *curr;
> + struct led_pattern *next;
> + struct mutex lock;
> + u32 npatterns;
> + u32 repeat;
> + bool is_indefinite;
> + struct timer_list timer;
> +};
> +
> +static void
