Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

drivers: regulator: add support for AXP2101 power management IC #82474

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

drivers: regulator: add support for AXP2101 power management IC #82474

wants to merge 1 commit into from
+562 −114

Conversation

lfelten
Copy link
Contributor

@lfelten lfelten commented Dec 3, 2024

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

gmarull
gmarull reviewed Dec 5, 2024
View reviewed changes
dts/bindings/regulator/x-powers,axp2101-regulator.yaml Outdated
Comment on lines 13 to 17
pmic@68 {
reg = <0x68>;
...
regulators {
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.

tests/drivers/build_all/regulator/i2c.dtsi
@@ -76,6 +76,30 @@ axp192@4 {
};
};

axp2101@7 {
compatible = "x-powers,axp2101";
Copy link
Member

Choose a reason for hiding this comment

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

where's this?

drivers/regulator/regulator_axp2101.c Outdated

ret = i2c_reg_update_byte_dt(&config->i2c, config->desc->enable_reg,
config->desc->enable_mask, 0u);
if (ret != 0) {
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

drivers/regulator/regulator_axp2101.c Outdated
const struct regulator_axp2101_desc *desc;
const struct i2c_dt_spec i2c;

LOG_INSTANCE_PTR_DECLARE(log);
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
Collaborator

kartben commented Dec 12, 2024 •
edited
Loading

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 •
edited
Loading

@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.

soburi
soburi requested changes Dec 16, 2024
View reviewed changes
drivers/regulator/regulator_axp2101.c Outdated Show resolved Hide resolved
@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 soburi and gmarull December 18, 2024 07:18
soburi
soburi previously approved these changes Dec 19, 2024
View reviewed changes
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
View reviewed changes
@lfelten
Copy link
Contributor Author

lfelten commented Dec 20, 2024

x-powers,axp2101-regulator.yaml updated.

soburi
soburi requested changes Dec 21, 2024
View reviewed changes
drivers/regulator/regulator_axp192.c Outdated Show resolved Hide resolved
drivers/regulator/regulator_axp192.c Outdated Show resolved Hide resolved
drivers/regulator/regulator_axp192.c Outdated Show resolved Hide resolved
drivers/regulator/regulator_axp192.c Outdated Show resolved Hide resolved
drivers/regulator/regulator_axp192.c Outdated Show resolved Hide resolved
@lfelten lfelten force-pushed the xpowers_axp2101 branch 4 times, most recently from 89e610a to bbee302 Compare December 23, 2024 10:06
@lfelten
Copy link
Contributor Author

lfelten commented Dec 23, 2024 •
edited
Loading

Fixed m5stack boards m5stack_core2 and m5stickc_plus defconfigs.
Test kernel.multiprocessing.smp.minimallibc fails on board m5stack_core2/esp32/procpu
I don't think that is related to this driver, might be related to #56011

The current fails are related to SMP issues. I will rebase the driver when they are fixed.

@lfelten
Copy link
Contributor Author

lfelten commented Dec 25, 2024

Rebased to main, the SMP related build failure was fixed (thanks @sylvioalves !)
Now there is a crypto related issue.

@lfelten lfelten force-pushed the xpowers_axp2101 branch 3 times, most recently from 416a2d9 to 7908746 Compare January 3, 2025 19:02
@lfelten lfelten mentioned this pull request Jan 10, 2025
Closed
@lfelten
Copy link
Contributor Author

lfelten commented Jan 11, 2025 •
edited
Loading

I will rebase when #83813 is merged

@lfelten lfelten requested review from soburi, gmarull and kartben January 14, 2025 08:59
@lfelten
Copy link
Contributor Author

lfelten commented Jan 14, 2025

Rebased to main now that #82813 was merged

@lfelten lfelten mentioned this pull request Jan 14, 2025
Draft
@soburi
Copy link
Member

soburi commented Jan 15, 2025

@lfelten

From what I've tried, there seem to be some general issues with using ranges, and the values ​​set in the registers seem strange. Have you checked this?

@lfelten
Copy link
Contributor Author

lfelten commented Jan 15, 2025 •
edited
Loading

@lfelten

From what I've tried, there seem to be some general issues with using ranges, and the values ​​set in the registers seem strange. Have you checked this?

I have checked some of the LDOs on my board using a multimeter. So the settings do set the correct voltage on the LDOs I tested (within sane ranges for this board. Need to check which ones, it has been a few weeks). I don't know why that should be ranges, though.

@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 •
edited
Loading

@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

@lfelten
drivers: regulator: add support for AXP2101 power management IC
c19a915 
    add initial support for the AXP2101 power management IC from X-powers

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

Signed-off-by: Lothar Felten <[email protected]>
@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 approved these changes Jan 26, 2025
View reviewed changes
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

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

@soburi soburi soburi approved these changes

@aasinclair aasinclair Awaiting requested review from aasinclair

@danieldegrasse danieldegrasse Awaiting requested review from danieldegrasse

@gmarull gmarull Awaiting requested review from gmarull

@kartben kartben Awaiting requested review from kartben

Requested changes must be addressed to merge this pull request.

Assignees

@gmarull gmarull

Labels
area: MFD area: Regulators
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lfelten @kartben @soburi @gmarull @zephyrbot