Re: [PATCH v4 3/3] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator
From: Neil Armstrong
Date: Wed Oct 28 2026 - 11:39:14 EST
On 10/28/26 16:06, Andy Shevchenko wrote:
On Wed, Oct 28, 2026 at 02:19:06PM +0100, 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, 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 (1 * MILLI) /* 1000 microOhm */
+#define MAX34417_MAX_RSENSE (100 * MILLI) /* 100000 microOhm */
+#define MAX34417_DEFAULT_RSENSE (1 * MILLI) /* 1000 microOhm */
Add unit suffix. The comment versus MILLI multiplier is quite confusing.
What I read, it's milli-Ohms by value.
...
+struct max34417_data {
+ struct iio_dev *indio_dev;
+ struct regmap *regmap;
+ struct mutex lock;
+ u32 input_correction[MAX34417_CHANNEL_COUNT];
+ const char *input_label[MAX34417_CHANNEL_COUNT];
+ bool cam_mode;
Don't you have 4-byte gap here in some cases? Can you confirm the layout
with `pahole`? On top you can check binary size by running `bloat-o-meter`.
+ u64 acc_power[MAX34417_CHANNEL_COUNT];
+ u64 acc_count[MAX34417_CHANNEL_COUNT];
+};
...
+static bool max34417_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MAX34417_DID_REG:
+ return false;
+ default:
+ /* MAX34417_CONTROL_REG field OVF is volatile */
Why not add a case instead of a comment?
The case is handled by the default, the comment is here to
explain why MAX34417_CONTROL_REG is considered as volatile.
+ return true;
+ }
+}
...
+static int max34417_reset_smm(struct max34417_data *max34417)
+{
+ int rc;
+
+ /* Set SMM=0 to re-init the SMM mode */
+ rc = regmap_clear_bits(max34417->regmap, MAX34417_CONTROL_REG,
+ MAX34417_CONTROL_SMM);
With
struct regmap *map = max34417->regmap;
this becomes a single line
rc = regmap_clear_bits(map, MAX34417_CONTROL_REG, MAX34417_CONTROL_SMM);
Use this trick elsewhere where appropriate.
Ack
+ if (rc)
+ return rc;
+
+ /* Reset accumulators after setting SMM=0 */
+ return regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+}
...
+ *val = lower_32_bits(power);
+ *val2 = upper_32_bits(power);
Have you included wordpart.h?
Will add
...
+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);
sysfs.h for sysfs_emit().
Will add
+ case IIO_POWER:
+ 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 unsigned int max34417_calc_input_correction(u32 rsense_microOhm)
rsense_uOhm is shorter
Used that initially, will revert to that
+{
+ /* (100 milliOhm / rsense) * MAX34417_PWR_CORRECTION_SCALE */
Comment in this form is almost useless. Rather explain that we except
the result in the certain units or something like this.
Will try to improve
+ return (MAX34417_MAX_RSENSE * MAX34417_PWR_CORRECTION_SCALE) / rsense_microOhm;
+}
...
+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;
*map is shorter
Ok
+ int rc;
+
+ 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->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_microOhm, index;
rsense_uOhm
+ 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",
-ECHRNG
Ack
+ 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_microOhm);
+ if (rc)
+ return dev_err_probe(dev, rc,
+ "failure reading shunt property of %pfwP\n",
+ node);
+
+ if (rsense_microOhm < MAX34417_MIN_RSENSE ||
+ rsense_microOhm > MAX34417_MAX_RSENSE)
+ return dev_err_probe(dev, -EINVAL,
+ "invalid shunt value %d of %pfwP\n",
+ rsense_microOhm, node);
+
+ max34417->input_correction[index] =
+ max34417_calc_input_correction(rsense_microOhm);
+ }
+ }
+
+ 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;
+
+ /*
+ * Reset mode and set accumulator width to Wide
+ *
+ * The MAX34407 variant could be supported by setting the
+ * MAX34417_CONTROL_MODE field to 0 and handling a smaller
+ * accumulator width.
+ */
+ rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, MAX34417_CONTROL_MODE);
You have local variable for regmap, use it.
+ if (rc)
+ return dev_err_probe(dev, rc, "Error writing control register\n");
+
+ /* Reset the accumulators */
+ rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+ if (rc)
+ return rc;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
Thanks,
Neil