On 28 June 2018 at 17:18, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
> Hi Andy,
>
> On 28 June 2018 at 16:31, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>> On Thu, Jun 28, 2018 at 8:16 AM, 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.
>>
>>> +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
