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

[PATCH v6 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 Joules.

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 Analog Devices 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 v6:
- Fixed sashiko comments
  - Fixed the binding example indentation
  - Added a timestamp to make sure we don't call UPDATE in less than 1ms
  - Switched ENERGY to return Joules instead of mJ, simplifying the scale formlula and use IIO_VAL_FRACTIONAL_LOG2
- Dropped using SMM and using the default mode, simplifies the process for same result
- Added software overflow check
- Switch to s64/s32 everywhere to make sure we don't mess the sign since we return signed int values
- Added readable_noinc_reg to mark the data regs as noinc
- Added explicit comments on the OVF check, UPDATE sequencing and byte0 content
- Dropped the channel type in the channel label
- Moved the DT channel probe into a separate function
- Link to v5: https://patch.msgid.link/20261102-topic-sm8x50-iio-max34417-adc-v5-0-0ffa5e5f2d5c@linaro.org

Changes in v5:
- Collected review tags
- Fixed patch 2 commit message
- Added sysfs.h include
- Finished all struct max34417_data with .
- Added explicit MAX34417_CONTROL_REG: to is_volatile callback
- Added local *map to all functions, shortening regmap calls
- Explicited the 1ms UPDATE delay for manual mode
- Simplified max34417_check_ovf logic
- Simplified acc_count == 0 logic
- Used iio_val_s64_decompose
- Used regmap_update_bits to compine regmap clear and set
- Added switch instead of if/else for IIO_CHAN_INFO_SCALE
- Changed max34417_calc_input_correction comment and return type
- Switch to rsense_uOhm
- Return -ECHRNG for invalide channel count
- Link to v4: https://patch.msgid.link/20261028-topic-sm8x50-iio-max34417-adc-v4-0-d4baaf3ee903@linaro.org

Changes in v4:
- Fix bindings with Conor review on v2
 - Rename to Analog Devices and "adi" prefix
 - Move reg before #*-cells
 - Remove "label: true"
 - Make supplies required
 - Update DT example
- Rebase on linux-next
- Fix RSENSE defines
- Drop dev from max34417_data struct
- Move acc_count to u64
- Add regmap writeable/readable/volatile callbacks
- Use regmap_set/clear_bits() when needed
- Fix MAX34417_CHANNEL layout
- Drop useless comments
- Move i in the "for (int i...") and use i++ instead of ++i
- Move mutex guard into iio callbacks
- Use lower_32_bits/upper_32_bits instead of FIELD_GET()
- Use div_u64() when returning the Power scale to make sure it runs on 32bit systems
- Reorganize the get_label() callback
- Use devm_mutex_init
- Drop fwnode_property_present(node, "reg") before getting reg
- Rename rsense variable to rsense_microOhm
- Drop !rsense test
- Register iio device after device init
- Add comment about supporting MAX34407
- Link to v3: https://patch.msgid.link/20261013-topic-sm8x50-iio-max34417-adc-v3-0-d3604ae4a229@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>
To: Michael Hennerich <Michael.Hennerich@analog.com>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux@analog.com

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

 .../devicetree/bindings/iio/adc/adi,max34417.yaml  | 103 +++
 drivers/iio/adc/Kconfig                            |  11 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/max34417.c                         | 845 +++++++++++++++++++++
 include/linux/unaligned.h                          |  27 +
 5 files changed, 987 insertions(+)
---
base-commit: 54729cd85a044e8e2ee3f73ddda2d7c2fb39e199
change-id: 20260923-topic-sm8x50-iio-max34417-adc-209e880533fb

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

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

[Neil Armstrong]

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

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 .../devicetree/bindings/iio/adc/adi,max34417.yaml  | 103 +++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max34417.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max34417.yaml
new file mode 100644
index 000000000000..77934a9baa81
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,max34417.yaml
@@ -0,0 +1,103 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,max34417.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices 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: adi,max34417
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  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
+
+      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
+  - vdd-supply
+  - vio-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@10 {
+            compatible = "adi,max34417";
+            reg = <0x10>;
+
+            vdd-supply = <&max34417_vdd>;
+            vio-supply = <&max34417_vio>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+                label = "ch0";
+                shunt-resistor-micro-ohms = <5000>;
+            };
+
+            channel@1 {
+                reg = <1>;
+                label = "ch1";
+            };
+
+            channel@2 {
+                reg = <2>;
+            };
+        };
+    };

-- 
2.34.1

[PATCH v6 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]
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
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

[PATCH v6 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, 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 Joules.

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 | 845 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 857 insertions(+)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a802d8ee4dd9..b7d66dffdb89 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1144,6 +1144,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 MAX40080
 	tristate "Analog Devices MAX40080 Current Sense Amplifier"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 9e82c21dbd37..26ddf3828fe4 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -97,6 +97,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_MAX40080) += max40080.o
 obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
 obj-$(CONFIG_MAX9611) += max9611.o
diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
new file mode 100644
index 000000000000..f12d0e8ef1a6
--- /dev/null
+++ b/drivers/iio/adc/max34417.c
@@ -0,0 +1,845 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IIO driver for Analog Devices 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/ktime.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/sysfs.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		(1 * MILLI) /* 1000 microOhm */
+#define MAX34417_MAX_RSENSE		(100 * MILLI) /* 100000 microOhm */
+#define MAX34417_DEFAULT_RSENSE		(1 * MILLI) /* 1000 microOhm */
+
+#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.
+ * @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.
+ * @acc_power:		Saved power accumulator for each channel when in CAM mode.
+ * @acc_count:		Saved counter for each channel when in CAM mode.
+ * @overflow:		Indicates if a channel saved data has overflowed.
+ * @last_update:	Timestamp of the last UPDATE call in CAM mode.
+ * @cam_mode:		State of the Continuous Accumulate Mode feature.
+ */
+struct max34417_data {
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	struct mutex lock;
+	u32 input_correction[MAX34417_CHANNEL_COUNT];
+	const char *input_label[MAX34417_CHANNEL_COUNT];
+	s64 acc_power[MAX34417_CHANNEL_COUNT];
+	s64 acc_count[MAX34417_CHANNEL_COUNT];
+	bool overflow[MAX34417_CHANNEL_COUNT];
+	ktime_t last_update;
+	bool cam_mode;
+};
+
+static bool max34417_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MAX34417_UPDATE_REG:
+	case MAX34417_CONTROL_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool max34417_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MAX34417_UPDATE_REG:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static bool max34417_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MAX34417_DID_REG:
+		return false;
+	/* MAX34417_CONTROL_REG field OVF is volatile */
+	case MAX34417_CONTROL_REG:
+	default:
+		return true;
+	}
+}
+
+static bool max34417_readable_noinc_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MAX34417_UPDATE_REG:
+	case MAX34417_CONTROL_REG:
+	case MAX34417_DID_REG:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static const struct regmap_config max34417_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= MAX34417_BULK_VOLTAGE_READOUT_REG,
+	.writeable_reg	= max34417_writeable_reg,
+	.readable_reg	= max34417_readable_reg,
+	.volatile_reg	= max34417_volatile_reg,
+	/*
+	 * While the DATA registers are not FIFOs, each register
+	 * type has a different byte count and requires a
+	 * Block Read transaction to read the value
+	 */
+	.readable_noinc_reg = max34417_readable_noinc_reg,
+	/* No cache since the only non-volatile register is read-only */
+};
+
+#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_read_acc_count(struct max34417_data *max34417, u32 *acc_count)
+{
+	u8 buf[MAX34417_ACC_COUNT_READ_LENGTH];
+	struct regmap *map = max34417->regmap;
+	int rc;
+
+	rc = regmap_noinc_read(map, MAX34417_ACC_COUNT_REG, &buf, sizeof(buf));
+	if (rc)
+		return rc;
+
+	/* Ignore first byte which contains the Byte Count */
+	*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,
+				   s64 *acc_power)
+{
+	struct regmap *map = max34417->regmap;
+	u8 buf[MAX34417_POWER_READ_LENGTH];
+	int rc;
+
+	rc = regmap_noinc_read(map, address, &buf, sizeof(buf));
+	if (rc)
+		return rc;
+
+	/* Ignore first byte which contains the Byte Count */
+	*acc_power = get_unaligned_be56(&buf[MAX34417_POWER_READ_OFFSET]);
+
+	return 0;
+}
+
+/*
+ * Check the OVF status bit indicating if one of the channels
+ * has reached overflow, meaning one of the accumulators has
+ * saturated and the MAX34417 stopped measuring.
+ * The registers contains valid measurements, not wrapped,
+ * and were already added to the local context accumulators.
+ * The OVF bit is required to be cleared followed by an UPDATE
+ * command to resume the accumulation mode sequence.
+ */
+static int max34417_check_ovf(struct max34417_data *max34417)
+{
+	struct regmap *map = max34417->regmap;
+	u32 control;
+	int rc;
+
+	rc = regmap_read(map, MAX34417_CONTROL_REG, &control);
+	if (rc)
+		return rc;
+
+	if (!(control & MAX34417_CONTROL_OVF))
+		return 0;
+
+	rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
+			       MAX34417_CONTROL_OVF);
+	if (rc)
+		return rc;
+
+	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	/* Save the timestamp for the next read */
+	max34417->last_update = ktime_get();
+
+	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)
+{
+	s64 acc_power;
+	s32 acc_count;
+	int rc;
+
+	rc = max34417_read_acc_count(max34417, &acc_count);
+	if (rc)
+		return rc;
+
+	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;
+
+		/* Checked for overflow on local accumulators */
+		if (check_add_overflow(max34417->acc_power[i], acc_power,
+				       &max34417->acc_power[i]) ||
+		    check_add_overflow(max34417->acc_count[i], acc_count,
+				       &max34417->acc_count[i])) {
+			max34417->overflow[i] = true;
+		}
+	}
+
+	/* Check for overflow */
+	return max34417_check_ovf(max34417);
+}
+
+static int max34417_read_voltage(struct max34417_data *max34417,
+				 const struct iio_chan_spec *chan, int *val)
+{
+	s64 delta_us = ktime_us_delta(ktime_get(), max34417->last_update);
+	struct regmap *map = max34417->regmap;
+	u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
+	int rc;
+
+	/* Sleep up to 1msec to make sure we have a measurement cycle done */
+	if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
+		fsleep((1 * USEC_PER_MSEC) - delta_us);
+
+	/*
+	 * When not in CAM mode, we use the default mode where each
+	 * UPDATE command will move the current accumulated data to the
+	 * SMbus registers, reset those accumulators and run a new
+	 * measurement cycle.
+	 */
+	if (!max34417->cam_mode) {
+		/* Reset the accumulators and perform a measurement cycle */
+		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+		if (rc)
+			return rc;
+		/*
+		 * UPDATE commands should be no less than 1msec apart for reliable
+		 * measurements, since in Manual mode the SPS is 1kHz, we need wait
+		 * for the measurement to finish before moving the data in the SMbus
+		 * registers.
+		 */
+		fsleep(1 * USEC_PER_MSEC);
+	}
+
+	/*
+	 * Move the new UPDATE data into the SMBus read registers.
+	 * This will also reset the accumulators, returning the
+	 * power accumulated since the last UPDATE.
+	 */
+	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	/* Save the timestamp for the next read */
+	max34417->last_update = ktime_get();
+
+	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(map, chan->address, &buf, sizeof(buf));
+	if (rc)
+		return rc;
+
+	/* Ignore first byte which contains the Byte Count & data starts at Bit 2 */
+	*val = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]) >> 2;
+
+	return IIO_VAL_INT;
+}
+
+static int max34417_read_power(struct max34417_data *max34417,
+			       const struct iio_chan_spec *chan,
+			       int *val, int *val2)
+{
+	s64 delta_us = ktime_us_delta(ktime_get(), max34417->last_update);
+	struct regmap *map = max34417->regmap;
+	s64 power;
+	int rc;
+
+	/* Sleep up to 1msec to make sure we have a measurement cycle done */
+	if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
+		fsleep((1 * USEC_PER_MSEC) - delta_us);
+
+	/*
+	 * When not in CAM mode, we use the default mode where each
+	 * UPDATE command will move the current accumulated data to the
+	 * SMbus registers, reset those accumulators and run a new
+	 * measurement cycle.
+	 */
+	if (!max34417->cam_mode) {
+		/* Reset the accumulators and perform a measurement cycle */
+		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+		if (rc)
+			return rc;
+		/*
+		 * UPDATE commands should be no less than 1msec apart for reliable
+		 * measurements, since in Manual mode the SPS is 1kHz, we need wait
+		 * for the measurement to finish before moving the data in the SMbus
+		 * registers.
+		 */
+		fsleep(1 * USEC_PER_MSEC);
+	}
+
+	/*
+	 * Move the new UPDATE data into the SMBus read registers.
+	 * This will also reset the accumulators, returning the
+	 * power accumulated since the last UPDATE.
+	 */
+	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	/* Save the timestamp for the next read */
+	max34417->last_update = ktime_get();
+
+	/*
+	 * Only get the accumulator counter in CAM mode, in normal mode
+	 * we only accumulate once, so the counter is 1
+	 */
+	if (max34417->cam_mode) {
+		s64 acc_count;
+		s64 acc_power;
+
+		/* Retrieve all channels to avoid missing data for other channels */
+		rc = max34417_save_accumulators(max34417);
+		if (rc)
+			return rc;
+
+		if (max34417->overflow[chan->channel]) {
+			max34417->overflow[chan->channel] = false;
+			max34417->acc_power[chan->channel] = 0;
+			max34417->acc_count[chan->channel] = 0;
+			return -EOVERFLOW;
+		}
+
+		/* 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 (acc_count == 0) {
+			dev_warn_ratelimited(&max34417->indio_dev->dev,
+					     "unexpected zero accumulator count\n");
+			power = 0;
+		} else {
+			power = div64_s64(acc_power, acc_count);
+		}
+	} else {
+		rc = max34417_read_acc_power(max34417, chan->address, &power);
+		if (rc)
+			return rc;
+	}
+
+	iio_val_s64_decompose(power, val, val2);
+
+	return IIO_VAL_INT_64;
+}
+
+static int max34417_read_energy(struct max34417_data *max34417,
+			       const struct iio_chan_spec *chan,
+			       int *val, int *val2)
+{
+	struct regmap *map = max34417->regmap;
+	s64 delta_us = ktime_us_delta(ktime_get(), max34417->last_update);
+	s64 acc_power;
+	int rc;
+
+	/* Sleep up to 1msec to make sure we have a measurement cycle done */
+	if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
+		fsleep((1 * USEC_PER_MSEC) - delta_us);
+
+	/* Move the data to the SMBus registers */
+	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	/* Save the timestamp for the next read */
+	max34417->last_update = ktime_get();
+
+	/* Retrieve all channels to avoid missing data for other channels */
+	rc = max34417_save_accumulators(max34417);
+	if (rc)
+		return rc;
+
+	if (max34417->overflow[chan->channel]) {
+		max34417->overflow[chan->channel] = false;
+		max34417->acc_power[chan->channel] = 0;
+		max34417->acc_count[chan->channel] = 0;
+		return -EOVERFLOW;
+	}
+
+	/* 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;
+
+	iio_val_s64_decompose(acc_power, val, val2);
+
+	return IIO_VAL_INT_64;
+}
+
+/*
+ * The Continuous Accumulation Mode (CAM) will automatically
+ * run measurement cycles 1536 times per second and update
+ * the ACC_COUNT register, while in normal mode the measument
+ * cycle is only done after an UPDATE command.
+ * The Single Measurement Mode (SMM) is like CAM, but each
+ * measurement cycle is triggered with an UPDATE command
+ * but the accumulators registers are not cleared out on the
+ * UPDATE command.
+ */
+static int max34417_setup_cam_mode(struct max34417_data *max34417, int val)
+{
+	struct regmap *map = max34417->regmap;
+	int rc;
+
+	if (max34417->cam_mode && !val) {
+		rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
+				       MAX34417_CONTROL_CAM);
+		if (rc)
+			return rc;
+
+		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+		if (rc)
+			return rc;
+	} else if (!max34417->cam_mode && val) {
+		/*
+		 * 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_clear_bits(map, MAX34417_CONTROL_REG,
+				       MAX34417_CONTROL_CAM);
+		if (rc)
+			return rc;
+
+		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+		if (rc)
+			return rc;
+
+		rc = regmap_set_bits(map, MAX34417_CONTROL_REG, MAX34417_CONTROL_CAM);
+		if (rc)
+			return rc;
+
+		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+		if (rc)
+			return rc;
+
+		/* Reset counters and accumulators */
+		for (int i = 0; i < MAX34417_CHANNEL_COUNT; i++) {
+			max34417->acc_power[i] = 0;
+			max34417->acc_count[i] = 0;
+			max34417->overflow[i] = false;
+		}
+	}
+
+	/* Save the timestamp for the next read */
+	max34417->last_update = ktime_get();
+
+	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);
+
+	guard(mutex)(&max34417->lock);
+
+	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:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			/* Scale to mV */
+			*val = MAX34417_VOLTAGE_CORRECTION_SCALE * MILLI;
+			*val2 = MAX34417_VOLTAGE_FULL_SCALE_BITS;
+
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_POWER:
+			/*
+			 * Scale to mW
+			 * (1000 * (Power Accumulation / Counter) * Correction) / 2^30
+			 */
+			*val = max34417->input_correction[chan->channel] * MILLIWATT_PER_WATT;
+			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS;
+
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_ENERGY:
+			/*
+			 * Scale to Joules
+			 * P=((Power Accumulation / Counter) * Correction) / 2^30
+			 * t=(Counter / Samples per Second)
+			 * E=(P * t)
+			 * Can be simplified to:
+			 * (Power Accumulation * Correction) / (2^30 * SPS)
+			 * But the denominator won't fit in 32bit and is not power of 2.
+			 * But we can divide the numerator by SPS and shift both terms
+			 * by a large power of two number and still use IIO_VAL_FRACTIONAL_LOG2
+			 */
+			*val = div_u64(max34417->input_correction[chan->channel] << 19,
+				       MAX34417_FAST_SPS_CAM);
+			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS + 19;
+
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			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);
+
+	guard(mutex)(&max34417->lock);
+
+	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 (input_label)
+		return sysfs_emit(label, "%s\n", input_label);
+
+	return sysfs_emit(label, "channel%d\n", chan->channel);
+}
+
+static const struct iio_info max34417_info = {
+	.read_raw	= max34417_read_raw,
+	.write_raw	= max34417_write_raw,
+	.read_label	= max34417_read_label,
+};
+
+static u32 max34417_calc_input_correction(u32 rsense_uOhm)
+{
+	/* Derived from Table 14. Correction Factors for Various Current-Sense Resistor Values */
+	return (MAX34417_MAX_RSENSE * MAX34417_PWR_CORRECTION_SCALE) / rsense_uOhm;
+}
+
+static int max34417_probe_channel_node(struct device *dev,
+				       struct max34417_data *max34417,
+				       struct fwnode_handle *node)
+{
+	u32 rsense_uOhm, index;
+	int rc;
+
+	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, -ECHRNG, "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_uOhm);
+		if (rc)
+			return dev_err_probe(dev, rc,
+					     "failure reading shunt property of %pfwP\n",
+					     node);
+
+		if (rsense_uOhm < MAX34417_MIN_RSENSE ||
+		    rsense_uOhm > MAX34417_MAX_RSENSE)
+			return dev_err_probe(dev, -EINVAL,
+					     "invalid shunt value %d of %pfwP\n",
+					     rsense_uOhm, node);
+
+		max34417->input_correction[index] =
+			max34417_calc_input_correction(rsense_uOhm);
+	}
+
+	return 0;
+}
+
+static int max34417_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct max34417_data *max34417;
+	struct iio_dev *indio_dev;
+	struct regmap *map;
+	int rc;
+
+	map = devm_regmap_init_i2c(client, &max34417_regmap_config);
+	if (IS_ERR(map))
+		return dev_err_probe(dev, PTR_ERR(map), "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 = map;
+	max34417->indio_dev = indio_dev;
+
+	rc = devm_mutex_init(dev, &max34417->lock);
+	if (rc)
+		return rc;
+
+	/* Set default input correction for all channels */
+	for (int 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) {
+		rc = max34417_probe_channel_node(dev, max34417, node);
+		if (rc)
+			return rc;
+	}
+
+	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;
+
+	/*
+	 * Reset mode and set accumulator width to Wide
+	 *
+	 * The MAX34407 variant could be supported by setting the
+	 * MAX34417_CONTROL_MODE field to 0 and handling a smaller
+	 * accumulator width.
+	 */
+	rc = regmap_write(map, MAX34417_CONTROL_REG, MAX34417_CONTROL_MODE);
+	if (rc)
+		return dev_err_probe(dev, rc, "Error writing control register\n");
+
+	/* Reset the accumulators */
+	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
+	if (rc)
+		return rc;
+
+	/* Save the timestamp for the next read */
+	max34417->last_update = ktime_get();
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id max34417_of_match[] = {
+	{ .compatible = "adi,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("Analog Devices MAX34417 ADC driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v6 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 3 potential issue(s) to consider:
- [Medium] iio: adc: max34417: energy scale underflows to zero in max34417_read_raw()
- [Medium] iio: adc: max34417: shared accumulators clobbered on CAM power/energy read
- [Medium] iio: adc: max34417: missing 1 ms delay and premature UPDATE on read/setup
--

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

iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

This commit adds an IIO ADC driver for the Analog Devices MAX34417
four-channel power accumulator, supporting single-shot voltage and power
readings as well as continuous accumulation mode for average power and
energy measurements over I2C/SMBus.

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

[ ... ]

> +static int max34417_check_ovf(struct max34417_data *max34417)
> +{
> +	struct regmap *map = max34417->regmap;
> +	u32 control;
> +	int rc;
> +
> +	rc = regmap_read(map, MAX34417_CONTROL_REG, &control);
> +	if (rc)
> +		return rc;
> +
> +	if (!(control & MAX34417_CONTROL_OVF))
> +		return 0;
> +
> +	rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
> +			       MAX34417_CONTROL_OVF);
> +	if (rc)
> +		return rc;
> +
> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/* Save the timestamp for the next read */
> +	max34417->last_update = ktime_get();
> +
> +	return 0;
> +}

[ ... ]

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

[ ... ]

> +	/*
> +	 * Move the new UPDATE data into the SMBus read registers.
> +	 * This will also reset the accumulators, returning the
> +	 * power accumulated since the last UPDATE.
> +	 */
> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/* Save the timestamp for the next read */
> +	max34417->last_update = ktime_get();
> +
> +	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(map, chan->address, &buf, sizeof(buf));
> +	if (rc)
> +		return rc;
> +
> +	/* Ignore first byte which contains the Byte Count & data starts at Bit 2 */
> +	*val = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]) >> 2;
> +
> +	return IIO_VAL_INT;
> +}

[Severity: Medium]
As reported by Sashiko on v5 [1], in drivers/iio/adc/max34417.c, can the
MAX34417_UPDATE_REG sequencing in max34417_check_ovf(),
max34417_read_voltage(), and max34417_setup_cam_mode() violate the 1 ms
minimum interval between UPDATE commands or overwrite latched voltage data?

In max34417_read_voltage(), max34417_read_power(), and
max34417_read_energy(), the caller writes MAX34417_UPDATE_REG and then
immediately calls max34417_save_accumulators() -> max34417_check_ovf(). If
MAX34417_CONTROL_OVF is set, max34417_check_ovf() clears OVF and writes
MAX34417_UPDATE_REG a second time without waiting 1 ms after the caller's
UPDATE command.

Furthermore, in max34417_read_voltage(), max34417_save_accumulators() is
called before regmap_noinc_read(map, chan->address, &buf, sizeof(buf)). If
MAX34417_CONTROL_OVF was set because an accumulator saturated and stopped
measuring, won't the second UPDATE write inside max34417_check_ovf()
overwrite the latched SMBus registers before the voltage register is read?

Similarly, in max34417_setup_cam_mode():

	} else if (!max34417->cam_mode && val) {
		rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
				       MAX34417_CONTROL_CAM);
		if (rc)
			return rc;

		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
		if (rc)
			return rc;

		rc = regmap_set_bits(map, MAX34417_CONTROL_REG, MAX34417_CONTROL_CAM);
		if (rc)
			return rc;

		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);

the function does not check max34417->last_update before its first write to
MAX34417_UPDATE_REG, and when enabling CAM mode it writes
MAX34417_UPDATE_REG twice back-to-back without a 1 ms delay.

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

[ ... ]

> +	if (max34417->cam_mode) {
> +		s64 acc_count;
> +		s64 acc_power;
> +
> +		/* Retrieve all channels to avoid missing data for other channels */
> +		rc = max34417_save_accumulators(max34417);
> +		if (rc)
> +			return rc;
> +
> +		if (max34417->overflow[chan->channel]) {
> +			max34417->overflow[chan->channel] = false;
> +			max34417->acc_power[chan->channel] = 0;
> +			max34417->acc_count[chan->channel] = 0;
> +			return -EOVERFLOW;
> +		}
> +
> +		/* 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 (acc_count == 0) {
> +			dev_warn_ratelimited(&max34417->indio_dev->dev,
> +					     "unexpected zero accumulator count\n");
> +			power = 0;
> +		} else {
> +			power = div64_s64(acc_power, acc_count);
> +		}
> +	} else {
> +		rc = max34417_read_acc_power(max34417, chan->address, &power);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	iio_val_s64_decompose(power, val, val2);
> +
> +	return IIO_VAL_INT_64;
> +}
> +
> +static int max34417_read_energy(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{
> +	struct regmap *map = max34417->regmap;
> +	s64 delta_us = ktime_us_delta(ktime_get(), max34417->last_update);
> +	s64 acc_power;
> +	int rc;
> +
> +	/* Sleep up to 1msec to make sure we have a measurement cycle done */
> +	if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
> +		fsleep((1 * USEC_PER_MSEC) - delta_us);
> +
> +	/* Move the data to the SMBus registers */
> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/* Save the timestamp for the next read */
> +	max34417->last_update = ktime_get();
> +
> +	/* Retrieve all channels to avoid missing data for other channels */
> +	rc = max34417_save_accumulators(max34417);
> +	if (rc)
> +		return rc;
> +
> +	if (max34417->overflow[chan->channel]) {
> +		max34417->overflow[chan->channel] = false;
> +		max34417->acc_power[chan->channel] = 0;
> +		max34417->acc_count[chan->channel] = 0;
> +		return -EOVERFLOW;
> +	}
> +
> +	/* 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;
> +
> +	iio_val_s64_decompose(acc_power, val, val2);
> +
> +	return IIO_VAL_INT_64;
> +}

[Severity: Medium]
As reported by Sashiko on v5 [2], when cam_mode is enabled in
drivers/iio/adc/max34417.c, won't reading average power in
max34417_read_power() and reading energy in max34417_read_energy() clobber
each other's state for the same channel?

Both functions consume and zero the shared per-channel fields
max34417->acc_power[chan->channel], max34417->acc_count[chan->channel], and
max34417->overflow[chan->channel]. As a result, reading in_powerX_mean_raw
discards the energy accumulated in software for channel X since the last
energy read, and reading in_energyX_raw resets the accumulated power and
sample counter needed for the next average power calculation (as well as
resetting the energy reading itself on every read instead of accumulating
monotonically while CAM mode is enabled).

[ ... ]

> +static int max34417_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{

[ ... ]

> +		case IIO_ENERGY:
> +			/*
> +			 * Scale to Joules
> +			 * P=((Power Accumulation / Counter) * Correction) / 2^30
> +			 * t=(Counter / Samples per Second)
> +			 * E=(P * t)
> +			 * Can be simplified to:
> +			 * (Power Accumulation * Correction) / (2^30 * SPS)
> +			 * But the denominator won't fit in 32bit and is not power of 2.
> +			 * But we can divide the numerator by SPS and shift both terms
> +			 * by a large power of two number and still use IIO_VAL_FRACTIONAL_LOG2
> +			 */
> +			*val = div_u64(max34417->input_correction[chan->channel] << 19,
> +				       MAX34417_FAST_SPS_CAM);
> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS + 19;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}

