[PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Baolin Wang]

This patch adds one new led trigger that LED device can configure
the software or hardware pattern and trigger it.

Consumers can write 'pattern' file to enable the software pattern
which alters the brightness for the specified duration with one
software timer.

Moreover consumers can write 'hw_pattern' file to enable the hardware
pattern for some LED controllers which can autonomously control
brightness over time, according to some preprogrammed hardware
patterns.

Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v13:
 - Add duration validation for gradual dimming.
 - Coding style optimization.

Changes from v12:
 - Add gradual dimming support for software pattern.

Changes from v11:
 - Change -1 means repeat indefinitely.

Changes from v10:
 - Change 'int' to 'u32' for delta_t field.

Changes from v9:
 - None.

Changes from v8:
 - None.

Changes from v7:
 - Move the SC27XX hardware patterns description into its own ABI file.

Changes from v6:
 - Improve commit message.
 - Optimize the description of the hw_pattern file.
 - Simplify some logics.

Changes from v5:
 - Add one 'hw_pattern' file for hardware patterns.

Changes from v4:
 - Change the repeat file to return the originally written number.
 - Improve comments.
 - Fix some build warnings.

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    |  76 ++++
 drivers/leds/trigger/Kconfig                       |   7 +
 drivers/leds/trigger/Makefile                      |   1 +
 drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
 include/linux/leds.h                               |  15 +
 5 files changed, 530 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 0000000..22d7af7
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
@@ -0,0 +1,76 @@
+What:		/sys/class/leds/<led>/pattern
+Date:		September 2018
+KernelVersion:	4.20
+Description:
+		Specify a software pattern for the LED, that supports altering
+		the brightness for the specified duration with one software
+		timer. It can do gradual dimming and constant brightness.
+
+		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.
+
+		1. When doing gradual dimming, the led brightness will be updated
+		every 50 milliseconds, so the duration of each step should not
+		less than 50 milliseconds. The gradual dimming format of the
+		software pattern values should be:
+		"brightness_1 duration_1 brightness_2 duration_2 brightness_3
+		duration_3 ...". For example:
+
+		echo 0 1000 255 2000 > pattern
+
+		It will make the LED go gradually from zero-intensity to max (255)
+		intensity in 1000 milliseconds, then back to zero intensity in 2000
+		milliseconds:
+
+		LED brightness
+		    ^
+		255-|       / \            / \            /
+		    |      /    \         /    \         /
+		    |     /       \      /       \      /
+		    |    /          \   /          \   /
+		  0-|   /             \/             \/
+		    +---0----1----2----3----4----5----6------------> time (s)
+
+		2. To make the LED go instantly from one brigntess value to another,
+		we should use use zero-time lengths. So the format should be:
+		"brightness_1 duration_1 brightness_1 0 brightness_2 duration_2
+		brightness_2 0 ...". For example:
+
+		echo 0 1000 0 0 255 2000 255 0 > pattern
+
+		It will make the LED stay off for one second, then stay at max brightness
+		for two seconds:
+
+		LED brightness
+		    ^
+		255-|        +---------+    +---------+
+		    |        |         |    |         |
+		    |        |         |    |         |
+		    |        |         |    |         |
+		  0-|   -----+         +----+         +----
+		    +---0----1----2----3----4----5----6------------> time (s)
+
+What:		/sys/class/leds/<led>/hw_pattern
+Date:		September 2018
+KernelVersion:	4.20
+Description:
+		Specify a hardware pattern for the LED, for LED hardware that
+		supports autonomously controlling brightness over time, according
+		to some preprogrammed hardware patterns.
+
+		Since different LED hardware can have different semantics of
+		hardware patterns, each driver is expected to provide its own
+		description for the hardware patterns in their ABI documentation
+		file.
+
+What:		/sys/class/leds/<led>/repeat
+Date:		September 2018
+KernelVersion:	4.20
+Description:
+		Specify a pattern repeat number. -1 means repeat indefinitely,
+		other negative numbers and number 0 are invalid.
+
+		This file will always return the originally written repeat
+		number.
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 0000000..92a0fd0
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-pattern.c
@@ -0,0 +1,431 @@
+// 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 <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+
+#define MAX_PATTERNS		1024
+/*
+ * When doing gradual dimming, the led brightness will be updated
+ * every 50 milliseconds.
+ */
+#define UPDATE_INTERVAL		50
+
+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;
+	int repeat;
+	int last_repeat;
+	int delta_t;
+	bool is_indefinite;
+	bool is_hw_pattern;
+	struct timer_list timer;
+};
+
+static void pattern_trig_update_patterns(struct pattern_trig_data *data)
+{
+	data->curr = data->next;
+	if (!data->is_indefinite && data->curr == data->patterns)
+		data->repeat--;
+
+	if (data->next == data->patterns + data->npatterns - 1)
+		data->next = data->patterns;
+	else
+		data->next++;
+
+	data->delta_t = 0;
+}
+
+static int pattern_trig_compute_brightness(struct pattern_trig_data *data)
+{
+	int step_brightness;
+
+	if (data->delta_t == 0)
+		return data->curr->brightness;
+
+	step_brightness = abs(data->next->brightness - data->curr->brightness);
+	step_brightness = data->delta_t * step_brightness / data->curr->delta_t;
+
+	if (data->next->brightness > data->curr->brightness)
+		return data->curr->brightness + step_brightness;
+	else
+		return data->curr->brightness - step_brightness;
+}
+
+static void pattern_trig_timer_function(struct timer_list *t)
+{
+	struct pattern_trig_data *data = from_timer(data, t, timer);
+
+	mutex_lock(&data->lock);
+
+	for (; ;) {
+		if (!data->is_indefinite && !data->repeat)
+			break;
+
+		if (data->curr->brightness == data->next->brightness) {
+			/* Constant brightness */
+			led_set_brightness(data->led_cdev,
+					   data->curr->brightness);
+			mod_timer(&data->timer,
+				  jiffies + msecs_to_jiffies(data->curr->delta_t));
+
+			/* Skip the step with zero duration */
+			pattern_trig_update_patterns(data);
+			/* Select next step */
+			pattern_trig_update_patterns(data);
+			break;
+		} else {
+			/* Gradual dimming */
+
+			/*
+			 * If the accumulation time is larger than current
+			 * step duration, we should go next one and re-check
+			 * if we repeated done.
+			 */
+			if (data->delta_t > data->curr->delta_t) {
+				pattern_trig_update_patterns(data);
+				continue;
+			}
+
+			led_set_brightness(data->led_cdev,
+					   pattern_trig_compute_brightness(data));
+			mod_timer(&data->timer,
+				  jiffies + msecs_to_jiffies(UPDATE_INTERVAL));
+			/* Accumulate the gradual dimming time */
+			data->delta_t += UPDATE_INTERVAL;
+			break;
+		}
+	}
+
+	mutex_unlock(&data->lock);
+}
+
+static bool pattern_trig_is_sane(struct pattern_trig_data *data)
+{
+	int i;
+
+	/* At least 2 steps for software pattern. */
+	if (data->npatterns < 2)
+		return false;
+
+	/*
+	 * No need valid the duration for constant brightness, just
+	 * return true.
+	 */
+	if (data->patterns[0].brightness == data->patterns[1].brightness)
+		return true;
+
+	/*
+	 * Should valid the duration if is less than 50 ms for gradual
+	 * dimming mode.
+	 */
+	for (i = 0; i < data->npatterns; i++) {
+		if (data->patterns[i].delta_t < UPDATE_INTERVAL)
+			return false;
+	}
+
+	return true;
+}
+
+static int pattern_trig_start_pattern(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+
+	if (!data->npatterns)
+		return 0;
+
+	if (data->is_hw_pattern) {
+		return led_cdev->pattern_set(led_cdev, data->patterns,
+					     data->npatterns, data->repeat);
+	}
+
+	if (!pattern_trig_is_sane(data))
+		return -EINVAL;
+
+	data->delta_t = 0;
+	data->curr = data->patterns;
+	data->next = data->npatterns > 1 ? data->patterns + 1 : data->patterns;
+	data->timer.expires = jiffies;
+	add_timer(&data->timer);
+
+	return 0;
+}
+
+static ssize_t repeat_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+	int repeat;
+
+	mutex_lock(&data->lock);
+
+	repeat = data->last_repeat;
+
+	mutex_unlock(&data->lock);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", repeat);
+}
+
+static ssize_t repeat_store(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;
+	int err, res;
+
+	err = kstrtos32(buf, 10, &res);
+	if (err)
+		return err;
+
+	/* Number 0 and negative numbers except -1 are invalid. */
+	if (res < -1 || res == 0)
+		return -EINVAL;
+
+	/*
+	 * Clear previous patterns' performence firstly, and remove the timer
+	 * without mutex lock to avoid dead lock.
+	 */
+	del_timer_sync(&data->timer);
+
+	mutex_lock(&data->lock);
+
+	if (data->is_hw_pattern)
+		led_cdev->pattern_clear(led_cdev);
+
+	data->last_repeat = data->repeat = res;
+	/* -1 means repeat indefinitely */
+	if (data->repeat == -1)
+		data->is_indefinite = true;
+	else
+		data->is_indefinite = false;
+
+	err = pattern_trig_start_pattern(led_cdev);
+
+	mutex_unlock(&data->lock);
+	return err < 0 ? err : count;
+}
+
+static DEVICE_ATTR_RW(repeat);
+
+static ssize_t pattern_trig_show_patterns(struct pattern_trig_data *data,
+					  char *buf, bool hw_pattern)
+{
+	ssize_t count = 0;
+	int i;
+
+	mutex_lock(&data->lock);
+
+	if (!data->npatterns || (data->is_hw_pattern ^ hw_pattern))
+		goto out;
+
+	for (i = 0; i < data->npatterns; i++) {
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "%d %u ",
+				   data->patterns[i].brightness,
+				   data->patterns[i].delta_t);
+	}
+
+	buf[count - 1] = '\n';
+
+out:
+	mutex_unlock(&data->lock);
+	return count;
+}
+
+static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev,
+					   const char *buf, size_t count,
+					   bool hw_pattern)
+{
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+	int ccount, cr, offset = 0, err = 0;
+
+	/*
+	 * Clear previous patterns' performence firstly, and remove the timer
+	 * without mutex lock to avoid dead lock.
+	 */
+	del_timer_sync(&data->timer);
+
+	mutex_lock(&data->lock);
+
+	if (data->is_hw_pattern)
+		led_cdev->pattern_clear(led_cdev);
+
+	data->is_hw_pattern = hw_pattern;
+	data->npatterns = 0;
+
+	while (offset < count - 1 && data->npatterns < MAX_PATTERNS) {
+		cr = 0;
+		ccount = sscanf(buf + offset, "%d %u %n",
+				&data->patterns[data->npatterns].brightness,
+				&data->patterns[data->npatterns].delta_t, &cr);
+		if (ccount != 2) {
+			data->npatterns = 0;
+			err = -EINVAL;
+			goto out;
+		}
+
+		offset += cr;
+		data->npatterns++;
+	}
+
+	err = pattern_trig_start_pattern(led_cdev);
+	if (err)
+		data->npatterns = 0;
+
+out:
+	mutex_unlock(&data->lock);
+	return err < 0 ? err : count;
+}
+
+static ssize_t pattern_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+
+	return pattern_trig_show_patterns(data, buf, false);
+}
+
+static ssize_t pattern_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return pattern_trig_store_patterns(led_cdev, buf, count, false);
+}
+
+static DEVICE_ATTR_RW(pattern);
+
+static ssize_t hw_pattern_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+
+	return pattern_trig_show_patterns(data, buf, true);
+}
+
+static ssize_t hw_pattern_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return pattern_trig_store_patterns(led_cdev, buf, count, true);
+}
+
+static DEVICE_ATTR_RW(hw_pattern);
+
+static umode_t pattern_trig_attrs_mode(struct kobject *kobj,
+				       struct attribute *attr, int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	if (attr == &dev_attr_repeat.attr || attr == &dev_attr_pattern.attr)
+		return attr->mode;
+	else if (attr == &dev_attr_hw_pattern.attr && led_cdev->pattern_set)
+		return attr->mode;
+
+	return 0;
+}
+
+static struct attribute *pattern_trig_attrs[] = {
+	&dev_attr_pattern.attr,
+	&dev_attr_hw_pattern.attr,
+	&dev_attr_repeat.attr,
+	NULL
+};
+
+static const struct attribute_group pattern_trig_group = {
+	.attrs = pattern_trig_attrs,
+	.is_visible = pattern_trig_attrs_mode,
+};
+
+static const struct attribute_group *pattern_trig_groups[] = {
+	&pattern_trig_group,
+	NULL,
+};
+
+static int pattern_trig_activate(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *data;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	if (!!led_cdev->pattern_set ^ !!led_cdev->pattern_clear) {
+		dev_warn(led_cdev->dev,
+			 "Hardware pattern ops validation failed\n");
+		led_cdev->pattern_set = NULL;
+		led_cdev->pattern_clear = NULL;
+	}
+
+	data->is_indefinite = true;
+	data->last_repeat = -1;
+	mutex_init(&data->lock);
+	data->led_cdev = led_cdev;
+	led_set_trigger_data(led_cdev, data);
+	timer_setup(&data->timer, pattern_trig_timer_function, 0);
+	led_cdev->activated = true;
+
+	return 0;
+}
+
+static void pattern_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+
+	if (!led_cdev->activated)
+		return;
+
+	if (led_cdev->pattern_clear)
+		led_cdev->pattern_clear(led_cdev);
+
+	del_timer_sync(&data->timer);
+
+	led_set_brightness(led_cdev, LED_OFF);
+	kfree(data);
+	led_cdev->activated = false;
+}
+
+static struct led_trigger pattern_led_trigger = {
+	.name = "pattern",
+	.activate = pattern_trig_activate,
+	.deactivate = pattern_trig_deactivate,
+	.groups = pattern_trig_groups,
+};
+
+static int __init pattern_trig_init(void)
+{
+	return led_trigger_register(&pattern_led_trigger);
+}
+
+static void __exit pattern_trig_exit(void)
+{
+	led_trigger_unregister(&pattern_led_trigger);
+}
+
+module_init(pattern_trig_init);
+module_exit(pattern_trig_exit);
+
+MODULE_AUTHOR("Raphael Teysseyre <rteysseyre@gmail.com");
+MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org");
+MODULE_DESCRIPTION("LED Pattern trigger");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 834683d..7393a31 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -22,6 +22,7 @@
 #include <linux/workqueue.h>
 
 struct device;
+struct led_pattern;
 /*
  * LED Core
  */
@@ -88,6 +89,10 @@ struct led_classdev {
 				     unsigned long *delay_on,
 				     unsigned long *delay_off);
 
+	int (*pattern_set)(struct led_classdev *led_cdev,
+			   struct led_pattern *pattern, u32 len, int repeat);
+	int (*pattern_clear)(struct led_classdev *led_cdev);
+
 	struct device		*dev;
 	const struct attribute_group	**groups;
 
@@ -472,4 +477,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
 	struct led_classdev *led_cdev, enum led_brightness brightness) { }
 #endif
 
+/**
+ * struct led_pattern - pattern interval settings
+ * @delta_t: pattern interval delay, in milliseconds
+ * @brightness: pattern interval brightness
+ */
+struct led_pattern {
+	u32 delta_t;
+	int brightness;
+};
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
1.9.1

[PATCH v14 2/2] leds: sc27xx: Add pattern_set/clear interfaces for LED controller

[Baolin Wang]

This patch implements the 'pattern_set'and 'pattern_clear'
interfaces to support SC27XX LED breathing mode.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Acked-by: Pavel Machek <pavel@ucw.c>
---
Changes from v13:
 - None.

Changes from v12:
 - None.

Changes from v11:
 - None.

Changes from v10:
 - Add duration alignment function suggested by Jacek.
 - Add acked tag from Pavel.

Changes from v9:
 - Optimize the ABI documentation file.
 - Update the brightness value in hardware pattern mode.

Changes from v8:
 - Optimize the ABI documentation file.

Changes from v7:
 - Add its own ABI documentation file.

Changes from v6:
 - None.

Changes from v5:
 - None.

Changes from v4:
 - None.

Changes from v3:
- None.

Changes from v2:
 - None.

Changes from v1:
 - Remove pattern_get interface.
---
 .../ABI/testing/sysfs-class-led-driver-sc27xx      |  22 ++++
 drivers/leds/leds-sc27xx-bltc.c                    | 121 +++++++++++++++++++++
 2 files changed, 143 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx

diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
new file mode 100644
index 0000000..45b1e60
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
@@ -0,0 +1,22 @@
+What:		/sys/class/leds/<led>/hw_pattern
+Date:		September 2018
+KernelVersion:	4.20
+Description:
+		Specify a hardware pattern for the SC27XX LED. For the SC27XX
+		LED controller, it only supports 4 stages to make a single
+		hardware pattern, which is used to configure the rise time,
+		high time, fall time and low time for the breathing mode.
+
+		For the breathing mode, the SC27XX LED only expects one brightness
+		for the high stage. To be compatible with the hardware pattern
+		format, we should set brightness as 0 for rise stage, fall
+		stage and low stage.
+
+		Min stage duration: 125 ms
+		Max stage duration: 31875 ms
+
+		Since the stage duration step is 125 ms, the duration should be
+		a multiplier of 125, like 125ms, 250ms, 375ms, 500ms ... 31875ms.
+
+		Thus the format of the hardware pattern values should be:
+		"0 rise_duration brightness high_duration 0 fall_duration 0 low_duration".
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..fecf27f 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -32,8 +32,18 @@
 #define SC27XX_DUTY_MASK	GENMASK(15, 0)
 #define SC27XX_MOD_MASK		GENMASK(7, 0)
 
+#define SC27XX_CURVE_SHIFT	8
+#define SC27XX_CURVE_L_MASK	GENMASK(7, 0)
+#define SC27XX_CURVE_H_MASK	GENMASK(15, 8)
+
 #define SC27XX_LEDS_OFFSET	0x10
 #define SC27XX_LEDS_MAX		3
+#define SC27XX_LEDS_PATTERN_CNT	4
+/* Stage duration step, in milliseconds */
+#define SC27XX_LEDS_STEP	125
+/* Minimum and maximum duration, in milliseconds */
+#define SC27XX_DELTA_T_MIN	SC27XX_LEDS_STEP
+#define SC27XX_DELTA_T_MAX	(SC27XX_LEDS_STEP * 255)
 
 struct sc27xx_led {
 	char name[LED_MAX_NAME_SIZE];
@@ -122,6 +132,113 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum led_brightness value)
 	return err;
 }
 
+static void sc27xx_led_clamp_align_delta_t(u32 *delta_t)
+{
+	u32 v, offset, t = *delta_t;
+
+	v = t + SC27XX_LEDS_STEP / 2;
+	v = clamp_t(u32, v, SC27XX_DELTA_T_MIN, SC27XX_DELTA_T_MAX);
+	offset = v - SC27XX_DELTA_T_MIN;
+	offset = SC27XX_LEDS_STEP * (offset / SC27XX_LEDS_STEP);
+
+	*delta_t = SC27XX_DELTA_T_MIN + offset;
+}
+
+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(&leds->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);
+
+	ldev->brightness = LED_OFF;
+
+	mutex_unlock(&leds->priv->lock);
+
+	return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+				  struct led_pattern *pattern,
+				  u32 len, int repeat)
+{
+	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 tuples 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(&leds->priv->lock);
+
+	sc27xx_led_clamp_align_delta_t(&pattern[0].delta_t);
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+				 SC27XX_CURVE_L_MASK,
+				 pattern[0].delta_t / SC27XX_LEDS_STEP);
+	if (err)
+		goto out;
+
+	sc27xx_led_clamp_align_delta_t(&pattern[1].delta_t);
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+				 SC27XX_CURVE_L_MASK,
+				 pattern[1].delta_t / SC27XX_LEDS_STEP);
+	if (err)
+		goto out;
+
+	sc27xx_led_clamp_align_delta_t(&pattern[2].delta_t);
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+				 SC27XX_CURVE_H_MASK,
+				 (pattern[2].delta_t / SC27XX_LEDS_STEP) <<
+				 SC27XX_CURVE_SHIFT);
+	if (err)
+		goto out;
+
+	sc27xx_led_clamp_align_delta_t(&pattern[3].delta_t);
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+				 SC27XX_CURVE_H_MASK,
+				 (pattern[3].delta_t / SC27XX_LEDS_STEP) <<
+				 SC27XX_CURVE_SHIFT);
+	if (err)
+		goto out;
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_DUTY,
+				 SC27XX_DUTY_MASK,
+				 (pattern[1].brightness << SC27XX_DUTY_SHIFT) |
+				 SC27XX_MOD_MASK);
+	if (err)
+		goto out;
+
+	/* Enable the LED breathing mode */
+	err = regmap_update_bits(regmap, ctrl_base,
+				 SC27XX_LED_RUN << ctrl_shift,
+				 SC27XX_LED_RUN << ctrl_shift);
+	if (!err)
+		ldev->brightness = pattern[1].brightness;
+
+out:
+	mutex_unlock(&leds->priv->lock);
+
+	return err;
+}
+
 static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv)
 {
 	int i, err;
@@ -140,6 +257,9 @@ static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv)
 		led->priv = priv;
 		led->ldev.name = led->name;
 		led->ldev.brightness_set_blocking = sc27xx_led_set;
+		led->ldev.pattern_set = sc27xx_led_pattern_set;
+		led->ldev.pattern_clear = sc27xx_led_pattern_clear;
+		led->ldev.default_trigger = "pattern";
 
 		err = devm_led_classdev_register(dev, &led->ldev);
 		if (err)
@@ -241,4 +361,5 @@ static int sc27xx_led_remove(struct platform_device *pdev)
 
 MODULE_DESCRIPTION("Spreadtrum SC27xx breathing light controller driver");
 MODULE_AUTHOR("Xiaotong Lu <xiaotong.lu@spreadtrum.com>");
+MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
 MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Jacek Anaszewski]

Hi Baolin,

Thank you for the v14. We'll probably need v15, though :-)

I added the comments in the code below.

On 10/02/2018 05:43 PM, Baolin Wang wrote:
> This patch adds one new led trigger that LED device can configure
> the software or hardware pattern and trigger it.
> 
> Consumers can write 'pattern' file to enable the software pattern
> which alters the brightness for the specified duration with one
> software timer.
> 
> Moreover consumers can write 'hw_pattern' file to enable the hardware
> pattern for some LED controllers which can autonomously control
> brightness over time, according to some preprogrammed hardware
> patterns.
> 
> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v13:
>  - Add duration validation for gradual dimming.
>  - Coding style optimization.
> 
> Changes from v12:
>  - Add gradual dimming support for software pattern.
> 
> Changes from v11:
>  - Change -1 means repeat indefinitely.
> 
> Changes from v10:
>  - Change 'int' to 'u32' for delta_t field.
> 
> Changes from v9:
>  - None.
> 
> Changes from v8:
>  - None.
> 
> Changes from v7:
>  - Move the SC27XX hardware patterns description into its own ABI file.
> 
> Changes from v6:
>  - Improve commit message.
>  - Optimize the description of the hw_pattern file.
>  - Simplify some logics.
> 
> Changes from v5:
>  - Add one 'hw_pattern' file for hardware patterns.
> 
> Changes from v4:
>  - Change the repeat file to return the originally written number.
>  - Improve comments.
>  - Fix some build warnings.
> 
> 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    |  76 ++++
>  drivers/leds/trigger/Kconfig                       |   7 +
>  drivers/leds/trigger/Makefile                      |   1 +
>  drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
>  include/linux/leds.h                               |  15 +
>  5 files changed, 530 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 0000000..22d7af7
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> @@ -0,0 +1,76 @@
> +What:		/sys/class/leds/<led>/pattern
> +Date:		September 2018
> +KernelVersion:	4.20
> +Description:
> +		Specify a software pattern for the LED, that supports altering
> +		the brightness for the specified duration with one software
> +		timer. It can do gradual dimming and constant brightness.
> +
> +		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.
> +
> +		1. When doing gradual dimming, the led brightness will be updated
> +		every 50 milliseconds, so the duration of each step should not
> +		less than 50 milliseconds.

I'd like to avoid this constraint. Lowest supported delta_t should be 1.

We should only prevent entering dimming mode if current delta_t
is lower than UPDATE_INTERVAL.

I also propose to make the dimming interval configurable via sysfs
dimming_interval file.

