Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
From: Jacek Anaszewski
Date: Thu Jul 12 2018 - 17:41:10 EST
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@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
From: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
---
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