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

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

[Baolin Wang]

Some LED controllers have support for autonomously controlling
brightness over time, according to some preprogrammed pattern or
function.

This patch adds pattern trigger that LED device can configure the
pattern and trigger it.

Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 .../ABI/testing/sysfs-class-led-trigger-pattern    |   21 ++
 drivers/leds/trigger/Kconfig                       |   10 +
 drivers/leds/trigger/Makefile                      |    1 +
 drivers/leds/trigger/ledtrig-pattern.c             |  349 ++++++++++++++++++++
 include/linux/leds.h                               |   19 ++
 5 files changed, 400 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..c52da34
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
@@ -0,0 +1,21 @@
+What:		/sys/class/leds/<led>/pattern
+Date:		August 2018
+KernelVersion:	4.19
+Description:
+		Specify a pattern for the LED, for LED hardware that support
+		altering the brightness as a function of time.
+
+		The pattern is given by a series of tuples, of brightness and
+		duration (ms). The LED is expected to traverse the series and
+		each brightness value for the specified duration. Duration of
+		0 means brightness should immediately change to new value.
+
+		The format of the pattern values should be:
+		"brightness_1 duration_1, brightness_2 duration_2, brightness_3
+		duration_3, ...".
+
+What:		/sys/class/leds/<led>/repeat
+Date:		August 2018
+KernelVersion:	4.19
+Description:
+		Specify a pattern repeat number. 0 means repeat indefinitely.
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index a2559b4..a03afcd 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA
 	  This enables direct flash/torch on/off by the driver, kernel space.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_PATTERN
+       tristate "LED Pattern Trigger"
+       depends on LEDS_TRIGGERS
+       help
+         This allows LEDs blinking with an arbitrary pattern. Can be useful
+         on embedded systems with no screen to give out a status code to
+         a human.
+
+         If unsure, say N
+
 config LEDS_TRIGGER_PANIC
 	bool "LED Panic Trigger"
 	depends on LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index f3cfe19..c5d180e 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..b709aa1
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-pattern.c
@@ -0,0 +1,349 @@
+// 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>
+
+/*
+ * The "pattern" attribute contains at most PAGE_SIZE characters. Each line
+ * in this attribute is at least 4 characters long (a 1-digit number for the
+ * led brighntess, a space, a 1-digit number for the time, a semi-colon).
+ * Therefore, there is at most PAGE_SIZE/4 patterns.
+ */
+#define MAX_PATTERNS		(PAGE_SIZE / 4)
+#define PATTERN_SEPARATOR	","
+
+struct pattern_trig_data {
+	struct led_classdev *led_cdev;
+	struct led_pattern *patterns;
+	struct led_pattern *curr;
+	struct led_pattern *next;
+	struct mutex lock;
+	u32 npatterns;
+	u32 repeat;
+	bool is_indefinite;
+	struct timer_list timer;
+};
+
+static void 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++;
+}
+
+static void pattern_trig_timer_function(struct timer_list *t)
+{
+	struct pattern_trig_data *data = from_timer(data, t, timer);
+
+	mutex_lock(&data->lock);
+
+	if (!data->is_indefinite && !data->repeat) {
+		mutex_unlock(&data->lock);
+		return;
+	}
+
+	led_set_brightness(data->led_cdev, data->curr->brightness);
+	mod_timer(&data->timer, jiffies + msecs_to_jiffies(data->curr->delta_t));
+	pattern_trig_update_patterns(data);
+
+	mutex_unlock(&data->lock);
+}
+
+static int pattern_trig_start_pattern(struct pattern_trig_data *data,
+				       struct led_classdev *led_cdev)
+{
+	if (!data->npatterns)
+		return 0;
+
+	if (led_cdev->pattern_clear)
+		led_cdev->pattern_clear(led_cdev);
+
+	if (led_cdev->pattern_set) {
+		return led_cdev->pattern_set(led_cdev, data->patterns,
+					     data->npatterns, data->repeat);
+	}
+
+	data->curr = data->patterns;
+	data->next = data->patterns + 1;
+	data->timer.expires = jiffies;
+	add_timer(&data->timer);
+
+	return 0;
+}
+
+static ssize_t pattern_trig_show_repeat(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 err;
+	u32 repeat;
+
+	mutex_lock(&data->lock);
+
+	if (led_cdev->pattern_get) {
+		err = led_cdev->pattern_get(led_cdev, data->patterns,
+					    &data->npatterns, &data->repeat);
+		if (err) {
+			mutex_unlock(&data->lock);
+			return err;
+		}
+	}
+
+	repeat = data->repeat;
+	mutex_unlock(&data->lock);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", repeat);
+}
+
+static ssize_t pattern_trig_store_repeat(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+	unsigned long res;
+	int err;
+
+	err = kstrtoul(buf, 10, &res);
+	if (err)
+		return err;
+
+	del_timer_sync(&data->timer);
+
+	mutex_lock(&data->lock);
+	data->repeat = res;
+
+	/* 0 means repeat indefinitely */
+	if (!data->repeat)
+		data->is_indefinite = true;
+	else
+		data->is_indefinite = false;
+
+	err = pattern_trig_start_pattern(data, led_cdev);
+	if (err) {
+		mutex_unlock(&data->lock);
+		return err;
+	}
+
+	mutex_unlock(&data->lock);
+	return count;
+}
+
+static DEVICE_ATTR(repeat, 0644, pattern_trig_show_repeat,
+		   pattern_trig_store_repeat);
+
+static ssize_t pattern_trig_show_pattern(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;
+	ssize_t count = 0;
+	int err, i;
+
+	mutex_lock(&data->lock);
+
+	if (led_cdev->pattern_get) {
+		err = led_cdev->pattern_get(led_cdev, data->patterns,
+					    &data->npatterns, &data->repeat);
+		if (err) {
+			mutex_unlock(&data->lock);
+			return err;
+		}
+	}
+
+	if (!data->npatterns) {
+		mutex_unlock(&data->lock);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < data->npatterns; i++) {
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "%d %d" PATTERN_SEPARATOR,
+				   data->patterns[i].brightness,
+				   data->patterns[i].delta_t);
+	}
+
+	buf[count - 1] = '\n';
+	buf[count] = '\0';
+
+	mutex_unlock(&data->lock);
+	return count + 1;
+}
+
+static ssize_t pattern_trig_store_pattern(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, cr, ccount, tcr = 0;
+
+	del_timer_sync(&data->timer);
+
+	mutex_lock(&data->lock);
+
+	for (data->npatterns = 0; data->npatterns < MAX_PATTERNS;
+	     data->npatterns++) {
+		/* Characters read on one conversion */
+		cr = 0;
+		/* Number of successful conversions */
+		ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n",
+				&data->patterns[data->npatterns].brightness,
+				&data->patterns[data->npatterns].delta_t, &cr);
+
+		/* Total characters read */
+		tcr += cr;
+		if (cr)
+			continue;
+
+		/* Invalid syntax or end of pattern */
+		if (ccount == 2)
+			data->npatterns++;
+
+		err = pattern_trig_start_pattern(data, led_cdev);
+		if (err) {
+			mutex_unlock(&data->lock);
+			return err;
+		}
+
+		mutex_unlock(&data->lock);
+		return count;
+	}
+
+	/* Shouldn't reach that */
+	WARN(1, "MAX_NSTEP too small. Please report\n");
+	mutex_unlock(&data->lock);
+	return count;
+}
+
+static DEVICE_ATTR(pattern, 0644, pattern_trig_show_pattern,
+		   pattern_trig_store_pattern);
+
+static int pattern_trig_create_sysfs_files(struct device *dev)
+{
+	int err;
+
+	err = device_create_file(dev, &dev_attr_repeat);
+	if (err)
+		return err;
+
+	err = device_create_file(dev, &dev_attr_pattern);
+	if (err)
+		device_remove_file(dev, &dev_attr_repeat);
+
+	return err;
+}
+
+static void pattern_trig_remove_sysfs_files(struct device *dev)
+{
+	device_remove_file(dev, &dev_attr_pattern);
+	device_remove_file(dev, &dev_attr_repeat);
+}
+
+static int pattern_trig_initialize_data(struct pattern_trig_data *data)
+{
+	data->patterns = kcalloc(MAX_PATTERNS, sizeof(struct led_pattern),
+				 GFP_KERNEL);
+	if (!data->patterns)
+		return -ENOMEM;
+
+	data->is_indefinite = true;
+	mutex_init(&data->lock);
+
+	return 0;
+}
+
+static void pattern_trig_clear_data(struct pattern_trig_data *data)
+{
+	kfree(data->patterns);
+}
+
+static void pattern_trig_activate(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *data;
+	int err;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return;
+
+	err = pattern_trig_initialize_data(data);
+	if (err) {
+		kfree(data);
+		return;
+	}
+
+	err = pattern_trig_create_sysfs_files(led_cdev->dev);
+	if (err) {
+		pattern_trig_clear_data(data);
+		kfree(data);
+		return;
+	}
+
+	data->led_cdev = led_cdev;
+	led_cdev->trigger_data = data;
+	timer_setup(&data->timer, pattern_trig_timer_function, 0);
+	led_cdev->activated = true;
+}
+
+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);
+	pattern_trig_remove_sysfs_files(led_cdev->dev);
+	led_set_brightness(led_cdev, LED_OFF);
+	pattern_trig_clear_data(data);
+	kfree(data);
+	led_cdev->trigger_data = NULL;
+	led_cdev->activated = false;
+}
+
+static struct led_trigger pattern_led_trigger = {
+	.name = "pattern",
+	.activate = pattern_trig_activate,
+	.deactivate = pattern_trig_deactivate,
+};
+
+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 b7e8255..bea02f0 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,14 @@ struct led_classdev {
 				     unsigned long *delay_on,
 				     unsigned long *delay_off);
 
+	int (*pattern_set)(struct led_classdev *led_cdev,
+			   struct led_pattern *pattern, int len,
+			   unsigned repeat);
+	int (*pattern_get)(struct led_classdev *led_cdev,
+			   struct led_pattern *pattern, int *len,
+			   unsigned *repeat);
+	int (*pattern_clear)(struct led_classdev *led_cdev);
+
 	struct device		*dev;
 	const struct attribute_group	**groups;
 
@@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
 	struct led_classdev *led_cdev, enum led_brightness brightness) { }
 #endif
 
+/**
+ * struct led_pattern - brightness value in a pattern
+ * @delta_t: delay until next entry, in milliseconds
+ * @brightness: brightness at time = 0
+ */
+struct led_pattern {
+	int delta_t;
+	int brightness;
+};
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
1.7.9.5

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

[Baolin Wang]

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

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/leds/leds-sc27xx-bltc.c |  151 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 151 insertions(+)

diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..ebe23e1 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -32,8 +32,13 @@
 #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
 
 struct sc27xx_led {
 	char name[LED_MAX_NAME_SIZE];
@@ -122,6 +127,148 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum led_brightness value)
 	return err;
 }
 
+static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
+{
+	struct sc27xx_led *leds = to_sc27xx_led(ldev);
+	struct regmap *regmap = leds->priv->regmap;
+	u32 base = sc27xx_led_get_offset(leds);
+	u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+	u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+	int err;
+
+	mutex_lock(&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);
+
+	mutex_unlock(&leds->priv->lock);
+
+	return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+				  struct led_pattern *pattern,
+				  int len, u32 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 patterns to configure the rise time, high time, fall
+	 * time and low time to enable the breathing mode.
+	 */
+	if (len != SC27XX_LEDS_PATTERN_CNT)
+		return -EINVAL;
+
+	mutex_lock(&leds->priv->lock);
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+				 SC27XX_CURVE_L_MASK, pattern[0].delta_t);
+	if (err)
+		goto out;
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+				 SC27XX_CURVE_L_MASK, pattern[1].delta_t);
+	if (err)
+		goto out;
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+				 SC27XX_CURVE_H_MASK,
+				 pattern[2].delta_t << SC27XX_CURVE_SHIFT);
+	if (err)
+		goto out;
+
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+				 SC27XX_CURVE_H_MASK,
+				 pattern[3].delta_t << SC27XX_CURVE_SHIFT);
+	if (err)
+		goto out;
+
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_DUTY,
+				 SC27XX_DUTY_MASK,
+				 (pattern[0].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);
+
+out:
+	mutex_unlock(&leds->priv->lock);
+
+	return err;
+}
+
+static int sc27xx_led_pattern_get(struct led_classdev *ldev,
+				  struct led_pattern *pattern, int *len,
+				  u32 *repeat)
+{
+	struct sc27xx_led *leds = to_sc27xx_led(ldev);
+	u32 base = sc27xx_led_get_offset(leds);
+	struct regmap *regmap = leds->priv->regmap;
+	int i, err;
+	u32 val;
+
+	mutex_lock(&leds->priv->lock);
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val);
+	if (err)
+		goto out;
+
+	pattern[0].delta_t = val & SC27XX_CURVE_L_MASK;
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val);
+	if (err)
+		goto out;
+
+	pattern[1].delta_t = val & SC27XX_CURVE_L_MASK;
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val);
+	if (err)
+		goto out;
+
+	pattern[2].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT;
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val);
+	if (err)
+		goto out;
+
+	pattern[3].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT;
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_DUTY, &val);
+	if (err)
+		goto out;
+
+	mutex_unlock(&leds->priv->lock);
+
+	val = (val & SC27XX_DUTY_MASK) >> SC27XX_DUTY_SHIFT;
+	for (i = 0; i < SC27XX_LEDS_PATTERN_CNT; i++)
+		pattern[i].brightness = val;
+
+	*len = SC27XX_LEDS_PATTERN_CNT;
+	*repeat = 0;
+
+	return 0;
+
+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 +287,10 @@ 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_get = sc27xx_led_pattern_get;
+		led->ldev.pattern_clear = sc27xx_led_pattern_clear;
+		led->ldev.default_trigger = "pattern";
 
 		err = devm_led_classdev_register(dev, &led->ldev);
 		if (err)
