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

[Jonathan Cameron <jic23@kernel.org>]

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

> 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.

> +		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.

> +		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?

> +
> +		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?

> +			    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!

> +	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.

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