From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756392AbcCUPgJ (ORCPT ); Mon, 21 Mar 2016 11:36:09 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:64181 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754257AbcCUPgH (ORCPT ); Mon, 21 Mar 2016 11:36:07 -0400 X-AuditID: cbfec7f4-f796c6d000001486-5d-56f014e359d7 Message-id: <56F014DC.7060405@samsung.com> Date: Mon, 21 Mar 2016 16:35:56 +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> In-reply-to: <56EDA471.1060801@gmail.com> Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrBLMWRmVeSWpSXmKPExsVy+t/xa7qPRT6EGVx8I2mx6P0MVovLu+aw WWx9s47Rgdlj56y77B6fN8kFMEVx2aSk5mSWpRbp2yVwZaz6NoG5oFetYsnXu6wNjJNluxg5 OSQETCQOz+tnh7DFJC7cW8/WxcjFISSwlFHi6/8lTBDOM0aJycd+MXcxcnDwCmhJTHigCWKy CKhKXD0lCNLLJmAo8fPFayYQW1QgQuLP6X2sIDavgKDEj8n3WEBsEZDO12vYQGxmgUqJZ22v wWxhAVeJaWfWMEOsWs4osXPnXLAGTgFNiYXPDzNDNJhJPGpZB2XLS2xe85Z5AqPALCQ7ZiEp m4WkbAEj8ypG0dTS5ILipPRcQ73ixNzi0rx0veT83E2MkPD8soNx8TGrQ4wCHIxKPLwMq96H CbEmlhVX5h5ilOBgVhLh9XgIFOJNSaysSi3Kjy8qzUktPsQozcGiJM47d9f7ECGB9MSS1OzU 1ILUIpgsEwenVAOj0b+KoMpb4jHWW1dMCn6RwNnh5PB9W96Cj0Erllw8phPJps+8gFf8sIJ2 4b6HKRYzL7vaNCg+jpun4Pj8ir6Qmunumjuxl9+rz5Kbxq8n/9s7Kji9/L1OW8nVfY0+sot/ azYlRQtpap9vD1v1JjzThrFZIWXZ0qja2Sph1d+jr1zTfrZANFyJpTgj0VCLuag4EQBU0XZN SwIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. >>>>> 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