linux-kernel - Re: [PATCH v16 2/3] leds: add basic support for TI/National Semiconductor LP5812 LED Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a9aa5064-5bd8-4340-a1b6-bec6895dde57@wanadoo.fr>
Date: Sat, 18 Oct 2025 09:22:48 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Nam Tran <trannamatk@...il.com>, lee@...nel.org
Cc: pavel@...nel.org, gregkh@...uxfoundation.org, rdunlap@...radead.org,
 krzk+dt@...nel.org, robh@...nel.org, conor+dt@...nel.org, corbet@....net,
 linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v16 2/3] leds: add basic support for TI/National
 Semiconductor LP5812 LED Driver


Le 13/10/2025 à 19:35, Nam Tran a écrit :
> The LP5812 is a 4x3 matrix RGB LED driver with an autonomous animation
> engine and time-cross-multiplexing (TCM) support for up to 12 LEDs or
> 4 RGB LEDs. Each LED can be configured through the related registers
> to realize vivid and fancy lighting effects.
>
> This patch adds minimal driver support for the LP5812, implementing
> only the essential functionality: I2C communication with the device,
> LED registration, brightness control in manual mode, and basic sysfs
> interfaces for LED configuration and fault monitoring.
>
> Signed-off-by: Nam Tran <trannamatk@...il.com>

Hi,

2 small nitpicks, if a v17 is need for other reasons.

...

> +static int lp5812_parse_led(struct device_node *np,
> +			    struct lp5812_led_config *cfg,
> +			    int led_index)
> +{
> +	int num_colors = 0, ret;
> +
> +	of_property_read_string(np, "label", &cfg[led_index].name);
> +
> +	ret = of_property_read_u32(np, "reg", &cfg[led_index].chan_nr);
> +	if (ret)
> +		return ret;
> +
> +	for_each_available_child_of_node_scoped(np, child) {
> +		ret = lp5812_parse_led_channel(child, cfg, led_index, num_colors);

In order to simplify lp5812_parse_led_channel() and save an argument 
here, we could pass &cfg[led_index] directly.

> +		if (ret)
> +			return ret;
> +		num_colors++;
> +	}
> +
> +	if (num_colors == 0) {
> +		ret = lp5812_parse_led_channel(np, cfg, led_index, 0);
> +		if (ret)
> +			return ret;
> +		num_colors = 1;
> +		cfg[led_index].is_sc_led = true;
> +	} else {
> +		cfg[led_index].is_sc_led = false;
> +	}
> +
> +	cfg[led_index].num_colors = num_colors;
> +
> +	return 0;
> +}

...

> +/* Chip specific configurations */
> +static struct lp5812_device_config lp5812_cfg = {

This could be const.

> +	.reg_reset = {
> +		.addr = LP5812_REG_RESET,
> +		.val  = LP5812_RESET
> +	},

CJ