Re: [PATCH] led: core: RfC - add RGB LED handling to the core

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi Heiner,

Thank you for the patch. We have to ask a question here - what benefits
these modifications bring about. Isn't all of this achievable in the
user space? And I think that RGB LED problem can be boiled down to even
more generic one, namely: how to make all the sub-LEDs controllable with
a single LED class device. This subject pops out from time to time and
it'd be good to have it tackled finally. Please refer to [1] and follow
the links.

[1] http://www.spinics.net/lists/linux-leds/msg04508.html

Best Regards,
Jacek Anaszewski

On 01/10/2016 09:27 PM, Heiner Kallweit wrote:
> When playing with a ThingM Blink(1) USB RGB LED device I found that there
> are few drivers for RGB LED's spread across the kernel and each one
> implements the RGB functionality in its own way.
> Some examples:
> - drivers/hid/hid-thingm.c
> - drivers/usb/misc/usbled.c
> - drivers/leds/leds-bd2802.c
> - drivers/leds/leds-blinkm.c
> ...
> IMHO it would make sense to add generic RGB functionality to the LED core.
>
> Things I didn't like too much in other driver implementations:
> - one led_classdev per color or at least
> - one sysfs attribute per color
> Colors are not really independent therefore I'd prefer one led_classdev
> per RGB LED. Also userspace should be able to change the color with one
> call -> therefore only one sysfs attribute for the RGB values.
>
> Also the current brightness-related functionality should not be effected
> by the RGB extension.
>
> This patch is intended to demonstrate the idea of the extension. Most likely
> it's not ready yet for submission.
>
> Main idea is to base the effective RGB value on a combination of brightness
> and a scaled-to-max RGB value.
> The RGB-related callbacks are basically the same as for brightness.
> RGB functionally can be switched on with a new flag in the led_classdev.flags
> bitmap.
>
> Experimentally I changed the thingm driver to use this extension and it works
> quite well. As one result the driver could be very much simplified.
>
> Any feedback is appreciated.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>   drivers/leds/led-class.c |  62 +++++++++++++++++++++++++++
>   drivers/leds/led-core.c  | 106 +++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/leds/leds.h      |  12 ++++++
>   include/linux/leds.h     |  13 ++++++
>   4 files changed, 193 insertions(+)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 14139c3..5e4c2f2 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -64,6 +64,47 @@ unlock:
>   }
>   static DEVICE_ATTR_RW(brightness);
>
> +static ssize_t rgb_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	u32 rgb_val;
> +
> +	mutex_lock(&led_cdev->led_access);
> +	led_update_rgb_val(led_cdev);
> +	rgb_val = led_get_rgb_val(led_cdev);
> +	mutex_unlock(&led_cdev->led_access);
> +
> +	return sprintf(buf, "0x%06x\n", rgb_val);
> +}
> +
> +static ssize_t rgb_store(struct device *dev, struct device_attribute *attr,
> +			 const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	unsigned long state;
> +	ssize_t ret;
> +
> +	mutex_lock(&led_cdev->led_access);
> +
> +	if (led_sysfs_is_disabled(led_cdev)) {
> +		ret = -EBUSY;
> +		goto unlock;
> +	}
> +
> +	ret = kstrtoul(buf, 0, &state);
> +	if (ret)
> +		goto unlock;
> +
> +	led_set_rgb_val(led_cdev, state);
> +
> +	ret = size;
> +unlock:
> +	mutex_unlock(&led_cdev->led_access);
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(rgb);
> +
>   static ssize_t max_brightness_show(struct device *dev,
>   		struct device_attribute *attr, char *buf)
>   {
> @@ -190,6 +231,16 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>   	char name[64];
>   	int ret;
>
> +	/* FLASH is not supported for RGB LEDs so far
> +	 * and RGB enforces max_brightness = LED_FULL.
> +	 * Initialize the color as white.
> +	 */
> +	if (led_isRGB(led_cdev)) {
> +		led_cdev->flags &= ~LED_DEV_CAP_FLASH;
> +		led_cdev->max_brightness = LED_FULL;
> +		led_cdev->rgb_val = 0xffffff;
> +	}
> +
>   	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>   	if (ret < 0)
>   		return ret;
> @@ -203,6 +254,14 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>   		dev_warn(parent, "Led %s renamed to %s due to name collision",
>   				led_cdev->name, dev_name(led_cdev->dev));
>
> +	if (led_isRGB(led_cdev)) {
> +		ret = device_create_file(led_cdev->dev, &dev_attr_rgb);
> +		if (ret) {
> +			device_unregister(led_cdev->dev);
> +			return ret;
> +		}
> +	}
> +
>   #ifdef CONFIG_LEDS_TRIGGERS
>   	init_rwsem(&led_cdev->trigger_lock);
>   #endif
> @@ -252,6 +311,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>
>   	flush_work(&led_cdev->set_brightness_work);
>
> +	if (led_isRGB(led_cdev))
> +		device_remove_file(led_cdev->dev, &dev_attr_rgb);
> +
>   	device_unregister(led_cdev->dev);
>
>   	down_write(&leds_list_lock);
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 19e1e60..6563bd3 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -25,6 +25,48 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>   LIST_HEAD(leds_list);
>   EXPORT_SYMBOL_GPL(leds_list);
>
> +static void led_set_rgb_raw(struct led_classdev *cdev, u32 rgb)
> +{
> +	u32 red_raw = (rgb >> 16) & 0xff;
> +	u32 green_raw = (rgb >> 8) & 0xff;
> +	u32 blue_raw = rgb & 0xff;
> +	u32 max_raw, red, green, blue;
> +
> +	max_raw = max(red_raw, green_raw);
> +	if (blue_raw > max_raw)
> +		max_raw = blue_raw;
> +
> +	if (!max_raw) {
> +		cdev->brightness = 0;
> +		cdev->rgb_val = 0;
> +		return;
> +	}
> +
> +	red = DIV_ROUND_CLOSEST(red_raw * LED_FULL, max_raw);
> +	green = DIV_ROUND_CLOSEST(green_raw * LED_FULL, max_raw);
> +	blue = DIV_ROUND_CLOSEST(blue_raw * LED_FULL, max_raw);
> +
> +	cdev->brightness = max_raw;
> +	cdev->rgb_val = (red << 16) + (green << 8) + blue;
> +}
> +
> +static u32 led_get_rgb_raw(struct led_classdev *cdev, bool delayed)
> +{
> +	u32 rgb = delayed ? cdev->delayed_rgb_value : cdev->rgb_val;
> +	u32 brightness = delayed ? cdev->delayed_set_value :
> +			 cdev->brightness;
> +	u32 red = (rgb >> 16) & 0xff;
> +	u32 green = (rgb >> 8) & 0xff;
> +	u32 blue = rgb & 0xff;
> +	u32 red_raw, green_raw, blue_raw;
> +
> +	red_raw = DIV_ROUND_CLOSEST(red * brightness, LED_FULL);
> +	green_raw = DIV_ROUND_CLOSEST(green * brightness, LED_FULL);
> +	blue_raw = DIV_ROUND_CLOSEST(blue * brightness, LED_FULL);
> +
> +	return (red_raw << 16) + (green_raw << 8) + blue_raw;
> +}
> +
>   static void led_timer_function(unsigned long data)
>   {
>   	struct led_classdev *led_cdev = (void *)data;
> @@ -91,6 +133,21 @@ static void set_brightness_delayed(struct work_struct *ws)
>   		led_cdev->flags &= ~LED_BLINK_DISABLE;
>   	}
>
> +	if (led_isRGB(led_cdev)) {
> +		u32 rgb = led_get_rgb_raw(led_cdev, true);
> +
> +		if (led_cdev->rgb_set)
> +			led_cdev->rgb_set(led_cdev, rgb);
> +		else if (led_cdev->rgb_set_blocking)
> +			ret = led_cdev->rgb_set_blocking(led_cdev, rgb);
> +		else
> +			ret = -EOPNOTSUPP;
> +		if (ret < 0)
> +			dev_err(led_cdev->dev,
> +				"Setting LED RGB value failed (%d)\n", ret);
> +		return;
> +	}
> +
>   	if (led_cdev->brightness_set)
>   		led_cdev->brightness_set(led_cdev, led_cdev->delayed_set_value);
>   	else if (led_cdev->brightness_set_blocking)
> @@ -229,9 +286,35 @@ void led_set_brightness(struct led_classdev *led_cdev,
>   }
>   EXPORT_SYMBOL_GPL(led_set_brightness);
>
> +void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val)
> +{
> +	if (!led_isRGB(led_cdev) || (led_cdev->flags & LED_SUSPENDED))
> +		return;
> +
> +	led_set_rgb_raw(led_cdev, rgb_val);
> +
> +	if (led_cdev->rgb_set) {
> +		led_cdev->rgb_set(led_cdev, rgb_val);
> +		return;
> +	}
> +
> +	/* If RGB setting can sleep, delegate it to a work queue task */
> +	led_cdev->delayed_set_value = led_cdev->brightness;
> +	led_cdev->delayed_rgb_value = led_cdev->rgb_val;
> +	schedule_work(&led_cdev->set_brightness_work);
> +}
> +EXPORT_SYMBOL_GPL(led_set_rgb_val);
> +
>   void led_set_brightness_nopm(struct led_classdev *led_cdev,
>   			      enum led_brightness value)
>   {
> +	if (led_isRGB(led_cdev) && led_cdev->rgb_set) {
> +		u32 rgb = led_get_rgb_raw(led_cdev, false);
> +
> +		led_cdev->rgb_set(led_cdev, rgb);
> +		return;
> +	}
> +
>   	/* Use brightness_set op if available, it is guaranteed not to sleep */
>   	if (led_cdev->brightness_set) {
>   		led_cdev->brightness_set(led_cdev, value);
> @@ -240,6 +323,7 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
>
>   	/* If brightness setting can sleep, delegate it to a work queue task */
>   	led_cdev->delayed_set_value = value;
> +	led_cdev->delayed_rgb_value = led_cdev->rgb_val;
>   	schedule_work(&led_cdev->set_brightness_work);
>   }
>   EXPORT_SYMBOL_GPL(led_set_brightness_nopm);
> @@ -267,6 +351,12 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
>   	if (led_cdev->flags & LED_SUSPENDED)
>   		return 0;
>
> +	if (led_isRGB(led_cdev) && led_cdev->rgb_set_blocking) {
> +		u32 rgb = led_get_rgb_raw(led_cdev, false);
> +
> +		return led_cdev->rgb_set_blocking(led_cdev, rgb);
> +	}
> +
>   	if (led_cdev->brightness_set_blocking)
>   		return led_cdev->brightness_set_blocking(led_cdev,
>   							 led_cdev->brightness);
> @@ -290,6 +380,22 @@ int led_update_brightness(struct led_classdev *led_cdev)
>   }
>   EXPORT_SYMBOL_GPL(led_update_brightness);
>
> +int led_update_rgb_val(struct led_classdev *led_cdev)
> +{
> +	s32 ret = 0;
> +
> +	if (led_isRGB(led_cdev) && led_cdev->rgb_get) {
> +		ret = led_cdev->rgb_get(led_cdev);
> +		if (ret >= 0) {
> +			led_set_rgb_raw(led_cdev, ret);
> +			return 0;
> +		}
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(led_update_rgb_val);
> +
>   /* Caller must ensure led_cdev->led_access held */
>   void led_sysfs_disable(struct led_classdev *led_cdev)
>   {
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index db3f20d..c733777 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -16,17 +16,29 @@
>   #include <linux/rwsem.h>
>   #include <linux/leds.h>
>
> +static inline bool led_isRGB(struct led_classdev *led_cdev)
> +{
> +	return (led_cdev->flags & LED_RGB) != 0;
> +}
> +
>   static inline int led_get_brightness(struct led_classdev *led_cdev)
>   {
>   	return led_cdev->brightness;
>   }
>
> +static inline u32 led_get_rgb_val(struct led_classdev *led_cdev)
> +{
> +	return led_cdev->rgb_val;
> +}
> +
>   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);
> +void led_set_rgb_val(struct led_classdev *led_cdev, u32 rgb_val);
> +int led_update_rgb_val(struct led_classdev *led_cdev);
>
>   extern struct rw_semaphore leds_list_lock;
>   extern struct list_head leds_list;
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 4429887..83d2912 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -35,6 +35,7 @@ struct led_classdev {
>   	const char		*name;
>   	enum led_brightness	 brightness;
>   	enum led_brightness	 max_brightness;
> +	u32			 rgb_val;
>   	int			 flags;
>
>   	/* Lower 16 bits reflect status */
> @@ -48,6 +49,7 @@ struct led_classdev {
>   #define LED_BLINK_DISABLE	(1 << 21)
>   #define LED_SYSFS_DISABLE	(1 << 22)
>   #define LED_DEV_CAP_FLASH	(1 << 23)
> +#define LED_RGB			(1 << 24)
>
>   	/* Set LED brightness level */
>   	/* Must not sleep. If no non-blocking version can be provided
> @@ -56,15 +58,25 @@ struct led_classdev {
>   	 */
>   	void		(*brightness_set)(struct led_classdev *led_cdev,
>   					  enum led_brightness brightness);
> +
> +	void		(*rgb_set)(struct led_classdev *led_cdev,
> +				   u32 rgb_val);
>   	/*
>   	 * Set LED brightness level immediately - it can block the caller for
>   	 * the time required for accessing a LED device register.
>   	 */
>   	int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>   				       enum led_brightness brightness);
> +
> +	int (*rgb_set_blocking)(struct led_classdev *led_cdev,
> +				u32 rgb_val);
> +
>   	/* Get LED brightness level */
>   	enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
>
> +	/* Get LED RGB val */
> +	s32 (*rgb_get)(struct led_classdev *led_cdev);
> +
>   	/*
>   	 * Activate hardware accelerated blink, delays are in milliseconds
>   	 * and if both are zero then a sensible default should be chosen.
> @@ -90,6 +102,7 @@ struct led_classdev {
>
>   	struct work_struct	set_brightness_work;
>   	int			delayed_set_value;
> +	u32			delayed_rgb_value;
>
>   #ifdef CONFIG_LEDS_TRIGGERS
>   	/* Protects the trigger data below */
>