This is a sixth version of the patch set to support RT5033 Flash Led.
It is based on RFC [1] from Jacek's patch set.
Changes since v6:
- Make functions to convert from LED config data to register value
- Add variables to save LED config data in rt5033_led struct
- Rename some functions suitably
- Change to set register of flash brightness / timeout when it was changed.
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 | 541 +++++++++++++++++++++
include/linux/mfd/rt5033-private.h | 51 ++
5 files changed, 647 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-rt5033.txt
create mode 100644 drivers/leds/leds-rt5033.c
--
2.0.5
This patch adds the device tree bindings for RT5033 flash LEDs.
Signed-off-by: Ingi Kim <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
.../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
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.
Each of LED outputs can contorl a separate LED sharing their setting,
and can be connected together for a single connected LED.
One LED is represented by one child node.
Signed-off-by: Ingi Kim <[email protected]>
---
drivers/leds/Kconfig | 8 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-rt5033.c | 541 +++++++++++++++++++++++++++++++++++++
include/linux/mfd/rt5033-private.h | 51 ++++
4 files changed, 601 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..256df74
--- /dev/null
+++ b/drivers/leds/leds-rt5033.c
@@ -0,0 +1,541 @@
+/*
+ * led driver for RT5033
+ *
+ * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
+ * Ingi Kim <[email protected]>
+ *
+ * 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
+
+#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 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];
+
+ u32 current_flash_timeout;
+ u32 current_flash_brightness;
+
+ 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 u8 rt5033_torch_brightness_to_reg(u32 ua)
+{
+ return (ua - RT5033_LED_TORCH_BRIGHTNESS_MIN) /
+ RT5033_LED_TORCH_BRIGHTNESS_STEP;
+}
+
+static u8 rt5033_flash_brightness_to_reg(u32 ua)
+{
+ return (ua - RT5033_LED_FLASH_BRIGHTNESS_MIN) /
+ RT5033_LED_FLASH_BRIGHTNESS_STEP;
+}
+
+static u8 rt5033_flash_timeout_to_reg(u32 us)
+{
+ return (us - RT5033_LED_FLASH_TIMEOUT_MIN) /
+ RT5033_LED_FLASH_TIMEOUT_STEP;
+}
+
+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_get_iout_to_set(struct rt5033_led *led,
+ enum rt5033_fled fled_id)
+{
+ u8 fled_bit;
+
+ if (led->iout_joint)
+ fled_bit = FLED1_IOUT | FLED2_IOUT;
+ else
+ fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
+
+ return fled_bit;
+}
+
+static int rt5033_led_iout_disable(struct rt5033_led *led,
+ enum rt5033_fled fled_id)
+{
+ int ret;
+ u8 fled_bit;
+
+ fled_bit = rt5033_get_iout_to_set(led, fled_id);
+ led->current_iout &= ~fled_bit;
+
+ ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
+ RT5033_FLED_FUNC1_MASK,
+ RT5033_FLED_PINCTRL | led->current_iout);
+
+ return ret;
+}
+
+static int rt5033_set_flash_current(struct rt5033_led *led, u32 micro_amp)
+{
+ u8 v;
+ int ret;
+
+ v = rt5033_flash_brightness_to_reg(micro_amp);
+
+ ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL1, v);
+ if (ret < 0)
+ return ret;
+
+ led->current_flash_brightness = micro_amp;
+
+ return 0;
+}
+
+static int rt5033_set_timeout(struct rt5033_led *led, u32 microsec)
+{
+ u8 v;
+ int ret;
+
+ v = rt5033_flash_timeout_to_reg(microsec);
+
+ ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL2, v);
+ if (ret < 0)
+ return ret;
+
+ led->current_flash_timeout = microsec;
+
+ return 0;
+}
+
+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_disable(led, fled_id);
+ goto torch_unlock;
+ }
+
+ fled_bit = rt5033_get_iout_to_set(led, fled_id);
+
+ ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
+ RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
+ if (ret < 0)
+ goto torch_unlock;
+
+ if (led->current_iout != fled_bit) {
+ ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
+ RT5033_FLED_FUNC1_MASK,
+ RT5033_FLED_PINCTRL | fled_bit);
+ if (ret < 0)
+ goto torch_unlock;
+ led->current_iout = fled_bit;
+ }
+ 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);
+
+ mutex_lock(&led->lock);
+ sub_led->flash_brightness = 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);
+
+ mutex_lock(&led->lock);
+ sub_led->flash_timeout = timeout;
+ 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_get_iout_to_set(led, fled_id);
+ led->current_iout = fled_bit;
+
+ if (state == 0) {
+ ret = rt5033_led_iout_disable(led, fled_id);
+ if (ret < 0)
+ goto strobe_unlock;
+ ret = regmap_update_bits(led->regmap,
+ RT5033_REG_FLED_FUNCTION2,
+ RT5033_FLED_FUNC2_MASK, 0);
+ goto strobe_unlock;
+ }
+
+ if (sub_led->flash_brightness != led->current_flash_brightness) {
+ ret = rt5033_set_flash_current(led, sub_led->flash_brightness);
+ if (ret < 0)
+ goto strobe_unlock;
+ }
+
+ if (sub_led->flash_timeout != led->current_flash_timeout) {
+ ret = rt5033_set_timeout(led, 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;
+
+ /* 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_torch_brightness_to_reg(
+ led_cfg->torch_max_microamp[fled_id]);
+ led_cdev->flags |= LED_DEV_CAP_FLASH;
+
+ rt5033_init_flash_properties(sub_led, led_cfg);
+
+ sub_led->flash_timeout = fled_cdev->timeout.val;
+ sub_led->flash_brightness = fled_cdev->brightness.val;
+}
+
+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 <[email protected]>");
+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
Hi Ingi,
Thanks for the update.
On 11/25/2015 11:22 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.
>
> Each of LED outputs can contorl a separate LED sharing their setting,
s/contorl/control/
> and can be connected together for a single connected LED.
> One LED is represented by one child node.
>
> Signed-off-by: Ingi Kim <[email protected]>
> ---
> drivers/leds/Kconfig | 8 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-rt5033.c | 541 +++++++++++++++++++++++++++++++++++++
> include/linux/mfd/rt5033-private.h | 51 ++++
> 4 files changed, 601 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..256df74
> --- /dev/null
> +++ b/drivers/leds/leds-rt5033.c
> @@ -0,0 +1,541 @@
> +/*
> + * led driver for RT5033
> + *
> + * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
> + * Ingi Kim <[email protected]>
> + *
> + * 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
> +
> +#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 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];
> +
> + u32 current_flash_timeout;
> + u32 current_flash_brightness;
> +
> + 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 u8 rt5033_torch_brightness_to_reg(u32 ua)
> +{
> + return (ua - RT5033_LED_TORCH_BRIGHTNESS_MIN) /
> + RT5033_LED_TORCH_BRIGHTNESS_STEP;
> +}
> +
> +static u8 rt5033_flash_brightness_to_reg(u32 ua)
> +{
> + return (ua - RT5033_LED_FLASH_BRIGHTNESS_MIN) /
> + RT5033_LED_FLASH_BRIGHTNESS_STEP;
> +}
> +
> +static u8 rt5033_flash_timeout_to_reg(u32 us)
> +{
> + return (us - RT5033_LED_FLASH_TIMEOUT_MIN) /
> + RT5033_LED_FLASH_TIMEOUT_STEP;
> +}
Previous solution with macro was fine. Please bring it back, but rename
as I asked in the previous review.
> +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_get_iout_to_set(struct rt5033_led *led,
> + enum rt5033_fled fled_id)
> +{
> + u8 fled_bit;
> +
> + if (led->iout_joint)
> + fled_bit = FLED1_IOUT | FLED2_IOUT;
> + else
> + fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
> +
> + return fled_bit;
> +}
> +
> +static int rt5033_led_iout_disable(struct rt5033_led *led,
> + enum rt5033_fled fled_id)
> +{
> + int ret;
> + u8 fled_bit;
> +
> + fled_bit = rt5033_get_iout_to_set(led, fled_id);
> + led->current_iout &= ~fled_bit;
> +
> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
> + RT5033_FLED_FUNC1_MASK,
> + RT5033_FLED_PINCTRL | led->current_iout);
> +
> + return ret;
> +}
> +
> +static int rt5033_set_flash_current(struct rt5033_led *led, u32 micro_amp)
> +{
> + u8 v;
> + int ret;
> +
> + v = rt5033_flash_brightness_to_reg(micro_amp);
> +
> + ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL1, v);
> + if (ret < 0)
> + return ret;
> +
> + led->current_flash_brightness = micro_amp;
> +
> + return 0;
> +}
> +
> +static int rt5033_set_timeout(struct rt5033_led *led, u32 microsec)
Please rename it to rt5033_set_flash_timeout.
> +{
> + u8 v;
> + int ret;
> +
> + v = rt5033_flash_timeout_to_reg(microsec);
> +
> + ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL2, v);
> + if (ret < 0)
> + return ret;
> +
> + led->current_flash_timeout = microsec;
> +
> + return 0;
> +}
> +
> +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_disable(led, fled_id);
> + goto torch_unlock;
> + }
> +
> + fled_bit = rt5033_get_iout_to_set(led, fled_id);
> +
> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
> + RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
regmap_update_bits always incurs at least one I2C read transmission.
Instead, you could just call regmap_write only if brightness to be set
is different than the brightness actually set. You would need to
add current_torch_brightness property to the struct rt5033_led for that.
> + if (ret < 0)
> + goto torch_unlock;
> +
> + if (led->current_iout != fled_bit) {
> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
> + RT5033_FLED_FUNC1_MASK,
> + RT5033_FLED_PINCTRL | fled_bit);
Use regmap_write here.
> + if (ret < 0)
> + goto torch_unlock;
> + led->current_iout = fled_bit;
> + }
> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
> + RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED);
Ditto.
> +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);
> +
> + mutex_lock(&led->lock);
> + sub_led->flash_brightness = brightness;
> + mutex_unlock(&led->lock);
Mutex protection is redundant in this case. You would need it if device
state was also changed here.
BTW why flash brightness can't be written to the device here?
> +
> + 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);
> +
> + mutex_lock(&led->lock);
> + sub_led->flash_timeout = timeout;
> + mutex_unlock(&led->lock);
Ditto.
> + 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_get_iout_to_set(led, fled_id);
> + led->current_iout = fled_bit;
> +
> + if (state == 0) {
> + ret = rt5033_led_iout_disable(led, fled_id);
> + if (ret < 0)
> + goto strobe_unlock;
> + ret = regmap_update_bits(led->regmap,
> + RT5033_REG_FLED_FUNCTION2,
> + RT5033_FLED_FUNC2_MASK, 0);
> + goto strobe_unlock;
> + }
> +
> + if (sub_led->flash_brightness != led->current_flash_brightness) {
> + ret = rt5033_set_flash_current(led, sub_led->flash_brightness);
> + if (ret < 0)
> + goto strobe_unlock;
> + }
> +
> + if (sub_led->flash_timeout != led->current_flash_timeout) {
> + ret = rt5033_set_timeout(led, 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);
Use regmap_write instead of regmap_update_bits.
> +
> + 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;
Actually only one generic variable of this type is needed here,
e.g. setting or prop.
> + enum rt5033_fled fled_id = sub_led->fled_id;
> +
> + tm_set = &fled_cdev->timeout;
setting = &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;
setting = &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;
> +
> + /* 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_torch_brightness_to_reg(
> + led_cfg->torch_max_microamp[fled_id]);
> + led_cdev->flags |= LED_DEV_CAP_FLASH;
> +
> + rt5033_init_flash_properties(sub_led, led_cfg);
> +
> + sub_led->flash_timeout = fled_cdev->timeout.val;
> + sub_led->flash_brightness = fled_cdev->brightness.val;
> +}
> +
> +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);
You're setting this register for the first time, so regmap_write should
be used. Please refer to the regmap_update_bits definition in
drivers/base/regmap/regmap.c. There's no point in using
regmap_update_bits if you want to write all bits. The third
argument of the function is a bitmask, which determines which bits
should be updated.
> + 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 <[email protected]>");
> +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
Bitmask should define group of bits that control single
functionality. There is no point in defining a bit mask
for the whole register width.
> +#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
Ditto.
> +#define RT5033_FLED_ENFLED 0x01
> +#define RT5033_FLED_SREG_STRB 0x80
> +
> +/* RT5033 FLED Strobe Control1 */
> +#define RT5033_FLED_STRBCTRL1_MASK 0xFF
Ditto.
> +#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX 0xE0
> +#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF 0x0D
> +#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX 0x1F
Don't mix value constraints with bitmask .
You don't use above MAX and DEF macros in the driver, but
instead you define following set of macros in leds-rt5033.c:
#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
These can be moved to this file, but below bit field
definitions.
Besides, you could add bitmasks for "Timeout Current Level"
adn "Fled Strobe Current" bitfields, that are documented
for this register.
> +
> +/* RT5033 FLED Strobe Control2 */
> +#define RT5033_FLED_STRBCTRL2_MASK 0x3F
> +#define RT5033_FLED_STRBCTRL2_TM_DEF 0x0F
> +#define RT5033_FLED_STRBCTRL2_TM_MAX 0x3F
Insted of the above three please just add bitmask definition for the
"FLED Strobe Timeout" bits.
> +/* 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
Similarly, just add bitmask definitions for "FLED Torch Current"
and "LPB".
> +/* RT5033 FLED Control2 */
> +#define RT5033_FLED_CTRL2_MASK 0xFF
This bitmask is useless.
> +#define RT5033_FLED_CTRL2_VMID_DEF 0x37
Please remove this.
> +#define RT5033_FLED_CTRL2_VMID_MAX 0x3F
Rename MAX to MASK.
> +#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
Above three are useless.
> +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60
Rename DEF to MASK.
> +#define RT5033_FLED_CTRL4_TRACK_CLK 0x80
> +
> +/* RT5033 FLED Control5 */
> +#define RT5033_FLED_CTRL5_MASK 0xC0
> +#define RT5033_FLED_CTRL5_RESV 0x10
Remove both above lines.
> +#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
Hi Jacek,
Thanks for the review.
On 2015년 11월 26일 00:13, Jacek Anaszewski wrote:
> Hi Ingi,
>
> Thanks for the update.
>
> On 11/25/2015 11:22 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.
>>
>> Each of LED outputs can contorl a separate LED sharing their setting,
>
> s/contorl/control/
>
Oh, I see, Thanks
>> and can be connected together for a single connected LED.
>> One LED is represented by one child node.
>>
>> Signed-off-by: Ingi Kim <[email protected]>
>> ---
>> drivers/leds/Kconfig | 8 +
>> drivers/leds/Makefile | 1 +
>> drivers/leds/leds-rt5033.c | 541 +++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/rt5033-private.h | 51 ++++
>> 4 files changed, 601 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..256df74
>> --- /dev/null
>> +++ b/drivers/leds/leds-rt5033.c
>> @@ -0,0 +1,541 @@
>> +/*
>> + * led driver for RT5033
>> + *
>> + * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
>> + * Ingi Kim <[email protected]>
>> + *
>> + * 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
>> +
>> +#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 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];
>> +
>> + u32 current_flash_timeout;
>> + u32 current_flash_brightness;
>> +
>> + 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 u8 rt5033_torch_brightness_to_reg(u32 ua)
>> +{
>> + return (ua - RT5033_LED_TORCH_BRIGHTNESS_MIN) /
>> + RT5033_LED_TORCH_BRIGHTNESS_STEP;
>> +}
>> +
>> +static u8 rt5033_flash_brightness_to_reg(u32 ua)
>> +{
>> + return (ua - RT5033_LED_FLASH_BRIGHTNESS_MIN) /
>> + RT5033_LED_FLASH_BRIGHTNESS_STEP;
>> +}
>> +
>> +static u8 rt5033_flash_timeout_to_reg(u32 us)
>> +{
>> + return (us - RT5033_LED_FLASH_TIMEOUT_MIN) /
>> + RT5033_LED_FLASH_TIMEOUT_STEP;
>> +}
>
> Previous solution with macro was fine. Please bring it back, but rename
> as I asked in the previous review.
>
Okay, I'll change it
>> +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_get_iout_to_set(struct rt5033_led *led,
>> + enum rt5033_fled fled_id)
>> +{
>> + u8 fled_bit;
>> +
>> + if (led->iout_joint)
>> + fled_bit = FLED1_IOUT | FLED2_IOUT;
>> + else
>> + fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
>> +
>> + return fled_bit;
>> +}
>> +
>> +static int rt5033_led_iout_disable(struct rt5033_led *led,
>> + enum rt5033_fled fled_id)
>> +{
>> + int ret;
>> + u8 fled_bit;
>> +
>> + fled_bit = rt5033_get_iout_to_set(led, fled_id);
>> + led->current_iout &= ~fled_bit;
>> +
>> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>> + RT5033_FLED_FUNC1_MASK,
>> + RT5033_FLED_PINCTRL | led->current_iout);
>> +
>> + return ret;
>> +}
>> +
>> +static int rt5033_set_flash_current(struct rt5033_led *led, u32 micro_amp)
>> +{
>> + u8 v;
>> + int ret;
>> +
>> + v = rt5033_flash_brightness_to_reg(micro_amp);
>> +
>> + ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL1, v);
>> + if (ret < 0)
>> + return ret;
>> +
>> + led->current_flash_brightness = micro_amp;
>> +
>> + return 0;
>> +}
>> +
>> +static int rt5033_set_timeout(struct rt5033_led *led, u32 microsec)
>
> Please rename it to rt5033_set_flash_timeout.
>
Ok, I'll do
>> +{
>> + u8 v;
>> + int ret;
>> +
>> + v = rt5033_flash_timeout_to_reg(microsec);
>> +
>> + ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL2, v);
>> + if (ret < 0)
>> + return ret;
>> +
>> + led->current_flash_timeout = microsec;
>> +
>> + return 0;
>> +}
>> +
>> +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_disable(led, fled_id);
>> + goto torch_unlock;
>> + }
>> +
>> + fled_bit = rt5033_get_iout_to_set(led, fled_id);
>> +
>> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
>> + RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
>
> regmap_update_bits always incurs at least one I2C read transmission.
> Instead, you could just call regmap_write only if brightness to be set is different than the brightness actually set. You would need to
> add current_torch_brightness property to the struct rt5033_led for that.
>
Thanks, regmap_write seems to be more suitable.
I'll change it.
>> + if (ret < 0)
>> + goto torch_unlock;
>> +
>> + if (led->current_iout != fled_bit) {
>> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>> + RT5033_FLED_FUNC1_MASK,
>> + RT5033_FLED_PINCTRL | fled_bit);
>
> Use regmap_write here.
>
Thanks,
>> + if (ret < 0)
>> + goto torch_unlock;
>> + led->current_iout = fled_bit;
>> + }
>> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
>> + RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED);
>
> Ditto.
>
Thanks,
>> +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);
>> +
>> + mutex_lock(&led->lock);
>> + sub_led->flash_brightness = brightness;
>> + mutex_unlock(&led->lock);
>
> Mutex protection is redundant in this case. You would need it if device
> state was also changed here.
Okay, I'll remove it.
>
> BTW why flash brightness can't be written to the device here?
>
Flash brightness is only affected when FLED flashed (strobing).
So, I think it is better to be written in rt5033_led_flash_strobe_set function.
>> +
>> + 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);
>> +
>> + mutex_lock(&led->lock);
>> + sub_led->flash_timeout = timeout;
>> + mutex_unlock(&led->lock);
>
> Ditto.
>
Thanks,
>> + 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_get_iout_to_set(led, fled_id);
>> + led->current_iout = fled_bit;
>> +
>> + if (state == 0) {
>> + ret = rt5033_led_iout_disable(led, fled_id);
>> + if (ret < 0)
>> + goto strobe_unlock;
>> + ret = regmap_update_bits(led->regmap,
>> + RT5033_REG_FLED_FUNCTION2,
>> + RT5033_FLED_FUNC2_MASK, 0);
>> + goto strobe_unlock;
>> + }
>> +
>> + if (sub_led->flash_brightness != led->current_flash_brightness) {
>> + ret = rt5033_set_flash_current(led, sub_led->flash_brightness);
>> + if (ret < 0)
>> + goto strobe_unlock;
>> + }
>> +
>> + if (sub_led->flash_timeout != led->current_flash_timeout) {
>> + ret = rt5033_set_timeout(led, 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);
>
> Use regmap_write instead of regmap_update_bits.
>
Thanks,
>> +
>> + 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;
>
> Actually only one generic variable of this type is needed here,
> e.g. setting or prop.
>
Thanks, I'll change
>> + enum rt5033_fled fled_id = sub_led->fled_id;
>> +
>> + tm_set = &fled_cdev->timeout;
>
> setting = &fled_cdev->timeout;
>
Thanks,
>> + 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;
>
> setting = &fled_cdev->brightness;
>
Thanks,
>> + 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;
>> +
>> + /* 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_torch_brightness_to_reg(
>> + led_cfg->torch_max_microamp[fled_id]);
>> + led_cdev->flags |= LED_DEV_CAP_FLASH;
>> +
>> + rt5033_init_flash_properties(sub_led, led_cfg);
>> +
>> + sub_led->flash_timeout = fled_cdev->timeout.val;
>> + sub_led->flash_brightness = fled_cdev->brightness.val;
>> +}
>> +
>> +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);
>
> You're setting this register for the first time, so regmap_write should
> be used. Please refer to the regmap_update_bits definition in
> drivers/base/regmap/regmap.c. There's no point in using
> regmap_update_bits if you want to write all bits. The third
> argument of the function is a bitmask, which determines which bits
> should be updated.
>
That's right, It is better to use regmap_write. I'll change it.
>> + 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 <[email protected]>");
>> +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
>
> Bitmask should define group of bits that control single
> functionality. There is no point in defining a bit mask
> for the whole register width.
>
Thanks, I'll remove it.
>> +#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
>
> Ditto.
>
Thanks,
>> +#define RT5033_FLED_ENFLED 0x01
>> +#define RT5033_FLED_SREG_STRB 0x80
>> +
>> +/* RT5033 FLED Strobe Control1 */
>> +#define RT5033_FLED_STRBCTRL1_MASK 0xFF
>
> Ditto.
>
Thanks,
>> +#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX 0xE0
>> +#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF 0x0D
>> +#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX 0x1F
>
> Don't mix value constraints with bitmask .
> You don't use above MAX and DEF macros in the driver, but
> instead you define following set of macros in leds-rt5033.c:
>
> #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
>
> These can be moved to this file, but below bit field
> definitions.
>
> Besides, you could add bitmasks for "Timeout Current Level"
> adn "Fled Strobe Current" bitfields, that are documented
> for this register.
>
Thanks, I understand.
I'll change it
>> +
>> +/* RT5033 FLED Strobe Control2 */
>> +#define RT5033_FLED_STRBCTRL2_MASK 0x3F
>> +#define RT5033_FLED_STRBCTRL2_TM_DEF 0x0F
>> +#define RT5033_FLED_STRBCTRL2_TM_MAX 0x3F
>
> Insted of the above three please just add bitmask definition for the
> "FLED Strobe Timeout" bits.
>
Thanks, I'll change it
>> +/* 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
>
> Similarly, just add bitmask definitions for "FLED Torch Current"
> and "LPB".
>
Thanks, I'll change it
>> +/* RT5033 FLED Control2 */
>> +#define RT5033_FLED_CTRL2_MASK 0xFF
>
> This bitmask is useless.
>
Thanks,
>> +#define RT5033_FLED_CTRL2_VMID_DEF 0x37
>
> Please remove this.
>
Thanks,
>> +#define RT5033_FLED_CTRL2_VMID_MAX 0x3F
>
> Rename MAX to MASK.
>
Thanks, I'll change it.
>> +#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
>
> Above three are useless.
>
Thanks,
>> +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60
>
> Rename DEF to MASK.
>
Thanks,
>> +#define RT5033_FLED_CTRL4_TRACK_CLK 0x80
>> +
>> +/* RT5033 FLED Control5 */
>> +#define RT5033_FLED_CTRL5_MASK 0xC0
>> +#define RT5033_FLED_CTRL5_RESV 0x10
>
> Remove both above lines.
>
Thanks,
>> +#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
>>
>
>
On Wed, 25 Nov 2015, 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.
>
> Each of LED outputs can contorl a separate LED sharing their setting,
> and can be connected together for a single connected LED.
> One LED is represented by one child node.
>
> Signed-off-by: Ingi Kim <[email protected]>
> ---
> drivers/leds/Kconfig | 8 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-rt5033.c | 541 +++++++++++++++++++++++++++++++++++++
> include/linux/mfd/rt5033-private.h | 51 ++++
Acked-by: Lee Jones <[email protected]>
> 4 files changed, 601 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..256df74
> --- /dev/null
> +++ b/drivers/leds/leds-rt5033.c
> @@ -0,0 +1,541 @@
> +/*
> + * led driver for RT5033
> + *
> + * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
> + * Ingi Kim <[email protected]>
> + *
> + * 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
> +
> +#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 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];
> +
> + u32 current_flash_timeout;
> + u32 current_flash_brightness;
> +
> + 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 u8 rt5033_torch_brightness_to_reg(u32 ua)
> +{
> + return (ua - RT5033_LED_TORCH_BRIGHTNESS_MIN) /
> + RT5033_LED_TORCH_BRIGHTNESS_STEP;
> +}
> +
> +static u8 rt5033_flash_brightness_to_reg(u32 ua)
> +{
> + return (ua - RT5033_LED_FLASH_BRIGHTNESS_MIN) /
> + RT5033_LED_FLASH_BRIGHTNESS_STEP;
> +}
> +
> +static u8 rt5033_flash_timeout_to_reg(u32 us)
> +{
> + return (us - RT5033_LED_FLASH_TIMEOUT_MIN) /
> + RT5033_LED_FLASH_TIMEOUT_STEP;
> +}
> +
> +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_get_iout_to_set(struct rt5033_led *led,
> + enum rt5033_fled fled_id)
> +{
> + u8 fled_bit;
> +
> + if (led->iout_joint)
> + fled_bit = FLED1_IOUT | FLED2_IOUT;
> + else
> + fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
> +
> + return fled_bit;
> +}
> +
> +static int rt5033_led_iout_disable(struct rt5033_led *led,
> + enum rt5033_fled fled_id)
> +{
> + int ret;
> + u8 fled_bit;
> +
> + fled_bit = rt5033_get_iout_to_set(led, fled_id);
> + led->current_iout &= ~fled_bit;
> +
> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
> + RT5033_FLED_FUNC1_MASK,
> + RT5033_FLED_PINCTRL | led->current_iout);
> +
> + return ret;
> +}
> +
> +static int rt5033_set_flash_current(struct rt5033_led *led, u32 micro_amp)
> +{
> + u8 v;
> + int ret;
> +
> + v = rt5033_flash_brightness_to_reg(micro_amp);
> +
> + ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL1, v);
> + if (ret < 0)
> + return ret;
> +
> + led->current_flash_brightness = micro_amp;
> +
> + return 0;
> +}
> +
> +static int rt5033_set_timeout(struct rt5033_led *led, u32 microsec)
> +{
> + u8 v;
> + int ret;
> +
> + v = rt5033_flash_timeout_to_reg(microsec);
> +
> + ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL2, v);
> + if (ret < 0)
> + return ret;
> +
> + led->current_flash_timeout = microsec;
> +
> + return 0;
> +}
> +
> +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_disable(led, fled_id);
> + goto torch_unlock;
> + }
> +
> + fled_bit = rt5033_get_iout_to_set(led, fled_id);
> +
> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
> + RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
> + if (ret < 0)
> + goto torch_unlock;
> +
> + if (led->current_iout != fled_bit) {
> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
> + RT5033_FLED_FUNC1_MASK,
> + RT5033_FLED_PINCTRL | fled_bit);
> + if (ret < 0)
> + goto torch_unlock;
> + led->current_iout = fled_bit;
> + }
> + 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);
> +
> + mutex_lock(&led->lock);
> + sub_led->flash_brightness = 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);
> +
> + mutex_lock(&led->lock);
> + sub_led->flash_timeout = timeout;
> + 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_get_iout_to_set(led, fled_id);
> + led->current_iout = fled_bit;
> +
> + if (state == 0) {
> + ret = rt5033_led_iout_disable(led, fled_id);
> + if (ret < 0)
> + goto strobe_unlock;
> + ret = regmap_update_bits(led->regmap,
> + RT5033_REG_FLED_FUNCTION2,
> + RT5033_FLED_FUNC2_MASK, 0);
> + goto strobe_unlock;
> + }
> +
> + if (sub_led->flash_brightness != led->current_flash_brightness) {
> + ret = rt5033_set_flash_current(led, sub_led->flash_brightness);
> + if (ret < 0)
> + goto strobe_unlock;
> + }
> +
> + if (sub_led->flash_timeout != led->current_flash_timeout) {
> + ret = rt5033_set_timeout(led, 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;
> +
> + /* 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_torch_brightness_to_reg(
> + led_cfg->torch_max_microamp[fled_id]);
> + led_cdev->flags |= LED_DEV_CAP_FLASH;
> +
> + rt5033_init_flash_properties(sub_led, led_cfg);
> +
> + sub_led->flash_timeout = fled_cdev->timeout.val;
> + sub_led->flash_brightness = fled_cdev->brightness.val;
> +}
> +
> +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 <[email protected]>");
> +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
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, 26 Nov 2015, Ingi Kim wrote:
> On 2015년 11월 26일 00:13, Jacek Anaszewski wrote:
> > Hi Ingi,
> >
> > Thanks for the update.
> >
> > On 11/25/2015 11:22 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.
> >>
> >> Each of LED outputs can contorl a separate LED sharing their setting,
> >
> > s/contorl/control/
> >
>
> Oh, I see, Thanks
>
> >> and can be connected together for a single connected LED.
> >> One LED is represented by one child node.
> >>
> >> Signed-off-by: Ingi Kim <[email protected]>
> >> ---
> >> drivers/leds/Kconfig | 8 +
> >> drivers/leds/Makefile | 1 +
> >> drivers/leds/leds-rt5033.c | 541 +++++++++++++++++++++++++++++++++++++
> >> include/linux/mfd/rt5033-private.h | 51 ++++
> >> 4 files changed, 601 insertions(+)
> >> create mode 100644 drivers/leds/leds-rt5033.c
[...]
> >> 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
> >
> > Bitmask should define group of bits that control single
> > functionality. There is no point in defining a bit mask
> > for the whole register width.
> >
>
> Thanks, I'll remove it.
>
> >> +#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
> >
> > Ditto.
> >
>
> Thanks,
>
> >> +#define RT5033_FLED_ENFLED 0x01
> >> +#define RT5033_FLED_SREG_STRB 0x80
> >> +
> >> +/* RT5033 FLED Strobe Control1 */
> >> +#define RT5033_FLED_STRBCTRL1_MASK 0xFF
> >
> > Ditto.
> >
>
> Thanks,
>
> >> +#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX 0xE0
> >> +#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF 0x0D
> >> +#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX 0x1F
> >
> > Don't mix value constraints with bitmask .
> > You don't use above MAX and DEF macros in the driver, but
> > instead you define following set of macros in leds-rt5033.c:
> >
> > #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
> >
> > These can be moved to this file, but below bit field
> > definitions.
> >
> > Besides, you could add bitmasks for "Timeout Current Level"
> > adn "Fled Strobe Current" bitfields, that are documented
> > for this register.
> >
>
> Thanks, I understand.
> I'll change it
>
> >> +
> >> +/* RT5033 FLED Strobe Control2 */
> >> +#define RT5033_FLED_STRBCTRL2_MASK 0x3F
> >> +#define RT5033_FLED_STRBCTRL2_TM_DEF 0x0F
> >> +#define RT5033_FLED_STRBCTRL2_TM_MAX 0x3F
> >
> > Insted of the above three please just add bitmask definition for the
> > "FLED Strobe Timeout" bits.
> >
>
> Thanks, I'll change it
>
> >> +/* 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
> >
> > Similarly, just add bitmask definitions for "FLED Torch Current"
> > and "LPB".
> >
>
> Thanks, I'll change it
>
> >> +/* RT5033 FLED Control2 */
> >> +#define RT5033_FLED_CTRL2_MASK 0xFF
> >
> > This bitmask is useless.
> >
>
> Thanks,
>
> >> +#define RT5033_FLED_CTRL2_VMID_DEF 0x37
> >
> > Please remove this.
> >
>
> Thanks,
>
> >> +#define RT5033_FLED_CTRL2_VMID_MAX 0x3F
> >
> > Rename MAX to MASK.
> >
>
> Thanks, I'll change it.
>
> >> +#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
> >
> > Above three are useless.
> >
>
> Thanks,
>
> >> +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60
> >
> > Rename DEF to MASK.
> >
>
> Thanks,
>
> >> +#define RT5033_FLED_CTRL4_TRACK_CLK 0x80
> >> +
> >> +/* RT5033 FLED Control5 */
> >> +#define RT5033_FLED_CTRL5_MASK 0xC0
> >> +#define RT5033_FLED_CTRL5_RESV 0x10
> >
> > Remove both above lines.
> >
>
> Thanks,
> >> +#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
Once you've made these changes, please carry my Ack across.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Ingi,
On 11/26/2015 09:02 AM, Ingi Kim wrote:
[...]
>>> +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);
>>> +
>>> + mutex_lock(&led->lock);
>>> + sub_led->flash_brightness = brightness;
>>> + mutex_unlock(&led->lock);
>>
>> Mutex protection is redundant in this case. You would need it if device
>> state was also changed here.
>
> Okay, I'll remove it.
>
>>
>> BTW why flash brightness can't be written to the device here?
>>
>
> Flash brightness is only affected when FLED flashed (strobing).
> So, I think it is better to be written in rt5033_led_flash_strobe_set function.
strobe_set op should strobe the flash ASAP, and delegating brightness
setting there extends a delay between calling strobe_set op
and actual flash strobe. If you set the brightness here, then you
wouldn't have to do that in the strobe_set op, of course unless the
the brightness is altered through the API of the other LED, in two
separate LEDs case.
>>> +
>>> + 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);
>>> +
>>> + mutex_lock(&led->lock);
>>> + sub_led->flash_timeout = timeout;
>>> + mutex_unlock(&led->lock);
>>
>> Ditto.
>>
Timeout should be also written here.
If you will add regmap_write in both ops, then mutex protection will
have to be preserved, to assure consistency between registers state
and sub_led->flash_brightness and sub_led->flash_timeout state.
>
>>> +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60
>>
>> Rename DEF to MASK.
Hmm, here it should be: Rename MAX to MASK.
--
Best Regards,
Jacek Anaszewski
Hi Jacek,
On 2015년 11월 26일 18:43, Jacek Anaszewski wrote:
> Hi Ingi,
>
> On 11/26/2015 09:02 AM, Ingi Kim wrote:
> [...]
>>>> +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);
>>>> +
>>>> + mutex_lock(&led->lock);
>>>> + sub_led->flash_brightness = brightness;
>>>> + mutex_unlock(&led->lock);
>>>
>>> Mutex protection is redundant in this case. You would need it if device
>>> state was also changed here.
>>
>> Okay, I'll remove it.
>>
>>>
>>> BTW why flash brightness can't be written to the device here?
>>>
>>
>> Flash brightness is only affected when FLED flashed (strobing).
>> So, I think it is better to be written in rt5033_led_flash_strobe_set function.
>
> strobe_set op should strobe the flash ASAP, and delegating brightness
> setting there extends a delay between calling strobe_set op
> and actual flash strobe. If you set the brightness here, then you
> wouldn't have to do that in the strobe_set op, of course unless the
> the brightness is altered through the API of the other LED, in two
> separate LEDs case.
>
The brightness may be able to change its brightness in two separate LEDs case as you see.
So, I think it would be better to write brightness setting in strobe_op.
In consideration of delay, of course, the brightness is set just when it would be changed.
>>>> +
>>>> + 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);
>>>> +
>>>> + mutex_lock(&led->lock);
>>>> + sub_led->flash_timeout = timeout;
>>>> + mutex_unlock(&led->lock);
>>>
>>> Ditto.
>>>
>
> Timeout should be also written here.
>
The timeout may be able to change its flash timeout in two separate LEDs case as you see.
So, I think it would be better to write timeout setting in strobe_op.
In consideration of delay, of course, the timeout is set just when it would be changed.
> If you will add regmap_write in both ops, then mutex protection will
> have to be preserved, to assure consistency between registers state
> and sub_led->flash_brightness and sub_led->flash_timeout state.
>
Thanks, but mutex protection is useless in this case.
so I try to remove it.
>>
>>>> +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60
>>>
>>> Rename DEF to MASK.
>
> Hmm, here it should be: Rename MAX to MASK.
>
Oh
Okay, Thanks :)
Hi, Lee
Thanks to the review,
I'll carry your Ack when I reflect Jacek's comment.
On 2015년 11월 26일 18:11, Lee Jones wrote:
> On Wed, 25 Nov 2015, 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.
>>
>> Each of LED outputs can contorl a separate LED sharing their setting,
>> and can be connected together for a single connected LED.
>> One LED is represented by one child node.
>>
>> Signed-off-by: Ingi Kim <[email protected]>
>> ---
>> drivers/leds/Kconfig | 8 +
>> drivers/leds/Makefile | 1 +
>> drivers/leds/leds-rt5033.c | 541 +++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/rt5033-private.h | 51 ++++
>
> Acked-by: Lee Jones <[email protected]>
>
>> 4 files changed, 601 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..256df74
>> --- /dev/null
>> +++ b/drivers/leds/leds-rt5033.c
>> @@ -0,0 +1,541 @@
>> +/*
>> + * led driver for RT5033
>> + *
>> + * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
>> + * Ingi Kim <[email protected]>
>> + *
>> + * 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
>> +
>> +#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 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];
>> +
>> + u32 current_flash_timeout;
>> + u32 current_flash_brightness;
>> +
>> + 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 u8 rt5033_torch_brightness_to_reg(u32 ua)
>> +{
>> + return (ua - RT5033_LED_TORCH_BRIGHTNESS_MIN) /
>> + RT5033_LED_TORCH_BRIGHTNESS_STEP;
>> +}
>> +
>> +static u8 rt5033_flash_brightness_to_reg(u32 ua)
>> +{
>> + return (ua - RT5033_LED_FLASH_BRIGHTNESS_MIN) /
>> + RT5033_LED_FLASH_BRIGHTNESS_STEP;
>> +}
>> +
>> +static u8 rt5033_flash_timeout_to_reg(u32 us)
>> +{
>> + return (us - RT5033_LED_FLASH_TIMEOUT_MIN) /
>> + RT5033_LED_FLASH_TIMEOUT_STEP;
>> +}
>> +
>> +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_get_iout_to_set(struct rt5033_led *led,
>> + enum rt5033_fled fled_id)
>> +{
>> + u8 fled_bit;
>> +
>> + if (led->iout_joint)
>> + fled_bit = FLED1_IOUT | FLED2_IOUT;
>> + else
>> + fled_bit = (fled_id == FLED1) ? FLED1_IOUT : FLED2_IOUT;
>> +
>> + return fled_bit;
>> +}
>> +
>> +static int rt5033_led_iout_disable(struct rt5033_led *led,
>> + enum rt5033_fled fled_id)
>> +{
>> + int ret;
>> + u8 fled_bit;
>> +
>> + fled_bit = rt5033_get_iout_to_set(led, fled_id);
>> + led->current_iout &= ~fled_bit;
>> +
>> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>> + RT5033_FLED_FUNC1_MASK,
>> + RT5033_FLED_PINCTRL | led->current_iout);
>> +
>> + return ret;
>> +}
>> +
>> +static int rt5033_set_flash_current(struct rt5033_led *led, u32 micro_amp)
>> +{
>> + u8 v;
>> + int ret;
>> +
>> + v = rt5033_flash_brightness_to_reg(micro_amp);
>> +
>> + ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL1, v);
>> + if (ret < 0)
>> + return ret;
>> +
>> + led->current_flash_brightness = micro_amp;
>> +
>> + return 0;
>> +}
>> +
>> +static int rt5033_set_timeout(struct rt5033_led *led, u32 microsec)
>> +{
>> + u8 v;
>> + int ret;
>> +
>> + v = rt5033_flash_timeout_to_reg(microsec);
>> +
>> + ret = regmap_write(led->regmap, RT5033_REG_FLED_STROBE_CTRL2, v);
>> + if (ret < 0)
>> + return ret;
>> +
>> + led->current_flash_timeout = microsec;
>> +
>> + return 0;
>> +}
>> +
>> +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_disable(led, fled_id);
>> + goto torch_unlock;
>> + }
>> +
>> + fled_bit = rt5033_get_iout_to_set(led, fled_id);
>> +
>> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
>> + RT5033_FLED_CTRL1_MASK, (brightness - 1) << 4);
>> + if (ret < 0)
>> + goto torch_unlock;
>> +
>> + if (led->current_iout != fled_bit) {
>> + ret = regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
>> + RT5033_FLED_FUNC1_MASK,
>> + RT5033_FLED_PINCTRL | fled_bit);
>> + if (ret < 0)
>> + goto torch_unlock;
>> + led->current_iout = fled_bit;
>> + }
>> + 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);
>> +
>> + mutex_lock(&led->lock);
>> + sub_led->flash_brightness = 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);
>> +
>> + mutex_lock(&led->lock);
>> + sub_led->flash_timeout = timeout;
>> + 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_get_iout_to_set(led, fled_id);
>> + led->current_iout = fled_bit;
>> +
>> + if (state == 0) {
>> + ret = rt5033_led_iout_disable(led, fled_id);
>> + if (ret < 0)
>> + goto strobe_unlock;
>> + ret = regmap_update_bits(led->regmap,
>> + RT5033_REG_FLED_FUNCTION2,
>> + RT5033_FLED_FUNC2_MASK, 0);
>> + goto strobe_unlock;
>> + }
>> +
>> + if (sub_led->flash_brightness != led->current_flash_brightness) {
>> + ret = rt5033_set_flash_current(led, sub_led->flash_brightness);
>> + if (ret < 0)
>> + goto strobe_unlock;
>> + }
>> +
>> + if (sub_led->flash_timeout != led->current_flash_timeout) {
>> + ret = rt5033_set_timeout(led, 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;
>> +
>> + /* 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_torch_brightness_to_reg(
>> + led_cfg->torch_max_microamp[fled_id]);
>> + led_cdev->flags |= LED_DEV_CAP_FLASH;
>> +
>> + rt5033_init_flash_properties(sub_led, led_cfg);
>> +
>> + sub_led->flash_timeout = fled_cdev->timeout.val;
>> + sub_led->flash_brightness = fled_cdev->brightness.val;
>> +}
>> +
>> +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 <[email protected]>");
>> +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
>
Hi Ingi,
On 11/30/2015 03:31 AM, Ingi Kim wrote:
> Hi Jacek,
>
> On 2015년 11월 26일 18:43, Jacek Anaszewski wrote:
>> Hi Ingi,
>>
>> On 11/26/2015 09:02 AM, Ingi Kim wrote:
>> [...]
>>>>> +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);
>>>>> +
>>>>> + mutex_lock(&led->lock);
>>>>> + sub_led->flash_brightness = brightness;
>>>>> + mutex_unlock(&led->lock);
>>>>
>>>> Mutex protection is redundant in this case. You would need it if device
>>>> state was also changed here.
>>>
>>> Okay, I'll remove it.
>>>
>>>>
>>>> BTW why flash brightness can't be written to the device here?
>>>>
>>>
>>> Flash brightness is only affected when FLED flashed (strobing).
>>> So, I think it is better to be written in rt5033_led_flash_strobe_set function.
>>
>> strobe_set op should strobe the flash ASAP, and delegating brightness
>> setting there extends a delay between calling strobe_set op
>> and actual flash strobe. If you set the brightness here, then you
>> wouldn't have to do that in the strobe_set op, of course unless the
>> the brightness is altered through the API of the other LED, in two
>> separate LEDs case.
>>
>
> The brightness may be able to change its brightness in two separate LEDs case as you see.
> So, I think it would be better to write brightness setting in strobe_op.
Could you motivate your statement, please? Why would it be better?
> In consideration of delay, of course, the brightness is set just when it would be changed.
I think that joint iout arrangement will be prevailing - this is the
case for your board, isn't it? With the modification I am proposing
the gain is clear.
>>>>> +
>>>>> + 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);
>>>>> +
>>>>> + mutex_lock(&led->lock);
>>>>> + sub_led->flash_timeout = timeout;
>>>>> + mutex_unlock(&led->lock);
>>>>
>>>> Ditto.
>>>>
>>
>> Timeout should be also written here.
>>
>
> The timeout may be able to change its flash timeout in two separate LEDs case as you see.
> So, I think it would be better to write timeout setting in strobe_op.
> In consideration of delay, of course, the timeout is set just when it would be changed.
>
>> If you will add regmap_write in both ops, then mutex protection will
>> have to be preserved, to assure consistency between registers state
>> and sub_led->flash_brightness and sub_led->flash_timeout state.
>>
>
> Thanks, but mutex protection is useless in this case.
> so I try to remove it.
>
>>>
>>>>> +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60
>>>>
>>>> Rename DEF to MASK.
>>
>> Hmm, here it should be: Rename MAX to MASK.
>>
>
> Oh
> Okay, Thanks :)
>
--
Best Regards,
Jacek Anaszewski
Hi Jacek,
On 2015년 11월 30일 19:59, Jacek Anaszewski wrote:
> Hi Ingi,
>
> On 11/30/2015 03:31 AM, Ingi Kim wrote:
>> Hi Jacek,
>>
>> On 2015년 11월 26일 18:43, Jacek Anaszewski wrote:
>>> Hi Ingi,
>>>
>>> On 11/26/2015 09:02 AM, Ingi Kim wrote:
>>> [...]
>>>>>> +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);
>>>>>> +
>>>>>> + mutex_lock(&led->lock);
>>>>>> + sub_led->flash_brightness = brightness;
>>>>>> + mutex_unlock(&led->lock);
>>>>>
>>>>> Mutex protection is redundant in this case. You would need it if device
>>>>> state was also changed here.
>>>>
>>>> Okay, I'll remove it.
>>>>
>>>>>
>>>>> BTW why flash brightness can't be written to the device here?
>>>>>
>>>>
>>>> Flash brightness is only affected when FLED flashed (strobing).
>>>> So, I think it is better to be written in rt5033_led_flash_strobe_set function.
>>>
>>> strobe_set op should strobe the flash ASAP, and delegating brightness
>>> setting there extends a delay between calling strobe_set op
>>> and actual flash strobe. If you set the brightness here, then you
>>> wouldn't have to do that in the strobe_set op, of course unless the
>>> the brightness is altered through the API of the other LED, in two
>>> separate LEDs case.
>>>
>>
>> The brightness may be able to change its brightness in two separate LEDs case as you see.
>> So, I think it would be better to write brightness setting in strobe_op.
>
> Could you motivate your statement, please? Why would it be better?
>
>> In consideration of delay, of course, the brightness is set just when it would be changed.
>
> I think that joint iout arrangement will be prevailing - this is the
> case for your board, isn't it? With the modification I am proposing
> the gain is clear.
>
You're right, thanks.
Did you mean that flash attributes should be written
on their ops(flash brightness, flash timeout)?
let me update the driver on your suggestion.
>>>>>> +
>>>>>> + 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);
>>>>>> +
>>>>>> + mutex_lock(&led->lock);
>>>>>> + sub_led->flash_timeout = timeout;
>>>>>> + mutex_unlock(&led->lock);
>>>>>
>>>>> Ditto.
>>>>>
>>>
>>> Timeout should be also written here.
>>>
>>
>> The timeout may be able to change its flash timeout in two separate LEDs case as you see.
>> So, I think it would be better to write timeout setting in strobe_op.
>> In consideration of delay, of course, the timeout is set just when it would be changed.
>>
>>> If you will add regmap_write in both ops, then mutex protection will
>>> have to be preserved, to assure consistency between registers state
>>> and sub_led->flash_brightness and sub_led->flash_timeout state.
>>>
>>
>> Thanks, but mutex protection is useless in this case.
>> so I try to remove it.
>>
>>>>
>>>>>> +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60
>>>>>
>>>>> Rename DEF to MASK.
>>>
>>> Hmm, here it should be: Rename MAX to MASK.
>>>
>>
>> Oh
>> Okay, Thanks :)
>>
>
>
On 12/01/2015 02:54 AM, Ingi Kim wrote:
> Hi Jacek,
>
> On 2015년 11월 30일 19:59, Jacek Anaszewski wrote:
>> Hi Ingi,
>>
>> On 11/30/2015 03:31 AM, Ingi Kim wrote:
>>> Hi Jacek,
>>>
>>> On 2015년 11월 26일 18:43, Jacek Anaszewski wrote:
>>>> Hi Ingi,
>>>>
>>>> On 11/26/2015 09:02 AM, Ingi Kim wrote:
>>>> [...]
>>>>>>> +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);
>>>>>>> +
>>>>>>> + mutex_lock(&led->lock);
>>>>>>> + sub_led->flash_brightness = brightness;
>>>>>>> + mutex_unlock(&led->lock);
>>>>>>
>>>>>> Mutex protection is redundant in this case. You would need it if device
>>>>>> state was also changed here.
>>>>>
>>>>> Okay, I'll remove it.
>>>>>
>>>>>>
>>>>>> BTW why flash brightness can't be written to the device here?
>>>>>>
>>>>>
>>>>> Flash brightness is only affected when FLED flashed (strobing).
>>>>> So, I think it is better to be written in rt5033_led_flash_strobe_set function.
>>>>
>>>> strobe_set op should strobe the flash ASAP, and delegating brightness
>>>> setting there extends a delay between calling strobe_set op
>>>> and actual flash strobe. If you set the brightness here, then you
>>>> wouldn't have to do that in the strobe_set op, of course unless the
>>>> the brightness is altered through the API of the other LED, in two
>>>> separate LEDs case.
>>>>
>>>
>>> The brightness may be able to change its brightness in two separate LEDs case as you see.
>>> So, I think it would be better to write brightness setting in strobe_op.
>>
>> Could you motivate your statement, please? Why would it be better?
>>
>>> In consideration of delay, of course, the brightness is set just when it would be changed.
>>
>> I think that joint iout arrangement will be prevailing - this is the
>> case for your board, isn't it? With the modification I am proposing
>> the gain is clear.
>>
>
> You're right, thanks.
> Did you mean that flash attributes should be written
> on their ops(flash brightness, flash timeout)?
Both in those ops and conditionally in the strobe_set op,
in order to handle two LEDs case, when the other LED has
altered any of the shared settings.
> let me update the driver on your suggestion.
>
>>>>>>> +
>>>>>>> + 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);
>>>>>>> +
>>>>>>> + mutex_lock(&led->lock);
>>>>>>> + sub_led->flash_timeout = timeout;
>>>>>>> + mutex_unlock(&led->lock);
>>>>>>
>>>>>> Ditto.
>>>>>>
>>>>
>>>> Timeout should be also written here.
>>>>
>>>
>>> The timeout may be able to change its flash timeout in two separate LEDs case as you see.
>>> So, I think it would be better to write timeout setting in strobe_op.
>>> In consideration of delay, of course, the timeout is set just when it would be changed.
>>>
>>>> If you will add regmap_write in both ops, then mutex protection will
>>>> have to be preserved, to assure consistency between registers state
>>>> and sub_led->flash_brightness and sub_led->flash_timeout state.
>>>>
>>>
>>> Thanks, but mutex protection is useless in this case.
>>> so I try to remove it.
>>>
>>>>>
>>>>>>> +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60
>>>>>>
>>>>>> Rename DEF to MASK.
>>>>
>>>> Hmm, here it should be: Rename MAX to MASK.
>>>>
>>>
>>> Oh
>>> Okay, Thanks :)
>>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
--
Best Regards,
Jacek Anaszewski