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

[PATCH v3 0/3] 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 supports
getting the channels voltage, average power and accumulated average
power and energy over an I2C/SMBUS serial interface.

The MAX34417 datasheet only specified on how to calculate the
accumulated average power, but with the Sample Per Second rate
we can easily get accumulated Energy in mJ.

By default the IIO driver only supports single-shot Voltage
and Power sensing, but the accumulation can be enabled by
enabling the Energy IIO channel which then enables the
Continuous Accumulation Mode.

The counter is common to all channels, so when reading a single
channel we will copy all accumulators to registers, so in order
to permit reading individual channels we must accumulate the
other channels in the driver when Continuous Accumulation Mode
is enabled.

The get_unaligned_be56() helper is added since we effectly
get an unaligned 56bit value from the HW.

While Maxim originally provided an HWMON driver for this
IC, using IIO makes much more sense because it provides
much more flexibility to support getting the power values
and will make it easier to support trigger to dump all
channels in a single shot and then support the Bulk Update
feature to update all MAX34417 on the same bus via the
trigger mechanism.

The v3 implementation has been completely revised since
v1/v2 to better match the capabilities of the MAX34417 and
allow getting accumulated Energy plus provide single
channel read without loosing other channels accumulations.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v3:
- Implement Continuous Accumulate Mode and default to single-shot mode
- Fix includes
- Better define Rsense and default rsensd in driver & bindings
- Define all read length and offsets
- Define the SPS bits
- Fix comments first letters
- Fix max register
- Add ENERGY channel with shared enable for Continuous Accumulate Mode
- By default reset the MAX34417 counters for each readout to do a single-shot measure
- Handle zero counter
- Add get_unaligned_be56
- Use get_unaligned_* macros
- Better handle the DT properties and report errors
- Reset the MAX34417 at the end of probe
- Link to v2: https://patch.msgid.link/20260924-topic-sm8x50-iio-max34417-adc-v2-0-9a0609e72f5c@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

To: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
To: Nuno Sá <nuno.sa@analog.com>
To: Andy Shevchenko <andy@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Neil Armstrong <neil.armstrong@linaro.org>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Marius Cristea (1):
      unaligned: introduce be56 unaligned accessors

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                         | 777 +++++++++++++++++++++
 include/linux/unaligned.h                          |  27 +
 5 files changed, 916 insertions(+)
---
base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
change-id: 20260923-topic-sm8x50-iio-max34417-adc-209e880533fb

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

[PATCH v3 1/3] 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

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

[Conor Dooley]

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

On Tue, Oct 13, 2026 at 04:07:11PM +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>

Previous review still applies?
https://lore.kernel.org/all/20260924-circling-punk-e75ea71e60b0@spud/

pw-bot: changes-requested

Thanks,
Conor.

> ---
>  .../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
> 

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

[PATCH v3 2/3] unaligned: introduce be56 unaligned accessors

[Neil Armstrong]

From: Marius Cristea <marius.cristea@microchip.com>

The MAX34417, IIO driver, is using some unaligned 56 bit registers.
Provide some helper accessors in preparation for the new driver.

Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
[narmstrong: update commit message and move to include/linux/unaligned.h]
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 include/linux/unaligned.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/linux/unaligned.h b/include/linux/unaligned.h
index 4a9651017e3c..2105743b1800 100644
--- a/include/linux/unaligned.h
+++ b/include/linux/unaligned.h
@@ -143,4 +143,31 @@ static inline u64 get_unaligned_be48(const void *p)
 	return __get_unaligned_be48(p);
 }
 
+static inline void __put_unaligned_be56(const u64 val, u8 *p)
+{
+	*p++ = (val >> 48) & 0xff;
+	*p++ = (val >> 40) & 0xff;
+	*p++ = (val >> 32) & 0xff;
+	*p++ = (val >> 24) & 0xff;
+	*p++ = (val >> 16) & 0xff;
+	*p++ = (val >> 8) & 0xff;
+	*p++ = val & 0xff;
+}
+
+static inline void put_unaligned_be56(const u64 val, void *p)
+{
+	__put_unaligned_be56(val, p);
+}
+
+static inline u64 __get_unaligned_be56(const u8 *p)
+{
+	return (u64)p[0] << 48 | (u64)p[1] << 40 | (u64)p[2] << 32 |
+		(u64)p[3] << 24 | p[4] << 16 | p[5] << 8 | p[6];
+}
+
+static inline u64 get_unaligned_be56(const void *p)
+{
+	return __get_unaligned_be56(p);
+}
+
 #endif /* __LINUX_UNALIGNED_H */

-- 
2.34.1

Re: [PATCH v3 2/3] unaligned: introduce be56 unaligned accessors

[Andy Shevchenko]

On Tue, Oct 13, 2026 at 04:07:12PM +0200, Neil Armstrong wrote:

> The MAX34417, IIO driver, is using some unaligned 56 bit registers.
> Provide some helper accessors in preparation for the new driver.

No objections from me.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

(Yes, implementation may vary for all of them as to use combinations of existed
 e32/e24/e16 calls but it should be measured for the performance / code
 generation / et cetera).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 3/3] 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, average power and accumulated average
power and energy over an I2C/SMBUS serial interface.

The MAX34417 datasheet only specified on how to calculate the
accumulated average power, but with the Sample Per Second rate
we can easily get accumulated Energy in mJ.

By default the IIO driver only supports single-shot Voltage
and Power sensing, but the accumulation can be enabled by
enabling the Energy IIO channel which then enables the
Continuous Accumulation Mode.

