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

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

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.

> +
> +Required properties:
> +- compatible : "crane,el15203000"
> +- #address-cells : must be 1
> +- #size-cells : must be 0
> +
> +Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
> +apply. In particular, "reg" and "spi-max-frequency" properties must be given.
> +
> +Optional LED sub-node properties:
> +- function:
> +	see Documentation/devicetree/bindings/leds/common.txt
> +- color:
> +	see Documentation/devicetree/bindings/leds/common.txt
> +- label:
> +	see Documentation/devicetree/bindings/leds/common.txt (deprecated)
> +
> +Example
> +-------
> +
> +#include <dt-bindings/leds/common.h>
> +
> +led-controller@0 {
> +	compatible = "crane,el15203000";
> +	reg = <0>;
> +	spi-max-frequency = <50000>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	/* water pipe */
> +	led@50 {
> +		reg = <0x50>;
> +		function = "pipe";
> +		color = <LED_COLOR_ID_RED>;
> +	};
> +
> +	/* screen frame */
> +	led@53 {
> +		reg = <0x53>;
> +		function = "screen";
> +		color = <LED_COLOR_ID_RED>;
> +	};
> +
> +	/* vending area */
> +	led@56 {
> +		reg = <0x56>;
> +		function = "vend";
> +		color = <LED_COLOR_ID_RED>;
> +	};
> +};
> 

-- 
Best regards,
Jacek Anaszewski