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

[Baolin Wang]

On 6 September 2018 at 09:43, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
> Hi Jacek,
>
> On 6 September 2018 at 03:14, Jacek Anaszewski
> <jacek.anaszewski@xxxxxxxxx> wrote:
>> Hi Baolin,
>>
>> Thanks for the v9.
>>
>> On 09/05/2018 09:20 AM, 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 v8:
>>>  - Optimize the ABI documentation file.
>>>
>>> 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      |   20 +++++
>>>  drivers/leds/leds-sc27xx-bltc.c                    |   93 ++++++++++++++++++++
>>>  2 files changed, 113 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..391ca6e
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>>> @@ -0,0 +1,20 @@
>>> +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 stages to make a single
>>> +             hardware pattern, which is used to configure the rise time,
>>> +             high time, fall time and low time for the breathing mode.
>>> +
>>> +             For the breathing mode, the SC27XX LED only expects one brightness
>>> +             for the high stage. To be compatible with the hardware pattern
>>> +             format, we should set brightness as 0 for rise stage, fall
>>> +             stage and low stage.
>>> +
>>> +             Min stage duration: 1
>>> +             Max stage duration: 255
>>> +             Stage duration step: 125 ms
>>
>> It seems that min and max stage duration are given in device
>> specific levels in contrary to the step which is given in ms.
>> Please keep it consistent. If I'm getting it right then duration
>> constraints should be given as follows:
>>
>> Min stage duration: 125 ms
>> Max stage duration: 31875 ms
>
> This is not good for users. Since what we set into LED registers is
> step counters, and each step is 125 ms. Which means it only support
> 125ms, 250ms, 375ms, 500ms .... 31875ms.
>
> If user set stage duration to 200ms, what we actually set into LED
> registers is 1 step counter (200ms / 125ms = 1), which means the
> actual duration is 125ms. That will confuse the user. So I still like
> to force user to set the step counter which can show the real
> duration.

Ah, sorry for misunderstanding, to keep consistent with the
documentation, I will change the duration unit to ms and add more
documentation for the duration step in next version. Thanks.

-- 
Baolin Wang
Best Regards