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

hwmon: (pmbus/adp1050): Support adp1051 and adp1055 #2629

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

hwmon: (pmbus/adp1050): Support adp1051 and adp1055 #2629

wants to merge 2 commits into from
+135 −14

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)

@actorreno actorreno force-pushed the dev/adp1050_support branch 6 times, most recently from 48bf3ba to 2e1673e Compare October 25, 2024 02:16
@actorreno
dt-bindings: hwmon: pmbus: adp1051 and adp1055: add bindings.
9f4b738 
Add dt-bindings for adp1051 and adp1055 pmbus.
ADP1051: 6 PWM for I/O Voltage, I/O Current, Temperature
ADP1055: 6 PWM for I/O Voltage, I/O Current, Power, Temperature

Signed-off-by: Alexis Torreno <alexisczezar.torreno@analog.com>
@actorreno
hwmon: pmbus : adp1051 and adp1055: Add driver support
ffc9249 
ADP1051: 6 PWM for I/O Voltage, I/O Current, Temperature
ADP1055: 6 PWM for I/O Voltage, I/O Current, Power, Temperature

Signed-off-by: Alexis Torreno <alexisczezar.torreno@analog.com>
nunojsa
nunojsa reviewed Oct 25, 2024
View reviewed changes
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.

Another hint is to run:

git log --oneline drivers/hwmon/pmbus to look for the style used in that dir for commit titles. The most common seems to be (adapted for your case):

hwmon: (pmbus/adp1050): Support adp1051 and adp1055

Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
@@ -10,16 +10,35 @@ maintainers:
- Radu Sabau <radu.sabau@analog.com>

description: |
The ADP1050 is used to monitor system voltages, currents and temperatures.
The ADP1050, ADP1051, and ADP1055 are used to monitor system voltages,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not scale. Instead say "The ADP1050 and similar devices..."

Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
The ADP1050 is used to monitor system voltages, currents and temperatures.
The ADP1050, ADP1051, and ADP1055 are used to monitor system voltages,
currents, power and temperatures.

Through the PMBus interface, the ADP1050 targets isolated power supplies
and has four individual monitors for input/output voltage, input current
and temperature.
Datasheet:
https://www.analog.com/en/products/adp1050.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just add here the links to new datasheets

Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml

hwmon@70 {
compatible = "adi,adp1050";
compatible = "adi,adp1050", "adi,adp1051";
Copy link
Collaborator

Choose a reason for hiding this comment

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

It does not work like this... If you can fallback to some compatible, that needs to be stated in the bindings which is not.

Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
reg = <0x70>;
vcc-supply = <&vcc>;
status = "okay";
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the status

Documentation/devicetree/bindings/hwmon/pmbus/adi,adp1050.yaml
compatible = "adi,adp1055";
reg = <0x4b>;
vcc-supply = <&vcc>;
status = "okay";
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it#s only a matter of the address being different, I would not bother in adding another example

drivers/hwmon/pmbus/adp1050.c
| PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
| PMBUS_HAVE_IIN | PMBUS_HAVE_TEMP
| PMBUS_HAVE_STATUS_TEMP,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also unrelated change. I would drop it (or add it in a different patch)

drivers/hwmon/pmbus/adp1050.c
ADP1050,
ADP1051,
ADP1055,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drop the id

drivers/hwmon/pmbus/adp1050.c
{ .compatible = "adi,adp1050"},
{ .compatible = "adi,adp1050", .data = (void *)ADP1050},
{ .compatible = "adi,adp1051", .data = (void *)ADP1051},
{ .compatible = "adi,adp1055", .data = (void *)ADP1055},
Copy link
Collaborator

Choose a reason for hiding this comment

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

pass in directly the pmbus_driver_info structs...

drivers/hwmon/pmbus/adp1050.c
{"adp1050", 0},
{"adp1050", ADP1050},
{"adp1051", ADP1051},
{"adp1055", ADP1055},
Copy link
Collaborator

Choose a reason for hiding this comment

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

here you need a cast but there are examples:

https://elixir.bootlin.com/linux/v6.11.5/source/drivers/iio/addac/ad74413r.c#L1530

drivers/hwmon/pmbus/adp1050.c
return pmbus_do_probe(client, &adp1050_info);
enum ADP1050_type type;

type = (enum ADP1050_type)(uintptr_t)device_get_match_data(&client->dev);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now you can properly do error checking:

info = device_get_match_data();
if (!info)
    return -ENODEV;

@actorreno actorreno changed the title Added support for ADP1051 and ADP1055 on existing ADP1050 driver hwmon: (pmbus/adp1050): Support adp1051 and adp1055 Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nunojsa nunojsa nunojsa left review comments

@mhennerich mhennerich Awaiting requested review from mhennerich mhennerich is a code owner

@dbogdan dbogdan Awaiting requested review from dbogdan dbogdan is a code owner

@btogorean btogorean Awaiting requested review from btogorean btogorean is a code owner

@chegbeli chegbeli Awaiting requested review from chegbeli chegbeli is a code owner

@buha buha Awaiting requested review from buha buha is a code owner

@amiclaus amiclaus Awaiting requested review from amiclaus amiclaus is a code owner

@rbolboac rbolboac Awaiting requested review from rbolboac rbolboac is a code owner

@danmois danmois Awaiting requested review from danmois danmois is a code owner

@CiprianRegus CiprianRegus Awaiting requested review from CiprianRegus CiprianRegus is a code owner

@machschmitt machschmitt Awaiting requested review from machschmitt machschmitt is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@actorreno @nunojsa