[PATCH v3 0/2] leds: mt6360: Add LED driver for MT6360

[PATCH v3 0/2] leds: mt6360: Add LED driver for MT6360

[Gene Chen]

In-Reply-To: 

This patch series add MT6360 LED support contains driver and binding document

Gene Chen (2)
 leds: mt6360: Add LED driver for MT6360
 dt-bindings: leds: Add bindings for MT6360 LED

 Documentation/devicetree/bindings/leds/leds-mt6360.yaml |  105 ++
 drivers/leds/Kconfig                                    |   11 
 drivers/leds/Makefile                                   |    1 
 drivers/leds/leds-mt6360.c                              |  681 ++++++++++++++++
 4 files changed, 798 insertions(+)

changelogs between v1 & v2
 - add led driver with mfd

changelogs between v2 & v3
 - independent add led driver
 - add dt-binding document
 - refactor macros definition for easy to debug
 - parse device tree by fwnode
 - use devm*ext to register led class device


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Gene Chen]

From: Gene Chen <gene_chen@richtek.com>

Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
and 4-channel RGB LED support Register/Flash/Breath Mode

Signed-off-by: Gene Chen <gene_chen@richtek.com>
---
 drivers/leds/Kconfig       |  11 +
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-mt6360.c | 681 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 693 insertions(+)
 create mode 100644 drivers/leds/leds-mt6360.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c181df..94a6d83 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -271,6 +271,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
+	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.
+	  Includes 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 c2c7d7a..5596427 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -66,6 +66,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..bd6fa48
--- /dev/null
+++ b/drivers/leds/leds-mt6360.c
@@ -0,0 +1,681 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2020 MediaTek Inc.
+//
+// Author: Gene Chen <gene_chen@richtek.com>
+
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/led-class-flash.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <media/v4l2-flash-led-class.h>
+
+enum {
+	MT6360_LED_ISNK1 = 0,
+	MT6360_LED_ISNK2,
+	MT6360_LED_ISNK3,
+	MT6360_LED_ISNK4,
+	MT6360_LED_FLASH1,
+	MT6360_LED_FLASH2,
+	MT6360_MAX_LEDS,
+};
+
+#define MT6360_REG_RGBEN		0x380
+#define MT6360_REG_ISNK(_led_no)	(0x381 + (_led_no))
+#define MT6360_ISNK_ENMASK(_led_no)	BIT(7 - (_led_no))
+#define MT6360_ISNK_MASK		0x1F
+#define MT6360_CHRINDSEL_MASK		BIT(3)
+
+#define MT6360_REG_FLEDEN		0x37E
+#define MT6360_REG_STRBTO		0x373
+#define MT6360_REG_FLEDBASE(_id)	(0x372 + 4 * (_id - MT6360_LED_FLASH1))
+#define MT6360_REG_FLEDISTRB(_id)	(MT6360_REG_FLEDBASE(_id) + 2)
+#define MT6360_REG_FLEDITOR(_id)	(MT6360_REG_FLEDBASE(_id) + 3)
+#define MT6360_REG_CHGSTAT2		0x3E1
+#define MT6360_REG_FLEDSTAT1		0x3E9
+#define MT6360_ITORCH_MASK		GENMASK(4, 0)
+#define MT6360_ISTROBE_MASK		GENMASK(6, 0)
+#define MT6360_STRBTO_MASK		GENMASK(6, 0)
+#define MT6360_TORCHEN_MASK		BIT(3)
+#define MT6360_STROBEN_MASK		BIT(2)
+#define MT6360_FLCSEN_MASK(_id)		BIT(MT6360_LED_FLASH2 - _id)
+#define MT6360_FLEDCHGVINOVP_MASK	BIT(3)
+#define MT6360_FLED1STRBTO_MASK		BIT(11)
+#define MT6360_FLED2STRBTO_MASK		BIT(10)
+#define MT6360_FLED1STRB_MASK		BIT(9)
+#define MT6360_FLED2STRB_MASK		BIT(8)
+#define MT6360_FLED1SHORT_MASK		BIT(7)
+#define MT6360_FLED2SHORT_MASK		BIT(6)
+#define MT6360_FLEDLVF_MASK		BIT(3)
+
+/* 0 means led_off, add one for dummy */
+#define MT6360_ISNK1_MAXLEVEL		13
+#define MT6360_ISNK4_MAXLEVEL		31
+
+#define MT6360_ITORCH_MIN		25000
+#define MT6360_ITORCH_STEP		12500
+#define MT6360_ITORCH_MAX		400000
+#define MT6360_ISTRB_MIN		50000
+#define MT6360_ISTRB_STEP		12500
+#define MT6360_ISTRB_MAX		1500000
+#define MT6360_STRBTO_MIN		64000
+#define MT6360_STRBTO_STEP		32000
+#define MT6360_STRBTO_MAX		2432000
+
+#define FLED_TORCH_FLAG_MASK		0x0c
+#define FLED_TORCH_FLAG_SHFT		2
+#define FLED_STROBE_FLAG_MASK		0x03
+
+#define STATE_OFF			0
+#define STATE_KEEP			1
+#define STATE_ON			2
+
+struct mt6360_led {
+	union {
+		struct led_classdev isnk;
+		struct led_classdev_flash flash;
+	};
+	struct v4l2_flash *v4l2_flash;
+	struct mt6360_priv *priv;
+	u32 led_no;
+	u32 default_state;
+};
+
+struct mt6360_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct mt6360_led *leds[MT6360_MAX_LEDS];
+	unsigned int fled_strobe_used;
+	unsigned int fled_torch_used;
+};
+
+static int mt6360_isnk_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
+{
+	struct mt6360_led *led = container_of(lcdev, struct mt6360_led, isnk);
+	struct mt6360_priv *priv = led->priv;
+	u32 enable_mask = MT6360_ISNK_ENMASK(led->led_no);
+	u32 val = level ? MT6360_ISNK_ENMASK(led->led_no) : 0;
+	int ret;
+
+	dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
+
+	if (level) {
+		ret = regmap_update_bits(priv->regmap, MT6360_REG_ISNK(led->led_no),
+					 MT6360_ISNK_MASK, level - 1);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, enable_mask, val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mt6360_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
+{
+	struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
+	struct mt6360_priv *priv = led->priv;
+	u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
+	u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
+	u32 prev = priv->fled_torch_used, curr;
+	int ret;
+
+	dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
+	if (priv->fled_strobe_used) {
+		dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
+		return -EINVAL;
+	}
+
+	if (level)
+		curr = prev | BIT(led->led_no);
+	else
+		curr = prev & (~BIT(led->led_no));
+
+	if (curr)
+		val |= MT6360_TORCHEN_MASK;
+
+	if (level) {
+		ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDITOR(led->led_no),
+					 MT6360_ITORCH_MASK, level - 1);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask, val);
+	if (ret)
+		return ret;
+
+	priv->fled_torch_used = curr;
+
+	return 0;
+}
+
+static int mt6360_strobe_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)
+{
+	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
+	struct led_classdev *lcdev = &fl_cdev->led_cdev;
+
+	dev_dbg(lcdev->dev, "[%d] strobe brightness %d\n", led->led_no, brightness);
+	return 0;
+}
+
+static int _mt6360_strobe_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)
+{
+	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
+	struct mt6360_priv *priv = led->priv;
+	struct led_flash_setting *s = &fl_cdev->brightness;
+	u32 val = (brightness - s->min) / s->step;
+
+	return regmap_update_bits(priv->regmap, MT6360_REG_FLEDISTRB(led->led_no),
+				 MT6360_ISTROBE_MASK, val);
+}
+
+static int mt6360_strobe_set(struct led_classdev_flash *fl_cdev, bool state)
+{
+	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
+	struct mt6360_priv *priv = led->priv;
+	struct led_classdev *lcdev = &fl_cdev->led_cdev;
+	struct led_flash_setting *s = &fl_cdev->brightness;
+	u32 enable_mask = MT6360_STROBEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
+	u32 val = state ? MT6360_FLCSEN_MASK(led->led_no) : 0;
+	u32 prev = priv->fled_strobe_used, curr;
+	int ret;
+
+	dev_dbg(lcdev->dev, "[%d] strobe state %d\n", led->led_no, state);
+	if (priv->fled_torch_used) {
+		dev_warn(lcdev->dev, "Please disable torch first [0x%x]\n", priv->fled_torch_used);
+		return -EINVAL;
+	}
+
+	if (state)
+		curr = prev | BIT(led->led_no);
+	else
+		curr = prev & (~BIT(led->led_no));
+
+	if (curr)
+		val |= MT6360_STROBEN_MASK;
+
+	ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask, val);
+	if (ret) {
+		dev_err(lcdev->dev, "[%d] control current source %d fail\n", led->led_no, state);
+		return ret;
+	}
+
+	/* used to prevent flash current spike when torch on */
+	ret = _mt6360_strobe_brightness_set(fl_cdev, state ? s->val : s->min);
+	if (ret)
+		return ret;
+
+	if (!prev && curr)
+		usleep_range(5000, 6000);
+	else if (prev && !curr)
+		udelay(500);
+
+	priv->fled_strobe_used = curr;
+	return 0;
+}
+
+static int mt6360_strobe_get(struct led_classdev_flash *fl_cdev, bool *state)
+{
+	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
+	struct mt6360_priv *priv = led->priv;
+
+	*state = !!(priv->fled_strobe_used & BIT(led->led_no));
+	return 0;
+}
+
+static int mt6360_timeout_set(struct led_classdev_flash *fl_cdev, u32 timeout)
+{
+	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
+	struct mt6360_priv *priv = led->priv;
+	struct led_flash_setting *s = &fl_cdev->timeout;
+	u32 val = (timeout - s->min) / s->step;
+
+	return regmap_update_bits(priv->regmap, MT6360_REG_STRBTO, MT6360_STRBTO_MASK, val);
+
+}
+
+static int mt6360_fault_get(struct led_classdev_flash *fl_cdev, u32 *fault)
+{
+	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
+	struct mt6360_priv *priv = led->priv;
+	u16 fled_stat;
+	unsigned int chg_stat, strobe_timeout_mask, fled_short_mask;
+	u32 rfault = 0;
+	int ret;
+
+	ret = regmap_read(priv->regmap, MT6360_REG_CHGSTAT2, &chg_stat);
+	if (ret)
+		return ret;
+
+	ret = regmap_raw_read(priv->regmap, MT6360_REG_FLEDSTAT1, &fled_stat, sizeof(fled_stat));
+	if (ret)
+		return ret;
+
+	if (led->led_no == MT6360_LED_FLASH1) {
+		strobe_timeout_mask = MT6360_FLED1STRBTO_MASK;
+		fled_short_mask = MT6360_FLED1SHORT_MASK;
+
+	} else {
+		strobe_timeout_mask = MT6360_FLED2STRBTO_MASK;
+		fled_short_mask = MT6360_FLED2SHORT_MASK;
+	}
+
+	if (chg_stat & MT6360_FLEDCHGVINOVP_MASK)
+		rfault |= LED_FAULT_INPUT_VOLTAGE;
+
+	if (fled_stat & strobe_timeout_mask)
+		rfault |= LED_FAULT_TIMEOUT;
+
+	if (fled_stat & fled_short_mask)
+		rfault |= LED_FAULT_SHORT_CIRCUIT;
+
+	if (fled_stat & MT6360_FLEDLVF_MASK)
+		rfault |= LED_FAULT_UNDER_VOLTAGE;
+
+	*fault = rfault;
+	return 0;
+}
+
+static const struct led_flash_ops mt6360_flash_ops = {
+	.flash_brightness_set = mt6360_strobe_brightness_set,
+	.strobe_set = mt6360_strobe_set,
+	.strobe_get = mt6360_strobe_get,
+	.timeout_set = mt6360_timeout_set,
+	.fault_get = mt6360_fault_get,
+};
+
+static int mt6360_isnk_init_default_state(struct mt6360_led *led)
+{
+	struct mt6360_priv *priv = led->priv;
+	unsigned int regval;
+	u32 level;
+	int ret;
+
+	ret = regmap_read(priv->regmap, MT6360_REG_ISNK(led->led_no), &regval);
+	if (ret)
+		return ret;
+	level = regval & MT6360_ISNK_MASK;
+
+	ret = regmap_read(priv->regmap, MT6360_REG_RGBEN, &regval);
+	if (ret)
+		return ret;
+
+	if (regval & MT6360_ISNK_ENMASK(led->led_no))
+		level += 1;
+	else
+		level = LED_OFF;
+
+	switch (led->default_state) {
+	case STATE_ON:
+		led->isnk.brightness = led->isnk.max_brightness;
+		break;
+	case STATE_KEEP:
+		led->isnk.brightness = min(level, led->isnk.max_brightness);
+		break;
+	default:
+		led->isnk.brightness = LED_OFF;
+	}
+
+	return mt6360_isnk_brightness_set(&led->isnk, led->isnk.brightness);
+}
+
+static int mt6360_isnk_register(struct device *parent, struct mt6360_led *led,
+				struct led_init_data *init_data)
+{
+	struct mt6360_priv *priv = led->priv;
+	int ret;
+
+	if (led->led_no == MT6360_LED_ISNK1) {
+		/* change isink to sw mode */
+		ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, MT6360_CHRINDSEL_MASK,
+					 MT6360_CHRINDSEL_MASK);
+		if (ret) {
+			dev_err(parent, "Failed to config ISNK1 to SW mode\n");
+			return ret;
+		}
+	}
+
+	ret = mt6360_isnk_init_default_state(led);
+	if (ret) {
+		dev_err(parent, "Failed to init %d isnk state\n", led->led_no);
+		return ret;
+	}
+
+	ret = devm_led_classdev_register_ext(parent, &led->isnk, init_data);
+	if (ret) {
+		dev_err(parent, "Couldn't register isink %d\n", led->led_no);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mt6360_flash_init_default_state(struct mt6360_led *led)
+{
+	struct led_classdev_flash *flash = &led->flash;
+	struct mt6360_priv *priv = led->priv;
+	u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
+	u32 level;
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(priv->regmap, MT6360_REG_FLEDITOR(led->led_no), &regval);
+	if (ret)
+		return ret;
+	level = regval & MT6360_ITORCH_MASK;
+
+	ret = regmap_read(priv->regmap, MT6360_REG_FLEDEN, &regval);
+	if (ret)
+		return ret;
+
+	if ((regval & enable_mask) == enable_mask)
+		level += 1;
+	else
+		level = LED_OFF;
+
+	switch (led->default_state) {
+	case STATE_ON:
+		flash->led_cdev.brightness = flash->led_cdev.max_brightness;
+		break;
+	case STATE_KEEP:
+		flash->led_cdev.brightness = min(level, flash->led_cdev.max_brightness);
+		break;
+	default:
+		flash->led_cdev.brightness = LED_OFF;
+	}
+
+	return mt6360_torch_brightness_set(&flash->led_cdev, flash->led_cdev.brightness);
+}
+
+#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
+static int mt6360_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
+{
+	struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
+	struct mt6360_led *led = container_of(flash, struct mt6360_led, flash);
+	struct mt6360_priv *priv = led->priv;
+	u32 enable_mask = MT6360_FLCSEN_MASK(led->led_no);
+	int ret;
+
+	ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask,
+				 enable ? enable_mask : 0);
+	if (ret)
+		return ret;
+
+	if (enable)
+		priv->fled_strobe_used |= BIT(led->led_no);
+	else
+		priv->fled_strobe_used &= (~BIT(led->led_no));
+
+	return 0;
+}
+
+static const struct v4l2_flash_ops v4l2_flash_ops = {
+	.external_strobe_set = mt6360_flash_external_strobe_set,
+};
+
+static void mt6360_flash_init_v4l2_config(struct mt6360_led *led, struct v4l2_flash_config *config)
+{
+	struct led_classdev_flash *flash = &led->flash;
+	struct led_classdev *lcdev = &flash->led_cdev;
+	struct led_flash_setting *s = &config->intensity;
+
+	snprintf(config->dev_name, sizeof(config->dev_name), "%s", lcdev->name);
+
+	s->min = MT6360_ITORCH_MIN;
+	s->step = MT6360_ITORCH_STEP;
+	s->val = s->max = (s->min) + (lcdev->max_brightness - 1) * s->step;
+
+	config->has_external_strobe = 1;
+}
+#else
+static const struct v4l2_flash_ops v4l2_flash_ops;
+
+static void mt6360_flash_init_v4l2_config(struct mt6360_led *led, struct v4l2_flash_config *config)
+{
+}
+#endif
+
+static int mt6360_flash_register(struct device *parent, struct mt6360_led *led,
+				 struct led_init_data *init_data)
+{
+	struct v4l2_flash_config v4l2_config = {0};
+	int ret;
+
+	ret = mt6360_flash_init_default_state(led);
+	if (ret) {
+		dev_err(parent, "Failed to init %d flash state\n", led->led_no);
+		return ret;
+	}
+
+	ret = devm_led_classdev_flash_register_ext(parent, &led->flash, init_data);
+	if (ret) {
+		dev_err(parent, "Couldn't register flash %d\n", led->led_no);
+		return ret;
+	}
+
+	mt6360_flash_init_v4l2_config(led, &v4l2_config);
+	led->v4l2_flash = v4l2_flash_init(parent, init_data->fwnode, &led->flash, &v4l2_flash_ops,
+					  &v4l2_config);
+	if (IS_ERR(led->v4l2_flash)) {
+		dev_err(parent, "Failed to register %d v4l2 sd\n", led->led_no);
+		return PTR_ERR(led->v4l2_flash);
+	}
+
+	return 0;
+}
+
+static int mt6360_init_isnk_properties(struct mt6360_led *led, struct led_init_data *init_data)
+{
+	struct led_classdev *isnk = &led->isnk;
+
+	if (led->led_no == MT6360_LED_ISNK4)
+		isnk->max_brightness = MT6360_ISNK4_MAXLEVEL;
+	else
+		isnk->max_brightness = MT6360_ISNK1_MAXLEVEL;
+
+	isnk->brightness_set_blocking = mt6360_isnk_brightness_set;
+
+	fwnode_property_read_string(init_data->fwnode, "linux,default-trigger",
+				    &isnk->default_trigger);
+
+	return 0;
+}
+
+static void clamp_align(u32 *v, u32 min, u32 max, u32 step)
+{
+	*v = clamp_val(*v, min, max);
+	if (step > 1)
+		*v = (*v - min) / step * step + min;
+}
+
+static int mt6360_init_flash_properties(struct mt6360_led *led, struct led_init_data *init_data)
+{
+	struct led_classdev_flash *flash = &led->flash;
+	struct led_classdev *lcdev = &flash->led_cdev;
+	struct led_flash_setting *s;
+	u32 val;
+	int ret;
+
+	ret = fwnode_property_read_u32(init_data->fwnode, "led-max-microamp", &val);
+	if (ret)
+		val = MT6360_ITORCH_MIN;
+	else
+		clamp_align(&val, MT6360_ITORCH_MIN, MT6360_ITORCH_MAX, MT6360_ITORCH_STEP);
+
+	lcdev->max_brightness = (val - MT6360_ITORCH_MIN) / MT6360_ITORCH_STEP + 1;
+	lcdev->brightness_set_blocking = mt6360_torch_brightness_set;
+	lcdev->flags |= LED_DEV_CAP_FLASH;
+
+	ret = fwnode_property_read_u32(init_data->fwnode, "flash-max-microamp", &val);
+	if (ret)
+		val = MT6360_ISTRB_MIN;
+	else
+		clamp_align(&val, MT6360_ISTRB_MIN, MT6360_ISTRB_MAX, MT6360_ISTRB_STEP);
+
+	s = &flash->brightness;
+	s->min = MT6360_ISTRB_MIN;
+	s->step = MT6360_ISTRB_STEP;
+	s->val = s->max = val;
+
+	/* always configure as min level when off to prevent flash current spike */
+	ret = _mt6360_strobe_brightness_set(flash, s->min);
+	if (ret)
+		return ret;
+
+	ret = fwnode_property_read_u32(init_data->fwnode, "flash-max-timeout-us", &val);
+	if (ret)
+		val = MT6360_STRBTO_MIN;
+	else
+		clamp_align(&val, MT6360_STRBTO_MIN, MT6360_STRBTO_MAX, MT6360_STRBTO_STEP);
+
+	s = &flash->timeout;
+	s->min = MT6360_STRBTO_MIN;
+	s->step = MT6360_STRBTO_STEP;
+	s->val = s->max = val;
+
+	flash->ops = &mt6360_flash_ops;
+
+	return 0;
+}
+
+static int mt6360_init_common_properties(struct mt6360_led *led, struct led_init_data *init_data)
+{
+	const char *str;
+
+	if (!fwnode_property_read_string(init_data->fwnode, "default-state", &str)) {
+		if (!strcmp(str, "on"))
+			led->default_state = STATE_ON;
+		else if (!strcmp(str, "keep"))
+			led->default_state = STATE_KEEP;
+		else
+			led->default_state = STATE_OFF;
+	}
+
+	return 0;
+}
+
+static int mt6360_led_probe(struct platform_device *pdev)
+{
+	struct mt6360_priv *priv;
+	struct fwnode_handle *child;
+	int i, ret;
+
+	ret = device_get_child_node_count(&pdev->dev);
+	if (!ret || ret > MT6360_MAX_LEDS)
+		return -EINVAL;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+
+	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;
+		struct led_init_data init_data = { .fwnode = child, };
+		u32 reg;
+
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret || reg >= MT6360_MAX_LEDS || priv->leds[reg]) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
+		if (!led) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		led->led_no = reg;
+		led->priv = priv;
+
+		ret = mt6360_init_common_properties(led, &init_data);
+		if (ret)
+			goto out;
+
+		switch (reg) {
+		case MT6360_LED_ISNK1 ... MT6360_LED_ISNK4:
+			ret = mt6360_init_isnk_properties(led, &init_data);
+			if (ret)
+				goto out;
+
+			ret = mt6360_isnk_register(&pdev->dev, led, &init_data);
+			break;
+		default:
+			ret = mt6360_init_flash_properties(led, &init_data);
+			if (ret)
+				goto out;
+
+			ret = mt6360_flash_register(&pdev->dev, led, &init_data);
+		}
+
+		if (ret)
+			goto out;
+
+		priv->leds[reg] = led;
+	}
+
+	platform_set_drvdata(pdev, priv);
+	return 0;
+out:
+	for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
+		struct mt6360_led *led = priv->leds[i];
+
+		if (led && led->v4l2_flash)
+			v4l2_flash_release(led->v4l2_flash);
+
+	}
+
+	return ret;
+}
+
+static int mt6360_led_remove(struct platform_device *pdev)
+{
+	struct mt6360_priv *priv = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
+		struct mt6360_led *led = priv->leds[i];
+
+		if (led && led->v4l2_flash)
+			v4l2_flash_release(led->v4l2_flash);
+
+	}
+
+	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",
+		.of_match_table = 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");
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/2] dt-bindings: leds: Add bindings for MT6360 LED

[Gene Chen]

From: Gene Chen <gene_chen@richtek.com>

Add bindings document for LED support on MT6360 PMIC

Signed-off-by: Gene Chen <gene_chen@richtek.com>
---
 .../devicetree/bindings/leds/leds-mt6360.yaml      | 105 +++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6360.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-mt6360.yaml b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
new file mode 100644
index 0000000..72914c6
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
@@ -0,0 +1,105 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-mt6360.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LED driver for MT6360 PMIC from MediaTek Integrated.
+
+maintainers:
+  - Gene Chen <gene_chen@richtek.com>
+
+description: |
+  This module is part of the MT6360 MFD device.
+  The LED controller is represented as a sub-node of the PMIC node on
+  the device tree.
+  This device has six current sinks.
+
+properties:
+  compatible:
+    const: mediatek,mt6360-led
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[0-5]$":
+    type: object
+    description: |
+      Properties for a single LED.
+
+    properties:
+      reg:
+        description: Index of the LED.
+        enum:
+          - 0 # LED output INDICATOR1
+          - 1 # LED output INDICATOR2
+          - 2 # LED output INDICATOR3
+          - 3 # LED output INDICATOR4
+          - 4 # LED output FLED1
+          - 5 # LED output FLED2
+
+required:
+  - compatible
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+   #include <dt-bindings/leds/common.h>
+   led-controller {
+     compatible = "mediatek,mt6360-led";
+     #address-cells = <1>;
+     #size-cells = <0>;
+
+     led@0 {
+       reg = <0>;
+       function = LED_FUNCTION_INDICATOR;
+       color = <LED_COLOR_ID_RED>;
+       default-state = "off";
+     };
+     led@1 {
+       reg = <1>;
+       function = LED_FUNCTION_INDICATOR;
+       color = <LED_COLOR_ID_GREEN>;
+       default-state = "off";
+     };
+     led@2 {
+       reg = <2>;
+       function = LED_FUNCTION_INDICATOR;
+       color = <LED_COLOR_ID_BLUE>;
+       default-state = "off";
+     };
+     led@3 {
+       reg = <3>;
+       function = LED_FUNCTION_INDICATOR;
+       color = <LED_COLOR_ID_AMBER>;
+       default-state = "off";
+     };
+     led@4 {
+       reg = <4>;
+       function = LED_FUNCTION_FLASH;
+       color = <LED_COLOR_ID_WHITE>;
+       function-enumerator = <1>;
+       default-state = "off";
+       led-max-microamp = <200000>;
+       flash-max-microamp = <500000>;
+       flash-max-timeout-us = <1024000>;
+     };
+     led@5 {
+       reg = <5>;
+       function = LED_FUNCTION_FLASH;
+       color = <LED_COLOR_ID_WHITE>;
+       function-enumerator = <2>;
+       default-state = "off";
+       led-max-microamp = <200000>;
+       flash-max-microamp = <500000>;
+       flash-max-timeout-us = <1024000>;
+     };
+   };
+...
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] dt-bindings: leds: Add bindings for MT6360 LED

[Dan Murphy]

Gene

On 9/7/20 5:27 AM, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
>
> Add bindings document for LED support on MT6360 PMIC
>
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>   .../devicetree/bindings/leds/leds-mt6360.yaml      | 105 +++++++++++++++++++++
>   1 file changed, 105 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6360.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-mt6360.yaml b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
> new file mode 100644
> index 0000000..72914c6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-mt6360.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LED driver for MT6360 PMIC from MediaTek Integrated.
> +
> +maintainers:
> +  - Gene Chen <gene_chen@richtek.com>
> +
> +description: |
> +  This module is part of the MT6360 MFD device.
> +  The LED controller is represented as a sub-node of the PMIC node on
> +  the device tree.
> +  This device has six current sinks.
> +

A bit of a nitpick but wouldn't the commit message in patch 1/2 be a 
better description of the hardware then this?

Other wise

Reviewed-by: Dan Murphy <dmurphy@ti.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Dan Murphy]

Gene

On 9/7/20 5:27 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 4-channel RGB LED support Register/Flash/Breath Mode
>
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>   drivers/leds/Kconfig       |  11 +
>   drivers/leds/Makefile      |   1 +
>   drivers/leds/leds-mt6360.c | 681 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 693 insertions(+)
>   create mode 100644 drivers/leds/leds-mt6360.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 1c181df..94a6d83 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -271,6 +271,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
> +	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.
> +	  Includes 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 c2c7d7a..5596427 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -66,6 +66,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..bd6fa48
> --- /dev/null
> +++ b/drivers/leds/leds-mt6360.c
> @@ -0,0 +1,681 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (C) 2020 MediaTek Inc.
> +//
> +// Author: Gene Chen <gene_chen@richtek.com>
> +
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <media/v4l2-flash-led-class.h>
> +
> +enum {
> +	MT6360_LED_ISNK1 = 0,
> +	MT6360_LED_ISNK2,
> +	MT6360_LED_ISNK3,
> +	MT6360_LED_ISNK4,
> +	MT6360_LED_FLASH1,
> +	MT6360_LED_FLASH2,
> +	MT6360_MAX_LEDS,
> +};
> +
> +#define MT6360_REG_RGBEN		0x380
> +#define MT6360_REG_ISNK(_led_no)	(0x381 + (_led_no))
> +#define MT6360_ISNK_ENMASK(_led_no)	BIT(7 - (_led_no))
> +#define MT6360_ISNK_MASK		0x1F
> +#define MT6360_CHRINDSEL_MASK		BIT(3)
> +
> +#define MT6360_REG_FLEDEN		0x37E
> +#define MT6360_REG_STRBTO		0x373
> +#define MT6360_REG_FLEDBASE(_id)	(0x372 + 4 * (_id - MT6360_LED_FLASH1))
> +#define MT6360_REG_FLEDISTRB(_id)	(MT6360_REG_FLEDBASE(_id) + 2)
> +#define MT6360_REG_FLEDITOR(_id)	(MT6360_REG_FLEDBASE(_id) + 3)
> +#define MT6360_REG_CHGSTAT2		0x3E1
> +#define MT6360_REG_FLEDSTAT1		0x3E9
> +#define MT6360_ITORCH_MASK		GENMASK(4, 0)
> +#define MT6360_ISTROBE_MASK		GENMASK(6, 0)
> +#define MT6360_STRBTO_MASK		GENMASK(6, 0)
> +#define MT6360_TORCHEN_MASK		BIT(3)
> +#define MT6360_STROBEN_MASK		BIT(2)
> +#define MT6360_FLCSEN_MASK(_id)		BIT(MT6360_LED_FLASH2 - _id)
> +#define MT6360_FLEDCHGVINOVP_MASK	BIT(3)
> +#define MT6360_FLED1STRBTO_MASK		BIT(11)
> +#define MT6360_FLED2STRBTO_MASK		BIT(10)
> +#define MT6360_FLED1STRB_MASK		BIT(9)
> +#define MT6360_FLED2STRB_MASK		BIT(8)
> +#define MT6360_FLED1SHORT_MASK		BIT(7)
> +#define MT6360_FLED2SHORT_MASK		BIT(6)
> +#define MT6360_FLEDLVF_MASK		BIT(3)
> +
> +/* 0 means led_off, add one for dummy */
> +#define MT6360_ISNK1_MAXLEVEL		13
> +#define MT6360_ISNK4_MAXLEVEL		31
> +
> +#define MT6360_ITORCH_MIN		25000
> +#define MT6360_ITORCH_STEP		12500
> +#define MT6360_ITORCH_MAX		400000
> +#define MT6360_ISTRB_MIN		50000
> +#define MT6360_ISTRB_STEP		12500
> +#define MT6360_ISTRB_MAX		1500000
> +#define MT6360_STRBTO_MIN		64000
> +#define MT6360_STRBTO_STEP		32000
> +#define MT6360_STRBTO_MAX		2432000
> +
> +#define FLED_TORCH_FLAG_MASK		0x0c
> +#define FLED_TORCH_FLAG_SHFT		2
> +#define FLED_STROBE_FLAG_MASK		0x03
> +
> +#define STATE_OFF			0
> +#define STATE_KEEP			1
> +#define STATE_ON			2
> +
> +struct mt6360_led {
> +	union {
> +		struct led_classdev isnk;
> +		struct led_classdev_flash flash;
> +	};
> +	struct v4l2_flash *v4l2_flash;
> +	struct mt6360_priv *priv;
> +	u32 led_no;
> +	u32 default_state;
> +};
> +
> +struct mt6360_priv {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct mt6360_led *leds[MT6360_MAX_LEDS];

I would prefer to use a flexible array here as you are not guaranteed 
that the DT will contain 6 LED entries and it will simplify your probe 
and DT parsing.

There are examples of using the flexible array

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-cr0014114.c

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-lm3697.c

> +	unsigned int fled_strobe_used;
> +	unsigned int fled_torch_used;
> +};
> +
> +static int mt6360_isnk_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> +{
> +	struct mt6360_led *led = container_of(lcdev, struct mt6360_led, isnk);
> +	struct mt6360_priv *priv = led->priv;
> +	u32 enable_mask = MT6360_ISNK_ENMASK(led->led_no);
> +	u32 val = level ? MT6360_ISNK_ENMASK(led->led_no) : 0;
> +	int ret;
> +
> +	dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> +
> +	if (level) {
> +		ret = regmap_update_bits(priv->regmap, MT6360_REG_ISNK(led->led_no),
> +					 MT6360_ISNK_MASK, level - 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, enable_mask, val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int mt6360_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> +{
> +	struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
> +	struct mt6360_priv *priv = led->priv;
> +	u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> +	u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> +	u32 prev = priv->fled_torch_used, curr;
> +	int ret;
> +
> +	dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> +	if (priv->fled_strobe_used) {
> +		dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
> +		return -EINVAL;
> +	}
> +
> +	if (level)
> +		curr = prev | BIT(led->led_no);
> +	else
> +		curr = prev & (~BIT(led->led_no));
> +
> +	if (curr)
> +		val |= MT6360_TORCHEN_MASK;
> +
> +	if (level) {
> +		ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDITOR(led->led_no),
> +					 MT6360_ITORCH_MASK, level - 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask, val);
> +	if (ret)
> +		return ret;
> +
> +	priv->fled_torch_used = curr;
> +
> +	return 0;
> +}
> +
> +static int mt6360_strobe_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct led_classdev *lcdev = &fl_cdev->led_cdev;
> +
> +	dev_dbg(lcdev->dev, "[%d] strobe brightness %d\n", led->led_no, brightness);
> +	return 0;
> +}
> +
> +static int _mt6360_strobe_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	struct led_flash_setting *s = &fl_cdev->brightness;
> +	u32 val = (brightness - s->min) / s->step;
> +
> +	return regmap_update_bits(priv->regmap, MT6360_REG_FLEDISTRB(led->led_no),
> +				 MT6360_ISTROBE_MASK, val);
> +}
> +
> +static int mt6360_strobe_set(struct led_classdev_flash *fl_cdev, bool state)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	struct led_classdev *lcdev = &fl_cdev->led_cdev;
> +	struct led_flash_setting *s = &fl_cdev->brightness;
> +	u32 enable_mask = MT6360_STROBEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> +	u32 val = state ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> +	u32 prev = priv->fled_strobe_used, curr;
> +	int ret;
> +
> +	dev_dbg(lcdev->dev, "[%d] strobe state %d\n", led->led_no, state);
> +	if (priv->fled_torch_used) {
> +		dev_warn(lcdev->dev, "Please disable torch first [0x%x]\n", priv->fled_torch_used);
> +		return -EINVAL;
> +	}
> +
> +	if (state)
> +		curr = prev | BIT(led->led_no);
> +	else
> +		curr = prev & (~BIT(led->led_no));
> +
> +	if (curr)
> +		val |= MT6360_STROBEN_MASK;
> +
> +	ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask, val);
> +	if (ret) {
> +		dev_err(lcdev->dev, "[%d] control current source %d fail\n", led->led_no, state);
> +		return ret;
> +	}
> +
> +	/* used to prevent flash current spike when torch on */
> +	ret = _mt6360_strobe_brightness_set(fl_cdev, state ? s->val : s->min);
> +	if (ret)
> +		return ret;
> +
> +	if (!prev && curr)
> +		usleep_range(5000, 6000);
> +	else if (prev && !curr)
> +		udelay(500);
> +
> +	priv->fled_strobe_used = curr;
> +	return 0;
> +}
> +
> +static int mt6360_strobe_get(struct led_classdev_flash *fl_cdev, bool *state)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +
> +	*state = !!(priv->fled_strobe_used & BIT(led->led_no));
> +	return 0;
> +}
> +
> +static int mt6360_timeout_set(struct led_classdev_flash *fl_cdev, u32 timeout)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	struct led_flash_setting *s = &fl_cdev->timeout;
> +	u32 val = (timeout - s->min) / s->step;
> +
> +	return regmap_update_bits(priv->regmap, MT6360_REG_STRBTO, MT6360_STRBTO_MASK, val);
> +
> +}
> +
> +static int mt6360_fault_get(struct led_classdev_flash *fl_cdev, u32 *fault)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	u16 fled_stat;
> +	unsigned int chg_stat, strobe_timeout_mask, fled_short_mask;
> +	u32 rfault = 0;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_CHGSTAT2, &chg_stat);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_raw_read(priv->regmap, MT6360_REG_FLEDSTAT1, &fled_stat, sizeof(fled_stat));
> +	if (ret)
> +		return ret;
> +
> +	if (led->led_no == MT6360_LED_FLASH1) {
> +		strobe_timeout_mask = MT6360_FLED1STRBTO_MASK;
> +		fled_short_mask = MT6360_FLED1SHORT_MASK;
> +
> +	} else {
> +		strobe_timeout_mask = MT6360_FLED2STRBTO_MASK;
> +		fled_short_mask = MT6360_FLED2SHORT_MASK;
> +	}
> +
> +	if (chg_stat & MT6360_FLEDCHGVINOVP_MASK)
> +		rfault |= LED_FAULT_INPUT_VOLTAGE;
> +
> +	if (fled_stat & strobe_timeout_mask)
> +		rfault |= LED_FAULT_TIMEOUT;
> +
> +	if (fled_stat & fled_short_mask)
> +		rfault |= LED_FAULT_SHORT_CIRCUIT;
> +
> +	if (fled_stat & MT6360_FLEDLVF_MASK)
> +		rfault |= LED_FAULT_UNDER_VOLTAGE;
> +
> +	*fault = rfault;
> +	return 0;
> +}
> +
> +static const struct led_flash_ops mt6360_flash_ops = {
> +	.flash_brightness_set = mt6360_strobe_brightness_set,
> +	.strobe_set = mt6360_strobe_set,
> +	.strobe_get = mt6360_strobe_get,
> +	.timeout_set = mt6360_timeout_set,
> +	.fault_get = mt6360_fault_get,
> +};
> +
> +static int mt6360_isnk_init_default_state(struct mt6360_led *led)
> +{
> +	struct mt6360_priv *priv = led->priv;
> +	unsigned int regval;
> +	u32 level;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_ISNK(led->led_no), &regval);
> +	if (ret)
> +		return ret;
> +	level = regval & MT6360_ISNK_MASK;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_RGBEN, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (regval & MT6360_ISNK_ENMASK(led->led_no))
> +		level += 1;
> +	else
> +		level = LED_OFF;
> +
> +	switch (led->default_state) {
> +	case STATE_ON:
> +		led->isnk.brightness = led->isnk.max_brightness;
> +		break;
> +	case STATE_KEEP:
> +		led->isnk.brightness = min(level, led->isnk.max_brightness);
> +		break;
> +	default:
> +		led->isnk.brightness = LED_OFF;
> +	}
> +
> +	return mt6360_isnk_brightness_set(&led->isnk, led->isnk.brightness);
> +}
> +
> +static int mt6360_isnk_register(struct device *parent, struct mt6360_led *led,
> +				struct led_init_data *init_data)
> +{
> +	struct mt6360_priv *priv = led->priv;
> +	int ret;
> +
> +	if (led->led_no == MT6360_LED_ISNK1) {
> +		/* change isink to sw mode */
> +		ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, MT6360_CHRINDSEL_MASK,
> +					 MT6360_CHRINDSEL_MASK);
> +		if (ret) {
> +			dev_err(parent, "Failed to config ISNK1 to SW mode\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = mt6360_isnk_init_default_state(led);
> +	if (ret) {
> +		dev_err(parent, "Failed to init %d isnk state\n", led->led_no);
> +		return ret;
> +	}
> +
> +	ret = devm_led_classdev_register_ext(parent, &led->isnk, init_data);
> +	if (ret) {
> +		dev_err(parent, "Couldn't register isink %d\n", led->led_no);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6360_flash_init_default_state(struct mt6360_led *led)
> +{
> +	struct led_classdev_flash *flash = &led->flash;
> +	struct mt6360_priv *priv = led->priv;
> +	u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> +	u32 level;
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_FLEDITOR(led->led_no), &regval);
> +	if (ret)
> +		return ret;
> +	level = regval & MT6360_ITORCH_MASK;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_FLEDEN, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if ((regval & enable_mask) == enable_mask)
> +		level += 1;
> +	else
> +		level = LED_OFF;
> +
> +	switch (led->default_state) {
> +	case STATE_ON:
> +		flash->led_cdev.brightness = flash->led_cdev.max_brightness;
> +		break;
> +	case STATE_KEEP:
> +		flash->led_cdev.brightness = min(level, flash->led_cdev.max_brightness);
> +		break;
> +	default:
> +		flash->led_cdev.brightness = LED_OFF;
> +	}
> +
> +	return mt6360_torch_brightness_set(&flash->led_cdev, flash->led_cdev.brightness);
> +}
> +
> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> +static int mt6360_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> +{
> +	struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
> +	struct mt6360_led *led = container_of(flash, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	u32 enable_mask = MT6360_FLCSEN_MASK(led->led_no);
> +	int ret;
> +
> +	ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask,
> +				 enable ? enable_mask : 0);
> +	if (ret)
> +		return ret;
> +
> +	if (enable)
> +		priv->fled_strobe_used |= BIT(led->led_no);
> +	else
> +		priv->fled_strobe_used &= (~BIT(led->led_no));
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_flash_ops v4l2_flash_ops = {
> +	.external_strobe_set = mt6360_flash_external_strobe_set,
> +};
> +
> +static void mt6360_flash_init_v4l2_config(struct mt6360_led *led, struct v4l2_flash_config *config)
> +{
> +	struct led_classdev_flash *flash = &led->flash;
> +	struct led_classdev *lcdev = &flash->led_cdev;
> +	struct led_flash_setting *s = &config->intensity;
> +
> +	snprintf(config->dev_name, sizeof(config->dev_name), "%s", lcdev->name);
> +
> +	s->min = MT6360_ITORCH_MIN;
> +	s->step = MT6360_ITORCH_STEP;
> +	s->val = s->max = (s->min) + (lcdev->max_brightness - 1) * s->step;
> +
> +	config->has_external_strobe = 1;
> +}
> +#else
> +static const struct v4l2_flash_ops v4l2_flash_ops;
> +
> +static void mt6360_flash_init_v4l2_config(struct mt6360_led *led, struct v4l2_flash_config *config)
> +{
> +}
> +#endif
> +
> +static int mt6360_flash_register(struct device *parent, struct mt6360_led *led,
> +				 struct led_init_data *init_data)
> +{
> +	struct v4l2_flash_config v4l2_config = {0};
> +	int ret;
> +
> +	ret = mt6360_flash_init_default_state(led);
> +	if (ret) {
> +		dev_err(parent, "Failed to init %d flash state\n", led->led_no);
> +		return ret;
> +	}
> +
> +	ret = devm_led_classdev_flash_register_ext(parent, &led->flash, init_data);
> +	if (ret) {
> +		dev_err(parent, "Couldn't register flash %d\n", led->led_no);
> +		return ret;
> +	}
> +
> +	mt6360_flash_init_v4l2_config(led, &v4l2_config);
> +	led->v4l2_flash = v4l2_flash_init(parent, init_data->fwnode, &led->flash, &v4l2_flash_ops,
> +					  &v4l2_config);
> +	if (IS_ERR(led->v4l2_flash)) {
> +		dev_err(parent, "Failed to register %d v4l2 sd\n", led->led_no);
> +		return PTR_ERR(led->v4l2_flash);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6360_init_isnk_properties(struct mt6360_led *led, struct led_init_data *init_data)
> +{
> +	struct led_classdev *isnk = &led->isnk;
> +
> +	if (led->led_no == MT6360_LED_ISNK4)
> +		isnk->max_brightness = MT6360_ISNK4_MAXLEVEL;
> +	else
> +		isnk->max_brightness = MT6360_ISNK1_MAXLEVEL;
> +
> +	isnk->brightness_set_blocking = mt6360_isnk_brightness_set;
> +
> +	fwnode_property_read_string(init_data->fwnode, "linux,default-trigger",
> +				    &isnk->default_trigger);
> +
> +	return 0;
> +}
> +
> +static void clamp_align(u32 *v, u32 min, u32 max, u32 step)
> +{
> +	*v = clamp_val(*v, min, max);
> +	if (step > 1)
> +		*v = (*v - min) / step * step + min;
> +}
> +
> +static int mt6360_init_flash_properties(struct mt6360_led *led, struct led_init_data *init_data)
> +{
> +	struct led_classdev_flash *flash = &led->flash;
> +	struct led_classdev *lcdev = &flash->led_cdev;
> +	struct led_flash_setting *s;
> +	u32 val;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(init_data->fwnode, "led-max-microamp", &val);
> +	if (ret)
> +		val = MT6360_ITORCH_MIN;
> +	else
> +		clamp_align(&val, MT6360_ITORCH_MIN, MT6360_ITORCH_MAX, MT6360_ITORCH_STEP);
> +
> +	lcdev->max_brightness = (val - MT6360_ITORCH_MIN) / MT6360_ITORCH_STEP + 1;
> +	lcdev->brightness_set_blocking = mt6360_torch_brightness_set;
> +	lcdev->flags |= LED_DEV_CAP_FLASH;
> +
> +	ret = fwnode_property_read_u32(init_data->fwnode, "flash-max-microamp", &val);
> +	if (ret)
> +		val = MT6360_ISTRB_MIN;
> +	else
> +		clamp_align(&val, MT6360_ISTRB_MIN, MT6360_ISTRB_MAX, MT6360_ISTRB_STEP);
> +
> +	s = &flash->brightness;
> +	s->min = MT6360_ISTRB_MIN;
> +	s->step = MT6360_ISTRB_STEP;
> +	s->val = s->max = val;
> +
> +	/* always configure as min level when off to prevent flash current spike */
> +	ret = _mt6360_strobe_brightness_set(flash, s->min);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_property_read_u32(init_data->fwnode, "flash-max-timeout-us", &val);
> +	if (ret)
> +		val = MT6360_STRBTO_MIN;
> +	else
> +		clamp_align(&val, MT6360_STRBTO_MIN, MT6360_STRBTO_MAX, MT6360_STRBTO_STEP);
> +
> +	s = &flash->timeout;
> +	s->min = MT6360_STRBTO_MIN;
> +	s->step = MT6360_STRBTO_STEP;
> +	s->val = s->max = val;
> +
> +	flash->ops = &mt6360_flash_ops;
> +
> +	return 0;
> +}
> +
> +static int mt6360_init_common_properties(struct mt6360_led *led, struct led_init_data *init_data)
> +{
> +	const char *str;
> +
> +	if (!fwnode_property_read_string(init_data->fwnode, "default-state", &str)) {
> +		if (!strcmp(str, "on"))
> +			led->default_state = STATE_ON;
> +		else if (!strcmp(str, "keep"))
> +			led->default_state = STATE_KEEP;
> +		else
> +			led->default_state = STATE_OFF;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6360_led_probe(struct platform_device *pdev)
> +{
> +	struct mt6360_priv *priv;
> +	struct fwnode_handle *child;
> +	int i, ret;
> +
> +	ret = device_get_child_node_count(&pdev->dev);
> +	if (!ret || ret > MT6360_MAX_LEDS)
> +		return -EINVAL;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;
> +
> +	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;
> +		struct led_init_data init_data = { .fwnode = child, };
> +		u32 reg;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret || reg >= MT6360_MAX_LEDS || priv->leds[reg]) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);

Using the flexible array in the mt6360_priv will eliminate this allocation.

Dan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/2] leds: mt6360: Add LED driver for MT6360

[Dan Murphy]

Gene

On 9/7/20 5:27 AM, Gene Chen wrote:
> In-Reply-To:
>
> This patch series add MT6360 LED support contains driver and binding document

I cannot find the v2 patch series for this.

Dan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] dt-bindings: leds: Add bindings for MT6360 LED

[Pavel Machek]

Hi!
> From: Gene Chen <gene_chen@richtek.com>
> 
> Add bindings document for LED support on MT6360 PMIC
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>

Put this first in the series.

Acked-by: Pavel Machek <pavel@ucw.cz>
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Pavel Machek]

Hi!

> From: Gene Chen <gene_chen@richtek.com>
> 
> Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> and 4-channel RGB LED support Register/Flash/Breath Mode
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>  drivers/leds/Kconfig       |  11 +
>  drivers/leds/Makefile      |   1 +
>  drivers/leds/leds-mt6360.c | 681 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 693 insertions(+)
>  create mode 100644 drivers/leds/leds-mt6360.c
> 
> +	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.
> +	  Includes Low-VF and short protection.
> +

80 columns. And perhaps user does not need to know about protections... and actually
about independend sources, either.

"Enable this for RGB LED and flash LED support on..."?

> +static int mt6360_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> +{
> +	struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
> +	struct mt6360_priv *priv = led->priv;
> +	u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> +	u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> +	u32 prev = priv->fled_torch_used, curr;
> +	int ret;
> +
> +	dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> +	if (priv->fled_strobe_used) {
> +		dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
> +		return -EINVAL;
> +	}

So... how does its userland interface look like?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/2] leds: mt6360: Add LED driver for MT6360

[Gene Chen]

Dan Murphy <dmurphy@ti.com> 於 2020年9月8日 週二 下午10:14寫道：
>
> Gene
>
> On 9/7/20 5:27 AM, Gene Chen wrote:
> > In-Reply-To:
> >
> > This patch series add MT6360 LED support contains driver and binding document
>
> I cannot find the v2 patch series for this.
>
> Dan
>

There is a little confuse of patch v2 which title didn't show the version.
I list the patch sequence as below

[v1] leds: mt6360: Add LED driver for MT6360
https://patchwork.kernel.org/patch/11586959/
[v2] [1/2] leds: mt6360: Add LED driver for MT6360
https://patchwork.kernel.org/patch/11737995/
[v3] [v3,1/2] leds: mt6360: Add LED driver for MT6360
https://patchwork.kernel.org/patch/11760491/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Andy Shevchenko]

On Mon, Sep 7, 2020 at 1:31 PM Gene Chen <gene.chen.richtek@gmail.com> wrote:
>
> From: Gene Chen <gene_chen@richtek.com>
>
> Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> and 4-channel RGB LED support Register/Flash/Breath Mode

I'm wondering why you don't use struct led_classdev_flash.

...

> +//
> +// Copyright (C) 2020 MediaTek Inc.
> +//

Do you really need these two // lines?

...

> +enum {
> +       MT6360_LED_ISNK1 = 0,
> +       MT6360_LED_ISNK2,
> +       MT6360_LED_ISNK3,
> +       MT6360_LED_ISNK4,
> +       MT6360_LED_FLASH1,
> +       MT6360_LED_FLASH2,

> +       MT6360_MAX_LEDS,

No comma for terminator entry.

> +};

...

> +#define MT6360_ISNK_MASK               0x1F

GENMASK()

...

> +#define MT6360_ITORCH_MIN              25000
> +#define MT6360_ITORCH_STEP             12500
> +#define MT6360_ITORCH_MAX              400000
> +#define MT6360_ISTRB_MIN               50000
> +#define MT6360_ISTRB_STEP              12500
> +#define MT6360_ISTRB_MAX               1500000
> +#define MT6360_STRBTO_MIN              64000
> +#define MT6360_STRBTO_STEP             32000
> +#define MT6360_STRBTO_MAX              2432000

Add unit suffixes, please.

...

> +#define FLED_TORCH_FLAG_MASK           0x0c

> +#define FLED_STROBE_FLAG_MASK          0x03

GENMASK()

...

> +       dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);

Not production noise.

...

> +       ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, enable_mask, val);
> +       if (ret)
> +               return ret;
> +
> +       return 0;

return regmap...

> +       u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;

Why parens?

...

> +       dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);

Noise.

...

> +       if (priv->fled_strobe_used) {
> +               dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
> +               return -EINVAL;

Hmm... Shouldn't be guaranteed by some framework?

...

> +               curr = prev & (~BIT(led->led_no));

Too many parens.

...

> +static int mt6360_strobe_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)
> +{
> +       struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +       struct led_classdev *lcdev = &fl_cdev->led_cdev;
> +

> +       dev_dbg(lcdev->dev, "[%d] strobe brightness %d\n", led->led_no, brightness);

Noise. Point of this entire function?

> +       return 0;
> +}

...

> +       dev_dbg(lcdev->dev, "[%d] strobe state %d\n", led->led_no, state);

Noise.

If you wish to do it right, add trace events to the framework.

...

> +       if (priv->fled_torch_used) {

> +               dev_warn(lcdev->dev, "Please disable torch first [0x%x]\n", priv->fled_torch_used);

Again, why the warning? Can this be a part of the framework?

> +               return -EINVAL;
> +       }

...

> +               curr = prev & (~BIT(led->led_no));

Too many parens.

...

> +       if (!prev && curr)
> +               usleep_range(5000, 6000);
> +       else if (prev && !curr)
> +               udelay(500);

These delays must be explained.

...

> +       if (led->led_no == MT6360_LED_FLASH1) {
> +               strobe_timeout_mask = MT6360_FLED1STRBTO_MASK;
> +               fled_short_mask = MT6360_FLED1SHORT_MASK;

> +

Redundant blank line.

> +       } else {
> +               strobe_timeout_mask = MT6360_FLED2STRBTO_MASK;
> +               fled_short_mask = MT6360_FLED2SHORT_MASK;
> +       }

...

> +static int mt6360_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> +{
> +       struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
> +       struct mt6360_led *led = container_of(flash, struct mt6360_led, flash);
> +       struct mt6360_priv *priv = led->priv;

> +       u32 enable_mask = MT6360_FLCSEN_MASK(led->led_no);

enable_mask -> mask
  u32 value = enable ? mask : 0;

> +       int ret;
> +
> +       ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask,

> +                                enable ? enable_mask : 0);

  ret =  ... mask, value);