[ ... ]

> +static u32 max34417_calc_input_correction(u32 rsense_uOhm)
> +{
> +	/* Derived from Table 14. Correction Factors for Various Current-Sense Resistor Values */
> +	return (MAX34417_MAX_RSENSE * MAX34417_PWR_CORRECTION_SCALE) / rsense_uOhm;
> +}

[Severity: Medium]
As reported by Sashiko on v5 [3], doesn't the IIO_ENERGY scale calculation in
max34417_read_raw() underflow to zero in sysfs for almost all supported
shunt resistor values?

In max34417_probe_channel_node(), shunt-resistor-micro-ohms is accepted in
the range 1000 to 100000 micro-Ohms. max34417_calc_input_correction()
computes 2400000 / rsense_uOhm with integer division (which also truncates
non-integer correction factors such as 2400000 / 90000 = 26 instead of
26.667), yielding input_correction in the range [24, 2400].

In max34417_read_raw() for IIO_CHAN_INFO_SCALE on IIO_ENERGY, *val is in the
range [8192, 819200] and *val2 is 49 with IIO_VAL_FRACTIONAL_LOG2. When
__iio_format_value() in drivers/iio/industrialio-core.c formats
IIO_VAL_FRACTIONAL_LOG2:

	case IIO_VAL_FRACTIONAL_LOG2:
		tmp2 = shift_right((s64)vals[0] * 1000000000LL, vals[1]);
		tmp0 = (int)div_s64_rem(tmp2, 1000000000LL, &tmp1);

because 2^49 is 562,949,953,421,312, any rsense_uOhm >= 1456 micro-Ohms
produces vals[0] <= 562517, so (s64)vals[0] * 1000000000LL < 2^49 and tmp2
becomes 0, causing in_energyX_scale to read as 0.000000000. Even at 1000
micro-Ohms, tmp2 truncates to 1 (0.000000001 instead of 0.0000000014551915,
losing 31.3% precision), and in-kernel consumers such as
rescale_process_scale() in drivers/iio/afe/iio-rescale.c cast ULL(1) << *val2
to int, truncating 1ULL << 49 to 0. Would returning a 64-bit decimal scale
type such as IIO_VAL_DECIMAL64_FEMTO via iio_val_s64_decompose() avoid this
underflow?

[1] https://lore.kernel.org/r/20261102-topic-sm8x50-iio-max34417-adc-v5-3-0ffa5e5f2d5c@linaro.org
[2] https://lore.kernel.org/r/20261102-topic-sm8x50-iio-max34417-adc-v5-3-0ffa5e5f2d5c@linaro.org
[3] https://lore.kernel.org/r/20261102-topic-sm8x50-iio-max34417-adc-v5-3-0ffa5e5f2d5c@linaro.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-topic-sm8x50-iio-max34417-adc-v6-0-63b4e8279633@linaro.org?part=3

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

[Neil Armstrong]

On 11/5/26 14:00, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] iio: adc: max34417: energy scale underflows to zero in max34417_read_raw()
> - [Medium] iio: adc: max34417: shared accumulators clobbered on CAM power/energy read
> - [Medium] iio: adc: max34417: missing 1 ms delay and premature UPDATE on read/setup
> --
> 
> commit b2b3b8bbfcafc2973680f358dc7e0d74750a2a05
> Author: Neil Armstrong <neil.armstrong@linaro.org>
> 
> iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator
> 
> This commit adds an IIO ADC driver for the Analog Devices MAX34417
> four-channel power accumulator, supporting single-shot voltage and power
> readings as well as continuous accumulation mode for average power and
> energy measurements over I2C/SMBus.
> 
>> diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
>> new file mode 100644
>> index 0000000000000..f12d0e8ef1a68
>> --- /dev/null
>> +++ b/drivers/iio/adc/max34417.c
> 
> [ ... ]
> 
>> +static int max34417_check_ovf(struct max34417_data *max34417)
>> +{
>> +	struct regmap *map = max34417->regmap;
>> +	u32 control;
>> +	int rc;
>> +
>> +	rc = regmap_read(map, MAX34417_CONTROL_REG, &control);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (!(control & MAX34417_CONTROL_OVF))
>> +		return 0;
>> +
>> +	rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
>> +			       MAX34417_CONTROL_OVF);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Save the timestamp for the next read */
>> +	max34417->last_update = ktime_get();
>> +
>> +	return 0;
>> +}
> 
> [ ... ]
> 
>> +static int max34417_read_voltage(struct max34417_data *max34417,
>> +				 const struct iio_chan_spec *chan, int *val)
>> +{
> 
> [ ... ]
> 
>> +	/*
>> +	 * Move the new UPDATE data into the SMBus read registers.
>> +	 * This will also reset the accumulators, returning the
>> +	 * power accumulated since the last UPDATE.
>> +	 */
>> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Save the timestamp for the next read */
>> +	max34417->last_update = ktime_get();
>> +
>> +	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(map, chan->address, &buf, sizeof(buf));
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Ignore first byte which contains the Byte Count & data starts at Bit 2 */
>> +	*val = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]) >> 2;
>> +
>> +	return IIO_VAL_INT;
>> +}
> 
> [Severity: Medium]
> As reported by Sashiko on v5 [1], in drivers/iio/adc/max34417.c, can the
> MAX34417_UPDATE_REG sequencing in max34417_check_ovf(),
> max34417_read_voltage(), and max34417_setup_cam_mode() violate the 1 ms
> minimum interval between UPDATE commands or overwrite latched voltage data?
> 
> In max34417_read_voltage(), max34417_read_power(), and
> max34417_read_energy(), the caller writes MAX34417_UPDATE_REG and then
> immediately calls max34417_save_accumulators() -> max34417_check_ovf(). If
> MAX34417_CONTROL_OVF is set, max34417_check_ovf() clears OVF and writes
> MAX34417_UPDATE_REG a second time without waiting 1 ms after the caller's
> UPDATE command.

Yes because the measurement has stalled, so it's ok here.

> 
> Furthermore, in max34417_read_voltage(), max34417_save_accumulators() is
> called before regmap_noinc_read(map, chan->address, &buf, sizeof(buf)). If
> MAX34417_CONTROL_OVF was set because an accumulator saturated and stopped
> measuring, won't the second UPDATE write inside max34417_check_ovf()
> overwrite the latched SMBus registers before the voltage register is read?

No

> 
> Similarly, in max34417_setup_cam_mode():
> 
> 	} else if (!max34417->cam_mode && val) {
> 		rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
> 				       MAX34417_CONTROL_CAM);
> 		if (rc)
> 			return rc;
> 
> 		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> 		if (rc)
> 			return rc;
> 
> 		rc = regmap_set_bits(map, MAX34417_CONTROL_REG, MAX34417_CONTROL_CAM);
> 		if (rc)
> 			return rc;
> 
> 		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> 
> the function does not check max34417->last_update before its first write to
> MAX34417_UPDATE_REG, and when enabling CAM mode it writes
> MAX34417_UPDATE_REG twice back-to-back without a 1 ms delay.

No because changing the mode will reset the counters, the 1ms is only
to get valid data between UPDATE commands, not configuration changes.

