| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20151126091243.GQ12874@x1>
Date: Thu, 26 Nov 2015 09:12:43 +0000
From: Lee Jones <lee.jones@...aro.org>
To: Ingi Kim <ingi2.kim@...sung.com>
Cc: Jacek Anaszewski <j.anaszewski@...sung.com>, robh+dt@...nel.org,
pawel.moll@....com, mark.rutland@....com,
ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
sameo@...ux.intel.com, rpurdie@...ys.net, inki.dae@...sung.com,
sw0312.kim@...sung.com, beomho.seo@...sung.com,
andi.shyti@...sung.com, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH v6 2/2] leds: rt5033: Add RT5033 Flash led device driver
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@...sung.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists