Re: [PATCH v6 3/3] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator
From: Neil Armstrong
Date: Fri Nov 06 2026 - 04:33:14 EST
Hi,
On 11/5/26 18:32, Andy Shevchenko wrote:
On Thu, Nov 05, 2026 at 01:42:48PM +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 Joules.
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.
I have a few nit-picks, mostly about the style. The only two seems more
important, actually the very first two (_uOhm suffix and use of a bitmap).
...
+#define MAX34417_MIN_RSENSE (1 * MILLI) /* 1000 microOhm */
+#define MAX34417_MAX_RSENSE (100 * MILLI) /* 100000 microOhm */
+#define MAX34417_DEFAULT_RSENSE (1 * MILLI) /* 1000 microOhm */
Why not adding a unit suffix _uOhm and drop the comments?
Moreover the burden of a typo is now in the comments and in such a case may
confuse the reader.
Right, indeed it's better
...
+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];
+ s64 acc_power[MAX34417_CHANNEL_COUNT];
+ s64 acc_count[MAX34417_CHANNEL_COUNT];
+ bool overflow[MAX34417_CHANNEL_COUNT];
Why not a bitmap?
DECLARE_BITMAP(overflow, MAX34417_CHANNEL_COUNT);
Yes, I'll switch to that
fsleep((1 * USEC_PER_MSEC) - delta_us);
+ ktime_t last_update;
+ bool cam_mode;
+};
...
+static int max34417_check_ovf(struct max34417_data *max34417)
+{
+ struct regmap *map = max34417->regmap;
+ u32 control;
+ int rc;
+
+ rc = regmap_read(map, MAX34417_CONTROL_REG, &control);
+ if (rc)
+ return rc;
+
+ if (!(control & MAX34417_CONTROL_OVF))
+ return 0;
+ rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
+ MAX34417_CONTROL_OVF);
It's precisely a single line
+ if (rc)
+ return rc;
+
+ rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+ if (rc)
+ return rc;
+
+ /* Save the timestamp for the next read */
+ max34417->last_update = ktime_get();
+
+ return 0;
+}
...
+{
+ s64 delta_us = ktime_us_delta(ktime_get(), max34417->last_update);
It's better to split the assignment of the variable (that's going to be
validated) from maintenance p.o.v.
+ struct regmap *map = max34417->regmap;
+ u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
+ int rc;
+
+ /* Sleep up to 1msec to make sure we have a measurement cycle done */
+ if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
+ fsleep((1 * USEC_PER_MSEC) - delta_us);
struct regmap *map = max34417->regmap;
u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
s64 delta_us;
int rc;
/* Sleep up to 1msec to make sure we have a measurement cycle done */
delta_us = ktime_us_delta(ktime_get(), max34417->last_update);
if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
Yeah looks better
+ /*
+ * When not in CAM mode, we use the default mode where each
+ * UPDATE command will move the current accumulated data to the
+ * SMbus registers, reset those accumulators and run a new
+ * measurement cycle.
+ */
+ if (!max34417->cam_mode) {
+ /* Reset the accumulators and perform a measurement cycle */
+ rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+ if (rc)
+ return rc;
+ /*
+ * UPDATE commands should be no less than 1msec apart for reliable
+ * measurements, since in Manual mode the SPS is 1kHz, we need wait
+ * for the measurement to finish before moving the data in the SMbus
+ * registers.
+ */
This comment goes up to 85 characters...
+ fsleep(1 * USEC_PER_MSEC);
+ }
+
+ /*
+ * Move the new UPDATE data into the SMBus read registers.
+ * This will also reset the accumulators, returning the
+ * power accumulated since the last UPDATE.
+ */
...and this one only up to 67. Can you bring some consistency to that?
Same comment to other similar cases all over the code.
Will fix
+ rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+ if (rc)
+ return rc;
+
+ /* Save the timestamp for the next read */
+ max34417->last_update = ktime_get();
+
+ if (max34417->cam_mode) {
+
Redundant blank line.
+ /* Retrieve all channels to avoid missing data for other channels */
+ rc = max34417_save_accumulators(max34417);
+ if (rc)
+ return rc;
+ }
+
+ rc = regmap_noinc_read(map, chan->address, &buf, sizeof(buf));
+ if (rc)
+ return rc;
+
+ /* Ignore first byte which contains the Byte Count & data starts at Bit 2 */
+ *val = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]) >> 2;
+
+ return IIO_VAL_INT;
+}
...
+static int max34417_read_energy(struct max34417_data *max34417,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2)
+{
+ struct regmap *map = max34417->regmap;
+ s64 delta_us = ktime_us_delta(ktime_get(), max34417->last_update);
Split the assignment.
+ s64 acc_power;
+ int rc;
+
+ /* Sleep up to 1msec to make sure we have a measurement cycle done */
+ if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
+ fsleep((1 * USEC_PER_MSEC) - delta_us);
+
+ /* Move the data to the SMBus registers */
+ rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+ if (rc)
+ return rc;
+
+ /* Save the timestamp for the next read */
+ max34417->last_update = ktime_get();
+
+ /* Retrieve all channels to avoid missing data for other channels */
+ rc = max34417_save_accumulators(max34417);
+ if (rc)
+ return rc;
+
+ if (max34417->overflow[chan->channel]) {
+ max34417->overflow[chan->channel] = false;
+ max34417->acc_power[chan->channel] = 0;
+ max34417->acc_count[chan->channel] = 0;
+ return -EOVERFLOW;
+ }
+
+ /* Get current channel power and reset them */
+ acc_power = max34417->acc_power[chan->channel];
+ max34417->acc_power[chan->channel] = 0;
+ max34417->acc_count[chan->channel] = 0;
+
+ iio_val_s64_decompose(acc_power, val, val2);
+
+ return IIO_VAL_INT_64;
+}
...
+static int max34417_probe_channel_node(struct device *dev,
+ struct max34417_data *max34417,
+ struct fwnode_handle *node)
+{
+ u32 rsense_uOhm, index;
+ int rc;
+
+ 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",
+ node);
+ }
Can be
const char *propname;
...
propname = "label";
...
propname = "shunt-resistor-micro-ohms";
+ 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",
...and here you can refer to it in full using %s.
+ node);
+
+ if (rsense_uOhm < MAX34417_MIN_RSENSE ||
+ rsense_uOhm > MAX34417_MAX_RSENSE)
+ return dev_err_probe(dev, -EINVAL,
-ERANGE?
+ "invalid shunt value %d of %pfwP\n",
Same here — use %s and propname.
Good suggestion, will try
Thanks!
Neil
+ rsense_uOhm, node);
+
+ max34417->input_correction[index] =
+ max34417_calc_input_correction(rsense_uOhm);
+ }
+
+ return 0;
+}