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

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

[Florian Vaussard]

Hello Jacek and Pavel,

Winter came, then spring, summer and automn went away. Snow came again,
and now here is the v4 of the NCP5623 patch... Never too late!

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

---
v3 -> v4:
- Rebased on Linux 4.15
- Moved ncp5623_of_match[] after probe
- Fixed computation of max_brightness
- Some more simplifications
- Updated copyright and SPDX license

v2 -> v3:
- Rebased on latest leds/for-next
- Minor fixes to the binding documentation
- Removed ncp5623_set_pwm() by inlining it directly inside the caller
- Removed ncp5623_destroy_devices() as we are already using devm_ flavours
- Got rid of the 'active' state variable by using 'led_no' instead
- Some other cosmetic fixes

v1 -> v2:
- Adapted the DT binding (led-max-microamp for each LED node)
- Removed underscores from node names in the example
- Use brightness_set_blocking to avoid workqueue
- Introduced LED_to_CMD macro to avoid switch statement
- Various other fixes

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      |  60 ++++++
 drivers/leds/Kconfig                               |  11 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-ncp5623.c                        | 228 +++++++++++++++++++++
 4 files changed, 300 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt
 create mode 100644 drivers/leds/leds-ncp5623.c

-- 
2.13.6

[PATCH v4 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 also be controlled. To
do so, the led-max-microamp property is used by each LED sub-node. The
maximum value is then found and used as a limit to compute the final
intensity of the current source. If a LED happens to have a lower limit,
the PWM is then used to limit the current to the requested value.

In order to control the current source, 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@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/leds/leds-ncp5623.txt      | 60 ++++++++++++++++++++++
 1 file changed, 60 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 000000000000..d83e5094343e
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
@@ -0,0 +1,60 @@
+* 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. It depends
+    on the value of the external bias resistor Rbias, following
+    Iref = Vref / Rbias with Vref = 0.6V. This is used to set the intensity of
+    the current that can be provided by the internal current source, based on
+    the maximum current permitted by LED sub-nodes (see below), but no more than
+    Imax = 2400 * Iref.
+
+LED sub-nodes
+=============
+
+Required properties:
+  - reg : LED channel number (0..2)
+  - led-max-microamp: Maximum allowable current inside the LED in microampere.
+    This property is used to limit the PWM ratio, based on the intensity of the
+    internal current source (see above).
+
+Optional properties:
+  - label: see Documentation/devicetree/bindings/leds/common.txt
+  - linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
+
+Example
+=======
+
+led1: ncp5623@38 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "onnn,ncp5623";
+	reg = <0x38>;
+	onnn,led-iref-microamp = <10>;
+
+	led1r@0 {
+		label = "ncp:power:red";
+		linux,default-trigger = "default-on";
+		reg = <0>;
+		led-max-microamp = <20000>;
+	};
+
+	led1b@1 {
+		label = "ncp:power:blue";
+		reg = <1>;
+		led-max-microamp = <20000>;
+	};
+
+	led1g@2 {
+		label = "ncp:power:green";
+		reg = <2>;
+		led-max-microamp = <20000>;
+	};
+};
-- 
2.13.6

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

[Pavel Machek]

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

Hi!

> +Required properties:
> +  - compatible: Should be "onnn,ncp5623"
> +  - reg: I2C slave address (fixed to 0x38)
..
> +Required properties:
> +  - reg : LED channel number (0..2)

For consistency, there should be no space between reg and :.

Anyway, this can be fixed later.

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

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

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

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

[Jacek Anaszewski]

Hi Florian,

Thanks for the v4.

On 02/21/2018 10:46 PM, 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 also be controlled. To
> do so, the led-max-microamp property is used by each LED sub-node. The
> maximum value is then found and used as a limit to compute the final
> intensity of the current source. If a LED happens to have a lower limit,
> the PWM is then used to limit the current to the requested value.
> 
> In order to control the current source, 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@gmail.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/leds/leds-ncp5623.txt      | 60 ++++++++++++++++++++++
>  1 file changed, 60 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 000000000000..d83e5094343e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
> @@ -0,0 +1,60 @@
> +* 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. It depends
> +    on the value of the external bias resistor Rbias, following
> +    Iref = Vref / Rbias with Vref = 0.6V. This is used to set the intensity of
> +    the current that can be provided by the internal current source, based on
> +    the maximum current permitted by LED sub-nodes (see below), but no more than
> +    Imax = 2400 * Iref.
> +
> +LED sub-nodes
> +=============
> +
> +Required properties:
> +  - reg : LED channel number (0..2)
> +  - led-max-microamp: Maximum allowable current inside the LED in microampere.
> +    This property is used to limit the PWM ratio, based on the intensity of the
> +    internal current source (see above).
> +
> +Optional properties:
> +  - label: see Documentation/devicetree/bindings/leds/common.txt
> +  - linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example
> +=======
> +
> +led1: ncp5623@38 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "onnn,ncp5623";
> +	reg = <0x38>;
> +	onnn,led-iref-microamp = <10>;
> +
> +	led1r@0 {
> +		label = "ncp:power:red";
> +		linux,default-trigger = "default-on";
> +		reg = <0>;
> +		led-max-microamp = <20000>;
> +	};
> +
> +	led1b@1 {
> +		label = "ncp:power:blue";
> +		reg = <1>;
> +		led-max-microamp = <20000>;
> +	};
> +
> +	led1g@2 {
> +		label = "ncp:power:green";
> +		reg = <2>;
> +		led-max-microamp = <20000>;
> +	};
> +};
> 

Some time ago we was instructed how our DT bindings should
look like. See [0]. In view of that, the above should be changed
as below. Please note dropped "ncp:" segment from label. Also
LED class device name pattern is "devicename:colour:function",
so you will have to reverse the order of segments in your labels.
Last but not least - couldn't the LED functions be better
distinguishable - now we have "power" for all three LEDs?


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

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

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

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

You can refer to drivers/leds/leds-lp8860.c on how to construct
the label. Generally from the discussions with DT maintainer
it turned out that we shouldn't have controller name in the LED
class device name at all, but it will have to be addressed globally
for all LED class drivers at once at some point.


[0] https://patchwork.kernel.org/patch/10093757/

-- 
Best regards,
Jacek Anaszewski

[PATCH v4 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@gmail.com>
---
 drivers/leds/Kconfig        |  11 +++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-ncp5623.c | 228 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 240 insertions(+)
 create mode 100644 drivers/leds/leds-ncp5623.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 318a28fd58fe..344b96ec7803 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -706,6 +706,17 @@ config LEDS_NIC78BX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-nic78bx.
 
+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.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index a2a6b5a4f86d..3ef8186ca942 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -74,6 +74,7 @@ obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
 obj-$(CONFIG_LEDS_MLXCPLD)		+= leds-mlxcpld.o
 obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
 obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.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 000000000000..005caae67397
--- /dev/null
+++ b/drivers/leds/leds-ncp5623.c
@@ -0,0 +1,228 @@
+/*
+ * Copyright 2018 Florian Vaussard <florian.vaussard@gmail.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#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>
+
+#define NCP5623_MAX_LEDS	3
+#define NCP5623_MAX_STEPS	31
+#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 LED_TO_PWM_CMD(led)	((0x02 + led - 1) << 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 {
+	int led_no;
+	u32 led_max_current;
+	struct led_classdev ldev;
+	struct ncp5623_priv *priv;
+};
+
+struct ncp5623_priv {
+	struct ncp5623_led leds[NCP5623_MAX_LEDS];
+	u32 led_iref;
+	u32 leds_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 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_brightness_set(struct led_classdev *led_cdev,
+				  enum led_brightness brightness)
+{
+	struct ncp5623_led *led = ldev_to_led(led_cdev);
+
+	return ncp5623_send_cmd(led->priv, LED_TO_PWM_CMD(led->led_no),
+				brightness);
+}
+
+static int ncp5623_configure(struct device *dev,
+			     struct ncp5623_priv *priv)
+{
+	unsigned int i;
+	unsigned int n;
+	struct ncp5623_led *led;
+	int effective_current;
+	int err;
+
+	/* Setup the internal current source, avoid negative values */
+	n = 31 - min_t(unsigned int, 31, DIV_ROUND_UP(2400 * priv->led_iref,
+						      priv->leds_max_current));
+
+	effective_current = 2400 * priv->led_iref / (NCP5623_MAX_CURRENT - n);
+	dev_dbg(dev, "setting maximum current to %u uA\n", effective_current);
+
+	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->led_no == 0)
+			continue;
+
+		led->priv = priv;
+		led->ldev.brightness_set_blocking = ncp5623_brightness_set;
+
+		led->ldev.max_brightness =
+			min_t(unsigned int, NCP5623_MAX_STEPS,
+			      led->led_max_current * NCP5623_MAX_STEPS /
+			      effective_current);
+
+		err = devm_led_classdev_register(dev, &led->ldev);
+		if (err < 0) {
+			dev_err(dev, "couldn't register LED %s\n",
+				led->ldev.name);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int ncp5623_parse_dt(struct ncp5623_priv *priv, struct device_node *np)
+{
+	struct device_node *child;
+	struct ncp5623_led *led;
+	u32 reg;
+	int count;
+	int err;
+
+	err = of_property_read_u32(np, "onnn,led-iref-microamp",
+				   &priv->led_iref);
+	if (err)
+		return -EINVAL;
+
+	count = of_get_child_count(np);
+	if (count == 0 || count > NCP5623_MAX_LEDS)
+		return -EINVAL;
+
+	for_each_child_of_node(np, child) {
+		err = of_property_read_u32(child, "reg", &reg);
+		if (err)
+			return err;
+
+		/* reg should be 0 ... NCP5623_MAX_LEDS - 1 */
+		if (reg >= NCP5623_MAX_LEDS) {
+			err = -EINVAL;
+			goto dt_child_parse_error;
+		}
+
+		led = &priv->leds[reg];
+
+		/* while valid led_no is 1 ... NCP5623_MAX_LEDS */
+		if (led->led_no > 0) {
+			/* Already registered */
+			err = -EINVAL;
+			goto dt_child_parse_error;
+		}
+		led->led_no = reg + 1;
+
+		err = of_property_read_u32(child, "led-max-microamp",
+					   &led->led_max_current);
+		if (err || led->led_max_current > NCP5623_MAX_CURRENT_UA)
+			return -EINVAL;
+		if (led->led_max_current > priv->leds_max_current)
+			priv->leds_max_current = led->led_max_current;
+
+		led->ldev.name =
+			of_get_property(child, "label", NULL) ? : child->name;
+		led->ldev.default_trigger =
+			of_get_property(child, "linux,default-trigger", NULL);
+	}
+
+	return 0;
+
+dt_child_parse_error:
+	of_node_put(child);
+
+	return err;
+}
+
+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;
+	struct ncp5623_priv *priv;
+	int err;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	i2c_set_clientdata(client, priv);
+
+	err = ncp5623_parse_dt(priv, np);
+	if (err)
+		return err;
+
+	return ncp5623_configure(dev, priv);
+}
+
+static const struct of_device_id ncp5623_of_match[] = {
+	{ .compatible = "onnn,ncp5623" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ncp5623_of_match);
+
+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,
+	.id_table = ncp5623_id,
+};
+
+module_i2c_driver(ncp5623_driver);
+
+MODULE_AUTHOR("Florian Vaussard <florian.vaussard@gmail.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("NCP5623 LED driver");
-- 
2.13.6

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

[Andy Shevchenko]

On Wed, Feb 21, 2018 at 11:46 PM, Florian Vaussard
<florian.vaussard@gmail.com> 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.

First of all, why it's rebased on top of v4.15 while we have v4.16-rc2 already?

> +/*
> + * Copyright 2018 Florian Vaussard <florian.vaussard@gmail.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */

Check license-rules.rst.


> +#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)

Just put 5 instead. My eyes hurts by above.

> +#define LED_TO_PWM_CMD(led)    ((0x02 + led - 1) << NCP5623_CMD_SHIFT)

led -> (led)

> +
> +#define NCP5623_DATA_MASK      GENMASK(NCP5623_CMD_SHIFT - 1, 0)

While here is OK.

> +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev)

More usual pattern is
to_ncp5623_led()

> +       return (err < 0 ? err : 0);

Where this style comes from?
I mean redundant parens.

> +       err = of_property_read_u32(np, "onnn,led-iref-microamp",
> +                                  &priv->led_iref);

Don't we have some generic property for iref:s?

> +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;
> +       struct ncp5623_priv *priv;
> +       int err;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->client = client;
> +       i2c_set_clientdata(client, priv);
> +
> +       err = ncp5623_parse_dt(priv, np);
> +       if (err)
> +               return err;
> +

> +       return ncp5623_configure(dev, priv);

This sounds wrong.
You call configure while it does two unrelated things: configure + registration.

Name it properly and move closer to ->probe().

> +static const struct i2c_device_id ncp5623_id[] = {
> +       { "ncp5623" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(i2c, ncp5623_id);

Do you really need this?
Consider to switch to ->probe_new().

> +               .of_match_table = of_match_ptr(ncp5623_of_match),

Here you will get a compiler warning.
Remove this pointless of_match_ptr().

-- 
With Best Regards,
Andy Shevchenko

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

[Pavel Machek]

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

Hi!

> +module_i2c_driver(ncp5623_driver);
> +
> +MODULE_AUTHOR("Florian Vaussard <florian.vaussard@gmail.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("NCP5623 LED driver");

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

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

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