> 
>> +static int max34417_read_power(struct max34417_data *max34417,
>> +			       const struct iio_chan_spec *chan,
>> +			       int *val, int *val2)
>> +{
> 
> [ ... ]
> 
>> +	if (max34417->cam_mode) {
>> +		s64 acc_count;
>> +		s64 acc_power;
>> +
>> +		/* Retrieve all channels to avoid missing data for other channels */
>> +		rc = max34417_save_accumulators(max34417);
>> +		if (rc)
>> +			return rc;
>> +
>> +		if (max34417->overflow[chan->channel]) {
>> +			max34417->overflow[chan->channel] = false;
>> +			max34417->acc_power[chan->channel] = 0;
>> +			max34417->acc_count[chan->channel] = 0;
>> +			return -EOVERFLOW;
>> +		}
>> +
>> +		/* 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 (acc_count == 0) {
>> +			dev_warn_ratelimited(&max34417->indio_dev->dev,
>> +					     "unexpected zero accumulator count\n");
>> +			power = 0;
>> +		} else {
>> +			power = div64_s64(acc_power, acc_count);
>> +		}
>> +	} else {
>> +		rc = max34417_read_acc_power(max34417, chan->address, &power);
>> +		if (rc)
>> +			return rc;
>> +	}
>> +
>> +	iio_val_s64_decompose(power, val, val2);
>> +
>> +	return IIO_VAL_INT_64;
>> +}
>> +
>> +static int max34417_read_energy(struct max34417_data *max34417,
>> +			       const struct iio_chan_spec *chan,
>> +			       int *val, int *val2)
>> +{
>> +	struct regmap *map = max34417->regmap;
>> +	s64 delta_us = ktime_us_delta(ktime_get(), max34417->last_update);
>> +	s64 acc_power;
>> +	int rc;
>> +
>> +	/* Sleep up to 1msec to make sure we have a measurement cycle done */
>> +	if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
>> +		fsleep((1 * USEC_PER_MSEC) - delta_us);
>> +
>> +	/* Move the data to the SMBus registers */
>> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* Save the timestamp for the next read */
>> +	max34417->last_update = ktime_get();
>> +
>> +	/* Retrieve all channels to avoid missing data for other channels */
>> +	rc = max34417_save_accumulators(max34417);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (max34417->overflow[chan->channel]) {
>> +		max34417->overflow[chan->channel] = false;
>> +		max34417->acc_power[chan->channel] = 0;
>> +		max34417->acc_count[chan->channel] = 0;
>> +		return -EOVERFLOW;
>> +	}
>> +
>> +	/* 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;
>> +
>> +	iio_val_s64_decompose(acc_power, val, val2);
>> +
>> +	return IIO_VAL_INT_64;
>> +}
> 
> [Severity: Medium]
> As reported by Sashiko on v5 [2], when cam_mode is enabled in
> drivers/iio/adc/max34417.c, won't reading average power in
> max34417_read_power() and reading energy in max34417_read_energy() clobber
> each other's state for the same channel?
> 
> Both functions consume and zero the shared per-channel fields
> max34417->acc_power[chan->channel], max34417->acc_count[chan->channel], and
> max34417->overflow[chan->channel]. As a result, reading in_powerX_mean_raw
> discards the energy accumulated in software for channel X since the last
> energy read, and reading in_energyX_raw resets the accumulated power and
> sample counter needed for the next average power calculation (as well as
> resetting the energy reading itself on every read instead of accumulating
> monotonically while CAM mode is enabled).

No, the hardware works like that, when calling UPDATE it resets the accumulators,
so this simply emulates the hardware by accumulating in software for the other
channels to avoid data loss.

> 
> [ ... ]
> 
>> +static int max34417_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val, int *val2, long mask)
>> +{
> 
> [ ... ]
> 
>> +		case IIO_ENERGY:
>> +			/*
>> +			 * Scale to Joules
>> +			 * P=((Power Accumulation / Counter) * Correction) / 2^30
>> +			 * t=(Counter / Samples per Second)
>> +			 * E=(P * t)
>> +			 * Can be simplified to:
>> +			 * (Power Accumulation * Correction) / (2^30 * SPS)
>> +			 * But the denominator won't fit in 32bit and is not power of 2.
>> +			 * But we can divide the numerator by SPS and shift both terms
>> +			 * by a large power of two number and still use IIO_VAL_FRACTIONAL_LOG2
>> +			 */
>> +			*val = div_u64(max34417->input_correction[chan->channel] << 19,
>> +				       MAX34417_FAST_SPS_CAM);
>> +			*val2 = MAX34417_PWR_AVG_FULL_SCALE_BITS + 19;
>> +
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		default:
>> +			return -EINVAL;
>> +		}
> 
> [ ... ]
> 
>> +static u32 max34417_calc_input_correction(u32 rsense_uOhm)
>> +{
>> +	/* Derived from Table 14. Correction Factors for Various Current-Sense Resistor Values */
>> +	return (MAX34417_MAX_RSENSE * MAX34417_PWR_CORRECTION_SCALE) / rsense_uOhm;
>> +}
> 
> [Severity: Medium]
> As reported by Sashiko on v5 [3], doesn't the IIO_ENERGY scale calculation in
> max34417_read_raw() underflow to zero in sysfs for almost all supported
> shunt resistor values?
> 
> In max34417_probe_channel_node(), shunt-resistor-micro-ohms is accepted in
> the range 1000 to 100000 micro-Ohms. max34417_calc_input_correction()
> computes 2400000 / rsense_uOhm with integer division (which also truncates
> non-integer correction factors such as 2400000 / 90000 = 26 instead of
> 26.667), yielding input_correction in the range [24, 2400].
> 
> In max34417_read_raw() for IIO_CHAN_INFO_SCALE on IIO_ENERGY, *val is in the
> range [8192, 819200] and *val2 is 49 with IIO_VAL_FRACTIONAL_LOG2. When
> __iio_format_value() in drivers/iio/industrialio-core.c formats
> IIO_VAL_FRACTIONAL_LOG2:
> 
> 	case IIO_VAL_FRACTIONAL_LOG2:
> 		tmp2 = shift_right((s64)vals[0] * 1000000000LL, vals[1]);
> 		tmp0 = (int)div_s64_rem(tmp2, 1000000000LL, &tmp1);
> 
> because 2^49 is 562,949,953,421,312, any rsense_uOhm >= 1456 micro-Ohms
> produces vals[0] <= 562517, so (s64)vals[0] * 1000000000LL < 2^49 and tmp2
> becomes 0, causing in_energyX_scale to read as 0.000000000. Even at 1000
> micro-Ohms, tmp2 truncates to 1 (0.000000001 instead of 0.0000000014551915,
> losing 31.3% precision), and in-kernel consumers such as
> rescale_process_scale() in drivers/iio/afe/iio-rescale.c cast ULL(1) << *val2
> to int, truncating 1ULL << 49 to 0. Would returning a 64-bit decimal scale
> type such as IIO_VAL_DECIMAL64_FEMTO via iio_val_s64_decompose() avoid this
> underflow?

Good point, will return FEMTO based value.

Neil

> 
> [1] https://lore.kernel.org/r/20261102-topic-sm8x50-iio-max34417-adc-v5-3-0ffa5e5f2d5c@linaro.org
> [2] https://lore.kernel.org/r/20261102-topic-sm8x50-iio-max34417-adc-v5-3-0ffa5e5f2d5c@linaro.org
> [3] https://lore.kernel.org/r/20261102-topic-sm8x50-iio-max34417-adc-v5-3-0ffa5e5f2d5c@linaro.org
> 

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

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 01:42:48PM +0100, 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, 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 Joules.
> 
> 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.

I have a few nit-picks, mostly about the style. The only two seems more
important, actually the very first two (_uOhm suffix and use of a bitmap).

...

> +#define MAX34417_MIN_RSENSE		(1 * MILLI) /* 1000 microOhm */
> +#define MAX34417_MAX_RSENSE		(100 * MILLI) /* 100000 microOhm */
> +#define MAX34417_DEFAULT_RSENSE		(1 * MILLI) /* 1000 microOhm */

Why not adding a unit suffix _uOhm and drop the comments?

Moreover the burden of a typo is now in the comments and in such a case may
confuse the reader.

...

> +struct max34417_data {
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	struct mutex lock;
> +	u32 input_correction[MAX34417_CHANNEL_COUNT];
> +	const char *input_label[MAX34417_CHANNEL_COUNT];
> +	s64 acc_power[MAX34417_CHANNEL_COUNT];
> +	s64 acc_count[MAX34417_CHANNEL_COUNT];

> +	bool overflow[MAX34417_CHANNEL_COUNT];

Why not a bitmap?

	DECLARE_BITMAP(overflow, MAX34417_CHANNEL_COUNT);

> +	ktime_t last_update;
> +	bool cam_mode;
> +};

...

> +static int max34417_check_ovf(struct max34417_data *max34417)
> +{
> +	struct regmap *map = max34417->regmap;
> +	u32 control;
> +	int rc;
> +
> +	rc = regmap_read(map, MAX34417_CONTROL_REG, &control);
> +	if (rc)
> +		return rc;
> +
> +	if (!(control & MAX34417_CONTROL_OVF))
> +		return 0;

> +	rc = regmap_clear_bits(map, MAX34417_CONTROL_REG,
> +			       MAX34417_CONTROL_OVF);

It's precisely a single line

> +	if (rc)
> +		return rc;
> +
> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/* Save the timestamp for the next read */
> +	max34417->last_update = ktime_get();
> +
> +	return 0;
> +}

...

> +{
> +	s64 delta_us = ktime_us_delta(ktime_get(), max34417->last_update);

It's better to split the assignment of the variable (that's going to be
validated) from maintenance p.o.v.

> +	struct regmap *map = max34417->regmap;
> +	u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
> +	int rc;
> +
> +	/* Sleep up to 1msec to make sure we have a measurement cycle done */
> +	if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
> +		fsleep((1 * USEC_PER_MSEC) - delta_us);

	struct regmap *map = max34417->regmap;
	u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
	s64 delta_us;
	int rc;

	/* Sleep up to 1msec to make sure we have a measurement cycle done */
	delta_us = ktime_us_delta(ktime_get(), max34417->last_update);
	if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
		fsleep((1 * USEC_PER_MSEC) - delta_us);

> +	/*
> +	 * When not in CAM mode, we use the default mode where each
> +	 * UPDATE command will move the current accumulated data to the
> +	 * SMbus registers, reset those accumulators and run a new
> +	 * measurement cycle.
> +	 */
> +	if (!max34417->cam_mode) {
> +		/* Reset the accumulators and perform a measurement cycle */
> +		rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +		if (rc)
> +			return rc;
> +		/*
> +		 * UPDATE commands should be no less than 1msec apart for reliable
> +		 * measurements, since in Manual mode the SPS is 1kHz, we need wait
> +		 * for the measurement to finish before moving the data in the SMbus
> +		 * registers.
> +		 */

This comment goes up to 85 characters...

> +		fsleep(1 * USEC_PER_MSEC);
> +	}
> +
> +	/*
> +	 * Move the new UPDATE data into the SMBus read registers.
> +	 * This will also reset the accumulators, returning the
> +	 * power accumulated since the last UPDATE.
> +	 */

...and this one only up to 67. Can you bring some consistency to that?
Same comment to other similar cases all over the code.

> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/* Save the timestamp for the next read */
> +	max34417->last_update = ktime_get();
> +
> +	if (max34417->cam_mode) {

> +

Redundant blank line.

> +		/* Retrieve all channels to avoid missing data for other channels */
> +		rc = max34417_save_accumulators(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	rc = regmap_noinc_read(map, chan->address, &buf, sizeof(buf));
> +	if (rc)
> +		return rc;
> +
> +	/* Ignore first byte which contains the Byte Count & data starts at Bit 2 */
> +	*val = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]) >> 2;
> +
> +	return IIO_VAL_INT;
> +}

...

> +static int max34417_read_energy(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{
> +	struct regmap *map = max34417->regmap;
> +	s64 delta_us = ktime_us_delta(ktime_get(), max34417->last_update);

Split the assignment.

> +	s64 acc_power;
> +	int rc;
> +
> +	/* Sleep up to 1msec to make sure we have a measurement cycle done */
> +	if (delta_us >= 0 && delta_us < (1 * USEC_PER_MSEC))
> +		fsleep((1 * USEC_PER_MSEC) - delta_us);
> +
> +	/* Move the data to the SMBus registers */
> +	rc = regmap_write(map, MAX34417_UPDATE_REG, 1);
> +	if (rc)
> +		return rc;
> +
> +	/* Save the timestamp for the next read */
> +	max34417->last_update = ktime_get();
> +
> +	/* Retrieve all channels to avoid missing data for other channels */
> +	rc = max34417_save_accumulators(max34417);
> +	if (rc)
> +		return rc;
> +
> +	if (max34417->overflow[chan->channel]) {
> +		max34417->overflow[chan->channel] = false;
> +		max34417->acc_power[chan->channel] = 0;
> +		max34417->acc_count[chan->channel] = 0;
> +		return -EOVERFLOW;
> +	}
> +
> +	/* 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;
> +
> +	iio_val_s64_decompose(acc_power, val, val2);
> +
> +	return IIO_VAL_INT_64;
> +}

...

> +static int max34417_probe_channel_node(struct device *dev,
> +				       struct max34417_data *max34417,
> +				       struct fwnode_handle *node)
> +{
> +	u32 rsense_uOhm, index;
> +	int rc;
> +
> +	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, -ECHRNG, "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);
> +	}

Can be

	const char *propname;
	...
	propname = "label";
	...
	propname = "shunt-resistor-micro-ohms";

> +	if (fwnode_property_present(node, "shunt-resistor-micro-ohms")) {
> +		rc = fwnode_property_read_u32(node, "shunt-resistor-micro-ohms",
> +					      &rsense_uOhm);
> +		if (rc)
> +			return dev_err_probe(dev, rc,
> +					     "failure reading shunt property of %pfwP\n",

...and here you can refer to it in full using %s.

> +					     node);
> +
> +		if (rsense_uOhm < MAX34417_MIN_RSENSE ||
> +		    rsense_uOhm > MAX34417_MAX_RSENSE)
> +			return dev_err_probe(dev, -EINVAL,

-ERANGE?

> +					     "invalid shunt value %d of %pfwP\n",

Same here — use %s and propname.

> +					     rsense_uOhm, node);
> +
> +		max34417->input_correction[index] =
> +			max34417_calc_input_correction(rsense_uOhm);
> +	}
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko