[PATCH 0/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[PATCH 0/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[Florian Vaussard]

Hello,

This series add a new driver for On Semiconductor NCP5623, a 3-channel I2C
LED driver. It is used in our design to drive a RGB LED.

The first patch introduces the device tree binding, while the second patch
adds the driver itself.

Best regards,
Florian

Florian Vaussard (2):
  leds: ncp5623: Add device tree binding documentation
  leds: Add driver for NCP5623 3-channel I2C LED driver

 .../devicetree/bindings/leds/leds-ncp5623.txt      |  44 ++++
 drivers/leds/Kconfig                               |  11 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-ncp5623.c                        | 265 +++++++++++++++++++++
 4 files changed, 321 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt
 create mode 100644 drivers/leds/leds-ncp5623.c

-- 
2.5.0

[PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Florian Vaussard]

Add device tree binding documentation for On Semiconductor NCP5623 I2C
LED driver. The driver can independently control the PWM of the 3
channels with 32 levels of intensity.

The current delivered by the current source can be controlled using the
led-max-microamp property. In order to control this value, it is also
necessary to know the current on the Iref pin, hence the
onnn,led-iref-microamp property. It is usually set using an external
bias resistor, following Iref = Vref/Rbias with Vref=0.6V.

Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
---
 .../devicetree/bindings/leds/leds-ncp5623.txt      | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
new file mode 100644
index 0000000..0dc8345
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
@@ -0,0 +1,44 @@
+* ON Semiconductor - NCP5623 3-Channel LED Driver
+
+The NCP5623 is a 3-channel I2C LED driver. The brightness of each
+channel can be independently set using 32 levels. Each LED is represented
+as a sub-node of the device.
+
+Required properties:
+  - compatible: Should be "onnn,ncp5623"
+  - reg: I2C slave address (fixed to 0x38)
+  - #address-cells: must be 1
+  - #size-cells: must be 0
+  - onnn,led-iref-microamp: Current on the Iref pin in microampere
+  - led-max-microamp: Desired maximum current for each LED in microampere
+                      (maximum 30000uA)
+
+LED sub-node properties:
+  - reg : LED channel number (0..2)
+  - For other LED properties see:
+      Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+led1: ncp5623@38 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "onnn,ncp5623";
+	reg = <0x38>;
+
+	led1_r@0 {
+		label = "ncp:power:red";
+		reg = <0>;
+		linux,default-trigger = "default-on";
+	};
+
+	led1_b@1 {
+		label = "ncp:power:blue";
+		reg = <1>;
+	};
+
+	led1_g@2 {
+		label = "ncp:power:green";
+		reg = <2>;
+	};
+};
-- 
2.5.0

[PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[Florian Vaussard]

The NCP5623 is a 3-channel LED driver from On Semiconductor controlled
through I2C. The PWM of each channel can be independently set with 32
distinct levels. In addition, the intensity of the current source can be
globally set using an external bias resistor fixing the reference
current (Iref) and a dedicated register (ILED), following the
relationship:

I = 2400*Iref/(31-ILED)

with Iref = Vref/Rbias, and Vref = 0.6V.

Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
---
 drivers/leds/Kconfig        |  11 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-ncp5623.c | 265 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 277 insertions(+)
 create mode 100644 drivers/leds/leds-ncp5623.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 5ae2834..6d3e44d 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -588,6 +588,17 @@ config LEDS_BLINKM
 	  This option enables support for the BlinkM RGB LED connected
 	  through I2C. Say Y to enable support for the BlinkM LED.
 
+config LEDS_NCP5623
+	tristate "LED Support for NCP5623 I2C chip"
+	depends on LEDS_CLASS
+	depends on I2C
+	help
+	  This option enables support for LEDs connected to NCP5623
+	  LED driver chips accessed via the I2C bus.
+	  Driver supports brightness control.
+
+	  Say Y to enable this driver.
+
 config LEDS_POWERNV
 	tristate "LED support for PowerNV Platform"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index cb2013d..a2f0e10 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
 obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
 obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
 obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
+obj-$(CONFIG_LEDS_NCP5623)		+= leds-ncp5623.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c
new file mode 100644
index 0000000..a341e4a
--- /dev/null
+++ b/drivers/leds/leds-ncp5623.c
@@ -0,0 +1,265 @@
+/*
+ * Copyright 2016 Florian Vaussard <florian.vaussard@heig-vd.ch>
+ *
+ * Based on leds-tlc591xx.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+
+#include <linux/bitops.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+#define NCP5623_MAX_LEDS	3
+#define NCP5623_MAX_STEPS	32
+#define NCP5623_MAX_CURRENT	31
+#define NCP5623_MAX_CURRENT_UA	30000
+
+#define NCP5623_CMD_SHIFT	5
+#define CMD_SHUTDOWN		(0x00 << NCP5623_CMD_SHIFT)
+#define CMD_ILED		(0x01 << NCP5623_CMD_SHIFT)
+#define CMD_PWM1		(0x02 << NCP5623_CMD_SHIFT)
+#define CMD_PWM2		(0x03 << NCP5623_CMD_SHIFT)
+#define CMD_PWM3		(0x04 << NCP5623_CMD_SHIFT)
+#define CMD_UPWARD_DIM		(0x05 << NCP5623_CMD_SHIFT)
+#define CMD_DOWNWARD_DIM	(0x06 << NCP5623_CMD_SHIFT)
+#define CMD_DIM_STEP		(0x07 << NCP5623_CMD_SHIFT)
+
+#define NCP5623_DATA_MASK	GENMASK(NCP5623_CMD_SHIFT - 1, 0)
+
+#define NCP5623_CMD(cmd, data)	(cmd | (data & NCP5623_DATA_MASK))
+
+struct ncp5623_led {
+	bool active;
+	unsigned int led_no;
+	struct led_classdev ldev;
+	struct work_struct work;
+	struct ncp5623_priv *priv;
+};
+
+struct ncp5623_priv {
+	struct ncp5623_led leds[NCP5623_MAX_LEDS];
+	u32 led_iref;
+	u32 led_max_current;
+	struct i2c_client *client;
+};
+
+static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev)
+{
+	return container_of(ldev, struct ncp5623_led, ldev);
+}
+
+static struct ncp5623_led *work_to_led(struct work_struct *work)
+{
+	return container_of(work, struct ncp5623_led, work);
+}
+
+static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data)
+{
+	char cmd_data[1] = { NCP5623_CMD(cmd, data) };
+	int err;
+
+	err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data));
+	return (err < 0 ? err : 0);
+}
+
+static int ncp5623_set_pwm(struct ncp5623_led *led, u8 brightness)
+{
+	struct ncp5623_priv *priv = led->priv;
+	u8 cmd;
+
+	switch (led->led_no) {
+	case 0:
+		cmd = CMD_PWM1;
+		break;
+	case 1:
+		cmd = CMD_PWM2;
+		break;
+	case 2:
+		cmd = CMD_PWM3;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ncp5623_send_cmd(priv, cmd, brightness);
+}
+
+static void ncp5623_led_work(struct work_struct *work)
+{
+	struct ncp5623_led *led = work_to_led(work);
+	enum led_brightness brightness = led->ldev.brightness;
+	int err;
+
+	err = ncp5623_set_pwm(led, brightness);
+
+	if (err < 0)
+		dev_err(led->ldev.dev, "failed setting brightness\n");
+}
+
+static void ncp5623_brightness_set(struct led_classdev *led_cdev,
+				   enum led_brightness brightness)
+{
+	struct ncp5623_led *led = ldev_to_led(led_cdev);
+
+	led->ldev.brightness = brightness;
+	schedule_work(&led->work);
+}
+
+static void ncp5623_destroy_devices(struct ncp5623_priv *priv)
+{
+	struct ncp5623_led *led;
+	int i;
+
+	for (i = 0; i < NCP5623_MAX_LEDS; i++) {
+		led = &priv->leds[i];
+		if (led->active) {
+			led_classdev_unregister(&led->ldev);
+			cancel_work_sync(&led->work);
+		}
+	}
+}
+
+static int ncp5623_configure(struct device *dev,
+			     struct ncp5623_priv *priv)
+{
+	unsigned int i;
+	unsigned int n;
+	struct ncp5623_led *led;
+	int err;
+
+	/* Compute the value of ILED register to honor led_max_current */
+	n = 2400 * priv->led_iref / priv->led_max_current + 1;
+	if (n > NCP5623_MAX_CURRENT)
+		n = NCP5623_MAX_CURRENT;
+	n = NCP5623_MAX_CURRENT - n;
+
+	dev_dbg(dev, "setting maximum current to %u uA\n",
+		2400 * priv->led_iref / (NCP5623_MAX_CURRENT - n));
+
+	err = ncp5623_send_cmd(priv, CMD_ILED, n);
+	if (err < 0) {
+		dev_err(dev, "cannot set the current\n");
+		return err;
+	}
+
+	/* Setup each individual LED */
+	for (i = 0; i < NCP5623_MAX_LEDS; i++) {
+		led = &priv->leds[i];
+
+		if (!led->active)
+			continue;
+
+		led->priv = priv;
+		led->led_no = i;
+		led->ldev.brightness_set = ncp5623_brightness_set;
+		led->ldev.max_brightness = NCP5623_MAX_STEPS - 1;
+		INIT_WORK(&led->work, ncp5623_led_work);
+		err = led_classdev_register(dev, &led->ldev);
+		if (err < 0) {
+			dev_err(dev, "couldn't register LED %s\n",
+				led->ldev.name);
+			goto exit;
+		}
+	}
+
+	return 0;
+
+exit:
+	ncp5623_destroy_devices(priv);
+	return err;
+}
+
+static const struct of_device_id ncp5623_of_match[] = {
+	{ .compatible = "onnn,ncp5623" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ncp5623_of_match);
+
+static int ncp5623_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct device_node *np = dev->of_node, *child;
+	struct ncp5623_priv *priv;
+	struct ncp5623_led *led;
+	u32 reg;
+	int err, count;
+
+	count = of_get_child_count(np);
+	if (!count || count > NCP5623_MAX_LEDS)
+		return -EINVAL;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+
+	i2c_set_clientdata(client, priv);
+
+	err = of_property_read_u32(np, "onnn,led-iref-microamp",
+				   &priv->led_iref);
+	if (err)
+		return -EINVAL;
+	err = of_property_read_u32(np, "led-max-microamp",
+				   &priv->led_max_current);
+	if (err || priv->led_max_current > NCP5623_MAX_CURRENT_UA)
+		return -EINVAL;
+
+	for_each_child_of_node(np, child) {
+		err = of_property_read_u32(child, "reg", &reg);
+		if (err)
+			return err;
+		if (reg < 0 || reg >= NCP5623_MAX_LEDS)
+			return -EINVAL;
+		led = &priv->leds[reg];
+		if (led->active)
+			return -EINVAL;
+		led->active = true;
+		led->ldev.name =
+			of_get_property(child, "label", NULL) ? : child->name;
+		led->ldev.default_trigger =
+			of_get_property(child, "linux,default-trigger", NULL);
+	}
+	return ncp5623_configure(dev, priv);
+}
+
+static int ncp5623_remove(struct i2c_client *client)
+{
+	struct ncp5623_priv *priv = i2c_get_clientdata(client);
+
+	ncp5623_destroy_devices(priv);
+
+	return 0;
+}
+
+static const struct i2c_device_id ncp5623_id[] = {
+	{ "ncp5623" },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ncp5623_id);
+
+static struct i2c_driver ncp5623_driver = {
+	.driver = {
+		.name = "ncp5623",
+		.of_match_table = of_match_ptr(ncp5623_of_match),
+	},
+	.probe = ncp5623_probe,
+	.remove = ncp5623_remove,
+	.id_table = ncp5623_id,
+};
+
+module_i2c_driver(ncp5623_driver);
+
+MODULE_AUTHOR("Florian Vaussard <florian.vaussard@heig-vd.ch>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("NCP5623 LED driver");
-- 
2.5.0

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Jacek Anaszewski]

Hi Florian,

Thanks for the patch. I have two remarks below.

On 06/21/2016 09:29 AM, Florian Vaussard wrote:
> Add device tree binding documentation for On Semiconductor NCP5623 I2C
> LED driver. The driver can independently control the PWM of the 3
> channels with 32 levels of intensity.
>
> The current delivered by the current source can be controlled using the
> led-max-microamp property. In order to control this value, it is also
> necessary to know the current on the Iref pin, hence the
> onnn,led-iref-microamp property. It is usually set using an external
> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> ---
>   .../devicetree/bindings/leds/leds-ncp5623.txt      | 44 ++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
> new file mode 100644
> index 0000000..0dc8345
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
> @@ -0,0 +1,44 @@
> +* ON Semiconductor - NCP5623 3-Channel LED Driver
> +
> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
> +channel can be independently set using 32 levels. Each LED is represented
> +as a sub-node of the device.
> +
> +Required properties:
> +  - compatible: Should be "onnn,ncp5623"
> +  - reg: I2C slave address (fixed to 0x38)
> +  - #address-cells: must be 1
> +  - #size-cells: must be 0
> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere

I think that you don't need this property. Just provide the formula for
calculating led-max-microamp value, similarly as you're doing that in
the commit message.

> +  - led-max-microamp: Desired maximum current for each LED in microampere
> +                      (maximum 30000uA)

Please add instead of (maximum ...):

Valid values: min - max, step by N (rounded {up|down})

E.g.

Valid values: 10000 - 30000, step by 1000 (rounded down)

> +
> +LED sub-node properties:
> +  - reg : LED channel number (0..2)
> +  - For other LED properties see:
> +      Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +led1: ncp5623@38 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "onnn,ncp5623";
> +	reg = <0x38>;
> +
> +	led1_r@0 {
> +		label = "ncp:power:red";
> +		reg = <0>;
> +		linux,default-trigger = "default-on";
> +	};
> +
> +	led1_b@1 {
> +		label = "ncp:power:blue";
> +		reg = <1>;
> +	};
> +
> +	led1_g@2 {
> +		label = "ncp:power:green";
> +		reg = <2>;
> +	};
> +};
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[Jacek Anaszewski]

Hi Florian,

Thanks for the patch. Please refer to my comments in the code.

On 06/21/2016 09:29 AM, Florian Vaussard wrote:
> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled
> through I2C. The PWM of each channel can be independently set with 32
> distinct levels. In addition, the intensity of the current source can be
> globally set using an external bias resistor fixing the reference
> current (Iref) and a dedicated register (ILED), following the
> relationship:
>
> I = 2400*Iref/(31-ILED)
>
> with Iref = Vref/Rbias, and Vref = 0.6V.
>
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> ---
>   drivers/leds/Kconfig        |  11 ++
>   drivers/leds/Makefile       |   1 +
>   drivers/leds/leds-ncp5623.c | 265 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 277 insertions(+)
>   create mode 100644 drivers/leds/leds-ncp5623.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 5ae2834..6d3e44d 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -588,6 +588,17 @@ config LEDS_BLINKM
>   	  This option enables support for the BlinkM RGB LED connected
>   	  through I2C. Say Y to enable support for the BlinkM LED.
>
> +config LEDS_NCP5623
> +	tristate "LED Support for NCP5623 I2C chip"
> +	depends on LEDS_CLASS
> +	depends on I2C
> +	help
> +	  This option enables support for LEDs connected to NCP5623
> +	  LED driver chips accessed via the I2C bus.
> +	  Driver supports brightness control.
> +
> +	  Say Y to enable this driver.
> +
>   config LEDS_POWERNV
>   	tristate "LED support for PowerNV Platform"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index cb2013d..a2f0e10 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
>   obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>   obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
>   obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o
> +obj-$(CONFIG_LEDS_NCP5623)		+= leds-ncp5623.o
>
>   # LED SPI Drivers
>   obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c
> new file mode 100644
> index 0000000..a341e4a
> --- /dev/null
> +++ b/drivers/leds/leds-ncp5623.c
> @@ -0,0 +1,265 @@
> +/*
> + * Copyright 2016 Florian Vaussard <florian.vaussard@heig-vd.ch>
> + *
> + * Based on leds-tlc591xx.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +#define NCP5623_MAX_LEDS	3
> +#define NCP5623_MAX_STEPS	32
> +#define NCP5623_MAX_CURRENT	31
> +#define NCP5623_MAX_CURRENT_UA	30000
> +
> +#define NCP5623_CMD_SHIFT	5
> +#define CMD_SHUTDOWN		(0x00 << NCP5623_CMD_SHIFT)
> +#define CMD_ILED		(0x01 << NCP5623_CMD_SHIFT)
> +#define CMD_PWM1		(0x02 << NCP5623_CMD_SHIFT)
> +#define CMD_PWM2		(0x03 << NCP5623_CMD_SHIFT)
> +#define CMD_PWM3		(0x04 << NCP5623_CMD_SHIFT)
> +#define CMD_UPWARD_DIM		(0x05 << NCP5623_CMD_SHIFT)
> +#define CMD_DOWNWARD_DIM	(0x06 << NCP5623_CMD_SHIFT)
> +#define CMD_DIM_STEP		(0x07 << NCP5623_CMD_SHIFT)
> +
> +#define NCP5623_DATA_MASK	GENMASK(NCP5623_CMD_SHIFT - 1, 0)
> +
> +#define NCP5623_CMD(cmd, data)	(cmd | (data & NCP5623_DATA_MASK))
> +
> +struct ncp5623_led {
> +	bool active;
> +	unsigned int led_no;
> +	struct led_classdev ldev;
> +	struct work_struct work;
> +	struct ncp5623_priv *priv;
> +};
> +
> +struct ncp5623_priv {
> +	struct ncp5623_led leds[NCP5623_MAX_LEDS];

Please allocate memory dynamically, depending on the number
of LEDs defined in a Device Tree.

> +	u32 led_iref;
> +	u32 led_max_current;
> +	struct i2c_client *client;
> +};
> +
> +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev)
> +{
> +	return container_of(ldev, struct ncp5623_led, ldev);
> +}
> +
> +static struct ncp5623_led *work_to_led(struct work_struct *work)
> +{
> +	return container_of(work, struct ncp5623_led, work);
> +}

If you registered brightness_set_blocking op instead of brightness_set,
then the workqueue wouldn't be required in the driver.

> +static int ncp5623_send_cmd(struct ncp5623_priv *priv, u8 cmd, u8 data)
> +{
> +	char cmd_data[1] = { NCP5623_CMD(cmd, data) };
> +	int err;
> +
> +	err = i2c_master_send(priv->client, cmd_data, ARRAY_SIZE(cmd_data));

Please add empty line here.

> +	return (err < 0 ? err : 0);
> +}
> +
> +static int ncp5623_set_pwm(struct ncp5623_led *led, u8 brightness)
> +{
> +	struct ncp5623_priv *priv = led->priv;
> +	u8 cmd;
> +
> +	switch (led->led_no) {
> +	case 0:
> +		cmd = CMD_PWM1;
> +		break;
> +	case 1:
> +		cmd = CMD_PWM2;
> +		break;
> +	case 2:
> +		cmd = CMD_PWM3;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Macro for calculating cmd basing on led->led_no would allow
for getting rid of the switch statement.

> +
> +	return ncp5623_send_cmd(priv, cmd, brightness);
> +}
> +
> +static void ncp5623_led_work(struct work_struct *work)
> +{
> +	struct ncp5623_led *led = work_to_led(work);
> +	enum led_brightness brightness = led->ldev.brightness;
> +	int err;
> +
> +	err = ncp5623_set_pwm(led, brightness);
> +
> +	if (err < 0)
> +		dev_err(led->ldev.dev, "failed setting brightness\n");
> +}
> +
> +static void ncp5623_brightness_set(struct led_classdev *led_cdev,
> +				   enum led_brightness brightness)
> +{
> +	struct ncp5623_led *led = ldev_to_led(led_cdev);
> +
> +	led->ldev.brightness = brightness;

This is secured by LED core.

> +	schedule_work(&led->work);
> +}
> +
> +static void ncp5623_destroy_devices(struct ncp5623_priv *priv)
> +{
> +	struct ncp5623_led *led;
> +	int i;
> +
> +	for (i = 0; i < NCP5623_MAX_LEDS; i++) {
> +		led = &priv->leds[i];
> +		if (led->active) {
> +			led_classdev_unregister(&led->ldev);
> +			cancel_work_sync(&led->work);
> +		}
> +	}
> +}
> +
> +static int ncp5623_configure(struct device *dev,
> +			     struct ncp5623_priv *priv)
> +{
> +	unsigned int i;
> +	unsigned int n;
> +	struct ncp5623_led *led;
> +	int err;
> +
> +	/* Compute the value of ILED register to honor led_max_current */
> +	n = 2400 * priv->led_iref / priv->led_max_current + 1;
> +	if (n > NCP5623_MAX_CURRENT)
> +		n = NCP5623_MAX_CURRENT;
> +	n = NCP5623_MAX_CURRENT - n;
> +
> +	dev_dbg(dev, "setting maximum current to %u uA\n",
> +		2400 * priv->led_iref / (NCP5623_MAX_CURRENT - n));
> +
> +	err = ncp5623_send_cmd(priv, CMD_ILED, n);
> +	if (err < 0) {
> +		dev_err(dev, "cannot set the current\n");
> +		return err;
> +	}
> +
> +	/* Setup each individual LED */
> +	for (i = 0; i < NCP5623_MAX_LEDS; i++) {
> +		led = &priv->leds[i];
> +
> +		if (!led->active)
> +			continue;
> +
> +		led->priv = priv;
> +		led->led_no = i;
> +		led->ldev.brightness_set = ncp5623_brightness_set;
> +		led->ldev.max_brightness = NCP5623_MAX_STEPS - 1;

max_brightness should be asserted according to led-max-microamp
property.

> +		INIT_WORK(&led->work, ncp5623_led_work);

Please remove it and use brightness_set_blocking op.

> +		err = led_classdev_register(dev, &led->ldev);

Use devm prefixed version.

> +		if (err < 0) {
> +			dev_err(dev, "couldn't register LED %s\n",
> +				led->ldev.name);
> +			goto exit;
> +		}
> +	}
> +
> +	return 0;
> +
> +exit:
> +	ncp5623_destroy_devices(priv);
> +	return err;
> +}
> +
> +static const struct of_device_id ncp5623_of_match[] = {
> +	{ .compatible = "onnn,ncp5623" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ncp5623_of_match);
> +
> +static int ncp5623_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct device_node *np = dev->of_node, *child;
> +	struct ncp5623_priv *priv;
> +	struct ncp5623_led *led;
> +	u32 reg;
> +	int err, count;
> +
> +	count = of_get_child_count(np);
> +	if (!count || count > NCP5623_MAX_LEDS)
> +		return -EINVAL;

Please separate DT parsing from LED class device registration.
You could add ncp5623_parse_dt(). This is kind of standard.

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = client;
> +
> +	i2c_set_clientdata(client, priv);
> +
> +	err = of_property_read_u32(np, "onnn,led-iref-microamp",
> +				   &priv->led_iref);
> +	if (err)
> +		return -EINVAL;
> +	err = of_property_read_u32(np, "led-max-microamp",
> +				   &priv->led_max_current);
> +	if (err || priv->led_max_current > NCP5623_MAX_CURRENT_UA)
> +		return -EINVAL;
> +
> +	for_each_child_of_node(np, child) {
> +		err = of_property_read_u32(child, "reg", &reg);
> +		if (err)
> +			return err;
> +		if (reg < 0 || reg >= NCP5623_MAX_LEDS)
> +			return -EINVAL;

You need to call of_node_put if breaking this loop. Please refer
to the other LED class drivers.

> +		led = &priv->leds[reg];
> +		if (led->active)
> +			return -EINVAL;
> +		led->active = true;
> +		led->ldev.name =
> +			of_get_property(child, "label", NULL) ? : child->name;
> +		led->ldev.default_trigger =
> +			of_get_property(child, "linux,default-trigger", NULL);

Please mention also default-trigger property in the DT documenation.

> +	}
> +	return ncp5623_configure(dev, priv);
> +}
> +
> +static int ncp5623_remove(struct i2c_client *client)
> +{
> +	struct ncp5623_priv *priv = i2c_get_clientdata(client);
> +
> +	ncp5623_destroy_devices(priv);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ncp5623_id[] = {
> +	{ "ncp5623" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, ncp5623_id);
> +
> +static struct i2c_driver ncp5623_driver = {
> +	.driver = {
> +		.name = "ncp5623",
> +		.of_match_table = of_match_ptr(ncp5623_of_match),
> +	},
> +	.probe = ncp5623_probe,
> +	.remove = ncp5623_remove,
> +	.id_table = ncp5623_id,
> +};
> +
> +module_i2c_driver(ncp5623_driver);
> +
> +MODULE_AUTHOR("Florian Vaussard <florian.vaussard@heig-vd.ch>");
> +MODULE_LICENSE("GPL");

You mentioned GPL v2 at the top.

> +MODULE_DESCRIPTION("NCP5623 LED driver");
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Rob Herring]

On Tue, Jun 21, 2016 at 09:29:13AM +0200, Florian Vaussard wrote:
> Add device tree binding documentation for On Semiconductor NCP5623 I2C
> LED driver. The driver can independently control the PWM of the 3
> channels with 32 levels of intensity.
> 
> The current delivered by the current source can be controlled using the
> led-max-microamp property. In order to control this value, it is also
> necessary to know the current on the Iref pin, hence the
> onnn,led-iref-microamp property. It is usually set using an external
> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
> ---
>  .../devicetree/bindings/leds/leds-ncp5623.txt      | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
> new file mode 100644
> index 0000000..0dc8345
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
> @@ -0,0 +1,44 @@
> +* ON Semiconductor - NCP5623 3-Channel LED Driver
> +
> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
> +channel can be independently set using 32 levels. Each LED is represented
> +as a sub-node of the device.
> +
> +Required properties:
> +  - compatible: Should be "onnn,ncp5623"
> +  - reg: I2C slave address (fixed to 0x38)
> +  - #address-cells: must be 1
> +  - #size-cells: must be 0
> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
> +  - led-max-microamp: Desired maximum current for each LED in microampere
> +                      (maximum 30000uA)
> +
> +LED sub-node properties:
> +  - reg : LED channel number (0..2)
> +  - For other LED properties see:
> +      Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +led1: ncp5623@38 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "onnn,ncp5623";
> +	reg = <0x38>;
> +
> +	led1_r@0 {

Don't use underscores in node names.

> +		label = "ncp:power:red";
> +		reg = <0>;
> +		linux,default-trigger = "default-on";
> +	};
> +
> +	led1_b@1 {
> +		label = "ncp:power:blue";
> +		reg = <1>;
> +	};
> +
> +	led1_g@2 {
> +		label = "ncp:power:green";
> +		reg = <2>;
> +	};
> +};
> -- 
> 2.5.0
> 

Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[Florian Vaussard]

