| Message ID | 1544613406-27026-2-git-send-email-krzk@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | leds: trigger: Add pattern initialization from Device Tree | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Wed 2018-12-12 12:16:42, Krzysztof Kozlowski wrote: > Document new linux,trigger-pattern property for initialization of LED > pattern trigger. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index aa1399814a2a..3daccd4ea8a3 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -37,6 +37,42 @@ Optional properties for child nodes: > "ide-disk" - LED indicates IDE disk activity (deprecated), > in new implementations use "disk-activity" > "timer" - LED flashes at a fixed, configurable rate > + "pattern" - LED alters the brightness for the specified duration with one > + software timer (requires "led-pattern" property) > + > +- led-pattern : String with default pattern for certain triggers. Each trigger > + may parse this string differently: > + - one-shot : two numbers specifying delay on and delay off, > + - timer : two numbers specifying delay on and delay off, Needs specifying that numbers are in miliseconds, at the very least. > + - pattern : 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. > + > + 1. For gradual dimming, the dimming interval now is set as 50 > + milliseconds. So the tuple with duration less than dimming > + interval (50ms) is treated as a step change of brightness, > + i.e. the subsequent brightness will be applied without adding > + intervening dimming intervals. > + > + The gradual dimming format of the software pattern values should be: > + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 > + duration_3 ...". > + For example "0 1000 255 2000" will make the LED go gradually > + from zero-intensity to max (255) intensity in 1000 > + milliseconds, then back to zero intensity in 2000 > + milliseconds. > + > + 2. To make the LED go instantly from one brightness value to > + another, pattern should use zero-time lengths (the brightness > + must be same as the previous tuple's). So the format should be: > + "brightness_1 duration_1 brightness_1 0 brightness_2 > + duration_2 brightness_2 0 ...". > + For example "0 1000 0 0 255 2000 255 0" will make the LED > + stay off for one second, then stay at max brightness for two > + seconds. > + No. Duplicated code is bad, and so is duplicated documentation. Maybe the documentation should be in device tree part, but lets put it into separate file, and reference it from the sysfs docs. 50msec for step change is really a implementation detail of Linux. May be worth documenting for sysfs, but not for device tree description. People should always specify 0 if they want step change. Actually, no, I don't think this is quite suitable. These are arrays of integers. They probably should not be specified as strings in dts...? (Plus, of course, timer and one-shot are subset of pattern, all we need is to specify number of repeats...) Pavel
On Wed, 12 Dec 2018 at 13:32, Pavel Machek <pavel@ucw.cz> wrote: > > On Wed 2018-12-12 12:16:42, Krzysztof Kozlowski wrote: > > Document new linux,trigger-pattern property for initialization of LED > > pattern trigger. > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > > index aa1399814a2a..3daccd4ea8a3 100644 > > --- a/Documentation/devicetree/bindings/leds/common.txt > > +++ b/Documentation/devicetree/bindings/leds/common.txt > > @@ -37,6 +37,42 @@ Optional properties for child nodes: > > "ide-disk" - LED indicates IDE disk activity (deprecated), > > in new implementations use "disk-activity" > > "timer" - LED flashes at a fixed, configurable rate > > + "pattern" - LED alters the brightness for the specified duration with one > > + software timer (requires "led-pattern" property) > > + > > +- led-pattern : String with default pattern for certain triggers. Each trigger > > + may parse this string differently: > > + - one-shot : two numbers specifying delay on and delay off, > > + - timer : two numbers specifying delay on and delay > off, > > Needs specifying that numbers are in miliseconds, at the very least. Sure. > > + - pattern : 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. > > + > > + 1. For gradual dimming, the dimming interval now is set as 50 > > + milliseconds. So the tuple with duration less than dimming > > + interval (50ms) is treated as a step change of brightness, > > + i.e. the subsequent brightness will be applied without adding > > + intervening dimming intervals. > > + > > + The gradual dimming format of the software pattern values should be: > > + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 > > + duration_3 ...". > > + For example "0 1000 255 2000" will make the LED go gradually > > + from zero-intensity to max (255) intensity in 1000 > > + milliseconds, then back to zero intensity in 2000 > > + milliseconds. > > + > > + 2. To make the LED go instantly from one brightness value to > > + another, pattern should use zero-time lengths (the brightness > > + must be same as the previous tuple's). So the format should be: > > + "brightness_1 duration_1 brightness_1 0 brightness_2 > > + duration_2 brightness_2 0 ...". > > + For example "0 1000 0 0 255 2000 255 0" will make the LED > > + stay off for one second, then stay at max brightness for two > > + seconds. > > + > > No. Duplicated code is bad, and so is duplicated documentation. Maybe > the documentation should be in device tree part, but lets put it into > separate file, and reference it from the sysfs docs. I guess this would satisfy both needs - no external references from bindings and no duplication. > 50msec for step change is really a implementation detail of Linux. May > be worth documenting for sysfs, but not for device tree > description. People should always specify 0 if they want step change. > > Actually, no, I don't think this is quite suitable. > > These are arrays of integers. They probably should not be specified as > strings in dts...? Makes sense although this would limit the possible formats for other triggers and LED drivers. Probably there are not many of such use cases (pattern different than integer numbers) so maybe there is no point to worry about it too much. Best regards, Krzysztof > > (Plus, of course, timer and one-shot are subset of pattern, all we > need is to specify number of repeats...) > > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, Dec 12, 2018 at 12:16:42PM +0100, Krzysztof Kozlowski wrote: > Document new linux,trigger-pattern property for initialization of LED > pattern trigger. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index aa1399814a2a..3daccd4ea8a3 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -37,6 +37,42 @@ Optional properties for child nodes: > "ide-disk" - LED indicates IDE disk activity (deprecated), > in new implementations use "disk-activity" > "timer" - LED flashes at a fixed, configurable rate > + "pattern" - LED alters the brightness for the specified duration with one > + software timer (requires "led-pattern" property) > + > +- led-pattern : String with default pattern for certain triggers. Each trigger > + may parse this string differently: > + - one-shot : two numbers specifying delay on and delay off, > + - timer : two numbers specifying delay on and delay off, I misunderstood that these triggers applied to this. Is there any point to supporting these modes? Aren't they just a subset or simplification of a pattern? Can we control the timer frequency from DT? The pattern could address that limitation. > + - pattern : The pattern is given by a series of tuples, of Don't you need one-shot vs. repeat modes for patterns too? I could imagine you'd want a pattern for any trigger. > + 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. > + > + 1. For gradual dimming, the dimming interval now is set as 50 > + milliseconds. So the tuple with duration less than dimming > + interval (50ms) is treated as a step change of brightness, > + i.e. the subsequent brightness will be applied without adding > + intervening dimming intervals. > + > + The gradual dimming format of the software pattern values should be: > + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 > + duration_3 ...". > + For example "0 1000 255 2000" will make the LED go gradually > + from zero-intensity to max (255) intensity in 1000 > + milliseconds, then back to zero intensity in 2000 > + milliseconds. > + > + 2. To make the LED go instantly from one brightness value to > + another, pattern should use zero-time lengths (the brightness > + must be same as the previous tuple's). So the format should be: > + "brightness_1 duration_1 brightness_1 0 brightness_2 > + duration_2 brightness_2 0 ...". > + For example "0 1000 0 0 255 2000 255 0" will make the LED > + stay off for one second, then stay at max brightness for two > + seconds. > + > > - led-max-microamp : Maximum LED supply current in microamperes. This property > can be made mandatory for the board configurations > -- > 2.7.4 >
On Mon, 17 Dec 2018 at 23:40, Rob Herring <robh@kernel.org> wrote: > > On Wed, Dec 12, 2018 at 12:16:42PM +0100, Krzysztof Kozlowski wrote: > > Document new linux,trigger-pattern property for initialization of LED > > pattern trigger. > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > > index aa1399814a2a..3daccd4ea8a3 100644 > > --- a/Documentation/devicetree/bindings/leds/common.txt > > +++ b/Documentation/devicetree/bindings/leds/common.txt > > @@ -37,6 +37,42 @@ Optional properties for child nodes: > > "ide-disk" - LED indicates IDE disk activity (deprecated), > > in new implementations use "disk-activity" > > "timer" - LED flashes at a fixed, configurable rate > > + "pattern" - LED alters the brightness for the specified duration with one > > + software timer (requires "led-pattern" property) > > + > > +- led-pattern : String with default pattern for certain triggers. Each trigger > > + may parse this string differently: > > + - one-shot : two numbers specifying delay on and delay off, > > + - timer : two numbers specifying delay on and delay off, > > I misunderstood that these triggers applied to this. Is there any point > to supporting these modes? I am not sure whether I explained this clearly enough, so let me be more specific. The "led-pattern" is a string with pattern which will be interpreted differently, based on LED trigger. One-shot and timer triggers expect two numbers. Pattern trigger expects full pattern. > Aren't they just a subset or simplification > of a pattern? Yes, they are. > > Can we control the timer frequency from DT? The pattern could address > that limitation. No, currently we cannot. This patch with pattern solves that issue. > > > + - pattern : The pattern is given by a series of tuples, of > > Don't you need one-shot vs. repeat modes for patterns too? Repeat mode is defined by trigger. One-shot is one time. Timer and patter triggers are repeating. > > I could imagine you'd want a pattern for any trigger. In general I think that only these three triggers require such initialization through pattern. Other triggers - like heartbeat or activity - have they own bindings (or do not need any). Following Pavel's suggestion, I could convert it to array of integers which still should be suitable for these and any future triggers operating on numbers. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index aa1399814a2a..3daccd4ea8a3 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -37,6 +37,42 @@ Optional properties for child nodes: "ide-disk" - LED indicates IDE disk activity (deprecated), in new implementations use "disk-activity" "timer" - LED flashes at a fixed, configurable rate + "pattern" - LED alters the brightness for the specified duration with one + software timer (requires "led-pattern" property) + +- led-pattern : String with default pattern for certain triggers. Each trigger + may parse this string differently: + - one-shot : two numbers specifying delay on and delay off, + - timer : two numbers specifying delay on and delay off, + - pattern : 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. + + 1. For gradual dimming, the dimming interval now is set as 50 + milliseconds. So the tuple with duration less than dimming + interval (50ms) is treated as a step change of brightness, + i.e. the subsequent brightness will be applied without adding + intervening dimming intervals. + + The gradual dimming format of the software pattern values should be: + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 + duration_3 ...". + For example "0 1000 255 2000" will make the LED go gradually + from zero-intensity to max (255) intensity in 1000 + milliseconds, then back to zero intensity in 2000 + milliseconds. + + 2. To make the LED go instantly from one brightness value to + another, pattern should use zero-time lengths (the brightness + must be same as the previous tuple's). So the format should be: + "brightness_1 duration_1 brightness_1 0 brightness_2 + duration_2 brightness_2 0 ...". + For example "0 1000 0 0 255 2000 255 0" will make the LED + stay off for one second, then stay at max brightness for two + seconds. + - led-max-microamp : Maximum LED supply current in microamperes. This property can be made mandatory for the board configurations
Document new linux,trigger-pattern property for initialization of LED pattern trigger. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- Documentation/devicetree/bindings/leds/common.txt | 36 +++++++++++++++++++++++ 1 file changed, 36 insertions(+)