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

[PATCH v2 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>
---
Changes in v2:
- switch to shunt-resistor-micro-ohms no more required
- removed gpio.h from example
- Fixed max34417->MAX34417 in Kconfig and comments
- Added missing includes and remove unneeded
- Fixed typos in comments
- Switched to fsleep()
- Better aligned max34417_read_power declararation
- Handled 0 acc_count
- Switched to GENMASK_ULL() for 32bits systems
- Added missing empty lines
- Moved the input correction into a helper
- Set default input correction for all channels
- Switched to dev_err_probe() to return from probe
- Switched to device_for_each_child_node_scoped()
- Handled invalid shunt-resistor-micro-ohms value
- Link to v1: https://patch.msgid.link/20260923-topic-sm8x50-iio-max34417-adc-v1-0-41d4ba1bfc41@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           | 100 ++++++
 drivers/iio/adc/Kconfig                            |  11 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/max34417.c                         | 374 +++++++++++++++++++++
 4 files changed, 486 insertions(+)
---
base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
change-id: 20260923-topic-sm8x50-iio-max34417-adc-209e880533fb

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

[PATCH v2 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           | 100 +++++++++++++++++++++
 1 file changed, 100 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..058af61cc04e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34417.yaml
@@ -0,0 +1,100 @@
+# 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
+
+      shunt-resistor-micro-ohms:
+        description:
+          Value in micro Ohms of the shunt resistor connected between the RS+ and RS- inputs.
+        minimum: 1000
+        maximum: 100000
+        default: 1000
+
+    required:
+      - reg
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    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";
+                  shunt-resistor-micro-ohms = <5000>;
+              };
+
+              channel@1 {
+                  reg = <1>;
+                  label = "ch1";
+             };
+
+              channel@2 {
+                  reg = <2>;
+             };
+        };
+    };

-- 
2.34.1

[PATCH v2 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 | 374 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 386 insertions(+)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 415e519ad4eb..15360c78d3e5 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..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
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/init.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.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 */
+	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);
+	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);
+	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);
+
+	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)
+			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 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);
+
+	/* 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);
+		else if (index >= MAX34417_CHANNEL_COUNT)
+			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);
+		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);
+}
+
+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 v2 2/2] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[Joshua Crofts]

On Thu, 24 Sep 2026 15:14:16 +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>
> ---

Unfortunately you were too fast and there was another reply on v1 :(

-- 
Kind regards,
Joshua Crofts

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3931 bytes --]

On Thu, Sep 24, 2026 at 03:14:15PM +0200, Neil Armstrong wrote:
> 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           | 100 +++++++++++++++++++++
>  1 file changed, 100 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..058af61cc04e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34417.yaml
> @@ -0,0 +1,100 @@
> +# 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

ADI wants people to use their prefix for newly added devices,
particularly if their release is after the maxim purchase.

> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1

Atypical ordering, usually reg is before these cells props.

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

This doesn't do anything, when you have unevalatedProperties: false.
If these are the only valid properties, use additionalProperties: false
instead. Otherwise, you can just remove this line.

> +
> +      shunt-resistor-micro-ohms:
> +        description:
> +          Value in micro Ohms of the shunt resistor connected between the RS+ and RS- inputs.
> +        minimum: 1000
> +        maximum: 100000
> +        default: 1000
> +
> +    required:
> +      - reg
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg

Typically in IIO, supplies are made required unless the device can
operate without them.

Cheers,
Conor.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    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";
> +                  shunt-resistor-micro-ohms = <5000>;
> +              };
> +
> +              channel@1 {
> +                  reg = <1>;
> +                  label = "ch1";
> +             };
> +
> +              channel@2 {
> +                  reg = <2>;
> +             };
> +        };
> +    };
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Jonathan Cameron]

On Thu, 24 Sep 2026 15:14:14 +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 channel
> voltage and accumulated average power over an I2C/SMBUS serial interface.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

Hi Neil,

As pointed out too fast for a v2.  You aren't new upstream so
that shouldn't come as a surprise!

Secondly for devices that are all about monitoring power supplies
etc we always ask for a clear statement of why IIO rather than
hwmon + to CC at least the maintainer and often the hwmon list.

There are various valid reasons for that choice, but it is good
if they are clearly stated for discussion.

Thanks,

Jonathan

> ---
> Changes in v2:
> - switch to shunt-resistor-micro-ohms no more required
> - removed gpio.h from example
> - Fixed max34417->MAX34417 in Kconfig and comments
> - Added missing includes and remove unneeded
> - Fixed typos in comments
> - Switched to fsleep()
> - Better aligned max34417_read_power declararation
> - Handled 0 acc_count
> - Switched to GENMASK_ULL() for 32bits systems
> - Added missing empty lines
> - Moved the input correction into a helper
> - Set default input correction for all channels
> - Switched to dev_err_probe() to return from probe
> - Switched to device_for_each_child_node_scoped()
> - Handled invalid shunt-resistor-micro-ohms value
> - Link to v1: https://patch.msgid.link/20260923-topic-sm8x50-iio-max34417-adc-v1-0-41d4ba1bfc41@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           | 100 ++++++
>  drivers/iio/adc/Kconfig                            |  11 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/max34417.c                         | 374 +++++++++++++++++++++
>  4 files changed, 486 insertions(+)
> ---
> base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
> change-id: 20260923-topic-sm8x50-iio-max34417-adc-209e880533fb
> 
> Best regards,
> --  
> Neil Armstrong <neil.armstrong@linaro.org>
> 

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

[Jonathan Cameron]

On Thu, 24 Sep 2026 15:14:16 +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>

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.

> +		.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;
> +
> +	*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];
Kernel types so u32, u64, u8

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

> +
> +	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)
To reduce indent I'd flip it
		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.

> +			*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;
> +	}
> +}


> +
> +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);
For new code
	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.

> +
> +	/* Set default input correction for all channels */
> +	for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i)
	for (unsigned int i = 0; .... 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)
> +			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);
For optional properties, we generally now check for them first then if the property is
there can make errors reasons to fail

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

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

[Neil Armstrong]

Hi Jonathan,

On 9/25/26 05:13, Jonathan Cameron wrote:
> On Thu, 24 Sep 2026 15:14:14 +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 channel
>> voltage and accumulated average power over an I2C/SMBUS serial interface.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> 
> Hi Neil,
> 
> As pointed out too fast for a v2.  You aren't new upstream so
> that shouldn't come as a surprise!

Yeah sorry, but with the initial feedback v1 was far form beeing
acceptable, will adjust timings for next versions.

> 
> Secondly for devices that are all about monitoring power supplies
> etc we always ask for a clear statement of why IIO rather than
> hwmon + to CC at least the maintainer and often the hwmon list.

Sure, thanks for the suggestion.

> 
> There are various valid reasons for that choice, but it is good
> if they are clearly stated for discussion.

I don't honestly have an strong opinion on that, for me IIO offers
much more options to retrieve data from the sensor and adding the
complex feature offered. The IIO triggers for example would
perfectly match with the bulk readout we coulnd't implement with
the hwmon API.

Thanks,
Neil

> 
> Thanks,
> 
> Jonathan
> 
>> ---
>> Changes in v2:
>> - switch to shunt-resistor-micro-ohms no more required
>> - removed gpio.h from example
>> - Fixed max34417->MAX34417 in Kconfig and comments
>> - Added missing includes and remove unneeded
>> - Fixed typos in comments
>> - Switched to fsleep()
>> - Better aligned max34417_read_power declararation
>> - Handled 0 acc_count
>> - Switched to GENMASK_ULL() for 32bits systems
>> - Added missing empty lines
>> - Moved the input correction into a helper
>> - Set default input correction for all channels
>> - Switched to dev_err_probe() to return from probe
>> - Switched to device_for_each_child_node_scoped()
>> - Handled invalid shunt-resistor-micro-ohms value
>> - Link to v1: https://patch.msgid.link/20260923-topic-sm8x50-iio-max34417-adc-v1-0-41d4ba1bfc41@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           | 100 ++++++
>>   drivers/iio/adc/Kconfig                            |  11 +
>>   drivers/iio/adc/Makefile                           |   1 +
>>   drivers/iio/adc/max34417.c                         | 374 +++++++++++++++++++++
>>   4 files changed, 486 insertions(+)
>> ---
>> base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
>> change-id: 20260923-topic-sm8x50-iio-max34417-adc-209e880533fb
>>
>> Best regards,
>> --
>> Neil Armstrong <neil.armstrong@linaro.org>
>>
> 

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

[Neil Armstrong]

Hi,

On 9/25/26 05:26, Jonathan Cameron wrote:
> On Thu, 24 Sep 2026 15:14:16 +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>
> 
> 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;
>> +
>> +	*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];
> Kernel types so u32, u64, u8
> 
>> +	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

> 
>> +
>> +	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)
> To reduce indent I'd flip it
> 		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;
>> +	}
>> +}
> 
> 
>> +
>> +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);
> For new code
> 	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.
> 
>> +
>> +	/* Set default input correction for all channels */
>> +	for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i)
> 	for (unsigned int i = 0; .... 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)
>> +			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);
> For optional properties, we generally now check for them first then if the property is
> 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