dt-bindings: leds: Add NCP5623 multi-LED Controller

NCP5623 is DC-DC multi-LEDs driver which has three PWMs that can be programmed up to 32 steps giving 32768 colors hue.

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

Signed-off-by Abdel Alkuor <[email protected]>

---
Changes in v2:
- Remove all custom attributes and use hw pattern instead
- Remove filename from the driver description
- Fix coding style
- Destroy the muttex in shutdown callback
- Register mcled device using none devm version as unregistering mcled device calls ncp5632_set_led which uses mutex hence we need to make sure the mutex is still available during the unregistering process.
- Link to v1: https://lore.kernel.org/linux-kernel/[email protected]/T/

 drivers/leds/rgb/Kconfig        |  11 ++
 drivers/leds/rgb/Makefile       |   1 +
 drivers/leds/rgb/leds-ncp5623.c | 257 ++++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/leds/rgb/leds-ncp5623.c

drivers/leds/rgb/Kconfig +11 -0

index a6a21f564673..81ab6a526a78 100644
@@ -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

drivers/leds/rgb/Makefile +1 -0

index 243f31e4d70d..a501fd27f179 100644
@@ -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

drivers/leds/rgb/leds-ncp5623.c new file +4 -0

new file mode 100644
index 000000000000..5a5c770bb61e
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * NCP5623 Multi-LED Driver
+ *

drivers/leds/rgb/leds-ncp5623.c new file +21 -0

+#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
+
+struct ncp5623 {
+	struct i2c_client *client;
+	struct led_classdev_mc mc_dev;
+	struct mutex lock;
+
+	int 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);

drivers/leds/rgb/leds-ncp5623.c new file +13 -0

+	if (ncp->delay && time_is_after_jiffies(ncp->delay))
+		return -EBUSY;
+
+	ncp->delay = 0;
+
+	for (int 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;
+	}

drivers/leds/rgb/leds-ncp5623.c new file +34 -0

+	return 0;
+}
+
+static int ncp5623_pattern_set(struct led_classdev *cdev,
+			       struct led_pattern *pattern,
+			       u32 len, int repeat)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct ncp5623 *ncp = container_of(mc_cdev, struct ncp5623, mc_dev);
+	int brightness_diff;
+	u8 reg;
+	int ret;
+
+	guard(mutex)(&ncp->lock);
+
+	if (ncp->delay && time_is_after_jiffies(ncp->delay))
+		return -EBUSY;
+
+	ncp->delay = 0;
+
+	if (pattern[0].delta_t > 240 || (pattern[0].delta_t % 8) != 0)
+		return -EINVAL;
+
+	brightness_diff = pattern[0].brightness - ncp->old_brightness;
+
+	if (brightness_diff == 0)
+		return 0;
+
+	if (pattern[0].delta_t) {
+		if (brightness_diff > 0)
+			reg = NCP5623_UPWARD_STEP_REG;
+		else
+			reg = NCP5623_DOWNWARD_STEP_REG;
+	} else {

drivers/leds/rgb/leds-ncp5623.c new file +17 -0

+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 = { };
+	int num_subleds = 0;
+	struct ncp5623 *ncp;
+	struct mc_subled *subled_info;
+	u32 color_index;
+	u32 reg;
+	int ret;
+
+	ncp = devm_kzalloc(dev, sizeof(*ncp), GFP_KERNEL);
+	if (!ncp)
+		return -ENOMEM;
+
+	ncp->client = client;

drivers/leds/rgb/leds-ncp5623.c new file +29 -0

+	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;
+	ncp->mc_dev.led_cdev.pattern_set = ncp5623_pattern_set;
+	ncp->mc_dev.led_cdev.pattern_clear = ncp5623_pattern_clear;

drivers/leds/rgb/leds-ncp5623.c new file +55 -0

+	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);
+
+	mutex_lock(&ncp->lock);
+	ncp->delay = 0;
+	mutex_unlock(&ncp->lock);
+
+	ncp5623_write(client, NCP5623_DIMMING_TIME_REG, 0);
+	led_classdev_multicolor_unregister(&ncp->mc_dev);
+	mutex_destroy(&ncp->lock);
+}
+
+static void ncp5623_shutdown(struct i2c_client *client)
+{
+	struct ncp5623 *ncp = i2c_get_clientdata(client);
+
+	if (!(ncp->mc_dev.led_cdev.flags & LED_RETAIN_AT_SHUTDOWN))
+		ncp5623_write(client, NCP5623_SHUTDOWN_REG, 0);
+
+	mutex_destroy(&ncp->lock);
+}
+
+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 <[email protected]>");
+MODULE_DESCRIPTION("NCP5623 Multi-LED driver");
+MODULE_LICENSE("GPL");

Signature · 1 line

2.34.1

Quoted 1 line

+#define NCP5623_REG(x) ((x) << 0x5)

What's 0x5? Probably worth defining.

Review

Quoted 2 lines

(quoted diff) +1 -0

+	guard(mutex)(&ncp->lock);

Are these self-unlocking?

drivers/leds/rgb/leds-ncp5623.c line 70

Quoted 2 lines

drivers/leds/rgb/leds-ncp5623.c new file +1 -0

+	ncp->old_brightness = brightness;

The nomenclature is confusing here.

For the most part, this will carry the present value, no?

Review

Quoted 2 lines

(quoted diff) +2 -0

+	ret = ncp5623_write(ncp->client,
+			    NCP5623_DIMMING_TIME_REG, pattern[0].delta_t / 8);

Why 8? Magic numbers should be replaced with #defines.

drivers/leds/rgb/leds-ncp5623.c line 134

Quoted 4 lines

drivers/leds/rgb/leds-ncp5623.c new file +4 -0

+static int ncp5623_pattern_clear(struct led_classdev *led_cdev)
+{
+	return 0;
+}

Not sure I see the point in this.

Is the .pattern_clear() compulsorily?

Quoted 11 lines

drivers/leds/rgb/leds-ncp5623.c new file +11 -0

+	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;
+	ncp->mc_dev.led_cdev.pattern_set = ncp5623_pattern_set;
+	ncp->mc_dev.led_cdev.pattern_clear = ncp5623_pattern_clear;
+	ncp->mc_dev.led_cdev.default_trigger = "pattern";
+
+	mutex_init(&ncp->lock);
+	i2c_set_clientdata(client, ncp);

drivers/leds/rgb/leds-ncp5623.c line 201

Quoted 6 lines

drivers/leds/rgb/leds-ncp5623.c new file +6 -0

+
+	ret = led_classdev_multicolor_register_ext(dev, &ncp->mc_dev, &init_data);
+	if (ret)
+		goto destroy_lock;
+
+	fwnode_handle_put(mc_node);

Didn't you just store this ~16 lines up?

On Fri, Mar 01, 2024 at 08:50:46AM +0000, Lee Jones wrote:

Hi Lee,

Quoted 3 lines

Quoted 1 line

+#define NCP5623_REG(x) ((x) << 0x5)

What's 0x5? Probably worth defining.

This is a function offset. I'll add a define.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+	guard(mutex)(&ncp->lock);

Are these self-unlocking?

Correct. Here is a short introduction about it
https://www.marcusfolkesson.se/blog/mutex-guards-in-the-linux-kernel/

Quoted 6 lines

drivers/leds/rgb/leds-ncp5623.c line 70

Quoted 1 line

drivers/leds/rgb/leds-ncp5623.c new file +1 -0

+	ncp->old_brightness = brightness;

The nomenclature is confusing here.

For the most part, this will carry the present value, no?

Yes, I'll change it to current_brightness instead

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	ret = ncp5623_write(ncp->client,
+			    NCP5623_DIMMING_TIME_REG, pattern[0].delta_t / 8);

Why 8? Magic numbers should be replaced with #defines.

This is dim step in ms. I'll add a define for it.

Quoted 9 lines

drivers/leds/rgb/leds-ncp5623.c line 134

Quoted 4 lines

drivers/leds/rgb/leds-ncp5623.c new file +4 -0

+static int ncp5623_pattern_clear(struct led_classdev *led_cdev)
+{
+	return 0;
+}

Not sure I see the point in this.

Is the .pattern_clear() compulsorily?

Unfortunately, it is. For example, in pattern_trig_store_patterns, when hw pattern is used, it is expected to have pattern_clear implemented.

static ssize_t pattern_trig_store_patterns(struct led_classdev *led_cdev,
const char *buf, const u32 *buf_int, size_t count, bool hw_pattern)

{
	...
         if (data->is_hw_pattern)
                 led_cdev->pattern_clear(led_cdev);

...
}

NCP5623 is DC-DC multi-LED controller which can be used for RGB illumination or backlight LCD display.

Signed-off-by Abdel Alkuor <[email protected]>

---