-- 
1.7.9.5

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

[Bjorn Andersson]

On Mon 30 Jul 05:29 PDT 2018, Baolin Wang wrote:

> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
> 
> This patch adds pattern trigger that LED device can configure the
> pattern and trigger it.
> 
> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  .../ABI/testing/sysfs-class-led-trigger-pattern    |   21 ++
>  drivers/leds/trigger/Kconfig                       |   10 +
>  drivers/leds/trigger/Makefile                      |    1 +
>  drivers/leds/trigger/ledtrig-pattern.c             |  349 ++++++++++++++++++++
>  include/linux/leds.h                               |   19 ++
>  5 files changed, 400 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..c52da34
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> @@ -0,0 +1,21 @@
> +What:		/sys/class/leds/<led>/pattern
> +Date:		August 2018
> +KernelVersion:	4.19
> +Description:
> +		Specify a pattern for the LED, for LED hardware that support
> +		altering the brightness as a function of time.
> +
> +		The pattern is given by a series of tuples, of brightness and
> +		duration (ms). The LED is expected to traverse the series and
> +		each brightness value for the specified duration. Duration of
> +		0 means brightness should immediately change to new value.
> +
> +		The format of the pattern values should be:
> +		"brightness_1 duration_1, brightness_2 duration_2, brightness_3
> +		duration_3, ...".
> +
> +What:		/sys/class/leds/<led>/repeat
> +Date:		August 2018
> +KernelVersion:	4.19
> +Description:
> +		Specify a pattern repeat number. 0 means repeat indefinitely.

If 0 means infinity, does 1 mean "repeat 1 time"? If so how would I
specify that I want the pattern to run one time (i.e. 0 repetitions).

> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index a2559b4..a03afcd 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA
>  	  This enables direct flash/torch on/off by the driver, kernel space.
>  	  If unsure, say Y.
>  
> +config LEDS_TRIGGER_PATTERN
> +       tristate "LED Pattern Trigger"
> +       depends on LEDS_TRIGGERS
> +       help
> +         This allows LEDs blinking with an arbitrary pattern. Can be useful
> +         on embedded systems with no screen to give out a status code to
> +         a human.

While the pattern mechanism could be used to communicate some message
the use cases we've seen so far is all about enabling hardware to pulse
LEDs instead of blinking them...

> +
> +         If unsure, say N
> +
>  config LEDS_TRIGGER_PANIC
>  	bool "LED Panic Trigger"
>  	depends on LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index f3cfe19..c5d180e 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..b709aa1
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,349 @@
> +// 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.

Might be a coincidence, but parts of this patch looks pretty close to
mine...

> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +
> +/*
> + * The "pattern" attribute contains at most PAGE_SIZE characters. Each line
> + * in this attribute is at least 4 characters long (a 1-digit number for the
> + * led brighntess, a space, a 1-digit number for the time, a semi-colon).
> + * Therefore, there is at most PAGE_SIZE/4 patterns.
> + */

The brightness is a number between 0 and LED_FULL (or max_brightness)
and delta_t is measured in ms. So neither of these are likely to be a
single digit very often.

> +#define MAX_PATTERNS		(PAGE_SIZE / 4)
> +#define PATTERN_SEPARATOR	","
> +
> +struct pattern_trig_data {
> +	struct led_classdev *led_cdev;
> +	struct led_pattern *patterns;
> +	struct led_pattern *curr;
> +	struct led_pattern *next;
> +	struct mutex lock;
> +	u32 npatterns;
> +	u32 repeat;
> +	bool is_indefinite;
> +	struct timer_list timer;
> +};
> +
> +static void pattern_trig_update_patterns(struct pattern_trig_data *data)

I think you should just inline this logic into
pattern_trig_timer_function() to make it obvious that this is only
invoked from the timer function.

And if you do this you can decrement data->repeat unconditionally after
the check for !is_indefinite && repeat == 0.

> +{
> +	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++;

How about replacing this conditional with:

	data->next = (data->next + 1) % data->npatterns;

> +}
> +
> +static void pattern_trig_timer_function(struct timer_list *t)
> +{
> +	struct pattern_trig_data *data = from_timer(data, t, timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	if (!data->is_indefinite && !data->repeat) {
> +		mutex_unlock(&data->lock);
> +		return;
> +	}
> +
> +	led_set_brightness(data->led_cdev, data->curr->brightness);
> +	mod_timer(&data->timer, jiffies + msecs_to_jiffies(data->curr->delta_t));
> +	pattern_trig_update_patterns(data);
> +
> +	mutex_unlock(&data->lock);
> +}
> +
> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
> +				       struct led_classdev *led_cdev)
> +{
> +	if (!data->npatterns)
> +		return 0;
> +
> +	if (led_cdev->pattern_clear)
> +		led_cdev->pattern_clear(led_cdev);
> +
> +	if (led_cdev->pattern_set) {

In my proposal setting an empty pattern would invoke pattern_clear() and
setting a pattern would invoke pattern_set(), this allows the driver to
fail the pattern_set() without clearing the old pattern.

> +		return led_cdev->pattern_set(led_cdev, data->patterns,
> +					     data->npatterns, data->repeat);

repeat is communicated to the driver, but is_indefinite is not. At least
in my driver the latter

> +	}

I think it would be useful to put the rest in an else statement, to
clarify that the patterns is either implemented by the driver or by the
software timer. It might also make sense to group the check for
pattern_clear() and pattern_set(), as it wouldn't make sense for the
driver to implement only one of these.

> +
> +	data->curr = data->patterns;
> +	data->next = data->patterns + 1;
> +	data->timer.expires = jiffies;
> +	add_timer(&data->timer);
> +
> +	return 0;
> +}
> +
> +static ssize_t pattern_trig_show_repeat(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 err;
> +	u32 repeat;
> +
> +	mutex_lock(&data->lock);
> +
> +	if (led_cdev->pattern_get) {
> +		err = led_cdev->pattern_get(led_cdev, data->patterns,
> +					    &data->npatterns, &data->repeat);

I don't see why "repeat" would have changed since you told the driver,
so data->repeat should have the right value already.

> +		if (err) {
> +			mutex_unlock(&data->lock);
> +			return err;
> +		}
> +	}
> +
> +	repeat = data->repeat;
> +	mutex_unlock(&data->lock);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%u\n", repeat);
> +}
> +
> +static ssize_t pattern_trig_store_repeat(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +	unsigned long res;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &res);
> +	if (err)
> +		return err;
> +
> +	del_timer_sync(&data->timer);
> +
> +	mutex_lock(&data->lock);
> +	data->repeat = res;
> +
> +	/* 0 means repeat indefinitely */

I think that it would be better to specify indefinite by writing e.g.
"Inf" to the repeat.

Also note that it's possible that there might be hardware restrictions
that only allows specific combinations of these; e.g. the Qualcomm
hardware can only do no-repeat or repeat-forever.

> +	if (!data->repeat)
> +		data->is_indefinite = true;
> +	else
> +		data->is_indefinite = false;
> +
> +	err = pattern_trig_start_pattern(data, led_cdev);
> +	if (err) {
> +		mutex_unlock(&data->lock);
> +		return err;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(repeat, 0644, pattern_trig_show_repeat,
> +		   pattern_trig_store_repeat);
> +
> +static ssize_t pattern_trig_show_pattern(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;
> +	ssize_t count = 0;
> +	int err, i;
> +
> +	mutex_lock(&data->lock);
> +
> +	if (led_cdev->pattern_get) {
> +		err = led_cdev->pattern_get(led_cdev, data->patterns,
> +					    &data->npatterns, &data->repeat);
> +		if (err) {
> +			mutex_unlock(&data->lock);
> +			return err;
> +		}
> +	}

Rather than sometimes calling pattern_get() here, how about defining
that pattern_set() should update data->patterns before returning and
just rely on data->patterns being right.

(The reason for the pattern not being the same as passed in store would
be to allow the driver to adjust the parameters to the match the
hardware requirements and present this to the user)

> +
> +	if (!data->npatterns) {
> +		mutex_unlock(&data->lock);
> +		return -EINVAL;
> +	}

Rather than failing I think you should return the empty string if the
pattern is empty.

> +
> +	for (i = 0; i < data->npatterns; i++) {
> +		count += scnprintf(buf + count, PAGE_SIZE - count,
> +				   "%d %d" PATTERN_SEPARATOR,
> +				   data->patterns[i].brightness,
> +				   data->patterns[i].delta_t);
> +	}
> +
> +	buf[count - 1] = '\n';
> +	buf[count] = '\0';
> +
> +	mutex_unlock(&data->lock);
> +	return count + 1;

As you're returning a string I think you should return the number of
chars in the string, not including the '\0'.

> +}
> +
> +static ssize_t pattern_trig_store_pattern(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, cr, ccount, tcr = 0;
> +
> +	del_timer_sync(&data->timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	for (data->npatterns = 0; data->npatterns < MAX_PATTERNS;
> +	     data->npatterns++) {
> +		/* Characters read on one conversion */
> +		cr = 0;
> +		/* Number of successful conversions */
> +		ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n",
> +				&data->patterns[data->npatterns].brightness,
> +				&data->patterns[data->npatterns].delta_t, &cr);

I'm not fond of the use of sscanf() here, but at least you should make
sure that tcr doesn't go past PAGE_SIZE.

> +
> +		/* Total characters read */
> +		tcr += cr;
> +		if (cr)
> +			continue;

This loop is non-intuitive...

You loop while npatterns is less than MAX_PATTERNS and the first half
will loop if sscanf() matched the pattern otherwise you'll return from
withing the loop.

> +
> +		/* Invalid syntax or end of pattern */
> +		if (ccount == 2)

I guess you'll get here if you match the two %d, but not the
PATTERN_SEPARATOR? Wouldn't it be useful to fail here if ccount is !2.

> +			data->npatterns++;
> +
> +		err = pattern_trig_start_pattern(data, led_cdev);
> +		if (err) {
> +			mutex_unlock(&data->lock);
> +			return err;
> +		}
> +
> +		mutex_unlock(&data->lock);
> +		return count;
> +	}
> +
> +	/* Shouldn't reach that */

This means that you failed to set the pattern, so you probably shouldn't
return count - but rather -EINVAL or something.

> +	WARN(1, "MAX_NSTEP too small. Please report\n");
> +	mutex_unlock(&data->lock);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(pattern, 0644, pattern_trig_show_pattern,
> +		   pattern_trig_store_pattern);
> +
> +static int pattern_trig_create_sysfs_files(struct device *dev)
> +{
> +	int err;
> +
> +	err = device_create_file(dev, &dev_attr_repeat);
> +	if (err)
> +		return err;
> +
> +	err = device_create_file(dev, &dev_attr_pattern);
> +	if (err)
> +		device_remove_file(dev, &dev_attr_repeat);
> +
> +	return err;
> +}
> +
> +static void pattern_trig_remove_sysfs_files(struct device *dev)
> +{
> +	device_remove_file(dev, &dev_attr_pattern);
> +	device_remove_file(dev, &dev_attr_repeat);
> +}
> +
> +static int pattern_trig_initialize_data(struct pattern_trig_data *data)

These two functions would be better to just inline, but...

> +{
> +	data->patterns = kcalloc(MAX_PATTERNS, sizeof(struct led_pattern),
> +				 GFP_KERNEL);

...as this always allocate 1024 entries of led_pattern associated with
the pattern_trig_data you should just add 1024 entries of led_pattern to
the pattern_trig_data struct.

> +	if (!data->patterns)
> +		return -ENOMEM;
> +
> +	data->is_indefinite = true;
> +	mutex_init(&data->lock);
> +
> +	return 0;
> +}
> +
> +static void pattern_trig_clear_data(struct pattern_trig_data *data)
> +{
> +	kfree(data->patterns);
> +}
> +
> +static void pattern_trig_activate(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data;
> +	int err;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return;
> +
> +	err = pattern_trig_initialize_data(data);
> +	if (err) {
> +		kfree(data);
> +		return;
> +	}
> +
> +	err = pattern_trig_create_sysfs_files(led_cdev->dev);
> +	if (err) {
> +		pattern_trig_clear_data(data);
> +		kfree(data);
> +		return;
> +	}
> +
> +	data->led_cdev = led_cdev;
> +	led_cdev->trigger_data = data;
> +	timer_setup(&data->timer, pattern_trig_timer_function, 0);
> +	led_cdev->activated = true;
> +}
> +
> +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);

This should only happen when not having using the driver based
mechanism, so put this in an else to clarify this.

> +	pattern_trig_remove_sysfs_files(led_cdev->dev);

Inline this and one doesn't have to jump around to figure out what's
going on.

> +	led_set_brightness(led_cdev, LED_OFF);
> +	pattern_trig_clear_data(data);
> +	kfree(data);
> +	led_cdev->trigger_data = NULL;
> +	led_cdev->activated = false;
> +}
> +
> +static struct led_trigger pattern_led_trigger = {
> +	.name = "pattern",
> +	.activate = pattern_trig_activate,
> +	.deactivate = pattern_trig_deactivate,
> +};
> +
> +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 b7e8255..bea02f0 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,14 @@ struct led_classdev {
>  				     unsigned long *delay_on,
>  				     unsigned long *delay_off);
>  
> +	int (*pattern_set)(struct led_classdev *led_cdev,
> +			   struct led_pattern *pattern, int len,
> +			   unsigned repeat);
> +	int (*pattern_get)(struct led_classdev *led_cdev,
> +			   struct led_pattern *pattern, int *len,
> +			   unsigned *repeat);
> +	int (*pattern_clear)(struct led_classdev *led_cdev);
> +
>  	struct device		*dev;
>  	const struct attribute_group	**groups;
>  
> @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
>  	struct led_classdev *led_cdev, enum led_brightness brightness) { }
>  #endif
>  
> +/**
> + * struct led_pattern - brightness value in a pattern
> + * @delta_t: delay until next entry, in milliseconds
> + * @brightness: brightness at time = 0
> + */
> +struct led_pattern {
> +	int delta_t;
> +	int brightness;
> +};
> +
>  #endif		/* __LINUX_LEDS_H_INCLUDED */
> -- 
> 1.7.9.5
> 

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

[Baolin Wang]

Hi Bjorn,

On 31 July 2018 at 11:54, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> On Mon 30 Jul 05:29 PDT 2018, Baolin Wang wrote:
>
>> Some LED controllers have support for autonomously controlling
>> brightness over time, according to some preprogrammed pattern or
>> function.
>>
>> This patch adds pattern trigger that LED device can configure the
>> pattern and trigger it.
>>
>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  .../ABI/testing/sysfs-class-led-trigger-pattern    |   21 ++
>>  drivers/leds/trigger/Kconfig                       |   10 +
>>  drivers/leds/trigger/Makefile                      |    1 +
>>  drivers/leds/trigger/ledtrig-pattern.c             |  349 ++++++++++++++++++++
>>  include/linux/leds.h                               |   19 ++
>>  5 files changed, 400 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..c52da34
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> @@ -0,0 +1,21 @@
>> +What:                /sys/class/leds/<led>/pattern
>> +Date:                August 2018
>> +KernelVersion:       4.19
>> +Description:
>> +             Specify a pattern for the LED, for LED hardware that support
>> +             altering the brightness as a function of time.
>> +
>> +             The pattern is given by a series of tuples, of brightness and
>> +             duration (ms). The LED is expected to traverse the series and
>> +             each brightness value for the specified duration. Duration of
>> +             0 means brightness should immediately change to new value.
>> +
>> +             The format of the pattern values should be:
>> +             "brightness_1 duration_1, brightness_2 duration_2, brightness_3
>> +             duration_3, ...".
>> +
>> +What:                /sys/class/leds/<led>/repeat
>> +Date:                August 2018
>> +KernelVersion:       4.19
>> +Description:
>> +             Specify a pattern repeat number. 0 means repeat indefinitely.
>
> If 0 means infinity, does 1 mean "repeat 1 time"? If so how would I

Right.

> specify that I want the pattern to run one time (i.e. 0 repetitions).

If you set repeat to 1 to make the driver clear the  is_indefinite
flag. So when the repeat number decreases to 0, it will stop.

>
>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>> index a2559b4..a03afcd 100644
>> --- a/drivers/leds/trigger/Kconfig
>> +++ b/drivers/leds/trigger/Kconfig
>> @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA
>>         This enables direct flash/torch on/off by the driver, kernel space.
>>         If unsure, say Y.
>>
>> +config LEDS_TRIGGER_PATTERN
>> +       tristate "LED Pattern Trigger"
>> +       depends on LEDS_TRIGGERS
>> +       help
>> +         This allows LEDs blinking with an arbitrary pattern. Can be useful
>> +         on embedded systems with no screen to give out a status code to
>> +         a human.
>
> While the pattern mechanism could be used to communicate some message
> the use cases we've seen so far is all about enabling hardware to pulse
> LEDs instead of blinking them...

Right. I will improve the message here.

>
>> +
>> +         If unsure, say N
>> +
>>  config LEDS_TRIGGER_PANIC
>>       bool "LED Panic Trigger"
>>       depends on LEDS_TRIGGERS
>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
>> index f3cfe19..c5d180e 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..b709aa1
>> --- /dev/null
>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>> @@ -0,0 +1,349 @@
>> +// 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.
>
> Might be a coincidence, but parts of this patch looks pretty close to
> mine...
>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/slab.h>
>> +#include <linux/timer.h>
>> +
>> +/*
>> + * The "pattern" attribute contains at most PAGE_SIZE characters. Each line
>> + * in this attribute is at least 4 characters long (a 1-digit number for the
>> + * led brighntess, a space, a 1-digit number for the time, a semi-colon).
>> + * Therefore, there is at most PAGE_SIZE/4 patterns.
>> + */
>
> The brightness is a number between 0 and LED_FULL (or max_brightness)
> and delta_t is measured in ms. So neither of these are likely to be a
> single digit very often.

OK. Will fix these comments.

>
>> +#define MAX_PATTERNS         (PAGE_SIZE / 4)
>> +#define PATTERN_SEPARATOR    ","
>> +
>> +struct pattern_trig_data {
>> +     struct led_classdev *led_cdev;
>> +     struct led_pattern *patterns;
>> +     struct led_pattern *curr;
>> +     struct led_pattern *next;
>> +     struct mutex lock;
>> +     u32 npatterns;
>> +     u32 repeat;
>> +     bool is_indefinite;
>> +     struct timer_list timer;
>> +};
>> +
>> +static void pattern_trig_update_patterns(struct pattern_trig_data *data)
>
> I think you should just inline this logic into
> pattern_trig_timer_function() to make it obvious that this is only
> invoked from the timer function.
>
> And if you do this you can decrement data->repeat unconditionally after
> the check for !is_indefinite && repeat == 0.

OK.

>
>> +{
>> +     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++;
>
> How about replacing this conditional with:
>
>         data->next = (data->next + 1) % data->npatterns;

Good suggestion. Will do in next version.

>
>> +}
>> +
>> +static void pattern_trig_timer_function(struct timer_list *t)
>> +{
>> +     struct pattern_trig_data *data = from_timer(data, t, timer);
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     if (!data->is_indefinite && !data->repeat) {
>> +             mutex_unlock(&data->lock);
>> +             return;
>> +     }
>> +
>> +     led_set_brightness(data->led_cdev, data->curr->brightness);
>> +     mod_timer(&data->timer, jiffies + msecs_to_jiffies(data->curr->delta_t));
>> +     pattern_trig_update_patterns(data);
>> +
>> +     mutex_unlock(&data->lock);
>> +}
>> +
>> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
>> +                                    struct led_classdev *led_cdev)
>> +{
>> +     if (!data->npatterns)
>> +             return 0;
>> +
>> +     if (led_cdev->pattern_clear)
>> +             led_cdev->pattern_clear(led_cdev);
>> +
>> +     if (led_cdev->pattern_set) {
>
> In my proposal setting an empty pattern would invoke pattern_clear() and
> setting a pattern would invoke pattern_set(), this allows the driver to
> fail the pattern_set() without clearing the old pattern.

Pavel wanted "echo none > trigger" to clear the pattern. So I am fine
remove the 'pattern_clear' operation in this function.

>
>> +             return led_cdev->pattern_set(led_cdev, data->patterns,
>> +                                          data->npatterns, data->repeat);
>
> repeat is communicated to the driver, but is_indefinite is not. At least
> in my driver the latter
>
>> +     }
>
> I think it would be useful to put the rest in an else statement, to
> clarify that the patterns is either implemented by the driver or by the
> software timer. It might also make sense to group the check for
> pattern_clear() and pattern_set(), as it wouldn't make sense for the
> driver to implement only one of these.

But checkpatch will complain the 'else' statement is redundant, maybe
add some comments here to make things clear?

>
>> +
>> +     data->curr = data->patterns;
>> +     data->next = data->patterns + 1;
>> +     data->timer.expires = jiffies;
>> +     add_timer(&data->timer);
>> +
>> +     return 0;
>> +}
>> +
>> +static ssize_t pattern_trig_show_repeat(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 err;
>> +     u32 repeat;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     if (led_cdev->pattern_get) {
>> +             err = led_cdev->pattern_get(led_cdev, data->patterns,
>> +                                         &data->npatterns, &data->repeat);
>
> I don't see why "repeat" would have changed since you told the driver,
> so data->repeat should have the right value already.

Some drivers don't care about the repeat, and it will always repeat
indefinitely. So no matter what values set from userspace, driver can
set it to 0.

>
>> +             if (err) {
>> +                     mutex_unlock(&data->lock);
>> +                     return err;
>> +             }
>> +     }
>> +
>> +     repeat = data->repeat;
>> +     mutex_unlock(&data->lock);
>> +
>> +     return scnprintf(buf, PAGE_SIZE, "%u\n", repeat);
>> +}
>> +
>> +static ssize_t pattern_trig_store_repeat(struct device *dev,
>> +                                      struct device_attribute *attr,
>> +                                      const char *buf, size_t count)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +     struct pattern_trig_data *data = led_cdev->trigger_data;
>> +     unsigned long res;
>> +     int err;
>> +
>> +     err = kstrtoul(buf, 10, &res);
>> +     if (err)
>> +             return err;
>> +
>> +     del_timer_sync(&data->timer);
>> +
>> +     mutex_lock(&data->lock);
>> +     data->repeat = res;
>> +
>> +     /* 0 means repeat indefinitely */
>
> I think that it would be better to specify indefinite by writing e.g.
> "Inf" to the repeat.
>
> Also note that it's possible that there might be hardware restrictions
> that only allows specific combinations of these; e.g. the Qualcomm
> hardware can only do no-repeat or repeat-forever.

Can you set repeat to 1 to represent no-repeat and set to 0 to
represent repeat-forever?

>
>> +     if (!data->repeat)
>> +             data->is_indefinite = true;
>> +     else
>> +             data->is_indefinite = false;
>> +
>> +     err = pattern_trig_start_pattern(data, led_cdev);
>> +     if (err) {
>> +             mutex_unlock(&data->lock);
>> +             return err;
>> +     }
>> +
>> +     mutex_unlock(&data->lock);
>> +     return count;
>> +}
>> +
>> +static DEVICE_ATTR(repeat, 0644, pattern_trig_show_repeat,
>> +                pattern_trig_store_repeat);
>> +
>> +static ssize_t pattern_trig_show_pattern(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;
>> +     ssize_t count = 0;
>> +     int err, i;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     if (led_cdev->pattern_get) {
>> +             err = led_cdev->pattern_get(led_cdev, data->patterns,
>> +                                         &data->npatterns, &data->repeat);
>> +             if (err) {
>> +                     mutex_unlock(&data->lock);
>> +                     return err;
>> +             }
>> +     }
>
> Rather than sometimes calling pattern_get() here, how about defining
> that pattern_set() should update data->patterns before returning and
> just rely on data->patterns being right.
>
> (The reason for the pattern not being the same as passed in store would
> be to allow the driver to adjust the parameters to the match the
> hardware requirements and present this to the user)

Hmmm, so we can remove the 'pattern_get' interface and always trust
data->patterns.

>
>> +
>> +     if (!data->npatterns) {
>> +             mutex_unlock(&data->lock);
>> +             return -EINVAL;
>> +     }
>
> Rather than failing I think you should return the empty string if the
> pattern is empty.

Sure.

>
>> +
>> +     for (i = 0; i < data->npatterns; i++) {
>> +             count += scnprintf(buf + count, PAGE_SIZE - count,
>> +                                "%d %d" PATTERN_SEPARATOR,
>> +                                data->patterns[i].brightness,
>> +                                data->patterns[i].delta_t);
>> +     }
>> +
>> +     buf[count - 1] = '\n';
>> +     buf[count] = '\0';
>> +
>> +     mutex_unlock(&data->lock);
>> +     return count + 1;
>
> As you're returning a string I think you should return the number of
> chars in the string, not including the '\0'.

Sure.

>
>> +}
>> +
>> +static ssize_t pattern_trig_store_pattern(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, cr, ccount, tcr = 0;
>> +
>> +     del_timer_sync(&data->timer);
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     for (data->npatterns = 0; data->npatterns < MAX_PATTERNS;
>> +          data->npatterns++) {
>> +             /* Characters read on one conversion */
>> +             cr = 0;
>> +             /* Number of successful conversions */
>> +             ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n",
>> +                             &data->patterns[data->npatterns].brightness,
>> +                             &data->patterns[data->npatterns].delta_t, &cr);
>
> I'm not fond of the use of sscanf() here, but at least you should make
> sure that tcr doesn't go past PAGE_SIZE.

Yes.

>
>> +
>> +             /* Total characters read */
>> +             tcr += cr;
>> +             if (cr)
>> +                     continue;
>
> This loop is non-intuitive...
>
> You loop while npatterns is less than MAX_PATTERNS and the first half
> will loop if sscanf() matched the pattern otherwise you'll return from
> withing the loop.

OK. Will improve this loop here in next version.

>
>> +
>> +             /* Invalid syntax or end of pattern */
>> +             if (ccount == 2)
>
> I guess you'll get here if you match the two %d, but not the
> PATTERN_SEPARATOR? Wouldn't it be useful to fail here if ccount is !2.

Yes. OK, I will fail it if ccount is !2.

>
>> +                     data->npatterns++;
>> +
>> +             err = pattern_trig_start_pattern(data, led_cdev);
>> +             if (err) {
>> +                     mutex_unlock(&data->lock);
>> +                     return err;
>> +             }
>> +
>> +             mutex_unlock(&data->lock);
>> +             return count;
>> +     }
>> +
>> +     /* Shouldn't reach that */
>
> This means that you failed to set the pattern, so you probably shouldn't
> return count - but rather -EINVAL or something.

Right.

>
>> +     WARN(1, "MAX_NSTEP too small. Please report\n");
>> +     mutex_unlock(&data->lock);
>> +     return count;
>> +}
>> +
>> +static DEVICE_ATTR(pattern, 0644, pattern_trig_show_pattern,
>> +                pattern_trig_store_pattern);
>> +
>> +static int pattern_trig_create_sysfs_files(struct device *dev)
>> +{
>> +     int err;
>> +
>> +     err = device_create_file(dev, &dev_attr_repeat);
>> +     if (err)
>> +             return err;
>> +
>> +     err = device_create_file(dev, &dev_attr_pattern);
>> +     if (err)
>> +             device_remove_file(dev, &dev_attr_repeat);
>> +
>> +     return err;
>> +}
>> +
>> +static void pattern_trig_remove_sysfs_files(struct device *dev)
>> +{
>> +     device_remove_file(dev, &dev_attr_pattern);
>> +     device_remove_file(dev, &dev_attr_repeat);
>> +}
>> +
>> +static int pattern_trig_initialize_data(struct pattern_trig_data *data)
>
> These two functions would be better to just inline, but...

Will re-consider these two functions.

>
>> +{
>> +     data->patterns = kcalloc(MAX_PATTERNS, sizeof(struct led_pattern),
>> +                              GFP_KERNEL);
>
> ...as this always allocate 1024 entries of led_pattern associated with
> the pattern_trig_data you should just add 1024 entries of led_pattern to
> the pattern_trig_data struct.

Right.

>
>> +     if (!data->patterns)
>> +             return -ENOMEM;
>> +
>> +     data->is_indefinite = true;
>> +     mutex_init(&data->lock);
>> +
>> +     return 0;
>> +}
>> +
>> +static void pattern_trig_clear_data(struct pattern_trig_data *data)
>> +{
>> +     kfree(data->patterns);
>> +}
>> +
>> +static void pattern_trig_activate(struct led_classdev *led_cdev)
>> +{
>> +     struct pattern_trig_data *data;
>> +     int err;
>> +
>> +     data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +     if (!data)
>> +             return;
>> +
>> +     err = pattern_trig_initialize_data(data);
>> +     if (err) {
>> +             kfree(data);
>> +             return;
>> +     }
>> +
>> +     err = pattern_trig_create_sysfs_files(led_cdev->dev);
>> +     if (err) {
>> +             pattern_trig_clear_data(data);
>> +             kfree(data);
>> +             return;
>> +     }
>> +
>> +     data->led_cdev = led_cdev;
>> +     led_cdev->trigger_data = data;
>> +     timer_setup(&data->timer, pattern_trig_timer_function, 0);
>> +     led_cdev->activated = true;
>> +}
>> +
>> +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);
>
> This should only happen when not having using the driver based
> mechanism, so put this in an else to clarify this.

OK.

>
>> +     pattern_trig_remove_sysfs_files(led_cdev->dev);
>
> Inline this and one doesn't have to jump around to figure out what's
> going on.

Sure. Thanks for your useful comments.

>
>> +     led_set_brightness(led_cdev, LED_OFF);
>> +     pattern_trig_clear_data(data);
>> +     kfree(data);
>> +     led_cdev->trigger_data = NULL;
>> +     led_cdev->activated = false;
>> +}
>> +
>> +static struct led_trigger pattern_led_trigger = {
>> +     .name = "pattern",
>> +     .activate = pattern_trig_activate,
>> +     .deactivate = pattern_trig_deactivate,
>> +};
>> +
>> +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 b7e8255..bea02f0 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,14 @@ struct led_classdev {
>>                                    unsigned long *delay_on,
>>                                    unsigned long *delay_off);
>>
>> +     int (*pattern_set)(struct led_classdev *led_cdev,
>> +                        struct led_pattern *pattern, int len,
>> +                        unsigned repeat);
>> +     int (*pattern_get)(struct led_classdev *led_cdev,
>> +                        struct led_pattern *pattern, int *len,
>> +                        unsigned *repeat);
>> +     int (*pattern_clear)(struct led_classdev *led_cdev);
>> +
>>       struct device           *dev;
>>       const struct attribute_group    **groups;
>>
>> @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
>>       struct led_classdev *led_cdev, enum led_brightness brightness) { }
>>  #endif
>>
>> +/**
>> + * struct led_pattern - brightness value in a pattern
>> + * @delta_t: delay until next entry, in milliseconds
>> + * @brightness: brightness at time = 0
>> + */
>> +struct led_pattern {
>> +     int delta_t;
>> +     int brightness;
>> +};
>> +
>>  #endif               /* __LINUX_LEDS_H_INCLUDED */
>> --
>> 1.7.9.5
>>



-- 
Baolin Wang
Best Regards

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

[Baolin Wang]

On 31 July 2018 at 14:22, Baolin Wang <baolin.wang@linaro.org> wrote:
> Hi Bjorn,
>
> On 31 July 2018 at 11:54, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>> On Mon 30 Jul 05:29 PDT 2018, Baolin Wang wrote:
>>
>>> Some LED controllers have support for autonomously controlling
>>> brightness over time, according to some preprogrammed pattern or
>>> function.
>>>
>>> This patch adds pattern trigger that LED device can configure the
>>> pattern and trigger it.
>>>
>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>>  .../ABI/testing/sysfs-class-led-trigger-pattern    |   21 ++
>>>  drivers/leds/trigger/Kconfig                       |   10 +
>>>  drivers/leds/trigger/Makefile                      |    1 +
>>>  drivers/leds/trigger/ledtrig-pattern.c             |  349 ++++++++++++++++++++
>>>  include/linux/leds.h                               |   19 ++
>>>  5 files changed, 400 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..c52da34
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>> @@ -0,0 +1,21 @@
>>> +What:                /sys/class/leds/<led>/pattern
>>> +Date:                August 2018
>>> +KernelVersion:       4.19
>>> +Description:
>>> +             Specify a pattern for the LED, for LED hardware that support
>>> +             altering the brightness as a function of time.
>>> +
>>> +             The pattern is given by a series of tuples, of brightness and
>>> +             duration (ms). The LED is expected to traverse the series and
>>> +             each brightness value for the specified duration. Duration of
>>> +             0 means brightness should immediately change to new value.
>>> +
>>> +             The format of the pattern values should be:
>>> +             "brightness_1 duration_1, brightness_2 duration_2, brightness_3
>>> +             duration_3, ...".
>>> +
>>> +What:                /sys/class/leds/<led>/repeat
>>> +Date:                August 2018
>>> +KernelVersion:       4.19
>>> +Description:
>>> +             Specify a pattern repeat number. 0 means repeat indefinitely.
>>
>> If 0 means infinity, does 1 mean "repeat 1 time"? If so how would I
>
> Right.
>
>> specify that I want the pattern to run one time (i.e. 0 repetitions).
>
> If you set repeat to 1 to make the driver clear the  is_indefinite
> flag. So when the repeat number decreases to 0, it will stop.
>
>>
>>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>>> index a2559b4..a03afcd 100644
>>> --- a/drivers/leds/trigger/Kconfig
>>> +++ b/drivers/leds/trigger/Kconfig
>>> @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA
>>>         This enables direct flash/torch on/off by the driver, kernel space.
>>>         If unsure, say Y.
>>>
>>> +config LEDS_TRIGGER_PATTERN
>>> +       tristate "LED Pattern Trigger"
>>> +       depends on LEDS_TRIGGERS
>>> +       help
>>> +         This allows LEDs blinking with an arbitrary pattern. Can be useful
>>> +         on embedded systems with no screen to give out a status code to
>>> +         a human.
>>
>> While the pattern mechanism could be used to communicate some message
>> the use cases we've seen so far is all about enabling hardware to pulse
>> LEDs instead of blinking them...
>
> Right. I will improve the message here.
>
>>
>>> +
>>> +         If unsure, say N
>>> +
>>>  config LEDS_TRIGGER_PANIC
>>>       bool "LED Panic Trigger"
>>>       depends on LEDS_TRIGGERS
>>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
>>> index f3cfe19..c5d180e 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..b709aa1
>>> --- /dev/null
>>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>>> @@ -0,0 +1,349 @@
>>> +// 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.
>>
>> Might be a coincidence, but parts of this patch looks pretty close to
>> mine...
>>
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/timer.h>
>>> +
>>> +/*
>>> + * The "pattern" attribute contains at most PAGE_SIZE characters. Each line
>>> + * in this attribute is at least 4 characters long (a 1-digit number for the
>>> + * led brighntess, a space, a 1-digit number for the time, a semi-colon).
>>> + * Therefore, there is at most PAGE_SIZE/4 patterns.
>>> + */
>>
>> The brightness is a number between 0 and LED_FULL (or max_brightness)
>> and delta_t is measured in ms. So neither of these are likely to be a
>> single digit very often.
>
> OK. Will fix these comments.
>
>>
>>> +#define MAX_PATTERNS         (PAGE_SIZE / 4)
>>> +#define PATTERN_SEPARATOR    ","
>>> +
>>> +struct pattern_trig_data {
>>> +     struct led_classdev *led_cdev;
>>> +     struct led_pattern *patterns;
>>> +     struct led_pattern *curr;
>>> +     struct led_pattern *next;
>>> +     struct mutex lock;
>>> +     u32 npatterns;
>>> +     u32 repeat;
>>> +     bool is_indefinite;
>>> +     struct timer_list timer;
>>> +};
>>> +
>>> +static void pattern_trig_update_patterns(struct pattern_trig_data *data)
>>
>> I think you should just inline this logic into
>> pattern_trig_timer_function() to make it obvious that this is only
>> invoked from the timer function.
>>
>> And if you do this you can decrement data->repeat unconditionally after
>> the check for !is_indefinite && repeat == 0.
>
> OK.

We can not remove the condition, since we still should make sure the
data->curr points the fist pattern again.

>
>>
>>> +{
>>> +     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++;
>>
>> How about replacing this conditional with:
>>
>>         data->next = (data->next + 1) % data->npatterns;
>
> Good suggestion. Will do in next version.

Re-think about this. The data->next is one pointer which points next
pattern not one index. So I think I can not do like this.

>
>>
>>> +}
>>> +
>>> +static void pattern_trig_timer_function(struct timer_list *t)
>>> +{
>>> +     struct pattern_trig_data *data = from_timer(data, t, timer);
>>> +
>>> +     mutex_lock(&data->lock);
>>> +
>>> +     if (!data->is_indefinite && !data->repeat) {
>>> +             mutex_unlock(&data->lock);
>>> +             return;
>>> +     }
>>> +
>>> +     led_set_brightness(data->led_cdev, data->curr->brightness);
>>> +     mod_timer(&data->timer, jiffies + msecs_to_jiffies(data->curr->delta_t));
>>> +     pattern_trig_update_patterns(data);
>>> +
>>> +     mutex_unlock(&data->lock);
>>> +}
>>> +
>>> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
>>> +                                    struct led_classdev *led_cdev)
>>> +{
>>> +     if (!data->npatterns)
>>> +             return 0;
>>> +
>>> +     if (led_cdev->pattern_clear)
>>> +             led_cdev->pattern_clear(led_cdev);
>>> +
>>> +     if (led_cdev->pattern_set) {
>>
>> In my proposal setting an empty pattern would invoke pattern_clear() and
>> setting a pattern would invoke pattern_set(), this allows the driver to
>> fail the pattern_set() without clearing the old pattern.
>
> Pavel wanted "echo none > trigger" to clear the pattern. So I am fine
> remove the 'pattern_clear' operation in this function.
>
>>
>>> +             return led_cdev->pattern_set(led_cdev, data->patterns,
>>> +                                          data->npatterns, data->repeat);
>>
>> repeat is communicated to the driver, but is_indefinite is not. At least
>> in my driver the latter

Sorry I missed this comments. I think the repeat number can be easily
converted to decide if is indefinite in your driver.

>>
>>> +     }
>>
>> I think it would be useful to put the rest in an else statement, to
>> clarify that the patterns is either implemented by the driver or by the
>> software timer. It might also make sense to group the check for
>> pattern_clear() and pattern_set(), as it wouldn't make sense for the
>> driver to implement only one of these.
>
> But checkpatch will complain the 'else' statement is redundant, maybe
> add some comments here to make things clear?
>
>>
>>> +
>>> +     data->curr = data->patterns;
>>> +     data->next = data->patterns + 1;
>>> +     data->timer.expires = jiffies;
>>> +     add_timer(&data->timer);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static ssize_t pattern_trig_show_repeat(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 err;
>>> +     u32 repeat;
>>> +
>>> +     mutex_lock(&data->lock);
>>> +
>>> +     if (led_cdev->pattern_get) {
>>> +             err = led_cdev->pattern_get(led_cdev, data->patterns,
>>> +                                         &data->npatterns, &data->repeat);
>>
>> I don't see why "repeat" would have changed since you told the driver,
>> so data->repeat should have the right value already.
>
> Some drivers don't care about the repeat, and it will always repeat
> indefinitely. So no matter what values set from userspace, driver can
> set it to 0.
>
>>
>>> +             if (err) {
>>> +                     mutex_unlock(&data->lock);
>>> +                     return err;
>>> +             }
>>> +     }
>>> +
>>> +     repeat = data->repeat;
>>> +     mutex_unlock(&data->lock);
>>> +
>>> +     return scnprintf(buf, PAGE_SIZE, "%u\n", repeat);
>>> +}
>>> +
>>> +static ssize_t pattern_trig_store_repeat(struct device *dev,
>>> +                                      struct device_attribute *attr,
>>> +                                      const char *buf, size_t count)
>>> +{
>>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> +     struct pattern_trig_data *data = led_cdev->trigger_data;
>>> +     unsigned long res;
>>> +     int err;
>>> +
>>> +     err = kstrtoul(buf, 10, &res);
>>> +     if (err)
>>> +             return err;
>>> +
>>> +     del_timer_sync(&data->timer);
>>> +
>>> +     mutex_lock(&data->lock);
>>> +     data->repeat = res;
>>> +
>>> +     /* 0 means repeat indefinitely */
>>
>> I think that it would be better to specify indefinite by writing e.g.
>> "Inf" to the repeat.
>>
>> Also note that it's possible that there might be hardware restrictions
>> that only allows specific combinations of these; e.g. the Qualcomm
>> hardware can only do no-repeat or repeat-forever.
>
> Can you set repeat to 1 to represent no-repeat and set to 0 to
> represent repeat-forever?
>
>>
>>> +     if (!data->repeat)
>>> +             data->is_indefinite = true;
>>> +     else
>>> +             data->is_indefinite = false;
>>> +
>>> +     err = pattern_trig_start_pattern(data, led_cdev);
>>> +     if (err) {
>>> +             mutex_unlock(&data->lock);
>>> +             return err;
>>> +     }
>>> +
>>> +     mutex_unlock(&data->lock);
>>> +     return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR(repeat, 0644, pattern_trig_show_repeat,
>>> +                pattern_trig_store_repeat);
>>> +
>>> +static ssize_t pattern_trig_show_pattern(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;
>>> +     ssize_t count = 0;
>>> +     int err, i;
>>> +
>>> +     mutex_lock(&data->lock);
>>> +
>>> +     if (led_cdev->pattern_get) {
>>> +             err = led_cdev->pattern_get(led_cdev, data->patterns,
>>> +                                         &data->npatterns, &data->repeat);
>>> +             if (err) {
>>> +                     mutex_unlock(&data->lock);
>>> +                     return err;
>>> +             }
>>> +     }
>>
>> Rather than sometimes calling pattern_get() here, how about defining
>> that pattern_set() should update data->patterns before returning and
>> just rely on data->patterns being right.
>>
>> (The reason for the pattern not being the same as passed in store would
>> be to allow the driver to adjust the parameters to the match the
>> hardware requirements and present this to the user)
>
> Hmmm, so we can remove the 'pattern_get' interface and always trust
> data->patterns.
>
>>
>>> +
>>> +     if (!data->npatterns) {
>>> +             mutex_unlock(&data->lock);
>>> +             return -EINVAL;
>>> +     }
>>
>> Rather than failing I think you should return the empty string if the
>> pattern is empty.
>
> Sure.
>
>>
>>> +
>>> +     for (i = 0; i < data->npatterns; i++) {
>>> +             count += scnprintf(buf + count, PAGE_SIZE - count,
>>> +                                "%d %d" PATTERN_SEPARATOR,
>>> +                                data->patterns[i].brightness,
>>> +                                data->patterns[i].delta_t);
>>> +     }
>>> +
>>> +     buf[count - 1] = '\n';
>>> +     buf[count] = '\0';
>>> +
>>> +     mutex_unlock(&data->lock);
>>> +     return count + 1;
>>
>> As you're returning a string I think you should return the number of
>> chars in the string, not including the '\0'.
>
> Sure.
>
>>
>>> +}
>>> +
>>> +static ssize_t pattern_trig_store_pattern(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, cr, ccount, tcr = 0;
>>> +
>>> +     del_timer_sync(&data->timer);
>>> +
>>> +     mutex_lock(&data->lock);
>>> +
>>> +     for (data->npatterns = 0; data->npatterns < MAX_PATTERNS;
>>> +          data->npatterns++) {
>>> +             /* Characters read on one conversion */
>>> +             cr = 0;
>>> +             /* Number of successful conversions */
>>> +             ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n",
>>> +                             &data->patterns[data->npatterns].brightness,
>>> +                             &data->patterns[data->npatterns].delta_t, &cr);
>>
>> I'm not fond of the use of sscanf() here, but at least you should make
>> sure that tcr doesn't go past PAGE_SIZE.
>
> Yes.
>
>>
>>> +
>>> +             /* Total characters read */
>>> +             tcr += cr;
>>> +             if (cr)
>>> +                     continue;
>>
>> This loop is non-intuitive...
>>
>> You loop while npatterns is less than MAX_PATTERNS and the first half
>> will loop if sscanf() matched the pattern otherwise you'll return from
>> withing the loop.
>
> OK. Will improve this loop here in next version.
>
>>
>>> +
>>> +             /* Invalid syntax or end of pattern */
>>> +             if (ccount == 2)
>>
>> I guess you'll get here if you match the two %d, but not the
>> PATTERN_SEPARATOR? Wouldn't it be useful to fail here if ccount is !2.
>
> Yes. OK, I will fail it if ccount is !2.
>
>>
>>> +                     data->npatterns++;
>>> +
>>> +             err = pattern_trig_start_pattern(data, led_cdev);
>>> +             if (err) {
>>> +                     mutex_unlock(&data->lock);
>>> +                     return err;
>>> +             }
>>> +
>>> +             mutex_unlock(&data->lock);
>>> +             return count;
>>> +     }
>>> +
>>> +     /* Shouldn't reach that */
>>
>> This means that you failed to set the pattern, so you probably shouldn't
>> return count - but rather -EINVAL or something.
>
> Right.
>
>>
>>> +     WARN(1, "MAX_NSTEP too small. Please report\n");
>>> +     mutex_unlock(&data->lock);
>>> +     return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR(pattern, 0644, pattern_trig_show_pattern,
>>> +                pattern_trig_store_pattern);
>>> +
>>> +static int pattern_trig_create_sysfs_files(struct device *dev)
>>> +{
>>> +     int err;
>>> +
>>> +     err = device_create_file(dev, &dev_attr_repeat);
>>> +     if (err)
>>> +             return err;
>>> +
>>> +     err = device_create_file(dev, &dev_attr_pattern);
>>> +     if (err)
>>> +             device_remove_file(dev, &dev_attr_repeat);
>>> +
>>> +     return err;
>>> +}
>>> +
>>> +static void pattern_trig_remove_sysfs_files(struct device *dev)
>>> +{
>>> +     device_remove_file(dev, &dev_attr_pattern);
>>> +     device_remove_file(dev, &dev_attr_repeat);
>>> +}
>>> +
>>> +static int pattern_trig_initialize_data(struct pattern_trig_data *data)
>>
>> These two functions would be better to just inline, but...
>
> Will re-consider these two functions.
>
>>
>>> +{
>>> +     data->patterns = kcalloc(MAX_PATTERNS, sizeof(struct led_pattern),
>>> +                              GFP_KERNEL);
>>
>> ...as this always allocate 1024 entries of led_pattern associated with
>> the pattern_trig_data you should just add 1024 entries of led_pattern to
>> the pattern_trig_data struct.
>
> Right.
>
>>
>>> +     if (!data->patterns)
>>> +             return -ENOMEM;
>>> +
>>> +     data->is_indefinite = true;
>>> +     mutex_init(&data->lock);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static void pattern_trig_clear_data(struct pattern_trig_data *data)
>>> +{
>>> +     kfree(data->patterns);
>>> +}
>>> +
>>> +static void pattern_trig_activate(struct led_classdev *led_cdev)
>>> +{
>>> +     struct pattern_trig_data *data;
>>> +     int err;
>>> +
>>> +     data = kzalloc(sizeof(*data), GFP_KERNEL);
>>> +     if (!data)
>>> +             return;
>>> +
>>> +     err = pattern_trig_initialize_data(data);
>>> +     if (err) {
>>> +             kfree(data);
>>> +             return;
>>> +     }
>>> +
>>> +     err = pattern_trig_create_sysfs_files(led_cdev->dev);
>>> +     if (err) {
>>> +             pattern_trig_clear_data(data);
>>> +             kfree(data);
>>> +             return;
>>> +     }
>>> +
>>> +     data->led_cdev = led_cdev;
>>> +     led_cdev->trigger_data = data;
>>> +     timer_setup(&data->timer, pattern_trig_timer_function, 0);
>>> +     led_cdev->activated = true;
>>> +}
>>> +
>>> +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);
>>
>> This should only happen when not having using the driver based
>> mechanism, so put this in an else to clarify this.
>
> OK.
>
>>
>>> +     pattern_trig_remove_sysfs_files(led_cdev->dev);
>>
>> Inline this and one doesn't have to jump around to figure out what's
>> going on.
>
> Sure. Thanks for your useful comments.
>
>>
>>> +     led_set_brightness(led_cdev, LED_OFF);
>>> +     pattern_trig_clear_data(data);
>>> +     kfree(data);
>>> +     led_cdev->trigger_data = NULL;
>>> +     led_cdev->activated = false;
>>> +}
>>> +
>>> +static struct led_trigger pattern_led_trigger = {
>>> +     .name = "pattern",
>>> +     .activate = pattern_trig_activate,
>>> +     .deactivate = pattern_trig_deactivate,
>>> +};
>>> +
>>> +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 b7e8255..bea02f0 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,14 @@ struct led_classdev {
>>>                                    unsigned long *delay_on,
>>>                                    unsigned long *delay_off);
>>>
>>> +     int (*pattern_set)(struct led_classdev *led_cdev,
>>> +                        struct led_pattern *pattern, int len,
>>> +                        unsigned repeat);
>>> +     int (*pattern_get)(struct led_classdev *led_cdev,
>>> +                        struct led_pattern *pattern, int *len,
>>> +                        unsigned *repeat);
>>> +     int (*pattern_clear)(struct led_classdev *led_cdev);
>>> +
>>>       struct device           *dev;
>>>       const struct attribute_group    **groups;
>>>
>>> @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
>>>       struct led_classdev *led_cdev, enum led_brightness brightness) { }
>>>  #endif
>>>
>>> +/**
>>> + * struct led_pattern - brightness value in a pattern
>>> + * @delta_t: delay until next entry, in milliseconds
>>> + * @brightness: brightness at time = 0
>>> + */
>>> +struct led_pattern {
>>> +     int delta_t;
>>> +     int brightness;
>>> +};
>>> +
>>>  #endif               /* __LINUX_LEDS_H_INCLUDED */
>>> --
>>> 1.7.9.5
>>>
>
>
>
> --
> Baolin Wang
> Best Regards



-- 
Baolin Wang
Best Regards

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

[Jacek Anaszewski]

Hi Baolin,

Thank you for the patch set. I have one note regarding the way how
trigger specific sysfs attributes are created/removed, please refer
to my comment in the code below.

On 07/30/2018 02:29 PM, Baolin Wang wrote:
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
> 
> This patch adds pattern trigger that LED device can configure the
> pattern and trigger it.
> 
> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  .../ABI/testing/sysfs-class-led-trigger-pattern    |   21 ++
>  drivers/leds/trigger/Kconfig                       |   10 +
>  drivers/leds/trigger/Makefile                      |    1 +
>  drivers/leds/trigger/ledtrig-pattern.c             |  349 ++++++++++++++++++++
>  include/linux/leds.h                               |   19 ++
>  5 files changed, 400 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..c52da34
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> @@ -0,0 +1,21 @@
> +What:		/sys/class/leds/<led>/pattern
> +Date:		August 2018
> +KernelVersion:	4.19
> +Description:
> +		Specify a pattern for the LED, for LED hardware that support
> +		altering the brightness as a function of time.
> +
> +		The pattern is given by a series of tuples, of brightness and
> +		duration (ms). The LED is expected to traverse the series and
> +		each brightness value for the specified duration. Duration of
> +		0 means brightness should immediately change to new value.
> +
> +		The format of the pattern values should be:
> +		"brightness_1 duration_1, brightness_2 duration_2, brightness_3
> +		duration_3, ...".
> +
> +What:		/sys/class/leds/<led>/repeat
> +Date:		August 2018
> +KernelVersion:	4.19
> +Description:
> +		Specify a pattern repeat number. 0 means repeat indefinitely.
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index a2559b4..a03afcd 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA
>  	  This enables direct flash/torch on/off by the driver, kernel space.
>  	  If unsure, say Y.
>  
> +config LEDS_TRIGGER_PATTERN
> +       tristate "LED Pattern Trigger"
> +       depends on LEDS_TRIGGERS
> +       help
> +         This allows LEDs blinking with an arbitrary pattern. Can be useful
> +         on embedded systems with no screen to give out a status code to
> +         a human.
> +
> +         If unsure, say N
> +
>  config LEDS_TRIGGER_PANIC
>  	bool "LED Panic Trigger"
>  	depends on LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index f3cfe19..c5d180e 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..b709aa1
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,349 @@
> +// 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>
> +
> +/*
> + * The "pattern" attribute contains at most PAGE_SIZE characters. Each line
> + * in this attribute is at least 4 characters long (a 1-digit number for the
> + * led brighntess, a space, a 1-digit number for the time, a semi-colon).
> + * Therefore, there is at most PAGE_SIZE/4 patterns.
> + */
> +#define MAX_PATTERNS		(PAGE_SIZE / 4)
> +#define PATTERN_SEPARATOR	","
> +
> +struct pattern_trig_data {
> +	struct led_classdev *led_cdev;
> +	struct led_pattern *patterns;
> +	struct led_pattern *curr;
> +	struct led_pattern *next;
> +	struct mutex lock;
> +	u32 npatterns;
> +	u32 repeat;
> +	bool is_indefinite;
> +	struct timer_list timer;
> +};
> +
> +static void 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++;
> +}
> +
> +static void pattern_trig_timer_function(struct timer_list *t)
> +{
> +	struct pattern_trig_data *data = from_timer(data, t, timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	if (!data->is_indefinite && !data->repeat) {
> +		mutex_unlock(&data->lock);
> +		return;
> +	}
> +
> +	led_set_brightness(data->led_cdev, data->curr->brightness);
> +	mod_timer(&data->timer, jiffies + msecs_to_jiffies(data->curr->delta_t));
> +	pattern_trig_update_patterns(data);
> +
> +	mutex_unlock(&data->lock);
> +}
> +
> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
> +				       struct led_classdev *led_cdev)
> +{
> +	if (!data->npatterns)
> +		return 0;
> +
> +	if (led_cdev->pattern_clear)
> +		led_cdev->pattern_clear(led_cdev);
> +
> +	if (led_cdev->pattern_set) {
> +		return led_cdev->pattern_set(led_cdev, data->patterns,
> +					     data->npatterns, data->repeat);
> +	}
> +
> +	data->curr = data->patterns;
> +	data->next = data->patterns + 1;
> +	data->timer.expires = jiffies;
> +	add_timer(&data->timer);
> +
> +	return 0;
> +}
> +
> +static ssize_t pattern_trig_show_repeat(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 err;
> +	u32 repeat;
> +
> +	mutex_lock(&data->lock);
> +
> +	if (led_cdev->pattern_get) {
> +		err = led_cdev->pattern_get(led_cdev, data->patterns,
> +					    &data->npatterns, &data->repeat);
> +		if (err) {
> +			mutex_unlock(&data->lock);
> +			return err;
> +		}
> +	}
> +
> +	repeat = data->repeat;
> +	mutex_unlock(&data->lock);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%u\n", repeat);
> +}
> +
> +static ssize_t pattern_trig_store_repeat(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct pattern_trig_data *data = led_cdev->trigger_data;
> +	unsigned long res;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &res);
> +	if (err)
> +		return err;
> +
> +	del_timer_sync(&data->timer);
> +
> +	mutex_lock(&data->lock);
> +	data->repeat = res;
> +
> +	/* 0 means repeat indefinitely */
> +	if (!data->repeat)
> +		data->is_indefinite = true;
> +	else
> +		data->is_indefinite = false;
> +
> +	err = pattern_trig_start_pattern(data, led_cdev);
> +	if (err) {
> +		mutex_unlock(&data->lock);
> +		return err;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(repeat, 0644, pattern_trig_show_repeat,
> +		   pattern_trig_store_repeat);
> +
> +static ssize_t pattern_trig_show_pattern(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;
> +	ssize_t count = 0;
> +	int err, i;
> +
> +	mutex_lock(&data->lock);
> +
> +	if (led_cdev->pattern_get) {
> +		err = led_cdev->pattern_get(led_cdev, data->patterns,
> +					    &data->npatterns, &data->repeat);
> +		if (err) {
> +			mutex_unlock(&data->lock);
> +			return err;
> +		}
> +	}
> +
> +	if (!data->npatterns) {
> +		mutex_unlock(&data->lock);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < data->npatterns; i++) {
> +		count += scnprintf(buf + count, PAGE_SIZE - count,
> +				   "%d %d" PATTERN_SEPARATOR,
> +				   data->patterns[i].brightness,
> +				   data->patterns[i].delta_t);
> +	}
> +
> +	buf[count - 1] = '\n';
> +	buf[count] = '\0';
> +
> +	mutex_unlock(&data->lock);
> +	return count + 1;
> +}
> +
> +static ssize_t pattern_trig_store_pattern(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, cr, ccount, tcr = 0;
> +
> +	del_timer_sync(&data->timer);
> +
> +	mutex_lock(&data->lock);
> +
> +	for (data->npatterns = 0; data->npatterns < MAX_PATTERNS;
> +	     data->npatterns++) {
> +		/* Characters read on one conversion */
> +		cr = 0;
> +		/* Number of successful conversions */
> +		ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n",
> +				&data->patterns[data->npatterns].brightness,
> +				&data->patterns[data->npatterns].delta_t, &cr);
> +
> +		/* Total characters read */
> +		tcr += cr;
> +		if (cr)
> +			continue;
> +
> +		/* Invalid syntax or end of pattern */
> +		if (ccount == 2)
> +			data->npatterns++;
> +
> +		err = pattern_trig_start_pattern(data, led_cdev);
> +		if (err) {
> +			mutex_unlock(&data->lock);
> +			return err;
> +		}
> +
> +		mutex_unlock(&data->lock);
> +		return count;
> +	}
> +
> +	/* Shouldn't reach that */
> +	WARN(1, "MAX_NSTEP too small. Please report\n");
> +	mutex_unlock(&data->lock);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(pattern, 0644, pattern_trig_show_pattern,
> +		   pattern_trig_store_pattern);
> +
> +static int pattern_trig_create_sysfs_files(struct device *dev)
> +{
> +	int err;
> +
> +	err = device_create_file(dev, &dev_attr_repeat);
> +	if (err)
> +		return err;
> +
> +	err = device_create_file(dev, &dev_attr_pattern);
> +	if (err)
> +		device_remove_file(dev, &dev_attr_repeat);

We've recently merged patch set of Uwe Kleine-König that adds
groups attribute to the struct led_trigger, and updates all
triggers to use it. Please follow the new approach - refer to
linux-next or directly to
https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/log/?h=for-next.

> +	return err;
> +}
> +
> +static void pattern_trig_remove_sysfs_files(struct device *dev)
> +{
> +	device_remove_file(dev, &dev_attr_pattern);
> +	device_remove_file(dev, &dev_attr_repeat);
> +}
> +
> +static int pattern_trig_initialize_data(struct pattern_trig_data *data)
> +{
> +	data->patterns = kcalloc(MAX_PATTERNS, sizeof(struct led_pattern),
> +				 GFP_KERNEL);
> +	if (!data->patterns)
> +		return -ENOMEM;
> +
> +	data->is_indefinite = true;
> +	mutex_init(&data->lock);
> +
> +	return 0;
> +}
> +
> +static void pattern_trig_clear_data(struct pattern_trig_data *data)
> +{
> +	kfree(data->patterns);
> +}
> +
> +static void pattern_trig_activate(struct led_classdev *led_cdev)
> +{
> +	struct pattern_trig_data *data;
> +	int err;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return;
> +
> +	err = pattern_trig_initialize_data(data);
> +	if (err) {
> +		kfree(data);
> +		return;
> +	}
> +
> +	err = pattern_trig_create_sysfs_files(led_cdev->dev);
> +	if (err) {
> +		pattern_trig_clear_data(data);
> +		kfree(data);
> +		return;
> +	}
> +
> +	data->led_cdev = led_cdev;
> +	led_cdev->trigger_data = data;
> +	timer_setup(&data->timer, pattern_trig_timer_function, 0);
> +	led_cdev->activated = true;
> +}
> +
> +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);
> +	pattern_trig_remove_sysfs_files(led_cdev->dev);
> +	led_set_brightness(led_cdev, LED_OFF);
> +	pattern_trig_clear_data(data);
> +	kfree(data);
> +	led_cdev->trigger_data = NULL;
> +	led_cdev->activated = false;
> +}
> +
> +static struct led_trigger pattern_led_trigger = {
> +	.name = "pattern",
> +	.activate = pattern_trig_activate,
> +	.deactivate = pattern_trig_deactivate,
> +};
> +
> +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 b7e8255..bea02f0 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,14 @@ struct led_classdev {
>  				     unsigned long *delay_on,
>  				     unsigned long *delay_off);
>  
> +	int (*pattern_set)(struct led_classdev *led_cdev,
> +			   struct led_pattern *pattern, int len,
> +			   unsigned repeat);
> +	int (*pattern_get)(struct led_classdev *led_cdev,
> +			   struct led_pattern *pattern, int *len,
> +			   unsigned *repeat);
> +	int (*pattern_clear)(struct led_classdev *led_cdev);
> +
>  	struct device		*dev;
>  	const struct attribute_group	**groups;
>  
> @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
>  	struct led_classdev *led_cdev, enum led_brightness brightness) { }
>  #endif
>  
> +/**
> + * struct led_pattern - brightness value in a pattern
> + * @delta_t: delay until next entry, in milliseconds
> + * @brightness: brightness at time = 0
> + */
> +struct led_pattern {
> +	int delta_t;
> +	int brightness;
> +};
> +
>  #endif		/* __LINUX_LEDS_H_INCLUDED */
> 

