Re: [PATCH v15 RESEND 1/2] leds: rgb: mt6370: Add MediaTek MT6370 current sink type LED Indicator support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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