Hi Jacek,

Le 21. 06. 16 à 17:29, Jacek Anaszewski a écrit :
> Hi Florian,
> 
> Thanks for the patch. Please refer to my comments in the code.
> 

Thanks for your review. I completely agree with you, I will address your
comments in V2. I just have one small question below.

> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled
>> through I2C. The PWM of each channel can be independently set with 32
>> distinct levels. In addition, the intensity of the current source can be
>> globally set using an external bias resistor fixing the reference
>> current (Iref) and a dedicated register (ILED), following the
>> relationship:
>>
>> I = 2400*Iref/(31-ILED)
>>
>> with Iref = Vref/Rbias, and Vref = 0.6V.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>> ---
>>   drivers/leds/Kconfig        |  11 ++
>>   drivers/leds/Makefile       |   1 +
>>   drivers/leds/leds-ncp5623.c | 265 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 277 insertions(+)
>>   create mode 100644 drivers/leds/leds-ncp5623.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 5ae2834..6d3e44d 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -588,6 +588,17 @@ config LEDS_BLINKM
>>         This option enables support for the BlinkM RGB LED connected
>>         through I2C. Say Y to enable support for the BlinkM LED.
>>
>> +config LEDS_NCP5623
>> +    tristate "LED Support for NCP5623 I2C chip"
>> +    depends on LEDS_CLASS
>> +    depends on I2C
>> +    help
>> +      This option enables support for LEDs connected to NCP5623
>> +      LED driver chips accessed via the I2C bus.
>> +      Driver supports brightness control.
>> +
>> +      Say Y to enable this driver.
>> +
>>   config LEDS_POWERNV
>>       tristate "LED support for PowerNV Platform"
>>       depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index cb2013d..a2f0e10 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692)        += leds-ktd2692.o
>>   obj-$(CONFIG_LEDS_POWERNV)        += leds-powernv.o
>>   obj-$(CONFIG_LEDS_SEAD3)        += leds-sead3.o
>>   obj-$(CONFIG_LEDS_IS31FL32XX)        += leds-is31fl32xx.o
>> +obj-$(CONFIG_LEDS_NCP5623)        += leds-ncp5623.o
>>
>>   # LED SPI Drivers
>>   obj-$(CONFIG_LEDS_DAC124S085)        += leds-dac124s085.o
>> diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c
>> new file mode 100644
>> index 0000000..a341e4a
>> --- /dev/null
>> +++ b/drivers/leds/leds-ncp5623.c
>> @@ -0,0 +1,265 @@
>> +/*
>> + * Copyright 2016 Florian Vaussard <florian.vaussard@heig-vd.ch>
>> + *
>> + * Based on leds-tlc591xx.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; version 2 of the License.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/workqueue.h>
>> +
>> +#define NCP5623_MAX_LEDS    3
>> +#define NCP5623_MAX_STEPS    32
>> +#define NCP5623_MAX_CURRENT    31
>> +#define NCP5623_MAX_CURRENT_UA    30000
>> +
>> +#define NCP5623_CMD_SHIFT    5
>> +#define CMD_SHUTDOWN        (0x00 << NCP5623_CMD_SHIFT)
>> +#define CMD_ILED        (0x01 << NCP5623_CMD_SHIFT)
>> +#define CMD_PWM1        (0x02 << NCP5623_CMD_SHIFT)
>> +#define CMD_PWM2        (0x03 << NCP5623_CMD_SHIFT)
>> +#define CMD_PWM3        (0x04 << NCP5623_CMD_SHIFT)
>> +#define CMD_UPWARD_DIM        (0x05 << NCP5623_CMD_SHIFT)
>> +#define CMD_DOWNWARD_DIM    (0x06 << NCP5623_CMD_SHIFT)
>> +#define CMD_DIM_STEP        (0x07 << NCP5623_CMD_SHIFT)
>> +
>> +#define NCP5623_DATA_MASK    GENMASK(NCP5623_CMD_SHIFT - 1, 0)
>> +
>> +#define NCP5623_CMD(cmd, data)    (cmd | (data & NCP5623_DATA_MASK))
>> +

[snip]

>> +
>> +static int ncp5623_configure(struct device *dev,
>> +                 struct ncp5623_priv *priv)
>> +{
>> +    unsigned int i;
>> +    unsigned int n;
>> +    struct ncp5623_led *led;
>> +    int err;
>> +
>> +    /* Compute the value of ILED register to honor led_max_current */
>> +    n = 2400 * priv->led_iref / priv->led_max_current + 1;
>> +    if (n > NCP5623_MAX_CURRENT)
>> +        n = NCP5623_MAX_CURRENT;
>> +    n = NCP5623_MAX_CURRENT - n;
>> +
>> +    dev_dbg(dev, "setting maximum current to %u uA\n",
>> +        2400 * priv->led_iref / (NCP5623_MAX_CURRENT - n));
>> +
>> +    err = ncp5623_send_cmd(priv, CMD_ILED, n);
>> +    if (err < 0) {
>> +        dev_err(dev, "cannot set the current\n");
>> +        return err;
>> +    }
>> +
>> +    /* Setup each individual LED */
>> +    for (i = 0; i < NCP5623_MAX_LEDS; i++) {
>> +        led = &priv->leds[i];
>> +
>> +        if (!led->active)
>> +            continue;
>> +
>> +        led->priv = priv;
>> +        led->led_no = i;
>> +        led->ldev.brightness_set = ncp5623_brightness_set;
>> +        led->ldev.max_brightness = NCP5623_MAX_STEPS - 1;
> 
> max_brightness should be asserted according to led-max-microamp
> property.
> 

We can set two parameters on this chip:

- The intensity of the current source (common to all LEDs) from 0 to
NCP5623_MAX_CURRENT (31)
- The PWM for each LED independently from 0 to NCP5623_MAX_STEPS-1 (31)

My understanding was to use the led-max-microamp property to set up the current
source so that even with a duty cycle of 100% (PWM=31) the LED cannot be
damaged. In this case, I think that is unnecessary to assert max_brightness, as
we are guaranteed by design that led-max-microamp will not be surpassed for any
value of the PWM. Would you suggest another approach?

>> +        INIT_WORK(&led->work, ncp5623_led_work);
> 
> Please remove it and use brightness_set_blocking op.
> 
>> +        err = led_classdev_register(dev, &led->ldev);
> 
> Use devm prefixed version.
> 
>> +        if (err < 0) {
>> +            dev_err(dev, "couldn't register LED %s\n",
>> +                led->ldev.name);
>> +            goto exit;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +
>> +exit:
>> +    ncp5623_destroy_devices(priv);
>> +    return err;
>> +}
>> +

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Florian Vaussard]

Hi Jacek,

Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit :
> Hi Florian,
> 
> Thanks for the patch. I have two remarks below.
> 
> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>> LED driver. The driver can independently control the PWM of the 3
>> channels with 32 levels of intensity.
>>
>> The current delivered by the current source can be controlled using the
>> led-max-microamp property. In order to control this value, it is also
>> necessary to know the current on the Iref pin, hence the
>> onnn,led-iref-microamp property. It is usually set using an external
>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>> ---
>>   .../devicetree/bindings/leds/leds-ncp5623.txt      | 44 ++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>> new file mode 100644
>> index 0000000..0dc8345
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>> @@ -0,0 +1,44 @@
>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>> +
>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>> +channel can be independently set using 32 levels. Each LED is represented
>> +as a sub-node of the device.
>> +
>> +Required properties:
>> +  - compatible: Should be "onnn,ncp5623"
>> +  - reg: I2C slave address (fixed to 0x38)
>> +  - #address-cells: must be 1
>> +  - #size-cells: must be 0
>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
> 
> I think that you don't need this property. Just provide the formula for
> calculating led-max-microamp value, similarly as you're doing that in
> the commit message.
> 

I am not completely sure to understand your suggestion. So at the end, I have to
compute the value of the register (let call it 'ILED') that I need to send to
chip to configure the current source. The formula is:

ILED = 31 - 2400*Iref/led-max-microamp

I need two pieces of information that depends on the hardware setup; the current
on the Iref pin (which is setup by a bias resistor Iref = 0.6V/Rbias), and the
desired output current (usually limited due to the chosen LED or the required
brightness).

On the other hand, I could also simply ask people to put the right value of
'ILED' (between 0 - 31) directly in the device tree, but this is less user-friendly.

>> +  - led-max-microamp: Desired maximum current for each LED in microampere
>> +                      (maximum 30000uA)
> 
> Please add instead of (maximum ...):
> 
> Valid values: min - max, step by N (rounded {up|down})
> 
> E.g.
> 
> Valid values: 10000 - 30000, step by 1000 (rounded down)
> 

This is unfortunately not a linear relationship,

led-max-microamp = 2400*Iref/(31-ILED)

thus steps are not constant. This can be seen on figure 5 (p.9) of the datasheet
[1].

Thanks for your review,
Florian

[1] http://www.onsemi.com/pub/Collateral/NCP5623-D.PDF

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Florian Vaussard]

Hi Rob,

Le 21. 06. 16 à 23:52, Rob Herring a écrit :
> On Tue, Jun 21, 2016 at 09:29:13AM +0200, Florian Vaussard wrote:
>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>> LED driver. The driver can independently control the PWM of the 3
>> channels with 32 levels of intensity.
>>
>> The current delivered by the current source can be controlled using the
>> led-max-microamp property. In order to control this value, it is also
>> necessary to know the current on the Iref pin, hence the
>> onnn,led-iref-microamp property. It is usually set using an external
>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>> ---
>>  .../devicetree/bindings/leds/leds-ncp5623.txt      | 44 ++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>> new file mode 100644
>> index 0000000..0dc8345
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>> @@ -0,0 +1,44 @@
>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>> +
>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>> +channel can be independently set using 32 levels. Each LED is represented
>> +as a sub-node of the device.
>> +
>> +Required properties:
>> +  - compatible: Should be "onnn,ncp5623"
>> +  - reg: I2C slave address (fixed to 0x38)
>> +  - #address-cells: must be 1
>> +  - #size-cells: must be 0
>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
>> +  - led-max-microamp: Desired maximum current for each LED in microampere
>> +                      (maximum 30000uA)
>> +
>> +LED sub-node properties:
>> +  - reg : LED channel number (0..2)
>> +  - For other LED properties see:
>> +      Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Example:
>> +
>> +led1: ncp5623@38 {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	compatible = "onnn,ncp5623";
>> +	reg = <0x38>;
>> +
>> +	led1_r@0 {
> 
> Don't use underscores in node names.
> 

Sorry for the mistake, I will fix.

Thanks for the review,
Florian

Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[Jacek Anaszewski]

Hi Florian,

On 06/22/2016 08:08 AM, Florian Vaussard wrote:
> Hi Jacek,
>
> Le 21. 06. 16 à 17:29, Jacek Anaszewski a écrit :
>> Hi Florian,
>>
>> Thanks for the patch. Please refer to my comments in the code.
>>
>
> Thanks for your review. I completely agree with you, I will address your
> comments in V2. I just have one small question below.
>
>> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>>> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled
>>> through I2C. The PWM of each channel can be independently set with 32
>>> distinct levels. In addition, the intensity of the current source can be
>>> globally set using an external bias resistor fixing the reference
>>> current (Iref) and a dedicated register (ILED), following the
>>> relationship:
>>>
>>> I = 2400*Iref/(31-ILED)
>>>
>>> with Iref = Vref/Rbias, and Vref = 0.6V.
>>>
>>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>>> ---
>>>    drivers/leds/Kconfig        |  11 ++
>>>    drivers/leds/Makefile       |   1 +
>>>    drivers/leds/leds-ncp5623.c | 265 ++++++++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 277 insertions(+)
>>>    create mode 100644 drivers/leds/leds-ncp5623.c
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 5ae2834..6d3e44d 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -588,6 +588,17 @@ config LEDS_BLINKM
>>>          This option enables support for the BlinkM RGB LED connected
>>>          through I2C. Say Y to enable support for the BlinkM LED.
>>>
>>> +config LEDS_NCP5623
>>> +    tristate "LED Support for NCP5623 I2C chip"
>>> +    depends on LEDS_CLASS
>>> +    depends on I2C
>>> +    help
>>> +      This option enables support for LEDs connected to NCP5623
>>> +      LED driver chips accessed via the I2C bus.
>>> +      Driver supports brightness control.
>>> +
>>> +      Say Y to enable this driver.
>>> +
>>>    config LEDS_POWERNV
>>>        tristate "LED support for PowerNV Platform"
>>>        depends on LEDS_CLASS
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index cb2013d..a2f0e10 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692)        += leds-ktd2692.o
>>>    obj-$(CONFIG_LEDS_POWERNV)        += leds-powernv.o
>>>    obj-$(CONFIG_LEDS_SEAD3)        += leds-sead3.o
>>>    obj-$(CONFIG_LEDS_IS31FL32XX)        += leds-is31fl32xx.o
>>> +obj-$(CONFIG_LEDS_NCP5623)        += leds-ncp5623.o
>>>
>>>    # LED SPI Drivers
>>>    obj-$(CONFIG_LEDS_DAC124S085)        += leds-dac124s085.o
>>> diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c
>>> new file mode 100644
>>> index 0000000..a341e4a
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-ncp5623.c
>>> @@ -0,0 +1,265 @@
>>> +/*
>>> + * Copyright 2016 Florian Vaussard <florian.vaussard@heig-vd.ch>
>>> + *
>>> + * Based on leds-tlc591xx.c
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; version 2 of the License.
>>> + */
>>> +
>>> +#include <linux/bitops.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#define NCP5623_MAX_LEDS    3
>>> +#define NCP5623_MAX_STEPS    32
>>> +#define NCP5623_MAX_CURRENT    31
>>> +#define NCP5623_MAX_CURRENT_UA    30000
>>> +
>>> +#define NCP5623_CMD_SHIFT    5
>>> +#define CMD_SHUTDOWN        (0x00 << NCP5623_CMD_SHIFT)
>>> +#define CMD_ILED        (0x01 << NCP5623_CMD_SHIFT)
>>> +#define CMD_PWM1        (0x02 << NCP5623_CMD_SHIFT)
>>> +#define CMD_PWM2        (0x03 << NCP5623_CMD_SHIFT)
>>> +#define CMD_PWM3        (0x04 << NCP5623_CMD_SHIFT)
>>> +#define CMD_UPWARD_DIM        (0x05 << NCP5623_CMD_SHIFT)
>>> +#define CMD_DOWNWARD_DIM    (0x06 << NCP5623_CMD_SHIFT)
>>> +#define CMD_DIM_STEP        (0x07 << NCP5623_CMD_SHIFT)
>>> +
>>> +#define NCP5623_DATA_MASK    GENMASK(NCP5623_CMD_SHIFT - 1, 0)
>>> +
>>> +#define NCP5623_CMD(cmd, data)    (cmd | (data & NCP5623_DATA_MASK))
>>> +
>
> [snip]
>
>>> +
>>> +static int ncp5623_configure(struct device *dev,
>>> +                 struct ncp5623_priv *priv)
>>> +{
>>> +    unsigned int i;
>>> +    unsigned int n;
>>> +    struct ncp5623_led *led;
>>> +    int err;
>>> +
>>> +    /* Compute the value of ILED register to honor led_max_current */
>>> +    n = 2400 * priv->led_iref / priv->led_max_current + 1;
>>> +    if (n > NCP5623_MAX_CURRENT)
>>> +        n = NCP5623_MAX_CURRENT;
>>> +    n = NCP5623_MAX_CURRENT - n;
>>> +
>>> +    dev_dbg(dev, "setting maximum current to %u uA\n",
>>> +        2400 * priv->led_iref / (NCP5623_MAX_CURRENT - n));
>>> +
>>> +    err = ncp5623_send_cmd(priv, CMD_ILED, n);
>>> +    if (err < 0) {
>>> +        dev_err(dev, "cannot set the current\n");
>>> +        return err;
>>> +    }
>>> +
>>> +    /* Setup each individual LED */
>>> +    for (i = 0; i < NCP5623_MAX_LEDS; i++) {
>>> +        led = &priv->leds[i];
>>> +
>>> +        if (!led->active)
>>> +            continue;
>>> +
>>> +        led->priv = priv;
>>> +        led->led_no = i;
>>> +        led->ldev.brightness_set = ncp5623_brightness_set;
>>> +        led->ldev.max_brightness = NCP5623_MAX_STEPS - 1;
>>
>> max_brightness should be asserted according to led-max-microamp
>> property.
>>
>
> We can set two parameters on this chip:
>
> - The intensity of the current source (common to all LEDs) from 0 to
> NCP5623_MAX_CURRENT (31)
> - The PWM for each LED independently from 0 to NCP5623_MAX_STEPS-1 (31)
>
> My understanding was to use the led-max-microamp property to set up the current
> source so that even with a duty cycle of 100% (PWM=31) the LED cannot be
> damaged. In this case, I think that is unnecessary to assert max_brightness, as
> we are guaranteed by design that led-max-microamp will not be surpassed for any
> value of the PWM. Would you suggest another approach?

Yes, if step 31 reflects led-max-microamp value, then max_brightness
should be 31, regardless of led-max-microamp value.

>>> +        INIT_WORK(&led->work, ncp5623_led_work);
>>
>> Please remove it and use brightness_set_blocking op.
>>
>>> +        err = led_classdev_register(dev, &led->ldev);
>>
>> Use devm prefixed version.
>>
>>> +        if (err < 0) {
>>> +            dev_err(dev, "couldn't register LED %s\n",
>>> +                led->ldev.name);
>>> +            goto exit;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +exit:
>>> +    ncp5623_destroy_devices(priv);
>>> +    return err;
>>> +}
>>> +
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Jacek Anaszewski]

Hi Florian,

On 06/22/2016 08:08 AM, Florian Vaussard wrote:
> Hi Jacek,
>
> Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit :
>> Hi Florian,
>>
>> Thanks for the patch. I have two remarks below.
>>
>> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>>> LED driver. The driver can independently control the PWM of the 3
>>> channels with 32 levels of intensity.
>>>
>>> The current delivered by the current source can be controlled using the
>>> led-max-microamp property. In order to control this value, it is also
>>> necessary to know the current on the Iref pin, hence the
>>> onnn,led-iref-microamp property. It is usually set using an external
>>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>>
>>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>>> ---
>>>    .../devicetree/bindings/leds/leds-ncp5623.txt      | 44 ++++++++++++++++++++++
>>>    1 file changed, 44 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>> new file mode 100644
>>> index 0000000..0dc8345
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>> @@ -0,0 +1,44 @@
>>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>>> +
>>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>>> +channel can be independently set using 32 levels. Each LED is represented
>>> +as a sub-node of the device.
>>> +
>>> +Required properties:
>>> +  - compatible: Should be "onnn,ncp5623"
>>> +  - reg: I2C slave address (fixed to 0x38)
>>> +  - #address-cells: must be 1
>>> +  - #size-cells: must be 0
>>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
>>
>> I think that you don't need this property. Just provide the formula for
>> calculating led-max-microamp value, similarly as you're doing that in
>> the commit message.
>>
>
> I am not completely sure to understand your suggestion. So at the end, I have to
> compute the value of the register (let call it 'ILED') that I need to send to
> chip to configure the current source. The formula is:
>
> ILED = 31 - 2400*Iref/led-max-microamp

led-max-microamp is the maximum current value for given LED.
According to the documentation it can be calculated as follows:

ILEDmax = Iref * 2400 / (31 - n)

Since this is global setting for all LEDs, then I'd always set n to 30,
and calculate max_brightness value for each LED separately, basing on
led-max-microamp property value. Effectively, I'm revoking my previous
statement about setting max_brightness to fixed level.

You can compare drivers/leds/leds-aat1290.c and its bindings, as it
uses similar approach.

>
> I need two pieces of information that depends on the hardware setup; the current
> on the Iref pin (which is setup by a bias resistor Iref = 0.6V/Rbias), and the
> desired output current (usually limited due to the chosen LED or the required
> brightness).
>
> On the other hand, I could also simply ask people to put the right value of
> 'ILED' (between 0 - 31) directly in the device tree, but this is less user-friendly.
>
>>> +  - led-max-microamp: Desired maximum current for each LED in microampere
>>> +                      (maximum 30000uA)
>>
>> Please add instead of (maximum ...):
>>
>> Valid values: min - max, step by N (rounded {up|down})
>>
>> E.g.
>>
>> Valid values: 10000 - 30000, step by 1000 (rounded down)
>>
>
> This is unfortunately not a linear relationship,

OK, please define only max and min value then.

> led-max-microamp = 2400*Iref/(31-ILED)
>
> thus steps are not constant. This can be seen on figure 5 (p.9) of the datasheet
> [1].
>
> Thanks for your review,
> Florian
>
> [1] http://www.onsemi.com/pub/Collateral/NCP5623-D.PDF
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Jacek Anaszewski]

On 06/21/2016 05:28 PM, Jacek Anaszewski wrote:
> Hi Florian,
>
> Thanks for the patch. I have two remarks below.
>
> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>> LED driver. The driver can independently control the PWM of the 3
>> channels with 32 levels of intensity.
>>
>> The current delivered by the current source can be controlled using the
>> led-max-microamp property. In order to control this value, it is also
>> necessary to know the current on the Iref pin, hence the
>> onnn,led-iref-microamp property. It is usually set using an external
>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>> ---
>>   .../devicetree/bindings/leds/leds-ncp5623.txt      | 44
>> ++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>> new file mode 100644
>> index 0000000..0dc8345
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>> @@ -0,0 +1,44 @@
>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>> +
>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>> +channel can be independently set using 32 levels. Each LED is
>> represented
>> +as a sub-node of the device.
>> +
>> +Required properties:
>> +  - compatible: Should be "onnn,ncp5623"
>> +  - reg: I2C slave address (fixed to 0x38)
>> +  - #address-cells: must be 1
>> +  - #size-cells: must be 0
>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
>
> I think that you don't need this property. Just provide the formula for
> calculating led-max-microamp value, similarly as you're doing that in
> the commit message.
>
>> +  - led-max-microamp: Desired maximum current for each LED in
>> microampere
>> +                      (maximum 30000uA)
>
> Please add instead of (maximum ...):
>
> Valid values: min - max, step by N (rounded {up|down})
>
> E.g.
>
> Valid values: 10000 - 30000, step by 1000 (rounded down)
>
>> +
>> +LED sub-node properties:
>> +  - reg : LED channel number (0..2)
>> +  - For other LED properties see:
>> +      Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Example:
>> +
>> +led1: ncp5623@38 {
>> +    #address-cells = <1>;
>> +    #size-cells = <0>;
>> +    compatible = "onnn,ncp5623";
>> +    reg = <0x38>;
>> +
>> +    led1_r@0 {
>> +        label = "ncp:power:red";
>> +        reg = <0>;
>> +        linux,default-trigger = "default-on";

Please add also led-max-microamp to all child nodes in this example.

>> +    };
>> +
>> +    led1_b@1 {
>> +        label = "ncp:power:blue";
>> +        reg = <1>;
>> +    };
>> +
>> +    led1_g@2 {
>> +        label = "ncp:power:green";
>> +        reg = <2>;
>> +    };
>> +};
>>
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Florian Vaussard]

Hi Jacek,

Le 22. 06. 16 à 10:51, Jacek Anaszewski a écrit :
> Hi Florian,
> 
> On 06/22/2016 08:08 AM, Florian Vaussard wrote:
>> Hi Jacek,
>>
>> Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit :
>>> Hi Florian,
>>>
>>> Thanks for the patch. I have two remarks below.
>>>
>>> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>>>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>>>> LED driver. The driver can independently control the PWM of the 3
>>>> channels with 32 levels of intensity.
>>>>
>>>> The current delivered by the current source can be controlled using the
>>>> led-max-microamp property. In order to control this value, it is also
>>>> necessary to know the current on the Iref pin, hence the
>>>> onnn,led-iref-microamp property. It is usually set using an external
>>>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>>>
>>>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>>>> ---
>>>>    .../devicetree/bindings/leds/leds-ncp5623.txt      | 44
>>>> ++++++++++++++++++++++
>>>>    1 file changed, 44 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>> new file mode 100644
>>>> index 0000000..0dc8345
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>> @@ -0,0 +1,44 @@
>>>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>>>> +
>>>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>>>> +channel can be independently set using 32 levels. Each LED is represented
>>>> +as a sub-node of the device.
>>>> +
>>>> +Required properties:
>>>> +  - compatible: Should be "onnn,ncp5623"
>>>> +  - reg: I2C slave address (fixed to 0x38)
>>>> +  - #address-cells: must be 1
>>>> +  - #size-cells: must be 0
>>>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
>>>
>>> I think that you don't need this property. Just provide the formula for
>>> calculating led-max-microamp value, similarly as you're doing that in
>>> the commit message.
>>>
>>
>> I am not completely sure to understand your suggestion. So at the end, I have to
>> compute the value of the register (let call it 'ILED') that I need to send to
>> chip to configure the current source. The formula is:
>>
>> ILED = 31 - 2400*Iref/led-max-microamp
> 
> led-max-microamp is the maximum current value for given LED.
> According to the documentation it can be calculated as follows:
> 
> ILEDmax = Iref * 2400 / (31 - n)
> 
> Since this is global setting for all LEDs, then I'd always set n to 30,
> and calculate max_brightness value for each LED separately, basing on
> led-max-microamp property value. Effectively, I'm revoking my previous
> statement about setting max_brightness to fixed level.
> 

Ok your proposal simplifies a bit the handling. Thus ILEDmax of the current
source would be always equal to Iref * 2400 and we use the PWM to limit the
current inside the LED. The only downside of this approach is a reduced number
of possible PWM steps, thus a limited number of RGB colors.

Regarding the DT binding, this would mean something like this:

ncp5623@38 {
	#address-cells = <1>;
	#size-cells = <0>;
	compatible = "onnn,ncp5623";
	reg = <0x38>;
	led-max-microamp = <30000>;

	ledr@0 {
		label = "ncp:power:red";
		reg = <0>;
		linux,default-trigger = "default-on";
		led-max-microamp = <5000>;
	};

	ledb@1 {
		label = "ncp:power:blue";
		reg = <1>;
		led-max-microamp = <5000>;
	};

	ledg@2 {
		label = "ncp:power:green";
		reg = <2>;
		led-max-microamp = <5000>;
	};
};

The led-max-microamp property of the root node is used to infer Iref, and the
led-max-microamp property inside each LED node is used to compute the maximum
allowed PWM ratio (thus max_brightness).

Would it be fine like this?

> You can compare drivers/leds/leds-aat1290.c and its bindings, as it
> uses similar approach.
> 

Thanks for the pointer, interesting reading. In this case the flash-max-microamp
property is implicitly used to get the value of Rset, and led-max-microamp is
used to compute the flash/movie-mode ratio. Indeed similar but not exactly the
same, as the NCP5623 allows a finer control on the current using one register to
configure the current source and one register for the PWM.

Regards,
Florian

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Jacek Anaszewski]

On 06/22/2016 04:25 PM, Florian Vaussard wrote:
> Hi Jacek,
>
> Le 22. 06. 16 à 10:51, Jacek Anaszewski a écrit :
>> Hi Florian,
>>
>> On 06/22/2016 08:08 AM, Florian Vaussard wrote:
>>> Hi Jacek,
>>>
>>> Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit :
>>>> Hi Florian,
>>>>
>>>> Thanks for the patch. I have two remarks below.
>>>>
>>>> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>>>>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>>>>> LED driver. The driver can independently control the PWM of the 3
>>>>> channels with 32 levels of intensity.
>>>>>
>>>>> The current delivered by the current source can be controlled using the
>>>>> led-max-microamp property. In order to control this value, it is also
>>>>> necessary to know the current on the Iref pin, hence the
>>>>> onnn,led-iref-microamp property. It is usually set using an external
>>>>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>>>>
>>>>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>>>>> ---
>>>>>     .../devicetree/bindings/leds/leds-ncp5623.txt      | 44
>>>>> ++++++++++++++++++++++
>>>>>     1 file changed, 44 insertions(+)
>>>>>     create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>> new file mode 100644
>>>>> index 0000000..0dc8345
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>> @@ -0,0 +1,44 @@
>>>>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>>>>> +
>>>>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>>>>> +channel can be independently set using 32 levels. Each LED is represented
>>>>> +as a sub-node of the device.
>>>>> +
>>>>> +Required properties:
>>>>> +  - compatible: Should be "onnn,ncp5623"
>>>>> +  - reg: I2C slave address (fixed to 0x38)
>>>>> +  - #address-cells: must be 1
>>>>> +  - #size-cells: must be 0
>>>>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
>>>>
>>>> I think that you don't need this property. Just provide the formula for
>>>> calculating led-max-microamp value, similarly as you're doing that in
>>>> the commit message.
>>>>
>>>
>>> I am not completely sure to understand your suggestion. So at the end, I have to
>>> compute the value of the register (let call it 'ILED') that I need to send to
>>> chip to configure the current source. The formula is:
>>>
>>> ILED = 31 - 2400*Iref/led-max-microamp
>>
>> led-max-microamp is the maximum current value for given LED.
>> According to the documentation it can be calculated as follows:
>>
>> ILEDmax = Iref * 2400 / (31 - n)
>>
>> Since this is global setting for all LEDs, then I'd always set n to 30,
>> and calculate max_brightness value for each LED separately, basing on
>> led-max-microamp property value. Effectively, I'm revoking my previous
>> statement about setting max_brightness to fixed level.
>>
>
> Ok your proposal simplifies a bit the handling. Thus ILEDmax of the current
> source would be always equal to Iref * 2400 and we use the PWM to limit the
> current inside the LED. The only downside of this approach is a reduced number
> of possible PWM steps, thus a limited number of RGB colors.

Yes, but by max_brightness being always 31, lowering led-max-microamp
results in decreasing the amount of current per brightness level.
Effectively, a human ability to notice perceived brightness level
change also decreases then.

In the approach I proposed this limitation is reflected in reduced
amount of available brightness levels.

> Regarding the DT binding, this would mean something like this:
>
> ncp5623@38 {
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	compatible = "onnn,ncp5623";
> 	reg = <0x38>;
> 	led-max-microamp = <30000>;

Please drop it from here. It doesn't need to be configurable.
You can hard code this in the driver.

>
> 	ledr@0 {
> 		label = "ncp:power:red";
> 		reg = <0>;
> 		linux,default-trigger = "default-on";
> 		led-max-microamp = <5000>;

Is 5mA the maximum allowed current value for the LEDs on the board
you're using? Is brightness level change easily noticeable by max
current set to 5mA and max_brightness set to 31? It would be good
to empirically check this configuration.

> 	};
>
> 	ledb@1 {
> 		label = "ncp:power:blue";
> 		reg = <1>;
> 		led-max-microamp = <5000>;
> 	};
>
> 	ledg@2 {
> 		label = "ncp:power:green";
> 		reg = <2>;
> 		led-max-microamp = <5000>;
> 	};
> };
>
> The led-max-microamp property of the root node is used to infer Iref, and the
> led-max-microamp property inside each LED node is used to compute the maximum
> allowed PWM ratio (thus max_brightness).
>
> Would it be fine like this?
>
>> You can compare drivers/leds/leds-aat1290.c and its bindings, as it
>> uses similar approach.
>>
>
> Thanks for the pointer, interesting reading. In this case the flash-max-microamp
> property is implicitly used to get the value of Rset, and led-max-microamp is
> used to compute the flash/movie-mode ratio. Indeed similar but not exactly the
> same, as the NCP5623 allows a finer control on the current using one register to
> configure the current source and one register for the PWM.

Right, but it shows how led-max-microamp can be used to infer
max_brightness level. This is quite new DT property with not too many
users, because previously LED class drivers had been defining
max_brightness directly in a Device Tree. Nonetheless brightness level
was eventually considered not suitable unit for describing hardware
property.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Florian Vaussard]

Hi Jacek,

On 06/23/2016 09:23 AM, Jacek Anaszewski wrote:
> On 06/22/2016 04:25 PM, Florian Vaussard wrote:
>> Hi Jacek,
>>
>> Le 22. 06. 16 à 10:51, Jacek Anaszewski a écrit :
>>> Hi Florian,
>>>
>>> On 06/22/2016 08:08 AM, Florian Vaussard wrote:
>>>> Hi Jacek,
>>>>
>>>> Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit :
>>>>> Hi Florian,
>>>>>
>>>>> Thanks for the patch. I have two remarks below.
>>>>>
>>>>> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>>>>>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>>>>>> LED driver. The driver can independently control the PWM of the 3
>>>>>> channels with 32 levels of intensity.
>>>>>>
>>>>>> The current delivered by the current source can be controlled using the
>>>>>> led-max-microamp property. In order to control this value, it is also
>>>>>> necessary to know the current on the Iref pin, hence the
>>>>>> onnn,led-iref-microamp property. It is usually set using an external
>>>>>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>>>>>
>>>>>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>>>>>> ---
>>>>>>     .../devicetree/bindings/leds/leds-ncp5623.txt      | 44
>>>>>> ++++++++++++++++++++++
>>>>>>     1 file changed, 44 insertions(+)
>>>>>>     create mode 100644
>>>>>> Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..0dc8345
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>> @@ -0,0 +1,44 @@
>>>>>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>>>>>> +
>>>>>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>>>>>> +channel can be independently set using 32 levels. Each LED is represented
>>>>>> +as a sub-node of the device.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +  - compatible: Should be "onnn,ncp5623"
>>>>>> +  - reg: I2C slave address (fixed to 0x38)
>>>>>> +  - #address-cells: must be 1
>>>>>> +  - #size-cells: must be 0
>>>>>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
>>>>>
>>>>> I think that you don't need this property. Just provide the formula for
>>>>> calculating led-max-microamp value, similarly as you're doing that in
>>>>> the commit message.
>>>>>
>>>>
>>>> I am not completely sure to understand your suggestion. So at the end, I
>>>> have to
>>>> compute the value of the register (let call it 'ILED') that I need to send to
>>>> chip to configure the current source. The formula is:
>>>>
>>>> ILED = 31 - 2400*Iref/led-max-microamp
>>>
>>> led-max-microamp is the maximum current value for given LED.
>>> According to the documentation it can be calculated as follows:
>>>
>>> ILEDmax = Iref * 2400 / (31 - n)
>>>
>>> Since this is global setting for all LEDs, then I'd always set n to 30,
>>> and calculate max_brightness value for each LED separately, basing on
>>> led-max-microamp property value. Effectively, I'm revoking my previous
>>> statement about setting max_brightness to fixed level.
>>>
>>
>> Ok your proposal simplifies a bit the handling. Thus ILEDmax of the current
>> source would be always equal to Iref * 2400 and we use the PWM to limit the
>> current inside the LED. The only downside of this approach is a reduced number
>> of possible PWM steps, thus a limited number of RGB colors.
> 
> Yes, but by max_brightness being always 31, lowering led-max-microamp
> results in decreasing the amount of current per brightness level.
> Effectively, a human ability to notice perceived brightness level
> change also decreases then.
> 
> In the approach I proposed this limitation is reflected in reduced
> amount of available brightness levels.
> 
>> Regarding the DT binding, this would mean something like this:
>>
>> ncp5623@38 {
>>     #address-cells = <1>;
>>     #size-cells = <0>;
>>     compatible = "onnn,ncp5623";
>>     reg = <0x38>;
>>     led-max-microamp = <30000>;
> 
> Please drop it from here. It doesn't need to be configurable.
> You can hard code this in the driver.
> 

It is not user configurable, but it is a hardware configuration imposed by the
bias resistor on the Iref pin (ILEDmax = 2400*Iref = 2400*0.6V/Rbias). So I
cannot hard code it as it can change from one design to another. And I need this
piece of information to compute the maximum allowable PWM ratio.

>>
>>     ledr@0 {
>>         label = "ncp:power:red";
>>         reg = <0>;
>>         linux,default-trigger = "default-on";
>>         led-max-microamp = <5000>;
> 
> Is 5mA the maximum allowed current value for the LEDs on the board
> you're using? Is brightness level change easily noticeable by max
> current set to 5mA and max_brightness set to 31? It would be good
> to empirically check this configuration.
> 

No the maximum is 20mA on our board, but limiting to 5mA is safer to avoid
blinding the user :) This RGB led is quite powerful...

Some experiments:
1) When setting the current source at 5mA, the PWM steps are easily noticeable
at low brightness (below 50%). Above the eye is not sensitive enough. Thus on
the 32768 possible colours, I agree that not all will be distinguishable.
2) When setting the current source at 20mA, the PWM steps are even more visible
at low brightness. As I have to keep the PWM ratio below 25% to satisfy the 5mA
limit, all the 7 steps (brightness = [0; 7]) are clearly noticeable. This also
means only 512 different colours. For sure in this case they are all
distinguishable :)

With your proposal, the hardware fix is probably to decrease Iref by increasing
the bias resistor. This way the PWM steps would be smaller and less noticeable.
But a hardware fix is not always possible.

>>     };
>>
>>     ledb@1 {
>>         label = "ncp:power:blue";
>>         reg = <1>;
>>         led-max-microamp = <5000>;
>>     };
>>
>>     ledg@2 {
>>         label = "ncp:power:green";
>>         reg = <2>;
>>         led-max-microamp = <5000>;
>>     };
>> };
>>
>> The led-max-microamp property of the root node is used to infer Iref, and the
>> led-max-microamp property inside each LED node is used to compute the maximum
>> allowed PWM ratio (thus max_brightness).
>>
>> Would it be fine like this?
>>
>>> You can compare drivers/leds/leds-aat1290.c and its bindings, as it
>>> uses similar approach.
>>>
>>
>> Thanks for the pointer, interesting reading. In this case the flash-max-microamp
>> property is implicitly used to get the value of Rset, and led-max-microamp is
>> used to compute the flash/movie-mode ratio. Indeed similar but not exactly the
>> same, as the NCP5623 allows a finer control on the current using one register to
>> configure the current source and one register for the PWM.
> 
> Right, but it shows how led-max-microamp can be used to infer
> max_brightness level. This is quite new DT property with not too many
> users, because previously LED class drivers had been defining
> max_brightness directly in a Device Tree. Nonetheless brightness level
> was eventually considered not suitable unit for describing hardware
> property.
> 

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Jacek Anaszewski]

On 06/23/2016 10:32 AM, Florian Vaussard wrote:
> Hi Jacek,
>
> On 06/23/2016 09:23 AM, Jacek Anaszewski wrote:
>> On 06/22/2016 04:25 PM, Florian Vaussard wrote:
>>> Hi Jacek,
>>>
>>> Le 22. 06. 16 à 10:51, Jacek Anaszewski a écrit :
>>>> Hi Florian,
>>>>
>>>> On 06/22/2016 08:08 AM, Florian Vaussard wrote:
>>>>> Hi Jacek,
>>>>>
>>>>> Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit :
>>>>>> Hi Florian,
>>>>>>
>>>>>> Thanks for the patch. I have two remarks below.
>>>>>>
>>>>>> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>>>>>>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>>>>>>> LED driver. The driver can independently control the PWM of the 3
>>>>>>> channels with 32 levels of intensity.
>>>>>>>
>>>>>>> The current delivered by the current source can be controlled using the
>>>>>>> led-max-microamp property. In order to control this value, it is also
>>>>>>> necessary to know the current on the Iref pin, hence the
>>>>>>> onnn,led-iref-microamp property. It is usually set using an external
>>>>>>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>>>>>>
>>>>>>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>>>>>>> ---
>>>>>>>      .../devicetree/bindings/leds/leds-ncp5623.txt      | 44
>>>>>>> ++++++++++++++++++++++
>>>>>>>      1 file changed, 44 insertions(+)
>>>>>>>      create mode 100644
>>>>>>> Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..0dc8345
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>> @@ -0,0 +1,44 @@
>>>>>>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>>>>>>> +
>>>>>>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>>>>>>> +channel can be independently set using 32 levels. Each LED is represented
>>>>>>> +as a sub-node of the device.
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +  - compatible: Should be "onnn,ncp5623"
>>>>>>> +  - reg: I2C slave address (fixed to 0x38)
>>>>>>> +  - #address-cells: must be 1
>>>>>>> +  - #size-cells: must be 0
>>>>>>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
>>>>>>
>>>>>> I think that you don't need this property. Just provide the formula for
>>>>>> calculating led-max-microamp value, similarly as you're doing that in
>>>>>> the commit message.
>>>>>>
>>>>>
>>>>> I am not completely sure to understand your suggestion. So at the end, I
>>>>> have to
>>>>> compute the value of the register (let call it 'ILED') that I need to send to
>>>>> chip to configure the current source. The formula is:
>>>>>
>>>>> ILED = 31 - 2400*Iref/led-max-microamp
>>>>
>>>> led-max-microamp is the maximum current value for given LED.
>>>> According to the documentation it can be calculated as follows:
>>>>
>>>> ILEDmax = Iref * 2400 / (31 - n)
>>>>
>>>> Since this is global setting for all LEDs, then I'd always set n to 30,
>>>> and calculate max_brightness value for each LED separately, basing on
>>>> led-max-microamp property value. Effectively, I'm revoking my previous
>>>> statement about setting max_brightness to fixed level.
>>>>
>>>
>>> Ok your proposal simplifies a bit the handling. Thus ILEDmax of the current
>>> source would be always equal to Iref * 2400 and we use the PWM to limit the
>>> current inside the LED. The only downside of this approach is a reduced number
>>> of possible PWM steps, thus a limited number of RGB colors.
>>
>> Yes, but by max_brightness being always 31, lowering led-max-microamp
>> results in decreasing the amount of current per brightness level.
>> Effectively, a human ability to notice perceived brightness level
>> change also decreases then.
>>
>> In the approach I proposed this limitation is reflected in reduced
>> amount of available brightness levels.
>>
>>> Regarding the DT binding, this would mean something like this:
>>>
>>> ncp5623@38 {
>>>      #address-cells = <1>;
>>>      #size-cells = <0>;
>>>      compatible = "onnn,ncp5623";
>>>      reg = <0x38>;
>>>      led-max-microamp = <30000>;
>>
>> Please drop it from here. It doesn't need to be configurable.
>> You can hard code this in the driver.
>>
>
> It is not user configurable, but it is a hardware configuration imposed by the
> bias resistor on the Iref pin (ILEDmax = 2400*Iref = 2400*0.6V/Rbias). So I
> cannot hard code it as it can change from one design to another. And I need this
> piece of information to compute the maximum allowable PWM ratio.

OK, please keep here the property you initially introduced for that:

onnn,led-iref-microamp

>
>>>
>>>      ledr@0 {
>>>          label = "ncp:power:red";
>>>          reg = <0>;
>>>          linux,default-trigger = "default-on";
>>>          led-max-microamp = <5000>;
>>
>> Is 5mA the maximum allowed current value for the LEDs on the board
>> you're using? Is brightness level change easily noticeable by max
>> current set to 5mA and max_brightness set to 31? It would be good
>> to empirically check this configuration.
>>
>
> No the maximum is 20mA on our board, but limiting to 5mA is safer to avoid
> blinding the user :) This RGB led is quite powerful...
>
> Some experiments:
> 1) When setting the current source at 5mA, the PWM steps are easily noticeable
> at low brightness (below 50%). Above the eye is not sensitive enough. Thus on
> the 32768 possible colours, I agree that not all will be distinguishable.
> 2) When setting the current source at 20mA, the PWM steps are even more visible
> at low brightness. As I have to keep the PWM ratio below 25% to satisfy the 5mA
> limit, all the 7 steps (brightness = [0; 7]) are clearly noticeable. This also
> means only 512 different colours. For sure in this case they are all
> distinguishable :)
>
> With your proposal, the hardware fix is probably to decrease Iref by increasing
> the bias resistor. This way the PWM steps would be smaller and less noticeable.
> But a hardware fix is not always possible.

It would be nice to have all 31 levels available per LED, but is it
feasible having that ILED register is global for all LEDs? It seems that
we couldn't set different maximum current for each LED then.
Am I right or I am missing something?

>>>      };
>>>
>>>      ledb@1 {
>>>          label = "ncp:power:blue";
>>>          reg = <1>;
>>>          led-max-microamp = <5000>;
>>>      };
>>>
>>>      ledg@2 {
>>>          label = "ncp:power:green";
>>>          reg = <2>;
>>>          led-max-microamp = <5000>;
>>>      };
>>> };
>>>
>>> The led-max-microamp property of the root node is used to infer Iref, and the
>>> led-max-microamp property inside each LED node is used to compute the maximum
>>> allowed PWM ratio (thus max_brightness).
>>>
>>> Would it be fine like this?
>>>
>>>> You can compare drivers/leds/leds-aat1290.c and its bindings, as it
>>>> uses similar approach.
>>>>
>>>
>>> Thanks for the pointer, interesting reading. In this case the flash-max-microamp
>>> property is implicitly used to get the value of Rset, and led-max-microamp is
>>> used to compute the flash/movie-mode ratio. Indeed similar but not exactly the
>>> same, as the NCP5623 allows a finer control on the current using one register to
>>> configure the current source and one register for the PWM.
>>
>> Right, but it shows how led-max-microamp can be used to infer
>> max_brightness level. This is quite new DT property with not too many
>> users, because previously LED class drivers had been defining
>> max_brightness directly in a Device Tree. Nonetheless brightness level
>> was eventually considered not suitable unit for describing hardware
>> property.
>>
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Florian Vaussard]

On 06/23/2016 01:21 PM, Jacek Anaszewski wrote:
> On 06/23/2016 10:32 AM, Florian Vaussard wrote:
>> Hi Jacek,
>>
>> On 06/23/2016 09:23 AM, Jacek Anaszewski wrote:
>>> On 06/22/2016 04:25 PM, Florian Vaussard wrote:
>>>> Hi Jacek,
>>>>
>>>> Le 22. 06. 16 à 10:51, Jacek Anaszewski a écrit :
>>>>> Hi Florian,
>>>>>
>>>>> On 06/22/2016 08:08 AM, Florian Vaussard wrote:
>>>>>> Hi Jacek,
>>>>>>
>>>>>> Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit :
>>>>>>> Hi Florian,
>>>>>>>
>>>>>>> Thanks for the patch. I have two remarks below.
>>>>>>>
>>>>>>> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>>>>>>>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>>>>>>>> LED driver. The driver can independently control the PWM of the 3
>>>>>>>> channels with 32 levels of intensity.
>>>>>>>>
>>>>>>>> The current delivered by the current source can be controlled using the
>>>>>>>> led-max-microamp property. In order to control this value, it is also
>>>>>>>> necessary to know the current on the Iref pin, hence the
>>>>>>>> onnn,led-iref-microamp property. It is usually set using an external
>>>>>>>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>>>>>>>
>>>>>>>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>>>>>>>> ---
>>>>>>>>      .../devicetree/bindings/leds/leds-ncp5623.txt      | 44
>>>>>>>> ++++++++++++++++++++++
>>>>>>>>      1 file changed, 44 insertions(+)
>>>>>>>>      create mode 100644
>>>>>>>> Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..0dc8345
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>>> @@ -0,0 +1,44 @@
>>>>>>>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>>>>>>>> +
>>>>>>>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>>>>>>>> +channel can be independently set using 32 levels. Each LED is represented
>>>>>>>> +as a sub-node of the device.
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +  - compatible: Should be "onnn,ncp5623"
>>>>>>>> +  - reg: I2C slave address (fixed to 0x38)
>>>>>>>> +  - #address-cells: must be 1
>>>>>>>> +  - #size-cells: must be 0
>>>>>>>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
>>>>>>>
>>>>>>> I think that you don't need this property. Just provide the formula for
>>>>>>> calculating led-max-microamp value, similarly as you're doing that in
>>>>>>> the commit message.
>>>>>>>
>>>>>>
>>>>>> I am not completely sure to understand your suggestion. So at the end, I
>>>>>> have to
>>>>>> compute the value of the register (let call it 'ILED') that I need to send to
>>>>>> chip to configure the current source. The formula is:
>>>>>>
>>>>>> ILED = 31 - 2400*Iref/led-max-microamp
>>>>>
>>>>> led-max-microamp is the maximum current value for given LED.
>>>>> According to the documentation it can be calculated as follows:
>>>>>
>>>>> ILEDmax = Iref * 2400 / (31 - n)
>>>>>
>>>>> Since this is global setting for all LEDs, then I'd always set n to 30,
>>>>> and calculate max_brightness value for each LED separately, basing on
>>>>> led-max-microamp property value. Effectively, I'm revoking my previous
>>>>> statement about setting max_brightness to fixed level.
>>>>>
>>>>
>>>> Ok your proposal simplifies a bit the handling. Thus ILEDmax of the current
>>>> source would be always equal to Iref * 2400 and we use the PWM to limit the
>>>> current inside the LED. The only downside of this approach is a reduced number
>>>> of possible PWM steps, thus a limited number of RGB colors.
>>>
>>> Yes, but by max_brightness being always 31, lowering led-max-microamp
>>> results in decreasing the amount of current per brightness level.
>>> Effectively, a human ability to notice perceived brightness level
>>> change also decreases then.
>>>
>>> In the approach I proposed this limitation is reflected in reduced
>>> amount of available brightness levels.
>>>
>>>> Regarding the DT binding, this would mean something like this:
>>>>
>>>> ncp5623@38 {
>>>>      #address-cells = <1>;
>>>>      #size-cells = <0>;
>>>>      compatible = "onnn,ncp5623";
>>>>      reg = <0x38>;
>>>>      led-max-microamp = <30000>;
>>>
>>> Please drop it from here. It doesn't need to be configurable.
>>> You can hard code this in the driver.
>>>
>>
>> It is not user configurable, but it is a hardware configuration imposed by the
>> bias resistor on the Iref pin (ILEDmax = 2400*Iref = 2400*0.6V/Rbias). So I
>> cannot hard code it as it can change from one design to another. And I need this
>> piece of information to compute the maximum allowable PWM ratio.
> 
> OK, please keep here the property you initially introduced for that:
> 
> onnn,led-iref-microamp
> 

Ok.

>>
>>>>
>>>>      ledr@0 {
>>>>          label = "ncp:power:red";
>>>>          reg = <0>;
>>>>          linux,default-trigger = "default-on";
>>>>          led-max-microamp = <5000>;
>>>
>>> Is 5mA the maximum allowed current value for the LEDs on the board
>>> you're using? Is brightness level change easily noticeable by max
>>> current set to 5mA and max_brightness set to 31? It would be good
>>> to empirically check this configuration.
>>>
>>
>> No the maximum is 20mA on our board, but limiting to 5mA is safer to avoid
>> blinding the user :) This RGB led is quite powerful...
>>
>> Some experiments:
>> 1) When setting the current source at 5mA, the PWM steps are easily noticeable
>> at low brightness (below 50%). Above the eye is not sensitive enough. Thus on
>> the 32768 possible colours, I agree that not all will be distinguishable.
>> 2) When setting the current source at 20mA, the PWM steps are even more visible
>> at low brightness. As I have to keep the PWM ratio below 25% to satisfy the 5mA
>> limit, all the 7 steps (brightness = [0; 7]) are clearly noticeable. This also
>> means only 512 different colours. For sure in this case they are all
>> distinguishable :)
>>
>> With your proposal, the hardware fix is probably to decrease Iref by increasing
>> the bias resistor. This way the PWM steps would be smaller and less noticeable.
>> But a hardware fix is not always possible.
> 
> It would be nice to have all 31 levels available per LED, but is it
> feasible having that ILED register is global for all LEDs? It seems that
> we couldn't set different maximum current for each LED then.
> Am I right or I am missing something?
> 

If led-max-microamp is different for each LED, we can select the highest one and
use it to compute the ILED register. Then we limit the PWM ratio for the LEDs
that have a lowest led-max-microamp. I guess that this is the best compromise.

Regards,
Florian

>>>>      };
>>>>
>>>>      ledb@1 {
>>>>          label = "ncp:power:blue";
>>>>          reg = <1>;
>>>>          led-max-microamp = <5000>;
>>>>      };
>>>>
>>>>      ledg@2 {
>>>>          label = "ncp:power:green";
>>>>          reg = <2>;
>>>>          led-max-microamp = <5000>;
>>>>      };
>>>> };
>>>>
>>>> The led-max-microamp property of the root node is used to infer Iref, and the
>>>> led-max-microamp property inside each LED node is used to compute the maximum
>>>> allowed PWM ratio (thus max_brightness).
>>>>
>>>> Would it be fine like this?
>>>>
>>>>> You can compare drivers/leds/leds-aat1290.c and its bindings, as it
>>>>> uses similar approach.
>>>>>
>>>>
>>>> Thanks for the pointer, interesting reading. In this case the
>>>> flash-max-microamp
>>>> property is implicitly used to get the value of Rset, and led-max-microamp is
>>>> used to compute the flash/movie-mode ratio. Indeed similar but not exactly the
>>>> same, as the NCP5623 allows a finer control on the current using one
>>>> register to
>>>> configure the current source and one register for the PWM.
>>>
>>> Right, but it shows how led-max-microamp can be used to infer
>>> max_brightness level. This is quite new DT property with not too many
>>> users, because previously LED class drivers had been defining
>>> max_brightness directly in a Device Tree. Nonetheless brightness level
>>> was eventually considered not suitable unit for describing hardware
>>> property.
>>>
>>
>>
> 
> 

Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation

[Jacek Anaszewski]

On 06/23/2016 02:01 PM, Florian Vaussard wrote:
>
>
> On 06/23/2016 01:21 PM, Jacek Anaszewski wrote:
>> On 06/23/2016 10:32 AM, Florian Vaussard wrote:
>>> Hi Jacek,
>>>
>>> On 06/23/2016 09:23 AM, Jacek Anaszewski wrote:
>>>> On 06/22/2016 04:25 PM, Florian Vaussard wrote:
>>>>> Hi Jacek,
>>>>>
>>>>> Le 22. 06. 16 à 10:51, Jacek Anaszewski a écrit :
>>>>>> Hi Florian,
>>>>>>
>>>>>> On 06/22/2016 08:08 AM, Florian Vaussard wrote:
>>>>>>> Hi Jacek,
>>>>>>>
>>>>>>> Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit :
>>>>>>>> Hi Florian,
>>>>>>>>
>>>>>>>> Thanks for the patch. I have two remarks below.
>>>>>>>>
>>>>>>>> On 06/21/2016 09:29 AM, Florian Vaussard wrote:
>>>>>>>>> Add device tree binding documentation for On Semiconductor NCP5623 I2C
>>>>>>>>> LED driver. The driver can independently control the PWM of the 3
>>>>>>>>> channels with 32 levels of intensity.
>>>>>>>>>
>>>>>>>>> The current delivered by the current source can be controlled using the
>>>>>>>>> led-max-microamp property. In order to control this value, it is also
>>>>>>>>> necessary to know the current on the Iref pin, hence the
>>>>>>>>> onnn,led-iref-microamp property. It is usually set using an external
>>>>>>>>> bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Florian Vaussard <florian.vaussard@heig-vd.ch>
>>>>>>>>> ---
>>>>>>>>>       .../devicetree/bindings/leds/leds-ncp5623.txt      | 44
>>>>>>>>> ++++++++++++++++++++++
>>>>>>>>>       1 file changed, 44 insertions(+)
>>>>>>>>>       create mode 100644
>>>>>>>>> Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>>>> b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000..0dc8345
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
>>>>>>>>> @@ -0,0 +1,44 @@
>>>>>>>>> +* ON Semiconductor - NCP5623 3-Channel LED Driver
>>>>>>>>> +
>>>>>>>>> +The NCP5623 is a 3-channel I2C LED driver. The brightness of each
>>>>>>>>> +channel can be independently set using 32 levels. Each LED is represented
>>>>>>>>> +as a sub-node of the device.
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +  - compatible: Should be "onnn,ncp5623"
>>>>>>>>> +  - reg: I2C slave address (fixed to 0x38)
>>>>>>>>> +  - #address-cells: must be 1
>>>>>>>>> +  - #size-cells: must be 0
>>>>>>>>> +  - onnn,led-iref-microamp: Current on the Iref pin in microampere
>>>>>>>>
>>>>>>>> I think that you don't need this property. Just provide the formula for
>>>>>>>> calculating led-max-microamp value, similarly as you're doing that in
>>>>>>>> the commit message.
>>>>>>>>
>>>>>>>
>>>>>>> I am not completely sure to understand your suggestion. So at the end, I
>>>>>>> have to
>>>>>>> compute the value of the register (let call it 'ILED') that I need to send to
>>>>>>> chip to configure the current source. The formula is:
>>>>>>>
>>>>>>> ILED = 31 - 2400*Iref/led-max-microamp
>>>>>>
>>>>>> led-max-microamp is the maximum current value for given LED.
>>>>>> According to the documentation it can be calculated as follows:
>>>>>>
>>>>>> ILEDmax = Iref * 2400 / (31 - n)
>>>>>>
>>>>>> Since this is global setting for all LEDs, then I'd always set n to 30,
>>>>>> and calculate max_brightness value for each LED separately, basing on
>>>>>> led-max-microamp property value. Effectively, I'm revoking my previous
>>>>>> statement about setting max_brightness to fixed level.
>>>>>>
>>>>>
>>>>> Ok your proposal simplifies a bit the handling. Thus ILEDmax of the current
>>>>> source would be always equal to Iref * 2400 and we use the PWM to limit the
>>>>> current inside the LED. The only downside of this approach is a reduced number
>>>>> of possible PWM steps, thus a limited number of RGB colors.
>>>>
>>>> Yes, but by max_brightness being always 31, lowering led-max-microamp
>>>> results in decreasing the amount of current per brightness level.
>>>> Effectively, a human ability to notice perceived brightness level
>>>> change also decreases then.
>>>>
>>>> In the approach I proposed this limitation is reflected in reduced
>>>> amount of available brightness levels.
>>>>
>>>>> Regarding the DT binding, this would mean something like this:
>>>>>
>>>>> ncp5623@38 {
>>>>>       #address-cells = <1>;
>>>>>       #size-cells = <0>;
>>>>>       compatible = "onnn,ncp5623";
>>>>>       reg = <0x38>;
>>>>>       led-max-microamp = <30000>;
>>>>
>>>> Please drop it from here. It doesn't need to be configurable.
>>>> You can hard code this in the driver.
>>>>
>>>
>>> It is not user configurable, but it is a hardware configuration imposed by the
>>> bias resistor on the Iref pin (ILEDmax = 2400*Iref = 2400*0.6V/Rbias). So I
>>> cannot hard code it as it can change from one design to another. And I need this
>>> piece of information to compute the maximum allowable PWM ratio.
>>
>> OK, please keep here the property you initially introduced for that:
>>
>> onnn,led-iref-microamp
>>
>
> Ok.
>
>>>
>>>>>
>>>>>       ledr@0 {
>>>>>           label = "ncp:power:red";
>>>>>           reg = <0>;
>>>>>           linux,default-trigger = "default-on";
>>>>>           led-max-microamp = <5000>;
>>>>
>>>> Is 5mA the maximum allowed current value for the LEDs on the board
>>>> you're using? Is brightness level change easily noticeable by max
>>>> current set to 5mA and max_brightness set to 31? It would be good
>>>> to empirically check this configuration.
>>>>
>>>
>>> No the maximum is 20mA on our board, but limiting to 5mA is safer to avoid
>>> blinding the user :) This RGB led is quite powerful...
>>>
>>> Some experiments:
>>> 1) When setting the current source at 5mA, the PWM steps are easily noticeable
>>> at low brightness (below 50%). Above the eye is not sensitive enough. Thus on
>>> the 32768 possible colours, I agree that not all will be distinguishable.
>>> 2) When setting the current source at 20mA, the PWM steps are even more visible
>>> at low brightness. As I have to keep the PWM ratio below 25% to satisfy the 5mA
>>> limit, all the 7 steps (brightness = [0; 7]) are clearly noticeable. This also
>>> means only 512 different colours. For sure in this case they are all
>>> distinguishable :)
>>>
>>> With your proposal, the hardware fix is probably to decrease Iref by increasing
>>> the bias resistor. This way the PWM steps would be smaller and less noticeable.
>>> But a hardware fix is not always possible.
>>
>> It would be nice to have all 31 levels available per LED, but is it
>> feasible having that ILED register is global for all LEDs? It seems that
>> we couldn't set different maximum current for each LED then.
>> Am I right or I am missing something?
>>
>
> If led-max-microamp is different for each LED, we can select the highest one and
> use it to compute the ILED register. Then we limit the PWM ratio for the LEDs
> that have a lowest led-max-microamp. I guess that this is the best compromise.

Sounds good. Please proceed accordingly.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[Pavel Machek]

Hi!

> >+struct ncp5623_led {
> >+	bool active;
> >+	unsigned int led_no;
> >+	struct led_classdev ldev;
> >+	struct work_struct work;
> >+	struct ncp5623_priv *priv;
> >+};
> >+
> >+struct ncp5623_priv {
> >+	struct ncp5623_led leds[NCP5623_MAX_LEDS];
> 
> Please allocate memory dynamically, depending on the number
> of LEDs defined in a Device Tree.

MAX_LEDs is three. Are you sure overhead of dynamic allocation is
worth it?

And if this is for RGB leds... very probably device will want to use
all 3 channels.

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

Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[Florian Vaussard]

Hi Pavel,

Le 26. 06. 16 à 23:49, Pavel Machek a écrit :
> Hi!
> 
>>> +struct ncp5623_led {
>>> +	bool active;
>>> +	unsigned int led_no;
>>> +	struct led_classdev ldev;
>>> +	struct work_struct work;
>>> +	struct ncp5623_priv *priv;
>>> +};
>>> +
>>> +struct ncp5623_priv {
>>> +	struct ncp5623_led leds[NCP5623_MAX_LEDS];
>>
>> Please allocate memory dynamically, depending on the number
>> of LEDs defined in a Device Tree.
> 
> MAX_LEDs is three. Are you sure overhead of dynamic allocation is
> worth it?
> 
> And if this is for RGB leds... very probably device will want to use
> all 3 channels.
> 

I was about to raise the same question during the v2 of this patch. In addition
to your arguments, this also changes the way this array is indexed.

Currently the LED number is used as index, but with dynamic allocation I have to
use an abstract index. This makes some logic a bit harder, especially to check
if the same LED is declared twice in the device tree (duplicated 'reg' property).

Best,
Florian

Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[Jacek Anaszewski]

Hi Florian and Pavel,

On 06/27/2016 07:46 AM, Florian Vaussard wrote:
> Hi Pavel,
>
> Le 26. 06. 16 à 23:49, Pavel Machek a écrit :
>> Hi!
>>
>>>> +struct ncp5623_led {
>>>> +	bool active;
>>>> +	unsigned int led_no;
>>>> +	struct led_classdev ldev;
>>>> +	struct work_struct work;
>>>> +	struct ncp5623_priv *priv;
>>>> +};
>>>> +
>>>> +struct ncp5623_priv {
>>>> +	struct ncp5623_led leds[NCP5623_MAX_LEDS];
>>>
>>> Please allocate memory dynamically, depending on the number
>>> of LEDs defined in a Device Tree.
>>
>> MAX_LEDs is three. Are you sure overhead of dynamic allocation is
>> worth it?
>>
>> And if this is for RGB leds... very probably device will want to use
>> all 3 channels.
>>
>
> I was about to raise the same question during the v2 of this patch. In addition
> to your arguments, this also changes the way this array is indexed.
>
> Currently the LED number is used as index, but with dynamic allocation I have to
> use an abstract index. This makes some logic a bit harder, especially to check
> if the same LED is declared twice in the device tree (duplicated 'reg' property).

Fair enough. Please ignore my remark then.

-- 
Best regards,
Jacek Anaszewski