Re: [PATCH 2/2] dt-bindings: leds: Add binding for ti,lp5816

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] The driver overwrites the initialized mutex with zeroes during a compound struct assignment, causing an inevitable crash on first use.
- [High] The driver ignores the master brightness parameter and fails to use the multi-color calculation helper.
- [High] The driver resets the hardware in `remove()` before the LED is unregistered, causing a use-after-disable window.
- [High] Missing register update command in brightness setter prevents hardware from applying new values.
- [Medium] The commit introduces custom, device-specific sysfs attributes (`fade` and `fade_mode`) that hardcode specific RGBW colors, bypassing standard LED subsystem mechanisms.
- [Medium] The device tree binding fails to define standard LED child nodes and omits common standard LED properties, breaking hardware description principles.
- [Medium] The device is exposed to user-space before its hardware is initialized.
- [Low] The device tree binding description explicitly references the operating system's software driver.
- [Low] Use of undeclared identifier `MAX` causes a compilation error.
- [Low] The patch commit message violates the LED subsystem naming convention.
--

--- Patch [2]: [PATCH 2/2] dt-bindings: leds: Add binding for ti,lp5816 ---
commit b91ef8e4c561a1f06d4c4b39a848e64275caccbb
Author: Alistair Bell <[email protected]>

dt-bindings: leds: Add binding for ti,lp5816

Add devicetree binding for Texas Instruments LP5816
4-channel I2C LED device.

> dt-bindings: leds: Add binding for ti,lp5816

[Severity: Low]
Does this commit message subject violate the LED subsystem naming convention?
The subsystem guidelines expect the prefix to follow the format:
leds: <Driver>: <Capitalized description>.

> diff --git a/Documentation/ABI/testing/sysfs-class-leds-lp5816 b/Documentation/ABI/testing/sysfs-class-leds-lp5816
> new file mode 100644
> index 0000000000000..2d1fd107abf67
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-leds-lp5816
> @@ -0,0 +1,123 @@
> +What: /sys/class/leds/<device>/fade
> +Date: July 2026
> +KernelVersion: 7.2
> +Contact: <[email protected]>
> +Description:
> +	Enables/disables hardware fading of each individual LED and sets transition duration
> +	between an old intensity and new intensity
> +
> +	Format: <enable_red> <enable_green> <enable_blue> <enable_white> <duration>

[Severity: Medium]
Does introducing these custom, device-specific sysfs attributes that hardcode
specific RGBW colors bypass the standard LED subsystem mechanisms?
This forces user-space tools to implement custom string parsing for this
specific chip.

> diff --git a/Documentation/devicetree/bindings/leds/ti,lp5816.yaml b/Documentation/devicetree/bindings/leds/ti,lp5816.yaml
> new file mode 100644
> index 0000000000000..d33489c306dfc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/ti,lp5816.yaml
[ ... ]
> +properties:
> +  compatible:
> +    const: ti,lp5816
> +    description: |
> +      Specifies device as LP5816 and the uses that kernel driver

[Severity: Low]
Does this description improperly reference the operating system's software
driver?
Device tree bindings should be OS-agnostic hardware descriptions.

[ ... ]
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false

[Severity: Medium]
Does this schema fail to define standard LED child nodes?
By dropping child node specifications and omitting common standard LED
properties, a device tree attempting to define standard LED properties
like color or function will fail validation.

> 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);

[Severity: Low]
Will this result in a compilation error?
The uppercase MAX macro is undeclared; the standard kernel macro for maximum
values is lowercase max().

[ ... ]
> +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;

[Severity: High]
Does this ignore the master brightness parameter?
The core functionality of setting the LED brightness appears broken because
the driver does not use led_mc_calc_color_components(mc, br) and ignores the
requested master brightness, making it impossible to turn off the LED via the
standard API.

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

[Severity: High]
Are we missing the register update command here?
Without issuing a REG_UPDATE_CMD after the bulk write, the brightness changes
requested via the standard API will fail to take effect on the hardware.

> +	mutex_unlock(&chip->mtx);
> +	return 0;
> +}
[ ... ]
> +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) {

[Severity: High]
Will this compound struct assignment overwrite the initialized mutex?
Standard C semantics zero-initialize omitted members in a compound literal,
which will wipe the internal state of chip->mtx and cause a kernel oops or
deadlock on the first mutex_lock() call.

> +		.dev = &client->dev,
> +		.regmap = devm_regmap_init_i2c(client, &lp5816_regmap_config),
> +		.subleds = {
> +			{ .color_index = LED_COLOR_ID_RED, .intensity = 0 },
> +			{ .color_index = LED_COLOR_ID_GREEN, .intensity = 0 },
> +			{ .color_index = LED_COLOR_ID_BLUE, .intensity = 0 },
> +			{ .color_index = LED_COLOR_ID_WHITE, .intensity = 0 },
> +		},
> +	};
[ ... ]
> +	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: Medium]
Is it safe to expose the device to user-space before its hardware is initialized?
If udev or user-space accesses the device after devm_led_classdev_multicolor_register()
but before lp5816_startup() finishes, the hardware reset in the startup sequence
could destroy that state.

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

[Severity: High]
Does resetting the hardware in remove() before the LED is unregistered cause a
use-after-disable window?

Because the LED class device is registered using a devres API, it will be
unregistered automatically after remove() returns. That automatic cleanup calls
led_set_brightness() to turn off the LED, which will write I2C commands to a
device that has already been reset.

> +	mutex_unlock(&chip->mtx);
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2