Re: [PATCH v15 RESEND 2/2] leds: flash: mt6370: Add MediaTek MT6370 flashlight support

[Andy Shevchenko]

On Tue, Jan 03, 2023 at 03:00:09PM +0800, ChiaEn Wu wrote:
> From: Alice Chen <alice_chen@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 MT6370 Flash LED driver. Flash LED in MT6370
> has 2 channels and support torch/strobe mode.

...

>  obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
>  obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
> +obj-$(CONFIG_LEDS_MT6370_FLASH)	+= leds-mt6370-flash.o

Can it be kept ordered?

...

> +struct mt6370_priv {

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

Do you need both of them?

> +	struct mutex lock;
> +	unsigned int fled_strobe_used;
> +	unsigned int fled_torch_used;
> +	unsigned int leds_active;
> +	unsigned int leds_count;
> +	struct mt6370_led leds[];
> +};

...

> +static int _mt6370_flash_brightness_set(struct led_classdev_flash *fl_cdev,
> +					u32 brightness)
> +{
> +	struct mt6370_led *led = to_mt6370_led(fl_cdev, flash);
> +	struct mt6370_priv *priv = led->priv;
> +	struct led_flash_setting *setting = &fl_cdev->brightness;
> +	u32 val = (brightness - setting->min) / setting->step;
> +	int ret, i;
> +
> +	if (led->led_no == MT6370_LED_JOINT) {
> +		u32 flevel[MT6370_MAX_LEDS];
> +
> +		flevel[0] = val / 2;
> +		flevel[1] = val - flevel[0];
> +		for (i = 0; i < MT6370_MAX_LEDS; i++) {
> +			ret = regmap_update_bits(priv->regmap,
> +						 MT6370_REG_FLEDISTRB(i),
> +						 MT6370_ISTROBE_MASK, flevel[i]);
> +			if (ret)
> +				break;
> +		}
> +
> +		return ret;

> +	} else {

Redundant 'else', just use }.

> +		return regmap_update_bits(priv->regmap,
> +					  MT6370_REG_FLEDISTRB(led->led_no),
> +					  MT6370_ISTROBE_MASK, val);

> +	}

No need.

> +}

...

> +	mutex_lock(&priv->lock);
> +	ret = regmap_update_bits(priv->regmap, MT6370_REG_STRBTO,
> +				 MT6370_STRBTO_MASK, val);
> +	mutex_unlock(&priv->lock);

I'm wondering now if you are using regmap lock, if so, what is the difference
between it and your mutex?

Depends on the answer and code flow, maybe one of them can gone.

...

> +	strscpy(config->dev_name, lcdev->dev->kobj.name,

Is it open coded dev_name()?

> +		sizeof(config->dev_name));

...

> +	num = fwnode_property_count_u32(init_data->fwnode, "led-sources");
> +	if (num < 1 || num > MT6370_MAX_LEDS)

Again, is the second part critical?

> +		return dev_err_probe(priv->dev, -EINVAL,
> +				     "Not specified or wrong number of led-sources\n");

...

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

Ditto.

> +		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, };
> +
> +		led->priv = priv;
> +		led->default_state = led_init_default_state_get(init_data.fwnode);
> +
> +		ret = mt6370_init_flash_properties(led, &init_data);
> +		if (ret)

Mind reference count.

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

Ditto.

> +			return ret;
> +
> +		i++;
> +	}

...

> +static const struct of_device_id mt6370_led_of_id[] = {
> +	{ .compatible = "mediatek,mt6370-flashlight" },

> +	{ /* sentinel */ },

A comma is not needed at the terminating entry.

> +};

-- 
With Best Regards,
Andy Shevchenko