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

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

[Baolin Wang]

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

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

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

Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v7:
 - Move the SC27XX hardware patterns description into its own ABI file.

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

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

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

Changes from v3:
 - Reset pattern number to 0 if user provides incorrect pattern string.
 - Support one pattern.

Changes from v2:
 - Remove hardware_pattern boolen.
 - Chnage the pattern string format.

Changes from v1:
 - Use ATTRIBUTE_GROUPS() to define attributes.
 - Introduce hardware_pattern flag to determine if software pattern
 or hardware pattern.
 - Re-implement pattern_trig_store_pattern() function.
 - Remove pattern_get() interface.
 - Improve comments.
 - Other small optimization.
---
 .../ABI/testing/sysfs-class-led-trigger-pattern    |   38 +++
 drivers/leds/trigger/Kconfig                       |    7 +
 drivers/leds/trigger/Makefile                      |    1 +
 drivers/leds/trigger/ledtrig-pattern.c             |  337 ++++++++++++++++++++
 include/linux/leds.h                               |   16 +
 5 files changed, 399 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..8cc5099
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
@@ -0,0 +1,38 @@
+What:		/sys/class/leds/<led>/pattern
+Date:		September 2018
+KernelVersion:	4.20
+Description:
+		Specify a software pattern for the LED, that supports altering
+		the brightness for the specified duration with one software
+		timer.
+
+		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 software pattern values should be:
+		"brightness_1 duration_1 brightness_2 duration_2 brightness_3
+		duration_3 ...".
+
+What:		/sys/class/leds/<led>/hw_pattern
+Date:		September 2018
+KernelVersion:	4.20
+Description:
+		Specify a hardware pattern for the LED, for LED hardware that
+		supports autonomously controlling brightness over time, according
+		to some preprogrammed hardware patterns.
+
+		Since different LED hardware can have different semantics of
+		hardware patterns, each driver is expected to provide its own
+		description for the hardware patterns in their ABI documentation
+		file.
+
+What:		/sys/class/leds/<led>/repeat
+Date:		September 2018
+KernelVersion:	4.20
+Description:
+		Specify a pattern repeat number. 0 means repeat indefinitely.
+
+		This file will always return the originally written repeat
+		number.
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 4018af7..b76fc3c 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV
 	  This allows LEDs to be controlled by network device activity.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_PATTERN
+	tristate "LED Pattern Trigger"
+	help
+	  This allows LEDs to be controlled by a software or hardware pattern
+	  which is a series of tuples, of brightness and duration (ms).
+	  If unsure, say N
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index f3cfe19..9bcb64e 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
 obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
 obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
+obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
new file mode 100644
index 0000000..0179191
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-pattern.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * LED pattern trigger
+ *
+ * Idea discussed with Pavel Machek. Raphael Teysseyre implemented
+ * the first version, Baolin Wang simplified and improved the approach.
+ */
+
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+
+#define MAX_PATTERNS		1024
+
+struct pattern_trig_data {
+	struct led_classdev *led_cdev;
+	struct led_pattern patterns[MAX_PATTERNS];
+	struct led_pattern *curr;
+	struct led_pattern *next;
+	struct mutex lock;
+	u32 npatterns;
+	u32 repeat;
+	u32 last_repeat;
+	bool is_indefinite;
+	bool is_hw_pattern;
+	struct timer_list timer;
+};
+
+static void pattern_trig_update_patterns(struct pattern_trig_data *data)
+{
+	data->curr = data->next;
+	if (!data->is_indefinite && data->curr == data->patterns)
+		data->repeat--;
+
+	if (data->next == data->patterns + data->npatterns - 1)
+		data->next = data->patterns;
+	else
+		data->next++;
+}
+
+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 led_classdev *led_cdev)
+{
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+
+	if (!data->npatterns)
+		return 0;
+
+	if (data->is_hw_pattern) {
+		return led_cdev->pattern_set(led_cdev, data->patterns,
+					     data->npatterns, data->repeat);
+	}
+
+	data->curr = data->patterns;
+	data->next = data->npatterns > 1 ? data->patterns + 1 : data->patterns;
+	data->timer.expires = jiffies;
+	add_timer(&data->timer);
+
+	return 0;
+}
+
+static ssize_t repeat_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+	u32 repeat;
+
+	mutex_lock(&data->lock);
+
+	repeat = data->last_repeat;
+
+	mutex_unlock(&data->lock);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", repeat);
+}
+
+static ssize_t repeat_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+	unsigned long res;
+	int err;
+
+	err = kstrtoul(buf, 10, &res);
+	if (err)
+		return err;
+
+	/*
+	 * Clear previous patterns' performence firstly, and remove the timer
+	 * without mutex lock to avoid dead lock.
+	 */
+	del_timer_sync(&data->timer);
+
+	mutex_lock(&data->lock);
+
+	if (data->is_hw_pattern)
+		led_cdev->pattern_clear(led_cdev);
+
+	data->last_repeat = data->repeat = res;
+	/* 0 means repeat indefinitely */
+	data->is_indefinite = !data->repeat;
+
+	err = pattern_trig_start_pattern(led_cdev);
+
+	mutex_unlock(&data->lock);
+	return err < 0 ? err : count;
+}
+
+static DEVICE_ATTR_RW(repeat);
+
+static ssize_t pattern_trig_show_patterns(struct pattern_trig_data *data,
+					  char *buf, bool hw_pattern)
+{
+	ssize_t count = 0;
+	int i;
+
+	mutex_lock(&data->lock);
+
+	if (!data->npatterns || (data->is_hw_pattern ^ hw_pattern))
+		goto out;
+
+	for (i = 0; i < data->npatterns; i++) {
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "%d %d ",
+				   data->patterns[i].brightness,
+				   data->patterns[i].delta_t);
+	}
+
+	buf[count - 1] = '\n';
+
+out:
+	mutex_unlock(&data->lock);
+	return count;
+}
+
+static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev,
+					   const char *buf, size_t count,
+					   bool hw_pattern)
+{
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+	int ccount, cr, offset = 0, err = 0;
+
+	/*
+	 * Clear previous patterns' performence firstly, and remove the timer
+	 * without mutex lock to avoid dead lock.
+	 */
+	del_timer_sync(&data->timer);
+
+	mutex_lock(&data->lock);
+
+	if (data->is_hw_pattern)
+		led_cdev->pattern_clear(led_cdev);
+
+	data->is_hw_pattern = hw_pattern;
+	data->npatterns = 0;
+
+	while (offset < count - 1 && data->npatterns < MAX_PATTERNS) {
+		cr = 0;
+		ccount = sscanf(buf + offset, "%d %d %n",
+				&data->patterns[data->npatterns].brightness,
+				&data->patterns[data->npatterns].delta_t, &cr);
+		if (ccount != 2) {
+			data->npatterns = 0;
+			err = -EINVAL;
+			goto out;
+		}
+
+		offset += cr;
+		data->npatterns++;
+	}
+
+	err = pattern_trig_start_pattern(led_cdev);
+
+out:
+	mutex_unlock(&data->lock);
+	return err < 0 ? err : count;
+}
+
+static ssize_t pattern_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+
+	return pattern_trig_show_patterns(data, buf, false);
+}
+
+static ssize_t pattern_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return pattern_trig_store_patterns(led_cdev, buf, count, false);
+}
+
+static DEVICE_ATTR_RW(pattern);
+
+static ssize_t hw_pattern_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+
+	return pattern_trig_show_patterns(data, buf, true);
+}
+
+static ssize_t hw_pattern_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return pattern_trig_store_patterns(led_cdev, buf, count, true);
+}
+
+static DEVICE_ATTR_RW(hw_pattern);
+
+static umode_t pattern_trig_attrs_mode(struct kobject *kobj,
+				       struct attribute *attr, int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	if (attr == &dev_attr_repeat.attr || attr == &dev_attr_pattern.attr)
+		return attr->mode;
+	else if (attr == &dev_attr_hw_pattern.attr && led_cdev->pattern_set)
+		return attr->mode;
+
+	return 0;
+}
+
+static struct attribute *pattern_trig_attrs[] = {
+	&dev_attr_pattern.attr,
+	&dev_attr_hw_pattern.attr,
+	&dev_attr_repeat.attr,
+	NULL
+};
+
+static const struct attribute_group pattern_trig_group = {
+	.attrs = pattern_trig_attrs,
+	.is_visible = pattern_trig_attrs_mode,
+};
+
+static const struct attribute_group *pattern_trig_groups[] = {
+	&pattern_trig_group,
+	NULL,
+};
+
+static int pattern_trig_activate(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *data;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	if (!!led_cdev->pattern_set ^ !!led_cdev->pattern_clear) {
+		dev_warn(led_cdev->dev,
+			 "Hardware pattern ops validation failed\n");
+		led_cdev->pattern_set = NULL;
+		led_cdev->pattern_clear = NULL;
+	}
+
+	data->is_indefinite = true;
+	mutex_init(&data->lock);
+	data->led_cdev = led_cdev;
+	led_set_trigger_data(led_cdev, data);
+	timer_setup(&data->timer, pattern_trig_timer_function, 0);
+	led_cdev->activated = true;
+
+	return 0;
+}
+
+static void pattern_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct pattern_trig_data *data = led_cdev->trigger_data;
+
+	if (!led_cdev->activated)
+		return;
+
+	if (led_cdev->pattern_clear)
+		led_cdev->pattern_clear(led_cdev);
+
+	del_timer_sync(&data->timer);
+
+	led_set_brightness(led_cdev, LED_OFF);
+	kfree(data);
+	led_cdev->activated = false;
+}
+
+static struct led_trigger pattern_led_trigger = {
+	.name = "pattern",
+	.activate = pattern_trig_activate,
+	.deactivate = pattern_trig_deactivate,
+	.groups = pattern_trig_groups,
+};
+
+static int __init pattern_trig_init(void)
+{
+	return led_trigger_register(&pattern_led_trigger);
+}
+
+static void __exit pattern_trig_exit(void)
+{
+	led_trigger_unregister(&pattern_led_trigger);
+}
+
+module_init(pattern_trig_init);
+module_exit(pattern_trig_exit);
+
+MODULE_AUTHOR("Raphael Teysseyre <rteysseyre@gmail.com");
+MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org");
+MODULE_DESCRIPTION("LED Pattern trigger");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 834683d..74fc2c6 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,11 @@ 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 int repeat);
+	int (*pattern_clear)(struct led_classdev *led_cdev);
+
 	struct device		*dev;
 	const struct attribute_group	**groups;
 
@@ -472,4 +478,14 @@ static inline void led_classdev_notify_brightness_hw_changed(
 	struct led_classdev *led_cdev, enum led_brightness brightness) { }
 #endif
 
+/**
+ * struct led_pattern - pattern interval settings
+ * @delta_t: pattern interval delay, in milliseconds
+ * @brightness: pattern interval brightness
+ */
+struct led_pattern {
+	int delta_t;
+	int brightness;
+};
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
1.7.9.5

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

[Baolin Wang]

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

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v7:
 - Add its own ABI documentation file.

Changes from v6:
 - None.

Changes from v5:
 - None.

Changes from v4:
 - None.

Changes from v3:
 - None.

Changes from v2:
 - None.

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

diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
new file mode 100644
index 0000000..ecef3ba
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
@@ -0,0 +1,11 @@
+What:		/sys/class/leds/<led>/hw_pattern
+Date:		September 2018
+KernelVersion:	4.20
+Description:
+		Specify a hardware pattern for the SC27XX LED. For the SC27XX
+		LED controller, it only supports 4 hardware patterns to configure
+		the low time, rise time, high time and fall time for the breathing
+		mode, and each stage duration unit is 125ms. So the format of
+		the hardware pattern values should be:
+		"brightness_1 duration_1 brightness_2 duration_2 brightness_3
+		duration_3 brightness_4 duration_4".
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..297dd43 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,91 @@ 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_register(struct device *dev, struct sc27xx_led_priv *priv)
 {
 	int i, err;
@@ -140,6 +230,9 @@ static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv)
 		led->priv = priv;
 		led->ldev.name = led->name;
 		led->ldev.brightness_set_blocking = sc27xx_led_set;
+		led->ldev.pattern_set = sc27xx_led_pattern_set;
+		led->ldev.pattern_clear = sc27xx_led_pattern_clear;
+		led->ldev.default_trigger = "pattern";
 
 		err = devm_led_classdev_register(dev, &led->ldev);
 		if (err)
@@ -241,4 +334,5 @@ static int sc27xx_led_remove(struct platform_device *pdev)
 
 MODULE_DESCRIPTION("Spreadtrum SC27xx breathing light controller driver");
 MODULE_AUTHOR("Xiaotong Lu <xiaotong.lu@spreadtrum.com>");
+MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org>");
 MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

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

[Jacek Anaszewski]

Hi Baolin,

On 09/04/2018 01:01 PM, Baolin Wang wrote:
> This patch implements the 'pattern_set'and 'pattern_clear'
> interfaces to support SC27XX LED breathing mode.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v7:
>  - Add its own ABI documentation file.
> 
> Changes from v6:
>  - None.
> 
> Changes from v5:
>  - None.
> 
> Changes from v4:
>  - None.
> 
> Changes from v3:
>  - None.
> 
> Changes from v2:
>  - None.
> 
> Changes from v1:
>  - Remove pattern_get interface.
> ---
>  .../ABI/testing/sysfs-class-led-driver-sc27xx      |   11 +++
>  drivers/leds/leds-sc27xx-bltc.c                    |   94 ++++++++++++++++++++
>  2 files changed, 105 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
> new file mode 100644
> index 0000000..ecef3ba
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
> @@ -0,0 +1,11 @@
> +What:		/sys/class/leds/<led>/hw_pattern
> +Date:		September 2018
> +KernelVersion:	4.20
> +Description:
> +		Specify a hardware pattern for the SC27XX LED. For the SC27XX
> +		LED controller, it only supports 4 hardware patterns to configure

If I understand it correctly then the four components: low, rise, high,
and fall time make a single pattern. So calling it "4 hardware patterns"
can be misleading. Below you're using "stage" notion - it would be good
to use it consequently on the whole span of this document.

> +		the low time, rise time, high time and fall time for the breathing
> +		mode, and each stage duration unit is 125ms. So the format of
> +		the hardware pattern values should be:

I'd be more precise here:

Min stage duration: ???
Max stage duration: ???
Stage duration step: 125 ms

> +		"brightness_1 duration_1 brightness_2 duration_2 brightness_3
> +		duration_3 brightness_4 duration_4".

Looking at sc27xx_led_pattern_set() it doesn't seem like you would
use brightnesses other then brightness_1. I assume that the low
brightness is fixed to 0 and the high brightness is the brightness_1.
If yes, than we don't need brightnesses in this pattern definition,
since the current brightness will suffice.

You'd need to alter the hw_pattern description to say that the
brightness currently set is to be used as a high brightness, and the
hw_pattern for this driver consists only of the four delta_t components.

This is clear example of what I had on mind when having doubts
about using tuples for pattern_set.

Alternatively, if we want to enforce tuples format for hw_pattern,
and if we want to be consistent - we'd need to require defining target
brightness for each stage properly, i.e.

echo "100 500 100 500 0 500 0 500" > pattern

Which would mean:

1. Rise from brightness 0 to 100 for 500 ms.
2. Keep brightness 100 for 500 ms.
3. Fall from brightness 100 to 0 for 500 ms.
4. Keep brightness 0 for 500 ms.

-- 
Best regards,
Jacek Anaszewski

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

[Baolin Wang]

Hi Jacek,

On 5 September 2018 at 04:19, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> Hi Baolin,
>
> On 09/04/2018 01:01 PM, Baolin Wang wrote:
>> This patch implements the 'pattern_set'and 'pattern_clear'
>> interfaces to support SC27XX LED breathing mode.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v7:
>>  - Add its own ABI documentation file.
>>
>> Changes from v6:
>>  - None.
>>
>> Changes from v5:
>>  - None.
>>
>> Changes from v4:
>>  - None.
>>
>> Changes from v3:
>>  - None.
>>
>> Changes from v2:
>>  - None.
>>
>> Changes from v1:
>>  - Remove pattern_get interface.
>> ---
>>  .../ABI/testing/sysfs-class-led-driver-sc27xx      |   11 +++
>>  drivers/leds/leds-sc27xx-bltc.c                    |   94 ++++++++++++++++++++
>>  2 files changed, 105 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> new file mode 100644
>> index 0000000..ecef3ba
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> @@ -0,0 +1,11 @@
>> +What:                /sys/class/leds/<led>/hw_pattern
>> +Date:                September 2018
>> +KernelVersion:       4.20
>> +Description:
>> +             Specify a hardware pattern for the SC27XX LED. For the SC27XX
>> +             LED controller, it only supports 4 hardware patterns to configure
>
> If I understand it correctly then the four components: low, rise, high,
> and fall time make a single pattern. So calling it "4 hardware patterns"
> can be misleading. Below you're using "stage" notion - it would be good
> to use it consequently on the whole span of this document.

Sure. I will modify the documentation to avoid misleading words.

>
>> +             the low time, rise time, high time and fall time for the breathing
>> +             mode, and each stage duration unit is 125ms. So the format of
>> +             the hardware pattern values should be:
>
> I'd be more precise here:
>
> Min stage duration: ???
> Max stage duration: ???
> Stage duration step: 125 ms

OK.

>
>> +             "brightness_1 duration_1 brightness_2 duration_2 brightness_3
>> +             duration_3 brightness_4 duration_4".
>
> Looking at sc27xx_led_pattern_set() it doesn't seem like you would
> use brightnesses other then brightness_1. I assume that the low
> brightness is fixed to 0 and the high brightness is the brightness_1.
> If yes, than we don't need brightnesses in this pattern definition,
> since the current brightness will suffice.
>
> You'd need to alter the hw_pattern description to say that the
> brightness currently set is to be used as a high brightness, and the
> hw_pattern for this driver consists only of the four delta_t components.

Sorry for confusing here. For SC27XX LED, the 4 stages use one same
brightness. To be compatible with the hardware pattern format, so we
force to set one brightness for each stage. I will add some comments
to explain the LED expects the same brightness for each stage instead
of using the current brightness file which is not used for our
breathing mode.

>
> This is clear example of what I had on mind when having doubts
> about using tuples for pattern_set.
>
> Alternatively, if we want to enforce tuples format for hw_pattern,
> and if we want to be consistent - we'd need to require defining target
> brightness for each stage properly, i.e.
>
> echo "100 500 100 500 0 500 0 500" > pattern
>
> Which would mean:
>
> 1. Rise from brightness 0 to 100 for 500 ms.
> 2. Keep brightness 100 for 500 ms.
> 3. Fall from brightness 100 to 0 for 500 ms.
> 4. Keep brightness 0 for 500 ms.
>
> --
> Best regards,
> Jacek Anaszewski



-- 
Baolin Wang
Best Regards

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

[Baolin Wang]

On 5 September 2018 at 10:43, Baolin Wang <baolin.wang@linaro.org> wrote:
> Hi Jacek,
>
> On 5 September 2018 at 04:19, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> Hi Baolin,
>>
>> On 09/04/2018 01:01 PM, Baolin Wang wrote:
>>> This patch implements the 'pattern_set'and 'pattern_clear'
>>> interfaces to support SC27XX LED breathing mode.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>> Changes from v7:
>>>  - Add its own ABI documentation file.
>>>
>>> Changes from v6:
>>>  - None.
>>>
>>> Changes from v5:
>>>  - None.
>>>
>>> Changes from v4:
>>>  - None.
>>>
>>> Changes from v3:
>>>  - None.
>>>
>>> Changes from v2:
>>>  - None.
>>>
>>> Changes from v1:
>>>  - Remove pattern_get interface.
>>> ---
>>>  .../ABI/testing/sysfs-class-led-driver-sc27xx      |   11 +++
>>>  drivers/leds/leds-sc27xx-bltc.c                    |   94 ++++++++++++++++++++
>>>  2 files changed, 105 insertions(+)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>>> new file mode 100644
>>> index 0000000..ecef3ba
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>>> @@ -0,0 +1,11 @@
>>> +What:                /sys/class/leds/<led>/hw_pattern
>>> +Date:                September 2018
>>> +KernelVersion:       4.20
>>> +Description:
>>> +             Specify a hardware pattern for the SC27XX LED. For the SC27XX
>>> +             LED controller, it only supports 4 hardware patterns to configure
>>
>> If I understand it correctly then the four components: low, rise, high,
>> and fall time make a single pattern. So calling it "4 hardware patterns"
>> can be misleading. Below you're using "stage" notion - it would be good
>> to use it consequently on the whole span of this document.
>
> Sure. I will modify the documentation to avoid misleading words.
>
>>
>>> +             the low time, rise time, high time and fall time for the breathing
>>> +             mode, and each stage duration unit is 125ms. So the format of
>>> +             the hardware pattern values should be:
>>
>> I'd be more precise here:
>>
>> Min stage duration: ???
>> Max stage duration: ???
>> Stage duration step: 125 ms
>
> OK.
>
>>
>>> +             "brightness_1 duration_1 brightness_2 duration_2 brightness_3
>>> +             duration_3 brightness_4 duration_4".
>>
>> Looking at sc27xx_led_pattern_set() it doesn't seem like you would
>> use brightnesses other then brightness_1. I assume that the low
>> brightness is fixed to 0 and the high brightness is the brightness_1.
>> If yes, than we don't need brightnesses in this pattern definition,
>> since the current brightness will suffice.
>>
>> You'd need to alter the hw_pattern description to say that the
>> brightness currently set is to be used as a high brightness, and the
>> hw_pattern for this driver consists only of the four delta_t components.
>
> Sorry for confusing here. For SC27XX LED, the 4 stages use one same
> brightness. To be compatible with the hardware pattern format, so we
> force to set one brightness for each stage. I will add some comments
> to explain the LED expects the same brightness for each stage instead
> of using the current brightness file which is not used for our
> breathing mode.
>
>>
>> This is clear example of what I had on mind when having doubts
>> about using tuples for pattern_set.
>>
>> Alternatively, if we want to enforce tuples format for hw_pattern,
>> and if we want to be consistent - we'd need to require defining target
>> brightness for each stage properly, i.e.
>>
>> echo "100 500 100 500 0 500 0 500" > pattern

After more thinking, I will change the description to reflect the real
brightness according to your suggestion. Please see my new patch.
Thanks.

>>
>> Which would mean:
>>
>> 1. Rise from brightness 0 to 100 for 500 ms.
>> 2. Keep brightness 100 for 500 ms.
>> 3. Fall from brightness 100 to 0 for 500 ms.
>> 4. Keep brightness 0 for 500 ms.
>>
>> --
>> Best regards,
>> Jacek Anaszewski
>
>
>
> --
> Baolin Wang
> Best Regards



-- 
Baolin Wang
Best Regards

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

[Bjorn Andersson]

On Tue 04 Sep 04:01 PDT 2018, Baolin Wang wrote:

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

So, after a full circle we're back at drivers with support for hardware
patterns should have their own ABI for setting that pattern.

The controls for my hardware is:
* a list of brightness values
* the rate of the pattern
* a flag to indicate that the pattern should be played from start
  to end, end to start or start to end to start
* a boolean indicating if the pattern should be played once or repeated
  indefinitely.

Given that the interface now is hw specific, what benefit is there to
attempt to cram these 4 knobs into "hw_pattern"? Or am I allowed to
create additional files for the latter three?

> +What:		/sys/class/leds/<led>/repeat
> +Date:		September 2018
> +KernelVersion:	4.20
> +Description:
> +		Specify a pattern repeat number. 0 means repeat indefinitely.
> +
> +		This file will always return the originally written repeat
> +		number.

I'm still convinced that this will confuse our users and to me it would
be more logical if this denotes the number of times the pattern should
be repeated, with e.g. negative numbers denoting infinite.

In particular I expect to have to explain why my driver expects that you
write 0 in the file named "repeat" to make it repeat and 1 to make it
not repeat.

Regards,
Bjorn

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

[Jacek Anaszewski]

Hi Bjorn,

On 09/08/2018 07:02 AM, Bjorn Andersson wrote:
> On Tue 04 Sep 04:01 PDT 2018, Baolin Wang wrote:
> 
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> [..]
>> +What:		/sys/class/leds/<led>/hw_pattern
>> +Date:		September 2018
>> +KernelVersion:	4.20
>> +Description:
>> +		Specify a hardware pattern for the LED, for LED hardware that
>> +		supports autonomously controlling brightness over time, according
>> +		to some preprogrammed hardware patterns.
>> +
>> +		Since different LED hardware can have different semantics of
>> +		hardware patterns, each driver is expected to provide its own
>> +		description for the hardware patterns in their ABI documentation
>> +		file.
>> +
> 
> So, after a full circle we're back at drivers with support for hardware
> patterns should have their own ABI for setting that pattern.
> 
> The controls for my hardware is:
> * a list of brightness values
> * the rate of the pattern
> * a flag to indicate that the pattern should be played from start
>   to end, end to start or start to end to start
> * a boolean indicating if the pattern should be played once or repeated
>   indefinitely.
> 
> Given that the interface now is hw specific, what benefit is there to
> attempt to cram these 4 knobs into "hw_pattern"? Or am I allowed to
> create additional files for the latter three?

So this is an argument corroborating my concerns raised in [0].
I really think that we should allow for custom pattern interfaces
defined by LED class drivers.

>> +What:		/sys/class/leds/<led>/repeat
>> +Date:		September 2018
>> +KernelVersion:	4.20
>> +Description:
>> +		Specify a pattern repeat number. 0 means repeat indefinitely.
>> +
>> +		This file will always return the originally written repeat
>> +		number.
> 
> I'm still convinced that this will confuse our users and to me it would
> be more logical if this denotes the number of times the pattern should
> be repeated, with e.g. negative numbers denoting infinite.

Sounds reasonable. Let's change this semantics as you propose.

> In particular I expect to have to explain why my driver expects that you
> write 0 in the file named "repeat" to make it repeat and 1 to make it
> not repeat.



[0] https://lkml.org/lkml/2018/9/3/1192

-- 
Best regards,
Jacek Anaszewski

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

[Baolin Wang]

On 9 September 2018 at 04:19, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> Hi Bjorn,
>
> On 09/08/2018 07:02 AM, Bjorn Andersson wrote:
>> On Tue 04 Sep 04:01 PDT 2018, Baolin Wang wrote:
>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> [..]
>>> +What:               /sys/class/leds/<led>/hw_pattern
>>> +Date:               September 2018
>>> +KernelVersion:      4.20
>>> +Description:
>>> +            Specify a hardware pattern for the LED, for LED hardware that
>>> +            supports autonomously controlling brightness over time, according
>>> +            to some preprogrammed hardware patterns.
>>> +
>>> +            Since different LED hardware can have different semantics of
>>> +            hardware patterns, each driver is expected to provide its own
>>> +            description for the hardware patterns in their ABI documentation
>>> +            file.
>>> +
>>
>> So, after a full circle we're back at drivers with support for hardware
>> patterns should have their own ABI for setting that pattern.
>>
>> The controls for my hardware is:
>> * a list of brightness values
>> * the rate of the pattern
>> * a flag to indicate that the pattern should be played from start
>>   to end, end to start or start to end to start
>> * a boolean indicating if the pattern should be played once or repeated
>>   indefinitely.
>>
>> Given that the interface now is hw specific, what benefit is there to
>> attempt to cram these 4 knobs into "hw_pattern"? Or am I allowed to
>> create additional files for the latter three?
>
> So this is an argument corroborating my concerns raised in [0].
> I really think that we should allow for custom pattern interfaces
> defined by LED class drivers.
>
>>> +What:               /sys/class/leds/<led>/repeat
>>> +Date:               September 2018
>>> +KernelVersion:      4.20
>>> +Description:
>>> +            Specify a pattern repeat number. 0 means repeat indefinitely.
>>> +
>>> +            This file will always return the originally written repeat
>>> +            number.
>>
>> I'm still convinced that this will confuse our users and to me it would
>> be more logical if this denotes the number of times the pattern should
>> be repeated, with e.g. negative numbers denoting infinite.
>
> Sounds reasonable. Let's change this semantics as you propose.
>
>> In particular I expect to have to explain why my driver expects that you
>> write 0 in the file named "repeat" to make it repeat and 1 to make it
>> not repeat.

Hm, so there are some cases we need to make clear.
1) If negative numbers present infinite, so what's the meaning of number 0?
2) What we should show for users if repeat number is negative, just
show negative numbers or one string "infinite"?

-- 
Baolin Wang
Best Regards

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

[Pavel Machek]

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

On Fri 2018-09-07 22:02:08, Bjorn Andersson wrote:
> On Tue 04 Sep 04:01 PDT 2018, Baolin Wang wrote:
> 
> > diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> [..]
> > +What:		/sys/class/leds/<led>/hw_pattern
> > +Date:		September 2018
> > +KernelVersion:	4.20
> > +Description:
> > +		Specify a hardware pattern for the LED, for LED hardware that
> > +		supports autonomously controlling brightness over time, according
> > +		to some preprogrammed hardware patterns.
> > +
> > +		Since different LED hardware can have different semantics of
> > +		hardware patterns, each driver is expected to provide its own
> > +		description for the hardware patterns in their ABI documentation
> > +		file.
> > +
> 
> So, after a full circle we're back at drivers with support for hardware
> patterns should have their own ABI for setting that pattern.
> 
> The controls for my hardware is:
> * a list of brightness values
> * the rate of the pattern
> * a flag to indicate that the pattern should be played from start
>   to end, end to start or start to end to start
> * a boolean indicating if the pattern should be played once or repeated
>   indefinitely.

No, we are not back to full circle.

Or at least we should not be.

Yes, hw_pattern can have some limitation pattern does not, but if you
take values from hw_pattern file and put them into pattern file, you
should get the same pattern (with more power being consumed). And that
property is kind of important for me, because it should keep the ABI
reasonable.


> > +What:		/sys/class/leds/<led>/repeat
> > +Date:		September 2018
> > +KernelVersion:	4.20
> > +Description:
> > +		Specify a pattern repeat number. 0 means repeat indefinitely.
> > +
> > +		This file will always return the originally written repeat
> > +		number.
> 
> I'm still convinced that this will confuse our users and to me it would
> be more logical if this denotes the number of times the pattern should
> be repeated, with e.g. negative numbers denoting infinite.
> 
> In particular I expect to have to explain why my driver expects that you
> write 0 in the file named "repeat" to make it repeat and 1 to make it
> not repeat.

Ok, -1 works for me :-).

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

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

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

[Pavel Machek]

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

Hi!

> >> I'm still convinced that this will confuse our users and to me it would
> >> be more logical if this denotes the number of times the pattern should
> >> be repeated, with e.g. negative numbers denoting infinite.
> >
> > Sounds reasonable. Let's change this semantics as you propose.
> >
> >> In particular I expect to have to explain why my driver expects that you
> >> write 0 in the file named "repeat" to make it repeat and 1 to make it
> >> not repeat.
> 
> Hm, so there are some cases we need to make clear.
> 1) If negative numbers present infinite, so what's the meaning of number 0?
> 2) What we should show for users if repeat number is negative, just
> show negative numbers or one string "infinite"?

I'd say just -1 is infinite, anything else is error.

And yes, reading it should just display -1.

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

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

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

[Baolin Wang]

On 11 September 2018 at 05:20, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> >> I'm still convinced that this will confuse our users and to me it would
>> >> be more logical if this denotes the number of times the pattern should
>> >> be repeated, with e.g. negative numbers denoting infinite.
>> >
>> > Sounds reasonable. Let's change this semantics as you propose.
>> >
>> >> In particular I expect to have to explain why my driver expects that you
>> >> write 0 in the file named "repeat" to make it repeat and 1 to make it
>> >> not repeat.
>>
>> Hm, so there are some cases we need to make clear.
>> 1) If negative numbers present infinite, so what's the meaning of number 0?
>> 2) What we should show for users if repeat number is negative, just
>> show negative numbers or one string "infinite"?
>
> I'd say just -1 is infinite, anything else is error.
>
> And yes, reading it should just display -1.

OK. Thanks.

-- 
Baolin Wang
Best Regards

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

[Jacek Anaszewski]

Pavel,

On 09/10/2018 11:19 PM, Pavel Machek wrote:
> On Fri 2018-09-07 22:02:08, Bjorn Andersson wrote:
>> On Tue 04 Sep 04:01 PDT 2018, Baolin Wang wrote:
>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>> [..]
>>> +What:		/sys/class/leds/<led>/hw_pattern
>>> +Date:		September 2018
>>> +KernelVersion:	4.20
>>> +Description:
>>> +		Specify a hardware pattern for the LED, for LED hardware that
>>> +		supports autonomously controlling brightness over time, according
>>> +		to some preprogrammed hardware patterns.
>>> +
>>> +		Since different LED hardware can have different semantics of
>>> +		hardware patterns, each driver is expected to provide its own
>>> +		description for the hardware patterns in their ABI documentation
>>> +		file.
>>> +
>>
>> So, after a full circle we're back at drivers with support for hardware
>> patterns should have their own ABI for setting that pattern.
>>
>> The controls for my hardware is:
>> * a list of brightness values
>> * the rate of the pattern
>> * a flag to indicate that the pattern should be played from start
>>   to end, end to start or start to end to start
>> * a boolean indicating if the pattern should be played once or repeated
>>   indefinitely.
> 
> No, we are not back to full circle.
> 
> Or at least we should not be.
> 
> Yes, hw_pattern can have some limitation pattern does not, but if you
> take values from hw_pattern file and put them into pattern file, you
> should get the same pattern (with more power being consumed). And that
> property is kind of important for me, because it should keep the ABI
> reasonable.

If you looked at what we agreed on with Baolin, you'd realize
that this property is in no way preserved.
This is what the whole story is about - we're introducing hw_pattern
because of difficulties in describing breathing pattern by a series
of [brightness delta_t] tuples.

And Bjorn presented another example. I'm inclined to leave the
definition of hw_pattern semantics to the LED class drivers,
and allow them to create related sysfs files.

>>> +What:		/sys/class/leds/<led>/repeat
>>> +Date:		September 2018
>>> +KernelVersion:	4.20
>>> +Description:
>>> +		Specify a pattern repeat number. 0 means repeat indefinitely.
>>> +
>>> +		This file will always return the originally written repeat
>>> +		number.
>>
>> I'm still convinced that this will confuse our users and to me it would
>> be more logical if this denotes the number of times the pattern should
>> be repeated, with e.g. negative numbers denoting infinite.
>>
>> In particular I expect to have to explain why my driver expects that you
>> write 0 in the file named "repeat" to make it repeat and 1 to make it
>> not repeat.
> 
> Ok, -1 works for me :-).
> 
> Thanks,
> 									Pavel
> 

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

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

Hi!

