Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] leds: mt6360: Add LED driver for MT6360
To:     Gene Chen <gene.chen.richtek@gmail.com>, pavel@ucw.cz,
        matthias.bgg@gmail.com
Cc:     dmurphy@ti.com, linux-leds@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        gene_chen@richtek.com, Wilma.Wu@mediatek.com,
        shufan_lee@richtek.com, cy_huang@richtek.com,
        benjamin.chao@mediatek.com
References: <1591252004-12681-1-git-send-email-gene.chen.richtek@gmail.com>
From:   Jacek Anaszewski <jacek.anaszewski@gmail.com>
Message-ID: <db3fbc4d-0caa-b1fd-6b75-e3dd4027af31@gmail.com>
Date:   Sat, 6 Jun 2020 00:27:05 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <1591252004-12681-1-git-send-email-gene.chen.richtek@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Gene,

Thank you for the patch. Please find my comments in the code below.

On 6/4/20 8:26 AM, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> and 3-channel RGB LED support Register/Flash/Breath Mode
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> base-commit: 098c4adf249c198519a4abebe482b1e6b8c50e47
> ---
>   drivers/leds/Kconfig       |   11 +
>   drivers/leds/Makefile      |    1 +
>   drivers/leds/leds-mt6360.c | 1061 ++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/mfd/mt6360.h |    6 +-
>   4 files changed, 1078 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/leds/leds-mt6360.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index c664d84..c47be91 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -229,6 +229,17 @@ 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_FLASH && OF

I can't find DT bindings for this driver. Neither in this patch,
nor in mfd bindings.

> +	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.
> +	  Support Torch and Strobe mode independently current source.
> +	  Include Low-VF and short protection.
> +
>   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 45235d5..2883b4d 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_LEDS_MIKROTIK_RB532)	+= leds-rb532.o
>   obj-$(CONFIG_LEDS_MLXCPLD)		+= leds-mlxcpld.o
>   obj-$(CONFIG_LEDS_MLXREG)		+= leds-mlxreg.o
>   obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
> +obj-$(CONFIG_LEDS_MT6360)		+= leds-mt6360.o
>   obj-$(CONFIG_LEDS_NET48XX)		+= leds-net48xx.o
>   obj-$(CONFIG_LEDS_NETXBIG)		+= leds-netxbig.o
>   obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
> diff --git a/drivers/leds/leds-mt6360.c b/drivers/leds/leds-mt6360.c
> new file mode 100644
> index 0000000..3e62547
> --- /dev/null
> +++ b/drivers/leds/leds-mt6360.c
> @@ -0,0 +1,1061 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 MediaTek Inc.
> + *
> + * Author: Gene Chen <gene_chen@richtek.com>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/led-class-flash.h>
> +#include <media/v4l2-flash-led-class.h>
> +
> +#include <linux/mfd/mt6360.h>
> +
> +enum {
> +	MT6360_LED_ISINK1 = 0,
> +	MT6360_LED_ISINK2,
> +	MT6360_LED_ISINK3,
> +	MT6360_LED_ISINK4,
> +	MT6360_LED_MAX,
> +};
> +
> +enum {
> +	MT6360_LEDMODE_PWM = 0,
> +	MT6360_LEDMODE_BREATH,
> +	MT6360_LEDMODE_CC,
> +	MT6360_LEDMODE_MAX,
> +};
> +
> +enum {
> +	MT6360_FLED_CH1 = 0,
> +	MT6360_FLED_CH2,
> +	MT6360_FLED_MAX,
> +};
> +
> +/* ILED setting/reg */
> +#define MT6360_SINKCUR_MAX1	(0x0d)
> +#define MT6360_SINKCUR_MAX2	(0x0d)
> +#define MT6360_SINKCUR_MAX3	(0x0d)
> +#define MT6360_SINKCUR_MAX4	(0x1f)

There is much redundancy and needless code obscurity in this
approach. I propose to provide macros for calculating particular
register value basing on passed argument.

e.g.

#define MT6360_SINKCUR_MAX(val) (((val) == 4) ? 0x1f : 0x0d)

> +#define MT6360_CURRSEL_REG1	(MT6360_PMU_RGB1_ISNK)
> +#define MT6360_CURRSEL_REG2	(MT6360_PMU_RGB2_ISNK)
> +#define MT6360_CURRSEL_REG3	(MT6360_PMU_RGB3_ISNK)
> +#define MT6360_CURRSEL_REG4	(MT6360_PMU_RGB_ML_ISNK)
> +#define MT6360_CURRSEL_MASK1	(0x0f)
> +#define MT6360_CURRSEL_MASK2	(0x0f)
> +#define MT6360_CURRSEL_MASK3	(0x0f)
> +#define MT6360_CURRSEL_MASK4	(0x1f)
> +#define MT6360_LEDMODE_REG1	(MT6360_PMU_RGB1_ISNK)
> +#define MT6360_LEDMODE_REG2	(MT6360_PMU_RGB2_ISNK)
> +#define MT6360_LEDMODE_REG3	(MT6360_PMU_RGB3_ISNK)
> +#define MT6360_LEDMODE_REG4	(0)
> +#define MT6360_LEDMODE_MASK1	(0xc0)
> +#define MT6360_LEDMODE_MASK2	(0xc0)
> +#define MT6360_LEDMODE_MASK3	(0xc0)
> +#define MT6360_LEDMODE_MASK4	(0)
> +#define MT6360_PWMDUTY_REG1	(MT6360_PMU_RGB1_DIM)
> +#define MT6360_PWMDUTY_REG2	(MT6360_PMU_RGB2_DIM)
> +#define MT6360_PWMDUTY_REG3	(MT6360_PMU_RGB3_DIM)
> +#define MT6360_PWMDUTY_REG4	(0)
> +#define MT6360_PWMDUTY_MASK1	(0xff)
> +#define MT6360_PWMDUTY_MASK2	(0xff)
> +#define MT6360_PWMDUTY_MASK3	(0xff)
> +#define MT6360_PWMDUTY_MASK4	(0)
> +#define MT6360_PWMFREQ_REG1	(MT6360_PMU_RGB12_Freq)
> +#define MT6360_PWMFREQ_REG2	(MT6360_PMU_RGB12_Freq)
> +#define MT6360_PWMFREQ_REG3	(MT6360_PMU_RGB34_Freq)
> +#define MT6360_PWMFREQ_REG4	(0)
> +#define MT6360_PWMFREQ_MASK1	(0xe0)
> +#define MT6360_PWMFREQ_MASK2	(0x1c)
> +#define MT6360_PWMFREQ_MASK3	(0xe0)
> +#define MT6360_PWMFREQ_MASK4	(0)
> +#define MT6360_BREATH_REGBASE1	(MT6360_PMU_RGB1_Tr)
> +#define MT6360_BREATH_REGBASE2	(MT6360_PMU_RGB2_Tr)
> +#define MT6360_BREATH_REGBASE3	(MT6360_PMU_RGB3_Tr)
> +#define MT6360_BREATH_REGBASE4	(0)
> +#define MT6360_LEDEN_MASK1	(0x80)
> +#define MT6360_LEDEN_MASK2	(0x40)
> +#define MT6360_LEDEN_MASK3	(0x20)
> +#define MT6360_LEDEN_MASK4	(0x10)
> +#define MT6360_LEDEN_REG	(MT6360_PMU_RGB_EN)
> +#define MT6360_LEDALLEN_MASK	(0xf0)
> +
> +#define MT6360_CHRIND_MASK	(0x08)
> +
> +/* pattern order -> toff, tr1, tr2, ton, tf1, tf2 */
> +#define MT6360_BRPATTERN_NUM	(6)
> +#define MT6360_BREATHREG_NUM	(3)
> +
> +/* FLED setting */
> +#define MT6360_CSENABLE_REG1	(MT6360_PMU_FLED_EN)
> +#define MT6360_CSENABLE_MASK1	(0x02)
> +#define MT6360_CSENABLE_REG2	(MT6360_PMU_FLED_EN)
> +#define MT6360_CSENABLE_MASK2	(0x01)
> +#define MT6360_TORBRIGHT_MAX1	(0x1f)
> +#define MT6360_TORBRIGHT_MAX2	(0x1f)
> +#define MT6360_TORBRIGHT_REG1	(MT6360_PMU_FLED1_TOR_CTRL)
> +#define MT6360_TORBRIGHT_MASK1	(0x1f)
> +#define MT6360_STRBRIGHT_REG1	(MT6360_PMU_FLED1_STRB_CTRL2)
> +#define MT6360_STRBRIGHT_MASK1	(0x7f)
> +#define MT6360_TORBRIGHT_REG2	(MT6360_PMU_FLED2_TOR_CTRL)
> +#define MT6360_TORBRIGHT_MASK2	(0x1f)
> +#define MT6360_STRBRIGHT_REG2	(MT6360_PMU_FLED2_STRB_CTRL2)
> +#define MT6360_STRBRIGHT_MASK2	(0x7f)
> +#define MT6360_TORENABLE_REG1	(MT6360_PMU_FLED_EN)
> +#define MT6360_TORENABLE_MASK1	(0x08)
> +#define MT6360_TORENABLE_REG2	(MT6360_PMU_FLED_EN)
> +#define MT6360_TORENABLE_MASK2	(0x08)
> +#define MT6360_STRBENABLE_REG1	(MT6360_PMU_FLED_EN)
> +#define MT6360_STRBENABLE_MASK1 (0x06)
> +#define MT6360_STRBENABLE_REG2	(MT6360_PMU_FLED_EN)
> +#define MT6360_STRBENABLE_MASK2 (0x04)
> +#define MT6360_STRBTIMEOUT_REG	(MT6360_PMU_FLED_STRB_CTRL)
> +#define MT6360_STRBTIMEOUT_MASK	(0x7f)
> +#define MT6360_TORCHCUR_MIN	(25000)
> +#define MT6360_TORCHCUR_STEP	(12500)
> +#define MT6360_TORCHCUR_MAX	(400000)
> +#define MT6360_STROBECUR_MIN	(50000)
> +#define MT6360_STROBECUR_STEP	(12500)
> +#define MT6360_STROBECUR_MAX	(1500000)
> +#define MT6360_STRBTIMEOUT_MIN	(64000)
> +#define MT6360_STRBTIMEOUT_STEP	(32000)
> +#define MT6360_STRBTIMEOUT_MAX	(2432000)
> +
> +#define MT6360_MASK_ULTRA_STROBE	(0x80)
> +#define MT6360_SHIFT_ULTRA_STROBE	(7)
> +
> +#define MT6360_FLEDSUPPORT_FAULTS	(LED_FAULT_UNDER_VOLTAGE |\
> +					 LED_FAULT_SHORT_CIRCUIT |\
> +					 LED_FAULT_INPUT_VOLTAGE |\
> +					 LED_FAULT_TIMEOUT)
> +
> +struct mt6360_led_platform_data {
> +	u32 rgbon_sync;
> +	u32 fled1_ultraistrb;
> +	u32 fled2_ultraistrb;
> +};
> +
> +struct breath_element_cfg {
> +	/* base, step in ms */
> +	unsigned int base;
> +	unsigned int step;
> +	unsigned int maxval;
> +	unsigned int reg_offset;
> +	unsigned int reg_mask;

With the change of approach you could avoid keeping these
values like this.

> +};
> +
> +struct mt6360_led_classdev {
> +	struct led_classdev cdev;
> +	int index;
> +	struct device_node *np;
> +	unsigned int currsel_reg;
> +	unsigned int currsel_mask;
> +	unsigned int enable_mask;
> +	unsigned int mode_reg;
> +	unsigned int mode_mask;
> +	unsigned int pwmduty_reg;
> +	unsigned int pwmduty_mask;
> +	unsigned int pwmfreq_reg;
> +	unsigned int pwmfreq_mask;
> +	unsigned int breath_regbase;

Ditto.

> +};
> +
> +struct mt6360_fled_classdev {
> +	struct led_classdev_flash fl_cdev;
> +	int index;
> +	struct v4l2_flash *v4l2_flash;
> +	struct device_node *np;
> +	unsigned int cs_enable_reg;
> +	unsigned int cs_enable_mask;
> +	unsigned int torch_bright_reg;
> +	unsigned int torch_bright_mask;
> +	unsigned int torch_enable_reg;
> +	unsigned int torch_enable_mask;
> +	unsigned int strobe_bright_reg;
> +	unsigned int strobe_bright_mask;
> +	unsigned int strobe_enable_reg;
> +	unsigned int strobe_enable_mask;
> +	unsigned int strobe_external_reg;
> +	unsigned int strobe_external_mask;

Ditto.

> +	u32 faults;
> +};
> +
> +struct mt6360_led_data {
> +	struct device *dev;
> +	struct mt6360_led_platform_data *pdata;
> +	struct regmap *regmap;
> +	struct mt6360_led_classdev mtled_cdev[MT6360_LED_MAX];
> +	struct mt6360_fled_classdev mtfled_cdev[MT6360_FLED_MAX];
> +	unsigned long fl_torch_flags;
> +	unsigned long fl_strobe_flags;
> +};
> +
> +static const struct mt6360_led_platform_data def_platform_data = {
> +	.rgbon_sync = 0,
> +	.fled1_ultraistrb = 1,
> +	.fled2_ultraistrb = 1,
> +};
> +
> +static int mt6360_led_brightness_set(struct led_classdev *cdev,
> +				      enum led_brightness brightness)
> +{
> +	struct mt6360_led_classdev *mtled_cdev =
> +					     (struct mt6360_led_classdev *)cdev;
> +	struct mt6360_led_data *mld = dev_get_drvdata(cdev->dev->parent);
> +	int shift, sync_regval = 0, ret;
> +
> +	/* if isink1 user control, set chrind function to sw mode */
> +	if (mtled_cdev->index == MT6360_LED_ISINK1) {
> +		ret = regmap_update_bits(mld->regmap,
> +				   MT6360_PMU_RGB_EN, MT6360_CHRIND_MASK, 0xff);
> +		if (ret < 0)
> +			dev_err(cdev->dev, "disable chrind func fail\n");
> +	}

This may be personal preference, but I would prefer adding newlines
at the end of logical blocks of code.

E.g. like here.

> +	if (brightness == LED_OFF) {
> +		ret = regmap_update_bits(mld->regmap,
> +				  MT6360_LEDEN_REG, mtled_cdev->enable_mask, 0);
> +		if (ret < 0)
> +			return ret;
> +		if (mtled_cdev->mode_reg == 0)
> +			goto out_bright_set;
> +		/* if off, force config to cc_mode */
> +		shift = ffs(mtled_cdev->mode_mask) - 1;
> +		ret = regmap_update_bits(mld->regmap, mtled_cdev->mode_reg,
> +			     mtled_cdev->mode_mask, MT6360_LEDMODE_CC << shift);
> +		if (ret < 0)
> +			dev_err(cdev->dev, "config cc mode fail\n");
> +		goto out_bright_set;
> +	}

Here.

> +	shift = ffs(mtled_cdev->currsel_mask) - 1;
> +	brightness -= 1;
> +	ret = regmap_update_bits(mld->regmap, mtled_cdev->currsel_reg,
> +				 mtled_cdev->currsel_mask, brightness << shift);
> +	if (ret < 0)
> +		return ret;

Here.

And so on for all similar occurrences.

> +	if (mld->pdata->rgbon_sync) {
> +		ret = regmap_read(mld->regmap, MT6360_LEDEN_REG,  &sync_regval);
> +		if (ret < 0)
> +			goto out_bright_set;
> +		ret = regmap_update_bits(mld->regmap,
> +				     MT6360_LEDEN_REG, MT6360_LEDALLEN_MASK, 0);
> +		if (ret < 0)
> +			goto out_bright_set;
> +		sync_regval |= mtled_cdev->enable_mask;
> +		ret = regmap_update_bits(mld->regmap, MT6360_LEDEN_REG,
> +					 MT6360_LEDALLEN_MASK, sync_regval);
> +	} else {
> +		ret = regmap_update_bits(mld->regmap, MT6360_LEDEN_REG,
> +					 mtled_cdev->enable_mask, 0xff);
> +	}
> +out_bright_set:
> +	return ret;
> +}
> +
> +static enum led_brightness mt6360_led_brightness_get(struct led_classdev *cdev)
> +{
> +	struct mt6360_led_classdev *mtled_cdev =
> +					     (struct mt6360_led_classdev *)cdev;
> +	struct mt6360_led_data *mld = dev_get_drvdata(cdev->dev->parent);
> +	unsigned int regval = 0;
> +	int shift = ffs(mtled_cdev->currsel_mask) - 1, ret;
> +
> +	ret = regmap_read(mld->regmap, MT6360_LEDEN_REG, &regval);
> +	if (ret < 0) {
> +		dev_err(cdev->dev, "%s: get enable fail\n", __func__);
> +		return LED_OFF;
> +	}
> +	if (!(regval & mtled_cdev->enable_mask))
> +		return LED_OFF;
> +	ret = regmap_read(mld->regmap, mtled_cdev->currsel_reg, &regval);
> +	if (ret < 0) {
> +		dev_err(cdev->dev, "%s: get isink fail\n", __func__);
> +		return LED_OFF;
> +	}
> +	regval &= mtled_cdev->currsel_mask;
> +	regval >>= shift;
> +	return (regval + 1);
> +}
> +
> +static const unsigned int dim_freqs[] = {
> +	4, 8, 250, 500, 1000, 2000, 4000, 8000,
> +};
> +
> +static int mt6360_led_blink_set(struct led_classdev *cdev,
> +			     unsigned long *delay_on,  unsigned long *delay_off)
> +{
> +	struct mt6360_led_classdev *mtled_cdev =
> +					     (struct mt6360_led_classdev *)cdev;
> +	struct mt6360_led_data *mld = dev_get_drvdata(cdev->dev->parent);
> +	int freq, duty, shift, sum, ret;
> +
> +	if (mtled_cdev->mode_reg == 0)
> +		return -ENOTSUPP;
> +	if (*delay_on == 0 && *delay_off == 0)
> +		*delay_on = *delay_off = 500;
> +	sum = *delay_on + *delay_off;
> +	for (freq = 0; freq < ARRAY_SIZE(dim_freqs); freq++) {
> +		if (sum <= dim_freqs[freq])
> +			break;
> +	}
> +	if (freq == ARRAY_SIZE(dim_freqs)) {
> +		dev_err(cdev->dev, "exceed pwm frequency max\n");
> +		return -EINVAL;
> +	}
> +	/* invert */
> +	freq = ARRAY_SIZE(dim_freqs) - 1 - freq;
> +	shift = ffs(mtled_cdev->pwmfreq_mask) - 1;
> +	ret = regmap_update_bits(mld->regmap, mtled_cdev->pwmfreq_reg,
> +				 mtled_cdev->pwmfreq_mask, freq << shift);
> +	if (ret < 0) {
> +		dev_err(cdev->dev, "Failed to set pwmfreq\n");
> +		return ret;
> +	}
> +	duty = 255 * (*delay_on) / sum;
> +	shift = ffs(mtled_cdev->pwmduty_mask) - 1;
> +	ret = regmap_update_bits(mld->regmap, mtled_cdev->pwmduty_reg,
> +				 mtled_cdev->pwmduty_mask, duty << shift);
> +	if (ret < 0) {
> +		dev_err(cdev->dev, "Failed to set pwmduty\n");
> +		return ret;
> +	}
> +	shift = ffs(mtled_cdev->mode_mask) - 1;
> +	ret = regmap_update_bits(mld->regmap, mtled_cdev->mode_reg,
> +			    mtled_cdev->mode_mask, MT6360_LEDMODE_PWM << shift);
> +	return ret;
> +}
> +
> +#define MT6360_LED_DESC(_id)  {						\
> +	.cdev = {							\
> +		.name = "mt6360_isink" #_id,				\
> +		.max_brightness = MT6360_SINKCUR_MAX##_id,		\
> +		.brightness_set_blocking = mt6360_led_brightness_set,	\
> +		.brightness_get = mt6360_led_brightness_get,		\
> +		.blink_set = mt6360_led_blink_set,			\
> +	},								\
> +	.index = MT6360_LED_ISINK##_id,					\
> +	.currsel_reg = MT6360_CURRSEL_REG##_id,				\
> +	.currsel_mask = MT6360_CURRSEL_MASK##_id,			\
> +	.enable_mask = MT6360_LEDEN_MASK##_id,				\
> +	.mode_reg = MT6360_LEDMODE_REG##_id,				\
> +	.mode_mask = MT6360_LEDMODE_MASK##_id,				\
> +	.pwmduty_reg = MT6360_PWMDUTY_REG##_id,				\
> +	.pwmduty_mask = MT6360_PWMDUTY_MASK##_id,			\
> +	.pwmfreq_reg = MT6360_PWMFREQ_REG##_id,				\
> +	.pwmfreq_mask = MT6360_PWMFREQ_MASK##_id,			\
> +	.breath_regbase = MT6360_BREATH_REGBASE##_id,			\
> +}

Let's better avoid code generation via macros. It horribly hinders code
analysis and debugging.

> +
> +/* ISINK 1/2/3 for RGBLED, ISINK4 for MoonLight */
> +static const struct mt6360_led_classdev def_led_classdev[MT6360_LED_MAX] = {
> +	MT6360_LED_DESC(1),
> +	MT6360_LED_DESC(2),
> +	MT6360_LED_DESC(3),
> +	MT6360_LED_DESC(4),
> +};
> +
> +static inline bool mt6360_fled_check_flags_if_any(unsigned long *flags)
> +{
> +	return (*flags) ? true : false;

Just do "return *flags" and it will be cast to bool automatically.

Although I think that the function is redundant.

> +}
> +
> +static int mt6360_fled_strobe_brightness_set(
> +			   struct led_classdev_flash *fled_cdev, u32 brightness)
> +{
> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> +	struct led_flash_setting *fs = &fled_cdev->brightness;
> +	struct mt6360_fled_classdev *mtfled_cdev = (void *)fled_cdev;
> +	int shift;
> +	u32 val;
> +
> +	val = brightness;
> +	val = (val - fs->min) / fs->step;
> +	shift = ffs(mtfled_cdev->strobe_bright_mask) - 1;
> +	return regmap_update_bits(mld->regmap, mtfled_cdev->strobe_bright_reg,
> +				 mtfled_cdev->strobe_bright_mask, val << shift);
> +}
> +
> +static int mt6360_fled_strobe_brightness_get(
> +			  struct led_classdev_flash *fled_cdev, u32 *brightness)
> +{
> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> +	struct led_flash_setting *fs = &fled_cdev->brightness;
> +	struct mt6360_fled_classdev *mtfled_cdev = (void *)fled_cdev;
> +	int shift, ret;
> +	u32 regval = 0;
> +
> +	ret = regmap_read(mld->regmap, mtfled_cdev->strobe_bright_reg, &regval);
> +	if (ret < 0)
> +		return ret;
> +	regval &= mtfled_cdev->strobe_bright_mask;
> +	shift = ffs(mtfled_cdev->strobe_bright_mask) - 1;
> +	regval >>= shift;
> +	/* convert to microamp value */
> +	*brightness = regval * fs->step + fs->min;
> +	return 0;
> +}
> +
> +static int mt6360_fled_strobe_set(
> +			       struct led_classdev_flash *fled_cdev, bool state)
> +{
> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> +	struct mt6360_fled_classdev *mtfled_cdev = (void *)fled_cdev;
> +	int id = mtfled_cdev->index, ret;
> +
> +	if (!(state ^ test_bit(id, &mld->fl_strobe_flags)))
> +		return 0;
> +	if (mt6360_fled_check_flags_if_any(&mld->fl_torch_flags)) {
> +		dev_err(led_cdev->dev,
> +			"Disable all leds torch [%lu]\n", mld->fl_torch_flags);
> +		return -EINVAL;
> +	}
> +	ret = regmap_update_bits(mld->regmap, mtfled_cdev->cs_enable_reg,
> +				 mtfled_cdev->cs_enable_mask, state ? 0xff : 0);
> +	if (ret < 0) {
> +		dev_err(led_cdev->dev, "Fail to set cs enable [%d]\n", state);
> +		goto out_strobe_set;
> +	}
> +	ret = regmap_update_bits(mld->regmap, mtfled_cdev->strobe_enable_reg,
> +			     mtfled_cdev->strobe_enable_mask, state ? 0xff : 0);
> +	if (ret < 0) {
> +		dev_err(led_cdev->dev, "Fail to set strb enable [%d]\n", state);
> +		goto out_strobe_set;
> +	}
> +	if (state) {
> +		if (!mt6360_fled_check_flags_if_any(&mld->fl_strobe_flags))
> +			usleep_range(5000, 6000);
> +		set_bit(id, &mld->fl_strobe_flags);
> +		mtfled_cdev->faults = 0;
> +	} else {
> +		clear_bit(id, &mld->fl_strobe_flags);
> +		if (!mt6360_fled_check_flags_if_any(&mld->fl_strobe_flags))
> +			usleep_range(400, 500);
> +	}
> +out_strobe_set:
> +	return ret;
> +}
> +
> +static int mt6360_fled_strobe_get(
> +			      struct led_classdev_flash *fled_cdev, bool *state)
> +{
> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> +	struct mt6360_fled_classdev *mtfled_cdev = (void *)fled_cdev;
> +	int id = mtfled_cdev->index;
> +
> +	*state = test_bit(id, &mld->fl_strobe_flags) ? true : false;
> +	return 0;
> +}
> +
> +static int mt6360_fled_strobe_timeout_set(
> +			      struct led_classdev_flash *fled_cdev, u32 timeout)
> +{
> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> +	struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> +	struct led_flash_setting *ts = &fled_cdev->timeout;
> +	int shift, ret;
> +	u32 regval;
> +
> +	regval = (timeout - ts->min) / ts->step;
> +	shift = ffs(MT6360_STRBTIMEOUT_MASK) - 1;
> +	ret = regmap_update_bits(mld->regmap, MT6360_STRBTIMEOUT_REG,
> +				 MT6360_STRBTIMEOUT_MASK, regval << shift);
> +	return ret;
> +}
> +
> +static int mt6360_fled_strobe_fault_get(
> +			       struct led_classdev_flash *fled_cdev, u32 *fault)
> +{
> +	struct mt6360_fled_classdev *mtfled_cdev = (void *)fled_cdev;
> +
> +	*fault = mtfled_cdev->faults;
> +	return 0;
> +}
> +
> +static const struct led_flash_ops mt6360_fled_ops = {
> +	.flash_brightness_set = mt6360_fled_strobe_brightness_set,
> +	.flash_brightness_get = mt6360_fled_strobe_brightness_get,
> +	.strobe_set = mt6360_fled_strobe_set,
> +	.strobe_get = mt6360_fled_strobe_get,
> +	.timeout_set = mt6360_fled_strobe_timeout_set,
> +	.fault_get = mt6360_fled_strobe_fault_get,
> +};
> +
> +static int mt6360_fled_brightness_set(struct led_classdev *led_cdev,
> +				      enum led_brightness brightness)
> +{
> +	struct led_classdev_flash *lcf = lcdev_to_flcdev(led_cdev);
> +	struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> +	struct mt6360_fled_classdev *mtfled_cdev = (void *)lcf;
> +	int id = mtfled_cdev->index, shift, keep, ret;
> +
> +	if (mt6360_fled_check_flags_if_any(&mld->fl_strobe_flags)) {
> +		dev_err(led_cdev->dev,
> +		       "Disable all leds strobe [%lu]\n", mld->fl_strobe_flags);
> +		return -EINVAL;
> +	}
> +	if (brightness == LED_OFF) {
> +		clear_bit(id, &mld->fl_torch_flags);
> +		keep = mt6360_fled_check_flags_if_any(&mld->fl_torch_flags);
> +		ret = regmap_update_bits(mld->regmap,
> +					 mtfled_cdev->torch_enable_reg,
> +					 mtfled_cdev->torch_enable_mask,
> +					 keep ? 0xff : 0);
> +		if (ret < 0) {
> +			dev_err(led_cdev->dev, "Fail to set torch disable\n");
> +			goto out_bright_set;
> +		}
> +		ret = regmap_update_bits(mld->regmap,
> +					 mtfled_cdev->cs_enable_reg,
> +					 mtfled_cdev->cs_enable_mask, 0);
> +		if (ret < 0)
> +			dev_err(led_cdev->dev, "Fail to set torch disable\n");
> +		goto out_bright_set;
> +	}
> +	shift = ffs(mtfled_cdev->torch_bright_mask) - 1;
> +	brightness -= 1;
> +	ret = regmap_update_bits(mld->regmap, mtfled_cdev->torch_bright_reg,
> +			   mtfled_cdev->torch_bright_mask, brightness << shift);
> +	if (ret < 0) {
> +		dev_err(led_cdev->dev,
> +			"Fail to set torch bright [%d]\n", brightness);
> +		goto out_bright_set;
> +	}
> +	ret = regmap_update_bits(mld->regmap, mtfled_cdev->cs_enable_reg,
> +				 mtfled_cdev->cs_enable_mask, 0xff);
> +	if (ret < 0) {
> +		dev_err(led_cdev->dev, "Fail to set cs enable\n");
> +		goto out_bright_set;
> +	}
> +	ret = regmap_update_bits(mld->regmap, mtfled_cdev->torch_enable_reg,
> +				 mtfled_cdev->torch_enable_mask, 0xff);
> +	set_bit(id, &mld->fl_torch_flags);
> +out_bright_set:
> +	return ret;
> +}
> +
> +static enum led_brightness mt6360_fled_brightness_get(
> +						  struct led_classdev *led_cdev)
> +{
> +	struct led_classdev_flash *lcf = lcdev_to_flcdev(led_cdev);
> +	struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> +	struct mt6360_fled_classdev *mtfled_cdev = (void *)lcf;
> +	int id = mtfled_cdev->index, shift, ret;
> +	u32 regval = 0;
> +
> +	if (!test_bit(id, &mld->fl_torch_flags))
> +		return LED_OFF;
> +	ret = regmap_read(mld->regmap, mtfled_cdev->torch_bright_reg, &regval);
> +	if (ret < 0) {
> +		dev_err(led_cdev->dev, "%s: Fail to get torb reg\n", __func__);
> +		return LED_OFF;
> +	}
> +	shift = ffs(mtfled_cdev->torch_bright_mask) - 1;
> +	regval &= mtfled_cdev->torch_bright_mask;
> +	regval >>= shift;
> +	return (regval + 1);
> +}
> +
> +#define MT6360_FLED_DESC(_id)  {					\
> +	.fl_cdev = {							\
> +	 .led_cdev = {							\
> +		.name = "mt6360_fled_ch" #_id,				\
> +		.max_brightness = MT6360_TORBRIGHT_MAX##_id,		\
> +		.brightness_set_blocking =  mt6360_fled_brightness_set,	\
> +		.brightness_get = mt6360_fled_brightness_get,		\
> +		.flags = LED_DEV_CAP_FLASH,				\
> +	 },								\
> +	 .brightness = {						\
> +		.min = MT6360_STROBECUR_MIN,				\
> +		.step = MT6360_STROBECUR_STEP,				\
> +		.max = MT6360_STROBECUR_MAX,				\
> +		.val = MT6360_STROBECUR_MIN,				\
> +	 },								\
> +	 .timeout = {							\
> +		.min = MT6360_STRBTIMEOUT_MIN,				\
> +		.step = MT6360_STRBTIMEOUT_STEP,			\
> +		.max = MT6360_STRBTIMEOUT_MAX,				\
> +		.val = MT6360_STRBTIMEOUT_MIN,				\
> +	 },								\
> +	 .ops = &mt6360_fled_ops,					\
> +	},								\
> +	.index = MT6360_FLED_CH##_id,					\
> +	.cs_enable_reg = MT6360_CSENABLE_REG##_id,			\
> +	.cs_enable_mask = MT6360_CSENABLE_MASK##_id,			\
> +	.torch_bright_reg = MT6360_TORBRIGHT_REG##_id,			\
> +	.torch_bright_mask = MT6360_TORBRIGHT_MASK##_id,		\
> +	.torch_enable_reg = MT6360_TORENABLE_REG##_id,			\
> +	.torch_enable_mask = MT6360_TORENABLE_MASK##_id,		\
> +	.strobe_bright_reg = MT6360_STRBRIGHT_REG##_id,			\
> +	.strobe_bright_mask = MT6360_STRBRIGHT_MASK##_id,		\
> +	.strobe_enable_reg = MT6360_STRBENABLE_REG##_id,		\
> +	.strobe_enable_mask = MT6360_STRBENABLE_MASK##_id,		\
> +}
> +
> +static const struct mt6360_fled_classdev def_fled_classdev[MT6360_FLED_MAX] = {
> +	MT6360_FLED_DESC(1),
> +	MT6360_FLED_DESC(2),
> +};
> +
> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> +static int mt6360_fled_external_strobe_set(
> +				     struct v4l2_flash *v4l2_flash, bool enable)
> +{
> +	struct led_classdev_flash *lcf = v4l2_flash->fled_cdev;
> +	struct led_classdev *led_cdev = &lcf->led_cdev;
> +	struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> +	struct mt6360_fled_classdev *mtfled_cdev = (void *)lcf;
> +	int id = mtfled_cdev->index, ret;
> +
> +	if (!(enable ^ test_bit(id, &mld->fl_strobe_flags)))
> +		return 0;
> +	if (mt6360_fled_check_flags_if_any(&mld->fl_torch_flags)) {
> +		dev_err(led_cdev->dev,
> +			"Disable all leds torch [%lu]\n", mld->fl_torch_flags);
> +		return -EINVAL;
> +	}
> +	ret = regmap_update_bits(mld->regmap, mtfled_cdev->cs_enable_reg,
> +			  mtfled_cdev->cs_enable_mask, enable ? 0xff : 0);
> +	if (enable) {
> +		set_bit(id, &mld->fl_strobe_flags);
> +		mtfled_cdev->faults = 0;
> +	} else
> +		clear_bit(id, &mld->fl_strobe_flags);
> +	return ret;
> +}
> +
> +static const struct v4l2_flash_ops v4l2_flash_ops = {
> +	.external_strobe_set = mt6360_fled_external_strobe_set,
> +};
> +
> +static void mt6360_init_v4l2_flash_config(
> +				       struct mt6360_fled_classdev *mtfled_cdev,
> +				       struct v4l2_flash_config *config)
> +{
> +	struct led_flash_setting *torch_intensity = &config->intensity;
> +	struct led_classdev *led_cdev = &(mtfled_cdev->fl_cdev.led_cdev);
> +	s32 val;
> +
> +	snprintf(config->dev_name, sizeof(config->dev_name),
> +		 "%s", mtfled_cdev->fl_cdev.led_cdev.name);
> +	torch_intensity->min = MT6360_TORCHCUR_MIN;
> +	torch_intensity->step = MT6360_TORCHCUR_STEP;
> +	val = MT6360_TORCHCUR_MIN;
> +	val += ((led_cdev->max_brightness - 1) * MT6360_TORCHCUR_STEP);
> +	torch_intensity->val = torch_intensity->max = val;
> +	config->flash_faults |= MT6360_FLEDSUPPORT_FAULTS;
> +	config->has_external_strobe = 1;
> +}
> +#else
> +static const struct v4l2_flash_ops v4l2_flash_ops;
> +
> +static void mt6360_init_v4l2_flash_config(
> +				       struct mt6360_fled_classdev *mtfled_cdev,
> +				       struct v4l2_flash_config *config)
> +{
> +}
> +#endif /* IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS) */
> +
> +static irqreturn_t mt6360_pmu_fled_lvf_evt_handler(int irq, void *data)
> +{
> +	struct mt6360_led_data *mld = data;
> +
> +	dev_err(mld->dev, "%s\n", __func__);
> +	mld->mtfled_cdev[MT6360_FLED_CH1].faults |= LED_FAULT_UNDER_VOLTAGE;
> +	mld->mtfled_cdev[MT6360_FLED_CH2].faults |= LED_FAULT_UNDER_VOLTAGE;
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t mt6360_pmu_fled2_short_evt_handler(int irq, void *data)
> +{
> +	struct mt6360_led_data *mld = data;
> +
> +	dev_err(mld->dev, "%s\n", __func__);
> +	mld->mtfled_cdev[MT6360_FLED_CH2].faults |= LED_FAULT_SHORT_CIRCUIT;
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t mt6360_pmu_fled1_short_evt_handler(int irq, void *data)
> +{
> +	struct mt6360_led_data *mld = data;
> +
> +	dev_err(mld->dev, "%s\n", __func__);
> +	mld->mtfled_cdev[MT6360_FLED_CH1].faults |= LED_FAULT_SHORT_CIRCUIT;
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t mt6360_pmu_fled2_strb_to_evt_handler(int irq, void *data)
> +{
> +	struct mt6360_led_data *mld = data;
> +
> +	mld->mtfled_cdev[MT6360_FLED_CH2].faults |= LED_FAULT_TIMEOUT;
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t mt6360_pmu_fled1_strb_to_evt_handler(int irq, void *data)
> +{
> +	struct mt6360_led_data *mld = data;
> +
> +	mld->mtfled_cdev[MT6360_FLED_CH1].faults |= LED_FAULT_TIMEOUT;
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t mt6360_pmu_fled_chg_vinovp_evt_handler(int irq, void *data)
> +{
> +	struct mt6360_led_data *mld = data;
> +
> +	dev_warn(mld->dev, "%s\n", __func__);
> +	mld->mtfled_cdev[MT6360_FLED_CH1].faults |= LED_FAULT_INPUT_VOLTAGE;
> +	mld->mtfled_cdev[MT6360_FLED_CH2].faults |= LED_FAULT_INPUT_VOLTAGE;
> +	return IRQ_HANDLED;
> +}
> +
> +static struct mt6360_pmu_irq_desc mt6360_pmu_fled_irq_desc[] = {
> +	{ "fled_chg_vinovp_evt",  mt6360_pmu_fled_chg_vinovp_evt_handler },
> +	{ "fled_lvf_evt", mt6360_pmu_fled_lvf_evt_handler },
> +	{ "fled2_short_evt", mt6360_pmu_fled2_short_evt_handler },
> +	{ "fled1_short_evt", mt6360_pmu_fled1_short_evt_handler },
> +	{ "fled2_strb_to_evt", mt6360_pmu_fled2_strb_to_evt_handler },
> +	{ "fled1_strb_to_evt", mt6360_pmu_fled1_strb_to_evt_handler },
> +};
> +
> +static int mt6360_fled_irq_register(struct platform_device *pdev)
> +{
> +	struct mt6360_pmu_irq_desc *irq_desc;
> +	int i, irq, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(mt6360_pmu_fled_irq_desc); i++) {
> +		irq_desc = mt6360_pmu_fled_irq_desc + i;
> +		if (unlikely(!irq_desc->name))
> +			continue;
> +		irq = platform_get_irq_byname(pdev, irq_desc->name);
> +		if (irq < 0)
> +			continue;
> +		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +						irq_desc->irq_handler,
> +						IRQF_TRIGGER_FALLING,
> +						irq_desc->name,
> +						platform_get_drvdata(pdev));
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"request %s irq fail\n", irq_desc->name);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int mt6360_iled_parse_dt(struct device *dev,
> +				struct mt6360_led_data *mld)
> +{
> +	struct device_node *iled_np, *child;
> +	struct mt6360_led_classdev *mtled_cdev;
> +	u32 val;
> +	int ret;
> +
> +	if (!dev->of_node)
> +		return 0;
> +	iled_np = of_find_node_by_name(dev->of_node, "iled");
> +	if (!iled_np)
> +		return 0;
> +	for_each_available_child_of_node(iled_np, child) {
> +		ret = of_property_read_u32(child, "reg", &val);
> +		if (ret) {
> +			dev_err(dev, "Fail to read reg property\n");
> +			continue;
> +		}
> +		if (val >= MT6360_LED_MAX) {
> +			dev_err(dev, "Invalid iled reg [%u]\n", val);
> +			ret = -EINVAL;
> +			goto out_iled_dt;
> +		}
> +		mtled_cdev = mld->mtled_cdev + val;
> +
> +		of_property_read_string(child,
> +					"label", &(mtled_cdev->cdev.name));

If you used new "*ext" LED registration API the LED core would do LED 
name parsing for you. You'd have to use 'function' and 'color' DT
properties instead of label then.

> +		of_property_read_string(child, "linux,default-trigger",
> +					&(mtled_cdev->cdev.default_trigger));
> +		mtled_cdev->np = child;
> +	}
> +	return 0;
> +out_iled_dt:
> +	of_node_put(child);
> +	return ret;
> +}
> +
> +static int mt6360_fled_parse_dt(struct device *dev,
> +				struct mt6360_led_data *mld)
> +{
> +	struct device_node *fled_np, *child;
> +	struct mt6360_fled_classdev *mtfled_cdev;
> +	struct led_classdev *led_cdev;
> +	struct led_flash_setting *fs;
> +	u32 val;
> +	int ret;
> +
> +	if (!dev->of_node)
> +		return 0;
> +	fled_np = of_find_node_by_name(dev->of_node, "fled");
> +	if (!fled_np)
> +		return 0;
> +	for_each_available_child_of_node(fled_np, child) {
> +		ret = of_property_read_u32(child, "reg", &val);
> +		if (ret) {
> +			dev_err(dev, "Fail to read reg property\n");
> +			continue;
> +		}
> +		if (val >= MT6360_FLED_MAX) {
> +			dev_err(dev, "Invalid fled reg [%u]\n", val);
> +			ret = -EINVAL;
> +			goto out_fled_dt;
> +		}
> +		mtfled_cdev = mld->mtfled_cdev + val;
> +
> +		of_property_read_string(child, "label",
> +					&(mtfled_cdev->fl_cdev.led_cdev.name));

Ditto.

> +		ret = of_property_read_u32(child, "led-max-microamp", &val);
> +		if (ret) {
> +			dev_warn(dev, "led-max-microamp property missing\n");
> +			val = MT6360_TORCHCUR_MIN;
> +		}
> +		if (val < MT6360_TORCHCUR_MIN)
> +			val = MT6360_TORCHCUR_MIN;
> +		val = (val - MT6360_TORCHCUR_MIN) / MT6360_TORCHCUR_STEP + 1;
> +		led_cdev = &(mtfled_cdev->fl_cdev.led_cdev);
> +		led_cdev->max_brightness = min(led_cdev->max_brightness, val);
> +		ret = of_property_read_u32(child, "flash-max-microamp", &val);
> +		if (ret) {
> +			dev_warn(dev, "flash-max-microamp property missing\n");
> +			val = MT6360_STROBECUR_MIN;
> +		}
> +		if (val < MT6360_STROBECUR_MIN)
> +			val = MT6360_STROBECUR_MIN;
> +		fs = &(mtfled_cdev->fl_cdev.brightness);
> +		fs->val = fs->max = min(fs->max, val);
> +		ret = of_property_read_u32(child, "flash-max-timeout", &val);
> +		if (ret) {
> +			dev_warn(dev, "flash-max-timeout property missing\n");
> +			val = MT6360_STRBTIMEOUT_MIN;
> +		}
> +		if (val < MT6360_STRBTIMEOUT_MIN)
> +			val = MT6360_STRBTIMEOUT_MIN;
> +		fs = &(mtfled_cdev->fl_cdev.timeout);
> +		fs->val = fs->max = min(fs->max, val);
> +		mtfled_cdev->np = child;
> +	}
> +	return 0;
> +out_fled_dt:
> +	of_node_put(child);
> +	return ret;
> +}
> +
> +static void mt6360_led_parse_dt_data(struct device *dev,
> +				     struct mt6360_led_platform_data *pdata)
> +{
> +	struct device_node *np = dev->of_node;
> +	int i, ret;
> +	const struct {
> +		const char *name;
> +		u32 *val_ptr;
> +	} u32_opts[] = {
> +		{ "rgbon_sync", &pdata->rgbon_sync },
> +		{ "fled1_ultraistrb", &pdata->fled1_ultraistrb },
> +		{ "fled2_ultraistrb", &pdata->fled2_ultraistrb },
> +	};

Whereas it may be thought of as an optimization, I'd prefer to stick
to the sequential one-by-one property parsing.

Introducing all of a sudden the iteration like below you introduce
unnecessary confusion.

Also, the function name in not telling too much. And again - we don't
seem to have bindings.

> +	memcpy(pdata, &def_platform_data, sizeof(*pdata));
> +	for (i = 0; i < ARRAY_SIZE(u32_opts); i++) {
> +		ret = of_property_read_u32(np, u32_opts[i].name,
> +					   u32_opts[i].val_ptr);
> +		if (ret)
> +			dev_err(dev, "error reading '%s'\n", u32_opts[i].name);
> +	}
> +}
> +
> +static int mt6360_led_apply_pdata(struct mt6360_led_data *mld,
> +				   struct mt6360_led_platform_data *pdata)

This function seems to affect only fleds, so its name should reflect
that.

> +{
> +	int i, ret;
> +	const struct {
> +		u32 *val_ptr;
> +		u8 reg;
> +		u8 mask;
> +		u8 shift;
> +	} sel_props[] = {
> +		{
> +			&pdata->fled1_ultraistrb, MT6360_PMU_FLED1_STRB_CTRL2,
> +			MT6360_MASK_ULTRA_STROBE, MT6360_SHIFT_ULTRA_STROBE,
> +		},
> +		{
> +			&pdata->fled2_ultraistrb, MT6360_PMU_FLED2_STRB_CTRL2,
> +			MT6360_MASK_ULTRA_STROBE, MT6360_SHIFT_ULTRA_STROBE,
> +		},
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(sel_props); i++) {
> +		ret = regmap_update_bits(mld->regmap,
> +					 sel_props[i].reg,
> +					 sel_props[i].mask,
> +					 *sel_props[i].val_ptr <<
> +						sel_props[i].shift);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +static int mt6360_led_probe(struct platform_device *pdev)
> +{
> +	struct mt6360_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct mt6360_led_data *mld;
> +	struct mt6360_led_classdev *mtled_cdev;
> +	struct mt6360_fled_classdev *mtfled_cdev;
> +	struct v4l2_flash_config v4l2_config;
> +	int i, ret;
> +
> +	mld = devm_kzalloc(&pdev->dev, sizeof(*mld), GFP_KERNEL);
> +	if (!mld)
> +		return -ENOMEM;
> +
> +	if (pdev->dev.of_node) {
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +		mt6360_led_parse_dt_data(&pdev->dev, pdata);
> +	}
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "no platform data specified\n");
> +		return -EINVAL;
> +	}
> +
> +	mld->dev = &pdev->dev;
> +	mld->pdata = pdata;
> +	platform_set_drvdata(pdev, mld);
> +
> +	mld->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!mld->regmap) {
> +		dev_err(&pdev->dev, "Failed to get parent regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = mt6360_led_apply_pdata(mld, pdata);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "apply pdata fail\n");
> +		return ret;
> +	}
> +
> +	memcpy(mld->mtled_cdev, def_led_classdev, sizeof(def_led_classdev));
> +	ret = mt6360_iled_parse_dt(&pdev->dev, mld);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to parse iled dt\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < MT6360_LED_MAX; i++) {
> +		mtled_cdev = mld->mtled_cdev + i;

You don't seem to check how many LEDs were defined in DT.

> +		ret = devm_led_classdev_register(&pdev->dev,
> +						 &(mtled_cdev->cdev));

Please use devm*ext() API.

> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "Failed to register led[%d]\n", i);
> +			return ret;
> +		}
> +		mtled_cdev->cdev.dev->of_node = mtled_cdev->np;
> +	}
> +
> +	memcpy(mld->mtfled_cdev, def_fled_classdev, sizeof(def_fled_classdev));
> +	ret = mt6360_fled_parse_dt(&pdev->dev, mld);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Fail to parse fled dt\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < MT6360_FLED_MAX; ++) {
> +		mtfled_cdev = mld->mtfled_cdev + i;

You don't seem to check how many LEDs were defined in DT.

> +		ret = led_classdev_flash_register(&pdev->dev,
> +						  &mtfled_cdev->fl_cdev);

Please use devm*ext() API.

> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "Failed to register fled[%d]\n", i);
> +			goto out_fled_cdev;
> +		}
> +	}
> +
> +	for (i = 0; i < MT6360_FLED_MAX; i++) {
> +		mtfled_cdev = mld->mtfled_cdev + i;
> +		memset(&v4l2_config, 0, sizeof(v4l2_config));
> +		mt6360_init_v4l2_flash_config(mtfled_cdev, &v4l2_config);
> +		mtfled_cdev->v4l2_flash = v4l2_flash_init(&pdev->dev,
> +					      of_fwnode_handle(mtfled_cdev->np),
> +					      &mtfled_cdev->fl_cdev,
> +					      &v4l2_flash_ops, &v4l2_config);
> +		if (IS_ERR(mtfled_cdev->v4l2_flash)) {
> +			dev_err(&pdev->dev, "Failed to register v4l2_sd\n");
> +			ret = PTR_ERR(mtfled_cdev->v4l2_flash);
> +			goto out_v4l2_sd;
> +		}
> +	}
> +
> +	ret = mt6360_fled_irq_register(pdev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to register irqs\n");
> +		goto out_v4l2_sd;
> +	}
> +
> +	return 0;
> +out_v4l2_sd:
> +	while (--i >= 0) {
> +		mtfled_cdev = mld->mtfled_cdev + i;
> +		v4l2_flash_release(mtfled_cdev->v4l2_flash);
> +	}
> +	i = MT6360_FLED_MAX;
> +out_fled_cdev:
> +	while (--i >= 0) {
> +		mtfled_cdev = mld->mtfled_cdev + i;
> +		led_classdev_flash_unregister(&mtfled_cdev->fl_cdev);
> +	}
> +	return ret;
> +}
> +
> +static int mt6360_led_remove(struct platform_device *pdev)
> +{
> +	struct mt6360_led_data *mld = platform_get_drvdata(pdev);
> +	struct mt6360_fled_classdev *mtfled_cdev;
> +	int i;
> +
> +	for (i = 0; i < MT6360_FLED_MAX; i++) {
> +		mtfled_cdev = mld->mtfled_cdev + i;
> +		v4l2_flash_release(mtfled_cdev->v4l2_flash);
> +		led_classdev_flash_unregister(&mtfled_cdev->fl_cdev);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
> +	{ .compatible = "mediatek,mt6360_led", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mt6360_led_of_id);
> +
> +static struct platform_driver mt6360_led_driver = {
> +	.driver = {
> +		.name = "mt6360_led",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(mt6360_led_of_id),
> +	},
> +	.probe = mt6360_led_probe,
> +	.remove = mt6360_led_remove,
> +};
> +module_platform_driver(mt6360_led_driver);
> +
> +MODULE_AUTHOR("Gene Chen <gene_chen@richtek.com>");
> +MODULE_DESCRIPTION("MT6360 Led Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/mt6360.h b/include/linux/mfd/mt6360.h
> index ea13040..2b81ab5 100644
> --- a/include/linux/mfd/mt6360.h
> +++ b/include/linux/mfd/mt6360.h
> @@ -29,6 +29,11 @@ struct mt6360_pmu_data {
>   	unsigned int chip_rev;
>   };
>   
> +struct mt6360_pmu_irq_desc {
> +	const char *name;
> +	irq_handler_t irq_handler;
> +};
> +
>   /* PMU register defininition */
>   #define MT6360_PMU_DEV_INFO			(0x00)
>   #define MT6360_PMU_CORE_CTRL1			(0x01)
> @@ -236,5 +241,4 @@ struct mt6360_pmu_data {
>   #define CHIP_VEN_MASK				(0xF0)
>   #define CHIP_VEN_MT6360				(0x50)
>   #define CHIP_REV_MASK				(0x0F)
> -
>   #endif /* __MT6360_H__ */
> 

-- 
Best regards,
Jacek Anaszewski