Re: [PATCH v5 3/3] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] iio: adc: max34417: shared accumulators clobbered on CAM power/energy read
- [Medium] iio: adc: max34417: missing 1 ms delay between UPDATE commands
- [Medium] iio: adc: max34417: scale truncation and mJ unit in max34417_read_raw()
- [Medium] iio: adc: max34417: missing UPDATE before CAM in max34417_setup_cam_mode()
- [Low] iio: adc: max34417: reuse max34417_reset_smm() in max34417_trigger_smm()
--

commit e5d0251e2d5be0cc82051f421a62b0919d01161f
Author: Neil Armstrong <neil.armstrong@linaro.org>

iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

This commit adds an IIO ADC driver for the Analog Devices MAX34417
four-channel current and voltage monitor, supporting single-shot and
continuous accumulation measurements of voltage, average power, and energy
over I2C/SMBus.

> diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
> new file mode 100644
> index 0000000000000..e679cf142141b
> --- /dev/null
> +++ b/drivers/iio/adc/max34417.c

[ ... ]

> +static int max34417_reset_smm(struct max34417_data *max34417)
> +{
> +	struct regmap *map = max34417->regmap;
> +	int rc;
> +
> +	/* Set SMM=0 to re-init the SMM mode */
> +	rc = regmap_clear_bits(map, MAX34417_CONTROL_REG, MAX34417_CONTROL_SMM);
> +	if (rc)
> +		return rc;
> +
> +	/* Reset accumulators after setting SMM=0 */
> +	return regmap_write(map, MAX34417_UPDATE_REG, 1);
> +}
> +
> +static int max34417_trigger_smm(struct max34417_data *max34417)
> +{
> +	struct regmap *map = max34417->regmap;
> +	u32 control;
> +	int rc;
> +
> +	rc = regmap_read(map, MAX34417_CONTROL_REG, &control);
> +	if (rc)
> +		return rc;
> +
> +	if (control & MAX34417_CONTROL_SMM) {
> +		/* Set SMM=0 to re-init the SMM mode */
> +		rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
> +				       MAX34417_CONTROL_SMM);
> +		if (rc)
> +			return rc;
> +
> +		/* Reset accumulators after setting SMM=0 */
> +		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +		if (rc)
> +			return rc;
> +	}

[Severity: Low]
This isn't a bug, but could max34417_trigger_smm() call
max34417_reset_smm(max34417) here instead of duplicating the
regmap_clear_bits() and regmap_write() sequence?

> +
> +	/* Enable SMM to do a single accumulation */
> +	rc = regmap_set_bits(map, MAX34417_CONTROL_REG, MAX34417_CONTROL_SMM);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * After the SMM bit is changed, the UPDATE command should be sent
> +	 * to reset the accumulators and perform the selected scan operation.
> +	 */
> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +	/*
> +	 * UPDATE commands should be no less than 1msec apart for reliable
> +	 * measurements, since in Manual mode the SPS is 1kHz, we need wait
> +	 * for the measurement to finish before moving the data in the SMbus
> +	 * registers.
> +	 */
> +	fsleep(1 * USEC_PER_MSEC);
> +
> +	return 0;
> +}

[ ... ]

