[PATCH v2 1/2] dt-bindings: leds: Add SC27xx breathing light controller documentation

[PATCH v2 1/2] dt-bindings: leds: Add SC27xx breathing light controller documentation

[Baolin Wang]

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>
---
Changes since v1:
 - Change the compatible string to be one explicit SoC name.
 - Change the child node name.
 - Change to be upper case for the first character.
---
 .../devicetree/bindings/leds/leds-sc27xx-bltc.txt  |   41 ++++++++++++++++++++
 1 file changed, 41 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..b3de7fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt
@@ -0,0 +1,41 @@
+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,sc2731-bltc".
+- #address-cells: Must be 1.
+- #size-cells: Must be 0.
+- reg: Specify controller address.
+
+Required child properties:
+- reg: Number of LED line (could be from 0 to 2).
+
+Optional child properties:
+- label: See Documentation/devicetree/bindings/leds/common.txt.
+
+Examples:
+
+led-controller@200 {
+	compatible = "sprd,sc2731-bltc";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x200>;
+
+	led@0 {
+		label = "red";
+		reg = <0x0>;
+	};
+
+	led@1 {
+		label = "green";
+		reg = <0x1>;
+	};
+
+	led@2 {
+		label = "blue";
+		reg = <0x2>;
+	};
+};
-- 
1.7.9.5

[PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Baolin Wang]

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>
---
Changes since v1:
 - Add ABI documentation.
 - Add mutex protection in case of concurrent access.
 - Change the LED device name pattern.
 - Fix build warning.
---
 .../ABI/testing/sysfs-class-led-driver-sc27xx      |   19 +
 drivers/leds/Kconfig                               |   11 +
 drivers/leds/Makefile                              |    1 +
 drivers/leds/leds-sc27xx-bltc.c                    |  387 ++++++++++++++++++++
 4 files changed, 418 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
 create mode 100644 drivers/leds/leds-sc27xx-bltc.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
new file mode 100644
index 0000000..22166fb
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
@@ -0,0 +1,19 @@
+What:		/sys/class/leds/<led>/rise_time
+What:		/sys/class/leds/<led>/high_time
+What:		/sys/class/leds/<led>/fall_time
+What:		/sys/class/leds/<led>/low_time
+Date:		May 2018
+KernelVersion:	4.18
+Contact:	Xiaotong Lu <xiaotong.lu@spreadtrum.com>
+Description:
+		Set the pattern generator rise, high, fall and low
+		times (0..63). It's unit is 0.125s, it should be > 0.
+
+		1 - 125 ms
+		2 - 250 ms
+		3 - 375 ms
+		...
+		...
+		...
+		62 - 7.75 s
+		63 - 7.875 s
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..61c5b72
--- /dev/null
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -0,0 +1,387 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Spreadtrum Communications Inc.
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <uapi/linux/uleds.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
+#define SC27XX_LEDS_TIME_MAX	63
+
+/*
+ * 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 rise, high, fall and low 4
+ * stages, and the duration of each stage is configurable.
+ */
+enum sc27xx_led_config {
+	SC27XX_RISE_TIME,
+	SC27XX_FALL_TIME,
+	SC27XX_HIGH_TIME,
+	SC27XX_LOW_TIME,
+};
+
+struct sc27xx_led {
+	char name[LED_MAX_NAME_SIZE];
+	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;
+	struct mutex lock;
+	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);
+	int err;
+
+	mutex_lock(&leds->priv->lock);
+	leds->value = value;
+	if (value == LED_OFF)
+		err = sc27xx_led_disable(leds);
+	else
+		err = sc27xx_led_enable(leds);
+	mutex_unlock(&leds->priv->lock);
+
+	return err;
+}
+
+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;
+
+	mutex_lock(&leds->priv->lock);
+	switch (config) {
+	case SC27XX_RISE_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;
+	}
+
+	if (!err && !leds->breath_mode)
+		leds->breath_mode = true;
+
+	mutex_unlock(&leds->priv->lock);
+
+	return err;
+}
+
+static ssize_t rise_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) || val > SC27XX_LEDS_TIME_MAX)
+		return -EINVAL;
+
+	err = sc27xx_led_config(leds, SC27XX_RISE_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) || val > SC27XX_LEDS_TIME_MAX)
+		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) || val > SC27XX_LEDS_TIME_MAX)
+		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) || val > SC27XX_LEDS_TIME_MAX)
+		return -EINVAL;
+
+	err = sc27xx_led_config(leds, SC27XX_LOW_TIME, val);
+	if (err)
+		return err;
+
+	return size;
+}
+
+static DEVICE_ATTR_WO(rise_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_rise_time.attr,
+	&dev_attr_fall_time.attr,
+	&dev_attr_high_time.attr,
+	&dev_attr_low_time.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(sc27xx_led);
+
+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.name = led->name;
+		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;
+	const char *str;
+	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;
+
+	mutex_init(&priv->lock);
+	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", &reg);
+		if (err) {
+			of_node_put(child);
+			return err;
+		}
+
+		if (reg >= SC27XX_LEDS_MAX || priv->leds[reg].active) {
+			of_node_put(child);
+			return -EINVAL;
+		}
+
+		priv->leds[reg].active = true;
+
+		err = of_property_read_string(child, "label", &str);
+		if (err)
+			snprintf(priv->leds[reg].name, LED_MAX_NAME_SIZE,
+				 "sc27xx::");
+		else
+			snprintf(priv->leds[reg].name, LED_MAX_NAME_SIZE,
+				 "sc27xx:%s", str);
+	}
+
+	return sc27xx_led_register(dev, priv);
+}
+
+static const struct of_device_id sc27xx_led_of_match[] = {
+	{ .compatible = "sprd,sc2731-bltc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sc27xx_led_of_match);
+
+static struct platform_driver sc27xx_led_driver = {
+	.driver = {
+		.name = "sprd-bltc",
+		.of_match_table = sc27xx_led_of_match,
+	},
+	.probe = sc27xx_led_probe,
+};
+
+module_platform_driver(sc27xx_led_driver);
+
+MODULE_DESCRIPTION("Spreadtrum SC27xx breathing light controller driver");
+MODULE_AUTHOR("Xiaotong Lu <xiaotong.lu@spreadtrum.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v2 1/2] dt-bindings: leds: Add SC27xx breathing light controller documentation

[Rob Herring]

On Tue, May 08, 2018 at 01:39:44PM +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>
> ---
> Changes since v1:
>  - Change the compatible string to be one explicit SoC name.
>  - Change the child node name.
>  - Change to be upper case for the first character.
> ---
>  .../devicetree/bindings/leds/leds-sc27xx-bltc.txt  |   41 ++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Jacek Anaszewski]

Hi Baolin,

Thank you for the updated version.

I have few notes below, please take a look.

On 05/08/2018 07:39 AM, 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>
> ---
> Changes since v1:
>   - Add ABI documentation.
>   - Add mutex protection in case of concurrent access.
>   - Change the LED device name pattern.
>   - Fix build warning.
> ---
>   .../ABI/testing/sysfs-class-led-driver-sc27xx      |   19 +
>   drivers/leds/Kconfig                               |   11 +
>   drivers/leds/Makefile                              |    1 +
>   drivers/leds/leds-sc27xx-bltc.c                    |  387 ++++++++++++++++++++
>   4 files changed, 418 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>   create mode 100644 drivers/leds/leds-sc27xx-bltc.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
> new file mode 100644
> index 0000000..22166fb
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
> @@ -0,0 +1,19 @@
> +What:		/sys/class/leds/<led>/rise_time
> +What:		/sys/class/leds/<led>/high_time
> +What:		/sys/class/leds/<led>/fall_time
> +What:		/sys/class/leds/<led>/low_time
> +Date:		May 2018
> +KernelVersion:	4.18
> +Contact:	Xiaotong Lu <xiaotong.lu@spreadtrum.com>
> +Description:
> +		Set the pattern generator rise, high, fall and low
> +		times (0..63). It's unit is 0.125s, it should be > 0.
> +
> +		1 - 125 ms
> +		2 - 250 ms
> +		3 - 375 ms
> +		...
> +		...
> +		...
> +		62 - 7.75 s
> +		63 - 7.875 s

It would be good to describe here how altering these settings
interacts with brightness changes. E.g., AFAICS from the code
the pattern is turned off when setting brightness to 0, and
turned on when writing any of these files.

I'm also not sure if making these files RO is a good idea.
How about making them RW? Any brightness change would
reset those values to 0 and turn off pattern generator.

Otherwise it would be not possible to check whether pattern
generator is enabled or not.

> 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..61c5b72
> --- /dev/null
> +++ b/drivers/leds/leds-sc27xx-bltc.c
> @@ -0,0 +1,387 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Spreadtrum Communications Inc.
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <uapi/linux/uleds.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
> +#define SC27XX_LEDS_TIME_MAX	63
> +
> +/*
> + * 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 rise, high, fall and low 4
> + * stages, and the duration of each stage is configurable.
> + */
> +enum sc27xx_led_config {
> +	SC27XX_RISE_TIME,
> +	SC27XX_FALL_TIME,
> +	SC27XX_HIGH_TIME,
> +	SC27XX_LOW_TIME,
> +};
> +
> +struct sc27xx_led {
> +	char name[LED_MAX_NAME_SIZE];
> +	struct led_classdev ldev;
> +	struct sc27xx_led_priv *priv;
> +	enum led_brightness value;

This property is redundant - you have brightness in the
struct led_classdev.

> +	u8 line;
> +	bool breath_mode;
> +	bool active;
> +};
> +
> +struct sc27xx_led_priv {
> +	struct sc27xx_led leds[SC27XX_LEDS_MAX];
> +	struct regmap *regmap;
> +	struct mutex lock;
> +	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);
> +	int err;
> +
> +	mutex_lock(&leds->priv->lock);

Empty line here please.

> +	leds->value = value;

It should not be needed at all.

> +	if (value == LED_OFF)
> +		err = sc27xx_led_disable(leds);
> +	else
> +		err = sc27xx_led_enable(leds);

Empty line here please.

> +	mutex_unlock(&leds->priv->lock);
> +
> +	return err;
> +}
> +
> +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;
> +
> +	mutex_lock(&leds->priv->lock);

Empty line here please.

> +	switch (config) {
> +	case SC27XX_RISE_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;
> +	}
> +
> +	if (!err && !leds->breath_mode)
> +		leds->breath_mode = true;
> +
> +	mutex_unlock(&leds->priv->lock);
> +
> +	return err;
> +}
> +
> +static ssize_t rise_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) || val > SC27XX_LEDS_TIME_MAX)
> +		return -EINVAL;
> +
> +	err = sc27xx_led_config(leds, SC27XX_RISE_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) || val > SC27XX_LEDS_TIME_MAX)
> +		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) || val > SC27XX_LEDS_TIME_MAX)
> +		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) || val > SC27XX_LEDS_TIME_MAX)
> +		return -EINVAL;
> +
> +	err = sc27xx_led_config(leds, SC27XX_LOW_TIME, val);
> +	if (err)
> +		return err;
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_WO(rise_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_rise_time.attr,
> +	&dev_attr_fall_time.attr,
> +	&dev_attr_high_time.attr,
> +	&dev_attr_low_time.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(sc27xx_led);
> +
> +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.name = led->name;
> +		led->ldev.brightness_set_blocking = sc27xx_led_set;
> +		led->ldev.max_brightness = LED_FULL;

LED core will set max_brightness to LED_FULL if initialized to 0,
so we can skip this line.

> +		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;
> +	const char *str;
> +	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;
> +
> +	mutex_init(&priv->lock);
> +	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", &reg);
> +		if (err) {
> +			of_node_put(child);

Now we need also mutex_destroy() in the error path, so please add 
relevant label and go there from here.

> +			return err;
> +		}
> +
> +		if (reg >= SC27XX_LEDS_MAX || priv->leds[reg].active) {
> +			of_node_put(child);
> +			return -EINVAL;

Ditto.

> +		}
> +
> +		priv->leds[reg].active = true;
> +
> +		err = of_property_read_string(child, "label", &str);
> +		if (err)
> +			snprintf(priv->leds[reg].name, LED_MAX_NAME_SIZE,
> +				 "sc27xx::");
> +		else
> +			snprintf(priv->leds[reg].name, LED_MAX_NAME_SIZE,
> +				 "sc27xx:%s", str);
> +	}
> +
> +	return sc27xx_led_register(dev, priv);
> +}
> +
> +static const struct of_device_id sc27xx_led_of_match[] = {
> +	{ .compatible = "sprd,sc2731-bltc", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sc27xx_led_of_match);
> +
> +static struct platform_driver sc27xx_led_driver = {
> +	.driver = {
> +		.name = "sprd-bltc",
> +		.of_match_table = sc27xx_led_of_match,
> +	},
> +	.probe = sc27xx_led_probe,

"remove" op will be needed as well for mutex_destroy().

> +};
> +
> +module_platform_driver(sc27xx_led_driver);
> +
> +MODULE_DESCRIPTION("Spreadtrum SC27xx breathing light controller driver");
> +MODULE_AUTHOR("Xiaotong Lu <xiaotong.lu@spreadtrum.com>");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Baolin Wang]

