Skip to content

Conversation

@lfelten
Copy link
Contributor

@lfelten lfelten commented Dec 3, 2024

Add initial support for the AXP2101 power management IC from X-powers

Comment on lines 13 to 17
Copy link
Member

Choose a reason for hiding this comment

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

you are missing a top-level compatible here, this is a MFD device. It's ok to just add support for regulators now, but DT layout needs to be designed according to how device is.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (ret != 0) {
if (ret < 0) {

others as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @gmarull
thanks, I changed the if statements

Copy link
Member

Choose a reason for hiding this comment

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

Is instance level logging really needed? Very few drivers do this, and, in most cases, I'd say there's only a single regulator instance on a system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @gmarull,
I remove the per instance logging.

@lfelten lfelten marked this pull request as draft December 10, 2024 09:15
@lfelten lfelten force-pushed the xpowers_axp2101 branch 2 times, most recently from f02b2fc to 24296fd Compare December 12, 2024 21:32
@kartben
Copy link
Contributor

kartben commented Dec 12, 2024

This is a duplicate of #78098? You may want to avoid duplicating effort @lfelten

@lfelten
Copy link
Contributor Author

lfelten commented Dec 12, 2024

This is a duplicate of #78098?

Indeed, sorry, didn't check before submitting this one.

@lfelten lfelten marked this pull request as ready for review December 13, 2024 13:03
@lfelten
Copy link
Contributor Author

lfelten commented Dec 16, 2024

@kartben @gmarull @soburi @ranranff
I compared the drivers, they have similar functionality except that ranranff added the work mode setting.
Should I add the workmode setting here or fix ranranffs driver?

@soburi
Copy link
Member

soburi commented Dec 16, 2024

@lfelten

Your and Ranranff's codes are obviously similar because they are copied from regulator_axp192.c. Since the implementation is almost the same, and only the definition is different, I think it is preferable to support it as an extension of regulator_axp192.c.

I tried making something based on your code.

soburi@6c74c9d
(And one previous commit.)

I have yet to test it, but I hope it helps.

@lfelten
Copy link
Contributor Author

lfelten commented Dec 16, 2024

@soburi
Thanks!
I have a patch that adds the sets the workmode for this driver (#82474).
I can test it on one board, but that only uses some DC/DC and LDOs.

@lfelten

Your and Ranranff's codes are obviously similar because they are copied from regulator_axp192.c. Since the implementation is almost the same, and only the definition is different, I think it is preferable to support it as an extension of regulator_axp192.c.

I tried making something based on your code.

soburi@6c74c9d (And one previous commit.)

I have yet to test it, but I hope it helps.

@lfelten
Copy link
Contributor Author

lfelten commented Dec 17, 2024

@soburi
The logging was still per instance. @gmarull asked to remove this as usually there is only one PMIC on a board/design.
The tests on my board were successful.

@lfelten lfelten requested review from gmarull and soburi December 18, 2024 07:18
soburi
soburi previously approved these changes Dec 19, 2024
Copy link
Member

@soburi soburi left a comment

Choose a reason for hiding this comment

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

LGTM.

It would help if you still addressed pointed by @gmarull #82474 (comment).

@lfelten
Copy link
Contributor Author

lfelten commented Dec 19, 2024

LGTM.

It would help if you still addressed pointed by @gmarull #82474 (comment).

Thanks!
Sorry, that got lost while merging. I added a new version (this is the only change).

soburi
soburi previously approved these changes Dec 19, 2024
@lfelten lfelten force-pushed the xpowers_axp2101 branch 2 times, most recently from 30eba8f to c7553b6 Compare January 23, 2025 09:21
@lfelten
Copy link
Contributor Author

lfelten commented Jan 23, 2025

@soburi I still have no hardware that uses a DC/DC besides DCDC1, sorry.
Here is a summary of the changes:

  • converted all ranges to unsigned decimal notation
  • corrected ranges for dcdc2, dcdc3, dcdc4
  • corrected vsel masks for dcdc2, dcdc3, dcdc4, dcdc5
  • consolidated ranges for ald1/2/3/4 and bldo1/2 into abldox (range is the same)
  • rebased to main

@soburi
Copy link
Member

soburi commented Jan 26, 2025

@lfelten

I still have no hardware that uses a DC/DC besides DCDC1, sorry.

Please note this in the commit message about this.

soburi
soburi previously approved these changes Jan 26, 2025
Copy link
Member

@soburi soburi left a comment

Choose a reason for hiding this comment

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

LGTM

@soburi
Copy link
Member

soburi commented Jan 27, 2025

@gmarull
Could you take a look again?

@lfelten
Copy link
Contributor Author

lfelten commented Jan 31, 2025

@soburi I added the remark to the git commit log as you requested and rebased it to main.
@gmarull can you please review?

@lfelten lfelten force-pushed the xpowers_axp2101 branch 4 times, most recently from a5e6394 to fcf787c Compare February 3, 2025 22:13
@lfelten lfelten requested review from gmarull and soburi February 4, 2025 06:27

DT_INST_FOREACH_STATUS_OKAY(MFD_AXP192_DEFINE);
DT_FOREACH_STATUS_OKAY_VARGS(x_powers_axp192, MFD_AXP192_DEFINE, 192);
DT_FOREACH_STATUS_OKAY_VARGS(x_powers_axp2101, MFD_AXP192_DEFINE, 2101);
Copy link
Member

Choose a reason for hiding this comment

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

rename definition macro to make it clear that covers 2 pmic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope I understood it right: I renamed MFD_AXP192_DEFINE to MFD_AXP192__AXP2101_DEFINE to indicate it applies to axp192 and axp2101.

#include <zephyr/logging/log_instance.h>
#include <zephyr/drivers/mfd/axp192.h>

LOG_MODULE_REGISTER(regulator_axp192, CONFIG_REGULATOR_LOG_LEVEL);
Copy link
Member

Choose a reason for hiding this comment

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

why moving this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, moved it back, sorry.

@lfelten lfelten force-pushed the xpowers_axp2101 branch 2 times, most recently from 7bf4fd4 to f211b83 Compare February 17, 2025 21:25
Add initial support for the AXP2101 power management IC from X-powers.
Remark: only DC/DC1 and ALDO have been tested on real hardware.

Co-authored-by: TOKITA Hiroshi <[email protected]>
Co-authored-by: Gerard Marull-Paretas <[email protected]>

Signed-off-by: Lothar Felten <[email protected]>
@lfelten
Copy link
Contributor Author

lfelten commented Feb 18, 2025

Rebased to current main branch
@gmarull: your requested changes were added.
I also fixed the register defines (#ifdef... clause) for AXP2101

@lfelten lfelten requested a review from gmarull February 18, 2025 08:45
@fabiobaltieri fabiobaltieri added this to the v4.2.0 milestone Feb 27, 2025
@carlescufi carlescufi merged commit 1c50c3a into zephyrproject-rtos:main Mar 7, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants