Message ID | 5a502ec29251c019ddad8f3314ab45fc0f6feaf7.1536027873.git.baolin.wang@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | [v8,1/2] leds: core: Introduce LED pattern trigger | expand |
Hi Baolin, On 09/04/2018 01:01 PM, Baolin Wang wrote: > This patch implements the 'pattern_set'and 'pattern_clear' > interfaces to support SC27XX LED breathing mode. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > Changes from v7: > - Add its own ABI documentation file. > > Changes from v6: > - None. > > Changes from v5: > - None. > > Changes from v4: > - None. > > Changes from v3: > - None. > > Changes from v2: > - None. > > Changes from v1: > - Remove pattern_get interface. > --- > .../ABI/testing/sysfs-class-led-driver-sc27xx | 11 +++ > drivers/leds/leds-sc27xx-bltc.c | 94 ++++++++++++++++++++ > 2 files changed, 105 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx > > diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx > new file mode 100644 > index 0000000..ecef3ba > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx > @@ -0,0 +1,11 @@ > +What: /sys/class/leds/<led>/hw_pattern > +Date: September 2018 > +KernelVersion: 4.20 > +Description: > + Specify a hardware pattern for the SC27XX LED. For the SC27XX > + LED controller, it only supports 4 hardware patterns to configure If I understand it correctly then the four components: low, rise, high, and fall time make a single pattern. So calling it "4 hardware patterns" can be misleading. Below you're using "stage" notion - it would be good to use it consequently on the whole span of this document. > + the low time, rise time, high time and fall time for the breathing > + mode, and each stage duration unit is 125ms. So the format of > + the hardware pattern values should be: I'd be more precise here: Min stage duration: ??? Max stage duration: ??? Stage duration step: 125 ms > + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 > + duration_3 brightness_4 duration_4". Looking at sc27xx_led_pattern_set() it doesn't seem like you would use brightnesses other then brightness_1. I assume that the low brightness is fixed to 0 and the high brightness is the brightness_1. If yes, than we don't need brightnesses in this pattern definition, since the current brightness will suffice. You'd need to alter the hw_pattern description to say that the brightness currently set is to be used as a high brightness, and the hw_pattern for this driver consists only of the four delta_t components. This is clear example of what I had on mind when having doubts about using tuples for pattern_set. Alternatively, if we want to enforce tuples format for hw_pattern, and if we want to be consistent - we'd need to require defining target brightness for each stage properly, i.e. echo "100 500 100 500 0 500 0 500" > pattern Which would mean: 1. Rise from brightness 0 to 100 for 500 ms. 2. Keep brightness 100 for 500 ms. 3. Fall from brightness 100 to 0 for 500 ms. 4. Keep brightness 0 for 500 ms. -- Best regards, Jacek Anaszewski
Hi Jacek, On 5 September 2018 at 04:19, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote: > Hi Baolin, > > On 09/04/2018 01:01 PM, Baolin Wang wrote: >> This patch implements the 'pattern_set'and 'pattern_clear' >> interfaces to support SC27XX LED breathing mode. >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> --- >> Changes from v7: >> - Add its own ABI documentation file. >> >> Changes from v6: >> - None. >> >> Changes from v5: >> - None. >> >> Changes from v4: >> - None. >> >> Changes from v3: >> - None. >> >> Changes from v2: >> - None. >> >> Changes from v1: >> - Remove pattern_get interface. >> --- >> .../ABI/testing/sysfs-class-led-driver-sc27xx | 11 +++ >> drivers/leds/leds-sc27xx-bltc.c | 94 ++++++++++++++++++++ >> 2 files changed, 105 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx >> >> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx >> new file mode 100644 >> index 0000000..ecef3ba >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx >> @@ -0,0 +1,11 @@ >> +What: /sys/class/leds/<led>/hw_pattern >> +Date: September 2018 >> +KernelVersion: 4.20 >> +Description: >> + Specify a hardware pattern for the SC27XX LED. For the SC27XX >> + LED controller, it only supports 4 hardware patterns to configure > > If I understand it correctly then the four components: low, rise, high, > and fall time make a single pattern. So calling it "4 hardware patterns" > can be misleading. Below you're using "stage" notion - it would be good > to use it consequently on the whole span of this document. Sure. I will modify the documentation to avoid misleading words. > >> + the low time, rise time, high time and fall time for the breathing >> + mode, and each stage duration unit is 125ms. So the format of >> + the hardware pattern values should be: > > I'd be more precise here: > > Min stage duration: ??? > Max stage duration: ??? > Stage duration step: 125 ms OK. > >> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 >> + duration_3 brightness_4 duration_4". > > Looking at sc27xx_led_pattern_set() it doesn't seem like you would > use brightnesses other then brightness_1. I assume that the low > brightness is fixed to 0 and the high brightness is the brightness_1. > If yes, than we don't need brightnesses in this pattern definition, > since the current brightness will suffice. > > You'd need to alter the hw_pattern description to say that the > brightness currently set is to be used as a high brightness, and the > hw_pattern for this driver consists only of the four delta_t components. Sorry for confusing here. For SC27XX LED, the 4 stages use one same brightness. To be compatible with the hardware pattern format, so we force to set one brightness for each stage. I will add some comments to explain the LED expects the same brightness for each stage instead of using the current brightness file which is not used for our breathing mode. > > This is clear example of what I had on mind when having doubts > about using tuples for pattern_set. > > Alternatively, if we want to enforce tuples format for hw_pattern, > and if we want to be consistent - we'd need to require defining target > brightness for each stage properly, i.e. > > echo "100 500 100 500 0 500 0 500" > pattern > > Which would mean: > > 1. Rise from brightness 0 to 100 for 500 ms. > 2. Keep brightness 100 for 500 ms. > 3. Fall from brightness 100 to 0 for 500 ms. > 4. Keep brightness 0 for 500 ms. > > -- > Best regards, > Jacek Anaszewski -- Baolin Wang Best Regards
On 5 September 2018 at 10:43, Baolin Wang <baolin.wang@linaro.org> wrote: > Hi Jacek, > > On 5 September 2018 at 04:19, Jacek Anaszewski > <jacek.anaszewski@gmail.com> wrote: >> Hi Baolin, >> >> On 09/04/2018 01:01 PM, Baolin Wang wrote: >>> This patch implements the 'pattern_set'and 'pattern_clear' >>> interfaces to support SC27XX LED breathing mode. >>> >>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>> --- >>> Changes from v7: >>> - Add its own ABI documentation file. >>> >>> Changes from v6: >>> - None. >>> >>> Changes from v5: >>> - None. >>> >>> Changes from v4: >>> - None. >>> >>> Changes from v3: >>> - None. >>> >>> Changes from v2: >>> - None. >>> >>> Changes from v1: >>> - Remove pattern_get interface. >>> --- >>> .../ABI/testing/sysfs-class-led-driver-sc27xx | 11 +++ >>> drivers/leds/leds-sc27xx-bltc.c | 94 ++++++++++++++++++++ >>> 2 files changed, 105 insertions(+) >>> create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx >>> >>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx >>> new file mode 100644 >>> index 0000000..ecef3ba >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx >>> @@ -0,0 +1,11 @@ >>> +What: /sys/class/leds/<led>/hw_pattern >>> +Date: September 2018 >>> +KernelVersion: 4.20 >>> +Description: >>> + Specify a hardware pattern for the SC27XX LED. For the SC27XX >>> + LED controller, it only supports 4 hardware patterns to configure >> >> If I understand it correctly then the four components: low, rise, high, >> and fall time make a single pattern. So calling it "4 hardware patterns" >> can be misleading. Below you're using "stage" notion - it would be good >> to use it consequently on the whole span of this document. > > Sure. I will modify the documentation to avoid misleading words. > >> >>> + the low time, rise time, high time and fall time for the breathing >>> + mode, and each stage duration unit is 125ms. So the format of >>> + the hardware pattern values should be: >> >> I'd be more precise here: >> >> Min stage duration: ??? >> Max stage duration: ??? >> Stage duration step: 125 ms > > OK. > >> >>> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 >>> + duration_3 brightness_4 duration_4". >> >> Looking at sc27xx_led_pattern_set() it doesn't seem like you would >> use brightnesses other then brightness_1. I assume that the low >> brightness is fixed to 0 and the high brightness is the brightness_1. >> If yes, than we don't need brightnesses in this pattern definition, >> since the current brightness will suffice. >> >> You'd need to alter the hw_pattern description to say that the >> brightness currently set is to be used as a high brightness, and the >> hw_pattern for this driver consists only of the four delta_t components. > > Sorry for confusing here. For SC27XX LED, the 4 stages use one same > brightness. To be compatible with the hardware pattern format, so we > force to set one brightness for each stage. I will add some comments > to explain the LED expects the same brightness for each stage instead > of using the current brightness file which is not used for our > breathing mode. > >> >> This is clear example of what I had on mind when having doubts >> about using tuples for pattern_set. >> >> Alternatively, if we want to enforce tuples format for hw_pattern, >> and if we want to be consistent - we'd need to require defining target >> brightness for each stage properly, i.e. >> >> echo "100 500 100 500 0 500 0 500" > pattern After more thinking, I will change the description to reflect the real brightness according to your suggestion. Please see my new patch. Thanks. >> >> Which would mean: >> >> 1. Rise from brightness 0 to 100 for 500 ms. >> 2. Keep brightness 100 for 500 ms. >> 3. Fall from brightness 100 to 0 for 500 ms. >> 4. Keep brightness 0 for 500 ms. >> >> -- >> Best regards, >> Jacek Anaszewski > > > > -- > Baolin Wang > Best Regards -- Baolin Wang Best Regards
On Tue 04 Sep 04:01 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. > + So, after a full circle we're back at drivers with support for hardware patterns should have their own ABI for setting that pattern. The controls for my hardware is: * a list of brightness values * the rate of the pattern * a flag to indicate that the pattern should be played from start to end, end to start or start to end to start * a boolean indicating if the pattern should be played once or repeated indefinitely. Given that the interface now is hw specific, what benefit is there to attempt to cram these 4 knobs into "hw_pattern"? Or am I allowed to create additional files for the latter three? > +What: /sys/class/leds/<led>/repeat > +Date: September 2018 > +KernelVersion: 4.20 > +Description: > + Specify a pattern repeat number. 0 means repeat indefinitely. > + > + This file will always return the originally written repeat > + number. I'm still convinced that this will confuse our users and to me it would be more logical if this denotes the number of times the pattern should be repeated, with e.g. negative numbers denoting infinite. In particular I expect to have to explain why my driver expects that you write 0 in the file named "repeat" to make it repeat and 1 to make it not repeat. Regards, Bjorn
Hi Bjorn, On 09/08/2018 07:02 AM, Bjorn Andersson wrote: > On Tue 04 Sep 04:01 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. >> + > > So, after a full circle we're back at drivers with support for hardware > patterns should have their own ABI for setting that pattern. > > The controls for my hardware is: > * a list of brightness values > * the rate of the pattern > * a flag to indicate that the pattern should be played from start > to end, end to start or start to end to start > * a boolean indicating if the pattern should be played once or repeated > indefinitely. > > Given that the interface now is hw specific, what benefit is there to > attempt to cram these 4 knobs into "hw_pattern"? Or am I allowed to > create additional files for the latter three? So this is an argument corroborating my concerns raised in [0]. I really think that we should allow for custom pattern interfaces defined by LED class drivers. >> +What: /sys/class/leds/<led>/repeat >> +Date: September 2018 >> +KernelVersion: 4.20 >> +Description: >> + Specify a pattern repeat number. 0 means repeat indefinitely. >> + >> + This file will always return the originally written repeat >> + number. > > I'm still convinced that this will confuse our users and to me it would > be more logical if this denotes the number of times the pattern should > be repeated, with e.g. negative numbers denoting infinite. Sounds reasonable. Let's change this semantics as you propose. > In particular I expect to have to explain why my driver expects that you > write 0 in the file named "repeat" to make it repeat and 1 to make it > not repeat. [0] https://lkml.org/lkml/2018/9/3/1192 -- Best regards, Jacek Anaszewski
On Fri 2018-09-07 22:02:08, Bjorn Andersson wrote: > On Tue 04 Sep 04:01 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. > > + > > So, after a full circle we're back at drivers with support for hardware > patterns should have their own ABI for setting that pattern. > > The controls for my hardware is: > * a list of brightness values > * the rate of the pattern > * a flag to indicate that the pattern should be played from start > to end, end to start or start to end to start > * a boolean indicating if the pattern should be played once or repeated > indefinitely. No, we are not back to full circle. Or at least we should not be. Yes, hw_pattern can have some limitation pattern does not, but if you take values from hw_pattern file and put them into pattern file, you should get the same pattern (with more power being consumed). And that property is kind of important for me, because it should keep the ABI reasonable. > > +What: /sys/class/leds/<led>/repeat > > +Date: September 2018 > > +KernelVersion: 4.20 > > +Description: > > + Specify a pattern repeat number. 0 means repeat indefinitely. > > + > > + This file will always return the originally written repeat > > + number. > > I'm still convinced that this will confuse our users and to me it would > be more logical if this denotes the number of times the pattern should > be repeated, with e.g. negative numbers denoting infinite. > > In particular I expect to have to explain why my driver expects that you > write 0 in the file named "repeat" to make it repeat and 1 to make it > not repeat. Ok, -1 works for me :-). Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi! > >> I'm still convinced that this will confuse our users and to me it would > >> be more logical if this denotes the number of times the pattern should > >> be repeated, with e.g. negative numbers denoting infinite. > > > > Sounds reasonable. Let's change this semantics as you propose. > > > >> In particular I expect to have to explain why my driver expects that you > >> write 0 in the file named "repeat" to make it repeat and 1 to make it > >> not repeat. > > Hm, so there are some cases we need to make clear. > 1) If negative numbers present infinite, so what's the meaning of number 0? > 2) What we should show for users if repeat number is negative, just > show negative numbers or one string "infinite"? I'd say just -1 is infinite, anything else is error. And yes, reading it should just display -1. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 11 September 2018 at 05:20, Pavel Machek <pavel@ucw.cz> wrote: > Hi! > >> >> I'm still convinced that this will confuse our users and to me it would >> >> be more logical if this denotes the number of times the pattern should >> >> be repeated, with e.g. negative numbers denoting infinite. >> > >> > Sounds reasonable. Let's change this semantics as you propose. >> > >> >> In particular I expect to have to explain why my driver expects that you >> >> write 0 in the file named "repeat" to make it repeat and 1 to make it >> >> not repeat. >> >> Hm, so there are some cases we need to make clear. >> 1) If negative numbers present infinite, so what's the meaning of number 0? >> 2) What we should show for users if repeat number is negative, just >> show negative numbers or one string "infinite"? > > I'd say just -1 is infinite, anything else is error. > > And yes, reading it should just display -1. OK. Thanks. -- Baolin Wang Best Regards
Pavel, On 09/10/2018 11:19 PM, Pavel Machek wrote: > On Fri 2018-09-07 22:02:08, Bjorn Andersson wrote: >> On Tue 04 Sep 04:01 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. >>> + >> >> So, after a full circle we're back at drivers with support for hardware >> patterns should have their own ABI for setting that pattern. >> >> The controls for my hardware is: >> * a list of brightness values >> * the rate of the pattern >> * a flag to indicate that the pattern should be played from start >> to end, end to start or start to end to start >> * a boolean indicating if the pattern should be played once or repeated >> indefinitely. > > No, we are not back to full circle. > > Or at least we should not be. > > Yes, hw_pattern can have some limitation pattern does not, but if you > take values from hw_pattern file and put them into pattern file, you > should get the same pattern (with more power being consumed). And that > property is kind of important for me, because it should keep the ABI > reasonable. If you looked at what we agreed on with Baolin, you'd realize that this property is in no way preserved. This is what the whole story is about - we're introducing hw_pattern because of difficulties in describing breathing pattern by a series of [brightness delta_t] tuples. And Bjorn presented another example. I'm inclined to leave the definition of hw_pattern semantics to the LED class drivers, and allow them to create related sysfs files. >>> +What: /sys/class/leds/<led>/repeat >>> +Date: September 2018 >>> +KernelVersion: 4.20 >>> +Description: >>> + Specify a pattern repeat number. 0 means repeat indefinitely. >>> + >>> + This file will always return the originally written repeat >>> + number. >> >> I'm still convinced that this will confuse our users and to me it would >> be more logical if this denotes the number of times the pattern should >> be repeated, with e.g. negative numbers denoting infinite. >> >> In particular I expect to have to explain why my driver expects that you >> write 0 in the file named "repeat" to make it repeat and 1 to make it >> not repeat. > > Ok, -1 works for me :-). > > Thanks, > Pavel > -- Best regards, Jacek Anaszewski
Hi! > >>> 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. > >>> + > >> > >> So, after a full circle we're back at drivers with support for hardware > >> patterns should have their own ABI for setting that pattern. > >> > >> The controls for my hardware is: > >> * a list of brightness values > >> * the rate of the pattern > >> * a flag to indicate that the pattern should be played from start > >> to end, end to start or start to end to start > >> * a boolean indicating if the pattern should be played once or repeated > >> indefinitely. > > > > No, we are not back to full circle. > > > > Or at least we should not be. > > > > Yes, hw_pattern can have some limitation pattern does not, but if you > > take values from hw_pattern file and put them into pattern file, you > > should get the same pattern (with more power being consumed). And that > > property is kind of important for me, because it should keep the ABI > > reasonable. > > If you looked at what we agreed on with Baolin, you'd realize > that this property is in no way preserved. > This is what the whole story is about - we're introducing hw_pattern > because of difficulties in describing breathing pattern by a series > of [brightness delta_t] tuples. > > And Bjorn presented another example. I'm inclined to leave the > definition of hw_pattern semantics to the LED class drivers, > and allow them to create related sysfs files. Please lets not do that. We already have drivers that do that and it is complete nightmare. Some take binary code for the tiny CPU driving the LED. What exactly is the problem? [brightness delta_t] can describe anything single LED can do in finite time. You are right, that [brightness delta_t] sequence may get rather long, and it may be hard to turn that sequence into parameters. Are there any _interesting_ sequences hardware can do but [brightness delta_t] can not store reasonably? Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi! > >>> No, we are not back to full circle. > >>> > >>> Or at least we should not be. > >>> > >>> Yes, hw_pattern can have some limitation pattern does not, but if you > >>> take values from hw_pattern file and put them into pattern file, you > >>> should get the same pattern (with more power being consumed). And that > >>> property is kind of important for me, because it should keep the ABI > >>> reasonable. > >> > >> If you looked at what we agreed on with Baolin, you'd realize > >> that this property is in no way preserved. > >> This is what the whole story is about - we're introducing hw_pattern > >> because of difficulties in describing breathing pattern by a series > >> of [brightness delta_t] tuples. > >> > >> And Bjorn presented another example. I'm inclined to leave the > >> definition of hw_pattern semantics to the LED class drivers, > >> and allow them to create related sysfs files. > > > > Please lets not do that. > > > > We already have drivers that do that and it is complete > > nightmare. Some take binary code for the tiny CPU driving the LED. > > > > What exactly is the problem? [brightness delta_t] can describe > > You wrote: > > <quote> > Yes, hw_pattern can have some limitation pattern does not, but if you > take values from hw_pattern file and put them into pattern file, you > should get the same pattern (with more power being consumed). > </quote> > > The problem is that we decided to introduce hw_pattern to > to take away from drivers a responsibility for translating > a series of tuples, approximating the brightness curve, > to the values that can be written to the hardware registers. > > Because this is what would need to be done to check if hw can support > given series of tuples and activate it. Actually with this approach > we wouldn't need hw_pattern at all, since pattern alone would do. > But implementation thereof is what we could call a nightmare. > > What follows, your claim from the quotation is inaccurate: > values from hw_pattern written to the pattern file will not > produce the same pattern, at least in case of what was proposed > in [0] for drivers/leds/leds-sc27xx-bltc.c. That sounds easy, see below. > > anything single LED can do in finite time. You are right, that > > [brightness delta_t] sequence may get rather long, and it may be hard > > to turn that sequence into parameters. Are there any _interesting_ > > sequences hardware can do but [brightness delta_t] can not store > > reasonably? > > Please propose the implementation of pattern_set for > drivers/leds/leds-sc27xx-bltc.c breathing pattern, that will > setup breathing mode basing on the values from tuples. > > Use Baolin's patch [0] for a reference of what hardware expects. > > [0] https://lore.kernel.org/patchwork/patch/984246/ Yep, so we change documentation to require 0 rise_duration brightness high_duration brightness fall_duration 0 low_duration" ...and we are done; at least as long as user writes expected pattern to the file. 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. What am I missing? Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 09/12/2018 10:41 PM, Pavel Machek wrote: > Hi! > >>>>> No, we are not back to full circle. >>>>> >>>>> Or at least we should not be. >>>>> >>>>> Yes, hw_pattern can have some limitation pattern does not, but if you >>>>> take values from hw_pattern file and put them into pattern file, you >>>>> should get the same pattern (with more power being consumed). And that >>>>> property is kind of important for me, because it should keep the ABI >>>>> reasonable. >>>> >>>> If you looked at what we agreed on with Baolin, you'd realize >>>> that this property is in no way preserved. >>>> This is what the whole story is about - we're introducing hw_pattern >>>> because of difficulties in describing breathing pattern by a series >>>> of [brightness delta_t] tuples. >>>> >>>> And Bjorn presented another example. I'm inclined to leave the >>>> definition of hw_pattern semantics to the LED class drivers, >>>> and allow them to create related sysfs files. >>> >>> Please lets not do that. >>> >>> We already have drivers that do that and it is complete >>> nightmare. Some take binary code for the tiny CPU driving the LED. >>> >>> What exactly is the problem? [brightness delta_t] can describe >> >> You wrote: >> >> <quote> >> Yes, hw_pattern can have some limitation pattern does not, but if you >> take values from hw_pattern file and put them into pattern file, you >> should get the same pattern (with more power being consumed). >> </quote> >> >> The problem is that we decided to introduce hw_pattern to >> to take away from drivers a responsibility for translating >> a series of tuples, approximating the brightness curve, >> to the values that can be written to the hardware registers. >> >> Because this is what would need to be done to check if hw can support >> given series of tuples and activate it. Actually with this approach >> we wouldn't need hw_pattern at all, since pattern alone would do. >> But implementation thereof is what we could call a nightmare. >> >> What follows, your claim from the quotation is inaccurate: >> values from hw_pattern written to the pattern file will not >> produce the same pattern, at least in case of what was proposed >> in [0] for drivers/leds/leds-sc27xx-bltc.c. > > That sounds easy, see below. > >>> anything single LED can do in finite time. You are right, that >>> [brightness delta_t] sequence may get rather long, and it may be hard >>> to turn that sequence into parameters. Are there any _interesting_ >>> sequences hardware can do but [brightness delta_t] can not store >>> reasonably? >> >> Please propose the implementation of pattern_set for >> drivers/leds/leds-sc27xx-bltc.c breathing pattern, that will >> setup breathing mode basing on the values from tuples. >> >> Use Baolin's patch [0] for a reference of what hardware expects. >> >> [0] https://lore.kernel.org/patchwork/patch/984246/ > > Yep, so we change documentation to require > > 0 rise_duration brightness high_duration brightness fall_duration 0 low_duration" > > ...and we are done; at least as long as user writes expected pattern > to the file. > > 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. > > What am I missing? I suppose that breathing pattern means smooth rise and fall of brightness. I'd even say that it should be a non-linear function. I'm not sure if you noticed my quotation of your statement from the previous message, because the modification you proposed doesn't introduce any novelty to the related matter. The difference between pattern and discussed hw_pattern semantics is that: - hw_pattern semantics for leds-sc27xx-bltc.c requires only four tuples to setup the breathing pattern. - the same four tuples written to the pattern file would result in four brightness changes -- Best regards, Jacek Anaszewski
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..8cc5099 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern @@ -0,0 +1,38 @@ +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. 0 means repeat indefinitely. + + 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..0179191 --- /dev/null +++ b/drivers/leds/trigger/ledtrig-pattern.c @@ -0,0 +1,337 @@ +// 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; + u32 repeat; + u32 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; + u32 repeat; + + mutex_lock(&data->lock); + + repeat = data->last_repeat; + + mutex_unlock(&data->lock); + + return scnprintf(buf, PAGE_SIZE, "%u\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; + unsigned long res; + int err; + + err = kstrtoul(buf, 10, &res); + if (err) + return err; + + /* + * 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; + /* 0 means repeat indefinitely */ + data->is_indefinite = !data->repeat; + + 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 %d ", + 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 %d %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; + 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..74fc2c6 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -22,6 +22,7 @@ #include <linux/workqueue.h> struct device; +struct led_pattern; /* * LED Core */ @@ -88,6 +89,11 @@ struct led_classdev { unsigned long *delay_on, unsigned long *delay_off); + int (*pattern_set)(struct led_classdev *led_cdev, + struct led_pattern *pattern, int len, + unsigned int repeat); + int (*pattern_clear)(struct led_classdev *led_cdev); + struct device *dev; const struct attribute_group **groups; @@ -472,4 +478,14 @@ static inline void led_classdev_notify_brightness_hw_changed( struct led_classdev *led_cdev, enum led_brightness brightness) { } #endif +/** + * struct led_pattern - pattern interval settings + * @delta_t: pattern interval delay, in milliseconds + * @brightness: pattern interval brightness + */ +struct led_pattern { + int delta_t; + int brightness; +}; + #endif /* __LINUX_LEDS_H_INCLUDED */