[v4,1/2] dt-bindings: Add docs for EL15203000
diff mbox series

Message ID 20190808203204.8614-1-oleg@kaa.org.ua
State Changes Requested
Headers show
Series
  • [v4,1/2] dt-bindings: Add docs for EL15203000
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Oleh Kravchenko Aug. 8, 2019, 8:32 p.m. UTC
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         | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt

Comments

Jacek Anaszewski Aug. 13, 2019, 8:28 p.m. UTC | #1
Hi Oleh,

Thank you for the patch set.

On 8/8/19 10:32 PM, Oleh Kravchenko wrote:
> This patch adds a LED class driver for the RGB LEDs found on
> the Crane Merchandising System EL15203000 LEDs board
> (aka RED LEDs board).
> 
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
>  .../testing/sysfs-class-led-driver-el15203000 |  22 +
>  drivers/leds/Kconfig                          |  13 +
>  drivers/leds/Makefile                         |   1 +
>  drivers/leds/leds-el15203000.c                | 478 ++++++++++++++++++
>  4 files changed, 514 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-el15203000
>  create mode 100644 drivers/leds/leds-el15203000.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-el15203000 b/Documentation/ABI/testing/sysfs-class-led-driver-el15203000
> new file mode 100644
> index 000000000000..91a483e493d9
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-el15203000
> @@ -0,0 +1,22 @@
> +What:		/sys/class/leds/<led>/hw_pattern
> +Date:		August 2019
> +KernelVersion:	5.3
> +Description:
> +		Specify a hardware pattern for the EL15203000 LED.
> +		The LEDs board supports only predefined patterns by firmware
> +		for specific LEDs.
> +
> +		Breathing mode for Screen frame light tube:
> +		"0 4000 1 4000"
> +
> +		Cascade mode for Pipe LED:
> +		"1 800 2 800 4 800 8 800 16 800 1 800 2 800 4 800 8 800 16 800"

Why the sequence "1 800 2 800 4 800 8 800 16 800" is duplicated here?
It seems redundant. But aside of that - aren't the timings modifiable?
In other words - are these all patterns somehow configurable or they are
pre-programmed?

> +
> +		Inverted cascade mode for Pipe LED:
> +		"30 800 29 800 27 800 23 800 15 800 30 800 29 800 27 800 23 800 15 800"

Similar duplication here.

> +
> +		Bounce mode for Pipe LED:
> +		"1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"

Instead of two repeating "16 800" you could provide "16 1600".
But here again is the question whether these values are configurable.
From what I can see in your driver implementation you're expecting
exactly the values provided in these examples to enable given hardware
pattern (led_pattern_cmp()).

> +
> +		Inverted bounce mode for Pipe LED:
> +		"30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"
Dan Murphy Aug. 13, 2019, 8:31 p.m. UTC | #2
Jacek

Need your input below

On 8/8/19 3:32 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         | 47 +++++++++++++++++++
>   1 file changed, 47 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..4c2245babfdc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
> @@ -0,0 +1,47 @@
> +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.
> +
> +Required properties:
> +- compatible : "crane,el15203000"
> +- reg :
> +	see Documentation/devicetree/bindings/spi/spi-bus.txt
> +- spi-max-frequency : (optional)
> +	see Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Optional LED sub-node properties:
> +- label :
> +	see Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger :
> +	see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example
> +-------
> +
> +led-controller@0 {
> +	compatible = "crane,el15203000";
> +	reg = <0>;
> +	spi-max-frequency = <50000>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	/* water pipe */
> +	pipe@50 {
> +		reg = <0x50>;
> +		label = "red:pipe";

Should we use the color and function property here?

Not sure what function would be for pipe, screen or vending but there may be

comparable functions that may fit.

Dan

<snip>
Oleh Kravchenko Aug. 13, 2019, 8:37 p.m. UTC | #3
Hello Jacek,

Thank you for your useful review,

13.08.19 23:28, Jacek Anaszewski пише:
> Hi Oleh,
> 
> Thank you for the patch set.
> 
> On 8/8/19 10:32 PM, Oleh Kravchenko wrote:
>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-el15203000
>> @@ -0,0 +1,22 @@
>> +What:		/sys/class/leds/<led>/hw_pattern
>> +Date:		August 2019
>> +KernelVersion:	5.3
>> +Description:
>> +		Specify a hardware pattern for the EL15203000 LED.
>> +		The LEDs board supports only predefined patterns by firmware
>> +		for specific LEDs.
>> +
>> +		Breathing mode for Screen frame light tube:
>> +		"0 4000 1 4000"
>> +
>> +		Cascade mode for Pipe LED:
>> +		"1 800 2 800 4 800 8 800 16 800 1 800 2 800 4 800 8 800 16 800"
> 
> Why the sequence "1 800 2 800 4 800 8 800 16 800" is duplicated here?
> It seems redundant. But aside of that - aren't the timings modifiable?
> In other words - are these all patterns somehow configurable or they are
> pre-programmed?
> 

All pattern is predefined, you can't change them at all.
I just tried to describe real things what happened in LED board.
It's ticks every 800 milliseconds for Pipe LEDs.

>> +
>> +		Inverted cascade mode for Pipe LED:
>> +		"30 800 29 800 27 800 23 800 15 800 30 800 29 800 27 800 23 800 15 800"
> 
> Similar duplication here.
> 
>> +
>> +		Bounce mode for Pipe LED:
>> +		"1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"
> 
> Instead of two repeating "16 800" you could provide "16 1600".
> But here again is the question whether these values are configurable.
> From what I can see in your driver implementation you're expecting
> exactly the values provided in these examples to enable given hardware
> pattern (led_pattern_cmp()).
> 
>> +
>> +		Inverted bounce mode for Pipe LED:
>> +		"30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"
> 

Should I cut this patterns to smaller? Or let keep it?
Jacek Anaszewski Aug. 14, 2019, 6:18 p.m. UTC | #4
Hi Oleh,

On 8/13/19 10:37 PM, Oleh Kravchenko wrote:
> Hello Jacek,
> 
> Thank you for your useful review,
> 
> 13.08.19 23:28, Jacek Anaszewski пише:
>> Hi Oleh,
>>
>> Thank you for the patch set.
>>
>> On 8/8/19 10:32 PM, Oleh Kravchenko wrote:
>>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-el15203000
>>> @@ -0,0 +1,22 @@
>>> +What:		/sys/class/leds/<led>/hw_pattern
>>> +Date:		August 2019
>>> +KernelVersion:	5.3
>>> +Description:
>>> +		Specify a hardware pattern for the EL15203000 LED.
>>> +		The LEDs board supports only predefined patterns by firmware
>>> +		for specific LEDs.
>>> +
>>> +		Breathing mode for Screen frame light tube:
>>> +		"0 4000 1 4000"
>>> +
>>> +		Cascade mode for Pipe LED:
>>> +		"1 800 2 800 4 800 8 800 16 800 1 800 2 800 4 800 8 800 16 800"
>>
>> Why the sequence "1 800 2 800 4 800 8 800 16 800" is duplicated here?
>> It seems redundant. But aside of that - aren't the timings modifiable?
>> In other words - are these all patterns somehow configurable or they are
>> pre-programmed?
>>
> 
> All pattern is predefined, you can't change them at all.
> I just tried to describe real things what happened in LED board.
> It's ticks every 800 milliseconds for Pipe LEDs.

It makes me wonder how you figured out the values? If you have
a documentation for this controller, could you share how the pattern
settings are documented?

>>> +
>>> +		Inverted cascade mode for Pipe LED:
>>> +		"30 800 29 800 27 800 23 800 15 800 30 800 29 800 27 800 23 800 15 800"
>>
>> Similar duplication here.
>>
>>> +
>>> +		Bounce mode for Pipe LED:
>>> +		"1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"
>>
>> Instead of two repeating "16 800" you could provide "16 1600".
>> But here again is the question whether these values are configurable.
>> From what I can see in your driver implementation you're expecting
>> exactly the values provided in these examples to enable given hardware
>> pattern (led_pattern_cmp()).
>>
>>> +
>>> +		Inverted bounce mode for Pipe LED:
>>> +		"30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"
>>
> 
> Should I cut this patterns to smaller? Or let keep it?
> 

For the first two we could do without sequence duplication.
Oleh Kravchenko Aug. 14, 2019, 7:46 p.m. UTC | #5
Hello Jacek,

14.08.19 21:18, Jacek Anaszewski пише:
>>> Why the sequence "1 800 2 800 4 800 8 800 16 800" is duplicated here?
>>> It seems redundant. But aside of that - aren't the timings modifiable?
>>> In other words - are these all patterns somehow configurable or they are
>>> pre-programmed?
>>>
>>
>> All pattern is predefined, you can't change them at all.
>> I just tried to describe real things what happened in LED board.
>> It's ticks every 800 milliseconds for Pipe LEDs.
> 
> It makes me wonder how you figured out the values? If you have
> a documentation for this controller, could you share how the pattern
> settings are documented?

I saw the code of firmware.
Not sure if I can find any documentation for it right now.
 
>>>> +
>>>> +		Inverted cascade mode for Pipe LED:
>>>> +		"30 800 29 800 27 800 23 800 15 800 30 800 29 800 27 800 23 800 15 800"
>>>
>>> Similar duplication here.
>>>
>>>> +
>>>> +		Bounce mode for Pipe LED:
>>>> +		"1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"
>>>
>>> Instead of two repeating "16 800" you could provide "16 1600".
>>> But here again is the question whether these values are configurable.
>>> From what I can see in your driver implementation you're expecting
>>> exactly the values provided in these examples to enable given hardware
>>> pattern (led_pattern_cmp()).
>>>
>>>> +
>>>> +		Inverted bounce mode for Pipe LED:
>>>> +		"30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"
>>>
>>
>> Should I cut this patterns to smaller? Or let keep it?
>>
> 
> For the first two we could do without sequence duplication.

Ok, I will reduce it.
Jacek Anaszewski Aug. 14, 2019, 7:54 p.m. UTC | #6
Dan,

On 8/13/19 10:31 PM, Dan Murphy wrote:
> Jacek
> 
> Need your input below
> 
> On 8/8/19 3:32 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         | 47 +++++++++++++++++++
>>   1 file changed, 47 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..4c2245babfdc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
>> @@ -0,0 +1,47 @@
>> +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.
>> +
>> +Required properties:
>> +- compatible : "crane,el15203000"
>> +- reg :
>> +    see Documentation/devicetree/bindings/spi/spi-bus.txt
>> +- spi-max-frequency : (optional)
>> +    see Documentation/devicetree/bindings/spi/spi-bus.txt
>> +
>> +Optional LED sub-node properties:
>> +- label :
>> +    see Documentation/devicetree/bindings/leds/common.txt
>> +- linux,default-trigger :
>> +    see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Example
>> +-------
>> +
>> +led-controller@0 {
>> +    compatible = "crane,el15203000";
>> +    reg = <0>;
>> +    spi-max-frequency = <50000>;
>> +    #address-cells = <1>;
>> +    #size-cells = <0>;
>> +
>> +    /* water pipe */
>> +    pipe@50 {
>> +        reg = <0x50>;
>> +        label = "red:pipe";
> 
> Should we use the color and function property here?

Yes, label is already deprecated in the common LED bindings
in linux-next. We need separate color and function here.

> Not sure what function would be for pipe, screen or vending but there
> may be
> 
> comparable functions that may fit.

These are functions specific to this board and it is not something
that could have some vast system-wide use. Common LED functions aim
rather to unify the naming for LEDs on standard system devices and
peripherals.

I'd not strive for creating common LED_FUNCTION macros for these.
Jacek Anaszewski Aug. 14, 2019, 7:57 p.m. UTC | #7
On 8/14/19 9:46 PM, Oleh Kravchenko wrote:
> Hello Jacek,
> 
> 14.08.19 21:18, Jacek Anaszewski пише:
>>>> Why the sequence "1 800 2 800 4 800 8 800 16 800" is duplicated here?
>>>> It seems redundant. But aside of that - aren't the timings modifiable?
>>>> In other words - are these all patterns somehow configurable or they are
>>>> pre-programmed?
>>>>
>>>
>>> All pattern is predefined, you can't change them at all.
>>> I just tried to describe real things what happened in LED board.
>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>
>> It makes me wonder how you figured out the values? If you have
>> a documentation for this controller, could you share how the pattern
>> settings are documented?
> 
> I saw the code of firmware.
> Not sure if I can find any documentation for it right now.

Have you tried to alter the values? Or check what happens when
the duplication is removed?

>>>>> +
>>>>> +		Inverted cascade mode for Pipe LED:
>>>>> +		"30 800 29 800 27 800 23 800 15 800 30 800 29 800 27 800 23 800 15 800"
>>>>
>>>> Similar duplication here.
>>>>
>>>>> +
>>>>> +		Bounce mode for Pipe LED:
>>>>> +		"1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"
>>>>
>>>> Instead of two repeating "16 800" you could provide "16 1600".
>>>> But here again is the question whether these values are configurable.
>>>> From what I can see in your driver implementation you're expecting
>>>> exactly the values provided in these examples to enable given hardware
>>>> pattern (led_pattern_cmp()).
>>>>
>>>>> +
>>>>> +		Inverted bounce mode for Pipe LED:
>>>>> +		"30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"
>>>>
>>>
>>> Should I cut this patterns to smaller? Or let keep it?
>>>
>>
>> For the first two we could do without sequence duplication.
> 
> Ok, I will reduce it.

Please hold on for a while. I will have some more remarks to the driver,
just collecting missing info for now to gain more complete view on this
device.
Oleh Kravchenko Aug. 14, 2019, 8:16 p.m. UTC | #8
Hello Jacek,

14.08.19 22:57, Jacek Anaszewski пише:
>>>>
>>>> All pattern is predefined, you can't change them at all.
>>>> I just tried to describe real things what happened in LED board.
>>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>>
>>> It makes me wonder how you figured out the values? If you have
>>> a documentation for this controller, could you share how the pattern
>>> settings are documented?
>>
>> I saw the code of firmware.
>> Not sure if I can find any documentation for it right now.
> 
> Have you tried to alter the values? Or check what happens when
> the duplication is removed?

What do you mean alter? It doesn't make any sense.
Board is accepts only brightness level from '0' to '5'.
I'm really confused :-)
 
>>>
>>> For the first two we could do without sequence duplication.
>>
>> Ok, I will reduce it.
> 
> Please hold on for a while. I will have some more remarks to the driver,
> just collecting missing info for now to gain more complete view on this
> device.

Here is the full story:

EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
It's provide access to 3 LEDs:

- First LED (Screen) is a light tube around big 21" screen
  It's have 3 brightness levels:
  * OFF
  * ON
  * Breathing mode (8 seconds full cycle)
- Second LED (Vending area) is highlight coffee cap
  * OFF
  * ON
- Third LED (Pipe) is actually virtual, because consists from 5 LEDs
  * OFF for all 5 LEDs
  * ON for all 5 LEDs
  * Cascade
  * Inverses cascade
  * Bounce
  * Inverses bounce
Jacek Anaszewski Aug. 14, 2019, 8:53 p.m. UTC | #9
On 8/14/19 10:16 PM, Oleh Kravchenko wrote:
> Hello Jacek,
> 
> 14.08.19 22:57, Jacek Anaszewski пише:
>>>>>
>>>>> All pattern is predefined, you can't change them at all.
>>>>> I just tried to describe real things what happened in LED board.
>>>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>>>
>>>> It makes me wonder how you figured out the values? If you have
>>>> a documentation for this controller, could you share how the pattern
>>>> settings are documented?
>>>
>>> I saw the code of firmware.
>>> Not sure if I can find any documentation for it right now.
>>
>> Have you tried to alter the values? Or check what happens when
>> the duplication is removed?
> 
> What do you mean alter? It doesn't make any sense.
> Board is accepts only brightness level from '0' to '5'.
> I'm really confused :-)

OK, I didn't analyze the driver thoroughly enough.
Now everything is clear to me. Patterns are triggered just
by writing two-byte command.

>>>>
>>>> For the first two we could do without sequence duplication.
>>>
>>> Ok, I will reduce it.
>>
>> Please hold on for a while. I will have some more remarks to the driver,
>> just collecting missing info for now to gain more complete view on this
>> device.
> 
> Here is the full story:
> 
> EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
> It's provide access to 3 LEDs:
> 
> - First LED (Screen) is a light tube around big 21" screen
>   It's have 3 brightness levels:
>   * OFF
>   * ON
>   * Breathing mode (8 seconds full cycle)

OK, so this is LED string. We would need to bend rules to make it
appearing as a single LED class device, but allowing non-synchronous
blinking of LEDs in the string.

> - Second LED (Vending area) is highlight coffee cap
>   * OFF
>   * ON
> - Third LED (Pipe) is actually virtual, because consists from 5 LEDs
>   * OFF for all 5 LEDs
>   * ON for all 5 LEDs
>   * Cascade
>   * Inverses cascade
>   * Bounce
>   * Inverses bounce
Similarly in case of this one. I will give you a feedback on how
to define the patterns within few days.
Oleh Kravchenko Aug. 14, 2019, 8:58 p.m. UTC | #10
By the way,

14.08.19 23:53, Jacek Anaszewski пише:
> On 8/14/19 10:16 PM, Oleh Kravchenko wrote:
>> Hello Jacek,
>>
>> 14.08.19 22:57, Jacek Anaszewski пише:
>>>>>>
>>>>>> All pattern is predefined, you can't change them at all.
>>>>>> I just tried to describe real things what happened in LED board.
>>>>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>>>>
>>>>> It makes me wonder how you figured out the values? If you have
>>>>> a documentation for this controller, could you share how the pattern
>>>>> settings are documented?
>>>>
>>>> I saw the code of firmware.
>>>> Not sure if I can find any documentation for it right now.
>>>
>>> Have you tried to alter the values? Or check what happens when
>>> the duplication is removed?
>>
>> What do you mean alter? It doesn't make any sense.
>> Board is accepts only brightness level from '0' to '5'.
>> I'm really confused :-)
> 
> OK, I didn't analyze the driver thoroughly enough.
> Now everything is clear to me. Patterns are triggered just
> by writing two-byte command.
> 
>>>>>
>>>>> For the first two we could do without sequence duplication.
>>>>
>>>> Ok, I will reduce it.
>>>
>>> Please hold on for a while. I will have some more remarks to the driver,
>>> just collecting missing info for now to gain more complete view on this
>>> device.
>>
>> Here is the full story:
>>
>> EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
>> It's provide access to 3 LEDs:
>>
>> - First LED (Screen) is a light tube around big 21" screen
>>   It's have 3 brightness levels:
>>   * OFF
>>   * ON
>>   * Breathing mode (8 seconds full cycle)
> 
> OK, so this is LED string. We would need to bend rules to make it
> appearing as a single LED class device, but allowing non-synchronous
> blinking of LEDs in the string.
> 
>> - Second LED (Vending area) is highlight coffee cap
>>   * OFF
>>   * ON
>> - Third LED (Pipe) is actually virtual, because consists from 5 LEDs
>>   * OFF for all 5 LEDs
>>   * ON for all 5 LEDs
>>   * Cascade
>>   * Inverses cascade
>>   * Bounce
>>   * Inverses bounce

Full cycle takes 8 seconds too.
So actually it's splitted into 10 stages, each takes 800ms.

> Similarly in case of this one. I will give you a feedback on how
> to define the patterns within few days.
> 

Take you time.
Pavel Machek Aug. 17, 2019, 3:02 p.m. UTC | #11
Hi!

> >>>> All pattern is predefined, you can't change them at all.
> >>>> I just tried to describe real things what happened in LED board.
> >>>> It's ticks every 800 milliseconds for Pipe LEDs.
> >>>
> >>> It makes me wonder how you figured out the values? If you have
> >>> a documentation for this controller, could you share how the pattern
> >>> settings are documented?
> >>
> >> I saw the code of firmware.
> >> Not sure if I can find any documentation for it right now.
> > 
> > Have you tried to alter the values? Or check what happens when
> > the duplication is removed?
> 
> What do you mean alter? It doesn't make any sense.
> Board is accepts only brightness level from '0' to '5'.
> I'm really confused :-)

For the record, what you did seems ok to me.

> >> Ok, I will reduce it.
> > 
> > Please hold on for a while. I will have some more remarks to the driver,
> > just collecting missing info for now to gain more complete view on this
> > device.
> 
> Here is the full story:
> 
> EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
> It's provide access to 3 LEDs:
> 
> - First LED (Screen) is a light tube around big 21" screen
>   It's have 3 brightness levels:
>   * OFF
>   * ON
>   * Breathing mode (8 seconds full cycle)
> - Second LED (Vending area) is highlight coffee cap
>   * OFF
>   * ON

Again, this is ok.

> - Third LED (Pipe) is actually virtual, because consists from 5 LEDs
>   * OFF for all 5 LEDs
>   * ON for all 5 LEDs
>   * Cascade
>   * Inverses cascade
>   * Bounce
>   * Inverses bounce

But having one virtual LED that is really five LEDs scares me a tiny
bit. But I guess its ok for your case...

Best regards,
									Pavel
Jacek Anaszewski Aug. 18, 2019, 1:41 p.m. UTC | #12
Hi Oleh,

On 8/8/19 10:32 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         | 47 +++++++++++++++++++
>  1 file changed, 47 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..4c2245babfdc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
> @@ -0,0 +1,47 @@
> +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.
> +
> +Required properties:
> +- compatible : "crane,el15203000"
> +- reg :
> +	see Documentation/devicetree/bindings/spi/spi-bus.txt
> +- spi-max-frequency : (optional)
> +	see Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Optional LED sub-node properties:
> +- label :
> +	see Documentation/devicetree/bindings/leds/common.txt

Please change this label description to the below:

- 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).

> +- linux,default-trigger :
> +	see Documentation/devicetree/bindings/leds/common.txt
> +
> +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 */
> +	pipe@50 {

s/pipe/led/

> +		reg = <0x50>;
> +		label = "red:pipe";

label is now deprecated.

Please use function and color:

	function = "pipe";
	color = <LED_COLOR_ID_RED>;


> +	};
> +
> +	/* screen frame */
> +	screen@53 {

s/screen/led/

> +		reg = <0x53>;
> +		label = "red:screen";

	function = "screen";
	color = <LED_COLOR_ID_RED>;

> +	};
> +
> +	/* vending area */
> +	vend@56 {

s/vend/led/

> +		reg = <0x56>;
> +		label = "red:vend";

	function = "vend";
	color = <LED_COLOR_ID_RED>;

> +	};
> +};
>
Jacek Anaszewski Aug. 18, 2019, 1:47 p.m. UTC | #13
Hi Oleh,

Please find my review remarks below.

On 8/8/19 10:32 PM, Oleh Kravchenko wrote:
> This patch adds a LED class driver for the RGB LEDs found on
> the Crane Merchandising System EL15203000 LEDs board
> (aka RED LEDs board).
> 
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
>  .../testing/sysfs-class-led-driver-el15203000 |  22 +
>  drivers/leds/Kconfig                          |  13 +
>  drivers/leds/Makefile                         |   1 +
>  drivers/leds/leds-el15203000.c                | 478 ++++++++++++++++++
>  4 files changed, 514 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-el15203000
>  create mode 100644 drivers/leds/leds-el15203000.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-el15203000 b/Documentation/ABI/testing/sysfs-class-led-driver-el15203000
> new file mode 100644
> index 000000000000..91a483e493d9
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-el15203000
> @@ -0,0 +1,22 @@
> +What:		/sys/class/leds/<led>/hw_pattern
> +Date:		August 2019
> +KernelVersion:	5.3
> +Description:
> +		Specify a hardware pattern for the EL15203000 LED.
> +		The LEDs board supports only predefined patterns by firmware
> +		for specific LEDs.
> +
> +		Breathing mode for Screen frame light tube:
> +		"0 4000 1 4000"
> +
> +		Cascade mode for Pipe LED:
> +		"1 800 2 800 4 800 8 800 16 800 1 800 2 800 4 800 8 800 16 800"

There's no point in repeating same sequence. Just require to provide it
once:

"1 800 2 800 4 800 8 800 16 800"

> +
> +		Inverted cascade mode for Pipe LED:
> +		"30 800 29 800 27 800 23 800 15 800 30 800 29 800 27 800 23 800 15 800"

The same story:

"30 800 29 800 27 800 23 800 15 800"

> +
> +		Bounce mode for Pipe LED:
> +		"1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"
> +
> +		Inverted bounce mode for Pipe LED:
> +		"30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"

These can be left as they are now.

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 760f73a49c9f..0cbdb9ba5213 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -129,6 +129,19 @@ config LEDS_CR0014114
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-cr0014114.
>  
> +config LEDS_EL15203000
> +	tristate "LED Support for Crane EL15203000"
> +	depends on LEDS_CLASS
> +	depends on SPI
> +	depends on OF
> +	help
> +	  This option enables support for EL15203000 LED Board
> +	  (aka RED LED board) which is widely used in coffee vending
> +	  machines produced by Crane Merchandising Systems.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-el15203000.
> +
>  config LEDS_LM3530
>  	tristate "LCD Backlight driver for LM3530"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 1e9702ebffee..1f193ffc2feb 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
>  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> +obj-$(CONFIG_LEDS_EL15203000)		+= leds-el15203000.o
>  
>  # LED Userspace Drivers
>  obj-$(CONFIG_LEDS_USER)			+= uleds.o
> diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
> new file mode 100644
> index 000000000000..c62da5ec6630
> --- /dev/null
> +++ b/drivers/leds/leds-el15203000.c
> @@ -0,0 +1,478 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Crane Merchandising Systems. All rights reserved.
> +// Copyright (C) 2019 Oleh Kravchenko <oleg@kaa.org.ua>
> +
> +#include <linux/delay.h>
> +#include <linux/leds.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +#include <uapi/linux/uleds.h>
> +
> +/*
> + * EL15203000 SPI protocol description:
> + * +-----+------------+
> + * | LED | BRIGHTNESS |
> + * +-----+------------+
> + * |  1  |      1     |
> + * +-----+------------+
> + * (*) LEDs MCU board expects 20 msec delay per byte.
> + *
> + * LEDs:
> + * +----------+--------------+-------------------------------------------+
> + * |    ID    |     NAME     |         DESCRIPTION                       |
> + * +----------+--------------+-------------------------------------------+
> + * | 'P' 0x50 |     Pipe     | Consists from 5 LEDs, controlled by board |
> + * +----------+--------------+-------------------------------------------+
> + * | 'S' 0x53 | Screen frame | Light tube around the screen              |
> + * +----------+--------------+-------------------------------------------+
> + * | 'V' 0x56 | Vending area | Highlights a cup of coffee                |
> + * +----------+--------------+-------------------------------------------+
> + *
> + * BRIGHTNESS:
> + * +----------+-----------------+--------------+--------------+
> + * |  VALUES  |       PIPE      | SCREEN FRAME | VENDING AREA |
> + * +----------+-----------------+--------------+--------------+
> + * | '0' 0x30 |                      Off                      |
> + * +----------+-----------------------------------------------+
> + * | '1' 0x31 |                      On                       |
> + * +----------+-----------------+--------------+--------------+
> + * | '2' 0x32 |     Cascade     |   Breathing  |
> + * +----------+-----------------+--------------+
> + * | '3' 0x33 | Inverse cascade |
> + * +----------+-----------------+
> + * | '4' 0x34 |     Bounce      |
> + * +----------+-----------------+
> + * | '5' 0x35 | Inverse bounce  |
> + * +----------+-----------------+
> + */
> +
> +/* EL15203000 default settings */
> +#define EL_FW_DELAY_USEC	20000
> +
> +enum el15203000_brightness {

Please change 'brightness' to 'command' or so. Brightness is misleading
since this device supports in fact only one level of brightness for
non-pattern use case.

> +	/* for all LEDs */
> +	EL_OFF			= '0',
> +	EL_ON			= '1',
> +
> +	/* for Screen LED */
> +	EL_SCREEN_BREATHING	= '2',
> +
> +	/* for Pipe LED */
> +	EL_PIPE_CASCADE		= '2',
> +	EL_PIPE_INV_CASCADE	= '3',
> +	EL_PIPE_BOUNCE		= '4',
> +	EL_PIPE_INV_BOUNCE	= '5',
> +};
> +
> +struct el15203000_led {
> +	struct el15203000	*priv;
> +	struct led_classdev	ldev;
> +	char			name[LED_MAX_NAME_SIZE];

This field will not be needed if we switch to
devm_led_classdev_register_ext().

> +	u8			reg;
> +};
> +
> +struct el15203000 {
> +	struct device		*dev;
> +	struct mutex		lock;
> +	struct spi_device	*spi;
> +	unsigned long		delay;
> +	size_t			count;
> +	struct el15203000_led	leds[];
> +};
> +
> +static int el15203000_cmd(struct el15203000_led *led, u8 brightness)
> +{
> +	int		ret;
> +	u8		cmd[2];
> +	size_t		i;
> +	unsigned long	jdelay, udelay, now;
> +
> +	mutex_lock(&led->priv->lock);
> +
> +	dev_dbg(led->priv->dev, "Set brightness of %s to %d",
> +		led->name, brightness);
> +
> +	/* to avoid SPI mistiming with firmware we should wait some time */
> +	now = jiffies;
> +	if (time_after(led->priv->delay, now)) {
> +		jdelay = led->priv->delay - now;
> +		udelay = jiffies_to_usecs(jdelay);
> +
> +		dev_dbg(led->priv->dev, "Wait %luus (%lu jiffies) to sync",
> +			udelay, jdelay);
> +
> +		usleep_range(udelay, udelay + 1);
> +	}
> +
> +	cmd[0] = led->reg;
> +	cmd[1] = brightness;
> +
> +	for (i = 0; i < ARRAY_SIZE(cmd); i++) {
> +		if (i)
> +			usleep_range(EL_FW_DELAY_USEC,
> +				     EL_FW_DELAY_USEC + 1);
> +
> +		ret = spi_write(led->priv->spi, &cmd[i], sizeof(cmd[i]));
> +		if (ret) {
> +			dev_err(led->priv->dev,
> +				"spi_write() error %d", ret);
> +			break;
> +		}
> +	}
> +
> +	led->priv->delay = jiffies + usecs_to_jiffies(EL_FW_DELAY_USEC);
> +
> +	mutex_unlock(&led->priv->lock);
> +
> +	return ret;
> +}
> +
> +static int el15203000_set_blocking(struct led_classdev *ldev,
> +				   enum led_brightness brightness)
> +{
> +	struct el15203000_led	*led = container_of(ldev,
> +						    struct el15203000_led,
> +						    ldev);
> +
> +	return el15203000_cmd(led, brightness == LED_OFF ? EL_OFF : EL_ON);
> +}
> +
> +static int led_pattern_cmp(const struct led_pattern *p1, u32 p1_len,
> +			   const struct led_pattern *p2, u32 p2_len)
> +{
> +	u32 i;
> +
> +	if (p1_len != p2_len)
> +		return -1;
> +
> +	for (i = 0; i < p1_len; i ++)
> +		if (p1[i].delta_t != p2[i].delta_t ||
> +		    p1[i].brightness != p2[i].brightness)
> +			return -1;
> +
> +	return 0;
> +}
> +
> +int el15203000_pattern_set_S(struct led_classdev *ldev,
> +			     struct led_pattern *pattern,
> +			     u32 len, int repeat)
> +{
> +	struct el15203000_led	*led = container_of(ldev,
> +						    struct el15203000_led,
> +						    ldev);
> +	struct led_pattern	scr_pattern[] = {{
> +		.delta_t	= 4000,
> +		.brightness	= 0,
> +	}, {
> +		.delta_t	= 4000,
> +		.brightness	= 1,
> +	}};
> +
> +	if (repeat > 0)
> +		return -EINVAL;
> +
> +	if (led_pattern_cmp(scr_pattern, ARRAY_SIZE(scr_pattern), pattern, len))
> +		return -EINVAL;
> +
> +	dev_dbg(led->priv->dev, "Breathing mode for '%s'", led->name);
> +
> +	return el15203000_cmd(led, EL_SCREEN_BREATHING);
> +}
> +
> +int el15203000_pattern_set_P(struct led_classdev *ldev,
> +			   struct led_pattern *pattern,
> +			   u32 len, int repeat)
> +{
> +	struct el15203000_led	*led = container_of(ldev,
> +						    struct el15203000_led,
> +						    ldev);
> +
> +	struct led_pattern cascade[] = {{
> +		.delta_t	= 800,
> +		.brightness	= 1,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 2,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 4,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 8,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 16,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 1,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 2,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 4,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 8,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 16,
> +	}};
> +
> +	struct led_pattern inv_cascade[] = {{
> +		.delta_t	= 800,
> +		.brightness	= 30,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 29,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 27,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 23,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 15,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 30,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 29,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 27,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 23,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 15,
> +	}};
> +
> +	struct led_pattern	bounce[] = {{
> +		.delta_t	= 800,
> +		.brightness	= 1,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 2,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 4,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 8,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 16,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 16,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 8,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 4,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 2,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 1,
> +	}};
> +
> +	struct led_pattern	inv_bounce[] = {{
> +		.delta_t	= 800,
> +		.brightness	= 30,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 29,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 27,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 23,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 15,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 15,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 23,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 27,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 29,
> +	}, {
> +		.delta_t	= 800,
> +		.brightness	= 30,
> +	}};

There is no need for these arrays. We can come up with formulas for
validating the pattern tuples. Basically all of them involve differences
in brightness being powers of two:

"1 800 2 800 4 800 8 800 16 800"

Here each next step should be equal to (1 << i) staring from i = 0.

"30 800 29 800 27 800 23 800 15 800"

brightness = 30;
for (i = 0; i < pattern_len; i++) {
	br_diff = (i > 0) ? (1 << (i - 1)) : 0;
	brightness -= br_diff;
}

"1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"

"30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"

Above two can be handled by slightly modifying what I proposed above.

And for delta_t just require by default always 800.

> +	if (repeat > 0)
> +		return -EINVAL;
> +
> +	if (!led_pattern_cmp(cascade, ARRAY_SIZE(cascade), pattern, len)) {
> +		dev_dbg(led->priv->dev, "Cascade mode for '%s'", led->name);
> +		return el15203000_cmd(led, EL_PIPE_CASCADE);
> +	} else if (!led_pattern_cmp(inv_cascade, ARRAY_SIZE(inv_cascade), pattern, len)) {
> +		dev_dbg(led->priv->dev, "Inverse cascade mode for '%s'", led->name);
> +		return el15203000_cmd(led, EL_PIPE_INV_CASCADE);
> +	} else if (!led_pattern_cmp(bounce, ARRAY_SIZE(bounce), pattern, len)) {
> +		dev_dbg(led->priv->dev, "Bounce mode for '%s'", led->name);
> +		return el15203000_cmd(led, EL_PIPE_BOUNCE);
> +	} else if (!led_pattern_cmp(inv_bounce, ARRAY_SIZE(inv_bounce), pattern, len)) {
> +		dev_dbg(led->priv->dev, "Inverse bounce mode for '%s'", led->name);
> +		return el15203000_cmd(led, EL_PIPE_INV_BOUNCE);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +int el15203000_pattern_clear(struct led_classdev *ldev)
> +{
> +	struct el15203000_led	*led = container_of(ldev,
> +						    struct el15203000_led,
> +						    ldev);
> +
> +	return el15203000_cmd(led, EL_OFF);
> +}
> +
> +static int el15203000_probe_dt(struct el15203000 *priv)
> +{
> +	size_t			i = 0;
> +	struct el15203000_led	*led;
> +	struct fwnode_handle	*child;
> +	int			ret;
> +	const char		*str;
> +	u32			reg;
> +
> +	device_for_each_child_node(priv->dev, child) {
		struct led_init_data init_data = {};

Please add above declaration here.

> +		led = &priv->leds[i];
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret) {
> +			dev_err(priv->dev, "LED without ID number");
> +			fwnode_handle_put(child);
> +
> +			return ret;
> +		}
> +
> +		if (reg > U8_MAX) {
> +			dev_err(priv->dev, "LED value %d is invalid", reg);
> +			fwnode_handle_put(child);
> +
> +			return -EINVAL;
> +		}
> +
> +		led->reg = reg;
> +
> +		ret = fwnode_property_read_string(child, "label", &str);
> +		if (ret)
> +			str = ":";
> +		snprintf(led->name, sizeof(led->name), "el15203000:%s", str);

And instead of the four above lines please just do the below assignment:

		init_data.fwnode = child;

but let's do it right before calling devm_led_classdev_register_ext().

> +
> +		fwnode_property_read_string(child, "linux,default-trigger",
> +					    &led->ldev.default_trigger);
> +
> +		led->ldev.max_brightness	  = LED_ON;
> +		led->priv			  = priv;
> +		led->ldev.name			  = led->name;

Please also remove the above line.

> +		led->ldev.brightness_set_blocking = el15203000_set_blocking;
> +
> +		if (reg == 'S') {
> +			led->ldev.pattern_set	= el15203000_pattern_set_S;
> +			led->ldev.pattern_clear	= el15203000_pattern_clear;
> +		} else if (reg == 'P') {
> +			led->ldev.pattern_set	= el15203000_pattern_set_P;
> +			led->ldev.pattern_clear	= el15203000_pattern_clear;
> +		}
> +
> +		ret = devm_led_classdev_register(priv->dev, &led->ldev);

And here let's switch to the new LED registration API:

		ret = devm_led_classdev_register_ext(priv->dev, &led->ldev, &init_data);


> +		if (ret) {
> +			dev_err(priv->dev,
> +				"failed to register LED device %s, err %d",
> +				led->name, ret);
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		i++;
> +	}
> +
> +	return ret;
> +}
> +
> +static int el15203000_probe(struct spi_device *spi)
> +{
> +	struct el15203000	*priv;
> +	size_t			count;
> +	int			ret;
> +
> +	count = device_get_child_node_count(&spi->dev);
> +	if (!count) {
> +		dev_err(&spi->dev, "LEDs are not defined in device tree!");
> +		return -ENODEV;
> +	}
> +
> +	priv = devm_kzalloc(&spi->dev, struct_size(priv, leds, count),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mutex_init(&priv->lock);
> +	priv->count	= count;
> +	priv->dev	= &spi->dev;
> +	priv->spi	= spi;
> +	priv->delay	= jiffies -
> +			  usecs_to_jiffies(EL_FW_DELAY_USEC);
> +
> +	ret = el15203000_probe_dt(priv);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, priv);
> +	dev_dbg(priv->dev, "%zd LEDs registered", priv->count);

Please remove this debug log.

> +
> +	return 0;
> +}
> +
> +static int el15203000_remove(struct spi_device *spi)
> +{
> +	struct el15203000 *priv = spi_get_drvdata(spi);
> +
> +	mutex_destroy(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id el15203000_dt_ids[] = {
> +	{ .compatible = "crane,el15203000", },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, el15203000_dt_ids);
> +
> +static struct spi_driver el15203000_driver = {
> +	.probe		= el15203000_probe,
> +	.remove		= el15203000_remove,
> +	.driver = {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= el15203000_dt_ids,
> +	},
> +};
> +
> +module_spi_driver(el15203000_driver);
> +
> +MODULE_AUTHOR("Oleh Kravchenko <oleg@kaa.org.ua>");
> +MODULE_DESCRIPTION("el15203000 LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:el15203000");
> 

Please also fix the issues checkpatch.pl complains about.
Oleh Kravchenko Aug. 20, 2019, 7:46 a.m. UTC | #14
Hello Pavel,

17.08.19 18:02, Pavel Machek пише:
> Hi!
> 
>>>>>> All pattern is predefined, you can't change them at all.
>>>>>> I just tried to describe real things what happened in LED board.
>>>>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>>>>
>>>>> It makes me wonder how you figured out the values? If you have
>>>>> a documentation for this controller, could you share how the pattern
>>>>> settings are documented?
>>>>
>>>> I saw the code of firmware.
>>>> Not sure if I can find any documentation for it right now.
>>>
>>> Have you tried to alter the values? Or check what happens when
>>> the duplication is removed?
>>
>> What do you mean alter? It doesn't make any sense.
>> Board is accepts only brightness level from '0' to '5'.
>> I'm really confused :-)
> 
> For the record, what you did seems ok to me.

Thanks :)
 
>>>> Ok, I will reduce it.
>>>
>>> Please hold on for a while. I will have some more remarks to the driver,
>>> just collecting missing info for now to gain more complete view on this
>>> device.
>>
>> Here is the full story:
>>
>> EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
>> It's provide access to 3 LEDs:
>>
>> - First LED (Screen) is a light tube around big 21" screen
>>   It's have 3 brightness levels:
>>   * OFF
>>   * ON
>>   * Breathing mode (8 seconds full cycle)
>> - Second LED (Vending area) is highlight coffee cap
>>   * OFF
>>   * ON
> 
> Again, this is ok.
> 
>> - Third LED (Pipe) is actually virtual, because consists from 5 LEDs
>>   * OFF for all 5 LEDs
>>   * ON for all 5 LEDs
>>   * Cascade
>>   * Inverses cascade
>>   * Bounce
>>   * Inverses bounce
> 
> But having one virtual LED that is really five LEDs scares me a tiny
> bit. But I guess its ok for your case...
> 
> Best regards,
> 									Pavel
> 
> 
> 
>
Oleh Kravchenko Aug. 20, 2019, 7:50 a.m. UTC | #15
Hello Jacek,
thanks for your review.

18.08.19 16:41, Jacek Anaszewski пише:
> Hi Oleh,
> 
> On 8/8/19 10:32 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         | 47 +++++++++++++++++++
>>  1 file changed, 47 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..4c2245babfdc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
>> @@ -0,0 +1,47 @@
>> +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.
>> +
>> +Required properties:
>> +- compatible : "crane,el15203000"
>> +- reg :
>> +	see Documentation/devicetree/bindings/spi/spi-bus.txt
>> +- spi-max-frequency : (optional)
>> +	see Documentation/devicetree/bindings/spi/spi-bus.txt
>> +
>> +Optional LED sub-node properties:
>> +- label :
>> +	see Documentation/devicetree/bindings/leds/common.txt
> 
> Please change this label description to the below:
> 
> - 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).
> 
>> +- linux,default-trigger :
>> +	see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +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 */
>> +	pipe@50 {
> 
> s/pipe/led/
> 
>> +		reg = <0x50>;
>> +		label = "red:pipe";
> 
> label is now deprecated.
> 
> Please use function and color:
> 
> 	function = "pipe";
> 	color = <LED_COLOR_ID_RED>;
> 
> 
>> +	};
>> +
>> +	/* screen frame */
>> +	screen@53 {
> 
> s/screen/led/
> 
>> +		reg = <0x53>;
>> +		label = "red:screen";
> 
> 	function = "screen";
> 	color = <LED_COLOR_ID_RED>;
> 
>> +	};
>> +
>> +	/* vending area */
>> +	vend@56 {
> 
> s/vend/led/
> 
>> +		reg = <0x56>;
>> +		label = "red:vend";
> 
> 	function = "vend";
> 	color = <LED_COLOR_ID_RED>;
> 
>> +	};
>> +};
>>
>

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/leds/leds-el15203000.txt b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
new file mode 100644
index 000000000000..4c2245babfdc
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
@@ -0,0 +1,47 @@ 
+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.
+
+Required properties:
+- compatible : "crane,el15203000"
+- reg :
+	see Documentation/devicetree/bindings/spi/spi-bus.txt
+- spi-max-frequency : (optional)
+	see Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Optional LED sub-node properties:
+- label :
+	see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger :
+	see Documentation/devicetree/bindings/leds/common.txt
+
+Example
+-------
+
+led-controller@0 {
+	compatible = "crane,el15203000";
+	reg = <0>;
+	spi-max-frequency = <50000>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* water pipe */
+	pipe@50 {
+		reg = <0x50>;
+		label = "red:pipe";
+	};
+
+	/* screen frame */
+	screen@53 {
+		reg = <0x53>;
+		label = "red:screen";
+	};
+
+	/* vending area */
+	vend@56 {
+		reg = <0x56>;
+		label = "red:vend";
+	};
+};