From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932781AbcCRNK2 (ORCPT ); Fri, 18 Mar 2016 09:10:28 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:39217 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932158AbcCRNKZ (ORCPT ); Fri, 18 Mar 2016 09:10:25 -0400 X-AuditID: cbfec7f5-f79b16d000005389-ae-56ebfe3ea1fd Message-id: <56EBFE3D.4000308@samsung.com> Date: Fri, 18 Mar 2016 14:10:21 +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> In-reply-to: <56EB0B4D.6040809@gmail.com> Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrBLMWRmVeSWpSXmKPExsVy+t/xq7p2/16HGdy7xG+x6P0MVovLu+aw WWx9s47Rgdlj56y77B6fN8kFMEVx2aSk5mSWpRbp2yVwZazo7WArOCRbcfGYYAPjDLEuRk4O CQETiUMfNzBD2GISF+6tZ+ti5OIQEljKKPH2+idWCOcZo8S+Y9MZQap4BbQkNjx+wQJiswio Slze9AQsziZgKPHzxWsmEFtUIELiz+l9rBD1ghI/Jt8DqxcB6p3weg0biM0sUCnxrO01mC0s 4Cox7cwasCuEBDIlFn1sBOvlFNCUuHHlGBNEvZnEo5Z1zBC2vMTmNW+ZJzAKzEKyYhaSsllI yhYwMq9iFE0tTS4oTkrPNdIrTswtLs1L10vOz93ECAnPrzsYlx6zOsQowMGoxMP7oup1mBBr YllxZe4hRgkOZiUR3oO/gUK8KYmVValF+fFFpTmpxYcYpTlYlMR5Z+56HyIkkJ5YkpqdmlqQ WgSTZeLglGpgVF6tmH7hBI+dh1/TvVeT7nBsdD9z6FdByiqLR07qbN9ecv3OT36kyj3rplRi /Rpr+wNNu1v7tA74l+bzGZYVx8ySEZk99eSG118OnSo7Hxhqt2qZ6O67LU25U8Lm8p1PFd2T qv2Y4ZFEjrV+TPaV921fe773dHyMYps08cL5H5bzNX4fNFWYq8RSnJFoqMVcVJwIAJgLJNdL AgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. >>> 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. >>> } >>> } >>> 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