Re: [PATCH v11 5/5] leds: mt6360: Add LED driver for MT6360

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Gene,

On 12/2/20 11:46 AM, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> 3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for
> moonlight LED.
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>   drivers/leds/Kconfig       |  13 +
>   drivers/leds/Makefile      |   1 +
>   drivers/leds/leds-mt6360.c | 827 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 841 insertions(+)
>   create mode 100644 drivers/leds/leds-mt6360.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 1c181df..4f533bc 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -271,6 +271,19 @@ config LEDS_MT6323
>   	  This option enables support for on-chip LED drivers found on
>   	  Mediatek MT6323 PMIC.
>   
> +config LEDS_MT6360
> +	tristate "LED Support for Mediatek MT6360 PMIC"
> +	depends on LEDS_CLASS && OF
> +	depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
> +	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
> +	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
> +	depends on MFD_MT6360
> +	help
> +	  This option enables support for dual Flash LED drivers found on
> +	  Mediatek MT6360 PMIC.
> +	  Independent current sources supply for each flash LED support torch
> +	  and strobe mode.
> +
>   config LEDS_S3C24XX
>   	tristate "LED Support for Samsung S3C24XX GPIO LEDs"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index c2c7d7a..5596427 100644
[...]
> +static int mt6360_led_probe(struct platform_device *pdev)
> +{
> +	struct mt6360_priv *priv;
> +	struct fwnode_handle *child;
> +	size_t count;
> +	int i = 0, ret;
> +
> +	count = device_get_child_node_count(&pdev->dev);
> +	if (!count || count > MT6360_MAX_LEDS) {
> +		dev_err(&pdev->dev, "No child node or node count over max led number %lu\n", count);
> +		return -EINVAL;
> +	}
> +
> +	priv = devm_kzalloc(&pdev->dev, struct_size(priv, leds, count), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->leds_count = count;
> +	priv->dev = &pdev->dev;
> +	mutex_init(&priv->lock);
> +
> +	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!priv->regmap) {
> +		dev_err(&pdev->dev, "Failed to get parent regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	device_for_each_child_node(&pdev->dev, child) {
> +		struct mt6360_led *led = priv->leds + i;
> +		struct led_init_data init_data = { .fwnode = child, };
> +		u32 reg, led_color;
> +
> +		ret = fwnode_property_read_u32(child, "color", &led_color);
> +		if (ret)
> +			goto out_flash_release;
> +
> +		if (led_color == LED_COLOR_ID_RGB || led_color == LED_COLOR_ID_MULTI)
> +			reg = MT6360_VIRTUAL_MULTICOLOR;
> +		else {
> +			ret = fwnode_property_read_u32(child, "reg", &reg);
> +			if (ret)
> +				goto out_flash_release;
> +
> +			if (reg >= MT6360_MAX_LEDS) {
> +				ret = -EINVAL;
> +				goto out_flash_release;
> +			}
> +		}
> +
> +		if (priv->leds_active & BIT(reg)) {
> +			ret = -EINVAL;
> +			goto out_flash_release;
> +		}
> +		priv->leds_active |= BIT(reg);
> +
> +		led->led_no = reg;
> +		led->priv = priv;
> +
> +		ret = mt6360_init_common_properties(led, &init_data);
> +		if (ret)
> +			goto out_flash_release;
> +
> +		if (reg == MT6360_VIRTUAL_MULTICOLOR ||
> +			(reg >= MT6360_LED_ISNK1 && reg <= MT6360_LED_ISNKML))
> +			ret = mt6360_init_isnk_properties(led, &init_data);
> +		else
> +			ret = mt6360_init_flash_properties(led, &init_data);
> +
> +		if (ret)
> +			goto out_flash_release;
> +
> +		ret = mt6360_led_register(&pdev->dev, led, &init_data);
> +		if (ret)
> +			goto out_flash_release;
> +
> +		i++;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +	return 0;
> +
> +out_flash_release:
> +	mt6360_v4l2_flash_release(priv);

The need for releasing v4l2_flash devices here and not other LEDs is not
obvious at first glance. Of course this is due to the fact that
LED/flash registration functions have devm support but v4l2_flash
doesn't. It would be good to cover it at some point.

If you added that support to
drivers/media/v4l2-core/v4l2-flash-led-class.c then you could
simplify the code in this driver quite nicely.

But it's up to you.

That being said:

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

-- 
Best regards,
Jacek Anaszewski