On Tue, Jan 03, 2023 at 03:00:07PM +0800, ChiaEn Wu wrote:
> From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> + * Alice Chen <alice_chen@xxxxxxxxxxx>
> + *
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", ®);
> + 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", ®);
> + 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
