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

[Lee Jones]

On Mon, 04 Mar 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@xxxxxxxxx>
> ---
> Changes in v3:
>  - Add defines for magic numbers
>  - Fix code style
>  - Add a comment how ncp->delay is calculated
>  - Don't free mc_node when probe is successful
>  - Link to v2: https://lore.kernel.org/all/20240217230956.630522-2-alkuor@xxxxxxxxx/
> 
> 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@xxxxxxxxxx/T/
>  drivers/leds/rgb/Kconfig        |  11 ++
>  drivers/leds/rgb/Makefile       |   1 +
>  drivers/leds/rgb/leds-ncp5623.c | 271 ++++++++++++++++++++++++++++++++
>  3 files changed, 283 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..b669c55c5483
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-ncp5623.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NCP5623 Multi-LED Driver
> + *
> + * Author: Abdel Alkuor <alkuor@xxxxxxxxx>
> + * 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_FUNCTION_OFFSET		0x5
> +#define NCP5623_REG(x)			((x) << NCP5623_FUNCTION_OFFSET)
> +
> +#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
> +#define NCP5623_MAX_DIM_TIME		240 /* ms */
> +#define NCP5623_DIM_STEP		8   /* ms */

Patch applied, however ...

Please follow-up with the following changes:

#define NCP5623_MAX_DIM_TIME_MS		240
#define NCP5623_DIM_STEP_MS		8

-- 
Lee Jones [李琼斯]