drivers: led: Introduce LP5521 driver #73194

AyushKot96 · 2024-05-23T06:20:19Z

Description

This PR adds a new common driver for controlling LP5521, supported by CX1825 (in-tree hardware) and LP5562

Changes

Introduce LP55XX LED Driver to control LP5521 and LP5562.
Remove old LP5562 driver files.
Add LP5521 support to existing croxel_cx1825.

Testing

Testing has been done using samples/drivers/led_lp5521 application on croxel_cx1825 board.

west build -b croxel_cx1825/nrf52840 samples/drivers/led_lp5521

Due to unavailability of LP5562, its testing is currently pending.

ubieda

First pass review

samples/drivers/led_lp5521/sample.yaml

samples/drivers/led_lp5521/README.rst

samples/drivers/led_lp5521/src/main.c

dts/bindings/led/ti,lp5521.yaml

drivers/led/lp5521.c

RickBruyninckx · 2024-06-10T14:39:18Z

@simonguinot while we're at it, can't we just make a lp55xx driver covering all of them? (I will look Monday how much the lp55xx family shares functionality)

Thanks @RickBruyninckx. Let me know !

I glanced at the drivers made for linux and documented at TI and they have a seperate driver for each of the leddrivers but with a "leds-lp55xx-common.h" file for the generic lp55xx stuff and a common devicetree binding:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml

We can do something similar right?

ubieda · 2024-06-10T16:22:22Z

Friendly reminder: Feature Freeze for the next Release is this upcoming friday. Do you believe this PR can be targetted to get in by then, @AyushKot96 @simonguinot?

@ubieda I think it's unlikely. For now my opinion is that some of the lp55xx drivers should be merged. So if I'm correct, the lp5521 driver cannot be merged into its current form. And again I won't rush it.

There's a clear upside in refactoring these drivers into common code, as it makes it easier to maintain and catch bugs. As pointed out by @RickBruyninckx, this has been somewhat done on Linux so it's possible.

However, I disagree with this being a requirement for merging this PR, for the following reasons:

