Re: [PATCH v9 1/2] dt-bindings: Add docs for EL15203000

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

On 9/18/19 11:17 PM, Oleh Kravchenko wrote:
> Hello Jacek,
> 
> 19.09.19 00:02, Jacek Anaszewski пише:
>> Hi Oleh,
>>
>> Thank you for the update.
>>
>> I have some comments below. Please take a look.
>>
>> On 9/18/19 12:52 PM, Oleh Kravchenko wrote:
>>> Add documentation and example for dt-bindings EL15203000.
>>> LED board (aka RED LED board) from Crane Merchandising Systems.
>>>
>>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>>> ---
>>>  .../bindings/leds/leds-el15203000.txt         | 147 ++++++++++++++++++
>>>  1 file changed, 147 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-el15203000.txt b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
>>> new file mode 100644
>>> index 000000000000..4a9b29cc9f46
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
>>> @@ -0,0 +1,147 @@
>>> +Crane Merchandising System - EL15203000 LED driver
>>> +--------------------------------------------------
>>> +
>>> +This LED Board (aka RED LEDs board) is widely used in
>>> +coffee vending machines produced by Crane Merchandising Systems.
>>> +The board manages 3 LEDs and supports predefined blinking patterns
>>> +for specific leds.
>>> +
>>> +Vending area LED encoded with symbol 'V' (hex code 0x56).
>>> +Doesn't have any hardware blinking pattern.
>>> +
>>> +Screen light tube LED which surrounds vending machine screen and
>>> +encoded with symbol 'S' (hex code 0x53). Supports blinking breathing pattern:
>>> +
>>> +    ^
>>> +    |
>>> +Max >_____***___________**
>>> +    |    *   *         *
>>> +    |   *     *       *
>>> +    |  *       *     *
>>> +    | *         *   *
>>> +Min >*___________***______
>>> +    |
>>> +    +------^------^------> time (sec)
>>> +    0      4      8
>>> +
>>> +
>>> +Water Pipe LED actually consists from 5 LEDs
>>
>> "(hex code 0x50)" is here missing if you want to be consistent.
>>
>>> +that exposed by protocol like one LED. Supports next patterns:
>>> +
>>> +- cascade pattern
>>> +
>>> +     ^
>>> +     |
>>> +LED0 >*****____________________*****____________________*****
>>> +     |
>>> +LED1 >_____*****____________________*****____________________
>>> +     |
>>> +LED2 >__________*****____________________*****_______________
>>> +     |
>>> +LED3 >_______________*****____________________*****__________
>>> +     |
>>> +LED4 >____________________*****____________________*****_____
>>> +     |
>>> +     +----^----^----^----^----^----^----^----^----^----^----> time (sec)
>>> +     0   0.8  1.6  2.4  3.2   4   4.8  5.6  6.4  7.2   8
>>> +
>>> +- inversed cascade pattern
>>> +
>>> +     ^
>>> +     |
>>> +LED0 >_____********************_____********************_____
>>> +     |
>>> +LED1 >*****_____********************_____********************
>>> +     |
>>> +LED2 >**********_____********************_____***************
>>> +     |
>>> +LED3 >***************_____********************_____**********
>>> +     |
>>> +LED4 >********************_____********************_____*****
>>> +     |
>>> +     +----^----^----^----^----^----^----^----^----^----^----> time (sec)
>>> +     0   0.8  1.6  2.4  3.2   4   4.8  5.6  6.4  7.2   8
>>> +
>>> +- bounce pattern
>>> +
>>> +     ^
>>> +     |
>>> +LED0 >*****________________________________________*****_____
>>> +     |
>>> +LED1 >_____*****______________________________*****_____*****
>>> +     |
>>> +LED2 >__________*****____________________*****_______________
>>> +     |
>>> +LED3 >_______________*****__________*****____________________
>>> +     |
>>> +LED4 >____________________**********_________________________
>>> +     |
>>> +     +----^----^----^----^----^----^----^----^----^----^----> time (sec)
>>> +     0   0.8  1.6  2.4  3.2   4   4.8  5.6  6.4  7.2   8
>>> +
>>> +- inversed bounce pattern
>>> +
>>> +     ^
>>> +     |
>>> +LED0 >_____****************************************_____*****
>>> +     |
>>> +LED1 >*****_____******************************_____*****_____
>>> +     |
>>> +LED2 >**********_____********************_____***************
>>> +     |
>>> +LED3 >***************_____**********_____********************
>>> +     |
>>> +LED4 >********************__________*************************
>>> +     |
>>> +     +----^----^----^----^----^----^----^----^----^----^----> time (sec)
>>> +     0   0.8  1.6  2.4  3.2   4   4.8  5.6  6.4  7.2   8
>>
>> Regarding this ASCII art - I presume you wanted to follow
>> Documentation/devicetree/bindings/leds/leds-trigger-pattern.txt.
>>
>> It was added to bindings because we support 'pattern' value
>> for linux,default-trigger, which in turn looks for 'led-pattern'
>> property, whose format needs to be documented in the LED bindings.
>>
>> leds-trigger-pattern.txt documents only common syntax for software
>> pattern engine. Currently we don't have a means to setup hw_pattern
>> for the pattern trigger from DT, which is obvious omission and your
>> patch just brings it to light.
>>
>> That said, I propose to fix it alongside and introduce led-hw-pattern
>> property. When present, ledtrig-pattern would setup the pattern
>> using pattern_set op, similarly as if it was set via sysfs hw_pattern
>> file.
>>
>> Only in this case placing the pattern depiction here would be justified.
>> Otherwise, it would have to land in the ABI documentation.
> 
> You are okay, if I move it to Documentation/ABI/testing/sysfs-class-led-driver-el15203000 ?
> 

Yes, we can cover led-hw-pattern property later.

-- 
Best regards,
Jacek Anaszewski