Re: [PATCH v14 1/2] leds: core: Introduce LED pattern trigger

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Baolin,

On 10/09/2018 02:01 PM, Baolin Wang wrote:
> Hi Jacek and Pavel,
> 
> On 5 October 2018 at 04:00, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>> Hi Baolin,
>>
>> On 10/03/2018 03:21 AM, Baolin Wang wrote:
>>> Hi Jacek,
>>>
>>> On 3 October 2018 at 04:25, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>>>> Hi Baolin,
>>>>
>>>> Thank you for the v14. We'll probably need v15, though :-)
>>>>
>>>> I added the comments in the code below.
>>>>
>>>> On 10/02/2018 05:43 PM, Baolin Wang wrote:
>>>>> This patch adds one new led trigger that LED device can configure
>>>>> the software or hardware pattern and trigger it.
>>>>>
>>>>> Consumers can write 'pattern' file to enable the software pattern
>>>>> which alters the brightness for the specified duration with one
>>>>> software timer.
>>>>>
>>>>> Moreover consumers can write 'hw_pattern' file to enable the hardware
>>>>> pattern for some LED controllers which can autonomously control
>>>>> brightness over time, according to some preprogrammed hardware
>>>>> patterns.
>>>>>
>>>>> Signed-off-by: Raphael Teysseyre <rteysseyre@gmail.com>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>> ---
>>>>> Changes from v13:
>>>>>  - Add duration validation for gradual dimming.
>>>>>  - Coding style optimization.
>>>>>
>>>>> Changes from v12:
>>>>>  - Add gradual dimming support for software pattern.
>>>>>
>>>>> Changes from v11:
>>>>>  - Change -1 means repeat indefinitely.
>>>>>
>>>>> Changes from v10:
>>>>>  - Change 'int' to 'u32' for delta_t field.
>>>>>
>>>>> Changes from v9:
>>>>>  - None.
>>>>>
>>>>> Changes from v8:
>>>>>  - None.
>>>>>
>>>>> Changes from v7:
>>>>>  - Move the SC27XX hardware patterns description into its own ABI file.
>>>>>
>>>>> Changes from v6:
>>>>>  - Improve commit message.
>>>>>  - Optimize the description of the hw_pattern file.
>>>>>  - Simplify some logics.
>>>>>
>>>>> Changes from v5:
>>>>>  - Add one 'hw_pattern' file for hardware patterns.
>>>>>
>>>>> Changes from v4:
>>>>>  - Change the repeat file to return the originally written number.
>>>>>  - Improve comments.
>>>>>  - Fix some build warnings.
>>>>>
>>>>> Changes from v3:
>>>>>  - Reset pattern number to 0 if user provides incorrect pattern string.
>>>>>  - Support one pattern.
>>>>>
>>>>> Changes from v2:
>>>>>  - Remove hardware_pattern boolen.
>>>>>  - Chnage the pattern string format.
>>>>>
>>>>> Changes from v1:
>>>>>  - Use ATTRIBUTE_GROUPS() to define attributes.
>>>>>  - Introduce hardware_pattern flag to determine if software pattern
>>>>>  or hardware pattern.
>>>>>  - Re-implement pattern_trig_store_pattern() function.
>>>>>  - Remove pattern_get() interface.
>>>>>  - Improve comments.
>>>>>  - Other small optimization.
>>>>> ---
>>>>>  .../ABI/testing/sysfs-class-led-trigger-pattern    |  76 ++++
>>>>>  drivers/leds/trigger/Kconfig                       |   7 +
>>>>>  drivers/leds/trigger/Makefile                      |   1 +
>>>>>  drivers/leds/trigger/ledtrig-pattern.c             | 431 +++++++++++++++++++++
>>>>>  include/linux/leds.h                               |  15 +
>>>>>  5 files changed, 530 insertions(+)
>>>>>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>>>>  create mode 100644 drivers/leds/trigger/ledtrig-pattern.c
>>>>>
>>>>> 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..22d7af7
>>>>> --- /dev/null
>>>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>>>> @@ -0,0 +1,76 @@
>>>>> +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. It can do gradual dimming and constant brightness.
>>>>> +
>>>>> +             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. When doing gradual dimming, the led brightness will be updated
>>>>> +             every 50 milliseconds, so the duration of each step should not
>>>>> +             less than 50 milliseconds.
>>>>
>>>> I'd like to avoid this constraint. Lowest supported delta_t should be 1.
>>>>
>>>> We should only prevent entering dimming mode if current delta_t
>>>> is lower than UPDATE_INTERVAL.
>>>
>>> I do not think so. If the pattern format is used for dimming and the
>>> delta_t is lower than UPDATE_INTERVAL, we should return errors. Since
>>> in this case, we can not change to constant mode.
>>
>> I'll play with the code at weekend and will let you know my findings.
> 
> Thanks Jacek. Do you have any more suggestion?

Sorry to say, but I had really busy weekend and failed to carve out
time for looking into that. It's queued up with the highest priority
on my todo list of things needing more than five minutes of attention.

> BTW, we have not get a consensus about adding one 'dimming_interval'
> file to set the dimming interval instead of fixed 50 ms.
> Pavel, I want to hear your suggestion before I submit new patch set? Thanks.

I think that this feature belongs to the "nice to have" category,
and lack of it will not prevent us from merging the patch set.

-- 
Best regards,
Jacek Anaszewski