| Message ID | 099f4075ec489e425b5f1e7668a078c05f0d8509.1525427961.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: leds: Add SC27xx breathing light controller documentation | expand |
Hi Rob, On 8 May 2018 at 05:10, Rob Herring <robh@kernel.org> wrote: > On Fri, May 04, 2018 at 06:08:35PM +0800, Baolin Wang wrote: >> This patch adds the binding documentation for Spreadtrum SC27xx series >> breathing light controller, which supports 3 outputs: red LED, green >> LED and blue LED. >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> --- >> .../devicetree/bindings/leds/leds-sc27xx-bltc.txt | 39 ++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt >> >> diff --git a/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt b/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt >> new file mode 100644 >> index 0000000..d4e267d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt >> @@ -0,0 +1,39 @@ >> +LEDs connected to Spreadtrum SC27XX PMIC breathing light controller >> + >> +The SC27xx breathing light controller supports to 3 outputs: >> +red LED, green LED and blue LED. Each LED can work at normal >> +PWM mode or breath light mode. >> + >> +Required properties: >> +- compatible: should be "sprd,sc27xx-bltc". > > Don't use wildcards in compatible strings. Will change to one explicit Soc name. Thanks. -- Baolin.wang Best Regards
Hi Jacek, On 8 May 2018 at 04:13, Jacek Anaszewski <jacek.anaszewski@gmail.com> 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 <xiaotong.lu@spreadtrum.com> >> >> 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 <xiaotong.lu@spreadtrum.com> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> --- >> 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 0000000..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 <linux/leds.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> + >> +/* PMIC global control register definition */ >> +#define SC27XX_MODULE_EN0 0xc08 >> +#define SC27XX_CLK_EN0 0xc18 >> +#define SC27XX_RGB_CTRL 0xebc >> + >> +#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_TYPE BIT(1) >> + >> +#define SC27XX_DUTY_SHIFT 8 >> +#define SC27XX_DUTY_MASK GENMASK(15, 0) >> +#define SC27XX_MOD_MASK GENMASK(7, 0) >> + >> +#define SC27XX_CURVE_SHIFT 8 >> +#define SC27XX_CURVE_L_MASK GENMASK(7, 0) >> +#define SC27XX_CURVE_H_MASK GENMASK(15, 8) >> + >> +#define SC27XX_LEDS_OFFSET 0x10 >> +#define SC27XX_LEDS_MAX 3 >> + >> +/* >> + * 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; >> + int err; >> + >> + err = regmap_update_bits(regmap, base + SC27XX_LEDS_DUTY, >> + SC27XX_DUTY_MASK, >> + (leds->value << SC27XX_DUTY_SHIFT) | >> + SC27XX_MOD_MASK); >> + if (err) >> + return err; >> + >> + if (leds->breath_mode) >> + return regmap_update_bits(regmap, ctrl_base, >> + SC27XX_LED_RUN << ctrl_shift, >> + SC27XX_LED_RUN << ctrl_shift); >> + >> + return regmap_update_bits(regmap, ctrl_base, >> + (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, >> + (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift); >> +} >> + >> +static int sc27xx_led_disable(struct sc27xx_led *leds) >> +{ >> + u8 ctrl_shift = SC27XX_CTRL_SHIFT * leds->line; >> + >> + leds->breath_mode = false; >> + >> + return regmap_update_bits(leds->priv->regmap, >> + leds->priv->base + SC27XX_LEDS_CTRL, >> + (SC27XX_LED_RUN | SC27XX_LED_TYPE) << ctrl_shift, >> 0); >> +} >> + >> +static int sc27xx_led_set(struct led_classdev *ldev, enum led_brightness >> value) >> +{ >> + struct sc27xx_led *leds = to_sc27xx_led(ldev); > > > You need mutex protection here to ensure that the device will > not be left in an inconsistent state in case of concurrent access > from userspace. Indeed, will add one lock in next version. > > >> + leds->value = value; >> + if (value == LED_OFF) >> + return sc27xx_led_disable(leds); >> + >> + return sc27xx_led_enable(leds); >> +} >> + >> +static int sc27xx_led_config(struct sc27xx_led *leds, >> + enum sc27xx_led_config config, u32 val) >> +{ >> + u32 base = sc27xx_led_get_offset(leds); >> + struct regmap *regmap = leds->priv->regmap; >> + int err; >> + >> + switch (config) { >> + case SC27XX_RAISE_TIME: >> + err = regmap_update_bits(regmap, base + >> SC27XX_LEDS_CURVE0, >> + SC27XX_CURVE_L_MASK, val); >> + break; >> + case SC27XX_FALL_TIME: >> + err = regmap_update_bits(regmap, base + >> SC27XX_LEDS_CURVE0, >> + SC27XX_CURVE_H_MASK, >> + val << SC27XX_CURVE_SHIFT); >> + break; >> + case SC27XX_HIGH_TIME: >> + err = regmap_update_bits(regmap, base + >> SC27XX_LEDS_CURVE1, >> + SC27XX_CURVE_L_MASK, val); >> + break; >> + case SC27XX_LOW_TIME: >> + err = regmap_update_bits(regmap, base + >> SC27XX_LEDS_CURVE1, >> + SC27XX_CURVE_H_MASK, >> + val << SC27XX_CURVE_SHIFT); >> + break; >> + default: >> + err = -ENOTSUPP; >> + break; >> + } > > > Ditto. OK. > > >> + >> + if (!err && !leds->breath_mode) >> + leds->breath_mode = true; >> + >> + return err; >> +} >> + >> +static ssize_t raise_time_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + struct led_classdev *ldev = dev_get_drvdata(dev); >> + struct sc27xx_led *leds = to_sc27xx_led(ldev); >> + u32 val; >> + int err; >> + >> + if (kstrtou32(buf, 10, &val)) >> + return -EINVAL; >> + >> + err = sc27xx_led_config(leds, SC27XX_RAISE_TIME, val); >> + if (err) >> + return err; >> + >> + return size; >> +} >> + >> +static ssize_t fall_time_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + struct led_classdev *ldev = dev_get_drvdata(dev); >> + struct sc27xx_led *leds = to_sc27xx_led(ldev); >> + u32 val; >> + int err; >> + >> + if (kstrtou32(buf, 10, &val)) >> + return -EINVAL; >> + >> + err = sc27xx_led_config(leds, SC27XX_FALL_TIME, val); >> + if (err) >> + return err; >> + >> + return size; >> +} >> + >> +static ssize_t high_time_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + struct led_classdev *ldev = dev_get_drvdata(dev); >> + struct sc27xx_led *leds = to_sc27xx_led(ldev); >> + u32 val; >> + int err; >> + >> + if (kstrtou32(buf, 10, &val)) >> + return -EINVAL; >> + >> + err = sc27xx_led_config(leds, SC27XX_HIGH_TIME, val); >> + if (err) >> + return err; >> + >> + return size; >> +} >> + >> +static ssize_t low_time_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + struct led_classdev *ldev = dev_get_drvdata(dev); >> + struct sc27xx_led *leds = to_sc27xx_led(ldev); >> + u32 val; >> + int err; >> + >> + if (kstrtou32(buf, 10, &val)) >> + return -EINVAL; >> + >> + err = sc27xx_led_config(leds, SC27XX_LOW_TIME, val); >> + if (err) >> + return err; >> + >> + return size; >> +} >> + >> +static DEVICE_ATTR_WO(raise_time); >> +static DEVICE_ATTR_WO(fall_time); >> +static DEVICE_ATTR_WO(high_time); >> +static DEVICE_ATTR_WO(low_time); >> + >> +static struct attribute *sc27xx_led_attrs[] = { >> + &dev_attr_raise_time.attr, >> + &dev_attr_fall_time.attr, >> + &dev_attr_high_time.attr, >> + &dev_attr_low_time.attr, >> + NULL, >> +}; >> +ATTRIBUTE_GROUPS(sc27xx_led); > > > Please add ABI documentation for this sysfs interface. OK. > > >> +static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv >> *priv) >> +{ >> + int i, err; >> + >> + err = sc27xx_led_init(priv->regmap); >> + if (err) >> + return err; >> + >> + for (i = 0; i < SC27XX_LEDS_MAX; i++) { >> + struct sc27xx_led *led = &priv->leds[i]; >> + >> + if (!led->active) >> + continue; >> + >> + led->line = i; >> + led->priv = priv; >> + led->ldev.brightness_set_blocking = sc27xx_led_set; >> + led->ldev.max_brightness = LED_FULL; >> + led->ldev.groups = sc27xx_led_groups; >> + >> + err = devm_led_classdev_register(dev, &led->ldev); >> + if (err) >> + return err; >> + } >> + >> + return 0; >> +} >> + >> +static int sc27xx_led_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node, *child; >> + struct sc27xx_led_priv *priv; >> + u32 base, count, reg; >> + int err; >> + >> + count = of_get_child_count(np); >> + if (!count || count > SC27XX_LEDS_MAX) >> + return -EINVAL; >> + >> + err = of_property_read_u32(np, "reg", &base); >> + if (err) { >> + dev_err(dev, "fail to get reg of property\n"); >> + return err; >> + } >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + priv->base = base; >> + priv->regmap = dev_get_regmap(dev->parent, NULL); >> + if (IS_ERR(priv->regmap)) { >> + err = PTR_ERR(priv->regmap); >> + dev_err(dev, "failed to get regmap: %d\n", err); >> + return err; >> + } >> + >> + for_each_child_of_node(np, child) { >> + err = of_property_read_u32(child, "reg", ®); >> + if (err) { >> + of_node_put(child); >> + return err; >> + } >> + >> + if (reg < 0 || reg >= SC27XX_LEDS_MAX >> + || priv->leds[reg].active) { >> + of_node_put(child); >> + return -EINVAL; >> + } >> + >> + priv->leds[reg].active = true; >> + priv->leds[reg].ldev.name = >> + of_get_property(child, "label", NULL) ? : >> child->name; > > > LED class device naming pattern requires devicename section. > Please use "sc27xx::" for the case when label is not available > and concatenate "sc27xx:" with the child->name otherwise. > > You can refer to drivers/leds/leds-cr0014114.c in this regard. > We're sorting out issues around LED class device naming, so this > is quite new approach. Make sense. Will fix it in next version. Thanks for your helpful comments. -- Baolin.wang Best Regards
diff --git a/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt b/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt new file mode 100644 index 0000000..d4e267d --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt @@ -0,0 +1,39 @@ +LEDs connected to Spreadtrum SC27XX PMIC breathing light controller + +The SC27xx breathing light controller supports to 3 outputs: +red LED, green LED and blue LED. Each LED can work at normal +PWM mode or breath light mode. + +Required properties: +- compatible: should be "sprd,sc27xx-bltc". +- #address-cells: must be 1. +- #size-cells: must be 0. +- reg: specify controller address. + +LED sub-node properties: +- reg: number of LED line (could be from 0 to 2). +- label: (optional) name of LED. + +Examples: + +led-controller@200 { + compatible = "sprd,sc27xx-bltc"; + #address-cells = <1>; + #size-cells = <0>; + reg = <0x200>; + + red@0 { + label = "red"; + reg = <0x0>; + }; + + green@1 { + label = "green"; + reg = <0x1>; + }; + + blue@2 { + label = "blue"; + reg = <0x2>; + }; +};
This patch adds the binding documentation for Spreadtrum SC27xx series breathing light controller, which supports 3 outputs: red LED, green LED and blue LED. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- .../devicetree/bindings/leds/leds-sc27xx-bltc.txt | 39 ++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt -- 1.7.9.5