| Message ID | 03bc9a51ccae38cdc86934746aee9f75ec667cfd.1530162340.git.baolin.wang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] leds: core: Introduce generic pattern interface | expand |
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
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
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
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 */