Hi Jacek, On 5 September 2018 at 04:19, Jacek Anaszewski <jacek.anaszew...@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.w...@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
