[PATCH v5 0/2] Add RT5033 Flash LED driver

[PATCH v5 0/2] Add RT5033 Flash LED driver

[Ingi Kim]

This is a fifth version of the patch set to support RT5033 Flash Led.
It is based on RFC [1] from Jacek's patch set.

Changes since v5:
 - Rebase on Jacek's devel branch
   git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
 - Remove work queue from driver.
 - Change brightness_set_sync ops to brightness_set_blocking ops.
 - Remove "iout_torch_max" and "iout_flash_max" values in rt5033_led struct.
 - Add missed case for distinguishing between setting brightness.
   for iout_joint case and for individual LEDs.
 - Add missed mutex_lock.
 - Remove guard to check dev->of_node.
 - Change type of value to const __be32 to get return value of of_find_property.

Changes since v4:
 - Use of_node_put() when DT parse miss
 - Move struct(rt5033_led) from include/linux/mfd/rt5033.h
   to local driver/leds/leds-rt5033.c
 - Remove MODULE_DEVICE_TABLE
 - Add interface to handle two LEDs.

Changes since v3:
 - Use mutex and work queue
 - Split brightness set func (sync / async)
 - Add flash API (flash_brightness_set)
 - Move struct(rt5033_led_config_data) to local area
 - Code clean

Changes since v2:
 - Split MFC code from rt5033 flash led patch
 - Fix typo error
 - Change naming of mfd register back again
 - Fix compile error

Original cover letter:
 This patch supports flash led of RT5033 PMIC.

[1] https://lkml.org/lkml/2015/8/20/426

Ingi Kim (2):
  leds: rt5033: Add DT binding for RT5033
  leds: rt5033: Add RT5033 Flash led device driver

 .../devicetree/bindings/leds/leds-rt5033.txt       |  46 ++
 drivers/leds/Kconfig                               |   8 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-rt5033.c                         | 522 +++++++++++++++++++++
 include/linux/mfd/rt5033-private.h                 |  51 ++
 5 files changed, 628 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-rt5033.txt
 create mode 100644 drivers/leds/leds-rt5033.c

-- 
2.0.5

[PATCH v5 1/2] leds: rt5033: Add DT binding for RT5033

[Ingi Kim]

This patch adds the device tree bindings for RT5033 flash LEDs.

Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/leds/leds-rt5033.txt       | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-rt5033.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-rt5033.txt b/Documentation/devicetree/bindings/leds/leds-rt5033.txt
new file mode 100644
index 0000000..17159d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-rt5033.txt
@@ -0,0 +1,46 @@
+* Richtek Technology Corporation - RT5033 Flash LED Driver
+
+The RT5033 Flash LED Circuit is designed for one or two LEDs driving
+for torch and strobe applications, it provides an I2C software command
+to trigger the torch and strobe operation.
+
+There are two LED outputs available - FLED1 and FLED2. Each of them can
+control a separate LED or they can be connected together to double
+the maximum current for a single connected LED. One LED is represented
+by one child node.
+
+Required properties:
+- compatible : Must be "richtek,rt5033-led".
+
+A discrete LED element connected to the device must be represented by a child
+node - see Documentation/devicetree/bindings/leds/common.txt.
+
+Required properties for the LED child node:
+  See Documentation/devicetree/bindings/leds/common.txt
+- led-sources : device current output identifiers:  0 - FLED1, 1 - FLED2
+- led-max-microamp : 12.5mA to 200mA, step by 12.5mA.
+- flash-max-microamp :
+	Turn on one LED channel : 50mA to 800mA, step by 25mA.
+	Turn on two LED channels : 50mA to 600mA, step by 25mA.
+- flash-max-timeout-us : 64ms to 1216ms, step by 32ms.
+
+Optional properties for the LED child node:
+- label : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+rt5033 {
+	compatible = "richtek,rt5033";
+
+	led {
+		compatible = "richtek,rt5033-led";
+
+		flash-led {
+			label = "rt5033-flash";
+			led-sources = <0>, <1>;
+			led-max-microamp = <200000>;
+			flash-max-microamp = <800000>;
+			flash-max-timeout-us = <1216000>;
+		};
+	};
+}
-- 
2.0.5

[PATCH v5 2/2] leds: rt5033: Add RT5033 Flash led device driver

[Ingi Kim]

This patch adds device driver of Richtek RT5033 PMIC.
The RT5033 Flash LED Circuit is designed for one or two LEDs driving
for torch and strobe applications, it provides an I2C software command
to trigger the torch and strobe operation.

Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
---
 drivers/leds/Kconfig               |   8 +
 drivers/leds/Makefile              |   1 +
 drivers/leds/leds-rt5033.c         | 522 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/rt5033-private.h |  51 ++++
 4 files changed, 582 insertions(+)
 create mode 100644 drivers/leds/leds-rt5033.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b1ab8bd..f41ac9b 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -345,6 +345,14 @@ config LEDS_PCA963X
 	  LED driver chip accessed via the I2C bus. Supported
 	  devices include PCA9633 and PCA9634
 
+config LEDS_RT5033
+	tristate "LED support for RT5033 PMIC"
+	depends on LEDS_CLASS_FLASH && OF
+	depends on MFD_RT5033
+	help
+	  This option enables support for on-chip LED driver on
+	  RT5033 PMIC.
+
 config LEDS_WM831X_STATUS
 	tristate "LED support for status LEDs on WM831x PMICs"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e9d53092..77cfad5 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
 obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
 obj-$(CONFIG_LEDS_SUNFIRE)		+= leds-sunfire.o
 obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
+obj-$(CONFIG_LEDS_RT5033)		+= leds-rt5033.o
 obj-$(CONFIG_LEDS_GPIO_REGISTER)	+= leds-gpio-register.o
 obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
 obj-$(CONFIG_LEDS_LP3944)		+= leds-lp3944.o
diff --git a/drivers/leds/leds-rt5033.c b/drivers/leds/leds-rt5033.c
new file mode 100644
index 0000000..a28554a
--- /dev/null
+++ b/drivers/leds/leds-rt5033.c
@@ -0,0 +1,522 @@
+/*
+ * led driver for RT5033
+ *
+ * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
+ * Ingi Kim <ingi2.kim@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/led-class-flash.h>
+#include <linux/mfd/rt5033.h>
+#include <linux/mfd/rt5033-private.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+
+#define RT5033_LED_FLASH_TIMEOUT_MIN		64000
+#define RT5033_LED_FLASH_TIMEOUT_STEP		32000
+#define RT5033_LED_FLASH_BRIGHTNESS_MIN		50000
+#define RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH	600000
+#define RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH	800000
+#define RT5033_LED_FLASH_BRIGHTNESS_STEP	25000
+#define RT5033_LED_TORCH_BRIGHTNESS_MIN		12500
+#define RT5033_LED_TORCH_BRIGHTNESS_STEP	12500
+
+/* Macro to get offset of rt5033_led_config_data */
+#define RT5033_LED_CONFIG_DATA_OFFSET(val, step, min)	(((val) - (min)) \
+							/ (step))
+#define FLED1_IOUT		(BIT(0))
+#define FLED2_IOUT		(BIT(1))
+
+enum rt5033_fled {
+	FLED1,
+	FLED2,
+};
+
+struct rt5033_sub_led {
+	enum rt5033_fled fled_id;
+	struct led_classdev_flash fled_cdev;
+
+	u32 torch_brightness;
+	u32 flash_brightness;
+	u32 flash_timeout;
+};
+
+/* RT5033 Flash led platform data */
+struct rt5033_led {
+	struct device *dev;
+	struct mutex lock;
+	struct regmap *regmap;
+	struct rt5033_sub_led sub_leds[2];
+
+	bool iout_joint;
+	u8 fled_mask;
+	u8 current_iout;
+};
+
+struct rt5033_led_config_data {
+	const char *label[2];
+	u32 flash_max_microamp[2];
+	u32 flash_max_timeout[2];
+	u32 torch_max_microamp[2];
+	u32 num_leds;
+};
+
+static struct rt5033_sub_led *flcdev_to_sub_led(
+					struct led_classdev_flash *fled_cdev)
+{
+	return container_of(fled_cdev, struct rt5033_sub_led, fled_cdev);
+}
+
+static struct rt5033_led *sub_led_to_led(struct rt5033_sub_led *sub_led)
+{
+	return container_of(sub_led, struct rt5033_led,
+			    sub_leds[sub_led->fled_id]);
+}
+
+static bool rt5033_fled_used(struct rt5033_led *led, enum rt5033_fled fled_id)
+{
+	u8 fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
+
+	return led->fled_mask & fled_bit;
+}
+
+static u8 rt5033_fled_iout_get(struct rt5033_led *led, enum rt5033_fled fled_id)
+{
+	u8 fled_bit;
+
+	if (led->iout_joint)
+		fled_bit = led->fled_mask;
+	else
+		fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
+
+	return fled_bit;
+}
+
+static int rt5033_led_iout_set_off(struct rt5033_led *led,
+				   enum rt5033_fled fled_id)
+{
+	int ret;
+	u8 fled_bit, value;
+
+	fled_bit = rt5033_fled_iout_get(led, fled_id);
+	led->current_iout ^= fled_bit;
+	led->sub_leds[fled_id].fled_cdev.led_cdev.brightness = 0;
+
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
+				 RT5033_FLED_FUNC1_MASK,
+				 RT5033_FLED_PINCTRL | led->current_iout);
+	if (ret < 0)
+		return ret;
+
+	if (led->current_iout) {
+		value = led->sub_leds[!fled_id].fled_cdev.led_cdev.brightness;
+		ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
+					 RT5033_FLED_CTRL1_MASK,
+					 (value - 1) << 4);
+	}
+
+	return ret;
+}
+
+static int rt5033_led_brightness_set(struct led_classdev *led_cdev,
+				     enum led_brightness brightness)
+{
+	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
+	struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
+	struct rt5033_led *led = sub_led_to_led(sub_led);
+	int fled_id = sub_led->fled_id, ret;
+	u8 fled_bit;
+
+	mutex_lock(&led->lock);
+
+	if (!brightness) {
+		ret = rt5033_led_iout_set_off(led, fled_id);
+		goto torch_unlock;
+	}
+
+	fled_bit = rt5033_fled_iout_get(led, fled_id);
+	led->current_iout |= fled_bit;
+
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
+				 RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
+	if (ret < 0)
+		goto torch_unlock;
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
+				 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
+				 led->current_iout);
+	if (ret < 0)
+		goto torch_unlock;
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
+				 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED);
+
+torch_unlock:
+	mutex_unlock(&led->lock);
+	return ret;
+}
+
+static int rt5033_led_flash_brightness_set(struct led_classdev_flash *fled_cdev,
+					   u32 brightness)
+{
+	struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
+	struct rt5033_led *led = sub_led_to_led(sub_led);
+	u32 flash_brightness_offset;
+
+	mutex_lock(&led->lock);
+
+	flash_brightness_offset =
+		RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.brightness.val,
+					     sub_led->fled_cdev.brightness.step,
+					     sub_led->fled_cdev.brightness.min);
+	sub_led->flash_brightness = flash_brightness_offset;
+
+	mutex_unlock(&led->lock);
+
+	return 0;
+}
+
+static int rt5033_led_flash_timeout_set(struct led_classdev_flash *fled_cdev,
+					u32 timeout)
+{
+	struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
+	struct rt5033_led *led = sub_led_to_led(sub_led);
+	u32 flash_tm_offset;
+
+	mutex_lock(&led->lock);
+
+	flash_tm_offset =
+		RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.timeout.val,
+					      sub_led->fled_cdev.timeout.step,
+					      sub_led->fled_cdev.timeout.min);
+	sub_led->flash_timeout = flash_tm_offset;
+
+	mutex_unlock(&led->lock);
+
+	return 0;
+}
+
+static int rt5033_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
+				       bool state)
+{
+	struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
+	struct rt5033_led *led = sub_led_to_led(sub_led);
+	enum rt5033_fled fled_id = sub_led->fled_id;
+	int ret;
+	u8 fled_bit;
+
+	mutex_lock(&led->lock);
+
+	fled_bit = rt5033_fled_iout_get(led, fled_id);
+	led->current_iout |= fled_bit;
+
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
+				 RT5033_FLED_FUNC2_MASK, 0);
+	if (ret < 0)
+		goto strobe_unlock;
+
+	if (!state) {
+		ret = rt5033_led_iout_set_off(led, fled_id);
+		goto strobe_unlock;
+	}
+
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL1,
+				 RT5033_FLED_STRBCTRL1_MASK,
+				 sub_led->flash_brightness);
+	if (ret < 0)
+		goto strobe_unlock;
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL2,
+				 RT5033_FLED_STRBCTRL2_MASK,
+				 sub_led->flash_timeout);
+	if (ret < 0)
+		goto strobe_unlock;
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
+				 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
+				 RT5033_FLED_STRB_SEL | led->current_iout);
+	if (ret < 0)
+		goto strobe_unlock;
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
+				 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED |
+				 RT5033_FLED_SREG_STRB);
+
+	led->current_iout = 0;
+
+strobe_unlock:
+	mutex_unlock(&led->lock);
+	return ret;
+}
+
+static const struct led_flash_ops flash_ops = {
+	.flash_brightness_set = rt5033_led_flash_brightness_set,
+	.strobe_set = rt5033_led_flash_strobe_set,
+	.timeout_set = rt5033_led_flash_timeout_set,
+};
+
+static void rt5033_init_flash_properties(struct rt5033_sub_led *sub_led,
+					 struct rt5033_led_config_data *led_cfg)
+{
+	struct led_classdev_flash *fled_cdev = &sub_led->fled_cdev;
+	struct rt5033_led *led = sub_led_to_led(sub_led);
+	struct led_flash_setting *tm_set, *fl_set;
+	enum rt5033_fled fled_id = sub_led->fled_id;
+
+	tm_set = &fled_cdev->timeout;
+	tm_set->min = RT5033_LED_FLASH_TIMEOUT_MIN;
+	tm_set->max = led_cfg->flash_max_timeout[fled_id];
+	tm_set->step = RT5033_LED_FLASH_TIMEOUT_STEP;
+	tm_set->val = tm_set->max;
+
+	fl_set = &fled_cdev->brightness;
+	fl_set->min = RT5033_LED_FLASH_BRIGHTNESS_MIN;
+	if (led->iout_joint)
+		fl_set->max = min(led_cfg->flash_max_microamp[FLED1] +
+				  led_cfg->flash_max_microamp[FLED2],
+				  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH);
+	else
+		fl_set->max = min(led_cfg->flash_max_microamp[fled_id],
+				  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH);
+	fl_set->step = RT5033_LED_FLASH_BRIGHTNESS_STEP;
+	fl_set->val = fl_set->max;
+}
+
+static void rt5033_led_init_fled_cdev(struct rt5033_sub_led *sub_led,
+				      struct rt5033_led_config_data *led_cfg)
+{
+	struct led_classdev_flash *fled_cdev;
+	struct led_classdev *led_cdev;
+	enum rt5033_fled fled_id = sub_led->fled_id;
+	u32 flash_tm_offset, flash_brightness_offset;
+
+	/* Initialize LED Flash class device */
+	fled_cdev = &sub_led->fled_cdev;
+	fled_cdev->ops = &flash_ops;
+	led_cdev = &fled_cdev->led_cdev;
+
+	led_cdev->name = led_cfg->label[fled_id];
+
+	led_cdev->brightness_set_blocking = rt5033_led_brightness_set;
+	led_cdev->max_brightness = RT5033_LED_CONFIG_DATA_OFFSET(
+				   led_cfg->torch_max_microamp[fled_id],
+				   RT5033_LED_TORCH_BRIGHTNESS_STEP,
+				   RT5033_LED_TORCH_BRIGHTNESS_MIN);
+	led_cdev->flags |= LED_DEV_CAP_FLASH;
+
+	rt5033_init_flash_properties(sub_led, led_cfg);
+
+	flash_tm_offset = RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->timeout.val,
+							fled_cdev->timeout.step,
+							fled_cdev->timeout.min);
+	flash_brightness_offset =
+		RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->brightness.val,
+					      fled_cdev->brightness.step,
+					      fled_cdev->brightness.min);
+
+	sub_led->flash_timeout = flash_tm_offset;
+	sub_led->flash_brightness = flash_brightness_offset;
+}
+
+static int rt5033_led_parse_dt(struct rt5033_led *led, struct device *dev,
+			       struct rt5033_led_config_data *cfg,
+			       struct device_node **sub_nodes)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *child_node;
+	struct rt5033_sub_led *sub_leds = led->sub_leds;
+	struct property *prop;
+	u32 led_sources[2];
+	enum rt5033_fled fled_id;
+	int i, ret;
+
+	for_each_available_child_of_node(np, child_node) {
+		prop = of_find_property(child_node, "led-sources", NULL);
+		if (prop) {
+			const __be32 *srcs = NULL;
+
+			for (i = 0; i < ARRAY_SIZE(led_sources); ++i) {
+				srcs = of_prop_next_u32(prop, srcs,
+							&led_sources[i]);
+				if (!srcs)
+					break;
+			}
+		} else {
+			dev_err(dev, "led-sources DT property missing\n");
+			ret = -EINVAL;
+			goto err_parse_dt;
+		}
+
+		if (i == 2) {
+			fled_id = FLED1;
+			led->fled_mask = FLED1_IOUT | FLED2_IOUT;
+		} else if (led_sources[0] == FLED1) {
+			fled_id = FLED1;
+			led->fled_mask |= FLED1_IOUT;
+		} else if (led_sources[0] == FLED2) {
+			fled_id = FLED2;
+			led->fled_mask |= FLED2_IOUT;
+		} else {
+			dev_err(dev, "Wrong led-sources DT property value.\n");
+			ret = -EINVAL;
+			goto err_parse_dt;
+		}
+
+		if (sub_nodes[fled_id]) {
+			dev_err(dev,
+				"Conflicting \"led-sources\" DT properties\n");
+			ret = -EINVAL;
+			goto err_parse_dt;
+		}
+
+		sub_nodes[fled_id] = child_node;
+		sub_leds[fled_id].fled_id = fled_id;
+
+		cfg->label[fled_id] =
+			of_get_property(child_node, "label", NULL) ? :
+					child_node->name;
+
+		ret = of_property_read_u32(child_node, "led-max-microamp",
+					   &cfg->torch_max_microamp[fled_id]);
+		if (ret < 0) {
+			dev_err(dev, "failed to parse led-max-microamp\n");
+			goto err_parse_dt;
+		}
+
+		ret = of_property_read_u32(child_node, "flash-max-microamp",
+					   &cfg->flash_max_microamp[fled_id]);
+		if (ret < 0) {
+			dev_err(dev, "failed to parse flash-max-microamp\n");
+			goto err_parse_dt;
+		}
+
+		ret = of_property_read_u32(child_node, "flash-max-timeout-us",
+					   &cfg->flash_max_timeout[fled_id]);
+		if (ret < 0) {
+			dev_err(dev, "failed to parse flash-max-timeout-us\n");
+			goto err_parse_dt;
+		}
+
+		if (++cfg->num_leds == 2 ||
+		    (rt5033_fled_used(led, FLED1) &&
+		     rt5033_fled_used(led, FLED2))) {
+			of_node_put(child_node);
+			break;
+		}
+	}
+
+	if (cfg->num_leds == 0) {
+		dev_err(dev, "No DT child node found for connected LED(s).\n");
+		return -EINVAL;
+	}
+
+	return 0;
+
+err_parse_dt:
+	of_node_put(child_node);
+	return ret;
+}
+
+static int rt5033_led_probe(struct platform_device *pdev)
+{
+	struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent);
+	struct rt5033_led *led;
+	struct rt5033_sub_led *sub_leds;
+	struct device_node *sub_nodes[2] = {};
+	struct rt5033_led_config_data led_cfg = {};
+	int init_fled_cdev[2], i, ret;
+
+	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, led);
+	led->dev = &pdev->dev;
+	led->regmap = rt5033->regmap;
+	sub_leds = led->sub_leds;
+
+	ret = rt5033_led_parse_dt(led, &pdev->dev, &led_cfg, sub_nodes);
+	if (ret < 0)
+		return ret;
+
+	if (led_cfg.num_leds == 1 && rt5033_fled_used(led, FLED1) &&
+	    rt5033_fled_used(led, FLED2))
+		led->iout_joint = true;
+
+	mutex_init(&led->lock);
+
+	init_fled_cdev[FLED1] =
+			led->iout_joint || rt5033_fled_used(led, FLED1);
+	init_fled_cdev[FLED2] =
+			!led->iout_joint && rt5033_fled_used(led, FLED2);
+
+	for (i = FLED1; i <= FLED2; ++i) {
+		if (!init_fled_cdev[i])
+			continue;
+
+		rt5033_led_init_fled_cdev(&sub_leds[i], &led_cfg);
+		ret = led_classdev_flash_register(led->dev,
+						  &sub_leds[i].fled_cdev);
+		if (ret < 0) {
+			if (i == FLED2)
+				goto err_register_led2;
+			else
+				goto err_register_led1;
+		}
+	}
+
+	led->current_iout = 0;
+	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
+				 RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET);
+	if (ret < 0)
+		dev_dbg(led->dev, "Failed to reset flash led (%d)\n", ret);
+
+	return 0;
+
+err_register_led2:
+	/* It is possible than only FLED2 was to be registered */
+	if (!init_fled_cdev[FLED1])
+		goto err_register_led1;
+	led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
+err_register_led1:
+	mutex_destroy(&led->lock);
+
+	return ret;
+}
+
+static int rt5033_led_remove(struct platform_device *pdev)
+{
+	struct rt5033_led *led = platform_get_drvdata(pdev);
+	struct rt5033_sub_led *sub_leds = led->sub_leds;
+
+	if (led->iout_joint || rt5033_fled_used(led, FLED1))
+		led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
+
+	if (!led->iout_joint && rt5033_fled_used(led, FLED2))
+		led_classdev_flash_unregister(&sub_leds[FLED2].fled_cdev);
+
+	mutex_destroy(&led->lock);
+
+	return 0;
+}
+
+static const struct of_device_id rt5033_led_match[] = {
+	{ .compatible = "richtek,rt5033-led", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rt5033_led_match);
+
+static struct platform_driver rt5033_led_driver = {
+	.driver = {
+		.name = "rt5033-led",
+		.of_match_table = rt5033_led_match,
+	},
+	.probe		= rt5033_led_probe,
+	.remove		= rt5033_led_remove,
+};
+module_platform_driver(rt5033_led_driver);
+
+MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>");
+MODULE_DESCRIPTION("Richtek RT5033 LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:rt5033-led");
diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
index 1b63fc2..b20c7e4 100644
--- a/include/linux/mfd/rt5033-private.h
+++ b/include/linux/mfd/rt5033-private.h
@@ -93,6 +93,57 @@ enum rt5033_reg {
 #define RT5033_RT_CTRL1_UUG_MASK	0x02
 #define RT5033_RT_HZ_MASK		0x01
 
+/* RT5033 FLED Function1 register */
+#define RT5033_FLED_FUNC1_MASK		0x97
+#define RT5033_FLED_EN_LEDCS1		0x01
+#define RT5033_FLED_EN_LEDCS2		0x02
+#define RT5033_FLED_STRB_SEL		0x04
+#define RT5033_FLED_PINCTRL		0x10
+#define RT5033_FLED_RESET		0x80
+
+/* RT5033 FLED Function2 register */
+#define RT5033_FLED_FUNC2_MASK		0x81
+#define RT5033_FLED_ENFLED		0x01
+#define RT5033_FLED_SREG_STRB		0x80
+
+/* RT5033 FLED Strobe Control1 */
+#define RT5033_FLED_STRBCTRL1_MASK		0xFF
+#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX	0xE0
+#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF	0x0D
+#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX	0x1F
+
+/* RT5033 FLED Strobe Control2 */
+#define RT5033_FLED_STRBCTRL2_MASK	0x3F
+#define RT5033_FLED_STRBCTRL2_TM_DEF	0x0F
+#define RT5033_FLED_STRBCTRL2_TM_MAX	0x3F
+
+/* RT5033 FLED Control1 */
+#define RT5033_FLED_CTRL1_MASK		0xF7
+#define RT5033_FLED_CTRL1_TORCH_CUR_DEF	0x20
+#define RT5033_FLED_CTRL1_TORCH_CUR_MAX	0xF0
+#define RT5033_FLED_CTRL1_LBP_DEF	0x02
+#define RT5033_FLED_CTRL1_LBP_MAX	0x07
+
+/* RT5033 FLED Control2 */
+#define RT5033_FLED_CTRL2_MASK		0xFF
+#define RT5033_FLED_CTRL2_VMID_DEF	0x37
+#define RT5033_FLED_CTRL2_VMID_MAX	0x3F
+#define RT5033_FLED_CTRL2_TRACK_ALIVE	0x40
+#define RT5033_FLED_CTRL2_MID_TRACK	0x80
+
+/* RT5033 FLED Control4 */
+#define RT5033_FLED_CTRL4_MASK		0xE0
+#define RT5033_FLED_CTRL4_RESV		0x14
+#define RT5033_FLED_CTRL4_VTRREG_DEF	0x40
+#define RT5033_FLED_CTRL4_VTRREG_MAX	0x60
+#define RT5033_FLED_CTRL4_TRACK_CLK	0x80
+
+/* RT5033 FLED Control5 */
+#define RT5033_FLED_CTRL5_MASK		0xC0
+#define RT5033_FLED_CTRL5_RESV		0x10
+#define RT5033_FLED_CTRL5_TA_GOOD	0x40
+#define RT5033_FLED_CTRL5_TA_EXIST	0x80
+
 /* RT5033 control register */
 #define RT5033_CTRL_FCCM_BUCK_MASK		0x00
 #define RT5033_CTRL_BUCKOMS_MASK		0x01
-- 
2.0.5

Re: [PATCH v5 2/2] leds: rt5033: Add RT5033 Flash led device driver

[Jacek Anaszewski]

Hi Ingi,

Thanks for the update. I have few comments below.

On 11/17/2015 10:04 AM, Ingi Kim wrote:
> This patch adds device driver of Richtek RT5033 PMIC.
> The RT5033 Flash LED Circuit is designed for one or two LEDs driving
> for torch and strobe applications, it provides an I2C software command
> to trigger the torch and strobe operation.
>
> Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
> ---
>   drivers/leds/Kconfig               |   8 +
>   drivers/leds/Makefile              |   1 +
>   drivers/leds/leds-rt5033.c         | 522 +++++++++++++++++++++++++++++++++++++
>   include/linux/mfd/rt5033-private.h |  51 ++++
>   4 files changed, 582 insertions(+)
>   create mode 100644 drivers/leds/leds-rt5033.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index b1ab8bd..f41ac9b 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -345,6 +345,14 @@ config LEDS_PCA963X
>   	  LED driver chip accessed via the I2C bus. Supported
>   	  devices include PCA9633 and PCA9634
>
> +config LEDS_RT5033
> +	tristate "LED support for RT5033 PMIC"
> +	depends on LEDS_CLASS_FLASH && OF
> +	depends on MFD_RT5033
> +	help
> +	  This option enables support for on-chip LED driver on
> +	  RT5033 PMIC.
> +
>   config LEDS_WM831X_STATUS
>   	tristate "LED support for status LEDs on WM831x PMICs"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index e9d53092..77cfad5 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
>   obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
>   obj-$(CONFIG_LEDS_SUNFIRE)		+= leds-sunfire.o
>   obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
> +obj-$(CONFIG_LEDS_RT5033)		+= leds-rt5033.o
>   obj-$(CONFIG_LEDS_GPIO_REGISTER)	+= leds-gpio-register.o
>   obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
>   obj-$(CONFIG_LEDS_LP3944)		+= leds-lp3944.o
> diff --git a/drivers/leds/leds-rt5033.c b/drivers/leds/leds-rt5033.c
> new file mode 100644
> index 0000000..a28554a
> --- /dev/null
> +++ b/drivers/leds/leds-rt5033.c
> @@ -0,0 +1,522 @@
> +/*
> + * led driver for RT5033
> + *
> + * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
> + * Ingi Kim <ingi2.kim@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/led-class-flash.h>
> +#include <linux/mfd/rt5033.h>
> +#include <linux/mfd/rt5033-private.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#define RT5033_LED_FLASH_TIMEOUT_MIN		64000
> +#define RT5033_LED_FLASH_TIMEOUT_STEP		32000
> +#define RT5033_LED_FLASH_BRIGHTNESS_MIN		50000
> +#define RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH	600000
> +#define RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH	800000
> +#define RT5033_LED_FLASH_BRIGHTNESS_STEP	25000
> +#define RT5033_LED_TORCH_BRIGHTNESS_MIN		12500
> +#define RT5033_LED_TORCH_BRIGHTNESS_STEP	12500
> +
> +/* Macro to get offset of rt5033_led_config_data */
> +#define RT5033_LED_CONFIG_DATA_OFFSET(val, step, min)	(((val) - (min)) \
> +							/ (step))
> +#define FLED1_IOUT		(BIT(0))
> +#define FLED2_IOUT		(BIT(1))
> +
> +enum rt5033_fled {
> +	FLED1,
> +	FLED2,
> +};
> +
> +struct rt5033_sub_led {
> +	enum rt5033_fled fled_id;
> +	struct led_classdev_flash fled_cdev;
> +
> +	u32 torch_brightness;
> +	u32 flash_brightness;
> +	u32 flash_timeout;
> +};
> +
> +/* RT5033 Flash led platform data */
> +struct rt5033_led {
> +	struct device *dev;
> +	struct mutex lock;
> +	struct regmap *regmap;
> +	struct rt5033_sub_led sub_leds[2];
> +
> +	bool iout_joint;
> +	u8 fled_mask;
> +	u8 current_iout;
> +};
> +
> +struct rt5033_led_config_data {
> +	const char *label[2];
> +	u32 flash_max_microamp[2];
> +	u32 flash_max_timeout[2];
> +	u32 torch_max_microamp[2];
> +	u32 num_leds;
> +};
> +
> +static struct rt5033_sub_led *flcdev_to_sub_led(
> +					struct led_classdev_flash *fled_cdev)
> +{
> +	return container_of(fled_cdev, struct rt5033_sub_led, fled_cdev);
> +}
> +
> +static struct rt5033_led *sub_led_to_led(struct rt5033_sub_led *sub_led)
> +{
> +	return container_of(sub_led, struct rt5033_led,
> +			    sub_leds[sub_led->fled_id]);
> +}
> +
> +static bool rt5033_fled_used(struct rt5033_led *led, enum rt5033_fled fled_id)
> +{
> +	u8 fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
> +
> +	return led->fled_mask & fled_bit;
> +}
> +
> +static u8 rt5033_fled_iout_get(struct rt5033_led *led, enum rt5033_fled fled_id)
> +{
> +	u8 fled_bit;
> +
> +	if (led->iout_joint)
> +		fled_bit = led->fled_mask;
> +	else
> +		fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
> +
> +	return fled_bit;
> +}

What is the purpose of this function? It seems to return
a value that is always equal to led->fled_mask, regardless of the
fled_id passed. Just use led->fled_mask directly.

> +static int rt5033_led_iout_set_off(struct rt5033_led *led,
> +				   enum rt5033_fled fled_id)
> +{
> +	int ret;
> +	u8 fled_bit, value;
> +
> +	fled_bit = rt5033_fled_iout_get(led, fled_id);
> +	led->current_iout ^= fled_bit;
> +	led->sub_leds[fled_id].fled_cdev.led_cdev.brightness = 0;
> +
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
> +				 RT5033_FLED_FUNC1_MASK,
> +				 RT5033_FLED_PINCTRL | led->current_iout);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (led->current_iout) {
> +		value = led->sub_leds[!fled_id].fled_cdev.led_cdev.brightness;
> +		ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
> +					 RT5033_FLED_CTRL1_MASK,
> +					 (value - 1) << 4);
> +	}

I infer that the purpose of this function is to turn the LED off.
Could you explain why two commands are needed for this?

> +	return ret;
> +}
> +
> +static int rt5033_led_brightness_set(struct led_classdev *led_cdev,
> +				     enum led_brightness brightness)
> +{
> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
> +	struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
> +	struct rt5033_led *led = sub_led_to_led(sub_led);
> +	int fled_id = sub_led->fled_id, ret;
> +	u8 fled_bit;
> +
> +	mutex_lock(&led->lock);
> +
> +	if (!brightness) {
> +		ret = rt5033_led_iout_set_off(led, fled_id);
> +		goto torch_unlock;
> +	}
> +
> +	fled_bit = rt5033_fled_iout_get(led, fled_id);
> +	led->current_iout |= fled_bit;

You don't need fled_bit, just assign the result to led->current_iout.

> +
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
> +				 RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
> +	if (ret < 0)
> +		goto torch_unlock;
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
> +				 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
> +				 led->current_iout);
> +	if (ret < 0)
> +		goto torch_unlock;
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
> +				 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED);

Could you shortly describe the function of above three registers?

> +torch_unlock:
> +	mutex_unlock(&led->lock);
> +	return ret;
> +}
> +
> +static int rt5033_led_flash_brightness_set(struct led_classdev_flash *fled_cdev,
> +					   u32 brightness)
> +{
> +	struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
> +	struct rt5033_led *led = sub_led_to_led(sub_led);
> +	u32 flash_brightness_offset;
> +
> +	mutex_lock(&led->lock);
> +
> +	flash_brightness_offset =
> +		RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.brightness.val,
> +					     sub_led->fled_cdev.brightness.step,
> +					     sub_led->fled_cdev.brightness.min);

Please align two above lines to the opening parenthesis.

> +	sub_led->flash_brightness = flash_brightness_offset;

flash_brightness_offset variable is redundant.

> +
> +	mutex_unlock(&led->lock);
> +
> +	return 0;
> +}
> +
> +static int rt5033_led_flash_timeout_set(struct led_classdev_flash *fled_cdev,
> +					u32 timeout)
> +{
> +	struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
> +	struct rt5033_led *led = sub_led_to_led(sub_led);
> +	u32 flash_tm_offset;
> +
> +	mutex_lock(&led->lock);
> +
> +	flash_tm_offset =
> +		RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.timeout.val,
> +					      sub_led->fled_cdev.timeout.step,
> +					      sub_led->fled_cdev.timeout.min);
> +	sub_led->flash_timeout = flash_tm_offset;

flash_tm_offset variable is redundant.

> +	mutex_unlock(&led->lock);
> +
> +	return 0;
> +}
> +
> +static int rt5033_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
> +				       bool state)
> +{
> +	struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
> +	struct rt5033_led *led = sub_led_to_led(sub_led);
> +	enum rt5033_fled fled_id = sub_led->fled_id;
> +	int ret;
> +	u8 fled_bit;
> +
> +	mutex_lock(&led->lock);
> +
> +	fled_bit = rt5033_fled_iout_get(led, fled_id);
> +	led->current_iout |= fled_bit;
> +
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
> +				 RT5033_FLED_FUNC2_MASK, 0);
> +	if (ret < 0)
> +		goto strobe_unlock;
> +
> +	if (!state) {
> +		ret = rt5033_led_iout_set_off(led, fled_id);
> +		goto strobe_unlock;
> +	}
> +
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL1,
> +				 RT5033_FLED_STRBCTRL1_MASK,
> +				 sub_led->flash_brightness);

Does the device allow for setting flash brightness separately for
each iout? Currently it looks like the setting is shared.

> +	if (ret < 0)
> +		goto strobe_unlock;
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL2,
> +				 RT5033_FLED_STRBCTRL2_MASK,
> +				 sub_led->flash_timeout);
> +	if (ret < 0)
> +		goto strobe_unlock;
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
> +				 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
> +				 RT5033_FLED_STRB_SEL | led->current_iout);
> +	if (ret < 0)
> +		goto strobe_unlock;
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
> +				 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED |
> +				 RT5033_FLED_SREG_STRB);
> +
> +	led->current_iout = 0;
> +
> +strobe_unlock:
> +	mutex_unlock(&led->lock);
> +	return ret;
> +}
> +
> +static const struct led_flash_ops flash_ops = {
> +	.flash_brightness_set = rt5033_led_flash_brightness_set,
> +	.strobe_set = rt5033_led_flash_strobe_set,
> +	.timeout_set = rt5033_led_flash_timeout_set,
> +};
> +
> +static void rt5033_init_flash_properties(struct rt5033_sub_led *sub_led,
> +					 struct rt5033_led_config_data *led_cfg)
> +{
> +	struct led_classdev_flash *fled_cdev = &sub_led->fled_cdev;
> +	struct rt5033_led *led = sub_led_to_led(sub_led);
> +	struct led_flash_setting *tm_set, *fl_set;
> +	enum rt5033_fled fled_id = sub_led->fled_id;
> +
> +	tm_set = &fled_cdev->timeout;
> +	tm_set->min = RT5033_LED_FLASH_TIMEOUT_MIN;
> +	tm_set->max = led_cfg->flash_max_timeout[fled_id];
> +	tm_set->step = RT5033_LED_FLASH_TIMEOUT_STEP;
> +	tm_set->val = tm_set->max;
> +
> +	fl_set = &fled_cdev->brightness;
> +	fl_set->min = RT5033_LED_FLASH_BRIGHTNESS_MIN;
> +	if (led->iout_joint)
> +		fl_set->max = min(led_cfg->flash_max_microamp[FLED1] +
> +				  led_cfg->flash_max_microamp[FLED2],
> +				  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH);
> +	else
> +		fl_set->max = min(led_cfg->flash_max_microamp[fled_id],
> +				  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH);
> +	fl_set->step = RT5033_LED_FLASH_BRIGHTNESS_STEP;
> +	fl_set->val = fl_set->max;
> +}
> +
> +static void rt5033_led_init_fled_cdev(struct rt5033_sub_led *sub_led,
> +				      struct rt5033_led_config_data *led_cfg)
> +{
> +	struct led_classdev_flash *fled_cdev;
> +	struct led_classdev *led_cdev;
> +	enum rt5033_fled fled_id = sub_led->fled_id;
> +	u32 flash_tm_offset, flash_brightness_offset;
> +
> +	/* Initialize LED Flash class device */
> +	fled_cdev = &sub_led->fled_cdev;
> +	fled_cdev->ops = &flash_ops;
> +	led_cdev = &fled_cdev->led_cdev;
> +
> +	led_cdev->name = led_cfg->label[fled_id];
> +
> +	led_cdev->brightness_set_blocking = rt5033_led_brightness_set;
> +	led_cdev->max_brightness = RT5033_LED_CONFIG_DATA_OFFSET(
> +				   led_cfg->torch_max_microamp[fled_id],
> +				   RT5033_LED_TORCH_BRIGHTNESS_STEP,
> +				   RT5033_LED_TORCH_BRIGHTNESS_MIN);
> +	led_cdev->flags |= LED_DEV_CAP_FLASH;
> +
> +	rt5033_init_flash_properties(sub_led, led_cfg);
> +
> +	flash_tm_offset = RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->timeout.val,
> +							fled_cdev->timeout.step,
> +							fled_cdev->timeout.min);
> +	flash_brightness_offset =
> +		RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->brightness.val,
> +					      fled_cdev->brightness.step,
> +					      fled_cdev->brightness.min);
> +
> +	sub_led->flash_timeout = flash_tm_offset;
> +	sub_led->flash_brightness = flash_brightness_offset;

Same here - there is no need for flash_tm_offset and
flash_brightness_offset variables.

> +}
> +
> +static int rt5033_led_parse_dt(struct rt5033_led *led, struct device *dev,
> +			       struct rt5033_led_config_data *cfg,
> +			       struct device_node **sub_nodes)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct device_node *child_node;
> +	struct rt5033_sub_led *sub_leds = led->sub_leds;
> +	struct property *prop;
> +	u32 led_sources[2];
> +	enum rt5033_fled fled_id;
> +	int i, ret;
> +
> +	for_each_available_child_of_node(np, child_node) {
> +		prop = of_find_property(child_node, "led-sources", NULL);
> +		if (prop) {
> +			const __be32 *srcs = NULL;
> +
> +			for (i = 0; i < ARRAY_SIZE(led_sources); ++i) {
> +				srcs = of_prop_next_u32(prop, srcs,
> +							&led_sources[i]);
> +				if (!srcs)
> +					break;
> +			}
> +		} else {
> +			dev_err(dev, "led-sources DT property missing\n");
> +			ret = -EINVAL;
> +			goto err_parse_dt;
> +		}
> +
> +		if (i == 2) {
> +			fled_id = FLED1;
> +			led->fled_mask = FLED1_IOUT | FLED2_IOUT;
> +		} else if (led_sources[0] == FLED1) {
> +			fled_id = FLED1;
> +			led->fled_mask |= FLED1_IOUT;
> +		} else if (led_sources[0] == FLED2) {
> +			fled_id = FLED2;
> +			led->fled_mask |= FLED2_IOUT;
> +		} else {
> +			dev_err(dev, "Wrong led-sources DT property value.\n");
> +			ret = -EINVAL;
> +			goto err_parse_dt;
> +		}
> +
> +		if (sub_nodes[fled_id]) {
> +			dev_err(dev,
> +				"Conflicting \"led-sources\" DT properties\n");
> +			ret = -EINVAL;
> +			goto err_parse_dt;
> +		}
> +
> +		sub_nodes[fled_id] = child_node;
> +		sub_leds[fled_id].fled_id = fled_id;
> +
> +		cfg->label[fled_id] =
> +			of_get_property(child_node, "label", NULL) ? :
> +					child_node->name;
> +
> +		ret = of_property_read_u32(child_node, "led-max-microamp",
> +					   &cfg->torch_max_microamp[fled_id]);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to parse led-max-microamp\n");
> +			goto err_parse_dt;
> +		}
> +
> +		ret = of_property_read_u32(child_node, "flash-max-microamp",
> +					   &cfg->flash_max_microamp[fled_id]);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to parse flash-max-microamp\n");
> +			goto err_parse_dt;
> +		}
> +
> +		ret = of_property_read_u32(child_node, "flash-max-timeout-us",
> +					   &cfg->flash_max_timeout[fled_id]);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to parse flash-max-timeout-us\n");
> +			goto err_parse_dt;
> +		}
> +
> +		if (++cfg->num_leds == 2 ||
> +		    (rt5033_fled_used(led, FLED1) &&
> +		     rt5033_fled_used(led, FLED2))) {
> +			of_node_put(child_node);
> +			break;
> +		}
> +	}
> +
> +	if (cfg->num_leds == 0) {
> +		dev_err(dev, "No DT child node found for connected LED(s).\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +
> +err_parse_dt:
> +	of_node_put(child_node);
> +	return ret;
> +}
> +
> +static int rt5033_led_probe(struct platform_device *pdev)
> +{
> +	struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent);
> +	struct rt5033_led *led;
> +	struct rt5033_sub_led *sub_leds;
> +	struct device_node *sub_nodes[2] = {};
> +	struct rt5033_led_config_data led_cfg = {};
> +	int init_fled_cdev[2], i, ret;
> +
> +	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, led);
> +	led->dev = &pdev->dev;
> +	led->regmap = rt5033->regmap;
> +	sub_leds = led->sub_leds;
> +
> +	ret = rt5033_led_parse_dt(led, &pdev->dev, &led_cfg, sub_nodes);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (led_cfg.num_leds == 1 && rt5033_fled_used(led, FLED1) &&
> +	    rt5033_fled_used(led, FLED2))
> +		led->iout_joint = true;
> +
> +	mutex_init(&led->lock);
> +
> +	init_fled_cdev[FLED1] =
> +			led->iout_joint || rt5033_fled_used(led, FLED1);
> +	init_fled_cdev[FLED2] =
> +			!led->iout_joint && rt5033_fled_used(led, FLED2);
> +
> +	for (i = FLED1; i <= FLED2; ++i) {
> +		if (!init_fled_cdev[i])
> +			continue;
> +
> +		rt5033_led_init_fled_cdev(&sub_leds[i], &led_cfg);
> +		ret = led_classdev_flash_register(led->dev,
> +						  &sub_leds[i].fled_cdev);
> +		if (ret < 0) {
> +			if (i == FLED2)
> +				goto err_register_led2;
> +			else
> +				goto err_register_led1;
> +		}
> +	}
> +
> +	led->current_iout = 0;
> +	ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
> +				 RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET);
> +	if (ret < 0)
> +		dev_dbg(led->dev, "Failed to reset flash led (%d)\n", ret);
> +
> +	return 0;
> +
> +err_register_led2:
> +	/* It is possible than only FLED2 was to be registered */
> +	if (!init_fled_cdev[FLED1])
> +		goto err_register_led1;
> +	led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
> +err_register_led1:
> +	mutex_destroy(&led->lock);
> +
> +	return ret;
> +}
> +
> +static int rt5033_led_remove(struct platform_device *pdev)
> +{
> +	struct rt5033_led *led = platform_get_drvdata(pdev);
> +	struct rt5033_sub_led *sub_leds = led->sub_leds;
> +
> +	if (led->iout_joint || rt5033_fled_used(led, FLED1))
> +		led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
> +
> +	if (!led->iout_joint && rt5033_fled_used(led, FLED2))
> +		led_classdev_flash_unregister(&sub_leds[FLED2].fled_cdev);
> +
> +	mutex_destroy(&led->lock);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rt5033_led_match[] = {
> +	{ .compatible = "richtek,rt5033-led", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, rt5033_led_match);
> +
> +static struct platform_driver rt5033_led_driver = {
> +	.driver = {
> +		.name = "rt5033-led",
> +		.of_match_table = rt5033_led_match,
> +	},
> +	.probe		= rt5033_led_probe,
> +	.remove		= rt5033_led_remove,
> +};
> +module_platform_driver(rt5033_led_driver);
> +
> +MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>");
> +MODULE_DESCRIPTION("Richtek RT5033 LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:rt5033-led");
> diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
> index 1b63fc2..b20c7e4 100644
> --- a/include/linux/mfd/rt5033-private.h
> +++ b/include/linux/mfd/rt5033-private.h
> @@ -93,6 +93,57 @@ enum rt5033_reg {
>   #define RT5033_RT_CTRL1_UUG_MASK	0x02
>   #define RT5033_RT_HZ_MASK		0x01
>
> +/* RT5033 FLED Function1 register */
> +#define RT5033_FLED_FUNC1_MASK		0x97
> +#define RT5033_FLED_EN_LEDCS1		0x01
> +#define RT5033_FLED_EN_LEDCS2		0x02
> +#define RT5033_FLED_STRB_SEL		0x04
> +#define RT5033_FLED_PINCTRL		0x10
> +#define RT5033_FLED_RESET		0x80
> +
> +/* RT5033 FLED Function2 register */
> +#define RT5033_FLED_FUNC2_MASK		0x81
> +#define RT5033_FLED_ENFLED		0x01
> +#define RT5033_FLED_SREG_STRB		0x80
> +
> +/* RT5033 FLED Strobe Control1 */
> +#define RT5033_FLED_STRBCTRL1_MASK		0xFF
> +#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX	0xE0
> +#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF	0x0D
> +#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX	0x1F
> +
> +/* RT5033 FLED Strobe Control2 */
> +#define RT5033_FLED_STRBCTRL2_MASK	0x3F
> +#define RT5033_FLED_STRBCTRL2_TM_DEF	0x0F
> +#define RT5033_FLED_STRBCTRL2_TM_MAX	0x3F
> +
> +/* RT5033 FLED Control1 */
> +#define RT5033_FLED_CTRL1_MASK		0xF7
> +#define RT5033_FLED_CTRL1_TORCH_CUR_DEF	0x20
> +#define RT5033_FLED_CTRL1_TORCH_CUR_MAX	0xF0
> +#define RT5033_FLED_CTRL1_LBP_DEF	0x02
> +#define RT5033_FLED_CTRL1_LBP_MAX	0x07
> +
> +/* RT5033 FLED Control2 */
> +#define RT5033_FLED_CTRL2_MASK		0xFF
> +#define RT5033_FLED_CTRL2_VMID_DEF	0x37
> +#define RT5033_FLED_CTRL2_VMID_MAX	0x3F
> +#define RT5033_FLED_CTRL2_TRACK_ALIVE	0x40
> +#define RT5033_FLED_CTRL2_MID_TRACK	0x80
> +
> +/* RT5033 FLED Control4 */
> +#define RT5033_FLED_CTRL4_MASK		0xE0
> +#define RT5033_FLED_CTRL4_RESV		0x14
> +#define RT5033_FLED_CTRL4_VTRREG_DEF	0x40
> +#define RT5033_FLED_CTRL4_VTRREG_MAX	0x60
> +#define RT5033_FLED_CTRL4_TRACK_CLK	0x80
> +
> +/* RT5033 FLED Control5 */
> +#define RT5033_FLED_CTRL5_MASK		0xC0
> +#define RT5033_FLED_CTRL5_RESV		0x10
> +#define RT5033_FLED_CTRL5_TA_GOOD	0x40
> +#define RT5033_FLED_CTRL5_TA_EXIST	0x80
> +
>   /* RT5033 control register */
>   #define RT5033_CTRL_FCCM_BUCK_MASK		0x00
>   #define RT5033_CTRL_BUCKOMS_MASK		0x01
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v5 2/2] leds: rt5033: Add RT5033 Flash led device driver

[Ingi Kim]

Hi Jacek,

Thanks for the review.

I'm trying to reply in detail what you mentioned.
Please contact me If you need more detail to understand what I replied.

On 2015년 11월 19일 00:46, Jacek Anaszewski wrote:
> Hi Ingi,
> 
> Thanks for the update. I have few comments below.
> 
> On 11/17/2015 10:04 AM, Ingi Kim wrote:
>> This patch adds device driver of Richtek RT5033 PMIC.
>> The RT5033 Flash LED Circuit is designed for one or two LEDs driving
>> for torch and strobe applications, it provides an I2C software command
>> to trigger the torch and strobe operation.
>>
>> Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
>> ---
>>   drivers/leds/Kconfig               |   8 +
>>   drivers/leds/Makefile              |   1 +
>>   drivers/leds/leds-rt5033.c         | 522 +++++++++++++++++++++++++++++++++++++
>>   include/linux/mfd/rt5033-private.h |  51 ++++
>>   4 files changed, 582 insertions(+)
>>   create mode 100644 drivers/leds/leds-rt5033.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index b1ab8bd..f41ac9b 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -345,6 +345,14 @@ config LEDS_PCA963X
>>         LED driver chip accessed via the I2C bus. Supported
>>         devices include PCA9633 and PCA9634
>>
>> +config LEDS_RT5033
>> +    tristate "LED support for RT5033 PMIC"
>> +    depends on LEDS_CLASS_FLASH && OF
>> +    depends on MFD_RT5033
>> +    help
>> +      This option enables support for on-chip LED driver on
>> +      RT5033 PMIC.
>> +
>>   config LEDS_WM831X_STATUS
>>       tristate "LED support for status LEDs on WM831x PMICs"
>>       depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index e9d53092..77cfad5 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -23,6 +23,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE)        += leds-cobalt-qube.o
>>   obj-$(CONFIG_LEDS_COBALT_RAQ)        += leds-cobalt-raq.o
>>   obj-$(CONFIG_LEDS_SUNFIRE)        += leds-sunfire.o
>>   obj-$(CONFIG_LEDS_PCA9532)        += leds-pca9532.o
>> +obj-$(CONFIG_LEDS_RT5033)        += leds-rt5033.o
>>   obj-$(CONFIG_LEDS_GPIO_REGISTER)    += leds-gpio-register.o
>>   obj-$(CONFIG_LEDS_GPIO)            += leds-gpio.o
>>   obj-$(CONFIG_LEDS_LP3944)        += leds-lp3944.o
>> diff --git a/drivers/leds/leds-rt5033.c b/drivers/leds/leds-rt5033.c
>> new file mode 100644
>> index 0000000..a28554a
>> --- /dev/null
>> +++ b/drivers/leds/leds-rt5033.c
>> @@ -0,0 +1,522 @@
>> +/*
>> + * led driver for RT5033
>> + *
>> + * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
>> + * Ingi Kim <ingi2.kim@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <linux/led-class-flash.h>
>> +#include <linux/mfd/rt5033.h>
>> +#include <linux/mfd/rt5033-private.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define RT5033_LED_FLASH_TIMEOUT_MIN        64000
>> +#define RT5033_LED_FLASH_TIMEOUT_STEP        32000
>> +#define RT5033_LED_FLASH_BRIGHTNESS_MIN        50000
>> +#define RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH    600000
>> +#define RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH    800000
>> +#define RT5033_LED_FLASH_BRIGHTNESS_STEP    25000
>> +#define RT5033_LED_TORCH_BRIGHTNESS_MIN        12500
>> +#define RT5033_LED_TORCH_BRIGHTNESS_STEP    12500
>> +
>> +/* Macro to get offset of rt5033_led_config_data */
>> +#define RT5033_LED_CONFIG_DATA_OFFSET(val, step, min)    (((val) - (min)) \
>> +                            / (step))
>> +#define FLED1_IOUT        (BIT(0))
>> +#define FLED2_IOUT        (BIT(1))
>> +
>> +enum rt5033_fled {
>> +    FLED1,
>> +    FLED2,
>> +};
>> +
>> +struct rt5033_sub_led {
>> +    enum rt5033_fled fled_id;
>> +    struct led_classdev_flash fled_cdev;
>> +
>> +    u32 torch_brightness;
>> +    u32 flash_brightness;
>> +    u32 flash_timeout;
>> +};
>> +
>> +/* RT5033 Flash led platform data */
>> +struct rt5033_led {
>> +    struct device *dev;
>> +    struct mutex lock;
>> +    struct regmap *regmap;
>> +    struct rt5033_sub_led sub_leds[2];
>> +
>> +    bool iout_joint;
>> +    u8 fled_mask;
>> +    u8 current_iout;
>> +};
>> +
>> +struct rt5033_led_config_data {
>> +    const char *label[2];
>> +    u32 flash_max_microamp[2];
>> +    u32 flash_max_timeout[2];
>> +    u32 torch_max_microamp[2];
>> +    u32 num_leds;
>> +};
>> +
>> +static struct rt5033_sub_led *flcdev_to_sub_led(
>> +                    struct led_classdev_flash *fled_cdev)
>> +{
>> +    return container_of(fled_cdev, struct rt5033_sub_led, fled_cdev);
>> +}
>> +
>> +static struct rt5033_led *sub_led_to_led(struct rt5033_sub_led *sub_led)
>> +{
>> +    return container_of(sub_led, struct rt5033_led,
>> +                sub_leds[sub_led->fled_id]);
>> +}
>> +
>> +static bool rt5033_fled_used(struct rt5033_led *led, enum rt5033_fled fled_id)
>> +{
>> +    u8 fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
>> +
>> +    return led->fled_mask & fled_bit;
>> +}
>> +
>> +static u8 rt5033_fled_iout_get(struct rt5033_led *led, enum rt5033_fled fled_id)
>> +{
>> +    u8 fled_bit;
>> +
>> +    if (led->iout_joint)
>> +        fled_bit = led->fled_mask;
>> +    else
>> +        fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
>> +
>> +    return fled_bit;
>> +}
> 
> What is the purpose of this function? It seems to return
> a value that is always equal to led->fled_mask, regardless of the
> fled_id passed. Just use led->fled_mask directly.
> 

Uhm... I misunderstood it should be brighten separately.
But it supports just one channel to strobe or torch at once.

Right, It would be removed.
I will replace it.

>> +static int rt5033_led_iout_set_off(struct rt5033_led *led,
>> +                   enum rt5033_fled fled_id)
>> +{
>> +    int ret;
>> +    u8 fled_bit, value;
>> +
>> +    fled_bit = rt5033_fled_iout_get(led, fled_id);
>> +    led->current_iout ^= fled_bit;
>> +    led->sub_leds[fled_id].fled_cdev.led_cdev.brightness = 0;
>> +
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>> +                 RT5033_FLED_FUNC1_MASK,
>> +                 RT5033_FLED_PINCTRL | led->current_iout);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    if (led->current_iout) {
>> +        value = led->sub_leds[!fled_id].fled_cdev.led_cdev.brightness;
>> +        ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
>> +                     RT5033_FLED_CTRL1_MASK,
>> +                     (value - 1) << 4);
>> +    }
> 
> I infer that the purpose of this function is to turn the LED off.
> Could you explain why two commands are needed for this?
> 

The purpose of this function is to turn the LED off as you mentioned.
It should be removed.

>> +    return ret;
>> +}
>> +
>> +static int rt5033_led_brightness_set(struct led_classdev *led_cdev,
>> +                     enum led_brightness brightness)
>> +{
>> +    struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>> +    int fled_id = sub_led->fled_id, ret;
>> +    u8 fled_bit;
>> +
>> +    mutex_lock(&led->lock);
>> +
>> +    if (!brightness) {
>> +        ret = rt5033_led_iout_set_off(led, fled_id);
>> +        goto torch_unlock;
>> +    }
>> +
>> +    fled_bit = rt5033_fled_iout_get(led, fled_id);
>> +    led->current_iout |= fled_bit;
> 
> You don't need fled_bit, just assign the result to led->current_iout.
> 

Okay, I'll do

>> +
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
>> +                 RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
>> +    if (ret < 0)
>> +        goto torch_unlock;
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>> +                 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
>> +                 led->current_iout);
>> +    if (ret < 0)
>> +        goto torch_unlock;
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>> +                 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED);
> 
> Could you shortly describe the function of above three registers?
> 

RT5033_REG_FLED_CTRL1 : Control torch brightness. It can be set 12.5mA to 200mA.
RT5033_REG_FLED_FUNCTION1 : Set the operation mode (Torch / Strobe) and
			    determine which one gets control of LED between the two. (Pin or Register Bit)
RT5033_REG_FLED_FUNCTION2 : Set on/off flash strobe(one shot) and enable iout (<0>, <1>).

>> +torch_unlock:
>> +    mutex_unlock(&led->lock);
>> +    return ret;
>> +}
>> +
>> +static int rt5033_led_flash_brightness_set(struct led_classdev_flash *fled_cdev,
>> +                       u32 brightness)
>> +{
>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>> +    u32 flash_brightness_offset;
>> +
>> +    mutex_lock(&led->lock);
>> +
>> +    flash_brightness_offset =
>> +        RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.brightness.val,
>> +                         sub_led->fled_cdev.brightness.step,
>> +                         sub_led->fled_cdev.brightness.min);
> 
> Please align two above lines to the opening parenthesis.
> 

It will be over 80 characters when it aligns to the opening parenthesis.
So, I would fix these set as below if it is more better than before.
	flash_brightness_offset = RT5033_LED_CONFIG_DATA_OFFSET(
					sub_led->fled_cdev.brightness.val,
					sub_led->fled_cdev.brightness.step,
					sub_led->fled_cdev.brightness.min);

>> +    sub_led->flash_brightness = flash_brightness_offset;
> 
> flash_brightness_offset variable is redundant.
> 

The purpose of flash_brightness is that it saves offset of flash brightness to use easily.
To improve readability, It would be better to change name of variable
or to re-use RT5033_LED_CONFIG_DATA_OFFSET when it is needed.

>> +
>> +    mutex_unlock(&led->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int rt5033_led_flash_timeout_set(struct led_classdev_flash *fled_cdev,
>> +                    u32 timeout)
>> +{
>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>> +    u32 flash_tm_offset;
>> +
>> +    mutex_lock(&led->lock);
>> +
>> +    flash_tm_offset =
>> +        RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.timeout.val,
>> +                          sub_led->fled_cdev.timeout.step,
>> +                          sub_led->fled_cdev.timeout.min);
>> +    sub_led->flash_timeout = flash_tm_offset;
> 
> flash_tm_offset variable is redundant.
> 

ditto

>> +    mutex_unlock(&led->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int rt5033_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
>> +                       bool state)
>> +{
>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>> +    enum rt5033_fled fled_id = sub_led->fled_id;
>> +    int ret;
>> +    u8 fled_bit;
>> +
>> +    mutex_lock(&led->lock);
>> +
>> +    fled_bit = rt5033_fled_iout_get(led, fled_id);
>> +    led->current_iout |= fled_bit;
>> +
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>> +                 RT5033_FLED_FUNC2_MASK, 0);
>> +    if (ret < 0)
>> +        goto strobe_unlock;
>> +
>> +    if (!state) {
>> +        ret = rt5033_led_iout_set_off(led, fled_id);
>> +        goto strobe_unlock;
>> +    }
>> +
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL1,
>> +                 RT5033_FLED_STRBCTRL1_MASK,
>> +                 sub_led->flash_brightness);
> 
> Does the device allow for setting flash brightness separately for
> each iout? Currently it looks like the setting is shared.
> 

No, It doesn't. 
This device can't allow for setting flash brightness simultaneously.
The setting is shared as you see.

So each LED should keep their settings.

>> +    if (ret < 0)
>> +        goto strobe_unlock;
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL2,
>> +                 RT5033_FLED_STRBCTRL2_MASK,
>> +                 sub_led->flash_timeout);
>> +    if (ret < 0)
>> +        goto strobe_unlock;
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>> +                 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
>> +                 RT5033_FLED_STRB_SEL | led->current_iout);
>> +    if (ret < 0)
>> +        goto strobe_unlock;
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>> +                 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED |
>> +                 RT5033_FLED_SREG_STRB);
>> +
>> +    led->current_iout = 0;
>> +
>> +strobe_unlock:
>> +    mutex_unlock(&led->lock);
>> +    return ret;
>> +}
>> +
>> +static const struct led_flash_ops flash_ops = {
>> +    .flash_brightness_set = rt5033_led_flash_brightness_set,
>> +    .strobe_set = rt5033_led_flash_strobe_set,
>> +    .timeout_set = rt5033_led_flash_timeout_set,
>> +};
>> +
>> +static void rt5033_init_flash_properties(struct rt5033_sub_led *sub_led,
>> +                     struct rt5033_led_config_data *led_cfg)
>> +{
>> +    struct led_classdev_flash *fled_cdev = &sub_led->fled_cdev;
>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>> +    struct led_flash_setting *tm_set, *fl_set;
>> +    enum rt5033_fled fled_id = sub_led->fled_id;
>> +
>> +    tm_set = &fled_cdev->timeout;
>> +    tm_set->min = RT5033_LED_FLASH_TIMEOUT_MIN;
>> +    tm_set->max = led_cfg->flash_max_timeout[fled_id];
>> +    tm_set->step = RT5033_LED_FLASH_TIMEOUT_STEP;
>> +    tm_set->val = tm_set->max;
>> +
>> +    fl_set = &fled_cdev->brightness;
>> +    fl_set->min = RT5033_LED_FLASH_BRIGHTNESS_MIN;
>> +    if (led->iout_joint)
>> +        fl_set->max = min(led_cfg->flash_max_microamp[FLED1] +
>> +                  led_cfg->flash_max_microamp[FLED2],
>> +                  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH);
>> +    else
>> +        fl_set->max = min(led_cfg->flash_max_microamp[fled_id],
>> +                  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH);
>> +    fl_set->step = RT5033_LED_FLASH_BRIGHTNESS_STEP;
>> +    fl_set->val = fl_set->max;
>> +}
>> +
>> +static void rt5033_led_init_fled_cdev(struct rt5033_sub_led *sub_led,
>> +                      struct rt5033_led_config_data *led_cfg)
>> +{
>> +    struct led_classdev_flash *fled_cdev;
>> +    struct led_classdev *led_cdev;
>> +    enum rt5033_fled fled_id = sub_led->fled_id;
>> +    u32 flash_tm_offset, flash_brightness_offset;
>> +
>> +    /* Initialize LED Flash class device */
>> +    fled_cdev = &sub_led->fled_cdev;
>> +    fled_cdev->ops = &flash_ops;
>> +    led_cdev = &fled_cdev->led_cdev;
>> +
>> +    led_cdev->name = led_cfg->label[fled_id];
>> +
>> +    led_cdev->brightness_set_blocking = rt5033_led_brightness_set;
>> +    led_cdev->max_brightness = RT5033_LED_CONFIG_DATA_OFFSET(
>> +                   led_cfg->torch_max_microamp[fled_id],
>> +                   RT5033_LED_TORCH_BRIGHTNESS_STEP,
>> +                   RT5033_LED_TORCH_BRIGHTNESS_MIN);
>> +    led_cdev->flags |= LED_DEV_CAP_FLASH;
>> +
>> +    rt5033_init_flash_properties(sub_led, led_cfg);
>> +
>> +    flash_tm_offset = RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->timeout.val,
>> +                            fled_cdev->timeout.step,
>> +                            fled_cdev->timeout.min);
>> +    flash_brightness_offset =
>> +        RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->brightness.val,
>> +                          fled_cdev->brightness.step,
>> +                          fled_cdev->brightness.min);
>> +
>> +    sub_led->flash_timeout = flash_tm_offset;
>> +    sub_led->flash_brightness = flash_brightness_offset;
> 
> Same here - there is no need for flash_tm_offset and
> flash_brightness_offset variables.
> 

Ditto.


>> +}
>> +
>> +static int rt5033_led_parse_dt(struct rt5033_led *led, struct device *dev,
>> +                   struct rt5033_led_config_data *cfg,
>> +                   struct device_node **sub_nodes)
>> +{
>> +    struct device_node *np = dev->of_node;
>> +    struct device_node *child_node;
>> +    struct rt5033_sub_led *sub_leds = led->sub_leds;
>> +    struct property *prop;
>> +    u32 led_sources[2];
>> +    enum rt5033_fled fled_id;
>> +    int i, ret;
>> +
>> +    for_each_available_child_of_node(np, child_node) {
>> +        prop = of_find_property(child_node, "led-sources", NULL);
>> +        if (prop) {
>> +            const __be32 *srcs = NULL;
>> +
>> +            for (i = 0; i < ARRAY_SIZE(led_sources); ++i) {
>> +                srcs = of_prop_next_u32(prop, srcs,
>> +                            &led_sources[i]);
>> +                if (!srcs)
>> +                    break;
>> +            }
>> +        } else {
>> +            dev_err(dev, "led-sources DT property missing\n");
>> +            ret = -EINVAL;
>> +            goto err_parse_dt;
>> +        }
>> +
>> +        if (i == 2) {
>> +            fled_id = FLED1;
>> +            led->fled_mask = FLED1_IOUT | FLED2_IOUT;
>> +        } else if (led_sources[0] == FLED1) {
>> +            fled_id = FLED1;
>> +            led->fled_mask |= FLED1_IOUT;
>> +        } else if (led_sources[0] == FLED2) {
>> +            fled_id = FLED2;
>> +            led->fled_mask |= FLED2_IOUT;
>> +        } else {
>> +            dev_err(dev, "Wrong led-sources DT property value.\n");
>> +            ret = -EINVAL;
>> +            goto err_parse_dt;
>> +        }
>> +
>> +        if (sub_nodes[fled_id]) {
>> +            dev_err(dev,
>> +                "Conflicting \"led-sources\" DT properties\n");
>> +            ret = -EINVAL;
>> +            goto err_parse_dt;
>> +        }
>> +
>> +        sub_nodes[fled_id] = child_node;
>> +        sub_leds[fled_id].fled_id = fled_id;
>> +
>> +        cfg->label[fled_id] =
>> +            of_get_property(child_node, "label", NULL) ? :
>> +                    child_node->name;
>> +
>> +        ret = of_property_read_u32(child_node, "led-max-microamp",
>> +                       &cfg->torch_max_microamp[fled_id]);
>> +        if (ret < 0) {
>> +            dev_err(dev, "failed to parse led-max-microamp\n");
>> +            goto err_parse_dt;
>> +        }
>> +
>> +        ret = of_property_read_u32(child_node, "flash-max-microamp",
>> +                       &cfg->flash_max_microamp[fled_id]);
>> +        if (ret < 0) {
>> +            dev_err(dev, "failed to parse flash-max-microamp\n");
>> +            goto err_parse_dt;
>> +        }
>> +
>> +        ret = of_property_read_u32(child_node, "flash-max-timeout-us",
>> +                       &cfg->flash_max_timeout[fled_id]);
>> +        if (ret < 0) {
>> +            dev_err(dev, "failed to parse flash-max-timeout-us\n");
>> +            goto err_parse_dt;
>> +        }
>> +
>> +        if (++cfg->num_leds == 2 ||
>> +            (rt5033_fled_used(led, FLED1) &&
>> +             rt5033_fled_used(led, FLED2))) {
>> +            of_node_put(child_node);
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (cfg->num_leds == 0) {
>> +        dev_err(dev, "No DT child node found for connected LED(s).\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +
>> +err_parse_dt:
>> +    of_node_put(child_node);
>> +    return ret;
>> +}
>> +
>> +static int rt5033_led_probe(struct platform_device *pdev)
>> +{
>> +    struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent);
>> +    struct rt5033_led *led;
>> +    struct rt5033_sub_led *sub_leds;
>> +    struct device_node *sub_nodes[2] = {};
>> +    struct rt5033_led_config_data led_cfg = {};
>> +    int init_fled_cdev[2], i, ret;
>> +
>> +    led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
>> +    if (!led)
>> +        return -ENOMEM;
>> +
>> +    platform_set_drvdata(pdev, led);
>> +    led->dev = &pdev->dev;
>> +    led->regmap = rt5033->regmap;
>> +    sub_leds = led->sub_leds;
>> +
>> +    ret = rt5033_led_parse_dt(led, &pdev->dev, &led_cfg, sub_nodes);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    if (led_cfg.num_leds == 1 && rt5033_fled_used(led, FLED1) &&
>> +        rt5033_fled_used(led, FLED2))
>> +        led->iout_joint = true;
>> +
>> +    mutex_init(&led->lock);
>> +
>> +    init_fled_cdev[FLED1] =
>> +            led->iout_joint || rt5033_fled_used(led, FLED1);
>> +    init_fled_cdev[FLED2] =
>> +            !led->iout_joint && rt5033_fled_used(led, FLED2);
>> +
>> +    for (i = FLED1; i <= FLED2; ++i) {
>> +        if (!init_fled_cdev[i])
>> +            continue;
>> +
>> +        rt5033_led_init_fled_cdev(&sub_leds[i], &led_cfg);
>> +        ret = led_classdev_flash_register(led->dev,
>> +                          &sub_leds[i].fled_cdev);
>> +        if (ret < 0) {
>> +            if (i == FLED2)
>> +                goto err_register_led2;
>> +            else
>> +                goto err_register_led1;
>> +        }
>> +    }
>> +
>> +    led->current_iout = 0;
>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>> +                 RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET);
>> +    if (ret < 0)
>> +        dev_dbg(led->dev, "Failed to reset flash led (%d)\n", ret);
>> +
>> +    return 0;
>> +
>> +err_register_led2:
>> +    /* It is possible than only FLED2 was to be registered */
>> +    if (!init_fled_cdev[FLED1])
>> +        goto err_register_led1;
>> +    led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
>> +err_register_led1:
>> +    mutex_destroy(&led->lock);
>> +
>> +    return ret;
>> +}
>> +
>> +static int rt5033_led_remove(struct platform_device *pdev)
>> +{
>> +    struct rt5033_led *led = platform_get_drvdata(pdev);
>> +    struct rt5033_sub_led *sub_leds = led->sub_leds;
>> +
>> +    if (led->iout_joint || rt5033_fled_used(led, FLED1))
>> +        led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
>> +
>> +    if (!led->iout_joint && rt5033_fled_used(led, FLED2))
>> +        led_classdev_flash_unregister(&sub_leds[FLED2].fled_cdev);
>> +
>> +    mutex_destroy(&led->lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id rt5033_led_match[] = {
>> +    { .compatible = "richtek,rt5033-led", },
>> +    { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, rt5033_led_match);
>> +
>> +static struct platform_driver rt5033_led_driver = {
>> +    .driver = {
>> +        .name = "rt5033-led",
>> +        .of_match_table = rt5033_led_match,
>> +    },
>> +    .probe        = rt5033_led_probe,
>> +    .remove        = rt5033_led_remove,
>> +};
>> +module_platform_driver(rt5033_led_driver);
>> +
>> +MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>");
>> +MODULE_DESCRIPTION("Richtek RT5033 LED driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:rt5033-led");
>> diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
>> index 1b63fc2..b20c7e4 100644
>> --- a/include/linux/mfd/rt5033-private.h
>> +++ b/include/linux/mfd/rt5033-private.h
>> @@ -93,6 +93,57 @@ enum rt5033_reg {
>>   #define RT5033_RT_CTRL1_UUG_MASK    0x02
>>   #define RT5033_RT_HZ_MASK        0x01
>>
>> +/* RT5033 FLED Function1 register */
>> +#define RT5033_FLED_FUNC1_MASK        0x97
>> +#define RT5033_FLED_EN_LEDCS1        0x01
>> +#define RT5033_FLED_EN_LEDCS2        0x02
>> +#define RT5033_FLED_STRB_SEL        0x04
>> +#define RT5033_FLED_PINCTRL        0x10
>> +#define RT5033_FLED_RESET        0x80
>> +
>> +/* RT5033 FLED Function2 register */
>> +#define RT5033_FLED_FUNC2_MASK        0x81
>> +#define RT5033_FLED_ENFLED        0x01
>> +#define RT5033_FLED_SREG_STRB        0x80
>> +
>> +/* RT5033 FLED Strobe Control1 */
>> +#define RT5033_FLED_STRBCTRL1_MASK        0xFF
>> +#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX    0xE0
>> +#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF    0x0D
>> +#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX    0x1F
>> +
>> +/* RT5033 FLED Strobe Control2 */
>> +#define RT5033_FLED_STRBCTRL2_MASK    0x3F
>> +#define RT5033_FLED_STRBCTRL2_TM_DEF    0x0F
>> +#define RT5033_FLED_STRBCTRL2_TM_MAX    0x3F
>> +
>> +/* RT5033 FLED Control1 */
>> +#define RT5033_FLED_CTRL1_MASK        0xF7
>> +#define RT5033_FLED_CTRL1_TORCH_CUR_DEF    0x20
>> +#define RT5033_FLED_CTRL1_TORCH_CUR_MAX    0xF0
>> +#define RT5033_FLED_CTRL1_LBP_DEF    0x02
>> +#define RT5033_FLED_CTRL1_LBP_MAX    0x07
>> +
>> +/* RT5033 FLED Control2 */
>> +#define RT5033_FLED_CTRL2_MASK        0xFF
>> +#define RT5033_FLED_CTRL2_VMID_DEF    0x37
>> +#define RT5033_FLED_CTRL2_VMID_MAX    0x3F
>> +#define RT5033_FLED_CTRL2_TRACK_ALIVE    0x40
>> +#define RT5033_FLED_CTRL2_MID_TRACK    0x80
>> +
>> +/* RT5033 FLED Control4 */
>> +#define RT5033_FLED_CTRL4_MASK        0xE0
>> +#define RT5033_FLED_CTRL4_RESV        0x14
>> +#define RT5033_FLED_CTRL4_VTRREG_DEF    0x40
>> +#define RT5033_FLED_CTRL4_VTRREG_MAX    0x60
>> +#define RT5033_FLED_CTRL4_TRACK_CLK    0x80
>> +
>> +/* RT5033 FLED Control5 */
>> +#define RT5033_FLED_CTRL5_MASK        0xC0
>> +#define RT5033_FLED_CTRL5_RESV        0x10
>> +#define RT5033_FLED_CTRL5_TA_GOOD    0x40
>> +#define RT5033_FLED_CTRL5_TA_EXIST    0x80
>> +
>>   /* RT5033 control register */
>>   #define RT5033_CTRL_FCCM_BUCK_MASK        0x00
>>   #define RT5033_CTRL_BUCKOMS_MASK        0x01
>>
> 
> 

Re: [PATCH v5 2/2] leds: rt5033: Add RT5033 Flash led device driver

[Jacek Anaszewski]

Hi Ingi,

On 11/19/2015 11:07 AM, Ingi Kim wrote:
> Hi Jacek,
>
> Thanks for the review.

You're welcome.

> I'm trying to reply in detail what you mentioned.
> Please contact me If you need more detail to understand what I replied.
>
> On 2015년 11월 19일 00:46, Jacek Anaszewski wrote:
>> Hi Ingi,
>>
>> Thanks for the update. I have few comments below.
>>
>> On 11/17/2015 10:04 AM, Ingi Kim wrote:
>>> This patch adds device driver of Richtek RT5033 PMIC.
>>> The RT5033 Flash LED Circuit is designed for one or two LEDs driving
>>> for torch and strobe applications, it provides an I2C software command
>>> to trigger the torch and strobe operation.
>>>
>>> Signed-off-by: Ingi Kim <ingi2.kim@samsung.com>
>>> ---
>>>    drivers/leds/Kconfig               |   8 +
>>>    drivers/leds/Makefile              |   1 +
>>>    drivers/leds/leds-rt5033.c         | 522 +++++++++++++++++++++++++++++++++++++
>>>    include/linux/mfd/rt5033-private.h |  51 ++++
>>>    4 files changed, 582 insertions(+)
>>>    create mode 100644 drivers/leds/leds-rt5033.c
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index b1ab8bd..f41ac9b 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -345,6 +345,14 @@ config LEDS_PCA963X
>>>          LED driver chip accessed via the I2C bus. Supported
>>>          devices include PCA9633 and PCA9634
>>>
>>> +config LEDS_RT5033
>>> +    tristate "LED support for RT5033 PMIC"
>>> +    depends on LEDS_CLASS_FLASH && OF
>>> +    depends on MFD_RT5033
>>> +    help
>>> +      This option enables support for on-chip LED driver on
>>> +      RT5033 PMIC.
>>> +
>>>    config LEDS_WM831X_STATUS
>>>        tristate "LED support for status LEDs on WM831x PMICs"
>>>        depends on LEDS_CLASS
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index e9d53092..77cfad5 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -23,6 +23,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE)        += leds-cobalt-qube.o
>>>    obj-$(CONFIG_LEDS_COBALT_RAQ)        += leds-cobalt-raq.o
>>>    obj-$(CONFIG_LEDS_SUNFIRE)        += leds-sunfire.o
>>>    obj-$(CONFIG_LEDS_PCA9532)        += leds-pca9532.o
>>> +obj-$(CONFIG_LEDS_RT5033)        += leds-rt5033.o
>>>    obj-$(CONFIG_LEDS_GPIO_REGISTER)    += leds-gpio-register.o
>>>    obj-$(CONFIG_LEDS_GPIO)            += leds-gpio.o
>>>    obj-$(CONFIG_LEDS_LP3944)        += leds-lp3944.o
>>> diff --git a/drivers/leds/leds-rt5033.c b/drivers/leds/leds-rt5033.c
>>> new file mode 100644
>>> index 0000000..a28554a
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-rt5033.c
>>> @@ -0,0 +1,522 @@
>>> +/*
>>> + * led driver for RT5033
>>> + *
>>> + * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
>>> + * Ingi Kim <ingi2.kim@samsung.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + */
>>> +
>>> +#include <linux/led-class-flash.h>
>>> +#include <linux/mfd/rt5033.h>
>>> +#include <linux/mfd/rt5033-private.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#define RT5033_LED_FLASH_TIMEOUT_MIN        64000
>>> +#define RT5033_LED_FLASH_TIMEOUT_STEP        32000
>>> +#define RT5033_LED_FLASH_BRIGHTNESS_MIN        50000
>>> +#define RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH    600000
>>> +#define RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH    800000
>>> +#define RT5033_LED_FLASH_BRIGHTNESS_STEP    25000
>>> +#define RT5033_LED_TORCH_BRIGHTNESS_MIN        12500
>>> +#define RT5033_LED_TORCH_BRIGHTNESS_STEP    12500
>>> +
>>> +/* Macro to get offset of rt5033_led_config_data */
>>> +#define RT5033_LED_CONFIG_DATA_OFFSET(val, step, min)    (((val) - (min)) \
>>> +                            / (step))

I would rename this macro to RT5033_LED_DATA_TO_REG.

>>> +#define FLED1_IOUT        (BIT(0))
>>> +#define FLED2_IOUT        (BIT(1))
>>> +
>>> +enum rt5033_fled {
>>> +    FLED1,
>>> +    FLED2,
>>> +};
>>> +
>>> +struct rt5033_sub_led {
>>> +    enum rt5033_fled fled_id;
>>> +    struct led_classdev_flash fled_cdev;
>>> +
>>> +    u32 torch_brightness;
>>> +    u32 flash_brightness;
>>> +    u32 flash_timeout;
>>> +};
>>> +
>>> +/* RT5033 Flash led platform data */
>>> +struct rt5033_led {
>>> +    struct device *dev;
>>> +    struct mutex lock;
>>> +    struct regmap *regmap;
>>> +    struct rt5033_sub_led sub_leds[2];
>>> +
>>> +    bool iout_joint;
>>> +    u8 fled_mask;
>>> +    u8 current_iout;
>>> +};
>>> +
>>> +struct rt5033_led_config_data {
>>> +    const char *label[2];
>>> +    u32 flash_max_microamp[2];
>>> +    u32 flash_max_timeout[2];
>>> +    u32 torch_max_microamp[2];
>>> +    u32 num_leds;
>>> +};
>>> +
>>> +static struct rt5033_sub_led *flcdev_to_sub_led(
>>> +                    struct led_classdev_flash *fled_cdev)
>>> +{
>>> +    return container_of(fled_cdev, struct rt5033_sub_led, fled_cdev);
>>> +}
>>> +
>>> +static struct rt5033_led *sub_led_to_led(struct rt5033_sub_led *sub_led)
>>> +{
>>> +    return container_of(sub_led, struct rt5033_led,
>>> +                sub_leds[sub_led->fled_id]);
>>> +}
>>> +
>>> +static bool rt5033_fled_used(struct rt5033_led *led, enum rt5033_fled fled_id)
>>> +{
>>> +    u8 fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
>>> +
>>> +    return led->fled_mask & fled_bit;
>>> +}
>>> +
>>> +static u8 rt5033_fled_iout_get(struct rt5033_led *led, enum rt5033_fled fled_id)
>>> +{

I've reexamined this function and come to conclusion that it is
indeed useful. Please rename it to rt5033_get_iout_to_set, though.

>>> +    u8 fled_bit;
>>> +
>>> +    if (led->iout_joint)
>>> +        fled_bit = led->fled_mask;

s/led->fled_mask/FLED1_IOUT | FLED2_IOUT/

This way it will be more meaningful.

>>> +    else
>>> +        fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
>>> +
>>> +    return fled_bit;
>>> +}
>>
>> What is the purpose of this function? It seems to return
>> a value that is always equal to led->fled_mask, regardless of the
>> fled_id passed. Just use led->fled_mask directly.
>>
>
> Uhm... I misunderstood it should be brighten separately.
> But it supports just one channel to strobe or torch at once.
>
> Right, It would be removed.
> I will replace it.

No, let's keep it.

>>> +static int rt5033_led_iout_set_off(struct rt5033_led *led,
>>> +                   enum rt5033_fled fled_id)

Please rename  to rt5033_led_iout_disable.

>>> +{
>>> +    int ret;
>>> +    u8 fled_bit, value;
>>> +
>>> +    fled_bit = rt5033_fled_iout_get(led, fled_id);
>>> +    led->current_iout ^= fled_bit;

Please change '^=' to '&='.
We're only disabling iouts in this function.

>>> +    led->sub_leds[fled_id].fled_cdev.led_cdev.brightness = 0;

LED core takes care of it, please remove above line.

>>> +
>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>>> +                 RT5033_FLED_FUNC1_MASK,
>>> +                 RT5033_FLED_PINCTRL | led->current_iout);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    if (led->current_iout) {
>>> +        value = led->sub_leds[!fled_id].fled_cdev.led_cdev.brightness;
>>> +        ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
>>> +                     RT5033_FLED_CTRL1_MASK,
>>> +                     (value - 1) << 4);
>>> +    }

Don't set the other iout here.

>> I infer that the purpose of this function is to turn the LED off.
>> Could you explain why two commands are needed for this?
>>
>
> The purpose of this function is to turn the LED off as you mentioned.
> It should be removed.

Let's keep it either.

>>> +    return ret;
>>> +}
>>> +
>>> +static int rt5033_led_brightness_set(struct led_classdev *led_cdev,
>>> +                     enum led_brightness brightness)
>>> +{
>>> +    struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
>>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>> +    int fled_id = sub_led->fled_id, ret;
>>> +    u8 fled_bit;
>>> +
>>> +    mutex_lock(&led->lock);
>>> +
>>> +    if (!brightness) {
>>> +        ret = rt5033_led_iout_set_off(led, fled_id);
>>> +        goto torch_unlock;
>>> +    }
>>> +
>>> +    fled_bit = rt5033_fled_iout_get(led, fled_id);
>>> +    led->current_iout |= fled_bit;

Now it should be bare assignment, without OR operation.

>> You don't need fled_bit, just assign the result to led->current_iout.
>>
>
> Okay, I'll do
>
>>> +
>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
>>> +                 RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
>>> +    if (ret < 0)
>>> +        goto torch_unlock;
>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>>> +                 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
>>> +                 led->current_iout);

I'd set it only if led->current_iout has been changed.

>>> +    if (ret < 0)
>>> +        goto torch_unlock;
>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>>> +                 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED);
>>
>> Could you shortly describe the function of above three registers?
>>
>
> RT5033_REG_FLED_CTRL1 : Control torch brightness. It can be set 12.5mA to 200mA.
> RT5033_REG_FLED_FUNCTION1 : Set the operation mode (Torch / Strobe) and
> 			    determine which one gets control of LED between the two. (Pin or Register Bit)
> RT5033_REG_FLED_FUNCTION2 : Set on/off flash strobe(one shot) and enable iout (<0>, <1>).

Thanks for the explanation.

>>> +torch_unlock:
>>> +    mutex_unlock(&led->lock);
>>> +    return ret;
>>> +}
>>> +
>>> +static int rt5033_led_flash_brightness_set(struct led_classdev_flash *fled_cdev,
>>> +                       u32 brightness)
>>> +{
>>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>> +    u32 flash_brightness_offset;
>>> +
>>> +    mutex_lock(&led->lock);
>>> +
>>> +    flash_brightness_offset =
>>> +        RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.brightness.val,
>>> +                         sub_led->fled_cdev.brightness.step,
>>> +                         sub_led->fled_cdev.brightness.min);
>>
>> Please align two above lines to the opening parenthesis.
>>
>
> It will be over 80 characters when it aligns to the opening parenthesis.
> So, I would fix these set as below if it is more better than before.
> 	flash_brightness_offset = RT5033_LED_CONFIG_DATA_OFFSET(
> 					sub_led->fled_cdev.brightness.val,
> 					sub_led->fled_cdev.brightness.step,
> 					sub_led->fled_cdev.brightness.min);

OK.

>
>>> +    sub_led->flash_brightness = flash_brightness_offset;
>>
>> flash_brightness_offset variable is redundant.
>>
>
> The purpose of flash_brightness is that it saves offset of flash brightness to use easily.
> To improve readability, It would be better to change name of variable
> or to re-use RT5033_LED_CONFIG_DATA_OFFSET when it is needed.

It is not an offset, is is just a brightness level to be set.
Please assign macro result directly to sub_led->flash_brightness.

>>> +
>>> +    mutex_unlock(&led->lock);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int rt5033_led_flash_timeout_set(struct led_classdev_flash *fled_cdev,
>>> +                    u32 timeout)
>>> +{
>>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>> +    u32 flash_tm_offset;
>>> +
>>> +    mutex_lock(&led->lock);
>>> +
>>> +    flash_tm_offset =
>>> +        RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.timeout.val,
>>> +                          sub_led->fled_cdev.timeout.step,
>>> +                          sub_led->fled_cdev.timeout.min);
>>> +    sub_led->flash_timeout = flash_tm_offset;
>>
>> flash_tm_offset variable is redundant.
>>
>
> ditto

This is flash timeout level, not an offset.

>>> +    mutex_unlock(&led->lock);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int rt5033_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
>>> +                       bool state)
>>> +{
>>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>> +    enum rt5033_fled fled_id = sub_led->fled_id;
>>> +    int ret;
>>> +    u8 fled_bit;
>>> +
>>> +    mutex_lock(&led->lock);
>>> +
>>> +    fled_bit = rt5033_fled_iout_get(led, fled_id);
>>> +    led->current_iout |= fled_bit;

Now it should be bare assignment, without OR operation.

>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>>> +                 RT5033_FLED_FUNC2_MASK, 0);
>>> +    if (ret < 0)
>>> +        goto strobe_unlock;
>>> +
>>> +    if (!state) {
>>> +        ret = rt5033_led_iout_set_off(led, fled_id);
>>> +        goto strobe_unlock;
>>> +    }
>>> +
>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL1,
>>> +                 RT5033_FLED_STRBCTRL1_MASK,
>>> +                 sub_led->flash_brightness);

Couldn't this be set in the rt5033_led_flash_brightness_set,
and here only if the register state has been modified by the
other LED?

>> Does the device allow for setting flash brightness separately for
>> each iout? Currently it looks like the setting is shared.
>>
>
> No, It doesn't.
> This device can't allow for setting flash brightness simultaneously.
> The setting is shared as you see.

This should be mentioned in the commit message.

> So each LED should keep their settings.
>
>>> +    if (ret < 0)
>>> +        goto strobe_unlock;
>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL2,
>>> +                 RT5033_FLED_STRBCTRL2_MASK,
>>> +                 sub_led->flash_timeout);

Couldn't this be set in the rt5033_led_flash_timeout_set,
and here only if the register state has been modified by the
other LED?

>>> +    if (ret < 0)
>>> +        goto strobe_unlock;
>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>>> +                 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
>>> +                 RT5033_FLED_STRB_SEL | led->current_iout);
>>> +    if (ret < 0)
>>> +        goto strobe_unlock;

In case state == 0, we should immediately jump here and set SREG_STRB
bit to 0. Currently you seem not to support stopping flash strobe.

>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>>> +                 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED |
>>> +                 RT5033_FLED_SREG_STRB);
>>> +
>>> +    led->current_iout = 0;
>>> +
>>> +strobe_unlock:
>>> +    mutex_unlock(&led->lock);
>>> +    return ret;
>>> +}
>>> +
>>> +static const struct led_flash_ops flash_ops = {
>>> +    .flash_brightness_set = rt5033_led_flash_brightness_set,
>>> +    .strobe_set = rt5033_led_flash_strobe_set,
>>> +    .timeout_set = rt5033_led_flash_timeout_set,
>>> +};
>>> +
>>> +static void rt5033_init_flash_properties(struct rt5033_sub_led *sub_led,
>>> +                     struct rt5033_led_config_data *led_cfg)
>>> +{
>>> +    struct led_classdev_flash *fled_cdev = &sub_led->fled_cdev;
>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>> +    struct led_flash_setting *tm_set, *fl_set;
>>> +    enum rt5033_fled fled_id = sub_led->fled_id;
>>> +
>>> +    tm_set = &fled_cdev->timeout;
>>> +    tm_set->min = RT5033_LED_FLASH_TIMEOUT_MIN;
>>> +    tm_set->max = led_cfg->flash_max_timeout[fled_id];
>>> +    tm_set->step = RT5033_LED_FLASH_TIMEOUT_STEP;
>>> +    tm_set->val = tm_set->max;
>>> +
>>> +    fl_set = &fled_cdev->brightness;
>>> +    fl_set->min = RT5033_LED_FLASH_BRIGHTNESS_MIN;
>>> +    if (led->iout_joint)
>>> +        fl_set->max = min(led_cfg->flash_max_microamp[FLED1] +
>>> +                  led_cfg->flash_max_microamp[FLED2],
>>> +                  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH);
>>> +    else
>>> +        fl_set->max = min(led_cfg->flash_max_microamp[fled_id],
>>> +                  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH);
>>> +    fl_set->step = RT5033_LED_FLASH_BRIGHTNESS_STEP;
>>> +    fl_set->val = fl_set->max;
>>> +}
>>> +
>>> +static void rt5033_led_init_fled_cdev(struct rt5033_sub_led *sub_led,
>>> +                      struct rt5033_led_config_data *led_cfg)
>>> +{
>>> +    struct led_classdev_flash *fled_cdev;
>>> +    struct led_classdev *led_cdev;
>>> +    enum rt5033_fled fled_id = sub_led->fled_id;
>>> +    u32 flash_tm_offset, flash_brightness_offset;
>>> +
>>> +    /* Initialize LED Flash class device */
>>> +    fled_cdev = &sub_led->fled_cdev;
>>> +    fled_cdev->ops = &flash_ops;
>>> +    led_cdev = &fled_cdev->led_cdev;
>>> +
>>> +    led_cdev->name = led_cfg->label[fled_id];
>>> +
>>> +    led_cdev->brightness_set_blocking = rt5033_led_brightness_set;
>>> +    led_cdev->max_brightness = RT5033_LED_CONFIG_DATA_OFFSET(
>>> +                   led_cfg->torch_max_microamp[fled_id],
>>> +                   RT5033_LED_TORCH_BRIGHTNESS_STEP,
>>> +                   RT5033_LED_TORCH_BRIGHTNESS_MIN);
>>> +    led_cdev->flags |= LED_DEV_CAP_FLASH;
>>> +
>>> +    rt5033_init_flash_properties(sub_led, led_cfg);
>>> +
>>> +    flash_tm_offset = RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->timeout.val,
>>> +                            fled_cdev->timeout.step,
>>> +                            fled_cdev->timeout.min);
>>> +    flash_brightness_offset =
>>> +        RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->brightness.val,
>>> +                          fled_cdev->brightness.step,
>>> +                          fled_cdev->brightness.min);
>>> +
>>> +    sub_led->flash_timeout = flash_tm_offset;
>>> +    sub_led->flash_brightness = flash_brightness_offset;
>>
>> Same here - there is no need for flash_tm_offset and
>> flash_brightness_offset variables.
>>
>
> Ditto.
>
>
>>> +}
>>> +
>>> +static int rt5033_led_parse_dt(struct rt5033_led *led, struct device *dev,
>>> +                   struct rt5033_led_config_data *cfg,
>>> +                   struct device_node **sub_nodes)
>>> +{
>>> +    struct device_node *np = dev->of_node;
>>> +    struct device_node *child_node;
>>> +    struct rt5033_sub_led *sub_leds = led->sub_leds;
>>> +    struct property *prop;
>>> +    u32 led_sources[2];
>>> +    enum rt5033_fled fled_id;
>>> +    int i, ret;
>>> +
>>> +    for_each_available_child_of_node(np, child_node) {
>>> +        prop = of_find_property(child_node, "led-sources", NULL);
>>> +        if (prop) {
>>> +            const __be32 *srcs = NULL;
>>> +
>>> +            for (i = 0; i < ARRAY_SIZE(led_sources); ++i) {
>>> +                srcs = of_prop_next_u32(prop, srcs,
>>> +                            &led_sources[i]);
>>> +                if (!srcs)
>>> +                    break;
>>> +            }
>>> +        } else {
>>> +            dev_err(dev, "led-sources DT property missing\n");
>>> +            ret = -EINVAL;
>>> +            goto err_parse_dt;
>>> +        }
>>> +
>>> +        if (i == 2) {
>>> +            fled_id = FLED1;
>>> +            led->fled_mask = FLED1_IOUT | FLED2_IOUT;
>>> +        } else if (led_sources[0] == FLED1) {
>>> +            fled_id = FLED1;
>>> +            led->fled_mask |= FLED1_IOUT;
>>> +        } else if (led_sources[0] == FLED2) {
>>> +            fled_id = FLED2;
>>> +            led->fled_mask |= FLED2_IOUT;
>>> +        } else {
>>> +            dev_err(dev, "Wrong led-sources DT property value.\n");
>>> +            ret = -EINVAL;
>>> +            goto err_parse_dt;
>>> +        }
>>> +
>>> +        if (sub_nodes[fled_id]) {
>>> +            dev_err(dev,
>>> +                "Conflicting \"led-sources\" DT properties\n");
>>> +            ret = -EINVAL;
>>> +            goto err_parse_dt;
>>> +        }
>>> +
>>> +        sub_nodes[fled_id] = child_node;
>>> +        sub_leds[fled_id].fled_id = fled_id;
>>> +
>>> +        cfg->label[fled_id] =
>>> +            of_get_property(child_node, "label", NULL) ? :
>>> +                    child_node->name;
>>> +
>>> +        ret = of_property_read_u32(child_node, "led-max-microamp",
>>> +                       &cfg->torch_max_microamp[fled_id]);
>>> +        if (ret < 0) {
>>> +            dev_err(dev, "failed to parse led-max-microamp\n");
>>> +            goto err_parse_dt;
>>> +        }
>>> +
>>> +        ret = of_property_read_u32(child_node, "flash-max-microamp",
>>> +                       &cfg->flash_max_microamp[fled_id]);
>>> +        if (ret < 0) {
>>> +            dev_err(dev, "failed to parse flash-max-microamp\n");
>>> +            goto err_parse_dt;
>>> +        }
>>> +
>>> +        ret = of_property_read_u32(child_node, "flash-max-timeout-us",
>>> +                       &cfg->flash_max_timeout[fled_id]);
>>> +        if (ret < 0) {
>>> +            dev_err(dev, "failed to parse flash-max-timeout-us\n");
>>> +            goto err_parse_dt;
>>> +        }
>>> +
>>> +        if (++cfg->num_leds == 2 ||
>>> +            (rt5033_fled_used(led, FLED1) &&
>>> +             rt5033_fled_used(led, FLED2))) {
>>> +            of_node_put(child_node);
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    if (cfg->num_leds == 0) {
>>> +        dev_err(dev, "No DT child node found for connected LED(s).\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +err_parse_dt:
>>> +    of_node_put(child_node);
>>> +    return ret;
>>> +}
>>> +
>>> +static int rt5033_led_probe(struct platform_device *pdev)
>>> +{
>>> +    struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent);
>>> +    struct rt5033_led *led;
>>> +    struct rt5033_sub_led *sub_leds;
>>> +    struct device_node *sub_nodes[2] = {};
>>> +    struct rt5033_led_config_data led_cfg = {};
>>> +    int init_fled_cdev[2], i, ret;
>>> +
>>> +    led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
>>> +    if (!led)
>>> +        return -ENOMEM;
>>> +
>>> +    platform_set_drvdata(pdev, led);
>>> +    led->dev = &pdev->dev;
>>> +    led->regmap = rt5033->regmap;
>>> +    sub_leds = led->sub_leds;
>>> +
>>> +    ret = rt5033_led_parse_dt(led, &pdev->dev, &led_cfg, sub_nodes);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    if (led_cfg.num_leds == 1 && rt5033_fled_used(led, FLED1) &&
>>> +        rt5033_fled_used(led, FLED2))
>>> +        led->iout_joint = true;
>>> +
>>> +    mutex_init(&led->lock);
>>> +
>>> +    init_fled_cdev[FLED1] =
>>> +            led->iout_joint || rt5033_fled_used(led, FLED1);
>>> +    init_fled_cdev[FLED2] =
>>> +            !led->iout_joint && rt5033_fled_used(led, FLED2);
>>> +
>>> +    for (i = FLED1; i <= FLED2; ++i) {
>>> +        if (!init_fled_cdev[i])
>>> +            continue;
>>> +
>>> +        rt5033_led_init_fled_cdev(&sub_leds[i], &led_cfg);
>>> +        ret = led_classdev_flash_register(led->dev,
>>> +                          &sub_leds[i].fled_cdev);
>>> +        if (ret < 0) {
>>> +            if (i == FLED2)
>>> +                goto err_register_led2;
>>> +            else
>>> +                goto err_register_led1;
>>> +        }
>>> +    }
>>> +
>>> +    led->current_iout = 0;
>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>>> +                 RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET);
>>> +    if (ret < 0)
>>> +        dev_dbg(led->dev, "Failed to reset flash led (%d)\n", ret);
>>> +
>>> +    return 0;
>>> +
>>> +err_register_led2:
>>> +    /* It is possible than only FLED2 was to be registered */
>>> +    if (!init_fled_cdev[FLED1])
>>> +        goto err_register_led1;
>>> +    led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
>>> +err_register_led1:
>>> +    mutex_destroy(&led->lock);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int rt5033_led_remove(struct platform_device *pdev)
>>> +{
>>> +    struct rt5033_led *led = platform_get_drvdata(pdev);
>>> +    struct rt5033_sub_led *sub_leds = led->sub_leds;
>>> +
>>> +    if (led->iout_joint || rt5033_fled_used(led, FLED1))
>>> +        led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
>>> +
>>> +    if (!led->iout_joint && rt5033_fled_used(led, FLED2))
>>> +        led_classdev_flash_unregister(&sub_leds[FLED2].fled_cdev);
>>> +
>>> +    mutex_destroy(&led->lock);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct of_device_id rt5033_led_match[] = {
>>> +    { .compatible = "richtek,rt5033-led", },
>>> +    { /* sentinel */ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, rt5033_led_match);
>>> +
>>> +static struct platform_driver rt5033_led_driver = {
>>> +    .driver = {
>>> +        .name = "rt5033-led",
>>> +        .of_match_table = rt5033_led_match,
>>> +    },
>>> +    .probe        = rt5033_led_probe,
>>> +    .remove        = rt5033_led_remove,
>>> +};
>>> +module_platform_driver(rt5033_led_driver);
>>> +
>>> +MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>");
>>> +MODULE_DESCRIPTION("Richtek RT5033 LED driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_ALIAS("platform:rt5033-led");
>>> diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
>>> index 1b63fc2..b20c7e4 100644
>>> --- a/include/linux/mfd/rt5033-private.h
>>> +++ b/include/linux/mfd/rt5033-private.h
>>> @@ -93,6 +93,57 @@ enum rt5033_reg {
>>>    #define RT5033_RT_CTRL1_UUG_MASK    0x02
>>>    #define RT5033_RT_HZ_MASK        0x01
>>>
>>> +/* RT5033 FLED Function1 register */
>>> +#define RT5033_FLED_FUNC1_MASK        0x97
>>> +#define RT5033_FLED_EN_LEDCS1        0x01
>>> +#define RT5033_FLED_EN_LEDCS2        0x02
>>> +#define RT5033_FLED_STRB_SEL        0x04
>>> +#define RT5033_FLED_PINCTRL        0x10
>>> +#define RT5033_FLED_RESET        0x80
>>> +
>>> +/* RT5033 FLED Function2 register */
>>> +#define RT5033_FLED_FUNC2_MASK        0x81
>>> +#define RT5033_FLED_ENFLED        0x01
>>> +#define RT5033_FLED_SREG_STRB        0x80
>>> +
>>> +/* RT5033 FLED Strobe Control1 */
>>> +#define RT5033_FLED_STRBCTRL1_MASK        0xFF
>>> +#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX    0xE0
>>> +#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF    0x0D
>>> +#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX    0x1F
>>> +
>>> +/* RT5033 FLED Strobe Control2 */
>>> +#define RT5033_FLED_STRBCTRL2_MASK    0x3F
>>> +#define RT5033_FLED_STRBCTRL2_TM_DEF    0x0F
>>> +#define RT5033_FLED_STRBCTRL2_TM_MAX    0x3F
>>> +
>>> +/* RT5033 FLED Control1 */
>>> +#define RT5033_FLED_CTRL1_MASK        0xF7
>>> +#define RT5033_FLED_CTRL1_TORCH_CUR_DEF    0x20
>>> +#define RT5033_FLED_CTRL1_TORCH_CUR_MAX    0xF0
>>> +#define RT5033_FLED_CTRL1_LBP_DEF    0x02
>>> +#define RT5033_FLED_CTRL1_LBP_MAX    0x07
>>> +
>>> +/* RT5033 FLED Control2 */
>>> +#define RT5033_FLED_CTRL2_MASK        0xFF
>>> +#define RT5033_FLED_CTRL2_VMID_DEF    0x37
>>> +#define RT5033_FLED_CTRL2_VMID_MAX    0x3F
>>> +#define RT5033_FLED_CTRL2_TRACK_ALIVE    0x40
>>> +#define RT5033_FLED_CTRL2_MID_TRACK    0x80
>>> +
>>> +/* RT5033 FLED Control4 */
>>> +#define RT5033_FLED_CTRL4_MASK        0xE0
>>> +#define RT5033_FLED_CTRL4_RESV        0x14
>>> +#define RT5033_FLED_CTRL4_VTRREG_DEF    0x40
>>> +#define RT5033_FLED_CTRL4_VTRREG_MAX    0x60
>>> +#define RT5033_FLED_CTRL4_TRACK_CLK    0x80
>>> +
>>> +/* RT5033 FLED Control5 */
>>> +#define RT5033_FLED_CTRL5_MASK        0xC0
>>> +#define RT5033_FLED_CTRL5_RESV        0x10
>>> +#define RT5033_FLED_CTRL5_TA_GOOD    0x40
>>> +#define RT5033_FLED_CTRL5_TA_EXIST    0x80
>>> +
>>>    /* RT5033 control register */
>>>    #define RT5033_CTRL_FCCM_BUCK_MASK        0x00
>>>    #define RT5033_CTRL_BUCKOMS_MASK        0x01
>>>
>>
>>
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v5 2/2] leds: rt5033: Add RT5033 Flash led device driver

[Ingi Kim]

Hi Jacek,

Thanks for the review
I'm trying to fix all things what you mentioned.

I'll push next version soon.

On 2015년 11월 24일 21:16, Jacek Anaszewski wrote:
> Hi Ingi,
> 
> On 11/19/2015 11:07 AM, Ingi Kim wrote:
>> Hi Jacek,
>>
>> Thanks for the review.
> 
> You're welcome.
> 
>> I'm trying to reply in detail what you mentioned.
>> Please contact me If you need more detail to understand what I replied.
>>
>> On 2015년 11월 19일 00:46, Jacek Anaszewski wrote:
>>> Hi Ingi,
>>>
>>> Thanks for the update. I have few comments below.
>>>
>>> On 11/17/2015 10:04 AM, Ingi Kim wrote:
>>>> This patch adds device driver of Richtek RT5033 PMIC.
>>>> The RT5033 Flash LED Circuit is designed for one or two LEDs driving
>>>> for torch and strobe applications, it provides an I2C software command
>>>> to trigger the torch and strobe operation.
>>>>
>>>> Signed-off-by: Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>>> ---
>>>>    drivers/leds/Kconfig               |   8 +
>>>>    drivers/leds/Makefile              |   1 +
>>>>    drivers/leds/leds-rt5033.c         | 522 +++++++++++++++++++++++++++++++++++++
>>>>    include/linux/mfd/rt5033-private.h |  51 ++++
>>>>    4 files changed, 582 insertions(+)
>>>>    create mode 100644 drivers/leds/leds-rt5033.c
>>>>
>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>> index b1ab8bd..f41ac9b 100644
>>>> --- a/drivers/leds/Kconfig
>>>> +++ b/drivers/leds/Kconfig
>>>> @@ -345,6 +345,14 @@ config LEDS_PCA963X
>>>>          LED driver chip accessed via the I2C bus. Supported
>>>>          devices include PCA9633 and PCA9634
>>>>
>>>> +config LEDS_RT5033
>>>> +    tristate "LED support for RT5033 PMIC"
>>>> +    depends on LEDS_CLASS_FLASH && OF
>>>> +    depends on MFD_RT5033
>>>> +    help
>>>> +      This option enables support for on-chip LED driver on
>>>> +      RT5033 PMIC.
>>>> +
>>>>    config LEDS_WM831X_STATUS
>>>>        tristate "LED support for status LEDs on WM831x PMICs"
>>>>        depends on LEDS_CLASS
>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>>> index e9d53092..77cfad5 100644
>>>> --- a/drivers/leds/Makefile
>>>> +++ b/drivers/leds/Makefile
>>>> @@ -23,6 +23,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE)        += leds-cobalt-qube.o
>>>>    obj-$(CONFIG_LEDS_COBALT_RAQ)        += leds-cobalt-raq.o
>>>>    obj-$(CONFIG_LEDS_SUNFIRE)        += leds-sunfire.o
>>>>    obj-$(CONFIG_LEDS_PCA9532)        += leds-pca9532.o
>>>> +obj-$(CONFIG_LEDS_RT5033)        += leds-rt5033.o
>>>>    obj-$(CONFIG_LEDS_GPIO_REGISTER)    += leds-gpio-register.o
>>>>    obj-$(CONFIG_LEDS_GPIO)            += leds-gpio.o
>>>>    obj-$(CONFIG_LEDS_LP3944)        += leds-lp3944.o
>>>> diff --git a/drivers/leds/leds-rt5033.c b/drivers/leds/leds-rt5033.c
>>>> new file mode 100644
>>>> index 0000000..a28554a
>>>> --- /dev/null
>>>> +++ b/drivers/leds/leds-rt5033.c
>>>> @@ -0,0 +1,522 @@
>>>> +/*
>>>> + * led driver for RT5033
>>>> + *
>>>> + * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
>>>> + * Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/led-class-flash.h>
>>>> +#include <linux/mfd/rt5033.h>
>>>> +#include <linux/mfd/rt5033-private.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/platform_device.h>
>>>> +
>>>> +#define RT5033_LED_FLASH_TIMEOUT_MIN        64000
>>>> +#define RT5033_LED_FLASH_TIMEOUT_STEP        32000
>>>> +#define RT5033_LED_FLASH_BRIGHTNESS_MIN        50000
>>>> +#define RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH    600000
>>>> +#define RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH    800000
>>>> +#define RT5033_LED_FLASH_BRIGHTNESS_STEP    25000
>>>> +#define RT5033_LED_TORCH_BRIGHTNESS_MIN        12500
>>>> +#define RT5033_LED_TORCH_BRIGHTNESS_STEP    12500
>>>> +
>>>> +/* Macro to get offset of rt5033_led_config_data */
>>>> +#define RT5033_LED_CONFIG_DATA_OFFSET(val, step, min)    (((val) - (min)) \
>>>> +                            / (step))
> 
> I would rename this macro to RT5033_LED_DATA_TO_REG.
> 
>>>> +#define FLED1_IOUT        (BIT(0))
>>>> +#define FLED2_IOUT        (BIT(1))
>>>> +
>>>> +enum rt5033_fled {
>>>> +    FLED1,
>>>> +    FLED2,
>>>> +};
>>>> +
>>>> +struct rt5033_sub_led {
>>>> +    enum rt5033_fled fled_id;
>>>> +    struct led_classdev_flash fled_cdev;
>>>> +
>>>> +    u32 torch_brightness;
>>>> +    u32 flash_brightness;
>>>> +    u32 flash_timeout;
>>>> +};
>>>> +
>>>> +/* RT5033 Flash led platform data */
>>>> +struct rt5033_led {
>>>> +    struct device *dev;
>>>> +    struct mutex lock;
>>>> +    struct regmap *regmap;
>>>> +    struct rt5033_sub_led sub_leds[2];
>>>> +
>>>> +    bool iout_joint;
>>>> +    u8 fled_mask;
>>>> +    u8 current_iout;
>>>> +};
>>>> +
>>>> +struct rt5033_led_config_data {
>>>> +    const char *label[2];
>>>> +    u32 flash_max_microamp[2];
>>>> +    u32 flash_max_timeout[2];
>>>> +    u32 torch_max_microamp[2];
>>>> +    u32 num_leds;
>>>> +};
>>>> +
>>>> +static struct rt5033_sub_led *flcdev_to_sub_led(
>>>> +                    struct led_classdev_flash *fled_cdev)
>>>> +{
>>>> +    return container_of(fled_cdev, struct rt5033_sub_led, fled_cdev);
>>>> +}
>>>> +
>>>> +static struct rt5033_led *sub_led_to_led(struct rt5033_sub_led *sub_led)
>>>> +{
>>>> +    return container_of(sub_led, struct rt5033_led,
>>>> +                sub_leds[sub_led->fled_id]);
>>>> +}
>>>> +
>>>> +static bool rt5033_fled_used(struct rt5033_led *led, enum rt5033_fled fled_id)
>>>> +{
>>>> +    u8 fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
>>>> +
>>>> +    return led->fled_mask & fled_bit;
>>>> +}
>>>> +
>>>> +static u8 rt5033_fled_iout_get(struct rt5033_led *led, enum rt5033_fled fled_id)
>>>> +{
> 
> I've reexamined this function and come to conclusion that it is
> indeed useful. Please rename it to rt5033_get_iout_to_set, though.
> 
>>>> +    u8 fled_bit;
>>>> +
>>>> +    if (led->iout_joint)
>>>> +        fled_bit = led->fled_mask;
> 
> s/led->fled_mask/FLED1_IOUT | FLED2_IOUT/
> 
> This way it will be more meaningful.
> 
>>>> +    else
>>>> +        fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
>>>> +
>>>> +    return fled_bit;
>>>> +}
>>>
>>> What is the purpose of this function? It seems to return
>>> a value that is always equal to led->fled_mask, regardless of the
>>> fled_id passed. Just use led->fled_mask directly.
>>>
>>
>> Uhm... I misunderstood it should be brighten separately.
>> But it supports just one channel to strobe or torch at once.
>>
>> Right, It would be removed.
>> I will replace it.
> 
> No, let's keep it.
> 
>>>> +static int rt5033_led_iout_set_off(struct rt5033_led *led,
>>>> +                   enum rt5033_fled fled_id)
> 
> Please rename  to rt5033_led_iout_disable.
> 
>>>> +{
>>>> +    int ret;
>>>> +    u8 fled_bit, value;
>>>> +
>>>> +    fled_bit = rt5033_fled_iout_get(led, fled_id);
>>>> +    led->current_iout ^= fled_bit;
> 
> Please change '^=' to '&='.
> We're only disabling iouts in this function.
> 
>>>> +    led->sub_leds[fled_id].fled_cdev.led_cdev.brightness = 0;
> 
> LED core takes care of it, please remove above line.
> 
>>>> +
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>>>> +                 RT5033_FLED_FUNC1_MASK,
>>>> +                 RT5033_FLED_PINCTRL | led->current_iout);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    if (led->current_iout) {
>>>> +        value = led->sub_leds[!fled_id].fled_cdev.led_cdev.brightness;
>>>> +        ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
>>>> +                     RT5033_FLED_CTRL1_MASK,
>>>> +                     (value - 1) << 4);
>>>> +    }
> 
> Don't set the other iout here.
> 
>>> I infer that the purpose of this function is to turn the LED off.
>>> Could you explain why two commands are needed for this?
>>>
>>
>> The purpose of this function is to turn the LED off as you mentioned.
>> It should be removed.
> 
> Let's keep it either.
> 
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int rt5033_led_brightness_set(struct led_classdev *led_cdev,
>>>> +                     enum led_brightness brightness)
>>>> +{
>>>> +    struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
>>>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>>> +    int fled_id = sub_led->fled_id, ret;
>>>> +    u8 fled_bit;
>>>> +
>>>> +    mutex_lock(&led->lock);
>>>> +
>>>> +    if (!brightness) {
>>>> +        ret = rt5033_led_iout_set_off(led, fled_id);
>>>> +        goto torch_unlock;
>>>> +    }
>>>> +
>>>> +    fled_bit = rt5033_fled_iout_get(led, fled_id);
>>>> +    led->current_iout |= fled_bit;
> 
> Now it should be bare assignment, without OR operation.
> 
>>> You don't need fled_bit, just assign the result to led->current_iout.
>>>
>>
>> Okay, I'll do
>>
>>>> +
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
>>>> +                 RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
>>>> +    if (ret < 0)
>>>> +        goto torch_unlock;
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>>>> +                 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
>>>> +                 led->current_iout);
> 
> I'd set it only if led->current_iout has been changed.
> 
>>>> +    if (ret < 0)
>>>> +        goto torch_unlock;
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>>>> +                 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED);
>>>
>>> Could you shortly describe the function of above three registers?
>>>
>>
>> RT5033_REG_FLED_CTRL1 : Control torch brightness. It can be set 12.5mA to 200mA.
>> RT5033_REG_FLED_FUNCTION1 : Set the operation mode (Torch / Strobe) and
>>                 determine which one gets control of LED between the two. (Pin or Register Bit)
>> RT5033_REG_FLED_FUNCTION2 : Set on/off flash strobe(one shot) and enable iout (<0>, <1>).
> 
> Thanks for the explanation.
> 
>>>> +torch_unlock:
>>>> +    mutex_unlock(&led->lock);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int rt5033_led_flash_brightness_set(struct led_classdev_flash *fled_cdev,
>>>> +                       u32 brightness)
>>>> +{
>>>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>>> +    u32 flash_brightness_offset;
>>>> +
>>>> +    mutex_lock(&led->lock);
>>>> +
>>>> +    flash_brightness_offset =
>>>> +        RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.brightness.val,
>>>> +                         sub_led->fled_cdev.brightness.step,
>>>> +                         sub_led->fled_cdev.brightness.min);
>>>
>>> Please align two above lines to the opening parenthesis.
>>>
>>
>> It will be over 80 characters when it aligns to the opening parenthesis.
>> So, I would fix these set as below if it is more better than before.
>>     flash_brightness_offset = RT5033_LED_CONFIG_DATA_OFFSET(
>>                     sub_led->fled_cdev.brightness.val,
>>                     sub_led->fled_cdev.brightness.step,
>>                     sub_led->fled_cdev.brightness.min);
> 
> OK.
> 
>>
>>>> +    sub_led->flash_brightness = flash_brightness_offset;
>>>
>>> flash_brightness_offset variable is redundant.
>>>
>>
>> The purpose of flash_brightness is that it saves offset of flash brightness to use easily.
>> To improve readability, It would be better to change name of variable
>> or to re-use RT5033_LED_CONFIG_DATA_OFFSET when it is needed.
> 
> It is not an offset, is is just a brightness level to be set.
> Please assign macro result directly to sub_led->flash_brightness.
> 
>>>> +
>>>> +    mutex_unlock(&led->lock);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int rt5033_led_flash_timeout_set(struct led_classdev_flash *fled_cdev,
>>>> +                    u32 timeout)
>>>> +{
>>>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>>> +    u32 flash_tm_offset;
>>>> +
>>>> +    mutex_lock(&led->lock);
>>>> +
>>>> +    flash_tm_offset =
>>>> +        RT5033_LED_CONFIG_DATA_OFFSET(sub_led->fled_cdev.timeout.val,
>>>> +                          sub_led->fled_cdev.timeout.step,
>>>> +                          sub_led->fled_cdev.timeout.min);
>>>> +    sub_led->flash_timeout = flash_tm_offset;
>>>
>>> flash_tm_offset variable is redundant.
>>>
>>
>> ditto
> 
> This is flash timeout level, not an offset.
> 
>>>> +    mutex_unlock(&led->lock);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int rt5033_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
>>>> +                       bool state)
>>>> +{
>>>> +    struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
>>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>>> +    enum rt5033_fled fled_id = sub_led->fled_id;
>>>> +    int ret;
>>>> +    u8 fled_bit;
>>>> +
>>>> +    mutex_lock(&led->lock);
>>>> +
>>>> +    fled_bit = rt5033_fled_iout_get(led, fled_id);
>>>> +    led->current_iout |= fled_bit;
> 
> Now it should be bare assignment, without OR operation.
> 
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>>>> +                 RT5033_FLED_FUNC2_MASK, 0);
>>>> +    if (ret < 0)
>>>> +        goto strobe_unlock;
>>>> +
>>>> +    if (!state) {
>>>> +        ret = rt5033_led_iout_set_off(led, fled_id);
>>>> +        goto strobe_unlock;
>>>> +    }
>>>> +
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL1,
>>>> +                 RT5033_FLED_STRBCTRL1_MASK,
>>>> +                 sub_led->flash_brightness);
> 
> Couldn't this be set in the rt5033_led_flash_brightness_set,
> and here only if the register state has been modified by the
> other LED?
> 
>>> Does the device allow for setting flash brightness separately for
>>> each iout? Currently it looks like the setting is shared.
>>>
>>
>> No, It doesn't.
>> This device can't allow for setting flash brightness simultaneously.
>> The setting is shared as you see.
> 
> This should be mentioned in the commit message.
> 
>> So each LED should keep their settings.
>>
>>>> +    if (ret < 0)
>>>> +        goto strobe_unlock;
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL2,
>>>> +                 RT5033_FLED_STRBCTRL2_MASK,
>>>> +                 sub_led->flash_timeout);
> 
> Couldn't this be set in the rt5033_led_flash_timeout_set,
> and here only if the register state has been modified by the
> other LED?
> 
>>>> +    if (ret < 0)
>>>> +        goto strobe_unlock;
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>>>> +                 RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL |
>>>> +                 RT5033_FLED_STRB_SEL | led->current_iout);
>>>> +    if (ret < 0)
>>>> +        goto strobe_unlock;
> 
> In case state == 0, we should immediately jump here and set SREG_STRB
> bit to 0. Currently you seem not to support stopping flash strobe.
> 
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>>>> +                 RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED |
>>>> +                 RT5033_FLED_SREG_STRB);
>>>> +
>>>> +    led->current_iout = 0;
>>>> +
>>>> +strobe_unlock:
>>>> +    mutex_unlock(&led->lock);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const struct led_flash_ops flash_ops = {
>>>> +    .flash_brightness_set = rt5033_led_flash_brightness_set,
>>>> +    .strobe_set = rt5033_led_flash_strobe_set,
>>>> +    .timeout_set = rt5033_led_flash_timeout_set,
>>>> +};
>>>> +
>>>> +static void rt5033_init_flash_properties(struct rt5033_sub_led *sub_led,
>>>> +                     struct rt5033_led_config_data *led_cfg)
>>>> +{
>>>> +    struct led_classdev_flash *fled_cdev = &sub_led->fled_cdev;
>>>> +    struct rt5033_led *led = sub_led_to_led(sub_led);
>>>> +    struct led_flash_setting *tm_set, *fl_set;
>>>> +    enum rt5033_fled fled_id = sub_led->fled_id;
>>>> +
>>>> +    tm_set = &fled_cdev->timeout;
>>>> +    tm_set->min = RT5033_LED_FLASH_TIMEOUT_MIN;
>>>> +    tm_set->max = led_cfg->flash_max_timeout[fled_id];
>>>> +    tm_set->step = RT5033_LED_FLASH_TIMEOUT_STEP;
>>>> +    tm_set->val = tm_set->max;
>>>> +
>>>> +    fl_set = &fled_cdev->brightness;
>>>> +    fl_set->min = RT5033_LED_FLASH_BRIGHTNESS_MIN;
>>>> +    if (led->iout_joint)
>>>> +        fl_set->max = min(led_cfg->flash_max_microamp[FLED1] +
>>>> +                  led_cfg->flash_max_microamp[FLED2],
>>>> +                  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_2CH);
>>>> +    else
>>>> +        fl_set->max = min(led_cfg->flash_max_microamp[fled_id],
>>>> +                  (u32)RT5033_LED_FLASH_BRIGHTNESS_MAX_1CH);
>>>> +    fl_set->step = RT5033_LED_FLASH_BRIGHTNESS_STEP;
>>>> +    fl_set->val = fl_set->max;
>>>> +}
>>>> +
>>>> +static void rt5033_led_init_fled_cdev(struct rt5033_sub_led *sub_led,
>>>> +                      struct rt5033_led_config_data *led_cfg)
>>>> +{
>>>> +    struct led_classdev_flash *fled_cdev;
>>>> +    struct led_classdev *led_cdev;
>>>> +    enum rt5033_fled fled_id = sub_led->fled_id;
>>>> +    u32 flash_tm_offset, flash_brightness_offset;
>>>> +
>>>> +    /* Initialize LED Flash class device */
>>>> +    fled_cdev = &sub_led->fled_cdev;
>>>> +    fled_cdev->ops = &flash_ops;
>>>> +    led_cdev = &fled_cdev->led_cdev;
>>>> +
>>>> +    led_cdev->name = led_cfg->label[fled_id];
>>>> +
>>>> +    led_cdev->brightness_set_blocking = rt5033_led_brightness_set;
>>>> +    led_cdev->max_brightness = RT5033_LED_CONFIG_DATA_OFFSET(
>>>> +                   led_cfg->torch_max_microamp[fled_id],
>>>> +                   RT5033_LED_TORCH_BRIGHTNESS_STEP,
>>>> +                   RT5033_LED_TORCH_BRIGHTNESS_MIN);
>>>> +    led_cdev->flags |= LED_DEV_CAP_FLASH;
>>>> +
>>>> +    rt5033_init_flash_properties(sub_led, led_cfg);
>>>> +
>>>> +    flash_tm_offset = RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->timeout.val,
>>>> +                            fled_cdev->timeout.step,
>>>> +                            fled_cdev->timeout.min);
>>>> +    flash_brightness_offset =
>>>> +        RT5033_LED_CONFIG_DATA_OFFSET(fled_cdev->brightness.val,
>>>> +                          fled_cdev->brightness.step,
>>>> +                          fled_cdev->brightness.min);
>>>> +
>>>> +    sub_led->flash_timeout = flash_tm_offset;
>>>> +    sub_led->flash_brightness = flash_brightness_offset;
>>>
>>> Same here - there is no need for flash_tm_offset and
>>> flash_brightness_offset variables.
>>>
>>
>> Ditto.
>>
>>
>>>> +}
>>>> +
>>>> +static int rt5033_led_parse_dt(struct rt5033_led *led, struct device *dev,
>>>> +                   struct rt5033_led_config_data *cfg,
>>>> +                   struct device_node **sub_nodes)
>>>> +{
>>>> +    struct device_node *np = dev->of_node;
>>>> +    struct device_node *child_node;
>>>> +    struct rt5033_sub_led *sub_leds = led->sub_leds;
>>>> +    struct property *prop;
>>>> +    u32 led_sources[2];
>>>> +    enum rt5033_fled fled_id;
>>>> +    int i, ret;
>>>> +
>>>> +    for_each_available_child_of_node(np, child_node) {
>>>> +        prop = of_find_property(child_node, "led-sources", NULL);
>>>> +        if (prop) {
>>>> +            const __be32 *srcs = NULL;
>>>> +
>>>> +            for (i = 0; i < ARRAY_SIZE(led_sources); ++i) {
>>>> +                srcs = of_prop_next_u32(prop, srcs,
>>>> +                            &led_sources[i]);
>>>> +                if (!srcs)
>>>> +                    break;
>>>> +            }
>>>> +        } else {
>>>> +            dev_err(dev, "led-sources DT property missing\n");
>>>> +            ret = -EINVAL;
>>>> +            goto err_parse_dt;
>>>> +        }
>>>> +
>>>> +        if (i == 2) {
>>>> +            fled_id = FLED1;
>>>> +            led->fled_mask = FLED1_IOUT | FLED2_IOUT;
>>>> +        } else if (led_sources[0] == FLED1) {
>>>> +            fled_id = FLED1;
>>>> +            led->fled_mask |= FLED1_IOUT;
>>>> +        } else if (led_sources[0] == FLED2) {
>>>> +            fled_id = FLED2;
>>>> +            led->fled_mask |= FLED2_IOUT;
>>>> +        } else {
>>>> +            dev_err(dev, "Wrong led-sources DT property value.\n");
>>>> +            ret = -EINVAL;
>>>> +            goto err_parse_dt;
>>>> +        }
>>>> +
>>>> +        if (sub_nodes[fled_id]) {
>>>> +            dev_err(dev,
>>>> +                "Conflicting \"led-sources\" DT properties\n");
>>>> +            ret = -EINVAL;
>>>> +            goto err_parse_dt;
>>>> +        }
>>>> +
>>>> +        sub_nodes[fled_id] = child_node;
>>>> +        sub_leds[fled_id].fled_id = fled_id;
>>>> +
>>>> +        cfg->label[fled_id] =
>>>> +            of_get_property(child_node, "label", NULL) ? :
>>>> +                    child_node->name;
>>>> +
>>>> +        ret = of_property_read_u32(child_node, "led-max-microamp",
>>>> +                       &cfg->torch_max_microamp[fled_id]);
>>>> +        if (ret < 0) {
>>>> +            dev_err(dev, "failed to parse led-max-microamp\n");
>>>> +            goto err_parse_dt;
>>>> +        }
>>>> +
>>>> +        ret = of_property_read_u32(child_node, "flash-max-microamp",
>>>> +                       &cfg->flash_max_microamp[fled_id]);
>>>> +        if (ret < 0) {
>>>> +            dev_err(dev, "failed to parse flash-max-microamp\n");
>>>> +            goto err_parse_dt;
>>>> +        }
>>>> +
>>>> +        ret = of_property_read_u32(child_node, "flash-max-timeout-us",
>>>> +                       &cfg->flash_max_timeout[fled_id]);
>>>> +        if (ret < 0) {
>>>> +            dev_err(dev, "failed to parse flash-max-timeout-us\n");
>>>> +            goto err_parse_dt;
>>>> +        }
>>>> +
>>>> +        if (++cfg->num_leds == 2 ||
>>>> +            (rt5033_fled_used(led, FLED1) &&
>>>> +             rt5033_fled_used(led, FLED2))) {
>>>> +            of_node_put(child_node);
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if (cfg->num_leds == 0) {
>>>> +        dev_err(dev, "No DT child node found for connected LED(s).\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err_parse_dt:
>>>> +    of_node_put(child_node);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int rt5033_led_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent);
>>>> +    struct rt5033_led *led;
>>>> +    struct rt5033_sub_led *sub_leds;
>>>> +    struct device_node *sub_nodes[2] = {};
>>>> +    struct rt5033_led_config_data led_cfg = {};
>>>> +    int init_fled_cdev[2], i, ret;
>>>> +
>>>> +    led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
>>>> +    if (!led)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    platform_set_drvdata(pdev, led);
>>>> +    led->dev = &pdev->dev;
>>>> +    led->regmap = rt5033->regmap;
>>>> +    sub_leds = led->sub_leds;
>>>> +
>>>> +    ret = rt5033_led_parse_dt(led, &pdev->dev, &led_cfg, sub_nodes);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    if (led_cfg.num_leds == 1 && rt5033_fled_used(led, FLED1) &&
>>>> +        rt5033_fled_used(led, FLED2))
>>>> +        led->iout_joint = true;
>>>> +
>>>> +    mutex_init(&led->lock);
>>>> +
>>>> +    init_fled_cdev[FLED1] =
>>>> +            led->iout_joint || rt5033_fled_used(led, FLED1);
>>>> +    init_fled_cdev[FLED2] =
>>>> +            !led->iout_joint && rt5033_fled_used(led, FLED2);
>>>> +
>>>> +    for (i = FLED1; i <= FLED2; ++i) {
>>>> +        if (!init_fled_cdev[i])
>>>> +            continue;
>>>> +
>>>> +        rt5033_led_init_fled_cdev(&sub_leds[i], &led_cfg);
>>>> +        ret = led_classdev_flash_register(led->dev,
>>>> +                          &sub_leds[i].fled_cdev);
>>>> +        if (ret < 0) {
>>>> +            if (i == FLED2)
>>>> +                goto err_register_led2;
>>>> +            else
>>>> +                goto err_register_led1;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    led->current_iout = 0;
>>>> +    ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>>>> +                 RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET);
>>>> +    if (ret < 0)
>>>> +        dev_dbg(led->dev, "Failed to reset flash led (%d)\n", ret);
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err_register_led2:
>>>> +    /* It is possible than only FLED2 was to be registered */
>>>> +    if (!init_fled_cdev[FLED1])
>>>> +        goto err_register_led1;
>>>> +    led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
>>>> +err_register_led1:
>>>> +    mutex_destroy(&led->lock);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int rt5033_led_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct rt5033_led *led = platform_get_drvdata(pdev);
>>>> +    struct rt5033_sub_led *sub_leds = led->sub_leds;
>>>> +
>>>> +    if (led->iout_joint || rt5033_fled_used(led, FLED1))
>>>> +        led_classdev_flash_unregister(&sub_leds[FLED1].fled_cdev);
>>>> +
>>>> +    if (!led->iout_joint && rt5033_fled_used(led, FLED2))
>>>> +        led_classdev_flash_unregister(&sub_leds[FLED2].fled_cdev);
>>>> +
>>>> +    mutex_destroy(&led->lock);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id rt5033_led_match[] = {
>>>> +    { .compatible = "richtek,rt5033-led", },
>>>> +    { /* sentinel */ },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, rt5033_led_match);
>>>> +
>>>> +static struct platform_driver rt5033_led_driver = {
>>>> +    .driver = {
>>>> +        .name = "rt5033-led",
>>>> +        .of_match_table = rt5033_led_match,
>>>> +    },
>>>> +    .probe        = rt5033_led_probe,
>>>> +    .remove        = rt5033_led_remove,
>>>> +};
>>>> +module_platform_driver(rt5033_led_driver);
>>>> +
>>>> +MODULE_AUTHOR("Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>");
>>>> +MODULE_DESCRIPTION("Richtek RT5033 LED driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> +MODULE_ALIAS("platform:rt5033-led");
>>>> diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
>>>> index 1b63fc2..b20c7e4 100644
>>>> --- a/include/linux/mfd/rt5033-private.h
>>>> +++ b/include/linux/mfd/rt5033-private.h
>>>> @@ -93,6 +93,57 @@ enum rt5033_reg {
>>>>    #define RT5033_RT_CTRL1_UUG_MASK    0x02
>>>>    #define RT5033_RT_HZ_MASK        0x01
>>>>
>>>> +/* RT5033 FLED Function1 register */
>>>> +#define RT5033_FLED_FUNC1_MASK        0x97
>>>> +#define RT5033_FLED_EN_LEDCS1        0x01
>>>> +#define RT5033_FLED_EN_LEDCS2        0x02
>>>> +#define RT5033_FLED_STRB_SEL        0x04
>>>> +#define RT5033_FLED_PINCTRL        0x10
>>>> +#define RT5033_FLED_RESET        0x80
>>>> +
>>>> +/* RT5033 FLED Function2 register */
>>>> +#define RT5033_FLED_FUNC2_MASK        0x81
>>>> +#define RT5033_FLED_ENFLED        0x01
>>>> +#define RT5033_FLED_SREG_STRB        0x80
>>>> +
>>>> +/* RT5033 FLED Strobe Control1 */
>>>> +#define RT5033_FLED_STRBCTRL1_MASK        0xFF
>>>> +#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX    0xE0
>>>> +#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF    0x0D
>>>> +#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX    0x1F
>>>> +
>>>> +/* RT5033 FLED Strobe Control2 */
>>>> +#define RT5033_FLED_STRBCTRL2_MASK    0x3F
>>>> +#define RT5033_FLED_STRBCTRL2_TM_DEF    0x0F
>>>> +#define RT5033_FLED_STRBCTRL2_TM_MAX    0x3F
>>>> +
>>>> +/* RT5033 FLED Control1 */
>>>> +#define RT5033_FLED_CTRL1_MASK        0xF7
>>>> +#define RT5033_FLED_CTRL1_TORCH_CUR_DEF    0x20
>>>> +#define RT5033_FLED_CTRL1_TORCH_CUR_MAX    0xF0
>>>> +#define RT5033_FLED_CTRL1_LBP_DEF    0x02
>>>> +#define RT5033_FLED_CTRL1_LBP_MAX    0x07
>>>> +
>>>> +/* RT5033 FLED Control2 */
>>>> +#define RT5033_FLED_CTRL2_MASK        0xFF
>>>> +#define RT5033_FLED_CTRL2_VMID_DEF    0x37
>>>> +#define RT5033_FLED_CTRL2_VMID_MAX    0x3F
>>>> +#define RT5033_FLED_CTRL2_TRACK_ALIVE    0x40
>>>> +#define RT5033_FLED_CTRL2_MID_TRACK    0x80
>>>> +
>>>> +/* RT5033 FLED Control4 */
>>>> +#define RT5033_FLED_CTRL4_MASK        0xE0
>>>> +#define RT5033_FLED_CTRL4_RESV        0x14
>>>> +#define RT5033_FLED_CTRL4_VTRREG_DEF    0x40
>>>> +#define RT5033_FLED_CTRL4_VTRREG_MAX    0x60
>>>> +#define RT5033_FLED_CTRL4_TRACK_CLK    0x80
>>>> +
>>>> +/* RT5033 FLED Control5 */
>>>> +#define RT5033_FLED_CTRL5_MASK        0xC0
>>>> +#define RT5033_FLED_CTRL5_RESV        0x10
>>>> +#define RT5033_FLED_CTRL5_TA_GOOD    0x40
>>>> +#define RT5033_FLED_CTRL5_TA_EXIST    0x80
>>>> +
>>>>    /* RT5033 control register */
>>>>    #define RT5033_CTRL_FCCM_BUCK_MASK        0x00
>>>>    #define RT5033_CTRL_BUCKOMS_MASK        0x01
>>>>
>>>
>>>
>>
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html