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

[Neil Armstrong <neil.armstrong@linaro.org>]

Hi,

On 10/13/26 16:53, Andy Shevchenko wrote:
> On Tue, Oct 13, 2026 at 04:07:13PM +0200, Neil Armstrong wrote:
>> The MAX34417 is a specialized current and voltage monitor used to
>> determine power consumption of portable systems. The driver support
>> getting the channels voltage, average power and accumulated average
>> power and energy over an I2C/SMBUS serial interface.
>>
>> The MAX34417 datasheet only specified on how to calculate the
>> accumulated average power, but with the Sample Per Second rate
>> we can easily get accumulated Energy in mJ.
>>
>> By default the IIO driver only supports single-shot Voltage
>> and Power sensing, but the accumulation can be enabled by
>> enabling the Energy IIO channel which then enables the
>> Continuous Accumulation Mode.
>>
>> The counter is common to all channels, so when reading a single
>> channel we will copy all accumulators to registers, so in order
>> to permit reading individual channels we must accumulate the
>> other channels in the driver when Continuous Accumulation Mode
>> is enabled.
> 
> ...
> 
>> +#define MAX34417_MIN_RSENSE		(MILLI)	/* 1 milliOhm */
> 
> 1 * MILLI
> 
>> +#define MAX34417_MAX_RSENSE		(100 * MILLI) /* 100 milliOhm */
>> +#define MAX34417_DEFAULT_RSENSE		MAX34417_MIN_RSENSE
> 
> I would rather put the exact value here as well.

This would duplicate the value for no reason, right ?

> 
> ...
> 
>> +struct max34417_data {
>> +	struct iio_dev *indio_dev;
> 
>> +	struct regmap *regmap;
>> +	struct device *dev;
> 
> One of them seems redundant (I suppose the 'dev').

Probably yes

> 
>> +	struct mutex lock;
>> +	u32 input_correction[MAX34417_CHANNEL_COUNT];
>> +	const char *input_label[MAX34417_CHANNEL_COUNT];
>> +	bool cam_mode;
>> +	u64 acc_power[MAX34417_CHANNEL_COUNT];
>> +	u32 acc_count[MAX34417_CHANNEL_COUNT];
>> +};
>> +
>> +static const struct regmap_config max34417_regmap_config = {
>> +	.reg_bits	= 8,
>> +	.val_bits	= 8,
>> +	.max_register	= MAX34417_BULK_VOLTAGE_READOUT_REG,
> 
> No cache? All registers are volatile?
> 
>> +};
> 
> ...
> 
>> +#define MAX34417_CHANNEL(_index, _v_address, _power_address)	\
>> +	{							\
>> +		.type = IIO_VOLTAGE,				\
>> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) | \
>> +					  BIT(IIO_CHAN_INFO_SCALE), \
> 
> Instead of broken indentation at the end, use different style for these type of
> assignments, like
> 
> 		.info_mask_separate =
> 			BIT(IIO_CHAN_INFO_RAW) |		\
> 			BIT(IIO_CHAN_INFO_SCALE),		\
> 
> Also do not use brokenly indented '=', just a space after the field is enough.
> 
> 
>> +		.channel = (_index),				\
>> +		.address = (_v_address),			\
>> +		.indexed = 1,					\
>> +	},							\
>> +	{							\
>> +		.type = IIO_POWER,				\
>> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) | \
>> +					  BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \
>> +					  BIT(IIO_CHAN_INFO_SCALE), \
>> +		.channel = (_index),				\
>> +		.address = (_power_address),			\
>> +		.indexed = 1,					\
>> +	},							\
>> +	{							\
>> +		.type = IIO_ENERGY,				\
>> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) | \
>> +					  BIT(IIO_CHAN_INFO_SCALE), \
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_ENABLE), \
>> +		.channel = (_index),				\
>> +		.address = (_power_address),			\
>> +		.indexed = 1,					\
>> +	}
> 
> ...
> 
>> +static int max34417_reset_smm(struct max34417_data *max34417)
>> +{
>> +	u32 control;
>> +	int rc;
>> +
>> +	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Set SMM=0 to re-init the SMM mode */
>> +	control &= ~(MAX34417_CONTROL_SMM);
>> +	rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
>> +	if (rc)
>> +		return rc;
> 
> regmap_update_bits() / regmap_clear_bits()?
> 
>> +	/* Reset accumulators after setting SMM=0 */
>> +	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
>> +	if (rc)
>> +		return rc;
> 
> regmap_set_bits() ?
> 
>> +	return 0;
>> +}
> 
> ...
> 
> Also check the rest of the code for the similar RMW cases.
> 
> ...
> 
>> +	/*
>> +	 * UPDATE commands should be no less than 1msec apart for reliable
>> +	 * measurements.
>> +	 */
>> +	fsleep(1000);
> 
> 1 * USEC_PER_MSEC
> 
>> +	u32 acc_count;
>> +	int rc;
>> +
>> +	rc = max34417_read_acc_count(max34417, &acc_count);
>> +	if (rc)
>> +		return rc;
>> +
>> +	for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i) {
> 
> 'i' is not used outside of the loop AFAICS. Also why pre-increment?

I'm not used defining variables in the for loop, and bad habit...

> 
> 	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;
>> +
>> +		max34417->acc_power[i] += acc_power;
>> +		max34417->acc_count[i] += acc_count;
>> +	}
> 
> ...
> 
>> +	/* Check for overflow */
>> +	rc = max34417_check_ovf(max34417);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
> 
> 	return ..._check_ovf(...);
> 
> ...
> 
>> +	/* Ignore byte0 */
>> +	voltage = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]);
>> +	voltage >>= 2;
> 
> The above comment is odd. Please, clarify, ideally with a reference to
> the datasheet table / section / et cetera.

