[PATCH 1/2] dt: bindings: leds: Add NCP5623 multi-LED Controller

[PATCH 1/2] dt: bindings: leds: Add NCP5623 multi-LED Controller

[Abdel Alkuor]

NCP5623 is DC-DC multi-LED controller which can be used for
RGB illumination or backlight LCD display. NCP5623
provides 94% peak efficiency.

Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
---
 .../bindings/leds/onnn,ncp5623.yaml           | 98 +++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml

diff --git a/Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml b/Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml
new file mode 100644
index 000000000000..696bc7d8c8f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml
@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/onnn,ncp5623.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ON Semiconductor NCP5623 multi-LED Driver
+
+maintainers:
+  - Abdel Alkuor <alkuor@gmail.com>
+
+description: |
+  NCP5623 Triple Output I2C Controlled LED Driver.
+  https://www.onsemi.com/pdf/datasheet/ncp5623-d.pdf
+
+properties:
+  compatible:
+    enum:
+      - onnn,ncp5623
+
+  reg:
+    enum:
+      - 0x38
+
+  multi-led:
+    type: object
+    $ref: leds-class-multicolor.yaml#
+    unevaluatedProperties: false
+
+    properties:
+      "#address-cells":
+        const: 1
+
+      "#size-cells":
+        const: 0
+
+    patternProperties:
+      "^led@[0-2]$":
+        type: object
+        $ref: common.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          reg:
+            description: Index of the LED.
+            minimum: 0
+            maximum: 2
+
+        required:
+          - reg
+          - color
+
+    required:
+      - "#address-cells"
+      - "#size-cells"
+
+required:
+  - compatible
+  - reg
+  - multi-led
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@38 {
+            compatible = "onnn,ncp5623";
+            reg = <0x38>;
+
+            multi-led {
+                color = <LED_COLOR_ID_RGB>;
+
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                led@0 {
+                    reg = <0>;
+                    color = <LED_COLOR_ID_RED>;
+                };
+
+                led@1 {
+                    reg = <1>;
+                    color = <LED_COLOR_ID_GREEN>;
+                };
+
+                led@2 {
+                    reg = <2>;
+                    color = <LED_COLOR_ID_BLUE>;
+                };
+            };
+        };
+    };
-- 
2.34.1

[PATCH 2/2] leds: Add NCP5623 multi-led driver

[Abdel Alkuor]

NCP5623 is DC-DC multi-LEDs driver with 94% peak
efficiency. NCP5623 has three PWMs which can be
programmed up to 32 steps giving 32768 colors hue.

NCP5623 driver supports gradual dimming upward/downward
with programmable delay steps. Also, the driver supports
driving a single LED or multi-LED like RGB.

Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
---
 .../sysfs-class-led-multicolor-driver-ncp5623 |  46 +++
 drivers/leds/rgb/Kconfig                      |  11 +
 drivers/leds/rgb/Makefile                     |   1 +
 drivers/leds/rgb/leds-ncp5623.c               | 320 ++++++++++++++++++
 4 files changed, 378 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623
 create mode 100644 drivers/leds/rgb/leds-ncp5623.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 b/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623
new file mode 100644
index 000000000000..6b9f4849852b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623
@@ -0,0 +1,46 @@
+What:		/sys/class/leds/<led>/direction
+Date:		Feb 2024
+KernelVersion:	6.8
+Contact:	Abdel Alkuor <alkuor@gmail.com>
+Description:
+		Set gradual dimming direction which
+		can be either none, up, or down.
+		After setting the direction, brightness
+		can be set using one of 31 steps.
+
+		==== ======== ==== ======== ==== ========
+		Step ILED(mA) Step ILED(mA) Step ILED(mA)
+		0    0        11   1.38      22   3.06
+		1    0.92     12   1.45      23   3.45
+		2    0.95     13   1.53      24   3.94
+		3    0.98     14   1.62      25   4.60
+		4    1.02     15   1.72      26   5.52
+		5    1.06     16   1.84      27   6.90
+		6    1.10     17   1.97      28   9.20
+		7    1.15     18   2.12      29   13.80
+		8    1.20     19   2.30      30   27.60
+		9    1.25     20   2.50      31   27.60
+		10   1.31     21   2.76
+		==== ======== ==== ======== ==== ========
+
+What:		/sys/class/leds/<led>/dim_step
+Date:		Feb 2024
+KernelVersion:	6.8
+Contact:	Abdel Alkuor <alkuor@gmail.com>
+Description:
+		Set gradual dimming time.
+
+		==== ======== ==== ======== ==== ========
+		Step Time(ms) Step Time(ms) Step Time(ms)
+		0     0       11   88       22   176
+		1     8       12   96       23   184
+		2     16      13   104      24   192
+		3     24      14   112      25   200
+		4     32      15   120      26   208
+		5     40      16   128      27   216
+		6     48      17   136      28   224
+		7     56      18   144      29   232
+		8     64      19   152      30   240
+		9     72      20   160      31   248
+		10    80      21   168
+		==== ======== ==== ======== ==== ========
diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
index a6a21f564673..81ab6a526a78 100644
--- a/drivers/leds/rgb/Kconfig
+++ b/drivers/leds/rgb/Kconfig
@@ -27,6 +27,17 @@ config LEDS_KTD202X
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-ktd202x.
 
+config LEDS_NCP5623
+	tristate "LED support for NCP5623"
+	depends on I2C
+	depends on OF
+	help
+	  This option enables support for ON semiconductor NCP5623
+	  Triple Output I2C Controlled RGB LED Driver.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called leds-ncp5623.
+
 config LEDS_PWM_MULTICOLOR
 	tristate "PWM driven multi-color LED Support"
 	depends on PWM
diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
index 243f31e4d70d..a501fd27f179 100644
--- a/drivers/leds/rgb/Makefile
+++ b/drivers/leds/rgb/Makefile
@@ -2,6 +2,7 @@
 
 obj-$(CONFIG_LEDS_GROUP_MULTICOLOR)	+= leds-group-multicolor.o
 obj-$(CONFIG_LEDS_KTD202X)		+= leds-ktd202x.o
+obj-$(CONFIG_LEDS_NCP5623)		+= leds-ncp5623.o
 obj-$(CONFIG_LEDS_PWM_MULTICOLOR)	+= leds-pwm-multicolor.o
 obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-lpg.o
 obj-$(CONFIG_LEDS_MT6370_RGB)		+= leds-mt6370-rgb.o
diff --git a/drivers/leds/rgb/leds-ncp5623.c b/drivers/leds/rgb/leds-ncp5623.c
new file mode 100644
index 000000000000..e77dfca69ca3
--- /dev/null
+++ b/drivers/leds/rgb/leds-ncp5623.c
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * leds-ncp5623.c - Multi-LED Driver
+ *
+ * Author: Abdel Alkuor <alkuor@gmail.com>
+ * Datasheet: https://www.onsemi.com/pdf/datasheet/ncp5623-d.pdf
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/sysfs.h>
+#include <linux/workqueue.h>
+
+#include <linux/led-class-multicolor.h>
+
+#define NCP5623_REG(x)			((x) << 0x5)
+
+#define NCP5623_SHUTDOWN_REG		NCP5623_REG(0x0)
+#define NCP5623_ILED_REG		NCP5623_REG(0x1)
+#define NCP5623_PWM_REG(index)		NCP5623_REG(0x2 + (index))
+#define NCP5623_UPWARD_STEP_REG		NCP5623_REG(0x5)
+#define NCP5623_DOWNWARD_STEP_REG	NCP5623_REG(0x6)
+#define NCP5623_DIMMING_TIME_REG	NCP5623_REG(0x7)
+
+#define NCP5623_MAX_BRIGHTNESS		0x1f
+
+enum {
+	NCP5623_DIR_NONE,
+	NCP5623_DIR_UPWARD,
+	NCP5623_DIR_DOWNWARD,
+};
+
+static const char *const directions[] = {
+	[NCP5623_DIR_NONE]     = "none",
+	[NCP5623_DIR_UPWARD]   = "up",
+	[NCP5623_DIR_DOWNWARD] = "down",
+};
+
+struct ncp5623 {
+	struct i2c_client *client;
+	struct led_classdev_mc mc_dev;
+	struct mutex lock;
+
+	u8 direction;
+	u8 dim_step;
+	u8 old_brightness;
+	unsigned long delay;
+};
+
+static int ncp5623_write(struct i2c_client *client, u8 reg, u8 data)
+{
+	return i2c_smbus_write_byte_data(client, reg | data, 0);
+}
+
+static inline unsigned long
+ncp5623_get_completion_steps(u8 dir, int brightness, int old_brightness)
+{
+	int diff = brightness - old_brightness;
+
+	if (dir == NCP5623_DIR_UPWARD)
+		return diff <= 1 ? NCP5623_MAX_BRIGHTNESS + diff : diff;
+
+	return diff >= -1 ? NCP5623_MAX_BRIGHTNESS - diff : -diff;
+
+}
+
+static int ncp5623_brightness_set(struct led_classdev *cdev,
+				  enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
+	int ret;
+	u8 reg;
+	int i;
+
+	guard(mutex)(&ncp->lock);
+
+	if (ncp->delay && time_is_after_jiffies(ncp->delay))
+		return -EBUSY;
+
+	ncp->delay = 0;
+
+	for (i = 0; i < mc_cdev->num_colors; i++) {
+		ret = ncp5623_write(ncp->client,
+				    NCP5623_PWM_REG(mc_cdev->subled_info[i].channel),
+				    min(mc_cdev->subled_info[i].intensity,
+					NCP5623_MAX_BRIGHTNESS));
+		if (ret)
+			return ret;
+	}
+
+	switch (ncp->direction) {
+	case NCP5623_DIR_NONE:
+		reg = NCP5623_ILED_REG;
+		break;
+	case NCP5623_DIR_UPWARD:
+		reg = NCP5623_UPWARD_STEP_REG;
+		break;
+	case NCP5623_DIR_DOWNWARD:
+		reg = NCP5623_DOWNWARD_STEP_REG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = ncp5623_write(ncp->client, reg, brightness);
+	if (ret)
+		return ret;
+
+	if ((ncp->direction != NCP5623_DIR_NONE) && ncp->dim_step) {
+		ret = ncp5623_write(ncp->client,
+				    NCP5623_DIMMING_TIME_REG, ncp->dim_step);
+		if (ret)
+			return ret;
+
+		ncp->delay = ncp5623_get_completion_steps(ncp->direction,
+							  brightness,
+							  ncp->old_brightness);
+		/* dim step resolution is 8ms */
+		ncp->delay *= ncp->dim_step * 8;
+		ncp->delay = msecs_to_jiffies(ncp->delay) + jiffies;
+	}
+
+	ncp->old_brightness = brightness;
+
+	return 0;
+}
+
+static ssize_t dim_step_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(dev_get_drvdata(dev));
+	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
+	u8 value;
+	int ret;
+
+	ret = kstrtou8(buf, 0, &value);
+	if (ret)
+		return ret;
+
+	if (value > 0x1f)
+		return -EINVAL;
+
+	guard(mutex)(&ncp->lock);
+	ncp->dim_step = value;
+
+	return count;
+}
+
+static ssize_t dim_step_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(dev_get_drvdata(dev));
+	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
+
+	guard(mutex)(&ncp->lock);
+
+	return sysfs_emit(buf, "%u\n", ncp->dim_step);
+}
+
+static ssize_t direction_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(dev_get_drvdata(dev));
+	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
+	int ret;
+
+	ret = __sysfs_match_string(directions, ARRAY_SIZE(directions), buf);
+
+	switch (ret) {
+	case NCP5623_DIR_NONE:
+	case NCP5623_DIR_UPWARD:
+	case NCP5623_DIR_DOWNWARD:
+		guard(mutex)(&ncp->lock);
+		ncp->direction = ret;
+		return count;
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t direction_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(dev_get_drvdata(dev));
+	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
+
+	guard(mutex)(&ncp->lock);
+
+	return sysfs_emit(buf, "%s\n", directions[ncp->direction]);
+}
+
+
+static DEVICE_ATTR_RW(direction);
+static DEVICE_ATTR_RW(dim_step);
+
+static struct attribute *ncp5623_led_attrs[] = {
+	&dev_attr_direction.attr,
+	&dev_attr_dim_step.attr,
+	NULL
+};
+
+static const struct attribute_group ncp5623_led_group = {
+	.attrs = ncp5623_led_attrs,
+};
+
+static int ncp5623_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct fwnode_handle *mc_node, *led_node;
+	struct led_init_data init_data = { };
+	struct ncp5623 *ncp;
+	struct mc_subled *subled_info;
+	u32 color_index;
+	u32 reg;
+	int count = 0;
+	int ret;
+
+	ncp = devm_kzalloc(dev, sizeof(*ncp), GFP_KERNEL);
+	if (!ncp)
+		return -ENOMEM;
+
+	ncp->client = client;
+
+	mc_node = device_get_named_child_node(dev, "multi-led");
+	if (!mc_node)
+		return -EINVAL;
+
+	fwnode_for_each_child_node(mc_node, led_node)
+		count++;
+
+	subled_info = devm_kcalloc(dev, count, sizeof(*subled_info), GFP_KERNEL);
+	if (!subled_info) {
+		ret = -ENOMEM;
+		goto release_mc_node;
+	}
+
+	fwnode_for_each_available_child_node(mc_node, led_node) {
+		ret = fwnode_property_read_u32(led_node, "color", &color_index);
+		if (ret) {
+			fwnode_handle_put(led_node);
+			goto release_mc_node;
+		}
+
+		ret = fwnode_property_read_u32(led_node, "reg", &reg);
+		if (ret) {
+			fwnode_handle_put(led_node);
+			goto release_mc_node;
+		}
+
+		subled_info[ncp->mc_dev.num_colors].channel = reg;
+		subled_info[ncp->mc_dev.num_colors++].color_index = color_index;
+	}
+
+	init_data.fwnode = mc_node;
+
+	ncp->mc_dev.led_cdev.max_brightness = NCP5623_MAX_BRIGHTNESS;
+	ncp->mc_dev.subled_info = subled_info;
+	ncp->mc_dev.led_cdev.brightness_set_blocking = ncp5623_brightness_set;
+
+	mutex_init(&ncp->lock);
+	i2c_set_clientdata(client, ncp);
+
+	ret = devm_led_classdev_multicolor_register_ext(dev, &ncp->mc_dev, &init_data);
+	if (ret)
+		goto destroy_lock;
+
+	ret = sysfs_update_group(&ncp->mc_dev.led_cdev.dev->kobj, &ncp5623_led_group);
+	if (ret)
+		goto destroy_lock;
+
+	fwnode_handle_put(mc_node);
+
+	return 0;
+
+destroy_lock:
+	mutex_destroy(&ncp->lock);
+
+release_mc_node:
+	fwnode_handle_put(mc_node);
+
+	return ret;
+}
+
+static void ncp5623_remove(struct i2c_client *client)
+{
+	struct ncp5623 *ncp = i2c_get_clientdata(client);
+
+	ncp5623_write(client, NCP5623_SHUTDOWN_REG, 0);
+	mutex_destroy(&ncp->lock);
+}
+
+static void ncp5623_shutdown(struct i2c_client *client)
+{
+	ncp5623_write(client, NCP5623_SHUTDOWN_REG, 0);
+}
+
+static const struct of_device_id ncp5623_id[] = {
+	{ .compatible = "onnn,ncp5623" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ncp5623_id);
+
+static struct i2c_driver ncp5623_i2c_driver = {
+	.driver	= {
+		.name = "ncp5623",
+		.of_match_table = ncp5623_id,
+	},
+	.probe = ncp5623_probe,
+	.remove = ncp5623_remove,
+	.shutdown = ncp5623_shutdown,
+};
+
+module_i2c_driver(ncp5623_i2c_driver);
+
+MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com>");
+MODULE_DESCRIPTION("NCP5623 Multi-LED driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 2/2] leds: Add NCP5623 multi-led driver

[Lee Jones]

On Sat, 03 Feb 2024, Abdel Alkuor wrote:

> NCP5623 is DC-DC multi-LEDs driver with 94% peak

50 char lines look odd.  Please expand.

> efficiency. NCP5623 has three PWMs which can be
> programmed up to 32 steps giving 32768 colors hue.
> 
> NCP5623 driver supports gradual dimming upward/downward
> with programmable delay steps. Also, the driver supports
> driving a single LED or multi-LED like RGB.
> 
> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> ---
>  .../sysfs-class-led-multicolor-driver-ncp5623 |  46 +++
>  drivers/leds/rgb/Kconfig                      |  11 +
>  drivers/leds/rgb/Makefile                     |   1 +
>  drivers/leds/rgb/leds-ncp5623.c               | 320 ++++++++++++++++++
>  4 files changed, 378 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623
>  create mode 100644 drivers/leds/rgb/leds-ncp5623.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 b/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623
> new file mode 100644
> index 000000000000..6b9f4849852b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623
> @@ -0,0 +1,46 @@
> +What:		/sys/class/leds/<led>/direction

Not sure I understand this interface.

Can't we take the direction by comparing the old and new brightness?

> +Date:		Feb 2024
> +KernelVersion:	6.8
> +Contact:	Abdel Alkuor <alkuor@gmail.com>
> +Description:
> +		Set gradual dimming direction which
> +		can be either none, up, or down.
> +		After setting the direction, brightness
> +		can be set using one of 31 steps.
> +
> +		==== ======== ==== ======== ==== ========
> +		Step ILED(mA) Step ILED(mA) Step ILED(mA)
> +		0    0        11   1.38      22   3.06
> +		1    0.92     12   1.45      23   3.45
> +		2    0.95     13   1.53      24   3.94
> +		3    0.98     14   1.62      25   4.60
> +		4    1.02     15   1.72      26   5.52
> +		5    1.06     16   1.84      27   6.90
> +		6    1.10     17   1.97      28   9.20
> +		7    1.15     18   2.12      29   13.80
> +		8    1.20     19   2.30      30   27.60
> +		9    1.25     20   2.50      31   27.60
> +		10   1.31     21   2.76
> +		==== ======== ==== ======== ==== ========
> +
> +What:		/sys/class/leds/<led>/dim_step

The step principle seems a bit arbitrary.

Why not provide the time directly?

dim_step_delay?

I already see documentation for risetime and falltime.

Perhaps that will omit the need for both direction and step?

> +Date:		Feb 2024
> +KernelVersion:	6.8
> +Contact:	Abdel Alkuor <alkuor@gmail.com>
> +Description:
> +		Set gradual dimming time.
> +
> +		==== ======== ==== ======== ==== ========
> +		Step Time(ms) Step Time(ms) Step Time(ms)
> +		0     0       11   88       22   176
> +		1     8       12   96       23   184
> +		2     16      13   104      24   192
> +		3     24      14   112      25   200
> +		4     32      15   120      26   208
> +		5     40      16   128      27   216
> +		6     48      17   136      28   224
> +		7     56      18   144      29   232
> +		8     64      19   152      30   240
> +		9     72      20   160      31   248
> +		10    80      21   168
> +		==== ======== ==== ======== ==== ========
> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> index a6a21f564673..81ab6a526a78 100644
> --- a/drivers/leds/rgb/Kconfig
> +++ b/drivers/leds/rgb/Kconfig
> @@ -27,6 +27,17 @@ config LEDS_KTD202X
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-ktd202x.
>  
> +config LEDS_NCP5623
> +	tristate "LED support for NCP5623"
> +	depends on I2C
> +	depends on OF
> +	help
> +	  This option enables support for ON semiconductor NCP5623
> +	  Triple Output I2C Controlled RGB LED Driver.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-ncp5623.
> +
>  config LEDS_PWM_MULTICOLOR
>  	tristate "PWM driven multi-color LED Support"
>  	depends on PWM
> diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
> index 243f31e4d70d..a501fd27f179 100644
> --- a/drivers/leds/rgb/Makefile
> +++ b/drivers/leds/rgb/Makefile
> @@ -2,6 +2,7 @@
>  
>  obj-$(CONFIG_LEDS_GROUP_MULTICOLOR)	+= leds-group-multicolor.o
>  obj-$(CONFIG_LEDS_KTD202X)		+= leds-ktd202x.o
> +obj-$(CONFIG_LEDS_NCP5623)		+= leds-ncp5623.o
>  obj-$(CONFIG_LEDS_PWM_MULTICOLOR)	+= leds-pwm-multicolor.o
>  obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-lpg.o
>  obj-$(CONFIG_LEDS_MT6370_RGB)		+= leds-mt6370-rgb.o
> diff --git a/drivers/leds/rgb/leds-ncp5623.c b/drivers/leds/rgb/leds-ncp5623.c
> new file mode 100644
> index 000000000000..e77dfca69ca3
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-ncp5623.c
> @@ -0,0 +1,320 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * leds-ncp5623.c - Multi-LED Driver

Remove filenames they have a habit of becoming out of sync.


> + * Author: Abdel Alkuor <alkuor@gmail.com>
> + * Datasheet: https://www.onsemi.com/pdf/datasheet/ncp5623-d.pdf
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/sysfs.h>
> +#include <linux/workqueue.h>
> +
> +#include <linux/led-class-multicolor.h>
> +
> +#define NCP5623_REG(x)			((x) << 0x5)
> +
> +#define NCP5623_SHUTDOWN_REG		NCP5623_REG(0x0)
> +#define NCP5623_ILED_REG		NCP5623_REG(0x1)
> +#define NCP5623_PWM_REG(index)		NCP5623_REG(0x2 + (index))
> +#define NCP5623_UPWARD_STEP_REG		NCP5623_REG(0x5)
> +#define NCP5623_DOWNWARD_STEP_REG	NCP5623_REG(0x6)
> +#define NCP5623_DIMMING_TIME_REG	NCP5623_REG(0x7)
> +
> +#define NCP5623_MAX_BRIGHTNESS		0x1f
> +
> +enum {
> +	NCP5623_DIR_NONE,
> +	NCP5623_DIR_UPWARD,
> +	NCP5623_DIR_DOWNWARD,
> +};
> +
> +static const char *const directions[] = {
> +	[NCP5623_DIR_NONE]     = "none",

What does "none" do?

> +	[NCP5623_DIR_UPWARD]   = "up",
> +	[NCP5623_DIR_DOWNWARD] = "down",
> +};
> +
> +struct ncp5623 {
> +	struct i2c_client *client;
> +	struct led_classdev_mc mc_dev;
> +	struct mutex lock;
> +
> +	u8 direction;
> +	u8 dim_step;
> +	u8 old_brightness;
> +	unsigned long delay;
> +};
> +
> +static int ncp5623_write(struct i2c_client *client, u8 reg, u8 data)
> +{
> +	return i2c_smbus_write_byte_data(client, reg | data, 0);
> +}
> +
> +static inline unsigned long
> +ncp5623_get_completion_steps(u8 dir, int brightness, int old_brightness)
> +{

Why not pass in ncp and omit dir and old_brightness?

> +	int diff = brightness - old_brightness;
> +
> +	if (dir == NCP5623_DIR_UPWARD)
> +		return diff <= 1 ? NCP5623_MAX_BRIGHTNESS + diff : diff;
> +
> +	return diff >= -1 ? NCP5623_MAX_BRIGHTNESS - diff : -diff;
> +
> +}
> +
> +static int ncp5623_brightness_set(struct led_classdev *cdev,
> +				  enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
> +	int ret;
> +	u8 reg;
> +	int i;
+
> +	guard(mutex)(&ncp->lock);
> +
> +	if (ncp->delay && time_is_after_jiffies(ncp->delay))
> +		return -EBUSY;
> +
> +	ncp->delay = 0;
> +
> +	for (i = 0; i < mc_cdev->num_colors; i++) {

Looks like C++ style "for (int i" just became popular.

> +		ret = ncp5623_write(ncp->client,
> +				    NCP5623_PWM_REG(mc_cdev->subled_info[i].channel),
> +				    min(mc_cdev->subled_info[i].intensity,
> +					NCP5623_MAX_BRIGHTNESS));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (ncp->direction) {
> +	case NCP5623_DIR_NONE:
> +		reg = NCP5623_ILED_REG;
> +		break;
> +	case NCP5623_DIR_UPWARD:
> +		reg = NCP5623_UPWARD_STEP_REG;
> +		break;
> +	case NCP5623_DIR_DOWNWARD:
> +		reg = NCP5623_DOWNWARD_STEP_REG;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = ncp5623_write(ncp->client, reg, brightness);
> +	if (ret)
> +		return ret;
> +
> +	if ((ncp->direction != NCP5623_DIR_NONE) && ncp->dim_step) {
> +		ret = ncp5623_write(ncp->client,
> +				    NCP5623_DIMMING_TIME_REG, ncp->dim_step);
> +		if (ret)
> +			return ret;
> +
> +		ncp->delay = ncp5623_get_completion_steps(ncp->direction,
> +							  brightness,
> +							  ncp->old_brightness);
> +		/* dim step resolution is 8ms */

Start comments with a capital letter.

> +		ncp->delay *= ncp->dim_step * 8;
> +		ncp->delay = msecs_to_jiffies(ncp->delay) + jiffies;
> +	}
> +
> +	ncp->old_brightness = brightness;
> +
> +	return 0;
> +}
> +
> +static ssize_t dim_step_store(struct device *dev,
> +			      struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(dev_get_drvdata(dev));
> +	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
> +	u8 value;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 0, &value);
> +	if (ret)
> +		return ret;
> +
> +	if (value > 0x1f)
> +		return -EINVAL;
> +
> +	guard(mutex)(&ncp->lock);
> +	ncp->dim_step = value;
> +
> +	return count;
> +}
> +
> +static ssize_t dim_step_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(dev_get_drvdata(dev));
> +	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
> +
> +	guard(mutex)(&ncp->lock);
> +
> +	return sysfs_emit(buf, "%u\n", ncp->dim_step);
> +}
> +
> +static ssize_t direction_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(dev_get_drvdata(dev));
> +	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
> +	int ret;
> +
> +	ret = __sysfs_match_string(directions, ARRAY_SIZE(directions), buf);
> +
> +	switch (ret) {
> +	case NCP5623_DIR_NONE:
> +	case NCP5623_DIR_UPWARD:
> +	case NCP5623_DIR_DOWNWARD:
> +		guard(mutex)(&ncp->lock);
> +		ncp->direction = ret;

What's stopping you setting reg here?

> +		return count;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t direction_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(dev_get_drvdata(dev));
> +	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
> +
> +	guard(mutex)(&ncp->lock);
> +
> +	return sysfs_emit(buf, "%s\n", directions[ncp->direction]);
> +}
> +
> +

Too many '\n's

> +static DEVICE_ATTR_RW(direction);
> +static DEVICE_ATTR_RW(dim_step);
> +
> +static struct attribute *ncp5623_led_attrs[] = {
> +	&dev_attr_direction.attr,
> +	&dev_attr_dim_step.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group ncp5623_led_group = {
> +	.attrs = ncp5623_led_attrs,
> +};
> +
> +static int ncp5623_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *mc_node, *led_node;
> +	struct led_init_data init_data = { };
> +	struct ncp5623 *ncp;
> +	struct mc_subled *subled_info;
> +	u32 color_index;
> +	u32 reg;
> +	int count = 0;

num_subleds?

> +	int ret;
> +
> +	ncp = devm_kzalloc(dev, sizeof(*ncp), GFP_KERNEL);
> +	if (!ncp)
> +		return -ENOMEM;
> +
> +	ncp->client = client;
> +
> +	mc_node = device_get_named_child_node(dev, "multi-led");
> +	if (!mc_node)
> +		return -EINVAL;
> +
> +	fwnode_for_each_child_node(mc_node, led_node)
> +		count++;
> +
> +	subled_info = devm_kcalloc(dev, count, sizeof(*subled_info), GFP_KERNEL);
> +	if (!subled_info) {
> +		ret = -ENOMEM;
> +		goto release_mc_node;
> +	}
> +
> +	fwnode_for_each_available_child_node(mc_node, led_node) {
> +		ret = fwnode_property_read_u32(led_node, "color", &color_index);
> +		if (ret) {
> +			fwnode_handle_put(led_node);
> +			goto release_mc_node;
> +		}
> +
> +		ret = fwnode_property_read_u32(led_node, "reg", &reg);
> +		if (ret) {
> +			fwnode_handle_put(led_node);
> +			goto release_mc_node;
> +		}
> +
> +		subled_info[ncp->mc_dev.num_colors].channel = reg;
> +		subled_info[ncp->mc_dev.num_colors++].color_index = color_index;
> +	}
> +
> +	init_data.fwnode = mc_node;
> +
> +	ncp->mc_dev.led_cdev.max_brightness = NCP5623_MAX_BRIGHTNESS;
> +	ncp->mc_dev.subled_info = subled_info;
> +	ncp->mc_dev.led_cdev.brightness_set_blocking = ncp5623_brightness_set;
> +
> +	mutex_init(&ncp->lock);
> +	i2c_set_clientdata(client, ncp);
> +
> +	ret = devm_led_classdev_multicolor_register_ext(dev, &ncp->mc_dev, &init_data);
> +	if (ret)
> +		goto destroy_lock;
> +
> +	ret = sysfs_update_group(&ncp->mc_dev.led_cdev.dev->kobj, &ncp5623_led_group);
> +	if (ret)
> +		goto destroy_lock;
> +
> +	fwnode_handle_put(mc_node);
> +
> +	return 0;
> +
> +destroy_lock:
> +	mutex_destroy(&ncp->lock);
> +
> +release_mc_node:
> +	fwnode_handle_put(mc_node);
> +
> +	return ret;
> +}
> +
> +static void ncp5623_remove(struct i2c_client *client)
> +{
> +	struct ncp5623 *ncp = i2c_get_clientdata(client);
> +
> +	ncp5623_write(client, NCP5623_SHUTDOWN_REG, 0);
> +	mutex_destroy(&ncp->lock);
> +}
> +
> +static void ncp5623_shutdown(struct i2c_client *client)
> +{
> +	ncp5623_write(client, NCP5623_SHUTDOWN_REG, 0);

No need to destroy the mutex?

> +}
> +
> +static const struct of_device_id ncp5623_id[] = {
> +	{ .compatible = "onnn,ncp5623" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ncp5623_id);
> +
> +static struct i2c_driver ncp5623_i2c_driver = {
> +	.driver	= {
> +		.name = "ncp5623",
> +		.of_match_table = ncp5623_id,
> +	},
> +	.probe = ncp5623_probe,
> +	.remove = ncp5623_remove,
> +	.shutdown = ncp5623_shutdown,
> +};
> +
> +module_i2c_driver(ncp5623_i2c_driver);
> +
> +MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com>");
> +MODULE_DESCRIPTION("NCP5623 Multi-LED driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/2] leds: Add NCP5623 multi-led driver

[Abdel Alkuor]

On Thu, Feb 08, 2024 at 01:01:15PM +0000, Lee Jones wrote:
> On Sat, 03 Feb 2024, Abdel Alkuor wrote:
>
Hi Lee,

Please check the inline comment. All other comments will be addressed
in v2.

> > +What:		/sys/class/leds/<led>/dim_step
> 
> The step principle seems a bit arbitrary.
> 
> Why not provide the time directly?
> 
> dim_step_delay?
> 
> I already see documentation for risetime and falltime.
> 
> Perhaps that will omit the need for both direction and step?
>
I'm going to drop off both and use risetime and falltime. That being
said, the documented risetime and falltime for lm3533 use steps instead of
entering the time directly. This is my first time doing this, should I document
risetime/falltime in sysfs-class-led-multicolor-driver-ncp5623? or should
I update risetime/falltime in sysfs-class-led-driver-lm3533 to reflect
risetime/falltime for ncp5623?
> > +Date:		Feb 2024
> > +KernelVersion:	6.8
> > +Contact:	Abdel Alkuor <alkuor@gmail.com>
> > +Description:
> > +		Set gradual dimming time.
> > +
> > +		==== ======== ==== ======== ==== ========
> > +		Step Time(ms) Step Time(ms) Step Time(ms)
> > +		0     0       11   88       22   176
> > +		1     8       12   96       23   184
> > +		2     16      13   104      24   192
> > +		3     24      14   112      25   200
> > +		4     32      15   120      26   208
> > +		5     40      16   128      27   216
> > +		6     48      17   136      28   224
> > +		7     56      18   144      29   232
> > +		8     64      19   152      30   240
> > +		9     72      20   160      31   248
> > +		10    80      21   168
> > +		==== ======== ==== ======== ==== ========

Thanks,
Abdel

Re: [PATCH 2/2] leds: Add NCP5623 multi-led driver

[Lee Jones]

On Sun, 11 Feb 2024, Abdel Alkuor wrote:

> On Thu, Feb 08, 2024 at 01:01:15PM +0000, Lee Jones wrote:
> > On Sat, 03 Feb 2024, Abdel Alkuor wrote:
> >
> Hi Lee,
> 
> Please check the inline comment. All other comments will be addressed
> in v2.
> 
> > > +What:		/sys/class/leds/<led>/dim_step
> > 
> > The step principle seems a bit arbitrary.
> > 
> > Why not provide the time directly?
> > 
> > dim_step_delay?
> > 
> > I already see documentation for risetime and falltime.
> > 
> > Perhaps that will omit the need for both direction and step?
> >
> I'm going to drop off both and use risetime and falltime. That being
> said, the documented risetime and falltime for lm3533 use steps instead of
> entering the time directly. This is my first time doing this, should I document
> risetime/falltime in sysfs-class-led-multicolor-driver-ncp5623? or should
> I update risetime/falltime in sysfs-class-led-driver-lm3533 to reflect
> risetime/falltime for ncp5623?

Keep them separate please.

> > > +Date:		Feb 2024
> > > +KernelVersion:	6.8
> > > +Contact:	Abdel Alkuor <alkuor@gmail.com>
> > > +Description:
> > > +		Set gradual dimming time.
> > > +
> > > +		==== ======== ==== ======== ==== ========
> > > +		Step Time(ms) Step Time(ms) Step Time(ms)
> > > +		0     0       11   88       22   176
> > > +		1     8       12   96       23   184
> > > +		2     16      13   104      24   192
> > > +		3     24      14   112      25   200
> > > +		4     32      15   120      26   208
> > > +		5     40      16   128      27   216
> > > +		6     48      17   136      28   224
> > > +		7     56      18   144      29   232
> > > +		8     64      19   152      30   240
> > > +		9     72      20   160      31   248
> > > +		10    80      21   168
> > > +		==== ======== ==== ======== ==== ========
> 
> Thanks,
> Abdel

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/2] leds: Add NCP5623 multi-led driver

[Pavel Machek]

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

Hi!

>  .../sysfs-class-led-multicolor-driver-ncp5623 |  46 +++
>  drivers/leds/rgb/Kconfig                      |  11 +
>  drivers/leds/rgb/Makefile                     |   1 +
>  drivers/leds/rgb/leds-ncp5623.c               | 320 ++++++++++++++++++
>  4 files changed, 378 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623
>  create mode 100644 drivers/leds/rgb/leds-ncp5623.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623 b/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623
> new file mode 100644
> index 000000000000..6b9f4849852b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor-driver-ncp5623
> @@ -0,0 +1,46 @@
> +What:		/sys/class/leds/<led>/direction
> +Date:		Feb 2024
> +What:		/sys/class/leds/<led>/dim_step

You are reinventing hardware_pattern trigger. NAK.

I suggest you add basic support first, then look at hardware pattern
trigger and add that support in separate patch.

Thanks,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 2/2] leds: Add NCP5623 multi-led driver

[Abdel Alkuor]

On Tue, Feb 13, 2024 at 10:18:39AM +0100, Pavel Machek wrote:
Hi Pavel,
> > +What:		/sys/class/leds/<led>/dim_step
> 
> You are reinventing hardware_pattern trigger. NAK.
> 
> I suggest you add basic support first, then look at hardware pattern
> trigger and add that support in separate patch.
>
This makes a lot of sense and simplifies the driver.

I'd like to add the hardware pattern as part of this patch as it is already
implemented previously but I needed to do some clean up :)

Thanks,
Abdel

Re: [PATCH 1/2] dt: bindings: leds: Add NCP5623 multi-LED Controller

[Krzysztof Kozlowski]

On 03/02/2024 18:58, Abdel Alkuor wrote:
> NCP5623 is DC-DC multi-LED controller which can be used for
> RGB illumination or backlight LCD display. NCP5623
> provides 94% peak efficiency.

Drop marketing.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

It's dt-bindings.

> 
> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> ---
>  .../bindings/leds/onnn,ncp5623.yaml           | 98 +++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml b/Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml
> new file mode 100644
> index 000000000000..696bc7d8c8f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/onnn,ncp5623.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/onnn,ncp5623.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ON Semiconductor NCP5623 multi-LED Driver
> +
> +maintainers:
> +  - Abdel Alkuor <alkuor@gmail.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  NCP5623 Triple Output I2C Controlled LED Driver.
> +  https://www.onsemi.com/pdf/datasheet/ncp5623-d.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - onnn,ncp5623
> +
> +  reg:
> +    enum:

Instead "const", or just maxItems: 1


> +      - 0x38
> +
> +  multi-led:
> +    type: object
> +    $ref: leds-class-multicolor.yaml#
> +    unevaluatedProperties: false
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +    patternProperties:
> +      "^led@[0-2]$":
> +        type: object
> +        $ref: common.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          reg:
> +            description: Index of the LED.

Drop description, it is obvious.

> +            minimum: 0
> +            maximum: 2
> +
> +        required:
> +          - reg
> +          - color
> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"



Best regards,
Krzysztof