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 <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
---
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/<led>/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/<led>/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 <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+
+#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);
+ 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;
+ 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 <[email protected]");
+MODULE_AUTHOR("Baolin Wang <[email protected]");
+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 <linux/workqueue.h>
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 */
--
1.7.9.5
This patch implements the 'pattern_set'and 'pattern_clear'
interfaces to support SC27XX LED breathing mode.
Signed-off-by: Baolin Wang <[email protected]>
---
changes from v1:
- Remove pattern_get interface.
---
drivers/leds/leds-sc27xx-bltc.c | 94 +++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index 9d9b7aa..297dd43 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -32,8 +32,13 @@
#define SC27XX_DUTY_MASK GENMASK(15, 0)
#define SC27XX_MOD_MASK GENMASK(7, 0)
+#define SC27XX_CURVE_SHIFT 8
+#define SC27XX_CURVE_L_MASK GENMASK(7, 0)
+#define SC27XX_CURVE_H_MASK GENMASK(15, 8)
+
#define SC27XX_LEDS_OFFSET 0x10
#define SC27XX_LEDS_MAX 3
+#define SC27XX_LEDS_PATTERN_CNT 4
struct sc27xx_led {
char name[LED_MAX_NAME_SIZE];
@@ -122,6 +127,91 @@ static int sc27xx_led_set(struct led_classdev *ldev, enum led_brightness value)
return err;
}
+static int sc27xx_led_pattern_clear(struct led_classdev *ldev)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ struct regmap *regmap = leds->priv->regmap;
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ int err;
+
+ mutex_lock(&leds->priv->lock);
+
+ /* Reset the rise, high, fall and low time to zero. */
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE0, 0);
+ regmap_write(regmap, base + SC27XX_LEDS_CURVE1, 0);
+
+ err = regmap_update_bits(regmap, ctrl_base,
+ (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, 0);
+
+ mutex_unlock(&leds->priv->lock);
+
+ return err;
+}
+
+static int sc27xx_led_pattern_set(struct led_classdev *ldev,
+ struct led_pattern *pattern,
+ int len, u32 repeat)
+{
+ struct sc27xx_led *leds = to_sc27xx_led(ldev);
+ u32 base = sc27xx_led_get_offset(leds);
+ u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL;
+ u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line;
+ struct regmap *regmap = leds->priv->regmap;
+ int err;
+
+ /*
+ * Must contain 4 patterns to configure the rise time, high time, fall
+ * time and low time to enable the breathing mode.
+ */
+ if (len != SC27XX_LEDS_PATTERN_CNT)
+ return -EINVAL;
+
+ mutex_lock(&leds->priv->lock);
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+ SC27XX_CURVE_L_MASK, pattern[0].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+ SC27XX_CURVE_L_MASK, pattern[1].delta_t);
+ if (err)
+ goto out;
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE0,
+ SC27XX_CURVE_H_MASK,
+ pattern[2].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_CURVE1,
+ SC27XX_CURVE_H_MASK,
+ pattern[3].delta_t << SC27XX_CURVE_SHIFT);
+ if (err)
+ goto out;
+
+
+ err = regmap_update_bits(regmap, base + SC27XX_LEDS_DUTY,
+ SC27XX_DUTY_MASK,
+ (pattern[0].brightness << SC27XX_DUTY_SHIFT) |
+ SC27XX_MOD_MASK);
+ if (err)
+ goto out;
+
+ /* Enable the LED breathing mode */
+ err = regmap_update_bits(regmap, ctrl_base,
+ SC27XX_LED_RUN << ctrl_shift,
+ SC27XX_LED_RUN << ctrl_shift);
+
+out:
+ mutex_unlock(&leds->priv->lock);
+
+ return err;
+}
+
static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv)
{
int i, err;
@@ -140,6 +230,9 @@ static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv)
led->priv = priv;
led->ldev.name = led->name;
led->ldev.brightness_set_blocking = sc27xx_led_set;
+ led->ldev.pattern_set = sc27xx_led_pattern_set;
+ led->ldev.pattern_clear = sc27xx_led_pattern_clear;
+ led->ldev.default_trigger = "pattern";
err = devm_led_classdev_register(dev, &led->ldev);
if (err)
@@ -241,4 +334,5 @@ static int sc27xx_led_remove(struct platform_device *pdev)
MODULE_DESCRIPTION("Spreadtrum SC27xx breathing light controller driver");
MODULE_AUTHOR("Xiaotong Lu <[email protected]>");
+MODULE_AUTHOR("Baolin Wang <[email protected]>");
MODULE_LICENSE("GPL v2");
--
1.7.9.5
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 <[email protected]>
> Signed-off-by: Baolin Wang <[email protected]>
> ---
> 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/<led>/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/<led>/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 <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +
> +#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 <[email protected]");
> +MODULE_AUTHOR("Baolin Wang <[email protected]");
> +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 <linux/workqueue.h>
>
> 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
On Thu 02 Aug 14:21 PDT 2018, Jacek Anaszewski wrote:
> On 08/01/2018 11:01 AM, Baolin Wang wrote:
[..]
> > diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c
[..]
> > +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 agree, a failure to parse the entire pattern should result in an error
returned, rather than just silently truncating the pattern.
> 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.
>
I would prefer to see that we check that we have consumed the entire
input (accepting optional \n end), rather than having to prepend a count
to the string...
Regards,
Bjorn
Hi Jacek,
On 3 August 2018 at 05:21, Jacek Anaszewski <[email protected]> wrote:
> 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 <[email protected]>
>> Signed-off-by: Baolin Wang <[email protected]>
>> ---
>> 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/<led>/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/<led>/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 <linux/kernel.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/slab.h>
>> +#include <linux/timer.h>
>> +
>> +#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.
OK, I understand your concern. I will fix the issue without
introducing another pattern number. So I will not use a comma to
separate the patterns and change the pattern format as:
brightness1 time1 brightness2 time2 brightness3 time3 brightness4 time4 ...
Then we will easy to check out errors if user gives one incorrect
pattern string.
>
>> + 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.
Hmm, I am not sure I catch your points. If we remove the
hardware_pattern boolean, we still need to add check before
'del_timer_sync(&data->timer)' (In hardware pattern mode, we do not
need to delete the timer), also we still need to add check before
issuing pattern_set()/pattern_clear().
So I think introducing one boolean will make code easy to understand
what pattern mode (software pattern or hardware pattern) is using.
Thanks.
>
>> + 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 <[email protected]");
>> +MODULE_AUTHOR("Baolin Wang <[email protected]");
>> +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 <linux/workqueue.h>
>>
>> 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
--
Baolin Wang
Best Regards
Hi Baolin,
On 08/03/2018 10:05 AM, Baolin Wang wrote:
> Hi Jacek,
>
> On 3 August 2018 at 05:21, Jacek Anaszewski <[email protected]> wrote:
>> 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 <[email protected]>
>>> Signed-off-by: Baolin Wang <[email protected]>
>>> ---
>>> 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/<led>/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/<led>/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 <linux/kernel.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/timer.h>
>>> +
>>> +#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.
>
> OK, I understand your concern. I will fix the issue without
> introducing another pattern number. So I will not use a comma to
> separate the patterns and change the pattern format as:
> brightness1 time1 brightness2 time2 brightness3 time3 brightness4 time4 ...
>
> Then we will easy to check out errors if user gives one incorrect
> pattern string.
It would allow to eliminate only comma related typos, but what if user
provides other than numerical character? The truncation will occur
as well.
>>> + 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.
>
> Hmm, I am not sure I catch your points. If we remove the
> hardware_pattern boolean, we still need to add check before
> 'del_timer_sync(&data->timer)' (In hardware pattern mode, we do not
> need to delete the timer), also we still need to add check before
> issuing pattern_set()/pattern_clear().
If both pattern_set and pattern_get ops are initialized then we are
in the hardware pattern mode.
I forgot to nullify pattern_{set|get} ops in the proposed check above,
so it should be:
if (!!led_cdev->pattern_set ^ !!led_cdev->pattern_clear) {
dev_warn(led_cdev->dev, "Hardware pattern ops validation failed");
led_cdev->pattern_set = led_cdev->pattern_get = NULL;
}
From this moment the state of pattern_set alone describes the
mode we are in.
No need to introduce another variable for that.
Best regards,
Jacek Anaszewski
> So I think introducing one boolean will make code easy to understand
> what pattern mode (software pattern or hardware pattern) is using.
> Thanks.
>
>>
>>> + 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 <[email protected]");
>>> +MODULE_AUTHOR("Baolin Wang <[email protected]");
>>> +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 <linux/workqueue.h>
>>>
>>> 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
>
>
>
Hi Jacek,
On 3 August 2018 at 19:59, Jacek Anaszewski <[email protected]> wrote:
> Hi Baolin,
>
> On 08/03/2018 10:05 AM, Baolin Wang wrote:
>> Hi Jacek,
>>
>> On 3 August 2018 at 05:21, Jacek Anaszewski <[email protected]> wrote:
>>> 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 <[email protected]>
>>>> Signed-off-by: Baolin Wang <[email protected]>
>>>> ---
>>>> 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/<led>/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/<led>/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 <linux/kernel.h>
>>>> +#include <linux/leds.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/timer.h>
>>>> +
>>>> +#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.
>>
>> OK, I understand your concern. I will fix the issue without
>> introducing another pattern number. So I will not use a comma to
>> separate the patterns and change the pattern format as:
>> brightness1 time1 brightness2 time2 brightness3 time3 brightness4 time4 ...
>>
>> Then we will easy to check out errors if user gives one incorrect
>> pattern string.
>
> It would allow to eliminate only comma related typos, but what if user
> provides other than numerical character? The truncation will occur
> as well.
It will return -EINVAL error instead of the truncation.
echo " 50 100 100 100 50 100 0 100 " > pattern --- Correct pattern string
echo "a 100 100 100 50 100 0 100 " > pattern --- Incorrect pattern
string, will return error
echo "50 100 100 100 50 100 0 100 50" > pattern --- Incorrect
pattern string, will return error
So It will not truncate the string if user provides one incorrect
pattern string.
But if we use one comma as the pattern separator, it will introduce
complexity to valid below cases with sccanf function:
echo "50 100, 100 100, 50 100, 0 100" > pattern
echo "50 100, 100 100, 50 100, 0 100," > pattern
echo "50 100, 100 100, 50 100, 0 100, " > pattern
echo "50 100, 100 100, 50 100, 0 100 200" > pattern
echo "50 100, 100 100, 50 100 100, 0 100" > pattern
echo "50 100, 100 100, 50 100, 0 100 200," > pattern
echo "50 100, 100 100, 50 100, 0 100 200 " > pattern
So could you try the new version I send out, I think it will cover the
cases you mentioned.
>>>> + 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.
>>
>> Hmm, I am not sure I catch your points. If we remove the
>> hardware_pattern boolean, we still need to add check before
>> 'del_timer_sync(&data->timer)' (In hardware pattern mode, we do not
>> need to delete the timer), also we still need to add check before
>> issuing pattern_set()/pattern_clear().
>
> If both pattern_set and pattern_get ops are initialized then we are
> in the hardware pattern mode.
>
> I forgot to nullify pattern_{set|get} ops in the proposed check above,
> so it should be:
>
> if (!!led_cdev->pattern_set ^ !!led_cdev->pattern_clear) {
> dev_warn(led_cdev->dev, "Hardware pattern ops validation failed");
> led_cdev->pattern_set = led_cdev->pattern_get = NULL;
> }
>
> From this moment the state of pattern_set alone describes the
> mode we are in.
> No need to introduce another variable for that.
OK, will do. Thanks for your comments.
--
Baolin Wang
Best Regards