The counter is common to all channels, so when reading a single
channel we will copy all accumulators to registers, so in order
to permit reading individual channels we must accumulate the
other channels in the driver when Continuous Accumulation Mode
is enabled.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/iio/adc/Kconfig    |  11 +
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max34417.c | 777 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 789 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..00d7f20cd953
--- /dev/null
+++ b/drivers/iio/adc/max34417.c
@@ -0,0 +1,777 @@
+// 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, 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/unaligned.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_ACC_COUNT_READ_LENGTH	4
+#define MAX34417_ACC_COUNT_READ_OFFSET	1
+
+#define MAX34417_POWER_READ_LENGTH	8
+#define MAX34417_POWER_READ_OFFSET	1
+
+#define MAX34417_VOLTAGE_READ_LENGTH	3
+#define MAX34417_VOLTAGE_READ_OFFSET	1
+
+#define MAX34417_MIN_RSENSE		(MILLI)	/* 1 milliOhm */
+#define MAX34417_MAX_RSENSE		(100 * MILLI) /* 100 milliOhm */
+#define MAX34417_DEFAULT_RSENSE		MAX34417_MIN_RSENSE
+
+#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
+
+/* TODO implement IIO_CHAN_INFO_SAMP_FREQ for slow mode */
+#define MAX34417_SLOW_SPS_BIT			3 /* 8 SPS */
+#define MAX34417_FAST_SPS_BIT			10 /* 1024 SPS */
+#define MAX34417_FAST_SPS_CAM			1536 /* 1.5 * 1024 SPS */
+
+#define MAX34417_CHANNEL_COUNT			4
+
+/**
+ * struct max34417_data - MAX34417 specific data.
+ * @indio_dev:  IIO Device.
+ * @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.
+ * @cam_mode:		State of the Continuous Accumulate Mode feature
+ * @acc_power:		Saved power accumulator for each channel when in CAM mode
+ * @acc_count:		Saved counter for each channel when in CAM mode
+ */
+struct max34417_data {
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	struct device *dev;
+	struct mutex lock;
+	u32 input_correction[MAX34417_CHANNEL_COUNT];
+	const char *input_label[MAX34417_CHANNEL_COUNT];
+	bool cam_mode;
+	u64 acc_power[MAX34417_CHANNEL_COUNT];
+	u32 acc_count[MAX34417_CHANNEL_COUNT];
+};
+
+static const struct regmap_config max34417_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= MAX34417_BULK_VOLTAGE_READOUT_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_RAW) | \
+					  BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \
+					  BIT(IIO_CHAN_INFO_SCALE), \
+		.channel = (_index),				\
+		.address = (_power_address),			\
+		.indexed = 1,					\
+	},							\
+	{							\
+		.type = IIO_ENERGY,				\
+		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) | \
+					  BIT(IIO_CHAN_INFO_SCALE), \
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_ENABLE), \
+		.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_reset_smm(struct max34417_data *max34417)
+{
+	u32 control;
+	int rc;
+
+	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
+	if (rc)
+		return rc;
+
+	/* Set SMM=0 to re-init the SMM mode */
+	control &= ~(MAX34417_CONTROL_SMM);
+	rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
+	if (rc)
+		return rc;
+
+	/* Reset accumulators after setting SMM=0 */
+	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
+static int max34417_trigger_smm(struct max34417_data *max34417)
+{
+	u32 control;
+	int rc;
+
+	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
+	if (rc)
+		return rc;
+
+	if (control & MAX34417_CONTROL_SMM) {
+		/* Set SMM=0 to re-init the SMM mode */
+		control &= ~(MAX34417_CONTROL_SMM);
+		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
+		if (rc)
+			return rc;
+
+		/* Reset accumulators after setting SMM=0 */
+		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+		if (rc)
+			return rc;
+	}
+
+	/* Enable SMM to do a single accumulation */
+	control |= MAX34417_CONTROL_SMM;
+	rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
+	if (rc)
+		return rc;
+
+	/*
+	 * After the SMM bit is changed, the UPDATE command should be sent
+	 * to reset the accumulators and perform the selected scan operation.
+	 */
+	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+	/*
+	 * UPDATE commands should be no less than 1msec apart for reliable
+	 * measurements.
+	 */
+	fsleep(1000);
+
+	return 0;
+}
+
+static int max34417_read_acc_count(struct max34417_data *max34417, u32 *acc_count)
+{
+	u8 buf[MAX34417_ACC_COUNT_READ_LENGTH];
+	int rc;
+
+	rc = regmap_noinc_read(max34417->regmap, MAX34417_ACC_COUNT_REG,
+			       &buf, sizeof(buf));
+	if (rc)
+		return rc;
+
+	/* Ignore byte0 */
+	*acc_count = get_unaligned_be24(&buf[MAX34417_ACC_COUNT_READ_OFFSET]);
+
+	return 0;
+}
+
+static int max34417_read_acc_power(struct max34417_data *max34417, unsigned int address,
+				   u64 *acc_power)
+{
+	u8 buf[MAX34417_POWER_READ_LENGTH];
+	int rc;
+
+	rc = regmap_noinc_read(max34417->regmap, address, &buf, sizeof(buf));
+	if (rc)
+		return rc;
+
+	/* Ignore byte0 */
+	*acc_power = get_unaligned_be56(&buf[MAX34417_POWER_READ_OFFSET]);
+
+	return 0;
+}
+
+static int max34417_check_ovf(struct max34417_data *max34417)
+{
+	u32 control;
+	int rc;
+
+	/* Check for overflow */
+	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
+	if (rc)
+		return rc;
+
+	if (control & MAX34417_CONTROL_OVF) {
+		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG,
+				  control & ~MAX34417_CONTROL_OVF);
+		if (rc)
+			return rc;
+
+		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
+/*
+ * Save all power accumulators and counter after each UPDATE command
+ * since it will reset the internal registers for ALL channels,
+ * so in order to not loose any captured data we retrieve all registers
+ * and accumulate them in the context variables.
+ * When user gets either power or energy, those variables content
+ * will be returned and zero-ed to return the power accumulated
+ * since last read for each separate channel.
+ */
+static int max34417_save_accumulators(struct max34417_data *max34417)
+{
+	unsigned int i;
+	u64 acc_power;
+	u32 acc_count;
+	int rc;
+
+	rc = max34417_read_acc_count(max34417, &acc_count);
+	if (rc)
+		return rc;
+
+	for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i) {
+		rc = max34417_read_acc_power(max34417, MAX34417_PWR_ACC_1_REG + i,
+					     &acc_power);
+		if (rc)
+			return rc;
+
+		max34417->acc_power[i] += acc_power;
+		max34417->acc_count[i] += acc_count;
+	}
+
+	/* Check for overflow */
+	rc = max34417_check_ovf(max34417);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
+static int max34417_read_voltage(struct max34417_data *max34417,
+				 const struct iio_chan_spec *chan, int *val)
+{
+	u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
+	u16 voltage;
+	int rc;
+
+	guard(mutex)(&max34417->lock);
+
+	/* In SMM mode, trigger a measurement cycle manually */
+	if (!max34417->cam_mode) {
+		rc = max34417_trigger_smm(max34417);
+		if (rc)
+			return rc;
+	}
+
+	/* Move the new UPDATE data into the SMBus read registers */
+	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	if (max34417->cam_mode) {
+		/* Retrieve all channels to avoid missing data for other channels */
+		rc = max34417_save_accumulators(max34417);
+		if (rc)
+			return rc;
+	}
+
+	rc = regmap_noinc_read(max34417->regmap, chan->address, &buf, sizeof(buf));
+	if (rc)
+		return rc;
+
+	if (!max34417->cam_mode) {
+		rc = max34417_reset_smm(max34417);
+		if (rc)
+			return rc;
+	}
+
+	/* Ignore byte0 */
+	voltage = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]);
+	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)
+{
+	u64 power;
+	int rc;
+
+	guard(mutex)(&max34417->lock);
+
+	/* In SMM mode, trigger a measurement cycle manually */
+	if (!max34417->cam_mode) {
+		rc = max34417_trigger_smm(max34417);
+		if (rc)
+			return rc;
+	}
+
+	/*
+	 * Move the new UPDATE data into the SMBus read registers.
+	 * In CAM mode this will also reset the accumulators, returning
+	 * the power accumulated since the last UPDATE.
+	 */
+	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	/*
+	 * Only get the accumulator counter in CAM mode, in SMM mode
+	 * we only accumulate only once, so the counter is 1
+	 */
+	if (max34417->cam_mode) {
+		u32 acc_count;
+		u64 acc_power;
+
+		/* Retrieve all channels to avoid missing data for other channels */
+		rc = max34417_save_accumulators(max34417);
+		if (rc)
+			return rc;
+
+		/* Get current channel power, counter and reset them */
+		acc_power = max34417->acc_power[chan->channel];
+		acc_count = max34417->acc_count[chan->channel];
+		max34417->acc_power[chan->channel] = 0;
+		max34417->acc_count[chan->channel] = 0;
+
+		if (unlikely(acc_count == 0)) {
+			dev_warn(max34417->dev, "unexpected zero accumulator count\n");
+
+			*val = 0;
+			*val2 = 0;
+
+			return IIO_VAL_INT_64;
+		}
+
+		power = div_u64(acc_power, acc_count);
+	} else {
+		rc = max34417_read_acc_power(max34417, chan->address, &power);
+		if (rc)
+			return rc;
+
+		rc = max34417_reset_smm(max34417);
+		if (rc)
+			return rc;
+	}
+
+	*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_energy(struct max34417_data *max34417,
+			       const struct iio_chan_spec *chan,
+			       int *val, int *val2)
+{
+	u64 acc_power;
+	int rc;
+
+	guard(mutex)(&max34417->lock);
+
+	/*
+	 * Move the data to the SMBus registers
+	 */
+	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	/* Retrieve all channels to avoid missing data for other channels */
+	rc = max34417_save_accumulators(max34417);
+	if (rc)
+		return rc;
+
+	/* Get current channel power and reset them */
+	acc_power = max34417->acc_power[chan->channel];
+	max34417->acc_power[chan->channel] = 0;
+	max34417->acc_count[chan->channel] = 0;
+
+	*val = FIELD_GET(GENMASK_ULL(31, 0), acc_power);
+	*val2 = FIELD_GET(GENMASK_ULL(55, 32), acc_power);
+
+	return IIO_VAL_INT_64;
+}
+
+static int max34417_setup_cam_mode(struct max34417_data *max34417, int val)
+{
+	u32 control;
+	int rc, i;
+
+	guard(mutex)(&max34417->lock);
+
+	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
+	if (rc)
+		return rc;
+
+	if (max34417->cam_mode && !val) {
+		/* SMM and CAM modes should not be enabled at the same time */
+		control &= ~(MAX34417_CONTROL_CAM | MAX34417_CONTROL_SMM);
+		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
+		if (rc)
+			return rc;
+
+		/*
+		 * If CAM mode must be set, the Control byte must be set to
+		 * desired values with CAM = 0, then followed by an UPDATE
+		 */
+		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+		if (rc)
+			return rc;
+	} else if (!max34417->cam_mode && val) {
+		/*
+		 * After this, the CAM mode bit can be set and another
+		 * UPDATE must be sent.
+		 */
+		control |= MAX34417_CONTROL_CAM;
+		/* SMM and CAM modes should not be enabled at the same time */
+		control &= ~(MAX34417_CONTROL_SMM);
+		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
+		if (rc)
+			return rc;
+
+		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+		if (rc)
+			return rc;
+
+		/* Reset counters and accumulators */
+		for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i) {
+			max34417->acc_power[i] = 0;
+			max34417->acc_count[i] = 0;
+		}
+	}
+
+	max34417->cam_mode = val;
+
+	return 0;
+}
+
+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:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			return max34417_read_voltage(max34417, chan, val);
+		case IIO_POWER:
+			/* In default mode, we return a single measurement */
+			if (max34417->cam_mode)
+				return -EINVAL;
+
+			return max34417_read_power(max34417, chan, val, val2);
+		case IIO_ENERGY:
+			if (!max34417->cam_mode)
+				return -EINVAL;
+
+			return max34417_read_energy(max34417, chan, val, val2);
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_AVERAGE_RAW:
+		if (chan->type != IIO_POWER)
+			return -EINVAL;
+
+		/* In CAM mode, we return an average measurement */
+		if (!max34417->cam_mode)
+			return -EINVAL;
+
+		return max34417_read_power(max34417, chan, val, val2);
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_VOLTAGE) {
+			/* Scale to mV */
+			*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
+			 * (1000 * (Power Accumulation / Counter) * Correction) / 2^30
+			 */
+			*val = max34417->input_correction[chan->channel] * MILLI;
+			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
+
+			return IIO_VAL_FRACTIONAL_LOG2;
+		} else if (chan->type == IIO_ENERGY) {
+			/*
+			 * Scale to mJ
+			 * P=(1000 * (Power Accumulation / Counter) * Correction) / 2^30
+			 * t=(Counter / Samples per Second)
+			 * E=(P * t)
+			 * Can be simplified to:
+			 * (1000 * Power Accumulation * Correction) / (2^30 * SPS)
+			 * The denominator doesn't fit in 32bit so scale down both terms.
+			 */
+			*val = max34417->input_correction[chan->channel];
+			*val2 = ((u64)(1 << MAX34417_PWR_AVG_FULL_SCALE_BITS) *
+				 MAX34417_FAST_SPS_CAM) / MILLI;
+
+			return IIO_VAL_FRACTIONAL;
+		}
+
+		return -EINVAL;
+	case IIO_CHAN_INFO_ENABLE:
+		*val = max34417->cam_mode;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int max34417_write_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_ENABLE:
+		/* Setup the CAM mode for the 4 channels */
+		return max34417_setup_cam_mode(max34417, val);
+	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);
+	}
+
+	if (chan->type == IIO_ENERGY) {
+		if (input_label)
+			return sysfs_emit(label, "%s-energy\n", input_label);
+
+		return sysfs_emit(label, "channel%d-energy\n", chan->channel);
+	}
+
+	return 0;
+}
+
+static const struct iio_info max34417_info = {
+	.read_raw	= max34417_read_raw,
+	.write_raw	= max34417_write_raw,
+	.read_label	= max34417_read_label,
+};
+
+static unsigned int max34417_calc_input_correction(u32 rsense)
+{
+	/* (100 milliOhm / rsense) * MAX34417_PWR_CORRECTION_SCALE */
+	return (MAX34417_MAX_RSENSE * 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;
+	max34417->indio_dev = indio_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_present(node, "reg"))
+			return dev_err_probe(dev, -EINVAL, "missing reg property of %pfwP\n",
+					     node);
+
+		rc = fwnode_property_read_u32(node, "reg", &index);
+		if (rc)
+			return dev_err_probe(dev, rc, "failure reading reg property of %pfwP\n",
+					     node);
+
+		if (index >= MAX34417_CHANNEL_COUNT)
+			return dev_err_probe(dev, -EINVAL, "invalid reg %d of %pfwP\n",
+					     index, node);
+
+		if (fwnode_property_present(node, "label")) {
+			rc = fwnode_property_read_string(node, "label",
+							 &max34417->input_label[index]);
+			if (rc)
+				return dev_err_probe(dev, rc,
+						"failure reading label property of %pfwP\n",
+						node);
+		}
+
+		if (fwnode_property_present(node, "shunt-resistor-micro-ohms")) {
+			rc = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms",
+						      &rsense);
+			if (rc)
+				return dev_err_probe(dev, rc,
+						"failure reading shunt property of %pfwP\n",
+						node);
+
+			if (!rsense || rsense < MAX34417_MIN_RSENSE ||
+			    rsense > MAX34417_MAX_RSENSE)
+				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;
+
+	rc = devm_iio_device_register(dev, indio_dev);
+	if (rc)
+		return rc;
+
+	/* Reset mode and set accumulator width to Wide */
+	rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, MAX34417_CONTROL_MODE);
+	if (rc)
+		return dev_err_probe(max34417->dev, rc, "Error writing control register\n");
+
+	/* Reset the accumulators */
+	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
+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 v3 3/3] 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 6 potential issue(s) to consider:
- [High] iio: max34417: silent truncation of 64-bit software accumulator
- [High] iio: max34417: sysfs reads violate 1ms hardware UPDATE delay constraint
- [Medium] iio: max34417: u32 acc_count overflows causing corrupted power calculations
- [Medium] iio: max34417: device registered before hardware initialization completes
- [Medium] iio: max34417: unprivileged log-spam DoS via non-rate-limited dev_warn
- [Medium] iio: adc: max34417: data race and TOCTOU on cam_mode causes incorrect statistics
--

commit ebd13d02bb2cfb7ea4951f2b1c27883dde2b138e
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 supports getting the channels
voltage, average power and accumulated average power and energy 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..00d7f20cd9531
> --- /dev/null
> +++ b/drivers/iio/adc/max34417.c
> @@ -0,0 +1,777 @@

[ ... ]

> +struct max34417_data {
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct mutex lock;
> +	u32 input_correction[MAX34417_CHANNEL_COUNT];
> +	const char *input_label[MAX34417_CHANNEL_COUNT];
> +	bool cam_mode;
> +	u64 acc_power[MAX34417_CHANNEL_COUNT];
> +	u32 acc_count[MAX34417_CHANNEL_COUNT];
> +};

[Severity: Medium]
Will this 32-bit counter overflow during continuous long-term accumulation?

In Continuous Accumulation Mode (CAM), the hardware sample counter is repeatedly
added to this variable across sysfs reads. At the maximum sample rate of 1024
SPS, a 32-bit counter will overflow in approximately 48 days. When this
wrap-around occurs, the subsequent average power calculation
(div_u64(acc_power, acc_count)) will divide the large 64-bit acc_power by a
small or zero acc_count, resulting in a corrupted power reading or a division
by zero panic. Should acc_count be promoted to u64?

[ ... ]

> +static int max34417_read_voltage(struct max34417_data *max34417,
> +				 const struct iio_chan_spec *chan, int *val)
> +{
> +	u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
> +	u16 voltage;
> +	int rc;
> +
> +	guard(mutex)(&max34417->lock);
> +
> +	/* In SMM mode, trigger a measurement cycle manually */
> +	if (!max34417->cam_mode) {
> +		rc = max34417_trigger_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* Move the new UPDATE data into the SMBus read registers */
> +	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);

[Severity: High]
Does this bypass the hardware's 1ms UPDATE delay constraint?

The driver notes that "UPDATE commands should be no less than 1msec apart for
reliable measurements." While max34417_trigger_smm() properly enforces this
delay, the sysfs read handlers for CAM mode unconditionally write to
MAX34417_UPDATE_REG. If an unprivileged userspace process polls the sysfs
attributes faster than 1ms, it might violate this constraint, potentially
leading to unreliable measurements, missing samples, or bus lockups.

[ ... ]

> +static int max34417_read_power(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{

[ ... ]

> +	if (max34417->cam_mode) {
> +		u32 acc_count;
> +		u64 acc_power;
> +
> +		/* Retrieve all channels to avoid missing data for other channels */
> +		rc = max34417_save_accumulators(max34417);
> +		if (rc)
> +			return rc;
> +
> +		/* Get current channel power, counter and reset them */
> +		acc_power = max34417->acc_power[chan->channel];
> +		acc_count = max34417->acc_count[chan->channel];
> +		max34417->acc_power[chan->channel] = 0;
> +		max34417->acc_count[chan->channel] = 0;
> +
> +		if (unlikely(acc_count == 0)) {
> +			dev_warn(max34417->dev, "unexpected zero accumulator count\n");

[Severity: Medium]
Could this non-rate-limited warning lead to a log-spam denial of service?

If an unprivileged userspace process repeatedly reads the attribute in a tight
loop, it will extract data faster than the hardware's sample rate, causing the
counter to evaluate to 0. This blindly prints a warning using dev_warn()
without any rate limiting, which could flood the kernel log buffer and mask
legitimate kernel messages. Should dev_warn_ratelimited() be used instead?

> +			*val = 0;
> +			*val2 = 0;
> +
> +			return IIO_VAL_INT_64;
> +		}
> +
> +		power = div_u64(acc_power, acc_count);
> +	} else {
> +		rc = max34417_read_acc_power(max34417, chan->address, &power);
> +		if (rc)
> +			return rc;
> +
> +		rc = max34417_reset_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	*val = FIELD_GET(GENMASK_ULL(31, 0), power);
> +	*val2 = FIELD_GET(GENMASK_ULL(55, 32), power);

[Severity: High]
Does this silently truncate the upper 8 bits of the 64-bit software
accumulator?

GENMASK_ULL(55, 32) explicitly masks off bits 56-63, meaning any accumulated
value exceeding 56 bits is silently truncated and dropped before being
exported to IIO userspace. Since CAM mode continuously accumulates readings,
the u64 can easily exceed 56 bits in sustained operation, causing the
userspace reading to wrap around prematurely. Should this use
GENMASK_ULL(63, 32) or upper_32_bits(power)?

> +
> +	return IIO_VAL_INT_64;
> +}
> +
> +static int max34417_read_energy(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{

[ ... ]

> +	/* Get current channel power and reset them */
> +	acc_power = max34417->acc_power[chan->channel];
> +	max34417->acc_power[chan->channel] = 0;
> +	max34417->acc_count[chan->channel] = 0;
> +
> +	*val = FIELD_GET(GENMASK_ULL(31, 0), acc_power);
> +	*val2 = FIELD_GET(GENMASK_ULL(55, 32), acc_power);

[Severity: High]
Does this also silently truncate the upper 8 bits of the 64-bit accumulator?

Just as in max34417_read_power(), this GENMASK_ULL(55, 32) truncates bits
56-63 of acc_power.

[ ... ]

> +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:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			return max34417_read_voltage(max34417, chan, val);
> +		case IIO_POWER:
> +			/* In default mode, we return a single measurement */
> +			if (max34417->cam_mode)
> +				return -EINVAL;
> +
> +			return max34417_read_power(max34417, chan, val, val2);

[Severity: Medium]
Could this lockless check of cam_mode lead to a TOCTOU race and return
incorrect statistics?

Here, cam_mode is evaluated without holding the mutex. Concurrently, a sysfs
write to in_energy_enable could call max34417_setup_cam_mode(), taking the
mutex and setting cam_mode to true. When max34417_read_power() is then
called, it acquires the mutex, sees cam_mode is true, and executes the CAM
mode logic. This would unexpectedly return an averaged measurement instead of
a single-shot reading, violating the expected IIO channel behavior.

[ ... ]

> +static int max34417_probe(struct i2c_client *client)
> +{

[ ... ]

> +	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;
> +
> +	rc = devm_iio_device_register(dev, indio_dev);
> +	if (rc)
> +		return rc;
> +
> +	/* Reset mode and set accumulator width to Wide */
> +	rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, MAX34417_CONTROL_MODE);

[Severity: Medium]
Is it safe to register the IIO device before the hardware initialization
completes?

devm_iio_device_register() exposes the device and its sysfs attributes to
userspace immediately. A userspace process or udev rule could open the device
and trigger read_raw or write_raw callbacks before the hardware is placed
into the correct mode (MAX34417_CONTROL_MODE) or its internal accumulators
are reset. Should the hardware configuration happen prior to registration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-topic-sm8x50-iio-max34417-adc-v3-0-d3604ae4a229@linaro.org?part=3

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

[Neil Armstrong]

On 10/13/26 16:19, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] iio: max34417: silent truncation of 64-bit software accumulator
> - [High] iio: max34417: sysfs reads violate 1ms hardware UPDATE delay constraint
> - [Medium] iio: max34417: u32 acc_count overflows causing corrupted power calculations
> - [Medium] iio: max34417: device registered before hardware initialization completes
> - [Medium] iio: max34417: unprivileged log-spam DoS via non-rate-limited dev_warn
> - [Medium] iio: adc: max34417: data race and TOCTOU on cam_mode causes incorrect statistics
> --
> 
> commit ebd13d02bb2cfb7ea4951f2b1c27883dde2b138e
> 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 supports getting the channels
> voltage, average power and accumulated average power and energy 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..00d7f20cd9531
>> --- /dev/null
>> +++ b/drivers/iio/adc/max34417.c
>> @@ -0,0 +1,777 @@
> 
> [ ... ]
> 
>> +struct max34417_data {
>> +	struct iio_dev *indio_dev;
>> +	struct regmap *regmap;
>> +	struct device *dev;
>> +	struct mutex lock;
>> +	u32 input_correction[MAX34417_CHANNEL_COUNT];
>> +	const char *input_label[MAX34417_CHANNEL_COUNT];
>> +	bool cam_mode;
>> +	u64 acc_power[MAX34417_CHANNEL_COUNT];
>> +	u32 acc_count[MAX34417_CHANNEL_COUNT];
>> +};
> 
> [Severity: Medium]
> Will this 32-bit counter overflow during continuous long-term accumulation?
> 
> In Continuous Accumulation Mode (CAM), the hardware sample counter is repeatedly
> added to this variable across sysfs reads. At the maximum sample rate of 1024
> SPS, a 32-bit counter will overflow in approximately 48 days. When this
> wrap-around occurs, the subsequent average power calculation
> (div_u64(acc_power, acc_count)) will divide the large 64-bit acc_power by a
> small or zero acc_count, resulting in a corrupted power reading or a division
> by zero panic. Should acc_count be promoted to u64?

Eventually yes

> 
> [ ... ]
> 
>> +static int max34417_read_voltage(struct max34417_data *max34417,
>> +				 const struct iio_chan_spec *chan, int *val)
>> +{
>> +	u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
>> +	u16 voltage;
>> +	int rc;
>> +
>> +	guard(mutex)(&max34417->lock);
>> +
>> +	/* In SMM mode, trigger a measurement cycle manually */
>> +	if (!max34417->cam_mode) {
>> +		rc = max34417_trigger_smm(max34417);
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	/* Move the new UPDATE data into the SMBus read registers */
>> +	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> 
> [Severity: High]
> Does this bypass the hardware's 1ms UPDATE delay constraint?
> 
> The driver notes that "UPDATE commands should be no less than 1msec apart for
> reliable measurements." While max34417_trigger_smm() properly enforces this
> delay, the sysfs read handlers for CAM mode unconditionally write to
> MAX34417_UPDATE_REG. If an unprivileged userspace process polls the sysfs
> attributes faster than 1ms, it might violate this constraint, potentially
> leading to unreliable measurements, missing samples, or bus lockups.

The 1ms update delay is only for the Single Shot Mode, not for the Continuous Update Mode.

> 
> [ ... ]
> 
>> +static int max34417_read_power(struct max34417_data *max34417,
>> +			       const struct iio_chan_spec *chan,
>> +			       int *val, int *val2)
>> +{
> 
> [ ... ]
> 
>> +	if (max34417->cam_mode) {
>> +		u32 acc_count;
>> +		u64 acc_power;
>> +
>> +		/* Retrieve all channels to avoid missing data for other channels */
>> +		rc = max34417_save_accumulators(max34417);
>> +		if (rc)
>> +			return rc;
>> +
>> +		/* Get current channel power, counter and reset them */
>> +		acc_power = max34417->acc_power[chan->channel];
>> +		acc_count = max34417->acc_count[chan->channel];
>> +		max34417->acc_power[chan->channel] = 0;
>> +		max34417->acc_count[chan->channel] = 0;
>> +
>> +		if (unlikely(acc_count == 0)) {
>> +			dev_warn(max34417->dev, "unexpected zero accumulator count\n");
> 
> [Severity: Medium]
> Could this non-rate-limited warning lead to a log-spam denial of service?
> 
> If an unprivileged userspace process repeatedly reads the attribute in a tight
> loop, it will extract data faster than the hardware's sample rate, causing the
> counter to evaluate to 0. This blindly prints a warning using dev_warn()
> without any rate limiting, which could flood the kernel log buffer and mask
> legitimate kernel messages. Should dev_warn_ratelimited() be used instead?

Perhaps yes

> 
>> +			*val = 0;
>> +			*val2 = 0;
>> +
>> +			return IIO_VAL_INT_64;
>> +		}
>> +
>> +		power = div_u64(acc_power, acc_count);
>> +	} else {
>> +		rc = max34417_read_acc_power(max34417, chan->address, &power);
>> +		if (rc)
>> +			return rc;
>> +
>> +		rc = max34417_reset_smm(max34417);
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	*val = FIELD_GET(GENMASK_ULL(31, 0), power);
>> +	*val2 = FIELD_GET(GENMASK_ULL(55, 32), power);
> 
> [Severity: High]
> Does this silently truncate the upper 8 bits of the 64-bit software
> accumulator?
> 
> GENMASK_ULL(55, 32) explicitly masks off bits 56-63, meaning any accumulated
> value exceeding 56 bits is silently truncated and dropped before being
> exported to IIO userspace. Since CAM mode continuously accumulates readings,
> the u64 can easily exceed 56 bits in sustained operation, causing the
> userspace reading to wrap around prematurely. Should this use
> GENMASK_ULL(63, 32) or upper_32_bits(power)?

Value from register is 56bit, there's no truncation.

> 
>> +
>> +	return IIO_VAL_INT_64;
>> +}
>> +
>> +static int max34417_read_energy(struct max34417_data *max34417,
>> +			       const struct iio_chan_spec *chan,
>> +			       int *val, int *val2)
>> +{
> 
> [ ... ]
> 
>> +	/* Get current channel power and reset them */
>> +	acc_power = max34417->acc_power[chan->channel];
>> +	max34417->acc_power[chan->channel] = 0;
>> +	max34417->acc_count[chan->channel] = 0;
>> +
>> +	*val = FIELD_GET(GENMASK_ULL(31, 0), acc_power);
>> +	*val2 = FIELD_GET(GENMASK_ULL(55, 32), acc_power);
> 
> [Severity: High]
> Does this also silently truncate the upper 8 bits of the 64-bit accumulator?
> 
> Just as in max34417_read_power(), this GENMASK_ULL(55, 32) truncates bits
> 56-63 of acc_power.

Value from register is 56bit, there's no truncation.

> 
> [ ... ]
> 
>> +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:
>> +		switch (chan->type) {
>> +		case IIO_VOLTAGE:
>> +			return max34417_read_voltage(max34417, chan, val);
>> +		case IIO_POWER:
>> +			/* In default mode, we return a single measurement */
>> +			if (max34417->cam_mode)
>> +				return -EINVAL;
>> +
>> +			return max34417_read_power(max34417, chan, val, val2);
> 
> [Severity: Medium]
> Could this lockless check of cam_mode lead to a TOCTOU race and return
> incorrect statistics?
> 
> Here, cam_mode is evaluated without holding the mutex. Concurrently, a sysfs
> write to in_energy_enable could call max34417_setup_cam_mode(), taking the
> mutex and setting cam_mode to true. When max34417_read_power() is then
> called, it acquires the mutex, sees cam_mode is true, and executes the CAM
> mode logic. This would unexpectedly return an averaged measurement instead of
> a single-shot reading, violating the expected IIO channel behavior.

Good catch

> 
> [ ... ]
> 
>> +static int max34417_probe(struct i2c_client *client)
>> +{
> 
> [ ... ]
> 
>> +	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;
>> +
>> +	rc = devm_iio_device_register(dev, indio_dev);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Reset mode and set accumulator width to Wide */
>> +	rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, MAX34417_CONTROL_MODE);
> 
> [Severity: Medium]
> Is it safe to register the IIO device before the hardware initialization
> completes?
> 
> devm_iio_device_register() exposes the device and its sysfs attributes to
> userspace immediately. A userspace process or udev rule could open the device
> and trigger read_raw or write_raw callbacks before the hardware is placed
> into the correct mode (MAX34417_CONTROL_MODE) or its internal accumulators
> are reset. Should the hardware configuration happen prior to registration?
> 

Good catch

Neil

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

[Andy Shevchenko]

On Tue, Oct 13, 2026 at 04:07:13PM +0200, Neil Armstrong wrote:
> The MAX34417 is a specialized current and voltage monitor used to
> determine power consumption of portable systems. The driver support
> getting the channels voltage, average power and accumulated average
> power and energy over an I2C/SMBUS serial interface.
> 
> The MAX34417 datasheet only specified on how to calculate the
> accumulated average power, but with the Sample Per Second rate
> we can easily get accumulated Energy in mJ.
> 
> By default the IIO driver only supports single-shot Voltage
> and Power sensing, but the accumulation can be enabled by
> enabling the Energy IIO channel which then enables the
> Continuous Accumulation Mode.
> 
> The counter is common to all channels, so when reading a single
> channel we will copy all accumulators to registers, so in order
> to permit reading individual channels we must accumulate the
> other channels in the driver when Continuous Accumulation Mode
> is enabled.

...

> +#define MAX34417_MIN_RSENSE		(MILLI)	/* 1 milliOhm */

1 * MILLI

> +#define MAX34417_MAX_RSENSE		(100 * MILLI) /* 100 milliOhm */
> +#define MAX34417_DEFAULT_RSENSE		MAX34417_MIN_RSENSE

I would rather put the exact value here as well.

...

> +struct max34417_data {
> +	struct iio_dev *indio_dev;

> +	struct regmap *regmap;
> +	struct device *dev;

One of them seems redundant (I suppose the 'dev').

> +	struct mutex lock;
> +	u32 input_correction[MAX34417_CHANNEL_COUNT];
> +	const char *input_label[MAX34417_CHANNEL_COUNT];
> +	bool cam_mode;
> +	u64 acc_power[MAX34417_CHANNEL_COUNT];
> +	u32 acc_count[MAX34417_CHANNEL_COUNT];
> +};
> +
> +static const struct regmap_config max34417_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= MAX34417_BULK_VOLTAGE_READOUT_REG,

No cache? All registers are volatile?

> +};

...

> +#define MAX34417_CHANNEL(_index, _v_address, _power_address)	\
> +	{							\
> +		.type = IIO_VOLTAGE,				\
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) | \
> +					  BIT(IIO_CHAN_INFO_SCALE), \

Instead of broken indentation at the end, use different style for these type of
assignments, like

		.info_mask_separate =
			BIT(IIO_CHAN_INFO_RAW) |		\
			BIT(IIO_CHAN_INFO_SCALE),		\

Also do not use brokenly indented '=', just a space after the field is enough.


> +		.channel = (_index),				\
> +		.address = (_v_address),			\
> +		.indexed = 1,					\
> +	},							\
> +	{							\
> +		.type = IIO_POWER,				\
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) | \
> +					  BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \
> +					  BIT(IIO_CHAN_INFO_SCALE), \
> +		.channel = (_index),				\
> +		.address = (_power_address),			\
> +		.indexed = 1,					\
> +	},							\
> +	{							\
> +		.type = IIO_ENERGY,				\
> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) | \
> +					  BIT(IIO_CHAN_INFO_SCALE), \
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_ENABLE), \
> +		.channel = (_index),				\
> +		.address = (_power_address),			\
> +		.indexed = 1,					\
> +	}

...

> +static int max34417_reset_smm(struct max34417_data *max34417)
> +{
> +	u32 control;
> +	int rc;
> +
> +	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
> +	if (rc)
> +		return rc;
> +
> +	/* Set SMM=0 to re-init the SMM mode */
> +	control &= ~(MAX34417_CONTROL_SMM);
> +	rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
> +	if (rc)
> +		return rc;

regmap_update_bits() / regmap_clear_bits()?

> +	/* Reset accumulators after setting SMM=0 */
> +	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;

regmap_set_bits() ?

> +	return 0;
> +}

...

Also check the rest of the code for the similar RMW cases.

...

> +	/*
> +	 * UPDATE commands should be no less than 1msec apart for reliable
> +	 * measurements.
> +	 */
> +	fsleep(1000);

1 * USEC_PER_MSEC

> +	u32 acc_count;
> +	int rc;
> +
> +	rc = max34417_read_acc_count(max34417, &acc_count);
> +	if (rc)
> +		return rc;
> +
> +	for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i) {

'i' is not used outside of the loop AFAICS. Also why pre-increment?

	for (int i = 0; i < MAX34417_CHANNEL_COUNT; i++) {

> +		rc = max34417_read_acc_power(max34417, MAX34417_PWR_ACC_1_REG + i,
> +					     &acc_power);
> +		if (rc)
> +			return rc;
> +
> +		max34417->acc_power[i] += acc_power;
> +		max34417->acc_count[i] += acc_count;
> +	}

...

> +	/* Check for overflow */
> +	rc = max34417_check_ovf(max34417);
> +	if (rc)
> +		return rc;
> +
> +	return 0;

	return ..._check_ovf(...);

...

> +	/* Ignore byte0 */
> +	voltage = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]);
> +	voltage >>= 2;

The above comment is odd. Please, clarify, ideally with a reference to
the datasheet table / section / et cetera.

Also can be folded into a single expression:

	voltage = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]) >> 2;

