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

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 <alkuor@gmail.com>
---
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/20240208130115.GM689448@google.com/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

On Sat, 17 Feb 2024, Abdel Alkuor wrote:

> 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 <alkuor@gmail.com>
> ---
> 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/20240208130115.GM689448@google.com/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
> 
> 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..5a5c770bb61e
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-ncp5623.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NCP5623 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/led-class-multicolor.h>
> +
> +#define NCP5623_REG(x)			((x) << 0x5)

What's 0x5?  Probably worth defining.

> +#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);
> +}
> +
> +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;
> +
> +	guard(mutex)(&ncp->lock);

Are these self-unlocking?

> +	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;
> +	}
> +
> +	ret = ncp5623_write(ncp->client, NCP5623_DIMMING_TIME_REG, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = ncp5623_write(ncp->client, NCP5623_ILED_REG, brightness);
> +	if (ret)
> +		return ret;
> +
> +	ncp->old_brightness = brightness;

The nomenclature is confusing here.

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

> +	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 {
> +		reg = NCP5623_ILED_REG;
> +	}
> +
> +	ret = ncp5623_write(ncp->client, reg,
> +			    min(pattern[0].brightness, NCP5623_MAX_BRIGHTNESS));
> +	if (ret)
> +		return ret;
> +
> +	ret = ncp5623_write(ncp->client,
> +			    NCP5623_DIMMING_TIME_REG, pattern[0].delta_t / 8);

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

> +	if (ret)
> +		return ret;
> +
> +	if (abs(brightness_diff) == 1)
> +		ncp->delay = NCP5623_MAX_BRIGHTNESS + brightness_diff;
> +	else
> +		ncp->delay = abs(brightness_diff);

Please comment these lines.

> +	ncp->delay = msecs_to_jiffies(ncp->delay * pattern[0].delta_t) + jiffies;
> +
> +	ncp->old_brightness = pattern[0].brightness;
> +
> +	return 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?

> +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;
> +
> +	mc_node = device_get_named_child_node(dev, "multi-led");
> +	if (!mc_node)
> +		return -EINVAL;
> +
> +	fwnode_for_each_child_node(mc_node, led_node)
> +		num_subleds++;
> +
> +	subled_info = devm_kcalloc(dev, num_subleds,
> +				   sizeof(*subled_info), GFP_KERNEL);

No need to wrap here.  Checkpatch won't complain.

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

> +	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 <alkuor@gmail.com>");
> +MODULE_DESCRIPTION("NCP5623 Multi-LED driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
>

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

Hi Lee,
> > +#define NCP5623_REG(x)			((x) << 0x5)
> 
> What's 0x5?  Probably worth defining.
This is a function offset. I'll add a define.

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

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

> > +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);
 	...
}

> > +	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);
> > +
> > +	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?
> 
Ops! I'll remove it.

Thanks,
Abdel

On Mon, 04 Mar 2024, Abdel Alkuor wrote:

> On Fri, Mar 01, 2024 at 08:50:46AM +0000, Lee Jones wrote:
> 
> Hi Lee,
> > > +#define NCP5623_REG(x)			((x) << 0x5)
> > 
> > What's 0x5?  Probably worth defining.
> This is a function offset. I'll add a define.
> 
> > 
> > > +	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/

Neat.

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

Just 'brightness' will be fine.

> > > +	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.
> 
> > > +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);
>  	...
> }

Something's not right then.  If this is required, are you sure you're
not meant to do something here?  If there are times when this is not
required, it should be possible to omit it.