leds: core: add generic support for color LED's
PATCH v3 1/4 leds: core: add generic support for color LED's
Add generic support for color LED's.
Basic idea is to use enum led_brightness also for the hue and saturation color components.This allows to implement the color extension w/o changes to struct led_classdev.
A driver that wants to use this extension has to select LEDS_HSV in its Kconfig entry.
Flag LED_SET_COLOR allows to specify that hue / saturation should be overridden even if the provided values are zero.
Some examples for writing values to /sys/class/leds/<xx>/brightness:
(now also hex notation can be used)
255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
if the LED is switched on again later0x1000000 -> switch LED off and set also hue and saturation to 0 0x00ffff -> set full brightness, full saturation and set hue to 0 (red)
---
v2:
- move extension-specific code into a separate source file and introduce config symbol LEDS_HSV for it
- create separate patch for the extension to sysfs
- use variable name led_cdev as in the rest if the core
- rename to_hsv to led_validate_brightness
- rename LED_BRIGHTNESS_SET_COLOR to LED_SET_HSV
- introduce helper is_off for checking whether V part
of a HSV value is zero
v3:
- change Kconfig to use depend instead of select, add help message, and change config symbol to LEDS_COLOR
- change LED core object file name in Makefile
- rename flag LED_SET_HSV to LED_SET_COLOR
- rename is_off to led_is_off
- rename led_validate-brightness to led_confine_brightness
- rename variable in led_confine_brightness
- add dummy enum led_brightness value to enforce 32bit enum
- rename led-hsv-core.c to led-color-core.c
- move check of provided brightness value to led_confine_brightness
--- drivers/leds/Kconfig | 11 +++++++++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-class.c | 2 +- drivers/leds/led-color-core.c | 33 +++++++++++++++++++++++++++++++++ drivers/leds/led-core.c | 15 ++++++++------- drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 7 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 drivers/leds/led-color-core.c
drivers/leds/Kconfig +11 -0
@@ -13,6 +13,13 @@ menuconfig NEW_LEDS if NEW_LEDS +config LEDS_COLOR + bool "Color LED Support" + help + This option enables support for Color LED devices, mainly RGB LEDs. + Sysfs attribute brightness can be used to set also the color. + For details see Documentation/leds/leds-class.txt. + config LEDS_CLASS tristate "LED Class Support" help
@@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +if LEDS_COLOR +comment "Color LED drivers" +endif # LEDS_COLOR + comment "LED drivers" config LEDS_88PM860X
drivers/leds/Makefile +3 -1
@@ -1,6 +1,8 @@ # LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += led-core-objs.o +led-core-objs-y := led-core.o +led-core-objs-$(CONFIG_LEDS_COLOR) += led-color-core.o obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o
drivers/leds/led-class.c +1 -1
@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,if (ret) goto unlock; -if (state ==LED_OFF ) +if (led_is_off (state))led_trigger_remove (led_cdev);led_set_brightness (led_cdev, state);
drivers/leds/led-color-core.c new file +33 -0
@@ -0,0 +1,33 @@ +/* +* LED Class Color Support +* +* Author: Heiner Kallweit <[email protected]> +* +* 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/kernel.h> +#include <linux/leds.h> +#include "leds.h" + +#define LED_HUE_SAT_MASK 0x00ffff00 + +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness brightness; + +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness =0 ; +/* Use LED_SET_COLOR to set hue and saturation even if both are zero */ +else if (value &LED_SET_COLOR || value >LED_FULL ) + brightness = value &LED_HUE_SAT_MASK ; +else + brightness = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return brightness | +min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}
drivers/leds/led-core.c +8 -7
@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data) } brightness =led_get_brightness (led_cdev); -if (!brightness) { +if (led_is_off (brightness)) {/* Time to switch the LED on. */ brightness = led_cdev->blink_brightness; delay = led_cdev->blink_delay_on;
@@ -133,8 +133,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,if (current_brightness) led_cdev->blink_brightness = current_brightness;if (!led_cdev->blink_brightness) - led_cdev->blink_brightness = led_cdev->max_brightness; - + led_cdev->blink_brightness = +led_confine_brightness (led_cdev,LED_FULL ); led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;
@@ -235,12 +235,13 @@ void led_set_brightness(struct led_classdev *led_cdev,* work queue task to avoid problems in case we are called * from hard irq context. */ -if (brightness ==LED_OFF ) { +if (led_is_off (brightness)) { led_cdev->flags |=LED_BLINK_DISABLE ;schedule_work (&led_cdev->set_brightness_work); }else { led_cdev->flags |=LED_BLINK_BRIGHTNESS_CHANGE ; - led_cdev->blink_brightness = brightness; + led_cdev->blink_brightness = +led_confine_brightness (led_cdev, brightness); }return ; }
@@ -265,7 +266,7 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nopm);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value) { - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;
@@ -280,7 +281,7 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)return -EBUSY ; - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;
drivers/leds/leds.h +17 -0
@@ -16,17 +16,34 @@#include <linux/rwsem.h> #include <linux/leds.h> +#define LED_BRIGHTNESS_MASK 0x000000ff +static inline int led_get_brightness (struct led_classdev *led_cdev) {return led_cdev->brightness; } +static inline bool led_is_off (enum led_brightness brightness) +{ +return (brightness &LED_BRIGHTNESS_MASK ) ==LED_OFF ; +} +void led_init_core (struct led_classdev *led_cdev);void led_stop_software_blink (struct led_classdev *led_cdev);void led_set_brightness_nopm (struct led_classdev *led_cdev,enum led_brightness value);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value); +#if IS_ENABLED (CONFIG_LEDS_COLOR ) +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_confine_brightness ( +struct led_classdev *led_cdev,enum led_brightness value) +{ +return min (value, led_cdev->max_brightness); +} +#endif extern struct rw_semaphore leds_list_lock;extern struct list_head leds_list;
include/linux/leds.h +9 -0
@@ -29,8 +29,16 @@ enum led_brightness {LED_OFF =0 ,LED_HALF =127 ,LED_FULL =255 , +/* +* dummy enum value to make gcc use a 32 bit type for the enum +* even if compiled with -fshort-enums. This is needed for +* the enum to store hsv values. +*/ + LED_LEVEL_DUMMY = 0xffffffff, }; +#define LED_SET_COLOR BIT(24) +struct led_classdev {const char *name;enum led_brightness brightness;
@@ -50,6 +58,7 @@ struct led_classdev {#define LED_SYSFS_DISABLE (1 << 22)#define LED_DEV_CAP_FLASH (1 << 23)#define LED_HW_PLUGGABLE (1 << 24) +#define LED_DEV_CAP_HSV (1 << 25)/* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers
Signature · 1 line
2.7.1
Hi Heiner,
Quoted 58 lines · Heiner Kallweit
Add generic support for color LED's.
Basic idea is to use enum
led_brightnessalso for the hue and saturation color components.This allows to implement the color extension w/o changes to structled_classdev.
A driver that wants to use this extension has to selectLEDS_HSVin itsKconfigentry.Flag
LED_SET_COLORallows to specify that hue / saturation should be overridden even if the provided values are zero.Some examples for writing values to /sys/class/leds/<xx>/brightness:
(now also hex notation can be used)255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
if the LED is switched on again later0x1000000-> switch LED off and set also hue and saturation to 00x00ffff-> set full brightness, full saturation and set hue to 0 (red)Signed-off-by Heiner Kallweit <[email protected]>---
v2:
- move extension-specific code into a separate source file and introduce config symbolLEDS_HSVfor it
- create separate patch for the extension to sysfs
- use variable nameled_cdevas in the rest if the core
- renameto_hsvtoled_validate_brightness
- renameLED_BRIGHTNESS_SET_COLORtoLED_SET_HSV
- introduce helperis_offfor checking whether V part
of a HSV value is zero
v3:
- changeKconfigto use depend instead of select, add help message, and change config symbol toLEDS_COLOR
- change LED core object file name inMakefile
- rename flagLED_SET_HSVtoLED_SET_COLOR
- renameis_offtoled_is_off
- rename led_validate-brightness toled_confine_brightness
- rename variable inled_confine_brightness
- add dummy enumled_brightnessvalue to enforce 32bit enum
- renameled-hsv-core.ctoled-color-core.c
- move check of provided brightness value toled_confine_brightness--- drivers/leds/Kconfig | 11 +++++++++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-class.c | 2 +- drivers/leds/led-color-core.c | 33 +++++++++++++++++++++++++++++++++ drivers/leds/led-core.c | 15 ++++++++------- drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 7 files changed, 82 insertions(+), 9 deletions(-)create mode 100644
drivers/leds/led-color-core.cdrivers/leds/Kconfig +0
-0
drivers/leds/Kconfig menuconfig NEW_LEDS · line 16
Quoted 4 lines
drivers/leds/Kconfig +1
-0@@ -13,6 +13,13 @@ menuconfig NEW_LEDS if NEW_LEDS +config LEDS_COLOR
After thinking it over I came to the conclusion that LEDS_COLOR isn't suitable name for multi color LEDs. We already refer to LED colors in the leds-class.txt in the "LED Device Naming" section, with monochrome LEDs on mind.
I think that we should go for LEDS_RGB to match the name under which this type of LEDs appear on the market, and which reflects the color scheme these devices need to be provided with. I've been also thinking about LEDS_MULTICOLOR, but it looks kind awkward for me. Feel free to share other ideas.
drivers/leds/Kconfig menuconfig NEW_LEDS · line 19
Quoted 3 lines
drivers/leds/Kconfig +3
-0+ bool "Color LED Support" + help + This option enables support for Color LED devices, mainly RGB LEDs.
Here we should mention that RGB LEDs are handled with HSV interface.
Quoted 22 lines
drivers/leds/Kconfig +7
-0+ Sysfs attribute brightness can be used to set also the color. + For details see Documentation/leds/leds-class.txt. + config LEDS_CLASS tristate "LED Class Support" help@@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +if LEDS_COLOR +comment "Color LED drivers" +endif # LEDS_COLOR + comment "LED drivers" config LEDS_88PM860Xdrivers/leds/Makefile +0
-0
drivers/leds/Makefile line 5
Quoted 6 lines
drivers/leds/Makefile +3
-1@@ -1,6 +1,8 @@ # LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += led-core-objs.o +led-core-objs-y := led-core.o +led-core-objs-$(CONFIG_LEDS_COLOR) += led-color-core.o
We'd have to change this to led-rgb-core.o then.
Quoted 33 lines
drivers/leds/Makefile +0
-0obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.odrivers/leds/led-class.c +1
-1@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,if (ret) goto unlock; -if (state ==LED_OFF ) +if (led_is_off (state))led_trigger_remove (led_cdev);led_set_brightness (led_cdev, state);drivers/leds/led-color-core.c new file +11
-0@@ -0,0 +1,33 @@ +/* +* LED Class Color Support +* +* Author: Heiner Kallweit <[email protected]> +* +* 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. +* +*/ +
drivers/leds/led-color-core.c line 21
Quoted 10 lines
drivers/leds/led-color-core.c new file +10
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include "leds.h" + +#define LED_HUE_SAT_MASK 0x00ffff00 + +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness brightness;
s/brightness/brightness = 0/
drivers/leds/led-color-core.c line 24
Quoted 3 lines
drivers/leds/led-color-core.c new file +3
-0+ +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness =0 ;
And this check will be redundant.
Quoted 14 lines
drivers/leds/led-color-core.c new file +9
-0+/* Use LED_SET_COLOR to set hue and saturation even if both are zero */ + else if (value & LED_SET_COLOR || value > LED_FULL) + brightness = value &LED_HUE_SAT_MASK ; + else + brightness = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return brightness | +min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}drivers/leds/led-core.c +0
-0
drivers/leds/led-core.c static void led_timer_function(unsigned long data) · line 65
Quoted 5 lines
drivers/leds/led-core.c +1
-1@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data) } brightness =led_get_brightness (led_cdev); -if (!brightness) { +if (led_is_off (brightness)) {
Instead of adding led_is_off(), couldn't we extend led_get_brightness()?
drivers/leds/led-core.c static void led_set_software_blink(struct led_classdev *led_cdev, · line 137
Quoted 11 lines
drivers/leds/led-core.c +2
-2/* Time to switch the LED on. */ brightness = led_cdev->blink_brightness; delay = led_cdev->blink_delay_on;@@ -133,8 +133,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,if (current_brightness) led_cdev->blink_brightness = current_brightness;if (!led_cdev->blink_brightness) - led_cdev->blink_brightness = led_cdev->max_brightness; - + led_cdev->blink_brightness = +led_confine_brightness (led_cdev,LED_FULL );
I am still in favour of passing led_cdev->max_brightness instead of LED_FULL.
Quoted 83 lines
drivers/leds/led-core.c +5
-4led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;@@ -235,12 +235,13 @@ void led_set_brightness(struct led_classdev *led_cdev,* work queue task to avoid problems in case we are called * from hard irq context. */ -if (brightness ==LED_OFF ) { +if (led_is_off (brightness)) { led_cdev->flags |=LED_BLINK_DISABLE ;schedule_work (&led_cdev->set_brightness_work); }else { led_cdev->flags |=LED_BLINK_BRIGHTNESS_CHANGE ; - led_cdev->blink_brightness = brightness; + led_cdev->blink_brightness = +led_confine_brightness (led_cdev, brightness); }return ; }@@ -265,7 +266,7 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nopm);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value) { - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;@@ -280,7 +281,7 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)return -EBUSY ; - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;drivers/leds/leds.h +17
-0@@ -16,17 +16,34 @@#include <linux/rwsem.h> #include <linux/leds.h> +#define LED_BRIGHTNESS_MASK 0x000000ff +static inline int led_get_brightness (struct led_classdev *led_cdev) {return led_cdev->brightness; } +static inline bool led_is_off (enum led_brightness brightness) +{ +return (brightness &LED_BRIGHTNESS_MASK ) ==LED_OFF ; +} +void led_init_core (struct led_classdev *led_cdev);void led_stop_software_blink (struct led_classdev *led_cdev);void led_set_brightness_nopm (struct led_classdev *led_cdev,enum led_brightness value);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value); +#if IS_ENABLED (CONFIG_LEDS_COLOR ) +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_confine_brightness ( +struct led_classdev *led_cdev,enum led_brightness value) +{ +return min (value, led_cdev->max_brightness); +} +#endif extern struct rw_semaphore leds_list_lock;extern struct list_head leds_list;include/linux/leds.h +0
-0@@ -29,8 +29,16 @@ enum led_brightness {LED_OFF =0 , LED_HALF = 127,
include/linux/leds.h enum led_brightness { · line 40
Quoted 10 lines
include/linux/leds.h +7
-0LED_FULL =255 , +/* +* dummy enum value to make gcc use a 32 bit type for the enum +* even if compiled with -fshort-enums. This is needed for +* the enum to store hsv values. +*/ + LED_LEVEL_DUMMY = 0xffffffff, }; +#define LED_SET_COLOR BIT(24)
In HSV color model also changing brightness changes the color, which I didn't take into account while proposing this name. We need more accurate name for this macro. LED_SET_HUE_SAT?
Quoted 13 lines
include/linux/leds.h +2
-0+struct led_classdev {const char *name;enum led_brightness brightness;@@ -50,6 +58,7 @@ struct led_classdev {#define LED_SYSFS_DISABLE (1 << 22)#define LED_DEV_CAP_FLASH (1 << 23)#define LED_HW_PLUGGABLE (1 << 24) +#define LED_DEV_CAP_HSV (1 << 25)/* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 19.02.2016 um 14:59 schrieb Jacek Anaszewski:
Quoted 71 lines
Hi Heiner,
Quoted 58 lines · Heiner Kallweit
Add generic support for color LED's.
Basic idea is to use enum
led_brightnessalso for the hue and saturation color components.This allows to implement the color extension w/o changes to structled_classdev.
A driver that wants to use this extension has to selectLEDS_HSVin itsKconfigentry.Flag
LED_SET_COLORallows to specify that hue / saturation should be overridden even if the provided values are zero.Some examples for writing values to /sys/class/leds/<xx>/brightness:
(now also hex notation can be used)255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
if the LED is switched on again later0x1000000-> switch LED off and set also hue and saturation to 00x00ffff-> set full brightness, full saturation and set hue to 0 (red)Signed-off-by Heiner Kallweit <[email protected]>---
v2:
- move extension-specific code into a separate source file and introduce config symbolLEDS_HSVfor it
- create separate patch for the extension to sysfs
- use variable nameled_cdevas in the rest if the core
- renameto_hsvtoled_validate_brightness
- renameLED_BRIGHTNESS_SET_COLORtoLED_SET_HSV
- introduce helperis_offfor checking whether V part
of a HSV value is zero
v3:
- changeKconfigto use depend instead of select, add help message, and change config symbol toLEDS_COLOR
- change LED core object file name inMakefile
- rename flagLED_SET_HSVtoLED_SET_COLOR
- renameis_offtoled_is_off
- rename led_validate-brightness toled_confine_brightness
- rename variable inled_confine_brightness
- add dummy enumled_brightnessvalue to enforce 32bit enum
- renameled-hsv-core.ctoled-color-core.c
- move check of provided brightness value toled_confine_brightness--- drivers/leds/Kconfig | 11 +++++++++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-class.c | 2 +- drivers/leds/led-color-core.c | 33 +++++++++++++++++++++++++++++++++ drivers/leds/led-core.c | 15 ++++++++------- drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 7 files changed, 82 insertions(+), 9 deletions(-)create mode 100644
drivers/leds/led-color-core.cdrivers/leds/Kconfig +0
-0drivers/leds/Kconfig menuconfig NEW_LEDS · line 16
Quoted 4 lines
drivers/leds/Kconfig +1
-0@@ -13,6 +13,13 @@ menuconfig NEW_LEDS if NEW_LEDS +config LEDS_COLORAfter thinking it over I came to the conclusion that
LEDS_COLORisn't suitable name for multi color LEDs. We already refer to LED colors in the leds-class.txt in the "LED Device Naming" section, with monochrome LEDs on mind.
Quoted 6 lines
I think that we should go for
LEDS_RGBto match the name under which this type of LEDs appear on the market, and which reflects the color scheme these devices need to be provided with. I've been also thinking aboutLEDS_MULTICOLOR, but it looks kind awkward for me. Feel free to share other ideas.
LEDS_RGB is fine with me.
Quoted 83 lines
drivers/leds/Kconfig menuconfig NEW_LEDS · line 19
Quoted 3 lines
drivers/leds/Kconfig +3
-0+ bool "Color LED Support" + help + This option enables support for Color LED devices, mainly RGB LEDs.Here we should mention that RGB LEDs are handled with HSV interface.
Quoted 22 lines
drivers/leds/Kconfig +7
-0+ Sysfs attribute brightness can be used to set also the color. + For details see Documentation/leds/leds-class.txt. + config LEDS_CLASS tristate "LED Class Support" help@@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +if LEDS_COLOR +comment "Color LED drivers" +endif # LEDS_COLOR + comment "LED drivers" config LEDS_88PM860Xdrivers/leds/Makefile +0
-0drivers/leds/Makefile line 5
Quoted 6 lines
drivers/leds/Makefile +3
-1@@ -1,6 +1,8 @@ # LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += led-core-objs.o +led-core-objs-y := led-core.o +led-core-objs-$(CONFIG_LEDS_COLOR) += led-color-core.oWe'd have to change this to led-rgb-core.o then.
Quoted 33 lines
drivers/leds/Makefile +0
-0obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.odrivers/leds/led-class.c +1
-1@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,if (ret) goto unlock; -if (state ==LED_OFF ) +if (led_is_off (state))led_trigger_remove (led_cdev);led_set_brightness (led_cdev, state);drivers/leds/led-color-core.c new file +11
-0@@ -0,0 +1,33 @@ +/* +* LED Class Color Support +* +* Author: Heiner Kallweit <[email protected]> +* +* 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. +* +*/ +drivers/leds/led-color-core.c line 21
Quoted 10 lines
drivers/leds/led-color-core.c new file +10
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include "leds.h" + +#define LED_HUE_SAT_MASK 0x00ffff00 + +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness brightness;s/brightness/brightness = 0/
Quoted 6 lines
drivers/leds/led-color-core.c line 24
Quoted 3 lines
drivers/leds/led-color-core.c new file +3
-0+ +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness =0 ;And this check will be redundant.
Not really, because after the following comment the else clause of this check follows.
Quoted 16 lines
Quoted 16 lines
drivers/leds/led-color-core.c new file +9
-0+/* Use LED_SET_COLOR to set hue and saturation even if both are zero */ + else if (value & LED_SET_COLOR || value > LED_FULL) + brightness = value &LED_HUE_SAT_MASK ; + else + brightness = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return brightness | +min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}drivers/leds/led-core.c +0
-0@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data) }
Quoted 6 lines
drivers/leds/led-core.c static void led_timer_function(unsigned long data) · line 65
Quoted 3 lines
drivers/leds/led-core.c +1
-1brightness =led_get_brightness (led_cdev); -if (!brightness) { +if (led_is_off (brightness)) {Instead of adding
led_is_off(), couldn't we extendled_get_brightness()?
led_is_off() is used with different arguments in different places, therefore I think we still need this function.
The brightness variable is used in the subsequent code, so we have to keep also the assignment brightness = led_get_brightness(led_cdev); I'm not sure how this could be improved and what we would gain.
Quoted 6 lines
Quoted 6 lines
drivers/leds/led-core.c +0
-0/* Time to switch the LED on. */ brightness = led_cdev->blink_brightness; delay = led_cdev->blink_delay_on;@@ -133,8 +133,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,if (current_brightness) led_cdev->blink_brightness = current_brightness;
Quoted 9 lines
drivers/leds/led-core.c static void led_set_software_blink(struct led_classdev *led_cdev, · line 137
Quoted 5 lines
drivers/leds/led-core.c +2
-2if (!led_cdev->blink_brightness) - led_cdev->blink_brightness = led_cdev->max_brightness; - + led_cdev->blink_brightness = +led_confine_brightness (led_cdev,LED_FULL );I am still in favour of passing
led_cdev->max_brightnessinstead ofLED_FULL.
OK
Quoted 92 lines
Quoted 92 lines
drivers/leds/led-core.c +5
-4led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;@@ -235,12 +235,13 @@ void led_set_brightness(struct led_classdev *led_cdev,* work queue task to avoid problems in case we are called * from hard irq context. */ -if (brightness ==LED_OFF ) { +if (led_is_off (brightness)) { led_cdev->flags |=LED_BLINK_DISABLE ;schedule_work (&led_cdev->set_brightness_work); }else { led_cdev->flags |=LED_BLINK_BRIGHTNESS_CHANGE ; - led_cdev->blink_brightness = brightness; + led_cdev->blink_brightness = +led_confine_brightness (led_cdev, brightness); }return ; }@@ -265,7 +266,7 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nopm);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value) { - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;@@ -280,7 +281,7 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)return -EBUSY ; - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;drivers/leds/leds.h +17
-0@@ -16,17 +16,34 @@#include <linux/rwsem.h> #include <linux/leds.h> +#define LED_BRIGHTNESS_MASK 0x000000ff +static inline int led_get_brightness (struct led_classdev *led_cdev) {return led_cdev->brightness; } +static inline bool led_is_off (enum led_brightness brightness) +{ +return (brightness &LED_BRIGHTNESS_MASK ) ==LED_OFF ; +} +void led_init_core (struct led_classdev *led_cdev);void led_stop_software_blink (struct led_classdev *led_cdev);void led_set_brightness_nopm (struct led_classdev *led_cdev,enum led_brightness value);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value); +#if IS_ENABLED (CONFIG_LEDS_COLOR ) +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_confine_brightness ( +struct led_classdev *led_cdev,enum led_brightness value) +{ +return min (value, led_cdev->max_brightness); +} +#endif extern struct rw_semaphore leds_list_lock;extern struct list_head leds_list;include/linux/leds.h +6
-0@@ -29,8 +29,16 @@ enum led_brightness {LED_OFF =0 ,LED_HALF =127 ,LED_FULL =255 , +/* +* dummy enum value to make gcc use a 32 bit type for the enum +* even if compiled with -fshort-enums. This is needed for +* the enum to store hsv values. +*/ + LED_LEVEL_DUMMY = 0xffffffff, };
Quoted 6 lines
include/linux/leds.h enum led_brightness { · line 40
Quoted 1 line
include/linux/leds.h +1
-0+#define LED_SET_COLOR BIT(24)In HSV color model also changing brightness changes the color, which I didn't take into account while proposing this name. We need more accurate name for this macro.
LED_SET_HUE_SAT?
Sounds good.
Quoted 15 lines
Quoted 13 lines
include/linux/leds.h +2
-0+struct led_classdev {const char *name;enum led_brightness brightness;@@ -50,6 +58,7 @@ struct led_classdev {#define LED_SYSFS_DISABLE (1 << 22)#define LED_DEV_CAP_FLASH (1 << 23)#define LED_HW_PLUGGABLE (1 << 24) +#define LED_DEV_CAP_HSV (1 << 25)/* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers
Quoted 163 lines · Heiner Kallweit
Am 19.02.2016 um 14:59 schrieb Jacek Anaszewski:
Quoted 71 lines
Hi Heiner,
Quoted 58 lines · Heiner Kallweit
Add generic support for color LED's.
Basic idea is to use enum
led_brightnessalso for the hue and saturation color components.This allows to implement the color extension w/o changes to structled_classdev.
A driver that wants to use this extension has to selectLEDS_HSVin itsKconfigentry.Flag
LED_SET_COLORallows to specify that hue / saturation should be overridden even if the provided values are zero.Some examples for writing values to /sys/class/leds/<xx>/brightness:
(now also hex notation can be used)255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
if the LED is switched on again later0x1000000-> switch LED off and set also hue and saturation to 00x00ffff-> set full brightness, full saturation and set hue to 0 (red)Signed-off-by Heiner Kallweit <[email protected]>---
v2:
- move extension-specific code into a separate source file and introduce config symbolLEDS_HSVfor it
- create separate patch for the extension to sysfs
- use variable nameled_cdevas in the rest if the core
- renameto_hsvtoled_validate_brightness
- renameLED_BRIGHTNESS_SET_COLORtoLED_SET_HSV
- introduce helperis_offfor checking whether V part
of a HSV value is zero
v3:
- changeKconfigto use depend instead of select, add help message, and change config symbol toLEDS_COLOR
- change LED core object file name inMakefile
- rename flagLED_SET_HSVtoLED_SET_COLOR
- renameis_offtoled_is_off
- rename led_validate-brightness toled_confine_brightness
- rename variable inled_confine_brightness
- add dummy enumled_brightnessvalue to enforce 32bit enum
- renameled-hsv-core.ctoled-color-core.c
- move check of provided brightness value toled_confine_brightness--- drivers/leds/Kconfig | 11 +++++++++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-class.c | 2 +- drivers/leds/led-color-core.c | 33 +++++++++++++++++++++++++++++++++ drivers/leds/led-core.c | 15 ++++++++------- drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 7 files changed, 82 insertions(+), 9 deletions(-)create mode 100644
drivers/leds/led-color-core.cdrivers/leds/Kconfig +0
-0drivers/leds/Kconfig menuconfig NEW_LEDS · line 16
Quoted 4 lines
drivers/leds/Kconfig +1
-0@@ -13,6 +13,13 @@ menuconfig NEW_LEDS if NEW_LEDS +config LEDS_COLORAfter thinking it over I came to the conclusion that
LEDS_COLORisn't suitable name for multi color LEDs. We already refer to LED colors in the leds-class.txt in the "LED Device Naming" section, with monochrome LEDs on mind.Quoted 6 lines
I think that we should go for
LEDS_RGBto match the name under which this type of LEDs appear on the market, and which reflects the color scheme these devices need to be provided with. I've been also thinking aboutLEDS_MULTICOLOR, but it looks kind awkward for me. Feel free to share other ideas.
LEDS_RGBis fine with me.Quoted 83 lines
drivers/leds/Kconfig menuconfig NEW_LEDS · line 19
Quoted 3 lines
drivers/leds/Kconfig +3
-0+ bool "Color LED Support" + help + This option enables support for Color LED devices, mainly RGB LEDs.Here we should mention that RGB LEDs are handled with HSV interface.
Quoted 22 lines
drivers/leds/Kconfig +7
-0+ Sysfs attribute brightness can be used to set also the color. + For details see Documentation/leds/leds-class.txt. + config LEDS_CLASS tristate "LED Class Support" help@@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +if LEDS_COLOR +comment "Color LED drivers" +endif # LEDS_COLOR + comment "LED drivers" config LEDS_88PM860Xdrivers/leds/Makefile +0
-0drivers/leds/Makefile line 5
Quoted 6 lines
drivers/leds/Makefile +3
-1@@ -1,6 +1,8 @@ # LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += led-core-objs.o +led-core-objs-y := led-core.o +led-core-objs-$(CONFIG_LEDS_COLOR) += led-color-core.oWe'd have to change this to led-rgb-core.o then.
Quoted 33 lines
drivers/leds/Makefile +0
-0obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.odrivers/leds/led-class.c +1
-1@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,if (ret) goto unlock; -if (state ==LED_OFF ) +if (led_is_off (state))led_trigger_remove (led_cdev);led_set_brightness (led_cdev, state);drivers/leds/led-color-core.c new file +11
-0@@ -0,0 +1,33 @@ +/* +* LED Class Color Support +* +* Author: Heiner Kallweit <[email protected]> +* +* 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. +* +*/ +drivers/leds/led-color-core.c line 21
Quoted 10 lines
drivers/leds/led-color-core.c new file +10
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include "leds.h" + +#define LED_HUE_SAT_MASK 0x00ffff00 + +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness brightness;s/brightness/brightness = 0/
Quoted 8 lines
Quoted 6 lines
drivers/leds/led-color-core.c line 24
Quoted 3 lines
drivers/leds/led-color-core.c new file +3
-0+ +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness =0 ;And this check will be redundant.
Not really, because after the following comment the else clause of this check follows.
What about:
enum led_brightness brightness = 0;
/-----
/* Use LED_SET_COLOR to set hue and saturation even if both are zero */ if (value & LED_SET_COLOR || value > LED_FULL)
brightness = value & LED_HUE_SAT_MASK; else if(led_cdev->flags & LED_DEV_CAP_HSV) brightness = led_cdev->brightness & ~LED_BRIGHTNESS_MASK;
/-----
return brightness | min(value & LED_BRIGHTNESS_MASK,
led_cdev->max_brightness);
And the code between "/-----" could be placed in a separate function that would be a no-op if CONFIG_LEDS_RGB isn't defined.
Quoted 22 lines
Quoted 16 lines
Quoted 16 lines
drivers/leds/led-color-core.c new file +9
-0+/* Use LED_SET_COLOR to set hue and saturation even if both are zero */ + else if (value & LED_SET_COLOR || value > LED_FULL) + brightness = value &LED_HUE_SAT_MASK ; + else + brightness = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return brightness | +min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}drivers/leds/led-core.c +0
-0@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data) }Quoted 6 lines
drivers/leds/led-core.c static void led_timer_function(unsigned long data) · line 65
Quoted 3 lines
drivers/leds/led-core.c +1
-1brightness =led_get_brightness (led_cdev); -if (!brightness) { +if (led_is_off (brightness)) {Instead of adding
led_is_off(), couldn't we extendled_get_brightness()?
Quoted 5 lines
led_is_off()is used with different arguments in different places, therefore I think we still need this function.
The brightness variable is used in the subsequent code, so we have to keep also the assignment brightness =led_get_brightness(led_cdev); I'm not sure how this could be improved and what we would gain.
Right, led_is_off() name is misleading as it suggests that it tests the state of LED class device, whereas its purpose is only to test the variable passed. I'd rename it to e.g. is_brightness_set().
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
drivers/leds/led-core.c +0
-0/* Time to switch the LED on. */ brightness = led_cdev->blink_brightness; delay = led_cdev->blink_delay_on;@@ -133,8 +133,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,if (current_brightness) led_cdev->blink_brightness = current_brightness;Quoted 9 lines
drivers/leds/led-core.c static void led_set_software_blink(struct led_classdev *led_cdev, · line 137
Quoted 5 lines
drivers/leds/led-core.c +2
-2if (!led_cdev->blink_brightness) - led_cdev->blink_brightness = led_cdev->max_brightness; - + led_cdev->blink_brightness = +led_confine_brightness (led_cdev,LED_FULL );I am still in favour of passing
led_cdev->max_brightnessinstead ofLED_FULL.
Quoted 1 line
OK
Thanks.
Quoted 118 lines
Quoted 92 lines
Quoted 92 lines
drivers/leds/led-core.c +5
-4led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;@@ -235,12 +235,13 @@ void led_set_brightness(struct led_classdev *led_cdev,* work queue task to avoid problems in case we are called * from hard irq context. */ -if (brightness ==LED_OFF ) { +if (led_is_off (brightness)) { led_cdev->flags |=LED_BLINK_DISABLE ;schedule_work (&led_cdev->set_brightness_work); }else { led_cdev->flags |=LED_BLINK_BRIGHTNESS_CHANGE ; - led_cdev->blink_brightness = brightness; + led_cdev->blink_brightness = +led_confine_brightness (led_cdev, brightness); }return ; }@@ -265,7 +266,7 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nopm);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value) { - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;@@ -280,7 +281,7 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)return -EBUSY ; - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;drivers/leds/leds.h +17
-0@@ -16,17 +16,34 @@#include <linux/rwsem.h> #include <linux/leds.h> +#define LED_BRIGHTNESS_MASK 0x000000ff +static inline int led_get_brightness (struct led_classdev *led_cdev) {return led_cdev->brightness; } +static inline bool led_is_off (enum led_brightness brightness) +{ +return (brightness &LED_BRIGHTNESS_MASK ) ==LED_OFF ; +} +void led_init_core (struct led_classdev *led_cdev);void led_stop_software_blink (struct led_classdev *led_cdev);void led_set_brightness_nopm (struct led_classdev *led_cdev,enum led_brightness value);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value); +#if IS_ENABLED (CONFIG_LEDS_COLOR ) +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_confine_brightness ( +struct led_classdev *led_cdev,enum led_brightness value) +{ +return min (value, led_cdev->max_brightness); +} +#endif extern struct rw_semaphore leds_list_lock;extern struct list_head leds_list;include/linux/leds.h +6
-0@@ -29,8 +29,16 @@ enum led_brightness {LED_OFF =0 ,LED_HALF =127 ,LED_FULL =255 , +/* +* dummy enum value to make gcc use a 32 bit type for the enum +* even if compiled with -fshort-enums. This is needed for +* the enum to store hsv values. +*/ + LED_LEVEL_DUMMY = 0xffffffff, };Quoted 6 lines
include/linux/leds.h enum led_brightness { · line 40
Quoted 1 line
include/linux/leds.h +1
-0+#define LED_SET_COLOR BIT(24)In HSV color model also changing brightness changes the color, which I didn't take into account while proposing this name. We need more accurate name for this macro.
LED_SET_HUE_SAT?Sounds good.
Quoted 15 lines
Quoted 13 lines
include/linux/leds.h +2
-0+struct led_classdev {const char *name;enum led_brightness brightness;@@ -50,6 +58,7 @@ struct led_classdev {#define LED_SYSFS_DISABLE (1 << 22)#define LED_DEV_CAP_FLASH (1 << 23)#define LED_HW_PLUGGABLE (1 << 24) +#define LED_DEV_CAP_HSV (1 << 25)/* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 24.02.2016 um 09:44 schrieb Jacek Anaszewski:
Quoted 185 lines · Heiner Kallweit
Quoted 163 lines
Am 19.02.2016 um 14:59 schrieb Jacek Anaszewski:
Quoted 71 lines
Hi Heiner,
Quoted 58 lines · Heiner Kallweit
Add generic support for color LED's.
Basic idea is to use enum
led_brightnessalso for the hue and saturation color components.This allows to implement the color extension w/o changes to structled_classdev.
A driver that wants to use this extension has to selectLEDS_HSVin itsKconfigentry.Flag
LED_SET_COLORallows to specify that hue / saturation should be overridden even if the provided values are zero.Some examples for writing values to /sys/class/leds/<xx>/brightness:
(now also hex notation can be used)255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
if the LED is switched on again later0x1000000-> switch LED off and set also hue and saturation to 00x00ffff-> set full brightness, full saturation and set hue to 0 (red)Signed-off-by Heiner Kallweit <[email protected]>---
v2:
- move extension-specific code into a separate source file and introduce config symbolLEDS_HSVfor it
- create separate patch for the extension to sysfs
- use variable nameled_cdevas in the rest if the core
- renameto_hsvtoled_validate_brightness
- renameLED_BRIGHTNESS_SET_COLORtoLED_SET_HSV
- introduce helperis_offfor checking whether V part
of a HSV value is zero
v3:
- changeKconfigto use depend instead of select, add help message, and change config symbol toLEDS_COLOR
- change LED core object file name inMakefile
- rename flagLED_SET_HSVtoLED_SET_COLOR
- renameis_offtoled_is_off
- rename led_validate-brightness toled_confine_brightness
- rename variable inled_confine_brightness
- add dummy enumled_brightnessvalue to enforce 32bit enum
- renameled-hsv-core.ctoled-color-core.c
- move check of provided brightness value toled_confine_brightness--- drivers/leds/Kconfig | 11 +++++++++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-class.c | 2 +- drivers/leds/led-color-core.c | 33 +++++++++++++++++++++++++++++++++ drivers/leds/led-core.c | 15 ++++++++------- drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 7 files changed, 82 insertions(+), 9 deletions(-)create mode 100644
drivers/leds/led-color-core.cdrivers/leds/Kconfig +0
-0drivers/leds/Kconfig menuconfig NEW_LEDS · line 16
Quoted 4 lines
drivers/leds/Kconfig +1
-0@@ -13,6 +13,13 @@ menuconfig NEW_LEDS if NEW_LEDS +config LEDS_COLORAfter thinking it over I came to the conclusion that
LEDS_COLORisn't suitable name for multi color LEDs. We already refer to LED colors in the leds-class.txt in the "LED Device Naming" section, with monochrome LEDs on mind.Quoted 6 lines
I think that we should go for
LEDS_RGBto match the name under which this type of LEDs appear on the market, and which reflects the color scheme these devices need to be provided with. I've been also thinking aboutLEDS_MULTICOLOR, but it looks kind awkward for me. Feel free to share other ideas.
LEDS_RGBis fine with me.Quoted 83 lines
drivers/leds/Kconfig menuconfig NEW_LEDS · line 19
Quoted 3 lines
drivers/leds/Kconfig +3
-0+ bool "Color LED Support" + help + This option enables support for Color LED devices, mainly RGB LEDs.Here we should mention that RGB LEDs are handled with HSV interface.
Quoted 22 lines
drivers/leds/Kconfig +7
-0+ Sysfs attribute brightness can be used to set also the color. + For details see Documentation/leds/leds-class.txt. + config LEDS_CLASS tristate "LED Class Support" help@@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +if LEDS_COLOR +comment "Color LED drivers" +endif # LEDS_COLOR + comment "LED drivers" config LEDS_88PM860Xdrivers/leds/Makefile +0
-0drivers/leds/Makefile line 5
Quoted 6 lines
drivers/leds/Makefile +3
-1@@ -1,6 +1,8 @@ # LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += led-core-objs.o +led-core-objs-y := led-core.o +led-core-objs-$(CONFIG_LEDS_COLOR) += led-color-core.oWe'd have to change this to led-rgb-core.o then.
Quoted 33 lines
drivers/leds/Makefile +0
-0obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.odrivers/leds/led-class.c +1
-1@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,if (ret) goto unlock; -if (state ==LED_OFF ) +if (led_is_off (state))led_trigger_remove (led_cdev);led_set_brightness (led_cdev, state);drivers/leds/led-color-core.c new file +11
-0@@ -0,0 +1,33 @@ +/* +* LED Class Color Support +* +* Author: Heiner Kallweit <[email protected]> +* +* 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. +* +*/ +drivers/leds/led-color-core.c line 21
Quoted 10 lines
drivers/leds/led-color-core.c new file +10
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include "leds.h" + +#define LED_HUE_SAT_MASK 0x00ffff00 + +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness brightness;s/brightness/brightness = 0/
Quoted 8 lines
Quoted 6 lines
drivers/leds/led-color-core.c line 24
Quoted 3 lines
drivers/leds/led-color-core.c new file +3
-0+ +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness =0 ;And this check will be redundant.
Not really, because after the following comment the else clause of this check follows.
What about:
enum
led_brightnessbrightness = 0;/-----
/* Use
LED_SET_COLORto set hue and saturation even if both are zero */ if (value &LED_SET_COLOR|| value >LED_FULL)brightness = value & LED_HUE_SAT_MASK; else if(led_cdev->flags & LED_DEV_CAP_HSV) brightness = led_cdev->brightness & ~LED_BRIGHTNESS_MASK;
Quoted 9 lines
/-----
return brightness | min(value &
LED_BRIGHTNESS_MASK,led_cdev->max_brightness);And the code between "/-----" could be placed in a separate function that would be a no-op if
CONFIG_LEDS_RGBisn't defined.
The assumption that checking for CONFIG_LEDS_RGB is more or less the same as checking for flag LED_DEV_CAP_HSV is only true if we have LED's of one kind (RGB or monochrome) only in the system.
As soon as we have LED's of both kinds CONFIG_LEDS_RGB is set also for monochrome drivers. Therefore I think we can't change it in the proposed way.
Quoted 29 lines
Quoted 23 lines
Quoted 17 lines
Quoted 17 lines
drivers/leds/led-color-core.c new file +9
-0+/* Use LED_SET_COLOR to set hue and saturation even if both are zero */ + else if (value & LED_SET_COLOR || value > LED_FULL) + brightness = value &LED_HUE_SAT_MASK ; + else + brightness = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return brightness | +min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}drivers/leds/led-core.c +0
-0@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data) }Quoted 6 lines
drivers/leds/led-core.c static void led_timer_function(unsigned long data) · line 65
Quoted 3 lines
drivers/leds/led-core.c +1
-1brightness =led_get_brightness (led_cdev); -if (!brightness) { +if (led_is_off (brightness)) {Instead of adding
led_is_off(), couldn't we extendled_get_brightness()?Quoted 5 lines
led_is_off()is used with different arguments in different places, therefore I think we still need this function.
The brightness variable is used in the subsequent code, so we have to keep also the assignment brightness =led_get_brightness(led_cdev); I'm not sure how this could be improved and what we would gain.
Quoted 4 lines
Right,
led_is_off()name is misleading as it suggests that it tests the state of LED class device, whereas its purpose is only to test the variable passed. I'd rename it to e.g.is_brightness_set().
I agree that led_is_off is not the best choice. The problem is that led_ is meant to be just the standard function prefix in this driver, it's not meant in a "this led is off" way.
It was my understanding that all function names are supposed to have a led_ prefix, no matter whether the function is defined global or static.
As you propose a name w/o this prefix: Are you fine with static functions not having a name with led_ prefix?
Quoted 139 lines
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
drivers/leds/led-core.c +0
-0/* Time to switch the LED on. */ brightness = led_cdev->blink_brightness; delay = led_cdev->blink_delay_on;@@ -133,8 +133,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,if (current_brightness) led_cdev->blink_brightness = current_brightness;Quoted 9 lines
drivers/leds/led-core.c static void led_set_software_blink(struct led_classdev *led_cdev, · line 137
Quoted 5 lines
drivers/leds/led-core.c +2
-2if (!led_cdev->blink_brightness) - led_cdev->blink_brightness = led_cdev->max_brightness; - + led_cdev->blink_brightness = +led_confine_brightness (led_cdev,LED_FULL );I am still in favour of passing
led_cdev->max_brightnessinstead ofLED_FULL.Quoted 1 line
OK
Thanks.
Quoted 118 lines
Quoted 92 lines
Quoted 92 lines
drivers/leds/led-core.c +5
-4led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;@@ -235,12 +235,13 @@ void led_set_brightness(struct led_classdev *led_cdev,* work queue task to avoid problems in case we are called * from hard irq context. */ -if (brightness ==LED_OFF ) { +if (led_is_off (brightness)) { led_cdev->flags |=LED_BLINK_DISABLE ;schedule_work (&led_cdev->set_brightness_work); }else { led_cdev->flags |=LED_BLINK_BRIGHTNESS_CHANGE ; - led_cdev->blink_brightness = brightness; + led_cdev->blink_brightness = +led_confine_brightness (led_cdev, brightness); }return ; }@@ -265,7 +266,7 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nopm);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value) { - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;@@ -280,7 +281,7 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)return -EBUSY ; - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;drivers/leds/leds.h +17
-0@@ -16,17 +16,34 @@#include <linux/rwsem.h> #include <linux/leds.h> +#define LED_BRIGHTNESS_MASK 0x000000ff +static inline int led_get_brightness (struct led_classdev *led_cdev) {return led_cdev->brightness; } +static inline bool led_is_off (enum led_brightness brightness) +{ +return (brightness &LED_BRIGHTNESS_MASK ) ==LED_OFF ; +} +void led_init_core (struct led_classdev *led_cdev);void led_stop_software_blink (struct led_classdev *led_cdev);void led_set_brightness_nopm (struct led_classdev *led_cdev,enum led_brightness value);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value); +#if IS_ENABLED (CONFIG_LEDS_COLOR ) +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_confine_brightness ( +struct led_classdev *led_cdev,enum led_brightness value) +{ +return min (value, led_cdev->max_brightness); +} +#endif extern struct rw_semaphore leds_list_lock;extern struct list_head leds_list;include/linux/leds.h +6
-0@@ -29,8 +29,16 @@ enum led_brightness {LED_OFF =0 ,LED_HALF =127 ,LED_FULL =255 , +/* +* dummy enum value to make gcc use a 32 bit type for the enum +* even if compiled with -fshort-enums. This is needed for +* the enum to store hsv values. +*/ + LED_LEVEL_DUMMY = 0xffffffff, };Quoted 6 lines
include/linux/leds.h enum led_brightness { · line 40
Quoted 1 line
include/linux/leds.h +1
-0+#define LED_SET_COLOR BIT(24)In HSV color model also changing brightness changes the color, which I didn't take into account while proposing this name. We need more accurate name for this macro.
LED_SET_HUE_SAT?Sounds good.
Quoted 15 lines
Quoted 13 lines
include/linux/leds.h +2
-0+struct led_classdev {const char *name;enum led_brightness brightness;@@ -50,6 +58,7 @@ struct led_classdev {#define LED_SYSFS_DISABLE (1 << 22)#define LED_DEV_CAP_FLASH (1 << 23)#define LED_HW_PLUGGABLE (1 << 24) +#define LED_DEV_CAP_HSV (1 << 25)/* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers
Quoted 192 lines · Heiner Kallweit
Am 24.02.2016 um 09:44 schrieb Jacek Anaszewski:
Quoted 191 lines · Heiner Kallweit
Quoted 163 lines
Am 19.02.2016 um 14:59 schrieb Jacek Anaszewski:
Quoted 71 lines
Hi Heiner,
Quoted 58 lines · Heiner Kallweit
Add generic support for color LED's.
Basic idea is to use enum
led_brightnessalso for the hue and saturation color components.This allows to implement the color extension w/o changes to structled_classdev.
A driver that wants to use this extension has to selectLEDS_HSVin itsKconfigentry.Flag
LED_SET_COLORallows to specify that hue / saturation should be overridden even if the provided values are zero.Some examples for writing values to /sys/class/leds/<xx>/brightness:
(now also hex notation can be used)255 -> set full brightness and keep existing color if set
0 -> switch LED off but keep existing color so that it can be restored
if the LED is switched on again later0x1000000-> switch LED off and set also hue and saturation to 00x00ffff-> set full brightness, full saturation and set hue to 0 (red)Signed-off-by Heiner Kallweit <[email protected]>---
v2:
- move extension-specific code into a separate source file and introduce config symbolLEDS_HSVfor it
- create separate patch for the extension to sysfs
- use variable nameled_cdevas in the rest if the core
- renameto_hsvtoled_validate_brightness
- renameLED_BRIGHTNESS_SET_COLORtoLED_SET_HSV
- introduce helperis_offfor checking whether V part
of a HSV value is zero
v3:
- changeKconfigto use depend instead of select, add help message, and change config symbol toLEDS_COLOR
- change LED core object file name inMakefile
- rename flagLED_SET_HSVtoLED_SET_COLOR
- renameis_offtoled_is_off
- rename led_validate-brightness toled_confine_brightness
- rename variable inled_confine_brightness
- add dummy enumled_brightnessvalue to enforce 32bit enum
- renameled-hsv-core.ctoled-color-core.c
- move check of provided brightness value toled_confine_brightness--- drivers/leds/Kconfig | 11 +++++++++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-class.c | 2 +- drivers/leds/led-color-core.c | 33 +++++++++++++++++++++++++++++++++ drivers/leds/led-core.c | 15 ++++++++------- drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 7 files changed, 82 insertions(+), 9 deletions(-)create mode 100644
drivers/leds/led-color-core.cdrivers/leds/Kconfig +0
-0drivers/leds/Kconfig menuconfig NEW_LEDS · line 16
Quoted 4 lines
drivers/leds/Kconfig +1
-0@@ -13,6 +13,13 @@ menuconfig NEW_LEDS if NEW_LEDS +config LEDS_COLORAfter thinking it over I came to the conclusion that
LEDS_COLORisn't suitable name for multi color LEDs. We already refer to LED colors in the leds-class.txt in the "LED Device Naming" section, with monochrome LEDs on mind.Quoted 6 lines
I think that we should go for
LEDS_RGBto match the name under which this type of LEDs appear on the market, and which reflects the color scheme these devices need to be provided with. I've been also thinking aboutLEDS_MULTICOLOR, but it looks kind awkward for me. Feel free to share other ideas.
LEDS_RGBis fine with me.Quoted 83 lines
drivers/leds/Kconfig menuconfig NEW_LEDS · line 19
Quoted 3 lines
drivers/leds/Kconfig +3
-0+ bool "Color LED Support" + help + This option enables support for Color LED devices, mainly RGB LEDs.Here we should mention that RGB LEDs are handled with HSV interface.
Quoted 22 lines
drivers/leds/Kconfig +7
-0+ Sysfs attribute brightness can be used to set also the color. + For details see Documentation/leds/leds-class.txt. + config LEDS_CLASS tristate "LED Class Support" help@@ -29,6 +36,10 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +if LEDS_COLOR +comment "Color LED drivers" +endif # LEDS_COLOR + comment "LED drivers" config LEDS_88PM860Xdrivers/leds/Makefile +0
-0drivers/leds/Makefile line 5
Quoted 6 lines
drivers/leds/Makefile +3
-1@@ -1,6 +1,8 @@ # LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += led-core-objs.o +led-core-objs-y := led-core.o +led-core-objs-$(CONFIG_LEDS_COLOR) += led-color-core.oWe'd have to change this to led-rgb-core.o then.
Quoted 33 lines
drivers/leds/Makefile +0
-0obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.odrivers/leds/led-class.c +1
-1@@ -53,7 +53,7 @@ static ssize_t brightness_store(struct device *dev,if (ret) goto unlock; -if (state ==LED_OFF ) +if (led_is_off (state))led_trigger_remove (led_cdev);led_set_brightness (led_cdev, state);drivers/leds/led-color-core.c new file +11
-0@@ -0,0 +1,33 @@ +/* +* LED Class Color Support +* +* Author: Heiner Kallweit <[email protected]> +* +* 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. +* +*/ +drivers/leds/led-color-core.c line 21
Quoted 10 lines
drivers/leds/led-color-core.c new file +10
-0+#include <linux/kernel.h> +#include <linux/leds.h> +#include "leds.h" + +#define LED_HUE_SAT_MASK 0x00ffff00 + +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness brightness;s/brightness/brightness = 0/
Quoted 8 lines
Quoted 6 lines
drivers/leds/led-color-core.c line 24
Quoted 3 lines
drivers/leds/led-color-core.c new file +3
-0+ +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness =0 ;And this check will be redundant.
Not really, because after the following comment the else clause of this check follows.
What about:
enum
led_brightnessbrightness = 0;/-----
/* Use
LED_SET_COLORto set hue and saturation even if both are zero */ if (value &LED_SET_COLOR|| value >LED_FULL)brightness = value & LED_HUE_SAT_MASK; else if(led_cdev->flags & LED_DEV_CAP_HSV) brightness = led_cdev->brightness & ~LED_BRIGHTNESS_MASK;/-----
return brightness | min(value &
LED_BRIGHTNESS_MASK,led_cdev->max_brightness);
Quoted 8 lines
Quoted 3 lines
And the code between "/-----" could be placed in a separate function that would be a no-op if
CONFIG_LEDS_RGBisn't defined.The assumption that checking for
CONFIG_LEDS_RGBis more or less the same as checking for flagLED_DEV_CAP_HSVis only true if we have LED's of one kind (RGB or monochrome) only in the system.
As soon as we have LED's of both kindsCONFIG_LEDS_RGBis set also for monochrome drivers. Therefore I think we can't change it in the proposed way.
I've been trying to improve readability of this function and come to conclusion that your original approach from this patch is the best option (if ... else if ... else). Nevertheless we could initialize local brightness variable to 0 and export the >>if else if else<< to e.g. led_rgb_adjust_hue_sat().
I see the logical conflict here: rgb and hue_sat which is specific to hsv color model. We will have to add comments clarifying that.
Quoted 32 lines
Quoted 28 lines
Quoted 22 lines
Quoted 16 lines
Quoted 16 lines
drivers/leds/led-color-core.c new file +9
-0+/* Use LED_SET_COLOR to set hue and saturation even if both are zero */ + else if (value & LED_SET_COLOR || value > LED_FULL) + brightness = value &LED_HUE_SAT_MASK ; + else + brightness = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return brightness | +min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}drivers/leds/led-core.c +0
-0@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data) }Quoted 6 lines
drivers/leds/led-core.c static void led_timer_function(unsigned long data) · line 65
Quoted 3 lines
drivers/leds/led-core.c +1
-1brightness =led_get_brightness (led_cdev); -if (!brightness) { +if (led_is_off (brightness)) {Instead of adding
led_is_off(), couldn't we extendled_get_brightness()?Quoted 5 lines
led_is_off()is used with different arguments in different places, therefore I think we still need this function.
The brightness variable is used in the subsequent code, so we have to keep also the assignment brightness =led_get_brightness(led_cdev); I'm not sure how this could be improved and what we would gain.Quoted 4 lines
Right,
led_is_off()name is misleading as it suggests that it tests the state of LED class device, whereas its purpose is only to test the variable passed. I'd rename it to e.g.is_brightness_set().
Quoted 7 lines
I agree that
led_is_offis not the best choice. The problem is thatled_is meant to be just the standard function prefix in this driver, it's not meant in a "this led is off" way.
It was my understanding that all function names are supposed to have aled_prefix, no matter whether the function is defined global or static.
As you propose a name w/o this prefix: Are you fine with static functions not having a name withled_prefix?
Let's make it __is_brightness_set() - functions with two leading underscores are private by convention, so this will reduce the risk of name clashing virtually to 0.
Quoted 146 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
drivers/leds/led-core.c static void led_set_software_blink(struct led_classdev *led_cdev, · line 137
Quoted 11 lines
drivers/leds/led-core.c +2
-2/* Time to switch the LED on. */ brightness = led_cdev->blink_brightness; delay = led_cdev->blink_delay_on;@@ -133,8 +133,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,if (current_brightness) led_cdev->blink_brightness = current_brightness;if (!led_cdev->blink_brightness) - led_cdev->blink_brightness = led_cdev->max_brightness; - + led_cdev->blink_brightness = +led_confine_brightness (led_cdev,LED_FULL );I am still in favour of passing
led_cdev->max_brightnessinstead ofLED_FULL.Quoted 124 lines
Quoted 1 line
OK
Thanks.
Quoted 118 lines
Quoted 92 lines
Quoted 92 lines
drivers/leds/led-core.c +5
-4led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;@@ -235,12 +235,13 @@ void led_set_brightness(struct led_classdev *led_cdev,* work queue task to avoid problems in case we are called * from hard irq context. */ -if (brightness ==LED_OFF ) { +if (led_is_off (brightness)) { led_cdev->flags |=LED_BLINK_DISABLE ;schedule_work (&led_cdev->set_brightness_work); }else { led_cdev->flags |=LED_BLINK_BRIGHTNESS_CHANGE ; - led_cdev->blink_brightness = brightness; + led_cdev->blink_brightness = +led_confine_brightness (led_cdev, brightness); }return ; }@@ -265,7 +266,7 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nopm);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value) { - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;@@ -280,7 +281,7 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)return -EBUSY ; - led_cdev->brightness =min (value, led_cdev->max_brightness); + led_cdev->brightness =led_confine_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;drivers/leds/leds.h +17
-0@@ -16,17 +16,34 @@#include <linux/rwsem.h> #include <linux/leds.h> +#define LED_BRIGHTNESS_MASK 0x000000ff +static inline int led_get_brightness (struct led_classdev *led_cdev) {return led_cdev->brightness; } +static inline bool led_is_off (enum led_brightness brightness) +{ +return (brightness &LED_BRIGHTNESS_MASK ) ==LED_OFF ; +} +void led_init_core (struct led_classdev *led_cdev);void led_stop_software_blink (struct led_classdev *led_cdev);void led_set_brightness_nopm (struct led_classdev *led_cdev,enum led_brightness value);void led_set_brightness_nosleep (struct led_classdev *led_cdev,enum led_brightness value); +#if IS_ENABLED (CONFIG_LEDS_COLOR ) +enum led_brightness led_confine_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_confine_brightness ( +struct led_classdev *led_cdev,enum led_brightness value) +{ +return min (value, led_cdev->max_brightness); +} +#endif extern struct rw_semaphore leds_list_lock;extern struct list_head leds_list;include/linux/leds.h +6
-0@@ -29,8 +29,16 @@ enum led_brightness {LED_OFF =0 ,LED_HALF =127 ,LED_FULL =255 , +/* +* dummy enum value to make gcc use a 32 bit type for the enum +* even if compiled with -fshort-enums. This is needed for +* the enum to store hsv values. +*/ + LED_LEVEL_DUMMY = 0xffffffff, };Quoted 6 lines
include/linux/leds.h enum led_brightness { · line 40
Quoted 1 line
include/linux/leds.h +1
-0+#define LED_SET_COLOR BIT(24)In HSV color model also changing brightness changes the color, which I didn't take into account while proposing this name. We need more accurate name for this macro.
LED_SET_HUE_SAT?Sounds good.
Quoted 15 lines
Quoted 13 lines
include/linux/leds.h +2
-0+struct led_classdev {const char *name;enum led_brightness brightness;@@ -50,6 +58,7 @@ struct led_classdev {#define LED_SYSFS_DISABLE (1 << 22)#define LED_DEV_CAP_FLASH (1 << 23)#define LED_HW_PLUGGABLE (1 << 24) +#define LED_DEV_CAP_HSV (1 << 25)/* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers --
To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
Best regards, Jacek Anaszewski