[v3] leds: add LED driver for CR0014114 board

Message ID 20180313124548.12074-1-oleg@kaa.org.ua
State Changes Requested
Headers show
Series
  • [v3] leds: add LED driver for CR0014114 board
Related show

Commit Message

Oleh Kravchenko March 13, 2018, 12:45 p.m.
This patch adds a LED class driver for the RGB LEDs found on
the Crane Merchandising System CR0014114 LEDs board.

Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
---
 .../devicetree/bindings/leds/leds-cr0014114.txt    |  46 ++++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/leds/Kconfig                               |  13 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-cr0014114.c                      | 292 +++++++++++++++++++++
 5 files changed, 353 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-cr0014114.txt
 create mode 100644 drivers/leds/leds-cr0014114.c

Comments

Pavel Machek March 16, 2018, 9:14 p.m. | #1
On Tue 2018-03-13 14:45:48, Oleh Kravchenko wrote:
> This patch adds a LED class driver for the RGB LEDs found on
> the Crane Merchandising System CR0014114 LEDs board.
> 
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>

Acked-by: Pavel Machek <pavel@ucw.cz>

I just wonder if the recount stuff... could be somehow simplified.

									Pavel
Oleh Kravchenko March 16, 2018, 9:29 p.m. | #2
I can track state of recount stuff inside private struct,
it will allow to drop work and timer fields.

You will agree with it?

On 16.03.18 23:14, Pavel Machek wrote:
> On Tue 2018-03-13 14:45:48, Oleh Kravchenko wrote:
>> This patch adds a LED class driver for the RGB LEDs found on
>> the Crane Merchandising System CR0014114 LEDs board.
>>
>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> I just wonder if the recount stuff... could be somehow simplified.
> 
> 									Pavel
>
Pavel Machek March 16, 2018, 9:33 p.m. | #3
On Fri 2018-03-16 23:29:22, Oleh Kravchenko wrote:
> I can track state of recount stuff inside private struct,
> it will allow to drop work and timer fields.
> 
> You will agree with it?

That will not help much. Would it be possible to get away without the
timer, for example? Just schedule work for the future?

									Pavel
Oleh Kravchenko March 16, 2018, 9:40 p.m. | #4
Did you mean to do "recount" every 1000 brightness changes?

On 16.03.18 23:33, Pavel Machek wrote:
> On Fri 2018-03-16 23:29:22, Oleh Kravchenko wrote:
>> I can track state of recount stuff inside private struct,
>> it will allow to drop work and timer fields.
>>
>> You will agree with it?
> 
> That will not help much. Would it be possible to get away without the
> timer, for example? Just schedule work for the future?
> 
> 									Pavel
> 									
>
Pavel Machek March 16, 2018, 10:10 p.m. | #5
On Fri 2018-03-16 23:40:36, Oleh Kravchenko wrote:
> Did you mean to do "recount" every 1000 brightness changes?

If that works, yes.

My idea was... you are already using schedule_work, so why not
schedule_delayed_work() and get rid of timer?

https://static.lwn.net/images/pdf/LDD3/ch07.pdf

									Pavel
Rob Herring March 18, 2018, 12:49 p.m. | #6
On Tue, Mar 13, 2018 at 02:45:48PM +0200, Oleh Kravchenko wrote:
> This patch adds a LED class driver for the RGB LEDs found on
> the Crane Merchandising System CR0014114 LEDs board.
> 
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
>  .../devicetree/bindings/leds/leds-cr0014114.txt    |  46 ++++
>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +

Please split to separate patch.

>  drivers/leds/Kconfig                               |  13 +
>  drivers/leds/Makefile                              |   1 +
>  drivers/leds/leds-cr0014114.c                      | 292 +++++++++++++++++++++
>  5 files changed, 353 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>  create mode 100644 drivers/leds/leds-cr0014114.c
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-cr0014114.txt b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
> new file mode 100644
> index 000000000000..977a9929b04f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
> @@ -0,0 +1,46 @@
> +Crane Merchandising System - cr0014114 LED driver
> +-------------------------------------------------
> +
> +This LED Board is widely used in vending machines produced
> +by Crane Merchandising Systems.
> +
> +Required properties:
> +- compatible: "cms,cr0014114"
> +- reg: chip select address for the device
> +- spi-cpha: shifted clock phase mode is required
> +
> +LED sub-node properties:
> +- label : (optional)
> +	see Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger : (optional)
> +	see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example
> +-------
> +
> +cr0014114@0 {

leds@...

> +	compatible = "crane,cr0014114";
> +	reg = <0>;
> +	spi-max-frequency = <50000>;
> +	spi-cpha;
> +
> +	led0 {

Is '0' meaningful to the controller as an address/channel. If so, use 
reg.

> +		label = "cr0:red:";
> +	};
> +	led1 {
> +		label = "cr0:green:";
> +	};
> +	led2 {
> +		label = "cr0:blue:";
> +	};
> +	led3 {
> +		label = "cr1:red:";
> +	};
> +	led4 {
> +		label = "cr1:green:";
> +	};
> +	led5 {
> +		label = "cr1:blue:";
> +	};
> +	...
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index ae850d6c0ad3..f17949c365f5 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -75,6 +75,7 @@ cnxt	Conexant Systems, Inc.
>  compulab	CompuLab Ltd.
>  cortina	Cortina Systems, Inc.
>  cosmic	Cosmic Circuits
> +crane	Crane Connectivity Solutions
>  creative	Creative Technology Ltd
>  crystalfontz	Crystalfontz America, Inc.
>  cubietech	Cubietech, Ltd.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jacek Anaszewski March 18, 2018, 8:03 p.m. | #7
On 03/18/2018 01:49 PM, Rob Herring wrote:
> On Tue, Mar 13, 2018 at 02:45:48PM +0200, Oleh Kravchenko wrote:
>> This patch adds a LED class driver for the RGB LEDs found on
>> the Crane Merchandising System CR0014114 LEDs board.
>>
>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>> ---
>>  .../devicetree/bindings/leds/leds-cr0014114.txt    |  46 ++++
>>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
> 
> Please split to separate patch.
> 
>>  drivers/leds/Kconfig                               |  13 +
>>  drivers/leds/Makefile                              |   1 +
>>  drivers/leds/leds-cr0014114.c                      | 292 +++++++++++++++++++++
>>  5 files changed, 353 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>  create mode 100644 drivers/leds/leds-cr0014114.c
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-cr0014114.txt b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>> new file mode 100644
>> index 000000000000..977a9929b04f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>> @@ -0,0 +1,46 @@
>> +Crane Merchandising System - cr0014114 LED driver
>> +-------------------------------------------------
>> +
>> +This LED Board is widely used in vending machines produced
>> +by Crane Merchandising Systems.
>> +
>> +Required properties:
>> +- compatible: "cms,cr0014114"
>> +- reg: chip select address for the device
>> +- spi-cpha: shifted clock phase mode is required
>> +
>> +LED sub-node properties:
>> +- label : (optional)
>> +	see Documentation/devicetree/bindings/leds/common.txt
>> +- linux,default-trigger : (optional)
>> +	see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Example
>> +-------
>> +
>> +cr0014114@0 {
> 
> leds@...

In [0] you required it to be led-controller and the rest like below:


led-controller@12 {
  reg = <12>;

  led@0 {
    reg = <0>;
  };
  led@1 {
    reg = <1>;
  };
};


Since that time I started to require adhering to this naming pattern
for LED controller node and led@address for child nodes, where
applicable.

I plan on submitting a patch for common LED bindings, that will
switch device names to led-controller in DT examples.

With this scheme another problem arises, because now we have:

- label: The label for this LED. If omitted, the label is taken from
         the node name (excluding the unit address).

With that DT child node name is useless, because it is "led" for
each sub-led. Therefore I propose to make label property required,
and avoid using child DT node name as a fallback.

>> +	compatible = "crane,cr0014114";
>> +	reg = <0>;
>> +	spi-max-frequency = <50000>;
>> +	spi-cpha;
>> +
>> +	led0 {
> 
> Is '0' meaningful to the controller as an address/channel. If so, use 
> reg.
> 
>> +		label = "cr0:red:";
>> +	};
>> +	led1 {
>> +		label = "cr0:green:";
>> +	};
>> +	led2 {
>> +		label = "cr0:blue:";
>> +	};
>> +	led3 {
>> +		label = "cr1:red:";
>> +	};
>> +	led4 {
>> +		label = "cr1:green:";
>> +	};
>> +	led5 {
>> +		label = "cr1:blue:";
>> +	};
>> +	...
>> +};
>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> index ae850d6c0ad3..f17949c365f5 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> @@ -75,6 +75,7 @@ cnxt	Conexant Systems, Inc.
>>  compulab	CompuLab Ltd.
>>  cortina	Cortina Systems, Inc.
>>  cosmic	Cosmic Circuits
>> +crane	Crane Connectivity Solutions
>>  creative	Creative Technology Ltd
>>  crystalfontz	Crystalfontz America, Inc.
>>  cubietech	Cubietech, Ltd.
> 

[0] https://patchwork.kernel.org/patch/10093757/
Rob Herring March 18, 2018, 11:38 p.m. | #8
On Sun, Mar 18, 2018 at 3:03 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> On 03/18/2018 01:49 PM, Rob Herring wrote:
>> On Tue, Mar 13, 2018 at 02:45:48PM +0200, Oleh Kravchenko wrote:
>>> This patch adds a LED class driver for the RGB LEDs found on
>>> the Crane Merchandising System CR0014114 LEDs board.
>>>
>>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>>> ---
>>>  .../devicetree/bindings/leds/leds-cr0014114.txt    |  46 ++++
>>>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>>
>> Please split to separate patch.
>>
>>>  drivers/leds/Kconfig                               |  13 +
>>>  drivers/leds/Makefile                              |   1 +
>>>  drivers/leds/leds-cr0014114.c                      | 292 +++++++++++++++++++++
>>>  5 files changed, 353 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>  create mode 100644 drivers/leds/leds-cr0014114.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-cr0014114.txt b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>> new file mode 100644
>>> index 000000000000..977a9929b04f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>> @@ -0,0 +1,46 @@
>>> +Crane Merchandising System - cr0014114 LED driver
>>> +-------------------------------------------------
>>> +
>>> +This LED Board is widely used in vending machines produced
>>> +by Crane Merchandising Systems.
>>> +
>>> +Required properties:
>>> +- compatible: "cms,cr0014114"
>>> +- reg: chip select address for the device
>>> +- spi-cpha: shifted clock phase mode is required
>>> +
>>> +LED sub-node properties:
>>> +- label : (optional)
>>> +    see Documentation/devicetree/bindings/leds/common.txt
>>> +- linux,default-trigger : (optional)
>>> +    see Documentation/devicetree/bindings/leds/common.txt
>>> +
>>> +Example
>>> +-------
>>> +
>>> +cr0014114@0 {
>>
>> leds@...
>
> In [0] you required it to be led-controller and the rest like below:

Ah yes, you're right. That's what I get for trying to go off my memory.

>
>
> led-controller@12 {
>   reg = <12>;
>
>   led@0 {
>     reg = <0>;
>   };
>   led@1 {
>     reg = <1>;
>   };
> };
>
>
> Since that time I started to require adhering to this naming pattern
> for LED controller node and led@address for child nodes, where
> applicable.
>
> I plan on submitting a patch for common LED bindings, that will
> switch device names to led-controller in DT examples.
>
> With this scheme another problem arises, because now we have:
>
> - label: The label for this LED. If omitted, the label is taken from
>          the node name (excluding the unit address).
>
> With that DT child node name is useless, because it is "led" for
> each sub-led. Therefore I propose to make label property required,
> and avoid using child DT node name as a fallback.

There was never any guarantee that the same child node name was not
used elsewhere.

I think the OS needs to be able to cope with no label or even that
label is not unique. Suppose you have an overlay for a addon board and
you can have multiple boards connected. They'd all have the same
label. So moving to label only reduces the problem. You could use the
reg property and/or compatible of the parent to construct the names.
Or just append an IDR value to the name like we do on platform
devices. If the user didn't specify a label, then they shouldn't
really care what the name is, so just use "led.X". Though likely folks
will care if the name changes on them.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jacek Anaszewski March 19, 2018, 9:18 p.m. | #9
On 03/19/2018 12:38 AM, Rob Herring wrote:
> On Sun, Mar 18, 2018 at 3:03 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> On 03/18/2018 01:49 PM, Rob Herring wrote:
>>> On Tue, Mar 13, 2018 at 02:45:48PM +0200, Oleh Kravchenko wrote:
>>>> This patch adds a LED class driver for the RGB LEDs found on
>>>> the Crane Merchandising System CR0014114 LEDs board.
>>>>
>>>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>>>> ---
>>>>  .../devicetree/bindings/leds/leds-cr0014114.txt    |  46 ++++
>>>>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>>>
>>> Please split to separate patch.
>>>
>>>>  drivers/leds/Kconfig                               |  13 +
>>>>  drivers/leds/Makefile                              |   1 +
>>>>  drivers/leds/leds-cr0014114.c                      | 292 +++++++++++++++++++++
>>>>  5 files changed, 353 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>>  create mode 100644 drivers/leds/leds-cr0014114.c
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-cr0014114.txt b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>> new file mode 100644
>>>> index 000000000000..977a9929b04f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>> @@ -0,0 +1,46 @@
>>>> +Crane Merchandising System - cr0014114 LED driver
>>>> +-------------------------------------------------
>>>> +
>>>> +This LED Board is widely used in vending machines produced
>>>> +by Crane Merchandising Systems.
>>>> +
>>>> +Required properties:
>>>> +- compatible: "cms,cr0014114"
>>>> +- reg: chip select address for the device
>>>> +- spi-cpha: shifted clock phase mode is required
>>>> +
>>>> +LED sub-node properties:
>>>> +- label : (optional)
>>>> +    see Documentation/devicetree/bindings/leds/common.txt
>>>> +- linux,default-trigger : (optional)
>>>> +    see Documentation/devicetree/bindings/leds/common.txt
>>>> +
>>>> +Example
>>>> +-------
>>>> +
>>>> +cr0014114@0 {
>>>
>>> leds@...
>>
>> In [0] you required it to be led-controller and the rest like below:
> 
> Ah yes, you're right. That's what I get for trying to go off my memory.
> 
>>
>>
>> led-controller@12 {
>>   reg = <12>;
>>
>>   led@0 {
>>     reg = <0>;
>>   };
>>   led@1 {
>>     reg = <1>;
>>   };
>> };
>>
>>
>> Since that time I started to require adhering to this naming pattern
>> for LED controller node and led@address for child nodes, where
>> applicable.
>>
>> I plan on submitting a patch for common LED bindings, that will
>> switch device names to led-controller in DT examples.
>>
>> With this scheme another problem arises, because now we have:
>>
>> - label: The label for this LED. If omitted, the label is taken from
>>          the node name (excluding the unit address).
>>
>> With that DT child node name is useless, because it is "led" for
>> each sub-led. Therefore I propose to make label property required,
>> and avoid using child DT node name as a fallback.
> 
> There was never any guarantee that the same child node name was not
> used elsewhere.
> 
> I think the OS needs to be able to cope with no label or even that
> label is not unique. Suppose you have an overlay for a addon board and
> you can have multiple boards connected. They'd all have the same
> label.

LED subsystem is already protected against duplicated LED names:
a96aa64cb572 ("leds/led-class: Handle LEDs with the same name").

> So moving to label only reduces the problem. You could use the
> reg property and/or compatible of the parent to construct the names.
> Or just append an IDR value to the name like we do on platform
> devices. If the user didn't specify a label, then they shouldn't
> really care what the name is, so just use "led.X". Though likely folks
> will care if the name changes on them.

OK, so my main intention was to get rid of the part:

"If omitted, the label is taken from the node name (excluding
the unit address).",

because it won't make a sense after imposing an explicit requirement
on the child nodes to have only "led" in the main part of their names.

In effect, we need to change the fallback definition to make it sane.
You proposed reg property and/or compatible, but:

- reg property is not applicable in all cases (think of gpio LEDs).
- compatible on the other hand will have much in common with devicename,
  you once mentioned would be good to get rid of [0]:

"
A case I care about is I have a family of boards that all have a
common defined set of 4 LEDs. They could be driven by anything, but I
want the same interface presented to userspace across boards.
"

We also had a discussion about standardized LED function names, so I
retrieved all LED function names from dts files present in kernel,
and after unifying some similar occurrences I got something like
below. Perhaps it doesn't make a sense to add all of the below
definitions to the standard header, but some of them could certainly
find their place in e.g. include/dt-bindings/leds/led-functions.h.

Then we could have LED names in the form function:color, and below
defines could be used in dts files to avoid spawning similar LED names
describing the same function.

Of course in case of the names presented here with numerical suffixes
we could skip the suffix.


#define LED_FUNCTION_ACTIVITY "activity"
#define LED_FUNCTION_ADSL "adsl"
#define LED_FUNCTION_ALARM "alarm"
#define LED_FUNCTION_ALIVE "alive"
#define LED_FUNCTION_ALL "all"
#define LED_FUNCTION_APP "app"
#define LED_FUNCTION_AUX "aux"
#define LED_FUNCTION_BACKUP "backup"
#define LED_FUNCTION_BEEP "beep"
#define LED_FUNCTION_BLUETOOTH "bluetooth"
#define LED_FUNCTION_BOOT "boot"
#define LED_FUNCTION_BOTTOM "bottom"
#define LED_FUNCTION_BRICK-STATUS "brick-status"
#define LED_FUNCTION_BT "bt"
#define LED_FUNCTION_CEL "cel"
#define LED_FUNCTION_CEL-PWR "cel-pwr"
#define LED_FUNCTION_CEL-RESET "cel-reset"
#define LED_FUNCTION_CHRG "chrg"
#define LED_FUNCTION_COM "com"
#define LED_FUNCTION_COPY "copy"
#define LED_FUNCTION_CORE_MODULE "core_module"
#define LED_FUNCTION_CPU "cpu"
#define LED_FUNCTION_D "d"
#define LED_FUNCTION_DEBUG "debug"
#define LED_FUNCTION_DIA "dia"
#define LED_FUNCTION_DISK "disk"
#define LED_FUNCTION_DOWN "down"
#define LED_FUNCTION_DSL "dsl"
#define LED_FUNCTION_ENOCEAN "enocean"
#define LED_FUNCTION_ENTER "enter"
#define LED_FUNCTION_ERROR "error"
#define LED_FUNCTION_ESATA "esata"
#define LED_FUNCTION_ETHERNET-STATUS "ethernet-status"
#define LED_FUNCTION_FAIL "fail"
#define LED_FUNCTION_FAULT "fault"
#define LED_FUNCTION_FRONT "front"
#define LED_FUNCTION_FUNC "func"
#define LED_FUNCTION_G "g"
#define LED_FUNCTION_GHZ "ghz"
#define LED_FUNCTION_GHZ-1 "ghz-1"
#define LED_FUNCTION_GHZ-2 "ghz-2"
#define LED_FUNCTION_GPIO "gpio"
#define LED_FUNCTION_GSM "gsm"
#define LED_FUNCTION_HD "hd"
#define LED_FUNCTION_HDD "hdd"
#define LED_FUNCTION_HDDERR "hdderr"
#define LED_FUNCTION_HEALTH "health"
#define LED_FUNCTION_HEART "heart"
#define LED_FUNCTION_HEARTBEAT "heartbeat"
#define LED_FUNCTION_HOME "home"
#define LED_FUNCTION_INET "inet"
#define LED_FUNCTION_INFO "info"
#define LED_FUNCTION_INTERNET "internet"
#define LED_FUNCTION_KEYPAD "keypad"
#define LED_FUNCTION_L "l"
#define LED_FUNCTION_LAN "lan"
#define LED_FUNCTION_LEDB "ledb"
#define LED_FUNCTION_LEFT "left"
#define LED_FUNCTION_L_HDD "l_hdd"
#define LED_FUNCTION_LIVE "live"
#define LED_FUNCTION_LOGO "logo"
#define LED_FUNCTION_MICROSD "microsd"
#define LED_FUNCTION_MISC "misc"
#define LED_FUNCTION_MMC "mmc"
#define LED_FUNCTION_NAND "nand"
#define LED_FUNCTION_NETWORK "network"
#define LED_FUNCTION_ON "on"
#define LED_FUNCTION_ORANGE "orange"
#define LED_FUNCTION_OS "os"
#define LED_FUNCTION_PANEL "panel"
#define LED_FUNCTION_PMU_STAT "pmu_stat"
#define LED_FUNCTION_POWER "power"
#define LED_FUNCTION_PROG "prog"
#define LED_FUNCTION_PROGRAMMING "programming"
#define LED_FUNCTION_PROXIMITYSENSOR "proximitysensor"
#define LED_FUNCTION_PULSE "pulse"
#define LED_FUNCTION_PWR "pwr"
#define LED_FUNCTION_QSS "qss"
#define LED_FUNCTION_REBUILD "rebuild"
#define LED_FUNCTION_R_HDD "r_hdd"
#define LED_FUNCTION_RIGHT "right"
#define LED_FUNCTION_ROUTER "router"
#define LED_FUNCTION_RS "rs"
#define LED_FUNCTION_RX "rx"
#define LED_FUNCTION_SATA "sata"
#define LED_FUNCTION_SATA-L "sata-l"
#define LED_FUNCTION_SATA-R "sata-r"
#define LED_FUNCTION_SD "sd"
#define LED_FUNCTION_SLEEP "sleep"
#define LED_FUNCTION_STANDBY "standby"
#define LED_FUNCTION_STATE "state"
#define LED_FUNCTION_STATUS "status"
#define LED_FUNCTION_SW "sw"
#define LED_FUNCTION_SWRDY "swrdy"
#define LED_FUNCTION_SYS "sys"
#define LED_FUNCTION_SYSTEM "system"
#define LED_FUNCTION_SYSTEM-STATUS "system-status"
#define LED_FUNCTION_TEL "tel"
#define LED_FUNCTION_TOP "top"
#define LED_FUNCTION_TV "tv"
#define LED_FUNCTION_TX "tx"
#define LED_FUNCTION_UP "up"
#define LED_FUNCTION_USB "usb"
#define LED_FUNCTION_USB_1 "usb_1"
#define LED_FUNCTION_USB_2 "usb_2"
#define LED_FUNCTION_USB_COPY "usb_copy"
#define LED_FUNCTION_USB-PORT1 "usb-port1"
#define LED_FUNCTION_USB-PORT2 "usb-port2"
#define LED_FUNCTION_USER "user"
#define LED_FUNCTION_USR "usr"
#define LED_FUNCTION_WAN "wan"
#define LED_FUNCTION_WIFI "wifi"
#define LED_FUNCTION_WIFI_AP "wifi_ap"
#define LED_FUNCTION_WIFI-STATUS "wifi-status"
#define LED_FUNCTION_WIRELESS "wireless"
#define LED_FUNCTION_WLAN "wlan"
#define LED_FUNCTION_WLAN_G "wlan_g"
#define LED_FUNCTION_WMODE "wmode"
#define LED_FUNCTION_WPS "wps"


[0] https://patchwork.kernel.org/patch/10089047/
Rob Herring March 27, 2018, 3:31 p.m. | #10
On Mon, Mar 19, 2018 at 4:18 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> On 03/19/2018 12:38 AM, Rob Herring wrote:
>> On Sun, Mar 18, 2018 at 3:03 PM, Jacek Anaszewski
>> <jacek.anaszewski@gmail.com> wrote:
>>> On 03/18/2018 01:49 PM, Rob Herring wrote:
>>>> On Tue, Mar 13, 2018 at 02:45:48PM +0200, Oleh Kravchenko wrote:
>>>>> This patch adds a LED class driver for the RGB LEDs found on
>>>>> the Crane Merchandising System CR0014114 LEDs board.
>>>>>
>>>>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>>>>> ---
>>>>>  .../devicetree/bindings/leds/leds-cr0014114.txt    |  46 ++++
>>>>>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>>>>
>>>> Please split to separate patch.
>>>>
>>>>>  drivers/leds/Kconfig                               |  13 +
>>>>>  drivers/leds/Makefile                              |   1 +
>>>>>  drivers/leds/leds-cr0014114.c                      | 292 +++++++++++++++++++++
>>>>>  5 files changed, 353 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>>>  create mode 100644 drivers/leds/leds-cr0014114.c
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-cr0014114.txt b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>>> new file mode 100644
>>>>> index 000000000000..977a9929b04f
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>>> @@ -0,0 +1,46 @@
>>>>> +Crane Merchandising System - cr0014114 LED driver
>>>>> +-------------------------------------------------
>>>>> +
>>>>> +This LED Board is widely used in vending machines produced
>>>>> +by Crane Merchandising Systems.
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: "cms,cr0014114"
>>>>> +- reg: chip select address for the device
>>>>> +- spi-cpha: shifted clock phase mode is required
>>>>> +
>>>>> +LED sub-node properties:
>>>>> +- label : (optional)
>>>>> +    see Documentation/devicetree/bindings/leds/common.txt
>>>>> +- linux,default-trigger : (optional)
>>>>> +    see Documentation/devicetree/bindings/leds/common.txt
>>>>> +
>>>>> +Example
>>>>> +-------
>>>>> +
>>>>> +cr0014114@0 {
>>>>
>>>> leds@...
>>>
>>> In [0] you required it to be led-controller and the rest like below:
>>
>> Ah yes, you're right. That's what I get for trying to go off my memory.
>>
>>>
>>>
>>> led-controller@12 {
>>>   reg = <12>;
>>>
>>>   led@0 {
>>>     reg = <0>;
>>>   };
>>>   led@1 {
>>>     reg = <1>;
>>>   };
>>> };
>>>
>>>
>>> Since that time I started to require adhering to this naming pattern
>>> for LED controller node and led@address for child nodes, where
>>> applicable.
>>>
>>> I plan on submitting a patch for common LED bindings, that will
>>> switch device names to led-controller in DT examples.
>>>
>>> With this scheme another problem arises, because now we have:
>>>
>>> - label: The label for this LED. If omitted, the label is taken from
>>>          the node name (excluding the unit address).
>>>
>>> With that DT child node name is useless, because it is "led" for
>>> each sub-led. Therefore I propose to make label property required,
>>> and avoid using child DT node name as a fallback.
>>
>> There was never any guarantee that the same child node name was not
>> used elsewhere.
>>
>> I think the OS needs to be able to cope with no label or even that
>> label is not unique. Suppose you have an overlay for a addon board and
>> you can have multiple boards connected. They'd all have the same
>> label.
>
> LED subsystem is already protected against duplicated LED names:
> a96aa64cb572 ("leds/led-class: Handle LEDs with the same name").
>
>> So moving to label only reduces the problem. You could use the
>> reg property and/or compatible of the parent to construct the names.
>> Or just append an IDR value to the name like we do on platform
>> devices. If the user didn't specify a label, then they shouldn't
>> really care what the name is, so just use "led.X". Though likely folks
>> will care if the name changes on them.
>
> OK, so my main intention was to get rid of the part:
>
> "If omitted, the label is taken from the node name (excluding
> the unit address).",
>
> because it won't make a sense after imposing an explicit requirement
> on the child nodes to have only "led" in the main part of their names.

Okay, agreed. That statement never really belonged because really the
OS is free to do whatever it wants.

> In effect, we need to change the fallback definition to make it sane.
> You proposed reg property and/or compatible, but:
>
> - reg property is not applicable in all cases (think of gpio LEDs).
> - compatible on the other hand will have much in common with devicename,
>   you once mentioned would be good to get rid of [0]:
>
> "
> A case I care about is I have a family of boards that all have a
> common defined set of 4 LEDs. They could be driven by anything, but I
> want the same interface presented to userspace across boards.
> "

Yes, and "label" should provide me with that ability as the context
was the practice of putting the controller name in the label.

> We also had a discussion about standardized LED function names, so I
> retrieved all LED function names from dts files present in kernel,
> and after unifying some similar occurrences I got something like
> below. Perhaps it doesn't make a sense to add all of the below
> definitions to the standard header, but some of them could certainly
> find their place in e.g. include/dt-bindings/leds/led-functions.h.

Looks like we could further unify things though I suppose just
changing dts files could break users. But at least we could document
which ones to use for new dts files. Then hopefully we don't have more
"bluetooth" vs. "bt".

Yes, I'd certainly limit the list to ones where we have multiple
similar names and leave out the oddballs.

> Then we could have LED names in the form function:color, and below
> defines could be used in dts files to avoid spawning similar LED names
> describing the same function.
>
> Of course in case of the names presented here with numerical suffixes
> we could skip the suffix.
>
>
> #define LED_FUNCTION_ACTIVITY "activity"
> #define LED_FUNCTION_ADSL "adsl"
> #define LED_FUNCTION_ALARM "alarm"
> #define LED_FUNCTION_ALIVE "alive"
> #define LED_FUNCTION_ALL "all"
> #define LED_FUNCTION_APP "app"
> #define LED_FUNCTION_AUX "aux"
> #define LED_FUNCTION_BACKUP "backup"
> #define LED_FUNCTION_BEEP "beep"
> #define LED_FUNCTION_BLUETOOTH "bluetooth"
> #define LED_FUNCTION_BOOT "boot"
> #define LED_FUNCTION_BOTTOM "bottom"
> #define LED_FUNCTION_BRICK-STATUS "brick-status"
> #define LED_FUNCTION_BT "bt"
> #define LED_FUNCTION_CEL "cel"
> #define LED_FUNCTION_CEL-PWR "cel-pwr"
> #define LED_FUNCTION_CEL-RESET "cel-reset"
> #define LED_FUNCTION_CHRG "chrg"
> #define LED_FUNCTION_COM "com"
> #define LED_FUNCTION_COPY "copy"
> #define LED_FUNCTION_CORE_MODULE "core_module"
> #define LED_FUNCTION_CPU "cpu"
> #define LED_FUNCTION_D "d"
> #define LED_FUNCTION_DEBUG "debug"
> #define LED_FUNCTION_DIA "dia"
> #define LED_FUNCTION_DISK "disk"
> #define LED_FUNCTION_DOWN "down"
> #define LED_FUNCTION_DSL "dsl"
> #define LED_FUNCTION_ENOCEAN "enocean"
> #define LED_FUNCTION_ENTER "enter"
> #define LED_FUNCTION_ERROR "error"
> #define LED_FUNCTION_ESATA "esata"
> #define LED_FUNCTION_ETHERNET-STATUS "ethernet-status"
> #define LED_FUNCTION_FAIL "fail"
> #define LED_FUNCTION_FAULT "fault"
> #define LED_FUNCTION_FRONT "front"
> #define LED_FUNCTION_FUNC "func"
> #define LED_FUNCTION_G "g"
> #define LED_FUNCTION_GHZ "ghz"
> #define LED_FUNCTION_GHZ-1 "ghz-1"
> #define LED_FUNCTION_GHZ-2 "ghz-2"
> #define LED_FUNCTION_GPIO "gpio"
> #define LED_FUNCTION_GSM "gsm"
> #define LED_FUNCTION_HD "hd"
> #define LED_FUNCTION_HDD "hdd"
> #define LED_FUNCTION_HDDERR "hdderr"
> #define LED_FUNCTION_HEALTH "health"
> #define LED_FUNCTION_HEART "heart"
> #define LED_FUNCTION_HEARTBEAT "heartbeat"
> #define LED_FUNCTION_HOME "home"
> #define LED_FUNCTION_INET "inet"
> #define LED_FUNCTION_INFO "info"
> #define LED_FUNCTION_INTERNET "internet"
> #define LED_FUNCTION_KEYPAD "keypad"
> #define LED_FUNCTION_L "l"
> #define LED_FUNCTION_LAN "lan"
> #define LED_FUNCTION_LEDB "ledb"
> #define LED_FUNCTION_LEFT "left"
> #define LED_FUNCTION_L_HDD "l_hdd"
> #define LED_FUNCTION_LIVE "live"
> #define LED_FUNCTION_LOGO "logo"
> #define LED_FUNCTION_MICROSD "microsd"
> #define LED_FUNCTION_MISC "misc"
> #define LED_FUNCTION_MMC "mmc"
> #define LED_FUNCTION_NAND "nand"
> #define LED_FUNCTION_NETWORK "network"
> #define LED_FUNCTION_ON "on"
> #define LED_FUNCTION_ORANGE "orange"
> #define LED_FUNCTION_OS "os"
> #define LED_FUNCTION_PANEL "panel"
> #define LED_FUNCTION_PMU_STAT "pmu_stat"
> #define LED_FUNCTION_POWER "power"
> #define LED_FUNCTION_PROG "prog"
> #define LED_FUNCTION_PROGRAMMING "programming"
> #define LED_FUNCTION_PROXIMITYSENSOR "proximitysensor"
> #define LED_FUNCTION_PULSE "pulse"
> #define LED_FUNCTION_PWR "pwr"
> #define LED_FUNCTION_QSS "qss"
> #define LED_FUNCTION_REBUILD "rebuild"
> #define LED_FUNCTION_R_HDD "r_hdd"
> #define LED_FUNCTION_RIGHT "right"
> #define LED_FUNCTION_ROUTER "router"
> #define LED_FUNCTION_RS "rs"
> #define LED_FUNCTION_RX "rx"
> #define LED_FUNCTION_SATA "sata"
> #define LED_FUNCTION_SATA-L "sata-l"
> #define LED_FUNCTION_SATA-R "sata-r"
> #define LED_FUNCTION_SD "sd"
> #define LED_FUNCTION_SLEEP "sleep"
> #define LED_FUNCTION_STANDBY "standby"
> #define LED_FUNCTION_STATE "state"
> #define LED_FUNCTION_STATUS "status"
> #define LED_FUNCTION_SW "sw"
> #define LED_FUNCTION_SWRDY "swrdy"
> #define LED_FUNCTION_SYS "sys"
> #define LED_FUNCTION_SYSTEM "system"
> #define LED_FUNCTION_SYSTEM-STATUS "system-status"
> #define LED_FUNCTION_TEL "tel"
> #define LED_FUNCTION_TOP "top"
> #define LED_FUNCTION_TV "tv"
> #define LED_FUNCTION_TX "tx"
> #define LED_FUNCTION_UP "up"
> #define LED_FUNCTION_USB "usb"
> #define LED_FUNCTION_USB_1 "usb_1"
> #define LED_FUNCTION_USB_2 "usb_2"
> #define LED_FUNCTION_USB_COPY "usb_copy"
> #define LED_FUNCTION_USB-PORT1 "usb-port1"
> #define LED_FUNCTION_USB-PORT2 "usb-port2"
> #define LED_FUNCTION_USER "user"
> #define LED_FUNCTION_USR "usr"
> #define LED_FUNCTION_WAN "wan"
> #define LED_FUNCTION_WIFI "wifi"
> #define LED_FUNCTION_WIFI_AP "wifi_ap"
> #define LED_FUNCTION_WIFI-STATUS "wifi-status"
> #define LED_FUNCTION_WIRELESS "wireless"
> #define LED_FUNCTION_WLAN "wlan"
> #define LED_FUNCTION_WLAN_G "wlan_g"
> #define LED_FUNCTION_WMODE "wmode"
> #define LED_FUNCTION_WPS "wps"
>
>
> [0] https://patchwork.kernel.org/patch/10089047/
> --
> Best regards,
> Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jacek Anaszewski March 30, 2018, 10:53 a.m. | #11
On 03/27/2018 05:31 PM, Rob Herring wrote:
> On Mon, Mar 19, 2018 at 4:18 PM, Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> On 03/19/2018 12:38 AM, Rob Herring wrote:
>>> On Sun, Mar 18, 2018 at 3:03 PM, Jacek Anaszewski
>>> <jacek.anaszewski@gmail.com> wrote:
>>>> On 03/18/2018 01:49 PM, Rob Herring wrote:
>>>>> On Tue, Mar 13, 2018 at 02:45:48PM +0200, Oleh Kravchenko wrote:
>>>>>> This patch adds a LED class driver for the RGB LEDs found on
>>>>>> the Crane Merchandising System CR0014114 LEDs board.
>>>>>>
>>>>>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>>>>>> ---
>>>>>>  .../devicetree/bindings/leds/leds-cr0014114.txt    |  46 ++++
>>>>>>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
>>>>>
>>>>> Please split to separate patch.
>>>>>
>>>>>>  drivers/leds/Kconfig                               |  13 +
>>>>>>  drivers/leds/Makefile                              |   1 +
>>>>>>  drivers/leds/leds-cr0014114.c                      | 292 +++++++++++++++++++++
>>>>>>  5 files changed, 353 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>>>>  create mode 100644 drivers/leds/leds-cr0014114.c
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-cr0014114.txt b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..977a9929b04f
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
>>>>>> @@ -0,0 +1,46 @@
>>>>>> +Crane Merchandising System - cr0014114 LED driver
>>>>>> +-------------------------------------------------
>>>>>> +
>>>>>> +This LED Board is widely used in vending machines produced
>>>>>> +by Crane Merchandising Systems.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: "cms,cr0014114"
>>>>>> +- reg: chip select address for the device
>>>>>> +- spi-cpha: shifted clock phase mode is required
>>>>>> +
>>>>>> +LED sub-node properties:
>>>>>> +- label : (optional)
>>>>>> +    see Documentation/devicetree/bindings/leds/common.txt
>>>>>> +- linux,default-trigger : (optional)
>>>>>> +    see Documentation/devicetree/bindings/leds/common.txt
>>>>>> +
>>>>>> +Example
>>>>>> +-------
>>>>>> +
>>>>>> +cr0014114@0 {
>>>>>
>>>>> leds@...
>>>>
>>>> In [0] you required it to be led-controller and the rest like below:
>>>
>>> Ah yes, you're right. That's what I get for trying to go off my memory.
>>>
>>>>
>>>>
>>>> led-controller@12 {
>>>>   reg = <12>;
>>>>
>>>>   led@0 {
>>>>     reg = <0>;
>>>>   };
>>>>   led@1 {
>>>>     reg = <1>;
>>>>   };
>>>> };
>>>>
>>>>
>>>> Since that time I started to require adhering to this naming pattern
>>>> for LED controller node and led@address for child nodes, where
>>>> applicable.
>>>>
>>>> I plan on submitting a patch for common LED bindings, that will
>>>> switch device names to led-controller in DT examples.
>>>>
>>>> With this scheme another problem arises, because now we have:
>>>>
>>>> - label: The label for this LED. If omitted, the label is taken from
>>>>          the node name (excluding the unit address).
>>>>
>>>> With that DT child node name is useless, because it is "led" for
>>>> each sub-led. Therefore I propose to make label property required,
>>>> and avoid using child DT node name as a fallback.
>>>
>>> There was never any guarantee that the same child node name was not
>>> used elsewhere.
>>>
>>> I think the OS needs to be able to cope with no label or even that
>>> label is not unique. Suppose you have an overlay for a addon board and
>>> you can have multiple boards connected. They'd all have the same
>>> label.
>>
>> LED subsystem is already protected against duplicated LED names:
>> a96aa64cb572 ("leds/led-class: Handle LEDs with the same name").
>>
>>> So moving to label only reduces the problem. You could use the
>>> reg property and/or compatible of the parent to construct the names.
>>> Or just append an IDR value to the name like we do on platform
>>> devices. If the user didn't specify a label, then they shouldn't
>>> really care what the name is, so just use "led.X". Though likely folks
>>> will care if the name changes on them.
>>
>> OK, so my main intention was to get rid of the part:
>>
>> "If omitted, the label is taken from the node name (excluding
>> the unit address).",
>>
>> because it won't make a sense after imposing an explicit requirement
>> on the child nodes to have only "led" in the main part of their names.
> 
> Okay, agreed. That statement never really belonged because really the
> OS is free to do whatever it wants.

It implies that label property should be made required.

>> In effect, we need to change the fallback definition to make it sane.
>> You proposed reg property and/or compatible, but:
>>
>> - reg property is not applicable in all cases (think of gpio LEDs).
>> - compatible on the other hand will have much in common with devicename,
>>   you once mentioned would be good to get rid of [0]:
>>
>> "
>> A case I care about is I have a family of boards that all have a
>> common defined set of 4 LEDs. They could be driven by anything, but I
>> want the same interface presented to userspace across boards.
>> "
> 
> Yes, and "label" should provide me with that ability as the context
> was the practice of putting the controller name in the label.

Exactly. There are two options:

1.
- label format : "<color>:<function"
2.
- label format : "<function>"
- new "color" DT property

>> We also had a discussion about standardized LED function names, so I
>> retrieved all LED function names from dts files present in kernel,
>> and after unifying some similar occurrences I got something like
>> below. Perhaps it doesn't make a sense to add all of the below
>> definitions to the standard header, but some of them could certainly
>> find their place in e.g. include/dt-bindings/leds/led-functions.h.
> 
> Looks like we could further unify things though I suppose just
> changing dts files could break users.

The changes in dts files would have to be accompanied by changes in
DT parsing part of LED class drivers. Also, I wonder if it wouldn't
make sense to add config option to choose between old and new LED
naming convention to avoid userspace breakage.

It would allow to use dts files with old style labels with
newer kernels. The LED class device names would differ like below:

Old style name (current):
	/sys/class/leds/netxbig:red:power

New style name:
	/sys/class/leds/red:power (we could also think of dropping
                                   color from name and adding sysfs
                                   file for it)


> But at least we could document
> which ones to use for new dts files. Then hopefully we don't have more
> "bluetooth" vs. "bt".
> 
> Yes, I'd certainly limit the list to ones where we have multiple
> similar names and leave out the oddballs.
> 
>> Then we could have LED names in the form function:color, and below
>> defines could be used in dts files to avoid spawning similar LED names
>> describing the same function.
>>
>> Of course in case of the names presented here with numerical suffixes
>> we could skip the suffix.
>>
>>
>> #define LED_FUNCTION_ACTIVITY "activity"
>> #define LED_FUNCTION_ADSL "adsl"
>> #define LED_FUNCTION_ALARM "alarm"
>> #define LED_FUNCTION_ALIVE "alive"
>> #define LED_FUNCTION_ALL "all"
>> #define LED_FUNCTION_APP "app"
>> #define LED_FUNCTION_AUX "aux"
>> #define LED_FUNCTION_BACKUP "backup"
>> #define LED_FUNCTION_BEEP "beep"
>> #define LED_FUNCTION_BLUETOOTH "bluetooth"
>> #define LED_FUNCTION_BOOT "boot"
>> #define LED_FUNCTION_BOTTOM "bottom"
>> #define LED_FUNCTION_BRICK-STATUS "brick-status"
>> #define LED_FUNCTION_BT "bt"
>> #define LED_FUNCTION_CEL "cel"
>> #define LED_FUNCTION_CEL-PWR "cel-pwr"
>> #define LED_FUNCTION_CEL-RESET "cel-reset"
>> #define LED_FUNCTION_CHRG "chrg"
>> #define LED_FUNCTION_COM "com"
>> #define LED_FUNCTION_COPY "copy"
>> #define LED_FUNCTION_CORE_MODULE "core_module"
>> #define LED_FUNCTION_CPU "cpu"
>> #define LED_FUNCTION_D "d"
>> #define LED_FUNCTION_DEBUG "debug"
>> #define LED_FUNCTION_DIA "dia"
>> #define LED_FUNCTION_DISK "disk"
>> #define LED_FUNCTION_DOWN "down"
>> #define LED_FUNCTION_DSL "dsl"
>> #define LED_FUNCTION_ENOCEAN "enocean"
>> #define LED_FUNCTION_ENTER "enter"
>> #define LED_FUNCTION_ERROR "error"
>> #define LED_FUNCTION_ESATA "esata"
>> #define LED_FUNCTION_ETHERNET-STATUS "ethernet-status"
>> #define LED_FUNCTION_FAIL "fail"
>> #define LED_FUNCTION_FAULT "fault"
>> #define LED_FUNCTION_FRONT "front"
>> #define LED_FUNCTION_FUNC "func"
>> #define LED_FUNCTION_G "g"
>> #define LED_FUNCTION_GHZ "ghz"
>> #define LED_FUNCTION_GHZ-1 "ghz-1"
>> #define LED_FUNCTION_GHZ-2 "ghz-2"
>> #define LED_FUNCTION_GPIO "gpio"
>> #define LED_FUNCTION_GSM "gsm"
>> #define LED_FUNCTION_HD "hd"
>> #define LED_FUNCTION_HDD "hdd"
>> #define LED_FUNCTION_HDDERR "hdderr"
>> #define LED_FUNCTION_HEALTH "health"
>> #define LED_FUNCTION_HEART "heart"
>> #define LED_FUNCTION_HEARTBEAT "heartbeat"
>> #define LED_FUNCTION_HOME "home"
>> #define LED_FUNCTION_INET "inet"
>> #define LED_FUNCTION_INFO "info"
>> #define LED_FUNCTION_INTERNET "internet"
>> #define LED_FUNCTION_KEYPAD "keypad"
>> #define LED_FUNCTION_L "l"
>> #define LED_FUNCTION_LAN "lan"
>> #define LED_FUNCTION_LEDB "ledb"
>> #define LED_FUNCTION_LEFT "left"
>> #define LED_FUNCTION_L_HDD "l_hdd"
>> #define LED_FUNCTION_LIVE "live"
>> #define LED_FUNCTION_LOGO "logo"
>> #define LED_FUNCTION_MICROSD "microsd"
>> #define LED_FUNCTION_MISC "misc"
>> #define LED_FUNCTION_MMC "mmc"
>> #define LED_FUNCTION_NAND "nand"
>> #define LED_FUNCTION_NETWORK "network"
>> #define LED_FUNCTION_ON "on"
>> #define LED_FUNCTION_ORANGE "orange"
>> #define LED_FUNCTION_OS "os"
>> #define LED_FUNCTION_PANEL "panel"
>> #define LED_FUNCTION_PMU_STAT "pmu_stat"
>> #define LED_FUNCTION_POWER "power"
>> #define LED_FUNCTION_PROG "prog"
>> #define LED_FUNCTION_PROGRAMMING "programming"
>> #define LED_FUNCTION_PROXIMITYSENSOR "proximitysensor"
>> #define LED_FUNCTION_PULSE "pulse"
>> #define LED_FUNCTION_PWR "pwr"
>> #define LED_FUNCTION_QSS "qss"
>> #define LED_FUNCTION_REBUILD "rebuild"
>> #define LED_FUNCTION_R_HDD "r_hdd"
>> #define LED_FUNCTION_RIGHT "right"
>> #define LED_FUNCTION_ROUTER "router"
>> #define LED_FUNCTION_RS "rs"
>> #define LED_FUNCTION_RX "rx"
>> #define LED_FUNCTION_SATA "sata"
>> #define LED_FUNCTION_SATA-L "sata-l"
>> #define LED_FUNCTION_SATA-R "sata-r"
>> #define LED_FUNCTION_SD "sd"
>> #define LED_FUNCTION_SLEEP "sleep"
>> #define LED_FUNCTION_STANDBY "standby"
>> #define LED_FUNCTION_STATE "state"
>> #define LED_FUNCTION_STATUS "status"
>> #define LED_FUNCTION_SW "sw"
>> #define LED_FUNCTION_SWRDY "swrdy"
>> #define LED_FUNCTION_SYS "sys"
>> #define LED_FUNCTION_SYSTEM "system"
>> #define LED_FUNCTION_SYSTEM-STATUS "system-status"
>> #define LED_FUNCTION_TEL "tel"
>> #define LED_FUNCTION_TOP "top"
>> #define LED_FUNCTION_TV "tv"
>> #define LED_FUNCTION_TX "tx"
>> #define LED_FUNCTION_UP "up"
>> #define LED_FUNCTION_USB "usb"
>> #define LED_FUNCTION_USB_1 "usb_1"
>> #define LED_FUNCTION_USB_2 "usb_2"
>> #define LED_FUNCTION_USB_COPY "usb_copy"
>> #define LED_FUNCTION_USB-PORT1 "usb-port1"
>> #define LED_FUNCTION_USB-PORT2 "usb-port2"
>> #define LED_FUNCTION_USER "user"
>> #define LED_FUNCTION_USR "usr"
>> #define LED_FUNCTION_WAN "wan"
>> #define LED_FUNCTION_WIFI "wifi"
>> #define LED_FUNCTION_WIFI_AP "wifi_ap"
>> #define LED_FUNCTION_WIFI-STATUS "wifi-status"
>> #define LED_FUNCTION_WIRELESS "wireless"
>> #define LED_FUNCTION_WLAN "wlan"
>> #define LED_FUNCTION_WLAN_G "wlan_g"
>> #define LED_FUNCTION_WMODE "wmode"
>> #define LED_FUNCTION_WPS "wps"
>>
>>
>> [0] https://patchwork.kernel.org/patch/10089047/
>> --
>> Best regards,
>> Jacek Anaszewski
>
Pavel Machek April 15, 2018, 4:15 p.m. | #12
Hi!

> We also had a discussion about standardized LED function names, so I
> retrieved all LED function names from dts files present in kernel,
> and after unifying some similar occurrences I got something like
> below. Perhaps it doesn't make a sense to add all of the below
> definitions to the standard header, but some of them could certainly
> find their place in e.g. include/dt-bindings/leds/led-functions.h.

Yes, we should try to make names standard.

> Then we could have LED names in the form function:color, and below
> defines could be used in dts files to avoid spawning similar LED names
> describing the same function.

> Of course in case of the names presented here with numerical suffixes
> we could skip the suffix.

So we should certainly remove some stuff for the list. For example we
do not want "d" function, and we want to choose either "bluetooth" or
"bt", but don't want to have both.

Also for example "activity" should be removed, we need to be more
specific there. I suspect "nand" means activity on the nand flash, for
example.

> #define LED_FUNCTION_ACTIVITY "activity"
> #define LED_FUNCTION_ADSL "adsl"
> #define LED_FUNCTION_ALARM "alarm"
> #define LED_FUNCTION_ALIVE "alive"
> #define LED_FUNCTION_ALL "all"
> #define LED_FUNCTION_APP "app"
> #define LED_FUNCTION_AUX "aux"
> #define LED_FUNCTION_BACKUP "backup"
> #define LED_FUNCTION_BEEP "beep"
> #define LED_FUNCTION_BLUETOOTH "bluetooth"
> #define LED_FUNCTION_BOOT "boot"
> #define LED_FUNCTION_BOTTOM "bottom"
> #define LED_FUNCTION_BRICK-STATUS "brick-status"
> #define LED_FUNCTION_BT "bt"
> #define LED_FUNCTION_CEL "cel"
> #define LED_FUNCTION_CEL-PWR "cel-pwr"

									Pavel
Pavel Machek April 15, 2018, 4:15 p.m. | #13
Hi!

> >> A case I care about is I have a family of boards that all have a
> >> common defined set of 4 LEDs. They could be driven by anything, but I
> >> want the same interface presented to userspace across boards.
> >> "
> > 
> > Yes, and "label" should provide me with that ability as the context
> > was the practice of putting the controller name in the label.
> 
> Exactly. There are two options:
> 
> 1.
> - label format : "<color>:<function"
> 2.
> - label format : "<function>"
> - new "color" DT property

We should provide compatibility here.

So we should really have new "color" property, and probably new
"function" property, too.

> The changes in dts files would have to be accompanied by changes in
> DT parsing part of LED class drivers. Also, I wonder if it wouldn't
> make sense to add config option to choose between old and new LED
> naming convention to avoid userspace breakage.

Both old and new kernels are expected to be working with both old and
new dts...

And having config option choosing userland interfaces would not be nice.

> It would allow to use dts files with old style labels with
> newer kernels. The LED class device names would differ like below:
> 
> Old style name (current):
> 	/sys/class/leds/netxbig:red:power
> 
> New style name:
> 	/sys/class/leds/red:power (we could also think of dropping
>                                    color from name and adding sysfs
>                                    file for it)

I believe color needs to stay: it is common to have "green:battery"
for "battery ok" and "yellow:battery" for charging, for
example. Notification LED has three channels, "red", "green",
"blue"...

Thanks for looking into this,
									Pavel
Jacek Anaszewski April 15, 2018, 6:30 p.m. | #14
On 04/15/2018 06:15 PM, Pavel Machek wrote:
> Hi!
> 
>>>> A case I care about is I have a family of boards that all have a
>>>> common defined set of 4 LEDs. They could be driven by anything, but I
>>>> want the same interface presented to userspace across boards.
>>>> "
>>>
>>> Yes, and "label" should provide me with that ability as the context
>>> was the practice of putting the controller name in the label.
>>
>> Exactly. There are two options:
>>
>> 1.
>> - label format : "<color>:<function"
>> 2.
>> - label format : "<function>"
>> - new "color" DT property
> 
> We should provide compatibility here.
> 
> So we should really have new "color" property, and probably new
> "function" property, too.
> 
>> The changes in dts files would have to be accompanied by changes in
>> DT parsing part of LED class drivers. Also, I wonder if it wouldn't
>> make sense to add config option to choose between old and new LED
>> naming convention to avoid userspace breakage.
> 
> Both old and new kernels are expected to be working with both old and
> new dts...
> 
> And having config option choosing userland interfaces would not be nice.

Agreed. So the naming pattern will be chosen basing on whether
there is 'label' property available or the 'function' and 'color' ones.

>> It would allow to use dts files with old style labels with
>> newer kernels. The LED class device names would differ like below:
>>
>> Old style name (current):
>> 	/sys/class/leds/netxbig:red:power
>>
>> New style name:
>> 	/sys/class/leds/red:power (we could also think of dropping
>>                                    color from name and adding sysfs
>>                                    file for it)
> 
> I believe color needs to stay: it is common to have "green:battery"
> for "battery ok" and "yellow:battery" for charging, for
> example. Notification LED has three channels, "red", "green",
> "blue"...

I don't have a strong opinion on that, so ack.

> Thanks for looking into this,

Thanks for the feedback. I'll try to come up with an initial
patchset when I'll find a moment.

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-cr0014114.txt b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
new file mode 100644
index 000000000000..977a9929b04f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
@@ -0,0 +1,46 @@ 
+Crane Merchandising System - cr0014114 LED driver
+-------------------------------------------------
+
+This LED Board is widely used in vending machines produced
+by Crane Merchandising Systems.
+
+Required properties:
+- compatible: "cms,cr0014114"
+- reg: chip select address for the device
+- spi-cpha: shifted clock phase mode is required
+
+LED sub-node properties:
+- label : (optional)
+	see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger : (optional)
+	see Documentation/devicetree/bindings/leds/common.txt
+
+Example
+-------
+
+cr0014114@0 {
+	compatible = "crane,cr0014114";
+	reg = <0>;
+	spi-max-frequency = <50000>;
+	spi-cpha;
+
+	led0 {
+		label = "cr0:red:";
+	};
+	led1 {
+		label = "cr0:green:";
+	};
+	led2 {
+		label = "cr0:blue:";
+	};
+	led3 {
+		label = "cr1:red:";
+	};
+	led4 {
+		label = "cr1:green:";
+	};
+	led5 {
+		label = "cr1:blue:";
+	};
+	...
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index ae850d6c0ad3..f17949c365f5 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -75,6 +75,7 @@  cnxt	Conexant Systems, Inc.
 compulab	CompuLab Ltd.
 cortina	Cortina Systems, Inc.
 cosmic	Cosmic Circuits
+crane	Crane Connectivity Solutions
 creative	Creative Technology Ltd
 crystalfontz	Crystalfontz America, Inc.
 cubietech	Cubietech, Ltd.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2c896c0e69e1..5831814d01d0 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -104,6 +104,19 @@  config LEDS_CPCAP
 	  This option enables support for LEDs offered by Motorola's
 	  CPCAP PMIC.
 
+config LEDS_CR0014114
+	tristate "LED Support for Crane CR0014114"
+	depends on LEDS_CLASS
+	depends on SPI
+	depends on OF
+	help
+	  This option enables support for CR0014114 LED Board which
+	  is widely used in vending machines produced by
+	  Crane Merchandising Systems.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-cr0014114.
+
 config LEDS_LM3530
 	tristate "LCD Backlight driver for LM3530"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 91eca81cae82..0176e7335994 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -78,6 +78,7 @@  obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
 
 # LED SPI Drivers
+obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
 
 # LED Userspace Drivers
diff --git a/drivers/leds/leds-cr0014114.c b/drivers/leds/leds-cr0014114.c
new file mode 100644
index 000000000000..ef536a67dceb
--- /dev/null
+++ b/drivers/leds/leds-cr0014114.c
@@ -0,0 +1,292 @@ 
+/*
+ * LEDs driver for Crane CR0014114
+ *
+ * Copyright (C) 2018 Oleh Kravchenko <oleg@kaa.org.ua>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/property.h>
+#include <linux/spi/spi.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
+#include <linux/workqueue.h>
+
+/* CR0014114 SPI commands */
+#define CR_SET_BRIGHTNESS	0x80
+#define CR_INIT_REENUMERATE	0x81
+#define CR_NEXT_REENUMERATE	0x82
+
+/* CR0014114 default settings */
+#define CR_MAX_BRIGHTNESS	GENMASK(6, 0)
+#define CR_FW_DELAY_MSEC	10
+#define CR_RECOUNT_DELAY	(HZ * 3600)
+
+struct cr0014114_led {
+	const char		*name;
+	struct cr0014114	*priv;
+	struct led_classdev	ldev;
+	u8			brightness;
+};
+
+struct cr0014114 {
+	bool			do_recount;
+	size_t			count;
+	struct device		*dev;
+	struct mutex		lock;
+	struct spi_device	*spi;
+	struct timer_list	timer;
+	struct work_struct	work;
+	unsigned long		delay;
+	struct cr0014114_led	leds[];
+};
+
+static void cr0014114_calc_crc(u8 *buf, const size_t len)
+{
+	size_t	i;
+	u8	crc;
+
+	for (i = 1, crc = 1; i < len - 1; i++)
+		crc += buf[i];
+	crc |= BIT(7);
+
+	/* special case when CRC matches the SPI commands */
+	if (crc == CR_SET_BRIGHTNESS ||
+	    crc == CR_INIT_REENUMERATE ||
+	    crc == CR_NEXT_REENUMERATE)
+		crc = 0xfe;
+
+	buf[len - 1] = crc;
+}
+
+static int cr0014114_recount(struct cr0014114 *priv)
+{
+	int	ret;
+	size_t	i;
+	u8	cmd;
+
+	dev_dbg(priv->dev, "recount of LEDs is started\n");
+
+	do {
+		cmd = CR_INIT_REENUMERATE;
+		ret = spi_write(priv->spi, &cmd, sizeof(cmd));
+		if (ret)
+			break;
+
+		cmd = CR_NEXT_REENUMERATE;
+		for (i = 0; i < priv->count; i++) {
+			msleep(CR_FW_DELAY_MSEC);
+
+			ret = spi_write(priv->spi, &cmd, sizeof(cmd));
+			if (ret)
+				break;
+		}
+	} while (0);
+
+	dev_dbg(priv->dev, "recount of LEDs is complete, error: %d\n", ret);
+
+	return ret;
+}
+
+static int cr0014114_sync(struct cr0014114 *priv)
+{
+	int		ret;
+	size_t		i;
+	u8		data[priv->count + 2];
+	unsigned long	udelay, now = jiffies;
+
+	/* to avoid SPI mistiming with firmware we should wait some time */
+	if (time_after(priv->delay, now)) {
+		udelay = jiffies_to_usecs(priv->delay - now);
+		usleep_range(udelay, udelay + 1);
+	}
+
+	do {
+		if (unlikely(priv->do_recount)) {
+			ret = cr0014114_recount(priv);
+			if (ret)
+				break;
+
+			priv->do_recount = false;
+		}
+
+		data[0] = CR_SET_BRIGHTNESS;
+		for (i = 0; i < priv->count; i++)
+			data[i + 1] = priv->leds[i].brightness;
+		cr0014114_calc_crc(data, sizeof(data));
+
+		ret = spi_write(priv->spi, data, sizeof(data));
+		if (ret)
+			break;
+	} while (0);
+
+	priv->delay = jiffies + msecs_to_jiffies(CR_FW_DELAY_MSEC);
+
+	return ret;
+}
+
+static void cr0014114_recount_work(struct work_struct *work)
+{
+	int			ret;
+	struct cr0014114	*priv = container_of(work, struct cr0014114,
+						     work);
+
+	mutex_lock(&priv->lock);
+	priv->do_recount = true;
+	ret = cr0014114_sync(priv);
+	mutex_unlock(&priv->lock);
+
+	if (ret)
+		dev_warn(priv->dev, "recount LEDs failed %d\n", ret);
+}
+
+static int cr0014114_set_sync(struct led_classdev *ldev,
+			      enum led_brightness brightness)
+{
+	int			ret;
+	struct cr0014114_led    *led = container_of(ldev,
+						    struct cr0014114_led,
+						    ldev);
+
+	mutex_lock(&led->priv->lock);
+	led->brightness = (u8)brightness;
+	ret = cr0014114_sync(led->priv);
+	mutex_unlock(&led->priv->lock);
+
+	return ret;
+}
+
+static void cr0014114_recount_timer(struct timer_list *t)
+{
+	struct cr0014114 *priv = from_timer(priv, t, timer);
+
+	schedule_work(&priv->work);
+	mod_timer(&priv->timer, jiffies + CR_RECOUNT_DELAY);
+}
+
+static int cr0014114_probe_dt(struct cr0014114 *priv)
+{
+	size_t			i = 0;
+	struct cr0014114_led	*led;
+	struct fwnode_handle	*child;
+	struct device_node	*np;
+	int			ret;
+
+	device_for_each_child_node(priv->dev, child) {
+		np = to_of_node(child);
+		led = &priv->leds[i];
+
+		ret = fwnode_property_read_string(child, "label",
+						  &led->name);
+		if (ret && IS_ENABLED(CONFIG_OF) && np)
+			led->name = np->name;
+
+		if (!led->name) {
+			fwnode_handle_put(child);
+			return -EINVAL;
+		}
+
+		fwnode_property_read_string(child, "linux,default-trigger",
+					    &led->ldev.default_trigger);
+
+		led->priv			  = priv;
+		led->ldev.name			  = led->name;
+		led->ldev.brightness		  = LED_OFF;
+		led->ldev.max_brightness	  = CR_MAX_BRIGHTNESS;
+		led->ldev.brightness_set_blocking = cr0014114_set_sync;
+
+		ret = devm_of_led_classdev_register(priv->dev, np,
+						    &led->ldev);
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+
+		led->ldev.dev->of_node = np;
+
+		i++;
+	}
+
+	return 0;
+}
+
+static int cr0014114_probe(struct spi_device *spi)
+{
+	struct cr0014114	*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,
+			    sizeof(*priv) + sizeof(*priv->leds) * count,
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	mutex_init(&priv->lock);
+	INIT_WORK(&priv->work, cr0014114_recount_work);
+	priv->do_recount	= true;
+	priv->count		= count;
+	priv->dev		= &spi->dev;
+	priv->spi		= spi;
+
+	ret = cr0014114_probe_dt(priv);
+	if (ret)
+		return ret;
+
+	ret = cr0014114_sync(priv);
+	if (ret)
+		return ret;
+
+	/* setup recount timer to workaround buggy firmware */
+	timer_setup(&priv->timer, cr0014114_recount_timer, 0);
+	mod_timer(&priv->timer, jiffies + CR_RECOUNT_DELAY);
+
+	spi_set_drvdata(spi, priv);
+
+	return 0;
+}
+
+static int cr0014114_remove(struct spi_device *spi)
+{
+	struct cr0014114 *priv = spi_get_drvdata(spi);
+
+	cancel_work_sync(&priv->work);
+	del_timer_sync(&priv->timer);
+
+	return 0;
+}
+
+static const struct of_device_id cr0014114_dt_ids[] = {
+	{ .compatible = "crane,cr0014114", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, cr0014114_dt_ids);
+
+static struct spi_driver cr0014114_driver = {
+	.probe		= cr0014114_probe,
+	.remove		= cr0014114_remove,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= cr0014114_dt_ids,
+	},
+};
+
+module_spi_driver(cr0014114_driver);
+
+MODULE_AUTHOR("Oleh Kravchenko <oleg@kaa.org.ua>");
+MODULE_DESCRIPTION("cr0014114 LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:cr0014114");