...

> +		if (unlikely(acc_count == 0)) {

Why unlikely()? Is this a hot-path? What is the performance effect will it give?

> +			dev_warn(max34417->dev, "unexpected zero accumulator count\n");
> +
> +			*val = 0;
> +			*val2 = 0;
> +
> +			return IIO_VAL_INT_64;
> +		}

...

> +	/*
> +	 * Move the data to the SMBus registers
> +	 */

A single line comment.

...

> +static int max34417_setup_cam_mode(struct max34417_data *max34417, int val)
> +{
> +	u32 control;
> +	int rc, i;
> +
> +	guard(mutex)(&max34417->lock);
> +
> +	rc = regmap_read(max34417->regmap, MAX34417_CONTROL_REG, &control);
> +	if (rc)
> +		return rc;
> +
> +	if (max34417->cam_mode && !val) {
> +		/* SMM and CAM modes should not be enabled at the same time */
> +		control &= ~(MAX34417_CONTROL_CAM | MAX34417_CONTROL_SMM);
> +		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
> +		if (rc)
> +			return rc;
> +
> +		/*
> +		 * If CAM mode must be set, the Control byte must be set to
> +		 * desired values with CAM = 0, then followed by an UPDATE
> +		 */
> +		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> +		if (rc)
> +			return rc;
> +	} else if (!max34417->cam_mode && val) {
> +		/*
> +		 * After this, the CAM mode bit can be set and another
> +		 * UPDATE must be sent.
> +		 */
> +		control |= MAX34417_CONTROL_CAM;
> +		/* SMM and CAM modes should not be enabled at the same time */
> +		control &= ~(MAX34417_CONTROL_SMM);
> +		rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, control);
> +		if (rc)
> +			return rc;
> +
> +		rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);
> +		if (rc)
> +			return rc;
> +
> +		/* Reset counters and accumulators */
> +		for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i) {

		for (int i = 0; i < MAX34417_CHANNEL_COUNT; i++) {

> +			max34417->acc_power[i] = 0;
> +			max34417->acc_count[i] = 0;
> +		}
> +	}
> +
> +	max34417->cam_mode = val;
> +
> +	return 0;
> +}

