[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] dt-bindings: iio: use of vendor-specific rsense property instead of standard
- [Low] dt-bindings: iio: unnecessary gpio.h header include in example
- [Low] dt-bindings: iio: contradictory default and required constraints
--

commit 83b0b12b383dfd05c3631355160ac1fe40dc6802
Author: Neil Armstrong <neil.armstrong@linaro.org>

dt-bindings: iio: add: document the MAX34417 Four-Channel High Dynamic Range Power Accumulator

This commit documents the MAX34417 power accumulator device tree bindings.
The device uses an I2C interface to monitor power consumption in portable
systems.

> 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 0000000000000..6f2339a927710
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34417.yaml
[ ... ]
> +    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]

[Severity: Low]
Does this binding need the vendor-specific maxim,rsense-val-micro-ohms
property for the shunt resistor? The standard shunt-resistor-micro-ohms
property is well-established across multiple kernel subsystems for this
exact purpose and might be a better fit.

Additionally, is the hardcoded enum constraint necessary? It artificially
limits the possible resistor values, whereas physical shunt resistors can be
of any resistance.

> +        default: 1000
> +
> +    required:
> +      - reg
> +      - maxim,rsense-val-micro-ohms

[Severity: Low]
Can the default fallback ever be utilized for maxim,rsense-val-micro-ohms?
The property is defined with a default of 1000, but is simultaneously listed
in the required array for the subnode. Because it is required, schema
validation will fail if the property is missing in a device tree, making the
default logically dead.

[ ... ]
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>

[Severity: Low]
Is the dt-bindings/gpio/gpio.h include needed for the examples block?
Neither the device tree node nor its child nodes appear to declare or use
any GPIO properties or constants, so this might just be a copy-paste
artifact.

> +
> +    i2c {
> +        #address-cells = <1>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-topic-sm8x50-iio-max34417-adc-v1-0-41d4ba1bfc41@linaro.org?part=1

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

[Neil Armstrong]

On 9/23/26 21:18, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Low] dt-bindings: iio: use of vendor-specific rsense property instead of standard
> - [Low] dt-bindings: iio: unnecessary gpio.h header include in example
> - [Low] dt-bindings: iio: contradictory default and required constraints
> --
> 
> commit 83b0b12b383dfd05c3631355160ac1fe40dc6802
> Author: Neil Armstrong <neil.armstrong@linaro.org>
> 
> dt-bindings: iio: add: document the MAX34417 Four-Channel High Dynamic Range Power Accumulator
> 
> This commit documents the MAX34417 power accumulator device tree bindings.
> The device uses an I2C interface to monitor power consumption in portable
> systems.
> 
>> 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 0000000000000..6f2339a927710
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max34417.yaml
> [ ... ]
>> +    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]
> 
> [Severity: Low]
> Does this binding need the vendor-specific maxim,rsense-val-micro-ohms
> property for the shunt resistor? The standard shunt-resistor-micro-ohms
> property is well-established across multiple kernel subsystems for this
> exact purpose and might be a better fit.
> 
> Additionally, is the hardcoded enum constraint necessary? It artificially
> limits the possible resistor values, whereas physical shunt resistors can be
> of any resistance.

Good points

> 
>> +        default: 1000
>> +
>> +    required:
>> +      - reg
>> +      - maxim,rsense-val-micro-ohms
> 
> [Severity: Low]
> Can the default fallback ever be utilized for maxim,rsense-val-micro-ohms?
> The property is defined with a default of 1000, but is simultaneously listed
> in the required array for the subnode. Because it is required, schema
> validation will fail if the property is missing in a device tree, making the
> default logically dead.

Good point

> 
> [ ... ]
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
> 
> [Severity: Low]
> Is the dt-bindings/gpio/gpio.h include needed for the examples block?
> Neither the device tree node nor its child nodes appear to declare or use
> any GPIO properties or constants, so this might just be a copy-paste
> artifact.


Good point

> 
>> +
>> +    i2c {
>> +        #address-cells = <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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] iio: max34417: division by zero in max34417_read_power() when acc_count is 0
- [High] iio: max34417: division by zero in max34417_probe() if rsense is 0
- [High] iio: max34417: build failure on 32-bit architectures due to GENMASK(55, 32)
- [Medium] iio: max34417: fwnode handle leak on error paths in max34417_probe()
- [Medium] iio: max34417: undeclared channels have uninitialized scale of 0
--

commit 281cd76d0d1a3f010bbf1f4241c438deb9b82192
Author: Neil Armstrong <neil.armstrong@linaro.org>

    iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator
    
    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.

> diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
> new file mode 100644
> index 0000000000000..004401de6ce7d
> --- /dev/null
> +++ b/drivers/iio/adc/max34417.c

[ ... ]

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

[Severity: High]
Is it possible for acc_count to be zero here? If the hardware returns 0 (for
example, due to an incomplete conversion, I2C read glitch, or uninitialized
state), wouldn't this div_u64() cause a division by zero panic?

> +
> +	*val = FIELD_GET(GENMASK(31, 0), power);
> +	*val2 = FIELD_GET(GENMASK(55, 32), power);

[Severity: High]
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;
> +	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;

[Severity: Medium]
Does this expose an uninitialized scale of 0 to userspace for omitted
channels?

Since all 4 channels are unconditionally exposed by max34417_channels, but
input_correction is only populated for channels explicitly defined in the
device tree during probe, userspace reading the scale for a missing channel
might get a value of 0.

> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		}
> +		return -EINVAL;
> +	default:
> +		return -EINVAL;
> +	}
> +}

[ ... ]

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

[ ... ]

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

[Severity: Medium]
Does this code leak the fwnode reference? Since device_for_each_child_node()
takes a reference on the node during iteration, shouldn't fwnode_handle_put()
be called before returning early?

> +		} else if (index >= MAX34417_CHANNEL_COUNT) {
> +			dev_err(dev, "invalid reg %d of %pfwP\n", index, node);
> +			return -EINVAL;

[Severity: Medium]
Could this early return also leak the fwnode reference?

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

[Severity: High]
Can rsense be zero here?

If the device tree explicitly sets "maxim,rsense-val-micro-ohms" to 0,
fwnode_property_read_u32() will succeed and leave rsense as 0, which would
cause a division by zero when calculating correction.

> +
> +		max34417->input_correction[index] = correction;
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-topic-sm8x50-iio-max34417-adc-v1-0-41d4ba1bfc41@linaro.org?part=2

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

[Neil Armstrong]

On 9/23/26 21:19, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] iio: max34417: division by zero in max34417_read_power() when acc_count is 0
> - [High] iio: max34417: division by zero in max34417_probe() if rsense is 0
> - [High] iio: max34417: build failure on 32-bit architectures due to GENMASK(55, 32)
> - [Medium] iio: max34417: fwnode handle leak on error paths in max34417_probe()
> - [Medium] iio: max34417: undeclared channels have uninitialized scale of 0
> --
> 
> commit 281cd76d0d1a3f010bbf1f4241c438deb9b82192
> Author: Neil Armstrong <neil.armstrong@linaro.org>
> 
>      iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator
>      
>      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.
> 
>> diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
>> new file mode 100644
>> index 0000000000000..004401de6ce7d
>> --- /dev/null
>> +++ b/drivers/iio/adc/max34417.c
> 
> [ ... ]
> 
>> +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);
> 
> [Severity: High]
> Is it possible for acc_count to be zero here? If the hardware returns 0 (for
> example, due to an incomplete conversion, I2C read glitch, or uninitialized
> state), wouldn't this div_u64() cause a division by zero panic?

Good point

> 
>> +
>> +	*val = FIELD_GET(GENMASK(31, 0), power);
>> +	*val2 = FIELD_GET(GENMASK(55, 32), power);
> 
> [Severity: High]
> 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?

Good point

> 
>> +
>> +	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;
> 
> [Severity: Medium]
> Does this expose an uninitialized scale of 0 to userspace for omitted
> channels?
> 
> Since all 4 channels are unconditionally exposed by max34417_channels, but
> input_correction is only populated for channels explicitly defined in the
> device tree during probe, userspace reading the scale for a missing channel
> might get a value of 0.

Good point

> 
>> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
>> +
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		}
>> +		return -EINVAL;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
> 
> [ ... ]
> 
>> +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;
> 
> [ ... ]
> 
>> +	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;
> 
> [Severity: Medium]
> Does this code leak the fwnode reference? Since device_for_each_child_node()
> takes a reference on the node during iteration, shouldn't fwnode_handle_put()
> be called before returning early?


Good point

> 
>> +		} else if (index >= MAX34417_CHANNEL_COUNT) {
>> +			dev_err(dev, "invalid reg %d of %pfwP\n", index, node);
>> +			return -EINVAL;
> 
> [Severity: Medium]
> Could this early return also leak the fwnode reference?
> 
>> +		}
>> +
>> +		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;
> 
> [Severity: High]
> Can rsense be zero here?
> 
> If the device tree explicitly sets "maxim,rsense-val-micro-ohms" to 0,
> fwnode_property_read_u32() will succeed and leave rsense as 0, which would
> cause a division by zero when calculating correction.

Good point

> 
>> +
>> +		max34417->input_correction[index] = correction;
>> +	}
>> +
> 

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