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.

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

Add support for ADP5055 #2668

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add support for ADP5055 #2668

wants to merge 3 commits into from

Conversation

actorreno
Copy link

PR Description

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have tested the changes on the relevant hardware
  • I have updated the documentation outside this repo accordingly (if there is the case)

@nunojsa
Copy link
Collaborator

nunojsa commented Dec 6, 2024

Please, fix first the valid CI errors you have...

@actorreno actorreno force-pushed the dev/adp5055-regulator branch 9 times, most recently from 5d77901 to 36d5ba8 Compare December 9, 2024 02:38
@actorreno
Copy link
Author

actorreno commented Dec 9, 2024

It seems the checks returns some errors I couldn't capture when locally running commit check scripts like "./scripts/checkpatch <driver_file>"

I have fixed them except 1 last error which I am confused about since the error is not related to my files.
Screenshot below.

image

Edit: was fixed by rebasing to latest main

Add documentation for devicetree bindings for ADP5055.

Signed-off-by: Alexis Czezar Torreno <[email protected]>
Add ADI ADP5055 triple buck regulator driver support.

Signed-off-by: Alexis Czezar Torreno <[email protected]>
imply REGULATOR_ADP5055.

Signed-off-by: Alexis Czezar Torreno <[email protected]>
@actorreno actorreno force-pushed the dev/adp5055-regulator branch from 36d5ba8 to 03486f9 Compare December 18, 2024 04:25
Copy link
Contributor

@kister-jimenez kister-jimenez left a comment

Choose a reason for hiding this comment

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

ADP5055 is PMBUS compatible, should this be under hwmon/pmbus?

@actorreno
Copy link
Author

The datasheet did say "pmbus compatible" but it didn't have any pmbus commands the other typical pmbus dataseet contain
it also doesn't have an eeprom for the usual "pages" I see in other pmbus codes.

some colleagues agreed with me that it was just an i2c interface and decided it is better off in the regulator subsystem

@nunojsa
Copy link
Collaborator

nunojsa commented Jan 20, 2025

ADP5055 is PMBUS compatible, should this be under hwmon/pmbus?

Hmm, I do think this is a pertinent question... Looking at the description:

"The ADP5055 is a power management unit that combines three high performance buck regulators with a PMBus interface..."

So this looks like a typical power management device that fits in HWMON/PMBUs. Note that the pmbus subsystem also has an interface with the regulator subsystem and it will also check what standard PMbus registers are supported by the device. On top of that, you can pretty much bypass the standard registers and control the device as you want. Example, I see the VIDx_LOW/HIGH registers that could fit in the standard HWMON ABI and you totally support it even not using standard pmbus commands. There's also power good and overcurrent stuff that could very likely be made to work.

All the above to say that pmbus might be doable and allows you to better support this device (meaning, more features). You can also internally reach the apps engineer for this part and try to understand the actual PMBUS commands supported. Looking at the datasheet, I can see STATUS_CML to be supported.

Copy link
Collaborator

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

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

I'm still not convinced on the location of the driver but i'm already doing some comments on the DT bindings

0 - Physical EN pin is used for enabling.
1 - Internal Register is used for enabling.
2 - Both register and physical pin needed.
3 - Either register or physical pin.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks custom implementation of a subset of the ON_OFF_CONFIG command. Moreover, DT maintainers do not like much of properties like this where you just give the register code. You should have a string property and do the mapping in the driver (for register values). On top of that, you should likely control 3 gpios that should be mandatory for options 0 and 3.

Enables or disables over current protection
blanking (OCP) for all channels.
0 = Disabled
1 = Enabled
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for the 0/1 description. Should encode the state in the property name. Simply adi,ocp-blanking and if the property is enabled, then it's pretty clear we're enabling the feature.

items:
minimum: 0
maximum: 3
default: 3, 3, 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have a lot properties for the channels so I think it might be a good idea to create channel child nodes and have the properties per channel instead of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants