lkml.org 
[lkml]   [2018]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 1/2] leds: core: Introduce generic pattern interface
    Hi Jacek,

    On 13 July 2018 at 05:41, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
    > 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@gmail.com>
    >> 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@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.
    >>>>>
    >>>>> [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 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.

    Yes, you are right. OK, I will add some documentation for this. Thanks.

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



    --
    Baolin Wang
    Best Regards
    \
     
     \ /
      Last update: 2018-07-15 22:06    [W:4.438 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site