| Message ID | 67ebebf02edd6d8ee42a13b139733e9cc680ea86.1536631975.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v12,1/2] leds: core: Introduce LED pattern trigger | expand |
Hi Jacek and Pavel, On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: > This patch adds one new led trigger that LED device can configure > the software or hardware pattern and trigger it. > > Consumers can write 'pattern' file to enable the software pattern > which alters the brightness for the specified duration with one > software timer. > > Moreover consumers can write 'hw_pattern' file to enable the hardware > pattern for some LED controllers which can autonomously control > brightness over time, according to some preprogrammed hardware > patterns. > > Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > Changes from v11: > - Change -1 means repeat indefinitely. > > Changes from v10: > - Change 'int' to 'u32' for delta_t field. > > Changes from v9: > - None. > > Changes from v8: > - None. > > Changes from v7: > - Move the SC27XX hardware patterns description into its own ABI file. > > Changes from v6: > - Improve commit message. > - Optimize the description of the hw_pattern file. > - Simplify some logics. > > Changes from v5: > - Add one 'hw_pattern' file for hardware patterns. > > Changes from v4: > - Change the repeat file to return the originally written number. > - Improve comments. > - Fix some build warnings. > > Changes from v3: > - Reset pattern number to 0 if user provides incorrect pattern string. > - Support one pattern. > > Changes from v2: > - Remove hardware_pattern boolen. > - Chnage the pattern string format. > > Changes from v1: > - Use ATTRIBUTE_GROUPS() to define attributes. > - Introduce hardware_pattern flag to determine if software pattern > or hardware pattern. > - Re-implement pattern_trig_store_pattern() function. > - Remove pattern_get() interface. > - Improve comments. > - Other small optimization. > --- Do you have any comments for the v12 patch set? Thanks. -- Baolin Wang Best Regards
Hi Baolin, On 09/21/2018 05:31 AM, Baolin Wang wrote: > Hi Jacek and Pavel, > > On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: >> This patch adds one new led trigger that LED device can configure >> the software or hardware pattern and trigger it. >> >> Consumers can write 'pattern' file to enable the software pattern >> which alters the brightness for the specified duration with one >> software timer. >> >> Moreover consumers can write 'hw_pattern' file to enable the hardware >> pattern for some LED controllers which can autonomously control >> brightness over time, according to some preprogrammed hardware >> patterns. >> >> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> --- >> Changes from v11: >> - Change -1 means repeat indefinitely. >> >> Changes from v10: >> - Change 'int' to 'u32' for delta_t field. >> >> Changes from v9: >> - None. >> >> Changes from v8: >> - None. >> >> Changes from v7: >> - Move the SC27XX hardware patterns description into its own ABI file. >> >> Changes from v6: >> - Improve commit message. >> - Optimize the description of the hw_pattern file. >> - Simplify some logics. >> >> Changes from v5: >> - Add one 'hw_pattern' file for hardware patterns. >> >> Changes from v4: >> - Change the repeat file to return the originally written number. >> - Improve comments. >> - Fix some build warnings. >> >> Changes from v3: >> - Reset pattern number to 0 if user provides incorrect pattern string. >> - Support one pattern. >> >> Changes from v2: >> - Remove hardware_pattern boolen. >> - Chnage the pattern string format. >> >> Changes from v1: >> - Use ATTRIBUTE_GROUPS() to define attributes. >> - Introduce hardware_pattern flag to determine if software pattern >> or hardware pattern. >> - Re-implement pattern_trig_store_pattern() function. >> - Remove pattern_get() interface. >> - Improve comments. >> - Other small optimization. >> --- > > Do you have any comments for the v12 patch set? Thanks. We will probably have to remove hw_pattern from ledtrig-pattern since we are unable to come up with generic interface for it. Unless thread [0] will end up with some brilliant ideas. So far we're waiting for Pavel's reply. [0] https://lkml.org/lkml/2018/9/13/1216 -- Best regards, Jacek Anaszewski
On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: > Hi Baolin, > > On 09/21/2018 05:31 AM, Baolin Wang wrote: > > Hi Jacek and Pavel, > > > > On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: > >> This patch adds one new led trigger that LED device can configure > >> the software or hardware pattern and trigger it. > >> > >> Consumers can write 'pattern' file to enable the software pattern > >> which alters the brightness for the specified duration with one > >> software timer. > >> > >> Moreover consumers can write 'hw_pattern' file to enable the hardware > >> pattern for some LED controllers which can autonomously control > >> brightness over time, according to some preprogrammed hardware > >> patterns. > >> > >> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> > >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > >> --- > >> Changes from v11: > >> - Change -1 means repeat indefinitely. > >> > >> Changes from v10: > >> - Change 'int' to 'u32' for delta_t field. > >> > >> Changes from v9: > >> - None. > >> > >> Changes from v8: > >> - None. > >> > >> Changes from v7: > >> - Move the SC27XX hardware patterns description into its own ABI file. > >> > >> Changes from v6: > >> - Improve commit message. > >> - Optimize the description of the hw_pattern file. > >> - Simplify some logics. > >> > >> Changes from v5: > >> - Add one 'hw_pattern' file for hardware patterns. > >> > >> Changes from v4: > >> - Change the repeat file to return the originally written number. > >> - Improve comments. > >> - Fix some build warnings. > >> > >> Changes from v3: > >> - Reset pattern number to 0 if user provides incorrect pattern string. > >> - Support one pattern. > >> > >> Changes from v2: > >> - Remove hardware_pattern boolen. > >> - Chnage the pattern string format. > >> > >> Changes from v1: > >> - Use ATTRIBUTE_GROUPS() to define attributes. > >> - Introduce hardware_pattern flag to determine if software pattern > >> or hardware pattern. > >> - Re-implement pattern_trig_store_pattern() function. > >> - Remove pattern_get() interface. > >> - Improve comments. > >> - Other small optimization. > >> --- > > > > Do you have any comments for the v12 patch set? Thanks. > > We will probably have to remove hw_pattern from ledtrig-pattern > since we are unable to come up with generic interface for it. > Unless thread [0] will end up with some brilliant ideas. So far > we're waiting for Pavel's reply. I believe same semantics as pattern works well for hw_pattern, and suggested code changes to make it more robust. I don't really understand what issues you see there. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: > Hi Baolin, > > On 09/21/2018 05:31 AM, Baolin Wang wrote: > > Hi Jacek and Pavel, > > > > On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: > >> This patch adds one new led trigger that LED device can configure > >> the software or hardware pattern and trigger it. > >> > >> Consumers can write 'pattern' file to enable the software pattern > >> which alters the brightness for the specified duration with one > >> software timer. > >> > >> Moreover consumers can write 'hw_pattern' file to enable the hardware > >> pattern for some LED controllers which can autonomously control > >> brightness over time, according to some preprogrammed hardware > >> patterns. > >> > >> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> > >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > Do you have any comments for the v12 patch set? Thanks. > > We will probably have to remove hw_pattern from ledtrig-pattern > since we are unable to come up with generic interface for it. > Unless thread [0] will end up with some brilliant ideas. So far > we're waiting for Pavel's reply. > > [0] https://lkml.org/lkml/2018/9/13/1216 Patch 1/2 is not controversial; I believe we can apply it now. 2/2: Non-linear or not, can we just ignore that for now? We don't have well-defined meaning of what brightness 128 means, so... I believe we can do that reasonably safely. I don't exactly understand what you are saying with the four tuples. Yes, sc27xx currently has a limit where it can do single rise, hold, lower, hold at zero. That is described by pattern: "0 rise_duration brightness high_duration brightness fall_duration 0 low_duration" We should add this to sc27xx handler: > I'd actually like to see this at begining of function: > if (pattern[0].brightness != 0) > return -EINVAL; > if (pattern[2].brightness != 0) > return -EINVAL; > if (pattern[3].brightness != 0) > return -EINVAL; > if (pattern[1].brightness != pattern[2].brightness) > return -EINVAL; > > ..so if user writes something unexpected, he gets the error back. Then it is compatible -- or should be. Pattern trigger should do the same -- rise, hold, lower, hold at zero. I don't really see the problem. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: > Hi Baolin, > > On 09/21/2018 05:31 AM, Baolin Wang wrote: > > Hi Jacek and Pavel, > > > > On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: > >> This patch adds one new led trigger that LED device can configure > >> the software or hardware pattern and trigger it. > >> > >> Consumers can write 'pattern' file to enable the software pattern > >> which alters the brightness for the specified duration with one > >> software timer. > >> > >> Moreover consumers can write 'hw_pattern' file to enable the hardware > >> pattern for some LED controllers which can autonomously control > >> brightness over time, according to some preprogrammed hardware > >> patterns. > >> > >> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> > >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > Do you have any comments for the v12 patch set? Thanks. > > We will probably have to remove hw_pattern from ledtrig-pattern > since we are unable to come up with generic interface for it. > Unless thread [0] will end up with some brilliant ideas. So far > we're waiting for Pavel's reply. > > [0] https://lkml.org/lkml/2018/9/13/1216 To paint a picture: brightness rise hold lower hold down ^ XXXXXXXXXXXXXXX | X XX | X XX | X XXXXXXXXXXXXXXXXXXXXXXXXXX +-------------------------------------------------------> time This is what Baolin's hardware can do, right? This is also what pattern trigger can do, right? So all we need to do is match the two interfaces, so that hw_pattern returns -EINVAL on patterns hardware can not actually do. I believe I described code to do that in [0] above. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 09/21/2018 11:17 PM, Pavel Machek wrote: > On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: >> Hi Baolin, >> >> On 09/21/2018 05:31 AM, Baolin Wang wrote: >>> Hi Jacek and Pavel, >>> >>> On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: >>>> This patch adds one new led trigger that LED device can configure >>>> the software or hardware pattern and trigger it. >>>> >>>> Consumers can write 'pattern' file to enable the software pattern >>>> which alters the brightness for the specified duration with one >>>> software timer. >>>> >>>> Moreover consumers can write 'hw_pattern' file to enable the hardware >>>> pattern for some LED controllers which can autonomously control >>>> brightness over time, according to some preprogrammed hardware >>>> patterns. >>>> >>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> >>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > >>> Do you have any comments for the v12 patch set? Thanks. >> >> We will probably have to remove hw_pattern from ledtrig-pattern >> since we are unable to come up with generic interface for it. >> Unless thread [0] will end up with some brilliant ideas. So far >> we're waiting for Pavel's reply. >> >> [0] https://lkml.org/lkml/2018/9/13/1216 > > To paint a picture: > > brightness > > rise hold lower hold down > ^ XXXXXXXXXXXXXXX > | X XX > | X XX > | X XXXXXXXXXXXXXXXXXXXXXXXXXX > +-------------------------------------------------------> time > > This is what Baolin's hardware can do, right? > > This is also what pattern trigger can do, right? > > So all we need to do is match the two interfaces, so that hw_pattern > returns -EINVAL on patterns hardware can not actually do. > > I believe I described code to do that in [0] above. You said that we should get the same effect by writing the same series of tuples to either pattern or hw_pattern file. Below command consists of four tuples (marked with brackets to highlight), and it will activate breathing mode in Baolin's hw_pattern: "[0 rise_duration] [brightness high_duration] [brightness fall_duration] [0 low_duration]" Now, I can't see how these four tuples could force the software fallback to produce breathing effect you depicted. -- Best regards, Jacek Anaszewski
On Sat 2018-09-22 00:11:29, Jacek Anaszewski wrote: > On 09/21/2018 11:17 PM, Pavel Machek wrote: > > On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: > >> Hi Baolin, > >> > >> On 09/21/2018 05:31 AM, Baolin Wang wrote: > >>> Hi Jacek and Pavel, > >>> > >>> On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: > >>>> This patch adds one new led trigger that LED device can configure > >>>> the software or hardware pattern and trigger it. > >>>> > >>>> Consumers can write 'pattern' file to enable the software pattern > >>>> which alters the brightness for the specified duration with one > >>>> software timer. > >>>> > >>>> Moreover consumers can write 'hw_pattern' file to enable the hardware > >>>> pattern for some LED controllers which can autonomously control > >>>> brightness over time, according to some preprogrammed hardware > >>>> patterns. > >>>> > >>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> > >>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > >>> Do you have any comments for the v12 patch set? Thanks. > >> > >> We will probably have to remove hw_pattern from ledtrig-pattern > >> since we are unable to come up with generic interface for it. > >> Unless thread [0] will end up with some brilliant ideas. So far > >> we're waiting for Pavel's reply. > >> > >> [0] https://lkml.org/lkml/2018/9/13/1216 > > > > To paint a picture: > > > > brightness > > > > rise hold lower hold down > > ^ XXXXXXXXXXXXXXX > > | X XX > > | X XX > > | X XXXXXXXXXXXXXXXXXXXXXXXXXX > > +-------------------------------------------------------> time > > > > This is what Baolin's hardware can do, right? > > > > This is also what pattern trigger can do, right? > > > > So all we need to do is match the two interfaces, so that hw_pattern > > returns -EINVAL on patterns hardware can not actually do. > > > > I believe I described code to do that in [0] above. > > You said that we should get the same effect by writing the > same series of tuples to either pattern or hw_pattern file. > > Below command consists of four tuples (marked with brackets > to highlight), and it will activate breathing mode in Baolin's > hw_pattern: > > "[0 rise_duration] [brightness high_duration] [brightness fall_duration] > [0 low_duration]" > > Now, I can't see how these four tuples could force the software > fallback to produce breathing effect you depicted. I really should get some sleep now. But my intention was that software fallback produces just that with those four tuples. (If it does not, we can fix the software fallback to do just that). Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi, On 22 September 2018 at 06:18, Pavel Machek <pavel@ucw.cz> wrote: > On Sat 2018-09-22 00:11:29, Jacek Anaszewski wrote: >> On 09/21/2018 11:17 PM, Pavel Machek wrote: >> > On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: >> >> Hi Baolin, >> >> >> >> On 09/21/2018 05:31 AM, Baolin Wang wrote: >> >>> Hi Jacek and Pavel, >> >>> >> >>> On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: >> >>>> This patch adds one new led trigger that LED device can configure >> >>>> the software or hardware pattern and trigger it. >> >>>> >> >>>> Consumers can write 'pattern' file to enable the software pattern >> >>>> which alters the brightness for the specified duration with one >> >>>> software timer. >> >>>> >> >>>> Moreover consumers can write 'hw_pattern' file to enable the hardware >> >>>> pattern for some LED controllers which can autonomously control >> >>>> brightness over time, according to some preprogrammed hardware >> >>>> patterns. >> >>>> >> >>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> >> >>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> > >> >>> Do you have any comments for the v12 patch set? Thanks. >> >> >> >> We will probably have to remove hw_pattern from ledtrig-pattern >> >> since we are unable to come up with generic interface for it. >> >> Unless thread [0] will end up with some brilliant ideas. So far >> >> we're waiting for Pavel's reply. >> >> >> >> [0] https://lkml.org/lkml/2018/9/13/1216 >> > >> > To paint a picture: >> > >> > brightness >> > >> > rise hold lower hold down >> > ^ XXXXXXXXXXXXXXX >> > | X XX >> > | X XX >> > | X XXXXXXXXXXXXXXXXXXXXXXXXXX >> > +-------------------------------------------------------> time >> > >> > This is what Baolin's hardware can do, right? >> > >> > This is also what pattern trigger can do, right? >> > >> > So all we need to do is match the two interfaces, so that hw_pattern >> > returns -EINVAL on patterns hardware can not actually do. >> > >> > I believe I described code to do that in [0] above. >> >> You said that we should get the same effect by writing the >> same series of tuples to either pattern or hw_pattern file. >> >> Below command consists of four tuples (marked with brackets >> to highlight), and it will activate breathing mode in Baolin's >> hw_pattern: >> >> "[0 rise_duration] [brightness high_duration] [brightness fall_duration] >> [0 low_duration]" >> >> Now, I can't see how these four tuples could force the software >> fallback to produce breathing effect you depicted. > > I really should get some sleep now. But my intention was that software > fallback produces just that with those four tuples. (If it does not, > we can fix the software fallback to do just that). I agree with Jacek. For our SC27XX led, we just need set 4 components (low state, rise stage, high stage and fall stage) for one hardware pattern to enable the breathing mode, but it is hard to use software pattern to simulate the hardware breathing mode if failed to set the hardware pattern, especially for the rising time and falling time which are hard for software pattern to simulate. -- Baolin Wang Best Regards
On Sat 2018-09-22 00:18:13, Pavel Machek wrote: > On Sat 2018-09-22 00:11:29, Jacek Anaszewski wrote: > > On 09/21/2018 11:17 PM, Pavel Machek wrote: > > > On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: > > >> Hi Baolin, > > >> > > >> On 09/21/2018 05:31 AM, Baolin Wang wrote: > > >>> Hi Jacek and Pavel, > > >>> > > >>> On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: > > >>>> This patch adds one new led trigger that LED device can configure > > >>>> the software or hardware pattern and trigger it. > > >>>> > > >>>> Consumers can write 'pattern' file to enable the software pattern > > >>>> which alters the brightness for the specified duration with one > > >>>> software timer. > > >>>> > > >>>> Moreover consumers can write 'hw_pattern' file to enable the hardware > > >>>> pattern for some LED controllers which can autonomously control > > >>>> brightness over time, according to some preprogrammed hardware > > >>>> patterns. > > >>>> > > >>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> > > >>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > > > > >>> Do you have any comments for the v12 patch set? Thanks. > > >> > > >> We will probably have to remove hw_pattern from ledtrig-pattern > > >> since we are unable to come up with generic interface for it. > > >> Unless thread [0] will end up with some brilliant ideas. So far > > >> we're waiting for Pavel's reply. > > >> > > >> [0] https://lkml.org/lkml/2018/9/13/1216 > > > > > > To paint a picture: > > > > > > brightness > > > > > > rise hold lower hold down > > > ^ XXXXXXXXXXXXXXX > > > | X XX > > > | X XX > > > | X XXXXXXXXXXXXXXXXXXXXXXXXXX > > > +-------------------------------------------------------> time > > > > > > This is what Baolin's hardware can do, right? > > > > > > This is also what pattern trigger can do, right? > > > > > > So all we need to do is match the two interfaces, so that hw_pattern > > > returns -EINVAL on patterns hardware can not actually do. > > > > > > I believe I described code to do that in [0] above. > > > > You said that we should get the same effect by writing the > > same series of tuples to either pattern or hw_pattern file. > > > > Below command consists of four tuples (marked with brackets > > to highlight), and it will activate breathing mode in Baolin's > > hw_pattern: > > > > "[0 rise_duration] [brightness high_duration] [brightness fall_duration] > > [0 low_duration]" > > > > Now, I can't see how these four tuples could force the software > > fallback to produce breathing effect you depicted. > > I really should get some sleep now. But my intention was that software > fallback produces just that with those four tuples. (If it does not, > we can fix the software fallback to do just that). And you are right, v12 1/2 seems to do the wrong thing. My "brilliant idea" is to something closer to the original version I posted here. I'm attaching it for reference. I'm also attaching the original documentation. It was clearly designed to do smooth transitions, too. (But pattern is written in slightly different way there, AFAICT). Clearly, having same semantics for pattern and hw_pattern is possible. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html /* * 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"); LED Pattern Trigger =================== This is a LED trigger allowing arbitrary pattern execution. It can do gradual dimming. This trigger can be configured to repeat the pattern a number of times or indefinitely. This is intended as a way of communication for embedded systems with no screen. The trigger can be activated from user space on LED class devices as shown below: echo pattern > trigger This adds the following sysfs attributes to the LED: pattern - specifies the pattern. See syntax below. repeat - number of times the pattern must be repeated. writing -1 to this file will make the pattern repeat indefinitely. The pattern will be restarted each time a new value is written to the pattern or repeat attribute. When dimming, the LED brightness is set every 50 ms. pattern syntax: The pattern is specified in the pattern attribute with an array of comma- separated "brightness/length in miliseconds" values. The two components of each value are to be separated by a space. For example, assuming the driven LED supports intensity value from 0 to 255: echo 0 1000, 255 2000 > pattern Will make the LED go gradually from zero-intensity to max (255) intensity in 1000 milliseconds, then back to zero intensity in 2000 milliseconds: LED brightness ^ 255-| / \ / \ / | / \ / \ / | / \ / \ / | / \ / \ / 0-| / \/ \/ +---0----1----2----3----4----5----6------------> time (s) To make the LED go instantly from one brigntess value to another, use zero-time lengths. For example: echo 0 1000, 0 0, 255 2000, 255 0 > pattern Will make the LED stay off for one second, then stay at max brightness for two seconds: LED brightness ^ 255-| +---------+ +---------+ | | | | | | | | | | | | | | | 0-| -----+ +----+ +---- +---0----1----2----3----4----5----6------------> time (s) Patterns with less than two values, no value with time length > 50 milliseconds, or no two values with differing brightnesses will result in the LED being set at the brightness of the first value, or zero if the pattern contains no value. Patterns with invalid syntax are silently swallowed and will result in undefined behavior of the LED brightness. Because sysfs is used to define the pattern, patterns that need more than PAGE_SIZE characters to describe aren't supported. PAGE_SIZE is system dependent. # 4 step "charging" pattern echo "0 0, 0 1000, 50 500, 50 1000, 100 500, 100 1000, 150 500, 150 1000, 200 500, 200 1000, 0 100" > pattern
On 09/22/2018 09:44 PM, Pavel Machek wrote: > On Sat 2018-09-22 00:18:13, Pavel Machek wrote: >> On Sat 2018-09-22 00:11:29, Jacek Anaszewski wrote: >>> On 09/21/2018 11:17 PM, Pavel Machek wrote: >>>> On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: >>>>> Hi Baolin, >>>>> >>>>> On 09/21/2018 05:31 AM, Baolin Wang wrote: >>>>>> Hi Jacek and Pavel, >>>>>> >>>>>> On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: >>>>>>> This patch adds one new led trigger that LED device can configure >>>>>>> the software or hardware pattern and trigger it. >>>>>>> >>>>>>> Consumers can write 'pattern' file to enable the software pattern >>>>>>> which alters the brightness for the specified duration with one >>>>>>> software timer. >>>>>>> >>>>>>> Moreover consumers can write 'hw_pattern' file to enable the hardware >>>>>>> pattern for some LED controllers which can autonomously control >>>>>>> brightness over time, according to some preprogrammed hardware >>>>>>> patterns. >>>>>>> >>>>>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> >>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>>> >>>>>> Do you have any comments for the v12 patch set? Thanks. >>>>> >>>>> We will probably have to remove hw_pattern from ledtrig-pattern >>>>> since we are unable to come up with generic interface for it. >>>>> Unless thread [0] will end up with some brilliant ideas. So far >>>>> we're waiting for Pavel's reply. >>>>> >>>>> [0] https://lkml.org/lkml/2018/9/13/1216 >>>> >>>> To paint a picture: >>>> >>>> brightness >>>> >>>> rise hold lower hold down >>>> ^ XXXXXXXXXXXXXXX >>>> | X XX >>>> | X XX >>>> | X XXXXXXXXXXXXXXXXXXXXXXXXXX >>>> +-------------------------------------------------------> time >>>> >>>> This is what Baolin's hardware can do, right? >>>> >>>> This is also what pattern trigger can do, right? >>>> >>>> So all we need to do is match the two interfaces, so that hw_pattern >>>> returns -EINVAL on patterns hardware can not actually do. >>>> >>>> I believe I described code to do that in [0] above. >>> >>> You said that we should get the same effect by writing the >>> same series of tuples to either pattern or hw_pattern file. >>> >>> Below command consists of four tuples (marked with brackets >>> to highlight), and it will activate breathing mode in Baolin's >>> hw_pattern: >>> >>> "[0 rise_duration] [brightness high_duration] [brightness fall_duration] >>> [0 low_duration]" >>> >>> Now, I can't see how these four tuples could force the software >>> fallback to produce breathing effect you depicted. >> >> I really should get some sleep now. But my intention was that software >> fallback produces just that with those four tuples. (If it does not, >> we can fix the software fallback to do just that). > > And you are right, v12 1/2 seems to do the wrong thing. > > My "brilliant idea" is to something closer to the original version I > posted here. I'm attaching it for reference. > > I'm also attaching the original documentation. It was clearly designed > to do smooth transitions, too. (But pattern is written in slightly > different way there, AFAICT). > > Clearly, having same semantics for pattern and hw_pattern is possible. Thank you for the attachment. The documentation part makes everything clear. Comparing the patch from the attachment and the Baolin's patch there is one vital part missing, from the original pattern_trig_update(): if (data->next->brightness == data->curr->brightness) { [...] } 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)); } And the compute_brightness() implementation: 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; } With the above the linear gradual dimming is indeed feasible. And for non-linear dimming like breathing mode the hw_pattern will do. There is also vital discrepancy between the documentation and the proposed ledtrig-pattern implementation. The doc says: "Duration of 0 means brightness should immediately change to new value" This syntax seems to be not supported in the Baolin's patch. With all the above covered we will be almost there. Now, only the issues raised by Bjorn need a clarification: On 09/08/2018 07:02 AM, Bjorn Andersson wrote: [...] > The controls for my hardware is: > * a list of brightness values > * the rate of the pattern The two can be described using [brightness delta_t] tuples. > * a flag to indicate that the pattern should be played from start > to end, end to start or start to end to start As above, but the tuples would have to be suitably arranged. We won't need any specific flag to indicate how the pattern should be played, but instead explicitly give the tuples in the required order. For the start-end-start case the sequence of tuples will need to be tripled, but the middle part should be put in the reverse order. > * a boolean indicating if the pattern should be played once or repeated > indefinitely. We will have "repeat" file for that. Baolin, would you mind adding the support for gradual dimming to your ledtrig-timer.c implementation? -- Best regards, Jacek Anaszewski
On 23 September 2018 at 20:25, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote: > On 09/22/2018 09:44 PM, Pavel Machek wrote: >> On Sat 2018-09-22 00:18:13, Pavel Machek wrote: >>> On Sat 2018-09-22 00:11:29, Jacek Anaszewski wrote: >>>> On 09/21/2018 11:17 PM, Pavel Machek wrote: >>>>> On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: >>>>>> Hi Baolin, >>>>>> >>>>>> On 09/21/2018 05:31 AM, Baolin Wang wrote: >>>>>>> Hi Jacek and Pavel, >>>>>>> >>>>>>> On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: >>>>>>>> This patch adds one new led trigger that LED device can configure >>>>>>>> the software or hardware pattern and trigger it. >>>>>>>> >>>>>>>> Consumers can write 'pattern' file to enable the software pattern >>>>>>>> which alters the brightness for the specified duration with one >>>>>>>> software timer. >>>>>>>> >>>>>>>> Moreover consumers can write 'hw_pattern' file to enable the hardware >>>>>>>> pattern for some LED controllers which can autonomously control >>>>>>>> brightness over time, according to some preprogrammed hardware >>>>>>>> patterns. >>>>>>>> >>>>>>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> >>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>>>> >>>>>>> Do you have any comments for the v12 patch set? Thanks. >>>>>> >>>>>> We will probably have to remove hw_pattern from ledtrig-pattern >>>>>> since we are unable to come up with generic interface for it. >>>>>> Unless thread [0] will end up with some brilliant ideas. So far >>>>>> we're waiting for Pavel's reply. >>>>>> >>>>>> [0] https://lkml.org/lkml/2018/9/13/1216 >>>>> >>>>> To paint a picture: >>>>> >>>>> brightness >>>>> >>>>> rise hold lower hold down >>>>> ^ XXXXXXXXXXXXXXX >>>>> | X XX >>>>> | X XX >>>>> | X XXXXXXXXXXXXXXXXXXXXXXXXXX >>>>> +-------------------------------------------------------> time >>>>> >>>>> This is what Baolin's hardware can do, right? >>>>> >>>>> This is also what pattern trigger can do, right? >>>>> >>>>> So all we need to do is match the two interfaces, so that hw_pattern >>>>> returns -EINVAL on patterns hardware can not actually do. >>>>> >>>>> I believe I described code to do that in [0] above. >>>> >>>> You said that we should get the same effect by writing the >>>> same series of tuples to either pattern or hw_pattern file. >>>> >>>> Below command consists of four tuples (marked with brackets >>>> to highlight), and it will activate breathing mode in Baolin's >>>> hw_pattern: >>>> >>>> "[0 rise_duration] [brightness high_duration] [brightness fall_duration] >>>> [0 low_duration]" >>>> >>>> Now, I can't see how these four tuples could force the software >>>> fallback to produce breathing effect you depicted. >>> >>> I really should get some sleep now. But my intention was that software >>> fallback produces just that with those four tuples. (If it does not, >>> we can fix the software fallback to do just that). >> >> And you are right, v12 1/2 seems to do the wrong thing. >> >> My "brilliant idea" is to something closer to the original version I >> posted here. I'm attaching it for reference. >> >> I'm also attaching the original documentation. It was clearly designed >> to do smooth transitions, too. (But pattern is written in slightly >> different way there, AFAICT). >> >> Clearly, having same semantics for pattern and hw_pattern is possible. > > Thank you for the attachment. The documentation part makes everything > clear. Comparing the patch from the attachment and the Baolin's patch > there is one vital part missing, from the original > pattern_trig_update(): > > if (data->next->brightness == data->curr->brightness) { > [...] > } 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)); > } I have some confusions about this, if data->next->brightness != data->curr->brightness, we should always do gradual dimming? But suppose below pattan values: echo "0 100 20 100 40 100 80 100 100 100" > pattern I do not want to do gradual dimming, just change the brightness with duration. > And the compute_brightness() implementation: > > 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; > } > > With the above the linear gradual dimming is indeed feasible. > And for non-linear dimming like breathing mode the hw_pattern will do. I am still not sure when we need the linear gradual dimming? When failed to set hardware pattern? > > There is also vital discrepancy between the documentation and the > proposed ledtrig-pattern implementation. The doc says: > > "Duration of 0 means brightness should immediately change to new value" OK. Will add in next version. > > This syntax seems to be not supported in the Baolin's patch. > > With all the above covered we will be almost there. > > Now, only the issues raised by Bjorn need a clarification: > > On 09/08/2018 07:02 AM, Bjorn Andersson wrote: > [...] >> The controls for my hardware is: >> * a list of brightness values >> * the rate of the pattern > > The two can be described using [brightness delta_t] tuples. > >> * a flag to indicate that the pattern should be played from start >> to end, end to start or start to end to start > > As above, but the tuples would have to be suitably arranged. > > We won't need any specific flag to indicate how the pattern > should be played, but instead explicitly give the tuples in the > required order. For the start-end-start case the sequence of tuples > will need to be tripled, but the middle part should be put in the > reverse order. > >> * a boolean indicating if the pattern should be played once or repeated >> indefinitely. > > We will have "repeat" file for that. > > Baolin, would you mind adding the support for gradual dimming to your > ledtrig-timer.c implementation? Yes, I will do. Thanks. -- Baolin Wang Best Regards
On 25 September 2018 at 07:54, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Mon 10 Sep 19:47 PDT 2018, Baolin Wang wrote: > [..] >> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern > [..] >> +What: /sys/class/leds/<led>/hw_pattern >> +Date: September 2018 >> +KernelVersion: 4.20 >> +Description: >> + Specify a hardware pattern for the LED, for LED hardware that >> + supports autonomously controlling brightness over time, according >> + to some preprogrammed hardware patterns. >> + >> + Since different LED hardware can have different semantics of >> + hardware patterns, each driver is expected to provide its own >> + description for the hardware patterns in their ABI documentation >> + file. >> + >> +What: /sys/class/leds/<led>/repeat >> +Date: September 2018 >> +KernelVersion: 4.20 >> +Description: >> + Specify a pattern repeat number. -1 means repeat indefinitely, >> + other negative numbers and number 0 are invalid. >> + >> + This file will always return the originally written repeat >> + number. > > This captures the infinite case much better, thanks! > > Given that 0 is described as invalid, does this imply that setting a new > pattern the repeat will be disabled? And hence if my hardware supports > only infinite or no-repeat setting a new pattern is the way to disable > further repeats (rather than just setting repeat = 0)? It won't. A new pattern setting will not affect your infinite or no-repeat setting. > Regardless, I believe the semantics of these two files will work fine > for my Qualcomm LPG driver. Thanks. -- Baolin Wang Best Regards
Hi Baolin, On 09/25/2018 01:15 PM, Baolin Wang wrote: > On 23 September 2018 at 20:25, Jacek Anaszewski > <jacek.anaszewski@gmail.com> wrote: >> On 09/22/2018 09:44 PM, Pavel Machek wrote: >>> On Sat 2018-09-22 00:18:13, Pavel Machek wrote: >>>> On Sat 2018-09-22 00:11:29, Jacek Anaszewski wrote: >>>>> On 09/21/2018 11:17 PM, Pavel Machek wrote: >>>>>> On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote: >>>>>>> Hi Baolin, >>>>>>> >>>>>>> On 09/21/2018 05:31 AM, Baolin Wang wrote: >>>>>>>> Hi Jacek and Pavel, >>>>>>>> >>>>>>>> On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@linaro.org> wrote: >>>>>>>>> This patch adds one new led trigger that LED device can configure >>>>>>>>> the software or hardware pattern and trigger it. >>>>>>>>> >>>>>>>>> Consumers can write 'pattern' file to enable the software pattern >>>>>>>>> which alters the brightness for the specified duration with one >>>>>>>>> software timer. >>>>>>>>> >>>>>>>>> Moreover consumers can write 'hw_pattern' file to enable the hardware >>>>>>>>> pattern for some LED controllers which can autonomously control >>>>>>>>> brightness over time, according to some preprogrammed hardware >>>>>>>>> patterns. >>>>>>>>> >>>>>>>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com> >>>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>>>>> >>>>>>>> Do you have any comments for the v12 patch set? Thanks. >>>>>>> >>>>>>> We will probably have to remove hw_pattern from ledtrig-pattern >>>>>>> since we are unable to come up with generic interface for it. >>>>>>> Unless thread [0] will end up with some brilliant ideas. So far >>>>>>> we're waiting for Pavel's reply. >>>>>>> >>>>>>> [0] https://lkml.org/lkml/2018/9/13/1216 >>>>>> >>>>>> To paint a picture: >>>>>> >>>>>> brightness >>>>>> >>>>>> rise hold lower hold down >>>>>> ^ XXXXXXXXXXXXXXX >>>>>> | X XX >>>>>> | X XX >>>>>> | X XXXXXXXXXXXXXXXXXXXXXXXXXX >>>>>> +-------------------------------------------------------> time >>>>>> >>>>>> This is what Baolin's hardware can do, right? >>>>>> >>>>>> This is also what pattern trigger can do, right? >>>>>> >>>>>> So all we need to do is match the two interfaces, so that hw_pattern >>>>>> returns -EINVAL on patterns hardware can not actually do. >>>>>> >>>>>> I believe I described code to do that in [0] above. >>>>> >>>>> You said that we should get the same effect by writing the >>>>> same series of tuples to either pattern or hw_pattern file. >>>>> >>>>> Below command consists of four tuples (marked with brackets >>>>> to highlight), and it will activate breathing mode in Baolin's >>>>> hw_pattern: >>>>> >>>>> "[0 rise_duration] [brightness high_duration] [brightness fall_duration] >>>>> [0 low_duration]" >>>>> >>>>> Now, I can't see how these four tuples could force the software >>>>> fallback to produce breathing effect you depicted. >>>> >>>> I really should get some sleep now. But my intention was that software >>>> fallback produces just that with those four tuples. (If it does not, >>>> we can fix the software fallback to do just that). >>> >>> And you are right, v12 1/2 seems to do the wrong thing. >>> >>> My "brilliant idea" is to something closer to the original version I >>> posted here. I'm attaching it for reference. >>> >>> I'm also attaching the original documentation. It was clearly designed >>> to do smooth transitions, too. (But pattern is written in slightly >>> different way there, AFAICT). >>> >>> Clearly, having same semantics for pattern and hw_pattern is possible. >> >> Thank you for the attachment. The documentation part makes everything >> clear. Comparing the patch from the attachment and the Baolin's patch >> there is one vital part missing, from the original >> pattern_trig_update(): >> >> if (data->next->brightness == data->curr->brightness) { >> [...] >> } 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)); >> } > > I have some confusions about this, if data->next->brightness != > data->curr->brightness, we should always do gradual dimming? But > suppose below pattan values: > echo "0 100 20 100 40 100 80 100 100 100" > patter ledtrig-pattern.txt attached by Pavel explains this: "To make the LED go instantly from one brightness value to another, use zero-time lengths." Actually you have it also: "Duration of 0 means brightness should immediately change to new value" According to this, to get instant changes your pattern should look like this: echo "0 100 0 0 20 100 20 0 40 100 40 0 80 100 80 0 100 100" > pattern > I do not want to do gradual dimming, just change the brightness with duration. > >> And the compute_brightness() implementation: >> >> 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; >> } >> >> With the above the linear gradual dimming is indeed feasible. >> And for non-linear dimming like breathing mode the hw_pattern will do. > > I am still not sure when we need the linear gradual dimming? When > failed to set hardware pattern? No. Linear gradual dimming needs to be applied only if set via pattern file. In this case we shouldn't attempt to call pattern_set at all, since we know that we are asked for enabling software engine. Similarly, in the hw_pattern handler we should call only pattern_set, and return error code, without falling back to the software engine in case of error. >> There is also vital discrepancy between the documentation and the >> proposed ledtrig-pattern implementation. The doc says: >> >> "Duration of 0 means brightness should immediately change to new value" > > OK. Will add in next version. You already have it added. >> This syntax seems to be not supported in the Baolin's patch. >> >> With all the above covered we will be almost there. >> >> Now, only the issues raised by Bjorn need a clarification: >> >> On 09/08/2018 07:02 AM, Bjorn Andersson wrote: >> [...] >>> The controls for my hardware is: >>> * a list of brightness values >>> * the rate of the pattern >> >> The two can be described using [brightness delta_t] tuples. >> >>> * a flag to indicate that the pattern should be played from start >>> to end, end to start or start to end to start >> >> As above, but the tuples would have to be suitably arranged. >> >> We won't need any specific flag to indicate how the pattern >> should be played, but instead explicitly give the tuples in the >> required order. For the start-end-start case the sequence of tuples >> will need to be tripled, but the middle part should be put in the >> reverse order. >> >>> * a boolean indicating if the pattern should be played once or repeated >>> indefinitely. >> >> We will have "repeat" file for that. >> >> Baolin, would you mind adding the support for gradual dimming to your >> ledtrig-timer.c implementation? > > Yes, I will do. Thanks. > -- Best regards, Jacek Anaszewski
Hi! > > Actually you have it also: > > "Duration of 0 means brightness should immediately change to new value" > > > > According to this, to get instant changes your pattern should look > > like this: > > > > echo "0 100 0 0 20 100 20 0 40 100 40 0 80 100 80 0 100 100" > pattern > > Make sense. Thanks for your explanation. Thanks for all the work and sorry for all the confusion! Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern new file mode 100644 index 0000000..afff9e3 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern @@ -0,0 +1,39 @@ +What: /sys/class/leds/<led>/pattern +Date: September 2018 +KernelVersion: 4.20 +Description: + Specify a software pattern for the LED, that supports altering + the brightness for the specified duration with one software + timer. + + The pattern is given by a series of tuples, of brightness and + duration (ms). The LED is expected to traverse the series and + each brightness value for the specified duration. Duration of + 0 means brightness should immediately change to new value. + + The format of the software pattern values should be: + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 + duration_3 ...". + +What: /sys/class/leds/<led>/hw_pattern +Date: September 2018 +KernelVersion: 4.20 +Description: + Specify a hardware pattern for the LED, for LED hardware that + supports autonomously controlling brightness over time, according + to some preprogrammed hardware patterns. + + Since different LED hardware can have different semantics of + hardware patterns, each driver is expected to provide its own + description for the hardware patterns in their ABI documentation + file. + +What: /sys/class/leds/<led>/repeat +Date: September 2018 +KernelVersion: 4.20 +Description: + Specify a pattern repeat number. -1 means repeat indefinitely, + other negative numbers and number 0 are invalid. + + This file will always return the originally written repeat + number. diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig index 4018af7..b76fc3c 100644 --- a/drivers/leds/trigger/Kconfig +++ b/drivers/leds/trigger/Kconfig @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV This allows LEDs to be controlled by network device activity. If unsure, say Y. +config LEDS_TRIGGER_PATTERN + tristate "LED Pattern Trigger" + help + This allows LEDs to be controlled by a software or hardware pattern + which is a series of tuples, of brightness and duration (ms). + If unsure, say N + endif # LEDS_TRIGGERS diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile index f3cfe19..9bcb64e 100644 --- a/drivers/leds/trigger/Makefile +++ b/drivers/leds/trigger/Makefile @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c new file mode 100644 index 0000000..bbef402 --- /dev/null +++ b/drivers/leds/trigger/ledtrig-pattern.c @@ -0,0 +1,344 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * LED pattern trigger + * + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented + * the first version, Baolin Wang simplified and improved the approach. + */ + +#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/timer.h> + +#define MAX_PATTERNS 1024 + +struct pattern_trig_data { + struct led_classdev *led_cdev; + struct led_pattern patterns[MAX_PATTERNS]; + struct led_pattern *curr; + struct led_pattern *next; + struct mutex lock; + u32 npatterns; + int repeat; + int last_repeat; + bool is_indefinite; + bool is_hw_pattern; + struct timer_list timer; +}; + +static void pattern_trig_update_patterns(struct pattern_trig_data *data) +{ + data->curr = data->next; + if (!data->is_indefinite && data->curr == data->patterns) + data->repeat--; + + if (data->next == data->patterns + data->npatterns - 1) + data->next = data->patterns; + else + data->next++; +} + +static void pattern_trig_timer_function(struct timer_list *t) +{ + struct pattern_trig_data *data = from_timer(data, t, timer); + + mutex_lock(&data->lock); + + if (!data->is_indefinite && !data->repeat) { + mutex_unlock(&data->lock); + return; + } + + led_set_brightness(data->led_cdev, data->curr->brightness); + mod_timer(&data->timer, jiffies + msecs_to_jiffies(data->curr->delta_t)); + pattern_trig_update_patterns(data); + + mutex_unlock(&data->lock); +} + +static int pattern_trig_start_pattern(struct led_classdev *led_cdev) +{ + struct pattern_trig_data *data = led_cdev->trigger_data; + + if (!data->npatterns) + return 0; + + if (data->is_hw_pattern) { + return led_cdev->pattern_set(led_cdev, data->patterns, + data->npatterns, data->repeat); + } + + data->curr = data->patterns; + data->next = data->npatterns > 1 ? data->patterns + 1 : data->patterns; + data->timer.expires = jiffies; + add_timer(&data->timer); + + return 0; +} + +static ssize_t repeat_show(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; + int repeat; + + mutex_lock(&data->lock); + + repeat = data->last_repeat; + + mutex_unlock(&data->lock); + + return scnprintf(buf, PAGE_SIZE, "%d\n", repeat); +} + +static ssize_t repeat_store(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 err, res; + + err = kstrtos32(buf, 10, &res); + if (err) + return err; + + /* Number 0 and negative numbers except -1 are invalid. */ + if (res < -1 || res == 0) + return -EINVAL; + + /* + * Clear previous patterns' performence firstly, and remove the timer + * without mutex lock to avoid dead lock. + */ + del_timer_sync(&data->timer); + + mutex_lock(&data->lock); + + if (data->is_hw_pattern) + led_cdev->pattern_clear(led_cdev); + + data->last_repeat = data->repeat = res; + /* -1 means repeat indefinitely */ + if (data->repeat == -1) + data->is_indefinite = true; + else + data->is_indefinite = false; + + err = pattern_trig_start_pattern(led_cdev); + + mutex_unlock(&data->lock); + return err < 0 ? err : count; +} + +static DEVICE_ATTR_RW(repeat); + +static ssize_t pattern_trig_show_patterns(struct pattern_trig_data *data, + char *buf, bool hw_pattern) +{ + ssize_t count = 0; + int i; + + mutex_lock(&data->lock); + + if (!data->npatterns || (data->is_hw_pattern ^ hw_pattern)) + goto out; + + for (i = 0; i < data->npatterns; i++) { + count += scnprintf(buf + count, PAGE_SIZE - count, + "%d %u ", + data->patterns[i].brightness, + data->patterns[i].delta_t); + } + + buf[count - 1] = '\n'; + +out: + mutex_unlock(&data->lock); + return count; +} + +static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev, + const char *buf, size_t count, + bool hw_pattern) +{ + struct pattern_trig_data *data = led_cdev->trigger_data; + int ccount, cr, offset = 0, err = 0; + + /* + * Clear previous patterns' performence firstly, and remove the timer + * without mutex lock to avoid dead lock. + */ + del_timer_sync(&data->timer); + + mutex_lock(&data->lock); + + if (data->is_hw_pattern) + led_cdev->pattern_clear(led_cdev); + + data->is_hw_pattern = hw_pattern; + data->npatterns = 0; + + while (offset < count - 1 && data->npatterns < MAX_PATTERNS) { + cr = 0; + ccount = sscanf(buf + offset, "%d %u %n", + &data->patterns[data->npatterns].brightness, + &data->patterns[data->npatterns].delta_t, &cr); + if (ccount != 2) { + data->npatterns = 0; + err = -EINVAL; + goto out; + } + + offset += cr; + data->npatterns++; + } + + err = pattern_trig_start_pattern(led_cdev); + +out: + mutex_unlock(&data->lock); + return err < 0 ? err : count; +} + +static ssize_t pattern_show(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 pattern_trig_show_patterns(data, buf, false); +} + +static ssize_t pattern_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + + return pattern_trig_store_patterns(led_cdev, buf, count, false); +} + +static DEVICE_ATTR_RW(pattern); + +static ssize_t hw_pattern_show(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 pattern_trig_show_patterns(data, buf, true); +} + +static ssize_t hw_pattern_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct led_classdev *led_cdev = dev_get_drvdata(dev); + + return pattern_trig_store_patterns(led_cdev, buf, count, true); +} + +static DEVICE_ATTR_RW(hw_pattern); + +static umode_t pattern_trig_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_repeat.attr || attr == &dev_attr_pattern.attr) + return attr->mode; + else if (attr == &dev_attr_hw_pattern.attr && led_cdev->pattern_set) + return attr->mode; + + return 0; +} + +static struct attribute *pattern_trig_attrs[] = { + &dev_attr_pattern.attr, + &dev_attr_hw_pattern.attr, + &dev_attr_repeat.attr, + NULL +}; + +static const struct attribute_group pattern_trig_group = { + .attrs = pattern_trig_attrs, + .is_visible = pattern_trig_attrs_mode, +}; + +static const struct attribute_group *pattern_trig_groups[] = { + &pattern_trig_group, + NULL, +}; + +static int pattern_trig_activate(struct led_classdev *led_cdev) +{ + struct pattern_trig_data *data; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + if (!!led_cdev->pattern_set ^ !!led_cdev->pattern_clear) { + dev_warn(led_cdev->dev, + "Hardware pattern ops validation failed\n"); + led_cdev->pattern_set = NULL; + led_cdev->pattern_clear = NULL; + } + + data->is_indefinite = true; + data->last_repeat = -1; + mutex_init(&data->lock); + data->led_cdev = led_cdev; + led_set_trigger_data(led_cdev, data); + timer_setup(&data->timer, pattern_trig_timer_function, 0); + led_cdev->activated = true; + + return 0; +} + +static void pattern_trig_deactivate(struct led_classdev *led_cdev) +{ + struct pattern_trig_data *data = led_cdev->trigger_data; + + if (!led_cdev->activated) + return; + + if (led_cdev->pattern_clear) + led_cdev->pattern_clear(led_cdev); + + del_timer_sync(&data->timer); + + led_set_brightness(led_cdev, LED_OFF); + kfree(data); + led_cdev->activated = false; +} + +static struct led_trigger pattern_led_trigger = { + .name = "pattern", + .activate = pattern_trig_activate, + .deactivate = pattern_trig_deactivate, + .groups = pattern_trig_groups, +}; + +static int __init pattern_trig_init(void) +{ + return led_trigger_register(&pattern_led_trigger); +} + +static void __exit pattern_trig_exit(void) +{ + led_trigger_unregister(&pattern_led_trigger); +} + +module_init(pattern_trig_init); +module_exit(pattern_trig_exit); + +MODULE_AUTHOR("Raphael Teysseyre <rteysseyre@gmail.com"); +MODULE_AUTHOR("Baolin Wang <baolin.wang@linaro.org"); +MODULE_DESCRIPTION("LED Pattern trigger"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/leds.h b/include/linux/leds.h index 834683d..7393a31 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,10 @@ struct led_classdev { unsigned long *delay_on, unsigned long *delay_off); + int (*pattern_set)(struct led_classdev *led_cdev, + struct led_pattern *pattern, u32 len, int repeat); + int (*pattern_clear)(struct led_classdev *led_cdev); + struct device *dev; const struct attribute_group **groups; @@ -472,4 +477,14 @@ static inline void led_classdev_notify_brightness_hw_changed( struct led_classdev *led_cdev, enum led_brightness brightness) { } #endif +/** + * struct led_pattern - pattern interval settings + * @delta_t: pattern interval delay, in milliseconds + * @brightness: pattern interval brightness + */ +struct led_pattern { + u32 delta_t; + int brightness; +}; + #endif /* __LINUX_LEDS_H_INCLUDED */