drivers: led: lp596x controller by mrrosen-cpi · Pull Request #105246 · zephyrproject-rtos/zephyr

mrrosen-cpi · 2026-03-11T01:49:32Z

Initial implementation of TI LP586x family of LED Matrix drivers. Include devices LP5860 and LP6866 over I2C.

github-actions · 2026-03-11T01:50:17Z

Hello @mrrosen-cpi, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

drivers/led/lp586x.c

dsseng · 2026-03-11T10:11:37Z

drivers/led/lp586x.c

+		LOG_ERR("read dev_config3 failed");
+		return ret;
+	}
+	val = (val & 0xF1) | current_val;


Subjective readability nit, feel free to disregard or implement differently

Suggested change

val = (val & 0xF1) | current_val;

val = (val & 0xF1) | current_val;

Also, is this mask maybe worth moving to a named define?

Cleaned up; and did make the mask a macro

dsseng · 2026-03-11T10:15:13Z

drivers/led/lp586x.c

+		return ret;
+	}
+
+	uint8_t current_val = (config->max_current & 0x7) << 1;


Not sure if this definition should be moved to the top (i.e. start of the scope). I cannot remember where to find the rule so please do correct me if I'm wrong

Ive seen other parts of the Zephyr code define local variables mid-function and I cant find anywhere in the style guide where it asks for them all to be defined at the top of the function either. But, after I did a minor cleanup on this part of the code, I ended up doing that anyway

This is a common practice followed by the Linux kernel. The Zephyr LED code follow it too. Please, move this variable declaration at the top of the function.

pdgendt

Splitting the compatibles as is done now might be a bit overkill, but non-blocking.

Please add nodes to tests/drivers/build_all/led/app.overlay so they are built in CI.

pdgendt · 2026-03-12T07:34:27Z

drivers/led/lp586x.c

+#define LP586X_DEVICE(n, id)                                                                       \
+	static const struct lp586x_config lp##id##_config_##n = {                                  \
+		.i2c = I2C_DT_SPEC_INST_GET(n),                                                    \
+		.enable = GPIO_DT_SPEC_INST_GET_OR(n, enable_gpios, {0}),                          \
+		.data_mode = DT_INST_PROP_OR(n, data_mode, 1) - 1,                                 \
+		.num_lines = DT_INST_PROP(n, num_lines),                                           \
+		.num_channels = DT_INST_PROP(n, num_channels),                                     \
+		.channels_per_led = DT_INST_PROP(n, channels_per_led),                             \
+		.max_current = LP586X_MAX_CURRENT_UA_CONVERT(DT_INST_PROP(n, max_current_ua)),     \
+	};                                                                                         \
+                                                                                                   \
+	DEVICE_DT_INST_DEFINE(n, lp586x_led_init, NULL, NULL, &lp##id##_config_##n, POST_KERNEL,   \
+			      CONFIG_LED_INIT_PRIORITY, &lp586x_led_api);


Other than the name id does not appear to be used? You could opt to make a single lp586x compatible.

Most other LED drivers follow this pattern from what I saw if there are any differences in the family. In this case, the number of lines is family-dependent (up to 11 or up to 6, others in the family have different limits); so it somewhat depends on whether we want to have the build system enforce checks to make sure developers setup their devicetrees correctly, or just let the binding allow the maximum the family allows and if the devicetree is wrong for the particular part on their board, then it will silently compile and only fail if an application attempts to drive an LED that "doesnt exist." I dont disagree that the differences are very minor (thus the combined driver), and if there is a way to have a "ti,lp586x" compatible that can also enforce the line limits, that would be fine (I dont recall if something like "ti,lp5860", "ti,lp586x" as multiple compatibles would work, its not super pretty either but a common pattern). Just thought this was the cleanest and most consistent but if we dont like this pattern, we could make a single compatible.

mrrosen-cpi · 2026-03-13T16:56:41Z

I am a bit confused about the compliance check failing since it seems to point to a file this PR doesn't touch?

pdgendt · 2026-03-13T17:03:04Z

I am a bit confused about the compliance check failing since it seems to point to a file this PR doesn't touch?

I'm assuming this was fixed in #105389, please rebase.

kartben · 2026-03-20T20:38:17Z

dts/bindings/led/ti,lp5860.yaml

+      - 10
+      - 11
+    description: |
+      Number of lines connected to the device.


default value needs to be justified
https://docs.zephyrproject.org/latest/build/dts/bindings-upstream.html#rules-for-default-values

comment applies to the whole PR

Apologies, forgot to do that. All of them now should be addressed

Copilot

Pull request overview

Initial addition of a Zephyr LED controller driver and devicetree bindings for the TI LP586x LED matrix family (LP5860/LP5866) over I2C, plus enabling it in build-all LED tests.

Changes:

Add LP586x driver implementation (drivers/led/lp586x.c) and hook it into Kconfig/CMake.
Add devicetree bindings for LP586x common + per-part (LP5860/LP5866).
Extend the LED build-all test overlay with LP5860/LP5866 nodes.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/drivers/build_all/led/app.overlay	Adds LP5860/LP5866 test nodes on the build-all I2C bus.
dts/bindings/led/ti,lp586x.yaml	Common LP586x binding properties (I2C + LED controller).
dts/bindings/led/ti,lp5866.yaml	LP5866-compatible binding including LP586x common properties.
dts/bindings/led/ti,lp5860.yaml	LP5860-compatible binding including LP586x common properties.
drivers/led/lp586x.c	New LP586x LED driver (init + brightness/color setting).
drivers/led/Kconfig.lp586x	New Kconfig option to enable LP586x driver.
drivers/led/Kconfig	Sources the new LP586x Kconfig fragment.
drivers/led/CMakeLists.txt	Builds lp586x.c when CONFIG_LP586X is enabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

drivers/led/lp586x.c

tests/drivers/build_all/led/app.overlay

drivers/led/lp586x.c

dts/bindings/led/ti,lp586x.yaml

drivers/led/lp586x.c

mrrosen-cpi · 2026-03-28T02:09:11Z

There seems to be a conflict between clang-format and the compliance check for line 232. I tried to fix it in the cleanest way possible but unsure if its alright from style perspective.

Initial implementation of TI LP586x family of LED Matrix drivers. Include devices LP5860 and LP6866 over I2C. Signed-off-by: Michael Rosen <michael.rosen@chargepoint.com>

Add LP586x family devices (LP5860 and LP866) to build_all led test. Signed-off-by: Michael Rosen <michael.rosen@chargepoint.com>

sonarqubecloud · 2026-03-28T21:43:53Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

pdgendt · 2026-03-30T06:41:51Z

There seems to be a conflict between clang-format and the compliance check for line 232. I tried to fix it in the cleanest way possible but unsure if its alright from style perspective.

clang-format isn't enforced in CI.

zephyrbot added platform: TI SimpleLink Texas Instruments SimpleLink MCU area: LED Label to identify LED subsystem area: Devicetree Binding PR modifies or adds a Device Tree binding labels Mar 11, 2026

zephyrbot requested review from JarmouniA, bogdanovs, jeppenodgaard, jpanisbl, simonguinot and vaishnavachath March 11, 2026 01:51

zephyrbot assigned simonguinot Mar 11, 2026

pdgendt requested changes Mar 11, 2026

View reviewed changes

drivers/led/lp586x.c Outdated Show resolved Hide resolved

drivers/led/lp586x.c Outdated Show resolved Hide resolved

drivers/led/lp586x.c Show resolved Hide resolved

dsseng reviewed Mar 11, 2026

View reviewed changes

mrrosen-cpi force-pushed the mrrosen/lp586x branch from e9e769d to 41c7f79 Compare March 12, 2026 00:56

pdgendt reviewed Mar 12, 2026

View reviewed changes

mrrosen-cpi force-pushed the mrrosen/lp586x branch from 41c7f79 to d958054 Compare March 12, 2026 16:11

zephyrbot added the area: Tests Issues related to a particular existing or missing test label Mar 12, 2026

zephyrbot requested a review from nashif March 12, 2026 16:13

pdgendt previously approved these changes Mar 12, 2026

View reviewed changes

mrrosen-cpi force-pushed the mrrosen/lp586x branch from d958054 to 8925e1a Compare March 13, 2026 23:31

kartben requested changes Mar 20, 2026

View reviewed changes

kartben requested a review from Copilot March 20, 2026 20:39

Copilot started reviewing on behalf of kartben March 20, 2026 20:40 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

mrrosen-cpi dismissed pdgendt’s stale review via e5bd52e March 22, 2026 07:07

mrrosen-cpi force-pushed the mrrosen/lp586x branch 2 times, most recently from e5bd52e to 13e1fc4 Compare March 28, 2026 02:05

mrrosen-cpi added 2 commits March 28, 2026 14:11

drivers: led: lp596x controller

f2636ec

Initial implementation of TI LP586x family of LED Matrix drivers. Include devices LP5860 and LP6866 over I2C. Signed-off-by: Michael Rosen <michael.rosen@chargepoint.com>

tests: drivers: build_all: led: add lp5860 and lp5866 devices

7d41f90

Add LP586x family devices (LP5860 and LP866) to build_all led test. Signed-off-by: Michael Rosen <michael.rosen@chargepoint.com>

mrrosen-cpi force-pushed the mrrosen/lp586x branch from 13e1fc4 to 7d41f90 Compare March 28, 2026 21:12

	val = (val & 0xF1) \| current_val;

	val = (val & 0xF1) \| current_val;

Conversation

mrrosen-cpi commented Mar 11, 2026

Uh oh!

github-actions bot commented Mar 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdgendt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrrosen-cpi commented Mar 13, 2026

Uh oh!

pdgendt commented Mar 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrrosen-cpi commented Mar 28, 2026

Uh oh!

sonarqubecloud bot commented Mar 28, 2026

Quality Gate passed

Uh oh!

pdgendt commented Mar 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants