linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] leds: core: Introduce generic pattern interface
@ 2018-06-28  5:16 Baolin Wang
  2018-06-28  5:16 ` [PATCH v2 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller Baolin Wang
  2018-06-28  8:31 ` [PATCH v2 1/2] leds: core: Introduce generic pattern interface Andy Shevchenko
  0 siblings, 2 replies; 7+ messages in thread
From: Baolin Wang @ 2018-06-28  5:16 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 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                  |  121 +++++++++++++++++++++++++++++
 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..aec0284 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.18
+Description:
+	Specify a pattern for the LED, for LED hardware that support
+	altering the brightness as a function of time.
+
+	The pattern is given by a series of tuples, of brightness and
+	duration (ms). The LED is expected to traverse the series and
+	each brightness value for the specified duration. 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..1c23f63 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,125 @@ static ssize_t max_brightness_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(max_brightness);
 
+static ssize_t pattern_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct led_pattern *pattern;
+	size_t offset = 0;
+	int count, n, i;
+
+	if (!led_cdev->pattern_get)
+		return -EOPNOTSUPP;
+
+	pattern = led_cdev->pattern_get(led_cdev, &count);
+	if (IS_ERR_OR_NULL(pattern))
+		return PTR_ERR(pattern);
+
+	for (i = 0; i < count; i++) {
+		n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d",
+			     pattern[i].brightness, pattern[i].delta_t);
+
+		if (offset + n >= PAGE_SIZE)
+			goto err_nospc;
+
+		offset += n;
+
+		if (i < count - 1)
+			buf[offset++] = ' ';
+	}
+
+	buf[offset++] = '\n';
+
+	kfree(pattern);
+	return offset;
+
+err_nospc:
+	kfree(pattern);
+	return -ENOSPC;
+}
+
+static ssize_t pattern_store(struct device *dev,
+			     struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct led_pattern *pattern = NULL;
+	unsigned long val;
+	char *sbegin;
+	char *elem;
+	char *s;
+	int ret, len = 0;
+	bool odd = true;
+
+	s = sbegin = kstrndup(buf, size, GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	/* Trim trailing newline */
+	s[strcspn(s, "\n")] = '\0';
+
+	/* If the remaining string is empty, clear the pattern */
+	if (!s[0]) {
+		ret = led_cdev->pattern_clear(led_cdev);
+		goto out;
+	}
+
+	pattern = kcalloc(size, sizeof(*pattern), GFP_KERNEL);
+	if (!pattern) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* Parse out the brightness & delta_t touples */
+	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 +207,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 related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller
  2018-06-28  5:16 [PATCH v2 1/2] leds: core: Introduce generic pattern interface Baolin Wang
@ 2018-06-28  5:16 ` Baolin Wang
  2018-06-28  8:31 ` [PATCH v2 1/2] leds: core: Introduce generic pattern interface Andy Shevchenko
  1 sibling, 0 replies; 7+ messages in thread
From: Baolin Wang @ 2018-06-28  5:16 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 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 related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] leds: core: Introduce generic pattern interface
  2018-06-28  5:16 [PATCH v2 1/2] leds: core: Introduce generic pattern interface Baolin Wang
  2018-06-28  5:16 ` [PATCH v2 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller Baolin Wang
@ 2018-06-28  8:31 ` Andy Shevchenko
  2018-06-28  9:18   ` Baolin Wang
  1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2018-06-28  8:31 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Jacek Anaszewski, Pavel Machek, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, Linux Kernel Mailing List

On Thu, Jun 28, 2018 at 8:16 AM, 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.

> +What: /sys/class/leds/<led>/pattern
> +Date: June 2018
> +KernelVersion: 4.18

4.19 ?

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

Perhaps just hide an attribute completely?

> +       pattern = led_cdev->pattern_get(led_cdev, &count);
> +       if (IS_ERR_OR_NULL(pattern))
> +               return PTR_ERR(pattern);

Hmm.. Here you shadow NULL case by returning 0.
Even if it's correct behaviour IS_ERR_OR_NULL is a beast to hide such
subtle detail.

It also would be good idea to check for count == 0 and bail out
immediately. Otherwise you will print an extra blank line.

> +       for (i = 0; i < count; i++) {
> +               n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d",
> +                            pattern[i].brightness, pattern[i].delta_t);
> +

> +               if (offset + n >= PAGE_SIZE)
> +                       goto err_nospc;

> +
> +               offset += n;
> +

> +               if (i < count - 1)
> +                       buf[offset++] = ' ';

You might add this to the end of above format string and remove this
conditional completely...

> +       }
> +

> +       buf[offset++] = '\n';

...and use here something like

buf[offset - 1] = '\n';

(we have such patterns in the kernel)

> +
> +       kfree(pattern);
> +       return offset;
> +
> +err_nospc:
> +       kfree(pattern);
> +       return -ENOSPC;
> +}
> +
> +static ssize_t pattern_store(struct device *dev,
> +                            struct device_attribute *attr,
> +                            const char *buf, size_t size)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       struct led_pattern *pattern = NULL;
> +       unsigned long val;
> +       char *sbegin;
> +       char *elem;
> +       char *s;
> +       int ret, len = 0;
> +       bool odd = true;
> +
> +       s = sbegin = kstrndup(buf, size, GFP_KERNEL);
> +       if (!s)
> +               return -ENOMEM;
> +

> +       /* Trim trailing newline */
> +       s[strcspn(s, "\n")] = '\0';

It's usually done via strstrip().

sbegin = kstrndup();
...

s = strstrip(sbegin);

> +
> +       /* If the remaining string is empty, clear the pattern */
> +       if (!s[0]) {

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) {

This is effectivelly if (len % 2 == 0)

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

For partial data can we return different error code?
Does it make sense?

> +
> +       ret = led_cdev->pattern_set(led_cdev, pattern, len);
> +
> +out:
> +       kfree(pattern);
> +       kfree(sbegin);
> +       return ret < 0 ? ret : size;
> +}
> +static DEVICE_ATTR_RW(pattern);


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 1/2] leds: core: Introduce generic pattern interface
  2018-06-28  8:31 ` [PATCH v2 1/2] leds: core: Introduce generic pattern interface Andy Shevchenko
@ 2018-06-28  9:18   ` Baolin Wang
  2018-06-28  9:25     ` Baolin Wang
  2018-06-28 10:24     ` Andy Shevchenko
  0 siblings, 2 replies; 7+ messages in thread
From: Baolin Wang @ 2018-06-28  9:18 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jacek Anaszewski, Pavel Machek, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, Linux Kernel Mailing List

Hi Andy,

On 28 June 2018 at 16:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Jun 28, 2018 at 8:16 AM, 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.
>
>> +What: /sys/class/leds/<led>/pattern
>> +Date: June 2018
>> +KernelVersion: 4.18
>
> 4.19 ?

I think this will be merged in 4.18.

>
>> +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;
>> +
>
> Perhaps just hide an attribute completely?

Driver need implement the pattern_get() interface, otherwise we can
not get any available pattern values to show.

>
>> +       pattern = led_cdev->pattern_get(led_cdev, &count);
>> +       if (IS_ERR_OR_NULL(pattern))
>> +               return PTR_ERR(pattern);
>
> Hmm.. Here you shadow NULL case by returning 0.
> Even if it's correct behaviour IS_ERR_OR_NULL is a beast to hide such
> subtle detail.
>
> It also would be good idea to check for count == 0 and bail out
> immediately. Otherwise you will print an extra blank line.

We can not check count, since count can be not initialized if failed
to call pattern_get(). So maybe force user to return error pointer,
and we just check like:
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;
>> +
>
>> +               if (i < count - 1)
>> +                       buf[offset++] = ' ';
>
> You might add this to the end of above format string and remove this
> conditional completely...

Hmmm, I do not think we need add one extra ' ' to the end of format string.

>
>> +       }
>> +
>
>> +       buf[offset++] = '\n';
>
> ...and use here something like
>
> buf[offset - 1] = '\n';

I don't think so. We need increase the offset value at the same time.

>
> (we have such patterns in the kernel)
>
>> +
>> +       kfree(pattern);
>> +       return offset;
>> +
>> +err_nospc:
>> +       kfree(pattern);
>> +       return -ENOSPC;
>> +}
>> +
>> +static ssize_t pattern_store(struct device *dev,
>> +                            struct device_attribute *attr,
>> +                            const char *buf, size_t size)
>> +{
>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +       struct led_pattern *pattern = NULL;
>> +       unsigned long val;
>> +       char *sbegin;
>> +       char *elem;
>> +       char *s;
>> +       int ret, len = 0;
>> +       bool odd = true;
>> +
>> +       s = sbegin = kstrndup(buf, size, GFP_KERNEL);
>> +       if (!s)
>> +               return -ENOMEM;
>> +
>
>> +       /* Trim trailing newline */
>> +       s[strcspn(s, "\n")] = '\0';
>
> It's usually done via strstrip().
>
> sbegin = kstrndup();
> ...
>
> s = strstrip(sbegin);

Good idea, will change.

>
>> +
>> +       /* If the remaining string is empty, clear the pattern */
>> +       if (!s[0]) {
>
> if (!*s) ?

OK.

>
>> +               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) {
>
> This is effectivelly if (len % 2 == 0)

It is incorrect, we can not use len to decide the value is brightness
or delta. Here logical is to make sure we must keep <brightness
delta>, <brightness, delta> ......

>
>> +                       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;
>> +       }
>
> For partial data can we return different error code?
> Does it make sense?

Sorry I did not get you here. If user set incorrect pattern values, I
think '-EINVAL' is suitable. Thanks for your comments.

-- 
Baolin.wang
Best Regards

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

* Re: [PATCH v2 1/2] leds: core: Introduce generic pattern interface
  2018-06-28  9:18   ` Baolin Wang
@ 2018-06-28  9:25     ` Baolin Wang
  2018-06-28 10:24     ` Andy Shevchenko
  1 sibling, 0 replies; 7+ messages in thread
From: Baolin Wang @ 2018-06-28  9:25 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jacek Anaszewski, Pavel Machek, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, Linux Kernel Mailing List

On 28 June 2018 at 17:18, Baolin Wang <baolin.wang@linaro.org> wrote:
> Hi Andy,
>
> On 28 June 2018 at 16:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Thu, Jun 28, 2018 at 8:16 AM, 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.
>>
>>> +What: /sys/class/leds/<led>/pattern
>>> +Date: June 2018
>>> +KernelVersion: 4.18
>>
>> 4.19 ?
>
> I think this will be merged in 4.18.
>
>>
>>> +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;
>>> +
>>
>> Perhaps just hide an attribute completely?
>
> Driver need implement the pattern_get() interface, otherwise we can
> not get any available pattern values to show.
>
>>
>>> +       pattern = led_cdev->pattern_get(led_cdev, &count);
>>> +       if (IS_ERR_OR_NULL(pattern))
>>> +               return PTR_ERR(pattern);
>>
>> Hmm.. Here you shadow NULL case by returning 0.
>> Even if it's correct behaviour IS_ERR_OR_NULL is a beast to hide such
>> subtle detail.
>>
>> It also would be good idea to check for count == 0 and bail out
>> immediately. Otherwise you will print an extra blank line.
>
> We can not check count, since count can be not initialized if failed
> to call pattern_get(). So maybe force user to return error pointer,
> and we just check like:
> 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;
>>> +
>>
>>> +               if (i < count - 1)
>>> +                       buf[offset++] = ' ';
>>
>> You might add this to the end of above format string and remove this
>> conditional completely...
>
> Hmmm, I do not think we need add one extra ' ' to the end of format string.
>
>>
>>> +       }
>>> +
>>
>>> +       buf[offset++] = '\n';
>>
>> ...and use here something like
>>
>> buf[offset - 1] = '\n';
>
> I don't think so. We need increase the offset value at the same time.
>
>>
>> (we have such patterns in the kernel)
>>
>>> +
>>> +       kfree(pattern);
>>> +       return offset;
>>> +
>>> +err_nospc:
>>> +       kfree(pattern);
>>> +       return -ENOSPC;
>>> +}
>>> +
>>> +static ssize_t pattern_store(struct device *dev,
>>> +                            struct device_attribute *attr,
>>> +                            const char *buf, size_t size)
>>> +{
>>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> +       struct led_pattern *pattern = NULL;
>>> +       unsigned long val;
>>> +       char *sbegin;
>>> +       char *elem;
>>> +       char *s;
>>> +       int ret, len = 0;
>>> +       bool odd = true;
>>> +
>>> +       s = sbegin = kstrndup(buf, size, GFP_KERNEL);
>>> +       if (!s)
>>> +               return -ENOMEM;
>>> +
>>
>>> +       /* Trim trailing newline */
>>> +       s[strcspn(s, "\n")] = '\0';
>>
>> It's usually done via strstrip().
>>
>> sbegin = kstrndup();
>> ...
>>
>> s = strstrip(sbegin);
>
> Good idea, will change.

Wait, we can not use strstrip() here, strstrip() is used to remove
leading and trailing whitespace, this is not the case what we handled
here.

>
>>
>>> +
>>> +       /* If the remaining string is empty, clear the pattern */
>>> +       if (!s[0]) {
>>
>> if (!*s) ?
>
> OK.
>
>>
>>> +               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) {
>>
>> This is effectivelly if (len % 2 == 0)
>
> It is incorrect, we can not use len to decide the value is brightness
> or delta. Here logical is to make sure we must keep <brightness
> delta>, <brightness, delta> ......
>
>>
>>> +                       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;
>>> +       }
>>
>> For partial data can we return different error code?
>> Does it make sense?
>
> Sorry I did not get you here. If user set incorrect pattern values, I
> think '-EINVAL' is suitable. Thanks for your comments.
>
> --
> Baolin.wang
> Best Regards



-- 
Baolin.wang
Best Regards

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

* Re: [PATCH v2 1/2] leds: core: Introduce generic pattern interface
  2018-06-28  9:18   ` Baolin Wang
  2018-06-28  9:25     ` Baolin Wang
@ 2018-06-28 10:24     ` Andy Shevchenko
  2018-06-28 11:20       ` Baolin Wang
  1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2018-06-28 10:24 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Jacek Anaszewski, Pavel Machek, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, Linux Kernel Mailing List

On Thu, Jun 28, 2018 at 12:18 PM, Baolin Wang <baolin.wang@linaro.org> wrote:
> On 28 June 2018 at 16:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Thu, Jun 28, 2018 at 8:16 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

>>> +What: /sys/class/leds/<led>/pattern
>>> +Date: June 2018
>>> +KernelVersion: 4.18
>>
>> 4.19 ?
>
> I think this will be merged in 4.18.

Is it bug fix? I don't see how it would make v4.18.

>>> +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;
>>> +
>>
>> Perhaps just hide an attribute completely?
>
> Driver need implement the pattern_get() interface, otherwise we can
> not get any available pattern values to show.

It doesn't contradict with what I said. I proposed to just hide an
attribute from sysfs completely if there is no such callbacks
available.

>>> +       pattern = led_cdev->pattern_get(led_cdev, &count);
>>> +       if (IS_ERR_OR_NULL(pattern))
>>> +               return PTR_ERR(pattern);
>>
>> Hmm.. Here you shadow NULL case by returning 0.
>> Even if it's correct behaviour IS_ERR_OR_NULL is a beast to hide such
>> subtle detail.
>>
>> It also would be good idea to check for count == 0 and bail out
>> immediately. Otherwise you will print an extra blank line.
>
> We can not check count, since count can be not initialized if failed
> to call pattern_get(). So maybe force user to return error pointer,
> and we just check like:
> if (IS_ERR(pattern))
>         return PTR_ERR(pattern);

My question is can be counter 0 by some reason?

>>> +       for (i = 0; i < count; i++) {
>>> +               n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d",
>>> +                            pattern[i].brightness, pattern[i].delta_t);
>>> +
>>
>>> +               if (offset + n >= PAGE_SIZE)
>>> +                       goto err_nospc;
>>
>>> +
>>> +               offset += n;
>>> +
>>
>>> +               if (i < count - 1)
>>> +                       buf[offset++] = ' ';
>>
>> You might add this to the end of above format string and remove this
>> conditional completely...
>
> Hmmm, I do not think we need add one extra ' ' to the end of format string.

Why not? You do it anyway for all except last, but...

>>> +       buf[offset++] = '\n';
>>
>> ...and use here something like
>>
>> buf[offset - 1] = '\n';
>
> I don't think so. We need increase the offset value at the same time.

Nope, you don't need it. here we replace trailing space by '\n'.

>> (we have such patterns in the kernel)

...look at existing patterns in the kernel.

>>> +       /* Trim trailing newline */
>>> +       s[strcspn(s, "\n")] = '\0';
>>
>> It's usually done via strstrip().
>>
>> sbegin = kstrndup();
>> ...
>>
>> s = strstrip(sbegin);
>
> Good idea, will change.

Replying to your another message. And who cares about leading spaces?
Is it wrong to have them? Why?

>>> +       /* Parse out the brightness & delta_t touples */
>>> +       while ((elem = strsep(&s, " ")) != NULL) {
>>> +               ret = kstrtoul(elem, 10, &val);
>>> +               if (ret)
>>> +                       goto out;
>>> +
>>
>>> +               if (odd) {
>>
>> This is effectivelly if (len % 2 == 0)
>
> It is incorrect, we can not use len to decide the value is brightness
> or delta. Here logical is to make sure we must keep <brightness
> delta>, <brightness, delta> ......

Right, the problem is the format itself I suppose.
It's too error prone for one attribute.

What about to change it like

"brightness delta[, brightness delta[, ...]]"

and then you do

...elem = strsep(&s, ",")...

sscanf("%d %d") == 2

?

>>> +                       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;
>>> +       }
>>
>> For partial data can we return different error code?
>> Does it make sense?
>
> Sorry I did not get you here. If user set incorrect pattern values, I
> think '-EINVAL' is suitable.

OK.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 1/2] leds: core: Introduce generic pattern interface
  2018-06-28 10:24     ` Andy Shevchenko
@ 2018-06-28 11:20       ` Baolin Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Baolin Wang @ 2018-06-28 11:20 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jacek Anaszewski, Pavel Machek, Bjorn Andersson, Mark Brown,
	Linux LED Subsystem, Linux Kernel Mailing List

On 28 June 2018 at 18:24, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Jun 28, 2018 at 12:18 PM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On 28 June 2018 at 16:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Thu, Jun 28, 2018 at 8:16 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>>>> +What: /sys/class/leds/<led>/pattern
>>>> +Date: June 2018
>>>> +KernelVersion: 4.18
>>>
>>> 4.19 ?
>>
>> I think this will be merged in 4.18.
>
> Is it bug fix? I don't see how it would make v4.18.

OK. 4.19 is fine.

>>>> +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;
>>>> +
>>>
>>> Perhaps just hide an attribute completely?
>>
>> Driver need implement the pattern_get() interface, otherwise we can
>> not get any available pattern values to show.
>
> It doesn't contradict with what I said. I proposed to just hide an
> attribute from sysfs completely if there is no such callbacks
> available.

Ah, I got your points now. But the pattern is RW and we can not hide
it if only pattern_get() is not supplied without considering
pattern_set().

>
>>>> +       pattern = led_cdev->pattern_get(led_cdev, &count);
>>>> +       if (IS_ERR_OR_NULL(pattern))
>>>> +               return PTR_ERR(pattern);
>>>
>>> Hmm.. Here you shadow NULL case by returning 0.
>>> Even if it's correct behaviour IS_ERR_OR_NULL is a beast to hide such
>>> subtle detail.
>>>
>>> It also would be good idea to check for count == 0 and bail out
>>> immediately. Otherwise you will print an extra blank line.
>>
>> We can not check count, since count can be not initialized if failed
>> to call pattern_get(). So maybe force user to return error pointer,
>> and we just check like:
>> if (IS_ERR(pattern))
>>         return PTR_ERR(pattern);
>
> My question is can be counter 0 by some reason?

I am not sure, but I still think checking the return error pointer is
the common way to handle.

>>>> +       for (i = 0; i < count; i++) {
>>>> +               n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d",
>>>> +                            pattern[i].brightness, pattern[i].delta_t);
>>>> +
>>>
>>>> +               if (offset + n >= PAGE_SIZE)
>>>> +                       goto err_nospc;
>>>
>>>> +
>>>> +               offset += n;
>>>> +
>>>
>>>> +               if (i < count - 1)
>>>> +                       buf[offset++] = ' ';
>>>
>>> You might add this to the end of above format string and remove this
>>> conditional completely...
>>
>> Hmmm, I do not think we need add one extra ' ' to the end of format string.
>
> Why not? You do it anyway for all except last, but...

Previous whitespace is used to break up the brightness value and
delta_t value, but it is useless to add one extra ' ' to the end of
format string.

>
>>>> +       buf[offset++] = '\n';
>>>
>>> ...and use here something like
>>>
>>> buf[offset - 1] = '\n';
>>
>> I don't think so. We need increase the offset value at the same time.
>
> Nope, you don't need it. here we replace trailing space by '\n'.

But why we need add one extra useless space?

>
>>> (we have such patterns in the kernel)
>
> ...look at existing patterns in the kernel.
>
>>>> +       /* Trim trailing newline */
>>>> +       s[strcspn(s, "\n")] = '\0';
>>>
>>> It's usually done via strstrip().
>>>
>>> sbegin = kstrndup();
>>> ...
>>>
>>> s = strstrip(sbegin);
>>
>> Good idea, will change.
>
> Replying to your another message. And who cares about leading spaces?
> Is it wrong to have them? Why?

OK, will use strstrip() to remove leading and trailing space firstly.

>
>>>> +       /* Parse out the brightness & delta_t touples */
>>>> +       while ((elem = strsep(&s, " ")) != NULL) {
>>>> +               ret = kstrtoul(elem, 10, &val);
>>>> +               if (ret)
>>>> +                       goto out;
>>>> +
>>>
>>>> +               if (odd) {
>>>
>>> This is effectivelly if (len % 2 == 0)
>>
>> It is incorrect, we can not use len to decide the value is brightness
>> or delta. Here logical is to make sure we must keep <brightness
>> delta>, <brightness, delta> ......
>
> Right, the problem is the format itself I suppose.
> It's too error prone for one attribute.

The format was decided by previous discussion, so I still like to keep
the original approach here.

>
> What about to change it like
>
> "brightness delta[, brightness delta[, ...]]"
>
> and then you do
>
> ...elem = strsep(&s, ",")...
>
> sscanf("%d %d") == 2
>
> ?
>
>>>> +                       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;
>>>> +       }
>>>
>>> For partial data can we return different error code?
>>> Does it make sense?
>>
>> Sorry I did not get you here. If user set incorrect pattern values, I
>> think '-EINVAL' is suitable.
>
> OK.
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
Baolin.wang
Best Regards

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

end of thread, other threads:[~2018-06-28 11:20 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-28  5:16 [PATCH v2 1/2] leds: core: Introduce generic pattern interface Baolin Wang
2018-06-28  5:16 ` [PATCH v2 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller Baolin Wang
2018-06-28  8:31 ` [PATCH v2 1/2] leds: core: Introduce generic pattern interface Andy Shevchenko
2018-06-28  9:18   ` Baolin Wang
2018-06-28  9:25     ` Baolin Wang
2018-06-28 10:24     ` Andy Shevchenko
2018-06-28 11:20       ` Baolin Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).