[PATCH 0/2] iio: adc: add support for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[PATCH 0/2] iio: adc: add support for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Neil Armstrong]

The MAX34417 is a specialized current and voltage monitor used to determine
power consumption of portable systems. The driver support getting the channel
voltage and accumulated average power over an I2C/SMBUS serial interface.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Neil Armstrong (2):
      dt-bindings: iio: add: document the MAX34417 Four-Channel High Dynamic Range Power Accumulator
      iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

 .../bindings/iio/adc/maxim,max34417.yaml           |  99 ++++++
 drivers/iio/adc/Kconfig                            |  11 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/max34417.c                         | 356 +++++++++++++++++++++
 4 files changed, 467 insertions(+)
---
base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
change-id: 20260923-topic-sm8x50-iio-max34417-adc-209e880533fb

Best regards,
--  
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 1/2] dt-bindings: iio: add: document the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Neil Armstrong]

Document the Maxim MAX34417 Four-Channel High Dynamic Range Power
Accumulator used to monitor power consumption of portable systems.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 .../bindings/iio/adc/maxim,max34417.yaml           | 99 ++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max34417.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max34417.yaml
new file mode 100644
index 000000000000..6f2339a92771
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34417.yaml
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/maxim,max34417.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX34417 Four-Channel High Dynamic Range Power Accumulator
+
+maintainers:
+  - Neil Armstrong <neil.armstrong@linaro.org>
+
+description: |
+  The MAX34417 is a specialized current and voltage monitor used to determine
+  power consumption of portable systems. The device has a very wide dynamic
+  range (20,000:1) that allows for the accurate measurement of power in such
+  systems. The device is configured and monitored with a standard I2C/SMBus
+  serial interface. The unidirectional current sensor offers precision
+  high-side operation with a low full-scale sense voltage.
+
+  Specifications about the device can be found at:
+  https://www.analog.com/media/en/technical-documentation/data-sheets/max34417.pdf
+
+properties:
+  compatible:
+    const: maxim,max34417
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+  vio-supply: true
+
+patternProperties:
+  "^channel@[0-3]$":
+    $ref: adc.yaml
+    type: object
+    description:
+      Represents the internal channels of the sensor.
+
+    properties:
+      reg:
+        items:
+          - minimum: 0
+            maximum: 3
+
+      label: true
+
+      maxim,rsense-val-micro-ohms:
+        description:
+          Adjust the Rsense value to the resistor value
+        enum: [1000, 2000, 4000, 5000, 10000, 15000, 20000, 25000, 40000, 50000, 100000]
+        default: 1000
+
+    required:
+      - reg
+      - maxim,rsense-val-micro-ohms
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@10 {
+              compatible = "maxim,max34417";
+              reg = <0x10>;
+
+              #address-cells = <1>;
+              #size-cells = <0>;
+
+              channel@0 {
+                  reg = <0>;
+                  label = "ch0";
+                  maxim,rsense-val-micro-ohms = <5000>;
+              };
+
+              channel@1 {
+                  reg = <1>;
+                  label = "ch1";
+                  maxim,rsense-val-micro-ohms = <10000>;
+             };
+        };
+    };

-- 
2.34.1

[PATCH 2/2] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Neil Armstrong]

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@linaro.org>
---
 drivers/iio/adc/Kconfig    |  11 ++
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max34417.c | 356 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 368 insertions(+)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 415e519ad4eb..4c559bafd1ce 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1116,6 +1116,17 @@ config MAX34408
 	  To compile this driver as a module, choose M here: the module will be
 	  called max34408.
 
+config MAX34417
+	tristate "Maxim max34417 ADC driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say yes here to build ADC support for Maxim max34417 Four-Channel High
+	  Dynamic Range Power Accumulator.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max34417.
+
 config MAX77541_ADC
 	tristate "Analog Devices MAX77541 ADC driver"
 	depends on MFD_MAX77541
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index dcec0abb03b7..c667e7ecbc53 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MAX14001) += max14001.o
 obj-$(CONFIG_MAX34408) += max34408.o
+obj-$(CONFIG_MAX34417) += max34417.o
 obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
 obj-$(CONFIG_MAX9611) += max9611.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
new file mode 100644
index 000000000000..004401de6ce7
--- /dev/null
+++ b/drivers/iio/adc/max34417.c
@@ -0,0 +1,356 @@
+// 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
+ */
+
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+
+#define MAX34417_UPDATE_REG		0x0
+#define MAX34417_CONTROL_REG		0x1
+#define MAX34417_ACC_COUNT_REG		0x2
+
+#define MAX34417_PWR_ACC_1_REG		0x3
+#define MAX34417_PWR_ACC_2_REG		0x4
+#define MAX34417_PWR_ACC_3_REG		0x5
+#define MAX34417_PWR_ACC_4_REG		0x6
+
+#define MAX34417_V_CH1_REG		0x7
+#define MAX34417_V_CH2_REG		0x8
+#define MAX34417_V_CH3_REG		0x9
+#define MAX34417_V_CH4_REG		0xa
+
+#define MAX34417_DID_REG		0xf
+
+#define MAX34417_BULK_POWER_READOUT_REG		0x10
+#define MAX34417_BULK_VOLTAGE_READOUT_REG	0x11
+
+#define MAX34417_BULK_UPDATE_ADDRESS	0x2c
+#define MAX34417_BULK_UPDATE_REG	0x0
+
+/* Bit masks for control register */
+#define MAX34417_CONTROL_OVF		BIT(0)
+#define MAX34417_CONTROL_SLOW		BIT(1)
+#define MAX34417_CONTROL_PARK0		BIT(2)
+#define MAX34417_CONTROL_PARK1		BIT(3)
+#define MAX34417_CONTROL_PARK_EN	BIT(4)
+#define MAX34417_CONTROL_SMM		BIT(5)
+#define MAX34417_CONTROL_CAM		BIT(6)
+#define MAX34417_CONTROL_MODE		BIT(7)
+
+#define MAX34417_DEFAULT_CMM_WIDE	(MAX34417_CONTROL_MODE | MAX34417_CONTROL_SMM)
+
+#define MAX34417_DEFAULT_RSENSE		1000
+
+#define MAX34417_PWR_CORRECTION_SCALE		24
+#define MAX34417_PWR_AVG_FULL_SCALE_BITS	30
+
+#define MAX34417_VOLTAGE_CORRECTION_SCALE	24
+#define MAX34417_VOLTAGE_FULL_SCALE_BITS	14
+
+#define MAX34417_CHANNEL_COUNT			4
+
+/**
+ * struct max34417_data - max34417 specific data.
+ * @regmap:	device register map.
+ * @dev:	max34417 device.
+ * @lock:	lock for protecting access to device hardware registers, mostly
+ *		for reading common accumulator count and control register.
+ * @input_correction:	Correction based on the Rsense value from channel nodes.
+ * @input_label:	Channel label from channel nodes.
+ */
+struct max34417_data {
+	struct regmap *regmap;
+	struct device *dev;
+	struct mutex lock;
+	u32 input_correction[MAX34417_CHANNEL_COUNT];
+	const char *input_label[MAX34417_CHANNEL_COUNT];
+};
+
+static const struct regmap_config max34417_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= MAX34417_DID_REG,
+};
+
+#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,				\
+		.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 */
+	usleep_range(1000, 2000);
+
+	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);
+	voltage >>= 2;
+
+	*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);
+
+	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);
+
+	power = div_u64(power, acc_count);
+
+	*val = FIELD_GET(GENMASK(31, 0), power);
+	*val2 = FIELD_GET(GENMASK(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)
+			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 */
+			*val = MAX34417_VOLTAGE_CORRECTION_SCALE * MILLI;
+			*val2 = MAX34417_VOLTAGE_FULL_SCALE_BITS;
+
+			return IIO_VAL_FRACTIONAL_LOG2;
+		} else if (chan->type == IIO_POWER) {
+			/* 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;
+	}
+}
+
+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 (chan->type == IIO_VOLTAGE) {
+		if (input_label)
+			return sysfs_emit(label, "%s-voltage\n", input_label);
+		return sysfs_emit(label, "channel%d-voltage\n", chan->channel);
+	}
+
+	if (chan->type == IIO_POWER) {
+		if (input_label)
+			return sysfs_emit(label, "%s-power\n", input_label);
+		return sysfs_emit(label, "channel%d-power\n", chan->channel);
+	}
+
+	return 0;
+}
+
+static const struct iio_info max34417_info = {
+	.read_raw	= max34417_read_raw,
+	.read_label	= max34417_read_label,
+};
+
+static int max34417_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct max34417_data *max34417;
+	struct fwnode_handle *node;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	int rc;
+
+	regmap = devm_regmap_init_i2c(client, &max34417_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err_probe(dev, PTR_ERR(regmap),
+			      "regmap_init failed\n");
+		return PTR_ERR(regmap);
+	}
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*max34417));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	/* Get and enable regulators */
+	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);
+
+	device_for_each_child_node(dev, node) {
+		unsigned int correction;
+		u32 rsense, index;
+
+		if (fwnode_property_read_u32(node, "reg", &index)) {
+			dev_err(dev, "missing reg property of %pfwP\n", node);
+			return -EINVAL;
+		} else if (index >= MAX34417_CHANNEL_COUNT) {
+			dev_err(dev, "invalid reg %d of %pfwP\n", index, node);
+			return -EINVAL;
+		}
+
+		fwnode_property_read_string(node, "label", &max34417->input_label[index]);
+		if (fwnode_property_read_u32(node, "maxim,rsense-val-micro-ohms", &rsense))
+			rsense = MAX34417_DEFAULT_RSENSE;
+
+		/* (100 milliOhm / rsense) * MAX34417_PWR_CORRECTION_SCALE */
+		correction = (100 * MILLI * MAX34417_PWR_CORRECTION_SCALE) / rsense;
+
+		max34417->input_correction[index] = correction;
+	}
+
+	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) {
+		dev_err(max34417->dev, "Error (%d) writing control register\n", rc);
+		return rc;
+	}
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id max34417_of_match[] = {
+	{ .compatible = "maxim,max34417" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max34417_of_match);
+
+static const struct i2c_device_id max34417_id[] = {
+	{ .name = "max34417" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max34417_id);
+
+static struct i2c_driver max34417_driver = {
+	.driver = {
+		.name   = "max34417",
+		.of_match_table = max34417_of_match,
+	},
+	.probe = max34417_probe,
+	.id_table = max34417_id,
+};
+module_i2c_driver(max34417_driver);
+
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
+MODULE_DESCRIPTION("Maxim MAX34417 ADC driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH 2/2] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Joshua Crofts]

On Wed, 23 Sep 2026 21:10:23 +0200
Neil Armstrong <neil.armstrong@linaro.org> 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@linaro.org>
> ---

Hi Neil,

Several comments inline.

Josh

>  drivers/iio/adc/Kconfig    |  11 ++
>  drivers/iio/adc/Makefile   |   1 +
>  drivers/iio/adc/max34417.c | 356 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 368 insertions(+)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 415e519ad4eb..4c559bafd1ce 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1116,6 +1116,17 @@ config MAX34408
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max34408.
>  
> +config MAX34417
> +	tristate "Maxim max34417 ADC driver"

MAX34417

> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to build ADC support for Maxim max34417 Four-Channel High
> +	  Dynamic Range Power Accumulator.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max34417.
> +

...

> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/units.h>
> +

+ array_size.h, bits.h, err.h, math64.h, mutex.h, property.h, regulator/consumer.h,
sysfs.h, types.h

> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>

No need for this one, it's already in iio.h.

> +
> +#define MAX34417_UPDATE_REG		0x0
> +#define MAX34417_CONTROL_REG		0x1
> +#define MAX34417_ACC_COUNT_REG		0x2
> +

...

> +
> +/**
> + * struct max34417_data - max34417 specific data.
> + * @regmap:	device register map.
> + * @dev:	max34417 device.
> + * @lock:	lock for protecting access to device hardware registers, mostly

Nit-picking, but... Device, MAX34417, Lock.

> + *		for reading common accumulator count and control register.
> + * @input_correction:	Correction based on the Rsense value from channel nodes.
> + * @input_label:	Channel label from channel nodes.
> + */
> +struct max34417_data {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct mutex lock;
> +	u32 input_correction[MAX34417_CHANNEL_COUNT];
> +	const char *input_label[MAX34417_CHANNEL_COUNT];
> +};
> +
> +static const struct regmap_config max34417_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= MAX34417_DID_REG,
> +};
> +

...

> +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 */
> +	usleep_range(1000, 2000);

fsleep(1000) would be better.

> +
> +	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);
> +	voltage >>= 2;
> +
> +	*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)

The alignment is off here. Additionally, put val and val2 on a separate line.

> +{
> +	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);

Maybe a check that acc_count isn't zero in case of a bad read etc.?
It could cause a divide-by-zero error later.

> +
> +	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);
> +
> +	power = div_u64(power, acc_count);
> +
> +	*val = FIELD_GET(GENMASK(31, 0), power);
> +	*val2 = FIELD_GET(GENMASK(55, 32), power);

Sashiko points out: 

Will this build correctly on 32-bit architectures?

Since GENMASK() operates on unsigned long, which is 32 bits wide on those
platforms, attempting to mask bits 55..32 might result in an out-of-bounds
shift and trigger compiler errors or warnings like BUILD_BUG_ON_ZERO. Should
GENMASK_ULL(55, 32) be used instead?

> +
> +	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)
> +			return max34417_read_voltage(max34417, chan, val);
> +		return -EINVAL;

+ Blank line.

> +	case IIO_CHAN_INFO_AVERAGE_RAW:
> +		if (chan->type == IIO_POWER)
> +			return max34417_read_power(max34417, chan, val, val2);

+ Blank line.

> +		return -EINVAL;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_VOLTAGE) {
> +			/* Scale to mA */
> +			*val = MAX34417_VOLTAGE_CORRECTION_SCALE * MILLI;
> +			*val2 = MAX34417_VOLTAGE_FULL_SCALE_BITS;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		} else if (chan->type == IIO_POWER) {
> +			/* Scale to mW */
> +			*val = max34417->input_correction[chan->channel] * MILLI;
> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		}

+ Blank line.

> +		return -EINVAL;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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 (chan->type == IIO_VOLTAGE) {
> +		if (input_label)
> +			return sysfs_emit(label, "%s-voltage\n", input_label);

+ Blank line.

> +		return sysfs_emit(label, "channel%d-voltage\n", chan->channel);
> +	}
> +
> +	if (chan->type == IIO_POWER) {
> +		if (input_label)
> +			return sysfs_emit(label, "%s-power\n", input_label);

+ Blank line.

> +		return sysfs_emit(label, "channel%d-power\n", chan->channel);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct iio_info max34417_info = {
> +	.read_raw	= max34417_read_raw,
> +	.read_label	= max34417_read_label,
> +};
> +
> +static int max34417_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct max34417_data *max34417;
> +	struct fwnode_handle *node;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int rc;
> +
> +	regmap = devm_regmap_init_i2c(client, &max34417_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err_probe(dev, PTR_ERR(regmap),
> +			      "regmap_init failed\n");

One line is okay.

> +		return PTR_ERR(regmap);
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*max34417));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	/* Get and enable regulators */

Redundant comment IMO.

> +	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);
> +
> +	device_for_each_child_node(dev, node) {

scoped variant would be better, Sashiko points out this could leak on early
exit.

> +		unsigned int correction;
> +		u32 rsense, index;
> +
> +		if (fwnode_property_read_u32(node, "reg", &index)) {
> +			dev_err(dev, "missing reg property of %pfwP\n", node);
> +			return -EINVAL;
> +		} else if (index >= MAX34417_CHANNEL_COUNT) {
> +			dev_err(dev, "invalid reg %d of %pfwP\n", index, node);
> +			return -EINVAL;
> +		}
> +

Why not dev_err_probe()?

> +		fwnode_property_read_string(node, "label", &max34417->input_label[index]);
> +		if (fwnode_property_read_u32(node, "maxim,rsense-val-micro-ohms", &rsense))
> +			rsense = MAX34417_DEFAULT_RSENSE;
> +
> +		/* (100 milliOhm / rsense) * MAX34417_PWR_CORRECTION_SCALE */
> +		correction = (100 * MILLI * MAX34417_PWR_CORRECTION_SCALE) / rsense;
> +
> +		max34417->input_correction[index] = correction;
> +	}
> +
> +	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) {
> +		dev_err(max34417->dev, "Error (%d) writing control register\n", rc);

dev_err_probe()? + you can u

-- 
Kind regards,
Joshua Crofts

Re: [PATCH 2/2] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Andy Shevchenko]

On Thu, Sep 24, 2026 at 09:54:57AM +0200, Joshua Crofts wrote:
> On Wed, 23 Sep 2026 21:10:23 +0200
> Neil Armstrong <neil.armstrong@linaro.org> wrote:

Joshua, below also something to you to pay attention to on top of the good
parts you covered already.

...

> > +/**
> > + * struct max34417_data - max34417 specific data.
> > + * @regmap:	device register map.
> > + * @dev:	max34417 device.
> > + * @lock:	lock for protecting access to device hardware registers, mostly
> 
> Nit-picking, but... Device, MAX34417, Lock.

Generally speaking it should be consistent with whatever style is being chosen.
If we go with the first capitalized letter, then yes, otherwise below should go
to small first letter. In any case MAX part number should be capitalized (or
someone might think of it as struct max34417).

> > + *		for reading common accumulator count and control register.
> > + * @input_correction:	Correction based on the Rsense value from channel nodes.
> > + * @input_label:	Channel label from channel nodes.
> > + */

...

> > +static int max34417_read_voltage(struct max34417_data *max34417,
> > +				 const struct iio_chan_spec *chan, int *val)
> > +{
> > +	uint16_t voltage;
> > +	uint8_t buf[3];

uXX types, please. Everywhere.

> > +	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);

sizeof()

> > +	if (rc)
> > +		return rc;
> > +
> > +	voltage = buf[2] | ((uint64_t)buf[1] << 8);
> > +	voltage >>= 2;

Not sure what is in the buf[0], but verbatim the above is get_unaligned_le16().

> > +	*val = voltage;
> > +
> > +	return IIO_VAL_INT;
> > +}

...

> > +	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);

get_unaligned_be64() / be64_to_cpu().

...

> > +		fwnode_property_read_string(node, "label", &max34417->input_label[index]);
> > +		if (fwnode_property_read_u32(node, "maxim,rsense-val-micro-ohms", &rsense))
> > +			rsense = MAX34417_DEFAULT_RSENSE;

What if the property is there, but some issue has happened?
We have an idiomatic

	if (_property_present()) {
		rc = _property_read();
		if (rc)
			return ...rc...;
		...
	} else {
		...apply default...
	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Joshua Crofts]

On Thu, 24 Sep 2026 17:20:07 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Sep 24, 2026 at 09:54:57AM +0200, Joshua Crofts wrote:
> > On Wed, 23 Sep 2026 21:10:23 +0200
> > Neil Armstrong <neil.armstrong@linaro.org> wrote:  
> 
> Joshua, below also something to you to pay attention to on top of the good
> parts you covered already.

Thanks! Unfortunately a v2 already popped up on the list today...

-- 
Kind regards,
Joshua Crofts

Re: [PATCH 2/2] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Neil Armstrong]

Hi,

On 9/24/26 17:12, Joshua Crofts wrote:
> On Thu, 24 Sep 2026 17:20:07 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
>> On Thu, Sep 24, 2026 at 09:54:57AM +0200, Joshua Crofts wrote:
>>> On Wed, 23 Sep 2026 21:10:23 +0200
>>> Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>
>> Joshua, below also something to you to pay attention to on top of the good
>> parts you covered already.
> 
> Thanks! Unfortunately a v2 already popped up on the list today...
> 

Just ignore my V2, I'll apply Andy's suggestions in a v3.

Thanks,
Neil

Re: [PATCH 2/2] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Neil Armstrong]

On 9/24/26 16:20, Andy Shevchenko wrote:
> On Thu, Sep 24, 2026 at 09:54:57AM +0200, Joshua Crofts wrote:
>> On Wed, 23 Sep 2026 21:10:23 +0200
>> Neil Armstrong <neil.armstrong@linaro.org> wrote:
> 
> Joshua, below also something to you to pay attention to on top of the good
> parts you covered already.
> 
> ...
> 
>>> +/**
>>> + * struct max34417_data - max34417 specific data.
>>> + * @regmap:	device register map.
>>> + * @dev:	max34417 device.
>>> + * @lock:	lock for protecting access to device hardware registers, mostly
>>
>> Nit-picking, but... Device, MAX34417, Lock.
> 
> Generally speaking it should be consistent with whatever style is being chosen.
> If we go with the first capitalized letter, then yes, otherwise below should go
> to small first letter. In any case MAX part number should be capitalized (or
> someone might think of it as struct max34417).
> 
>>> + *		for reading common accumulator count and control register.
>>> + * @input_correction:	Correction based on the Rsense value from channel nodes.
>>> + * @input_label:	Channel label from channel nodes.
>>> + */
> 
> ...
> 
>>> +static int max34417_read_voltage(struct max34417_data *max34417,
>>> +				 const struct iio_chan_spec *chan, int *val)
>>> +{
>>> +	uint16_t voltage;
>>> +	uint8_t buf[3];
> 
> uXX types, please. Everywhere.
> 
>>> +	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);
> 
> sizeof()
> 
>>> +	if (rc)
>>> +		return rc;
>>> +
>>> +	voltage = buf[2] | ((uint64_t)buf[1] << 8);
>>> +	voltage >>= 2;
> 
> Not sure what is in the buf[0], but verbatim the above is get_unaligned_le16().
> 
>>> +	*val = voltage;
>>> +
>>> +	return IIO_VAL_INT;
>>> +}
> 
> ...
> 
>>> +	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);
> 
> get_unaligned_be64() / be64_to_cpu().

Will switch to get_unaligned_xx()

> 
> ...
> 
>>> +		fwnode_property_read_string(node, "label", &max34417->input_label[index]);
>>> +		if (fwnode_property_read_u32(node, "maxim,rsense-val-micro-ohms", &rsense))
>>> +			rsense = MAX34417_DEFAULT_RSENSE;
> 
> What if the property is there, but some issue has happened?
> We have an idiomatic
> 
> 	if (_property_present()) {
> 		rc = _property_read();
> 		if (rc)
> 			return ...rc...;
> 		...
> 	} else {
> 		...apply default...
> 	}
> 

Will switch to that

Thanks,
Neil