drivers: sensor: npm13xx_charger: improve sample fetching #94546
Conversation
zephyrbot
requested review from
MaureenHelm,
aasinclair,
anangl,
avisconti,
jaz1-nordic,
jeppenodgaard,
kl-cruz,
magp-nordic,
masz-nordic,
nika-nordic,
teburd,
tristan-google,
ubieda and
yperess
seov-nordic
force-pushed
the
npm13xx-charger-improve-measurement-fetching
branch
from
72ae685 to
7ea2956
Compare
|
| }; | ||
|
|
||
| return i2c_write_dt(&config->i2c, buff, sizeof(buff)); | ||
| return i2c_transfer_dt(&config->i2c, msg, 2); |
There was a problem hiding this comment.
Nitpick:
| return i2c_transfer_dt(&config->i2c, msg, 2); | |
| return i2c_transfer_dt(&config->i2c, msg, ARRAY_SIZE(msg)); |
|
@anangl or @masz-nordic, could you please take a look? |
|
Reassigned, see Line 2204 in 2d72d86 |
|
Retriggered CI. |
|
@seov-nordic This PR needs to be rebased to solve the CI failures. |
Change the npm13xx_charger fetch function to first trigger a sample and then block until the result is available. Signed-off-by: Sergei Ovchinnikov <[email protected]>
seov-nordic
force-pushed
the
npm13xx-charger-improve-measurement-fetching
branch
from
7ea2956 to
8b6d2ec
Compare
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves the sample fetching mechanism in the npm13xx_charger driver by changing the approach from triggering measurements individually to batching them together and waiting for completion. The key change is to trigger all ADC measurements at once and then block until the results are available, making the sampling process more efficient.
- Replaced sequential ADC triggering with burst triggering for all measurements
- Added proper timing synchronization with a calculated wait period
- Updated the MFD driver to support burst writes instead of individual register writes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| include/zephyr/drivers/mfd/npm13xx.h | Updated function signature and documentation for burst write functionality |
| drivers/sensor/nordic/npm13xx_charger/npm13xx_charger.c | Restructured sample fetching to batch ADC triggers and wait for completion |
| drivers/mfd/mfd_npm13xx.c | Implemented burst write function using I2C message transfer |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ret = mfd_npm13xx_reg_write_burst(config->mfd, ADC_BASE, ADC_OFFSET_TASK_VBAT, | ||
| (uint8_t []){1U, 1U, 1U}, 3U); |
There was a problem hiding this comment.
[nitpick] The compound literal array should be assigned to a named constant for better readability and maintainability. Consider defining a constant like static const uint8_t adc_trigger_pattern[] = {1U, 1U, 1U};
Copilot uses AI. Check for mistakes.
| int ret = mfd_npm13xx_reg_write_burst( | ||
| config->mfd, CHGR_BASE, CHGR_OFFSET_NTC_TEMPS + (idx * 2U), | ||
| code >> NTCTEMP_MSB_SHIFT, code & NTCTEMP_LSB_MASK); | ||
| (uint8_t []){code >> NTCTEMP_MSB_SHIFT, code & NTCTEMP_LSB_MASK}, |
There was a problem hiding this comment.
[nitpick] The compound literal array should be assigned to a named variable for better readability. Consider creating a temporary array variable like uint8_t ntc_data[] = {code >> NTCTEMP_MSB_SHIFT, code & NTCTEMP_LSB_MASK};
Copilot uses AI. Check for mistakes.
| int ret = mfd_npm13xx_reg_write_burst( | ||
| config->mfd, CHGR_BASE, CHGR_OFFSET_DIE_TEMPS + (idx * 2U), | ||
| code >> DIETEMP_MSB_SHIFT, code & DIETEMP_LSB_MASK); | ||
| (uint8_t []){code >> DIETEMP_MSB_SHIFT, code & DIETEMP_LSB_MASK}, |
There was a problem hiding this comment.
[nitpick] The compound literal array should be assigned to a named variable for better readability. Consider creating a temporary array variable like uint8_t temp_data[] = {code >> DIETEMP_MSB_SHIFT, code & DIETEMP_LSB_MASK};
Copilot uses AI. Check for mistakes.
| ret = mfd_npm13xx_reg_write_burst(config->mfd, CHGR_BASE, CHGR_OFFSET_ISET, | ||
| (uint8_t []){idx / 2U, idx & 1U}, 2U); |
There was a problem hiding this comment.
[nitpick] The compound literal array should be assigned to a named variable for better readability. Consider creating a temporary array variable like uint8_t charge_data[] = {idx / 2U, idx & 1U};
| ret = mfd_npm13xx_reg_write_burst(config->mfd, CHGR_BASE, CHGR_OFFSET_ISET, | |
| (uint8_t []){idx / 2U, idx & 1U}, 2U); | |
| uint8_t charge_data[] = {idx / 2U, idx & 1U}; | |
| ret = mfd_npm13xx_reg_write_burst(config->mfd, CHGR_BASE, CHGR_OFFSET_ISET, | |
| charge_data, 2U); |
Copilot uses AI. Check for mistakes.
| ret = mfd_npm13xx_reg_write_burst( | ||
| config->mfd, CHGR_BASE, CHGR_OFFSET_ISET_DISCHG, | ||
| npm1300_discharge_limits[config->dischg_limit_idx] / 2U, | ||
| npm1300_discharge_limits[config->dischg_limit_idx] & 1U); | ||
| (uint8_t []){npm1300_discharge_limits[config->dischg_limit_idx] / 2U, | ||
| npm1300_discharge_limits[config->dischg_limit_idx] & 1U}, 2U); |
There was a problem hiding this comment.
[nitpick] The compound literal array should be assigned to a named variable for better readability. Consider creating a temporary array variable like uint8_t discharge_data[] = {npm1300_discharge_limits[config->dischg_limit_idx] / 2U, npm1300_discharge_limits[config->dischg_limit_idx] & 1U};
Copilot uses AI. Check for mistakes.
|
@kartben I don't feel like addressing Copilot's nitpicks if it's fine by you :) |
I wouldn't necessarily say these qualify as nitpicks, but sure :) |
11 participants
Change the npm13xx_charger fetch function to first trigger a sample and then block until the result is available.