From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755770AbcCVIFX (ORCPT ); Tue, 22 Mar 2016 04:05:23 -0400 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 X-AuditID: cbfec7f4-f796c6d000001486-7e-56f0fcae5763 Message-id: <56F0FCAD.7060709@samsung.com> Date: Tue, 22 Mar 2016 09:05:01 +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> In-reply-to: <56F0309A.8020801@gmail.com> Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrFLMWRmVeSWpSXmKPExsVy+t/xy7rr/nwIMzj6Q9pi0fsZrBaXd81h s9j6Zh2jA7PHzll32T0+b5ILYIrisklJzcksSy3St0vgyni/fA97QZdexfN5G5gaGK8rdzFy ckgImEjsvXGbDcIWk7hwbz2QzcUhJLCUUeLl9k5mCOcZo0T7i7csIFW8AloSXQvmgtksAqoS jQ9+gtlsAoYSP1+8ZgKxRQUiJP6c3scKUS8o8WPyPbAaEaDeCa/XgG1jFqiUeNb2GswWFnCV mHZmDdSyp4wSK5/3sIMkOAU0JSZPvM0E0WAm8ahlHTOELS+xec1b5gmMArOQ7JiFpGwWkrIF jMyrGEVTS5MLipPScw31ihNzi0vz0vWS83M3MUJC9MsOxsXHrA4xCnAwKvHwNmz4ECbEmlhW XJl7iFGCg1lJhHfpV6AQb0piZVVqUX58UWlOavEhRmkOFiVx3rm73ocICaQnlqRmp6YWpBbB ZJk4OKUaGJkfsjdunFfA98pC3mn25twvQXfspqpM7ApJP56mrC/8y2hqqNuFknnJ75d9ORP4 ftM063UcHVq/Lc5qzD8j5jrvUmTnuS+hWo3LrghofJt9OPasbEbWx+jnhj+1JT2X9zyZf719 xobNOfOnCeX9nrux//nF9V9tl36TFuXKEIm8Op/3r8YWT24lluKMREMt5qLiRADiLK/VTQIA AA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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