Re: [PATCH v5 3/3] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator
From: Neil Armstrong
Date: Wed Nov 04 2026 - 03:37:12 EST
On 11/4/26 03:30, Jonathan Cameron wrote:
On Mon, 02 Nov 2026 15:49:06 +0100
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, 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>
The use of the regmap_noinc stuff to deal with the variable
size 'registers' is novel. Might be worth just adding a little
commentary on that to somewhere. Perhaps the regmap_config as a comment.
Other than that, just a question about why we need the postfixes
for the labels. That seems superfluous.
Thanks,
Jonathan
diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
new file mode 100644
index 000000000000..e679cf142141
--- /dev/null
+++ b/drivers/iio/adc/max34417.c
+As noted above. This isn't really true. You are just use noinc
+static const struct regmap_config max34417_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
reads to pretend it is. Those are normally only used for
fifo registers so I think we should have a comment here talking
about that choice.
Well, reg_bits is 8, but val_bits is 8 for the control & status registers,
for the data registers the value bits is variable, so it's kind of true.
But I should add the readable_noinc_reg callback, yes it's not FIFOs,
but both 3 data registers have different data width which should be
read using the noinc to use i2c_smbus_read_i2c_block_data().
I can provide my custom register read/write copied from regmap-i2c if
you find it better to avoid abusing regmap...
It is pretty neat so I'm not against it but it might rather
surprise anyone looking at the datasheet!
+ .max_register = MAX34417_BULK_VOLTAGE_READOUT_REG,
+ .writeable_reg = max34417_writeable_reg,
+ .readable_reg = max34417_readable_reg,
+ .volatile_reg = max34417_volatile_reg,
+ /* No cache since the only non-volatile register is read-only */
+};
+
+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 (input_label) {
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ return sysfs_emit(label, "%s-voltage\n", input_label);
Why add the postfix? The channel type will turn up in the file name anyway.
I don't think anything prevents us having the same label for channels of
different types. Is this a copy of something elsewhere?
It sounded better, but you're right I'll drop those.
+ case IIO_POWER:Same applies here. The postfix seems excessive.
+ return sysfs_emit(label, "%s-power\n", input_label);
+ case IIO_ENERGY:
+ return sysfs_emit(label, "%s-energy\n", input_label);
+ default:
+ return 0;
+ }
+ }
+
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ return sysfs_emit(label, "channel%d-voltage\n", chan->channel);
+ case IIO_POWER:
+ return sysfs_emit(label, "channel%d-power\n", chan->channel);
+ case IIO_ENERGY:
+ return sysfs_emit(label, "channel%d-energy\n", chan->channel);
+ default:
+ 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 *map;
+ int rc;
+
+ map = devm_regmap_init_i2c(client, &max34417_regmap_config);
+ if (IS_ERR(map))
+ return dev_err_probe(dev, PTR_ERR(map), "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 = map;
+ max34417->indio_dev = indio_dev;
+
+ rc = devm_mutex_init(dev, &max34417->lock);
+ if (rc)
+ return rc;
+
+ /* Set default input correction for all channels */
+ for (int 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_uOhm, index;
+
+ 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, -ECHRNG, "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",
Don't worry about going a bit longer - stcik to aligning after (
Or.. Maybe use a helper function for the per channel parsing.
Ack
Thanks,
Neil
+ node);
+ }
+
+ if (fwnode_property_present(node, "shunt-resistor-micro-ohms")) {
+ rc = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms",
+ &rsense_uOhm);
+ if (rc)
+ return dev_err_probe(dev, rc,
+ "failure reading shunt property of %pfwP\n",
+ node);
+
+ if (rsense_uOhm < MAX34417_MIN_RSENSE ||
+ rsense_uOhm > MAX34417_MAX_RSENSE)
+ return dev_err_probe(dev, -EINVAL,
+ "invalid shunt value %d of %pfwP\n",
+ rsense_uOhm, node);
+
+ max34417->input_correction[index] =
+ max34417_calc_input_correction(rsense_uOhm);
+ }
+ }