lkml.org 
[lkml]   [2018]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 1/2] leds: core: Introduce generic pattern interface
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

\
 
 \ /
  Last update: 2018-06-28 11:20    [W:0.072 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site