From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Subject: Re: [PATCH 1/3] leds: triggers: add support for RGB triggers Date: Tue, 22 Mar 2016 09:05:01 +0100 Message-ID: <56F0FCAD.7060709@samsung.com> References: <56E59FFE.8040203@googlemail.com> <56EAB407.60904@samsung.com> <56EB0B4D.6040809@gmail.com> <56EBFE3D.4000308@samsung.com> <56EDA471.1060801@gmail.com> <56F014DC.7060405@samsung.com> <56F0309A.8020801@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mailout3.w1.samsung.com ([210.118.77.13]:44387 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753264AbcCVIFG (ORCPT ); Tue, 22 Mar 2016 04:05:06 -0400 In-reply-to: <56F0309A.8020801@gmail.com> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Heiner Kallweit Cc: "linux-leds@vger.kernel.org" , Linux Kernel Mailing List On 03/21/2016 06:34 PM, Heiner Kallweit wrote: > Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski: >> On 03/19/2016 08:11 PM, Heiner Kallweit wrote: >>> Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski: >>>> On 03/17/2016 08:53 PM, Heiner Kallweit wrote: >>>>> Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski: >>>>>> Hi Heiner, >>>>>> >>>>>> On 03/13/2016 06:14 PM, Heiner Kallweit wrote: >>>>>>> Add basic support for RGB triggers. Triggers with flag LED_TRIG_CAP_RGB >>>>>>> set are available to RGB LED devices only. >>>>>>> >>>>>>> Signed-off-by: Heiner Kallweit >>>>>>> --- >>>>>>> drivers/leds/led-triggers.c | 15 ++++++++++++--- >>>>>>> include/linux/leds.h | 3 +++ >>>>>>> 2 files changed, 15 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c >>>>>>> index 2181581..3ccf88b 100644 >>>>>>> --- a/drivers/leds/led-triggers.c >>>>>>> +++ b/drivers/leds/led-triggers.c >>>>>>> @@ -30,6 +30,13 @@ static LIST_HEAD(trigger_list); >>>>>>> >>>>>>> /* Used by LED Class */ >>>>>>> >>>>>>> +static inline bool led_trig_check_rgb(struct led_trigger *trig, >>>>>>> + struct led_classdev *led_cdev) >>>>>>> +{ >>>>>>> + return !(trig->flags & LED_TRIG_CAP_RGB) || >>>>>>> + led_cdev->flags & LED_DEV_CAP_RGB; >>>>>>> +} >>>>>> >>>>>> Could you explain what is the purpose of this function? >>>>>> What actually do we want to check here? >>>>>> >>>>> Triggers using RGB functionality can't be used with non-RGB LED's. >>>>> This check checks for such unsupported combinations: >>>>> It returns false if the trigger uses RGB functionality but LED doesn't >>>>> support the RGB extension. >>>> >>>> We need more meaningful name for it. Maybe led_trigger_is_supported() ? >>>> And let's make it no-op for !CONFIG_LEDS_CLASS_RGB case. >>>> >>> OK, led_trigger_is_supported() is better. >>> >>> Making the function a no-op in the non-RGB case would have some impact: >>> We'd have to make sure that all public trigger functions are a de-facto no-op >>> for RGB triggers (at least register / unregister). Means we would need >>> something like this in each public trigger function: >>> >>> #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB) >>> if (trig->flags & LED_TRIG_CAP_RGB)) >>> return; >>> #endif >>> >>> I think this would add a lot of overhead and therefore IMHO it's better to >>> not make the check function a no-op. >> >> Wouldn't it suffice to make the no-op returning true? >> Preventing RGB trigger registration for non-RGB LED class configuration >> seems to be different thing, also to be considered. >> > No, it's not sufficient. Let's say the RGB extension is disabled and we have a RGB trigger. > The check is a no-op now (returns always true), therefore the RGB trigger would be displayed > in the list of available triggers also for all non-RGB LED's. If RGB trigger was made dependent on LED RGB class, then the related Kconfig symbol would remain undefined in !CONFIG_LEDS_CLASS_RGB case. > We could maximum remove the "|| led_cdev->flags & LED_DEV_CAP_RGB" from the check if > the RGB extension is disabled. But it's open whether this minimal gain in a non-critical > code path justifies this. > >>>>>>> ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, >>>>>>> const char *buf, size_t count) >>>>>>> { >>>>>>> @@ -52,12 +59,12 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, >>>>>>> down_read(&triggers_list_lock); >>>>>>> list_for_each_entry(trig, &trigger_list, next_trig) { >>>>>>> if (sysfs_streq(buf, trig->name)) { >>>>>>> + if (!led_trig_check_rgb(trig, led_cdev)) >>>>>>> + break; >>>>> >>>>> Check for the case that userspace wants to set a RGB trigger for a non-RGB LED via sysfs. >>>>> >>>>>>> down_write(&led_cdev->trigger_lock); >>>>>>> led_trigger_set(led_cdev, trig); >>>>>>> up_write(&led_cdev->trigger_lock); >>>>>>> - >>>>>>> - up_read(&triggers_list_lock); >>>>>>> - goto unlock; >>>>>>> + break; >>>> >>>> This seems to be an unrelated cleanup. Please submit it separately. >>>> >>> OK >>> >>>>>>> } >>>>>>> } >>>>>>> up_read(&triggers_list_lock); >>>>>>> @@ -84,6 +91,8 @@ ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, >>>>>>> len += sprintf(buf+len, "none "); >>>>>>> >>>>>>> list_for_each_entry(trig, &trigger_list, next_trig) { >>>>>>> + if (!led_trig_check_rgb(trig, led_cdev)) >>>>>>> + continue; >>>>> >>>>> Omit RGB triggers when listing the available triggers for a non-RGB LED via sysfs. >>>>> >>>>>>> if (led_cdev->trigger && !strcmp(led_cdev->trigger->name, >>>>>>> trig->name)) >>>>>>> len += sprintf(buf+len, "[%s] ", trig->name); >>>>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>>>>>> index 58e22e6..07eb074 100644 >>>>>>> --- a/include/linux/leds.h >>>>>>> +++ b/include/linux/leds.h >>>>>>> @@ -248,6 +248,9 @@ enum led_brightness led_hsv_to_rgb(enum led_brightness hsv); >>>>>>> struct led_trigger { >>>>>>> /* Trigger Properties */ >>>>>>> const char *name; >>>>>>> + u8 flags; >>>>>>> +#define LED_TRIG_CAP_RGB BIT(0) >>>>>>> + >>>>>>> void (*activate)(struct led_classdev *led_cdev); >>>>>>> void (*deactivate)(struct led_classdev *led_cdev); >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>> >>>> >>> >>> >>> >> >> > > > -- Best regards, Jacek Anaszewski