Re: [PATCH v2 2/2] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator
From: Neil Armstrong
Date: Fri Sep 25 2026 - 04:46:59 EST
Hi,
On 9/25/26 05:26, Jonathan Cameron wrote:
On Thu, 24 Sep 2026 15:14:16 +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 and accumulated average power over an
I2C/SMBUS serial interface.
Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
A few comments inline. For a new driver I'd wait a week before sending an
update. Whilst you've gotten quite a few reviews already it is good to make
sure any discussion has died down before moving on to the next version.
Thanks,
Jonathan
diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
new file mode 100644
index 000000000000..98d961c5ecee
--- /dev/null
+++ b/drivers/iio/adc/max34417.c
@@ -0,0 +1,374 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IIO driver for Maxim MAX34417 ADC, 4-Channels High Dynamic Range Power Accumulator
+ *
+ * Datasheet: https://www.analog.com/en/products/max34417.html
+ *
+ * TODO: Slow Mode, Continuous Accumulate Mode, Park Feature, Bulk Update, Perr_Verr Correction
+ */
+#define MAX34417_CHANNEL(_index, _v_address, _power_address) \
+ { \
+ .type = IIO_VOLTAGE, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .channel = (_index), \
+ .address = (_v_address), \
+ .indexed = 1, \
+ }, \
+ { \
+ .type = IIO_POWER, \
As below. This smells like it might not be an actual power channel if it
is accumulated on a fixed frequency. It'll be some sort of scaled IIO_ENERGY
channel. If you want to present it as power (which may make sense) then
it may need a little maths.
So as I understand the ENERGY would need to provide Joules. Which would
be doable if we take in account the accumulator sample rate (1024sps) from the
datasheet.
But this implementation tries to provide an initial support following
the MAX34417 datasheet which provides calculation for Average Power (page 18),
this is why I sticked to POWER and IIO_CHAN_INFO_AVERAGE_RAW.
But you're right, knowing the sample rate we could indeed calculate the energy.
I can try to do the math, but with manual updates it may no be very accurate so
the Continuous Accumulate Mode should be implemented to provide
accurate Energy measurements over time and would be enabled via an
IIO_CHAN_INFO_ENABLE.
+ .info_mask_separate = BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .channel = (_index), \
+ .address = (_power_address), \
+ .indexed = 1, \
+ }
+
+static const struct iio_chan_spec max34417_channels[] = {
+ MAX34417_CHANNEL(0, MAX34417_V_CH1_REG, MAX34417_PWR_ACC_1_REG),
+ MAX34417_CHANNEL(1, MAX34417_V_CH2_REG, MAX34417_PWR_ACC_2_REG),
+ MAX34417_CHANNEL(2, MAX34417_V_CH3_REG, MAX34417_PWR_ACC_3_REG),
+ MAX34417_CHANNEL(3, MAX34417_V_CH4_REG, MAX34417_PWR_ACC_4_REG),
+};
+
+/* TODO Implement trigger to update accumulator once and get all channels at once */
+
+static int max34417_accumulator_update(struct max34417_data *max34417)
+{
+ int rc;
+
+ rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+ if (rc) {
+ dev_err(max34417->dev, "Error (%d) writing update register\n", rc);
+ return rc;
+ }
+
+ /* Wait for accumulator update */
+ fsleep(1000);
+
+ return 0;
+}
+
+static int max34417_read_voltage(struct max34417_data *max34417,
+ const struct iio_chan_spec *chan, int *val)
+{
+ uint16_t voltage;
+ uint8_t buf[3];
+ int rc;
+
+ guard(mutex)(&max34417->lock);
+
+ rc = max34417_accumulator_update(max34417);
+ if (rc)
+ return rc;
+
+ rc = regmap_noinc_read(max34417->regmap, chan->address, &buf, 3);
+ if (rc)
+ return rc;
+
+ voltage = buf[2] | ((uint64_t)buf[1] << 8);
get_unaligned_be16();
+ voltage >>= 2;Kernel types so u32, u64, u8
+
+ *val = voltage;
+
+ return IIO_VAL_INT;
+}
+
+static int max34417_read_power(struct max34417_data *max34417,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2)
+{
+ uint32_t acc_count;
+ uint64_t power;
+ uint8_t buf[8];
+ int rc;
+
+ guard(mutex)(&max34417->lock);
+
+ rc = max34417_accumulator_update(max34417);
+ if (rc)
+ return rc;
+
+ rc = regmap_noinc_read(max34417->regmap, MAX34417_ACC_COUNT_REG,
+ &buf, 4);
+ if (rc)
+ return rc;
+
+ acc_count = buf[3] | ((uint64_t)buf[2] << 8) | ((uint64_t)buf[1] << 16);
get_unaligned_be24(buf);
+ if (!acc_count)
+ return -EIO;
+
+ rc = regmap_noinc_read(max34417->regmap, chan->address, &buf, 8);
+ if (rc)
+ return rc;
+
+ power = buf[7];
+ power |= ((uint64_t)buf[6] << 8UL);
+ power |= ((uint64_t)buf[5] << 16UL);
+ power |= ((uint64_t)buf[4] << 24UL);
+ power |= ((uint64_t)buf[3] << 32UL);
+ power |= ((uint64_t)buf[2] << 40UL);
+ power |= ((uint64_t)buf[1] << 48UL);
Hmm. i think this is the second 56 bit endian reader we've had
recently. Time to add get_unaligned_be56()
Indeed
+To reduce indent I'd flip it
+ power = div_u64(power, acc_count);
+
+ *val = FIELD_GET(GENMASK_ULL(31, 0), power);
+ *val2 = FIELD_GET(GENMASK_ULL(55, 32), power);
+
+ return IIO_VAL_INT_64;
+}
+
+static int max34417_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct max34417_data *max34417 = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (chan->type == IIO_VOLTAGE)
if (chan->type != IIO_VOLTAGE)
return -EINVAL;
+ return max34417_read_voltage(max34417, chan, val);
+
+ return -EINVAL;
+ case IIO_CHAN_INFO_AVERAGE_RAW:
+ if (chan->type == IIO_POWER)
+ return max34417_read_power(max34417, chan, val, val2);
+
+ return -EINVAL;
+ case IIO_CHAN_INFO_SCALE:
+ if (chan->type == IIO_VOLTAGE) {
+ /* Scale to mA */
On a voltage channel? That is unlikely to be correct.
Indeed
+ *val = MAX34417_VOLTAGE_CORRECTION_SCALE * MILLI;
+ *val2 = MAX34417_VOLTAGE_FULL_SCALE_BITS;
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+ } else if (chan->type == IIO_POWER) {
Actually power or accumulated power (otherwise known as energy!)
+ /* Scale to mW */
+ *val = max34417->input_correction[chan->channel] * MILLI;
+ *val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+ }
+
+ return -EINVAL;
+ default:
+ return -EINVAL;
+ }
+}
+For new code
+static unsigned int max34417_calc_input_correction(u32 rsense)
+{
+ /* (100 milliOhm / rsense) * MAX34417_PWR_CORRECTION_SCALE */
+ return (100 * MILLI * MAX34417_PWR_CORRECTION_SCALE) / rsense;
+}
+
+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;
+ mutex_init(&max34417->lock);
ret = devm_mutex_init(...)
if (ret)
return ret;
Brings some debug logic in which might be a little bit useful to someone
and it's cheap to do.
+for (unsigned int i = 0; .... i++)
+ /* 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_read_u32(node, "reg", &index))
+ return dev_err_probe(dev, -EINVAL, "missing reg property of %pfwP\n",
+ node);
returned, so no need to chase with an else.
+ else if (index >= MAX34417_CHANNEL_COUNT)For optional properties, we generally now check for them first then if the property is
+ return dev_err_probe(dev, -EINVAL, "invalid reg %d of %pfwP\n",
+ index, node);
+
+ fwnode_property_read_string(node, "label", &max34417->input_label[index]);
+
+ rc = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms", &rsense);
there can make errors reasons to fail
Will switch to that
if (fwnode_property_present()) {
rc = fwnode_property_read_u32();
if (rc)
return dev_err_probe();
etc
+ if (!rc) {
+ if (!rsense || rsense < 1000 || rsense > 100000)
+ 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;
+
+ /* Set as default Manual Mode & Wide ADC */
+ rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, MAX34417_DEFAULT_CMM_WIDE);
+ if (rc)
+ return dev_err_probe(max34417->dev, rc, "Error writing control register\n");
+
+ return devm_iio_device_register(dev, indio_dev);
+}
Thanks,
Neil