Received-SPF: pass (google.com: domain of linux-kernel+bounces-92319-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 5 Mar 2024 11:59:00 +0000
From: Lee Jones <lee@kernel.org>
To: Abdel Alkuor <alkuor@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Alice Chen <alice_chen@richtek.com>,
	Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	ChiYuan Huang <cy_huang@richtek.com>,
	=?iso-8859-1?Q?Andr=E9?= Apitzsch <git@apitzsch.eu>,
	Lukas Bulwahn <lukas.bulwahn@gmail.com>,
	Jean-Jacques Hiblot <jjhiblot@traphandler.com>,
	ChiaEn Wu <chiaen_wu@richtek.com>, linux-leds@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] leds: Add NCP5623 multi-led driver
Message-ID: <20240305115900.GC86322@google.com>
References: <20240305042049.1533279-1-alkuor@gmail.com>
 <20240305042049.1533279-2-alkuor@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240305042049.1533279-2-alkuor@gmail.com>

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@gmail.com>
> ---
> 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@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 | 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@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_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 [李琼斯]