-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add support for MAX17616 #2734
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
base: main
Are you sure you want to change the base?
Add support for MAX17616 #2734
Conversation
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
5513eaa to
94940e7
Compare
|
V2: Rebase to latest main |
3988f35 to
a021cb3
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
a021cb3 to
206c239
Compare
|
V3: Resolve identified Sphinx documentation errors |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
206c239 to
cbda533
Compare
|
V4: Remove diff headers included in max17616.h |
b1687b3 to
39f0f5a
Compare
|
/AzurePipelines run |
|
Commenter does not have sufficient privileges for PR 2734 in repo analogdevicesinc/no-OS |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
kseerp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, minor comments on my end.
drivers/power/max17616/max17616.c
Outdated
| if (ret == 0) | ||
| *value = max17616_direct_to_int(raw_value, | ||
| &max17616_vin_coeffs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error handling be done first, and only do the value conversion if the read was successful
ret = max17616_read_word(dev, MAX17616_CMD(MAX17616_READ_VIN),
&raw_value);
if (ret)
return ret;
*value = max17616_direct_to_int(raw_value,
&max17616_vin_coeffs);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
drivers/power/max17616/max17616.c
Outdated
| case MAX17616_POWER: | ||
| /* Calculate power from voltage and current */ | ||
| { | ||
| int vin, iout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable declarations should be moved to top of function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
drivers/power/max17616/max17616.c
Outdated
| if (ret == 0) | ||
| *value = vin * iout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error handling be done first before processing success case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
drivers/power/max17616/max17616.c
Outdated
| int ret = max17616_read_byte(dev,MAX17616_CMD(MAX17616_OPERATION), | ||
| &operation); | ||
|
|
||
| if (ret == 0) | ||
| *enabled = (operation & 0x80) ? true : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
drivers/power/max17616/max17616.c
Outdated
|
|
||
| /* Extract bits 7:6 and convert to enum */ | ||
| switch (raw_value & 0xC0) { | ||
| case 0x00: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using macros in switch cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
drivers/power/max17616/max17616.c
Outdated
|
|
||
| /* Extract bits 3:0 and convert to enum */ | ||
| switch (raw_value & 0x0F) { | ||
| case 0x00: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
drivers/power/max17616/max17616.h
Outdated
| MAX17616_STATUS_BIT_GENERAL = 0, // Not supported | ||
| MAX17616_STATUS_BIT_CML = 1, // Comms, memory, or logic Fault | ||
| MAX17616_STATUS_BIT_TEMPERATURE = 2, // Temperature Fault or Warning | ||
| MAX17616_STATUS_BIT_VIN_UV = 3, // Input Under Voltage Fault | ||
| MAX17616_STATUS_BIT_IOUT_OC = 4, // Output Over Current Fault | ||
| MAX17616_STATUS_BIT_VOUT_OV = 5, // An output overvoltage fault occurred | ||
| MAX17616_STATUS_BIT_OFF = 6, // Max17616 is not enabled | ||
| MAX17616_STATUS_BIT_BUSY = 7 // Not supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for explicit numbering since enum values are already in correct order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
drivers/power/max17616/max17616.h
Outdated
| MAX17616_STATUS_BIT_STARTUP = 0, // Not supported | ||
| MAX17616_STATUS_BIT_OTHER = 1, // Not supported | ||
| MAX17616_STATUS_BIT_FANS = 2, // Not supported | ||
| MAX17616_STATUS_BIT_POWER_GOOD = 3, // Not supported | ||
| MAX17616_STATUS_BIT_MFR = 4, // Manufacturer specific Fault | ||
| MAX17616_STATUS_BIT_INPUT = 5, // Input V, I, or P Fault | ||
| MAX17616_STATUS_BIT_IOUT_POUT = 6, // Output current or power Fault | ||
| MAX17616_STATUS_BIT_VOUT = 7 // Output voltage Fault |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
jemfgeronimo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes. Also, use the new example infrastructure like I mentioned here.
drivers/power/max17616/max17616.c
Outdated
| /* Step 1: Verify manufacturer ID */ | ||
| ret = max17616_verify_manufacturer_id(dev); | ||
| if (ret) | ||
| return ret; | ||
|
|
||
| /* Step 2: Identify chip variant */ | ||
| ret = max17616_identify_chip_variant(dev); | ||
| if (ret) | ||
| return ret; | ||
|
|
||
| /* Step 3: Verify PMBus revision */ | ||
| ret = max17616_verify_pmbus_revision(dev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments seem redundant since the function names are self-explanatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
| /* Initialize telemetry structure */ | ||
| memset(telemetry, 0, sizeof(struct max17616_telemetry)); | ||
|
|
||
| /* Read VIN using DIRECT format */ | ||
| ret = max17616_read_value(dev, MAX17616_VIN, &telemetry->vin); | ||
| if (ret == 0) | ||
| telemetry->valid_mask |= NO_OS_BIT(0); | ||
|
|
||
| /* Read VOUT using DIRECT format */ | ||
| ret = max17616_read_value(dev, MAX17616_VOUT, &telemetry->vout); | ||
| if (ret == 0) | ||
| telemetry->valid_mask |= NO_OS_BIT(1); | ||
|
|
||
| /* Read IOUT using DIRECT format */ | ||
| ret = max17616_read_value(dev, MAX17616_IOUT, &telemetry->iout); | ||
| if (ret == 0) | ||
| telemetry->valid_mask |= NO_OS_BIT(3); | ||
|
|
||
| /* Read Temperature using DIRECT format */ | ||
| ret = max17616_read_value(dev, MAX17616_TEMP, &telemetry->temp1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are redundant; the function calls already describe what’s happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
|
|
||
| int ret = max17616_read_byte(dev,MAX17616_CMD(MAX17616_OPERATION), | ||
| &operation); | ||
| int ret = max17616_read_byte(dev, MAX17616_CMD(MAX17616_OPERATION), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include max17616.c and max17616.h changes to the previous commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
| int value; | ||
| int ret; | ||
|
|
||
| /* Parse the input value */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
| case 0: { /* operation */ | ||
| bool enabled; | ||
| ret = max17616_get_operation_state(iio_max17616->max17616_dev, &enabled); | ||
| if (ret) | ||
| return ret; | ||
| return snprintf(buf, len, "%s", enabled ? "enabled" : "disabled"); | ||
| } | ||
|
|
||
| case 2: /* device_info */ | ||
| return snprintf(buf, len, "MAX17616/MAX17616A Protection IC"); | ||
|
|
||
| case 3: { /* fault_summary */ | ||
| struct max17616_status status; | ||
| ret = max17616_read_status(iio_max17616->max17616_dev, &status); | ||
| if (ret) | ||
| return ret; | ||
|
|
||
| if (status.word == 0) { | ||
| return snprintf(buf, len, "No faults"); | ||
| } | ||
|
|
||
| char fault_info[256] = {0}; | ||
| int pos = 0; | ||
|
|
||
| if (status.vout) pos += snprintf(fault_info + pos, sizeof(fault_info) - pos, | ||
| "VOUT_FAULT "); | ||
| if (status.iout) pos += snprintf(fault_info + pos, sizeof(fault_info) - pos, | ||
| "IOUT_FAULT "); | ||
| if (status.input) pos += snprintf(fault_info + pos, sizeof(fault_info) - pos, | ||
| "VIN_FAULT "); | ||
| if (status.temperature) pos += snprintf(fault_info + pos, | ||
| sizeof(fault_info) - pos, "TEMP_FAULT "); | ||
| if (status.cml) pos += snprintf(fault_info + pos, sizeof(fault_info) - pos, | ||
| "CML_FAULT "); | ||
| if (status.mfr_specific) pos += snprintf(fault_info + pos, | ||
| sizeof(fault_info) - pos, "MFR_FAULT "); | ||
|
|
||
| return snprintf(buf, len, "%s", fault_info); | ||
| } | ||
| case 4: { /* capability */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid magic numbers. Replace them with enum values for case labels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
| case 0: { /* operation */ | ||
| int enable = 0; | ||
| if (!strncmp(buf, "1", 1) || !strncasecmp(buf, "enable", 6)) { | ||
| enable = 1; | ||
| } | ||
| return max17616_set_operation_state(iio_max17616->max17616_dev, enable); | ||
| } | ||
|
|
||
| case 1: /* clear_faults */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto: magic numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
39f0f5a to
d74bdb5
Compare
|
V5:
|
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
d74bdb5 to
6c0d961
Compare
|
V6:
|
drivers/power/max17616/max17616.c
Outdated
| struct max17616_dev *dev; | ||
| int ret = -EINVAL; | ||
|
|
||
| dev = (struct max17616_dev *)no_os_calloc(0, sizeof(struct max17616_dev)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
| dev = (struct max17616_dev *)no_os_calloc(0, sizeof(struct max17616_dev)); | |
| dev = (struct max17616_dev *)no_os_calloc(1, sizeof(struct max17616_dev)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
| { | ||
| .name = "raw", | ||
| .show = max17616_iio_read_attr, | ||
| }, | ||
| { | ||
| .name = "scale", | ||
| .show = max17616_iio_read_attr, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing the same max17616_iio_read_attr function used to access both raw and scale, yet no priv to differentiate between them. Typically, raw represents the unscaled register value, while scale provides the conversion factor used to compute the physical quantity (raw * scale + offset). If I understand correctly, in this case, reading the raw and scale attributes would both return the same raw data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it returns the same data. For this case, with specs to use DIRECT method of transforming data, returned values are in V, I, or degC units already (similar cases are devices LTC4162L and LT8491 where the read values are in engineering units already and the scale factor used is 1). I'll use the .priv field and scale factor 1 for IIO compliance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Addressed in latest commit.
drivers/power/max17616/README.rst
Outdated
| }; | ||
| struct max17616_init_param max17616_ip = { | ||
| .i2c_init = &_i2c_ip, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .i2c_init = &_i2c_ip, | |
| .i2c_init = &i2c_ip, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
| INCS += \ | ||
| $(PLATFORM_DRIVERS)/maxim_gpio.h \ | ||
| $(PLATFORM_DRIVERS)/maxim_gpio_irq.h \ | ||
| $(PLATFORM_DRIVERS)/maxim_irq.h \ | ||
| $(PLATFORM_DRIVERS)/../common/maxim_dma.h \ | ||
| $(PLATFORM_DRIVERS)/maxim_i2c.h \ | ||
| $(PLATFORM_DRIVERS)/maxim_uart.h \ | ||
| $(PLATFORM_DRIVERS)/maxim_uart_stdio.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Fix indentions. Use tabs instead of spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
projects/max17616/README.rst
Outdated
| # Select the example you want to enable by choosing y for enabling and n for disabling | ||
| BASIC_EXAMPLE = n | ||
| IIO__EXAMPLE = y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| IIO__EXAMPLE = y | |
| IIO_EXAMPLE = y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed in latest commit.
6c0d961 to
696bd2b
Compare
|
V7:
|
Some of these changes were reverted from the latest push. |
Add header and source files for max17616 driver. Signed-off-by: Carlos Jones <[email protected]>
Signed-off-by: Carlos Jones <[email protected]>
Add README.rst documentation file for max17616 alongside other documentation related files. Signed-off-by: Carlos Jones <[email protected]>
Add project files for both basic and IIO examples for max17616. Signed-off-by: Carlos Jones <[email protected]>
Add README.rst documentation file for project alongside other documentation related files. Signed-off-by: Carlos Jones <[email protected]>
Add unit test files for the max17616 driver. Signed-off-by: Carlos Jones <[email protected]>
696bd2b to
c418df5
Compare
|
V8:
|
|
LGTM |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
LGTM |
Pull Request Description
The MAX17616/MAX17616A offers highly versatile and programmable protection boundaries for systems against input voltage faults and output overcurrent faults. Input-voltage faults (with positive polarity) are protected up to +80V (without Reverse Current Protection)/+75V (with Reverse Current Protection), by an internal nFET featuring low ON-resistance (20mΩ typ). The devices feature a programmable undervoltage-lockout (UVLO) thresholds by using external voltage- dividers. The MAX17616 features a programmable overvoltage-lockout (OVLO) while MAX17616A offers a programmable output voltage clamp function through the OVFB pin that features an output voltage limiting regulation during input transient surge events. Input undervoltage and overvoltage protection (MAX17616)/output voltage clamp function (MAX17616A) can be programmed across the entire 3V to 80V operating range.
PR Type
PR Checklist