> +       if (ret)
> +               return ret;
> +
> +       if (enable)
> +               priv->fled_strobe_used |= BIT(led->led_no);
> +       else
> +               priv->fled_strobe_used &= (~BIT(led->led_no));

Too many parens.

> +
> +       return 0;
> +}

...

> +       s->val = s->max = (s->min) + (lcdev->max_brightness - 1) * s->step;

Ditto.

...

> +static void clamp_align(u32 *v, u32 min, u32 max, u32 step)

Can we keep a similar API, i.e. return a new value rather than update old?

> +{

> +       *v = clamp_val(*v, min, max);

I would rather use a temporary variable (and it actually will be
required with above).

> +       if (step > 1)
> +               *v = (*v - min) / step * step + min;

Sounds like open coded rounddown().

> +}

...

> +       lcdev->max_brightness = (val - MT6360_ITORCH_MIN) / MT6360_ITORCH_STEP + 1;

DIV_ROUND_UP(val - MT6360_ITORCH_MIN, MT6360_ITORCH_STEP) ?

...

> +static int mt6360_init_common_properties(struct mt6360_led *led, struct led_init_data *init_data)
> +{
> +       const char *str;
> +
> +       if (!fwnode_property_read_string(init_data->fwnode, "default-state", &str)) {
> +               if (!strcmp(str, "on"))
> +                       led->default_state = STATE_ON;
> +               else if (!strcmp(str, "keep"))
> +                       led->default_state = STATE_KEEP;

> +               else

I wouldn't allow some garbage to be off.

> +                       led->default_state = STATE_OFF;
> +       }

What about

static const char * const states = { "on", "keep", "off" };

int ret;

ret = match_string(states, ARRAY_SIZE(states), str);
if (ret)
 ...

default_state = ret;

?

> +       return 0;
> +}

...

> +static int mt6360_led_probe(struct platform_device *pdev)
> +{
> +       struct mt6360_priv *priv;
> +       struct fwnode_handle *child;
> +       int i, ret;
> +

> +       priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +       if (!priv->regmap) {
> +               dev_err(&pdev->dev, "Failed to get parent regmap\n");
> +               return -ENODEV;
> +       }

...

> +out:

out_flash_leds_release: ?

> +       for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
> +               struct mt6360_led *led = priv->leds[i];
> +
> +               if (led && led->v4l2_flash)
> +                       v4l2_flash_release(led->v4l2_flash);
> +
> +       }

...

> +static int mt6360_led_remove(struct platform_device *pdev)
> +{
> +       struct mt6360_priv *priv = platform_get_drvdata(pdev);
> +       int i;
> +
> +       for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
> +               struct mt6360_led *led = priv->leds[i];
> +
> +               if (led && led->v4l2_flash)
> +                       v4l2_flash_release(led->v4l2_flash);
> +
> +       }

Looks like a code duplication.

> +
> +       return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
> +       { .compatible = "mediatek,mt6360-led", },

> +       {},

No need comma.

> +};


-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Gene Chen]

Pavel Machek <pavel@ucw.cz> 於 2020年9月9日 週三 上午6:25寫道：
>
> Hi!
>
> > From: Gene Chen <gene_chen@richtek.com>
> >
> > Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> > and 4-channel RGB LED support Register/Flash/Breath Mode
> >
> > Signed-off-by: Gene Chen <gene_chen@richtek.com>
> > ---
> >  drivers/leds/Kconfig       |  11 +
> >  drivers/leds/Makefile      |   1 +
> >  drivers/leds/leds-mt6360.c | 681 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 693 insertions(+)
> >  create mode 100644 drivers/leds/leds-mt6360.c
> >
> > +     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.
> > +       Includes Low-VF and short protection.
> > +
>
> 80 columns. And perhaps user does not need to know about protections... and actually
> about independend sources, either.
>

ACK

> "Enable this for RGB LED and flash LED support on..."?
>
> > +static int mt6360_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> > +{
> > +     struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
> > +     struct mt6360_priv *priv = led->priv;
> > +     u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> > +     u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> > +     u32 prev = priv->fled_torch_used, curr;
> > +     int ret;
> > +
> > +     dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> > +     if (priv->fled_strobe_used) {
> > +             dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
> > +             return -EINVAL;
> > +     }
>
> So... how does its userland interface look like?
>

1. set FLED1 brightness
# echo 1 > /sys/class/leds/white:flash1/flash_brightness
2. enable FLED1 strobe
# echo 1 > /sys/class/leds/white:flash1/flash_strobe
3 . turn off FLED1 strobe (just used to gaurantee the strobe mode
flash leds must be turned off)
# echo 0 > /sys/class/leds/white:flash1/flash_strobe

> Best regards,
>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Gene Chen]

Andy Shevchenko <andy.shevchenko@gmail.com> 於 2020年9月9日 週三 下午9:48寫道：
>
> On Mon, Sep 7, 2020 at 1:31 PM Gene Chen <gene.chen.richtek@gmail.com> wrote:
> >
> > From: Gene Chen <gene_chen@richtek.com>
> >
> > Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> > and 4-channel RGB LED support Register/Flash/Breath Mode
>
> I'm wondering why you don't use struct led_classdev_flash.
>
> ...
>

Both Flash and RGB LED use led_classdev_flash by
"devm_led_classdev_flash_register_ext".

> > +//
> > +// Copyright (C) 2020 MediaTek Inc.
> > +//
>
> Do you really need these two // lines?
>

ACK, I will remove it

> ...
>
> > +enum {
> > +       MT6360_LED_ISNK1 = 0,
> > +       MT6360_LED_ISNK2,
> > +       MT6360_LED_ISNK3,
> > +       MT6360_LED_ISNK4,
> > +       MT6360_LED_FLASH1,
> > +       MT6360_LED_FLASH2,
>
> > +       MT6360_MAX_LEDS,
>
> No comma for terminator entry.
>

ACK

> > +};
>
> ...
>
> > +#define MT6360_ISNK_MASK               0x1F
>
> GENMASK()
>
> ...
>
> > +#define MT6360_ITORCH_MIN              25000
> > +#define MT6360_ITORCH_STEP             12500
> > +#define MT6360_ITORCH_MAX              400000
> > +#define MT6360_ISTRB_MIN               50000
> > +#define MT6360_ISTRB_STEP              12500
> > +#define MT6360_ISTRB_MAX               1500000
> > +#define MT6360_STRBTO_MIN              64000
> > +#define MT6360_STRBTO_STEP             32000
> > +#define MT6360_STRBTO_MAX              2432000
>
> Add unit suffixes, please.
>

ACK

> ...
>
> > +#define FLED_TORCH_FLAG_MASK           0x0c
>
> > +#define FLED_STROBE_FLAG_MASK          0x03
>
> GENMASK()
>

ACK

> ...
>
> > +       dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
>
> Not production noise.
>

ACK

> ...
>
> > +       ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, enable_mask, val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
>
> return regmap...
>
> > +       u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
>
> Why parens?
>

ACK

> ...
>
> > +       dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
>
> Noise.
>

ACK

> ...
>
> > +       if (priv->fled_strobe_used) {
> > +               dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
> > +               return -EINVAL;
>
> Hmm... Shouldn't be guaranteed by some framework?
>

Because both Flash LED use single logically control.
It doesn't exist one LED is torch mode, and the other is strobe mode.

> ...
>
> > +               curr = prev & (~BIT(led->led_no));
>
> Too many parens.
>

ACK

> ...
>
> > +static int mt6360_strobe_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)
> > +{
> > +       struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> > +       struct led_classdev *lcdev = &fl_cdev->led_cdev;
> > +
>
> > +       dev_dbg(lcdev->dev, "[%d] strobe brightness %d\n", led->led_no, brightness);
>
> Noise. Point of this entire function?
>

ACK, I will remove it, reserve function entry only for register
ledcass_dev check ops exist

> > +       return 0;
> > +}
>
> ...
>
> > +       dev_dbg(lcdev->dev, "[%d] strobe state %d\n", led->led_no, state);
>
> Noise.
>
> If you wish to do it right, add trace events to the framework.
>

ACK, I will remove it.

> ...
>
> > +       if (priv->fled_torch_used) {
>
> > +               dev_warn(lcdev->dev, "Please disable torch first [0x%x]\n", priv->fled_torch_used);
>
> Again, why the warning? Can this be a part of the framework?
>

Same as above.

> > +               return -EINVAL;
> > +       }
>
> ...
>
> > +               curr = prev & (~BIT(led->led_no));
>
> Too many parens.
>

ACK

> ...
>
> > +       if (!prev && curr)
> > +               usleep_range(5000, 6000);
> > +       else if (prev && !curr)
> > +               udelay(500);
>
> These delays must be explained.
>

ACK

> ...
>
> > +       if (led->led_no == MT6360_LED_FLASH1) {
> > +               strobe_timeout_mask = MT6360_FLED1STRBTO_MASK;
> > +               fled_short_mask = MT6360_FLED1SHORT_MASK;
>
> > +
>
> Redundant blank line.
>

ACK

> > +       } else {
> > +               strobe_timeout_mask = MT6360_FLED2STRBTO_MASK;
> > +               fled_short_mask = MT6360_FLED2SHORT_MASK;
> > +       }
>
> ...
>
> > +static int mt6360_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> > +{
> > +       struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
> > +       struct mt6360_led *led = container_of(flash, struct mt6360_led, flash);
> > +       struct mt6360_priv *priv = led->priv;
>
> > +       u32 enable_mask = MT6360_FLCSEN_MASK(led->led_no);
>
> enable_mask -> mask
>   u32 value = enable ? mask : 0;
>
> > +       int ret;
> > +
> > +       ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask,
>
> > +                                enable ? enable_mask : 0);
>
>   ret =  ... mask, value);
>

ACK

> > +       if (ret)
> > +               return ret;
> > +
> > +       if (enable)
> > +               priv->fled_strobe_used |= BIT(led->led_no);
> > +       else
> > +               priv->fled_strobe_used &= (~BIT(led->led_no));
>
> Too many parens.
>

ACK

> > +
> > +       return 0;
> > +}
>
> ...
>
> > +       s->val = s->max = (s->min) + (lcdev->max_brightness - 1) * s->step;
>
> Ditto.
>

ACK

> ...
>
> > +static void clamp_align(u32 *v, u32 min, u32 max, u32 step)
>
> Can we keep a similar API, i.e. return a new value rather than update old?
>
> > +{
>
> > +       *v = clamp_val(*v, min, max);
>
> I would rather use a temporary variable (and it actually will be
> required with above).
>
> > +       if (step > 1)
> > +               *v = (*v - min) / step * step + min;
>
> Sounds like open coded rounddown().
>

ACK

> > +}
>
> ...
>
> > +       lcdev->max_brightness = (val - MT6360_ITORCH_MIN) / MT6360_ITORCH_STEP + 1;
>
> DIV_ROUND_UP(val - MT6360_ITORCH_MIN, MT6360_ITORCH_STEP) ?
>

This is mapping 0~val to 1~max_brightness as level.
I convert val below MT6360_ITORCH_STEP to 1 for ignore max_brightness
= 0, because 0 means disable.
There is a little difference from DIV_ROUND_UP.

> ...
>
> > +static int mt6360_init_common_properties(struct mt6360_led *led, struct led_init_data *init_data)
> > +{
> > +       const char *str;
> > +
> > +       if (!fwnode_property_read_string(init_data->fwnode, "default-state", &str)) {
> > +               if (!strcmp(str, "on"))
> > +                       led->default_state = STATE_ON;
> > +               else if (!strcmp(str, "keep"))
> > +                       led->default_state = STATE_KEEP;
>
> > +               else
>
> I wouldn't allow some garbage to be off.
>

ACK

> > +                       led->default_state = STATE_OFF;
> > +       }
>
> What about
>
> static const char * const states = { "on", "keep", "off" };
>
> int ret;
>
> ret = match_string(states, ARRAY_SIZE(states), str);
> if (ret)
>  ...
>
> default_state = ret;
>
> ?
>
> > +       return 0;
> > +}
>

ACK

> ...
>
> > +static int mt6360_led_probe(struct platform_device *pdev)
> > +{
> > +       struct mt6360_priv *priv;
> > +       struct fwnode_handle *child;
> > +       int i, ret;
> > +
>
> > +       priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > +       if (!priv->regmap) {
> > +               dev_err(&pdev->dev, "Failed to get parent regmap\n");
> > +               return -ENODEV;
> > +       }
>
> ...
>
> > +out:
>
> out_flash_leds_release: ?
>

ACK

> > +       for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
> > +               struct mt6360_led *led = priv->leds[i];
> > +
> > +               if (led && led->v4l2_flash)
> > +                       v4l2_flash_release(led->v4l2_flash);
> > +
> > +       }
>
> ...
>
> > +static int mt6360_led_remove(struct platform_device *pdev)
> > +{
> > +       struct mt6360_priv *priv = platform_get_drvdata(pdev);
> > +       int i;
> > +
> > +       for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
> > +               struct mt6360_led *led = priv->leds[i];
> > +
> > +               if (led && led->v4l2_flash)
> > +                       v4l2_flash_release(led->v4l2_flash);
> > +
> > +       }
>
> Looks like a code duplication.
>

ACK

> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
> > +       { .compatible = "mediatek,mt6360-led", },
>
> > +       {},
>
> No need comma.
>

ACK

> > +};
>
>
> --
> With Best Regards,
> Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1132 bytes --]

Hi!

> > > +enum {
> > > +       MT6360_LED_ISNK1 = 0,
> > > +       MT6360_LED_ISNK2,
> > > +       MT6360_LED_ISNK3,
> > > +       MT6360_LED_ISNK4,
> > > +       MT6360_LED_FLASH1,
> > > +       MT6360_LED_FLASH2,
> >
> > > +       MT6360_MAX_LEDS,
> >
> > No comma for terminator entry.
> >
> 
> ACK

Actually, that comma is fine. Its absence would be fine, too.

> > > +};
> >
> > ...
> >
> > > +#define MT6360_ISNK_MASK               0x1F
> >
> > GENMASK()

Again, that is fine.

> > > +#define FLED_TORCH_FLAG_MASK           0x0c
> >
> > > +#define FLED_STROBE_FLAG_MASK          0x03
> >
> > GENMASK()
> >
> 
> ACK

Again, that is fine.

> > > +       return 0;
> > > +}
> > > +
> > > +static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
> > > +       { .compatible = "mediatek,mt6360-led", },
> >
> > > +       {},
> >
> > No need comma.
> 
> ACK

It is also no hurting comma.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Andy Shevchenko]

On Thu, Sep 10, 2020 at 11:18 AM Pavel Machek <pavel@ucw.cz> wrote:

...

> > > > +enum {
> > > > +       MT6360_LED_ISNK1 = 0,
> > > > +       MT6360_LED_ISNK2,
> > > > +       MT6360_LED_ISNK3,
> > > > +       MT6360_LED_ISNK4,
> > > > +       MT6360_LED_FLASH1,
> > > > +       MT6360_LED_FLASH2,
> > >
> > > > +       MT6360_MAX_LEDS,
> > >
> > > No comma for terminator entry.
> > >
> >
> > ACK
>
> Actually, that comma is fine. Its absence would be fine, too.

It is slightly better not to have to prevent (theoretical) rebase or
other similar issues when a new item can go behind the terminator. In
such a case compiler can easily tell you if something is wrong.

> > > > +};

...

> > > > +static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
> > > > +       { .compatible = "mediatek,mt6360-led", },
> > >
> > > > +       {},
> > >
> > > No need comma.
> >
> > ACK
>
> It is also no hurting comma.

Same explanation. It doesn't hurt per se, but its absence might serve a purpose.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Andy Shevchenko]

On Thu, Sep 10, 2020 at 11:11 AM Gene Chen <gene.chen.richtek@gmail.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> 於 2020年9月9日 週三 下午9:48寫道：
> > On Mon, Sep 7, 2020 at 1:31 PM Gene Chen <gene.chen.richtek@gmail.com> wrote:
> > > From: Gene Chen <gene_chen@richtek.com>

...

> > > +       if (priv->fled_strobe_used) {
> > > +               dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
> > > +               return -EINVAL;
> >
> > Hmm... Shouldn't be guaranteed by some framework?
> >
>
> Because both Flash LED use single logically control.
> It doesn't exist one LED is torch mode, and the other is strobe mode.

You mean you have always an attribute for hardware even if it doesn't
support a feature?
Can you consider hiding attributes?

...

> > > +       lcdev->max_brightness = (val - MT6360_ITORCH_MIN) / MT6360_ITORCH_STEP + 1;
> >
> > DIV_ROUND_UP(val - MT6360_ITORCH_MIN, MT6360_ITORCH_STEP) ?
> >
>
> This is mapping 0~val to 1~max_brightness as level.
> I convert val below MT6360_ITORCH_STEP to 1 for ignore max_brightness
> = 0, because 0 means disable.
> There is a little difference from DIV_ROUND_UP.

What div_round_up does is
(x + y - 1) / y
What do you do

x / y + 1 = (x + y)/y = ((x + 1) + y - 1)/y = DIV_ROUND_UP(x+1,y)

So, DIV_ROUND_UP(val - MT6360_ITORCH_MIN + 1, MT6360_ITORCH_STEP) ?

(yes I made classical off-by-one error)

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 978 bytes --]

On Thu 2020-09-10 14:34:54, Andy Shevchenko wrote:
> On Thu, Sep 10, 2020 at 11:18 AM Pavel Machek <pavel@ucw.cz> wrote:
> 
> ...
> 
> > > > > +enum {
> > > > > +       MT6360_LED_ISNK1 = 0,
> > > > > +       MT6360_LED_ISNK2,
> > > > > +       MT6360_LED_ISNK3,
> > > > > +       MT6360_LED_ISNK4,
> > > > > +       MT6360_LED_FLASH1,
> > > > > +       MT6360_LED_FLASH2,
> > > >
> > > > > +       MT6360_MAX_LEDS,
> > > >
> > > > No comma for terminator entry.
> > > >
> > >
> > > ACK
> >
> > Actually, that comma is fine. Its absence would be fine, too.
> 
> It is slightly better not to have to prevent (theoretical) rebase or
> other similar issues when a new item can go behind the terminator. In
> such a case compiler can easily tell you if something is wrong.

Okay, I see your point.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1372 bytes --]

Hi!

> > > +{
> > > +     struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
> > > +     struct mt6360_priv *priv = led->priv;
> > > +     u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> > > +     u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> > > +     u32 prev = priv->fled_torch_used, curr;
> > > +     int ret;
> > > +
> > > +     dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> > > +     if (priv->fled_strobe_used) {
> > > +             dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
> > > +             return -EINVAL;
> > > +     }
> >
> > So... how does its userland interface look like?
> >
> 
> 1. set FLED1 brightness
> # echo 1 > /sys/class/leds/white:flash1/flash_brightness
> 2. enable FLED1 strobe
> # echo 1 > /sys/class/leds/white:flash1/flash_strobe
> 3 . turn off FLED1 strobe (just used to gaurantee the strobe mode
> flash leds must be turned off)
> # echo 0 > /sys/class/leds/white:flash1/flash_strobe

I believe I'd preffer only exposing torch functionality in
/sys/class/leds. .. strobe can be supported using v4l2 APIs.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Jacek Anaszewski]

On 9/10/20 2:29 PM, Pavel Machek wrote:
> Hi!
> 
>>>> +{
>>>> +     struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
>>>> +     struct mt6360_priv *priv = led->priv;
>>>> +     u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
>>>> +     u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
>>>> +     u32 prev = priv->fled_torch_used, curr;
>>>> +     int ret;
>>>> +
>>>> +     dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
>>>> +     if (priv->fled_strobe_used) {
>>>> +             dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
>>>> +             return -EINVAL;
>>>> +     }
>>>
>>> So... how does its userland interface look like?
>>>
>>
>> 1. set FLED1 brightness
>> # echo 1 > /sys/class/leds/white:flash1/flash_brightness
>> 2. enable FLED1 strobe
>> # echo 1 > /sys/class/leds/white:flash1/flash_strobe
>> 3 . turn off FLED1 strobe (just used to gaurantee the strobe mode
>> flash leds must be turned off)
>> # echo 0 > /sys/class/leds/white:flash1/flash_strobe
> 
> I believe I'd preffer only exposing torch functionality in
> /sys/class/leds. .. strobe can be supported using v4l2 APIs.

Actually having LED flash class without strobe is pointless.
If you looked at led_classdev_flash_register_ext() you would see that
it fails with uninitialized strobe_set op. And V4L2 API for strobing
flash calls strobe_set from LED flash class beneath.

That was the idea behind LED and V4L2 flash API unification - there
is one hardware driver needed, the V4L2 Flash layer just takes over
control over it when needed.

-- 
Best regards,
Jacek Anaszewski

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Pavel Machek]

Hi!

> > > 1. set FLED1 brightness
> > > # echo 1 > /sys/class/leds/white:flash1/flash_brightness
> > > 2. enable FLED1 strobe
> > > # echo 1 > /sys/class/leds/white:flash1/flash_strobe
> > > 3 . turn off FLED1 strobe (just used to gaurantee the strobe mode
> > > flash leds must be turned off)
> > > # echo 0 > /sys/class/leds/white:flash1/flash_strobe
> > 
> > I believe I'd preffer only exposing torch functionality in
> > /sys/class/leds. .. strobe can be supported using v4l2 APIs.
> 
> Actually having LED flash class without strobe is pointless.
> If you looked at led_classdev_flash_register_ext() you would see that
> it fails with uninitialized strobe_set op. And V4L2 API for strobing
> flash calls strobe_set from LED flash class beneath.
> 
> That was the idea behind LED and V4L2 flash API unification - there
> is one hardware driver needed, the V4L2 Flash layer just takes over
> control over it when needed.

I agree that one driver is enough.

But we should not need flash_strobe file in sysfs. Simply use V4L2 for
that.

Best regards,
								Pavel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Jacek Anaszewski]

On 9/10/20 10:25 PM, Pavel Machek wrote:
> Hi!
> 
>>>> 1. set FLED1 brightness
>>>> # echo 1 > /sys/class/leds/white:flash1/flash_brightness
>>>> 2. enable FLED1 strobe
>>>> # echo 1 > /sys/class/leds/white:flash1/flash_strobe
>>>> 3 . turn off FLED1 strobe (just used to gaurantee the strobe mode
>>>> flash leds must be turned off)
>>>> # echo 0 > /sys/class/leds/white:flash1/flash_strobe
>>>
>>> I believe I'd preffer only exposing torch functionality in
>>> /sys/class/leds. .. strobe can be supported using v4l2 APIs.
>>
>> Actually having LED flash class without strobe is pointless.
>> If you looked at led_classdev_flash_register_ext() you would see that
>> it fails with uninitialized strobe_set op. And V4L2 API for strobing
>> flash calls strobe_set from LED flash class beneath.
>>
>> That was the idea behind LED and V4L2 flash API unification - there
>> is one hardware driver needed, the V4L2 Flash layer just takes over
>> control over it when needed.
> 
> I agree that one driver is enough.
> 
> But we should not need flash_strobe file in sysfs. Simply use V4L2 for
> that.

Apart from the fact that we can't remove it from LED flash class ABI
now, why would you like to prevent the user from exploiting main feature
of the hardware?

-- 
Best regards,
Jacek Anaszewski

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Jacek Anaszewski]

Hi Gene,

Thanks for the update.

On 9/7/20 12:27 PM, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> and 4-channel RGB LED support Register/Flash/Breath Mode
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>   drivers/leds/Kconfig       |  11 +
>   drivers/leds/Makefile      |   1 +
>   drivers/leds/leds-mt6360.c | 681 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 693 insertions(+)
>   create mode 100644 drivers/leds/leds-mt6360.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 1c181df..94a6d83 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -271,6 +271,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
> +	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.
> +	  Includes 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 c2c7d7a..5596427 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -66,6 +66,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..bd6fa48
> --- /dev/null
> +++ b/drivers/leds/leds-mt6360.c
> @@ -0,0 +1,681 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (C) 2020 MediaTek Inc.
> +//
> +// Author: Gene Chen <gene_chen@richtek.com>
> +
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <media/v4l2-flash-led-class.h>
> +
> +enum {
> +	MT6360_LED_ISNK1 = 0,
> +	MT6360_LED_ISNK2,
> +	MT6360_LED_ISNK3,
> +	MT6360_LED_ISNK4,
> +	MT6360_LED_FLASH1,
> +	MT6360_LED_FLASH2,
> +	MT6360_MAX_LEDS,
> +};
> +
> +#define MT6360_REG_RGBEN		0x380
> +#define MT6360_REG_ISNK(_led_no)	(0x381 + (_led_no))
> +#define MT6360_ISNK_ENMASK(_led_no)	BIT(7 - (_led_no))
> +#define MT6360_ISNK_MASK		0x1F
> +#define MT6360_CHRINDSEL_MASK		BIT(3)
> +
> +#define MT6360_REG_FLEDEN		0x37E
> +#define MT6360_REG_STRBTO		0x373
> +#define MT6360_REG_FLEDBASE(_id)	(0x372 + 4 * (_id - MT6360_LED_FLASH1))
> +#define MT6360_REG_FLEDISTRB(_id)	(MT6360_REG_FLEDBASE(_id) + 2)
> +#define MT6360_REG_FLEDITOR(_id)	(MT6360_REG_FLEDBASE(_id) + 3)
> +#define MT6360_REG_CHGSTAT2		0x3E1
> +#define MT6360_REG_FLEDSTAT1		0x3E9
> +#define MT6360_ITORCH_MASK		GENMASK(4, 0)
> +#define MT6360_ISTROBE_MASK		GENMASK(6, 0)
> +#define MT6360_STRBTO_MASK		GENMASK(6, 0)
> +#define MT6360_TORCHEN_MASK		BIT(3)
> +#define MT6360_STROBEN_MASK		BIT(2)
> +#define MT6360_FLCSEN_MASK(_id)		BIT(MT6360_LED_FLASH2 - _id)
> +#define MT6360_FLEDCHGVINOVP_MASK	BIT(3)
> +#define MT6360_FLED1STRBTO_MASK		BIT(11)
> +#define MT6360_FLED2STRBTO_MASK		BIT(10)
> +#define MT6360_FLED1STRB_MASK		BIT(9)
> +#define MT6360_FLED2STRB_MASK		BIT(8)
> +#define MT6360_FLED1SHORT_MASK		BIT(7)
> +#define MT6360_FLED2SHORT_MASK		BIT(6)
> +#define MT6360_FLEDLVF_MASK		BIT(3)
> +
> +/* 0 means led_off, add one for dummy */
> +#define MT6360_ISNK1_MAXLEVEL		13
> +#define MT6360_ISNK4_MAXLEVEL		31
> +
> +#define MT6360_ITORCH_MIN		25000
> +#define MT6360_ITORCH_STEP		12500
> +#define MT6360_ITORCH_MAX		400000
> +#define MT6360_ISTRB_MIN		50000
> +#define MT6360_ISTRB_STEP		12500
> +#define MT6360_ISTRB_MAX		1500000
> +#define MT6360_STRBTO_MIN		64000
> +#define MT6360_STRBTO_STEP		32000
> +#define MT6360_STRBTO_MAX		2432000
> +
> +#define FLED_TORCH_FLAG_MASK		0x0c
> +#define FLED_TORCH_FLAG_SHFT		2
> +#define FLED_STROBE_FLAG_MASK		0x03
> +
> +#define STATE_OFF			0
> +#define STATE_KEEP			1
> +#define STATE_ON			2
> +
> +struct mt6360_led {
> +	union {
> +		struct led_classdev isnk;
> +		struct led_classdev_flash flash;
> +	};
> +	struct v4l2_flash *v4l2_flash;
> +	struct mt6360_priv *priv;
> +	u32 led_no;
> +	u32 default_state;
> +};
> +
> +struct mt6360_priv {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct mt6360_led *leds[MT6360_MAX_LEDS];
> +	unsigned int fled_strobe_used;
> +	unsigned int fled_torch_used;
> +};
> +
> +static int mt6360_isnk_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> +{
> +	struct mt6360_led *led = container_of(lcdev, struct mt6360_led, isnk);
> +	struct mt6360_priv *priv = led->priv;
> +	u32 enable_mask = MT6360_ISNK_ENMASK(led->led_no);
> +	u32 val = level ? MT6360_ISNK_ENMASK(led->led_no) : 0;
> +	int ret;
> +
> +	dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> +
> +	if (level) {
> +		ret = regmap_update_bits(priv->regmap, MT6360_REG_ISNK(led->led_no),
> +					 MT6360_ISNK_MASK, level - 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, enable_mask, val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int mt6360_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level)
> +{
> +	struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
> +	struct mt6360_priv *priv = led->priv;
> +	u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> +	u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> +	u32 prev = priv->fled_torch_used, curr;
> +	int ret;
> +
> +	dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> +	if (priv->fled_strobe_used) {
> +		dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);

Doesn't hardware handle that? IOW, what happens when you have enabled
both torch and flash? If flash just overrides torch mode, than you
should not prevent enabling torch in this case.

> +		return -EINVAL;
> +	}
> +
> +	if (level)
> +		curr = prev | BIT(led->led_no);
> +	else
> +		curr = prev & (~BIT(led->led_no));
> +
> +	if (curr)
> +		val |= MT6360_TORCHEN_MASK;
> +
> +	if (level) {
> +		ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDITOR(led->led_no),
> +					 MT6360_ITORCH_MASK, level - 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask, val);
> +	if (ret)
> +		return ret;
> +
> +	priv->fled_torch_used = curr;
> +
> +	return 0;
> +}
> +
> +static int mt6360_strobe_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)

s/strobe_brightness_set/flash_brightness_set/

Let's stick to the op name.

> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct led_classdev *lcdev = &fl_cdev->led_cdev;

It would be good to add comment here explaining why this function
doesn't do anything.

> +	dev_dbg(lcdev->dev, "[%d] strobe brightness %d\n", led->led_no, brightness);
> +	return 0;
> +}
> +
> +static int _mt6360_strobe_brightness_set(struct led_classdev_flash *fl_cdev, u32 brightness)

s/strobe_brightness_set/flash_brightness_set/

> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	struct led_flash_setting *s = &fl_cdev->brightness;
> +	u32 val = (brightness - s->min) / s->step;
> +
> +	return regmap_update_bits(priv->regmap, MT6360_REG_FLEDISTRB(led->led_no),
> +				 MT6360_ISTROBE_MASK, val);
> +}
> +
> +static int mt6360_strobe_set(struct led_classdev_flash *fl_cdev, bool state)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	struct led_classdev *lcdev = &fl_cdev->led_cdev;
> +	struct led_flash_setting *s = &fl_cdev->brightness;
> +	u32 enable_mask = MT6360_STROBEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> +	u32 val = state ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> +	u32 prev = priv->fled_strobe_used, curr;
> +	int ret;
> +
> +	dev_dbg(lcdev->dev, "[%d] strobe state %d\n", led->led_no, state);
> +	if (priv->fled_torch_used) {
> +		dev_warn(lcdev->dev, "Please disable torch first [0x%x]\n", priv->fled_torch_used);

Similar comment as in case of enabling torch mode above.
If flash overrides torch then we're OK without this guard.

> +		return -EINVAL;
> +	}
> +
> +	if (state)
> +		curr = prev | BIT(led->led_no);
> +	else
> +		curr = prev & (~BIT(led->led_no));
> +
> +	if (curr)
> +		val |= MT6360_STROBEN_MASK;
> +
> +	ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask, val);
> +	if (ret) {
> +		dev_err(lcdev->dev, "[%d] control current source %d fail\n", led->led_no, state);
> +		return ret;
> +	}
> +
> +	/* used to prevent flash current spike when torch on */
> +	ret = _mt6360_strobe_brightness_set(fl_cdev, state ? s->val : s->min);
> +	if (ret)
> +		return ret;
> +
> +	if (!prev && curr)
> +		usleep_range(5000, 6000);
> +	else if (prev && !curr)
> +		udelay(500);
> +
> +	priv->fled_strobe_used = curr;
> +	return 0;
> +}
> +
> +static int mt6360_strobe_get(struct led_classdev_flash *fl_cdev, bool *state)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +
> +	*state = !!(priv->fled_strobe_used & BIT(led->led_no));
> +	return 0;
> +}
> +
> +static int mt6360_timeout_set(struct led_classdev_flash *fl_cdev, u32 timeout)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	struct led_flash_setting *s = &fl_cdev->timeout;
> +	u32 val = (timeout - s->min) / s->step;
> +
> +	return regmap_update_bits(priv->regmap, MT6360_REG_STRBTO, MT6360_STRBTO_MASK, val);
> +
> +}
> +
> +static int mt6360_fault_get(struct led_classdev_flash *fl_cdev, u32 *fault)
> +{
> +	struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	u16 fled_stat;
> +	unsigned int chg_stat, strobe_timeout_mask, fled_short_mask;
> +	u32 rfault = 0;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_CHGSTAT2, &chg_stat);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_raw_read(priv->regmap, MT6360_REG_FLEDSTAT1, &fled_stat, sizeof(fled_stat));
> +	if (ret)
> +		return ret;
> +
> +	if (led->led_no == MT6360_LED_FLASH1) {
> +		strobe_timeout_mask = MT6360_FLED1STRBTO_MASK;
> +		fled_short_mask = MT6360_FLED1SHORT_MASK;
> +
> +	} else {
> +		strobe_timeout_mask = MT6360_FLED2STRBTO_MASK;
> +		fled_short_mask = MT6360_FLED2SHORT_MASK;
> +	}
> +
> +	if (chg_stat & MT6360_FLEDCHGVINOVP_MASK)
> +		rfault |= LED_FAULT_INPUT_VOLTAGE;
> +
> +	if (fled_stat & strobe_timeout_mask)
> +		rfault |= LED_FAULT_TIMEOUT;
> +
> +	if (fled_stat & fled_short_mask)
> +		rfault |= LED_FAULT_SHORT_CIRCUIT;
> +
> +	if (fled_stat & MT6360_FLEDLVF_MASK)
> +		rfault |= LED_FAULT_UNDER_VOLTAGE;
> +
> +	*fault = rfault;
> +	return 0;
> +}
> +
> +static const struct led_flash_ops mt6360_flash_ops = {
> +	.flash_brightness_set = mt6360_strobe_brightness_set,
> +	.strobe_set = mt6360_strobe_set,
> +	.strobe_get = mt6360_strobe_get,
> +	.timeout_set = mt6360_timeout_set,
> +	.fault_get = mt6360_fault_get,
> +};
> +
> +static int mt6360_isnk_init_default_state(struct mt6360_led *led)
> +{
> +	struct mt6360_priv *priv = led->priv;
> +	unsigned int regval;
> +	u32 level;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_ISNK(led->led_no), &regval);
> +	if (ret)
> +		return ret;
> +	level = regval & MT6360_ISNK_MASK;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_RGBEN, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (regval & MT6360_ISNK_ENMASK(led->led_no))
> +		level += 1;
> +	else
> +		level = LED_OFF;
> +
> +	switch (led->default_state) {
> +	case STATE_ON:
> +		led->isnk.brightness = led->isnk.max_brightness;
> +		break;
> +	case STATE_KEEP:
> +		led->isnk.brightness = min(level, led->isnk.max_brightness);
> +		break;
> +	default:
> +		led->isnk.brightness = LED_OFF;
> +	}
> +
> +	return mt6360_isnk_brightness_set(&led->isnk, led->isnk.brightness);
> +}
> +
> +static int mt6360_isnk_register(struct device *parent, struct mt6360_led *led,
> +				struct led_init_data *init_data)
> +{
> +	struct mt6360_priv *priv = led->priv;
> +	int ret;
> +
> +	if (led->led_no == MT6360_LED_ISNK1) {
> +		/* change isink to sw mode */
> +		ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, MT6360_CHRINDSEL_MASK,
> +					 MT6360_CHRINDSEL_MASK);
> +		if (ret) {
> +			dev_err(parent, "Failed to config ISNK1 to SW mode\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = mt6360_isnk_init_default_state(led);
> +	if (ret) {
> +		dev_err(parent, "Failed to init %d isnk state\n", led->led_no);
> +		return ret;
> +	}
> +
> +	ret = devm_led_classdev_register_ext(parent, &led->isnk, init_data);
> +	if (ret) {
> +		dev_err(parent, "Couldn't register isink %d\n", led->led_no);
> +		return ret;
> +	}

You probably want also to register V4L2 indicator sub-device.
See drivers/leds/leds-as3645a.c and look for the
v4l2_flash_indicator_init() for reference.

> +	return 0;
> +}
> +
> +static int mt6360_flash_init_default_state(struct mt6360_led *led)
> +{
> +	struct led_classdev_flash *flash = &led->flash;
> +	struct mt6360_priv *priv = led->priv;
> +	u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> +	u32 level;
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_FLEDITOR(led->led_no), &regval);
> +	if (ret)
> +		return ret;
> +	level = regval & MT6360_ITORCH_MASK;
> +
> +	ret = regmap_read(priv->regmap, MT6360_REG_FLEDEN, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if ((regval & enable_mask) == enable_mask)
> +		level += 1;
> +	else
> +		level = LED_OFF;
> +
> +	switch (led->default_state) {
> +	case STATE_ON:
> +		flash->led_cdev.brightness = flash->led_cdev.max_brightness;
> +		break;
> +	case STATE_KEEP:
> +		flash->led_cdev.brightness = min(level, flash->led_cdev.max_brightness);
> +		break;
> +	default:
> +		flash->led_cdev.brightness = LED_OFF;
> +	}
> +
> +	return mt6360_torch_brightness_set(&flash->led_cdev, flash->led_cdev.brightness);
> +}
> +
> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> +static int mt6360_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> +{
> +	struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
> +	struct mt6360_led *led = container_of(flash, struct mt6360_led, flash);
> +	struct mt6360_priv *priv = led->priv;
> +	u32 enable_mask = MT6360_FLCSEN_MASK(led->led_no);
> +	int ret;
> +
> +	ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask,
> +				 enable ? enable_mask : 0);
> +	if (ret)
> +		return ret;
> +
> +	if (enable)
> +		priv->fled_strobe_used |= BIT(led->led_no);
> +	else
> +		priv->fled_strobe_used &= (~BIT(led->led_no));
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_flash_ops v4l2_flash_ops = {
> +	.external_strobe_set = mt6360_flash_external_strobe_set,
> +};
> +
> +static void mt6360_flash_init_v4l2_config(struct mt6360_led *led, struct v4l2_flash_config *config)
> +{
> +	struct led_classdev_flash *flash = &led->flash;
> +	struct led_classdev *lcdev = &flash->led_cdev;
> +	struct led_flash_setting *s = &config->intensity;
> +
> +	snprintf(config->dev_name, sizeof(config->dev_name), "%s", lcdev->name);
> +
> +	s->min = MT6360_ITORCH_MIN;
> +	s->step = MT6360_ITORCH_STEP;
> +	s->val = s->max = (s->min) + (lcdev->max_brightness - 1) * s->step;
> +
> +	config->has_external_strobe = 1;
> +}
> +#else
> +static const struct v4l2_flash_ops v4l2_flash_ops;
> +
> +static void mt6360_flash_init_v4l2_config(struct mt6360_led *led, struct v4l2_flash_config *config)
> +{
> +}
> +#endif
> +
> +static int mt6360_flash_register(struct device *parent, struct mt6360_led *led,
> +				 struct led_init_data *init_data)
> +{
> +	struct v4l2_flash_config v4l2_config = {0};
> +	int ret;
> +
> +	ret = mt6360_flash_init_default_state(led);
> +	if (ret) {
> +		dev_err(parent, "Failed to init %d flash state\n", led->led_no);
> +		return ret;
> +	}
> +
> +	ret = devm_led_classdev_flash_register_ext(parent, &led->flash, init_data);
> +	if (ret) {
> +		dev_err(parent, "Couldn't register flash %d\n", led->led_no);
> +		return ret;
> +	}
> +
> +	mt6360_flash_init_v4l2_config(led, &v4l2_config);
> +	led->v4l2_flash = v4l2_flash_init(parent, init_data->fwnode, &led->flash, &v4l2_flash_ops,
> +					  &v4l2_config);
> +	if (IS_ERR(led->v4l2_flash)) {
> +		dev_err(parent, "Failed to register %d v4l2 sd\n", led->led_no);
> +		return PTR_ERR(led->v4l2_flash);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6360_init_isnk_properties(struct mt6360_led *led, struct led_init_data *init_data)
> +{
> +	struct led_classdev *isnk = &led->isnk;
> +
> +	if (led->led_no == MT6360_LED_ISNK4)
> +		isnk->max_brightness = MT6360_ISNK4_MAXLEVEL;
> +	else
> +		isnk->max_brightness = MT6360_ISNK1_MAXLEVEL;
> +
> +	isnk->brightness_set_blocking = mt6360_isnk_brightness_set;
> +
> +	fwnode_property_read_string(init_data->fwnode, "linux,default-trigger",
> +				    &isnk->default_trigger);
> +
> +	return 0;
> +}
> +
> +static void clamp_align(u32 *v, u32 min, u32 max, u32 step)
> +{
> +	*v = clamp_val(*v, min, max);
> +	if (step > 1)
> +		*v = (*v - min) / step * step + min;
> +}
> +
> +static int mt6360_init_flash_properties(struct mt6360_led *led, struct led_init_data *init_data)
> +{
> +	struct led_classdev_flash *flash = &led->flash;
> +	struct led_classdev *lcdev = &flash->led_cdev;
> +	struct led_flash_setting *s;
> +	u32 val;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(init_data->fwnode, "led-max-microamp", &val);
> +	if (ret)
> +		val = MT6360_ITORCH_MIN;

In case of missing property I would print warning and inform the user
that we're defaulting to min value. Same applies to similar occurrences
below.

> +	else
> +		clamp_align(&val, MT6360_ITORCH_MIN, MT6360_ITORCH_MAX, MT6360_ITORCH_STEP);
> +
> +	lcdev->max_brightness = (val - MT6360_ITORCH_MIN) / MT6360_ITORCH_STEP + 1;
> +	lcdev->brightness_set_blocking = mt6360_torch_brightness_set;
> +	lcdev->flags |= LED_DEV_CAP_FLASH;
> +
> +	ret = fwnode_property_read_u32(init_data->fwnode, "flash-max-microamp", &val);
> +	if (ret)
> +		val = MT6360_ISTRB_MIN;
> +	else
> +		clamp_align(&val, MT6360_ISTRB_MIN, MT6360_ISTRB_MAX, MT6360_ISTRB_STEP);
> +
> +	s = &flash->brightness;
> +	s->min = MT6360_ISTRB_MIN;
> +	s->step = MT6360_ISTRB_STEP;
> +	s->val = s->max = val;
> +
> +	/* always configure as min level when off to prevent flash current spike */
> +	ret = _mt6360_strobe_brightness_set(flash, s->min);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_property_read_u32(init_data->fwnode, "flash-max-timeout-us", &val);
> +	if (ret)
> +		val = MT6360_STRBTO_MIN;
> +	else
> +		clamp_align(&val, MT6360_STRBTO_MIN, MT6360_STRBTO_MAX, MT6360_STRBTO_STEP);
> +
> +	s = &flash->timeout;
> +	s->min = MT6360_STRBTO_MIN;
> +	s->step = MT6360_STRBTO_STEP;
> +	s->val = s->max = val;
> +
> +	flash->ops = &mt6360_flash_ops;
> +
> +	return 0;
> +}
> +
> +static int mt6360_init_common_properties(struct mt6360_led *led, struct led_init_data *init_data)
> +{
> +	const char *str;
> +
> +	if (!fwnode_property_read_string(init_data->fwnode, "default-state", &str)) {
> +		if (!strcmp(str, "on"))
> +			led->default_state = STATE_ON;
> +		else if (!strcmp(str, "keep"))
> +			led->default_state = STATE_KEEP;
> +		else
> +			led->default_state = STATE_OFF;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6360_led_probe(struct platform_device *pdev)
> +{
> +	struct mt6360_priv *priv;
> +	struct fwnode_handle *child;
> +	int i, ret;
> +
> +	ret = device_get_child_node_count(&pdev->dev);
> +	if (!ret || ret > MT6360_MAX_LEDS)
> +		return -EINVAL;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = &pdev->dev;
> +
> +	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;
> +		struct led_init_data init_data = { .fwnode = child, };
> +		u32 reg;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret || reg >= MT6360_MAX_LEDS || priv->leds[reg]) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> +		if (!led) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		led->led_no = reg;
> +		led->priv = priv;
> +
> +		ret = mt6360_init_common_properties(led, &init_data);
> +		if (ret)
> +			goto out;
> +
> +		switch (reg) {
> +		case MT6360_LED_ISNK1 ... MT6360_LED_ISNK4:
> +			ret = mt6360_init_isnk_properties(led, &init_data);
> +			if (ret)
> +				goto out;
> +
> +			ret = mt6360_isnk_register(&pdev->dev, led, &init_data);
> +			break;
> +		default:
> +			ret = mt6360_init_flash_properties(led, &init_data);
> +			if (ret)
> +				goto out;
> +
> +			ret = mt6360_flash_register(&pdev->dev, led, &init_data);
> +		}
> +
> +		if (ret)
> +			goto out;
> +
> +		priv->leds[reg] = led;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +	return 0;
> +out:
> +	for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
> +		struct mt6360_led *led = priv->leds[i];
> +
> +		if (led && led->v4l2_flash)
> +			v4l2_flash_release(led->v4l2_flash);
> +
> +	}
> +
> +	return ret;
> +}
> +
> +static int mt6360_led_remove(struct platform_device *pdev)
> +{
> +	struct mt6360_priv *priv = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
> +		struct mt6360_led *led = priv->leds[i];
> +
> +		if (led && led->v4l2_flash)
> +			v4l2_flash_release(led->v4l2_flash);
> +
> +	}

You're using this for loop twice, so it would be nice to wrap
it with a function to avoid code redundancy.

> +
> +	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",
> +		.of_match_table = 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");
> 

-- 
Best regards,
Jacek Anaszewski

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Gene Chen]

Pavel Machek <pavel@ucw.cz> 於 2020年9月11日 週五 下午3:05寫道：
>
> Hi!
>
> > >+{
> > >+    struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
> > >+    struct mt6360_priv *priv = led->priv;
> > >+    u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> > >+    u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> > >+    u32 prev = priv->fled_torch_used, curr;
> > >+    int ret;
> > >+
> > >+    dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> > >+    if (priv->fled_strobe_used) {
> > >+            dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
> >
> > Doesn't hardware handle that? IOW, what happens when you have enabled
> > both torch and flash? If flash just overrides torch mode, than you
> > should not prevent enabling torch in this case.
>
> Yep, this is strange/confusing... and was reason why I asked for not
> supporting strobe from sysfs.
>
> Could I get you to remove code you are not commenting at when
> reviewing?
>

MT6360 FLED register define is STROBE_EN/TORCH_EN/CS1/CS2 (current
source) 4 bits.
The STROBE_EN/TORCH_EN is shared by FLED1 and FLED2.
If I want to enable FLED1 torch mode, I set TORCH_EN and CS1
If I want to enable FLED2 strobe mode, I set STROBE_EN and CS2
For example I set FLED1 torch, then I set FLED2 strobe.
When I set FLED2 strobe, I will see the strobe current is FLED2 add
FLED1 current which is not I want.
So I need disable FLED1 torch first.
Considering every circumstances is complicated when share same H/W
logic control.
And the other problem is torch mode switch to strobe mode needs ramp
time because strobe and torch mode can't be co-exist.

> > >+MODULE_AUTHOR("Gene Chen <gene_chen@richtek.com>");
> > >+MODULE_DESCRIPTION("MT6360 Led Driver");
>
> Led -> LED.
>

ACK

>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1207 bytes --]

Hi!

> >+{
> >+	struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
> >+	struct mt6360_priv *priv = led->priv;
> >+	u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> >+	u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> >+	u32 prev = priv->fled_torch_used, curr;
> >+	int ret;
> >+
> >+	dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> >+	if (priv->fled_strobe_used) {
> >+		dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
> 
> Doesn't hardware handle that? IOW, what happens when you have enabled
> both torch and flash? If flash just overrides torch mode, than you
> should not prevent enabling torch in this case.

Yep, this is strange/confusing... and was reason why I asked for not
supporting strobe from sysfs.

Could I get you to remove code you are not commenting at when
reviewing?

> >+MODULE_AUTHOR("Gene Chen <gene_chen@richtek.com>");
> >+MODULE_DESCRIPTION("MT6360 Led Driver");

Led -> LED.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Jacek Anaszewski]

Hi Pavel,

On 9/11/20 9:05 AM, Pavel Machek wrote:
> Hi!
> 
>>> +{
>>> +	struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
>>> +	struct mt6360_priv *priv = led->priv;
>>> +	u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
>>> +	u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
>>> +	u32 prev = priv->fled_torch_used, curr;
>>> +	int ret;
>>> +
>>> +	dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
>>> +	if (priv->fled_strobe_used) {
>>> +		dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
>>
>> Doesn't hardware handle that? IOW, what happens when you have enabled
>> both torch and flash? If flash just overrides torch mode, than you
>> should not prevent enabling torch in this case.
> 
> Yep, this is strange/confusing... and was reason why I asked for not
> supporting strobe from sysfs.

What you say now is even more confusing when we look at your ack
under this patch:

commit 7aea8389a77abf9fde254aca2434a605c7704f58
Author: Jacek Anaszewski <j.anaszewski@samsung.com>
Date:   Fri Jan 9 07:22:51 2015 -0800

     leds: Add LED Flash class extension to the LED subsystem

     Some LED devices support two operation modes - torch and flash.
     This patch provides support for flash LED devices in the LED subsystem
     by introducing new sysfs attributes and kernel internal interface.
     The attributes being introduced are: flash_brightness, flash_strobe,
     flash_timeout, max_flash_timeout, max_flash_brightness, flash_fault,
     flash_sync_strobe and available_sync_leds. All the flash related
     features are placed in a separate module.

     The modifications aim to be compatible with V4L2 framework requirements
     related to the flash devices management. The design assumes that V4L2
     sub-device can take of the LED class device control and communicate
     with it through the kernel internal interface. When V4L2 Flash 
sub-device
     file is opened, the LED class device sysfs interface is made
     unavailable.

     Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
     Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
     Cc: Richard Purdie <rpurdie@rpsys.net>
     Acked-by: Pavel Machek <pavel@ucw.cz>
     Signed-off-by: Bryan Wu <cooloney@gmail.com>


> Could I get you to remove code you are not commenting at when
> reviewing?
> 
>>> +MODULE_AUTHOR("Gene Chen <gene_chen@richtek.com>");
>>> +MODULE_DESCRIPTION("MT6360 Led Driver");
> 
> Led -> LED.
> 
> 									Pavel
> 

-- 
Best regards,
Jacek Anaszewski

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360

[Jacek Anaszewski]

On 9/11/20 1:24 AM, Gene Chen wrote:
> Pavel Machek <pavel@ucw.cz> 於 2020年9月11日 週五 下午3:05寫道：
>>
>> Hi!
>>
>>>> +{
>>>> +    struct mt6360_led *led = container_of(lcdev, struct mt6360_led, flash.led_cdev);
>>>> +    struct mt6360_priv *priv = led->priv;
>>>> +    u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
>>>> +    u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
>>>> +    u32 prev = priv->fled_torch_used, curr;
>>>> +    int ret;
>>>> +
>>>> +    dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
>>>> +    if (priv->fled_strobe_used) {
>>>> +            dev_warn(lcdev->dev, "Please disable strobe first [%d]\n", priv->fled_strobe_used);
>>>
>>> Doesn't hardware handle that? IOW, what happens when you have enabled
>>> both torch and flash? If flash just overrides torch mode, than you
>>> should not prevent enabling torch in this case.
>>
>> Yep, this is strange/confusing... and was reason why I asked for not
>> supporting strobe from sysfs.
>>
>> Could I get you to remove code you are not commenting at when
>> reviewing?
>>
> 
> MT6360 FLED register define is STROBE_EN/TORCH_EN/CS1/CS2 (current
> source) 4 bits.
> The STROBE_EN/TORCH_EN is shared by FLED1 and FLED2.
> If I want to enable FLED1 torch mode, I set TORCH_EN and CS1
> If I want to enable FLED2 strobe mode, I set STROBE_EN and CS2
> For example I set FLED1 torch, then I set FLED2 strobe.
> When I set FLED2 strobe, I will see the strobe current is FLED2 add
> FLED1 current which is not I want.
> So I need disable FLED1 torch first.
> Considering every circumstances is complicated when share same H/W
> logic control.
> And the other problem is torch mode switch to strobe mode needs ramp
> time because strobe and torch mode can't be co-exist.

Thank you for the explanation. So we have to keep your guards
but I would return -EBUSY instead of -EINVAL.

This would be also consistent with what
drivers/media/v4l2-core/v4l2-flash-led-class.c
does in its v4l2_flash_s_ctrl(), case V4L2_CID_FLASH_STROBE - it returns
-EBUSY if __software_strobe_mode_inactive() returns false.

The advantage of V4L2 Flash interface is that it has LED_MODE that
can be set to torch or flash, but in LED subsystem we don't have
the counterpart.

-- 
Best regards,
Jacek Anaszewski

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] dt-bindings: leds: Add bindings for MT6360 LED

[Rob Herring]

On Mon, Sep 07, 2020 at 06:27:39PM +0800, Gene Chen wrote:
> From: Gene Chen <gene_chen@richtek.com>
> 
> Add bindings document for LED support on MT6360 PMIC
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>  .../devicetree/bindings/leds/leds-mt6360.yaml      | 105 +++++++++++++++++++++
>  1 file changed, 105 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6360.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-mt6360.yaml b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
> new file mode 100644
> index 0000000..72914c6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-mt6360.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LED driver for MT6360 PMIC from MediaTek Integrated.
> +
> +maintainers:
> +  - Gene Chen <gene_chen@richtek.com>
> +
> +description: |
> +  This module is part of the MT6360 MFD device.
> +  The LED controller is represented as a sub-node of the PMIC node on
> +  the device tree.
> +  This device has six current sinks.
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt6360-led
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^led@[0-5]$":
> +    type: object
> +    description: |

Don't need '|' if no line breaks/formatting to maintain.

> +      Properties for a single LED.

This needs to reference leds/common.yaml.

> +
> +    properties:
> +      reg:
> +        description: Index of the LED.
> +        enum:
> +          - 0 # LED output INDICATOR1
> +          - 1 # LED output INDICATOR2
> +          - 2 # LED output INDICATOR3
> +          - 3 # LED output INDICATOR4
> +          - 4 # LED output FLED1
> +          - 5 # LED output FLED2

       unevaluatedProperties: false

> +
> +required:
> +  - compatible
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +   #include <dt-bindings/leds/common.h>
> +   led-controller {
> +     compatible = "mediatek,mt6360-led";
> +     #address-cells = <1>;
> +     #size-cells = <0>;
> +
> +     led@0 {
> +       reg = <0>;
> +       function = LED_FUNCTION_INDICATOR;
> +       color = <LED_COLOR_ID_RED>;
> +       default-state = "off";
> +     };
> +     led@1 {
> +       reg = <1>;
> +       function = LED_FUNCTION_INDICATOR;
> +       color = <LED_COLOR_ID_GREEN>;
> +       default-state = "off";
> +     };
> +     led@2 {
> +       reg = <2>;
> +       function = LED_FUNCTION_INDICATOR;
> +       color = <LED_COLOR_ID_BLUE>;
> +       default-state = "off";
> +     };
> +     led@3 {
> +       reg = <3>;
> +       function = LED_FUNCTION_INDICATOR;
> +       color = <LED_COLOR_ID_AMBER>;
> +       default-state = "off";
> +     };
> +     led@4 {
> +       reg = <4>;
> +       function = LED_FUNCTION_FLASH;
> +       color = <LED_COLOR_ID_WHITE>;
> +       function-enumerator = <1>;
> +       default-state = "off";
> +       led-max-microamp = <200000>;
> +       flash-max-microamp = <500000>;
> +       flash-max-timeout-us = <1024000>;
> +     };
> +     led@5 {
> +       reg = <5>;
> +       function = LED_FUNCTION_FLASH;
> +       color = <LED_COLOR_ID_WHITE>;
> +       function-enumerator = <2>;
> +       default-state = "off";
> +       led-max-microamp = <200000>;
> +       flash-max-microamp = <500000>;
> +       flash-max-timeout-us = <1024000>;
> +     };
> +   };
> +...
> -- 
> 2.7.4
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel