From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Subject: Re: [PATCH] led: core: RfC - add RGB LED handling to the core Date: Mon, 25 Jan 2016 09:41:32 +0100 Message-ID: <56A5DFBC.2020509@samsung.com> References: <5692BEB6.6040807@gmail.com> <56978FB0.4080700@samsung.com> <5699539F.5030102@gmail.com> <569C1655.1030700@gmail.com> <56A4D3ED.1000002@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mailout1.w1.samsung.com ([210.118.77.11]:50321 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751117AbcAYIlf (ORCPT ); Mon, 25 Jan 2016 03:41:35 -0500 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout1.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0O1I004D92T9UJ70@mailout1.w1.samsung.com> for linux-leds@vger.kernel.org; Mon, 25 Jan 2016 08:41:33 +0000 (GMT) In-reply-to: <56A4D3ED.1000002@gmail.com> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Heiner Kallweit Cc: Jacek Anaszewski , linux-leds@vger.kernel.org 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 >>>>> --- >>>>> 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