...

> +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:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			return max34417_read_voltage(max34417, chan, val);
> +		case IIO_POWER:
> +			/* In default mode, we return a single measurement */
> +			if (max34417->cam_mode)
> +				return -EINVAL;
> +
> +			return max34417_read_power(max34417, chan, val, val2);
> +		case IIO_ENERGY:
> +			if (!max34417->cam_mode)
> +				return -EINVAL;
> +
> +			return max34417_read_energy(max34417, chan, val, val2);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_AVERAGE_RAW:
> +		if (chan->type != IIO_POWER)
> +			return -EINVAL;
> +
> +		/* In CAM mode, we return an average measurement */
> +		if (!max34417->cam_mode)
> +			return -EINVAL;
> +
> +		return max34417_read_power(max34417, chan, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_VOLTAGE) {
> +			/* Scale to mV */
> +			*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
> +			 * (1000 * (Power Accumulation / Counter) * Correction) / 2^30
> +			 */
> +			*val = max34417->input_correction[chan->channel] * MILLI;

MILLIWATT_PER_WATT

> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		} else if (chan->type == IIO_ENERGY) {
> +			/*
> +			 * Scale to mJ
> +			 * P=(1000 * (Power Accumulation / Counter) * Correction) / 2^30
> +			 * t=(Counter / Samples per Second)
> +			 * E=(P * t)
> +			 * Can be simplified to:
> +			 * (1000 * Power Accumulation * Correction) / (2^30 * SPS)
> +			 * The denominator doesn't fit in 32bit so scale down both terms.
> +			 */
> +			*val = max34417->input_correction[chan->channel];
> +			*val2 = ((u64)(1 << MAX34417_PWR_AVG_FULL_SCALE_BITS) *

Why not BIT_ULL() and drop that casting altogether? Also seems that it becomes
64-bit division. Does it compile on 32-bit?

> +				 MAX34417_FAST_SPS_CAM) / MILLI;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_ENABLE:
> +		*val = max34417->cam_mode;
> +
> +		return IIO_VAL_INT;
> +	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);
> +	}
> +
> +	if (chan->type == IIO_ENERGY) {
> +		if (input_label)
> +			return sysfs_emit(label, "%s-energy\n", input_label);
> +
> +		return sysfs_emit(label, "channel%d-energy\n", chan->channel);
> +	}
> +
> +	return 0;
> +}

This can be simplified, right?

	if (input_label) {
		switch (chan->type) {
		case IIO_VOLTAGE:
			return sysfs_emit(label, "%s-voltage\n", input_label);
		...
		default:
			return 0;
		}
	}

	switch (chan->type) {
	case IIO_VOLTAGE:
		return sysfs_emit(label, "channel%d-voltage\n", chan->channel);
	...
	default:
		return 0;
	}

...

> +	mutex_init(&max34417->lock);

Why not devm_mutex_init()?

...

> +	/* Set default input correction for all channels */
> +	for (i = 0; i < MAX34417_CHANNEL_COUNT; ++i)

	for (int i = 0; i < MAX34417_CHANNEL_COUNT; i++)

> +		max34417->input_correction[i] =
> +			max34417_calc_input_correction(MAX34417_DEFAULT_RSENSE);

...

> +		if (fwnode_property_present(node, "shunt-resistor-micro-ohms")) {
> +			rc = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms",
> +						      &rsense);

Name the variable with units, rsense_mOhm.

> +			if (rc)
> +				return dev_err_probe(dev, rc,
> +						"failure reading shunt property of %pfwP\n",
> +						node);
> +
> +			if (!rsense || rsense < MAX34417_MIN_RSENSE ||
> +			    rsense > MAX34417_MAX_RSENSE)

Please, make it logically split

			// also == 0 is okay for the actual values
			// (which have not a semantic of pointer or error)
			if (rsense == 0 ||
			    rsense < MAX34417_MIN_RSENSE || rsense > MAX34417_MAX_RSENSE)

> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid shunt value %d of %pfwP\n",
> +						     rsense, node);
> +
> +			max34417->input_correction[index] =
> +				max34417_calc_input_correction(rsense);
> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko