2018-07-13 06:22:35

by Baolin Wang

[permalink] [raw]
Subject: [PATCH v4 1/2] leds: core: Introduce generic pattern interface

From: Bjorn Andersson <[email protected]>

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 improvements.]

Signed-off-by: Bjorn Andersson <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
Changes from v3:
- Move the check in pattern_show() to of_led_classdev_register().
- Add more documentation to explain how to set/clear one pattern.

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 | 20 +++++
drivers/leds/led-class.c | 118 +++++++++++++++++++++++++++++
include/linux/leds.h | 19 +++++
3 files changed, 157 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..f4b73ad 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,23 @@ 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: July 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.
+
+ Writing non-empty string to this file will active the pattern,
+ and empty string will disable the pattern.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..0992a0e 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,119 @@ 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;
+
+ 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;
+ char *sbegin, *elem, *s;
+ unsigned long val;
+ int ret = 0, 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) {
+ if (led_cdev->pattern_clear)
+ 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 +201,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[] = {
@@ -256,6 +371,9 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
char name[LED_MAX_NAME_SIZE];
int ret;

+ if (led_cdev->pattern_set && !led_cdev->pattern_get)
+ return -EINVAL;
+
ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
if (ret < 0)
return ret;
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



2018-07-13 06:22:36

by Baolin Wang

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

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

Signed-off-by: Baolin Wang <[email protected]>
---
Changes from v3:
- None.

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


2018-07-13 20:08:09

by Jacek Anaszewski

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] leds: core: Introduce generic pattern interface

Hi Baolin,

Thank you for the update.

On 07/13/2018 08:21 AM, Baolin Wang wrote:
> From: Bjorn Andersson <[email protected]>
>
> 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 improvements.]
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> Changes from v3:
> - Move the check in pattern_show() to of_led_classdev_register().
> - Add more documentation to explain how to set/clear one pattern.
>
> 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 | 20 +++++
> drivers/leds/led-class.c | 118 +++++++++++++++++++++++++++++
> include/linux/leds.h | 19 +++++
> 3 files changed, 157 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..f4b73ad 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,23 @@ 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: July 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.
> +
> + Writing non-empty string to this file will active the pattern,

s/active/activate/

> + and empty string will disable the pattern.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3c7e348..0992a0e 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -74,6 +74,119 @@ 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;
> +
> + 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;
> + char *sbegin, *elem, *s;
> + unsigned long val;
> + int ret = 0, 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) {
> + if (led_cdev->pattern_clear)
> + ret = led_cdev->pattern_clear(led_cdev);

Why you don't require pattern_clear op to be initialized?
Without it there seems to be no other way to disable the pattern.

Please fail in led_classdev_register() if it is NULL, similarly
as when pattern_get is NULL.

> + 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 +201,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[] = {
> @@ -256,6 +371,9 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
> char name[LED_MAX_NAME_SIZE];
> int ret;
>
> + if (led_cdev->pattern_set && !led_cdev->pattern_get)

if (led_cdev->pattern_set && (!led_cdev->pattern_get ||
!led_cdev->pattern_clear)

> + return -EINVAL;
> +
> ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
> if (ret < 0)
> return ret;
> 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 */
>

--
Best regards,
Jacek Anaszewski

2018-07-14 02:45:30

by Baolin Wang

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] leds: core: Introduce generic pattern interface

Hi Jacek,

On 14 July 2018 at 04:07, Jacek Anaszewski <[email protected]> wrote:
> Hi Baolin,
>
> Thank you for the update.
>
>
> On 07/13/2018 08:21 AM, Baolin Wang wrote:
>>
>> From: Bjorn Andersson <[email protected]>
>>
>> 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 improvements.]
>>
>> Signed-off-by: Bjorn Andersson <[email protected]>
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>> Changes from v3:
>> - Move the check in pattern_show() to of_led_classdev_register().
>> - Add more documentation to explain how to set/clear one pattern.
>>
>> 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 | 20 +++++
>> drivers/leds/led-class.c | 118
>> +++++++++++++++++++++++++++++
>> include/linux/leds.h | 19 +++++
>> 3 files changed, 157 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>> b/Documentation/ABI/testing/sysfs-class-led
>> index 5f67f7a..f4b73ad 100644
>> --- a/Documentation/ABI/testing/sysfs-class-led
>> +++ b/Documentation/ABI/testing/sysfs-class-led
>> @@ -61,3 +61,23 @@ 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: July 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.
>> +
>> + Writing non-empty string to this file will active the
>> pattern,
>
>
> s/active/activate/

Will fix the typo.

>
>
>> + and empty string will disable the pattern.
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 3c7e348..0992a0e 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -74,6 +74,119 @@ 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;
>> +
>> + 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;
>> + char *sbegin, *elem, *s;
>> + unsigned long val;
>> + int ret = 0, 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) {
>> + if (led_cdev->pattern_clear)
>> + ret = led_cdev->pattern_clear(led_cdev);
>
>
> Why you don't require pattern_clear op to be initialized?
> Without it there seems to be no other way to disable the pattern.
>
> Please fail in led_classdev_register() if it is NULL, similarly
> as when pattern_get is NULL.

Ah, I misunderstood your previous comments. Yes, we can ceck it in
led_classdev_register().

>
>
>> + 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 +201,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[] = {
>> @@ -256,6 +371,9 @@ int of_led_classdev_register(struct device *parent,
>> struct device_node *np,
>> char name[LED_MAX_NAME_SIZE];
>> int ret;
>> + if (led_cdev->pattern_set && !led_cdev->pattern_get)
>
>
> if (led_cdev->pattern_set && (!led_cdev->pattern_get ||
> !led_cdev->pattern_clear)

Sure. Thanks for your help.

>
>
>> + return -EINVAL;
>> +
>> ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>> if (ret < 0)
>> return ret;
>> 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 */
>>
>
> --
> Best regards,
> Jacek Anaszewski



--
Baolin Wang
Best Regards

2018-07-14 06:31:05

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] leds: core: Introduce generic pattern interface


> @@ -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

"brightness".

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


Attachments:
(No filename) (416.00 B)
signature.asc (188.00 B)
Digital signature
Download all attachments

2018-07-16 02:15:29

by Baolin Wang

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] leds: core: Introduce generic pattern interface

On 14 July 2018 at 14:29, Pavel Machek <[email protected]> wrote:
>
>> @@ -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
>
> "brightness".

Will fix the typo. Thanks.

--
Baolin Wang
Best Regards