lkml.org 
[lkml]   [2016]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2016-03-18 14:41    [W:5.432 / U:0.204 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site