Skip to content

Extend LP503x driver to support LP50xx series#54594

Closed
dariis wants to merge 5 commits intozephyrproject-rtos:mainfrom
bytesatwork:drivers-led-lp50xx
Closed

Extend LP503x driver to support LP50xx series#54594
dariis wants to merge 5 commits intozephyrproject-rtos:mainfrom
bytesatwork:drivers-led-lp50xx

Conversation

@dariis
Copy link
Contributor

@dariis dariis commented Feb 8, 2023

Tested on custom board with LP5018 connected to Arduino uno connector on stm32h747i_disco.

@zephyrbot zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding platform: TI Texas Instruments area: LED Label to identify LED subsystem labels Feb 8, 2023
Rename to use driver for different lp50xx led controllers.

Signed-off-by: Rico Ganahl <rico.ganahl@bytesatwork.ch>
Prepare defines for LP50xx series.
Adapt define in lp503x sample.

Signed-off-by: Rico Ganahl <rico.ganahl@bytesatwork.ch>
Add base yml for defining LP50xx.

Signed-off-by: Rico Ganahl <rico.ganahl@bytesatwork.ch>
Add support for LP5009, LP5012, LP5018 and LP5024.
Changed from original driver to use last set brightness when turning
LED on and set enable gpio in init function.
Introduce feature to blink with software timer. Needs to be enabled
in Kconfig.
New optional DT property defalt-brightness to set the initial brightness
in percent. Additional optional DT property norm-value to normalize
brightness of the RGB LED. The value represents the sum of the RGB color
channels which corresponds to 100% brightness.
For perfect color representation the brightness normalization
should be disabled and handled by a so called color map (or a LUT)
introduced by the app.
Feel free to implement a more sophisticated brightness adaptation
like gamma correction.

Chip feature "LED Bank Control" is not supported.

Signed-off-by: Rico Ganahl <rico.ganahl@bytesatwork.ch>
Sample usage of LP5018 I2C LED controller.

Signed-off-by: Rico Ganahl <rico.ganahl@bytesatwork.ch>
@dariis dariis force-pushed the drivers-led-lp50xx branch from 37be5d2 to 45b6101 Compare February 8, 2023 09:48
@simonguinot
Copy link
Contributor

Hi @dariis.

Thanks for this PR.

We currently already have #46134 under review. It adds support for the LP50{09,12} series. So you'll have to rebase your work on the top of it once merged.


endif # DT_HAS_TI_LP5018_ENABLED

if LP50XX_ALLOW_SOFTWARE_BLINK
Copy link
Contributor

@simonguinot simonguinot Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit should be split into several (and eventually into several PRs): one or more to add controller series support and one commit per feature you want to add (like software blinking).

About software blinking... It is not something we want to implement at the driver level. We want end-driver to expose hardware capabilities through the LED API. If a controller can blink its LEDs, then led_api_blink() should be implemented. If not, then it should be let empty. Software blinking should be considered as a fallback if hardware support is missing. This way, generic software blinking can be implemented in LED core and shared between all the LED drivers.

Please have a look at #50511. The same arguments about color correction have been developed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some thoughts:

This commit should be split into several ...

Okay, let's wait for progress in #46134

About software blinking...

Are there any plans to do a LED core with software features which are not supported by the driver(s)?
Otherwise we could let the user decide via Kconfig as in this PR. What is your opinion regarding Kconfig?

Copy link
Contributor

@simonguinot simonguinot Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dariis. My preference is to have software LED effects implemented in some LED core code and used as fall-backs when there is no hardware support available (i.e. when the end-driver API function is missing). I am not working on it yet but it is in my todo list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to clarify:
To get it merged I have to remove the software blink part, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a start, yes. Eventually you also have to split the main commit (if possible). If you want to continue with this PR then I'll start the review process at the next iteration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dariis. My preference is to have software LED effects implemented in some LED core code and used as fall-backs when there is no hardware support available (i.e. when the end-driver API function is missing). I am not working on it yet but it is in my todo list.

Hi is there any progress on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dariis. My preference is to have software LED effects implemented in some LED core code and used as fall-backs when there is no hardware support available (i.e. when the end-driver API function is missing). I am not working on it yet but it is in my todo list.

Hi is there any progress on this?

No, it is still on my to-do list.

@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label Apr 10, 2023
@dariis
Copy link
Contributor Author

dariis commented Apr 12, 2023

@m-janus Are you still working on the #46134 PR?

@github-actions github-actions bot removed the Stale label Apr 13, 2023
@cfriedt cfriedt removed their request for review April 28, 2023 13:25
@dariis dariis mentioned this pull request Jun 15, 2023
@dariis
Copy link
Contributor Author

dariis commented Aug 4, 2023

close due to #59852

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Devicetree Binding PR modifies or adds a Device Tree binding area: LED Label to identify LED subsystem platform: TI Texas Instruments

5 participants