-- 
Best regards,
Jacek Anaszewski

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

[Baolin Wang]

Hi Jacek,

On 31 July 2018 at 18:53, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> Hi Baolin,
>
> Thank you for the patch set. I have one note regarding the way how
> trigger specific sysfs attributes are created/removed, please refer
> to my comment in the code below.
>
> On 07/30/2018 02:29 PM, Baolin Wang wrote:
>> Some LED controllers have support for autonomously controlling
>> brightness over time, according to some preprogrammed pattern or
>> function.
>>
>> This patch adds pattern trigger that LED device can configure the
>> pattern and trigger it.
>>
>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  .../ABI/testing/sysfs-class-led-trigger-pattern    |   21 ++
>>  drivers/leds/trigger/Kconfig                       |   10 +
>>  drivers/leds/trigger/Makefile                      |    1 +
>>  drivers/leds/trigger/ledtrig-pattern.c             |  349 ++++++++++++++++++++
>>  include/linux/leds.h                               |   19 ++
>>  5 files changed, 400 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..c52da34
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> @@ -0,0 +1,21 @@
>> +What:                /sys/class/leds/<led>/pattern
>> +Date:                August 2018
>> +KernelVersion:       4.19
>> +Description:
>> +             Specify a pattern for the LED, for LED hardware that support
>> +             altering the brightness as a function of time.
>> +
>> +             The pattern is given by a series of tuples, of brightness and
>> +             duration (ms). The LED is expected to traverse the series and
>> +             each brightness value for the specified duration. Duration of
>> +             0 means brightness should immediately change to new value.
>> +
>> +             The format of the pattern values should be:
>> +             "brightness_1 duration_1, brightness_2 duration_2, brightness_3
>> +             duration_3, ...".
>> +
>> +What:                /sys/class/leds/<led>/repeat
>> +Date:                August 2018
>> +KernelVersion:       4.19
>> +Description:
>> +             Specify a pattern repeat number. 0 means repeat indefinitely.
>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>> index a2559b4..a03afcd 100644
>> --- a/drivers/leds/trigger/Kconfig
>> +++ b/drivers/leds/trigger/Kconfig
>> @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA
>>         This enables direct flash/torch on/off by the driver, kernel space.
>>         If unsure, say Y.
>>
>> +config LEDS_TRIGGER_PATTERN
>> +       tristate "LED Pattern Trigger"
>> +       depends on LEDS_TRIGGERS
>> +       help
>> +         This allows LEDs blinking with an arbitrary pattern. Can be useful
>> +         on embedded systems with no screen to give out a status code to
>> +         a human.
>> +
>> +         If unsure, say N
>> +
>>  config LEDS_TRIGGER_PANIC
>>       bool "LED Panic Trigger"
>>       depends on LEDS_TRIGGERS
>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
>> index f3cfe19..c5d180e 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..b709aa1
>> --- /dev/null
>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>> @@ -0,0 +1,349 @@
>> +// 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>
>> +
>> +/*
>> + * The "pattern" attribute contains at most PAGE_SIZE characters. Each line
>> + * in this attribute is at least 4 characters long (a 1-digit number for the
>> + * led brighntess, a space, a 1-digit number for the time, a semi-colon).
>> + * Therefore, there is at most PAGE_SIZE/4 patterns.
>> + */
>> +#define MAX_PATTERNS         (PAGE_SIZE / 4)
>> +#define PATTERN_SEPARATOR    ","
>> +
>> +struct pattern_trig_data {
>> +     struct led_classdev *led_cdev;
>> +     struct led_pattern *patterns;
>> +     struct led_pattern *curr;
>> +     struct led_pattern *next;
>> +     struct mutex lock;
>> +     u32 npatterns;
>> +     u32 repeat;
>> +     bool is_indefinite;
>> +     struct timer_list timer;
>> +};
>> +
>> +static void 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++;
>> +}
>> +
>> +static void pattern_trig_timer_function(struct timer_list *t)
>> +{
>> +     struct pattern_trig_data *data = from_timer(data, t, timer);
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     if (!data->is_indefinite && !data->repeat) {
>> +             mutex_unlock(&data->lock);
>> +             return;
>> +     }
>> +
>> +     led_set_brightness(data->led_cdev, data->curr->brightness);
>> +     mod_timer(&data->timer, jiffies + msecs_to_jiffies(data->curr->delta_t));
>> +     pattern_trig_update_patterns(data);
>> +
>> +     mutex_unlock(&data->lock);
>> +}
>> +
>> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
>> +                                    struct led_classdev *led_cdev)
>> +{
>> +     if (!data->npatterns)
>> +             return 0;
>> +
>> +     if (led_cdev->pattern_clear)
>> +             led_cdev->pattern_clear(led_cdev);
>> +
>> +     if (led_cdev->pattern_set) {
>> +             return led_cdev->pattern_set(led_cdev, data->patterns,
>> +                                          data->npatterns, data->repeat);
>> +     }
>> +
>> +     data->curr = data->patterns;
>> +     data->next = data->patterns + 1;
>> +     data->timer.expires = jiffies;
>> +     add_timer(&data->timer);
>> +
>> +     return 0;
>> +}
>> +
>> +static ssize_t pattern_trig_show_repeat(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 err;
>> +     u32 repeat;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     if (led_cdev->pattern_get) {
>> +             err = led_cdev->pattern_get(led_cdev, data->patterns,
>> +                                         &data->npatterns, &data->repeat);
>> +             if (err) {
>> +                     mutex_unlock(&data->lock);
>> +                     return err;
>> +             }
>> +     }
>> +
>> +     repeat = data->repeat;
>> +     mutex_unlock(&data->lock);
>> +
>> +     return scnprintf(buf, PAGE_SIZE, "%u\n", repeat);
>> +}
>> +
>> +static ssize_t pattern_trig_store_repeat(struct device *dev,
>> +                                      struct device_attribute *attr,
>> +                                      const char *buf, size_t count)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +     struct pattern_trig_data *data = led_cdev->trigger_data;
>> +     unsigned long res;
>> +     int err;
>> +
>> +     err = kstrtoul(buf, 10, &res);
>> +     if (err)
>> +             return err;
>> +
>> +     del_timer_sync(&data->timer);
>> +
>> +     mutex_lock(&data->lock);
>> +     data->repeat = res;
>> +
>> +     /* 0 means repeat indefinitely */
>> +     if (!data->repeat)
>> +             data->is_indefinite = true;
>> +     else
>> +             data->is_indefinite = false;
>> +
>> +     err = pattern_trig_start_pattern(data, led_cdev);
>> +     if (err) {
>> +             mutex_unlock(&data->lock);
>> +             return err;
>> +     }
>> +
>> +     mutex_unlock(&data->lock);
>> +     return count;
>> +}
>> +
>> +static DEVICE_ATTR(repeat, 0644, pattern_trig_show_repeat,
>> +                pattern_trig_store_repeat);
>> +
>> +static ssize_t pattern_trig_show_pattern(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;
>> +     ssize_t count = 0;
>> +     int err, i;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     if (led_cdev->pattern_get) {
>> +             err = led_cdev->pattern_get(led_cdev, data->patterns,
>> +                                         &data->npatterns, &data->repeat);
>> +             if (err) {
>> +                     mutex_unlock(&data->lock);
>> +                     return err;
>> +             }
>> +     }
>> +
>> +     if (!data->npatterns) {
>> +             mutex_unlock(&data->lock);
>> +             return -EINVAL;
>> +     }
>> +
>> +     for (i = 0; i < data->npatterns; i++) {
>> +             count += scnprintf(buf + count, PAGE_SIZE - count,
>> +                                "%d %d" PATTERN_SEPARATOR,
>> +                                data->patterns[i].brightness,
>> +                                data->patterns[i].delta_t);
>> +     }
>> +
>> +     buf[count - 1] = '\n';
>> +     buf[count] = '\0';
>> +
>> +     mutex_unlock(&data->lock);
>> +     return count + 1;
>> +}
>> +
>> +static ssize_t pattern_trig_store_pattern(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, cr, ccount, tcr = 0;
>> +
>> +     del_timer_sync(&data->timer);
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     for (data->npatterns = 0; data->npatterns < MAX_PATTERNS;
>> +          data->npatterns++) {
>> +             /* Characters read on one conversion */
>> +             cr = 0;
>> +             /* Number of successful conversions */
>> +             ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n",
>> +                             &data->patterns[data->npatterns].brightness,
>> +                             &data->patterns[data->npatterns].delta_t, &cr);
>> +
>> +             /* Total characters read */
>> +             tcr += cr;
>> +             if (cr)
>> +                     continue;
>> +
>> +             /* Invalid syntax or end of pattern */
>> +             if (ccount == 2)
>> +                     data->npatterns++;
>> +
>> +             err = pattern_trig_start_pattern(data, led_cdev);
>> +             if (err) {
>> +                     mutex_unlock(&data->lock);
>> +                     return err;
>> +             }
>> +
>> +             mutex_unlock(&data->lock);
>> +             return count;
>> +     }
>> +
>> +     /* Shouldn't reach that */
>> +     WARN(1, "MAX_NSTEP too small. Please report\n");
>> +     mutex_unlock(&data->lock);
>> +     return count;
>> +}
>> +
>> +static DEVICE_ATTR(pattern, 0644, pattern_trig_show_pattern,
>> +                pattern_trig_store_pattern);
>> +
>> +static int pattern_trig_create_sysfs_files(struct device *dev)
>> +{
>> +     int err;
>> +
>> +     err = device_create_file(dev, &dev_attr_repeat);
>> +     if (err)
>> +             return err;
>> +
>> +     err = device_create_file(dev, &dev_attr_pattern);
>> +     if (err)
>> +             device_remove_file(dev, &dev_attr_repeat);
>
> We've recently merged patch set of Uwe Kleine-König that adds
> groups attribute to the struct led_trigger, and updates all
> triggers to use it. Please follow the new approach - refer to
> linux-next or directly to
> https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/log/?h=for-next.

Ah, Thanks to point out these updates which I missed. I will follow
them in next version.

>
>> +     return err;
>> +}
>> +
>> +static void pattern_trig_remove_sysfs_files(struct device *dev)
>> +{
>> +     device_remove_file(dev, &dev_attr_pattern);
>> +     device_remove_file(dev, &dev_attr_repeat);
>> +}
>> +
>> +static int pattern_trig_initialize_data(struct pattern_trig_data *data)
>> +{
>> +     data->patterns = kcalloc(MAX_PATTERNS, sizeof(struct led_pattern),
>> +                              GFP_KERNEL);
>> +     if (!data->patterns)
>> +             return -ENOMEM;
>> +
>> +     data->is_indefinite = true;
>> +     mutex_init(&data->lock);
>> +
>> +     return 0;
>> +}
>> +
>> +static void pattern_trig_clear_data(struct pattern_trig_data *data)
>> +{
>> +     kfree(data->patterns);
>> +}
>> +
>> +static void pattern_trig_activate(struct led_classdev *led_cdev)
>> +{
>> +     struct pattern_trig_data *data;
>> +     int err;
>> +
>> +     data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +     if (!data)
>> +             return;
>> +
>> +     err = pattern_trig_initialize_data(data);
>> +     if (err) {
>> +             kfree(data);
>> +             return;
>> +     }
>> +
>> +     err = pattern_trig_create_sysfs_files(led_cdev->dev);
>> +     if (err) {
>> +             pattern_trig_clear_data(data);
>> +             kfree(data);
>> +             return;
>> +     }
>> +
>> +     data->led_cdev = led_cdev;
>> +     led_cdev->trigger_data = data;
>> +     timer_setup(&data->timer, pattern_trig_timer_function, 0);
>> +     led_cdev->activated = true;
>> +}
>> +
>> +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);
>> +     pattern_trig_remove_sysfs_files(led_cdev->dev);
>> +     led_set_brightness(led_cdev, LED_OFF);
>> +     pattern_trig_clear_data(data);
>> +     kfree(data);
>> +     led_cdev->trigger_data = NULL;
>> +     led_cdev->activated = false;
>> +}
>> +
>> +static struct led_trigger pattern_led_trigger = {
>> +     .name = "pattern",
>> +     .activate = pattern_trig_activate,
>> +     .deactivate = pattern_trig_deactivate,
>> +};
>> +
>> +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 b7e8255..bea02f0 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,14 @@ struct led_classdev {
>>                                    unsigned long *delay_on,
>>                                    unsigned long *delay_off);
>>
>> +     int (*pattern_set)(struct led_classdev *led_cdev,
>> +                        struct led_pattern *pattern, int len,
>> +                        unsigned repeat);
>> +     int (*pattern_get)(struct led_classdev *led_cdev,
>> +                        struct led_pattern *pattern, int *len,
>> +                        unsigned *repeat);
>> +     int (*pattern_clear)(struct led_classdev *led_cdev);
>> +
>>       struct device           *dev;
>>       const struct attribute_group    **groups;
>>
>> @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
>>       struct led_classdev *led_cdev, enum led_brightness brightness) { }
>>  #endif
>>
>> +/**
>> + * struct led_pattern - brightness value in a pattern
>> + * @delta_t: delay until next entry, in milliseconds
>> + * @brightness: brightness at time = 0
>> + */
>> +struct led_pattern {
>> +     int delta_t;
>> +     int brightness;
>> +};
>> +
>>  #endif               /* __LINUX_LEDS_H_INCLUDED */
>>
>
> --
> Best regards,
> Jacek Anaszewski



-- 
Baolin Wang
Best Regards

[PATCH 1/2] leds: core: Introduce generic pattern interface

[Baolin Wang]

From: Bjorn Andersson <bjorn.andersson@linaro.org>

Some LED controllers have support for autonomously controlling
brightness over time, according to some preprogrammed pattern or
function.

This adds a new optional operator that LED class drivers can implement
if they support such functionality as well as a new device attribute to
configure the pattern for a given LED.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Note: I removed the pattern repeat check and will get the repeat number by adding
one extra file named 'pattern_repeat' according to previous discussion.
---
 Documentation/ABI/testing/sysfs-class-led |   16 ++++
 drivers/leds/led-class.c                  |  127 +++++++++++++++++++++++++++++
 include/linux/leds.h                      |   19 +++++
 3 files changed, 162 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..fe90a12 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,19 @@ Description:
 		gpio and backlight triggers. In case of the backlight trigger,
 		it is useful when driving a LED which is intended to indicate
 		a device in a standby like state.
+
+What: /sys/class/leds/<led>/pattern
+Date: June 2018
+KernelVersion: 4.18
+Description:
+	Specify a pattern for the LED, for LED hardware that support
+	altering the brightness as a function of time.
+
+	The pattern is given by a series of tuples, of brightness and
+	duration (ms). The LED is expected to traverse the series and
+	each brightness value for the specified duration.
+
+	As LED hardware might have different capabilities and precision
+	the requested pattern might be slighly adjusted by the driver
+	and the resulting pattern of such operation should be returned
+	when this file is read.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..5ebdd83 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,131 @@ static ssize_t max_brightness_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(max_brightness);
 
+static ssize_t pattern_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct led_pattern *pattern;
+	size_t offset = 0;
+	int count, n, i;
+
+	if (!led_cdev->pattern_get)
+		return -EOPNOTSUPP;
+
+	pattern = led_cdev->pattern_get(led_cdev, &count);
+	if (IS_ERR_OR_NULL(pattern))
+		return PTR_ERR(pattern);
+
+	for (i = 0; i < count; i++) {
+		n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d",
+			     pattern[i].brightness, pattern[i].delta_t);
+
+		if (offset + n >= PAGE_SIZE)
+			goto err_nospc;
+
+		offset += n;
+
+		if (i < count - 1)
+			buf[offset++] = ' ';
+	}
+
+	buf[offset++] = '\n';
+
+	kfree(pattern);
+	return offset;
+
+err_nospc:
+	kfree(pattern);
+	return -ENOSPC;
+}
+
+static ssize_t pattern_store(struct device *dev,
+			     struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct led_pattern *pattern = NULL;
+	unsigned long val;
+	char *sbegin;
+	char *elem;
+	char *s;
+	int ret, len = 0;
+	bool odd = true;
+
+	s = sbegin = kstrndup(buf, size, GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	/* Trim trailing newline */
+	s[strcspn(s, "\n")] = '\0';
+
+	/* If the remaining string is empty, clear the pattern */
+	if (!s[0]) {
+		ret = led_cdev->pattern_clear(led_cdev);
+		goto out;
+	}
+
+	pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
+	if (!pattern) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* Parse out the brightness & delta_t touples and check for repeat */
+	while ((elem = strsep(&s, " ")) != NULL) {
+		ret = kstrtoul(elem, 10, &val);
+		if (ret)
+			goto out;
+
+		if (odd) {
+			pattern[len].brightness = val;
+		} else {
+			/* Ensure we don't have any delta_t == 0 */
+			if (!val) {
+				ret = -EINVAL;
+				goto out;
+			}
+
+			pattern[len].delta_t = val;
+			len++;
+		}
+
+		odd = !odd;
+	}
+
+	/*
+	 * Fail if we didn't find any data points or last data point was partial
+	 */
+	if (!len || !odd) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = led_cdev->pattern_set(led_cdev, pattern, len);
+
+out:
+	kfree(pattern);
+	kfree(sbegin);
+	return ret < 0 ? ret : size;
+}
+static DEVICE_ATTR_RW(pattern);
+
+static umode_t led_class_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_brightness.attr)
+		return attr->mode;
+	if (attr == &dev_attr_max_brightness.attr)
+		return attr->mode;
+	if (attr == &dev_attr_pattern.attr && led_cdev->pattern_set)
+		return attr->mode;
+
+	return 0;
+}
+
 #ifdef CONFIG_LEDS_TRIGGERS
 static DEVICE_ATTR(trigger, 0644, led_trigger_show, led_trigger_store);
 static struct attribute *led_trigger_attrs[] = {
@@ -88,11 +213,13 @@ static ssize_t max_brightness_show(struct device *dev,
 static struct attribute *led_class_attrs[] = {
 	&dev_attr_brightness.attr,
 	&dev_attr_max_brightness.attr,
+	&dev_attr_pattern.attr,
 	NULL,
 };
 
 static const struct attribute_group led_group = {
 	.attrs = led_class_attrs,
+	.is_visible = led_class_attrs_mode,
 };
 
 static const struct attribute_group *led_groups[] = {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b7e8255..acdbb2f 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,14 @@ struct led_classdev {
 				     unsigned long *delay_on,
 				     unsigned long *delay_off);
 
+	int (*pattern_set)(struct led_classdev *led_cdev,
+			   struct led_pattern *pattern, int len);
+
+	int (*pattern_clear)(struct led_classdev *led_cdev);
+
+	struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev,
+					   int *len);
+
 	struct device		*dev;
 	const struct attribute_group	**groups;
 
@@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
 	struct led_classdev *led_cdev, enum led_brightness brightness) { }
 #endif
 
+/**
+ * struct led_pattern - brigheness value in a pattern
+ * @delta_t: delay until next entry, in milliseconds
+ * @brightness: brightness at time = 0
+ */
+struct led_pattern {
+	int delta_t;
+	int brightness;
+};
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
1.7.9.5

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

[Baolin Wang]

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

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/leds/leds-sc27xx-bltc.c |  160 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 160 insertions(+)

diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..898f92d 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -6,6 +6,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/slab.h>
 #include <uapi/linux/uleds.h>
 
 /* PMIC global control register definition */
@@ -32,8 +33,13 @@
 #define SC27XX_DUTY_MASK	GENMASK(15, 0)
 #define SC27XX_MOD_MASK		GENMASK(7, 0)
 
+#define SC27XX_CURVE_SHIFT	8
+#define SC27XX_CURVE_L_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
 
 struct sc27xx_led {
 	char name[LED_MAX_NAME_SIZE];
@@ -122,6 +128,157 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum led_brightness value)
 	return err;
 }
 
+static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
+{
+	struct sc27xx_led *leds = to_sc27xx_led(ldev);
+	struct regmap *regmap = leds->priv->regmap;
+	u32 base = sc27xx_led_get_offset(leds);
+	u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+	u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+	int err;
+
+	mutex_lock(&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);
+
+	mutex_unlock(&leds->priv->lock);
+
+	return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+				  struct led_pattern *pattern,
+				  int len)
+{
+	struct sc27xx_led *leds = to_sc27xx_led(ldev);
+	u32 base = sc27xx_led_get_offset(leds);
+	u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+	u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+	struct regmap *regmap = leds->priv->regmap;
+	int err;
+
+	/*
+	 * Must contain 4 patterns to configure the rise time, high time, fall
+	 * time and low time to enable the breathing mode.
+	 */
+	if (len != SC27XX_LEDS_PATTERN_CNT)
+		return -EINVAL;
+
+	mutex_lock(&leds->priv->lock);
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+				 SC27XX_CURVE_L_MASK, pattern[0].delta_t);
+	if (err)
+		goto out;
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+				 SC27XX_CURVE_L_MASK, pattern[1].delta_t);
+	if (err)
+		goto out;
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+				 SC27XX_CURVE_H_MASK,
+				 pattern[2].delta_t << SC27XX_CURVE_SHIFT);
+	if (err)
+		goto out;
+
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+				 SC27XX_CURVE_H_MASK,
+				 pattern[3].delta_t << SC27XX_CURVE_SHIFT);
+	if (err)
+		goto out;
+
+
+	err = regmap_update_bits(regmap, base + SC27XX_LEDS_DUTY,
+				 SC27XX_DUTY_MASK,
+				 (pattern[0].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);
+
+out:
+	mutex_unlock(&leds->priv->lock);
+
+	return err;
+}
+
+static struct led_pattern *sc27xx_led_pattern_get(struct led_classdev *ldev,
+						  int *len)
+{
+	struct sc27xx_led *leds = to_sc27xx_led(ldev);
+	u32 base = sc27xx_led_get_offset(leds);
+	struct regmap *regmap = leds->priv->regmap;
+	struct led_pattern *pattern;
+	int i, err;
+	u32 val;
+
+	/*
+	 * Must allocate 4 patterns to show the rise time, high time, fall time
+	 * and low time.
+	 */
+	pattern = kcalloc(SC27XX_LEDS_PATTERN_CNT, sizeof(*pattern),
+			  GFP_KERNEL);
+	if (!pattern)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&leds->priv->lock);
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val);
+	if (err)
+		goto out;
+
+	pattern[0].delta_t = val & SC27XX_CURVE_L_MASK;
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val);
+	if (err)
+		goto out;
+
+	pattern[1].delta_t = val & SC27XX_CURVE_L_MASK;
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val);
+	if (err)
+		goto out;
+
+	pattern[2].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT;
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val);
+	if (err)
+		goto out;
+
+	pattern[3].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT;
+
+	err = regmap_read(regmap, base + SC27XX_LEDS_DUTY, &val);
+	if (err)
+		goto out;
+
+	mutex_unlock(&leds->priv->lock);
+
+	val = (val & SC27XX_DUTY_MASK) >> SC27XX_DUTY_SHIFT;
+	for (i = 0; i < SC27XX_LEDS_PATTERN_CNT; i++)
+		pattern[i].brightness = val;
+
+	*len = SC27XX_LEDS_PATTERN_CNT;
+
+	return pattern;
+
+out:
+	mutex_unlock(&leds->priv->lock);
+	kfree(pattern);
+
+	return ERR_PTR(err);
+}
+
 static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv)
 {
 	int i, err;
@@ -140,6 +297,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_get = sc27xx_led_pattern_get;
+		led->ldev.pattern_clear = sc27xx_led_pattern_clear;
 
 		err = devm_led_classdev_register(dev, &led->ldev);
 		if (err)
-- 
1.7.9.5