Re: [PATCH 2/2] leds: rt5033: Add RT5033 Flash led device driver
Hello Lee Jones,
Thanks for the review.
I'll try to divide this patch and change name and comment.
Then push ver2 patch soon.
On 2015년 10월 05일 16:21, Lee Jones wrote:
> On Fri, 02 Oct 2015, Ingi Kim wrote:
>
>> This patch adds device driver of Richtek RT5033 PMIC.
>> The driver supports a current regulated output to drive
>> white LEDs for camera flash.
>>
>> Signed-off-by: Ingi Kim
>> ---
>> drivers/leds/Kconfig | 8 ++
>> drivers/leds/Makefile | 1 +
>> drivers/leds/leds-rt5033.c | 222
>> +
>> drivers/mfd/rt5033.c | 3 +
>> include/linux/mfd/rt5033-private.h | 67 +--
>> include/linux/mfd/rt5033.h | 27 -
>
> Please pull the MFD changes out into to separate patch(es).
>
>> 6 files changed, 317 insertions(+), 11 deletions(-)
>> create mode 100644 drivers/leds/leds-rt5033.c
>
> [...]
>
>> diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
>> index d60f916..035421c 100644
>> --- a/drivers/mfd/rt5033.c
>> +++ b/drivers/mfd/rt5033.c
>> @@ -47,6 +47,9 @@ static const struct mfd_cell rt5033_devs[] = {
>> }, {
>> .name = "rt5033-battery",
>> .of_compatible = "richtek,rt5033-battery",
>> +}, {
>> +.name = "rt5033-led",
>> +.of_compatible = "richtek,rt5033-led",
>> },
>> };
>
> Needs to be in a patch of its own.
>
>> diff --git a/include/linux/mfd/rt5033-private.h
>> b/include/linux/mfd/rt5033-private.h
>> index 1b63fc2..21c3aff 100644
>> --- a/include/linux/mfd/rt5033-private.h
>> +++ b/include/linux/mfd/rt5033-private.h
>> @@ -25,15 +25,15 @@ enum rt5033_reg {
>> /* Reserved 0x09~0x18 */
>> RT5033_REG_RT_CTRL1 = 0x19,
>> /* Reserved 0x1A~0x20 */
>> -RT5033_REG_FLED_FUNCTION1 = 0x21,
>> -RT5033_REG_FLED_FUNCTION2 = 0x22,
>> -RT5033_REG_FLED_STROBE_CTRL1= 0x23,
>> -RT5033_REG_FLED_STROBE_CTRL2= 0x24,
>> -RT5033_REG_FLED_CTRL1 = 0x25,
>> -RT5033_REG_FLED_CTRL2 = 0x26,
>> -RT5033_REG_FLED_CTRL3 = 0x27,
>> -RT5033_REG_FLED_CTRL4 = 0x28,
>> -RT5033_REG_FLED_CTRL5 = 0x29,
>> +RT5033_REG_FL_FUNCTION1 = 0x21,
>> +RT5033_REG_FL_FUNCTION2 = 0x22,
>> +RT5033_REG_FL_STROBE_CTRL1 = 0x23,
>> +RT5033_REG_FL_STROBE_CTRL2 = 0x24,
>> +RT5033_REG_FL_CTRL1 = 0x25,
>> +RT5033_REG_FL_CTRL2 = 0x26,
>> +RT5033_REG_FL_CTRL3 = 0x27,
>> +RT5033_REG_FL_CTRL4 = 0x28,
>> +RT5033_REG_FL_CTRL5 = 0x29,
>
> Why this change?
>
> The previous naming convention was better.
>
>> /* Reserved 0x2A~0x40 */
>> RT5033_REG_CTRL = 0x41,
>> RT5033_REG_BUCK_CTRL= 0x42,
>> @@ -93,6 +93,55 @@ enum rt5033_reg {
>> #define RT5033_RT_CTRL1_UUG_MASK0x02
>> #define RT5033_RT_HZ_MASK 0x01
>>
>> +/* RT5033 FLED Function1 register */
>> +#define RT5033_FL_FLED1_MASK0x94
>> +#define RT5033_FL_STROBE_SEL0x04
>> +#define RT5033_FL_PIN_CTRL 0x10
>> +#define RT5033_FL_RESET 0x80
>> +
>> +/* RT5033 FLED Function2 register */
>> +#define RT5033_FL_FLED2_MASK0x81
>> +#define RT5033_FL_ENFLED0x01
>> +#define RT5033_FL_SREG_STROBE 0x80
>> +
>> +/* RT5033 FLED Strobe Control1 */
>> +#define RT5033_FL_STRBCTRL1_MASK0xFF
>> +#define RT5033_FL_STRBCTRL1_TM_CUR_MAX 0xE0
>> +#define RT5033_FL_STRBCTRL1_FL_CUR_DEF 0x0D
>> +#define RT5033_FL_STRBCTRL1_FL_CUR_MAX 0x1F
>> +
>> +/* RT5033 FLED Strobe Control2 */
>> +#define RT5033_FL_STRBCTRL2_MASK0x3F
>> +#define RT5033_FL_STRBCTRL2_TM_DEF 0x0F
>> +#define RT5033_FL_STRBCTRL2_TM_MAX 0x3F
>> +
>> +/* RT5033 FLED Control1 */
>> +#define RT5033_FL_CTRL1_MASK0xF7
>> +#define RT5033_FL_CTRL1_TORCH_CUR_DEF 0x20
>> +#define RT5033_FL_CTRL1_TORCH_CUR_MAX 0xF0
>> +#define RT5033_FL_CTRL1_LBP_DEF 0x02
>> +#define RT5033_FL_CTRL1_LBP_MAX 0x07
>> +
>> +/* RT5033 FLED Control2 */
>> +#define RT5033_FL_CTRL2_MASK0xFF
>> +#define RT5033_FL_CTRL2_VMID_DEF0x37
>> +#define RT5033_FL_CTRL2_VMID_MAX0x3F
>> +#define RT5033_FL_CTRL2_TRACK_ALIVE 0x40
>> +#define RT5033_FL_CTRL2_MID_TRACK 0x80
>> +
>> +/* RT5033 FLED Control4 */
>> +#define RT5033_FL_CTRL4_MASK0xE0
>> +#define RT5033_FL_CTRL4_RESV0x14
>> +#define RT5033_FL_CTRL4_VTRREG_DEF 0x40
>> +#define RT5033_FL_CTRL4_VTRREG_MAX 0x60
>> +#define RT5033_FL_CTRL4_TRACK_CLK 0x80
>> +
>> +/* RT5033 FLED Control5 */
>> +#define RT5033_FL_CTRL5_MASK0xC0
>> +#define RT5033_FL_CTRL5_RESV0x10
>> +#define RT5033_FL_CTRL5_TA_GOOD 0x40
>> +#define RT5033_FL_CTRL5_TA_EXIST0x80
>> +
>> /* RT5033 control register */
>> #define R
Re: [PATCH 2/2] leds: rt5033: Add RT5033 Flash led device driver
On Fri, 02 Oct 2015, Ingi Kim wrote:
> This patch adds device driver of Richtek RT5033 PMIC.
> The driver supports a current regulated output to drive
> white LEDs for camera flash.
>
> Signed-off-by: Ingi Kim
> ---
> drivers/leds/Kconfig | 8 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-rt5033.c | 222
> +
> drivers/mfd/rt5033.c | 3 +
> include/linux/mfd/rt5033-private.h | 67 +--
> include/linux/mfd/rt5033.h | 27 -
Please pull the MFD changes out into to separate patch(es).
> 6 files changed, 317 insertions(+), 11 deletions(-)
> create mode 100644 drivers/leds/leds-rt5033.c
[...]
> diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
> index d60f916..035421c 100644
> --- a/drivers/mfd/rt5033.c
> +++ b/drivers/mfd/rt5033.c
> @@ -47,6 +47,9 @@ static const struct mfd_cell rt5033_devs[] = {
> }, {
> .name = "rt5033-battery",
> .of_compatible = "richtek,rt5033-battery",
> + }, {
> + .name = "rt5033-led",
> + .of_compatible = "richtek,rt5033-led",
> },
> };
Needs to be in a patch of its own.
> diff --git a/include/linux/mfd/rt5033-private.h
> b/include/linux/mfd/rt5033-private.h
> index 1b63fc2..21c3aff 100644
> --- a/include/linux/mfd/rt5033-private.h
> +++ b/include/linux/mfd/rt5033-private.h
> @@ -25,15 +25,15 @@ enum rt5033_reg {
> /* Reserved 0x09~0x18 */
> RT5033_REG_RT_CTRL1 = 0x19,
> /* Reserved 0x1A~0x20 */
> - RT5033_REG_FLED_FUNCTION1 = 0x21,
> - RT5033_REG_FLED_FUNCTION2 = 0x22,
> - RT5033_REG_FLED_STROBE_CTRL1= 0x23,
> - RT5033_REG_FLED_STROBE_CTRL2= 0x24,
> - RT5033_REG_FLED_CTRL1 = 0x25,
> - RT5033_REG_FLED_CTRL2 = 0x26,
> - RT5033_REG_FLED_CTRL3 = 0x27,
> - RT5033_REG_FLED_CTRL4 = 0x28,
> - RT5033_REG_FLED_CTRL5 = 0x29,
> + RT5033_REG_FL_FUNCTION1 = 0x21,
> + RT5033_REG_FL_FUNCTION2 = 0x22,
> + RT5033_REG_FL_STROBE_CTRL1 = 0x23,
> + RT5033_REG_FL_STROBE_CTRL2 = 0x24,
> + RT5033_REG_FL_CTRL1 = 0x25,
> + RT5033_REG_FL_CTRL2 = 0x26,
> + RT5033_REG_FL_CTRL3 = 0x27,
> + RT5033_REG_FL_CTRL4 = 0x28,
> + RT5033_REG_FL_CTRL5 = 0x29,
Why this change?
The previous naming convention was better.
> /* Reserved 0x2A~0x40 */
> RT5033_REG_CTRL = 0x41,
> RT5033_REG_BUCK_CTRL= 0x42,
> @@ -93,6 +93,55 @@ enum rt5033_reg {
> #define RT5033_RT_CTRL1_UUG_MASK 0x02
> #define RT5033_RT_HZ_MASK0x01
>
> +/* RT5033 FLED Function1 register */
> +#define RT5033_FL_FLED1_MASK 0x94
> +#define RT5033_FL_STROBE_SEL 0x04
> +#define RT5033_FL_PIN_CTRL 0x10
> +#define RT5033_FL_RESET 0x80
> +
> +/* RT5033 FLED Function2 register */
> +#define RT5033_FL_FLED2_MASK 0x81
> +#define RT5033_FL_ENFLED 0x01
> +#define RT5033_FL_SREG_STROBE0x80
> +
> +/* RT5033 FLED Strobe Control1 */
> +#define RT5033_FL_STRBCTRL1_MASK 0xFF
> +#define RT5033_FL_STRBCTRL1_TM_CUR_MAX 0xE0
> +#define RT5033_FL_STRBCTRL1_FL_CUR_DEF 0x0D
> +#define RT5033_FL_STRBCTRL1_FL_CUR_MAX 0x1F
> +
> +/* RT5033 FLED Strobe Control2 */
> +#define RT5033_FL_STRBCTRL2_MASK 0x3F
> +#define RT5033_FL_STRBCTRL2_TM_DEF 0x0F
> +#define RT5033_FL_STRBCTRL2_TM_MAX 0x3F
> +
> +/* RT5033 FLED Control1 */
> +#define RT5033_FL_CTRL1_MASK 0xF7
> +#define RT5033_FL_CTRL1_TORCH_CUR_DEF0x20
> +#define RT5033_FL_CTRL1_TORCH_CUR_MAX0xF0
> +#define RT5033_FL_CTRL1_LBP_DEF 0x02
> +#define RT5033_FL_CTRL1_LBP_MAX 0x07
> +
> +/* RT5033 FLED Control2 */
> +#define RT5033_FL_CTRL2_MASK 0xFF
> +#define RT5033_FL_CTRL2_VMID_DEF 0x37
> +#define RT5033_FL_CTRL2_VMID_MAX 0x3F
> +#define RT5033_FL_CTRL2_TRACK_ALIVE 0x40
> +#define RT5033_FL_CTRL2_MID_TRACK0x80
> +
> +/* RT5033 FLED Control4 */
> +#define RT5033_FL_CTRL4_MASK 0xE0
> +#define RT5033_FL_CTRL4_RESV 0x14
> +#define RT5033_FL_CTRL4_VTRREG_DEF 0x40
> +#define RT5033_FL_CTRL4_VTRREG_MAX 0x60
> +#define RT5033_FL_CTRL4_TRACK_CLK0x80
> +
> +/* RT5033 FLED Control5 */
> +#define RT5033_FL_CTRL5_MASK 0xC0
> +#define RT5033_FL_CTRL5_RESV 0x10
> +#define RT5033_FL_CTRL5_TA_GOOD 0x40
> +#define RT5033_FL_CTRL5_TA_EXIST 0x80
> +
> /* RT5033 control register */
> #define RT5033_CTRL_FCCM_BUCK_MASK 0x00
> #define RT5033_CTRL_BUCKOMS_MASK 0x01
> diff --git a/include/linux/mfd/rt5033.h b/include/linux/mfd/rt5033.h
> index 6cff5cf..45c97b7 100644
> --- a/include/linux/mfd/rt5033.h
> +++ b/include/linux/mfd/rt5033.h
> @@ -12,10 +12,11 @@
> #ifndef __RT5033_H__
> #de
Re: [PATCH 2/2] leds: rt5033: Add RT5033 Flash led device driver
Hi Ingi, [auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore] config: i386-allmodconfig (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=i386 All error/warnings (new ones prefixed by >>): In file included from include/linux/list.h:8:0, from include/linux/kobject.h:20, from include/linux/device.h:17, from include/linux/leds.h:15, from include/linux/led-class-flash.h:15, from include/linux/mfd/rt5033.h:15, from drivers/leds/leds-rt5033.c:13: drivers/leds/leds-rt5033.c: In function 'flcdev_to_led': >> include/linux/kernel.h:811:27: error: 'struct rt5033_led' has no member >> named 'fled_cdev' const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ >> drivers/leds/leds-rt5033.c:28:9: note: in expansion of macro 'container_of' return container_of(fled_cdev, struct rt5033_led, fled_cdev); ^ include/linux/kernel.h:811:48: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types] const typeof( ((type *)0)->member ) *__mptr = (ptr); \ ^ >> drivers/leds/leds-rt5033.c:28:9: note: in expansion of macro 'container_of' return container_of(fled_cdev, struct rt5033_led, fled_cdev); ^ In file included from include/linux/compiler.h:56:0, from include/linux/ioport.h:12, from include/linux/device.h:16, from include/linux/leds.h:15, from include/linux/led-class-flash.h:15, from include/linux/mfd/rt5033.h:15, from drivers/leds/leds-rt5033.c:13: >> include/linux/compiler-gcc.h:158:2: error: 'struct rt5033_led' has no member >> named 'fled_cdev' __builtin_offsetof(a, b) ^ include/linux/stddef.h:16:32: note: in expansion of macro '__compiler_offsetof' #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER) ^ include/linux/kernel.h:812:29: note: in expansion of macro 'offsetof' (type *)( (char *)__mptr - offsetof(type,member) );}) ^ >> drivers/leds/leds-rt5033.c:28:9: note: in expansion of macro 'container_of' return container_of(fled_cdev, struct rt5033_led, fled_cdev); ^ drivers/leds/leds-rt5033.c: In function 'rt5033_led_brightness_set': >> drivers/leds/leds-rt5033.c:42:8: error: 'RT5033_FL_FUNC1_MASK' undeclared >> (first use in this function) RT5033_FL_FUNC1_MASK, RT5033_FL_PINCTRL); ^ drivers/leds/leds-rt5033.c:42:8: note: each undeclared identifier is reported only once for each function it appears in >> drivers/leds/leds-rt5033.c:42:30: error: 'RT5033_FL_PINCTRL' undeclared >> (first use in this function) RT5033_FL_FUNC1_MASK, RT5033_FL_PINCTRL); ^ drivers/leds/leds-rt5033.c: In function 'rt5033_init_flash_timeout': >> drivers/leds/leds-rt5033.c:56:16: error: 'struct rt5033_led' has no member >> named 'fled_cdev' setting = &led->fled_cdev.timeout; ^ >> drivers/leds/leds-rt5033.c:58:20: error: 'struct rt5033_led' has no member >> named 'data' setting->max = led->data->flash_max_timeout; ^ drivers/leds/leds-rt5033.c:60:20: error: 'struct rt5033_led' has no member named 'data' setting->val = led->data->flash_max_timeout; ^ drivers/leds/leds-rt5033.c: In function 'rt5033_led_parse_dt': drivers/leds/leds-rt5033.c:83:5: error: 'struct rt5033_led' has no member named 'fled_cdev' led->fled_cdev.led_cdev.name = ^ drivers/leds/leds-rt5033.c:106:5: error: 'struct rt5033_led' has no member named 'data' led->data = data; ^ drivers/leds/leds-rt5033.c: In function 'rt5033_led_flash_strobe_set': drivers/leds/leds-rt5033.c:124:7: error: 'RT5033_FL_FUNC1_MASK' undeclared (first use in this function) RT5033_FL_FUNC1_MASK, RT5033_FL_RESET); ^ >> drivers/leds/leds-rt5033.c:133:30: error: 'RT5033_FL_STRB_SEL' undeclared >> (first use in this function) RT5033_FL_FUNC1_MASK, RT5033_FL_STRB_SEL ^ drivers/leds/leds-rt5033.c:134:10: error: 'RT5033_FL_PINCTRL' undeclared (first use in this function) | RT5033_FL_PINCTRL); ^ >> drivers/leds/leds-rt5033.c:136:8: error: 'RT5033_FL_FUNC2_MASK' undeclared >> (first use in this function) RT5033_FL_FUNC2_MASK, RT5033_FL_ENFLED ^ >> drivers/leds/leds-rt5033.c:137:10: error: 'RT5033_FL_SREG_STRB' undeclared >> (first use in this function) | RT5033_FL_SREG_STRB); ^ drivers/leds/leds-rt5033.c: In function 'rt5033_led_probe': drivers/leds/leds-rt5033.c:168
[PATCH 2/2] leds: rt5033: Add RT5033 Flash led device driver
This patch adds device driver of Richtek RT5033 PMIC.
The driver supports a current regulated output to drive
white LEDs for camera flash.
Signed-off-by: Ingi Kim
---
drivers/leds/Kconfig | 8 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-rt5033.c | 222 +
drivers/mfd/rt5033.c | 3 +
include/linux/mfd/rt5033-private.h | 67 +--
include/linux/mfd/rt5033.h | 27 -
6 files changed, 317 insertions(+), 11 deletions(-)
create mode 100644 drivers/leds/leds-rt5033.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 42990f2..29613e3 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 b503f92..bcc4d93 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 000..b154676
--- /dev/null
+++ b/drivers/leds/leds-rt5033.c
@@ -0,0 +1,222 @@
+/*
+ * led driver for RT5033
+ *
+ * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
+ * Ingi Kim
+ *
+ * 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
+#include
+#include
+#include
+
+#define RT5033_LED_FLASH_TIMEOUT_MIN 64000
+#define RT5033_LED_FLASH_TIMEOUT_STEPS 32000
+#define RT5033_LED_TORCH_CURRENT_LEVEL_MAX 16
+
+/* Macro for getting offset of flash timeout */
+#define GET_TIMEOUT_OFFSET(tm, step) ((tm) / (step) - 2)
+
+static struct rt5033_led *flcdev_to_led(
+ struct led_classdev_flash *fled_cdev)
+{
+ return container_of(fled_cdev, struct rt5033_led, fled_cdev);
+}
+
+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_led *led = flcdev_to_led(fled_cdev);
+
+ if (!brightness) {
+ regmap_update_bits(led->regmap, RT5033_REG_FL_FUNCTION2,
+ RT5033_FL_CTRL2_MASK, 0x0);
+ } else {
+ regmap_update_bits(led->regmap, RT5033_REG_FL_FUNCTION1,
+ RT5033_FL_FUNC1_MASK, RT5033_FL_PINCTRL);
+ regmap_update_bits(led->regmap, RT5033_REG_FL_CTRL1,
+ RT5033_FL_CTRL1_MASK, (brightness-1) << 4);
+ regmap_update_bits(led->regmap, RT5033_REG_FL_FUNCTION2,
+ RT5033_FL_CTRL2_MASK, RT5033_FL_ENFLED);
+ }
+
+ return 0;
+}
+
+static void rt5033_init_flash_timeout(struct rt5033_led *led)
+{
+ struct led_flash_setting *setting;
+
+ setting = &led->fled_cdev.timeout;
+ setting->min = RT5033_LED_FLASH_TIMEOUT_MIN;
+ setting->max = led->data->flash_max_timeout;
+ setting->step = RT5033_LED_FLASH_TIMEOUT_STEPS;
+ setting->val = led->data->flash_max_timeout;
+}
+
+static int rt5033_led_parse_dt(struct rt5033_led *led, struct device *dev)
+{
+ struct device_node *np = dev->of_node;
+ struct device_node *child_node;
+ struct rt5033_led_config_data *data;
+ int ret = 0;
+
+ if (!np)
+ return -ENXIO;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ child_node = of_get_next_available_child(np, NULL);
+ if (!child_node) {
+ dev_err(dev, "DT child node isn't found\n");
+ return -EINVAL;
+ }
+
+ led->fled_cdev.led_cdev.name =
+ of_get_property(child_node, "label", NULL) ? : child_node->name;
+
+ ret = of_property_read_u32(child_node, "led-max-microamp",
+
