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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>

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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
+ */
+
+#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@xxxxxxxxxxx>");
+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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux