Hi Heiner,
On 01/24/2016 02:38 PM, Heiner Kallweit wrote:
Am 17.01.2016 um 23:31 schrieb Jacek Anaszewski:
On 01/15/2016 09:16 PM, Heiner Kallweit wrote:
Am 14.01.2016 um 13:08 schrieb 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@xxxxxxxxx>
---
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(+)
[...]
+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;
+}
I think that we shouldn't impose specific way of calculating brightness
depending on the rgb value set. We should just pass value from userspace
to the driver.
Right, setting brightness from userspace is no problem.
But how about led_update_brightness? It's supposed to update the brightness
value in led_cdev with the actual value. And for a RGB LED we have only
the RGB values, so we need to calculate the brightness based on RGB values.
Or do you see a better way?
I thought that you had some device using both brightness and rgb
components settings (no idea if this could be sane in any way).
If there are only three color components, then why not just redefine
brightness to store three components in case of the LEDs with
LED_DEV_CAP_RGB flags set?
My devices just have the three color components (normal 8 bits per color).
Even in the new RGB mode I don't want to break the current brightness-based API
but extend it with RGB functionality. Therefore I think it's best to store
brightness and color separately.
Just one argument: If we store the color components only then we'll
lose the color information if the brightness is set to 0.
And I would like to support e.g. software blink in whatever color.
Currently blinking works properly and brightness value isn't being lost.
We store it in the led_cdev->blink_brightness property. Could you
present exact use case you'd like to support and which is not feasible
with current LED core design?
--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