Odd ? I mean it's not, we skip byte 0 and get unaligned big-endian 16bit at offset 1...

> 
> Also can be folded into a single expression:
> 
> 	voltage = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]) >> 2;
> 
> ...
> 
>> +		if (unlikely(acc_count == 0)) {
> 
> Why unlikely()? Is this a hot-path? What is the performance effect will it give?

It's highly unlikely yes, not specifically a hot path, will remove if to controversial.

> 
>> +			dev_warn(max34417->dev, "unexpected zero accumulator count\n");
>> +
>> +			*val = 0;
>> +			*val2 = 0;
>> +
>> +			return IIO_VAL_INT_64;
>> +		}
> 
> ...
> 
>> +	/*
>> +	 * Move the data to the SMBus registers
>> +	 */
> 
> A single line comment.
> 
> ...
> 
>> +static int max34417_setup_cam_mode(struct max34417_data *max34417, int val)
>> +{
>> +	u32 control;
>> +	int rc, i;
>> +
>> +	guard(mutex)(&max34417->lock);
>> +
>> +	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (max34417->cam_mode && !val) {
>> +		/* SMM and CAM modes should not be enabled at the same time */
>> +		control &= ~(MAX34417_CONTROL_CAM | MAX34417_CONTROL_SMM);
>> +		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
>> +		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(max34417->regmap, 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.
>> +		 */
>> +		control |= MAX34417_CONTROL_CAM;
>> +		/* SMM and CAM modes should not be enabled at the same time */
>> +		control &= ~(MAX34417_CONTROL_SMM);
>> +		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
>> +		if (rc)
>> +			return rc;
>> +
>> +		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
>> +		if (rc)
>> +			return rc;
>> +
>> +		/* Reset counters and accumulators */
>> +		for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i) {
> 
> 		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;
>> +}
> 
> ...
> 
>> +static int max34417_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val, int *val2, long mask)
>> +{
>> +	struct max34417_data *max34417 = iio_priv(indio_dev);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		switch (chan->type) {
>> +		case IIO_VOLTAGE:
>> +			return max34417_read_voltage(max34417, chan, val);
>> +		case IIO_POWER:
>> +			/* In default mode, we return a single measurement */
>> +			if (max34417->cam_mode)
>> +				return -EINVAL;
>> +
>> +			return max34417_read_power(max34417, chan, val, val2);
>> +		case IIO_ENERGY:
>> +			if (!max34417->cam_mode)
>> +				return -EINVAL;
>> +
>> +			return max34417_read_energy(max34417, chan, val, val2);
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	case IIO_CHAN_INFO_AVERAGE_RAW:
>> +		if (chan->type != IIO_POWER)
>> +			return -EINVAL;
>> +
>> +		/* In CAM mode, we return an average measurement */
>> +		if (!max34417->cam_mode)
>> +			return -EINVAL;
>> +
>> +		return max34417_read_power(max34417, chan, val, val2);
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (chan->type == IIO_VOLTAGE) {
>> +			/* Scale to mV */
>> +			*val = MAX34417_VOLTAGE_CORRECTION_SCALE * MILLI;
>> +			*val2 = MAX34417_VOLTAGE_FULL_SCALE_BITS;
>> +
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		} else if (chan->type == IIO_POWER) {
>> +			/*
>> +			 * Scale to mW
>> +			 * (1000 * (Power Accumulation / Counter) * Correction) / 2^30
>> +			 */
>> +			*val = max34417->input_correction[chan->channel] * MILLI;
> 
> MILLIWATT_PER_WATT
> 
>> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
>> +
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		} else if (chan->type == 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 = ((u64)(1 << MAX34417_PWR_AVG_FULL_SCALE_BITS) *
> 
> Why not BIT_ULL() and drop that casting altogether? Also seems that it becomes
> 64-bit division. Does it compile on 32-bit?

Good point, will need to check that

> 
>> +				 MAX34417_FAST_SPS_CAM) / MILLI;
>> +
>> +			return IIO_VAL_FRACTIONAL;
>> +		}
>> +
>> +		return -EINVAL;
>> +	case IIO_CHAN_INFO_ENABLE:
>> +		*val = max34417->cam_mode;
>> +
>> +		return IIO_VAL_INT;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
> 
> ...
> 
>> +static int max34417_read_label(struct iio_dev *indio_dev,
>> +			       struct iio_chan_spec const *chan,
>> +			       char *label)
>> +{
>> +	struct max34417_data *max34417 = iio_priv(indio_dev);
>> +	const char *input_label = max34417->input_label[chan->channel];
>> +
>> +	if (chan->type == IIO_VOLTAGE) {
>> +		if (input_label)
>> +			return sysfs_emit(label, "%s-voltage\n", input_label);
>> +
>> +		return sysfs_emit(label, "channel%d-voltage\n", chan->channel);
>> +	}
>> +
>> +	if (chan->type == IIO_POWER) {
>> +		if (input_label)
>> +			return sysfs_emit(label, "%s-power\n", input_label);
>> +
>> +		return sysfs_emit(label, "channel%d-power\n", chan->channel);
>> +	}
>> +
>> +	if (chan->type == IIO_ENERGY) {
>> +		if (input_label)
>> +			return sysfs_emit(label, "%s-energy\n", input_label);
>> +
>> +		return sysfs_emit(label, "channel%d-energy\n", chan->channel);
>> +	}
>> +
>> +	return 0;
>> +}
> 
> This can be simplified, right?

I don't find this really simplified TBH, just reorganized

> 
> 	if (input_label) {
> 		switch (chan->type) {
> 		case IIO_VOLTAGE:
> 			return sysfs_emit(label, "%s-voltage\n", input_label);
> 		...
> 		default:
> 			return 0;
> 		}
> 	}
> 
> 	switch (chan->type) {
> 	case IIO_VOLTAGE:
> 		return sysfs_emit(label, "channel%d-voltage\n", chan->channel);
> 	...
> 	default:
> 		return 0;
> 	}
> 
> ...
> 
>> +	mutex_init(&max34417->lock);
> 
> Why not devm_mutex_init()?
> 
> ...
> 
>> +	/* Set default input correction for all channels */
>> +	for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i)
> 
> 	for (int i = 0; i < MAX34417_CHANNEL_COUNT; i++)
> 
>> +		max34417->input_correction[i] =
>> +			max34417_calc_input_correction(MAX34417_DEFAULT_RSENSE);
> 
> ...
> 
>> +		if (fwnode_property_present(node, "shunt-resistor-micro-ohms")) {
>> +			rc = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms",
>> +						      &rsense);
> 
> Name the variable with units, rsense_mOhm.
> 
>> +			if (rc)
>> +				return dev_err_probe(dev, rc,
>> +						"failure reading shunt property of %pfwP\n",
>> +						node);
>> +
>> +			if (!rsense || rsense < MAX34417_MIN_RSENSE ||
>> +			    rsense > MAX34417_MAX_RSENSE)
> 
> Please, make it logically split
> 
> 			// also == 0 is okay for the actual values
> 			// (which have not a semantic of pointer or error)
> 			if (rsense == 0 ||
> 			    rsense < MAX34417_MIN_RSENSE || rsense > MAX34417_MAX_RSENSE)
> 
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "invalid shunt value %d of %pfwP\n",
>> +						     rsense, node);
>> +
>> +			max34417->input_correction[index] =
>> +				max34417_calc_input_correction(rsense);
>> +		}
>> +	}
> 

Thanks for the review,
Neil