lkml.org 
[lkml]   [2020]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v20 12/17] leds: lp55xx: Add multicolor framework support to lp55xx
    From
    Date
    Dan,

    On 4/27/20 8:17 PM, Dan Murphy wrote:
    > Jacek
    >
    > On 4/26/20 11:07 AM, Jacek Anaszewski wrote:
    >> Hi Dan,
    >>
    >> One nit below.
    >>
    >> On 4/23/20 5:55 PM, Dan Murphy wrote:
    >>> Add multicolor framework support for the lp55xx family.
    >>>
    >>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
    >>> ---
    >>>   drivers/leds/Kconfig                      |   1 +
    >>>   drivers/leds/leds-lp5521.c                |  14 +-
    >>>   drivers/leds/leds-lp5523.c                |  14 +-
    >>>   drivers/leds/leds-lp5562.c                |  13 +-
    >>>   drivers/leds/leds-lp55xx-common.c         | 178 +++++++++++++++++++---
    >>>   drivers/leds/leds-lp55xx-common.h         |  11 +-
    >>>   drivers/leds/leds-lp8501.c                |  14 +-
    >>>   include/linux/platform_data/leds-lp55xx.h |   8 +
    >>>   8 files changed, 207 insertions(+), 46 deletions(-)
    >>>
    >> [...]
    >>> diff --git a/drivers/leds/leds-lp55xx-common.h
    >>> b/drivers/leds/leds-lp55xx-common.h
    >>> index b9b1041e8143..a9d7ba3f2fd1 100644
    >>> --- a/drivers/leds/leds-lp55xx-common.h
    >>> +++ b/drivers/leds/leds-lp55xx-common.h
    >>> @@ -12,6 +12,8 @@
    >>>   #ifndef _LEDS_LP55XX_COMMON_H
    >>>   #define _LEDS_LP55XX_COMMON_H
    >>>   +#include <linux/led-class-multicolor.h>
    >>> +
    >>>   enum lp55xx_engine_index {
    >>>       LP55XX_ENGINE_INVALID,
    >>>       LP55XX_ENGINE_1,
    >>> @@ -109,6 +111,9 @@ struct lp55xx_device_config {
    >>>       /* access brightness register */
    >>>       int (*brightness_fn)(struct lp55xx_led *led);
    >>>   +    /* perform brightness value to multiple LEDs */
    >>> +    int (*multicolor_brightness_fn)(struct lp55xx_led *led);
    >>
    >> Isn't some word missing here? IMO it should be rephrased.
    >> Maybe "write multi color brightness to hw" ?
    >>
    > But brightness_fn does not say write_brightness_fn so I think this would
    > look weird to add write to the function name

    I referred to the comment, not to the function name.
    And the comment for brightness_fn is vague IMO.
    So I propose to change both comments:

    s/access brightness register/set LED brightness/
    s/perform brightness value to multiple LEDs/
    set multicolor LED brightness/

    --
    Best regards,
    Jacek Anaszewski

    \
     
     \ /
      Last update: 2020-04-27 21:42    [W:3.281 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site