Conversation
github-actions
bot
added
area: API
m-janus
requested review from
Mani-Sadhasivam,
MaureenHelm,
carlescufi,
galak and
nashif
as code owners
|
It might be worth adding that sensor to https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/build_all/led to be tested by the twister. |
github-actions
bot
added
the
area: Tests
m-janus
force-pushed
the
add_lp50xx_driver
branch
5 times, most recently
from
8b60384 to
bc98a4c
Compare
m-janus
force-pushed
the
add_lp50xx_driver
branch
7 times, most recently
from
bac5cd3 to
15831f8
Compare
m-janus
requested review from
Mani-Sadhasivam and
simonguinot
and removed request for
Mani-Sadhasivam
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
|
Hello, |
m-janus
force-pushed
the
add_lp50xx_driver
branch
from
c8c61f4 to
3cae12b
Compare
m-janus
force-pushed
the
add_lp50xx_driver
branch
from
3cae12b to
96b68b9
Compare
This modification extends this driver to handle also LP5009 and LP5012 devices. Align led_lp50xx sample code and faze board to changes. Align led tests to new version of driver. Signed-off-by: Marek Janus <marek.janus@grinn-global.com>
Add device reset at startup. Signed-off-by: Marek Janus <marek.janus@grinn-global.com>
EN pin manages shutdown mode and enables I2C interface. Signed-off-by: Marek Janus <marek.janus@grinn-global.com>
m-janus
force-pushed
the
add_lp50xx_driver
branch
from
96b68b9 to
f432396
Compare
|
Hello @m-janus. It is great to hear from you and thanks for updating this PR. I'll look at it this week-end. |
There was a problem hiding this comment.
Hi @m-janus,
Thanks again for this update. Except some mistakes, I believe the sample is in a decent shape. So to me, it validates that using a single sample for all the controller models is the right way to go. I really think we are close from merging this PR :)
Please see my review below. I'll come back for a final/deeper reading with your next version.
In addition, please have a look at #54594 (review) which came up yesterday. Maybe @dariis and you could share some ideas.
|
|
||
| #define LP5012_LED_BRIGHT_CHAN_BASE 3 | ||
| #define LP5012_LED_BRIGHT_CHAN(led) \ | ||
| (LP5012_LED_BRIGHT_CHAN_BASE + led) |
There was a problem hiding this comment.
Since the brightness base register is the same for LP5009 and LP5012:
#define LP5009_12_LED_BRIGHT_CHAN_BASE 3
#define LP5009_12_LED_BRIGHT_CHAN(led) \
(LP5009_12_LED_BRIGHT_CHAN_BASE + led)
|
|
||
| config LP50XX | ||
| bool "LP50XX LED controller" | ||
| depends on I2C |
There was a problem hiding this comment.
This breaks support on Faze board.
Please see db0a8c6
So this should be:
default y
depends on DT_HAS_TI_LP50XX_ENABLED
select I2C
|
|
||
| #define LED_INFO(led_node_id) \ | ||
| { \ | ||
| .label = DT_LABEL(led_node_id), \ |
There was a problem hiding this comment.
DT_LABEL is deprecated. You should have noticed the warnings when building...
Please don't change the code you don't need to.
|
|
||
| void main(void) | ||
| { | ||
| #if DT_NODE_HAS_COMPAT(DT_NODELABEL(i2c0), ti_lp5030) |
There was a problem hiding this comment.
This breaks support for the FaZe board because LP5030 LEDs are in i2c1 node.
Plus, I don't think you need these #if and #elif. You could use the following code:
const struct device *lp50xx_dev;
lp50xx_dev = DEVICE_DT_GET_ANY(ti_lp5009);
if (lp50xx_dev) {
...
}
lp50xx_dev = DEVICE_DT_GET_ANY(ti_lp5012);
if (lp50xx_dev) {
...
}
...
| LOG_INF("Found LED controller %s", lp5009_dev->name); | ||
| run_test(lp5009_dev, LP5009_MAX_LEDS, LP5009_12_LED_COL1_CHAN(0), | ||
| LP5009_LED_BRIGHT_CHAN_BASE); | ||
| #else |
There was a problem hiding this comment.
You also need to handle ti_lp5012 and ti_lp5036 compatibles.
| LOG_ERR("LED controller for LP50XX device not found"); | ||
| return; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Here is a suggestion for main():
void main(void)
{
const struct device *lp50xx_dev;
bool found = false;
lp50xx_dev = DEVICE_DT_GET_ANY(ti_lp5009);
if (lp50xx_dev) {
LOG_INF("Found LED controller %s", lp50xx_dev->name);
found = true;
if (device_is_ready(lp50xx_dev)) {
run_test(lp50xx_dev, LP5009_MAX_LEDS,
LP5009_12_LED_COL_CHAN_BASE,
LP5009_12_LED_BRIGHT_CHAN_BASE);
} else {
LOG_ERR("LED controller %s is not ready",
lp50xx_dev->name);
}
}
lp50xx_dev = DEVICE_DT_GET_ANY(ti_lp5012);
if (lp50xx_dev) {
...
}
lp50xx_dev = DEVICE_DT_GET_ANY(ti_lp5030);
if (lp50xx_dev) {
...
}
lp50xx_dev = DEVICE_DT_GET_ANY(ti_lp5036);
if (lp50xx_dev) {
...
}
if (!found) {
LOG_ERR("No LP50XX LED controller found");
}
}
Or you could come up with something better :)
|
|
Hi @m-janus. Please let us know what are your intentions with this PR. You are very close, it would be a shame to drop it now after all the efforts you already made. |
|
Hi @simonguinot! |
|
I would like to ask if there are any news on this PR regarding #54594 |
Hi @simonguinot I'm interested, but what does it mean exactly. |
It is really up to you. All the options are on the table. You can continue with #54594, maybe using some ideas from #46134. Or you can take over #46134 and add support of the LP5018 and LP5024 models on top of it. IMHO #46134 is closer from being merged. There are only a few remaining comments to address. In fact, I was also considering taking over. But I'm glad someone else is volunteering. Let me know how you want to proceed. |
|
Superseded by #59852 |
9 participants
This PR extends LP503x driver to work also with LP5009/12 chip. This is why some files need to be renamed.