Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp81371imm; Thu, 2 Aug 2018 14:22:53 -0700 (PDT) X-Google-Smtp-Source: AAOMgpc6bT6pzXTgNSF9pjTgq6wVcvEkWgY4HSznb6ny4OI/gkMBmKsPy0KNwRo0Npg8klh0DL61 X-Received: by 2002:a65:560a:: with SMTP id l10-v6mr1072095pgs.130.1533244973238; Thu, 02 Aug 2018 14:22:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1533244973; cv=none; d=google.com; s=arc-20160816; b=LDOIjsCv6P3I1tlMujVQxKhlWj91deMI42gE+oGivHFoJD7nioERfnOJxZ/YTqSS0X 0l1PbSKYQZiEaK7VUOU3bHk6RM1eCSnam36Wz2/7SnHNVSzB/nFhK6oy+/jnn28f+X/Y SuN95Zm6y+iN6MueYVC76o/rV2aSs+ycpy3l/962GIUrK/RmcNNqgHB7HvyFxq2vfeAV xdf+eP2hVsggg5UvV/dq21MqcB8mhXfDD1lX7mASBrMg/dOZOWFFMlf3FBh5ZLErGBia IO+I0afZ31qy3nK+1L5ifdENTDtWK4I4GxZMBPRoN8mUnzXTMPRB3yKUs6qq5M1wSXYr T4vQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:dkim-signature :arc-authentication-results; bh=UK4Uvv/dE645/bPlfXlAzUCsCELs35vPp6xOUlPZb2w=; b=Um7sj01fCE4zAbgyEhMV7i2WVjOOoltUehxw3aXRJBQHHuPSI5h92zw9NK9R6QjvzU e+heaLp9hqBmb8H2kDUILWrqOpB5T8+0wAGonVwyO/DNPy+bQLExLgB97aMavp3bPW2w KwOFRuIX6Miom3wjchFn2d/zX6aYdtfiME4jqvNsj4F+sfZb/zpR49XryPNxkqG7bFKo FBcWBNxXtrK3EclW4LZ16wa9iUjW7AbvrBREzCwWnPvJSNdzxHbkhuBXDuOs33BNb9iL YGdJEYq9qxCVB2xdOqwQHLm4hv4dzqYu3I1yRGIXSWpCzNO497cmARLmLVO96W2aD3cc mDww== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=XR1FRbFX; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id r21-v6si3017298pgi.690.2018.08.02.14.22.36; Thu, 02 Aug 2018 14:22:53 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=XR1FRbFX; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727439AbeHBXOg (ORCPT + 99 others); Thu, 2 Aug 2018 19:14:36 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:52165 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726881AbeHBXOg (ORCPT ); Thu, 2 Aug 2018 19:14:36 -0400 Received: by mail-wm0-f67.google.com with SMTP id y2-v6so4063208wma.1; Thu, 02 Aug 2018 14:21:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=UK4Uvv/dE645/bPlfXlAzUCsCELs35vPp6xOUlPZb2w=; b=XR1FRbFXoRhHLAAv+6jTTq/VuAexRkYbviDSWDNkpele+SmBLZRF4HGYfiImf54q19 qC315Zca9cdDYWd2MUtkwwlb2uakD4G63mRA6i2BQhlIfcOgDSSgyx0fNLqxql/oJrj9 rumZ4kWawoWKzd1RfeK5atp5EjrVD77jkMrmnQxSsHTBynbwuo2kYSPXNWR5g3uVusH9 gBB68eZ7NMAUwUuu1mjd9AzS6i0G+dYokf22l4xqKrkyLvZZvpg8qbd0Q6VL9VneoKJs xVHXDWrOK/ulhokqiJaBqxviXMtwXLD5mrFqCJwHFlE14p5JdzDbx3YnRcUpCDacCkZ7 C5DQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=UK4Uvv/dE645/bPlfXlAzUCsCELs35vPp6xOUlPZb2w=; b=aTOU0Xgqn7/Syi63v9c2V4EjeB4Cujvd3OSCEYoKT+RxvFbctIpY36gqEliZLkWXwu hGav7KNhqS+qV9oO/afDhdU9KZqm5Usua2JKEQLtUROeuX9W2cFiKHs4LmDt1q9qoc8t 2eOSLFxJzBh4prdWiITbkxq/eeKWLJSJbZHbpjkpOrMHfKKckdqUThfcQBvg49A2pg5o A6+BI/jp4hDqs7J4ExSJGST6MCa4Q7zi+iF4gqQZf8GlOv732Uh0RxgsU3ApP8yH9WgS rsi3KqoIZCrQUfySKegr6i925QI3+O+Qd4F9a6JiQNMjNXl0Mc4VIPBjtJfyttORPbWw al6Q== X-Gm-Message-State: AOUpUlHehimYV8HbSD6EtqQpyTivqlKpsKGUA3xtX4XPZE5gLsYajEpL NX/L1uZ2dGOHYVhoFSPbhs9KWebx X-Received: by 2002:a1c:8b81:: with SMTP id n123-v6mr3093868wmd.142.1533244897812; Thu, 02 Aug 2018 14:21:37 -0700 (PDT) Received: from [192.168.1.18] (dlw114.neoplus.adsl.tpnet.pl. [83.24.52.114]) by smtp.gmail.com with ESMTPSA id b202-v6sm4180737wme.22.2018.08.02.14.21.36 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Aug 2018 14:21:36 -0700 (PDT) Subject: Re: [PATCH v2 1/2] leds: core: Introduce LED pattern trigger To: Baolin Wang , pavel@ucw.cz Cc: rteysseyre@gmail.com, bjorn.andersson@linaro.org, david@lechnology.com, broonie@kernel.org, linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org References: <1bf9aed83d1756dd4f81590297ce7e0b9972bf85.1533112637.git.baolin.wang@linaro.org> From: Jacek Anaszewski Message-ID: <34c87589-ccaf-e541-7acb-efcf3bd306f6@gmail.com> Date: Thu, 2 Aug 2018 23:21:34 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1bf9aed83d1756dd4f81590297ce7e0b9972bf85.1533112637.git.baolin.wang@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Baolin, Thank you for addressing review remarks. I've played a bit with the interface and I have one conclusion regarding pattern parsing, please refer below. Also one tiny optimization request in pattern_trig_activate(). On 08/01/2018 11:01 AM, Baolin Wang wrote: > Some LED controllers have support for autonomously controlling > brightness over time, according to some preprogrammed pattern or > function. > > This patch adds pattern trigger that LED device can configure the > pattern and trigger it. > > Signed-off-by: Raphael Teysseyre > Signed-off-by: Baolin Wang > --- > Changes from v1: > - Use ATTRIBUTE_GROUPS() to define attributes. > - Introduce hardware_pattern flag to determine if software pattern > or hardware pattern. > - Re-implement pattern_trig_store_pattern() function. > - Remove pattern_get() interface. > - Improve comments. > - Other small optimization. > --- > .../ABI/testing/sysfs-class-led-trigger-pattern | 21 ++ > drivers/leds/trigger/Kconfig | 7 + > drivers/leds/trigger/Makefile | 1 + > drivers/leds/trigger/ledtrig-pattern.c | 273 ++++++++++++++++++++ > include/linux/leds.h | 16 ++ > 5 files changed, 318 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern > create mode 100644 drivers/leds/trigger/ledtrig-pattern.c > > diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern > new file mode 100644 > index 0000000..f9a4ac0 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern > @@ -0,0 +1,21 @@ > +What: /sys/class/leds//pattern > +Date: August 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. > + > + The format of the pattern values should be: > + "brightness_1 duration_1, brightness_2 duration_2, brightness_3 > + duration_3 ...". > + > +What: /sys/class/leds//repeat > +Date: August 2018 > +KernelVersion: 4.19 > +Description: > + Specify a pattern repeat number. 0 means repeat indefinitely. > diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig > index 4018af7..b76fc3c 100644 > --- a/drivers/leds/trigger/Kconfig > +++ b/drivers/leds/trigger/Kconfig > @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV > This allows LEDs to be controlled by network device activity. > If unsure, say Y. > > +config LEDS_TRIGGER_PATTERN > + tristate "LED Pattern Trigger" > + help > + This allows LEDs to be controlled by a software or hardware pattern > + which is a series of tuples, of brightness and duration (ms). > + If unsure, say N > + > endif # LEDS_TRIGGERS > diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile > index f3cfe19..9bcb64e 100644 > --- a/drivers/leds/trigger/Makefile > +++ b/drivers/leds/trigger/Makefile > @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o > obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o > obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o > obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o > +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o > diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c > new file mode 100644 > index 0000000..006874b > --- /dev/null > +++ b/drivers/leds/trigger/ledtrig-pattern.c > @@ -0,0 +1,273 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * LED pattern trigger > + * > + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented > + * the first version, Baolin Wang simplified and improved the approach. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define MAX_PATTERNS 1024 > +#define PATTERN_SEPARATOR "," > + > +struct pattern_trig_data { > + struct led_classdev *led_cdev; > + struct led_pattern patterns[MAX_PATTERNS]; > + struct led_pattern *curr; > + struct led_pattern *next; > + struct mutex lock; > + u32 npatterns; > + u32 repeat; > + bool is_indefinite; > + bool hardware_pattern; > + struct timer_list timer; > +}; > + > +static void pattern_trig_update_patterns(struct pattern_trig_data *data) > +{ > + data->curr = data->next; > + if (!data->is_indefinite && data->curr == data->patterns) > + data->repeat--; > + > + if (data->next == data->patterns + data->npatterns - 1) > + data->next = data->patterns; > + else > + data->next++; > +} > + > +static void pattern_trig_timer_function(struct timer_list *t) > +{ > + struct pattern_trig_data *data = from_timer(data, t, timer); > + > + mutex_lock(&data->lock); > + > + if (!data->is_indefinite && !data->repeat) { > + mutex_unlock(&data->lock); > + return; > + } > + > + led_set_brightness(data->led_cdev, data->curr->brightness); > + mod_timer(&data->timer, jiffies + msecs_to_jiffies(data->curr->delta_t)); > + pattern_trig_update_patterns(data); > + > + mutex_unlock(&data->lock); > +} > + > +static int pattern_trig_start_pattern(struct pattern_trig_data *data, > + struct led_classdev *led_cdev) > +{ > + if (!data->npatterns) > + return 0; > + > + if (data->hardware_pattern) { > + return led_cdev->pattern_set(led_cdev, data->patterns, > + data->npatterns, data->repeat); > + } > + > + data->curr = data->patterns; > + data->next = data->patterns + 1; > + data->timer.expires = jiffies; > + add_timer(&data->timer); > + > + return 0; > +} > + > +static ssize_t pattern_trig_show_repeat(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct pattern_trig_data *data = led_cdev->trigger_data; > + u32 repeat; > + > + mutex_lock(&data->lock); > + > + repeat = data->repeat; > + > + mutex_unlock(&data->lock); > + > + return scnprintf(buf, PAGE_SIZE, "%u\n", repeat); > +} > + > +static ssize_t pattern_trig_store_repeat(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct pattern_trig_data *data = led_cdev->trigger_data; > + unsigned long res; > + int err; > + > + err = kstrtoul(buf, 10, &res); > + if (err) > + return err; > + > + if (!data->hardware_pattern) > + del_timer_sync(&data->timer); > + > + mutex_lock(&data->lock); > + > + data->repeat = res; > + > + /* 0 means repeat indefinitely */ > + if (!data->repeat) > + data->is_indefinite = true; > + else > + data->is_indefinite = false; > + > + err = pattern_trig_start_pattern(data, led_cdev); > + > + mutex_unlock(&data->lock); > + return err < 0 ? err : count;; > +} > + > +static DEVICE_ATTR(repeat, 0644, pattern_trig_show_repeat, > + pattern_trig_store_repeat); > + > +static ssize_t pattern_trig_show_pattern(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct pattern_trig_data *data = led_cdev->trigger_data; > + ssize_t count = 0; > + int i; > + > + mutex_lock(&data->lock); > + > + if (!data->npatterns) > + goto out; > + > + for (i = 0; i < data->npatterns; i++) { > + count += scnprintf(buf + count, PAGE_SIZE - count, > + "%d %d" PATTERN_SEPARATOR, > + data->patterns[i].brightness, > + data->patterns[i].delta_t); > + } > + > + buf[count - 1] = '\n'; > + > +out: > + mutex_unlock(&data->lock); > + return count; > +} > + > +static ssize_t pattern_trig_store_pattern(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct pattern_trig_data *data = led_cdev->trigger_data; > + int cr, ccount, offset = 0, err = 0; > + > + if (!data->hardware_pattern) > + del_timer_sync(&data->timer); > + > + mutex_lock(&data->lock); > + > + data->npatterns = 0; > + while (offset < count - 1 && data->npatterns < MAX_PATTERNS) { > + cr = 0; > + ccount = sscanf(buf + offset, "%d %d " PATTERN_SEPARATOR "%n", > + &data->patterns[data->npatterns].brightness, > + &data->patterns[data->npatterns].delta_t, &cr); In case user makes a typo while constructing list of pattern tuples, e.g. he forgets a comma, the pattern gets silently truncated. User is able to detect the truncation by reading pattern file, but it is not an immediate feedback anyway. I propose that pattern format should require number of tuples in the first position which would allow to get rid of this ambiguity, since we could verify if the number of parsed tuples is as intended. > + if (ccount != 2) { > + err = -EINVAL; > + goto out; > + } > + > + offset += cr; > + data->npatterns++; > + /* end of pattern */ > + if (!cr) > + break; > + } > + > + err = pattern_trig_start_pattern(data, led_cdev); > + > +out: > + mutex_unlock(&data->lock); > + return err < 0 ? err : count; > +} > + > +static DEVICE_ATTR(pattern, 0644, pattern_trig_show_pattern, > + pattern_trig_store_pattern); > + > +static struct attribute *pattern_trig_attrs[] = { > + &dev_attr_pattern.attr, > + &dev_attr_repeat.attr, > + NULL > +}; > +ATTRIBUTE_GROUPS(pattern_trig); > + > +static int pattern_trig_activate(struct led_classdev *led_cdev) > +{ > + struct pattern_trig_data *data; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + if (led_cdev->pattern_set && led_cdev->pattern_clear) > + data->hardware_pattern = true; Instead of introducing hardware_pattern boolean, let's log warning message here in case: if (!!led_cdev->pattern_set ^ !!led_cdev->pattern_clear) dev_warn(led_cdev->dev, "Hardware pattern ops validation failed"); And later, having this validation behind us, we can be sure that we have hardware pattern support when led_cdev->pattern_set is initialized. > + else > + data->hardware_pattern = false; > + > + data->is_indefinite = true; > + mutex_init(&data->lock); > + data->led_cdev = led_cdev; > + led_set_trigger_data(led_cdev, data); > + timer_setup(&data->timer, pattern_trig_timer_function, 0); > + led_cdev->activated = true; > + > + return 0; > +} > + > +static void pattern_trig_deactivate(struct led_classdev *led_cdev) > +{ > + struct pattern_trig_data *data = led_cdev->trigger_data; > + > + if (!led_cdev->activated) > + return; > + > + if (data->hardware_pattern) > + led_cdev->pattern_clear(led_cdev); > + else > + del_timer_sync(&data->timer); > + > + led_set_brightness(led_cdev, LED_OFF); > + kfree(data); > + led_cdev->activated = false; > +} > + > +static struct led_trigger pattern_led_trigger = { > + .name = "pattern", > + .activate = pattern_trig_activate, > + .deactivate = pattern_trig_deactivate, > + .groups = pattern_trig_groups, > +}; > + > +static int __init pattern_trig_init(void) > +{ > + return led_trigger_register(&pattern_led_trigger); > +} > + > +static void __exit pattern_trig_exit(void) > +{ > + led_trigger_unregister(&pattern_led_trigger); > +} > + > +module_init(pattern_trig_init); > +module_exit(pattern_trig_exit); > + > +MODULE_AUTHOR("Raphael Teysseyre +MODULE_AUTHOR("Baolin Wang +MODULE_DESCRIPTION("LED Pattern trigger"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 834683d..c54712c 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -22,6 +22,7 @@ > #include > > struct device; > +struct led_pattern; > /* > * LED Core > */ > @@ -88,6 +89,11 @@ 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, > + unsigned repeat); > + int (*pattern_clear)(struct led_classdev *led_cdev); > + > struct device *dev; > const struct attribute_group **groups; > > @@ -472,4 +478,14 @@ static inline void led_classdev_notify_brightness_hw_changed( > struct led_classdev *led_cdev, enum led_brightness brightness) { } > #endif > > +/** > + * struct led_pattern - brightness 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 */ > -- Best regards, Jacek Anaszewski