lkml.org 
[lkml]   [2023]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v16 1/2] leds: rgb: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
    From
    Hi ChiaEn,

    On 1/17/23 09:53, ChiaEn Wu wrote:
    > From: ChiYuan Huang <cy_huang@richtek.com>
    >
    > The MediaTek MT6370 is a highly-integrated smart power management IC,
    > which includes a single cell Li-Ion/Li-Polymer switching battery
    > charger, a USB Type-C & Power Delivery (PD) controller, dual
    > Flash LED current sources, a RGB LED driver, a backlight WLED driver,
    > a display bias driver and a general LDO for portable devices.
    >
    > Add support for the MediaTek MT6370 Current Sink Type LED Indicator
    > driver. It can control four channels current-sink RGB LEDs with 3 modes:
    > constant current, PWM, and breath mode.
    >
    > Co-developed-by: Alice Chen <alice_chen@richtek.com>
    > Signed-off-by: Alice Chen <alice_chen@richtek.com>
    > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
    > Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.com>
    > ---
    >
    > v16
    > - Remove blank line in the head text.
    > - Move 'pwm_duty' to descrease the size of struct mt6370_pdata.
    > - Move the field 'F_RGB_EN' write out from if/else in
    > 'mt6370_isnk_brightness_set'.
    > - Remove 'dev' in struct mt6370_priv.
    > - Remove the proprety reading for 'linux,default-trigger', led core
    > already did it.
    > - Change the module license from 'GPL v2' to 'GPL'
    > In the head text, already deslcred SPDX license as 'GPL-2.0-only'.
    > ---
    > drivers/leds/rgb/Kconfig | 13 +
    > drivers/leds/rgb/Makefile | 1 +
    > drivers/leds/rgb/leds-mt6370-rgb.c | 1009 ++++++++++++++++++++++++++++++++++++
    > 3 files changed, 1023 insertions(+)
    > create mode 100644 drivers/leds/rgb/leds-mt6370-rgb.c
    [...]
    > +static int mt6370_gen_breath_pattern(struct mt6370_priv *priv,
    > + struct led_pattern *pattern, u32 len,
    > + u8 *pattern_val, u32 val_len)
    > +{
    > + enum mt6370_led_ranges sel_range;
    > + struct led_pattern *curr;
    > + unsigned int sel;
    > + u32 val = 0;
    > + int i;
    > +
    > + if (len < P_MAX_PATTERNS && val_len < P_MAX_PATTERNS / 2)
    > + return -EINVAL;
    > +
    > + /*
    > + * Pattern list
    > + * tr1: byte 0, b'[7: 4]
    > + * tr2: byte 0, b'[3: 0]
    > + * tf1: byte 1, b'[7: 4]
    > + * tf2: byte 1, b'[3: 0]
    > + * ton: byte 2, b'[7: 4]
    > + * toff: byte 2, b'[3: 0]
    > + */

    Please provide documentation for the hardware pattern format.
    Compare [0] and [1].

    Otherwise looks good to me:

    Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

    [0] Documentation/leds/leds-sc27xx.rst
    [1] Documentation/leds/leds-qcom-lpg.rst

    --
    Best regards,
    Jacek Anaszewski

    \
     
     \ /
      Last update: 2023-03-27 00:01    [W:6.936 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site