From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753291AbbJEKg6 (ORCPT ); Mon, 5 Oct 2015 06:36:58 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:45347 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753206AbbJEKgy (ORCPT ); Mon, 5 Oct 2015 06:36:54 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: cbfee68e-f791c6d000001498-0b-561252c34d77 Content-transfer-encoding: 8BIT Message-id: <561252C4.7@samsung.com> Date: Mon, 05 Oct 2015 19:36:52 +0900 From: Ingi Kim User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Lee Jones Cc: robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, rpurdie@rpsys.net, j.anaszewski@samsung.com, sameo@linux.intel.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org Subject: Re: [PATCH 2/2] leds: rt5033: Add RT5033 Flash led device driver References: <1443778875-18261-1-git-send-email-ingi2.kim@samsung.com> <1443778875-18261-3-git-send-email-ingi2.kim@samsung.com> <20151005072129.GE3377@x1> In-reply-to: <20151005072129.GE3377@x1> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrNIsWRmVeSWpSXmKPExsWyRsSkWPdIkFCYwYEmFov5R86xWvS/Wchq ce7VSkaL3qvPGS3ufz3KaHF51xw2i61v1jFaLL1+kcliwvS1LBate4+wW+ze9ZTV4nQ3qwOP x5p5axg9Lvf1MnmsXP6FzWPTqk42jzvX9rB5zDsZ6LFn/g9Wj74tqxg9Pm+SC+CM4rJJSc3J LEst0rdL4Mrof/ieuaBHv+LOnO0sDYzT1boYOTkkBEwkptzfxwRhi0lcuLeerYuRi0NIYAWj xOqHR5hhiuaeWMoOkZjFKDGleS4LSIJXQFDix+R7QDYHB7OAvMSRS9kQprrElCm5EOUPGCUm T+lnhyhXkfi1upMJpIZFQFXib7c+SJhNQE3ixoqFYDeICiRIHD/7A6xEBKj83BtzkDHMAlOZ JO6+7QKrERbwlPg24RErxPx5jBKnDs9iBUlwAu2d+/0uC0hCQmAih8SkhdfYQBIsAgIS3yYf ArtTQkBWYtMBqL8kJQ6uuMEygVFsFpJvZiF8MwvhmwWMzKsYRVMLkguKk9KLjPSKE3OLS/PS 9ZLzczcxAuP49L9nfTsYbx6wPsQowMGoxMMrkSQYJsSaWFZcmXuI0RTohonMUqLJ+cBkkVcS b2hsZmRhamJqbGRuaaYkzpsg9TNYSCA9sSQ1OzW1ILUovqg0J7X4ECMTB6dUA+OChmu/Jr12 YXMp+XPUhk8y59JBJv4ai9Q9vveKpVKXm5ySjZF/+FJA1PeEnhtTa//3YiGRjafDmfrXGs5e ffzPlpCWV+JBEy+vWZVTNdM9eEHsw195MpvfVSY+vZDNqG6//O/GroplhvVeB9Y2BLhNnmD4 6nCDQP3agOT2/vRJIZmbr7s1cCqxFGckGmoxFxUnAgDJzVch3gIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprBKsWRmVeSWpSXmKPExsVy+t9jQd3DQUJhBj+fW1rMP3KO1aL/zUJW i3OvVjJa9F59zmhx/+tRRovLu+awWWx9s47RYun1i0wWE6avZbFo3XuE3WL3rqesFqe7WR14 PNbMW8Pocbmvl8lj5fIvbB6bVnWyedy5tofNY97JQI8983+wevRtWcXo8XmTXABnVAOjTUZq YkpqkUJqXnJ+SmZeuq2Sd3C8c7ypmYGhrqGlhbmSQl5ibqqtkotPgK5bZg7QyUoKZYk5pUCh gMTiYiV9O0wTQkPcdC1gGiN0fUOC4HqMDNBAwhrGjP6H75kLevQr7szZztLAOF2ti5GTQ0LA RGLuiaXsELaYxIV769m6GLk4hARmMUpMaZ7LApLgFRCU+DH5HpDNwcEsIC9x5FI2hKkuMWVK LkT5A0aJyVP62SHKVSR+re5kAqlhEVCV+NutDxJmE1CTuLFiIROILSqQIHH87A+wEhGg8nNv zEHGMAtMZZK4+7YLrEZYwFPi24RHrBDz5zFKnDo8ixUkwQm0d+73uywTGIGORLhuFsJ1sxCu W8DIvIpRIrUguaA4KT3XMC+1XK84Mbe4NC9dLzk/dxMjOFU8k9rBeHCX+yFGAQ5GJR7eA/GC YUKsiWXFlbmHGCU4mJVEeC1dhcKEeFMSK6tSi/Lji0pzUosPMZoCvTeRWUo0OR+YxvJK4g2N TcyMLI3MDS2MjM2VxHlvHGIIExJITyxJzU5NLUgtgulj4uCUamBc4FzOLPTULHdTBWP3lO3S gvNeHbhsHap5/L4X469fPt6611pyOxtd1OKazYPvvj5fqbRnubF8zJ5YPdvF52RDeDlFo78L GFhyMLMYb58u+kzkecQNyc8HVm57Oy/kip387PRSgeDcrHoZtffRr0+3ZUs7MB7Wm9Kxb2XX 5HM1190+8q8/PkGJpTgj0VCLuag4EQAZvgIqKwMAAA== DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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_MASK 0x02 >> #define RT5033_RT_HZ_MASK 0x01 >> >> +/* 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_STROBE 0x80 >> + >> +/* 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_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_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_TRACK 0x80 >> + >> +/* 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_CLK 0x80 >> + >> +/* 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__ >> #define __RT5033_H__ >> >> -#include >> +#include >> #include >> -#include >> #include >> +#include >> +#include >> >> /* RT5033 regulator IDs */ >> enum rt5033_regulators { >> @@ -59,4 +60,26 @@ struct rt5033_charger { >> struct rt5033_charger_data *chg; >> }; >> >> +/* RT5033 led platform data */ >> + >> +struct rt5033_led_config_data { >> + /* maximum LED current in movie mode */ >> + u32 torch_max_microamp; >> + /* maximum LED current in flash mode */ >> + u32 flash_max_microamp; >> + /* maximum flash timeout */ >> + u32 flash_max_timeout; >> + /* max LED brightness level */ >> + enum led_brightness max_brightness; >> +}; > > Rid these comments. Use kerneldoc format instead. > >> +struct rt5033_led { >> + struct device *dev; >> + struct rt5033_dev *rt5033; >> + struct regmap *regmap; >> + >> + /* Related LED class device */ >> + struct led_classdev cdev; >> +}; >> + >> #endif /* __RT5033_H__ */ >