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

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

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
> 
>

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

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

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

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