First and foremost: This feature is important for us (Croxel) as it provides upstream support for a feature on our board (https://docs.zephyrproject.org/latest/boards/croxel/croxel_cx1825/doc/index.html) and we would highly benefit from it making it to the release.
The review process of a PR introducting a new driver and the refactoring of existing functional drivers into common code are very different from each other.
This suggested refactoring is not implemented for the existing LP55XX instances (https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/led/lp5562.c and https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/led/lp5569.c) so this PR is not breaking a pattern, nor ignoring an already filed issue. Because of this: this refactoring efforts naturally falls into a GH enhancement issue (which we can happily capture before merging this PR).

Long-story short: the benefits for getting this feature for 3.7.0 far outweigh the cost of introducing it as a separate driver now and refactoring it later.

@simonguinot @RickBruyninckx please, reconsider your stance on this.

ubieda · 2024-06-10T16:28:46Z

@AyushKot96 please fix CI failures. Rememeber you can run these locally:

twister -p croxel_cx1825/nrf52840 -T samples

ubieda · 2024-06-10T16:39:16Z

Because of this: this refactoring efforts naturally falls into a GH enhancement issue (which we can happily capture before merging this PR).

Issue captured: #74027

simonguinot · 2024-06-10T23:27:46Z

@simonguinot @RickBruyninckx please, reconsider your stance on this.

@ubieda I understand this feature is important for your company.

But here is my problem. I can see a lot of similarities between the lp5521 and lp5562 drivers. And I am opposed to the addition of a new driver duplicating code with an existing one if the latter can be extended. So I am trying to evaluate the cost of a such extension here and I am asking @AyushKot96 about it. I think it is a legitimate request which should have been anticipated considering that the code of the lp5521 driver has been copied from lp5562. Please note that I already raised this point two weeks ago and here is the answer I got:

Both IC's are similar but many register addresses are different. LP5562 driver wont work with LP5521.

About the registers addresses it is not accurate. I can see that most of them are identical. And I already know that the lp5562 driver won't work with the LP5521 controller in its current state. But that didn't answer my question...

@AyushKot96 is that possible to add support for the LP5521 controller to the lp5562 driver ? have you considered this option ? And why did choose to write a separate driver ?

AyushKot96 · 2024-06-11T06:24:23Z

@simonguinot @RickBruyninckx please, reconsider your stance on this.

@ubieda I understand this feature is important for your company.

But here is my problem. I can see a lot of similarities between the lp5521 and lp5562 drivers. And I am opposed to the addition of a new driver duplicating code with an existing one if the latter can be extended. So I am trying to evaluate the cost of a such extension here and I am asking @AyushKot96 about it. I think it is a legitimate request which should have been anticipated considering that the code of the lp5521 driver has been copied from lp5562. Please note that I already raised this point two weeks ago and here is the answer I got:

Both IC's are similar but many register addresses are different. LP5562 driver wont work with LP5521.

About the registers addresses it is not accurate. I can see that most of them are identical. And I already know that the lp5562 driver won't work with the LP5521 controller in its current state. But that didn't answer my question...

@AyushKot96 is that possible to add support for the LP5521 controller to the lp5562 driver ? have you considered this option ? And why did choose to write a separate driver ?

@simonguinot Upon revisiting the drivers, I came to know that I could have refactor them and make a common driver. I am prepared to address this in a separate pull request.

bbilas · 2024-06-11T09:34:01Z

Hi @AyushKot96,

Thanks for contributing this driver !

I started my review but the more code I read, the more I find it very similar to the lp5562 driver. So I stop here and I reiterate my initial question: what would prevent to have a common driver for the lp5521 and lp5562 chips ? I understand that there are slight differences with some registers but they can be abstracted. Please comment.

I agree with Simon. Please create a generic lp55xx driver that will support both variants. The current solution is unacceptable to me as there is no need to have another driver which was mostly copied from the other one.

kartben · 2024-06-11T09:51:50Z

@simonguinot Upon revisiting the drivers, I came to know that I could have refactor them and make a common driver. I am prepared to address this in a separate pull request.

Hi @AyushKot96 - I think it's fair that the maintainer of a particular area would not want to accept new code that's duplicated if it's clear that there is a better path. That's how technical debt can quickly add up in an open source project as the promised follow-up PR may or may not ever show up (and I am by no means implying that you, in particular, won't follow up, but you get the idea).

@ubieda

However, I disagree with this being a requirement for merging this PR, for the following reasons:

First and foremost: This feature is important for us (Croxel) as it provides upstream support for a feature on our board (docs.zephyrproject.org/latest/boards/croxel/croxel_cx1825/doc/index.html) and we would highly benefit from it making it to the release.

Of course! On the other hand it's also worth pointing out it's a not unsignificant amount of code that was submitted 3-ish weeks ahead of feature freeze :) And the feedback re: trying to identify common code was mentioned very early in the review process.

The review process of a PR introducting a new driver and the refactoring of existing functional drivers into common code are very different from each other.
This suggested refactoring is not implemented for the existing LP55XX instances (main/drivers/led/lp5562.c and main/drivers/led/lp5569.c) so this PR is not breaking a pattern, nor ignoring an already filed issue. Because of this: this refactoring efforts naturally falls into a GH enhancement issue (which we can happily capture before merging this PR).

It seems to me as the lp5569 driver is vastly different from the existing LP5562, where the LP5512 one is effectively copy-pasting a lot of existing code?

ubieda · 2024-06-11T14:50:20Z

@simonguinot @kartben Understood. We'll refactor the driver. Thanks!
CC: @AyushKot96

AyushKot96 · 2024-06-13T14:58:21Z

@simonguinot @ubieda Hey guys, I have pushed a new common driver for LP5521 and LP5562.
Please let me know your thoughts and feel free to suggest any changes you might have. Also, I have only tested this with the LP5521, as I do not have an LP5562 available at the moment.

- Add common driver for LP5521 and LP5562 - Remove LP5562 driver source and KConfig files - Add sample application for LP5521 Signed-off-by: Ayush Kothari <ayush@croxel.com>

-Add LP5521 node in croxel_cx1825 board dts file -Remove arduino_i2c and arduino_gpio from dts file to prevent shield sample build test Signed-off-by: Ayush Kothari <ayush@croxel.com>

ubieda · 2024-08-08T14:49:55Z

boards/croxel/croxel_cx1825/croxel_cx1825_nrf52840.dts

 };

-arduino_i2c: &i2c0 {
+&i2c0 {


Needs rebasing. This has been updated already: #74893

ubieda · 2024-08-08T14:56:35Z

@simonguinot Seems like the refactoring has been done. Would you mind taking another look at this? Thanks

simonguinot · 2024-08-08T16:51:53Z

@simonguinot Seems like the refactoring has been done. Would you mind taking another look at this? Thanks

Hello @ubieda,

I'll look at it next week.

Note that I already reviewed #72713 from @AyushKot96 last month but I didn't get any answer since... That's why I was hesitant to start the review for this PR.

ubieda · 2024-08-08T16:53:36Z

@simonguinot Seems like the refactoring has been done. Would you mind taking another look at this? Thanks

Hello @ubieda,

I'll look at it next week.

Note that I already reviewed #72713 from @AyushKot96 last month but I didn't get any answer since... That's why I was hesitant to start the review for this PR.

Thanks! We'll come back to that one soon (it's in our backlog). CC: @cx-anuj-pathak

simonguinot

Hi @AyushKot96,

You need to help me a little here. I can't review a such big commit with everything in it, including renamed files. You need to structure the pull request with several commits. For example:

rename the lp5562 driver and sample into lp55xx. So I'll be able to use git to visualize the changes coming in the next commits.
make the driver and sample generic to prepare for adding support of the LP5521 controller. Don't be afraid to split this step into several commits as well.
add support for the LP5521 controller to the driver.
add support for the LP5521 controller to the sample.

Thanks in advance !

cx-anuj-pathak · 2024-08-22T04:44:11Z

@simonguinot we have rearranged commits. but it went upto 14 commits, and in the process we have identified few other things too and a bug in lp5562 driver. So we have decided internally to break it down in 3-4 PR's.

add en gpio dts property and handling, PR filed Driver: led: lp5562: Added en_gpio DTS property and Implemented PM Support #77375
fix bug in the way blink is handled, if a engine is already linked, it is not reused in subsequent call to blink, instead a new IDLE engine is found and linked. Thus if 3 colors are in blink state, we can not change blink period without led_on/off call. Will file PR soon. PR filed Drivers: led: lp5562: re-use already linked engine #77408
Simplify and Refactor LP5562 code in generic format and add support for LP5521.

Let me know if you have any suggestions?
We'll close this PR

simonguinot · 2024-09-04T13:00:20Z

Hi @cx-anuj-pathak. Yes it seems fine to use several PRs !

biii-anuj-pathak · 2024-11-24T09:09:14Z

FIled a new PR #78968

AyushKot96 force-pushed the feature-add-lp5521-led-driver branch 9 times, most recently from e41727d to 6901f20 Compare May 24, 2024 07:20

ubieda self-requested a review May 24, 2024 11:17

ubieda changed the title ~~driver: led: Add driver for LP5521~~ drivers: led: Introduce LP5521 driver May 24, 2024

ubieda requested changes May 24, 2024

View reviewed changes

AyushKot96 force-pushed the feature-add-lp5521-led-driver branch 4 times, most recently from 7594d5b to 83eaa40 Compare May 27, 2024 13:00

AyushKot96 marked this pull request as ready for review May 27, 2024 14:34

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: LED Label to identify LED subsystem platform: TI SimpleLink Texas Instruments SimpleLink MCU area: Samples Samples labels May 27, 2024

zephyrbot requested review from bbilas, decsny, galak, kartben, Mani-Sadhasivam, nashif, simonguinot, vaishnavachath and vanti May 27, 2024 14:35

ubieda mentioned this pull request Jun 10, 2024

drivers: led: Refactor LP55XX drivers into common code #74027

Open

AyushKot96 force-pushed the feature-add-lp5521-led-driver branch from 3e34e30 to 01f3be2 Compare June 11, 2024 10:07

AyushKot96 force-pushed the feature-add-lp5521-led-driver branch from 01f3be2 to 281a4b7 Compare June 13, 2024 14:48

AyushKot96 requested review from simonguinot and ubieda June 13, 2024 14:58

AyushKot96 force-pushed the feature-add-lp5521-led-driver branch 3 times, most recently from ad819fe to 6917f69 Compare June 14, 2024 06:25

AyushKot96 added 2 commits June 15, 2024 18:28

driver: led: Add common LP55XX driver

8fdc6c1

- Add common driver for LP5521 and LP5562 - Remove LP5562 driver source and KConfig files - Add sample application for LP5521 Signed-off-by: Ayush Kothari <ayush@croxel.com>

boards: croxel: croxel_cx1825: Add LP5521 ic node

954a45f

-Add LP5521 node in croxel_cx1825 board dts file -Remove arduino_i2c and arduino_gpio from dts file to prevent shield sample build test Signed-off-by: Ayush Kothari <ayush@croxel.com>

AyushKot96 force-pushed the feature-add-lp5521-led-driver branch from 6917f69 to 954a45f Compare June 15, 2024 13:01

ubieda reviewed Aug 8, 2024

View reviewed changes

simonguinot requested changes Aug 13, 2024

View reviewed changes

ubieda closed this Sep 4, 2024

drivers: led: Introduce LP5521 driver #73194

drivers: led: Introduce LP5521 driver #73194

Uh oh!

Conversation

AyushKot96 commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Testing

Uh oh!

ubieda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RickBruyninckx commented Jun 10, 2024

Uh oh!

ubieda commented Jun 10, 2024

Uh oh!

ubieda commented Jun 10, 2024

Uh oh!

ubieda commented Jun 10, 2024

Uh oh!

simonguinot commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AyushKot96 commented Jun 11, 2024

Uh oh!

bbilas commented Jun 11, 2024

Uh oh!

kartben commented Jun 11, 2024

Uh oh!

ubieda commented Jun 11, 2024

Uh oh!

AyushKot96 commented Jun 13, 2024

Uh oh!

ubieda Aug 8, 2024

Choose a reason for hiding this comment

Uh oh!

ubieda commented Aug 8, 2024

Uh oh!

simonguinot commented Aug 8, 2024

Uh oh!

ubieda commented Aug 8, 2024

Uh oh!

simonguinot left a comment

Choose a reason for hiding this comment

Uh oh!

cx-anuj-pathak commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonguinot commented Sep 4, 2024

Uh oh!

biii-anuj-pathak commented Nov 24, 2024

Uh oh!

Uh oh!

AyushKot96 commented May 23, 2024 •

edited

Loading

simonguinot commented Jun 10, 2024 •

edited

Loading

cx-anuj-pathak commented Aug 22, 2024 •

edited

Loading