| Message ID | 20180313124548.12074-1-oleg@kaa.org.ua |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3] leds: add LED driver for CR0014114 board | expand |
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
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 >
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
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 > >
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
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
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/
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
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/
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
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 >
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
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
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.
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");
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