> >>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> >> [..]
> >>> +What:		/sys/class/leds/<led>/hw_pattern
> >>> +Date:		September 2018
> >>> +KernelVersion:	4.20
> >>> +Description:
> >>> +		Specify a hardware pattern for the LED, for LED hardware that
> >>> +		supports autonomously controlling brightness over time, according
> >>> +		to some preprogrammed hardware patterns.
> >>> +
> >>> +		Since different LED hardware can have different semantics of
> >>> +		hardware patterns, each driver is expected to provide its own
> >>> +		description for the hardware patterns in their ABI documentation
> >>> +		file.
> >>> +
> >>
> >> So, after a full circle we're back at drivers with support for hardware
> >> patterns should have their own ABI for setting that pattern.
> >>
> >> The controls for my hardware is:
> >> * a list of brightness values
> >> * the rate of the pattern
> >> * a flag to indicate that the pattern should be played from start
> >>   to end, end to start or start to end to start
> >> * a boolean indicating if the pattern should be played once or repeated
> >>   indefinitely.
> > 
> > No, we are not back to full circle.
> > 
> > Or at least we should not be.
> > 
> > Yes, hw_pattern can have some limitation pattern does not, but if you
> > take values from hw_pattern file and put them into pattern file, you
> > should get the same pattern (with more power being consumed). And that
> > property is kind of important for me, because it should keep the ABI
> > reasonable.
> 
> If you looked at what we agreed on with Baolin, you'd realize
> that this property is in no way preserved.
> This is what the whole story is about - we're introducing hw_pattern
> because of difficulties in describing breathing pattern by a series
> of [brightness delta_t] tuples.
> 
> And Bjorn presented another example. I'm inclined to leave the
> definition of hw_pattern semantics to the LED class drivers,
> and allow them to create related sysfs files.

Please lets not do that.

We already have drivers that do that and it is complete
nightmare. Some take binary code for the tiny CPU driving the LED.

What exactly is the problem? [brightness delta_t] can describe
anything single LED can do in finite time. You are right, that
[brightness delta_t] sequence may get rather long, and it may be hard
to turn that sequence into parameters. Are there any _interesting_
sequences hardware can do but [brightness delta_t] can not store
reasonably?

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

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

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

[Jacek Anaszewski]

On 09/12/2018 09:18 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>>> [..]
>>>>> +What:		/sys/class/leds/<led>/hw_pattern
>>>>> +Date:		September 2018
>>>>> +KernelVersion:	4.20
>>>>> +Description:
>>>>> +		Specify a hardware pattern for the LED, for LED hardware that
>>>>> +		supports autonomously controlling brightness over time, according
>>>>> +		to some preprogrammed hardware patterns.
>>>>> +
>>>>> +		Since different LED hardware can have different semantics of
>>>>> +		hardware patterns, each driver is expected to provide its own
>>>>> +		description for the hardware patterns in their ABI documentation
>>>>> +		file.
>>>>> +
>>>>
>>>> So, after a full circle we're back at drivers with support for hardware
>>>> patterns should have their own ABI for setting that pattern.
>>>>
>>>> The controls for my hardware is:
>>>> * a list of brightness values
>>>> * the rate of the pattern
>>>> * a flag to indicate that the pattern should be played from start
>>>>   to end, end to start or start to end to start
>>>> * a boolean indicating if the pattern should be played once or repeated
>>>>   indefinitely.
>>>
>>> No, we are not back to full circle.
>>>
>>> Or at least we should not be.
>>>
>>> Yes, hw_pattern can have some limitation pattern does not, but if you
>>> take values from hw_pattern file and put them into pattern file, you
>>> should get the same pattern (with more power being consumed). And that
>>> property is kind of important for me, because it should keep the ABI
>>> reasonable.
>>
>> If you looked at what we agreed on with Baolin, you'd realize
>> that this property is in no way preserved.
>> This is what the whole story is about - we're introducing hw_pattern
>> because of difficulties in describing breathing pattern by a series
>> of [brightness delta_t] tuples.
>>
>> And Bjorn presented another example. I'm inclined to leave the
>> definition of hw_pattern semantics to the LED class drivers,
>> and allow them to create related sysfs files.
> 
> Please lets not do that.
> 
> We already have drivers that do that and it is complete
> nightmare. Some take binary code for the tiny CPU driving the LED.
> 
> What exactly is the problem? [brightness delta_t] can describe

You wrote:

<quote>
Yes, hw_pattern can have some limitation pattern does not, but if you
take values from hw_pattern file and put them into pattern file, you
should get the same pattern (with more power being consumed).
</quote>

The problem is that we decided to introduce hw_pattern to
to take away from drivers a responsibility for translating
a series of tuples, approximating the brightness curve,
to the values that can be written to the hardware registers.

Because this is what would need to be done to check if hw can support
given series of tuples and activate it. Actually with this approach
we wouldn't need hw_pattern at all, since pattern alone would do.
But implementation thereof is what we could call a nightmare.

What follows, your claim from the quotation is inaccurate:
values from hw_pattern written to the pattern file will not
produce the same pattern, at least in case of what was proposed
in [0] for drivers/leds/leds-sc27xx-bltc.c.

> anything single LED can do in finite time. You are right, that
> [brightness delta_t] sequence may get rather long, and it may be hard
> to turn that sequence into parameters. Are there any _interesting_
> sequences hardware can do but [brightness delta_t] can not store
> reasonably?

Please propose the implementation of pattern_set for
drivers/leds/leds-sc27xx-bltc.c breathing pattern, that will
setup breathing mode basing on the values from tuples.

Use Baolin's patch [0] for a reference of what hardware expects.

[0] https://lore.kernel.org/patchwork/patch/984246/

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

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

Hi!

> >>> No, we are not back to full circle.
> >>>
> >>> Or at least we should not be.
> >>>
> >>> Yes, hw_pattern can have some limitation pattern does not, but if you
> >>> take values from hw_pattern file and put them into pattern file, you
> >>> should get the same pattern (with more power being consumed). And that
> >>> property is kind of important for me, because it should keep the ABI
> >>> reasonable.
> >>
> >> If you looked at what we agreed on with Baolin, you'd realize
> >> that this property is in no way preserved.
> >> This is what the whole story is about - we're introducing hw_pattern
> >> because of difficulties in describing breathing pattern by a series
> >> of [brightness delta_t] tuples.
> >>
> >> And Bjorn presented another example. I'm inclined to leave the
> >> definition of hw_pattern semantics to the LED class drivers,
> >> and allow them to create related sysfs files.
> > 
> > Please lets not do that.
> > 
> > We already have drivers that do that and it is complete
> > nightmare. Some take binary code for the tiny CPU driving the LED.
> > 
> > What exactly is the problem? [brightness delta_t] can describe
> 
> You wrote:
> 
> <quote>
> Yes, hw_pattern can have some limitation pattern does not, but if you
> take values from hw_pattern file and put them into pattern file, you
> should get the same pattern (with more power being consumed).
> </quote>
> 
> The problem is that we decided to introduce hw_pattern to
> to take away from drivers a responsibility for translating
> a series of tuples, approximating the brightness curve,
> to the values that can be written to the hardware registers.
> 
> Because this is what would need to be done to check if hw can support
> given series of tuples and activate it. Actually with this approach
> we wouldn't need hw_pattern at all, since pattern alone would do.
> But implementation thereof is what we could call a nightmare.
> 
> What follows, your claim from the quotation is inaccurate:
> values from hw_pattern written to the pattern file will not
> produce the same pattern, at least in case of what was proposed
> in [0] for drivers/leds/leds-sc27xx-bltc.c.

That sounds easy, see below.

> > anything single LED can do in finite time. You are right, that
> > [brightness delta_t] sequence may get rather long, and it may be hard
> > to turn that sequence into parameters. Are there any _interesting_
> > sequences hardware can do but [brightness delta_t] can not store
> > reasonably?
> 
> Please propose the implementation of pattern_set for
> drivers/leds/leds-sc27xx-bltc.c breathing pattern, that will
> setup breathing mode basing on the values from tuples.
> 
> Use Baolin's patch [0] for a reference of what hardware expects.
> 
> [0] https://lore.kernel.org/patchwork/patch/984246/

Yep, so we change documentation to require

0 rise_duration brightness high_duration brightness fall_duration 0 low_duration"

...and we are done; at least as long as user writes expected pattern
to the file.

I'd actually like to see this at begining of function:
    if (pattern[0].brightness != 0)
        return -EINVAL;
    if (pattern[2].brightness != 0)
        return -EINVAL;
    if (pattern[3].brightness != 0)
        return -EINVAL;
    if (pattern[1].brightness != pattern[2].brightness)
        return -EINVAL;

..so if user writes something unexpected, he gets the error back.

What am I missing?

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

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

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

[Jacek Anaszewski]

On 09/12/2018 10:41 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> No, we are not back to full circle.
>>>>>
>>>>> Or at least we should not be.
>>>>>
>>>>> Yes, hw_pattern can have some limitation pattern does not, but if you
>>>>> take values from hw_pattern file and put them into pattern file, you
>>>>> should get the same pattern (with more power being consumed). And that
>>>>> property is kind of important for me, because it should keep the ABI
>>>>> reasonable.
>>>>
>>>> If you looked at what we agreed on with Baolin, you'd realize
>>>> that this property is in no way preserved.
>>>> This is what the whole story is about - we're introducing hw_pattern
>>>> because of difficulties in describing breathing pattern by a series
>>>> of [brightness delta_t] tuples.
>>>>
>>>> And Bjorn presented another example. I'm inclined to leave the
>>>> definition of hw_pattern semantics to the LED class drivers,
>>>> and allow them to create related sysfs files.
>>>
>>> Please lets not do that.
>>>
>>> We already have drivers that do that and it is complete
>>> nightmare. Some take binary code for the tiny CPU driving the LED.
>>>
>>> What exactly is the problem? [brightness delta_t] can describe
>>
>> You wrote:
>>
>> <quote>
>> Yes, hw_pattern can have some limitation pattern does not, but if you
>> take values from hw_pattern file and put them into pattern file, you
>> should get the same pattern (with more power being consumed).
>> </quote>
>>
>> The problem is that we decided to introduce hw_pattern to
>> to take away from drivers a responsibility for translating
>> a series of tuples, approximating the brightness curve,
>> to the values that can be written to the hardware registers.
>>
>> Because this is what would need to be done to check if hw can support
>> given series of tuples and activate it. Actually with this approach
>> we wouldn't need hw_pattern at all, since pattern alone would do.
>> But implementation thereof is what we could call a nightmare.
>>
>> What follows, your claim from the quotation is inaccurate:
>> values from hw_pattern written to the pattern file will not
>> produce the same pattern, at least in case of what was proposed
>> in [0] for drivers/leds/leds-sc27xx-bltc.c.
> 
> That sounds easy, see below.
> 
>>> anything single LED can do in finite time. You are right, that
>>> [brightness delta_t] sequence may get rather long, and it may be hard
>>> to turn that sequence into parameters. Are there any _interesting_
>>> sequences hardware can do but [brightness delta_t] can not store
>>> reasonably?
>>
>> Please propose the implementation of pattern_set for
>> drivers/leds/leds-sc27xx-bltc.c breathing pattern, that will
>> setup breathing mode basing on the values from tuples.
>>
>> Use Baolin's patch [0] for a reference of what hardware expects.
>>
>> [0] https://lore.kernel.org/patchwork/patch/984246/
> 
> Yep, so we change documentation to require
> 
> 0 rise_duration brightness high_duration brightness fall_duration 0 low_duration"
> 
> ...and we are done; at least as long as user writes expected pattern
> to the file.
> 
> I'd actually like to see this at begining of function:
>     if (pattern[0].brightness != 0)
>         return -EINVAL;
>     if (pattern[2].brightness != 0)
>         return -EINVAL;
>     if (pattern[3].brightness != 0)
>         return -EINVAL;
>     if (pattern[1].brightness != pattern[2].brightness)
>         return -EINVAL;
> 
> ..so if user writes something unexpected, he gets the error back.
> 
> What am I missing?

I suppose that breathing pattern means smooth rise and fall of
brightness. I'd even say that it should be a non-linear function.

I'm not sure if you noticed my quotation of your statement from the
previous message, because the modification you proposed doesn't
introduce any novelty to the related matter.

The difference between pattern and discussed hw_pattern
semantics is that:

- hw_pattern semantics for leds-sc27xx-bltc.c requires only four
  tuples to setup the breathing pattern.

- the same four tuples written to the pattern file would result
  in four brightness changes

-- 
Best regards,
Jacek Anaszewski