Re: [PATCH 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver
Hi Jacek, On 8 May 2018 at 04:13, Jacek Anaszewski wrote: > Hi Baolin, > > Thank you for the patch. Generally this is a nice and clean driver, > but I noticed some locking related issues and one detail > regarding LED naming. Please refer below. > > > On 05/04/2018 12:08 PM, Baolin Wang wrote: >> >> From: Xiaotong Lu >> >> This patch adds Spreadtrum SC27xx PMIC series breathing light controller >> driver, which can support 3 LEDs. Each LED can work at normal PWM mode >> and breathing mode. >> >> Signed-off-by: Xiaotong Lu >> Signed-off-by: Baolin Wang >> --- >> drivers/leds/Kconfig| 11 ++ >> drivers/leds/Makefile |1 + >> drivers/leds/leds-sc27xx-bltc.c | 369 >> +++ >> 3 files changed, 381 insertions(+) >> create mode 100644 drivers/leds/leds-sc27xx-bltc.c >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index 2c896c0..319449b 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -647,6 +647,17 @@ config LEDS_IS31FL32XX >> LED controllers. They are I2C devices with multiple >> constant-current >> channels, each with independent 256-level PWM control. >> +config LEDS_SC27XX_BLTC >> + tristate "LED support for the SC27xx breathing light controller" >> + depends on LEDS_CLASS && MFD_SC27XX_PMIC >> + depends on OF >> + help >> + Say Y here to include support for the SC27xx breathing light >> controller >> + LEDs. >> + >> + This driver can also be built as a module. If so the module will >> be >> + called leds-sc27xx-bltc. >> + >> comment "LED driver for blink(1) USB RGB LED is under Special HID >> drivers (HID_THINGM)" >> config LEDS_BLINKM >> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >> index 91eca81..ff6917e 100644 >> --- a/drivers/leds/Makefile >> +++ b/drivers/leds/Makefile >> @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o >> obj-$(CONFIG_LEDS_NIC78BX)+= leds-nic78bx.o >> obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o >> obj-$(CONFIG_LEDS_LM3692X)+= leds-lm3692x.o >> +obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o >> # LED SPI Drivers >> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o >> diff --git a/drivers/leds/leds-sc27xx-bltc.c >> b/drivers/leds/leds-sc27xx-bltc.c >> new file mode 100644 >> index 000..0016a87 >> --- /dev/null >> +++ b/drivers/leds/leds-sc27xx-bltc.c >> @@ -0,0 +1,369 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2018 Spreadtrum Communications Inc. >> + */ > > > Please use uniform "//" comment style here. > > >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* PMIC global control register definition */ >> +#define SC27XX_MODULE_EN0 0xc08 >> +#define SC27XX_CLK_EN0 0xc18 >> +#define SC27XX_RGB_CTRL0xebc >> + >> +#define SC27XX_BLTC_EN BIT(9) >> +#define SC27XX_RTC_EN BIT(7) >> +#define SC27XX_RGB_PD BIT(0) >> + >> +/* Breathing light controller register definition */ >> +#define SC27XX_LEDS_CTRL 0x00 >> +#define SC27XX_LEDS_PRESCALE 0x04 >> +#define SC27XX_LEDS_DUTY 0x08 >> +#define SC27XX_LEDS_CURVE0 0x0c >> +#define SC27XX_LEDS_CURVE1 0x10 >> + >> +#define SC27XX_CTRL_SHIFT 4 >> +#define SC27XX_LED_RUN BIT(0) >> +#define SC27XX_LED_TYPEBIT(1) >> + >> +#define SC27XX_DUTY_SHIFT 8 >> +#define SC27XX_DUTY_MASK GENMASK(15, 0) >> +#define SC27XX_MOD_MASKGENMASK(7, 0) >> + >> +#define SC27XX_CURVE_SHIFT 8 >> +#define SC27XX_CURVE_L_MASKGENMASK(7, 0) >> +#define SC27XX_CURVE_H_MASKGENMASK(15, 8) >> + >> +#define SC27XX_LEDS_OFFSET 0x10 >> +#define SC27XX_LEDS_MAX3 >> + >> +/* >> + * The SC27xx breathing light controller can support 3 outputs: red LED, >> + * green LED and blue LED. Each LED can support normal PWM mode and >> breathing >> + * mode. >> + * >> + * In breathing mode, the LED output curve includes raise, high, fall and >> low 4 >> + * stages, and the duration of each stage is configurable. >> + */ >> +enum sc27xx_led_config { >> + SC27XX_RAISE_TIME, >> + SC27XX_FALL_TIME, >> + SC27XX_HIGH_TIME, >> + SC27XX_LOW_TIME, >> +}; >> + >> +struct sc27xx_led { >> + struct led_classdev ldev; >> + struct sc27xx_led_priv *priv; >> + enum led_brightness value; >> + u8 line; >> + bool breath_mode; >> + bool active; >> +}; >> + >> +struct sc27xx_led_priv { >> + struct sc27xx_led leds[SC27XX_LEDS_MAX]; >> + struct regmap *regmap; >> + u32 base; >> +}; >> + >> +#define to_sc27xx_led(ldev) \ >> + container_of(ldev, struct sc27xx_led, ldev) >> + >> +static int sc27xx_led_init(struct regmap *regmap) >> +{ >> + int err; >> + >> + err = regmap_update_bits(regmap, SC27XX_MODULE_E
Re: [PATCH 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver
Hi Baolin, Thank you for the patch. Generally this is a nice and clean driver, but I noticed some locking related issues and one detail regarding LED naming. Please refer below. On 05/04/2018 12:08 PM, Baolin Wang wrote: From: Xiaotong Lu This patch adds Spreadtrum SC27xx PMIC series breathing light controller driver, which can support 3 LEDs. Each LED can work at normal PWM mode and breathing mode. Signed-off-by: Xiaotong Lu Signed-off-by: Baolin Wang --- drivers/leds/Kconfig| 11 ++ drivers/leds/Makefile |1 + drivers/leds/leds-sc27xx-bltc.c | 369 +++ 3 files changed, 381 insertions(+) create mode 100644 drivers/leds/leds-sc27xx-bltc.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 2c896c0..319449b 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -647,6 +647,17 @@ config LEDS_IS31FL32XX LED controllers. They are I2C devices with multiple constant-current channels, each with independent 256-level PWM control. +config LEDS_SC27XX_BLTC + tristate "LED support for the SC27xx breathing light controller" + depends on LEDS_CLASS && MFD_SC27XX_PMIC + depends on OF + help + Say Y here to include support for the SC27xx breathing light controller + LEDs. + + This driver can also be built as a module. If so the module will be + called leds-sc27xx-bltc. + comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)" config LEDS_BLINKM diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 91eca81..ff6917e 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o obj-$(CONFIG_LEDS_NIC78BX)+= leds-nic78bx.o obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X)+= leds-lm3692x.o +obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o # LED SPI Drivers obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c new file mode 100644 index 000..0016a87 --- /dev/null +++ b/drivers/leds/leds-sc27xx-bltc.c @@ -0,0 +1,369 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 Spreadtrum Communications Inc. + */ Please use uniform "//" comment style here. + +#include +#include +#include +#include +#include + +/* PMIC global control register definition */ +#define SC27XX_MODULE_EN0 0xc08 +#define SC27XX_CLK_EN0 0xc18 +#define SC27XX_RGB_CTRL0xebc + +#define SC27XX_BLTC_EN BIT(9) +#define SC27XX_RTC_EN BIT(7) +#define SC27XX_RGB_PD BIT(0) + +/* Breathing light controller register definition */ +#define SC27XX_LEDS_CTRL 0x00 +#define SC27XX_LEDS_PRESCALE 0x04 +#define SC27XX_LEDS_DUTY 0x08 +#define SC27XX_LEDS_CURVE0 0x0c +#define SC27XX_LEDS_CURVE1 0x10 + +#define SC27XX_CTRL_SHIFT 4 +#define SC27XX_LED_RUN BIT(0) +#define SC27XX_LED_TYPEBIT(1) + +#define SC27XX_DUTY_SHIFT 8 +#define SC27XX_DUTY_MASK GENMASK(15, 0) +#define SC27XX_MOD_MASKGENMASK(7, 0) + +#define SC27XX_CURVE_SHIFT 8 +#define SC27XX_CURVE_L_MASKGENMASK(7, 0) +#define SC27XX_CURVE_H_MASKGENMASK(15, 8) + +#define SC27XX_LEDS_OFFSET 0x10 +#define SC27XX_LEDS_MAX3 + +/* + * The SC27xx breathing light controller can support 3 outputs: red LED, + * green LED and blue LED. Each LED can support normal PWM mode and breathing + * mode. + * + * In breathing mode, the LED output curve includes raise, high, fall and low 4 + * stages, and the duration of each stage is configurable. + */ +enum sc27xx_led_config { + SC27XX_RAISE_TIME, + SC27XX_FALL_TIME, + SC27XX_HIGH_TIME, + SC27XX_LOW_TIME, +}; + +struct sc27xx_led { + struct led_classdev ldev; + struct sc27xx_led_priv *priv; + enum led_brightness value; + u8 line; + bool breath_mode; + bool active; +}; + +struct sc27xx_led_priv { + struct sc27xx_led leds[SC27XX_LEDS_MAX]; + struct regmap *regmap; + u32 base; +}; + +#define to_sc27xx_led(ldev) \ + container_of(ldev, struct sc27xx_led, ldev) + +static int sc27xx_led_init(struct regmap *regmap) +{ + int err; + + err = regmap_update_bits(regmap, SC27XX_MODULE_EN0, SC27XX_BLTC_EN, +SC27XX_BLTC_EN); + if (err) + return err; + + err = regmap_update_bits(regmap, SC27XX_CLK_EN0, SC27XX_RTC_EN, +SC27XX_RTC_EN); + if (err) + return err; + + return regmap_update_bits(regmap, SC27XX_RGB_CTRL, SC27XX_RGB_PD, 0); +} + +static u32 sc27xx_led_get_offset(struct sc27xx_led *leds) +{ + return leds->priv->base + SC27XX_LEDS_OFFSET * leds-
RE: [PATCH 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver
Hi lkp, Thanks for your advise. I will improve in next version. Best Regards, 卢小通 Xiaotong Lu CPSD Dept. Unigroup Spreadtrum &RDA Technologies Co.,Ltd. Tel: 86-21-20360600 Ext. 2496 Fax: 86-21-20360700 E-mail: xiaotong...@unisoc.com Http://www.spreadtrum.com -Original Message- From: kbuild test robot [mailto:l...@intel.com] Sent: Saturday, May 05, 2018 1:27 PM To: Baolin Wang Cc: kbuild-...@01.org; jacek.anaszew...@gmail.com; pa...@ucw.cz; robh...@kernel.org; mark.rutl...@arm.com; Xiaotong Lu (卢小通); baolin.w...@linaro.org; broo...@kernel.org; linux-l...@vger.kernel.org; devicet...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver Hi Xiaotong, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on robh/for-next] [also build test WARNING on v4.17-rc3 next-20180504] [cannot apply to j.anaszewski-leds/for-next] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Baolin-Wang/dt-bindings-leds-Add-SC27xx-breathing-light-controller-documentation/20180504-200830 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next smatch warnings: drivers/leds/leds-sc27xx-bltc.c:337 sc27xx_led_probe() warn: unsigned 'reg' is never less than zero. vim +/reg +337 drivers/leds/leds-sc27xx-bltc.c 299 300 static int sc27xx_led_probe(struct platform_device *pdev) 301 { 302 struct device *dev = &pdev->dev; 303 struct device_node *np = dev->of_node, *child; 304 struct sc27xx_led_priv *priv; 305 u32 base, count, reg; 306 int err; 307 308 count = of_get_child_count(np); 309 if (!count || count > SC27XX_LEDS_MAX) 310 return -EINVAL; 311 312 err = of_property_read_u32(np, "reg", &base); 313 if (err) { 314 dev_err(dev, "fail to get reg of property\n"); 315 return err; 316 } 317 318 priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); 319 if (!priv) 320 return -ENOMEM; 321 322 priv->base = base; 323 priv->regmap = dev_get_regmap(dev->parent, NULL); 324 if (IS_ERR(priv->regmap)) { 325 err = PTR_ERR(priv->regmap); 326 dev_err(dev, "failed to get regmap: %d\n", err); 327 return err; 328 } 329 330 for_each_child_of_node(np, child) { 331 err = of_property_read_u32(child, "reg", ®); 332 if (err) { 333 of_node_put(child); 334 return err; 335 } 336 > 337 if (reg < 0 || reg >= SC27XX_LEDS_MAX 338 || priv->leds[reg].active) { 339 of_node_put(child); 340 return -EINVAL; 341 } 342 343 priv->leds[reg].active = true; 344 priv->leds[reg].ldev.name = 345 of_get_property(child, "label", NULL) ? : child->name; 346 } 347 348 return sc27xx_led_register(dev, priv); 349 } 350 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver
Hi Xiaotong, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on robh/for-next] [also build test WARNING on v4.17-rc3 next-20180504] [cannot apply to j.anaszewski-leds/for-next] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Baolin-Wang/dt-bindings-leds-Add-SC27xx-breathing-light-controller-documentation/20180504-200830 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next smatch warnings: drivers/leds/leds-sc27xx-bltc.c:337 sc27xx_led_probe() warn: unsigned 'reg' is never less than zero. vim +/reg +337 drivers/leds/leds-sc27xx-bltc.c 299 300 static int sc27xx_led_probe(struct platform_device *pdev) 301 { 302 struct device *dev = &pdev->dev; 303 struct device_node *np = dev->of_node, *child; 304 struct sc27xx_led_priv *priv; 305 u32 base, count, reg; 306 int err; 307 308 count = of_get_child_count(np); 309 if (!count || count > SC27XX_LEDS_MAX) 310 return -EINVAL; 311 312 err = of_property_read_u32(np, "reg", &base); 313 if (err) { 314 dev_err(dev, "fail to get reg of property\n"); 315 return err; 316 } 317 318 priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); 319 if (!priv) 320 return -ENOMEM; 321 322 priv->base = base; 323 priv->regmap = dev_get_regmap(dev->parent, NULL); 324 if (IS_ERR(priv->regmap)) { 325 err = PTR_ERR(priv->regmap); 326 dev_err(dev, "failed to get regmap: %d\n", err); 327 return err; 328 } 329 330 for_each_child_of_node(np, child) { 331 err = of_property_read_u32(child, "reg", ®); 332 if (err) { 333 of_node_put(child); 334 return err; 335 } 336 > 337 if (reg < 0 || reg >= SC27XX_LEDS_MAX 338 || priv->leds[reg].active) { 339 of_node_put(child); 340 return -EINVAL; 341 } 342 343 priv->leds[reg].active = true; 344 priv->leds[reg].ldev.name = 345 of_get_property(child, "label", NULL) ? : child->name; 346 } 347 348 return sc27xx_led_register(dev, priv); 349 } 350 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
[PATCH 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver
From: Xiaotong Lu This patch adds Spreadtrum SC27xx PMIC series breathing light controller driver, which can support 3 LEDs. Each LED can work at normal PWM mode and breathing mode. Signed-off-by: Xiaotong Lu Signed-off-by: Baolin Wang --- drivers/leds/Kconfig| 11 ++ drivers/leds/Makefile |1 + drivers/leds/leds-sc27xx-bltc.c | 369 +++ 3 files changed, 381 insertions(+) create mode 100644 drivers/leds/leds-sc27xx-bltc.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 2c896c0..319449b 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -647,6 +647,17 @@ config LEDS_IS31FL32XX LED controllers. They are I2C devices with multiple constant-current channels, each with independent 256-level PWM control. +config LEDS_SC27XX_BLTC + tristate "LED support for the SC27xx breathing light controller" + depends on LEDS_CLASS && MFD_SC27XX_PMIC + depends on OF + help + Say Y here to include support for the SC27xx breathing light controller + LEDs. + + This driver can also be built as a module. If so the module will be + called leds-sc27xx-bltc. + comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)" config LEDS_BLINKM diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 91eca81..ff6917e 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o +obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o # LED SPI Drivers obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c new file mode 100644 index 000..0016a87 --- /dev/null +++ b/drivers/leds/leds-sc27xx-bltc.c @@ -0,0 +1,369 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 Spreadtrum Communications Inc. + */ + +#include +#include +#include +#include +#include + +/* PMIC global control register definition */ +#define SC27XX_MODULE_EN0 0xc08 +#define SC27XX_CLK_EN0 0xc18 +#define SC27XX_RGB_CTRL0xebc + +#define SC27XX_BLTC_EN BIT(9) +#define SC27XX_RTC_EN BIT(7) +#define SC27XX_RGB_PD BIT(0) + +/* Breathing light controller register definition */ +#define SC27XX_LEDS_CTRL 0x00 +#define SC27XX_LEDS_PRESCALE 0x04 +#define SC27XX_LEDS_DUTY 0x08 +#define SC27XX_LEDS_CURVE0 0x0c +#define SC27XX_LEDS_CURVE1 0x10 + +#define SC27XX_CTRL_SHIFT 4 +#define SC27XX_LED_RUN BIT(0) +#define SC27XX_LED_TYPEBIT(1) + +#define SC27XX_DUTY_SHIFT 8 +#define SC27XX_DUTY_MASK GENMASK(15, 0) +#define SC27XX_MOD_MASKGENMASK(7, 0) + +#define SC27XX_CURVE_SHIFT 8 +#define SC27XX_CURVE_L_MASKGENMASK(7, 0) +#define SC27XX_CURVE_H_MASKGENMASK(15, 8) + +#define SC27XX_LEDS_OFFSET 0x10 +#define SC27XX_LEDS_MAX3 + +/* + * The SC27xx breathing light controller can support 3 outputs: red LED, + * green LED and blue LED. Each LED can support normal PWM mode and breathing + * mode. + * + * In breathing mode, the LED output curve includes raise, high, fall and low 4 + * stages, and the duration of each stage is configurable. + */ +enum sc27xx_led_config { + SC27XX_RAISE_TIME, + SC27XX_FALL_TIME, + SC27XX_HIGH_TIME, + SC27XX_LOW_TIME, +}; + +struct sc27xx_led { + struct led_classdev ldev; + struct sc27xx_led_priv *priv; + enum led_brightness value; + u8 line; + bool breath_mode; + bool active; +}; + +struct sc27xx_led_priv { + struct sc27xx_led leds[SC27XX_LEDS_MAX]; + struct regmap *regmap; + u32 base; +}; + +#define to_sc27xx_led(ldev) \ + container_of(ldev, struct sc27xx_led, ldev) + +static int sc27xx_led_init(struct regmap *regmap) +{ + int err; + + err = regmap_update_bits(regmap, SC27XX_MODULE_EN0, SC27XX_BLTC_EN, +SC27XX_BLTC_EN); + if (err) + return err; + + err = regmap_update_bits(regmap, SC27XX_CLK_EN0, SC27XX_RTC_EN, +SC27XX_RTC_EN); + if (err) + return err; + + return regmap_update_bits(regmap, SC27XX_RGB_CTRL, SC27XX_RGB_PD, 0); +} + +static u32 sc27xx_led_get_offset(struct sc27xx_led *leds) +{ + return leds->priv->base + SC27XX_LEDS_OFFSET * leds->line; +} + +static int sc27xx_led_enable(struct sc27xx_led *leds) +{ + u32 base = sc27xx_led_get_offset(leds); + u32 ctrl_base = leds->priv->base + SC27XX_LEDS_CTRL; + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line; + struct regmap *regmap = leds->priv->regmap;