Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753339AbbKZJM4 (ORCPT ); Thu, 26 Nov 2015 04:12:56 -0500 Received: from mail-wm0-f50.google.com ([74.125.82.50]:36011 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752823AbbKZJMs (ORCPT ); Thu, 26 Nov 2015 04:12:48 -0500 Date: Thu, 26 Nov 2015 09:12:43 +0000 From: Lee Jones To: Ingi Kim Cc: Jacek Anaszewski , robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, sameo@linux.intel.com, rpurdie@rpsys.net, inki.dae@samsung.com, sw0312.kim@samsung.com, beomho.seo@samsung.com, andi.shyti@samsung.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org Subject: Re: [PATCH v6 2/2] leds: rt5033: Add RT5033 Flash led device driver Message-ID: <20151126091243.GQ12874@x1> References: <1448446948-13729-1-git-send-email-ingi2.kim@samsung.com> <1448446948-13729-3-git-send-email-ingi2.kim@samsung.com> <5655D001.8090803@samsung.com> <5656BC88.2070603@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5656BC88.2070603@samsung.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5945 Lines: 200 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 > >> --- > >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/