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

[Lee Jones <lee.jones@linaro.org>]

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 <ingi2.kim@samsung.com>
> >> ---
> >>   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