Re: [PATCH v8 2/2] leds: sc27xx: Add pattern_set/clear interfaces for LED controller

[Baolin Wang]

Hi Jacek,

On 5 September 2018 at 04:19, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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