Hi Jacek,

On 9 May 2018 at 04:54, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> Hi Baolin,
>
> Thank you for the updated version.
>
> I have few notes below, please take a look.
>
>
> On 05/08/2018 07:39 AM, 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>
>> ---
>> Changes since v1:
>>   - Add ABI documentation.
>>   - Add mutex protection in case of concurrent access.
>>   - Change the LED device name pattern.
>>   - Fix build warning.
>> ---
>>   .../ABI/testing/sysfs-class-led-driver-sc27xx      |   19 +
>>   drivers/leds/Kconfig                               |   11 +
>>   drivers/leds/Makefile                              |    1 +
>>   drivers/leds/leds-sc27xx-bltc.c                    |  387
>> ++++++++++++++++++++
>>   4 files changed, 418 insertions(+)
>>   create mode 100644
>> Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>>   create mode 100644 drivers/leds/leds-sc27xx-bltc.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> new file mode 100644
>> index 0000000..22166fb
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> @@ -0,0 +1,19 @@
>> +What:          /sys/class/leds/<led>/rise_time
>> +What:          /sys/class/leds/<led>/high_time
>> +What:          /sys/class/leds/<led>/fall_time
>> +What:          /sys/class/leds/<led>/low_time
>> +Date:          May 2018
>> +KernelVersion: 4.18
>> +Contact:       Xiaotong Lu <xiaotong.lu@spreadtrum.com>
>> +Description:
>> +               Set the pattern generator rise, high, fall and low
>> +               times (0..63). It's unit is 0.125s, it should be > 0.
>> +
>> +               1 - 125 ms
>> +               2 - 250 ms
>> +               3 - 375 ms
>> +               ...
>> +               ...
>> +               ...
>> +               62 - 7.75 s
>> +               63 - 7.875 s
>
>
> It would be good to describe here how altering these settings
> interacts with brightness changes. E.g., AFAICS from the code
> the pattern is turned off when setting brightness to 0, and
> turned on when writing any of these files.

Correct. We need set rise, high, fall and low time firstly, then turn
on the LED with setting brightness > 0. Will add more description to
explain how to use them.

>
> I'm also not sure if making these files RO is a good idea.
> How about making them RW? Any brightness change would
> reset those values to 0 and turn off pattern generator.

Sure, will make them RW.

>
> Otherwise it would be not possible to check whether pattern
> generator is enabled or not.
>
>
>> 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..61c5b72
>> --- /dev/null
>> +++ b/drivers/leds/leds-sc27xx-bltc.c
>> @@ -0,0 +1,387 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2018 Spreadtrum Communications Inc.
>> +
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <uapi/linux/uleds.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
>> +#define SC27XX_LEDS_TIME_MAX   63
>> +
>> +/*
>> + * 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 rise, high, fall and
>> low 4
>> + * stages, and the duration of each stage is configurable.
>> + */
>> +enum sc27xx_led_config {
>> +       SC27XX_RISE_TIME,
>> +       SC27XX_FALL_TIME,
>> +       SC27XX_HIGH_TIME,
>> +       SC27XX_LOW_TIME,
>> +};
>> +
>> +struct sc27xx_led {
>> +       char name[LED_MAX_NAME_SIZE];
>> +       struct led_classdev ldev;
>> +       struct sc27xx_led_priv *priv;
>> +       enum led_brightness value;
>
>
> This property is redundant - you have brightness in the
> struct led_classdev.

Yes, will remove this.

>> +       u8 line;
>> +       bool breath_mode;
>> +       bool active;
>> +};
>> +
>> +struct sc27xx_led_priv {
>> +       struct sc27xx_led leds[SC27XX_LEDS_MAX];
>> +       struct regmap *regmap;
>> +       struct mutex lock;
>> +       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);
>> +       int err;
>> +
>> +       mutex_lock(&leds->priv->lock);
>
>
> Empty line here please.

OK.

>
>> +       leds->value = value;
>
>
> It should not be needed at all.

Yes.

>
>> +       if (value == LED_OFF)
>> +               err = sc27xx_led_disable(leds);
>> +       else
>> +               err = sc27xx_led_enable(leds);
>
>
> Empty line here please.

OK.

>
>> +       mutex_unlock(&leds->priv->lock);
>> +
>> +       return err;
>> +}
>> +
>> +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;
>> +
>> +       mutex_lock(&leds->priv->lock);
>
>
> Empty line here please.

OK.

>
>> +       switch (config) {
>> +       case SC27XX_RISE_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;
>> +       }
>> +
>> +       if (!err && !leds->breath_mode)
>> +               leds->breath_mode = true;
>> +
>> +       mutex_unlock(&leds->priv->lock);
>> +
>> +       return err;
>> +}
>> +
>> +static ssize_t rise_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) || val > SC27XX_LEDS_TIME_MAX)
>> +               return -EINVAL;
>> +
>> +       err = sc27xx_led_config(leds, SC27XX_RISE_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) || val > SC27XX_LEDS_TIME_MAX)
>> +               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) || val > SC27XX_LEDS_TIME_MAX)
>> +               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) || val > SC27XX_LEDS_TIME_MAX)
>> +               return -EINVAL;
>> +
>> +       err = sc27xx_led_config(leds, SC27XX_LOW_TIME, val);
>> +       if (err)
>> +               return err;
>> +
>> +       return size;
>> +}
>> +
>> +static DEVICE_ATTR_WO(rise_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_rise_time.attr,
>> +       &dev_attr_fall_time.attr,
>> +       &dev_attr_high_time.attr,
>> +       &dev_attr_low_time.attr,
>> +       NULL,
>> +};
>> +ATTRIBUTE_GROUPS(sc27xx_led);
>> +
>> +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.name = led->name;
>> +               led->ldev.brightness_set_blocking = sc27xx_led_set;
>> +               led->ldev.max_brightness = LED_FULL;
>
>
> LED core will set max_brightness to LED_FULL if initialized to 0,
> so we can skip this line.

Sure.

>> +               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;
>> +       const char *str;
>> +       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;
>> +
>> +       mutex_init(&priv->lock);
>> +       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", &reg);
>> +               if (err) {
>> +                       of_node_put(child);
>
>
> Now we need also mutex_destroy() in the error path, so please add relevant
> label and go there from here.

OK.

>
>> +                       return err;
>> +               }
>> +
>> +               if (reg >= SC27XX_LEDS_MAX || priv->leds[reg].active) {
>> +                       of_node_put(child);
>> +                       return -EINVAL;
>
>
> Ditto.
>
>
>> +               }
>> +
>> +               priv->leds[reg].active = true;
>> +
>> +               err = of_property_read_string(child, "label", &str);
>> +               if (err)
>> +                       snprintf(priv->leds[reg].name, LED_MAX_NAME_SIZE,
>> +                                "sc27xx::");
>> +               else
>> +                       snprintf(priv->leds[reg].name, LED_MAX_NAME_SIZE,
>> +                                "sc27xx:%s", str);
>> +       }
>> +
>> +       return sc27xx_led_register(dev, priv);
>> +}
>> +
>> +static const struct of_device_id sc27xx_led_of_match[] = {
>> +       { .compatible = "sprd,sc2731-bltc", },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, sc27xx_led_of_match);
>> +
>> +static struct platform_driver sc27xx_led_driver = {
>> +       .driver = {
>> +               .name = "sprd-bltc",
>> +               .of_match_table = sc27xx_led_of_match,
>> +       },
>> +       .probe = sc27xx_led_probe,
>
>
> "remove" op will be needed as well for mutex_destroy().

Yes. Thanks for your comments.

-- 
Baolin.wang
Best Regards

Re: [PATCH v2 1/2] dt-bindings: leds: Add SC27xx breathing light controller documentation

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

Hi!

> 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>
> ---
> Changes since v1:
>  - Change the compatible string to be one explicit SoC name.
>  - Change the child node name.
>  - Change to be upper case for the first character.
> ---
>  .../devicetree/bindings/leds/leds-sc27xx-bltc.txt  |   41 ++++++++++++++++++++
>  1 file changed, 41 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..b3de7fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt
> @@ -0,0 +1,41 @@
> +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,sc2731-bltc".

If this is for 2731, I'd say so in the title and the filename...?

> +- #address-cells: Must be 1.
> +- #size-cells: Must be 0.
> +- reg: Specify controller address.

the controller...

> +Required child properties:
> +- reg: Number of LED line (could be from 0 to 2).

Uff. "Port this LED is connected to"?

With that fixed...

Acked-by: Pavel Machek <pavel@ucw.cz>

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]

On Tue 2018-05-08 13:39:45, 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.
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
> new file mode 100644
> index 0000000..22166fb
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
> @@ -0,0 +1,19 @@
> +What:		/sys/class/leds/<led>/rise_time
> +What:		/sys/class/leds/<led>/high_time
> +What:		/sys/class/leds/<led>/fall_time
> +What:		/sys/class/leds/<led>/low_time
> +Date:		May 2018
> +KernelVersion:	4.18
> +Contact:	Xiaotong Lu <xiaotong.lu@spreadtrum.com>
> +Description:
> +		Set the pattern generator rise, high, fall and low
> +		times (0..63). It's unit is 0.125s, it should be > 0.
> +
> +		1 - 125 ms
> +		2 - 250 ms
> +		3 - 375 ms
> +		...
> +		...
> +		...
> +		62 - 7.75 s
> +		63 - 7.875 s

How does this interact with triggers? With manually setting
brightness? Are the pattern generators independend for the LEDs?

Can you generate white breathing pattern? If so, how?

How do you select between normal and breathing modes?

I'd specify times in miliseconds or something, this is way too
hardware specific.

Now... functionality like this is common between many LED
controllers. N900 could do this kind of "breathing", too, and it also
supports other patterns.

I believe we need interface common between different LED controllers.

And I guess it would be easiest if you dropped this part from initial
merge.

Thanks and best regards,

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Jacek Anaszewski]

Hi,

On 05/09/2018 04:25 PM, Pavel Machek wrote:
> On Tue 2018-05-08 13:39:45, 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.
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> new file mode 100644
>> index 0000000..22166fb
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> @@ -0,0 +1,19 @@
>> +What:		/sys/class/leds/<led>/rise_time
>> +What:		/sys/class/leds/<led>/high_time
>> +What:		/sys/class/leds/<led>/fall_time
>> +What:		/sys/class/leds/<led>/low_time
>> +Date:		May 2018
>> +KernelVersion:	4.18
>> +Contact:	Xiaotong Lu <xiaotong.lu@spreadtrum.com>
>> +Description:
>> +		Set the pattern generator rise, high, fall and low
>> +		times (0..63). It's unit is 0.125s, it should be > 0.
>> +
>> +		1 - 125 ms
>> +		2 - 250 ms
>> +		3 - 375 ms
>> +		...
>> +		...
>> +		...
>> +		62 - 7.75 s
>> +		63 - 7.875 s
> 
> How does this interact with triggers? With manually setting
> brightness? Are the pattern generators independend for the LEDs?
> 
> Can you generate white breathing pattern? If so, how?
> 
> How do you select between normal and breathing modes?
> 
> I'd specify times in miliseconds or something, this is way too
> hardware specific.

Agreed.

> Now... functionality like this is common between many LED
> controllers. N900 could do this kind of "breathing", too, and it also
> supports other patterns.
> 
> I believe we need interface common between different LED controllers.
> 
> And I guess it would be easiest if you dropped this part from initial
> merge.

I disagree here. We already had the same discussion at the occasion
of the patch [0] and it turned out to be a dead-end [1]. Now we have
neither the driver nor the generic pattern interface.

We also already have some older LED class drivers that implement custom
pattern interfaces (e.g. drivers/leds/leds-lm3533.c) and the same
approach can be applied in this case.

Regarding interaction with triggers - we could disable pattern
on setting any trigger and return -EBUSY from the pattern
interface if led_cdev->trigger is not NULL.

[0] https://lkml.org/lkml/2017/3/23/33
[1] https://lkml.org/lkml/2017/11/15/27

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] dt-bindings: leds: Add SC27xx breathing light controller documentation

[Jacek Anaszewski]

On 05/09/2018 04:25 PM, Pavel Machek wrote:
> Hi!
> 
>> 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>
>> ---
>> Changes since v1:
>>   - Change the compatible string to be one explicit SoC name.
>>   - Change the child node name.
>>   - Change to be upper case for the first character.
>> ---
>>   .../devicetree/bindings/leds/leds-sc27xx-bltc.txt  |   41 ++++++++++++++++++++
>>   1 file changed, 41 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..b3de7fc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt
>> @@ -0,0 +1,41 @@
>> +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,sc2731-bltc".
> 
> If this is for 2731, I'd say so in the title and the filename...?

Not necessarily, since apparently it is possible that other compatibles
might be added to it in the future.

> 
>> +- #address-cells: Must be 1.
>> +- #size-cells: Must be 0.
>> +- reg: Specify controller address.
> 
> the controller...
> 
>> +Required child properties:
>> +- reg: Number of LED line (could be from 0 to 2).
> 
> Uff. "Port this LED is connected to"?
> 
> With that fixed...
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> 									Pavel
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] dt-bindings: leds: Add SC27xx breathing light controller documentation

[Baolin Wang]

Hi Pavel,

On 9 May 2018 at 22:25, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> 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>
>> ---
>> Changes since v1:
>>  - Change the compatible string to be one explicit SoC name.
>>  - Change the child node name.
>>  - Change to be upper case for the first character.
>> ---
>>  .../devicetree/bindings/leds/leds-sc27xx-bltc.txt  |   41 ++++++++++++++++++++
>>  1 file changed, 41 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..b3de7fc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-sc27xx-bltc.txt
>> @@ -0,0 +1,41 @@
>> +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,sc2731-bltc".
>
> If this is for 2731, I'd say so in the title and the filename...?

I agree with Jacek's comments. We will add other PMICs belonging to
sc27xx series in future.

>
>> +- #address-cells: Must be 1.
>> +- #size-cells: Must be 0.
>> +- reg: Specify controller address.
>
> the controller...

Will fix.

>
>> +Required child properties:
>> +- reg: Number of LED line (could be from 0 to 2).
>
> Uff. "Port this LED is connected to"?

Will fix.

>
> With that fixed...
>
> Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks for your comments.

-- 
Baolin.wang
Best Regards

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Baolin Wang]

Hi Pavel,

On 9 May 2018 at 22:25, Pavel Machek <pavel@ucw.cz> wrote:
> On Tue 2018-05-08 13:39:45, 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.
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> new file mode 100644
>> index 0000000..22166fb
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> @@ -0,0 +1,19 @@
>> +What:                /sys/class/leds/<led>/rise_time
>> +What:                /sys/class/leds/<led>/high_time
>> +What:                /sys/class/leds/<led>/fall_time
>> +What:                /sys/class/leds/<led>/low_time
>> +Date:                May 2018
>> +KernelVersion:       4.18
>> +Contact:     Xiaotong Lu <xiaotong.lu@spreadtrum.com>
>> +Description:
>> +             Set the pattern generator rise, high, fall and low
>> +             times (0..63). It's unit is 0.125s, it should be > 0.
>> +
>> +             1 - 125 ms
>> +             2 - 250 ms
>> +             3 - 375 ms
>> +             ...
>> +             ...
>> +             ...
>> +             62 - 7.75 s
>> +             63 - 7.875 s
>
> How does this interact with triggers? With manually setting
> brightness? Are the pattern generators independend for the LEDs?

With depending on what mode user selected. If user set the
raise/high/fall/low time values, that means the LED will work at
breathing mode, and we set brightness to turn on/off the breathing
mode LED.

Yes, they are independent for each LED.

>
> Can you generate white breathing pattern? If so, how?

Yes, firstly we should set the raise/high/fall/low time values for red
LED, blue LED and green LED (each LED time values are same), then set
one brightness value to turn on the red LED, blue LED and green LED.

>
> How do you select between normal and breathing modes?

As I explained, when user set the raise/high/fall/low time values,
that means user selects the breathing mode of the LED. Otherwise the
LED works at normal mode.

>
> I'd specify times in miliseconds or something, this is way too
> hardware specific.

But our hardware specification defines magic numbers, not times values
in milliseconds. So I think we should follow our specification's
definition, and user should also follow the same rules.

>
> Now... functionality like this is common between many LED
> controllers. N900 could do this kind of "breathing", too, and it also
> supports other patterns.
>
> I believe we need interface common between different LED controllers.
>
> And I guess it would be easiest if you dropped this part from initial
> merge.
>
> Thanks and best regards,
>
>                                                                 Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html



-- 
Baolin.wang
Best Regards

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]

Hi!

> >>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.
> >>
> >>diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
> >>new file mode 100644
> >>index 0000000..22166fb
> >>--- /dev/null
> >>+++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
> >>@@ -0,0 +1,19 @@
> >>+What:		/sys/class/leds/<led>/rise_time
> >>+What:		/sys/class/leds/<led>/high_time
> >>+What:		/sys/class/leds/<led>/fall_time
> >>+What:		/sys/class/leds/<led>/low_time
> >>+Date:		May 2018
> >>+KernelVersion:	4.18
> >>+Contact:	Xiaotong Lu <xiaotong.lu@spreadtrum.com>
> >>+Description:
> >>+		Set the pattern generator rise, high, fall and low
> >>+		times (0..63). It's unit is 0.125s, it should be > 0.
> >>+
> >>+		1 - 125 ms
> >>+		2 - 250 ms
> >>+		3 - 375 ms
> >>+		...
> >>+		...
> >>+		...
> >>+		62 - 7.75 s
> >>+		63 - 7.875 s
> >
> >How does this interact with triggers? With manually setting
> >brightness? Are the pattern generators independend for the LEDs?
> >
> >Can you generate white breathing pattern? If so, how?
> >
> >How do you select between normal and breathing modes?
> >
> >I'd specify times in miliseconds or something, this is way too
> >hardware specific.
> 
> Agreed.
> 
> >Now... functionality like this is common between many LED
> >controllers. N900 could do this kind of "breathing", too, and it also
> >supports other patterns.
> >
> >I believe we need interface common between different LED controllers.
> >
> >And I guess it would be easiest if you dropped this part from initial
> >merge.
> 
> I disagree here. We already had the same discussion at the occasion
> of the patch [0] and it turned out to be a dead-end [1]. Now we have
> neither the driver nor the generic pattern interface.
> 
> We also already have some older LED class drivers that implement custom
> pattern interfaces (e.g. drivers/leds/leds-lm3533.c) and the same
> approach can be applied in this case.

Please don't. It was mistake to implement custom pattern interfaces
back then, it is still mistake now.
 
If we really need solution now, I'd recommend "pattern" file with

"<delta time> <brightness> <delta time> <brightness>".

In this specific case, hardware only supports patterns in this format:

low_time 0 rise_time 255 high_time 255 fall_time 0

so driver would simply -EINVAL on anything else.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Jacek Anaszewski]

Hi Pavel,

On 05/10/2018 01:37 PM, Pavel Machek wrote:
> Hi!
> 
>>>> 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.
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>>>> new file mode 100644
>>>> index 0000000..22166fb
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>>>> @@ -0,0 +1,19 @@
>>>> +What:		/sys/class/leds/<led>/rise_time
>>>> +What:		/sys/class/leds/<led>/high_time
>>>> +What:		/sys/class/leds/<led>/fall_time
>>>> +What:		/sys/class/leds/<led>/low_time
>>>> +Date:		May 2018
>>>> +KernelVersion:	4.18
>>>> +Contact:	Xiaotong Lu <xiaotong.lu@spreadtrum.com>
>>>> +Description:
>>>> +		Set the pattern generator rise, high, fall and low
>>>> +		times (0..63). It's unit is 0.125s, it should be > 0.
>>>> +
>>>> +		1 - 125 ms
>>>> +		2 - 250 ms
>>>> +		3 - 375 ms
>>>> +		...
>>>> +		...
>>>> +		...
>>>> +		62 - 7.75 s
>>>> +		63 - 7.875 s
>>>
>>> How does this interact with triggers? With manually setting
>>> brightness? Are the pattern generators independend for the LEDs?
>>>
>>> Can you generate white breathing pattern? If so, how?
>>>
>>> How do you select between normal and breathing modes?
>>>
>>> I'd specify times in miliseconds or something, this is way too
>>> hardware specific.
>>
>> Agreed.
>>
>>> Now... functionality like this is common between many LED
>>> controllers. N900 could do this kind of "breathing", too, and it also
>>> supports other patterns.
>>>
>>> I believe we need interface common between different LED controllers.
>>>
>>> And I guess it would be easiest if you dropped this part from initial
>>> merge.
>>
>> I disagree here. We already had the same discussion at the occasion
>> of the patch [0] and it turned out to be a dead-end [1]. Now we have
>> neither the driver nor the generic pattern interface.
>>
>> We also already have some older LED class drivers that implement custom
>> pattern interfaces (e.g. drivers/leds/leds-lm3533.c) and the same
>> approach can be applied in this case.
> 
> Please don't. It was mistake to implement custom pattern interfaces
> back then, it is still mistake now.

It turned out to be really hard to cover all known pattern generator
implementations with generic interface. Sure, it would be nice to have
one, but the whole discussion around [0] only unveiled the diversity of
parameters to cover. And still new devices appear on the market.

We would have to propose a set of pattern schemes and allow to
add new ones to it.

> If we really need solution now, I'd recommend "pattern" file with
> 
> "<delta time> <brightness> <delta time> <brightness>".
> 
> In this specific case, hardware only supports patterns in this format:
> 
> low_time 0 rise_time 255 high_time 255 fall_time 0
> 
> so driver would simply -EINVAL on anything else.

I'm fine with the pattern file, but the pattern format would have
to be defined in the per-driver ABI documentation. It wouldn't much
differ from the custom pattern approach though, unless I'm missing some
gain of having pattern setting in a uniformly named single sysfs file
(with semantics differing from driver to driver).

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2411 bytes --]

Hi!

> >>I disagree here. We already had the same discussion at the occasion
> >>of the patch [0] and it turned out to be a dead-end [1]. Now we have
> >>neither the driver nor the generic pattern interface.
> >>
> >>We also already have some older LED class drivers that implement custom
> >>pattern interfaces (e.g. drivers/leds/leds-lm3533.c) and the same
> >>approach can be applied in this case.
> >
> >Please don't. It was mistake to implement custom pattern interfaces
> >back then, it is still mistake now.
> 
> It turned out to be really hard to cover all known pattern generator
> implementations with generic interface. Sure, it would be nice to have
> one, but the whole discussion around [0] only unveiled the diversity of
> parameters to cover. And still new devices appear on the market.
> 
> We would have to propose a set of pattern schemes and allow to
> add new ones to it.

I believe that what I'm proposing below is close enough to universal.

> >If we really need solution now, I'd recommend "pattern" file with
> >
> >"<delta time> <brightness> <delta time> <brightness>".
> >
> >In this specific case, hardware only supports patterns in this format:
> >
> >low_time 0 rise_time 255 high_time 255 fall_time 0
> >
> >so driver would simply -EINVAL on anything else.
> 
> I'm fine with the pattern file, but the pattern format would have
> to be defined in the per-driver ABI documentation. It wouldn't much
> differ from the custom pattern approach though, unless I'm missing some
> gain of having pattern setting in a uniformly named single sysfs file
> (with semantics differing from driver to driver).

I'm proposing "<delta time> <brightness> ..." sysfs file. It certainly
covers this hardware, it would be enough to cover the Qualcomm Pulse
generator (IIRC), and it would cover most uses cases of Nokia N900's
LED.

Yes, we would need to document limitations of each chip. But it should
be easily possible to run pattern designed for Spreadtrum on N900,
even if it would not work the other way around.

(If someone really wants to run complex patterns on simple hardware,
we can provide software emulation using same file format. I believe I
still have that patch somewhere.)

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Jacek Anaszewski]

Hi Pavel,

On 05/12/2018 10:35 AM, Pavel Machek wrote:
> Hi!
>
>>>> I disagree here. We already had the same discussion at the occasion
>>>> of the patch [0] and it turned out to be a dead-end [1]. Now we have
>>>> neither the driver nor the generic pattern interface.
>>>>
>>>> We also already have some older LED class drivers that implement custom
>>>> pattern interfaces (e.g. drivers/leds/leds-lm3533.c) and the same
>>>> approach can be applied in this case.
>>>
>>> Please don't. It was mistake to implement custom pattern interfaces
>>> back then, it is still mistake now.
>>
>> It turned out to be really hard to cover all known pattern generator
>> implementations with generic interface. Sure, it would be nice to have
>> one, but the whole discussion around [0] only unveiled the diversity of
>> parameters to cover. And still new devices appear on the market.
>>
>> We would have to propose a set of pattern schemes and allow to
>> add new ones to it.
>
> I believe that what I'm proposing below is close enough to universal.
>
>>> If we really need solution now, I'd recommend "pattern" file with
>>>
>>> "<delta time> <brightness> <delta time> <brightness>".
>>>
>>> In this specific case, hardware only supports patterns in this format:
>>>
>>> low_time 0 rise_time 255 high_time 255 fall_time 0
>>>
>>> so driver would simply -EINVAL on anything else.
>>
>> I'm fine with the pattern file, but the pattern format would have
>> to be defined in the per-driver ABI documentation. It wouldn't much
>> differ from the custom pattern approach though, unless I'm missing some
>> gain of having pattern setting in a uniformly named single sysfs file
>> (with semantics differing from driver to driver).
>
> I'm proposing "<delta time> <brightness> ..." sysfs file. It certainly
> covers this hardware, it would be enough to cover the Qualcomm Pulse
> generator (IIRC), and it would cover most uses cases of Nokia N900's
> LED.
>
> Yes, we would need to document limitations of each chip. But it should
> be easily possible to run pattern designed for Spreadtrum on N900,
> even if it would not work the other way around.
>
> (If someone really wants to run complex patterns on simple hardware,
> we can provide software emulation using same file format. I believe I
> still have that patch somewhere.)

OK, I've revised the discussion under Qualcomm LPG patch set and
it seems that we have almost ready solution in [0], except the
pattern_repeat file you mention in [1]. So probably Baolin could
address your remarks from [1] and add pattern_repeat file to the
patch that begins thread [0].

[0] https://lkml.org/lkml/2017/11/15/27
[1] https://lkml.org/lkml/2017/12/8/470

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Baolin Wang]

Hi Jacek and Pavel,

On 13 May 2018 at 04:44, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> Hi Pavel,
>
>
> On 05/12/2018 10:35 AM, Pavel Machek wrote:
>>
>> Hi!
>>
>>>>> I disagree here. We already had the same discussion at the occasion
>>>>> of the patch [0] and it turned out to be a dead-end [1]. Now we have
>>>>> neither the driver nor the generic pattern interface.
>>>>>
>>>>> We also already have some older LED class drivers that implement custom
>>>>> pattern interfaces (e.g. drivers/leds/leds-lm3533.c) and the same
>>>>> approach can be applied in this case.
>>>>
>>>>
>>>> Please don't. It was mistake to implement custom pattern interfaces
>>>> back then, it is still mistake now.
>>>
>>>
>>> It turned out to be really hard to cover all known pattern generator
>>> implementations with generic interface. Sure, it would be nice to have
>>> one, but the whole discussion around [0] only unveiled the diversity of
>>> parameters to cover. And still new devices appear on the market.
>>>
>>> We would have to propose a set of pattern schemes and allow to
>>> add new ones to it.
>>
>>
>> I believe that what I'm proposing below is close enough to universal.
>>
>>>> If we really need solution now, I'd recommend "pattern" file with
>>>>
>>>> "<delta time> <brightness> <delta time> <brightness>".
>>>>
>>>> In this specific case, hardware only supports patterns in this format:
>>>>
>>>> low_time 0 rise_time 255 high_time 255 fall_time 0
>>>>
>>>> so driver would simply -EINVAL on anything else.
>>>
>>>
>>> I'm fine with the pattern file, but the pattern format would have
>>> to be defined in the per-driver ABI documentation. It wouldn't much
>>> differ from the custom pattern approach though, unless I'm missing some
>>> gain of having pattern setting in a uniformly named single sysfs file
>>> (with semantics differing from driver to driver).
>>
>>
>> I'm proposing "<delta time> <brightness> ..." sysfs file. It certainly
>> covers this hardware, it would be enough to cover the Qualcomm Pulse
>> generator (IIRC), and it would cover most uses cases of Nokia N900's
>> LED.
>>
>> Yes, we would need to document limitations of each chip. But it should
>> be easily possible to run pattern designed for Spreadtrum on N900,
>> even if it would not work the other way around.
>>
>> (If someone really wants to run complex patterns on simple hardware,
>> we can provide software emulation using same file format. I believe I
>> still have that patch somewhere.)
>
>
> OK, I've revised the discussion under Qualcomm LPG patch set and
> it seems that we have almost ready solution in [0], except the
> pattern_repeat file you mention in [1]. So probably Baolin could
> address your remarks from [1] and add pattern_repeat file to the
> patch that begins thread [0].
>
> [0] https://lkml.org/lkml/2017/11/15/27
> [1] https://lkml.org/lkml/2017/12/8/470

Thanks for your suggestion. So I will remove the sysfs part from the
new driver, then send incremental patches when introducing some common
LED interfaces.

-- 
Baolin.wang
Best Regards

Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Baolin Wang]

Hi Jacek and Bjorn,

> OK, I've revised the discussion under Qualcomm LPG patch set and
> it seems that we have almost ready solution in [0], except the
> pattern_repeat file you mention in [1]. So probably Baolin could
> address your remarks from [1] and add pattern_repeat file to the
> patch that begins thread [0].
>
> [0] https://lkml.org/lkml/2017/11/15/27
> [1] https://lkml.org/lkml/2017/12/8/470

I did not see Bjorn's patch introducing the 'pattern' and
'pattern_repeat' interfaces was resend, but we have some requirements
depend on this patch for Spreadtrum LED controller.

Bjorn, will you resend your patchset? and I will help to test it. Or
can I help to resend the patch with keeping your authorization?
Thanks.

-- 
Baolin.wang
Best Regards