Messages in this thread | | | Date | Fri, 18 Mar 2016 14:10:21 +0100 | From | Jacek Anaszewski <> | Subject | Re: [PATCH 1/3] leds: triggers: add support for RGB triggers |
| |
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 <hkallweit1@gmail.com> >>> --- >>> 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
| |