Re: [PATCH 1/2] leds: Texas Instruments LP5816 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] The driver overwrites its initialized mutex with zeroes via a struct compound literal assignment.
- [High] The `lp5816_brightness_set` function ignores the requested global brightness and writes unscaled relative intensities.
- [High] The LED classdev is registered and sysfs groups are added before hardware startup, creating initialization races.
- [High] The `lp5816_brightness_set()` function fails to latch updated PWM values because it omits the required `UPDATE_CMD`.
- [Medium] `lp5816_brightness_get` incorrectly calculates and returns the maximum relative color intensity instead of the global brightness.
- [Medium] Hardware is reset in `.remove()` while the `devm_`-managed LED classdev is still active, causing a teardown race.
- [Low] The driver hardcodes the LED device name, violating standard Device Tree naming guidelines.
- [Low] The commit message prefix does not follow the standard naming convention for the LED subsystem.
--

--- Patch [1]: [PATCH 1/2] leds: Texas Instruments LP5816 driver ---
commit e41d9f624b4be8a22f82da1e0e301eeb37f92003
Author: Alistair Bell <dev@alistairbell.org>
leds: Texas Instruments LP5816 driver

This patch adds support for the Texas Instruments LP5816 4-channel I2C RGBW
LED driver. It provides independent 4-channel control via the multicolor
sysfs class, as well as configurable fade effects and current limits.

> leds: Texas Instruments LP5816 driver

[Severity: Low]
This isn't a bug, but does this commit message prefix follow the standard
naming convention for the LED subsystem? The guidelines usually mandate a
capitalized format like leds: lp5816: Add support for ...

> diff --git a/drivers/leds/leds-lp5816.c b/drivers/leds/leds-lp5816.c
> new file mode 100644
> index 0000000000000..584c348a7fdb2
> --- /dev/null
> +++ b/drivers/leds/leds-lp5816.c
[ ... ]
> +static enum led_brightness lp5816_brightness_get(struct led_classdev *cdev)
> +{
> +	struct mc_subled *leds;
> +	struct lp5816 *chip;
> +	int intensity;
> +
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	mutex_lock(&chip->mtx);
> +	leds = lcdev_to_mccdev(cdev)->subled_info;
> +	intensity = MAX(leds[0].intensity, leds[1].intensity);
> +	intensity = MAX(intensity, MAX(leds[2].intensity, leds[3].intensity));
> +	mutex_unlock(&chip->mtx);
> +	return intensity & 0xff;
> +}

[Severity: Medium]
Does this return the actual global LED brightness state? It appears to
calculate and return the maximum of the relative sub-LED color weights
instead of returning the overall brightness level.

> +static int lp5816_brightness_set(struct led_classdev *cdev, enum led_brightness br)
> +{
> +	struct lp5816 *chip;
> +	struct led_classdev_mc *mc;
> +	u8 vals[4];
> +
> +	mc = lcdev_to_mccdev(cdev);
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	vals[0] = mc->subled_info[0].intensity & 0xff;
> +	vals[1] = mc->subled_info[1].intensity & 0xff;
> +	vals[2] = mc->subled_info[2].intensity & 0xff;
> +	vals[3] = mc->subled_info[3].intensity & 0xff;
> +
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	mutex_lock(&chip->mtx);
> +	regmap_bulk_write(chip->regmap, REG_OUT0_MANUAL_PWM, vals, ARRAY_SIZE(vals));
> +	mutex_unlock(&chip->mtx);
> +	return 0;
> +}

[Severity: High]
Does lp5816_brightness_set() ignore the requested global brightness
parameter br? It looks like it assigns the unscaled relative intensity
weights directly instead of calling led_mc_calc_color_components() to
compute the correct brightness values.

[Severity: High]
Are these updated PWM values actually applied to the hardware output?
It appears this function writes to the holding registers but does not send
the UPDATE_CMD to REG_UPDATE_CMD to latch the changes, which might leave
the hardware output entirely unchanged.

> +static int lp5816_probe(struct i2c_client *client)
> +{
> +	struct lp5816 *chip;
> +	char *name;
> +	int res;
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +			"failed to allocate lp5816 internal structure\n");
> +
> +	i2c_set_clientdata(client, chip);
> +	mutex_init(&chip->mtx);
> +
> +	(*chip) = (struct lp5816) {
> +		.dev = &client->dev,
> +		.regmap = devm_regmap_init_i2c(client, &lp5816_regmap_config),
> +		.subleds = {

[Severity: Critical]
Does this compound literal assignment overwrite the mutex that was just
initialized? Since the assignment explicitly zeroes out all unlisted fields
in the struct, it seems this will wipe out the mutex state and could lead
to a panic or lockdep warning on the first attempt to acquire chip->mtx.

[ ... ]
> +	if (IS_ERR(chip->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
> +				"failed to allocate register map\n");
> +
> +	name = devm_kasprintf(&client->dev, GFP_KERNEL, "%s:rgbw", dev_name(&client->dev));

[Severity: Low]
Is hardcoding the LED device name to end in :rgbw recommended here?
This might prevent standard userspace tools from correctly identifying the
LED by parsing Device Tree color and function properties per the LED
subsystem rules.

> +	if (!name)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +				"failed to allocate led-classdev name\n");
> +
> +	chip->mcdev = (struct led_classdev_mc){
> +		.led_cdev = (struct led_classdev){
> +			.brightness_set_blocking = lp5816_brightness_set,
> +			.brightness_get = lp5816_brightness_get,
> +			.name = name,
> +		},
> +		.num_colors = ARRAY_SIZE(chip->subleds),
> +		.subled_info = chip->subleds,
> +	};
> +
> +	res = devm_led_classdev_multicolor_register(&client->dev, &chip->mcdev);
> +	if (res < 0)
> +		return dev_err_probe(&client->dev, res, "failed to register classdev multicolor\n");
> +
> +	res = devm_device_add_group(chip->mcdev.led_cdev.dev, &lp5816_group);
> +	if (res < 0)
> +		return dev_err_probe(&client->dev, res, "failed to add led device group\n");
> +
> +	res = lp5816_startup(chip);

[Severity: High]
Could this registration sequence cause initialization races?
By registering the LED classdev and firing uevents before adding the custom
sysfs groups and before the hardware is actually powered on in
lp5816_startup(), udev might miss the custom groups. Additionally, any
immediate userspace writes could be wiped out when lp5816_startup()
subsequently issues a hard reset.

> +	if (res < 0)
> +		return dev_err_probe(&client->dev, res,
> +				"failed to execute chip startup procedure\n");
> +	return 0;
> +}
> +
> +static void lp5816_remove(struct i2c_client *client)
> +{
> +	struct lp5816 *chip;
> +
> +	chip = i2c_get_clientdata(client);
> +	if (!chip)
> +		return;
> +
> +	mutex_lock(&chip->mtx);
> +	regmap_multi_reg_write(chip->regmap, (const struct reg_sequence[]) {
> +		{ .reg = REG_RESET_CMD, .def = RESET_CMD },
> +		{ .reg = REG_UPDATE_CMD, .def = UPDATE_CMD }}, 2);
> +	mutex_unlock(&chip->mtx);
> +}

[Severity: Medium]
Can this manual hardware reset race with the managed teardown?
Because the LED class device is registered via devres, it is still active
when lp5816_remove() resets the chip. The LED core might attempt to set the
brightness to 0 during its unregistration phase later, writing to a reset
and sleeping chip.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712184318.78852-1-dev@alistairbell.org?part=1