> The gradual dimming format of the
> +		software pattern values should be:
> +		"brightness_1 duration_1 brightness_2 duration_2 brightness_3
> +		duration_3 ...". For example:
> +
> +		echo 0 1000 255 2000 > pattern
> +
> +		It will make the LED go gradually from zero-intensity to max (255)
> +		intensity in 1000 milliseconds, then back to zero intensity in 2000
> +		milliseconds:
> +
> +		LED brightness
> +		    ^
> +		255-|       / \            / \            /
> +		    |      /    \         /    \         /
> +		    |     /       \      /       \      /
> +		    |    /          \   /          \   /
> +		  0-|   /             \/             \/
> +		    +---0----1----2----3----4----5----6------------> time (s)
> +
> +		2. To make the LED go instantly from one brigntess value to another,
> +		we should use use zero-time lengths. So the format should be:
> +		"brightness_1 duration_1 brightness_1 0 brightness_2 duration_2
> +		brightness_2 0 ...". For example:
> +
> +		echo 0 1000 0 0 255 2000 255 0 > pattern
> +
> +		It will make the LED stay off for one second, then stay at max brightness
> +		for two seconds:
> +
> +		LED brightness
> +		    ^
> +		255-|        +---------+    +---------+
> +		    |        |         |    |         |
> +		    |        |         |    |         |
> +		    |        |         |    |         |
> +		  0-|   -----+         +----+         +----
> +		    +---0----1----2----3----4----5----6------------> time (s)
> +
> +What:		/sys/class/leds/<led>/hw_pattern
> +Date:		September 2018
> +KernelVersion:	4.20
> +Description:
> +		Specify a hardware pattern for the LED, for LED hardware that
> +		supports autonomously controlling brightness over time, according
> +		to some preprogrammed hardware patterns.
> +
> +		Since different LED hardware can have different semantics of
> +		hardware patterns, each driver is expected to provide its own
> +		description for the hardware patterns in their ABI documentation
> +		file.
> +
> +What:		/sys/class/leds/<led>/repeat
> +Date:		September 2018
> +KernelVersion:	4.20
> +Description:
> +		Specify a pattern repeat number. -1 means repeat indefinitely,
> +		other negative numbers and number 0 are invalid.
> +
> +		This file will always return the originally written repeat
> +		number.
> 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 0000000..92a0fd0
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,431 @@
> +// 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 <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +
> +#define MAX_PATTERNS		1024
> +/*
> + * When doing gradual dimming, the led brightness will be updated
> + * every 50 milliseconds.
> + */
> +#define UPDATE_INTERVAL		50
> +
> +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;
> +	int repeat;
> +	int last_repeat;
> +	int delta_t;
> +	bool is_indefinite;
> +	bool is_hw_pattern;
> +	struct timer_list timer;
> +};
> +
> +static void pattern_trig_update_patterns(struct pattern_trig_data *data)
> +{
> +	data->curr = data->next;
> +	if (!data->is_indefinite && data->curr == data->patterns)
> +		data->repeat--;
> +
> +	if (data->next == data->patterns + data->npatterns - 1)
> +		data->next = data->patterns;
> +	else
> +		data->next++;
> +
> +	data->delta_t = 0;
> +}
> +
> +static int pattern_trig_compute_brightness(struct pattern_trig_data *data)
> +{
> +	int step_brightness;
> +
> +	if (data->delta_t == 0)
> +		return data->curr->brightness;
> +
> +	step_brightness = abs(data->next->brightness - data->curr->brightness);
> +	step_brightness = data->delta_t * step_brightness / data->curr->delta_t;
> +
> +	if (data->next->brightness > data->curr->brightness)
> +		return data->curr->brightness + step_brightness;
> +	else
> +		return data->curr->brightness - step_brightness;
> +}
> +
> +static void pattern_trig_timer_function(struct timer_list *t)
> +{
> +	struct pattern_trig_data *data = from_timer(data, t, timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	for (; ;) {

This whitespace between semicolons looks odd. Please remove it.

> +		if (!data->is_indefinite && !data->repeat)
> +			break;
> +
> +		if (data->curr->brightness == data->next->brightness) {
> +			/* Constant brightness */
> +			led_set_brightness(data->led_cdev,
> +					   data->curr->brightness);
> +			mod_timer(&data->timer,
> +				  jiffies + msecs_to_jiffies(data->curr->delta_t));
> +
> +			/* Skip the step with zero duration */
> +			pattern_trig_update_patterns(data);
> +			/* Select next step */
> +			pattern_trig_update_patterns(data);
> +			break;
> +		} else {
> +			/* Gradual dimming */
> +
> +			/*
> +			 * If the accumulation time is larger than current
> +			 * step duration, we should go next one and re-check
> +			 * if we repeated done.
> +			 */
> +			if (data->delta_t > data->curr->delta_t) {
> +				pattern_trig_update_patterns(data);
> +				continue;
> +			}
> +
> +			led_set_brightness(data->led_cdev,
> +					   pattern_trig_compute_brightness(data));
> +			mod_timer(&data->timer,
> +				  jiffies + msecs_to_jiffies(UPDATE_INTERVAL));
> +			/* Accumulate the gradual dimming time */
> +			data->delta_t += UPDATE_INTERVAL;
> +			break;
> +		}

Why two "break" statements instead of one here?

> +	}
> +
> +	mutex_unlock(&data->lock);
> +}
> +
> +static bool pattern_trig_is_sane(struct pattern_trig_data *data)
> +{
> +	int i;
> +
> +	/* At least 2 steps for software pattern. */
> +	if (data->npatterns < 2)
> +		return false;
> +
> +	/*
> +	 * No need valid the duration for constant brightness, just
> +	 * return true.
> +	 */
> +	if (data->patterns[0].brightness == data->patterns[1].brightness)
> +		return true;
> +
> +	/*
> +	 * Should valid the duration if is less than 50 ms for gradual
> +	 * dimming mode.
> +	 */
> +	for (i = 0; i < data->npatterns; i++) {
> +		if (data->patterns[i].delta_t < UPDATE_INTERVAL)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int pattern_trig_start_pattern(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +
> +	if (!data->npatterns)
> +		return 0;
> +
> +	if (data->is_hw_pattern) {
> +		return led_cdev->pattern_set(led_cdev, data->patterns,
> +					     data->npatterns, data->repeat);
> +	}
> +
> +	if (!pattern_trig_is_sane(data))
> +		return -EINVAL;
> +
> +	data->delta_t = 0;
> +	data->curr = data->patterns;
> +	data->next = data->npatterns > 1 ? data->patterns + 1 : data->patterns;
> +	data->timer.expires = jiffies;
> +	add_timer(&data->timer);
> +
> +	return 0;
> +}
> +
> +static ssize_t repeat_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +	int repeat;
> +
> +	mutex_lock(&data->lock);
> +
> +	repeat = data->last_repeat;
> +
> +	mutex_unlock(&data->lock);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", repeat);
> +}
> +
> +static ssize_t repeat_store(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;
> +	int err, res;
> +
> +	err = kstrtos32(buf, 10, &res);
> +	if (err)
> +		return err;
> +
> +	/* Number 0 and negative numbers except -1 are invalid. */
> +	if (res < -1 || res == 0)
> +		return -EINVAL;
> +
> +	/*
> +	 * Clear previous patterns' performence firstly, and remove the timer
> +	 * without mutex lock to avoid dead lock.
> +	 */
> +	del_timer_sync(&data->timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	if (data->is_hw_pattern)
> +		led_cdev->pattern_clear(led_cdev);
> +
> +	data->last_repeat = data->repeat = res;
> +	/* -1 means repeat indefinitely */
> +	if (data->repeat == -1)
> +		data->is_indefinite = true;
> +	else
> +		data->is_indefinite = false;
> +
> +	err = pattern_trig_start_pattern(led_cdev);
> +
> +	mutex_unlock(&data->lock);
> +	return err < 0 ? err : count;
> +}
> +
> +static DEVICE_ATTR_RW(repeat);
> +
> +static ssize_t pattern_trig_show_patterns(struct pattern_trig_data *data,
> +					  char *buf, bool hw_pattern)
> +{
> +	ssize_t count = 0;
> +	int i;
> +
> +	mutex_lock(&data->lock);
> +
> +	if (!data->npatterns || (data->is_hw_pattern ^ hw_pattern))
> +		goto out;
> +
> +	for (i = 0; i < data->npatterns; i++) {
> +		count += scnprintf(buf + count, PAGE_SIZE - count,
> +				   "%d %u ",
> +				   data->patterns[i].brightness,
> +				   data->patterns[i].delta_t);
> +	}
> +
> +	buf[count - 1] = '\n';
> +
> +out:
> +	mutex_unlock(&data->lock);
> +	return count;
> +}
> +
> +static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev,
> +					   const char *buf, size_t count,
> +					   bool hw_pattern)
> +{
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +	int ccount, cr, offset = 0, err = 0;
> +
> +	/*
> +	 * Clear previous patterns' performence firstly, and remove the timer
> +	 * without mutex lock to avoid dead lock.
> +	 */
> +	del_timer_sync(&data->timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	if (data->is_hw_pattern)
> +		led_cdev->pattern_clear(led_cdev);
> +
> +	data->is_hw_pattern = hw_pattern;
> +	data->npatterns = 0;
> +
> +	while (offset < count - 1 && data->npatterns < MAX_PATTERNS) {
> +		cr = 0;
> +		ccount = sscanf(buf + offset, "%d %u %n",
> +				&data->patterns[data->npatterns].brightness,
> +				&data->patterns[data->npatterns].delta_t, &cr);
> +		if (ccount != 2) {
> +			data->npatterns = 0;
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		offset += cr;
> +		data->npatterns++;
> +	}
> +
> +	err = pattern_trig_start_pattern(led_cdev);
> +	if (err)
> +		data->npatterns = 0;
> +
> +out:
> +	mutex_unlock(&data->lock);
> +	return err < 0 ? err : count;
> +}
> +
> +static ssize_t pattern_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +
> +	return pattern_trig_show_patterns(data, buf, false);
> +}
> +
> +static ssize_t pattern_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	return pattern_trig_store_patterns(led_cdev, buf, count, false);
> +}
> +
> +static DEVICE_ATTR_RW(pattern);
> +
> +static ssize_t hw_pattern_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +
> +	return pattern_trig_show_patterns(data, buf, true);
> +}
> +
> +static ssize_t hw_pattern_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	return pattern_trig_store_patterns(led_cdev, buf, count, true);
> +}
> +
> +static DEVICE_ATTR_RW(hw_pattern);
> +
> +static umode_t pattern_trig_attrs_mode(struct kobject *kobj,
> +				       struct attribute *attr, int index)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	if (attr == &dev_attr_repeat.attr || attr == &dev_attr_pattern.attr)
> +		return attr->mode;
> +	else if (attr == &dev_attr_hw_pattern.attr && led_cdev->pattern_set)
> +		return attr->mode;
> +
> +	return 0;
> +}
> +
> +static struct attribute *pattern_trig_attrs[] = {
> +	&dev_attr_pattern.attr,
> +	&dev_attr_hw_pattern.attr,
> +	&dev_attr_repeat.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group pattern_trig_group = {
> +	.attrs = pattern_trig_attrs,
> +	.is_visible = pattern_trig_attrs_mode,
> +};
> +
> +static const struct attribute_group *pattern_trig_groups[] = {
> +	&pattern_trig_group,
> +	NULL,
> +};
> +
> +static int pattern_trig_activate(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	if (!!led_cdev->pattern_set ^ !!led_cdev->pattern_clear) {
> +		dev_warn(led_cdev->dev,
> +			 "Hardware pattern ops validation failed\n");
> +		led_cdev->pattern_set = NULL;
> +		led_cdev->pattern_clear = NULL;
> +	}
> +
> +	data->is_indefinite = true;
> +	data->last_repeat = -1;
> +	mutex_init(&data->lock);
> +	data->led_cdev = led_cdev;
> +	led_set_trigger_data(led_cdev, data);
> +	timer_setup(&data->timer, pattern_trig_timer_function, 0);
> +	led_cdev->activated = true;
> +
> +	return 0;
> +}
> +
> +static void pattern_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +
> +	if (!led_cdev->activated)
> +		return;
> +
> +	if (led_cdev->pattern_clear)
> +		led_cdev->pattern_clear(led_cdev);
> +
> +	del_timer_sync(&data->timer);
> +
> +	led_set_brightness(led_cdev, LED_OFF);
> +	kfree(data);
> +	led_cdev->activated = false;
> +}
> +
> +static struct led_trigger pattern_led_trigger = {
> +	.name = "pattern",
> +	.activate = pattern_trig_activate,
> +	.deactivate = pattern_trig_deactivate,
> +	.groups = pattern_trig_groups,
> +};
> +
> +static int __init pattern_trig_init(void)
> +{
> +	return led_trigger_register(&pattern_led_trigger);
> +}
> +
> +static void __exit pattern_trig_exit(void)
> +{
> +	led_trigger_unregister(&pattern_led_trigger);
> +}
> +
> +module_init(pattern_trig_init);
> +module_exit(pattern_trig_exit);
> +
> +MODULE_AUTHOR("Raphael Teysseyre <rteysseyre@gmail.com");
> +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org");
> +MODULE_DESCRIPTION("LED Pattern trigger");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 834683d..7393a31 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -22,6 +22,7 @@
>  #include <linux/workqueue.h>
>  
>  struct device;
> +struct led_pattern;
>  /*
>   * LED Core
>   */
> @@ -88,6 +89,10 @@ struct led_classdev {
>  				     unsigned long *delay_on,
>  				     unsigned long *delay_off);
>  
> +	int (*pattern_set)(struct led_classdev *led_cdev,
> +			   struct led_pattern *pattern, u32 len, int repeat);
> +	int (*pattern_clear)(struct led_classdev *led_cdev);
> +
>  	struct device		*dev;
>  	const struct attribute_group	**groups;
>  
> @@ -472,4 +477,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
>  	struct led_classdev *led_cdev, enum led_brightness brightness) { }
>  #endif
>  
> +/**
> + * struct led_pattern - pattern interval settings
> + * @delta_t: pattern interval delay, in milliseconds
> + * @brightness: pattern interval brightness
> + */
> +struct led_pattern {
> +	u32 delta_t;
> +	int brightness;
> +};
> +
>  #endif		/* __LINUX_LEDS_H_INCLUDED */
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Baolin Wang]

Hi Jacek,

On 3 October 2018 at 04:25, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> Hi Baolin,
>
> Thank you for the v14. We'll probably need v15, though :-)
>
> I added the comments in the code below.
>
> On 10/02/2018 05:43 PM, Baolin Wang wrote:
>> This patch adds one new led trigger that LED device can configure
>> the software or hardware pattern and trigger it.
>>
>> Consumers can write 'pattern' file to enable the software pattern
>> which alters the brightness for the specified duration with one
>> software timer.
>>
>> Moreover consumers can write 'hw_pattern' file to enable the hardware
>> pattern for some LED controllers which can autonomously control
>> brightness over time, according to some preprogrammed hardware
>> patterns.
>>
>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v13:
>>  - Add duration validation for gradual dimming.
>>  - Coding style optimization.
>>
>> Changes from v12:
>>  - Add gradual dimming support for software pattern.
>>
>> Changes from v11:
>>  - Change -1 means repeat indefinitely.
>>
>> Changes from v10:
>>  - Change 'int' to 'u32' for delta_t field.
>>
>> Changes from v9:
>>  - None.
>>
>> Changes from v8:
>>  - None.
>>
>> Changes from v7:
>>  - Move the SC27XX hardware patterns description into its own ABI file.
>>
>> Changes from v6:
>>  - Improve commit message.
>>  - Optimize the description of the hw_pattern file.
>>  - Simplify some logics.
>>
>> Changes from v5:
>>  - Add one 'hw_pattern' file for hardware patterns.
>>
>> Changes from v4:
>>  - Change the repeat file to return the originally written number.
>>  - Improve comments.
>>  - Fix some build warnings.
>>
>> 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    |  76 ++++
>>  drivers/leds/trigger/Kconfig                       |   7 +
>>  drivers/leds/trigger/Makefile                      |   1 +
>>  drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
>>  include/linux/leds.h                               |  15 +
>>  5 files changed, 530 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 0000000..22d7af7
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> @@ -0,0 +1,76 @@
>> +What:                /sys/class/leds/<led>/pattern
>> +Date:                September 2018
>> +KernelVersion:       4.20
>> +Description:
>> +             Specify a software pattern for the LED, that supports altering
>> +             the brightness for the specified duration with one software
>> +             timer. It can do gradual dimming and constant brightness.
>> +
>> +             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.
>> +
>> +             1. When doing gradual dimming, the led brightness will be updated
>> +             every 50 milliseconds, so the duration of each step should not
>> +             less than 50 milliseconds.
>
> I'd like to avoid this constraint. Lowest supported delta_t should be 1.
>
> We should only prevent entering dimming mode if current delta_t
> is lower than UPDATE_INTERVAL.

I do not think so. If the pattern format is used for dimming and the
delta_t is lower than UPDATE_INTERVAL, we should return errors. Since
in this case, we can not change to constant mode.

Like you mentioned:

echo "10 49 20 49" > pattern

We can not enter dimming, meanwhile the pattern format is not used for
constant brightness, so should return errors for the invalid pattern
values.

> I also propose to make the dimming interval configurable via sysfs
> dimming_interval file.

So the dimming_interva range is [1, 50] milliseconds?

I am fine with this. Pavel, do you also agree with adding one new file
to set the dimming interval or keep it as simple now?

>
>> The gradual dimming format of the
>> +             software pattern values should be:
>> +             "brightness_1 duration_1 brightness_2 duration_2 brightness_3
>> +             duration_3 ...". For example:
>> +
>> +             echo 0 1000 255 2000 > pattern
>> +
>> +             It will make the LED go gradually from zero-intensity to max (255)
>> +             intensity in 1000 milliseconds, then back to zero intensity in 2000
>> +             milliseconds:
>> +
>> +             LED brightness
>> +                 ^
>> +             255-|       / \            / \            /
>> +                 |      /    \         /    \         /
>> +                 |     /       \      /       \      /
>> +                 |    /          \   /          \   /
>> +               0-|   /             \/             \/
>> +                 +---0----1----2----3----4----5----6------------> time (s)
>> +
>> +             2. To make the LED go instantly from one brigntess value to another,
>> +             we should use use zero-time lengths. So the format should be:
>> +             "brightness_1 duration_1 brightness_1 0 brightness_2 duration_2
>> +             brightness_2 0 ...". For example:
>> +
>> +             echo 0 1000 0 0 255 2000 255 0 > pattern
>> +
>> +             It will make the LED stay off for one second, then stay at max brightness
>> +             for two seconds:
>> +
>> +             LED brightness
>> +                 ^
>> +             255-|        +---------+    +---------+
>> +                 |        |         |    |         |
>> +                 |        |         |    |         |
>> +                 |        |         |    |         |
>> +               0-|   -----+         +----+         +----
>> +                 +---0----1----2----3----4----5----6------------> time (s)
>> +
>> +What:                /sys/class/leds/<led>/hw_pattern
>> +Date:                September 2018
>> +KernelVersion:       4.20
>> +Description:
>> +             Specify a hardware pattern for the LED, for LED hardware that
>> +             supports autonomously controlling brightness over time, according
>> +             to some preprogrammed hardware patterns.
>> +
>> +             Since different LED hardware can have different semantics of
>> +             hardware patterns, each driver is expected to provide its own
>> +             description for the hardware patterns in their ABI documentation
>> +             file.
>> +
>> +What:                /sys/class/leds/<led>/repeat
>> +Date:                September 2018
>> +KernelVersion:       4.20
>> +Description:
>> +             Specify a pattern repeat number. -1 means repeat indefinitely,
>> +             other negative numbers and number 0 are invalid.
>> +
>> +             This file will always return the originally written repeat
>> +             number.
>> 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 0000000..92a0fd0
>> --- /dev/null
>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>> @@ -0,0 +1,431 @@
>> +// 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 <linux/kernel.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/slab.h>
>> +#include <linux/timer.h>
>> +
>> +#define MAX_PATTERNS         1024
>> +/*
>> + * When doing gradual dimming, the led brightness will be updated
>> + * every 50 milliseconds.
>> + */
>> +#define UPDATE_INTERVAL              50
>> +
>> +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;
>> +     int repeat;
>> +     int last_repeat;
>> +     int delta_t;
>> +     bool is_indefinite;
>> +     bool is_hw_pattern;
>> +     struct timer_list timer;
>> +};
>> +
>> +static void pattern_trig_update_patterns(struct pattern_trig_data *data)
>> +{
>> +     data->curr = data->next;
>> +     if (!data->is_indefinite && data->curr == data->patterns)
>> +             data->repeat--;
>> +
>> +     if (data->next == data->patterns + data->npatterns - 1)
>> +             data->next = data->patterns;
>> +     else
>> +             data->next++;
>> +
>> +     data->delta_t = 0;
>> +}
>> +
>> +static int pattern_trig_compute_brightness(struct pattern_trig_data *data)
>> +{
>> +     int step_brightness;
>> +
>> +     if (data->delta_t == 0)
>> +             return data->curr->brightness;
>> +
>> +     step_brightness = abs(data->next->brightness - data->curr->brightness);
>> +     step_brightness = data->delta_t * step_brightness / data->curr->delta_t;
>> +
>> +     if (data->next->brightness > data->curr->brightness)
>> +             return data->curr->brightness + step_brightness;
>> +     else
>> +             return data->curr->brightness - step_brightness;
>> +}
>> +
>> +static void pattern_trig_timer_function(struct timer_list *t)
>> +{
>> +     struct pattern_trig_data *data = from_timer(data, t, timer);
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     for (; ;) {
>
> This whitespace between semicolons looks odd. Please remove it.

Sure.

>
>> +             if (!data->is_indefinite && !data->repeat)
>> +                     break;
>> +
>> +             if (data->curr->brightness == data->next->brightness) {
>> +                     /* Constant brightness */
>> +                     led_set_brightness(data->led_cdev,
>> +                                        data->curr->brightness);
>> +                     mod_timer(&data->timer,
>> +                               jiffies + msecs_to_jiffies(data->curr->delta_t));
>> +
>> +                     /* Skip the step with zero duration */
>> +                     pattern_trig_update_patterns(data);
>> +                     /* Select next step */
>> +                     pattern_trig_update_patterns(data);
>> +                     break;
>> +             } else {
>> +                     /* Gradual dimming */
>> +
>> +                     /*
>> +                      * If the accumulation time is larger than current
>> +                      * step duration, we should go next one and re-check
>> +                      * if we repeated done.
>> +                      */
>> +                     if (data->delta_t > data->curr->delta_t) {
>> +                             pattern_trig_update_patterns(data);
>> +                             continue;
>> +                     }
>> +
>> +                     led_set_brightness(data->led_cdev,
>> +                                        pattern_trig_compute_brightness(data));
>> +                     mod_timer(&data->timer,
>> +                               jiffies + msecs_to_jiffies(UPDATE_INTERVAL));
>> +                     /* Accumulate the gradual dimming time */
>> +                     data->delta_t += UPDATE_INTERVAL;
>> +                     break;
>> +             }
>
> Why two "break" statements instead of one here?

Yes, can remove one. Sorry for the quick version patch set without
much self-checking in the night. Thanks for your comments.

-- 
Baolin Wang
Best Regards

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Jacek Anaszewski]

Hi Baolin,

On 10/03/2018 03:21 AM, Baolin Wang wrote:
> Hi Jacek,
> 
> On 3 October 2018 at 04:25, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>> Hi Baolin,
>>
>> Thank you for the v14. We'll probably need v15, though :-)
>>
>> I added the comments in the code below.
>>
>> On 10/02/2018 05:43 PM, Baolin Wang wrote:
>>> This patch adds one new led trigger that LED device can configure
>>> the software or hardware pattern and trigger it.
>>>
>>> Consumers can write 'pattern' file to enable the software pattern
>>> which alters the brightness for the specified duration with one
>>> software timer.
>>>
>>> Moreover consumers can write 'hw_pattern' file to enable the hardware
>>> pattern for some LED controllers which can autonomously control
>>> brightness over time, according to some preprogrammed hardware
>>> patterns.
>>>
>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>> Changes from v13:
>>>  - Add duration validation for gradual dimming.
>>>  - Coding style optimization.
>>>
>>> Changes from v12:
>>>  - Add gradual dimming support for software pattern.
>>>
>>> Changes from v11:
>>>  - Change -1 means repeat indefinitely.
>>>
>>> Changes from v10:
>>>  - Change 'int' to 'u32' for delta_t field.
>>>
>>> Changes from v9:
>>>  - None.
>>>
>>> Changes from v8:
>>>  - None.
>>>
>>> Changes from v7:
>>>  - Move the SC27XX hardware patterns description into its own ABI file.
>>>
>>> Changes from v6:
>>>  - Improve commit message.
>>>  - Optimize the description of the hw_pattern file.
>>>  - Simplify some logics.
>>>
>>> Changes from v5:
>>>  - Add one 'hw_pattern' file for hardware patterns.
>>>
>>> Changes from v4:
>>>  - Change the repeat file to return the originally written number.
>>>  - Improve comments.
>>>  - Fix some build warnings.
>>>
>>> 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    |  76 ++++
>>>  drivers/leds/trigger/Kconfig                       |   7 +
>>>  drivers/leds/trigger/Makefile                      |   1 +
>>>  drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
>>>  include/linux/leds.h                               |  15 +
>>>  5 files changed, 530 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 0000000..22d7af7
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>> @@ -0,0 +1,76 @@
>>> +What:                /sys/class/leds/<led>/pattern
>>> +Date:                September 2018
>>> +KernelVersion:       4.20
>>> +Description:
>>> +             Specify a software pattern for the LED, that supports altering
>>> +             the brightness for the specified duration with one software
>>> +             timer. It can do gradual dimming and constant brightness.
>>> +
>>> +             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.
>>> +
>>> +             1. When doing gradual dimming, the led brightness will be updated
>>> +             every 50 milliseconds, so the duration of each step should not
>>> +             less than 50 milliseconds.
>>
>> I'd like to avoid this constraint. Lowest supported delta_t should be 1.
>>
>> We should only prevent entering dimming mode if current delta_t
>> is lower than UPDATE_INTERVAL.
> 
> I do not think so. If the pattern format is used for dimming and the
> delta_t is lower than UPDATE_INTERVAL, we should return errors. Since
> in this case, we can not change to constant mode.

I'll play with the code at weekend and will let you know my findings.

Best regards,
Jacek Anaszewski


> Like you mentioned:
> 
> echo "10 49 20 49" > pattern
> 
> We can not enter dimming, meanwhile the pattern format is not used for
> constant brightness, so should return errors for the invalid pattern
> values.
> 
>> I also propose to make the dimming interval configurable via sysfs
>> dimming_interval file.
> 
> So the dimming_interva range is [1, 50] milliseconds?
> 
> I am fine with this. Pavel, do you also agree with adding one new file
> to set the dimming interval or keep it as simple now?
> 
>>
>>> The gradual dimming format of the
>>> +             software pattern values should be:
>>> +             "brightness_1 duration_1 brightness_2 duration_2 brightness_3
>>> +             duration_3 ...". For example:
>>> +
>>> +             echo 0 1000 255 2000 > pattern
>>> +
>>> +             It will make the LED go gradually from zero-intensity to max (255)
>>> +             intensity in 1000 milliseconds, then back to zero intensity in 2000
>>> +             milliseconds:
>>> +
>>> +             LED brightness
>>> +                 ^
>>> +             255-|       / \            / \            /
>>> +                 |      /    \         /    \         /
>>> +                 |     /       \      /       \      /
>>> +                 |    /          \   /          \   /
>>> +               0-|   /             \/             \/
>>> +                 +---0----1----2----3----4----5----6------------> time (s)
>>> +
>>> +             2. To make the LED go instantly from one brigntess value to another,
>>> +             we should use use zero-time lengths. So the format should be:
>>> +             "brightness_1 duration_1 brightness_1 0 brightness_2 duration_2
>>> +             brightness_2 0 ...". For example:
>>> +
>>> +             echo 0 1000 0 0 255 2000 255 0 > pattern
>>> +
>>> +             It will make the LED stay off for one second, then stay at max brightness
>>> +             for two seconds:
>>> +
>>> +             LED brightness
>>> +                 ^
>>> +             255-|        +---------+    +---------+
>>> +                 |        |         |    |         |
>>> +                 |        |         |    |         |
>>> +                 |        |         |    |         |
>>> +               0-|   -----+         +----+         +----
>>> +                 +---0----1----2----3----4----5----6------------> time (s)
>>> +
>>> +What:                /sys/class/leds/<led>/hw_pattern
>>> +Date:                September 2018
>>> +KernelVersion:       4.20
>>> +Description:
>>> +             Specify a hardware pattern for the LED, for LED hardware that
>>> +             supports autonomously controlling brightness over time, according
>>> +             to some preprogrammed hardware patterns.
>>> +
>>> +             Since different LED hardware can have different semantics of
>>> +             hardware patterns, each driver is expected to provide its own
>>> +             description for the hardware patterns in their ABI documentation
>>> +             file.
>>> +
>>> +What:                /sys/class/leds/<led>/repeat
>>> +Date:                September 2018
>>> +KernelVersion:       4.20
>>> +Description:
>>> +             Specify a pattern repeat number. -1 means repeat indefinitely,
>>> +             other negative numbers and number 0 are invalid.
>>> +
>>> +             This file will always return the originally written repeat
>>> +             number.
>>> 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 0000000..92a0fd0
>>> --- /dev/null
>>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>>> @@ -0,0 +1,431 @@
>>> +// 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 <linux/kernel.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/timer.h>
>>> +
>>> +#define MAX_PATTERNS         1024
>>> +/*
>>> + * When doing gradual dimming, the led brightness will be updated
>>> + * every 50 milliseconds.
>>> + */
>>> +#define UPDATE_INTERVAL              50
>>> +
>>> +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;
>>> +     int repeat;
>>> +     int last_repeat;
>>> +     int delta_t;
>>> +     bool is_indefinite;
>>> +     bool is_hw_pattern;
>>> +     struct timer_list timer;
>>> +};
>>> +
>>> +static void pattern_trig_update_patterns(struct pattern_trig_data *data)
>>> +{
>>> +     data->curr = data->next;
>>> +     if (!data->is_indefinite && data->curr == data->patterns)
>>> +             data->repeat--;
>>> +
>>> +     if (data->next == data->patterns + data->npatterns - 1)
>>> +             data->next = data->patterns;
>>> +     else
>>> +             data->next++;
>>> +
>>> +     data->delta_t = 0;
>>> +}
>>> +
>>> +static int pattern_trig_compute_brightness(struct pattern_trig_data *data)
>>> +{
>>> +     int step_brightness;
>>> +
>>> +     if (data->delta_t == 0)
>>> +             return data->curr->brightness;
>>> +
>>> +     step_brightness = abs(data->next->brightness - data->curr->brightness);
>>> +     step_brightness = data->delta_t * step_brightness / data->curr->delta_t;
>>> +
>>> +     if (data->next->brightness > data->curr->brightness)
>>> +             return data->curr->brightness + step_brightness;
>>> +     else
>>> +             return data->curr->brightness - step_brightness;
>>> +}
>>> +
>>> +static void pattern_trig_timer_function(struct timer_list *t)
>>> +{
>>> +     struct pattern_trig_data *data = from_timer(data, t, timer);
>>> +
>>> +     mutex_lock(&data->lock);
>>> +
>>> +     for (; ;) {
>>
>> This whitespace between semicolons looks odd. Please remove it.
> 
> Sure.
> 
>>
>>> +             if (!data->is_indefinite && !data->repeat)
>>> +                     break;
>>> +
>>> +             if (data->curr->brightness == data->next->brightness) {
>>> +                     /* Constant brightness */
>>> +                     led_set_brightness(data->led_cdev,
>>> +                                        data->curr->brightness);
>>> +                     mod_timer(&data->timer,
>>> +                               jiffies + msecs_to_jiffies(data->curr->delta_t));
>>> +
>>> +                     /* Skip the step with zero duration */
>>> +                     pattern_trig_update_patterns(data);
>>> +                     /* Select next step */
>>> +                     pattern_trig_update_patterns(data);
>>> +                     break;
>>> +             } else {
>>> +                     /* Gradual dimming */
>>> +
>>> +                     /*
>>> +                      * If the accumulation time is larger than current
>>> +                      * step duration, we should go next one and re-check
>>> +                      * if we repeated done.
>>> +                      */
>>> +                     if (data->delta_t > data->curr->delta_t) {
>>> +                             pattern_trig_update_patterns(data);
>>> +                             continue;
>>> +                     }
>>> +
>>> +                     led_set_brightness(data->led_cdev,
>>> +                                        pattern_trig_compute_brightness(data));
>>> +                     mod_timer(&data->timer,
>>> +                               jiffies + msecs_to_jiffies(UPDATE_INTERVAL));
>>> +                     /* Accumulate the gradual dimming time */
>>> +                     data->delta_t += UPDATE_INTERVAL;
>>> +                     break;
>>> +             }
>>
>> Why two "break" statements instead of one here?
> 
> Yes, can remove one. Sorry for the quick version patch set without
> much self-checking in the night. Thanks for your comments.
> 

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Baolin Wang]

Hi Jacek and Pavel,

On 5 October 2018 at 04:00, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> Hi Baolin,
>
> On 10/03/2018 03:21 AM, Baolin Wang wrote:
>> Hi Jacek,
>>
>> On 3 October 2018 at 04:25, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>>> Hi Baolin,
>>>
>>> Thank you for the v14. We'll probably need v15, though :-)
>>>
>>> I added the comments in the code below.
>>>
>>> On 10/02/2018 05:43 PM, Baolin Wang wrote:
>>>> This patch adds one new led trigger that LED device can configure
>>>> the software or hardware pattern and trigger it.
>>>>
>>>> Consumers can write 'pattern' file to enable the software pattern
>>>> which alters the brightness for the specified duration with one
>>>> software timer.
>>>>
>>>> Moreover consumers can write 'hw_pattern' file to enable the hardware
>>>> pattern for some LED controllers which can autonomously control
>>>> brightness over time, according to some preprogrammed hardware
>>>> patterns.
>>>>
>>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>> Changes from v13:
>>>>  - Add duration validation for gradual dimming.
>>>>  - Coding style optimization.
>>>>
>>>> Changes from v12:
>>>>  - Add gradual dimming support for software pattern.
>>>>
>>>> Changes from v11:
>>>>  - Change -1 means repeat indefinitely.
>>>>
>>>> Changes from v10:
>>>>  - Change 'int' to 'u32' for delta_t field.
>>>>
>>>> Changes from v9:
>>>>  - None.
>>>>
>>>> Changes from v8:
>>>>  - None.
>>>>
>>>> Changes from v7:
>>>>  - Move the SC27XX hardware patterns description into its own ABI file.
>>>>
>>>> Changes from v6:
>>>>  - Improve commit message.
>>>>  - Optimize the description of the hw_pattern file.
>>>>  - Simplify some logics.
>>>>
>>>> Changes from v5:
>>>>  - Add one 'hw_pattern' file for hardware patterns.
>>>>
>>>> Changes from v4:
>>>>  - Change the repeat file to return the originally written number.
>>>>  - Improve comments.
>>>>  - Fix some build warnings.
>>>>
>>>> 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    |  76 ++++
>>>>  drivers/leds/trigger/Kconfig                       |   7 +
>>>>  drivers/leds/trigger/Makefile                      |   1 +
>>>>  drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
>>>>  include/linux/leds.h                               |  15 +
>>>>  5 files changed, 530 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 0000000..22d7af7
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>>> @@ -0,0 +1,76 @@
>>>> +What:                /sys/class/leds/<led>/pattern
>>>> +Date:                September 2018
>>>> +KernelVersion:       4.20
>>>> +Description:
>>>> +             Specify a software pattern for the LED, that supports altering
>>>> +             the brightness for the specified duration with one software
>>>> +             timer. It can do gradual dimming and constant brightness.
>>>> +
>>>> +             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.
>>>> +
>>>> +             1. When doing gradual dimming, the led brightness will be updated
>>>> +             every 50 milliseconds, so the duration of each step should not
>>>> +             less than 50 milliseconds.
>>>
>>> I'd like to avoid this constraint. Lowest supported delta_t should be 1.
>>>
>>> We should only prevent entering dimming mode if current delta_t
>>> is lower than UPDATE_INTERVAL.
>>
>> I do not think so. If the pattern format is used for dimming and the
>> delta_t is lower than UPDATE_INTERVAL, we should return errors. Since
>> in this case, we can not change to constant mode.
>
> I'll play with the code at weekend and will let you know my findings.

Thanks Jacek. Do you have any more suggestion?

BTW, we have not get a consensus about adding one 'dimming_interval'
file to set the dimming interval instead of fixed 50 ms.
Pavel, I want to hear your suggestion before I submit new patch set? Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Jacek Anaszewski]

Hi Baolin,

On 10/09/2018 02:01 PM, Baolin Wang wrote:
> Hi Jacek and Pavel,
> 
> On 5 October 2018 at 04:00, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>> Hi Baolin,
>>
>> On 10/03/2018 03:21 AM, Baolin Wang wrote:
>>> Hi Jacek,
>>>
>>> On 3 October 2018 at 04:25, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>>>> Hi Baolin,
>>>>
>>>> Thank you for the v14. We'll probably need v15, though :-)
>>>>
>>>> I added the comments in the code below.
>>>>
>>>> On 10/02/2018 05:43 PM, Baolin Wang wrote:
>>>>> This patch adds one new led trigger that LED device can configure
>>>>> the software or hardware pattern and trigger it.
>>>>>
>>>>> Consumers can write 'pattern' file to enable the software pattern
>>>>> which alters the brightness for the specified duration with one
>>>>> software timer.
>>>>>
>>>>> Moreover consumers can write 'hw_pattern' file to enable the hardware
>>>>> pattern for some LED controllers which can autonomously control
>>>>> brightness over time, according to some preprogrammed hardware
>>>>> patterns.
>>>>>
>>>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>> ---
>>>>> Changes from v13:
>>>>>  - Add duration validation for gradual dimming.
>>>>>  - Coding style optimization.
>>>>>
>>>>> Changes from v12:
>>>>>  - Add gradual dimming support for software pattern.
>>>>>
>>>>> Changes from v11:
>>>>>  - Change -1 means repeat indefinitely.
>>>>>
>>>>> Changes from v10:
>>>>>  - Change 'int' to 'u32' for delta_t field.
>>>>>
>>>>> Changes from v9:
>>>>>  - None.
>>>>>
>>>>> Changes from v8:
>>>>>  - None.
>>>>>
>>>>> Changes from v7:
>>>>>  - Move the SC27XX hardware patterns description into its own ABI file.
>>>>>
>>>>> Changes from v6:
>>>>>  - Improve commit message.
>>>>>  - Optimize the description of the hw_pattern file.
>>>>>  - Simplify some logics.
>>>>>
>>>>> Changes from v5:
>>>>>  - Add one 'hw_pattern' file for hardware patterns.
>>>>>
>>>>> Changes from v4:
>>>>>  - Change the repeat file to return the originally written number.
>>>>>  - Improve comments.
>>>>>  - Fix some build warnings.
>>>>>
>>>>> 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    |  76 ++++
>>>>>  drivers/leds/trigger/Kconfig                       |   7 +
>>>>>  drivers/leds/trigger/Makefile                      |   1 +
>>>>>  drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
>>>>>  include/linux/leds.h                               |  15 +
>>>>>  5 files changed, 530 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 0000000..22d7af7
>>>>> --- /dev/null
>>>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>>>> @@ -0,0 +1,76 @@
>>>>> +What:                /sys/class/leds/<led>/pattern
>>>>> +Date:                September 2018
>>>>> +KernelVersion:       4.20
>>>>> +Description:
>>>>> +             Specify a software pattern for the LED, that supports altering
>>>>> +             the brightness for the specified duration with one software
>>>>> +             timer. It can do gradual dimming and constant brightness.
>>>>> +
>>>>> +             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.
>>>>> +
>>>>> +             1. When doing gradual dimming, the led brightness will be updated
>>>>> +             every 50 milliseconds, so the duration of each step should not
>>>>> +             less than 50 milliseconds.
>>>>
>>>> I'd like to avoid this constraint. Lowest supported delta_t should be 1.
>>>>
>>>> We should only prevent entering dimming mode if current delta_t
>>>> is lower than UPDATE_INTERVAL.
>>>
>>> I do not think so. If the pattern format is used for dimming and the
>>> delta_t is lower than UPDATE_INTERVAL, we should return errors. Since
>>> in this case, we can not change to constant mode.
>>
>> I'll play with the code at weekend and will let you know my findings.
> 
> Thanks Jacek. Do you have any more suggestion?

Sorry to say, but I had really busy weekend and failed to carve out
time for looking into that. It's queued up with the highest priority
on my todo list of things needing more than five minutes of attention.

> BTW, we have not get a consensus about adding one 'dimming_interval'
> file to set the dimming interval instead of fixed 50 ms.
> Pavel, I want to hear your suggestion before I submit new patch set? Thanks.

I think that this feature belongs to the "nice to have" category,
and lack of it will not prevent us from merging the patch set.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Baolin Wang]

Hi Jacek,

On 10 October 2018 at 02:37, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> Hi Baolin,
>
> On 10/09/2018 02:01 PM, Baolin Wang wrote:
>> Hi Jacek and Pavel,
>>
>> On 5 October 2018 at 04:00, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>>> Hi Baolin,
>>>
>>> On 10/03/2018 03:21 AM, Baolin Wang wrote:
>>>> Hi Jacek,
>>>>
>>>> On 3 October 2018 at 04:25, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>>>>> Hi Baolin,
>>>>>
>>>>> Thank you for the v14. We'll probably need v15, though :-)
>>>>>
>>>>> I added the comments in the code below.
>>>>>
>>>>> On 10/02/2018 05:43 PM, Baolin Wang wrote:
>>>>>> This patch adds one new led trigger that LED device can configure
>>>>>> the software or hardware pattern and trigger it.
>>>>>>
>>>>>> Consumers can write 'pattern' file to enable the software pattern
>>>>>> which alters the brightness for the specified duration with one
>>>>>> software timer.
>>>>>>
>>>>>> Moreover consumers can write 'hw_pattern' file to enable the hardware
>>>>>> pattern for some LED controllers which can autonomously control
>>>>>> brightness over time, according to some preprogrammed hardware
>>>>>> patterns.
>>>>>>
>>>>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>> ---
>>>>>> Changes from v13:
>>>>>>  - Add duration validation for gradual dimming.
>>>>>>  - Coding style optimization.
>>>>>>
>>>>>> Changes from v12:
>>>>>>  - Add gradual dimming support for software pattern.
>>>>>>
>>>>>> Changes from v11:
>>>>>>  - Change -1 means repeat indefinitely.
>>>>>>
>>>>>> Changes from v10:
>>>>>>  - Change 'int' to 'u32' for delta_t field.
>>>>>>
>>>>>> Changes from v9:
>>>>>>  - None.
>>>>>>
>>>>>> Changes from v8:
>>>>>>  - None.
>>>>>>
>>>>>> Changes from v7:
>>>>>>  - Move the SC27XX hardware patterns description into its own ABI file.
>>>>>>
>>>>>> Changes from v6:
>>>>>>  - Improve commit message.
>>>>>>  - Optimize the description of the hw_pattern file.
>>>>>>  - Simplify some logics.
>>>>>>
>>>>>> Changes from v5:
>>>>>>  - Add one 'hw_pattern' file for hardware patterns.
>>>>>>
>>>>>> Changes from v4:
>>>>>>  - Change the repeat file to return the originally written number.
>>>>>>  - Improve comments.
>>>>>>  - Fix some build warnings.
>>>>>>
>>>>>> 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    |  76 ++++
>>>>>>  drivers/leds/trigger/Kconfig                       |   7 +
>>>>>>  drivers/leds/trigger/Makefile                      |   1 +
>>>>>>  drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
>>>>>>  include/linux/leds.h                               |  15 +
>>>>>>  5 files changed, 530 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 0000000..22d7af7
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>>>>> @@ -0,0 +1,76 @@
>>>>>> +What:                /sys/class/leds/<led>/pattern
>>>>>> +Date:                September 2018
>>>>>> +KernelVersion:       4.20
>>>>>> +Description:
>>>>>> +             Specify a software pattern for the LED, that supports altering
>>>>>> +             the brightness for the specified duration with one software
>>>>>> +             timer. It can do gradual dimming and constant brightness.
>>>>>> +
>>>>>> +             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.
>>>>>> +
>>>>>> +             1. When doing gradual dimming, the led brightness will be updated
>>>>>> +             every 50 milliseconds, so the duration of each step should not
>>>>>> +             less than 50 milliseconds.
>>>>>
>>>>> I'd like to avoid this constraint. Lowest supported delta_t should be 1.
>>>>>
>>>>> We should only prevent entering dimming mode if current delta_t
>>>>> is lower than UPDATE_INTERVAL.
>>>>
>>>> I do not think so. If the pattern format is used for dimming and the
>>>> delta_t is lower than UPDATE_INTERVAL, we should return errors. Since
>>>> in this case, we can not change to constant mode.
>>>
>>> I'll play with the code at weekend and will let you know my findings.
>>
>> Thanks Jacek. Do you have any more suggestion?
>
> Sorry to say, but I had really busy weekend and failed to carve out
> time for looking into that. It's queued up with the highest priority
> on my todo list of things needing more than five minutes of attention.

Ah, okay, no hurry :)

>> BTW, we have not get a consensus about adding one 'dimming_interval'
>> file to set the dimming interval instead of fixed 50 ms.
>> Pavel, I want to hear your suggestion before I submit new patch set? Thanks.
>
> I think that this feature belongs to the "nice to have" category,
> and lack of it will not prevent us from merging the patch set.

OK. Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Pavel Machek]

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

Hi!

[Oops. I wrote this but forgot to send it? Anyway.. I guess it is
redundant now.]

> > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> > @@ -0,0 +1,76 @@
> > +What:		/sys/class/leds/<led>/pattern
> > +Date:		September 2018
> > +KernelVersion:	4.20
> > +Description:
> > +		Specify a software pattern for the LED, that supports altering
> > +		the brightness for the specified duration with one software
> > +		timer. It can do gradual dimming and constant brightness.
> > +
> > +		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.
> > +
> > +		1. When doing gradual dimming, the led brightness will be updated
> > +		every 50 milliseconds, so the duration of each step should not
> > +		less than 50 milliseconds.
> 
> I'd like to avoid this constraint. Lowest supported delta_t should be 1.
> 
> We should only prevent entering dimming mode if current delta_t
> is lower than UPDATE_INTERVAL.

I agree that documenting it like this is strange. I don't think this
should be part of ABI as we may want to adjust it in the future.

> I also propose to make the dimming interval configurable via sysfs
> dimming_interval file.

I believe we should provide "right" dimming_interval. 50msec sounds
good. If we find it is too low / too high, we might want to adjust it.

If we find out that different users need different values, I guess we
can still add "dimming_interval" knob, but I'd suggest waiting until
such need arises.

[Now, I don't have strong feelings here, and patch would probably be
okay with me even if it has dimming_interval, but I'd prefer to keep
it simple. We are at v14 of the patch...]

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Jacek Anaszewski]

Hi Baolin,

I've finally managed to test this patch and it turned out
that this implementation doesn't prevent what I had concerns
about, i.e. setting intervals less then 50ms.

What user needs to do to achieve that is just e.g.:

echo "10 1 10 0 20 1 20 0 30 1 30 0" > pattern

Please refer below to my remaining remarks.

On 10/02/2018 05:43 PM, Baolin Wang wrote:
> This patch adds one new led trigger that LED device can configure

s/one new led trigger/a new LED trigger/

> the software or hardware pattern and trigger it.

Please change the above line to the following:

to employ software or hardware pattern engine.

> 
> Consumers can write 'pattern' file to enable the software pattern
> which alters the brightness for the specified duration with one
> software timer.
> 
> Moreover consumers can write 'hw_pattern' file to enable the hardware
> pattern for some LED controllers which can autonomously control
> brightness over time, according to some preprogrammed hardware
> patterns.
> 
> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v13:
>  - Add duration validation for gradual dimming.
>  - Coding style optimization.
> 
> Changes from v12:
>  - Add gradual dimming support for software pattern.
> 
> Changes from v11:
>  - Change -1 means repeat indefinitely.
> 
> Changes from v10:
>  - Change 'int' to 'u32' for delta_t field.
> 
> Changes from v9:
>  - None.
> 
> Changes from v8:
>  - None.
> 
> Changes from v7:
>  - Move the SC27XX hardware patterns description into its own ABI file.
> 
> Changes from v6:
>  - Improve commit message.
>  - Optimize the description of the hw_pattern file.
>  - Simplify some logics.
> 
> Changes from v5:
>  - Add one 'hw_pattern' file for hardware patterns.
> 
> Changes from v4:
>  - Change the repeat file to return the originally written number.
>  - Improve comments.
>  - Fix some build warnings.
> 
> 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    |  76 ++++
>  drivers/leds/trigger/Kconfig                       |   7 +
>  drivers/leds/trigger/Makefile                      |   1 +
>  drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
>  include/linux/leds.h                               |  15 +
>  5 files changed, 530 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 0000000..22d7af7
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> @@ -0,0 +1,76 @@
> +What:		/sys/class/leds/<led>/pattern
> +Date:		September 2018
> +KernelVersion:	4.20
> +Description:
> +		Specify a software pattern for the LED, that supports altering
> +		the brightness for the specified duration with one software
> +		timer. It can do gradual dimming and constant brightness.

s/constant brightness/step change of brightness/

> +
> +		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.
> +
> +		1. When doing gradual dimming, the led brightness will be updated
> +		every 50 milliseconds, so the duration of each step should not
> +		less than 50 milliseconds. The gradual dimming format of the
> +		software pattern values should be:

Despite I pronounced the dimming_interval as only a "nice to have"
feature, now it looks to me to be really useful to have it included in
this patch. For the reasons below:

1) You could refer directly to the dimming_interval here.
2) I've been wondering how to treat the case when user requests the
   interval less than dimming_interval. In v13 you were skipping such
   tuple, and in v14 you're not allowing for it at all. How about,
   instead of the two above approaches, describe such a case as:

"The tuple with duration less than dimming_interval is treated as
a step change of brightness, i.e. the subsequent brightness will be
applied without adding intervening dimming intervals"

> +		"brightness_1 duration_1 brightness_2 duration_2 brightness_3
> +		duration_3 ...". For example:
> +
> +		echo 0 1000 255 2000 > pattern
> +
> +		It will make the LED go gradually from zero-intensity to max (255)
> +		intensity in 1000 milliseconds, then back to zero intensity in 2000
> +		milliseconds:
> +
> +		LED brightness
> +		    ^
> +		255-|       / \            / \            /
> +		    |      /    \         /    \         /
> +		    |     /       \      /       \      /
> +		    |    /          \   /          \   /
> +		  0-|   /             \/             \/
> +		    +---0----1----2----3----4----5----6------------> time (s)
> +
> +		2. To make the LED go instantly from one brigntess value to another,
> +		we should use use zero-time lengths. So the format should be:
> +		"brightness_1 duration_1 brightness_1 0 brightness_2 duration_2
> +		brightness_2 0 ...". For example:
> +
> +		echo 0 1000 0 0 255 2000 255 0 > pattern
> +
> +		It will make the LED stay off for one second, then stay at max brightness
> +		for two seconds:
> +
> +		LED brightness
> +		    ^
> +		255-|        +---------+    +---------+
> +		    |        |         |    |         |
> +		    |        |         |    |         |
> +		    |        |         |    |         |
> +		  0-|   -----+         +----+         +----
> +		    +---0----1----2----3----4----5----6------------> time (s)

I'd also add that writing malformed pattern deactivates any
active one.

> +
> +What:		/sys/class/leds/<led>/hw_pattern
> +Date:		September 2018
> +KernelVersion:	4.20
> +Description:
> +		Specify a hardware pattern for the LED, for LED hardware that
> +		supports autonomously controlling brightness over time, according
> +		to some preprogrammed hardware patterns.
> +
> +		Since different LED hardware can have different semantics of
> +		hardware patterns, each driver is expected to provide its own
> +		description for the hardware patterns in their ABI documentation
> +		file.

Add also:

It deactivates any active software pattern.

> +
> +What:		/sys/class/leds/<led>/repeat
> +Date:		September 2018
> +KernelVersion:	4.20
> +Description:
> +		Specify a pattern repeat number. -1 means repeat indefinitely,
> +		other negative numbers and number 0 are invalid.
> +
> +		This file will always return the originally written repeat
> +		number.
> 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 0000000..92a0fd0
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,431 @@
> +// 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 <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +
> +#define MAX_PATTERNS		1024
> +/*
> + * When doing gradual dimming, the led brightness will be updated
> + * every 50 milliseconds.
> + */
> +#define UPDATE_INTERVAL		50
> +
> +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;
> +	int repeat;
> +	int last_repeat;
> +	int delta_t;
> +	bool is_indefinite;
> +	bool is_hw_pattern;
> +	struct timer_list timer;
> +};
> +
> +static void pattern_trig_update_patterns(struct pattern_trig_data *data)
> +{
> +	data->curr = data->next;
> +	if (!data->is_indefinite && data->curr == data->patterns)
> +		data->repeat--;
> +
> +	if (data->next == data->patterns + data->npatterns - 1)
> +		data->next = data->patterns;
> +	else
> +		data->next++;
> +
> +	data->delta_t = 0;
> +}
> +
> +static int pattern_trig_compute_brightness(struct pattern_trig_data *data)
> +{
> +	int step_brightness;
> +
> +	if (data->delta_t == 0)
> +		return data->curr->brightness;
> +
> +	step_brightness = abs(data->next->brightness - data->curr->brightness);
> +	step_brightness = data->delta_t * step_brightness / data->curr->delta_t;
> +
> +	if (data->next->brightness > data->curr->brightness)
> +		return data->curr->brightness + step_brightness;
> +	else
> +		return data->curr->brightness - step_brightness;
> +}
> +
> +static void pattern_trig_timer_function(struct timer_list *t)
> +{
> +	struct pattern_trig_data *data = from_timer(data, t, timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	for (; ;) {
> +		if (!data->is_indefinite && !data->repeat)
> +			break;
> +
> +		if (data->curr->brightness == data->next->brightness) {
> +			/* Constant brightness */
> +			led_set_brightness(data->led_cdev,
> +					   data->curr->brightness);
> +			mod_timer(&data->timer,
> +				  jiffies + msecs_to_jiffies(data->curr->delta_t));
> +
> +			/* Skip the step with zero duration */
> +			pattern_trig_update_patterns(data);
> +			/* Select next step */
> +			pattern_trig_update_patterns(data);
> +			break;
> +		} else {
> +			/* Gradual dimming */
> +
> +			/*
> +			 * If the accumulation time is larger than current
> +			 * step duration, we should go next one and re-check
> +			 * if we repeated done.
> +			 */
> +			if (data->delta_t > data->curr->delta_t) {
> +				pattern_trig_update_patterns(data);
> +				continue;
> +			}
> +
> +			led_set_brightness(data->led_cdev,
> +					   pattern_trig_compute_brightness(data));
> +			mod_timer(&data->timer,
> +				  jiffies + msecs_to_jiffies(UPDATE_INTERVAL));
> +			/* Accumulate the gradual dimming time */
> +			data->delta_t += UPDATE_INTERVAL;
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&data->lock);
> +}
> +
> +static bool pattern_trig_is_sane(struct pattern_trig_data *data)
> +{
> +	int i;
> +
> +	/* At least 2 steps for software pattern. */
> +	if (data->npatterns < 2)
> +		return false;
> +
> +	/*
> +	 * No need valid the duration for constant brightness, just

s/No need valid/No need to validate/

> +	 * return true.
> +	 */
> +	if (data->patterns[0].brightness == data->patterns[1].brightness)
> +		return true;

Hmm, this should be covered in the ABI documentation.

> +	/*
> +	 * Should valid the duration if is less than 50 ms for gradual
> +	 * dimming mode.
> +	 */
> +	for (i = 0; i < data->npatterns; i++) {
> +		if (data->patterns[i].delta_t < UPDATE_INTERVAL)
> +			return false;
> +	}

And this could be skipped, provided we will agree on treating delta_t
less than dimming_interval as step brightness change.

> +
> +	return true;
> +}
> +
> +static int pattern_trig_start_pattern(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +
> +	if (!data->npatterns)
> +		return 0;
> +
> +	if (data->is_hw_pattern) {
> +		return led_cdev->pattern_set(led_cdev, data->patterns,
> +					     data->npatterns, data->repeat);
> +	}
> +
> +	if (!pattern_trig_is_sane(data))
> +		return -EINVAL;
> +
> +	data->delta_t = 0;
> +	data->curr = data->patterns;
> +	data->next = data->npatterns > 1 ? data->patterns + 1 : data->patterns;
> +	data->timer.expires = jiffies;
> +	add_timer(&data->timer);
> +
> +	return 0;
> +}
> +
> +static ssize_t repeat_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +	int repeat;
> +
> +	mutex_lock(&data->lock);
> +
> +	repeat = data->last_repeat;
> +
> +	mutex_unlock(&data->lock);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", repeat);
> +}
> +
> +static ssize_t repeat_store(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;
> +	int err, res;
> +
> +	err = kstrtos32(buf, 10, &res);
> +	if (err)
> +		return err;
> +
> +	/* Number 0 and negative numbers except -1 are invalid. */
> +	if (res < -1 || res == 0)
> +		return -EINVAL;
> +
> +	/*
> +	 * Clear previous patterns' performence firstly, and remove the timer
> +	 * without mutex lock to avoid dead lock.
> +	 */
> +	del_timer_sync(&data->timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	if (data->is_hw_pattern)
> +		led_cdev->pattern_clear(led_cdev);
> +
> +	data->last_repeat = data->repeat = res;
> +	/* -1 means repeat indefinitely */
> +	if (data->repeat == -1)
> +		data->is_indefinite = true;
> +	else
> +		data->is_indefinite = false;
> +
> +	err = pattern_trig_start_pattern(led_cdev);
> +
> +	mutex_unlock(&data->lock);
> +	return err < 0 ? err : count;
> +}
> +
> +static DEVICE_ATTR_RW(repeat);
> +
> +static ssize_t pattern_trig_show_patterns(struct pattern_trig_data *data,
> +					  char *buf, bool hw_pattern)
> +{
> +	ssize_t count = 0;
> +	int i;
> +
> +	mutex_lock(&data->lock);
> +
> +	if (!data->npatterns || (data->is_hw_pattern ^ hw_pattern))
> +		goto out;
> +
> +	for (i = 0; i < data->npatterns; i++) {
> +		count += scnprintf(buf + count, PAGE_SIZE - count,
> +				   "%d %u ",
> +				   data->patterns[i].brightness,
> +				   data->patterns[i].delta_t);
> +	}
> +
> +	buf[count - 1] = '\n';
> +
> +out:
> +	mutex_unlock(&data->lock);
> +	return count;
> +}
> +
> +static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev,
> +					   const char *buf, size_t count,
> +					   bool hw_pattern)
> +{
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +	int ccount, cr, offset = 0, err = 0;
> +
> +	/*
> +	 * Clear previous patterns' performence firstly, and remove the timer
> +	 * without mutex lock to avoid dead lock.
> +	 */
> +	del_timer_sync(&data->timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	if (data->is_hw_pattern)
> +		led_cdev->pattern_clear(led_cdev);
> +
> +	data->is_hw_pattern = hw_pattern;
> +	data->npatterns = 0;
> +
> +	while (offset < count - 1 && data->npatterns < MAX_PATTERNS) {
> +		cr = 0;
> +		ccount = sscanf(buf + offset, "%d %u %n",
> +				&data->patterns[data->npatterns].brightness,
> +				&data->patterns[data->npatterns].delta_t, &cr);
> +		if (ccount != 2) {
> +			data->npatterns = 0;
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		offset += cr;
> +		data->npatterns++;
> +	}
> +
> +	err = pattern_trig_start_pattern(led_cdev);
> +	if (err)
> +		data->npatterns = 0;
> +
> +out:
> +	mutex_unlock(&data->lock);
> +	return err < 0 ? err : count;
> +}
> +
> +static ssize_t pattern_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +
> +	return pattern_trig_show_patterns(data, buf, false);
> +}
> +
> +static ssize_t pattern_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	return pattern_trig_store_patterns(led_cdev, buf, count, false);
> +}
> +
> +static DEVICE_ATTR_RW(pattern);
> +
> +static ssize_t hw_pattern_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +
> +	return pattern_trig_show_patterns(data, buf, true);
> +}
> +
> +static ssize_t hw_pattern_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	return pattern_trig_store_patterns(led_cdev, buf, count, true);
> +}
> +
> +static DEVICE_ATTR_RW(hw_pattern);
> +
> +static umode_t pattern_trig_attrs_mode(struct kobject *kobj,
> +				       struct attribute *attr, int index)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	if (attr == &dev_attr_repeat.attr || attr == &dev_attr_pattern.attr)
> +		return attr->mode;
> +	else if (attr == &dev_attr_hw_pattern.attr && led_cdev->pattern_set)
> +		return attr->mode;
> +
> +	return 0;
> +}
> +
> +static struct attribute *pattern_trig_attrs[] = {
> +	&dev_attr_pattern.attr,
> +	&dev_attr_hw_pattern.attr,
> +	&dev_attr_repeat.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group pattern_trig_group = {
> +	.attrs = pattern_trig_attrs,
> +	.is_visible = pattern_trig_attrs_mode,
> +};
> +
> +static const struct attribute_group *pattern_trig_groups[] = {
> +	&pattern_trig_group,
> +	NULL,
> +};
> +
> +static int pattern_trig_activate(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	if (!!led_cdev->pattern_set ^ !!led_cdev->pattern_clear) {
> +		dev_warn(led_cdev->dev,
> +			 "Hardware pattern ops validation failed\n");
> +		led_cdev->pattern_set = NULL;
> +		led_cdev->pattern_clear = NULL;
> +	}
> +
> +	data->is_indefinite = true;
> +	data->last_repeat = -1;
> +	mutex_init(&data->lock);
> +	data->led_cdev = led_cdev;
> +	led_set_trigger_data(led_cdev, data);
> +	timer_setup(&data->timer, pattern_trig_timer_function, 0);
> +	led_cdev->activated = true;
> +
> +	return 0;
> +}
> +
> +static void pattern_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +
> +	if (!led_cdev->activated)
> +		return;
> +
> +	if (led_cdev->pattern_clear)
> +		led_cdev->pattern_clear(led_cdev);
> +
> +	del_timer_sync(&data->timer);
> +
> +	led_set_brightness(led_cdev, LED_OFF);
> +	kfree(data);
> +	led_cdev->activated = false;
> +}
> +
> +static struct led_trigger pattern_led_trigger = {
> +	.name = "pattern",
> +	.activate = pattern_trig_activate,
> +	.deactivate = pattern_trig_deactivate,
> +	.groups = pattern_trig_groups,
> +};
> +
> +static int __init pattern_trig_init(void)
> +{
> +	return led_trigger_register(&pattern_led_trigger);
> +}
> +
> +static void __exit pattern_trig_exit(void)
> +{
> +	led_trigger_unregister(&pattern_led_trigger);
> +}
> +
> +module_init(pattern_trig_init);
> +module_exit(pattern_trig_exit);
> +
> +MODULE_AUTHOR("Raphael Teysseyre <rteysseyre@gmail.com");
> +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org");
> +MODULE_DESCRIPTION("LED Pattern trigger");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 834683d..7393a31 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -22,6 +22,7 @@
>  #include <linux/workqueue.h>
>  
>  struct device;
> +struct led_pattern;
>  /*
>   * LED Core
>   */
> @@ -88,6 +89,10 @@ struct led_classdev {
>  				     unsigned long *delay_on,
>  				     unsigned long *delay_off);
>  
> +	int (*pattern_set)(struct led_classdev *led_cdev,
> +			   struct led_pattern *pattern, u32 len, int repeat);
> +	int (*pattern_clear)(struct led_classdev *led_cdev);
> +
>  	struct device		*dev;
>  	const struct attribute_group	**groups;
>  
> @@ -472,4 +477,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
>  	struct led_classdev *led_cdev, enum led_brightness brightness) { }
>  #endif
>  
> +/**
> + * struct led_pattern - pattern interval settings
> + * @delta_t: pattern interval delay, in milliseconds
> + * @brightness: pattern interval brightness
> + */
> +struct led_pattern {
> +	u32 delta_t;
> +	int brightness;
> +};
> +
>  #endif		/* __LINUX_LEDS_H_INCLUDED */
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Baolin Wang]

Hi Pavel,

On 11 October 2018 at 00:00, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> [Oops. I wrote this but forgot to send it? Anyway.. I guess it is
> redundant now.]
>
>> > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> > @@ -0,0 +1,76 @@
>> > +What:              /sys/class/leds/<led>/pattern
>> > +Date:              September 2018
>> > +KernelVersion:     4.20
>> > +Description:
>> > +           Specify a software pattern for the LED, that supports altering
>> > +           the brightness for the specified duration with one software
>> > +           timer. It can do gradual dimming and constant brightness.
>> > +
>> > +           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.
>> > +
>> > +           1. When doing gradual dimming, the led brightness will be updated
>> > +           every 50 milliseconds, so the duration of each step should not
>> > +           less than 50 milliseconds.
>>
>> I'd like to avoid this constraint. Lowest supported delta_t should be 1.
>>
>> We should only prevent entering dimming mode if current delta_t
>> is lower than UPDATE_INTERVAL.
>
> I agree that documenting it like this is strange. I don't think this
> should be part of ABI as we may want to adjust it in the future.
>
>> I also propose to make the dimming interval configurable via sysfs
>> dimming_interval file.
>
> I believe we should provide "right" dimming_interval. 50msec sounds
> good. If we find it is too low / too high, we might want to adjust it.
>
> If we find out that different users need different values, I guess we
> can still add "dimming_interval" knob, but I'd suggest waiting until
> such need arises.
>
> [Now, I don't have strong feelings here, and patch would probably be
> okay with me even if it has dimming_interval, but I'd prefer to keep
> it simple. We are at v14 of the patch...]

Make sense. I will leave this feature until someone has the
dimming_interval adjustment. Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Baolin Wang]

Hi Jacek,

On 11 October 2018 at 04:43, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> Hi Baolin,
>
> I've finally managed to test this patch and it turned out
> that this implementation doesn't prevent what I had concerns
> about, i.e. setting intervals less then 50ms.
>
> What user needs to do to achieve that is just e.g.:
>
> echo "10 1 10 0 20 1 20 0 30 1 30 0" > pattern
>
> Please refer below to my remaining remarks.

But this is not used for gradual dimming, this pattern is used for
constant brightness. According to the ABI, we do not need limit the
duration larger than 50 ms for constant brightness. Only for gradual
dimming, we need limit the duration larger than 50 ms, since the
interval step is 50ms.

But according to your suggestion as below:
> "The tuple with duration less than dimming_interval is treated as
> a step change of brightness, i.e. the subsequent brightness will be
> applied without adding intervening dimming intervals"

For gradual dimming, we also do not need to limit the duration larger
than 50 ms. So below patterns are all valid now:

echo "10 1 10 0 20 1 20 0 30 1 30 0" > pattern
echo "10 1  20 1  30 1 " > pattern


> On 10/02/2018 05:43 PM, Baolin Wang wrote:
>> This patch adds one new led trigger that LED device can configure
>
> s/one new led trigger/a new LED trigger/

Sure.

>
>> the software or hardware pattern and trigger it.
>
> Please change the above line to the following:
>
> to employ software or hardware pattern engine.

Sure.

>
>>
>> Consumers can write 'pattern' file to enable the software pattern
>> which alters the brightness for the specified duration with one
>> software timer.
>>
>> Moreover consumers can write 'hw_pattern' file to enable the hardware
>> pattern for some LED controllers which can autonomously control
>> brightness over time, according to some preprogrammed hardware
>> patterns.
>>
>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v13:
>>  - Add duration validation for gradual dimming.
>>  - Coding style optimization.
>>
>> Changes from v12:
>>  - Add gradual dimming support for software pattern.
>>
>> Changes from v11:
>>  - Change -1 means repeat indefinitely.
>>
>> Changes from v10:
>>  - Change 'int' to 'u32' for delta_t field.
>>
>> Changes from v9:
>>  - None.
>>
>> Changes from v8:
>>  - None.
>>
>> Changes from v7:
>>  - Move the SC27XX hardware patterns description into its own ABI file.
>>
>> Changes from v6:
>>  - Improve commit message.
>>  - Optimize the description of the hw_pattern file.
>>  - Simplify some logics.
>>
>> Changes from v5:
>>  - Add one 'hw_pattern' file for hardware patterns.
>>
>> Changes from v4:
>>  - Change the repeat file to return the originally written number.
>>  - Improve comments.
>>  - Fix some build warnings.
>>
>> 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    |  76 ++++
>>  drivers/leds/trigger/Kconfig                       |   7 +
>>  drivers/leds/trigger/Makefile                      |   1 +
>>  drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
>>  include/linux/leds.h                               |  15 +
>>  5 files changed, 530 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 0000000..22d7af7
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> @@ -0,0 +1,76 @@
>> +What:                /sys/class/leds/<led>/pattern
>> +Date:                September 2018
>> +KernelVersion:       4.20
>> +Description:
>> +             Specify a software pattern for the LED, that supports altering
>> +             the brightness for the specified duration with one software
>> +             timer. It can do gradual dimming and constant brightness.
>
> s/constant brightness/step change of brightness/

OK.

>
>> +
>> +             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.
>> +
>> +             1. When doing gradual dimming, the led brightness will be updated
>> +             every 50 milliseconds, so the duration of each step should not
>> +             less than 50 milliseconds. The gradual dimming format of the
>> +             software pattern values should be:
>
> Despite I pronounced the dimming_interval as only a "nice to have"
> feature, now it looks to me to be really useful to have it included in
> this patch. For the reasons below:
>
> 1) You could refer directly to the dimming_interval here.
> 2) I've been wondering how to treat the case when user requests the
>    interval less than dimming_interval. In v13 you were skipping such
>    tuple, and in v14 you're not allowing for it at all. How about,
>    instead of the two above approaches, describe such a case as:
>
> "The tuple with duration less than dimming_interval is treated as
> a step change of brightness, i.e. the subsequent brightness will be
> applied without adding intervening dimming intervals"

Good point. I will implement it in next version. So we will not limit
the duration larger than dimming_interval (50ms).

But according to Pavel's suggestion (I also agree with his points), we
can keep it simple now and leave the feature introducing the
'dimming_interval' file until someone complains 50ms interval should
be adjusted.

>
>> +             "brightness_1 duration_1 brightness_2 duration_2 brightness_3
>> +             duration_3 ...". For example:
>> +
>> +             echo 0 1000 255 2000 > pattern
>> +
>> +             It will make the LED go gradually from zero-intensity to max (255)
>> +             intensity in 1000 milliseconds, then back to zero intensity in 2000
>> +             milliseconds:
>> +
>> +             LED brightness
>> +                 ^
>> +             255-|       / \            / \            /
>> +                 |      /    \         /    \         /
>> +                 |     /       \      /       \      /
>> +                 |    /          \   /          \   /
>> +               0-|   /             \/             \/
>> +                 +---0----1----2----3----4----5----6------------> time (s)
>> +
>> +             2. To make the LED go instantly from one brigntess value to another,
>> +             we should use use zero-time lengths. So the format should be:
>> +             "brightness_1 duration_1 brightness_1 0 brightness_2 duration_2
>> +             brightness_2 0 ...". For example:
>> +
>> +             echo 0 1000 0 0 255 2000 255 0 > pattern
>> +
>> +             It will make the LED stay off for one second, then stay at max brightness
>> +             for two seconds:
>> +
>> +             LED brightness
>> +                 ^
>> +             255-|        +---------+    +---------+
>> +                 |        |         |    |         |
>> +                 |        |         |    |         |
>> +                 |        |         |    |         |
>> +               0-|   -----+         +----+         +----
>> +                 +---0----1----2----3----4----5----6------------> time (s)
>
> I'd also add that writing malformed pattern deactivates any
> active one.

Sure.

>
>> +
>> +What:                /sys/class/leds/<led>/hw_pattern
>> +Date:                September 2018
>> +KernelVersion:       4.20
>> +Description:
>> +             Specify a hardware pattern for the LED, for LED hardware that
>> +             supports autonomously controlling brightness over time, according
>> +             to some preprogrammed hardware patterns.
>> +
>> +             Since different LED hardware can have different semantics of
>> +             hardware patterns, each driver is expected to provide its own
>> +             description for the hardware patterns in their ABI documentation
>> +             file.
>
> Add also:
>
> It deactivates any active software pattern.

Sure.

>> +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;
>> +     int repeat;
>> +     int last_repeat;
>> +     int delta_t;
>> +     bool is_indefinite;
>> +     bool is_hw_pattern;
>> +     struct timer_list timer;
>> +};
>> +
>> +static void pattern_trig_update_patterns(struct pattern_trig_data *data)
>> +{
>> +     data->curr = data->next;
>> +     if (!data->is_indefinite && data->curr == data->patterns)
>> +             data->repeat--;
>> +
>> +     if (data->next == data->patterns + data->npatterns - 1)
>> +             data->next = data->patterns;
>> +     else
>> +             data->next++;
>> +
>> +     data->delta_t = 0;
>> +}
>> +
>> +static int pattern_trig_compute_brightness(struct pattern_trig_data *data)
>> +{
>> +     int step_brightness;
>> +
>> +     if (data->delta_t == 0)
>> +             return data->curr->brightness;
>> +
>> +     step_brightness = abs(data->next->brightness - data->curr->brightness);
>> +     step_brightness = data->delta_t * step_brightness / data->curr->delta_t;
>> +
>> +     if (data->next->brightness > data->curr->brightness)
>> +             return data->curr->brightness + step_brightness;
>> +     else
>> +             return data->curr->brightness - step_brightness;
>> +}
>> +
>> +static void pattern_trig_timer_function(struct timer_list *t)
>> +{
>> +     struct pattern_trig_data *data = from_timer(data, t, timer);
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     for (; ;) {
>> +             if (!data->is_indefinite && !data->repeat)
>> +                     break;
>> +
>> +             if (data->curr->brightness == data->next->brightness) {
>> +                     /* Constant brightness */
>> +                     led_set_brightness(data->led_cdev,
>> +                                        data->curr->brightness);
>> +                     mod_timer(&data->timer,
>> +                               jiffies + msecs_to_jiffies(data->curr->delta_t));
>> +
>> +                     /* Skip the step with zero duration */
>> +                     pattern_trig_update_patterns(data);
>> +                     /* Select next step */
>> +                     pattern_trig_update_patterns(data);
>> +                     break;
>> +             } else {
>> +                     /* Gradual dimming */
>> +
>> +                     /*
>> +                      * If the accumulation time is larger than current
>> +                      * step duration, we should go next one and re-check
>> +                      * if we repeated done.
>> +                      */
>> +                     if (data->delta_t > data->curr->delta_t) {
>> +                             pattern_trig_update_patterns(data);
>> +                             continue;
>> +                     }
>> +
>> +                     led_set_brightness(data->led_cdev,
>> +                                        pattern_trig_compute_brightness(data));
>> +                     mod_timer(&data->timer,
>> +                               jiffies + msecs_to_jiffies(UPDATE_INTERVAL));
>> +                     /* Accumulate the gradual dimming time */
>> +                     data->delta_t += UPDATE_INTERVAL;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     mutex_unlock(&data->lock);
>> +}
>> +
>> +static bool pattern_trig_is_sane(struct pattern_trig_data *data)
>> +{
>> +     int i;
>> +
>> +     /* At least 2 steps for software pattern. */
>> +     if (data->npatterns < 2)
>> +             return false;
>> +
>> +     /*
>> +      * No need valid the duration for constant brightness, just
>
> s/No need valid/No need to validate/

OK.

>
>> +      * return true.
>> +      */
>> +     if (data->patterns[0].brightness == data->patterns[1].brightness)
>> +             return true;
>
> Hmm, this should be covered in the ABI documentation.

OK.

>
>> +     /*
>> +      * Should valid the duration if is less than 50 ms for gradual
>> +      * dimming mode.
>> +      */
>> +     for (i = 0; i < data->npatterns; i++) {
>> +             if (data->patterns[i].delta_t < UPDATE_INTERVAL)
>> +                     return false;
>> +     }
>
> And this could be skipped, provided we will agree on treating delta_t
> less than dimming_interval as step brightness change.

Yes. Will remove this logic here and I think the
pattern_trig_is_sane() is redundant now which can be removed too.
Thanks for your comments.

-- 
Baolin Wang
Best Regards

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Geert Uytterhoeven]

Hi Baolin,

On Tue, Oct 2, 2018 at 6:39 PM Baolin Wang <baolin.wang@linaro.org> wrote:
> This patch adds one new led trigger that LED device can configure
> the software or hardware pattern and trigger it.
>
> Consumers can write 'pattern' file to enable the software pattern
> which alters the brightness for the specified duration with one
> software timer.
>
> Moreover consumers can write 'hw_pattern' file to enable the hardware
> pattern for some LED controllers which can autonomously control
> brightness over time, according to some preprogrammed hardware
> patterns.
>
> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,431 @@

> +static void pattern_trig_timer_function(struct timer_list *t)
> +{
> +       struct pattern_trig_data *data = from_timer(data, t, timer);
> +
> +       mutex_lock(&data->lock);

Timer functions run in interrupt context, hence if CONFIG_DEBUG_ATOMIC_SLEEP=y:

BUG: sleeping function called from invalid context at kernel/locking/mutex.c:254
in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
CPU: 1 PID: 0 Comm: swapper/1 Not tainted
4.20.0-rc1-koelsch-00841-ga338c8181013c1a9 #171
Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
[<c020f19c>] (unwind_backtrace) from [<c020aecc>] (show_stack+0x10/0x14)
[<c020aecc>] (show_stack) from [<c07affb8>] (dump_stack+0x7c/0x9c)
[<c07affb8>] (dump_stack) from [<c02417d4>] (___might_sleep+0xf4/0x158)
[<c02417d4>] (___might_sleep) from [<c07c92c4>] (mutex_lock+0x18/0x60)
[<c07c92c4>] (mutex_lock) from [<c067b28c>]
(pattern_trig_timer_function+0x1c/0x11c)
[<c067b28c>] (pattern_trig_timer_function) from [<c027f6fc>]
(call_timer_fn+0x1c/0x90)
[<c027f6fc>] (call_timer_fn) from [<c027f944>] (expire_timers+0x94/0xa4)
[<c027f944>] (expire_timers) from [<c027fc98>] (run_timer_softirq+0x108/0x15c)
[<c027fc98>] (run_timer_softirq) from [<c02021cc>] (__do_softirq+0x1d4/0x258)
[<c02021cc>] (__do_softirq) from [<c0224d24>] (irq_exit+0x64/0xc4)
[<c0224d24>] (irq_exit) from [<c0268dd0>] (__handle_domain_irq+0x80/0xb4)
[<c0268dd0>] (__handle_domain_irq) from [<c045e1b0>] (gic_handle_irq+0x58/0x90)
[<c045e1b0>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x74)
Exception stack(0xeb483f60 to 0xeb483fa8)
3f60: 00000000 00000000 eb9afaa0 c0217e80 00000000 ffffe000 00000000 c0e06408
3f80: 00000002 c0e0647c c0c6a5f0 00000000 c0e04900 eb483fb0 c0207ea8 c0207e98
3fa0: 60020013 ffffffff
[<c02019f8>] (__irq_svc) from [<c0207e98>] (arch_cpu_idle+0x1c/0x38)
[<c0207e98>] (arch_cpu_idle) from [<c0247ca8>] (do_idle+0x138/0x268)
[<c0247ca8>] (do_idle) from [<c0248050>] (cpu_startup_entry+0x18/0x1c)
[<c0248050>] (cpu_startup_entry) from [<402022ec>] (0x402022ec)

Either this should use a spinlock instead of a mutex, or the timer
function should kick a workqueue to do the real work.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Baolin Wang]

Hi Geert,

On 6 November 2018 at 23:57, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Baolin,
>
> On Tue, Oct 2, 2018 at 6:39 PM Baolin Wang <baolin.wang@linaro.org> wrote:
>> This patch adds one new led trigger that LED device can configure
>> the software or hardware pattern and trigger it.
>>
>> Consumers can write 'pattern' file to enable the software pattern
>> which alters the brightness for the specified duration with one
>> software timer.
>>
>> Moreover consumers can write 'hw_pattern' file to enable the hardware
>> pattern for some LED controllers which can autonomously control
>> brightness over time, according to some preprogrammed hardware
>> patterns.
>>
>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>
>> --- /dev/null
>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>> @@ -0,0 +1,431 @@
>
>> +static void pattern_trig_timer_function(struct timer_list *t)
>> +{
>> +       struct pattern_trig_data *data = from_timer(data, t, timer);
>> +
>> +       mutex_lock(&data->lock);
>
> Timer functions run in interrupt context, hence if CONFIG_DEBUG_ATOMIC_SLEEP=y:
>
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:254
> in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted
> 4.20.0-rc1-koelsch-00841-ga338c8181013c1a9 #171
> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> [<c020f19c>] (unwind_backtrace) from [<c020aecc>] (show_stack+0x10/0x14)
> [<c020aecc>] (show_stack) from [<c07affb8>] (dump_stack+0x7c/0x9c)
> [<c07affb8>] (dump_stack) from [<c02417d4>] (___might_sleep+0xf4/0x158)
> [<c02417d4>] (___might_sleep) from [<c07c92c4>] (mutex_lock+0x18/0x60)
> [<c07c92c4>] (mutex_lock) from [<c067b28c>]
> (pattern_trig_timer_function+0x1c/0x11c)
> [<c067b28c>] (pattern_trig_timer_function) from [<c027f6fc>]
> (call_timer_fn+0x1c/0x90)
> [<c027f6fc>] (call_timer_fn) from [<c027f944>] (expire_timers+0x94/0xa4)
> [<c027f944>] (expire_timers) from [<c027fc98>] (run_timer_softirq+0x108/0x15c)
> [<c027fc98>] (run_timer_softirq) from [<c02021cc>] (__do_softirq+0x1d4/0x258)
> [<c02021cc>] (__do_softirq) from [<c0224d24>] (irq_exit+0x64/0xc4)
> [<c0224d24>] (irq_exit) from [<c0268dd0>] (__handle_domain_irq+0x80/0xb4)
> [<c0268dd0>] (__handle_domain_irq) from [<c045e1b0>] (gic_handle_irq+0x58/0x90)
> [<c045e1b0>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x74)
> Exception stack(0xeb483f60 to 0xeb483fa8)
> 3f60: 00000000 00000000 eb9afaa0 c0217e80 00000000 ffffe000 00000000 c0e06408
> 3f80: 00000002 c0e0647c c0c6a5f0 00000000 c0e04900 eb483fb0 c0207ea8 c0207e98
> 3fa0: 60020013 ffffffff
> [<c02019f8>] (__irq_svc) from [<c0207e98>] (arch_cpu_idle+0x1c/0x38)
> [<c0207e98>] (arch_cpu_idle) from [<c0247ca8>] (do_idle+0x138/0x268)
> [<c0247ca8>] (do_idle) from [<c0248050>] (cpu_startup_entry+0x18/0x1c)
> [<c0248050>] (cpu_startup_entry) from [<402022ec>] (0x402022ec)
>
> Either this should use a spinlock instead of a mutex, or the timer
> function should kick a workqueue to do the real work.
>

Ah, sorry I missed this. Good catch.

After more thinking, the easy fix is that I think we can remove the
mutex lock in pattern_trig_timer_function(). Since the mutex lock is
used to protect the pattern trigger data, but before changing new
pattern trigger data (pattern values or repeat value) by users, we
always cancel the timer firstly to clear previous patterns'
performance. That means there is no race in
pattern_trig_timer_function(), so we can drop the mutex lock in
pattern_trig_timer_function() to avoid this issue.

I will send one patch to fix this issue. Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Pavel Machek]

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

On Wed 2018-11-07 11:23:52, Baolin Wang wrote:
> Hi Geert,
> 
> On 6 November 2018 at 23:57, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Hi Baolin,
> >
> > On Tue, Oct 2, 2018 at 6:39 PM Baolin Wang <baolin.wang@linaro.org> wrote:
> >> This patch adds one new led trigger that LED device can configure
> >> the software or hardware pattern and trigger it.
> >>
> >> Consumers can write 'pattern' file to enable the software pattern
> >> which alters the brightness for the specified duration with one
> >> software timer.
> >>
> >> Moreover consumers can write 'hw_pattern' file to enable the hardware
> >> pattern for some LED controllers which can autonomously control
> >> brightness over time, according to some preprogrammed hardware
> >> patterns.
> >>
> >> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> >
> >> --- /dev/null
> >> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> >> @@ -0,0 +1,431 @@
> >
> >> +static void pattern_trig_timer_function(struct timer_list *t)
> >> +{
> >> +       struct pattern_trig_data *data = from_timer(data, t, timer);
> >> +
> >> +       mutex_lock(&data->lock);
> >
> > Timer functions run in interrupt context, hence if CONFIG_DEBUG_ATOMIC_SLEEP=y:
> >
> > BUG: sleeping function called from invalid context at kernel/locking/mutex.c:254
> > in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/1
> > CPU: 1 PID: 0 Comm: swapper/1 Not tainted
> > 4.20.0-rc1-koelsch-00841-ga338c8181013c1a9 #171
> > Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> > [<c020f19c>] (unwind_backtrace) from [<c020aecc>] (show_stack+0x10/0x14)
> > [<c020aecc>] (show_stack) from [<c07affb8>] (dump_stack+0x7c/0x9c)
> > [<c07affb8>] (dump_stack) from [<c02417d4>] (___might_sleep+0xf4/0x158)
> > [<c02417d4>] (___might_sleep) from [<c07c92c4>] (mutex_lock+0x18/0x60)
> > [<c07c92c4>] (mutex_lock) from [<c067b28c>]
> > (pattern_trig_timer_function+0x1c/0x11c)
> > [<c067b28c>] (pattern_trig_timer_function) from [<c027f6fc>]
> > (call_timer_fn+0x1c/0x90)
> > [<c027f6fc>] (call_timer_fn) from [<c027f944>] (expire_timers+0x94/0xa4)
> > [<c027f944>] (expire_timers) from [<c027fc98>] (run_timer_softirq+0x108/0x15c)
> > [<c027fc98>] (run_timer_softirq) from [<c02021cc>] (__do_softirq+0x1d4/0x258)
> > [<c02021cc>] (__do_softirq) from [<c0224d24>] (irq_exit+0x64/0xc4)
> > [<c0224d24>] (irq_exit) from [<c0268dd0>] (__handle_domain_irq+0x80/0xb4)
> > [<c0268dd0>] (__handle_domain_irq) from [<c045e1b0>] (gic_handle_irq+0x58/0x90)
> > [<c045e1b0>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x74)
> > Exception stack(0xeb483f60 to 0xeb483fa8)
> > 3f60: 00000000 00000000 eb9afaa0 c0217e80 00000000 ffffe000 00000000 c0e06408
> > 3f80: 00000002 c0e0647c c0c6a5f0 00000000 c0e04900 eb483fb0 c0207ea8 c0207e98
> > 3fa0: 60020013 ffffffff
> > [<c02019f8>] (__irq_svc) from [<c0207e98>] (arch_cpu_idle+0x1c/0x38)
> > [<c0207e98>] (arch_cpu_idle) from [<c0247ca8>] (do_idle+0x138/0x268)
> > [<c0247ca8>] (do_idle) from [<c0248050>] (cpu_startup_entry+0x18/0x1c)
> > [<c0248050>] (cpu_startup_entry) from [<402022ec>] (0x402022ec)
> >
> > Either this should use a spinlock instead of a mutex, or the timer
> > function should kick a workqueue to do the real work.
> >
> 
> Ah, sorry I missed this. Good catch.
> 
> After more thinking, the easy fix is that I think we can remove the
> mutex lock in pattern_trig_timer_function(). Since the mutex lock is
> used to protect the pattern trigger data, but before changing new
> pattern trigger data (pattern values or repeat value) by users, we
> always cancel the timer firstly to clear previous patterns'
> performance. That means there is no race in
> pattern_trig_timer_function(), so we can drop the mutex lock in
> pattern_trig_timer_function() to avoid this issue.
> 
> I will send one patch to fix this issue. Thanks.

That's kind of interesting, but yes, it should work.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]