drivers: led: extend lp50xx driver#59852
Conversation
man-gc
requested review from
Mani-Sadhasivam,
MaureenHelm,
carlescufi,
galak,
nashif and
simonguinot
as code owners
zephyrbot
added
area: Devicetree Binding
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @man-gc, and thank you very much for your first pull request to the Zephyr project!
A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊
|
@man-gc if there are some commits you took from another PR then you have to keep their original authorship and the author's SOB (Signed-Off-By) as well. In the case you made some modifications, you can append your own SOB (and eventually detail the modifications in the commit description message if that's make sense). If you heavily rewrote an existing commit, then you can take the authorship but you have to mention the original author. Since several people worked on this PR, then it is important to give everyone the credit he deserves. Thanks for taking care :) |
|
Hi @simonguinot , thanks for the quick reply. I've read your remarks concerning the authorship and I will try to do my best to update the commits with credits where credit is due. This will be a little difficult since the two preceding PRs introduced the same modifications and, in practice, I really only used the first commit of #46134 as a base with the review comments applied. So, maybe I should mention @m-janus in this first commit (author, SOB ?). The fifth and sixth commit are similar to some other commits but have been rewritten from scratch by me during development. The idea is the same as the original ones, but does this count as a rewrite or should I mention the original author ? Anyway, I will happily apply any remarks regarding this issue. |
|
OK, I've tried to check what was in the original PRs and should be attributed to other authors and what constitutes my own "original" contributions. While I cannot deny that I've based my work upon the previous PRs as a source of inspiration, some parts were completely rewritten using other drivers as inspiration (ads1x1x for example) and even with comparison to linux driver. So my question is, what is the best way to give credit to the preceding contributions (authorship, SOB, mention) ? |
If you slightly modified the original patch, then you should retain its original authorship+SOB and append your own SOB indicating that your are conveying this patch. If you rewrote significant parts of the original patch, then you can take the authorship. And at the end of the commit description (before your SOB) you can add a mention like:
Or: Or more simply: You can also have a look at the Linux history for more examples. Since there is no rule you don't have to worry about the "form". The important thing is to give credit to the developers who worked on it. |
It's also worth mentioning of |
|
@simonguinot Any news on this PR ? |
Hi @man-gc, |
|
@simonguinot Thanks for the reply. FYI, I have been rebasing this branch before each push but I read afterwards I shouldn't. I hope this will not make it harder to review this PR and I will comply to the Contributor Expectations from now on. |
simonguinot
left a comment
simonguinot
left a comment
There was a problem hiding this comment.
Hi @man-gc,
Thanks again for this PR ! You did a really great job.
Please see my comments and requests below. I'll go further in the review with the next version.
drivers/led/lp50xx.c
Outdated
There was a problem hiding this comment.
We should return -ENOTSUP and not 0 if gpio_enable is not available.
There was a problem hiding this comment.
gpio_enable is not required, so returning 0 seems the right thing to do when it is not present in the devicetree.
There was a problem hiding this comment.
I am not familiar with the PM API but I think there is a misunderstanding here. TURN_ON and TURN_OFF actions are only triggered by a power domain when it is resumed or suspended. So the device/driver can prepare for the power to be removed/added. That's what not we are doing here.
So eventually we should move gpio_enable under the SUSPEND / RESUME action handling and return 0 for TURN_ON / TURN_OFF actions (because there is indeed nothing to do). the What do you think ?
There was a problem hiding this comment.
Maybe I can use the same behavior as lp5569, i.e. use the same action for TURN_ON/RESUME and TURN_OFF/SUSPEND, but configuration will be lost each time if I don't keep a copy of the registers to restore inside lp50xx_data, am I right ?
There was a problem hiding this comment.
No the implementation of the lp5569 driver doesn't look good. See the "Device actions x states" diagram on https://docs.zephyrproject.org/latest/services/pm/device.html. The SUSPEND action is always executed before the TURN_OFF action. So the lp5569 driver runs the same code twice to reach the OFF state... Same goes to reach the ON state. So I believe it is correct to ignore the TURN_ON and TURN_OFF PM action in a LED driver.
With your original implementation, gpio_enable is basically useless because it needs a power domain to go off (which is unlikely to happen).
But if you use gpio_enable when suspending the device, then you are indeed in trouble to restore the registers when resuming. In addition, suspending a device is actually quite different than powering it off...
There is another thing to consider: some apps may not want the LEDs to go off when the device is suspended. An application could even use the LEDs to notify users about the power status. But unfortunately there is no API to disable PM support for a given device/driver...
That's why in #56673, @danieldegrasse choose to implement blanking support through a private API. See the discussion 99bb953#r1250848989. Even if I don't like much the idea, I had nothing better to suggest. Maybe we could do the same here with gpio_enable ?
MeisterBob
left a comment
MeisterBob
left a comment
There was a problem hiding this comment.
I think that all commits in this PR shall be squashed - otherwise we break the bisectability of the samples, they won't build with only one of them applied.
|
Thank you. If I could, I would approve it. |
We used this approach on #46134 and it was not good... So here I was ready to give up on |
@simonguinot @MeisterBob Please be verbose about what you want me to squash together. I don't want to mess this PR by squashing too many commits together. I understand the issue with bisectability but having a big patch is not something desirable too. |
Yes, please squash them. In addition, please squash |
Add support for LP5009, LP5012, LP5018 and LP5024 devices which only differ by the number of LEDs they can control. Also, update application sample to run on all these new supported devices. Based on initial work from: - Marek Janus <marek.janus@grinn-global.com> - Rico Ganahl <rico.ganahl@bytesatwork.ch> Signed-off-by: Mathieu Anquetin <mathieu.anquetin@groupe-cahors.com>
Add nodes for all new devices supported by the updated lp50xx driver to get these enabled as part of the build test. Based on initial work from: - Marek Janus <marek.janus@grinn-global.com> - Rico Ganahl <rico.ganahl@bytesatwork.ch> Signed-off-by: Mathieu Anquetin <mathieu.anquetin@groupe-cahors.com>
Update compatible property to align it to the updated lp50xx driver. Based on initial work from: Marek Janus <marek.janus@grinn-global.com> Signed-off-by: Mathieu Anquetin <mathieu.anquetin@groupe-cahors.com>
The LP50XX family has a specific register to reset the configuration to default state from any other state. Use this instead of relying on the manual configuration of registers during startup. Based on initial work from: Marek Janus <marek.janus@grinn-global.com> Signed-off-by: Mathieu Anquetin <mathieu.anquetin@groupe-cahors.com>
Some boards may have connected the enable pin of the chipset to a GPIO. On these boards, it is necessary to configure and set this GPIO before using the chipset, otherwise the I2C circuitry is disabled. Based on initial work from: - Marek Janus <marek.janus@grinn-global.com> - Rico Ganahl <rico.ganahl@bytesatwork.ch> Signed-off-by: Mathieu Anquetin <mathieu.anquetin@groupe-cahors.com>
Enable device power management using the low-power modes of the LP50XX family. Signed-off-by: Mathieu Anquetin <mathieu.anquetin@groupe-cahors.com>
The led identifer should refer to devicetree ordering, not to the index used by the controller. This way, it will be possible to deactivate some leds or to reorganize the indexing if necessary. Signed-off-by: Mathieu Anquetin <mathieu.anquetin@groupe-cahors.com>
Some boards may have less LED child nodes in the DT than the maximum number of LEDs supported by the chipset. For these boards, the channel test must skip the missing LEDs when preparing the color buffer and not exit in error due to missing info. Also, some boards may have a color mapping that is different from RGB and this mapping should be used when providing the buffer to led_set_color(). Signed-off-by: Mathieu Anquetin <mathieu.anquetin@groupe-cahors.com>
simonguinot
left a comment
simonguinot
left a comment
There was a problem hiding this comment.
Thanks for the excellent work @man-gc !
|
Hi @bbilas. Please review this PR. |
It seems that changes related to the |
|
The The "boards: arm: faze: update for new lp50xx driver" patch is only a cleanup for the "ti,lp503x" which is not used anymore. So git-bisect is OK with the FaZe board too. And I think it is acceptable to have the clean-up change in a dedicated patch. So we are let with "tests: build_all: led: update for new lp50xx driver". And it is true that the |
That sounds fair enough but I would squash |
No no it fixes a bug of mine in the original driver :) It is important to have this one in a separate commit with a proper message giving an explanation. |
bbilas
left a comment
bbilas
left a comment
There was a problem hiding this comment.
@simonguinot @man-gc All right, let's open that damn cold champagne ;)
8 participants
This PR is based on previous work from PR #46134 and #54594 and has been validated on a custom board using TI LP5012 LED controller.