Skip to content

Conversation

@Emil-Juhl
Copy link
Contributor

drivers: led: lp5569: enable auto-increment

The lp5569 controller supports auto-increment for its registers. This is by
default enabled in the chip, but the driver actively disabled it.

Since auto-increment is useful for writing multiple consecutive registers,
enable the feature. The driver, however, doesn't perform such consecutive
writes and thus the existing behavior is not altered.

Signed-off-by: Emil Dahl Juhl [email protected]

drivers: led: lp5569: implement write_channels api

The lp5569 has multiple pwm outputs, and thus implementing the
write_channels api to set multiple values in a single call makes sense.

Implement the write_channels function with a basic range check on the
channel range.
Since the lp5569 supports auto-increment, all of the channels can be
written in one i2c transfer, starting from the pwm register of the
start_channel.

Signed-off-by: Emil Dahl Juhl [email protected]

Copy link

@jonas-rem jonas-rem left a comment

Choose a reason for hiding this comment

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

Could you also adapt the test for the driver so that this feature is used there? Some of the other tests for the led drivers already test the write_channels function.

@Emil-Juhl
Copy link
Contributor Author

Could you also adapt the test for the driver so that this feature is used there?

I haven't been able to find any existing test for the ti,lp5569 except for tests/drivers/build_all/led/src/main.c which does essentially nothing.
Could I ask you to point me to the testcode that you refer to? Thanks!

Some of the other tests for the led drivers already test the write_channels function.

Do you have a reference on that - again I wasn't really able to find anything. Though for this one, the led-api test, tests/drivers/led/led_api/src/test_led_api.c, seems relevant. It doesn't excercise the write_channels api though, but it could be extended to do so I guess. Is that, what you're going for?

@jonas-rem
Copy link

Could you also adapt the test for the driver so that this feature is used there?

I haven't been able to find any existing test for the ti,lp5569 except for tests/drivers/build_all/led/src/main.c which does essentially nothing. Could I ask you to point me to the testcode that you refer to? Thanks!

Some of the other tests for the led drivers already test the write_channels function.

Do you have a reference on that - again I wasn't really able to find anything. Though for this one, the led-api test, tests/drivers/led/led_api/src/test_led_api.c, seems relevant. It doesn't excercise the write_channels api though, but it could be extended to do so I guess. Is that, what you're going for?

Oh, it's not a test but a sample. You can find the sample for the lp5569 here:

samples/drivers/led/lp5569/

And for the other led drivers in the same head folder:

ls samples/drivers/led/
apa102c_bitbang  is31fl3194   is31fl3733  lp3943  lp5562  pca9633  sx1509b_intensity
index.rst        is31fl3216a  led_strip   lp50xx  lp5569  pwm      xec

@zephyrbot zephyrbot added the area: Samples Samples label Nov 25, 2024
@zephyrbot zephyrbot requested review from kartben and nashif November 25, 2024 09:38
@Emil-Juhl
Copy link
Contributor Author

Oh, it's not a test but a sample. You can find the sample for the lp5569 here:

samples/drivers/led/lp5569/

Thank you!
I've added usage of the led_write_channels api in 8365651
If this is not what you had in mind, let me know and we'll work it out :)

@Emil-Juhl Emil-Juhl requested a review from jonas-rem November 25, 2024 09:40
@jonas-rem
Copy link

Oh, it's not a test but a sample. You can find the sample for the lp5569 here:
samples/drivers/led/lp5569/

Thank you! I've added usage of the led_write_channels api in 8365651 If this is not what you had in mind, let me know and we'll work it out :)

Thanks! I think this is sufficient to demonstrate. I have a board with two lp5569 here and will test later today.

Copy link
Contributor

@simonguinot simonguinot left a comment

Choose a reason for hiding this comment

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

Hello @Emil-Juhl,

Thanks for implementing the write_channels() API for the lp5569 driver. It is clear and straightforward.

See below for the changes I am requesting.

Emil Dahl Juhl added 3 commits November 25, 2024 14:59
The lp5569 controller supports auto-increment for its registers. This is by
default enabled in the chip, but the driver actively disabled it.

Since auto-increment is useful for writing multiple consecutive registers,
enable the feature. The driver, however, doesn't perform such consecutive
writes and thus the existing behavior is not altered.

Signed-off-by: Emil Dahl Juhl <[email protected]>
The lp5569 has multiple pwm outputs, and thus implementing the
write_channels api to set multiple values in a single call makes sense.

Implement the write_channels function with a basic range check on the
channel range.
Since the lp5569 supports auto-increment, all of the channels can be
written in one i2c transfer, starting from the pwm register of the
start_channel.

Signed-off-by: Emil Dahl Juhl <[email protected]>
Add a simple demonstration of the led_write_channels api on the lp5569
driver sample.
The demonstration simply turns on all of the channels with a single call to
led_write_channels. Then the same is done for turning off the channels.
Thus, it doesn't add much visually, but it shows the usage of the api.

Signed-off-by: Emil Dahl Juhl <[email protected]>
@Emil-Juhl Emil-Juhl force-pushed the emdj/drivers/led/lp5569/support-write-channels branch from 8365651 to 57fc739 Compare November 25, 2024 13:59
@zephyrbot zephyrbot added the Release Notes To be mentioned in the release notes label Nov 25, 2024
@Emil-Juhl
Copy link
Contributor Author

@simonguinot Thanks for the comments, I've resolved the conversations - but if you don't think I've resolved the issues appropriately, just re-open them and let me know.

Copy link
Contributor

@simonguinot simonguinot left a comment

Choose a reason for hiding this comment

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

Thanks @Emil-Juhl !

@kartben kartben merged commit 2df905d into zephyrproject-rtos:main Nov 25, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: LED Label to identify LED subsystem area: Samples Samples Release Notes To be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants