LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH v3 1/2] leds: core: Introduce generic pattern interface
@ 2018-06-29  5:03 Baolin Wang
  2018-06-29  5:03 ` [PATCH v3 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller Baolin Wang
  2018-07-11 11:02 ` [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang
  0 siblings, 2 replies; 26+ messages in thread
From: Baolin Wang @ 2018-06-29  5:03 UTC (permalink / raw)
  To: jacek.anaszewski, pavel
  Cc: bjorn.andersson, baolin.wang, broonie, linux-leds, linux-kernel

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.

[Baolin Wang did some minor improvements.]

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v2:
 - Change kernel version to 4.19.
 - Force user to return error pointer if failed to issue pattern_get().
 - Use strstrip() to trim trailing newline.
 - Other optimization.

Changes from v1:
 - Add some comments suggested by Pavel.
 - Change 'delta_t' can be 0.

Note: I removed the pattern repeat check and will get the repeat number by adding
one extra file named 'pattern_repeat' according to previous discussion.
---
 Documentation/ABI/testing/sysfs-class-led |   17 +++++
 drivers/leds/led-class.c                  |  119 +++++++++++++++++++++++++++++
 include/linux/leds.h                      |   19 +++++
 3 files changed, 155 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..e01ac55 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,20 @@ Description:
 		gpio and backlight triggers. In case of the backlight trigger,
 		it is useful when driving a LED which is intended to indicate
 		a device in a standby like state.
+
+What: /sys/class/leds/<led>/pattern
+Date: June 2018
+KernelVersion: 4.19
+Description:
+	Specify a pattern for the LED, for LED hardware that support
+	altering the brightness as a function of time.
+
+	The pattern is given by a series of tuples, of brightness and
+	duration (ms). The LED is expected to traverse the series and
+	each brightness value for the specified duration. Duration of
+	0 means brightness should immediately change to new value.
+
+	As LED hardware might have different capabilities and precision
+	the requested pattern might be slighly adjusted by the driver
+	and the resulting pattern of such operation should be returned
+	when this file is read.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..8a685a2 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(max_brightness);
 
+static ssize_t pattern_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct led_pattern *pattern;
+	size_t offset = 0;
+	int count, n, i;
+
+	if (!led_cdev->pattern_get)
+		return -EOPNOTSUPP;
+
+	pattern = led_cdev->pattern_get(led_cdev, &count);
+	if (IS_ERR(pattern))
+		return PTR_ERR(pattern);
+
+	for (i = 0; i < count; i++) {
+		n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
+			     pattern[i].brightness, pattern[i].delta_t);
+
+		if (offset + n >= PAGE_SIZE)
+			goto err_nospc;
+
+		offset += n;
+	}
+
+	buf[offset - 1] = '\n';
+
+	kfree(pattern);
+	return offset;
+
+err_nospc:
+	kfree(pattern);
+	return -ENOSPC;
+}
+
+static ssize_t pattern_store(struct device *dev,
+			     struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct led_pattern *pattern = NULL;
+	unsigned long val;
+	char *sbegin;
+	char *elem;
+	char *s;
+	int ret, len = 0;
+	bool odd = true;
+
+	sbegin = kstrndup(buf, size, GFP_KERNEL);
+	if (!sbegin)
+		return -ENOMEM;
+
+	/*
+	 * Trim trailing newline, if the remaining string is empty,
+	 * clear the pattern.
+	 */
+	s = strstrip(sbegin);
+	if (!*s) {
+		ret = led_cdev->pattern_clear(led_cdev);
+		goto out;
+	}
+
+	pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
+	if (!pattern) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* Parse out the brightness & delta_t touples */
+	while ((elem = strsep(&s, " ")) != NULL) {
+		ret = kstrtoul(elem, 10, &val);
+		if (ret)
+			goto out;
+
+		if (odd) {
+			pattern[len].brightness = val;
+		} else {
+			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 +205,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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller
  2018-06-29  5:03 [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang
@ 2018-06-29  5:03 ` Baolin Wang
  2018-07-11 11:02 ` [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang
  1 sibling, 0 replies; 26+ messages in thread
From: Baolin Wang @ 2018-06-29  5:03 UTC (permalink / raw)
  To: jacek.anaszewski, pavel
  Cc: bjorn.andersson, baolin.wang, broonie, linux-leds, linux-kernel

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>
---
Changes from v2:
 - No updates.

 Changes from v1:
 - No updates.
---
 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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-06-29  5:03 [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang
  2018-06-29  5:03 ` [PATCH v3 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller Baolin Wang
@ 2018-07-11 11:02 ` Baolin Wang
  2018-07-11 21:10   ` Jacek Anaszewski
  1 sibling, 1 reply; 26+ messages in thread
From: Baolin Wang @ 2018-07-11 11:02 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: Bjorn Andersson, Baolin Wang, Mark Brown, Linux LED Subsystem, LKML

Hi Jacek and Pavel,

On 29 June 2018 at 13:03, Baolin Wang <baolin.wang@linaro.org> wrote:
> 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.
>
> [Baolin Wang did some minor improvements.]
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v2:
>  - Change kernel version to 4.19.
>  - Force user to return error pointer if failed to issue pattern_get().
>  - Use strstrip() to trim trailing newline.
>  - Other optimization.
>
> Changes from v1:
>  - Add some comments suggested by Pavel.
>  - Change 'delta_t' can be 0.
>
> Note: I removed the pattern repeat check and will get the repeat number by adding
> one extra file named 'pattern_repeat' according to previous discussion.
> ---

Do you have any comments for this version patch set? Thanks.

>  Documentation/ABI/testing/sysfs-class-led |   17 +++++
>  drivers/leds/led-class.c                  |  119 +++++++++++++++++++++++++++++
>  include/linux/leds.h                      |   19 +++++
>  3 files changed, 155 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..e01ac55 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,20 @@ Description:
>                 gpio and backlight triggers. In case of the backlight trigger,
>                 it is useful when driving a LED which is intended to indicate
>                 a device in a standby like state.
> +
> +What: /sys/class/leds/<led>/pattern
> +Date: June 2018
> +KernelVersion: 4.19
> +Description:
> +       Specify a pattern for the LED, for LED hardware that support
> +       altering the brightness as a function of time.
> +
> +       The pattern is given by a series of tuples, of brightness and
> +       duration (ms). The LED is expected to traverse the series and
> +       each brightness value for the specified duration. Duration of
> +       0 means brightness should immediately change to new value.
> +
> +       As LED hardware might have different capabilities and precision
> +       the requested pattern might be slighly adjusted by the driver
> +       and the resulting pattern of such operation should be returned
> +       when this file is read.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3c7e348..8a685a2 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(max_brightness);
>
> +static ssize_t pattern_show(struct device *dev,
> +                           struct device_attribute *attr, char *buf)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       struct led_pattern *pattern;
> +       size_t offset = 0;
> +       int count, n, i;
> +
> +       if (!led_cdev->pattern_get)
> +               return -EOPNOTSUPP;
> +
> +       pattern = led_cdev->pattern_get(led_cdev, &count);
> +       if (IS_ERR(pattern))
> +               return PTR_ERR(pattern);
> +
> +       for (i = 0; i < count; i++) {
> +               n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
> +                            pattern[i].brightness, pattern[i].delta_t);
> +
> +               if (offset + n >= PAGE_SIZE)
> +                       goto err_nospc;
> +
> +               offset += n;
> +       }
> +
> +       buf[offset - 1] = '\n';
> +
> +       kfree(pattern);
> +       return offset;
> +
> +err_nospc:
> +       kfree(pattern);
> +       return -ENOSPC;
> +}
> +
> +static ssize_t pattern_store(struct device *dev,
> +                            struct device_attribute *attr,
> +                            const char *buf, size_t size)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       struct led_pattern *pattern = NULL;
> +       unsigned long val;
> +       char *sbegin;
> +       char *elem;
> +       char *s;
> +       int ret, len = 0;
> +       bool odd = true;
> +
> +       sbegin = kstrndup(buf, size, GFP_KERNEL);
> +       if (!sbegin)
> +               return -ENOMEM;
> +
> +       /*
> +        * Trim trailing newline, if the remaining string is empty,
> +        * clear the pattern.
> +        */
> +       s = strstrip(sbegin);
> +       if (!*s) {
> +               ret = led_cdev->pattern_clear(led_cdev);
> +               goto out;
> +       }
> +
> +       pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
> +       if (!pattern) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       /* Parse out the brightness & delta_t touples */
> +       while ((elem = strsep(&s, " ")) != NULL) {
> +               ret = kstrtoul(elem, 10, &val);
> +               if (ret)
> +                       goto out;
> +
> +               if (odd) {
> +                       pattern[len].brightness = val;
> +               } else {
> +                       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 +205,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
>



-- 
Baolin Wang
Best Regards

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-11 11:02 ` [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang
@ 2018-07-11 21:10   ` Jacek Anaszewski
  2018-07-12 12:24     ` Baolin Wang
  0 siblings, 1 reply; 26+ messages in thread
From: Jacek Anaszewski @ 2018-07-11 21:10 UTC (permalink / raw)
  To: Baolin Wang, Pavel Machek
  Cc: Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

Hi Baolin.

On 07/11/2018 01:02 PM, Baolin Wang wrote:
> Hi Jacek and Pavel,
> 
> On 29 June 2018 at 13:03, Baolin Wang <baolin.wang@linaro.org> wrote:
>> 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.
>>
>> [Baolin Wang did some minor improvements.]
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v2:
>>   - Change kernel version to 4.19.
>>   - Force user to return error pointer if failed to issue pattern_get().
>>   - Use strstrip() to trim trailing newline.
>>   - Other optimization.
>>
>> Changes from v1:
>>   - Add some comments suggested by Pavel.
>>   - Change 'delta_t' can be 0.
>>
>> Note: I removed the pattern repeat check and will get the repeat number by adding
>> one extra file named 'pattern_repeat' according to previous discussion.
>> ---
> 
> Do you have any comments for this version patch set? Thanks.

I tried modifying uleds.c driver to support pattern ops, but
I'm getting segfault when doing "cat pattern". I didn't give
it serious testing and analysis - will do it at weekend.

It also drew my attention to the issue of desired pattern sysfs
interface semantics on uninitialized pattern. In your implementation
user seems to be unable to determine if the pattern is activated
or not. We should define the semantics for this use case and
describe it in the documentation. Possibly pattern could
return alone new line character then.

This is the code snippet I've used for testing pattern interface:

static struct led_pattern ptrn[10];
static int ptrn_len;

static int uled_pattern_clear(struct led_classdev *ldev)
{
	return 0;
}

static int uled_pattern_set(struct led_classdev *ldev,
				  struct led_pattern *pattern,
				  int len)
{
	int i;

	for (i = 0; i < len; i++) {
		ptrn[i].brightness = pattern[i].brightness;
		ptrn[i].delta_t = pattern[i].delta_t;
	}

	ptrn_len = len;

	return 0;
}

static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
						  int *len)
{
	int i;

	for (i = 0; i < ptrn_len; i++) {
		ptrn[i].brightness = 3;
		ptrn[i].delta_t = 5;
	}

	*len = ptrn_len;

	return ptrn;
}


> 
>>   Documentation/ABI/testing/sysfs-class-led |   17 +++++
>>   drivers/leds/led-class.c                  |  119 +++++++++++++++++++++++++++++
>>   include/linux/leds.h                      |   19 +++++
>>   3 files changed, 155 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
>> index 5f67f7a..e01ac55 100644
>> --- a/Documentation/ABI/testing/sysfs-class-led
>> +++ b/Documentation/ABI/testing/sysfs-class-led
>> @@ -61,3 +61,20 @@ Description:
>>                  gpio and backlight triggers. In case of the backlight trigger,
>>                  it is useful when driving a LED which is intended to indicate
>>                  a device in a standby like state.
>> +
>> +What: /sys/class/leds/<led>/pattern
>> +Date: June 2018
>> +KernelVersion: 4.19
>> +Description:
>> +       Specify a pattern for the LED, for LED hardware that support
>> +       altering the brightness as a function of time.
>> +
>> +       The pattern is given by a series of tuples, of brightness and
>> +       duration (ms). The LED is expected to traverse the series and
>> +       each brightness value for the specified duration. Duration of
>> +       0 means brightness should immediately change to new value.
>> +
>> +       As LED hardware might have different capabilities and precision
>> +       the requested pattern might be slighly adjusted by the driver
>> +       and the resulting pattern of such operation should be returned
>> +       when this file is read.
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 3c7e348..8a685a2 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device *dev,
>>   }
>>   static DEVICE_ATTR_RO(max_brightness);
>>
>> +static ssize_t pattern_show(struct device *dev,
>> +                           struct device_attribute *attr, char *buf)
>> +{
>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +       struct led_pattern *pattern;
>> +       size_t offset = 0;
>> +       int count, n, i;
>> +
>> +       if (!led_cdev->pattern_get)
>> +               return -EOPNOTSUPP;

Please check this in led_classdev_register() and fail
if pattern_set is initialized, but pattern_get or pattern_clear not.

>> +       pattern = led_cdev->pattern_get(led_cdev, &count);
>> +       if (IS_ERR(pattern))
>> +               return PTR_ERR(pattern);
>> +
>> +       for (i = 0; i < count; i++) {
>> +               n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
>> +                            pattern[i].brightness, pattern[i].delta_t);
>> +
>> +               if (offset + n >= PAGE_SIZE)
>> +                       goto err_nospc;
>> +
>> +               offset += n;
>> +       }
>> +
>> +       buf[offset - 1] = '\n';
>> +
>> +       kfree(pattern);
>> +       return offset;
>> +
>> +err_nospc:
>> +       kfree(pattern);
>> +       return -ENOSPC;
>> +}
>> +
>> +static ssize_t pattern_store(struct device *dev,
>> +                            struct device_attribute *attr,
>> +                            const char *buf, size_t size)
>> +{
>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +       struct led_pattern *pattern = NULL;
>> +       unsigned long val;
>> +       char *sbegin;
>> +       char *elem;
>> +       char *s;
>> +       int ret, len = 0;
>> +       bool odd = true;
>> +
>> +       sbegin = kstrndup(buf, size, GFP_KERNEL);
>> +       if (!sbegin)
>> +               return -ENOMEM;
>> +
>> +       /*
>> +        * Trim trailing newline, if the remaining string is empty,
>> +        * clear the pattern.
>> +        */
>> +       s = strstrip(sbegin);
>> +       if (!*s) {
>> +               ret = led_cdev->pattern_clear(led_cdev);
>> +               goto out;
>> +       }
>> +
>> +       pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
>> +       if (!pattern) {
>> +               ret = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       /* Parse out the brightness & delta_t touples */
>> +       while ((elem = strsep(&s, " ")) != NULL) {
>> +               ret = kstrtoul(elem, 10, &val);
>> +               if (ret)
>> +                       goto out;
>> +
>> +               if (odd) {
>> +                       pattern[len].brightness = val;
>> +               } else {
>> +                       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 +205,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,

This should not be needed if we'll validate pattern ops
in led_classdev_register().

>>   };
>>
>>   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
>>
> 
> 
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-11 21:10   ` Jacek Anaszewski
@ 2018-07-12 12:24     ` Baolin Wang
  2018-07-12 21:41       ` Jacek Anaszewski
  2018-07-14 21:20       ` Pavel Machek
  0 siblings, 2 replies; 26+ messages in thread
From: Baolin Wang @ 2018-07-12 12:24 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

Hi Jacek,

On 12 July 2018 at 05:10, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> Hi Baolin.
>
>
> On 07/11/2018 01:02 PM, Baolin Wang wrote:
>>
>> Hi Jacek and Pavel,
>>
>> On 29 June 2018 at 13:03, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>
>>> 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.
>>>
>>> [Baolin Wang did some minor improvements.]
>>>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>> Changes from v2:
>>>   - Change kernel version to 4.19.
>>>   - Force user to return error pointer if failed to issue pattern_get().
>>>   - Use strstrip() to trim trailing newline.
>>>   - Other optimization.
>>>
>>> Changes from v1:
>>>   - Add some comments suggested by Pavel.
>>>   - Change 'delta_t' can be 0.
>>>
>>> Note: I removed the pattern repeat check and will get the repeat number
>>> by adding
>>> one extra file named 'pattern_repeat' according to previous discussion.
>>> ---
>>
>>
>> Do you have any comments for this version patch set? Thanks.
>
>
> I tried modifying uleds.c driver to support pattern ops, but
> I'm getting segfault when doing "cat pattern". I didn't give
> it serious testing and analysis - will do it at weekend.
>
> It also drew my attention to the issue of desired pattern sysfs
> interface semantics on uninitialized pattern. In your implementation
> user seems to be unable to determine if the pattern is activated
> or not. We should define the semantics for this use case and
> describe it in the documentation. Possibly pattern could
> return alone new line character then.

I am not sure I get your points correctly. If user writes values to
pattern interface which means we activated the pattern.
If I am wrong, could you elaborate on the issue you concerned? Thanks.

>
> This is the code snippet I've used for testing pattern interface:
>
> static struct led_pattern ptrn[10];
> static int ptrn_len;
>
> static int uled_pattern_clear(struct led_classdev *ldev)
> {
>         return 0;
> }
>
> static int uled_pattern_set(struct led_classdev *ldev,
>                                   struct led_pattern *pattern,
>                                   int len)
> {
>         int i;
>
>         for (i = 0; i < len; i++) {
>                 ptrn[i].brightness = pattern[i].brightness;
>                 ptrn[i].delta_t = pattern[i].delta_t;
>         }
>
>         ptrn_len = len;
>
>         return 0;
> }
>
> static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
>                                                   int *len)
> {
>         int i;
>
>         for (i = 0; i < ptrn_len; i++) {
>                 ptrn[i].brightness = 3;
>                 ptrn[i].delta_t = 5;
>         }
>
>         *len = ptrn_len;
>
>         return ptrn;
>
> }

The reason you met segfault when doing "cat pattern" is you should not
return one static pattern array, since in pattern_show() it will help
to free the pattern memory, could you change to return one pattern
pointer with dynamic allocation like my patch 2?

>>
>>>   Documentation/ABI/testing/sysfs-class-led |   17 +++++
>>>   drivers/leds/led-class.c                  |  119
>>> +++++++++++++++++++++++++++++
>>>   include/linux/leds.h                      |   19 +++++
>>>   3 files changed, 155 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>>> b/Documentation/ABI/testing/sysfs-class-led
>>> index 5f67f7a..e01ac55 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>> @@ -61,3 +61,20 @@ Description:
>>>                  gpio and backlight triggers. In case of the backlight
>>> trigger,
>>>                  it is useful when driving a LED which is intended to
>>> indicate
>>>                  a device in a standby like state.
>>> +
>>> +What: /sys/class/leds/<led>/pattern
>>> +Date: June 2018
>>> +KernelVersion: 4.19
>>> +Description:
>>> +       Specify a pattern for the LED, for LED hardware that support
>>> +       altering the brightness as a function of time.
>>> +
>>> +       The pattern is given by a series of tuples, of brightness and
>>> +       duration (ms). The LED is expected to traverse the series and
>>> +       each brightness value for the specified duration. Duration of
>>> +       0 means brightness should immediately change to new value.
>>> +
>>> +       As LED hardware might have different capabilities and precision
>>> +       the requested pattern might be slighly adjusted by the driver
>>> +       and the resulting pattern of such operation should be returned
>>> +       when this file is read.
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 3c7e348..8a685a2 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device
>>> *dev,
>>>   }
>>>   static DEVICE_ATTR_RO(max_brightness);
>>>
>>> +static ssize_t pattern_show(struct device *dev,
>>> +                           struct device_attribute *attr, char *buf)
>>> +{
>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> +       struct led_pattern *pattern;
>>> +       size_t offset = 0;
>>> +       int count, n, i;
>>> +
>>> +       if (!led_cdev->pattern_get)
>>> +               return -EOPNOTSUPP;
>
>
> Please check this in led_classdev_register() and fail
> if pattern_set is initialized, but pattern_get or pattern_clear not.

Sure.

>>> +       pattern = led_cdev->pattern_get(led_cdev, &count);
>>> +       if (IS_ERR(pattern))
>>> +               return PTR_ERR(pattern);
>>> +
>>> +       for (i = 0; i < count; i++) {
>>> +               n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
>>> +                            pattern[i].brightness, pattern[i].delta_t);
>>> +
>>> +               if (offset + n >= PAGE_SIZE)
>>> +                       goto err_nospc;
>>> +
>>> +               offset += n;
>>> +       }
>>> +
>>> +       buf[offset - 1] = '\n';
>>> +
>>> +       kfree(pattern);
>>> +       return offset;
>>> +
>>> +err_nospc:
>>> +       kfree(pattern);
>>> +       return -ENOSPC;
>>> +}
>>> +
>>> +static ssize_t pattern_store(struct device *dev,
>>> +                            struct device_attribute *attr,
>>> +                            const char *buf, size_t size)
>>> +{
>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> +       struct led_pattern *pattern = NULL;
>>> +       unsigned long val;
>>> +       char *sbegin;
>>> +       char *elem;
>>> +       char *s;
>>> +       int ret, len = 0;
>>> +       bool odd = true;
>>> +
>>> +       sbegin = kstrndup(buf, size, GFP_KERNEL);
>>> +       if (!sbegin)
>>> +               return -ENOMEM;
>>> +
>>> +       /*
>>> +        * Trim trailing newline, if the remaining string is empty,
>>> +        * clear the pattern.
>>> +        */
>>> +       s = strstrip(sbegin);
>>> +       if (!*s) {
>>> +               ret = led_cdev->pattern_clear(led_cdev);
>>> +               goto out;
>>> +       }
>>> +
>>> +       pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
>>> +       if (!pattern) {
>>> +               ret = -ENOMEM;
>>> +               goto out;
>>> +       }
>>> +
>>> +       /* Parse out the brightness & delta_t touples */
>>> +       while ((elem = strsep(&s, " ")) != NULL) {
>>> +               ret = kstrtoul(elem, 10, &val);
>>> +               if (ret)
>>> +                       goto out;
>>> +
>>> +               if (odd) {
>>> +                       pattern[len].brightness = val;
>>> +               } else {
>>> +                       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 +205,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,
>
>
> This should not be needed if we'll validate pattern ops
> in led_classdev_register().

I am afraid we can not remove it. If the driver did not supply
pattern_set/get/clear, we should not still show the pattern interfaces
for userspace. Or am I missed anything?

Thanks for your comments.

-- 
Baolin Wang
Best Regards

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-12 12:24     ` Baolin Wang
@ 2018-07-12 21:41       ` Jacek Anaszewski
  2018-07-13  1:58         ` Baolin Wang
  2018-07-14 21:20       ` Pavel Machek
  1 sibling, 1 reply; 26+ messages in thread
From: Jacek Anaszewski @ 2018-07-12 21:41 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

Hi Baolin,

On 07/12/2018 02:24 PM, Baolin Wang wrote:
> Hi Jacek,
> 
> On 12 July 2018 at 05:10, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>> Hi Baolin.
>>
>>
>> On 07/11/2018 01:02 PM, Baolin Wang wrote:
>>>
>>> Hi Jacek and Pavel,
>>>
>>> On 29 June 2018 at 13:03, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>
>>>> 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.
>>>>
>>>> [Baolin Wang did some minor improvements.]
>>>>
>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>> Changes from v2:
>>>>    - Change kernel version to 4.19.
>>>>    - Force user to return error pointer if failed to issue pattern_get().
>>>>    - Use strstrip() to trim trailing newline.
>>>>    - Other optimization.
>>>>
>>>> Changes from v1:
>>>>    - Add some comments suggested by Pavel.
>>>>    - Change 'delta_t' can be 0.
>>>>
>>>> Note: I removed the pattern repeat check and will get the repeat number
>>>> by adding
>>>> one extra file named 'pattern_repeat' according to previous discussion.
>>>> ---
>>>
>>>
>>> Do you have any comments for this version patch set? Thanks.
>>
>>
>> I tried modifying uleds.c driver to support pattern ops, but
>> I'm getting segfault when doing "cat pattern". I didn't give
>> it serious testing and analysis - will do it at weekend.
>>
>> It also drew my attention to the issue of desired pattern sysfs
>> interface semantics on uninitialized pattern. In your implementation
>> user seems to be unable to determine if the pattern is activated
>> or not. We should define the semantics for this use case and
>> describe it in the documentation. Possibly pattern could
>> return alone new line character then.
> 
> I am not sure I get your points correctly. If user writes values to
> pattern interface which means we activated the pattern.
> If I am wrong, could you elaborate on the issue you concerned? Thanks.

Now I see, that writing empty string disables the pattern, right?
It should be explicitly stated in the pattern file documentation.

>> This is the code snippet I've used for testing pattern interface:
>>
>> static struct led_pattern ptrn[10];
>> static int ptrn_len;
>>
>> static int uled_pattern_clear(struct led_classdev *ldev)
>> {
>>          return 0;
>> }
>>
>> static int uled_pattern_set(struct led_classdev *ldev,
>>                                    struct led_pattern *pattern,
>>                                    int len)
>> {
>>          int i;
>>
>>          for (i = 0; i < len; i++) {
>>                  ptrn[i].brightness = pattern[i].brightness;
>>                  ptrn[i].delta_t = pattern[i].delta_t;
>>          }
>>
>>          ptrn_len = len;
>>
>>          return 0;
>> }
>>
>> static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
>>                                                    int *len)
>> {
>>          int i;
>>
>>          for (i = 0; i < ptrn_len; i++) {
>>                  ptrn[i].brightness = 3;
>>                  ptrn[i].delta_t = 5;
>>          }
>>
>>          *len = ptrn_len;
>>
>>          return ptrn;
>>
>> }
> 
> The reason you met segfault when doing "cat pattern" is you should not
> return one static pattern array, since in pattern_show() it will help
> to free the pattern memory, could you change to return one pattern
> pointer with dynamic allocation like my patch 2?

Thanks for pointing this out.

>>>>    Documentation/ABI/testing/sysfs-class-led |   17 +++++
>>>>    drivers/leds/led-class.c                  |  119
>>>> +++++++++++++++++++++++++++++
>>>>    include/linux/leds.h                      |   19 +++++
>>>>    3 files changed, 155 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>>>> b/Documentation/ABI/testing/sysfs-class-led
>>>> index 5f67f7a..e01ac55 100644
>>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>>> @@ -61,3 +61,20 @@ Description:
>>>>                   gpio and backlight triggers. In case of the backlight
>>>> trigger,
>>>>                   it is useful when driving a LED which is intended to
>>>> indicate
>>>>                   a device in a standby like state.
>>>> +
>>>> +What: /sys/class/leds/<led>/pattern
>>>> +Date: June 2018
>>>> +KernelVersion: 4.19
>>>> +Description:
>>>> +       Specify a pattern for the LED, for LED hardware that support
>>>> +       altering the brightness as a function of time.
>>>> +
>>>> +       The pattern is given by a series of tuples, of brightness and
>>>> +       duration (ms). The LED is expected to traverse the series and
>>>> +       each brightness value for the specified duration. Duration of
>>>> +       0 means brightness should immediately change to new value.
>>>> +
>>>> +       As LED hardware might have different capabilities and precision
>>>> +       the requested pattern might be slighly adjusted by the driver
>>>> +       and the resulting pattern of such operation should be returned
>>>> +       when this file is read.
>>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>>> index 3c7e348..8a685a2 100644
>>>> --- a/drivers/leds/led-class.c
>>>> +++ b/drivers/leds/led-class.c
>>>> @@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device
>>>> *dev,
>>>>    }
>>>>    static DEVICE_ATTR_RO(max_brightness);
>>>>
>>>> +static ssize_t pattern_show(struct device *dev,
>>>> +                           struct device_attribute *attr, char *buf)
>>>> +{
>>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>> +       struct led_pattern *pattern;
>>>> +       size_t offset = 0;
>>>> +       int count, n, i;
>>>> +
>>>> +       if (!led_cdev->pattern_get)
>>>> +               return -EOPNOTSUPP;
>>
>>
>> Please check this in led_classdev_register() and fail
>> if pattern_set is initialized, but pattern_get or pattern_clear not.
> 
> Sure.
> 
>>>> +       pattern = led_cdev->pattern_get(led_cdev, &count);
>>>> +       if (IS_ERR(pattern))
>>>> +               return PTR_ERR(pattern);
>>>> +
>>>> +       for (i = 0; i < count; i++) {
>>>> +               n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
>>>> +                            pattern[i].brightness, pattern[i].delta_t);
>>>> +
>>>> +               if (offset + n >= PAGE_SIZE)
>>>> +                       goto err_nospc;
>>>> +
>>>> +               offset += n;
>>>> +       }
>>>> +
>>>> +       buf[offset - 1] = '\n';
>>>> +
>>>> +       kfree(pattern);
>>>> +       return offset;
>>>> +
>>>> +err_nospc:
>>>> +       kfree(pattern);
>>>> +       return -ENOSPC;
>>>> +}
>>>> +
>>>> +static ssize_t pattern_store(struct device *dev,
>>>> +                            struct device_attribute *attr,
>>>> +                            const char *buf, size_t size)
>>>> +{
>>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>> +       struct led_pattern *pattern = NULL;
>>>> +       unsigned long val;
>>>> +       char *sbegin;
>>>> +       char *elem;
>>>> +       char *s;
>>>> +       int ret, len = 0;
>>>> +       bool odd = true;
>>>> +
>>>> +       sbegin = kstrndup(buf, size, GFP_KERNEL);
>>>> +       if (!sbegin)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       /*
>>>> +        * Trim trailing newline, if the remaining string is empty,
>>>> +        * clear the pattern.
>>>> +        */
>>>> +       s = strstrip(sbegin);
>>>> +       if (!*s) {
>>>> +               ret = led_cdev->pattern_clear(led_cdev);
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
>>>> +       if (!pattern) {
>>>> +               ret = -ENOMEM;
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       /* Parse out the brightness & delta_t touples */
>>>> +       while ((elem = strsep(&s, " ")) != NULL) {
>>>> +               ret = kstrtoul(elem, 10, &val);
>>>> +               if (ret)
>>>> +                       goto out;
>>>> +
>>>> +               if (odd) {
>>>> +                       pattern[len].brightness = val;
>>>> +               } else {
>>>> +                       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 +205,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,
>>
>>
>> This should not be needed if we'll validate pattern ops
>> in led_classdev_register().
> 
> I am afraid we can not remove it. If the driver did not supply
> pattern_set/get/clear, we should not still show the pattern interfaces
> for userspace.

You're right.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-12 21:41       ` Jacek Anaszewski
@ 2018-07-13  1:58         ` Baolin Wang
  0 siblings, 0 replies; 26+ messages in thread
From: Baolin Wang @ 2018-07-13  1:58 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

Hi Jacek,

On 13 July 2018 at 05:41, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> Hi Baolin,
>
>
> On 07/12/2018 02:24 PM, Baolin Wang wrote:
>>
>> Hi Jacek,
>>
>> On 12 July 2018 at 05:10, Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> wrote:
>>>
>>> Hi Baolin.
>>>
>>>
>>> On 07/11/2018 01:02 PM, Baolin Wang wrote:
>>>>
>>>>
>>>> Hi Jacek and Pavel,
>>>>
>>>> On 29 June 2018 at 13:03, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>>
>>>>>
>>>>> 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.
>>>>>
>>>>> [Baolin Wang did some minor improvements.]
>>>>>
>>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>> ---
>>>>> Changes from v2:
>>>>>    - Change kernel version to 4.19.
>>>>>    - Force user to return error pointer if failed to issue
>>>>> pattern_get().
>>>>>    - Use strstrip() to trim trailing newline.
>>>>>    - Other optimization.
>>>>>
>>>>> Changes from v1:
>>>>>    - Add some comments suggested by Pavel.
>>>>>    - Change 'delta_t' can be 0.
>>>>>
>>>>> Note: I removed the pattern repeat check and will get the repeat number
>>>>> by adding
>>>>> one extra file named 'pattern_repeat' according to previous discussion.
>>>>> ---
>>>>
>>>>
>>>>
>>>> Do you have any comments for this version patch set? Thanks.
>>>
>>>
>>>
>>> I tried modifying uleds.c driver to support pattern ops, but
>>> I'm getting segfault when doing "cat pattern". I didn't give
>>> it serious testing and analysis - will do it at weekend.
>>>
>>> It also drew my attention to the issue of desired pattern sysfs
>>> interface semantics on uninitialized pattern. In your implementation
>>> user seems to be unable to determine if the pattern is activated
>>> or not. We should define the semantics for this use case and
>>> describe it in the documentation. Possibly pattern could
>>> return alone new line character then.
>>
>>
>> I am not sure I get your points correctly. If user writes values to
>> pattern interface which means we activated the pattern.
>> If I am wrong, could you elaborate on the issue you concerned? Thanks.
>
>
> Now I see, that writing empty string disables the pattern, right?
> It should be explicitly stated in the pattern file documentation.

Yes, you are right. OK, I will add some documentation for this. Thanks.

>>> This is the code snippet I've used for testing pattern interface:
>>>
>>> static struct led_pattern ptrn[10];
>>> static int ptrn_len;
>>>
>>> static int uled_pattern_clear(struct led_classdev *ldev)
>>> {
>>>          return 0;
>>> }
>>>
>>> static int uled_pattern_set(struct led_classdev *ldev,
>>>                                    struct led_pattern *pattern,
>>>                                    int len)
>>> {
>>>          int i;
>>>
>>>          for (i = 0; i < len; i++) {
>>>                  ptrn[i].brightness = pattern[i].brightness;
>>>                  ptrn[i].delta_t = pattern[i].delta_t;
>>>          }
>>>
>>>          ptrn_len = len;
>>>
>>>          return 0;
>>> }
>>>
>>> static struct led_pattern *uled_pattern_get(struct led_classdev *ldev,
>>>                                                    int *len)
>>> {
>>>          int i;
>>>
>>>          for (i = 0; i < ptrn_len; i++) {
>>>                  ptrn[i].brightness = 3;
>>>                  ptrn[i].delta_t = 5;
>>>          }
>>>
>>>          *len = ptrn_len;
>>>
>>>          return ptrn;
>>>
>>> }
>>
>>
>> The reason you met segfault when doing "cat pattern" is you should not
>> return one static pattern array, since in pattern_show() it will help
>> to free the pattern memory, could you change to return one pattern
>> pointer with dynamic allocation like my patch 2?
>
>
> Thanks for pointing this out.
>
>
>>>>>    Documentation/ABI/testing/sysfs-class-led |   17 +++++
>>>>>    drivers/leds/led-class.c                  |  119
>>>>> +++++++++++++++++++++++++++++
>>>>>    include/linux/leds.h                      |   19 +++++
>>>>>    3 files changed, 155 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>>>>> b/Documentation/ABI/testing/sysfs-class-led
>>>>> index 5f67f7a..e01ac55 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>>>> @@ -61,3 +61,20 @@ Description:
>>>>>                   gpio and backlight triggers. In case of the backlight
>>>>> trigger,
>>>>>                   it is useful when driving a LED which is intended to
>>>>> indicate
>>>>>                   a device in a standby like state.
>>>>> +
>>>>> +What: /sys/class/leds/<led>/pattern
>>>>> +Date: June 2018
>>>>> +KernelVersion: 4.19
>>>>> +Description:
>>>>> +       Specify a pattern for the LED, for LED hardware that support
>>>>> +       altering the brightness as a function of time.
>>>>> +
>>>>> +       The pattern is given by a series of tuples, of brightness and
>>>>> +       duration (ms). The LED is expected to traverse the series and
>>>>> +       each brightness value for the specified duration. Duration of
>>>>> +       0 means brightness should immediately change to new value.
>>>>> +
>>>>> +       As LED hardware might have different capabilities and precision
>>>>> +       the requested pattern might be slighly adjusted by the driver
>>>>> +       and the resulting pattern of such operation should be returned
>>>>> +       when this file is read.
>>>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>>>> index 3c7e348..8a685a2 100644
>>>>> --- a/drivers/leds/led-class.c
>>>>> +++ b/drivers/leds/led-class.c
>>>>> @@ -74,6 +74,123 @@ static ssize_t max_brightness_show(struct device
>>>>> *dev,
>>>>>    }
>>>>>    static DEVICE_ATTR_RO(max_brightness);
>>>>>
>>>>> +static ssize_t pattern_show(struct device *dev,
>>>>> +                           struct device_attribute *attr, char *buf)
>>>>> +{
>>>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>>> +       struct led_pattern *pattern;
>>>>> +       size_t offset = 0;
>>>>> +       int count, n, i;
>>>>> +
>>>>> +       if (!led_cdev->pattern_get)
>>>>> +               return -EOPNOTSUPP;
>>>
>>>
>>>
>>> Please check this in led_classdev_register() and fail
>>> if pattern_set is initialized, but pattern_get or pattern_clear not.
>>
>>
>> Sure.
>>
>>>>> +       pattern = led_cdev->pattern_get(led_cdev, &count);
>>>>> +       if (IS_ERR(pattern))
>>>>> +               return PTR_ERR(pattern);
>>>>> +
>>>>> +       for (i = 0; i < count; i++) {
>>>>> +               n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d
>>>>> ",
>>>>> +                            pattern[i].brightness,
>>>>> pattern[i].delta_t);
>>>>> +
>>>>> +               if (offset + n >= PAGE_SIZE)
>>>>> +                       goto err_nospc;
>>>>> +
>>>>> +               offset += n;
>>>>> +       }
>>>>> +
>>>>> +       buf[offset - 1] = '\n';
>>>>> +
>>>>> +       kfree(pattern);
>>>>> +       return offset;
>>>>> +
>>>>> +err_nospc:
>>>>> +       kfree(pattern);
>>>>> +       return -ENOSPC;
>>>>> +}
>>>>> +
>>>>> +static ssize_t pattern_store(struct device *dev,
>>>>> +                            struct device_attribute *attr,
>>>>> +                            const char *buf, size_t size)
>>>>> +{
>>>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>>> +       struct led_pattern *pattern = NULL;
>>>>> +       unsigned long val;
>>>>> +       char *sbegin;
>>>>> +       char *elem;
>>>>> +       char *s;
>>>>> +       int ret, len = 0;
>>>>> +       bool odd = true;
>>>>> +
>>>>> +       sbegin = kstrndup(buf, size, GFP_KERNEL);
>>>>> +       if (!sbegin)
>>>>> +               return -ENOMEM;
>>>>> +
>>>>> +       /*
>>>>> +        * Trim trailing newline, if the remaining string is empty,
>>>>> +        * clear the pattern.
>>>>> +        */
>>>>> +       s = strstrip(sbegin);
>>>>> +       if (!*s) {
>>>>> +               ret = led_cdev->pattern_clear(led_cdev);
>>>>> +               goto out;
>>>>> +       }
>>>>> +
>>>>> +       pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
>>>>> +       if (!pattern) {
>>>>> +               ret = -ENOMEM;
>>>>> +               goto out;
>>>>> +       }
>>>>> +
>>>>> +       /* Parse out the brightness & delta_t touples */
>>>>> +       while ((elem = strsep(&s, " ")) != NULL) {
>>>>> +               ret = kstrtoul(elem, 10, &val);
>>>>> +               if (ret)
>>>>> +                       goto out;
>>>>> +
>>>>> +               if (odd) {
>>>>> +                       pattern[len].brightness = val;
>>>>> +               } else {
>>>>> +                       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 +205,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,
>>>
>>>
>>>
>>> This should not be needed if we'll validate pattern ops
>>> in led_classdev_register().
>>
>>
>> I am afraid we can not remove it. If the driver did not supply
>> pattern_set/get/clear, we should not still show the pattern interfaces
>> for userspace.
>
>
> You're right.
>
>
> --
> Best regards,
> Jacek Anaszewski



-- 
Baolin Wang
Best Regards

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-12 12:24     ` Baolin Wang
  2018-07-12 21:41       ` Jacek Anaszewski
@ 2018-07-14 21:20       ` Pavel Machek
  2018-07-14 22:02         ` Jacek Anaszewski
  1 sibling, 1 reply; 26+ messages in thread
From: Pavel Machek @ 2018-07-14 21:20 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Jacek Anaszewski, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

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

Hi!

> > It also drew my attention to the issue of desired pattern sysfs
> > interface semantics on uninitialized pattern. In your implementation
> > user seems to be unable to determine if the pattern is activated
> > or not. We should define the semantics for this use case and
> > describe it in the documentation. Possibly pattern could
> > return alone new line character then.

Let me take a step back: we have triggers.. like LED blinking.

How is that going to interact with patterns? We probably want the
patterns to be ignored in that case...?

Which suggest to me that we should treat patterns as a trigger. I
believe we do something similar with blinking already.

Then it is easy to determine if pattern is active, and pattern
vs. trigger issue is solved automatically.

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-14 21:20       ` Pavel Machek
@ 2018-07-14 22:02         ` Jacek Anaszewski
  2018-07-14 22:29           ` Pavel Machek
  0 siblings, 1 reply; 26+ messages in thread
From: Jacek Anaszewski @ 2018-07-14 22:02 UTC (permalink / raw)
  To: Pavel Machek, Baolin Wang
  Cc: Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

Hi Pavel,

On 07/14/2018 11:20 PM, Pavel Machek wrote:
> Hi!
> 
>>> It also drew my attention to the issue of desired pattern sysfs
>>> interface semantics on uninitialized pattern. In your implementation
>>> user seems to be unable to determine if the pattern is activated
>>> or not. We should define the semantics for this use case and
>>> describe it in the documentation. Possibly pattern could
>>> return alone new line character then.
> 
> Let me take a step back: we have triggers.. like LED blinking.
> 
> How is that going to interact with patterns? We probably want the
> patterns to be ignored in that case...?
> 
> Which suggest to me that we should treat patterns as a trigger. I
> believe we do something similar with blinking already.
> 
> Then it is easy to determine if pattern is active, and pattern
> vs. trigger issue is solved automatically.

I'm all for it. I proposed this approach during the previous
discussions related to possible pattern interface implementations,
but you seemed not to be so enthusiastic in [0].

[0] https://lkml.org/lkml/2017/4/7/350

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-14 22:02         ` Jacek Anaszewski
@ 2018-07-14 22:29           ` Pavel Machek
  2018-07-14 22:39             ` Pavel Machek
  0 siblings, 1 reply; 26+ messages in thread
From: Pavel Machek @ 2018-07-14 22:29 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

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

On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
> Hi Pavel,
> 
> On 07/14/2018 11:20 PM, Pavel Machek wrote:
> >Hi!
> >
> >>>It also drew my attention to the issue of desired pattern sysfs
> >>>interface semantics on uninitialized pattern. In your implementation
> >>>user seems to be unable to determine if the pattern is activated
> >>>or not. We should define the semantics for this use case and
> >>>describe it in the documentation. Possibly pattern could
> >>>return alone new line character then.
> >
> >Let me take a step back: we have triggers.. like LED blinking.
> >
> >How is that going to interact with patterns? We probably want the
> >patterns to be ignored in that case...?
> >
> >Which suggest to me that we should treat patterns as a trigger. I
> >believe we do something similar with blinking already.
> >
> >Then it is easy to determine if pattern is active, and pattern
> >vs. trigger issue is solved automatically.
> 
> I'm all for it. I proposed this approach during the previous
> discussions related to possible pattern interface implementations,
> but you seemed not to be so enthusiastic in [0].
> 
> [0] https://lkml.org/lkml/2017/4/7/350

Hmm. Reading my own email now, I can't decipher it.

I believe I meant "changing patterns from kernel in response to events
is probably overkill"... or something like that.

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-14 22:29           ` Pavel Machek
@ 2018-07-14 22:39             ` Pavel Machek
  2018-07-15 12:22               ` Jacek Anaszewski
  0 siblings, 1 reply; 26+ messages in thread
From: Pavel Machek @ 2018-07-14 22:39 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

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

On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
> > Hi Pavel,
> > 
> > On 07/14/2018 11:20 PM, Pavel Machek wrote:
> > >Hi!
> > >
> > >>>It also drew my attention to the issue of desired pattern sysfs
> > >>>interface semantics on uninitialized pattern. In your implementation
> > >>>user seems to be unable to determine if the pattern is activated
> > >>>or not. We should define the semantics for this use case and
> > >>>describe it in the documentation. Possibly pattern could
> > >>>return alone new line character then.
> > >
> > >Let me take a step back: we have triggers.. like LED blinking.
> > >
> > >How is that going to interact with patterns? We probably want the
> > >patterns to be ignored in that case...?
> > >
> > >Which suggest to me that we should treat patterns as a trigger. I
> > >believe we do something similar with blinking already.
> > >
> > >Then it is easy to determine if pattern is active, and pattern
> > >vs. trigger issue is solved automatically.
> > 
> > I'm all for it. I proposed this approach during the previous
> > discussions related to possible pattern interface implementations,
> > but you seemed not to be so enthusiastic in [0].
> > 
> > [0] https://lkml.org/lkml/2017/4/7/350
> 
> Hmm. Reading my own email now, I can't decipher it.
> 
> I believe I meant "changing patterns from kernel in response to events
> is probably overkill"... or something like that.

Anyway -- to clean up the confusion -- I'd like to see

echo pattern > trigger
echo "1 2 3 4 5 6 7 8" > somewhere

to activate pattern, and

echo none > trigger

to stop it.

Best regards,
								Pavel

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-14 22:39             ` Pavel Machek
@ 2018-07-15 12:22               ` Jacek Anaszewski
  2018-07-16  1:00                 ` David Lechner
  2018-07-16 11:08                 ` Baolin Wang
  0 siblings, 2 replies; 26+ messages in thread
From: Jacek Anaszewski @ 2018-07-15 12:22 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

On 07/15/2018 12:39 AM, Pavel Machek wrote:
> On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
>> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
>>> Hi Pavel,
>>>
>>> On 07/14/2018 11:20 PM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>> It also drew my attention to the issue of desired pattern sysfs
>>>>>> interface semantics on uninitialized pattern. In your implementation
>>>>>> user seems to be unable to determine if the pattern is activated
>>>>>> or not. We should define the semantics for this use case and
>>>>>> describe it in the documentation. Possibly pattern could
>>>>>> return alone new line character then.
>>>>
>>>> Let me take a step back: we have triggers.. like LED blinking.
>>>>
>>>> How is that going to interact with patterns? We probably want the
>>>> patterns to be ignored in that case...?
>>>>
>>>> Which suggest to me that we should treat patterns as a trigger. I
>>>> believe we do something similar with blinking already.
>>>>
>>>> Then it is easy to determine if pattern is active, and pattern
>>>> vs. trigger issue is solved automatically.
>>>
>>> I'm all for it. I proposed this approach during the previous
>>> discussions related to possible pattern interface implementations,
>>> but you seemed not to be so enthusiastic in [0].
>>>
>>> [0] https://lkml.org/lkml/2017/4/7/350
>>
>> Hmm. Reading my own email now, I can't decipher it.
>>
>> I believe I meant "changing patterns from kernel in response to events
>> is probably overkill"... or something like that.
> 
> Anyway -- to clean up the confusion -- I'd like to see
> 
> echo pattern > trigger
> echo "1 2 3 4 5 6 7 8" > somewhere

s/somewhere/pattern/

pattern trigger should create "pattern" file similarly how ledtrig-timer
creates delay_{on|off} files.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-15 12:22               ` Jacek Anaszewski
@ 2018-07-16  1:00                 ` David Lechner
  2018-07-16 20:29                   ` Jacek Anaszewski
  2018-07-16 11:08                 ` Baolin Wang
  1 sibling, 1 reply; 26+ messages in thread
From: David Lechner @ 2018-07-16  1:00 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Jacek Anaszewski, Pavel Machek, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

On 07/15/2018 07:22 AM, Jacek Anaszewski wrote:
> On 07/15/2018 12:39 AM, Pavel Machek wrote:
>> On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
>>> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
>>>> Hi Pavel,
>>>>
>>>> On 07/14/2018 11:20 PM, Pavel Machek wrote:
>>>>> Hi!
>>>>>
>>>>>>> It also drew my attention to the issue of desired pattern sysfs
>>>>>>> interface semantics on uninitialized pattern. In your implementation
>>>>>>> user seems to be unable to determine if the pattern is activated
>>>>>>> or not. We should define the semantics for this use case and
>>>>>>> describe it in the documentation. Possibly pattern could
>>>>>>> return alone new line character then.
>>>>>
>>>>> Let me take a step back: we have triggers.. like LED blinking.
>>>>>
>>>>> How is that going to interact with patterns? We probably want the
>>>>> patterns to be ignored in that case...?
>>>>>
>>>>> Which suggest to me that we should treat patterns as a trigger. I
>>>>> believe we do something similar with blinking already.
>>>>>
>>>>> Then it is easy to determine if pattern is active, and pattern
>>>>> vs. trigger issue is solved automatically.
>>>>
>>>> I'm all for it. I proposed this approach during the previous
>>>> discussions related to possible pattern interface implementations,
>>>> but you seemed not to be so enthusiastic in [0].
>>>>
>>>> [0] https://lkml.org/lkml/2017/4/7/350
>>>
>>> Hmm. Reading my own email now, I can't decipher it.
>>>
>>> I believe I meant "changing patterns from kernel in response to events
>>> is probably overkill"... or something like that.
>>
>> Anyway -- to clean up the confusion -- I'd like to see
>>
>> echo pattern > trigger
>> echo "1 2 3 4 5 6 7 8" > somewhere
> 
> s/somewhere/pattern/
> 
> pattern trigger should create "pattern" file similarly how ledtrig-timer
> creates delay_{on|off} files.
> 

I don't think this is the best way. For example, if you want more than one
LED to have the same pattern, then the patterns will not be synchronized
between the LEDs. The same things happens now with many of the existing
triggers. For example, if I have two LEDs side-by-side using the heartbeat
trigger, they may blink at the same time or they may not, which is not
very nice. I think we can make something better.

Perhaps a way to do this would be to use configfs to create a pattern
trigger that can be shared by multiple LEDs. Like this:

     mkdir /sys/kernel/config/leds/triggers/my-nice-pattern
     echo "1 2 3 4" > /sys/kernel/config/leds/triggers/my-nice-pattern/pattern
     echo my-nice-pattern > /sys/class/leds/led0/trigger
     echo my-nice-pattern > /sys/class/leds/led1/trigger


Please CC me on any future revisions of this series. I would like to test it.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-15 12:22               ` Jacek Anaszewski
  2018-07-16  1:00                 ` David Lechner
@ 2018-07-16 11:08                 ` Baolin Wang
  1 sibling, 0 replies; 26+ messages in thread
From: Baolin Wang @ 2018-07-16 11:08 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

On 15 July 2018 at 20:22, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> On 07/15/2018 12:39 AM, Pavel Machek wrote:
>>
>> On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
>>>
>>> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
>>>>
>>>> Hi Pavel,
>>>>
>>>> On 07/14/2018 11:20 PM, Pavel Machek wrote:
>>>>>
>>>>> Hi!
>>>>>
>>>>>>> It also drew my attention to the issue of desired pattern sysfs
>>>>>>> interface semantics on uninitialized pattern. In your implementation
>>>>>>> user seems to be unable to determine if the pattern is activated
>>>>>>> or not. We should define the semantics for this use case and
>>>>>>> describe it in the documentation. Possibly pattern could
>>>>>>> return alone new line character then.
>>>>>
>>>>>
>>>>> Let me take a step back: we have triggers.. like LED blinking.
>>>>>
>>>>> How is that going to interact with patterns? We probably want the
>>>>> patterns to be ignored in that case...?
>>>>>
>>>>> Which suggest to me that we should treat patterns as a trigger. I
>>>>> believe we do something similar with blinking already.
>>>>>
>>>>> Then it is easy to determine if pattern is active, and pattern
>>>>> vs. trigger issue is solved automatically.
>>>>
>>>>
>>>> I'm all for it. I proposed this approach during the previous
>>>> discussions related to possible pattern interface implementations,
>>>> but you seemed not to be so enthusiastic in [0].
>>>>
>>>> [0] https://lkml.org/lkml/2017/4/7/350
>>>
>>>
>>> Hmm. Reading my own email now, I can't decipher it.
>>>
>>> I believe I meant "changing patterns from kernel in response to events
>>> is probably overkill"... or something like that.
>>
>>
>> Anyway -- to clean up the confusion -- I'd like to see
>>
>> echo pattern > trigger
>> echo "1 2 3 4 5 6 7 8" > somewhere
>
>
> s/somewhere/pattern/
>
> pattern trigger should create "pattern" file similarly how ledtrig-timer
> creates delay_{on|off} files.

Yes. Anyway, I will submit V5 patchset with addressing previous
comments, but did not include pattern trigger issue.

-- 
Baolin Wang
Best Regards

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-16  1:00                 ` David Lechner
@ 2018-07-16 20:29                   ` Jacek Anaszewski
  2018-07-16 21:56                     ` Pavel Machek
  2018-07-18  7:56                     ` Pavel Machek
  0 siblings, 2 replies; 26+ messages in thread
From: Jacek Anaszewski @ 2018-07-16 20:29 UTC (permalink / raw)
  To: David Lechner, Baolin Wang
  Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

Hi David,

On 07/16/2018 03:00 AM, David Lechner wrote:
> On 07/15/2018 07:22 AM, Jacek Anaszewski wrote:
>> On 07/15/2018 12:39 AM, Pavel Machek wrote:
>>> On Sun 2018-07-15 00:29:25, Pavel Machek wrote:
>>>> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote:
>>>>> Hi Pavel,
>>>>>
>>>>> On 07/14/2018 11:20 PM, Pavel Machek wrote:
>>>>>> Hi!
>>>>>>
>>>>>>>> It also drew my attention to the issue of desired pattern sysfs
>>>>>>>> interface semantics on uninitialized pattern. In your 
>>>>>>>> implementation
>>>>>>>> user seems to be unable to determine if the pattern is activated
>>>>>>>> or not. We should define the semantics for this use case and
>>>>>>>> describe it in the documentation. Possibly pattern could
>>>>>>>> return alone new line character then.
>>>>>>
>>>>>> Let me take a step back: we have triggers.. like LED blinking.
>>>>>>
>>>>>> How is that going to interact with patterns? We probably want the
>>>>>> patterns to be ignored in that case...?
>>>>>>
>>>>>> Which suggest to me that we should treat patterns as a trigger. I
>>>>>> believe we do something similar with blinking already.
>>>>>>
>>>>>> Then it is easy to determine if pattern is active, and pattern
>>>>>> vs. trigger issue is solved automatically.
>>>>>
>>>>> I'm all for it. I proposed this approach during the previous
>>>>> discussions related to possible pattern interface implementations,
>>>>> but you seemed not to be so enthusiastic in [0].
>>>>>
>>>>> [0] https://lkml.org/lkml/2017/4/7/350
>>>>
>>>> Hmm. Reading my own email now, I can't decipher it.
>>>>
>>>> I believe I meant "changing patterns from kernel in response to events
>>>> is probably overkill"... or something like that.
>>>
>>> Anyway -- to clean up the confusion -- I'd like to see
>>>
>>> echo pattern > trigger
>>> echo "1 2 3 4 5 6 7 8" > somewhere
>>
>> s/somewhere/pattern/
>>
>> pattern trigger should create "pattern" file similarly how ledtrig-timer
>> creates delay_{on|off} files.
>>
> 
> I don't think this is the best way. For example, if you want more than one
> LED to have the same pattern, then the patterns will not be synchronized
> between the LEDs. The same things happens now with many of the existing
> triggers. For example, if I have two LEDs side-by-side using the heartbeat
> trigger, they may blink at the same time or they may not, which is not
> very nice. I think we can make something better.

It is virtually impossible to enforce synchronous blinking for the
LEDs driven by different hardware due to:

- different hardware means via which brightness is set (MMIO, I2C, SPI,
   PWM and other pulsed fashion based protocols),
- the need for deferring brightness setting to a workqueue task to
   allow for setting LED brightness from atomic context,
- contention on locks

For the LEDs driven by the same chip it would make more sense
to allow for synchronization, but it can be achieved on driver
level, with help of some subsystem level interface to indicate
which LEDs should be synchronized.

However, when we start to pretend that we can synchronize the
devices, we must answer how accurate we can be. The accuracy
will decrease as blink frequency rises. We'd need to define
reliability limit.

For different devices, this limit will be different, and it will also
depend on the CPU speed.

We've had few attempts of approaching the subject of synchronized
blinking but none of them proved to be good enough to be merged.

Frankly speaking I doubt it is good task for the system like Linux.

> Perhaps a way to do this would be to use configfs to create a pattern
> trigger that can be shared by multiple LEDs. Like this:
> 
>      mkdir /sys/kernel/config/leds/triggers/my-nice-pattern
>      echo "1 2 3 4" > 
> /sys/kernel/config/leds/triggers/my-nice-pattern/pattern
>      echo my-nice-pattern > /sys/class/leds/led0/trigger
>      echo my-nice-pattern > /sys/class/leds/led1/trigger
> 
> 
> Please CC me on any future revisions of this series. I would like to 
> test it.
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-16 20:29                   ` Jacek Anaszewski
@ 2018-07-16 21:56                     ` Pavel Machek
  2018-07-17 20:26                       ` Jacek Anaszewski
  2018-07-18  7:56                     ` Pavel Machek
  1 sibling, 1 reply; 26+ messages in thread
From: Pavel Machek @ 2018-07-16 21:56 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

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

Hi!

> >>>echo pattern > trigger
> >>>echo "1 2 3 4 5 6 7 8" > somewhere
> >>
> >>s/somewhere/pattern/
> >>
> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> >>creates delay_{on|off} files.
> >>
> >
> >I don't think this is the best way. For example, if you want more than one
> >LED to have the same pattern, then the patterns will not be synchronized
> >between the LEDs. The same things happens now with many of the existing
> >triggers. For example, if I have two LEDs side-by-side using the heartbeat
> >trigger, they may blink at the same time or they may not, which is not
> >very nice. I think we can make something better.

Yes, synchronization would be nice -- it is really a must for RGB LEDs
-- but I believe it is too much to ask from Baolin at the moment.

> It is virtually impossible to enforce synchronous blinking for the
> LEDs driven by different hardware due to:
> 
> - different hardware means via which brightness is set (MMIO, I2C, SPI,
>   PWM and other pulsed fashion based protocols),
> - the need for deferring brightness setting to a workqueue task to
>   allow for setting LED brightness from atomic context,
> - contention on locks

I disagree here. Yes, it would be hard to synchronize blinking down to
microseconds, but it would be easy to get synchronization right down
to miliseconds and humans will not be able to tell the difference.

> For the LEDs driven by the same chip it would make more sense
> to allow for synchronization, but it can be achieved on driver
> level, with help of some subsystem level interface to indicate
> which LEDs should be synchronized.
> 
> However, when we start to pretend that we can synchronize the
> devices, we must answer how accurate we can be. The accuracy
> will decrease as blink frequency rises. We'd need to define
> reliability limit.

We don't need _that_ ammount of overengineering. We just need to
synchronize them well enough :-).

> We've had few attempts of approaching the subject of synchronized
> blinking but none of them proved to be good enough to be merged.

I'm sure interested person could do something like that in less than
two weeks fulltime... It is not rocket science, just a lot of work in
kernel... 

But patterns are few years overdue and I believe we should not delay
them any further.

So... I guess I agree with Jacek in the end :-).

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-16 21:56                     ` Pavel Machek
@ 2018-07-17 20:26                       ` Jacek Anaszewski
  2018-07-17 21:07                         ` Pavel Machek
  0 siblings, 1 reply; 26+ messages in thread
From: Jacek Anaszewski @ 2018-07-17 20:26 UTC (permalink / raw)
  To: Pavel Machek
  Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

On 07/16/2018 11:56 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> echo pattern > trigger
>>>>> echo "1 2 3 4 5 6 7 8" > somewhere
>>>>
>>>> s/somewhere/pattern/
>>>>
>>>> pattern trigger should create "pattern" file similarly how ledtrig-timer
>>>> creates delay_{on|off} files.
>>>>
>>>
>>> I don't think this is the best way. For example, if you want more than one
>>> LED to have the same pattern, then the patterns will not be synchronized
>>> between the LEDs. The same things happens now with many of the existing
>>> triggers. For example, if I have two LEDs side-by-side using the heartbeat
>>> trigger, they may blink at the same time or they may not, which is not
>>> very nice. I think we can make something better.
> 
> Yes, synchronization would be nice -- it is really a must for RGB LEDs
> -- but I believe it is too much to ask from Baolin at the moment.
> 
>> It is virtually impossible to enforce synchronous blinking for the
>> LEDs driven by different hardware due to:
>>
>> - different hardware means via which brightness is set (MMIO, I2C, SPI,
>>    PWM and other pulsed fashion based protocols),
>> - the need for deferring brightness setting to a workqueue task to
>>    allow for setting LED brightness from atomic context,
>> - contention on locks
> 
> I disagree here. Yes, it would be hard to synchronize blinking down to
> microseconds, but it would be easy to get synchronization right down
> to miliseconds and humans will not be able to tell the difference.

There have been problems with blink interval stability close to
1ms, and thus there were some attempts of employing hr timers,
which in turn introduced a new class of issues related to
system performance etc.

>> For the LEDs driven by the same chip it would make more sense
>> to allow for synchronization, but it can be achieved on driver
>> level, with help of some subsystem level interface to indicate
>> which LEDs should be synchronized.
>>
>> However, when we start to pretend that we can synchronize the
>> devices, we must answer how accurate we can be. The accuracy
>> will decrease as blink frequency rises. We'd need to define
>> reliability limit.
> 
> We don't need _that_ ammount of overengineering. We just need to
> synchronize them well enough :-).

Well, it would be disappointing for the users to realize that
they don't get the effect advertised by the ABI documentation.

>> We've had few attempts of approaching the subject of synchronized
>> blinking but none of them proved to be good enough to be merged.
> 
> I'm sure interested person could do something like that in less than
> two weeks fulltime... It is not rocket science, just a lot of work in
> kernel...
> 
> But patterns are few years overdue and I believe we should not delay
> them any further.
> 
> So... I guess I agree with Jacek in the end :-).

How about taking Baolin's patches as of v5? Later, provided that
the pattern trigger yet to be implemented will create pattern file
on activation, we'll need to initialize default-trigger DT property,
to keep the interface unchanged.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-17 20:26                       ` Jacek Anaszewski
@ 2018-07-17 21:07                         ` Pavel Machek
  0 siblings, 0 replies; 26+ messages in thread
From: Pavel Machek @ 2018-07-17 21:07 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

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

Hi!

> >>- different hardware means via which brightness is set (MMIO, I2C, SPI,
> >>   PWM and other pulsed fashion based protocols),
> >>- the need for deferring brightness setting to a workqueue task to
> >>   allow for setting LED brightness from atomic context,
> >>- contention on locks
> >
> >I disagree here. Yes, it would be hard to synchronize blinking down to
> >microseconds, but it would be easy to get synchronization right down
> >to miliseconds and humans will not be able to tell the difference.
> 
> There have been problems with blink interval stability close to
> 1ms, and thus there were some attempts of employing hr timers,
> which in turn introduced a new class of issues related to
> system performance etc.

Yeah, well. This is LED subsystem. Noone should program blink
intervals at 1 msec.

> >>For the LEDs driven by the same chip it would make more sense
> >>to allow for synchronization, but it can be achieved on driver
> >>level, with help of some subsystem level interface to indicate
> >>which LEDs should be synchronized.
> >>
> >>However, when we start to pretend that we can synchronize the
> >>devices, we must answer how accurate we can be. The accuracy
> >>will decrease as blink frequency rises. We'd need to define
> >>reliability limit.
> >
> >We don't need _that_ ammount of overengineering. We just need to
> >synchronize them well enough :-).
> 
> Well, it would be disappointing for the users to realize that
> they don't get the effect advertised by the ABI documentation.

Linux is always best-effort, w.r.t. timing. And we can do well enough
that user will not see anything bad on "normal" systems.

> >>We've had few attempts of approaching the subject of synchronized
> >>blinking but none of them proved to be good enough to be merged.
> >
> >I'm sure interested person could do something like that in less than
> >two weeks fulltime... It is not rocket science, just a lot of work in
> >kernel...
> >
> >But patterns are few years overdue and I believe we should not delay
> >them any further.
> >
> >So... I guess I agree with Jacek in the end :-).
> 
> How about taking Baolin's patches as of v5? Later, provided that
> the pattern trigger yet to be implemented will create pattern file
> on activation, we'll need to initialize default-trigger DT property,
> to keep the interface unchanged.

I have yet to look at the v5 of patches. But I agree that we do not
need to design synchronization at this moment.

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-16 20:29                   ` Jacek Anaszewski
  2018-07-16 21:56                     ` Pavel Machek
@ 2018-07-18  7:56                     ` Pavel Machek
  2018-07-18 11:32                       ` Baolin Wang
  2018-07-18 18:54                       ` Jacek Anaszewski
  1 sibling, 2 replies; 26+ messages in thread
From: Pavel Machek @ 2018-07-18  7:56 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

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

Hi!

> >>>>I believe I meant "changing patterns from kernel in response to events
> >>>>is probably overkill"... or something like that.
> >>>
> >>>Anyway -- to clean up the confusion -- I'd like to see
> >>>
> >>>echo pattern > trigger
> >>>echo "1 2 3 4 5 6 7 8" > somewhere
> >>
> >>s/somewhere/pattern/
> >>
> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> >>creates delay_{on|off} files.

Yes, that sounds reasonable. v5 still says

+               Writing non-empty string to this file will activate the pattern,
+               and empty string will disable the pattern.

I'd deactivate the pattern by simply writing something else to the
trigger file.

Best regards,

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-18  7:56                     ` Pavel Machek
@ 2018-07-18 11:32                       ` Baolin Wang
  2018-07-18 12:08                         ` Pavel Machek
  2018-07-18 18:54                       ` Jacek Anaszewski
  1 sibling, 1 reply; 26+ messages in thread
From: Baolin Wang @ 2018-07-18 11:32 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

On 18 July 2018 at 15:56, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> >>>>I believe I meant "changing patterns from kernel in response to events
>> >>>>is probably overkill"... or something like that.
>> >>>
>> >>>Anyway -- to clean up the confusion -- I'd like to see
>> >>>
>> >>>echo pattern > trigger
>> >>>echo "1 2 3 4 5 6 7 8" > somewhere
>> >>
>> >>s/somewhere/pattern/
>> >>
>> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
>> >>creates delay_{on|off} files.
>
> Yes, that sounds reasonable. v5 still says
>
> +               Writing non-empty string to this file will activate the pattern,
> +               and empty string will disable the pattern.
>
> I'd deactivate the pattern by simply writing something else to the
> trigger file.

For the case we met in patch 2, it is not related with trigger things.
We just set some series of tuples including brightness and duration
(ms) to the hardware to enable the breath mode of the LED, we did not
trigger anything. So it is weird to write something to trigger file to
deactive the pattern.

-- 
Baolin Wang
Best Regards

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-18 11:32                       ` Baolin Wang
@ 2018-07-18 12:08                         ` Pavel Machek
  2018-07-18 17:00                           ` David Lechner
  0 siblings, 1 reply; 26+ messages in thread
From: Pavel Machek @ 2018-07-18 12:08 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

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

On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
> On 18 July 2018 at 15:56, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> >
> >> >>>>I believe I meant "changing patterns from kernel in response to events
> >> >>>>is probably overkill"... or something like that.
> >> >>>
> >> >>>Anyway -- to clean up the confusion -- I'd like to see
> >> >>>
> >> >>>echo pattern > trigger
> >> >>>echo "1 2 3 4 5 6 7 8" > somewhere
> >> >>
> >> >>s/somewhere/pattern/
> >> >>
> >> >>pattern trigger should create "pattern" file similarly how ledtrig-timer
> >> >>creates delay_{on|off} files.
> >
> > Yes, that sounds reasonable. v5 still says
> >
> > +               Writing non-empty string to this file will activate the pattern,
> > +               and empty string will disable the pattern.
> >
> > I'd deactivate the pattern by simply writing something else to the
> > trigger file.
> 
> For the case we met in patch 2, it is not related with trigger things.
> We just set some series of tuples including brightness and duration
> (ms) to the hardware to enable the breath mode of the LED, we did not
> trigger anything. So it is weird to write something to trigger file to
> deactive the pattern.

Confused. I thought that "breathing mode" would be handled similar way
to hardware blinking: userland selects pattern trigger, pattern file
appears (similar way to delay_on/delay_off files with blinking), he
configures it, hardware brightness follows the pattern ("breathing
mode"). If pattern is no longer required, echo none > trigger stops
it.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-18 12:08                         ` Pavel Machek
@ 2018-07-18 17:00                           ` David Lechner
  0 siblings, 0 replies; 26+ messages in thread
From: David Lechner @ 2018-07-18 17:00 UTC (permalink / raw)
  To: Pavel Machek, Baolin Wang
  Cc: Jacek Anaszewski, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML



On 7/18/18 7:08 AM, Pavel Machek wrote:
> On Wed 2018-07-18 19:32:01, Baolin Wang wrote:
>> On 18 July 2018 at 15:56, Pavel Machek <pavel@ucw.cz> wrote:
>>> Hi!
>>>
>>>>>>>> I believe I meant "changing patterns from kernel in response to events
>>>>>>>> is probably overkill"... or something like that.
>>>>>>>
>>>>>>> Anyway -- to clean up the confusion -- I'd like to see
>>>>>>>
>>>>>>> echo pattern > trigger
>>>>>>> echo "1 2 3 4 5 6 7 8" > somewhere
>>>>>>
>>>>>> s/somewhere/pattern/
>>>>>>
>>>>>> pattern trigger should create "pattern" file similarly how ledtrig-timer
>>>>>> creates delay_{on|off} files.
>>>
>>> Yes, that sounds reasonable. v5 still says
>>>
>>> +               Writing non-empty string to this file will activate the pattern,
>>> +               and empty string will disable the pattern.
>>>
>>> I'd deactivate the pattern by simply writing something else to the
>>> trigger file.
>>
>> For the case we met in patch 2, it is not related with trigger things.
>> We just set some series of tuples including brightness and duration
>> (ms) to the hardware to enable the breath mode of the LED, we did not
>> trigger anything. So it is weird to write something to trigger file to
>> deactive the pattern.
> 
> Confused. I thought that "breathing mode" would be handled similar way
> to hardware blinking: userland selects pattern trigger, pattern file
> appears (similar way to delay_on/delay_off files with blinking), he
> configures it, hardware brightness follows the pattern ("breathing
> mode"). If pattern is no longer required, echo none > trigger stops
> it.
> 									Pavel
> 

I was confused too when I first read this thread. But after reviewing 
v5, it is clear that this is _not_ a trigger (and it should not be a 
trigger). This is basically the equivalent the brightness attribute - 
except that now the brightness changes over time instead of being a 
constant value. This way, the pattern can be used in conjunction with 
triggers.

For example, one might want to set the _pattern_ to something like a 
"breathe" pattern and set the _trigger_ to the power supply charging 
full trigger. This way, the LED will be off until the battery is full 
and then the LED will "breath" when the battery is full.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-18  7:56                     ` Pavel Machek
  2018-07-18 11:32                       ` Baolin Wang
@ 2018-07-18 18:54                       ` Jacek Anaszewski
  2018-07-18 19:22                         ` Jacek Anaszewski
  1 sibling, 1 reply; 26+ messages in thread
From: Jacek Anaszewski @ 2018-07-18 18:54 UTC (permalink / raw)
  To: Pavel Machek
  Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

On 07/18/2018 09:56 AM, Pavel Machek wrote:
> Hi!
> 
>>>>>> I believe I meant "changing patterns from kernel in response to events
>>>>>> is probably overkill"... or something like that.
>>>>>
>>>>> Anyway -- to clean up the confusion -- I'd like to see
>>>>>
>>>>> echo pattern > trigger
>>>>> echo "1 2 3 4 5 6 7 8" > somewhere
>>>>
>>>> s/somewhere/pattern/
>>>>
>>>> pattern trigger should create "pattern" file similarly how ledtrig-timer
>>>> creates delay_{on|off} files.
> 
> Yes, that sounds reasonable. v5 still says
> 
> +               Writing non-empty string to this file will activate the pattern,
> +               and empty string will disable the pattern.
> 
> I'd deactivate the pattern by simply writing something else to the
> trigger file.

Please keep in mind that this is ABI documentation for the pattern file
to be exposed by LED core, and not by the pattern trigger, that, as we
agreed, will be implemented later. In this case, I'd go for
"echo 0 > brightness" as a command disabling pattern. The same operation
disables triggers, so later transition to using pattern trigger will be
seamless for userspace.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-18 18:54                       ` Jacek Anaszewski
@ 2018-07-18 19:22                         ` Jacek Anaszewski
  2018-07-18 22:13                           ` David Lechner
  2018-07-18 22:17                           ` Pavel Machek
  0 siblings, 2 replies; 26+ messages in thread
From: Jacek Anaszewski @ 2018-07-18 19:22 UTC (permalink / raw)
  To: Pavel Machek
  Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

On 07/18/2018 08:54 PM, Jacek Anaszewski wrote:
> On 07/18/2018 09:56 AM, Pavel Machek wrote:
>> Hi!
>>
>>>>>>> I believe I meant "changing patterns from kernel in response to 
>>>>>>> events
>>>>>>> is probably overkill"... or something like that.
>>>>>>
>>>>>> Anyway -- to clean up the confusion -- I'd like to see
>>>>>>
>>>>>> echo pattern > trigger
>>>>>> echo "1 2 3 4 5 6 7 8" > somewhere
>>>>>
>>>>> s/somewhere/pattern/
>>>>>
>>>>> pattern trigger should create "pattern" file similarly how 
>>>>> ledtrig-timer
>>>>> creates delay_{on|off} files.
>>
>> Yes, that sounds reasonable. v5 still says
>>
>> +               Writing non-empty string to this file will activate 
>> the pattern,
>> +               and empty string will disable the pattern.
>>
>> I'd deactivate the pattern by simply writing something else to the
>> trigger file.
> 
> Please keep in mind that this is ABI documentation for the pattern file
> to be exposed by LED core, and not by the pattern trigger, that, as we
> agreed, will be implemented later. In this case, I'd go for

Gosh, I got completely distracted by the recent discussion about
pattern synchronization.

So, to recap, we need to decide if we are taking Baolin's solution
or we're opting for implementing pattern trigger.

If we choose the latter, then we will also need some software
pattern engine in the trigger, to be applied as a software pattern
fallback for the devices without hardware pattern support.
It will certainly delay the contribution process, provided that Baolin
would find time for this work at all.

I'd just take v5 based solution for now (with improved semantics
of disabling pattern - in this case my reasoning from the message
I'm replying to is still valid),

> "echo 0 > brightness" as a command disabling pattern. The same operation
> disables triggers, so later transition to using pattern trigger will be
> seamless for userspace.
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-18 19:22                         ` Jacek Anaszewski
@ 2018-07-18 22:13                           ` David Lechner
  2018-07-18 22:17                           ` Pavel Machek
  1 sibling, 0 replies; 26+ messages in thread
From: David Lechner @ 2018-07-18 22:13 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML

On 07/18/2018 02:22 PM, Jacek Anaszewski wrote:
> On 07/18/2018 08:54 PM, Jacek Anaszewski wrote:
>> On 07/18/2018 09:56 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>>> I believe I meant "changing patterns from kernel in response to events
>>>>>>>> is probably overkill"... or something like that.
>>>>>>>
>>>>>>> Anyway -- to clean up the confusion -- I'd like to see
>>>>>>>
>>>>>>> echo pattern > trigger
>>>>>>> echo "1 2 3 4 5 6 7 8" > somewhere
>>>>>>
>>>>>> s/somewhere/pattern/
>>>>>>
>>>>>> pattern trigger should create "pattern" file similarly how ledtrig-timer
>>>>>> creates delay_{on|off} files.
>>>
>>> Yes, that sounds reasonable. v5 still says
>>>
>>> +               Writing non-empty string to this file will activate the pattern,
>>> +               and empty string will disable the pattern.
>>>
>>> I'd deactivate the pattern by simply writing something else to the
>>> trigger file.
>>
>> Please keep in mind that this is ABI documentation for the pattern file
>> to be exposed by LED core, and not by the pattern trigger, that, as we
>> agreed, will be implemented later. In this case, I'd go for
> 
> Gosh, I got completely distracted by the recent discussion about
> pattern synchronization.
> 
> So, to recap, we need to decide if we are taking Baolin's solution
> or we're opting for implementing pattern trigger.

I think we should take Baolin's solution.

> 
> If we choose the latter, then we will also need some software
> pattern engine in the trigger, to be applied as a software pattern
> fallback for the devices without hardware pattern support.
> It will certainly delay the contribution process, provided that Baolin
> would find time for this work at all.
> 
> I'd just take v5 based solution for now (with improved semantics
> of disabling pattern - in this case my reasoning from the message
> I'm replying to is still valid),
> 
>> "echo 0 > brightness" as a command disabling pattern. The same operation
>> disables triggers, so later transition to using pattern trigger will be
>> seamless for userspace.
>>
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
  2018-07-18 19:22                         ` Jacek Anaszewski
  2018-07-18 22:13                           ` David Lechner
@ 2018-07-18 22:17                           ` Pavel Machek
  1 sibling, 0 replies; 26+ messages in thread
From: Pavel Machek @ 2018-07-18 22:17 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, LKML

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

Hi!

> >Please keep in mind that this is ABI documentation for the pattern file
> >to be exposed by LED core, and not by the pattern trigger, that, as we
> >agreed, will be implemented later. In this case, I'd go for
> 
> Gosh, I got completely distracted by the recent discussion about
> pattern synchronization.
> 
> So, to recap, we need to decide if we are taking Baolin's solution
> or we're opting for implementing pattern trigger.
> 
> If we choose the latter, then we will also need some software
> pattern engine in the trigger, to be applied as a software pattern
> fallback for the devices without hardware pattern support.
> It will certainly delay the contribution process, provided that Baolin
> would find time for this work at all.

I'd recommend the latter. Yes, software pattern as a fallback would be
nice, but I have that code already... let me get it back to running
state, and figure out where to add interface for "hardware
acceleration". I'd like to have same interface to userland, whether
pattern can be done by hardware or by software.

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-29  5:03 [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang
2018-06-29  5:03 ` [PATCH v3 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller Baolin Wang
2018-07-11 11:02 ` [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang
2018-07-11 21:10   ` Jacek Anaszewski
2018-07-12 12:24     ` Baolin Wang
2018-07-12 21:41       ` Jacek Anaszewski
2018-07-13  1:58         ` Baolin Wang
2018-07-14 21:20       ` Pavel Machek
2018-07-14 22:02         ` Jacek Anaszewski
2018-07-14 22:29           ` Pavel Machek
2018-07-14 22:39             ` Pavel Machek
2018-07-15 12:22               ` Jacek Anaszewski
2018-07-16  1:00                 ` David Lechner
2018-07-16 20:29                   ` Jacek Anaszewski
2018-07-16 21:56                     ` Pavel Machek
2018-07-17 20:26                       ` Jacek Anaszewski
2018-07-17 21:07                         ` Pavel Machek
2018-07-18  7:56                     ` Pavel Machek
2018-07-18 11:32                       ` Baolin Wang
2018-07-18 12:08                         ` Pavel Machek
2018-07-18 17:00                           ` David Lechner
2018-07-18 18:54                       ` Jacek Anaszewski
2018-07-18 19:22                         ` Jacek Anaszewski
2018-07-18 22:13                           ` David Lechner
2018-07-18 22:17                           ` Pavel Machek
2018-07-16 11:08                 ` Baolin Wang

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox