Re: [PATCH v3 3/3] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator
From: Neil Armstrong
Date: Mon Oct 19 2026 - 11:51:03 EST
Hi,
On 10/18/26 00:47, Jonathan Cameron wrote:
On Tue, 13 Oct 2026 16:07:13 +0200
Neil Armstrong <neil.armstrong@xxxxxxxxxx> 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@xxxxxxxxxx>
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 */return regmap_write()
+
+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);
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;
+}