leds: core: add generic support for color LED's
PATCH v2 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_HSV 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
--- drivers/leds/Kconfig | 5 +++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-core.c | 17 ++++++++++------- drivers/leds/led-hsv-core.c | 30 ++++++++++++++++++++++++++++++ drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 6 files changed, 74 insertions(+), 8 deletions(-) create mode 100644 drivers/leds/led-hsv-core.c
drivers/leds/Kconfig +5 -0
@@ -29,6 +29,11 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +config LEDS_HSV + bool + depends on LEDS_CLASS + default n + 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) += ledcore.o +ledcore-y := led-core.o +ledcore-$(CONFIG_LEDS_HSV) += led-hsv-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-core.c +10 -7
@@ -62,7 +62,7 @@ static void led_timer_function(unsigned long data) } brightness =led_get_brightness (led_cdev); -if (!brightness) { +if (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_validate_brightness (led_cdev,LED_FULL ); led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;
@@ -225,6 +225,8 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);void led_set_brightness (struct led_classdev *led_cdev,enum led_brightness brightness) { +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness &=LED_BRIGHTNESS_MASK ;/* * In case blinking is on delay brightness setting * until the next timer tick.
@@ -235,12 +237,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 (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_validate_brightness (led_cdev, brightness); }return ; }
@@ -265,7 +268,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;
@@ -280,7 +283,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;
drivers/leds/led-hsv-core.c new file +30 -0
@@ -0,0 +1,30 @@ +/* +* 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_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness ret; + +/* Use LED_SET_HSV to set hue and saturation even if both are zero */ +if (value &LED_SET_HSV || value >LED_FULL ) + ret = value &LED_HUE_SAT_MASK ; +else + ret = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return ret |min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}
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 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_HSV ) +enum led_brightness led_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_validate_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_SIZE_DUMMY = 0xffffffff, }; +#define LED_SET_HSV 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,
Thanks for the update.
Quoted 7 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.
Let's make drivers depending on LEDS_HSV rather than selecting it.
Quoted 4 lines
Flag
LED_SET_HSVallows to specify that hue / saturation should be overridden even if the provided values are zero.
Quoted 8 lines
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)
At first glance this description doesn't justify the need for the flag. One could ask why caller can't decide about colour similarly as they decide about brightness. It would be good to add here some of reasoning from your replies to my review questions.
Quoted 26 lines
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--- drivers/leds/Kconfig | 5 +++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-core.c | 17 ++++++++++------- drivers/leds/led-hsv-core.c | 30 ++++++++++++++++++++++++++++++ drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 6 files changed, 74 insertions(+), 8 deletions(-)create mode 100644
drivers/leds/led-hsv-core.cdrivers/leds/Kconfig +0
-0
drivers/leds/Kconfig config LEDS_CLASS_FLASH · line 33
Quoted 5 lines
drivers/leds/Kconfig +2
-0@@ -29,6 +29,11 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +config LEDS_HSV + bool
Let's add help message too.
Quoted 25 lines
drivers/leds/Kconfig +3
-0+ depends on LEDS_CLASS + default n + comment "LED drivers" config LEDS_88PM860Xdrivers/leds/Makefile +3
-1@@ -1,6 +1,8 @@ # LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += ledcore.o +ledcore-y := led-core.o +ledcore-$(CONFIG_LEDS_HSV) += led-hsv-core.o obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.odrivers/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 (is_off (brightness)) {
s/is_off/led_is_off/
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_validate_brightness (led_cdev,LED_FULL );
This function name doesn't fit here, since term "validation" usually refers to validating user provided data.
led_confine_brightness() ?
And why LED_FULL and not led_cdev->max_brightness?
drivers/leds/led-core.c EXPORT_SYMBOL_GPL(led_stop_software_blink); · line 229
Quoted 9 lines
drivers/leds/led-core.c +2
-0led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;@@ -225,6 +225,8 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);void led_set_brightness (struct led_classdev *led_cdev,enum led_brightness brightness) { +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness &=LED_BRIGHTNESS_MASK ;
Why is this needed here?
Quoted 54 lines
drivers/leds/led-core.c +5
-4/* * In case blinking is on delay brightness setting * until the next timer tick. @@ -235,12 +237,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 (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_validate_brightness (led_cdev, brightness); }return ; }@@ -265,7 +268,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;@@ -280,7 +283,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;drivers/leds/led-hsv-core.c new file +11
-0@@ -0,0 +1,30 @@ +/* +* 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-hsv-core.c line 21
Quoted 10 lines
drivers/leds/led-hsv-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_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness ret;
s/ret/brightness/
Quoted 52 lines
drivers/leds/led-hsv-core.c new file +8
-0+/* Use LED_SET_HSV to set hue and saturation even if both are zero */ +if (value &LED_SET_HSV || value >LED_FULL ) + ret = value &LED_HUE_SAT_MASK ; +else + ret = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return ret |min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}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 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_HSV ) +enum led_brightness led_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_validate_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
include/linux/leds.h enum led_brightness { · line 38
Quoted 10 lines
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_SIZE_DUMMY = 0xffffffff, };
Don't refer to hsv here, since it also fixes generic LED class - brightness values over 255 can be truncated after passing to LED API when -fshort-enums is passed to gcc. I'd also change the name to e.g. LED_FULL_32BIT.
Please split it to a separate patch. This fixes patch d8082827d ("leds: make brightness type consistent across whole subsystem"). You can also add "Fixes" tag, according to the pattern presented in Documentation/SubmittingPatches. This way it will make it able to be added to stable kernel versions.
Quoted 14 lines
include/linux/leds.h +3
-0+#define LED_SET_HSV 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 · 2 lines
Best regards, Jacek Anaszewski
Am 17.02.2016 um 13:33 schrieb Jacek Anaszewski:
Quoted 4 lines
Hi Heiner,
Thanks for the update.
Thanks for the quick review. I'll rework the patch series accordingly. Below are few inquiries.
Quoted 11 lines · Heiner Kallweit
Quoted 7 lines
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.Let's make drivers depending on
LEDS_HSVrather than selecting it.
If somebody wants to build the driver for a LED needing the color extension then the driver won't be offered until he selects the color extension. This might not be easy to find out (unless user checks manually in Kconfig). Is there a specific reason why you prefer depend over select ?
Quoted 100 lines
Quoted 4 lines
Flag
LED_SET_HSVallows to specify that hue / saturation should be overridden even if the provided values are zero.Quoted 8 lines
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)At first glance this description doesn't justify the need for the flag. One could ask why caller can't decide about colour similarly as they decide about brightness. It would be good to add here some of reasoning from your replies to my review questions.
Quoted 26 lines
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--- drivers/leds/Kconfig | 5 +++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-core.c | 17 ++++++++++------- drivers/leds/led-hsv-core.c | 30 ++++++++++++++++++++++++++++++ drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 6 files changed, 74 insertions(+), 8 deletions(-)create mode 100644
drivers/leds/led-hsv-core.cdrivers/leds/Kconfig +0
-0drivers/leds/Kconfig config LEDS_CLASS_FLASH · line 33
Quoted 5 lines
drivers/leds/Kconfig +2
-0@@ -29,6 +29,11 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +config LEDS_HSV + boolLet's add help message too.
Quoted 25 lines
drivers/leds/Kconfig +3
-0+ depends on LEDS_CLASS + default n + comment "LED drivers" config LEDS_88PM860Xdrivers/leds/Makefile +3
-1@@ -1,6 +1,8 @@ # LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += ledcore.o +ledcore-y := led-core.o +ledcore-$(CONFIG_LEDS_HSV) += led-hsv-core.o obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.odrivers/leds/led-core.c +0
-0drivers/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 (is_off (brightness)) {s/is_off/led_is_off/
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_validate_brightness (led_cdev,LED_FULL );This function name doesn't fit here, since term "validation" usually refers to validating user provided data.
Quoted 4 lines
led_confine_brightness()?And why
LED_FULLand notled_cdev->max_brightness?
LED_FULL is reduced to max_brightness anyway by led_validate_brightness. IMHO LED_FULL makes clearer that the intention is: switch it on. max_brightness is a device-specific property that I'd like to hide as far as possible in the generic core code.
Quoted 4 lines
Quoted 4 lines
drivers/leds/led-core.c +0
-0led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;
Quoted 9 lines
drivers/leds/led-core.c EXPORT_SYMBOL_GPL(led_stop_software_blink); · line 229
Quoted 6 lines
drivers/leds/led-core.c +2
-0@@ -225,6 +225,8 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);void led_set_brightness (struct led_classdev *led_cdev,enum led_brightness brightness) { +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness &=LED_BRIGHTNESS_MASK ;Why is this needed here?
I thought about moving this check to another place anyway (to led_validate(confine)_brightness). Reason for the check is that user input via sysfs isn't checked elsewhere.
And setting hue / saturation for a monochrome LED doesn't make sense.
Quoted 126 lines
Quoted 54 lines
drivers/leds/led-core.c +5
-4/* * In case blinking is on delay brightness setting * until the next timer tick. @@ -235,12 +237,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 (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_validate_brightness (led_cdev, brightness); }return ; }@@ -265,7 +268,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;@@ -280,7 +283,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;drivers/leds/led-hsv-core.c new file +11
-0@@ -0,0 +1,30 @@ +/* +* 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-hsv-core.c line 21
Quoted 10 lines
drivers/leds/led-hsv-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_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness ret;s/ret/brightness/
Quoted 59 lines
drivers/leds/led-hsv-core.c new file +8
-0+/* Use LED_SET_HSV to set hue and saturation even if both are zero */ +if (value &LED_SET_HSV || value >LED_FULL ) + ret = value &LED_HUE_SAT_MASK ; +else + ret = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return ret |min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}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 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_HSV ) +enum led_brightness led_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_validate_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 +4
-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.
Quoted 9 lines
include/linux/leds.h enum led_brightness { · line 38
Quoted 3 lines
include/linux/leds.h +2
-0+*/ + LED_SIZE_DUMMY = 0xffffffff, };Don't refer to hsv here, since it also fixes generic LED class - brightness values over 255 can be truncated after passing to LED API when -fshort-enums is passed to gcc. I'd also change the name to e.g.
LED_FULL_32BIT.
Actually I have my doubts that brightness values >255 were ever considered / semantically allowed. LED_HALF is defined as 127 and LED_FULL as 255. Allowing brightness values > LED_FULL doesn't sound very logical. Full is full and more is not possible ..
Quoted 23 lines
Please split it to a separate patch. This fixes patch
d8082827d("leds: make brightness type consistent across whole subsystem"). You can also add "Fixes" tag, according to the pattern presented inDocumentation/SubmittingPatches. This way it will make it able to be added to stable kernel versions.Quoted 14 lines
include/linux/leds.h +3
-0+#define LED_SET_HSV 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
Quoted 17 lines · Heiner Kallweit
Am 17.02.2016 um 13:33 schrieb Jacek Anaszewski:
Quoted 4 lines
Hi Heiner,
Thanks for the update.
Thanks for the quick review. I'll rework the patch series accordingly. Below are few inquiries.
Quoted 9 lines · Heiner Kallweit
Quoted 7 lines
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.
Quoted 6 lines
Quoted 2 lines
Let's make drivers depending on
LEDS_HSVrather than selecting it.If somebody wants to build the driver for a LED needing the color extension then the driver won't be offered until he selects the color extension. This might not be easy to find out (unless user checks manually in
Kconfig). Is there a specific reason why you prefer depend over select ?
"LED Support for Color LEDs" category (I also propose to switch from LED_HSV to LED_COLOR and consequently led-hsv-core.c->led-color-core.c, since HSV is only the method of color representation) will be visible at the top of "LED Support" menu. One can turn it on and color LEDs will appear on the list.
Select is usually preferable in case there is no item in a given menu that can enable the driver. This applies to the cases e.g. when REGMAP_I2C is the dependency.
Quoted 64 lines
Quoted 64 lines
Quoted 4 lines
Flag
LED_SET_HSVallows to specify that hue / saturation should be overridden even if the provided values are zero.Quoted 8 lines
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)At first glance this description doesn't justify the need for the flag. One could ask why caller can't decide about colour similarly as they decide about brightness. It would be good to add here some of reasoning from your replies to my review questions.
Quoted 26 lines
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--- drivers/leds/Kconfig | 5 +++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-core.c | 17 ++++++++++------- drivers/leds/led-hsv-core.c | 30 ++++++++++++++++++++++++++++++ drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 6 files changed, 74 insertions(+), 8 deletions(-)create mode 100644
drivers/leds/led-hsv-core.cdrivers/leds/Kconfig +0
-0drivers/leds/Kconfig config LEDS_CLASS_FLASH · line 33
Quoted 5 lines
drivers/leds/Kconfig +2
-0@@ -29,6 +29,11 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +config LEDS_HSV + boolLet's add help message too.
Quoted 12 lines
drivers/leds/Kconfig +3
-0+ depends on LEDS_CLASS + default n + comment "LED drivers" config LEDS_88PM860Xdrivers/leds/Makefile +0
-0@@ -1,6 +1,8 @@
Quoted 3 lines
Quoted 3 lines
drivers/leds/Makefile line 3
Quoted 3 lines
drivers/leds/Makefile +1
-1# LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += ledcore.o
Please change ledcore.o to led-core-objs.o.
Quoted 33 lines
Quoted 33 lines
Quoted 10 lines
drivers/leds/Makefile +2
-0+ledcore-y := led-core.o +ledcore-$(CONFIG_LEDS_HSV) += led-hsv-core.o obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.odrivers/leds/led-core.c +0
-0drivers/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 (is_off (brightness)) {s/is_off/led_is_off/
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_validate_brightness (led_cdev,LED_FULL );This function name doesn't fit here, since term "validation" usually refers to validating user provided data.
Quoted 8 lines
Quoted 4 lines
led_confine_brightness()?And why
LED_FULLand notled_cdev->max_brightness?
LED_FULLis reduced tomax_brightnessanyway byled_validate_brightness. IMHOLED_FULLmakes clearer that the intention is: switch it on.max_brightnessis a device-specific property that I'd like to hide as far as possible in the generic core code.
Without checking led_validate_brightness() internals it looks like this patch was changing led_set_software_blink() semantics.
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/leds/led-core.c +0
-0led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;Quoted 9 lines
drivers/leds/led-core.c EXPORT_SYMBOL_GPL(led_stop_software_blink); · line 229
Quoted 6 lines
drivers/leds/led-core.c +2
-0@@ -225,6 +225,8 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);void led_set_brightness (struct led_classdev *led_cdev,enum led_brightness brightness) { +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness &=LED_BRIGHTNESS_MASK ;Why is this needed here?
Quoted 4 lines
I thought about moving this check to another place anyway (to led_validate(confine)_brightness). Reason for the check is that user input via sysfs isn't checked elsewhere.
And setting hue / saturation for a monochrome LED doesn't make sense.
Scattering this type of checking over the file makes the code harder to analyze. Please move it to led_confine_brightness().
Quoted 135 lines
Quoted 126 lines
Quoted 54 lines
drivers/leds/led-core.c +5
-4/* * In case blinking is on delay brightness setting * until the next timer tick. @@ -235,12 +237,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 (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_validate_brightness (led_cdev, brightness); }return ; }@@ -265,7 +268,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;@@ -280,7 +283,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;drivers/leds/led-hsv-core.c new file +11
-0@@ -0,0 +1,30 @@ +/* +* 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-hsv-core.c line 21
Quoted 10 lines
drivers/leds/led-hsv-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_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness ret;s/ret/brightness/
Quoted 59 lines
drivers/leds/led-hsv-core.c new file +8
-0+/* Use LED_SET_HSV to set hue and saturation even if both are zero */ +if (value &LED_SET_HSV || value >LED_FULL ) + ret = value &LED_HUE_SAT_MASK ; +else + ret = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return ret |min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}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 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_HSV ) +enum led_brightness led_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_validate_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 +4
-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. Quoted 9 lines
include/linux/leds.h enum led_brightness { · line 38
Quoted 3 lines
include/linux/leds.h +2
-0+*/ + LED_SIZE_DUMMY = 0xffffffff, };Don't refer to hsv here, since it also fixes generic LED class - brightness values over 255 can be truncated after passing to LED API when -fshort-enums is passed to gcc. I'd also change the name to e.g.
LED_FULL_32BIT.
Quoted 4 lines
Actually I have my doubts that brightness values >255 were ever considered / semantically allowed.
LED_HALFis defined as 127 andLED_FULLas 255. Allowing brightness values >LED_FULLdoesn't sound very logical. Full is full and more is not possible ..
I've skimmed throughout the LED class drivers and I don't see any using brightness levels above 255, so we can leave this change in this patch.
What about s/LED_SIZE_DUMMY/LED_LEVEL_DUMMY/ ?
Quoted 26 lines
Quoted 23 lines
Please split it to a separate patch. This fixes patch
d8082827d("leds: make brightness type consistent across whole subsystem"). You can also add "Fixes" tag, according to the pattern presented inDocumentation/SubmittingPatches. This way it will make it able to be added to stable kernel versions.Quoted 14 lines
include/linux/leds.h +3
-0+#define LED_SET_HSV 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 · 2 lines
Best regards, Jacek Anaszewski
Am 18.02.2016 um 11:17 schrieb Jacek Anaszewski:
Quoted 31 lines · Heiner Kallweit
Quoted 17 lines
Am 17.02.2016 um 13:33 schrieb Jacek Anaszewski:
Quoted 4 lines
Hi Heiner,
Thanks for the update.
Thanks for the quick review. I'll rework the patch series accordingly. Below are few inquiries.
Quoted 9 lines · Heiner Kallweit
Quoted 7 lines
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.Quoted 6 lines
Quoted 2 lines
Let's make drivers depending on
LEDS_HSVrather than selecting it.If somebody wants to build the driver for a LED needing the color extension then the driver won't be offered until he selects the color extension. This might not be easy to find out (unless user checks manually in
Kconfig). Is there a specific reason why you prefer depend over select ?"LED Support for Color LEDs" category (I also propose to switch from
LED_HSVtoLED_COLORand consequently led-hsv-core.c->led-color-core.c, since HSV is only the method of color representation) will be visible at the top of "LED Support" menu. One can turn it on and color LEDs will appear on the list.
Quoted 4 lines
Select is usually preferable in case there is no item in a given menu that can enable the driver. This applies to the cases e.g. when
REGMAP_I2Cis the dependency.
OK, thanks for the explanation. I'll consider this when preparing v3 of the patch series.
Quoted 64 lines
Quoted 64 lines
Quoted 64 lines
Quoted 4 lines
Flag
LED_SET_HSVallows to specify that hue / saturation should be overridden even if the provided values are zero.Quoted 8 lines
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)At first glance this description doesn't justify the need for the flag. One could ask why caller can't decide about colour similarly as they decide about brightness. It would be good to add here some of reasoning from your replies to my review questions.
Quoted 26 lines
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--- drivers/leds/Kconfig | 5 +++++ drivers/leds/Makefile | 4 +++- drivers/leds/led-core.c | 17 ++++++++++------- drivers/leds/led-hsv-core.c | 30 ++++++++++++++++++++++++++++++ drivers/leds/leds.h | 17 +++++++++++++++++ include/linux/leds.h | 9 +++++++++ 6 files changed, 74 insertions(+), 8 deletions(-)create mode 100644
drivers/leds/led-hsv-core.cdrivers/leds/Kconfig +0
-0drivers/leds/Kconfig config LEDS_CLASS_FLASH · line 33
Quoted 5 lines
drivers/leds/Kconfig +2
-0@@ -29,6 +29,11 @@ config LEDS_CLASS_FLASH for the flash related features of a LED device. It can be built as a module. +config LEDS_HSV + boolLet's add help message too.
Quoted 12 lines
drivers/leds/Kconfig +3
-0+ depends on LEDS_CLASS + default n + comment "LED drivers" config LEDS_88PM860Xdrivers/leds/Makefile +0
-0@@ -1,6 +1,8 @@
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
drivers/leds/Makefile line 3
Quoted 3 lines
drivers/leds/Makefile +1
-1# LED Core -obj-$(CONFIG_NEW_LEDS) += led-core.o +obj-$(CONFIG_NEW_LEDS) += ledcore.oPlease change ledcore.o to led-core-objs.o.
OK
Quoted 37 lines
Quoted 37 lines
Quoted 37 lines
Quoted 10 lines
drivers/leds/Makefile +2
-0+ledcore-y := led-core.o +ledcore-$(CONFIG_LEDS_HSV) += led-hsv-core.o obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.odrivers/leds/led-core.c +0
-0drivers/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 (is_off (brightness)) {s/is_off/led_is_off/
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_validate_brightness (led_cdev,LED_FULL );This function name doesn't fit here, since term "validation" usually refers to validating user provided data.
led_confine_brightness()?And why
LED_FULLand notled_cdev->max_brightness?
Quoted 8 lines
Quoted 4 lines
LED_FULLis reduced tomax_brightnessanyway byled_validate_brightness. IMHOLED_FULLmakes clearer that the intention is: switch it on.max_brightnessis a device-specific property that I'd like to hide as far as possible in the generic core code.Without checking
led_validate_brightness()internals it looks like this patch was changingled_set_software_blink()semantics.
As far as I can see it doesn't. In monochrome mode led_validate_brightness(led_cdev, LED_FULL) evaluates to led_cdev->max_brightness.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/leds/led-core.c EXPORT_SYMBOL_GPL(led_stop_software_blink); · line 229
Quoted 10 lines
drivers/leds/led-core.c +2
-0led_cdev->blink_delay_on = delay_on; led_cdev->blink_delay_off = delay_off;@@ -225,6 +225,8 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);void led_set_brightness (struct led_classdev *led_cdev,enum led_brightness brightness) { +if (!(led_cdev->flags &LED_DEV_CAP_HSV )) + brightness &=LED_BRIGHTNESS_MASK ;Why is this needed here?
Quoted 8 lines
Quoted 4 lines
I thought about moving this check to another place anyway (to led_validate(confine)_brightness). Reason for the check is that user input via sysfs isn't checked elsewhere.
And setting hue / saturation for a monochrome LED doesn't make sense.Scattering this type of checking over the file makes the code harder to analyze. Please move it to
led_confine_brightness().
That's exactly what I had in mind when saying "thought about moving this check to another place".
Quoted 140 lines
Quoted 135 lines
Quoted 126 lines
Quoted 54 lines
drivers/leds/led-core.c +5
-4/* * In case blinking is on delay brightness setting * until the next timer tick. @@ -235,12 +237,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 (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_validate_brightness (led_cdev, brightness); }return ; }@@ -265,7 +268,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return ;@@ -280,7 +283,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_validate_brightness (led_cdev, value);if (led_cdev->flags &LED_SUSPENDED )return 0 ;drivers/leds/led-hsv-core.c new file +11
-0@@ -0,0 +1,30 @@ +/* +* 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-hsv-core.c line 21
Quoted 10 lines
drivers/leds/led-hsv-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_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value) +{ +enum led_brightness ret;s/ret/brightness/
Quoted 59 lines
drivers/leds/led-hsv-core.c new file +8
-0+/* Use LED_SET_HSV to set hue and saturation even if both are zero */ +if (value &LED_SET_HSV || value >LED_FULL ) + ret = value &LED_HUE_SAT_MASK ; +else + ret = led_cdev->brightness & ~LED_BRIGHTNESS_MASK ; + +return ret |min (value &LED_BRIGHTNESS_MASK , led_cdev->max_brightness); +}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 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_HSV ) +enum led_brightness led_validate_brightness (struct led_classdev *led_cdev, +enum led_brightness value); +#else +static inline enum led_brightness led_validate_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 +4
-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. Quoted 9 lines
include/linux/leds.h enum led_brightness { · line 38
Quoted 3 lines
include/linux/leds.h +2
-0+*/ + LED_SIZE_DUMMY = 0xffffffff, };Don't refer to hsv here, since it also fixes generic LED class - brightness values over 255 can be truncated after passing to LED API when -fshort-enums is passed to gcc. I'd also change the name to e.g.
LED_FULL_32BIT.Quoted 4 lines
Actually I have my doubts that brightness values >255 were ever considered / semantically allowed.
LED_HALFis defined as 127 andLED_FULLas 255. Allowing brightness values >LED_FULLdoesn't sound very logical. Full is full and more is not possible ..
Quoted 6 lines
I've skimmed throughout the LED class drivers and I don't see any using brightness levels above 255, so we can leave this change in this patch.
What about s/LED_SIZE_DUMMY/LED_LEVEL_DUMMY/ ?
Fine with me.
Quoted 28 lines
Quoted 26 lines
Quoted 23 lines
Please split it to a separate patch. This fixes patch
d8082827d("leds: make brightness type consistent across whole subsystem"). You can also add "Fixes" tag, according to the pattern presented inDocumentation/SubmittingPatches. This way it will make it able to be added to stable kernel versions.Quoted 14 lines
include/linux/leds.h +3
-0+#define LED_SET_HSV 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
On 02/18/2016 09:34 PM, Heiner Kallweit wrote:
[...]
Quoted 24 lines
Quoted 24 lines
Quoted 23 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
/* Time to switch the LED on. */ brightness = led_cdev->blink_brightness; delay = led_cdev->blink_delay_on;drivers/leds/led-core.c +2
-2@@ -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_validate_brightness (led_cdev,LED_FULL );This function name doesn't fit here, since term "validation" usually refers to validating user provided data.
Quoted 4 lines
led_confine_brightness()?And why
LED_FULLand notled_cdev->max_brightness?
LED_FULLis reduced tomax_brightnessanyway byled_validate_brightness. IMHOLED_FULLmakes clearer that the intention is: switch it on.max_brightnessis a device-specific property that I'd like to hide as far as possible in the generic core code.
Quoted 6 lines
Quoted 3 lines
Without checking
led_validate_brightness()internals it looks like this patch was changingled_set_software_blink()semantics.As far as I can see it doesn't. In monochrome mode
led_validate_brightness(led_cdev, LED_FULL)evaluates toled_cdev->max_brightness.
I meant that looking at this change alone makes such an impression. Besides, if we now have led_confine_brightness(), then it implies that brightness will be limited up to LED_FULL level, whereas in fact the upper limit will be max_brightness for monochrome LEDs.
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 19.02.2016 um 14:59 schrieb Jacek Anaszewski:
Quoted 29 lines · Heiner Kallweit
[...]
Quoted 27 lines
Quoted 27 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
/* Time to switch the LED on. */ brightness = led_cdev->blink_brightness; delay = led_cdev->blink_delay_on;drivers/leds/led-core.c +2
-2@@ -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_validate_brightness (led_cdev,LED_FULL );This function name doesn't fit here, since term "validation" usually refers to validating user provided data.
Quoted 8 lines
Quoted 4 lines
led_confine_brightness()?And why
LED_FULLand notled_cdev->max_brightness?
LED_FULLis reduced tomax_brightnessanyway byled_validate_brightness. IMHOLED_FULLmakes clearer that the intention is: switch it on.max_brightnessis a device-specific property that I'd like to hide as far as possible in the generic core code.Without checking
led_validate_brightness()internals it looks like this patch was changingled_set_software_blink()semantics.
Quoted 9 lines
Quoted 3 lines
As far as I can see it doesn't. In monochrome mode
led_validate_brightness(led_cdev, LED_FULL)evaluates toled_cdev->max_brightness.I meant that looking at this change alone makes such an impression. Besides, if we now have
led_confine_brightness(), then it implies that brightness will be limited up toLED_FULLlevel, whereas in fact the upper limit will bemax_brightnessfor monochrome LEDs.
Ah, now I get you. Yes, at a first glance it seems like the semantics were changed. However I don't see this as a bad thing if we interpret LED_FULL as "maximum brightness of a device".