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/18/26 00:47, Jonathan Cameron wrote:
> On Tue, 13 Oct 2026 16:07:13 +0200
> Neil Armstrong <neil.armstrong@linaro.org> 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.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> 
> Reminder to check sashiko.  I haven't looked at whether they
> are right this time.
> https://sashiko.dev/#/patchset/20261013-topic-sm8x50-iio-max34417-adc-v3-0-d3604ae4a229%40linaro.org
> 

I did and there's some good catches

>> diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
>> new file mode 100644
>> index 000000000000..00d7f20cd953
>> --- /dev/null
>> +++ b/drivers/iio/adc/max34417.c
> 
>> +/* TODO Implement trigger to update accumulator once and get all channels at once */
>> +
>> +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;
>> +
>> +	/* Reset accumulators after setting SMM=0 */
>> +	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> return regmap_write()
> 
> Might as well save a few lines.
> 
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
>> +}
> 
> 
>> +
>> +static int max34417_check_ovf(struct max34417_data *max34417)
>> +{
>> +	u32 control;
>> +	int rc;
>> +
>> +	/* Check for overflow */
> 
> Given function name and the check below, not seeing value in this comment.
> 
>> +	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (control & MAX34417_CONTROL_OVF) {
>> +		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG,
>> +				  control & ~MAX34417_CONTROL_OVF);
>> +		if (rc)
>> +			return rc;
>> +
>> +		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * 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)
>> +{
>> +	unsigned int i;
>> +	u64 acc_power;
>> +	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) {
>> +		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);
> 
> return max34417_..
> 
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
>> +}
> 
> 
>> +
>> +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);
> 
> The comment doesn't makes sense given neither are enabled.

It's just some phrases from the Datasheet to pin point the process, will drop

> 
>> +		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
> 
> Feels like it would be cleaner as an update_bits() here and below rather than
> sharing that read above.

Yep will switch to _bits regmap ops

> 
>> +		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);
> 
> As above, perhaps an update_bits is simpler?
> 
>> +		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) {
>> +			max34417->acc_power[i] = 0;
>> +			max34417->acc_count[i] = 0;
>> +		}
>> +	}
>> +
>> +	max34417->cam_mode = val;
>> +
>> +	return 0;
>> +}
> 
> 
> 
>> +static int max34417_probe(struct i2c_client *client)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct max34417_data *max34417;
>> +	struct iio_dev *indio_dev;
>> +	struct regmap *regmap;
>> +	int rc, i;
>> +
>> +	regmap = devm_regmap_init_i2c(client, &max34417_regmap_config);
>> +	if (IS_ERR(regmap))
>> +		return dev_err_probe(dev, PTR_ERR(regmap), "regmap_init failed\n");
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*max34417));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	rc = devm_regulator_get_enable(dev, "vdd");
>> +	if (rc)
>> +		return dev_err_probe(dev, rc, "failed to get vdd regulator\n");
>> +
>> +	rc = devm_regulator_get_enable(dev, "vio");
>> +	if (rc)
>> +		return dev_err_probe(dev, rc, "failed to get vio regulator\n");
>> +
>> +	max34417 = iio_priv(indio_dev);
>> +	max34417->regmap = regmap;
>> +	max34417->dev = dev;
>> +	max34417->indio_dev = indio_dev;
>> +	mutex_init(&max34417->lock);
>> +
>> +	/* Set default input correction for all channels */
>> +	for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i)
>> +		max34417->input_correction[i] =
>> +			max34417_calc_input_correction(MAX34417_DEFAULT_RSENSE);
>> +
>> +	device_for_each_child_node_scoped(dev, node) {
>> +		u32 rsense, index;
>> +
>> +		if (!fwnode_property_present(node, "reg"))
>> +			return dev_err_probe(dev, -EINVAL, "missing reg property of %pfwP\n",
>> +					     node);
> 
> Bit unusual to check for a property that must be there.
> We tend to use present checks for optional properties. Is the
> following failure path not enough?

It should, I maybe added too much checks, will drop.

> 
>> +
>> +		rc = fwnode_property_read_u32(node, "reg", &index);
>> +		if (rc)
>> +			return dev_err_probe(dev, rc, "failure reading reg property of %pfwP\n",
>> +					     node);
>> +
>> +		if (index >= MAX34417_CHANNEL_COUNT)
>> +			return dev_err_probe(dev, -EINVAL, "invalid reg %d of %pfwP\n",
>> +					     index, node);
>> +
>> +		if (fwnode_property_present(node, "label")) {
>> +			rc = fwnode_property_read_string(node, "label",
>> +							 &max34417->input_label[index]);
>> +			if (rc)
>> +				return dev_err_probe(dev, rc,
>> +						"failure reading label property of %pfwP\n",
>> +						node);
>> +		}
>> +
>> +		if (fwnode_property_present(node, "shunt-resistor-micro-ohms")) {
>> +			rc = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms",
>> +						      &rsense);
>> +			if (rc)
>> +				return dev_err_probe(dev, rc,
>> +						"failure reading shunt property of %pfwP\n",
>> +						node);
>> +
>> +			if (!rsense || rsense < MAX34417_MIN_RSENSE ||
> 
> Is !rsense useful given that is less than min?

Indeed. leftover from previous version

> 
>> +			    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);
>> +		}
>> +	}
>> +
>> +	indio_dev->channels = max34417_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(max34417_channels);
>> +	indio_dev->name = "max34417";
>> +	indio_dev->info = &max34417_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	rc = devm_iio_device_register(dev, indio_dev);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Reset mode and set accumulator width to Wide */
> 
> The docs refer to this as choosing between the max34417 and the max34407.
> Any idea what that is going on about?  Is it that the setting this to 0 would
> make the device look just like the part with small accumulator?
> Seems like an easy win if you fancy adding support for that part as well as
> this would be the main difference I think (it's reserved on that part to 0)
> 
> Anyhow, just made me curious rather than a request to change anything!

Indeed, I don't feel confident enough to add max34407 support, but I can definitely
add a comment pointing this is the main difference between max34417 & max34407.

> 
>> +	rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, MAX34417_CONTROL_MODE);
>> +	if (rc)
>> +		return dev_err_probe(max34417->dev, rc, "Error writing control register\n");
>> +
>> +	/* Reset the accumulators */
>> +	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> 
> return regmap_write();
> 
> Though I'm not sure why you'd do these two after setting up the userspace
> interface (iio_device_register). Is there no potential race where something
> related to the accumulators gets read before they are reset?  Seems that would look
> a little more odd than the other way around where they aren't quite
> zero when they first become readable.

Yep, will move before,

Thanks,
Neil

> 
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
>> +}
>