> +static int max34417_check_ovf(struct max34417_data *max34417)
> +{
> +	struct regmap *map = max34417->regmap;
> +	u32 control;
> +	int rc;
> +
> +	rc = regmap_read(map, MAX34417_CONTROL_REG, &control);
> +	if (rc)
> +		return rc;
> +
> +	if (!(control & MAX34417_CONTROL_OVF))
> +		return 0;
> +
> +	rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
> +			       MAX34417_CONTROL_OVF);
> +	if (rc)
> +		return rc;
> +
> +	return regmap_write(map, MAX34417_UPDATE_REG, 1);
> +}
> +
> +/*
> + * Save all power accumulators and counter after each UPDATE command
> + * since it will reset the internal registers for ALL channels,
> + * so in order to not loose any captured data we retrieve all registers
> + * and accumulate them in the context variables.
> + * When user gets either power or energy, those variables content
> + * will be returned and zero-ed to return the power accumulated
> + * since last read for each separate channel.
> + */
> +static int max34417_save_accumulators(struct max34417_data *max34417)
> +{
> +	u64 acc_power;
> +	u32 acc_count;
> +	int rc;
> +
> +	rc = max34417_read_acc_count(max34417, &acc_count);
> +	if (rc)
> +		return rc;
> +
> +	for (int i = 0; i < MAX34417_CHANNEL_COUNT; i++) {
> +		rc = max34417_read_acc_power(max34417, MAX34417_PWR_ACC_1_REG + i,
> +					     &acc_power);
> +		if (rc)
> +			return rc;
> +
> +		/* TODO sanitize accumulators in case of overflow */
> +		max34417->acc_power[i] += acc_power;
> +		max34417->acc_count[i] += acc_count;
> +	}
> +
> +	/* Check for overflow */
> +	return max34417_check_ovf(max34417);
> +}
> +
> +static int max34417_read_voltage(struct max34417_data *max34417,
> +				 const struct iio_chan_spec *chan, int *val)
> +{
> +	struct regmap *map = max34417->regmap;
> +	u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
> +	int rc;
> +
> +	/* In SMM mode, trigger a measurement cycle manually */
> +	if (!max34417->cam_mode) {
> +		rc = max34417_trigger_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* Move the new UPDATE data into the SMBus read registers */
> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	if (max34417->cam_mode) {
> +		/* Retrieve all channels to avoid missing data for other channels */
> +		rc = max34417_save_accumulators(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	rc = regmap_noinc_read(map, chan->address, &buf, sizeof(buf));
> +	if (rc)
> +		return rc;
> +
> +	if (!max34417->cam_mode) {
> +		rc = max34417_reset_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}

[Severity: Medium]
As reported by Sashiko on v3 [1], writes to MAX34417_UPDATE_REG must be at
least 1 ms apart because a 4-channel scan takes roughly 976 us at 1024
samples per second in single-shot mode and 651 us at 1536 samples per
second in continuous accumulation mode, yet fsleep(1 * USEC_PER_MSEC) is
only called once at the end of max34417_trigger_smm().

In continuous accumulation mode, max34417_setup_cam_mode(),
max34417_read_voltage(), max34417_read_power(), and max34417_read_energy()
write MAX34417_UPDATE_REG without enforcing a minimum interval since the
previous update. If userspace reads in_powerY_mean_raw within 1 ms of
enabling in_energy_en, or reads the same channel twice within one scan
period, MAX34417_UPDATE_REG is written before a single sample is
accumulated, causing MAX34417_ACC_COUNT_REG to read as 0, triggering the
"unexpected zero accumulator count" warning, and resetting the in-progress
hardware scan.

Similarly, in max34417_save_accumulators() -> max34417_check_ovf(), if
MAX34417_CONTROL_OVF is set, it clears the flag and immediately writes
MAX34417_UPDATE_REG a second time without waiting 1 ms or saving samples
accumulated during the readout; in max34417_read_voltage(), this also
happens before chan->address is read.

In single-shot mode, max34417_read_voltage() and max34417_read_power()
write MAX34417_UPDATE_REG, perform one read, and immediately call
max34417_reset_smm(), which writes MAX34417_UPDATE_REG again without a 1 ms
delay (and max34417_trigger_smm() can also issue two back-to-back writes to
MAX34417_UPDATE_REG when MAX34417_CONTROL_SMM is already set).

Should the driver track the timestamp of the last MAX34417_UPDATE_REG write
or enforce the minimum delay across all paths that issue an update command?

> +
> +	*val = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]) >> 2;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int max34417_read_power(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{
> +	struct regmap *map = max34417->regmap;
> +	u64 power;
> +	int rc;
> +
> +	/* In SMM mode, trigger a measurement cycle manually */
> +	if (!max34417->cam_mode) {
> +		rc = max34417_trigger_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/*
> +	 * Move the new UPDATE data into the SMBus read registers.
> +	 * In CAM mode this will also reset the accumulators, returning
> +	 * the power accumulated since the last UPDATE.
> +	 */
> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Only get the accumulator counter in CAM mode, in SMM mode
> +	 * we only accumulate once, so the counter is 1
> +	 */
> +	if (max34417->cam_mode) {
> +		u64 acc_count;
> +		u64 acc_power;
> +
> +		/* Retrieve all channels to avoid missing data for other channels */
> +		rc = max34417_save_accumulators(max34417);
> +		if (rc)
> +			return rc;
> +
> +		/* Get current channel power, counter and reset them */
> +		acc_power = max34417->acc_power[chan->channel];
> +		acc_count = max34417->acc_count[chan->channel];
> +		max34417->acc_power[chan->channel] = 0;
> +		max34417->acc_count[chan->channel] = 0;
> +
> +		if (acc_count == 0) {
> +			dev_warn_ratelimited(&max34417->indio_dev->dev,
> +					     "unexpected zero accumulator count\n");
> +			power = 0;
> +		} else {
> +			power = div64_u64(acc_power, acc_count);
> +		}
> +	} else {
> +		rc = max34417_read_acc_power(max34417, chan->address, &power);
> +		if (rc)
> +			return rc;
> +
> +		rc = max34417_reset_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	iio_val_s64_decompose(power, val, val2);
> +
> +	return IIO_VAL_INT_64;
> +}
> +
> +static int max34417_read_energy(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{
> +	struct regmap *map = max34417->regmap;
> +	u64 acc_power;
> +	int rc;
> +
> +	/* Move the data to the SMBus registers */
> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/* Retrieve all channels to avoid missing data for other channels */
> +	rc = max34417_save_accumulators(max34417);
> +	if (rc)
> +		return rc;
> +
> +	/* Get current channel power and reset them */
> +	acc_power = max34417->acc_power[chan->channel];
> +	max34417->acc_power[chan->channel] = 0;
> +	max34417->acc_count[chan->channel] = 0;

[Severity: Medium]
When continuous accumulation mode is enabled, both max34417_read_power()
(via IIO_CHAN_INFO_AVERAGE_RAW) and max34417_read_energy() (via
IIO_CHAN_INFO_RAW) accumulate hardware readings into the same per-channel
arrays max34417->acc_power[chan->channel] and
max34417->acc_count[chan->channel], and both functions zero those entries on
read.

Doesn't reading in_powerY_mean_raw discard all energy accumulated on that
channel since the last in_energyY_raw read, and conversely doesn't reading
in_energyY_raw wipe out the accumulated power and sample count for
in_powerY_mean_raw (causing an immediate subsequent read of
in_powerY_mean_raw to see acc_count == 0 and warn)? Also, shouldn't
in_energyY_raw report monotonically accumulated energy while enabled rather
than resetting max34417->acc_power[chan->channel] to 0 on every read?

> +
> +	iio_val_s64_decompose(acc_power, val, val2);
> +
> +	return IIO_VAL_INT_64;
> +}
> +
> +static int max34417_setup_cam_mode(struct max34417_data *max34417, int val)
> +{
> +	struct regmap *map = max34417->regmap;
> +	int rc;
> +
> +	if (max34417->cam_mode && !val) {
> +		rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
> +				       MAX34417_CONTROL_CAM | MAX34417_CONTROL_SMM);
> +		if (rc)
> +			return rc;
> +
> +		/*
> +		 * If CAM mode must be set, the Control byte must be set to
> +		 * desired values with CAM = 0, then followed by an UPDATE
> +		 */
> +		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +		if (rc)
> +			return rc;
> +	} else if (!max34417->cam_mode && val) {
> +		/*
> +		 * After this, the CAM mode bit can be set and another
> +		 * UPDATE must be sent.
> +		 */
> +		rc = regmap_update_bits(map, MAX34417_CONTROL_REG,
> +					MAX34417_CONTROL_SMM | MAX34417_CONTROL_CAM,
> +					MAX34417_CONTROL_CAM);
> +		if (rc)
> +			return rc;
> +
> +		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +		if (rc)
> +			return rc;

[Severity: Medium]
As reported by Sashiko on v4 [2], the comment split across the two branches
of max34417_setup_cam_mode() notes that to enable continuous accumulation
mode, MAX34417_CONTROL_REG must first be written with MAX34417_CONTROL_CAM
cleared and latched with a write to MAX34417_UPDATE_REG before setting
MAX34417_CONTROL_CAM and sending a second MAX34417_UPDATE_REG write.

If a previous single-shot read failed after max34417_trigger_smm() set
MAX34417_CONTROL_SMM (before max34417_reset_smm() could clear it),
MAX34417_CONTROL_SMM remains set in hardware while max34417->cam_mode is
false. When max34417_setup_cam_mode() is then called with val != 0,
regmap_update_bits() clears MAX34417_CONTROL_SMM and sets
MAX34417_CONTROL_CAM in a single transaction without first latching the
CAM = 0, SMM = 0 state via MAX34417_UPDATE_REG.

Should the enable path in max34417_setup_cam_mode() clear
MAX34417_CONTROL_SMM (with MAX34417_CONTROL_CAM = 0) and write
MAX34417_UPDATE_REG before setting MAX34417_CONTROL_CAM?

> +
> +		/* Reset counters and accumulators */
> +		for (int i = 0; i < MAX34417_CHANNEL_COUNT; i++) {
> +			max34417->acc_power[i] = 0;
> +			max34417->acc_count[i] = 0;
> +		}
> +	}
> +
> +	max34417->cam_mode = val;
> +
> +	return 0;
> +}

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			/* Scale to mV */
> +			*val = MAX34417_VOLTAGE_CORRECTION_SCALE * MILLI;
> +			*val2 = MAX34417_VOLTAGE_FULL_SCALE_BITS;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case IIO_POWER:
> +			/*
> +			 * Scale to mW
> +			 * (1000 * (Power Accumulation / Counter) * Correction) / 2^30
> +			 */
> +			*val = max34417->input_correction[chan->channel] * MILLIWATT_PER_WATT;
> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case IIO_ENERGY:
> +			/*
> +			 * Scale to mJ
> +			 * P=(1000 * (Power Accumulation / Counter) * Correction) / 2^30
> +			 * t=(Counter / Samples per Second)
> +			 * E=(P * t)
> +			 * Can be simplified to:
> +			 * (1000 * Power Accumulation * Correction) / (2^30 * SPS)
> +			 * The denominator doesn't fit in 32bit so scale down both terms.
> +			 */
> +			*val = max34417->input_correction[chan->channel];
> +			*val2 = div_u64(BIT_ULL(MAX34417_PWR_AVG_FULL_SCALE_BITS) *
> +					MAX34417_FAST_SPS_CAM, MILLI);
> +
> +			return IIO_VAL_FRACTIONAL;
> +		default:
> +			return -EINVAL;
> +		}

[ ... ]

> +static u32 max34417_calc_input_correction(u32 rsense_uOhm)
> +{
> +	/* Derived from Table 14. Correction Factors for Various Current-Sense Resistor Values */
> +	return (MAX34417_MAX_RSENSE * MAX34417_PWR_CORRECTION_SCALE) / rsense_uOhm;
> +}

[Severity: Medium]
Does the scale calculation in max34417_read_raw() and
max34417_calc_input_correction() lose significant precision and use the
wrong unit for IIO_ENERGY?

1. For IIO_ENERGY, max34417_read_raw() sets *val to input_correction (which
   is 24 when rsense_uOhm is 100000 uOhm) and *val2 to
   div_u64(BIT_ULL(30) * 1536, 1000) = 1649267441 with IIO_VAL_FRACTIONAL.
   Because iio_format_value() formats IIO_VAL_FRACTIONAL as
   div_s64((s64)vals[0] * 1000000000LL, vals[1]) with 9 decimal places,
   24 * 10^9 / 1649267441 truncates to 14 (0.000000014 instead of
   0.000000014551915), losing about 3.8% precision.

2. In max34417_calc_input_correction(),
   (MAX34417_MAX_RSENSE * MAX34417_PWR_CORRECTION_SCALE) / rsense_uOhm
   divides 2400000 by rsense_uOhm using integer arithmetic before
   max34417_read_raw() scales by MILLIWATT_PER_WATT, truncating non-integer
   correction factors (for instance, rsense_uOhm = 90000 yields 26 instead
   of 26.667, a 2.5% error).

3. Documentation/ABI/testing/sysfs-bus-iio specifies that in_energy_raw and
   in_energy_scale produce values in Joules after applying the scale,
   whereas max34417_read_raw() scales IIO_ENERGY to millijoules.

[1] https://lore.kernel.org/r/sashiko-outbox-169896@kernel.org
[2] https://lore.kernel.org/r/20261028-topic-sm8x50-iio-max34417-adc-v4-3-d4baaf3ee903@linaro.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-topic-sm8x50-iio-max34417-adc-v5-0-0ffa5e5f2d5c@linaro.org?part=3