* [PATCH v3 1/2] leds: core: Introduce generic pattern interface @ 2018-06-29 5:03 Baolin Wang 2018-06-29 5:03 ` [PATCH v3 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller Baolin Wang 2018-07-11 11:02 ` [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang 0 siblings, 2 replies; 38+ messages in thread From: Baolin Wang @ 2018-06-29 5:03 UTC (permalink / raw) To: jacek.anaszewski, pavel Cc: bjorn.andersson, baolin.wang, broonie, linux-leds, linux-kernel 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. --- 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; + + 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, }; static const struct attribute_group *led_groups[] = { diff --git a/include/linux/leds.h b/include/linux/leds.h index b7e8255..acdbb2f 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,14 @@ 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); + + int (*pattern_clear)(struct led_classdev *led_cdev); + + struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev, + int *len); + struct device *dev; const struct attribute_group **groups; @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed( struct led_classdev *led_cdev, enum led_brightness brightness) { } #endif +/** + * struct led_pattern - brigheness 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller 2018-06-29 5:03 [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang @ 2018-06-29 5:03 ` Baolin Wang 2018-07-11 11:02 ` [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang 1 sibling, 0 replies; 38+ messages in thread From: Baolin Wang @ 2018-06-29 5:03 UTC (permalink / raw) To: jacek.anaszewski, pavel Cc: bjorn.andersson, baolin.wang, broonie, linux-leds, linux-kernel This patch implements the 'pattern_set', 'pattern_get' and 'pattern_clear' interfaces to support SC27XX LED breathing mode. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- Changes from v2: - No updates. Changes from v1: - No updates. --- drivers/leds/leds-sc27xx-bltc.c | 160 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 160 insertions(+) diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c index 9d9b7aa..898f92d 100644 --- a/drivers/leds/leds-sc27xx-bltc.c +++ b/drivers/leds/leds-sc27xx-bltc.c @@ -6,6 +6,7 @@ #include <linux/of.h> #include <linux/platform_device.h> #include <linux/regmap.h> +#include <linux/slab.h> #include <uapi/linux/uleds.h> /* PMIC global control register definition */ @@ -32,8 +33,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 +128,157 @@ 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) +{ + 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 struct led_pattern *sc27xx_led_pattern_get(struct led_classdev *ldev, + int *len) +{ + struct sc27xx_led *leds = to_sc27xx_led(ldev); + u32 base = sc27xx_led_get_offset(leds); + struct regmap *regmap = leds->priv->regmap; + struct led_pattern *pattern; + int i, err; + u32 val; + + /* + * Must allocate 4 patterns to show the rise time, high time, fall time + * and low time. + */ + pattern = kcalloc(SC27XX_LEDS_PATTERN_CNT, sizeof(*pattern), + GFP_KERNEL); + if (!pattern) + return ERR_PTR(-ENOMEM); + + mutex_lock(&leds->priv->lock); + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); + if (err) + goto out; + + pattern[0].delta_t = val & SC27XX_CURVE_L_MASK; + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); + if (err) + goto out; + + pattern[1].delta_t = val & SC27XX_CURVE_L_MASK; + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); + if (err) + goto out; + + pattern[2].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); + if (err) + goto out; + + pattern[3].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; + + err = regmap_read(regmap, base + SC27XX_LEDS_DUTY, &val); + if (err) + goto out; + + mutex_unlock(&leds->priv->lock); + + val = (val & SC27XX_DUTY_MASK) >> SC27XX_DUTY_SHIFT; + for (i = 0; i < SC27XX_LEDS_PATTERN_CNT; i++) + pattern[i].brightness = val; + + *len = SC27XX_LEDS_PATTERN_CNT; + + return pattern; + +out: + mutex_unlock(&leds->priv->lock); + kfree(pattern); + + return ERR_PTR(err); +} + static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv) { int i, err; @@ -140,6 +297,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_get = sc27xx_led_pattern_get; + led->ldev.pattern_clear = sc27xx_led_pattern_clear; err = devm_led_classdev_register(dev, &led->ldev); if (err) -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-06-29 5:03 [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang 2018-06-29 5:03 ` [PATCH v3 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller Baolin Wang @ 2018-07-11 11:02 ` Baolin Wang 2018-07-11 21:10 ` Jacek Anaszewski 1 sibling, 1 reply; 38+ messages in thread From: Baolin Wang @ 2018-07-11 11:02 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: Bjorn Andersson, Baolin Wang, Mark Brown, Linux LED Subsystem, LKML 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. > 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; > + > + 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, > }; > > static const struct attribute_group *led_groups[] = { > diff --git a/include/linux/leds.h b/include/linux/leds.h > index b7e8255..acdbb2f 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,14 @@ 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); > + > + int (*pattern_clear)(struct led_classdev *led_cdev); > + > + struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev, > + int *len); > + > struct device *dev; > const struct attribute_group **groups; > > @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed( > struct led_classdev *led_cdev, enum led_brightness brightness) { } > #endif > > +/** > + * struct led_pattern - brigheness 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 > -- Baolin Wang Best Regards ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-11 11:02 ` [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang @ 2018-07-11 21:10 ` Jacek Anaszewski 2018-07-12 12:24 ` Baolin Wang 0 siblings, 1 reply; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-11 21:10 UTC (permalink / raw) To: Baolin Wang, Pavel Machek Cc: Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML 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. 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; } > >> 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. >> + 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(). >> }; >> >> static const struct attribute_group *led_groups[] = { >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index b7e8255..acdbb2f 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,14 @@ 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); >> + >> + int (*pattern_clear)(struct led_classdev *led_cdev); >> + >> + struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev, >> + int *len); >> + >> struct device *dev; >> const struct attribute_group **groups; >> >> @@ -446,4 +455,14 @@ static inline void led_classdev_notify_brightness_hw_changed( >> struct led_classdev *led_cdev, enum led_brightness brightness) { } >> #endif >> >> +/** >> + * struct led_pattern - brigheness 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 >> > > > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-11 21:10 ` Jacek Anaszewski @ 2018-07-12 12:24 ` Baolin Wang 2018-07-12 21:41 ` Jacek Anaszewski 2018-07-14 21:20 ` Pavel Machek 0 siblings, 2 replies; 38+ messages in thread From: Baolin Wang @ 2018-07-12 12:24 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML 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. > > 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? >> >>> 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. Or am I missed anything? Thanks for your comments. -- Baolin Wang Best Regards ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-12 12:24 ` Baolin Wang @ 2018-07-12 21:41 ` Jacek Anaszewski 2018-07-13 1:58 ` Baolin Wang 2018-07-14 21:20 ` Pavel Machek 1 sibling, 1 reply; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-12 21:41 UTC (permalink / raw) To: Baolin Wang Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML 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. >> 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-12 21:41 ` Jacek Anaszewski @ 2018-07-13 1:58 ` Baolin Wang 0 siblings, 0 replies; 38+ messages in thread From: Baolin Wang @ 2018-07-13 1:58 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-12 12:24 ` Baolin Wang 2018-07-12 21:41 ` Jacek Anaszewski @ 2018-07-14 21:20 ` Pavel Machek 2018-07-14 22:02 ` Jacek Anaszewski 1 sibling, 1 reply; 38+ messages in thread From: Pavel Machek @ 2018-07-14 21:20 UTC (permalink / raw) To: Baolin Wang Cc: Jacek Anaszewski, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 983 bytes --] Hi! > > 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. Let me take a step back: we have triggers.. like LED blinking. How is that going to interact with patterns? We probably want the patterns to be ignored in that case...? Which suggest to me that we should treat patterns as a trigger. I believe we do something similar with blinking already. Then it is easy to determine if pattern is active, and pattern vs. trigger issue is solved automatically. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-14 21:20 ` Pavel Machek @ 2018-07-14 22:02 ` Jacek Anaszewski 2018-07-14 22:29 ` Pavel Machek 0 siblings, 1 reply; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-14 22:02 UTC (permalink / raw) To: Pavel Machek, Baolin Wang Cc: Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML Hi Pavel, On 07/14/2018 11:20 PM, Pavel Machek wrote: > Hi! > >>> 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. > > Let me take a step back: we have triggers.. like LED blinking. > > How is that going to interact with patterns? We probably want the > patterns to be ignored in that case...? > > Which suggest to me that we should treat patterns as a trigger. I > believe we do something similar with blinking already. > > Then it is easy to determine if pattern is active, and pattern > vs. trigger issue is solved automatically. I'm all for it. I proposed this approach during the previous discussions related to possible pattern interface implementations, but you seemed not to be so enthusiastic in [0]. [0] https://lkml.org/lkml/2017/4/7/350 -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-14 22:02 ` Jacek Anaszewski @ 2018-07-14 22:29 ` Pavel Machek 2018-07-14 22:39 ` Pavel Machek 0 siblings, 1 reply; 38+ messages in thread From: Pavel Machek @ 2018-07-14 22:29 UTC (permalink / raw) To: Jacek Anaszewski Cc: Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 1560 bytes --] On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: > Hi Pavel, > > On 07/14/2018 11:20 PM, Pavel Machek wrote: > >Hi! > > > >>>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. > > > >Let me take a step back: we have triggers.. like LED blinking. > > > >How is that going to interact with patterns? We probably want the > >patterns to be ignored in that case...? > > > >Which suggest to me that we should treat patterns as a trigger. I > >believe we do something similar with blinking already. > > > >Then it is easy to determine if pattern is active, and pattern > >vs. trigger issue is solved automatically. > > I'm all for it. I proposed this approach during the previous > discussions related to possible pattern interface implementations, > but you seemed not to be so enthusiastic in [0]. > > [0] https://lkml.org/lkml/2017/4/7/350 Hmm. Reading my own email now, I can't decipher it. I believe I meant "changing patterns from kernel in response to events is probably overkill"... or something like that. Sorry about confusion, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-14 22:29 ` Pavel Machek @ 2018-07-14 22:39 ` Pavel Machek 2018-07-15 12:22 ` Jacek Anaszewski 0 siblings, 1 reply; 38+ messages in thread From: Pavel Machek @ 2018-07-14 22:39 UTC (permalink / raw) To: Jacek Anaszewski Cc: Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 1855 bytes --] On Sun 2018-07-15 00:29:25, Pavel Machek wrote: > On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: > > Hi Pavel, > > > > On 07/14/2018 11:20 PM, Pavel Machek wrote: > > >Hi! > > > > > >>>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. > > > > > >Let me take a step back: we have triggers.. like LED blinking. > > > > > >How is that going to interact with patterns? We probably want the > > >patterns to be ignored in that case...? > > > > > >Which suggest to me that we should treat patterns as a trigger. I > > >believe we do something similar with blinking already. > > > > > >Then it is easy to determine if pattern is active, and pattern > > >vs. trigger issue is solved automatically. > > > > I'm all for it. I proposed this approach during the previous > > discussions related to possible pattern interface implementations, > > but you seemed not to be so enthusiastic in [0]. > > > > [0] https://lkml.org/lkml/2017/4/7/350 > > Hmm. Reading my own email now, I can't decipher it. > > I believe I meant "changing patterns from kernel in response to events > is probably overkill"... or something like that. Anyway -- to clean up the confusion -- I'd like to see echo pattern > trigger echo "1 2 3 4 5 6 7 8" > somewhere to activate pattern, and echo none > trigger to stop it. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-14 22:39 ` Pavel Machek @ 2018-07-15 12:22 ` Jacek Anaszewski 2018-07-16 1:00 ` David Lechner 2018-07-16 11:08 ` Baolin Wang 0 siblings, 2 replies; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-15 12:22 UTC (permalink / raw) To: Pavel Machek Cc: Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML On 07/15/2018 12:39 AM, Pavel Machek wrote: > On Sun 2018-07-15 00:29:25, Pavel Machek wrote: >> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: >>> Hi Pavel, >>> >>> On 07/14/2018 11:20 PM, Pavel Machek wrote: >>>> Hi! >>>> >>>>>> 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. >>>> >>>> Let me take a step back: we have triggers.. like LED blinking. >>>> >>>> How is that going to interact with patterns? We probably want the >>>> patterns to be ignored in that case...? >>>> >>>> Which suggest to me that we should treat patterns as a trigger. I >>>> believe we do something similar with blinking already. >>>> >>>> Then it is easy to determine if pattern is active, and pattern >>>> vs. trigger issue is solved automatically. >>> >>> I'm all for it. I proposed this approach during the previous >>> discussions related to possible pattern interface implementations, >>> but you seemed not to be so enthusiastic in [0]. >>> >>> [0] https://lkml.org/lkml/2017/4/7/350 >> >> Hmm. Reading my own email now, I can't decipher it. >> >> I believe I meant "changing patterns from kernel in response to events >> is probably overkill"... or something like that. > > Anyway -- to clean up the confusion -- I'd like to see > > echo pattern > trigger > echo "1 2 3 4 5 6 7 8" > somewhere s/somewhere/pattern/ pattern trigger should create "pattern" file similarly how ledtrig-timer creates delay_{on|off} files. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-15 12:22 ` Jacek Anaszewski @ 2018-07-16 1:00 ` David Lechner 2018-07-16 20:29 ` Jacek Anaszewski 2018-07-24 0:18 ` Bjorn Andersson 2018-07-16 11:08 ` Baolin Wang 1 sibling, 2 replies; 38+ messages in thread From: David Lechner @ 2018-07-16 1:00 UTC (permalink / raw) To: Baolin Wang Cc: Jacek Anaszewski, Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML On 07/15/2018 07:22 AM, Jacek Anaszewski wrote: > On 07/15/2018 12:39 AM, Pavel Machek wrote: >> On Sun 2018-07-15 00:29:25, Pavel Machek wrote: >>> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: >>>> Hi Pavel, >>>> >>>> On 07/14/2018 11:20 PM, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>> 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. >>>>> >>>>> Let me take a step back: we have triggers.. like LED blinking. >>>>> >>>>> How is that going to interact with patterns? We probably want the >>>>> patterns to be ignored in that case...? >>>>> >>>>> Which suggest to me that we should treat patterns as a trigger. I >>>>> believe we do something similar with blinking already. >>>>> >>>>> Then it is easy to determine if pattern is active, and pattern >>>>> vs. trigger issue is solved automatically. >>>> >>>> I'm all for it. I proposed this approach during the previous >>>> discussions related to possible pattern interface implementations, >>>> but you seemed not to be so enthusiastic in [0]. >>>> >>>> [0] https://lkml.org/lkml/2017/4/7/350 >>> >>> Hmm. Reading my own email now, I can't decipher it. >>> >>> I believe I meant "changing patterns from kernel in response to events >>> is probably overkill"... or something like that. >> >> Anyway -- to clean up the confusion -- I'd like to see >> >> echo pattern > trigger >> echo "1 2 3 4 5 6 7 8" > somewhere > > s/somewhere/pattern/ > > pattern trigger should create "pattern" file similarly how ledtrig-timer > creates delay_{on|off} files. > I don't think this is the best way. For example, if you want more than one LED to have the same pattern, then the patterns will not be synchronized between the LEDs. The same things happens now with many of the existing triggers. For example, if I have two LEDs side-by-side using the heartbeat trigger, they may blink at the same time or they may not, which is not very nice. I think we can make something better. Perhaps a way to do this would be to use configfs to create a pattern trigger that can be shared by multiple LEDs. Like this: mkdir /sys/kernel/config/leds/triggers/my-nice-pattern echo "1 2 3 4" > /sys/kernel/config/leds/triggers/my-nice-pattern/pattern echo my-nice-pattern > /sys/class/leds/led0/trigger echo my-nice-pattern > /sys/class/leds/led1/trigger Please CC me on any future revisions of this series. I would like to test it. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-16 1:00 ` David Lechner @ 2018-07-16 20:29 ` Jacek Anaszewski 2018-07-16 21:56 ` Pavel Machek 2018-07-18 7:56 ` Pavel Machek 2018-07-24 0:18 ` Bjorn Andersson 1 sibling, 2 replies; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-16 20:29 UTC (permalink / raw) To: David Lechner, Baolin Wang Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML Hi David, On 07/16/2018 03:00 AM, David Lechner wrote: > On 07/15/2018 07:22 AM, Jacek Anaszewski wrote: >> On 07/15/2018 12:39 AM, Pavel Machek wrote: >>> On Sun 2018-07-15 00:29:25, Pavel Machek wrote: >>>> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: >>>>> Hi Pavel, >>>>> >>>>> On 07/14/2018 11:20 PM, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>>> 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. >>>>>> >>>>>> Let me take a step back: we have triggers.. like LED blinking. >>>>>> >>>>>> How is that going to interact with patterns? We probably want the >>>>>> patterns to be ignored in that case...? >>>>>> >>>>>> Which suggest to me that we should treat patterns as a trigger. I >>>>>> believe we do something similar with blinking already. >>>>>> >>>>>> Then it is easy to determine if pattern is active, and pattern >>>>>> vs. trigger issue is solved automatically. >>>>> >>>>> I'm all for it. I proposed this approach during the previous >>>>> discussions related to possible pattern interface implementations, >>>>> but you seemed not to be so enthusiastic in [0]. >>>>> >>>>> [0] https://lkml.org/lkml/2017/4/7/350 >>>> >>>> Hmm. Reading my own email now, I can't decipher it. >>>> >>>> I believe I meant "changing patterns from kernel in response to events >>>> is probably overkill"... or something like that. >>> >>> Anyway -- to clean up the confusion -- I'd like to see >>> >>> echo pattern > trigger >>> echo "1 2 3 4 5 6 7 8" > somewhere >> >> s/somewhere/pattern/ >> >> pattern trigger should create "pattern" file similarly how ledtrig-timer >> creates delay_{on|off} files. >> > > I don't think this is the best way. For example, if you want more than one > LED to have the same pattern, then the patterns will not be synchronized > between the LEDs. The same things happens now with many of the existing > triggers. For example, if I have two LEDs side-by-side using the heartbeat > trigger, they may blink at the same time or they may not, which is not > very nice. I think we can make something better. It is virtually impossible to enforce synchronous blinking for the LEDs driven by different hardware due to: - different hardware means via which brightness is set (MMIO, I2C, SPI, PWM and other pulsed fashion based protocols), - the need for deferring brightness setting to a workqueue task to allow for setting LED brightness from atomic context, - contention on locks For the LEDs driven by the same chip it would make more sense to allow for synchronization, but it can be achieved on driver level, with help of some subsystem level interface to indicate which LEDs should be synchronized. However, when we start to pretend that we can synchronize the devices, we must answer how accurate we can be. The accuracy will decrease as blink frequency rises. We'd need to define reliability limit. For different devices, this limit will be different, and it will also depend on the CPU speed. We've had few attempts of approaching the subject of synchronized blinking but none of them proved to be good enough to be merged. Frankly speaking I doubt it is good task for the system like Linux. > Perhaps a way to do this would be to use configfs to create a pattern > trigger that can be shared by multiple LEDs. Like this: > > mkdir /sys/kernel/config/leds/triggers/my-nice-pattern > echo "1 2 3 4" > > /sys/kernel/config/leds/triggers/my-nice-pattern/pattern > echo my-nice-pattern > /sys/class/leds/led0/trigger > echo my-nice-pattern > /sys/class/leds/led1/trigger > > > Please CC me on any future revisions of this series. I would like to > test it. > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-16 20:29 ` Jacek Anaszewski @ 2018-07-16 21:56 ` Pavel Machek 2018-07-17 20:26 ` Jacek Anaszewski 2018-07-24 0:35 ` Bjorn Andersson 2018-07-18 7:56 ` Pavel Machek 1 sibling, 2 replies; 38+ messages in thread From: Pavel Machek @ 2018-07-16 21:56 UTC (permalink / raw) To: Jacek Anaszewski Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 2585 bytes --] Hi! > >>>echo pattern > trigger > >>>echo "1 2 3 4 5 6 7 8" > somewhere > >> > >>s/somewhere/pattern/ > >> > >>pattern trigger should create "pattern" file similarly how ledtrig-timer > >>creates delay_{on|off} files. > >> > > > >I don't think this is the best way. For example, if you want more than one > >LED to have the same pattern, then the patterns will not be synchronized > >between the LEDs. The same things happens now with many of the existing > >triggers. For example, if I have two LEDs side-by-side using the heartbeat > >trigger, they may blink at the same time or they may not, which is not > >very nice. I think we can make something better. Yes, synchronization would be nice -- it is really a must for RGB LEDs -- but I believe it is too much to ask from Baolin at the moment. > It is virtually impossible to enforce synchronous blinking for the > LEDs driven by different hardware due to: > > - different hardware means via which brightness is set (MMIO, I2C, SPI, > PWM and other pulsed fashion based protocols), > - the need for deferring brightness setting to a workqueue task to > allow for setting LED brightness from atomic context, > - contention on locks I disagree here. Yes, it would be hard to synchronize blinking down to microseconds, but it would be easy to get synchronization right down to miliseconds and humans will not be able to tell the difference. > For the LEDs driven by the same chip it would make more sense > to allow for synchronization, but it can be achieved on driver > level, with help of some subsystem level interface to indicate > which LEDs should be synchronized. > > However, when we start to pretend that we can synchronize the > devices, we must answer how accurate we can be. The accuracy > will decrease as blink frequency rises. We'd need to define > reliability limit. We don't need _that_ ammount of overengineering. We just need to synchronize them well enough :-). > We've had few attempts of approaching the subject of synchronized > blinking but none of them proved to be good enough to be merged. I'm sure interested person could do something like that in less than two weeks fulltime... It is not rocket science, just a lot of work in kernel... But patterns are few years overdue and I believe we should not delay them any further. So... I guess I agree with Jacek in the end :-). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-16 21:56 ` Pavel Machek @ 2018-07-17 20:26 ` Jacek Anaszewski 2018-07-17 21:07 ` Pavel Machek 2018-07-24 0:35 ` Bjorn Andersson 1 sibling, 1 reply; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-17 20:26 UTC (permalink / raw) To: Pavel Machek Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML On 07/16/2018 11:56 PM, Pavel Machek wrote: > Hi! > >>>>> echo pattern > trigger >>>>> echo "1 2 3 4 5 6 7 8" > somewhere >>>> >>>> s/somewhere/pattern/ >>>> >>>> pattern trigger should create "pattern" file similarly how ledtrig-timer >>>> creates delay_{on|off} files. >>>> >>> >>> I don't think this is the best way. For example, if you want more than one >>> LED to have the same pattern, then the patterns will not be synchronized >>> between the LEDs. The same things happens now with many of the existing >>> triggers. For example, if I have two LEDs side-by-side using the heartbeat >>> trigger, they may blink at the same time or they may not, which is not >>> very nice. I think we can make something better. > > Yes, synchronization would be nice -- it is really a must for RGB LEDs > -- but I believe it is too much to ask from Baolin at the moment. > >> It is virtually impossible to enforce synchronous blinking for the >> LEDs driven by different hardware due to: >> >> - different hardware means via which brightness is set (MMIO, I2C, SPI, >> PWM and other pulsed fashion based protocols), >> - the need for deferring brightness setting to a workqueue task to >> allow for setting LED brightness from atomic context, >> - contention on locks > > I disagree here. Yes, it would be hard to synchronize blinking down to > microseconds, but it would be easy to get synchronization right down > to miliseconds and humans will not be able to tell the difference. There have been problems with blink interval stability close to 1ms, and thus there were some attempts of employing hr timers, which in turn introduced a new class of issues related to system performance etc. >> For the LEDs driven by the same chip it would make more sense >> to allow for synchronization, but it can be achieved on driver >> level, with help of some subsystem level interface to indicate >> which LEDs should be synchronized. >> >> However, when we start to pretend that we can synchronize the >> devices, we must answer how accurate we can be. The accuracy >> will decrease as blink frequency rises. We'd need to define >> reliability limit. > > We don't need _that_ ammount of overengineering. We just need to > synchronize them well enough :-). Well, it would be disappointing for the users to realize that they don't get the effect advertised by the ABI documentation. >> We've had few attempts of approaching the subject of synchronized >> blinking but none of them proved to be good enough to be merged. > > I'm sure interested person could do something like that in less than > two weeks fulltime... It is not rocket science, just a lot of work in > kernel... > > But patterns are few years overdue and I believe we should not delay > them any further. > > So... I guess I agree with Jacek in the end :-). How about taking Baolin's patches as of v5? Later, provided that the pattern trigger yet to be implemented will create pattern file on activation, we'll need to initialize default-trigger DT property, to keep the interface unchanged. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-17 20:26 ` Jacek Anaszewski @ 2018-07-17 21:07 ` Pavel Machek 0 siblings, 0 replies; 38+ messages in thread From: Pavel Machek @ 2018-07-17 21:07 UTC (permalink / raw) To: Jacek Anaszewski Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 2680 bytes --] Hi! > >>- different hardware means via which brightness is set (MMIO, I2C, SPI, > >> PWM and other pulsed fashion based protocols), > >>- the need for deferring brightness setting to a workqueue task to > >> allow for setting LED brightness from atomic context, > >>- contention on locks > > > >I disagree here. Yes, it would be hard to synchronize blinking down to > >microseconds, but it would be easy to get synchronization right down > >to miliseconds and humans will not be able to tell the difference. > > There have been problems with blink interval stability close to > 1ms, and thus there were some attempts of employing hr timers, > which in turn introduced a new class of issues related to > system performance etc. Yeah, well. This is LED subsystem. Noone should program blink intervals at 1 msec. > >>For the LEDs driven by the same chip it would make more sense > >>to allow for synchronization, but it can be achieved on driver > >>level, with help of some subsystem level interface to indicate > >>which LEDs should be synchronized. > >> > >>However, when we start to pretend that we can synchronize the > >>devices, we must answer how accurate we can be. The accuracy > >>will decrease as blink frequency rises. We'd need to define > >>reliability limit. > > > >We don't need _that_ ammount of overengineering. We just need to > >synchronize them well enough :-). > > Well, it would be disappointing for the users to realize that > they don't get the effect advertised by the ABI documentation. Linux is always best-effort, w.r.t. timing. And we can do well enough that user will not see anything bad on "normal" systems. > >>We've had few attempts of approaching the subject of synchronized > >>blinking but none of them proved to be good enough to be merged. > > > >I'm sure interested person could do something like that in less than > >two weeks fulltime... It is not rocket science, just a lot of work in > >kernel... > > > >But patterns are few years overdue and I believe we should not delay > >them any further. > > > >So... I guess I agree with Jacek in the end :-). > > How about taking Baolin's patches as of v5? Later, provided that > the pattern trigger yet to be implemented will create pattern file > on activation, we'll need to initialize default-trigger DT property, > to keep the interface unchanged. I have yet to look at the v5 of patches. But I agree that we do not need to design synchronization at this moment. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-16 21:56 ` Pavel Machek 2018-07-17 20:26 ` Jacek Anaszewski @ 2018-07-24 0:35 ` Bjorn Andersson 1 sibling, 0 replies; 38+ messages in thread From: Bjorn Andersson @ 2018-07-24 0:35 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, David Lechner, Baolin Wang, Mark Brown, Linux LED Subsystem, LKML On Mon 16 Jul 14:56 PDT 2018, Pavel Machek wrote: > Hi! > > > >>>echo pattern > trigger > > >>>echo "1 2 3 4 5 6 7 8" > somewhere > > >> > > >>s/somewhere/pattern/ > > >> > > >>pattern trigger should create "pattern" file similarly how ledtrig-timer > > >>creates delay_{on|off} files. > > >> > > > > > >I don't think this is the best way. For example, if you want more than one > > >LED to have the same pattern, then the patterns will not be synchronized > > >between the LEDs. The same things happens now with many of the existing > > >triggers. For example, if I have two LEDs side-by-side using the heartbeat > > >trigger, they may blink at the same time or they may not, which is not > > >very nice. I think we can make something better. > > Yes, synchronization would be nice -- it is really a must for RGB LEDs > -- but I believe it is too much to ask from Baolin at the moment. > In my work on the Qualcomm LPG (which I should finish up and resend) I described the RGB as a single LED, with the color configured by Pavel's suggested HSV interface. This single LED would be given one pattern. This works fine for the typical use cases we've seen so far, but would not be enough to describe transitions between colors. I believe that a reasonable solution to this would be to extend the pattern to allow each value in the list to contain data for more than one channel - something that should be reasonable to add on top of this suggestion. > > It is virtually impossible to enforce synchronous blinking for the > > LEDs driven by different hardware due to: > > > > - different hardware means via which brightness is set (MMIO, I2C, SPI, > > PWM and other pulsed fashion based protocols), > > - the need for deferring brightness setting to a workqueue task to > > allow for setting LED brightness from atomic context, > > - contention on locks > > I disagree here. Yes, it would be hard to synchronize blinking down to > microseconds, but it would be easy to get synchronization right down > to miliseconds and humans will not be able to tell the difference. > I like the HSV approach for exposing multi color LEDs and we should be able to provide a "virtual" LED that groups some number of LEDs into one, to represent this use case when the hardware doesn't do directly. In such cases it would work quite weel to use the proposed pattern interface for setting the pattern of this virtual LED and having it cascade the pattern into the individual LEDs and then start them at about the same time... > > For the LEDs driven by the same chip it would make more sense > > to allow for synchronization, but it can be achieved on driver > > level, with help of some subsystem level interface to indicate > > which LEDs should be synchronized. > > > > However, when we start to pretend that we can synchronize the > > devices, we must answer how accurate we can be. The accuracy > > will decrease as blink frequency rises. We'd need to define > > reliability limit. > > We don't need _that_ ammount of overengineering. We just need to > synchronize them well enough :-). > > > We've had few attempts of approaching the subject of synchronized > > blinking but none of them proved to be good enough to be merged. > > I'm sure interested person could do something like that in less than > two weeks fulltime... It is not rocket science, just a lot of work in > kernel... > With the Qualcomm LPG driver I expect the hardware description to group the channels of a RGB and as such the driver will synchronize the pattern execution when the output is enabled. > But patterns are few years overdue and I believe we should not delay > them any further. > Sorry for not prioritizing reworking this patch based on your suggestion of describing it as a trigger, I still think this sounds reasonable. Regards, Bjorn ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-16 20:29 ` Jacek Anaszewski 2018-07-16 21:56 ` Pavel Machek @ 2018-07-18 7:56 ` Pavel Machek 2018-07-18 11:32 ` Baolin Wang 2018-07-18 18:54 ` Jacek Anaszewski 1 sibling, 2 replies; 38+ messages in thread From: Pavel Machek @ 2018-07-18 7:56 UTC (permalink / raw) To: Jacek Anaszewski Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 886 bytes --] Hi! > >>>>I believe I meant "changing patterns from kernel in response to events > >>>>is probably overkill"... or something like that. > >>> > >>>Anyway -- to clean up the confusion -- I'd like to see > >>> > >>>echo pattern > trigger > >>>echo "1 2 3 4 5 6 7 8" > somewhere > >> > >>s/somewhere/pattern/ > >> > >>pattern trigger should create "pattern" file similarly how ledtrig-timer > >>creates delay_{on|off} files. Yes, that sounds reasonable. v5 still says + Writing non-empty string to this file will activate the pattern, + and empty string will disable the pattern. I'd deactivate the pattern by simply writing something else to the trigger file. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-18 7:56 ` Pavel Machek @ 2018-07-18 11:32 ` Baolin Wang 2018-07-18 12:08 ` Pavel Machek 2018-07-18 18:54 ` Jacek Anaszewski 1 sibling, 1 reply; 38+ messages in thread From: Baolin Wang @ 2018-07-18 11:32 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML On 18 July 2018 at 15:56, Pavel Machek <pavel@ucw.cz> wrote: > Hi! > >> >>>>I believe I meant "changing patterns from kernel in response to events >> >>>>is probably overkill"... or something like that. >> >>> >> >>>Anyway -- to clean up the confusion -- I'd like to see >> >>> >> >>>echo pattern > trigger >> >>>echo "1 2 3 4 5 6 7 8" > somewhere >> >> >> >>s/somewhere/pattern/ >> >> >> >>pattern trigger should create "pattern" file similarly how ledtrig-timer >> >>creates delay_{on|off} files. > > Yes, that sounds reasonable. v5 still says > > + Writing non-empty string to this file will activate the pattern, > + and empty string will disable the pattern. > > I'd deactivate the pattern by simply writing something else to the > trigger file. For the case we met in patch 2, it is not related with trigger things. We just set some series of tuples including brightness and duration (ms) to the hardware to enable the breath mode of the LED, we did not trigger anything. So it is weird to write something to trigger file to deactive the pattern. -- Baolin Wang Best Regards ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-18 11:32 ` Baolin Wang @ 2018-07-18 12:08 ` Pavel Machek 2018-07-18 17:00 ` David Lechner 0 siblings, 1 reply; 38+ messages in thread From: Pavel Machek @ 2018-07-18 12:08 UTC (permalink / raw) To: Baolin Wang Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 1727 bytes --] On Wed 2018-07-18 19:32:01, Baolin Wang wrote: > On 18 July 2018 at 15:56, Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > >> >>>>I believe I meant "changing patterns from kernel in response to events > >> >>>>is probably overkill"... or something like that. > >> >>> > >> >>>Anyway -- to clean up the confusion -- I'd like to see > >> >>> > >> >>>echo pattern > trigger > >> >>>echo "1 2 3 4 5 6 7 8" > somewhere > >> >> > >> >>s/somewhere/pattern/ > >> >> > >> >>pattern trigger should create "pattern" file similarly how ledtrig-timer > >> >>creates delay_{on|off} files. > > > > Yes, that sounds reasonable. v5 still says > > > > + Writing non-empty string to this file will activate the pattern, > > + and empty string will disable the pattern. > > > > I'd deactivate the pattern by simply writing something else to the > > trigger file. > > For the case we met in patch 2, it is not related with trigger things. > We just set some series of tuples including brightness and duration > (ms) to the hardware to enable the breath mode of the LED, we did not > trigger anything. So it is weird to write something to trigger file to > deactive the pattern. Confused. I thought that "breathing mode" would be handled similar way to hardware blinking: userland selects pattern trigger, pattern file appears (similar way to delay_on/delay_off files with blinking), he configures it, hardware brightness follows the pattern ("breathing mode"). If pattern is no longer required, echo none > trigger stops it. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-18 12:08 ` Pavel Machek @ 2018-07-18 17:00 ` David Lechner 2018-07-20 19:11 ` Jacek Anaszewski 0 siblings, 1 reply; 38+ messages in thread From: David Lechner @ 2018-07-18 17:00 UTC (permalink / raw) To: Pavel Machek, Baolin Wang Cc: Jacek Anaszewski, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML On 7/18/18 7:08 AM, Pavel Machek wrote: > On Wed 2018-07-18 19:32:01, Baolin Wang wrote: >> On 18 July 2018 at 15:56, Pavel Machek <pavel@ucw.cz> wrote: >>> Hi! >>> >>>>>>>> I believe I meant "changing patterns from kernel in response to events >>>>>>>> is probably overkill"... or something like that. >>>>>>> >>>>>>> Anyway -- to clean up the confusion -- I'd like to see >>>>>>> >>>>>>> echo pattern > trigger >>>>>>> echo "1 2 3 4 5 6 7 8" > somewhere >>>>>> >>>>>> s/somewhere/pattern/ >>>>>> >>>>>> pattern trigger should create "pattern" file similarly how ledtrig-timer >>>>>> creates delay_{on|off} files. >>> >>> Yes, that sounds reasonable. v5 still says >>> >>> + Writing non-empty string to this file will activate the pattern, >>> + and empty string will disable the pattern. >>> >>> I'd deactivate the pattern by simply writing something else to the >>> trigger file. >> >> For the case we met in patch 2, it is not related with trigger things. >> We just set some series of tuples including brightness and duration >> (ms) to the hardware to enable the breath mode of the LED, we did not >> trigger anything. So it is weird to write something to trigger file to >> deactive the pattern. > > Confused. I thought that "breathing mode" would be handled similar way > to hardware blinking: userland selects pattern trigger, pattern file > appears (similar way to delay_on/delay_off files with blinking), he > configures it, hardware brightness follows the pattern ("breathing > mode"). If pattern is no longer required, echo none > trigger stops > it. > Pavel > I was confused too when I first read this thread. But after reviewing v5, it is clear that this is _not_ a trigger (and it should not be a trigger). This is basically the equivalent the brightness attribute - except that now the brightness changes over time instead of being a constant value. This way, the pattern can be used in conjunction with triggers. For example, one might want to set the _pattern_ to something like a "breathe" pattern and set the _trigger_ to the power supply charging full trigger. This way, the LED will be off until the battery is full and then the LED will "breath" when the battery is full. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-18 17:00 ` David Lechner @ 2018-07-20 19:11 ` Jacek Anaszewski 2018-07-24 0:55 ` Bjorn Andersson 0 siblings, 1 reply; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-20 19:11 UTC (permalink / raw) To: David Lechner, Pavel Machek, Baolin Wang Cc: Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML Hi David, On 07/18/2018 07:00 PM, David Lechner wrote: > > > On 7/18/18 7:08 AM, Pavel Machek wrote: >> On Wed 2018-07-18 19:32:01, Baolin Wang wrote: >>> On 18 July 2018 at 15:56, Pavel Machek <pavel@ucw.cz> wrote: >>>> Hi! >>>> >>>>>>>>> I believe I meant "changing patterns from kernel in response to >>>>>>>>> events >>>>>>>>> is probably overkill"... or something like that. >>>>>>>> >>>>>>>> Anyway -- to clean up the confusion -- I'd like to see >>>>>>>> >>>>>>>> echo pattern > trigger >>>>>>>> echo "1 2 3 4 5 6 7 8" > somewhere >>>>>>> >>>>>>> s/somewhere/pattern/ >>>>>>> >>>>>>> pattern trigger should create "pattern" file similarly how >>>>>>> ledtrig-timer >>>>>>> creates delay_{on|off} files. >>>> >>>> Yes, that sounds reasonable. v5 still says >>>> >>>> + Writing non-empty string to this file will activate >>>> the pattern, >>>> + and empty string will disable the pattern. >>>> >>>> I'd deactivate the pattern by simply writing something else to the >>>> trigger file. >>> >>> For the case we met in patch 2, it is not related with trigger things. >>> We just set some series of tuples including brightness and duration >>> (ms) to the hardware to enable the breath mode of the LED, we did not >>> trigger anything. So it is weird to write something to trigger file to >>> deactive the pattern. >> >> Confused. I thought that "breathing mode" would be handled similar way >> to hardware blinking: userland selects pattern trigger, pattern file >> appears (similar way to delay_on/delay_off files with blinking), he >> configures it, hardware brightness follows the pattern ("breathing >> mode"). If pattern is no longer required, echo none > trigger stops >> it. >> Pavel >> > > I was confused too when I first read this thread. But after reviewing > v5, it is clear that this is _not_ a trigger (and it should not be a > trigger). This is basically the equivalent the brightness attribute - > except that now the brightness changes over time instead of being a > constant value. Pattern trigger would be just more flexible version of existing ledtrig-timer.c, which also changes brightness over time. Trigger, by definition, is a kernel based source of LED events, so timer trigger falls under this definition, same way as pattern trigger would. What may cause confusion is the possibility of exploiting hardware capabilities, in case the requested settings fit in. ledtrig-timer fathom the hardware capabilities using blink_set op, and pattern trigger would use pattern_set op for that purpose. > This way, the pattern can be used in conjunction with > triggers. > > For example, one might want to set the _pattern_ to something like a > "breathe" pattern and set the _trigger_ to the power supply charging > full trigger. This way, the LED will be off until the battery is full > and then the LED will "breath" when the battery is full. AFAICS you comprehend "pattern trigger" notion as a LED trigger that activates pattern functionality. I'd say that you'd need a specialized "battery" trigger for that purpose, that instead of calling led_set_brightness_nosleep() would schedule call to led_trigger_set(led_cdev "pattern"), assuming that pattern trigger would be implemented as I explained above. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-20 19:11 ` Jacek Anaszewski @ 2018-07-24 0:55 ` Bjorn Andersson 0 siblings, 0 replies; 38+ messages in thread From: Bjorn Andersson @ 2018-07-24 0:55 UTC (permalink / raw) To: Jacek Anaszewski Cc: David Lechner, Pavel Machek, Baolin Wang, Mark Brown, Linux LED Subsystem, LKML On Fri 20 Jul 12:11 PDT 2018, Jacek Anaszewski wrote: > Hi David, > > On 07/18/2018 07:00 PM, David Lechner wrote: > > > > > > On 7/18/18 7:08 AM, Pavel Machek wrote: > > > On Wed 2018-07-18 19:32:01, Baolin Wang wrote: > > > > On 18 July 2018 at 15:56, Pavel Machek <pavel@ucw.cz> wrote: > > > > > Hi! > > > > > > > > > > > > > > > I believe I meant "changing patterns > > > > > > > > > > from kernel in response to events > > > > > > > > > > is probably overkill"... or something like that. > > > > > > > > > > > > > > > > > > Anyway -- to clean up the confusion -- I'd like to see > > > > > > > > > > > > > > > > > > echo pattern > trigger > > > > > > > > > echo "1 2 3 4 5 6 7 8" > somewhere > > > > > > > > > > > > > > > > s/somewhere/pattern/ > > > > > > > > > > > > > > > > pattern trigger should create "pattern" file > > > > > > > > similarly how ledtrig-timer > > > > > > > > creates delay_{on|off} files. > > > > > > > > > > Yes, that sounds reasonable. v5 still says > > > > > > > > > > + Writing non-empty string to this file will > > > > > activate the pattern, > > > > > + and empty string will disable the pattern. > > > > > > > > > > I'd deactivate the pattern by simply writing something else to the > > > > > trigger file. > > > > > > > > For the case we met in patch 2, it is not related with trigger things. > > > > We just set some series of tuples including brightness and duration > > > > (ms) to the hardware to enable the breath mode of the LED, we did not > > > > trigger anything. So it is weird to write something to trigger file to > > > > deactive the pattern. > > > > > > Confused. I thought that "breathing mode" would be handled similar way > > > to hardware blinking: userland selects pattern trigger, pattern file > > > appears (similar way to delay_on/delay_off files with blinking), he > > > configures it, hardware brightness follows the pattern ("breathing > > > mode"). If pattern is no longer required, echo none > trigger stops > > > it. > > > Pavel > > > > > > > I was confused too when I first read this thread. But after reviewing > > v5, it is clear that this is _not_ a trigger (and it should not be a > > trigger). This is basically the equivalent the brightness attribute - > > except that now the brightness changes over time instead of being a > > constant value. > > Pattern trigger would be just more flexible version of existing > ledtrig-timer.c, which also changes brightness over time. > > Trigger, by definition, is a kernel based source of LED events, > so timer trigger falls under this definition, same way as pattern > trigger would. > > What may cause confusion is the possibility of exploiting hardware > capabilities, in case the requested settings fit in. > ledtrig-timer fathom the hardware capabilities using blink_set op, > and pattern trigger would use pattern_set op for that purpose. > For the use cases I had in mind it's perfectly fine to describe this as a trigger. But that said, I rather quickly started playing around with associating patterns to trigger events; e.g. having a continuous pulse associated with the bluetooth "power" trigger or defining a smooth rampdown for the mmc activity trigger. > > This way, the pattern can be used in conjunction with triggers. > > > > For example, one might want to set the _pattern_ to something like a > > "breathe" pattern and set the _trigger_ to the power supply charging > > full trigger. This way, the LED will be off until the battery is full > > and then the LED will "breath" when the battery is full. > > AFAICS you comprehend "pattern trigger" notion as a LED trigger that > activates pattern functionality. I'd say that you'd need a specialized > "battery" trigger for that purpose, that instead of calling > led_set_brightness_nosleep() would schedule call to > led_trigger_set(led_cdev "pattern"), assuming that pattern trigger > would be implemented as I explained above. > Presumably the battery logic has a number of states; low batter discharging, low battery charging, full battery, levels in-between. Defining the levels for these and an appropriate UX seems like a job for something with a little bit of configurability and product specific logic. The transitions here would need to reconfigure the pattern, so it doesn't seem unreasonable for it to also set the pattern trigger. I'm not sure this logic does belong in the kernel, but I think that question is of less importance for the design of this. Regards, Bjorn ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-18 7:56 ` Pavel Machek 2018-07-18 11:32 ` Baolin Wang @ 2018-07-18 18:54 ` Jacek Anaszewski 2018-07-18 19:22 ` Jacek Anaszewski 1 sibling, 1 reply; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-18 18:54 UTC (permalink / raw) To: Pavel Machek Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML On 07/18/2018 09:56 AM, Pavel Machek wrote: > Hi! > >>>>>> I believe I meant "changing patterns from kernel in response to events >>>>>> is probably overkill"... or something like that. >>>>> >>>>> Anyway -- to clean up the confusion -- I'd like to see >>>>> >>>>> echo pattern > trigger >>>>> echo "1 2 3 4 5 6 7 8" > somewhere >>>> >>>> s/somewhere/pattern/ >>>> >>>> pattern trigger should create "pattern" file similarly how ledtrig-timer >>>> creates delay_{on|off} files. > > Yes, that sounds reasonable. v5 still says > > + Writing non-empty string to this file will activate the pattern, > + and empty string will disable the pattern. > > I'd deactivate the pattern by simply writing something else to the > trigger file. Please keep in mind that this is ABI documentation for the pattern file to be exposed by LED core, and not by the pattern trigger, that, as we agreed, will be implemented later. In this case, I'd go for "echo 0 > brightness" as a command disabling pattern. The same operation disables triggers, so later transition to using pattern trigger will be seamless for userspace. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-18 18:54 ` Jacek Anaszewski @ 2018-07-18 19:22 ` Jacek Anaszewski 2018-07-18 22:13 ` David Lechner 2018-07-18 22:17 ` Pavel Machek 0 siblings, 2 replies; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-18 19:22 UTC (permalink / raw) To: Pavel Machek Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML On 07/18/2018 08:54 PM, Jacek Anaszewski wrote: > On 07/18/2018 09:56 AM, Pavel Machek wrote: >> Hi! >> >>>>>>> I believe I meant "changing patterns from kernel in response to >>>>>>> events >>>>>>> is probably overkill"... or something like that. >>>>>> >>>>>> Anyway -- to clean up the confusion -- I'd like to see >>>>>> >>>>>> echo pattern > trigger >>>>>> echo "1 2 3 4 5 6 7 8" > somewhere >>>>> >>>>> s/somewhere/pattern/ >>>>> >>>>> pattern trigger should create "pattern" file similarly how >>>>> ledtrig-timer >>>>> creates delay_{on|off} files. >> >> Yes, that sounds reasonable. v5 still says >> >> + Writing non-empty string to this file will activate >> the pattern, >> + and empty string will disable the pattern. >> >> I'd deactivate the pattern by simply writing something else to the >> trigger file. > > Please keep in mind that this is ABI documentation for the pattern file > to be exposed by LED core, and not by the pattern trigger, that, as we > agreed, will be implemented later. In this case, I'd go for Gosh, I got completely distracted by the recent discussion about pattern synchronization. So, to recap, we need to decide if we are taking Baolin's solution or we're opting for implementing pattern trigger. If we choose the latter, then we will also need some software pattern engine in the trigger, to be applied as a software pattern fallback for the devices without hardware pattern support. It will certainly delay the contribution process, provided that Baolin would find time for this work at all. I'd just take v5 based solution for now (with improved semantics of disabling pattern - in this case my reasoning from the message I'm replying to is still valid), > "echo 0 > brightness" as a command disabling pattern. The same operation > disables triggers, so later transition to using pattern trigger will be > seamless for userspace. > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-18 19:22 ` Jacek Anaszewski @ 2018-07-18 22:13 ` David Lechner 2018-07-18 22:17 ` Pavel Machek 1 sibling, 0 replies; 38+ messages in thread From: David Lechner @ 2018-07-18 22:13 UTC (permalink / raw) To: Jacek Anaszewski, Pavel Machek Cc: Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML On 07/18/2018 02:22 PM, Jacek Anaszewski wrote: > On 07/18/2018 08:54 PM, Jacek Anaszewski wrote: >> On 07/18/2018 09:56 AM, Pavel Machek wrote: >>> Hi! >>> >>>>>>>> I believe I meant "changing patterns from kernel in response to events >>>>>>>> is probably overkill"... or something like that. >>>>>>> >>>>>>> Anyway -- to clean up the confusion -- I'd like to see >>>>>>> >>>>>>> echo pattern > trigger >>>>>>> echo "1 2 3 4 5 6 7 8" > somewhere >>>>>> >>>>>> s/somewhere/pattern/ >>>>>> >>>>>> pattern trigger should create "pattern" file similarly how ledtrig-timer >>>>>> creates delay_{on|off} files. >>> >>> Yes, that sounds reasonable. v5 still says >>> >>> + Writing non-empty string to this file will activate the pattern, >>> + and empty string will disable the pattern. >>> >>> I'd deactivate the pattern by simply writing something else to the >>> trigger file. >> >> Please keep in mind that this is ABI documentation for the pattern file >> to be exposed by LED core, and not by the pattern trigger, that, as we >> agreed, will be implemented later. In this case, I'd go for > > Gosh, I got completely distracted by the recent discussion about > pattern synchronization. > > So, to recap, we need to decide if we are taking Baolin's solution > or we're opting for implementing pattern trigger. I think we should take Baolin's solution. > > If we choose the latter, then we will also need some software > pattern engine in the trigger, to be applied as a software pattern > fallback for the devices without hardware pattern support. > It will certainly delay the contribution process, provided that Baolin > would find time for this work at all. > > I'd just take v5 based solution for now (with improved semantics > of disabling pattern - in this case my reasoning from the message > I'm replying to is still valid), > >> "echo 0 > brightness" as a command disabling pattern. The same operation >> disables triggers, so later transition to using pattern trigger will be >> seamless for userspace. >> > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-18 19:22 ` Jacek Anaszewski 2018-07-18 22:13 ` David Lechner @ 2018-07-18 22:17 ` Pavel Machek 2018-07-19 20:20 ` Pavel Machek 1 sibling, 1 reply; 38+ messages in thread From: Pavel Machek @ 2018-07-18 22:17 UTC (permalink / raw) To: Jacek Anaszewski Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 1258 bytes --] Hi! > >Please keep in mind that this is ABI documentation for the pattern file > >to be exposed by LED core, and not by the pattern trigger, that, as we > >agreed, will be implemented later. In this case, I'd go for > > Gosh, I got completely distracted by the recent discussion about > pattern synchronization. > > So, to recap, we need to decide if we are taking Baolin's solution > or we're opting for implementing pattern trigger. > > If we choose the latter, then we will also need some software > pattern engine in the trigger, to be applied as a software pattern > fallback for the devices without hardware pattern support. > It will certainly delay the contribution process, provided that Baolin > would find time for this work at all. I'd recommend the latter. Yes, software pattern as a fallback would be nice, but I have that code already... let me get it back to running state, and figure out where to add interface for "hardware acceleration". I'd like to have same interface to userland, whether pattern can be done by hardware or by software. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-18 22:17 ` Pavel Machek @ 2018-07-19 20:20 ` Pavel Machek 2018-07-20 18:08 ` Jacek Anaszewski 2018-07-23 6:59 ` Baolin Wang 0 siblings, 2 replies; 38+ messages in thread From: Pavel Machek @ 2018-07-19 20:20 UTC (permalink / raw) To: Jacek Anaszewski Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 20820 bytes --] Hi! > > >Please keep in mind that this is ABI documentation for the pattern file > > >to be exposed by LED core, and not by the pattern trigger, that, as we > > >agreed, will be implemented later. In this case, I'd go for > > > > Gosh, I got completely distracted by the recent discussion about > > pattern synchronization. > > > > So, to recap, we need to decide if we are taking Baolin's solution > > or we're opting for implementing pattern trigger. > > > > If we choose the latter, then we will also need some software > > pattern engine in the trigger, to be applied as a software pattern > > fallback for the devices without hardware pattern support. > > It will certainly delay the contribution process, provided that Baolin > > would find time for this work at all. > > I'd recommend the latter. Yes, software pattern as a fallback would be > nice, but I have that code already... let me get it back to running > state, and figure out where to add interface for "hardware > acceleration". I'd like to have same interface to userland, whether > pattern can be done by hardware or by software. For the record, I'd like something like this. (Software pattern should work. Hardware pattern... needs more work). diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index ede4fa0..8cf5962 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -51,6 +51,8 @@ static void led_timer_function(struct timer_list *t) unsigned long brightness; unsigned long delay; + /* FIXME spin_lock(led_cdev->lock); protecting led_cdev->flags? */ + if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) { led_set_brightness_nosleep(led_cdev, LED_OFF); clear_bit(LED_BLINK_SW, &led_cdev->work_flags); diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c index 9d9b7aa..898f92d 100644 --- a/drivers/leds/leds-sc27xx-bltc.c +++ b/drivers/leds/leds-sc27xx-bltc.c @@ -6,6 +6,7 @@ #include <linux/of.h> #include <linux/platform_device.h> #include <linux/regmap.h> +#include <linux/slab.h> #include <uapi/linux/uleds.h> /* PMIC global control register definition */ @@ -32,8 +33,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 +128,157 @@ 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) +{ + 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 struct led_pattern *sc27xx_led_pattern_get(struct led_classdev *ldev, + int *len) +{ + struct sc27xx_led *leds = to_sc27xx_led(ldev); + u32 base = sc27xx_led_get_offset(leds); + struct regmap *regmap = leds->priv->regmap; + struct led_pattern *pattern; + int i, err; + u32 val; + + /* + * Must allocate 4 patterns to show the rise time, high time, fall time + * and low time. + */ + pattern = kcalloc(SC27XX_LEDS_PATTERN_CNT, sizeof(*pattern), + GFP_KERNEL); + if (!pattern) + return ERR_PTR(-ENOMEM); + + mutex_lock(&leds->priv->lock); + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); + if (err) + goto out; + + pattern[0].delta_t = val & SC27XX_CURVE_L_MASK; + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); + if (err) + goto out; + + pattern[1].delta_t = val & SC27XX_CURVE_L_MASK; + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); + if (err) + goto out; + + pattern[2].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); + if (err) + goto out; + + pattern[3].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; + + err = regmap_read(regmap, base + SC27XX_LEDS_DUTY, &val); + if (err) + goto out; + + mutex_unlock(&leds->priv->lock); + + val = (val & SC27XX_DUTY_MASK) >> SC27XX_DUTY_SHIFT; + for (i = 0; i < SC27XX_LEDS_PATTERN_CNT; i++) + pattern[i].brightness = val; + + *len = SC27XX_LEDS_PATTERN_CNT; + + return pattern; + +out: + mutex_unlock(&leds->priv->lock); + kfree(pattern); + + return ERR_PTR(err); +} + static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv) { int i, err; @@ -140,6 +297,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_get = sc27xx_led_pattern_get; + led->ldev.pattern_clear = sc27xx_led_pattern_clear; err = devm_led_classdev_register(dev, &led->ldev); if (err) diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig index a2559b4..91ae5b0 100644 --- a/drivers/leds/trigger/Kconfig +++ b/drivers/leds/trigger/Kconfig @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA This enables direct flash/torch on/off by the driver, kernel space. If unsure, say Y. +config LEDS_TRIGGER_PATTERN + tristate "LED Pattern Trigger" + depends on LEDS_TRIGGERS + help + This allows LEDs blinking with an arbitrary pattern. Can be useful + on embedded systems with no screen to give out a status code to + a human. + + If unsure, say N + config LEDS_TRIGGER_PANIC bool "LED Panic Trigger" depends on LEDS_TRIGGERS diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile index f3cfe19..ba6f3b9 100644 --- a/drivers/leds/trigger/Makefile +++ b/drivers/leds/trigger/Makefile @@ -11,5 +11,6 @@ obj-$(CONFIG_LEDS_TRIGGER_ACTIVITY) += ledtrig-activity.o obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c new file mode 100644 index 0000000..d31808d --- /dev/null +++ b/drivers/leds/trigger/ledtrig-pattern.c @@ -0,0 +1,400 @@ +/* + * Arbitrary pattern trigger + * + * Copyright 2015, Epsiline + * + * Author : Raphaël Teysseyre <rteysseyre@gmail.com> + * + * Idea discussed with Pavel Machek <pavel@ucw.cz> on + * <linux-leds@vger.kernel.org> (march 2015, thread title + * [PATCH RFC] leds: Add status code trigger) + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + */ + +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/timer.h> +#include "../leds.h" + +struct pattern_step { + int brightness; + int time_ms; +}; + +struct pattern_trig_data { + struct led_classdev *led_cdev; + + struct pattern_step *steps; /* Array describing the pattern */ + struct mutex lock; + char is_sane; + struct pattern_step *curr; + struct pattern_step *next; + int time_ms; /* Time in current step */ + int nsteps; /* Number of steps */ + int repeat; /* < 0 means repeat indefinitely */ + struct timer_list timer; +}; + +#define MAX_NSTEPS (PAGE_SIZE/4) +/* The "pattern" attribute contains at most PAGE_SIZE characters. + Each line in this attribute is at least 4 characters long + (a 1-digit number for the led brighntess, a space, + a 1-digit number for the time, a semi-colon). + Therefore, there is at most PAGE_SIZE/4 steps. */ + +#define UPDATE_INTERVAL 50 +/* When doing gradual dimming, the led brightness + will be updated every UPDATE_INTERVAL milliseconds */ + +#define PATTERN_SEPARATOR "," + +static int pattern_trig_initialize_data(struct pattern_trig_data *data) +{ + mutex_init(&data->lock); + mutex_lock(&data->lock); + + data->is_sane = 0; + data->steps = kzalloc(MAX_NSTEPS*sizeof(struct pattern_step), + GFP_KERNEL); + if (!data->steps) + return -ENOMEM; + + data->curr = NULL; + data->next = NULL; + data->time_ms = 0; + data->nsteps = 0; + data->repeat = -1; + //data->timer = __TIMER_INITIALIZER(NULL, 0); + + mutex_unlock(&data->lock); + return 0; +} + +static void pattern_trig_clear_data(struct pattern_trig_data *data) +{ + data->is_sane = 0; + kfree(data->steps); +} + +/* + * is_sane : pattern checking. + * A pattern satisfying these three conditions is reported as sane : + * - At least two steps + * - At least one step with time >= UPDATE_INTERVAL + * - At least two steps with differing brightnesses + * When @data isn't sane, a sensible brightness + * default is suggested in @brightness + * + * DO NOT call pattern_trig_update on a not-sane pattern, + * you'll be punished with an infinite loop in the kernel. + */ +static int is_sane(struct pattern_trig_data *data, int *brightness) +{ + int i; + char stept_ok = 0; + char stepb_ok = 0; + + *brightness = 0; + if (data->nsteps < 1) + return 0; + + *brightness = data->steps[0].brightness; + if (data->nsteps < 2) + return 0; + + for (i = 0; i < data->nsteps; i++) { + if (data->steps[i].time_ms >= UPDATE_INTERVAL) { + /* FIXME: this is wrong */ + if (stepb_ok) + return 1; + stept_ok = 1; + } + if (data->steps[i].brightness != data->steps[0].brightness) { + if (stept_ok) + return 1; + stepb_ok = 1; + } + } + + return 0; +} + +static void reset_pattern(struct pattern_trig_data *data, + struct led_classdev *led_cdev) +{ + int brightness; + + if (led_cdev->pattern_clear) { + led_cdev->pattern_clear(led_cdev); + } + + del_timer_sync(&data->timer); + + if (led_cdev->pattern_set && led_cdev->pattern_set(led_cdev, data->steps, data->nsteps)) { + return; + } + + if (!is_sane(data, &brightness)) { + led_set_brightness(led_cdev, brightness); + return; + } + + data->curr = data->steps; + data->next = data->steps + 1; + data->time_ms = 0; + data->is_sane = 1; + + data->timer.expires = jiffies; + add_timer(&data->timer); +} + +/* --- Sysfs handling --- */ + +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; + + return scnprintf(buf, PAGE_SIZE, "%d\n", data->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; + long res; + int err; + + err = kstrtol(buf, 10, &res); + if (err) + return err; + + data->repeat = res < 0 ? -1 : res; + reset_pattern(data, led_cdev); + + return count; +} + +DEVICE_ATTR(repeat, S_IRUGO | S_IWUSR, + 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; + + if (!data->steps || !data->nsteps) + return 0; + + for (i = 0; i < data->nsteps; i++) + count += scnprintf(buf + count, PAGE_SIZE - count, + "%d %d" PATTERN_SEPARATOR, + data->steps[i].brightness, + data->steps[i].time_ms); + buf[count - 1] = '\n'; + buf[count] = '\0'; + + return count + 1; +} + +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 = 0; /* Characters read on one conversion */ + int tcr = 0; /* Total characters read */ + int ccount; /* Number of successful conversions */ + + mutex_lock(&data->lock); + data->is_sane = 0; + + for (data->nsteps = 0; data->nsteps < MAX_NSTEPS; data->nsteps++) { + cr = 0; + ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n", + &data->steps[data->nsteps].brightness, + &data->steps[data->nsteps].time_ms, &cr); + + if (!cr) { /* Invalid syntax or end of pattern */ + if (ccount == 2) + data->nsteps++; + mutex_unlock(&data->lock); + reset_pattern(data, led_cdev); + return count; + } + + tcr += cr; + } + + /* Shouldn't reach that */ + WARN(1, "MAX_NSTEP too small. Please report\n"); + mutex_unlock(&data->lock); + return count; +} + +DEVICE_ATTR(pattern, S_IRUGO | S_IWUSR, + pattern_trig_show_pattern, pattern_trig_store_pattern); + +static int pattern_trig_create_sysfs_files(struct device *dev) +{ + int err; + + err = device_create_file(dev, &dev_attr_repeat); + if (err) + return err; + + err = device_create_file(dev, &dev_attr_pattern); + if (err) + device_remove_file(dev, &dev_attr_repeat); + + return err; +} + +static void pattern_trig_remove_sysfs_files(struct device *dev) +{ + device_remove_file(dev, &dev_attr_pattern); + device_remove_file(dev, &dev_attr_repeat); +} + +/* --- Led intensity updating --- */ + +static int compute_brightness(struct pattern_trig_data *data) +{ + if (data->time_ms == 0) + return data->curr->brightness; + + if (data->curr->time_ms == 0) + return data->next->brightness; + + return data->curr->brightness + data->time_ms + * (data->next->brightness - data->curr->brightness) + / data->curr->time_ms; +} + +static void update_to_next_step(struct pattern_trig_data *data) +{ + data->curr = data->next; + if (data->curr == data->steps) + data->repeat--; + + if (data->next == data->steps + data->nsteps - 1) + data->next = data->steps; + else + data->next++; + + data->time_ms = 0; +} + +static void pattern_trig_update(struct timer_list *t) +{ + struct pattern_trig_data *data = from_timer(data, t, timer); + + mutex_lock(&data->lock); + + if (!data->is_sane || !data->repeat) { + mutex_unlock(&data->lock); + return; + } + + if (data->time_ms > data->curr->time_ms) + update_to_next_step(data); + + /* is_sane() checked that there is at least + one step with time_ms >= UPDATE_INTERVAL + so we won't go in an infinite loop */ + while (data->curr->time_ms < UPDATE_INTERVAL) + update_to_next_step(data); + + if (data->next->brightness == data->curr->brightness) { + /* Constant brightness for this step */ + led_set_brightness(data->led_cdev, data->curr->brightness); + mod_timer(&data->timer, jiffies + + msecs_to_jiffies(data->curr->time_ms)); + update_to_next_step(data); + } else { + /* Gradual dimming */ + led_set_brightness(data->led_cdev, compute_brightness(data)); + data->time_ms += UPDATE_INTERVAL; + mod_timer(&data->timer, jiffies + + msecs_to_jiffies(UPDATE_INTERVAL)); + } + + mutex_unlock(&data->lock); +} + +/* --- Trigger activation --- */ + +static void pattern_trig_activate(struct led_classdev *led_cdev) +{ + struct pattern_trig_data *data = NULL; + int err; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return; + + err = pattern_trig_initialize_data(data); + if (err) { + kfree(data); + return; + } + + data->led_cdev = led_cdev; + led_cdev->trigger_data = data; + timer_setup(&data->timer, pattern_trig_update, 0); + pattern_trig_create_sysfs_files(led_cdev->dev); +} + +static void pattern_trig_deactivate(struct led_classdev *led_cdev) +{ + struct pattern_trig_data *data = led_cdev->trigger_data; + + if (data) { + pattern_trig_remove_sysfs_files(led_cdev->dev); + del_timer_sync(&data->timer); + led_set_brightness(led_cdev, LED_OFF); + pattern_trig_clear_data(data); + kfree(data); + led_cdev->trigger_data = NULL; + } +} + +static struct led_trigger pattern_led_trigger = { + .name = "pattern", + .activate = pattern_trig_activate, + .deactivate = pattern_trig_deactivate, +}; + +/* --- Module loading/unloading --- */ + +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 <rteysseyre@gmail.com"); +MODULE_DESCRIPTION("Pattern LED trigger"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/leds.h b/include/linux/leds.h index b7e8255..6940ee2 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,14 @@ 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); + + int (*pattern_clear)(struct led_classdev *led_cdev); + + struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev, + int *len); + struct device *dev; const struct attribute_group **groups; @@ -101,7 +110,7 @@ struct led_classdev { void (*flash_resume)(struct led_classdev *led_cdev); struct work_struct set_brightness_work; - int delayed_set_value; + enum led_brightness delayed_set_value; #ifdef CONFIG_LEDS_TRIGGERS /* Protects the trigger data below */ @@ -446,4 +455,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 */ -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-19 20:20 ` Pavel Machek @ 2018-07-20 18:08 ` Jacek Anaszewski 2018-07-23 6:59 ` Baolin Wang 1 sibling, 0 replies; 38+ messages in thread From: Jacek Anaszewski @ 2018-07-20 18:08 UTC (permalink / raw) To: Pavel Machek Cc: David Lechner, Baolin Wang, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML Hi Pavel, On 07/19/2018 10:20 PM, Pavel Machek wrote: > Hi! > >>>> Please keep in mind that this is ABI documentation for the pattern file >>>> to be exposed by LED core, and not by the pattern trigger, that, as we >>>> agreed, will be implemented later. In this case, I'd go for >>> >>> Gosh, I got completely distracted by the recent discussion about >>> pattern synchronization. >>> >>> So, to recap, we need to decide if we are taking Baolin's solution >>> or we're opting for implementing pattern trigger. >>> >>> If we choose the latter, then we will also need some software >>> pattern engine in the trigger, to be applied as a software pattern >>> fallback for the devices without hardware pattern support. >>> It will certainly delay the contribution process, provided that Baolin >>> would find time for this work at all. >> >> I'd recommend the latter. Yes, software pattern as a fallback would be >> nice, but I have that code already... let me get it back to running >> state, and figure out where to add interface for "hardware >> acceleration". I'd like to have same interface to userland, whether >> pattern can be done by hardware or by software. > > For the record, I'd like something like this. (Software pattern should > work. Hardware pattern... needs more work). Thank you for the patch. I'll be able to comment on it in two weeks. I'll be offline during that time. Best regards, Jacek Anaszewski > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index ede4fa0..8cf5962 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -51,6 +51,8 @@ static void led_timer_function(struct timer_list *t) > unsigned long brightness; > unsigned long delay; > > + /* FIXME spin_lock(led_cdev->lock); protecting led_cdev->flags? */ > + > if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) { > led_set_brightness_nosleep(led_cdev, LED_OFF); > clear_bit(LED_BLINK_SW, &led_cdev->work_flags); > diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c > index 9d9b7aa..898f92d 100644 > --- a/drivers/leds/leds-sc27xx-bltc.c > +++ b/drivers/leds/leds-sc27xx-bltc.c > @@ -6,6 +6,7 @@ > #include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > +#include <linux/slab.h> > #include <uapi/linux/uleds.h> > > /* PMIC global control register definition */ > @@ -32,8 +33,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 +128,157 @@ 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) > +{ > + 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 struct led_pattern *sc27xx_led_pattern_get(struct led_classdev *ldev, > + int *len) > +{ > + struct sc27xx_led *leds = to_sc27xx_led(ldev); > + u32 base = sc27xx_led_get_offset(leds); > + struct regmap *regmap = leds->priv->regmap; > + struct led_pattern *pattern; > + int i, err; > + u32 val; > + > + /* > + * Must allocate 4 patterns to show the rise time, high time, fall time > + * and low time. > + */ > + pattern = kcalloc(SC27XX_LEDS_PATTERN_CNT, sizeof(*pattern), > + GFP_KERNEL); > + if (!pattern) > + return ERR_PTR(-ENOMEM); > + > + mutex_lock(&leds->priv->lock); > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); > + if (err) > + goto out; > + > + pattern[0].delta_t = val & SC27XX_CURVE_L_MASK; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); > + if (err) > + goto out; > + > + pattern[1].delta_t = val & SC27XX_CURVE_L_MASK; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); > + if (err) > + goto out; > + > + pattern[2].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); > + if (err) > + goto out; > + > + pattern[3].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_DUTY, &val); > + if (err) > + goto out; > + > + mutex_unlock(&leds->priv->lock); > + > + val = (val & SC27XX_DUTY_MASK) >> SC27XX_DUTY_SHIFT; > + for (i = 0; i < SC27XX_LEDS_PATTERN_CNT; i++) > + pattern[i].brightness = val; > + > + *len = SC27XX_LEDS_PATTERN_CNT; > + > + return pattern; > + > +out: > + mutex_unlock(&leds->priv->lock); > + kfree(pattern); > + > + return ERR_PTR(err); > +} > + > static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv) > { > int i, err; > @@ -140,6 +297,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_get = sc27xx_led_pattern_get; > + led->ldev.pattern_clear = sc27xx_led_pattern_clear; > > err = devm_led_classdev_register(dev, &led->ldev); > if (err) > diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig > index a2559b4..91ae5b0 100644 > --- a/drivers/leds/trigger/Kconfig > +++ b/drivers/leds/trigger/Kconfig > @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA > This enables direct flash/torch on/off by the driver, kernel space. > If unsure, say Y. > > +config LEDS_TRIGGER_PATTERN > + tristate "LED Pattern Trigger" > + depends on LEDS_TRIGGERS > + help > + This allows LEDs blinking with an arbitrary pattern. Can be useful > + on embedded systems with no screen to give out a status code to > + a human. > + > + If unsure, say N > + > config LEDS_TRIGGER_PANIC > bool "LED Panic Trigger" > depends on LEDS_TRIGGERS > diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile > index f3cfe19..ba6f3b9 100644 > --- a/drivers/leds/trigger/Makefile > +++ b/drivers/leds/trigger/Makefile > @@ -11,5 +11,6 @@ obj-$(CONFIG_LEDS_TRIGGER_ACTIVITY) += ledtrig-activity.o > obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o > obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o > obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o > +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o > obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o > obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o > diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c > new file mode 100644 > index 0000000..d31808d > --- /dev/null > +++ b/drivers/leds/trigger/ledtrig-pattern.c > @@ -0,0 +1,400 @@ > +/* > + * Arbitrary pattern trigger > + * > + * Copyright 2015, Epsiline > + * > + * Author : Raphaël Teysseyre <rteysseyre@gmail.com> > + * > + * Idea discussed with Pavel Machek <pavel@ucw.cz> on > + * <linux-leds@vger.kernel.org> (march 2015, thread title > + * [PATCH RFC] leds: Add status code trigger) > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + */ > + > +#include <linux/types.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/timer.h> > +#include "../leds.h" > + > +struct pattern_step { > + int brightness; > + int time_ms; > +}; > + > +struct pattern_trig_data { > + struct led_classdev *led_cdev; > + > + struct pattern_step *steps; /* Array describing the pattern */ > + struct mutex lock; > + char is_sane; > + struct pattern_step *curr; > + struct pattern_step *next; > + int time_ms; /* Time in current step */ > + int nsteps; /* Number of steps */ > + int repeat; /* < 0 means repeat indefinitely */ > + struct timer_list timer; > +}; > + > +#define MAX_NSTEPS (PAGE_SIZE/4) > +/* The "pattern" attribute contains at most PAGE_SIZE characters. > + Each line in this attribute is at least 4 characters long > + (a 1-digit number for the led brighntess, a space, > + a 1-digit number for the time, a semi-colon). > + Therefore, there is at most PAGE_SIZE/4 steps. */ > + > +#define UPDATE_INTERVAL 50 > +/* When doing gradual dimming, the led brightness > + will be updated every UPDATE_INTERVAL milliseconds */ > + > +#define PATTERN_SEPARATOR "," > + > +static int pattern_trig_initialize_data(struct pattern_trig_data *data) > +{ > + mutex_init(&data->lock); > + mutex_lock(&data->lock); > + > + data->is_sane = 0; > + data->steps = kzalloc(MAX_NSTEPS*sizeof(struct pattern_step), > + GFP_KERNEL); > + if (!data->steps) > + return -ENOMEM; > + > + data->curr = NULL; > + data->next = NULL; > + data->time_ms = 0; > + data->nsteps = 0; > + data->repeat = -1; > + //data->timer = __TIMER_INITIALIZER(NULL, 0); > + > + mutex_unlock(&data->lock); > + return 0; > +} > + > +static void pattern_trig_clear_data(struct pattern_trig_data *data) > +{ > + data->is_sane = 0; > + kfree(data->steps); > +} > + > +/* > + * is_sane : pattern checking. > + * A pattern satisfying these three conditions is reported as sane : > + * - At least two steps > + * - At least one step with time >= UPDATE_INTERVAL > + * - At least two steps with differing brightnesses > + * When @data isn't sane, a sensible brightness > + * default is suggested in @brightness > + * > + * DO NOT call pattern_trig_update on a not-sane pattern, > + * you'll be punished with an infinite loop in the kernel. > + */ > +static int is_sane(struct pattern_trig_data *data, int *brightness) > +{ > + int i; > + char stept_ok = 0; > + char stepb_ok = 0; > + > + *brightness = 0; > + if (data->nsteps < 1) > + return 0; > + > + *brightness = data->steps[0].brightness; > + if (data->nsteps < 2) > + return 0; > + > + for (i = 0; i < data->nsteps; i++) { > + if (data->steps[i].time_ms >= UPDATE_INTERVAL) { > + /* FIXME: this is wrong */ > + if (stepb_ok) > + return 1; > + stept_ok = 1; > + } > + if (data->steps[i].brightness != data->steps[0].brightness) { > + if (stept_ok) > + return 1; > + stepb_ok = 1; > + } > + } > + > + return 0; > +} > + > +static void reset_pattern(struct pattern_trig_data *data, > + struct led_classdev *led_cdev) > +{ > + int brightness; > + > + if (led_cdev->pattern_clear) { > + led_cdev->pattern_clear(led_cdev); > + } > + > + del_timer_sync(&data->timer); > + > + if (led_cdev->pattern_set && led_cdev->pattern_set(led_cdev, data->steps, data->nsteps)) { > + return; > + } > + > + if (!is_sane(data, &brightness)) { > + led_set_brightness(led_cdev, brightness); > + return; > + } > + > + data->curr = data->steps; > + data->next = data->steps + 1; > + data->time_ms = 0; > + data->is_sane = 1; > + > + data->timer.expires = jiffies; > + add_timer(&data->timer); > +} > + > +/* --- Sysfs handling --- */ > + > +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; > + > + return scnprintf(buf, PAGE_SIZE, "%d\n", data->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; > + long res; > + int err; > + > + err = kstrtol(buf, 10, &res); > + if (err) > + return err; > + > + data->repeat = res < 0 ? -1 : res; > + reset_pattern(data, led_cdev); > + > + return count; > +} > + > +DEVICE_ATTR(repeat, S_IRUGO | S_IWUSR, > + 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; > + > + if (!data->steps || !data->nsteps) > + return 0; > + > + for (i = 0; i < data->nsteps; i++) > + count += scnprintf(buf + count, PAGE_SIZE - count, > + "%d %d" PATTERN_SEPARATOR, > + data->steps[i].brightness, > + data->steps[i].time_ms); > + buf[count - 1] = '\n'; > + buf[count] = '\0'; > + > + return count + 1; > +} > + > +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 = 0; /* Characters read on one conversion */ > + int tcr = 0; /* Total characters read */ > + int ccount; /* Number of successful conversions */ > + > + mutex_lock(&data->lock); > + data->is_sane = 0; > + > + for (data->nsteps = 0; data->nsteps < MAX_NSTEPS; data->nsteps++) { > + cr = 0; > + ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n", > + &data->steps[data->nsteps].brightness, > + &data->steps[data->nsteps].time_ms, &cr); > + > + if (!cr) { /* Invalid syntax or end of pattern */ > + if (ccount == 2) > + data->nsteps++; > + mutex_unlock(&data->lock); > + reset_pattern(data, led_cdev); > + return count; > + } > + > + tcr += cr; > + } > + > + /* Shouldn't reach that */ > + WARN(1, "MAX_NSTEP too small. Please report\n"); > + mutex_unlock(&data->lock); > + return count; > +} > + > +DEVICE_ATTR(pattern, S_IRUGO | S_IWUSR, > + pattern_trig_show_pattern, pattern_trig_store_pattern); > + > +static int pattern_trig_create_sysfs_files(struct device *dev) > +{ > + int err; > + > + err = device_create_file(dev, &dev_attr_repeat); > + if (err) > + return err; > + > + err = device_create_file(dev, &dev_attr_pattern); > + if (err) > + device_remove_file(dev, &dev_attr_repeat); > + > + return err; > +} > + > +static void pattern_trig_remove_sysfs_files(struct device *dev) > +{ > + device_remove_file(dev, &dev_attr_pattern); > + device_remove_file(dev, &dev_attr_repeat); > +} > + > +/* --- Led intensity updating --- */ > + > +static int compute_brightness(struct pattern_trig_data *data) > +{ > + if (data->time_ms == 0) > + return data->curr->brightness; > + > + if (data->curr->time_ms == 0) > + return data->next->brightness; > + > + return data->curr->brightness + data->time_ms > + * (data->next->brightness - data->curr->brightness) > + / data->curr->time_ms; > +} > + > +static void update_to_next_step(struct pattern_trig_data *data) > +{ > + data->curr = data->next; > + if (data->curr == data->steps) > + data->repeat--; > + > + if (data->next == data->steps + data->nsteps - 1) > + data->next = data->steps; > + else > + data->next++; > + > + data->time_ms = 0; > +} > + > +static void pattern_trig_update(struct timer_list *t) > +{ > + struct pattern_trig_data *data = from_timer(data, t, timer); > + > + mutex_lock(&data->lock); > + > + if (!data->is_sane || !data->repeat) { > + mutex_unlock(&data->lock); > + return; > + } > + > + if (data->time_ms > data->curr->time_ms) > + update_to_next_step(data); > + > + /* is_sane() checked that there is at least > + one step with time_ms >= UPDATE_INTERVAL > + so we won't go in an infinite loop */ > + while (data->curr->time_ms < UPDATE_INTERVAL) > + update_to_next_step(data); > + > + if (data->next->brightness == data->curr->brightness) { > + /* Constant brightness for this step */ > + led_set_brightness(data->led_cdev, data->curr->brightness); > + mod_timer(&data->timer, jiffies > + + msecs_to_jiffies(data->curr->time_ms)); > + update_to_next_step(data); > + } else { > + /* Gradual dimming */ > + led_set_brightness(data->led_cdev, compute_brightness(data)); > + data->time_ms += UPDATE_INTERVAL; > + mod_timer(&data->timer, jiffies > + + msecs_to_jiffies(UPDATE_INTERVAL)); > + } > + > + mutex_unlock(&data->lock); > +} > + > +/* --- Trigger activation --- */ > + > +static void pattern_trig_activate(struct led_classdev *led_cdev) > +{ > + struct pattern_trig_data *data = NULL; > + int err; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return; > + > + err = pattern_trig_initialize_data(data); > + if (err) { > + kfree(data); > + return; > + } > + > + data->led_cdev = led_cdev; > + led_cdev->trigger_data = data; > + timer_setup(&data->timer, pattern_trig_update, 0); > + pattern_trig_create_sysfs_files(led_cdev->dev); > +} > + > +static void pattern_trig_deactivate(struct led_classdev *led_cdev) > +{ > + struct pattern_trig_data *data = led_cdev->trigger_data; > + > + if (data) { > + pattern_trig_remove_sysfs_files(led_cdev->dev); > + del_timer_sync(&data->timer); > + led_set_brightness(led_cdev, LED_OFF); > + pattern_trig_clear_data(data); > + kfree(data); > + led_cdev->trigger_data = NULL; > + } > +} > + > +static struct led_trigger pattern_led_trigger = { > + .name = "pattern", > + .activate = pattern_trig_activate, > + .deactivate = pattern_trig_deactivate, > +}; > + > +/* --- Module loading/unloading --- */ > + > +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 <rteysseyre@gmail.com"); > +MODULE_DESCRIPTION("Pattern LED trigger"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/leds.h b/include/linux/leds.h > index b7e8255..6940ee2 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,14 @@ 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); > + > + int (*pattern_clear)(struct led_classdev *led_cdev); > + > + struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev, > + int *len); > + > struct device *dev; > const struct attribute_group **groups; > > @@ -101,7 +110,7 @@ struct led_classdev { > void (*flash_resume)(struct led_classdev *led_cdev); > > struct work_struct set_brightness_work; > - int delayed_set_value; > + enum led_brightness delayed_set_value; > > #ifdef CONFIG_LEDS_TRIGGERS > /* Protects the trigger data below */ > @@ -446,4 +455,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 */ > > > > > ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-19 20:20 ` Pavel Machek 2018-07-20 18:08 ` Jacek Anaszewski @ 2018-07-23 6:59 ` Baolin Wang 2018-07-24 11:41 ` Pavel Machek 2018-07-24 11:50 ` Pavel Machek 1 sibling, 2 replies; 38+ messages in thread From: Baolin Wang @ 2018-07-23 6:59 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML Hi Pavel, On 20 July 2018 at 04:20, Pavel Machek <pavel@ucw.cz> wrote: > Hi! > >> > >Please keep in mind that this is ABI documentation for the pattern file >> > >to be exposed by LED core, and not by the pattern trigger, that, as we >> > >agreed, will be implemented later. In this case, I'd go for >> > >> > Gosh, I got completely distracted by the recent discussion about >> > pattern synchronization. >> > >> > So, to recap, we need to decide if we are taking Baolin's solution >> > or we're opting for implementing pattern trigger. >> > >> > If we choose the latter, then we will also need some software >> > pattern engine in the trigger, to be applied as a software pattern >> > fallback for the devices without hardware pattern support. >> > It will certainly delay the contribution process, provided that Baolin >> > would find time for this work at all. >> >> I'd recommend the latter. Yes, software pattern as a fallback would be >> nice, but I have that code already... let me get it back to running >> state, and figure out where to add interface for "hardware >> acceleration". I'd like to have same interface to userland, whether >> pattern can be done by hardware or by software. > > For the record, I'd like something like this. (Software pattern should > work. Hardware pattern... needs more work). Thanks for showing your thoughts. But I failed to compile your code, would you like to send out formal patches (Or only including software pattern, I will help to add hardware pattern part and do some testing with our driver)? Thanks. > > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index ede4fa0..8cf5962 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -51,6 +51,8 @@ static void led_timer_function(struct timer_list *t) > unsigned long brightness; > unsigned long delay; > > + /* FIXME spin_lock(led_cdev->lock); protecting led_cdev->flags? */ > + > if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) { > led_set_brightness_nosleep(led_cdev, LED_OFF); > clear_bit(LED_BLINK_SW, &led_cdev->work_flags); > diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c > index 9d9b7aa..898f92d 100644 > --- a/drivers/leds/leds-sc27xx-bltc.c > +++ b/drivers/leds/leds-sc27xx-bltc.c > @@ -6,6 +6,7 @@ > #include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > +#include <linux/slab.h> > #include <uapi/linux/uleds.h> > > /* PMIC global control register definition */ > @@ -32,8 +33,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 +128,157 @@ 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) > +{ > + 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 struct led_pattern *sc27xx_led_pattern_get(struct led_classdev *ldev, > + int *len) > +{ > + struct sc27xx_led *leds = to_sc27xx_led(ldev); > + u32 base = sc27xx_led_get_offset(leds); > + struct regmap *regmap = leds->priv->regmap; > + struct led_pattern *pattern; > + int i, err; > + u32 val; > + > + /* > + * Must allocate 4 patterns to show the rise time, high time, fall time > + * and low time. > + */ > + pattern = kcalloc(SC27XX_LEDS_PATTERN_CNT, sizeof(*pattern), > + GFP_KERNEL); > + if (!pattern) > + return ERR_PTR(-ENOMEM); > + > + mutex_lock(&leds->priv->lock); > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); > + if (err) > + goto out; > + > + pattern[0].delta_t = val & SC27XX_CURVE_L_MASK; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); > + if (err) > + goto out; > + > + pattern[1].delta_t = val & SC27XX_CURVE_L_MASK; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); > + if (err) > + goto out; > + > + pattern[2].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); > + if (err) > + goto out; > + > + pattern[3].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_DUTY, &val); > + if (err) > + goto out; > + > + mutex_unlock(&leds->priv->lock); > + > + val = (val & SC27XX_DUTY_MASK) >> SC27XX_DUTY_SHIFT; > + for (i = 0; i < SC27XX_LEDS_PATTERN_CNT; i++) > + pattern[i].brightness = val; > + > + *len = SC27XX_LEDS_PATTERN_CNT; > + > + return pattern; > + > +out: > + mutex_unlock(&leds->priv->lock); > + kfree(pattern); > + > + return ERR_PTR(err); > +} > + > static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv) > { > int i, err; > @@ -140,6 +297,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_get = sc27xx_led_pattern_get; > + led->ldev.pattern_clear = sc27xx_led_pattern_clear; > > err = devm_led_classdev_register(dev, &led->ldev); > if (err) > diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig > index a2559b4..91ae5b0 100644 > --- a/drivers/leds/trigger/Kconfig > +++ b/drivers/leds/trigger/Kconfig > @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA > This enables direct flash/torch on/off by the driver, kernel space. > If unsure, say Y. > > +config LEDS_TRIGGER_PATTERN > + tristate "LED Pattern Trigger" > + depends on LEDS_TRIGGERS > + help > + This allows LEDs blinking with an arbitrary pattern. Can be useful > + on embedded systems with no screen to give out a status code to > + a human. > + > + If unsure, say N > + > config LEDS_TRIGGER_PANIC > bool "LED Panic Trigger" > depends on LEDS_TRIGGERS > diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile > index f3cfe19..ba6f3b9 100644 > --- a/drivers/leds/trigger/Makefile > +++ b/drivers/leds/trigger/Makefile > @@ -11,5 +11,6 @@ obj-$(CONFIG_LEDS_TRIGGER_ACTIVITY) += ledtrig-activity.o > obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o > obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o > obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o > +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o > obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o > obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o > diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c > new file mode 100644 > index 0000000..d31808d > --- /dev/null > +++ b/drivers/leds/trigger/ledtrig-pattern.c > @@ -0,0 +1,400 @@ > +/* > + * Arbitrary pattern trigger > + * > + * Copyright 2015, Epsiline > + * > + * Author : Raphaël Teysseyre <rteysseyre@gmail.com> > + * > + * Idea discussed with Pavel Machek <pavel@ucw.cz> on > + * <linux-leds@vger.kernel.org> (march 2015, thread title > + * [PATCH RFC] leds: Add status code trigger) > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + */ > + > +#include <linux/types.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/timer.h> > +#include "../leds.h" > + > +struct pattern_step { > + int brightness; > + int time_ms; > +}; > + > +struct pattern_trig_data { > + struct led_classdev *led_cdev; > + > + struct pattern_step *steps; /* Array describing the pattern */ > + struct mutex lock; > + char is_sane; > + struct pattern_step *curr; > + struct pattern_step *next; > + int time_ms; /* Time in current step */ > + int nsteps; /* Number of steps */ > + int repeat; /* < 0 means repeat indefinitely */ > + struct timer_list timer; > +}; > + > +#define MAX_NSTEPS (PAGE_SIZE/4) > +/* The "pattern" attribute contains at most PAGE_SIZE characters. > + Each line in this attribute is at least 4 characters long > + (a 1-digit number for the led brighntess, a space, > + a 1-digit number for the time, a semi-colon). > + Therefore, there is at most PAGE_SIZE/4 steps. */ > + > +#define UPDATE_INTERVAL 50 > +/* When doing gradual dimming, the led brightness > + will be updated every UPDATE_INTERVAL milliseconds */ > + > +#define PATTERN_SEPARATOR "," > + > +static int pattern_trig_initialize_data(struct pattern_trig_data *data) > +{ > + mutex_init(&data->lock); > + mutex_lock(&data->lock); > + > + data->is_sane = 0; > + data->steps = kzalloc(MAX_NSTEPS*sizeof(struct pattern_step), > + GFP_KERNEL); > + if (!data->steps) > + return -ENOMEM; > + > + data->curr = NULL; > + data->next = NULL; > + data->time_ms = 0; > + data->nsteps = 0; > + data->repeat = -1; > + //data->timer = __TIMER_INITIALIZER(NULL, 0); > + > + mutex_unlock(&data->lock); > + return 0; > +} > + > +static void pattern_trig_clear_data(struct pattern_trig_data *data) > +{ > + data->is_sane = 0; > + kfree(data->steps); > +} > + > +/* > + * is_sane : pattern checking. > + * A pattern satisfying these three conditions is reported as sane : > + * - At least two steps > + * - At least one step with time >= UPDATE_INTERVAL > + * - At least two steps with differing brightnesses > + * When @data isn't sane, a sensible brightness > + * default is suggested in @brightness > + * > + * DO NOT call pattern_trig_update on a not-sane pattern, > + * you'll be punished with an infinite loop in the kernel. > + */ > +static int is_sane(struct pattern_trig_data *data, int *brightness) > +{ > + int i; > + char stept_ok = 0; > + char stepb_ok = 0; > + > + *brightness = 0; > + if (data->nsteps < 1) > + return 0; > + > + *brightness = data->steps[0].brightness; > + if (data->nsteps < 2) > + return 0; > + > + for (i = 0; i < data->nsteps; i++) { > + if (data->steps[i].time_ms >= UPDATE_INTERVAL) { > + /* FIXME: this is wrong */ > + if (stepb_ok) > + return 1; > + stept_ok = 1; > + } > + if (data->steps[i].brightness != data->steps[0].brightness) { > + if (stept_ok) > + return 1; > + stepb_ok = 1; > + } > + } > + > + return 0; > +} > + > +static void reset_pattern(struct pattern_trig_data *data, > + struct led_classdev *led_cdev) > +{ > + int brightness; > + > + if (led_cdev->pattern_clear) { > + led_cdev->pattern_clear(led_cdev); > + } > + > + del_timer_sync(&data->timer); > + > + if (led_cdev->pattern_set && led_cdev->pattern_set(led_cdev, data->steps, data->nsteps)) { > + return; > + } > + > + if (!is_sane(data, &brightness)) { > + led_set_brightness(led_cdev, brightness); > + return; > + } > + > + data->curr = data->steps; > + data->next = data->steps + 1; > + data->time_ms = 0; > + data->is_sane = 1; > + > + data->timer.expires = jiffies; > + add_timer(&data->timer); > +} > + > +/* --- Sysfs handling --- */ > + > +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; > + > + return scnprintf(buf, PAGE_SIZE, "%d\n", data->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; > + long res; > + int err; > + > + err = kstrtol(buf, 10, &res); > + if (err) > + return err; > + > + data->repeat = res < 0 ? -1 : res; > + reset_pattern(data, led_cdev); > + > + return count; > +} > + > +DEVICE_ATTR(repeat, S_IRUGO | S_IWUSR, > + 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; > + > + if (!data->steps || !data->nsteps) > + return 0; > + > + for (i = 0; i < data->nsteps; i++) > + count += scnprintf(buf + count, PAGE_SIZE - count, > + "%d %d" PATTERN_SEPARATOR, > + data->steps[i].brightness, > + data->steps[i].time_ms); > + buf[count - 1] = '\n'; > + buf[count] = '\0'; > + > + return count + 1; > +} > + > +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 = 0; /* Characters read on one conversion */ > + int tcr = 0; /* Total characters read */ > + int ccount; /* Number of successful conversions */ > + > + mutex_lock(&data->lock); > + data->is_sane = 0; > + > + for (data->nsteps = 0; data->nsteps < MAX_NSTEPS; data->nsteps++) { > + cr = 0; > + ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n", > + &data->steps[data->nsteps].brightness, > + &data->steps[data->nsteps].time_ms, &cr); > + > + if (!cr) { /* Invalid syntax or end of pattern */ > + if (ccount == 2) > + data->nsteps++; > + mutex_unlock(&data->lock); > + reset_pattern(data, led_cdev); > + return count; > + } > + > + tcr += cr; > + } > + > + /* Shouldn't reach that */ > + WARN(1, "MAX_NSTEP too small. Please report\n"); > + mutex_unlock(&data->lock); > + return count; > +} > + > +DEVICE_ATTR(pattern, S_IRUGO | S_IWUSR, > + pattern_trig_show_pattern, pattern_trig_store_pattern); > + > +static int pattern_trig_create_sysfs_files(struct device *dev) > +{ > + int err; > + > + err = device_create_file(dev, &dev_attr_repeat); > + if (err) > + return err; > + > + err = device_create_file(dev, &dev_attr_pattern); > + if (err) > + device_remove_file(dev, &dev_attr_repeat); > + > + return err; > +} > + > +static void pattern_trig_remove_sysfs_files(struct device *dev) > +{ > + device_remove_file(dev, &dev_attr_pattern); > + device_remove_file(dev, &dev_attr_repeat); > +} > + > +/* --- Led intensity updating --- */ > + > +static int compute_brightness(struct pattern_trig_data *data) > +{ > + if (data->time_ms == 0) > + return data->curr->brightness; > + > + if (data->curr->time_ms == 0) > + return data->next->brightness; > + > + return data->curr->brightness + data->time_ms > + * (data->next->brightness - data->curr->brightness) > + / data->curr->time_ms; > +} > + > +static void update_to_next_step(struct pattern_trig_data *data) > +{ > + data->curr = data->next; > + if (data->curr == data->steps) > + data->repeat--; > + > + if (data->next == data->steps + data->nsteps - 1) > + data->next = data->steps; > + else > + data->next++; > + > + data->time_ms = 0; > +} > + > +static void pattern_trig_update(struct timer_list *t) > +{ > + struct pattern_trig_data *data = from_timer(data, t, timer); > + > + mutex_lock(&data->lock); > + > + if (!data->is_sane || !data->repeat) { > + mutex_unlock(&data->lock); > + return; > + } > + > + if (data->time_ms > data->curr->time_ms) > + update_to_next_step(data); > + > + /* is_sane() checked that there is at least > + one step with time_ms >= UPDATE_INTERVAL > + so we won't go in an infinite loop */ > + while (data->curr->time_ms < UPDATE_INTERVAL) > + update_to_next_step(data); > + > + if (data->next->brightness == data->curr->brightness) { > + /* Constant brightness for this step */ > + led_set_brightness(data->led_cdev, data->curr->brightness); > + mod_timer(&data->timer, jiffies > + + msecs_to_jiffies(data->curr->time_ms)); > + update_to_next_step(data); > + } else { > + /* Gradual dimming */ > + led_set_brightness(data->led_cdev, compute_brightness(data)); > + data->time_ms += UPDATE_INTERVAL; > + mod_timer(&data->timer, jiffies > + + msecs_to_jiffies(UPDATE_INTERVAL)); > + } > + > + mutex_unlock(&data->lock); > +} > + > +/* --- Trigger activation --- */ > + > +static void pattern_trig_activate(struct led_classdev *led_cdev) > +{ > + struct pattern_trig_data *data = NULL; > + int err; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return; > + > + err = pattern_trig_initialize_data(data); > + if (err) { > + kfree(data); > + return; > + } > + > + data->led_cdev = led_cdev; > + led_cdev->trigger_data = data; > + timer_setup(&data->timer, pattern_trig_update, 0); > + pattern_trig_create_sysfs_files(led_cdev->dev); > +} > + > +static void pattern_trig_deactivate(struct led_classdev *led_cdev) > +{ > + struct pattern_trig_data *data = led_cdev->trigger_data; > + > + if (data) { > + pattern_trig_remove_sysfs_files(led_cdev->dev); > + del_timer_sync(&data->timer); > + led_set_brightness(led_cdev, LED_OFF); > + pattern_trig_clear_data(data); > + kfree(data); > + led_cdev->trigger_data = NULL; > + } > +} > + > +static struct led_trigger pattern_led_trigger = { > + .name = "pattern", > + .activate = pattern_trig_activate, > + .deactivate = pattern_trig_deactivate, > +}; > + > +/* --- Module loading/unloading --- */ > + > +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 <rteysseyre@gmail.com"); > +MODULE_DESCRIPTION("Pattern LED trigger"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/leds.h b/include/linux/leds.h > index b7e8255..6940ee2 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,14 @@ 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); > + > + int (*pattern_clear)(struct led_classdev *led_cdev); > + > + struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev, > + int *len); > + > struct device *dev; > const struct attribute_group **groups; > > @@ -101,7 +110,7 @@ struct led_classdev { > void (*flash_resume)(struct led_classdev *led_cdev); > > struct work_struct set_brightness_work; > - int delayed_set_value; > + enum led_brightness delayed_set_value; > > #ifdef CONFIG_LEDS_TRIGGERS > /* Protects the trigger data below */ > @@ -446,4 +455,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 */ > > > > > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- Baolin Wang Best Regards ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-23 6:59 ` Baolin Wang @ 2018-07-24 11:41 ` Pavel Machek 2018-07-27 5:15 ` Baolin Wang 2018-07-24 11:50 ` Pavel Machek 1 sibling, 1 reply; 38+ messages in thread From: Pavel Machek @ 2018-07-24 11:41 UTC (permalink / raw) To: Baolin Wang Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 20702 bytes --] Hi! > >> > >Please keep in mind that this is ABI documentation for the pattern file > >> > >to be exposed by LED core, and not by the pattern trigger, that, as we > >> > >agreed, will be implemented later. In this case, I'd go for > >> > > >> > Gosh, I got completely distracted by the recent discussion about > >> > pattern synchronization. > >> > > >> > So, to recap, we need to decide if we are taking Baolin's solution > >> > or we're opting for implementing pattern trigger. > >> > > >> > If we choose the latter, then we will also need some software > >> > pattern engine in the trigger, to be applied as a software pattern > >> > fallback for the devices without hardware pattern support. > >> > It will certainly delay the contribution process, provided that Baolin > >> > would find time for this work at all. > >> > >> I'd recommend the latter. Yes, software pattern as a fallback would be > >> nice, but I have that code already... let me get it back to running > >> state, and figure out where to add interface for "hardware > >> acceleration". I'd like to have same interface to userland, whether > >> pattern can be done by hardware or by software. > > > > For the record, I'd like something like this. (Software pattern should > > work. Hardware pattern... needs more work). > > Thanks for showing your thoughts. But I failed to compile your code, > would you like to send out formal patches (Or only including software > pattern, I will help to add hardware pattern part and do some testing > with our driver)? Thanks. This should be a bit better. I attempted to compile it with your driver, but whether it works is an open question. Signed-off-by: Pavel Machek <pavel@ucw.cz> diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c index 9d9b7aa..898f92d 100644 --- a/drivers/leds/leds-sc27xx-bltc.c +++ b/drivers/leds/leds-sc27xx-bltc.c @@ -6,6 +6,7 @@ #include <linux/of.h> #include <linux/platform_device.h> #include <linux/regmap.h> +#include <linux/slab.h> #include <uapi/linux/uleds.h> /* PMIC global control register definition */ @@ -32,8 +33,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 +128,157 @@ 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) +{ + 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 struct led_pattern *sc27xx_led_pattern_get(struct led_classdev *ldev, + int *len) +{ + struct sc27xx_led *leds = to_sc27xx_led(ldev); + u32 base = sc27xx_led_get_offset(leds); + struct regmap *regmap = leds->priv->regmap; + struct led_pattern *pattern; + int i, err; + u32 val; + + /* + * Must allocate 4 patterns to show the rise time, high time, fall time + * and low time. + */ + pattern = kcalloc(SC27XX_LEDS_PATTERN_CNT, sizeof(*pattern), + GFP_KERNEL); + if (!pattern) + return ERR_PTR(-ENOMEM); + + mutex_lock(&leds->priv->lock); + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); + if (err) + goto out; + + pattern[0].delta_t = val & SC27XX_CURVE_L_MASK; + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); + if (err) + goto out; + + pattern[1].delta_t = val & SC27XX_CURVE_L_MASK; + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); + if (err) + goto out; + + pattern[2].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; + + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); + if (err) + goto out; + + pattern[3].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; + + err = regmap_read(regmap, base + SC27XX_LEDS_DUTY, &val); + if (err) + goto out; + + mutex_unlock(&leds->priv->lock); + + val = (val & SC27XX_DUTY_MASK) >> SC27XX_DUTY_SHIFT; + for (i = 0; i < SC27XX_LEDS_PATTERN_CNT; i++) + pattern[i].brightness = val; + + *len = SC27XX_LEDS_PATTERN_CNT; + + return pattern; + +out: + mutex_unlock(&leds->priv->lock); + kfree(pattern); + + return ERR_PTR(err); +} + static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv) { int i, err; @@ -140,6 +297,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_get = sc27xx_led_pattern_get; + led->ldev.pattern_clear = sc27xx_led_pattern_clear; err = devm_led_classdev_register(dev, &led->ldev); if (err) diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig index a2559b4..91ae5b0 100644 --- a/drivers/leds/trigger/Kconfig +++ b/drivers/leds/trigger/Kconfig @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA This enables direct flash/torch on/off by the driver, kernel space. If unsure, say Y. +config LEDS_TRIGGER_PATTERN + tristate "LED Pattern Trigger" + depends on LEDS_TRIGGERS + help + This allows LEDs blinking with an arbitrary pattern. Can be useful + on embedded systems with no screen to give out a status code to + a human. + + If unsure, say N + config LEDS_TRIGGER_PANIC bool "LED Panic Trigger" depends on LEDS_TRIGGERS diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile index f3cfe19..ba6f3b9 100644 --- a/drivers/leds/trigger/Makefile +++ b/drivers/leds/trigger/Makefile @@ -11,5 +11,6 @@ obj-$(CONFIG_LEDS_TRIGGER_ACTIVITY) += ledtrig-activity.o obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c new file mode 100644 index 0000000..3dab050 --- /dev/null +++ b/drivers/leds/trigger/ledtrig-pattern.c @@ -0,0 +1,395 @@ +/* + * Arbitrary pattern trigger + * + * Copyright 2015, Epsiline + * + * Author : Raphaël Teysseyre <rteysseyre@gmail.com> + * + * Idea discussed with Pavel Machek <pavel@ucw.cz> on + * <linux-leds@vger.kernel.org> (march 2015, thread title + * [PATCH RFC] leds: Add status code trigger) + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + */ + +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/timer.h> +#include "../leds.h" + +struct pattern_trig_data { + struct led_classdev *led_cdev; + + struct led_pattern *steps; /* Array describing the pattern */ + struct mutex lock; + char is_sane; + struct led_pattern *curr; + struct led_pattern *next; + int delta_t; /* Time in current step */ + int nsteps; /* Number of steps */ + int repeat; /* < 0 means repeat indefinitely */ + struct timer_list timer; +}; + +#define MAX_NSTEPS (PAGE_SIZE/4) +/* The "pattern" attribute contains at most PAGE_SIZE characters. + Each line in this attribute is at least 4 characters long + (a 1-digit number for the led brighntess, a space, + a 1-digit number for the time, a semi-colon). + Therefore, there is at most PAGE_SIZE/4 steps. */ + +#define UPDATE_INTERVAL 50 +/* When doing gradual dimming, the led brightness + will be updated every UPDATE_INTERVAL milliseconds */ + +#define PATTERN_SEPARATOR "," + +static int pattern_trig_initialize_data(struct pattern_trig_data *data) +{ + mutex_init(&data->lock); + mutex_lock(&data->lock); + + data->is_sane = 0; + data->steps = kzalloc(MAX_NSTEPS*sizeof(struct led_pattern), + GFP_KERNEL); + if (!data->steps) + return -ENOMEM; + + data->curr = NULL; + data->next = NULL; + data->delta_t = 0; + data->nsteps = 0; + data->repeat = -1; + //data->timer = __TIMER_INITIALIZER(NULL, 0); + + mutex_unlock(&data->lock); + return 0; +} + +static void pattern_trig_clear_data(struct pattern_trig_data *data) +{ + data->is_sane = 0; + kfree(data->steps); +} + +/* + * is_sane : pattern checking. + * A pattern satisfying these three conditions is reported as sane : + * - At least two steps + * - At least one step with time >= UPDATE_INTERVAL + * - At least two steps with differing brightnesses + * When @data isn't sane, a sensible brightness + * default is suggested in @brightness + * + * DO NOT call pattern_trig_update on a not-sane pattern, + * you'll be punished with an infinite loop in the kernel. + */ +static int is_sane(struct pattern_trig_data *data, int *brightness) +{ + int i; + char stept_ok = 0; + char stepb_ok = 0; + + *brightness = 0; + if (data->nsteps < 1) + return 0; + + *brightness = data->steps[0].brightness; + if (data->nsteps < 2) + return 0; + + for (i = 0; i < data->nsteps; i++) { + if (data->steps[i].delta_t >= UPDATE_INTERVAL) { + /* FIXME: this is wrong */ + if (stepb_ok) + return 1; + stept_ok = 1; + } + if (data->steps[i].brightness != data->steps[0].brightness) { + if (stept_ok) + return 1; + stepb_ok = 1; + } + } + + return 0; +} + +static void reset_pattern(struct pattern_trig_data *data, + struct led_classdev *led_cdev) +{ + int brightness; + + if (led_cdev->pattern_clear) { + led_cdev->pattern_clear(led_cdev); + } + + del_timer_sync(&data->timer); + + if (led_cdev->pattern_set && led_cdev->pattern_set(led_cdev, data->steps, data->nsteps)) { + return; + } + + if (!is_sane(data, &brightness)) { + led_set_brightness(led_cdev, brightness); + return; + } + + data->curr = data->steps; + data->next = data->steps + 1; + data->delta_t = 0; + data->is_sane = 1; + + data->timer.expires = jiffies; + add_timer(&data->timer); +} + +/* --- Sysfs handling --- */ + +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; + + return scnprintf(buf, PAGE_SIZE, "%d\n", data->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; + long res; + int err; + + err = kstrtol(buf, 10, &res); + if (err) + return err; + + data->repeat = res < 0 ? -1 : res; + reset_pattern(data, led_cdev); + + return count; +} + +DEVICE_ATTR(repeat, S_IRUGO | S_IWUSR, + 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; + + if (!data->steps || !data->nsteps) + return 0; + + for (i = 0; i < data->nsteps; i++) + count += scnprintf(buf + count, PAGE_SIZE - count, + "%d %d" PATTERN_SEPARATOR, + data->steps[i].brightness, + data->steps[i].delta_t); + buf[count - 1] = '\n'; + buf[count] = '\0'; + + return count + 1; +} + +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 = 0; /* Characters read on one conversion */ + int tcr = 0; /* Total characters read */ + int ccount; /* Number of successful conversions */ + + mutex_lock(&data->lock); + data->is_sane = 0; + + for (data->nsteps = 0; data->nsteps < MAX_NSTEPS; data->nsteps++) { + cr = 0; + ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n", + &data->steps[data->nsteps].brightness, + &data->steps[data->nsteps].delta_t, &cr); + + if (!cr) { /* Invalid syntax or end of pattern */ + if (ccount == 2) + data->nsteps++; + mutex_unlock(&data->lock); + reset_pattern(data, led_cdev); + return count; + } + + tcr += cr; + } + + /* Shouldn't reach that */ + WARN(1, "MAX_NSTEP too small. Please report\n"); + mutex_unlock(&data->lock); + return count; +} + +DEVICE_ATTR(pattern, S_IRUGO | S_IWUSR, + pattern_trig_show_pattern, pattern_trig_store_pattern); + +static int pattern_trig_create_sysfs_files(struct device *dev) +{ + int err; + + err = device_create_file(dev, &dev_attr_repeat); + if (err) + return err; + + err = device_create_file(dev, &dev_attr_pattern); + if (err) + device_remove_file(dev, &dev_attr_repeat); + + return err; +} + +static void pattern_trig_remove_sysfs_files(struct device *dev) +{ + device_remove_file(dev, &dev_attr_pattern); + device_remove_file(dev, &dev_attr_repeat); +} + +/* --- Led intensity updating --- */ + +static int compute_brightness(struct pattern_trig_data *data) +{ + if (data->delta_t == 0) + return data->curr->brightness; + + if (data->curr->delta_t == 0) + return data->next->brightness; + + return data->curr->brightness + data->delta_t + * (data->next->brightness - data->curr->brightness) + / data->curr->delta_t; +} + +static void update_to_next_step(struct pattern_trig_data *data) +{ + data->curr = data->next; + if (data->curr == data->steps) + data->repeat--; + + if (data->next == data->steps + data->nsteps - 1) + data->next = data->steps; + else + data->next++; + + data->delta_t = 0; +} + +static void pattern_trig_update(struct timer_list *t) +{ + struct pattern_trig_data *data = from_timer(data, t, timer); + + mutex_lock(&data->lock); + + if (!data->is_sane || !data->repeat) { + mutex_unlock(&data->lock); + return; + } + + if (data->delta_t > data->curr->delta_t) + update_to_next_step(data); + + /* is_sane() checked that there is at least + one step with delta_t >= UPDATE_INTERVAL + so we won't go in an infinite loop */ + while (data->curr->delta_t < UPDATE_INTERVAL) + update_to_next_step(data); + + if (data->next->brightness == data->curr->brightness) { + /* Constant brightness for this step */ + led_set_brightness(data->led_cdev, data->curr->brightness); + mod_timer(&data->timer, jiffies + + msecs_to_jiffies(data->curr->delta_t)); + update_to_next_step(data); + } else { + /* Gradual dimming */ + led_set_brightness(data->led_cdev, compute_brightness(data)); + data->delta_t += UPDATE_INTERVAL; + mod_timer(&data->timer, jiffies + + msecs_to_jiffies(UPDATE_INTERVAL)); + } + + mutex_unlock(&data->lock); +} + +/* --- Trigger activation --- */ + +static void pattern_trig_activate(struct led_classdev *led_cdev) +{ + struct pattern_trig_data *data = NULL; + int err; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return; + + err = pattern_trig_initialize_data(data); + if (err) { + kfree(data); + return; + } + + data->led_cdev = led_cdev; + led_cdev->trigger_data = data; + timer_setup(&data->timer, pattern_trig_update, 0); + pattern_trig_create_sysfs_files(led_cdev->dev); +} + +static void pattern_trig_deactivate(struct led_classdev *led_cdev) +{ + struct pattern_trig_data *data = led_cdev->trigger_data; + + if (data) { + pattern_trig_remove_sysfs_files(led_cdev->dev); + del_timer_sync(&data->timer); + led_set_brightness(led_cdev, LED_OFF); + pattern_trig_clear_data(data); + kfree(data); + led_cdev->trigger_data = NULL; + } +} + +static struct led_trigger pattern_led_trigger = { + .name = "pattern", + .activate = pattern_trig_activate, + .deactivate = pattern_trig_deactivate, +}; + +/* --- Module loading/unloading --- */ + +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 <rteysseyre@gmail.com"); +MODULE_DESCRIPTION("Pattern LED trigger"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/leds.h b/include/linux/leds.h index 2fce962..39908ba 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 */ @@ -91,6 +92,14 @@ 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); + + int (*pattern_clear)(struct led_classdev *led_cdev); + + struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev, + int *len); + struct device *dev; const struct attribute_group **groups; @@ -104,7 +113,7 @@ struct led_classdev { void (*flash_resume)(struct led_classdev *led_cdev); struct work_struct set_brightness_work; - int delayed_set_value; + enum led_brightness delayed_set_value; #ifdef CONFIG_LEDS_TRIGGERS /* Protects the trigger data below */ @@ -471,4 +480,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 */ -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-24 11:41 ` Pavel Machek @ 2018-07-27 5:15 ` Baolin Wang 2018-07-27 8:36 ` Pavel Machek 0 siblings, 1 reply; 38+ messages in thread From: Baolin Wang @ 2018-07-27 5:15 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML Hi Pavel, On 24 July 2018 at 19:41, Pavel Machek <pavel@ucw.cz> wrote: > Hi! > >> >> > >Please keep in mind that this is ABI documentation for the pattern file >> >> > >to be exposed by LED core, and not by the pattern trigger, that, as we >> >> > >agreed, will be implemented later. In this case, I'd go for >> >> > >> >> > Gosh, I got completely distracted by the recent discussion about >> >> > pattern synchronization. >> >> > >> >> > So, to recap, we need to decide if we are taking Baolin's solution >> >> > or we're opting for implementing pattern trigger. >> >> > >> >> > If we choose the latter, then we will also need some software >> >> > pattern engine in the trigger, to be applied as a software pattern >> >> > fallback for the devices without hardware pattern support. >> >> > It will certainly delay the contribution process, provided that Baolin >> >> > would find time for this work at all. >> >> >> >> I'd recommend the latter. Yes, software pattern as a fallback would be >> >> nice, but I have that code already... let me get it back to running >> >> state, and figure out where to add interface for "hardware >> >> acceleration". I'd like to have same interface to userland, whether >> >> pattern can be done by hardware or by software. >> > >> > For the record, I'd like something like this. (Software pattern should >> > work. Hardware pattern... needs more work). >> >> Thanks for showing your thoughts. But I failed to compile your code, >> would you like to send out formal patches (Or only including software >> pattern, I will help to add hardware pattern part and do some testing >> with our driver)? Thanks. > > This should be a bit better. I attempted to compile it with your > driver, but whether it works is an open question. Sorry for late reply. I've compiled and tested this version on my platform, the hardware pattern can work with one small fix as below. if (led_cdev->pattern_set && !led_cdev->pattern_set(led_cdev, data->steps, data->nsteps)) { return; } But I saw there are lots coding style issues and something can be improved in this patch, so will you send out one clean patch (I will help to test the hardware pattern support)? Or I can help to improve this code and try to upstream it again? > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > > diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c > index 9d9b7aa..898f92d 100644 > --- a/drivers/leds/leds-sc27xx-bltc.c > +++ b/drivers/leds/leds-sc27xx-bltc.c > @@ -6,6 +6,7 @@ > #include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > +#include <linux/slab.h> > #include <uapi/linux/uleds.h> > > /* PMIC global control register definition */ > @@ -32,8 +33,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 +128,157 @@ 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) > +{ > + 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 struct led_pattern *sc27xx_led_pattern_get(struct led_classdev *ldev, > + int *len) > +{ > + struct sc27xx_led *leds = to_sc27xx_led(ldev); > + u32 base = sc27xx_led_get_offset(leds); > + struct regmap *regmap = leds->priv->regmap; > + struct led_pattern *pattern; > + int i, err; > + u32 val; > + > + /* > + * Must allocate 4 patterns to show the rise time, high time, fall time > + * and low time. > + */ > + pattern = kcalloc(SC27XX_LEDS_PATTERN_CNT, sizeof(*pattern), > + GFP_KERNEL); > + if (!pattern) > + return ERR_PTR(-ENOMEM); > + > + mutex_lock(&leds->priv->lock); > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); > + if (err) > + goto out; > + > + pattern[0].delta_t = val & SC27XX_CURVE_L_MASK; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); > + if (err) > + goto out; > + > + pattern[1].delta_t = val & SC27XX_CURVE_L_MASK; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE0, &val); > + if (err) > + goto out; > + > + pattern[2].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_CURVE1, &val); > + if (err) > + goto out; > + > + pattern[3].delta_t = (val & SC27XX_CURVE_H_MASK) >> SC27XX_CURVE_SHIFT; > + > + err = regmap_read(regmap, base + SC27XX_LEDS_DUTY, &val); > + if (err) > + goto out; > + > + mutex_unlock(&leds->priv->lock); > + > + val = (val & SC27XX_DUTY_MASK) >> SC27XX_DUTY_SHIFT; > + for (i = 0; i < SC27XX_LEDS_PATTERN_CNT; i++) > + pattern[i].brightness = val; > + > + *len = SC27XX_LEDS_PATTERN_CNT; > + > + return pattern; > + > +out: > + mutex_unlock(&leds->priv->lock); > + kfree(pattern); > + > + return ERR_PTR(err); > +} > + > static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv) > { > int i, err; > @@ -140,6 +297,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_get = sc27xx_led_pattern_get; > + led->ldev.pattern_clear = sc27xx_led_pattern_clear; > > err = devm_led_classdev_register(dev, &led->ldev); > if (err) > diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig > index a2559b4..91ae5b0 100644 > --- a/drivers/leds/trigger/Kconfig > +++ b/drivers/leds/trigger/Kconfig > @@ -125,6 +125,16 @@ config LEDS_TRIGGER_CAMERA > This enables direct flash/torch on/off by the driver, kernel space. > If unsure, say Y. > > +config LEDS_TRIGGER_PATTERN > + tristate "LED Pattern Trigger" > + depends on LEDS_TRIGGERS > + help > + This allows LEDs blinking with an arbitrary pattern. Can be useful > + on embedded systems with no screen to give out a status code to > + a human. > + > + If unsure, say N > + > config LEDS_TRIGGER_PANIC > bool "LED Panic Trigger" > depends on LEDS_TRIGGERS > diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile > index f3cfe19..ba6f3b9 100644 > --- a/drivers/leds/trigger/Makefile > +++ b/drivers/leds/trigger/Makefile > @@ -11,5 +11,6 @@ obj-$(CONFIG_LEDS_TRIGGER_ACTIVITY) += ledtrig-activity.o > obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o > obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o > obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o > +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o > obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o > obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o > diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c > new file mode 100644 > index 0000000..3dab050 > --- /dev/null > +++ b/drivers/leds/trigger/ledtrig-pattern.c > @@ -0,0 +1,395 @@ > +/* > + * Arbitrary pattern trigger > + * > + * Copyright 2015, Epsiline > + * > + * Author : Raphaël Teysseyre <rteysseyre@gmail.com> > + * > + * Idea discussed with Pavel Machek <pavel@ucw.cz> on > + * <linux-leds@vger.kernel.org> (march 2015, thread title > + * [PATCH RFC] leds: Add status code trigger) > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + */ > + > +#include <linux/types.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/timer.h> > +#include "../leds.h" > + > +struct pattern_trig_data { > + struct led_classdev *led_cdev; > + > + struct led_pattern *steps; /* Array describing the pattern */ > + struct mutex lock; > + char is_sane; > + struct led_pattern *curr; > + struct led_pattern *next; > + int delta_t; /* Time in current step */ > + int nsteps; /* Number of steps */ > + int repeat; /* < 0 means repeat indefinitely */ > + struct timer_list timer; > +}; > + > +#define MAX_NSTEPS (PAGE_SIZE/4) > +/* The "pattern" attribute contains at most PAGE_SIZE characters. > + Each line in this attribute is at least 4 characters long > + (a 1-digit number for the led brighntess, a space, > + a 1-digit number for the time, a semi-colon). > + Therefore, there is at most PAGE_SIZE/4 steps. */ > + > +#define UPDATE_INTERVAL 50 > +/* When doing gradual dimming, the led brightness > + will be updated every UPDATE_INTERVAL milliseconds */ > + > +#define PATTERN_SEPARATOR "," > + > +static int pattern_trig_initialize_data(struct pattern_trig_data *data) > +{ > + mutex_init(&data->lock); > + mutex_lock(&data->lock); > + > + data->is_sane = 0; > + data->steps = kzalloc(MAX_NSTEPS*sizeof(struct led_pattern), > + GFP_KERNEL); > + if (!data->steps) > + return -ENOMEM; > + > + data->curr = NULL; > + data->next = NULL; > + data->delta_t = 0; > + data->nsteps = 0; > + data->repeat = -1; > + //data->timer = __TIMER_INITIALIZER(NULL, 0); > + > + mutex_unlock(&data->lock); > + return 0; > +} > + > +static void pattern_trig_clear_data(struct pattern_trig_data *data) > +{ > + data->is_sane = 0; > + kfree(data->steps); > +} > + > +/* > + * is_sane : pattern checking. > + * A pattern satisfying these three conditions is reported as sane : > + * - At least two steps > + * - At least one step with time >= UPDATE_INTERVAL > + * - At least two steps with differing brightnesses > + * When @data isn't sane, a sensible brightness > + * default is suggested in @brightness > + * > + * DO NOT call pattern_trig_update on a not-sane pattern, > + * you'll be punished with an infinite loop in the kernel. > + */ > +static int is_sane(struct pattern_trig_data *data, int *brightness) > +{ > + int i; > + char stept_ok = 0; > + char stepb_ok = 0; > + > + *brightness = 0; > + if (data->nsteps < 1) > + return 0; > + > + *brightness = data->steps[0].brightness; > + if (data->nsteps < 2) > + return 0; > + > + for (i = 0; i < data->nsteps; i++) { > + if (data->steps[i].delta_t >= UPDATE_INTERVAL) { > + /* FIXME: this is wrong */ > + if (stepb_ok) > + return 1; > + stept_ok = 1; > + } > + if (data->steps[i].brightness != data->steps[0].brightness) { > + if (stept_ok) > + return 1; > + stepb_ok = 1; > + } > + } > + > + return 0; > +} > + > +static void reset_pattern(struct pattern_trig_data *data, > + struct led_classdev *led_cdev) > +{ > + int brightness; > + > + if (led_cdev->pattern_clear) { > + led_cdev->pattern_clear(led_cdev); > + } > + > + del_timer_sync(&data->timer); > + > + if (led_cdev->pattern_set && led_cdev->pattern_set(led_cdev, data->steps, data->nsteps)) { > + return; > + } > + > + if (!is_sane(data, &brightness)) { > + led_set_brightness(led_cdev, brightness); > + return; > + } > + > + data->curr = data->steps; > + data->next = data->steps + 1; > + data->delta_t = 0; > + data->is_sane = 1; > + > + data->timer.expires = jiffies; > + add_timer(&data->timer); > +} > + > +/* --- Sysfs handling --- */ > + > +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; > + > + return scnprintf(buf, PAGE_SIZE, "%d\n", data->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; > + long res; > + int err; > + > + err = kstrtol(buf, 10, &res); > + if (err) > + return err; > + > + data->repeat = res < 0 ? -1 : res; > + reset_pattern(data, led_cdev); > + > + return count; > +} > + > +DEVICE_ATTR(repeat, S_IRUGO | S_IWUSR, > + 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; > + > + if (!data->steps || !data->nsteps) > + return 0; > + > + for (i = 0; i < data->nsteps; i++) > + count += scnprintf(buf + count, PAGE_SIZE - count, > + "%d %d" PATTERN_SEPARATOR, > + data->steps[i].brightness, > + data->steps[i].delta_t); > + buf[count - 1] = '\n'; > + buf[count] = '\0'; > + > + return count + 1; > +} > + > +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 = 0; /* Characters read on one conversion */ > + int tcr = 0; /* Total characters read */ > + int ccount; /* Number of successful conversions */ > + > + mutex_lock(&data->lock); > + data->is_sane = 0; > + > + for (data->nsteps = 0; data->nsteps < MAX_NSTEPS; data->nsteps++) { > + cr = 0; > + ccount = sscanf(buf + tcr, "%d %d " PATTERN_SEPARATOR "%n", > + &data->steps[data->nsteps].brightness, > + &data->steps[data->nsteps].delta_t, &cr); > + > + if (!cr) { /* Invalid syntax or end of pattern */ > + if (ccount == 2) > + data->nsteps++; > + mutex_unlock(&data->lock); > + reset_pattern(data, led_cdev); > + return count; > + } > + > + tcr += cr; > + } > + > + /* Shouldn't reach that */ > + WARN(1, "MAX_NSTEP too small. Please report\n"); > + mutex_unlock(&data->lock); > + return count; > +} > + > +DEVICE_ATTR(pattern, S_IRUGO | S_IWUSR, > + pattern_trig_show_pattern, pattern_trig_store_pattern); > + > +static int pattern_trig_create_sysfs_files(struct device *dev) > +{ > + int err; > + > + err = device_create_file(dev, &dev_attr_repeat); > + if (err) > + return err; > + > + err = device_create_file(dev, &dev_attr_pattern); > + if (err) > + device_remove_file(dev, &dev_attr_repeat); > + > + return err; > +} > + > +static void pattern_trig_remove_sysfs_files(struct device *dev) > +{ > + device_remove_file(dev, &dev_attr_pattern); > + device_remove_file(dev, &dev_attr_repeat); > +} > + > +/* --- Led intensity updating --- */ > + > +static int compute_brightness(struct pattern_trig_data *data) > +{ > + if (data->delta_t == 0) > + return data->curr->brightness; > + > + if (data->curr->delta_t == 0) > + return data->next->brightness; > + > + return data->curr->brightness + data->delta_t > + * (data->next->brightness - data->curr->brightness) > + / data->curr->delta_t; > +} > + > +static void update_to_next_step(struct pattern_trig_data *data) > +{ > + data->curr = data->next; > + if (data->curr == data->steps) > + data->repeat--; > + > + if (data->next == data->steps + data->nsteps - 1) > + data->next = data->steps; > + else > + data->next++; > + > + data->delta_t = 0; > +} > + > +static void pattern_trig_update(struct timer_list *t) > +{ > + struct pattern_trig_data *data = from_timer(data, t, timer); > + > + mutex_lock(&data->lock); > + > + if (!data->is_sane || !data->repeat) { > + mutex_unlock(&data->lock); > + return; > + } > + > + if (data->delta_t > data->curr->delta_t) > + update_to_next_step(data); > + > + /* is_sane() checked that there is at least > + one step with delta_t >= UPDATE_INTERVAL > + so we won't go in an infinite loop */ > + while (data->curr->delta_t < UPDATE_INTERVAL) > + update_to_next_step(data); > + > + if (data->next->brightness == data->curr->brightness) { > + /* Constant brightness for this step */ > + led_set_brightness(data->led_cdev, data->curr->brightness); > + mod_timer(&data->timer, jiffies > + + msecs_to_jiffies(data->curr->delta_t)); > + update_to_next_step(data); > + } else { > + /* Gradual dimming */ > + led_set_brightness(data->led_cdev, compute_brightness(data)); > + data->delta_t += UPDATE_INTERVAL; > + mod_timer(&data->timer, jiffies > + + msecs_to_jiffies(UPDATE_INTERVAL)); > + } > + > + mutex_unlock(&data->lock); > +} > + > +/* --- Trigger activation --- */ > + > +static void pattern_trig_activate(struct led_classdev *led_cdev) > +{ > + struct pattern_trig_data *data = NULL; > + int err; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return; > + > + err = pattern_trig_initialize_data(data); > + if (err) { > + kfree(data); > + return; > + } > + > + data->led_cdev = led_cdev; > + led_cdev->trigger_data = data; > + timer_setup(&data->timer, pattern_trig_update, 0); > + pattern_trig_create_sysfs_files(led_cdev->dev); > +} > + > +static void pattern_trig_deactivate(struct led_classdev *led_cdev) > +{ > + struct pattern_trig_data *data = led_cdev->trigger_data; > + > + if (data) { > + pattern_trig_remove_sysfs_files(led_cdev->dev); > + del_timer_sync(&data->timer); > + led_set_brightness(led_cdev, LED_OFF); > + pattern_trig_clear_data(data); > + kfree(data); > + led_cdev->trigger_data = NULL; > + } > +} > + > +static struct led_trigger pattern_led_trigger = { > + .name = "pattern", > + .activate = pattern_trig_activate, > + .deactivate = pattern_trig_deactivate, > +}; > + > +/* --- Module loading/unloading --- */ > + > +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 <rteysseyre@gmail.com"); > +MODULE_DESCRIPTION("Pattern LED trigger"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 2fce962..39908ba 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 > */ > @@ -91,6 +92,14 @@ 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); > + > + int (*pattern_clear)(struct led_classdev *led_cdev); > + > + struct led_pattern *(*pattern_get)(struct led_classdev *led_cdev, > + int *len); > + > struct device *dev; > const struct attribute_group **groups; > > @@ -104,7 +113,7 @@ struct led_classdev { > void (*flash_resume)(struct led_classdev *led_cdev); > > struct work_struct set_brightness_work; > - int delayed_set_value; > + enum led_brightness delayed_set_value; > > #ifdef CONFIG_LEDS_TRIGGERS > /* Protects the trigger data below */ > @@ -471,4 +480,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 */ > > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- Baolin Wang Best Regards ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-27 5:15 ` Baolin Wang @ 2018-07-27 8:36 ` Pavel Machek 2018-07-27 8:41 ` Baolin Wang 0 siblings, 1 reply; 38+ messages in thread From: Pavel Machek @ 2018-07-27 8:36 UTC (permalink / raw) To: Baolin Wang Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 1034 bytes --] Hi! > > This should be a bit better. I attempted to compile it with your > > driver, but whether it works is an open question. > > Sorry for late reply. I've compiled and tested this version on my > platform, the hardware pattern can work with one small fix as below. > > if (led_cdev->pattern_set && !led_cdev->pattern_set(led_cdev, > data->steps, data->nsteps)) { > return; > } > > But I saw there are lots coding style issues and something can be > improved in this patch, so will you send out one clean patch (I will > help to test the hardware pattern support)? Or I can help to improve > this code and try to upstream it again? If you could do the upstreaming, that would be great. I tried to get hardware accelerated LED to work on N900, but that hardware is rather complex, so it would take me some time... Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-27 8:36 ` Pavel Machek @ 2018-07-27 8:41 ` Baolin Wang 0 siblings, 0 replies; 38+ messages in thread From: Baolin Wang @ 2018-07-27 8:41 UTC (permalink / raw) To: Pavel Machek Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML Hi Pavel, On 27 July 2018 at 16:36, Pavel Machek <pavel@ucw.cz> wrote: > Hi! > >> > This should be a bit better. I attempted to compile it with your >> > driver, but whether it works is an open question. >> >> Sorry for late reply. I've compiled and tested this version on my >> platform, the hardware pattern can work with one small fix as below. >> >> if (led_cdev->pattern_set && !led_cdev->pattern_set(led_cdev, >> data->steps, data->nsteps)) { >> return; >> } >> >> But I saw there are lots coding style issues and something can be >> improved in this patch, so will you send out one clean patch (I will >> help to test the hardware pattern support)? Or I can help to improve >> this code and try to upstream it again? > > If you could do the upstreaming, that would be great. OK, I will improve the code and test the software/hardware pattern, and upstream it again with keeping the original authorization. > I tried to get hardware accelerated LED to work on N900, but that > hardware is rather complex, so it would take me some time... -- Baolin Wang Best Regards ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-23 6:59 ` Baolin Wang 2018-07-24 11:41 ` Pavel Machek @ 2018-07-24 11:50 ` Pavel Machek 1 sibling, 0 replies; 38+ messages in thread From: Pavel Machek @ 2018-07-24 11:50 UTC (permalink / raw) To: Baolin Wang Cc: Jacek Anaszewski, David Lechner, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML [-- Attachment #1: Type: text/plain, Size: 672 bytes --] Hi! > Thanks for showing your thoughts. But I failed to compile your code, > would you like to send out formal patches (Or only including software > pattern, I will help to add hardware pattern part and do some testing > with our driver)? Thanks. Random thoughts: I am not sure "struct led_pattern" makes sense. Maybe array of integers would be enough? Should we have structure with whole array and length of the pattern? struct led_pattern { int len; struct led_pattern steps[]; } Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-16 1:00 ` David Lechner 2018-07-16 20:29 ` Jacek Anaszewski @ 2018-07-24 0:18 ` Bjorn Andersson 1 sibling, 0 replies; 38+ messages in thread From: Bjorn Andersson @ 2018-07-24 0:18 UTC (permalink / raw) To: David Lechner Cc: Baolin Wang, Jacek Anaszewski, Pavel Machek, Mark Brown, Linux LED Subsystem, LKML On Sun 15 Jul 18:00 PDT 2018, David Lechner wrote: > On 07/15/2018 07:22 AM, Jacek Anaszewski wrote: > > On 07/15/2018 12:39 AM, Pavel Machek wrote: > > > On Sun 2018-07-15 00:29:25, Pavel Machek wrote: > > > > On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: > > > > > Hi Pavel, > > > > > > > > > > On 07/14/2018 11:20 PM, Pavel Machek wrote: > > > > > > Hi! > > > > > > > > > > > > > > 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. > > > > > > > > > > > > Let me take a step back: we have triggers.. like LED blinking. > > > > > > > > > > > > How is that going to interact with patterns? We probably want the > > > > > > patterns to be ignored in that case...? > > > > > > > > > > > > Which suggest to me that we should treat patterns as a trigger. I > > > > > > believe we do something similar with blinking already. > > > > > > > > > > > > Then it is easy to determine if pattern is active, and pattern > > > > > > vs. trigger issue is solved automatically. > > > > > > > > > > I'm all for it. I proposed this approach during the previous > > > > > discussions related to possible pattern interface implementations, > > > > > but you seemed not to be so enthusiastic in [0]. > > > > > > > > > > [0] https://lkml.org/lkml/2017/4/7/350 > > > > > > > > Hmm. Reading my own email now, I can't decipher it. > > > > > > > > I believe I meant "changing patterns from kernel in response to events > > > > is probably overkill"... or something like that. > > > > > > Anyway -- to clean up the confusion -- I'd like to see > > > > > > echo pattern > trigger > > > echo "1 2 3 4 5 6 7 8" > somewhere > > > > s/somewhere/pattern/ > > > > pattern trigger should create "pattern" file similarly how ledtrig-timer > > creates delay_{on|off} files. > > > > I don't think this is the best way. For example, if you want more than one > LED to have the same pattern, then the patterns will not be synchronized > between the LEDs. The same things happens now with many of the existing > triggers. For example, if I have two LEDs side-by-side using the heartbeat > trigger, they may blink at the same time or they may not, which is not > very nice. I think we can make something better. > > Perhaps a way to do this would be to use configfs to create a pattern > trigger that can be shared by multiple LEDs. Like this: > > mkdir /sys/kernel/config/leds/triggers/my-nice-pattern > echo "1 2 3 4" > /sys/kernel/config/leds/triggers/my-nice-pattern/pattern > echo my-nice-pattern > /sys/class/leds/led0/trigger > echo my-nice-pattern > /sys/class/leds/led1/trigger > In the case where you describe this as two different LEDs (to Linux) and you rely on the two enable-calls to happen fairly quickly I think you can just as well specify the same pattern in two independent triggers. This also helps by not providing the illusion of there being any synchronization between them. Regards, Bjorn ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] leds: core: Introduce generic pattern interface 2018-07-15 12:22 ` Jacek Anaszewski 2018-07-16 1:00 ` David Lechner @ 2018-07-16 11:08 ` Baolin Wang 1 sibling, 0 replies; 38+ messages in thread From: Baolin Wang @ 2018-07-16 11:08 UTC (permalink / raw) To: Jacek Anaszewski Cc: Pavel Machek, Bjorn Andersson, Mark Brown, Linux LED Subsystem, LKML On 15 July 2018 at 20:22, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote: > On 07/15/2018 12:39 AM, Pavel Machek wrote: >> >> On Sun 2018-07-15 00:29:25, Pavel Machek wrote: >>> >>> On Sun 2018-07-15 00:02:57, Jacek Anaszewski wrote: >>>> >>>> Hi Pavel, >>>> >>>> On 07/14/2018 11:20 PM, Pavel Machek wrote: >>>>> >>>>> Hi! >>>>> >>>>>>> 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. >>>>> >>>>> >>>>> Let me take a step back: we have triggers.. like LED blinking. >>>>> >>>>> How is that going to interact with patterns? We probably want the >>>>> patterns to be ignored in that case...? >>>>> >>>>> Which suggest to me that we should treat patterns as a trigger. I >>>>> believe we do something similar with blinking already. >>>>> >>>>> Then it is easy to determine if pattern is active, and pattern >>>>> vs. trigger issue is solved automatically. >>>> >>>> >>>> I'm all for it. I proposed this approach during the previous >>>> discussions related to possible pattern interface implementations, >>>> but you seemed not to be so enthusiastic in [0]. >>>> >>>> [0] https://lkml.org/lkml/2017/4/7/350 >>> >>> >>> Hmm. Reading my own email now, I can't decipher it. >>> >>> I believe I meant "changing patterns from kernel in response to events >>> is probably overkill"... or something like that. >> >> >> Anyway -- to clean up the confusion -- I'd like to see >> >> echo pattern > trigger >> echo "1 2 3 4 5 6 7 8" > somewhere > > > s/somewhere/pattern/ > > pattern trigger should create "pattern" file similarly how ledtrig-timer > creates delay_{on|off} files. Yes. Anyway, I will submit V5 patchset with addressing previous comments, but did not include pattern trigger issue. -- Baolin Wang Best Regards ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2018-07-27 8:41 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-29 5:03 [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang 2018-06-29 5:03 ` [PATCH v3 2/2] leds: sc27xx: Add pattern_set/get/clear interfaces for LED controller Baolin Wang 2018-07-11 11:02 ` [PATCH v3 1/2] leds: core: Introduce generic pattern interface Baolin Wang 2018-07-11 21:10 ` Jacek Anaszewski 2018-07-12 12:24 ` Baolin Wang 2018-07-12 21:41 ` Jacek Anaszewski 2018-07-13 1:58 ` Baolin Wang 2018-07-14 21:20 ` Pavel Machek 2018-07-14 22:02 ` Jacek Anaszewski 2018-07-14 22:29 ` Pavel Machek 2018-07-14 22:39 ` Pavel Machek 2018-07-15 12:22 ` Jacek Anaszewski 2018-07-16 1:00 ` David Lechner 2018-07-16 20:29 ` Jacek Anaszewski 2018-07-16 21:56 ` Pavel Machek 2018-07-17 20:26 ` Jacek Anaszewski 2018-07-17 21:07 ` Pavel Machek 2018-07-24 0:35 ` Bjorn Andersson 2018-07-18 7:56 ` Pavel Machek 2018-07-18 11:32 ` Baolin Wang 2018-07-18 12:08 ` Pavel Machek 2018-07-18 17:00 ` David Lechner 2018-07-20 19:11 ` Jacek Anaszewski 2018-07-24 0:55 ` Bjorn Andersson 2018-07-18 18:54 ` Jacek Anaszewski 2018-07-18 19:22 ` Jacek Anaszewski 2018-07-18 22:13 ` David Lechner 2018-07-18 22:17 ` Pavel Machek 2018-07-19 20:20 ` Pavel Machek 2018-07-20 18:08 ` Jacek Anaszewski 2018-07-23 6:59 ` Baolin Wang 2018-07-24 11:41 ` Pavel Machek 2018-07-27 5:15 ` Baolin Wang 2018-07-27 8:36 ` Pavel Machek 2018-07-27 8:41 ` Baolin Wang 2018-07-24 11:50 ` Pavel Machek 2018-07-24 0:18 ` Bjorn Andersson 2018-07-16 11:08 ` Baolin Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).