From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758463AbcCVQAw (ORCPT ); Tue, 22 Mar 2016 12:00:52 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:19143 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752430AbcCVQAj (ORCPT ); Tue, 22 Mar 2016 12:00:39 -0400 X-AuditID: cbfec7f4-f796c6d000001486-11-56f16c2363f8 Message-id: <56F16C22.7090702@samsung.com> Date: Tue, 22 Mar 2016 17:00:34 +0100 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 MIME-version: 1.0 To: Heiner Kallweit Cc: "linux-leds@vger.kernel.org" , Linux Kernel Mailing List Subject: Re: [PATCH 1/3] leds: triggers: add support for RGB triggers 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> <56F0FCAD.7060709@samsung.com> <56F130C5.4000500@gmail.com> In-reply-to: <56F130C5.4000500@gmail.com> Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrJLMWRmVeSWpSXmKPExsVy+t/xq7rKOR/DDM61ilosej+D1eLyrjls FlvfrGN0YPbYOesuu8fnTXIBTFFcNimpOZllqUX6dglcGRvvP2ItOGFZcfDlRtYGxum6XYyc HBICJhKfv75jh7DFJC7cW8/WxcjFISSwlFHiy+617BDOM0aJq+3nmECqeAW0JN4/eA1mswio SnTM+gnWzSZgKPHzBURcVCBC4s/pfawQ9YISPybfYwGxRYB6J7xewwZiMwtUSjxrew1mCwu4 Skw7s4YZxBYSWMwkcfYJI4jNKaAp0fLyATNEvZnEo5Z1ULa8xOY1b5knMArMQrJiFpKyWUjK FjAyr2IUTS1NLihOSs811CtOzC0uzUvXS87P3cQICdAvOxgXH7M6xCjAwajEw9uw4UOYEGti WXFl7iFGCQ5mJRHeovSPYUK8KYmVValF+fFFpTmpxYcYpTlYlMR55+56HyIkkJ5YkpqdmlqQ WgSTZeLglGpgtPrJvHrSf77bM66wTN18eTPz0eNr+2blO5+/lLN4qfbl0NTkc9s/tF7+YZex ss34ov3jHZ2pC5dXPnyw6MnhrUsSe6/sDd3x7kLl9brFPx3332B8WF54p/emXJ+J6vYFQue+ Wbz+rljavPYvn/YZ6TOTj3h1q0m83aFvtrZF+8mO++zCpevf3C9XYinOSDTUYi4qTgQA4aN5 ikwCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/22/2016 12:47 PM, Heiner Kallweit wrote: > Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski: >> 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. >> > Making a RGB trigger dependent on LED RGB class would mean to enclose all calls to trigger > functions in the RGB trigger like this: > #if IS_ENABLED(CONFIG_LEDS_CLASS_RGB) > trigger_function() > #endif You probably think about the case when we have two triggers in single module, like in the planned {rgb-}heartbeat case? If so this is an argument for having RGB triggers in separate files. > This would apply to led_trigger_(un)register, led_trigger_event, led_trigger_blink, etc. > And I think it wouldn't be too nice to force other kernel modules wanting to implement > a RGB trigger to add these conditional compile statements. What other modules do you have on mind? LED triggers are implemented in their own files. > Alternatively, as mentioned before, we would have to add this to all public trigger functions: > > #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB) > if (trig->flags & LED_TRIG_CAP_RGB)) > return; > #endif > I think this would add significant overhead w/o gaining really something. > >>> 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