lkml.org 
[lkml]   [2023]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v15 RESEND 1/2] leds: rgb: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
    On Tue, Jan 03, 2023 at 03:00:07PM +0800, 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.

    ...

    > +// SPDX-License-Identifier: GPL-2.0-only
    > +/*
    > + * Copyright (C) 2022 Richtek Technology Corp.
    > + *
    > + * Authors:
    > + * ChiYuan Huang <cy_huang@richtek.com>
    > + * Alice Chen <alice_chen@richtek.com>

    > + *

    Redundant blank line.

    > + */

    ...

    > +#include <asm-generic/unaligned.h>

    asm/unaligned should work, no?

    ...

    > +struct mt6370_pdata {
    > + const unsigned int *tfreq;
    > + unsigned int tfreq_len;

    > + u8 pwm_duty;

    You may save a few bytes by moving this to the end of the structure.

    > + u16 reg_rgb1_tr;
    > + s16 reg_rgb_chrind_tr;
    > +};

    ...

    > +struct mt6370_priv {
    > + /* Per LED access lock */
    > + struct mutex lock;

    > + struct device *dev;
    > + struct regmap *regmap;

    Isn't one can be derived from the other?

    > + struct regmap_field *fields[F_MAX_FIELDS];
    > + const struct reg_field *reg_fields;
    > + const struct linear_range *ranges;
    > + struct reg_cfg *reg_cfgs;
    > + const struct mt6370_pdata *pdata;
    > + unsigned int leds_count;
    > + unsigned int leds_active;
    > + struct mt6370_led leds[];
    > +};

    ...

    > +static int mt6370_isnk_brightness_set(struct led_classdev *lcdev,
    > + enum led_brightness level)
    > +{
    > + struct mt6370_led *led = container_of(lcdev, struct mt6370_led, isink);
    > + struct mt6370_priv *priv = led->priv;
    > + unsigned int enable;
    > + int ret;
    > +
    > + mutex_lock(&priv->lock);
    > +
    > + ret = regmap_field_read(priv->fields[F_RGB_EN], &enable);
    > + if (ret)
    > + goto out_unlock;

    > + if (level == 0) {
    > + enable &= ~MT6370_CHEN_BIT(led->index);
    > +
    > + ret = mt6370_set_led_mode(priv, led->index,
    > + MT6370_LED_REG_MODE);
    > + if (ret)
    > + goto out_unlock;
    > +
    > + ret = regmap_field_write(priv->fields[F_RGB_EN], enable);
    > + } else {
    > + enable |= MT6370_CHEN_BIT(led->index);
    > +
    > + ret = mt6370_set_led_brightness(priv, led->index, level);
    > + if (ret)
    > + goto out_unlock;
    > +
    > + ret = regmap_field_write(priv->fields[F_RGB_EN], enable);
    > + }

    Hmm... Wouldn't be below the equivalent?

    if (level == 0) {
    enable &= ~MT6370_CHEN_BIT(led->index);

    ret = mt6370_set_led_mode(priv, led->index,
    MT6370_LED_REG_MODE);
    if (ret)
    goto out_unlock;
    } else {
    enable |= MT6370_CHEN_BIT(led->index);

    ret = mt6370_set_led_brightness(priv, led->index, level);
    if (ret)
    goto out_unlock;

    }

    ret = regmap_field_write(priv->fields[F_RGB_EN], enable);

    (Of course even if (ret) goto... is duplicated, but probably better to leave it
    as is now)

    > +out_unlock:
    > + mutex_unlock(&priv->lock);
    > +
    > + return ret;
    > +}

    ...

    > + fwnode_for_each_child_node(init_data->fwnode, child) {
    > + u32 reg, color;
    > +
    > + ret = fwnode_property_read_u32(child, "reg", &reg);
    > + if (ret || reg > MT6370_LED_ISNK3 ||
    > + priv->leds_active & BIT(reg))

    I don't see where you drop a reference count.

    > + return -EINVAL;
    > +
    > + ret = fwnode_property_read_u32(child, "color", &color);
    > + if (ret)

    Ditto.

    > + return dev_err_probe(dev, ret,
    > + "LED %d, no color specified\n",
    > + led->index);
    > +
    > + priv->leds_active |= BIT(reg);
    > + sub_led[num_color].color_index = color;
    > + sub_led[num_color].channel = reg;
    > + sub_led[num_color].intensity = 0;
    > + num_color++;
    > + }

    ...

    > + count = device_get_child_node_count(dev);
    > + if (!count || count > MT6370_MAX_LEDS)

    Is the second really critical? Can it be just a warning and clamping of the
    counter to the upper limit?

    > + return dev_err_probe(dev, -EINVAL,
    > + "No child node or node count over max LED number %zu\n",
    > + count);

    ...

    > + device_for_each_child_node(dev, child) {
    > + struct mt6370_led *led = priv->leds + i++;
    > + struct led_init_data init_data = { .fwnode = child };
    > + u32 reg, color;
    > +
    > + ret = fwnode_property_read_u32(child, "reg", &reg);
    > + if (ret)

    What about reference count?

    > + return dev_err_probe(dev, ret, "Failed to parse reg property\n");
    > +
    > + if (reg >= MT6370_MAX_LEDS)

    Ditto.

    > + return dev_err_probe(dev, -EINVAL, "Error reg property number\n");
    > +
    > + ret = fwnode_property_read_u32(child, "color", &color);
    > + if (ret)

    Ditto.

    > + return dev_err_probe(dev, ret, "Failed to parse color property\n");
    > +
    > + if (color == LED_COLOR_ID_RGB || color == LED_COLOR_ID_MULTI)
    > + reg = MT6370_VIRTUAL_MULTICOLOR;
    > +
    > + if (priv->leds_active & BIT(reg))

    Ditto.

    > + return dev_err_probe(dev, -EINVAL, "Duplicate reg property\n");
    > +
    > + priv->leds_active |= BIT(reg);
    > +
    > + led->index = reg;
    > + led->priv = priv;
    > +
    > + ret = mt6370_init_led_properties(led, &init_data);
    > + if (ret)

    Ditto.

    > + return ret;
    > +
    > + ret = mt6370_led_register(&pdev->dev, led, &init_data);
    > + if (ret)

    Ditto.

    > + return ret;
    > + }
    > +
    > + return 0;
    > +}

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2023-03-26 23:25    [W:7.311 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site