[PATCH v1 0/2] iio: adc: add MAX40080 current-sense amplifier driver

[PATCH v1 0/2] iio: adc: add MAX40080 current-sense amplifier driver

[Stefan Popa]

This series adds support for the Maxim MAX40080, a bidirectional
current-sense amplifier with an integrated 12-bit ADC and an I2C/SMBus
interface. It measures the voltage across an external shunt resistor and
the input bus voltage.

The driver operates in direct (INDIO_DIRECT_MODE) mode. Each raw read
triggers a single on-demand conversion (SMBus Quick Command) and reads
back the matched current/voltage pair, so results are always fresh. It
exposes the current and voltage channels with raw and scale attributes,
a configurable oversampling (digital averaging) ratio, and PEC-protected
register access. The two selectable current-sense ranges are exposed
through scale/scale_available (the range is chosen by writing the
desired scale); the current scale is derived from the
shunt-resistor-micro-ohms device-tree property.

Continuous FIFO buffering, threshold events and the alert interrupt are
intentionally left out of this initial submission and may be added
later.

Tested on hardware with four MAX40080 devices on an I2C bus.

Patch 1 adds the device-tree binding; patch 2 adds the driver.

Stefan Popa (2):
  dt-bindings: iio: adc: add maxim,max40080
  iio: adc: add MAX40080 current-sense amplifier driver

 .../bindings/iio/adc/maxim,max40080.yaml      |  55 ++
 MAINTAINERS                                   |   9 +
 drivers/iio/adc/Kconfig                       |  11 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/max40080.c                    | 587 ++++++++++++++++++
 5 files changed, 663 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml
 create mode 100644 drivers/iio/adc/max40080.c

-- 
2.51.0

[PATCH v1 1/2] dt-bindings: iio: adc: add maxim,max40080

[Stefan Popa]

Add device tree bindings for the Maxim MAX40080 bidirectional
current-sense amplifier with a 12-bit ADC and an I2C/SMBus interface.

Signed-off-by: Stefan Popa <stefan.popa@analog.com>
---
 .../bindings/iio/adc/maxim,max40080.yaml      | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml
new file mode 100644
index 0000000000000..4cda6cea6022e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/maxim,max40080.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim MAX40080 bidirectional current-sense amplifier
+
+maintainers:
+  - Ciprian Hegbeli <ciprian.hegbeli@analog.com>
+  - Stefan Popa <stefan.popa@analog.com>
+
+description: |
+  The MAX40080 is a high-precision, bidirectional current-sense amplifier with
+  an integrated 12-bit ADC and an I2C/SMBus interface. It measures the voltage
+  across an external shunt resistor and the input bus voltage, and stores the
+  results in an internal FIFO.
+
+  Datasheet:
+    https://www.analog.com/en/products/max40080.html
+
+properties:
+  compatible:
+    const: maxim,max40080
+
+  reg:
+    maxItems: 1
+
+  "#io-channel-cells":
+    const: 1
+
+  shunt-resistor-micro-ohms:
+    description:
+      Value of the current-sense shunt resistor connected between the IN+ and
+      IN- inputs. Used to scale the reported current.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@20 {
+            compatible = "maxim,max40080";
+            reg = <0x20>;
+            #io-channel-cells = <1>;
+            shunt-resistor-micro-ohms = <100000>;
+        };
+    };
-- 
2.51.0

Re: [PATCH v1 1/2] dt-bindings: iio: adc: add maxim,max40080

[Conor Dooley]

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

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable

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

Re: [PATCH v1 1/2] dt-bindings: iio: adc: add maxim,max40080

[David Lechner]

On 7/3/26 5:29 AM, Stefan Popa wrote:
> Add device tree bindings for the Maxim MAX40080 bidirectional
> current-sense amplifier with a 12-bit ADC and an I2C/SMBus interface.
> 
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> ---
>  .../bindings/iio/adc/maxim,max40080.yaml      | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml b/Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml
> new file mode 100644
> index 0000000000000..4cda6cea6022e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/maxim,max40080.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX40080 bidirectional current-sense amplifier
> +
> +maintainers:
> +  - Ciprian Hegbeli <ciprian.hegbeli@analog.com>
> +  - Stefan Popa <stefan.popa@analog.com>
> +
> +description: |
> +  The MAX40080 is a high-precision, bidirectional current-sense amplifier with
> +  an integrated 12-bit ADC and an I2C/SMBus interface. It measures the voltage
> +  across an external shunt resistor and the input bus voltage, and stores the
> +  results in an internal FIFO.
> +
> +  Datasheet:
> +    https://www.analog.com/en/products/max40080.html
> +
> +properties:
> +  compatible:
> +    const: maxim,max40080
> +
> +  reg:
> +    maxItems: 1

Missing vdd-supply for power and interrupts for ALERT output.

> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  shunt-resistor-micro-ohms:
> +    description:
> +      Value of the current-sense shunt resistor connected between the IN+ and
> +      IN- inputs. Used to scale the reported current.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@20 {
> +            compatible = "maxim,max40080";
> +            reg = <0x20>;
> +            #io-channel-cells = <1>;
> +            shunt-resistor-micro-ohms = <100000>;
> +        };
> +    };

[PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Stefan Popa]

The MAX40080 is a bidirectional current-sense amplifier with an
integrated 12-bit ADC and an I2C/SMBus interface. It measures the
voltage across an external shunt resistor and the input bus voltage,
storing the results in an internal FIFO.

Add a direct-mode IIO driver exposing the current and voltage channels
with raw, scale and hardware-gain attributes, a configurable
oversampling (digital averaging) ratio, and PEC-protected register
access. The current scale is derived from the shunt resistor value
described in the device tree.

Signed-off-by: Ciprian Hegbeli <ciprian.hegbeli@analog.com>
Signed-off-by: Stefan Popa <stefan.popa@analog.com>
---
 MAINTAINERS                |   9 +
 drivers/iio/adc/Kconfig    |  11 +
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max40080.c | 587 +++++++++++++++++++++++++++++++++++++
 4 files changed, 608 insertions(+)
 create mode 100644 drivers/iio/adc/max40080.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e087673237636..f50c1e00e12bb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15513,6 +15513,15 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 F:	drivers/iio/temperature/max30208.c
 
+MAXIM MAX40080 CURRENT SENSE AMPLIFIER DRIVER
+M:	Ciprian Hegbeli <ciprian.hegbeli@analog.com>
+M:	Stefan Popa <stefan.popa@analog.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml
+F:	drivers/iio/adc/max40080.c
+
 MAXIM MAX7360 KEYPAD LED MFD DRIVER
 M:	Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
 S:	Maintained
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 58da8255525e4..b651c57bbc3f5 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1041,6 +1041,17 @@ config MAX34408
 	  To compile this driver as a module, choose M here: the module will be
 	  called max34408.
 
+config MAX40080
+	tristate "Analog Devices MAX40080 Current Sense Amplifier"
+	depends on I2C
+	help
+	  Say yes here to build support for the Analog Devices MAX40080
+	  bidirectional current-sense amplifier with a 12-bit ADC and an I2C
+	  interface.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max40080.
+
 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 7cc8f9a12f763..e1953353c68a4 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MAX14001) += max14001.o
 obj-$(CONFIG_MAX34408) += max34408.o
+obj-$(CONFIG_MAX40080) += max40080.o
 obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
 obj-$(CONFIG_MAX9611) += max9611.o
 obj-$(CONFIG_MCP320X) += mcp320x.o
diff --git a/drivers/iio/adc/max40080.c b/drivers/iio/adc/max40080.c
new file mode 100644
index 0000000000000..441e1ce3dcffd
--- /dev/null
+++ b/drivers/iio/adc/max40080.c
@@ -0,0 +1,587 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MAX40080 Digital Current-Sense Amplifier driver
+ *
+ * Copyright 2026 Analog Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/i2c.h>
+#include <linux/iopoll.h>
+#include <linux/math64.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/unaligned.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+
+#define MAX40080_REG_CFG		0x00
+#define  MAX40080_MODE_MSK		GENMASK(2, 0)
+#define  MAX40080_PEC_EN_MSK		BIT(5)
+#define  MAX40080_RANGE_MSK		BIT(6)
+#define  MAX40080_FILTER_MSK		GENMASK(14, 12)
+
+#define MAX40080_REG_FIFO_CFG		0x0A
+#define  MAX40080_STORE_IV_MSK		GENMASK(1, 0)
+
+#define MAX40080_REG_IV			0x10
+/* Current is a 13-bit two's-complement value (magnitude + sign bit). */
+#define  MAX40080_IV_I_MSK		GENMASK(12, 0)
+#define  MAX40080_IV_I_SIGN_BIT		12
+#define  MAX40080_IV_V_MAG_MSK		GENMASK(27, 16)
+#define  MAX40080_IV_VALID_MSK		BIT(31)
+
+/* CFG.mode field */
+#define MAX40080_STDBY_MODE		0x00
+#define MAX40080_SINGLE_MODE		0x02	/* one conversion per Quick Command */
+
+/* FIFO_CFG.store_iv field */
+#define MAX40080_STORE_I_V		0x02
+
+#define MAX40080_ADC_RES		4096
+#define MAX40080_INTER_VREF_MV		1250
+#define MAX40080_V_BUFF_GAIN		30
+#define MAX40080_CSA_50MV_GAIN		25
+#define MAX40080_CSA_10MV_GAIN		125
+
+/*
+ * The RANGE field (CFG bit 6) selects one of two current-sense full-scale
+ * ranges (the MAX40080 supports exactly two: +/-50 mV and +/-10 mV). Ordered
+ * so that the array index equals the RANGE field value: index 0 = 50 mV range
+ * (gain 25 V/V), index 1 = 10 mV range (gain 125 V/V).
+ */
+static const int max40080_csa_gain[] = {
+	MAX40080_CSA_50MV_GAIN, MAX40080_CSA_10MV_GAIN,
+};
+
+#define MAX40080_NUM_RANGES	ARRAY_SIZE(max40080_csa_gain)
+
+struct max40080_state {
+	struct i2c_client *client;
+	/* Serializes read-modify-write access to the CFG register. */
+	struct mutex lock;
+	u32 shunt_resistor_uohm;
+	/*
+	 * Precomputed current scale (mA per code) for each RANGE setting, as
+	 * {integer, nano} pairs for IIO_VAL_INT_PLUS_NANO. The range is
+	 * selected by writing the corresponding scale.
+	 */
+	int current_scale[MAX40080_NUM_RANGES][2];
+};
+
+static const int max40080_oversampling_avail[] = { 1, 8, 16, 32, 64, 128 };
+
+static int max40080_update_bits(struct max40080_state *st, u8 reg,
+				u16 mask, u16 val)
+{
+	int ret;
+	int tmp;
+
+	guard(mutex)(&st->lock);
+
+	tmp = i2c_smbus_read_word_data(st->client, reg);
+	if (tmp < 0)
+		return tmp;
+
+	tmp &= ~mask;
+	tmp |= val & mask;
+
+	ret = i2c_smbus_write_word_data(st->client, reg, tmp);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * In single-measurement mode the device sits idle until it receives an SMBus
+ * Quick Command, then performs exactly one current and one voltage conversion
+ * and returns to idle. Triggering on demand this way (rather than running the
+ * FIFO continuously in active mode) means each read returns a fresh, coherent
+ * current/voltage pair instead of the oldest queued FIFO entry.
+ */
+static int max40080_trigger_measurement(struct max40080_state *st)
+{
+	return i2c_smbus_xfer(st->client->adapter, st->client->addr,
+			      st->client->flags, I2C_SMBUS_WRITE, 0,
+			      I2C_SMBUS_QUICK, NULL);
+}
+
+/*
+ * A single measurement holds the matched current/voltage pair in one 32-bit
+ * word (MAX40080_REG_IV). Reading all four bytes in one transaction returns
+ * both from the same conversion; reading the separate current (0x0C) and
+ * voltage (0x0E) registers would decorrelate the two channels.
+ *
+ * Unlike the word accesses used elsewhere, this is a plain I2C block read: the
+ * SMBus layer does not append or verify a PEC byte for it even when PEC is
+ * otherwise enabled for the device, so this transfer is not PEC protected.
+ */
+static int max40080_read_iv_once(struct max40080_state *st, u32 *iv)
+{
+	u8 buf[4];
+	int ret;
+
+	ret = i2c_smbus_read_i2c_block_data(st->client, MAX40080_REG_IV,
+					    sizeof(buf), buf);
+	if (ret < 0)
+		return ret;
+	if (ret != sizeof(buf))
+		return -EIO;
+
+	*iv = get_unaligned_le32(buf);
+
+	return 0;
+}
+
+static int max40080_read_iv(struct max40080_state *st, u32 *iv)
+{
+	int ret, io_ret;
+
+	guard(mutex)(&st->lock);
+
+	ret = max40080_trigger_measurement(st);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Wait for the conversion to complete by polling the FIFO valid bit
+	 * (or bail out on an I2C error). Polling the device's own status makes
+	 * this independent of the actual conversion time, which varies with the
+	 * oversampling ratio and the bus speed. The timeout is only a safety
+	 * ceiling: the worst case is the maximum 128x averaging on both the
+	 * current and voltage channels at the slowest 15 ksps base rate plus the
+	 * inter-channel switching time, i.e. roughly 20 ms; 50 ms leaves ample
+	 * margin.
+	 */
+	ret = read_poll_timeout(max40080_read_iv_once, io_ret,
+				io_ret || (*iv & MAX40080_IV_VALID_MSK),
+				500, 50000, false, st, iv);
+	if (ret)
+		return ret;
+
+	return io_ret;
+}
+
+static int max40080_get_current(struct max40080_state *st, int *val)
+{
+	u32 iv;
+	int ret;
+
+	ret = max40080_read_iv(st, &iv);
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(FIELD_GET(MAX40080_IV_I_MSK, iv),
+			     MAX40080_IV_I_SIGN_BIT);
+
+	return 0;
+}
+
+static int max40080_get_voltage(struct max40080_state *st, int *val)
+{
+	u32 iv;
+	int ret;
+
+	ret = max40080_read_iv(st, &iv);
+	if (ret)
+		return ret;
+
+	*val = FIELD_GET(MAX40080_IV_V_MAG_MSK, iv);
+
+	return 0;
+}
+
+static int max40080_get_range(struct max40080_state *st, unsigned int *range)
+{
+	int tmp;
+
+	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
+	if (tmp < 0)
+		return tmp;
+
+	*range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
+
+	return 0;
+}
+
+static int max40080_set_range(struct max40080_state *st, unsigned int range)
+{
+	return max40080_update_bits(st, MAX40080_REG_CFG, MAX40080_RANGE_MSK,
+				    FIELD_PREP(MAX40080_RANGE_MSK, range));
+}
+
+/*
+ * Precompute the current scale (mA per code) for each RANGE setting as
+ * {integer, nano} pairs. The shunt drop for a full-scale code is
+ *   Vref[mV] / (ADC_RES * gain)
+ * and current = Vshunt / Rshunt, so with Rshunt in micro-ohms the scale in
+ * mA/code is
+ *   Vref[mV] * NANO * MICRO / (ADC_RES * gain * Rshunt[uohm])
+ * expressed as an integer part plus a nano fractional part.
+ */
+static void max40080_calc_current_scale(struct max40080_state *st)
+{
+	unsigned int i;
+	u32 rem;
+	u64 tmp;
+
+	for (i = 0; i < MAX40080_NUM_RANGES; i++) {
+		tmp = (u64)MAX40080_INTER_VREF_MV * NANO * MICRO;
+		tmp = div64_u64(tmp, (u64)MAX40080_ADC_RES * max40080_csa_gain[i] *
+				st->shunt_resistor_uohm);
+		st->current_scale[i][0] = div_u64_rem(tmp, NANO, &rem);
+		st->current_scale[i][1] = rem;
+	}
+}
+
+/*
+ * The FILTER field selects digital averaging of N consecutive conversions
+ * (no averaging, 8, 16, 32, 64 or 128), which maps directly to the IIO
+ * oversampling ratio. Averaging reduces the effective output data rate by the
+ * same factor; the conversion rate itself is set by the separate ADC_RATE
+ * field.
+ */
+static int max40080_get_oversampling_ratio(struct max40080_state *st, int *val)
+{
+	int tmp;
+	u8 filter;
+
+	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
+	if (tmp < 0)
+		return tmp;
+
+	filter = FIELD_GET(MAX40080_FILTER_MSK, tmp);
+	*val = (filter == 0) ? 1 : (8 << (filter - 1));
+
+	return 0;
+}
+
+/*
+ * max40080_oversampling_avail[] is ordered so that its index is the FILTER
+ * field value (index 0 = no averaging, index 1 = 8x, ...). Return that index
+ * for an exact match, or -EINVAL for a value that is not on the list.
+ */
+static int max40080_oversampling_to_filter(int val)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(max40080_oversampling_avail); i++)
+		if (max40080_oversampling_avail[i] == val)
+			return i;
+
+	return -EINVAL;
+}
+
+static int max40080_set_oversampling_ratio(struct max40080_state *st, int val)
+{
+	int filter = max40080_oversampling_to_filter(val);
+
+	if (filter < 0)
+		return filter;
+
+	return max40080_update_bits(st, MAX40080_REG_CFG, MAX40080_FILTER_MSK,
+				    FIELD_PREP(MAX40080_FILTER_MSK, filter));
+}
+
+static int max40080_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val,
+			     int *val2,
+			     long mask)
+{
+	struct max40080_state *st = iio_priv(indio_dev);
+	unsigned int range;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->type == IIO_CURRENT) {
+			ret = max40080_get_current(st, val);
+			if (ret)
+				return ret;
+		} else if (chan->type == IIO_VOLTAGE) {
+			ret = max40080_get_voltage(st, val);
+			if (ret)
+				return ret;
+		}
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_CURRENT) {
+			/*
+			 * The selectable current-sense range is exposed through
+			 * scale: each RANGE setting has its own precomputed
+			 * mA-per-code value. Userspace picks the range by writing
+			 * the matching scale.
+			 */
+			ret = max40080_get_range(st, &range);
+			if (ret)
+				return ret;
+
+			*val = st->current_scale[range][0];
+			*val2 = st->current_scale[range][1];
+			return IIO_VAL_INT_PLUS_NANO;
+		}
+		/* voltage[mV] = raw * Vref[mV] * buffer_gain / ADC_RES */
+		*val = MAX40080_INTER_VREF_MV * MAX40080_V_BUFF_GAIN;
+		*val2 = MAX40080_ADC_RES;
+		return IIO_VAL_FRACTIONAL;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = max40080_get_oversampling_ratio(st, val);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int max40080_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	struct max40080_state *st = iio_priv(indio_dev);
+	unsigned int i;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		/* Only the current channel has a selectable range/scale. */
+		if (chan->type != IIO_CURRENT)
+			return -EINVAL;
+
+		for (i = 0; i < MAX40080_NUM_RANGES; i++)
+			if (val == st->current_scale[i][0] &&
+			    val2 == st->current_scale[i][1])
+				return max40080_set_range(st, i);
+
+		return -EINVAL;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = max40080_set_oversampling_ratio(st, val);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int max40080_write_raw_get_fmt(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return IIO_VAL_INT;
+	}
+}
+
+static int max40080_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long info)
+{
+	struct max40080_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type != IIO_CURRENT)
+			return -EINVAL;
+
+		*vals = (int *)st->current_scale;
+		*length = MAX40080_NUM_RANGES * 2;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = max40080_oversampling_avail;
+		*length = ARRAY_SIZE(max40080_oversampling_avail);
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int max40080_reg_access(struct iio_dev *indio_dev,
+			       unsigned int reg,
+			       unsigned int write_val,
+			       unsigned int *read_val)
+{
+	struct max40080_state *st = iio_priv(indio_dev);
+
+	if (read_val) {
+		int val = i2c_smbus_read_word_data(st->client, reg);
+
+		if (val < 0)
+			return val;
+		*read_val = val;
+		return 0;
+	}
+
+	return i2c_smbus_write_word_data(st->client, reg, write_val);
+}
+
+static const struct iio_info max40080_info = {
+	.read_raw = max40080_read_raw,
+	.write_raw = max40080_write_raw,
+	.write_raw_get_fmt = max40080_write_raw_get_fmt,
+	.read_avail = max40080_read_avail,
+	.debugfs_reg_access = &max40080_reg_access,
+};
+
+static const struct iio_chan_spec max40080_channels[] = {
+	{
+		.type = IIO_CURRENT,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available =
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+	},
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available =
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+	},
+};
+
+static int max40080_init(struct max40080_state *st, int oversampling_ratio)
+{
+	u16 fifo_cfg, cfg;
+	int ret, filter;
+
+	filter = max40080_oversampling_to_filter(oversampling_ratio);
+	if (filter < 0)
+		return filter;
+
+	/*
+	 * Put the device in standby before (re)configuring the FIFO: the FIFO
+	 * configuration register can only be written while the device is not
+	 * converting (e.g. after a probe following a warm reset). PEC is enabled
+	 * here and remains enabled for all later transactions.
+	 */
+	cfg = FIELD_PREP(MAX40080_MODE_MSK, MAX40080_STDBY_MODE) |
+	      FIELD_PREP(MAX40080_PEC_EN_MSK, 1);
+	ret = i2c_smbus_write_word_data(st->client, MAX40080_REG_CFG, cfg);
+	if (ret)
+		return ret;
+
+	/* Store a matched current+voltage pair per conversion. */
+	fifo_cfg = FIELD_PREP(MAX40080_STORE_IV_MSK, MAX40080_STORE_I_V);
+	ret = i2c_smbus_write_word_data(st->client, MAX40080_REG_FIFO_CFG,
+					fifo_cfg);
+	if (ret)
+		return ret;
+
+	/*
+	 * Use single-measurement mode: the device stays idle and converts once
+	 * per SMBus Quick Command (see max40080_trigger_measurement()), so each
+	 * read returns a fresh sample rather than a queued FIFO entry.
+	 */
+	cfg = FIELD_PREP(MAX40080_MODE_MSK, MAX40080_SINGLE_MODE) |
+	      FIELD_PREP(MAX40080_PEC_EN_MSK, 1) |
+	      FIELD_PREP(MAX40080_FILTER_MSK, filter);
+
+	ret = i2c_smbus_write_word_data(st->client, MAX40080_REG_CFG, cfg);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int max40080_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct max40080_state *st;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_WORD_DATA |
+				     I2C_FUNC_SMBUS_I2C_BLOCK |
+				     I2C_FUNC_SMBUS_QUICK))
+		return -EOPNOTSUPP;
+
+	client->flags |= I2C_CLIENT_PEC;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, indio_dev);
+
+	st = iio_priv(indio_dev);
+	st->client = client;
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
+				     &st->shunt_resistor_uohm))
+		st->shunt_resistor_uohm = 1000000; /* default 1 ohm */
+
+	if (!st->shunt_resistor_uohm)
+		return dev_err_probe(dev, -EINVAL,
+				     "shunt-resistor-micro-ohms must be non-zero\n");
+
+	max40080_calc_current_scale(st);
+
+	indio_dev->name = "max40080";
+	indio_dev->info = &max40080_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = max40080_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max40080_channels);
+
+	/* No averaging by default; configurable at runtime via sysfs. */
+	ret = max40080_init(st, 1);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct i2c_device_id max40080_i2c_ids[] = {
+	{ "max40080" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max40080_i2c_ids);
+
+static const struct of_device_id max40080_of_match[] = {
+	{ .compatible = "maxim,max40080" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max40080_of_match);
+
+static struct i2c_driver max40080_driver = {
+	.driver = {
+		.name = "max40080",
+		.of_match_table = max40080_of_match,
+	},
+	.probe = max40080_probe,
+	.id_table = max40080_i2c_ids,
+};
+module_i2c_driver(max40080_driver);
+
+MODULE_AUTHOR("Ciprian Hegbeli <ciprian.hegbeli@analog.com>");
+MODULE_AUTHOR("Stefan Popa <stefan.popa@analog.com>");
+MODULE_DESCRIPTION("Analog Devices MAX40080 current-sense amplifier driver");
+MODULE_LICENSE("GPL");
-- 
2.51.0

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Andy Shevchenko]

On Fri, Jul 03, 2026 at 01:29:32PM +0300, Stefan Popa wrote:
> The MAX40080 is a bidirectional current-sense amplifier with an
> integrated 12-bit ADC and an I2C/SMBus interface. It measures the
> voltage across an external shunt resistor and the input bus voltage,
> storing the results in an internal FIFO.
> 
> Add a direct-mode IIO driver exposing the current and voltage channels
> with raw, scale and hardware-gain attributes, a configurable
> oversampling (digital averaging) ratio, and PEC-protected register
> access. The current scale is derived from the shunt resistor value
> described in the device tree.

> Signed-off-by: Ciprian Hegbeli <ciprian.hegbeli@analog.com>
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>

Who is Ciprian? Messed up with tags?
(Missing Co-developed-by? Originally-by? Et cetera?)

...

> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/math64.h>

> +#include <linux/mod_devicetable.h>

There is a new development to use something else in the future for this.

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>

+ types.h // uXX

> +#include <linux/unaligned.h>
> +#include <linux/units.h>

...

> +#define MAX40080_ADC_RES		4096

RES is to odd: Resource? Result? Ah, resolution!

...


> +#define MAX40080_INTER_VREF_MV		1250

_mV


...

> +#define MAX40080_CSA_50MV_GAIN		25
> +#define MAX40080_CSA_10MV_GAIN		125

50mV
10mV

> +/*
> + * The RANGE field (CFG bit 6) selects one of two current-sense full-scale
> + * ranges (the MAX40080 supports exactly two: +/-50 mV and +/-10 mV). Ordered
> + * so that the array index equals the RANGE field value: index 0 = 50 mV range
> + * (gain 25 V/V), index 1 = 10 mV range (gain 125 V/V).
> + */
> +static const int max40080_csa_gain[] = {
> +	MAX40080_CSA_50MV_GAIN, MAX40080_CSA_10MV_GAIN,
> +};

With a given comment and one time use of those definitions, do we need them
at all?  I leave it to Jonathan, because my memory is weak on his preference
in the cases like this.

...

> +#define MAX40080_NUM_RANGES	ARRAY_SIZE(max40080_csa_gain)

Hmm... It's just a bit shorter, but one may argue that having ARRAY_SIZE()
inline makes it better to understand. And I rarely see definitions that
actually just a wrapper on ARRAY_SIZE() with hardcoded name.

...

> +static int max40080_update_bits(struct max40080_state *st, u8 reg,
> +				u16 mask, u16 val)
> +{
> +	int ret;
> +	int tmp;
> +
> +	guard(mutex)(&st->lock);
> +
> +	tmp = i2c_smbus_read_word_data(st->client, reg);
> +	if (tmp < 0)
> +		return tmp;

> +	tmp &= ~mask;
> +	tmp |= val & mask;

Standard pattern is one liner

	tmp = (tmp & ~mask) | (val & mask);

> +	ret = i2c_smbus_write_word_data(st->client, reg, tmp);
> +	if (ret < 0)

Do you need '< 0' for _write_word_data() cases?

> +		return ret;
> +
> +	return 0;

I think the

	return _write_word_data(...);

suffice.

> +}

...

> +/*
> + * In single-measurement mode the device sits idle until it receives an SMBus
> + * Quick Command, then performs exactly one current and one voltage conversion
> + * and returns to idle. Triggering on demand this way (rather than running the
> + * FIFO continuously in active mode) means each read returns a fresh, coherent
> + * current/voltage pair instead of the oldest queued FIFO entry.
> + */
> +static int max40080_trigger_measurement(struct max40080_state *st)
> +{

Even with

	struct i2c_cliend *client = st->client;

> +	return i2c_smbus_xfer(st->client->adapter, st->client->addr,
> +			      st->client->flags, I2C_SMBUS_WRITE, 0,
> +			      I2C_SMBUS_QUICK, NULL);

this becomes slightly better

	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
			      I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);

> +}

...

> +static int max40080_read_iv(struct max40080_state *st, u32 *iv)
> +{
> +	int ret, io_ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = max40080_trigger_measurement(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Wait for the conversion to complete by polling the FIFO valid bit
> +	 * (or bail out on an I2C error). Polling the device's own status makes
> +	 * this independent of the actual conversion time, which varies with the
> +	 * oversampling ratio and the bus speed. The timeout is only a safety
> +	 * ceiling: the worst case is the maximum 128x averaging on both the
> +	 * current and voltage channels at the slowest 15 ksps base rate plus the
> +	 * inter-channel switching time, i.e. roughly 20 ms; 50 ms leaves ample
> +	 * margin.
> +	 */
> +	ret = read_poll_timeout(max40080_read_iv_once, io_ret,
> +				io_ret || (*iv & MAX40080_IV_VALID_MSK),
> +				500, 50000, false, st, iv);

I would use

				1 * USEC_PER_MSEC, 50 * USEC_PER_MSEC,
				false, st, iv);

Yes, this doubles the minimum threshold. So, up to you, and in case of taking
the suggestion, include time.h.

> +	if (ret)
> +		return ret;
> +
> +	return io_ret;
> +}

...

> +static void max40080_calc_current_scale(struct max40080_state *st)
> +{
> +	unsigned int i;
> +	u32 rem;
> +	u64 tmp;

> +	for (i = 0; i < MAX40080_NUM_RANGES; i++) {

	for (unsigned int i = 0; i < ARRAY_SIZE(...); i++) {

> +		tmp = (u64)MAX40080_INTER_VREF_MV * NANO * MICRO;
> +		tmp = div64_u64(tmp, (u64)MAX40080_ADC_RES * max40080_csa_gain[i] *
> +				st->shunt_resistor_uohm);

I would do in one statement

		tmp = div64_u64((u64)MAX40080_INTER_VREF_MV * NANO * MICRO,
				(u64)MAX40080_ADC_RES * max40080_csa_gain[i] *
				st->shunt_resistor_uohm);

> +		st->current_scale[i][0] = div_u64_rem(tmp, NANO, &rem);
> +		st->current_scale[i][1] = rem;
> +	}
> +}

...

> +static int max40080_get_oversampling_ratio(struct max40080_state *st, int *val)
> +{
> +	int tmp;
> +	u8 filter;
> +
> +	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	filter = FIELD_GET(MAX40080_FILTER_MSK, tmp);
> +	*val = (filter == 0) ? 1 : (8 << (filter - 1));

Can we use max40080_oversampling_avail[] here?

> +	return 0;
> +}

...

> +static int max40080_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,

> +			     int *val,
> +			     int *val2,

These two can occupy a single line.

> +			     long mask)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);
> +	unsigned int range;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

> +		if (chan->type == IIO_CURRENT) {
> +			ret = max40080_get_current(st, val);
> +			if (ret)
> +				return ret;
> +		} else if (chan->type == IIO_VOLTAGE) {
> +			ret = max40080_get_voltage(st, val);
> +			if (ret)
> +				return ret;
> +		}

Hmm... Why not

		if (chan->type == IIO_CURRENT)
			ret = max40080_get_current(st, val);
		else if (chan->type == IIO_VOLTAGE)
			ret = max40080_get_voltage(st, val);
		else
			ret = 0; // Shouldn't we return an error?
		if (ret)
			return ret;

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_CURRENT) {
> +			/*
> +			 * The selectable current-sense range is exposed through
> +			 * scale: each RANGE setting has its own precomputed
> +			 * mA-per-code value. Userspace picks the range by writing
> +			 * the matching scale.
> +			 */
> +			ret = max40080_get_range(st, &range);
> +			if (ret)
> +				return ret;
> +
> +			*val = st->current_scale[range][0];
> +			*val2 = st->current_scale[range][1];
> +			return IIO_VAL_INT_PLUS_NANO;
> +		}
> +		/* voltage[mV] = raw * Vref[mV] * buffer_gain / ADC_RES */
> +		*val = MAX40080_INTER_VREF_MV * MAX40080_V_BUFF_GAIN;
> +		*val2 = MAX40080_ADC_RES;
> +		return IIO_VAL_FRACTIONAL;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = max40080_get_oversampling_ratio(st, val);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int max40080_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);
> +	unsigned int i;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		/* Only the current channel has a selectable range/scale. */
> +		if (chan->type != IIO_CURRENT)
> +			return -EINVAL;

> +		for (i = 0; i < MAX40080_NUM_RANGES; i++)

		for (unsigned int i = 0; i < ARRAY_SIZE(...); i++)

> +			if (val == st->current_scale[i][0] &&
> +			    val2 == st->current_scale[i][1])
> +				return max40080_set_range(st, i);
> +
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = max40080_set_oversampling_ratio(st, val);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

> +	return 0;

Return directly, it seems only one time use.

> +}

...

> +static int max40080_reg_access(struct iio_dev *indio_dev,
> +			       unsigned int reg,
> +			       unsigned int write_val,
> +			       unsigned int *read_val)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);

> +	if (read_val) {

Hmm... In this case perhaps

	if (!read_val)
		return i2c_smbus_write_word_data(st->client, reg, write_val);

makes code cleaner.

> +		int val = i2c_smbus_read_word_data(st->client, reg);
> +
> +		if (val < 0)
> +			return val;

The desired pattern is to have assignment separated from the definition.
The rationale is maintenance overhead and possibility of subtle mistakes.

> +		*read_val = val;
> +		return 0;
> +	}
> +
> +	return i2c_smbus_write_word_data(st->client, reg, write_val);
> +}

...

> +static int max40080_init(struct max40080_state *st, int oversampling_ratio)
> +{
> +	u16 fifo_cfg, cfg;
> +	int ret, filter;
> +
> +	filter = max40080_oversampling_to_filter(oversampling_ratio);
> +	if (filter < 0)
> +		return filter;
> +
> +	/*
> +	 * Put the device in standby before (re)configuring the FIFO: the FIFO
> +	 * configuration register can only be written while the device is not
> +	 * converting (e.g. after a probe following a warm reset). PEC is enabled
> +	 * here and remains enabled for all later transactions.
> +	 */
> +	cfg = FIELD_PREP(MAX40080_MODE_MSK, MAX40080_STDBY_MODE) |
> +	      FIELD_PREP(MAX40080_PEC_EN_MSK, 1);
> +	ret = i2c_smbus_write_word_data(st->client, MAX40080_REG_CFG, cfg);
> +	if (ret)
> +		return ret;
> +
> +	/* Store a matched current+voltage pair per conversion. */
> +	fifo_cfg = FIELD_PREP(MAX40080_STORE_IV_MSK, MAX40080_STORE_I_V);

> +	ret = i2c_smbus_write_word_data(st->client, MAX40080_REG_FIFO_CFG,
> +					fifo_cfg);

I would dare to make this a single line.

> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Use single-measurement mode: the device stays idle and converts once
> +	 * per SMBus Quick Command (see max40080_trigger_measurement()), so each
> +	 * read returns a fresh sample rather than a queued FIFO entry.
> +	 */
> +	cfg = FIELD_PREP(MAX40080_MODE_MSK, MAX40080_SINGLE_MODE) |
> +	      FIELD_PREP(MAX40080_PEC_EN_MSK, 1) |
> +	      FIELD_PREP(MAX40080_FILTER_MSK, filter);

> +	ret = i2c_smbus_write_word_data(st->client, MAX40080_REG_CFG, cfg);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Same as per above,

	return i2c_smbus_write_word_data(st->client, MAX40080_REG_CFG, cfg);

> +}

...

> +static int max40080_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct max40080_state *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WORD_DATA |
> +				     I2C_FUNC_SMBUS_I2C_BLOCK |
> +				     I2C_FUNC_SMBUS_QUICK))
> +		return -EOPNOTSUPP;
> +
> +	client->flags |= I2C_CLIENT_PEC;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;

> +	i2c_set_clientdata(client, indio_dev);

Is it used?

> +	st = iio_priv(indio_dev);
> +	st->client = client;
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> +				     &st->shunt_resistor_uohm))
> +		st->shunt_resistor_uohm = 1000000; /* default 1 ohm */

1 * MICRO

> +	if (!st->shunt_resistor_uohm)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "shunt-resistor-micro-ohms must be non-zero\n");
> +
> +	max40080_calc_current_scale(st);
> +
> +	indio_dev->name = "max40080";
> +	indio_dev->info = &max40080_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = max40080_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max40080_channels);
> +
> +	/* No averaging by default; configurable at runtime via sysfs. */
> +	ret = max40080_init(st, 1);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

...

> +static const struct i2c_device_id max40080_i2c_ids[] = {
> +	{ "max40080" },

Use C99 initialisers.

> +	{ }
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Jonathan Cameron]

> > +/*
> > + * The RANGE field (CFG bit 6) selects one of two current-sense full-scale
> > + * ranges (the MAX40080 supports exactly two: +/-50 mV and +/-10 mV). Ordered
> > + * so that the array index equals the RANGE field value: index 0 = 50 mV range
> > + * (gain 25 V/V), index 1 = 10 mV range (gain 125 V/V).
> > + */
> > +static const int max40080_csa_gain[] = {
> > +	MAX40080_CSA_50MV_GAIN, MAX40080_CSA_10MV_GAIN,
> > +};  
> 
> With a given comment and one time use of those definitions, do we need them
> at all?  I leave it to Jonathan, because my memory is weak on his preference
> in the cases like this.
> 

Defines for the indexes would be good so the ordering comment isn't needed.
The define as currently used don't benefit anyone.

static const int max400080_csa_gain[] = {
	[MAX400080_CFG_RANGE_50MV] = 25,
	[MAX400080_CFG_RANGE_10MV] = 125,
};

Related somewhat
> +#define MAX40080_REG_CFG		0x00
> +#define  MAX40080_MODE_MSK		GENMASK(2, 0)
> +#define  MAX40080_PEC_EN_MSK		BIT(5)
> +#define  MAX40080_RANGE_MSK		BIT(6)
> +#define  MAX40080_FILTER_MSK		GENMASK(14, 12)

The fields should be named so that it's is obvious which register they are in
then add field value defines where they are relevant - again naming them
to make register and field part of the name.

#define MAX400080_REG_CFG ...
#define   MAX40080_CFG_MODE_MSK ...
...
#define   MAX40080_CFG_RANGE_MSK
#define     MAX40080_CFG_RANGE_50MV	0
#define     MAX40080_CFG_RANGE_10MV     1

> ...

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Siratul Islam]

On Fri, 2026-07-03 at 13:29 +0300, Stefan Popa wrote:
> The MAX40080 is a bidirectional current-sense amplifier with an
> integrated 12-bit ADC and an I2C/SMBus interface. It measures the
> voltage across an external shunt resistor and the input bus voltage,
> storing the results in an internal FIFO.
> 
> 
Hi! I already looked at Andy's review and decided to add a few more stuff.
...
>  
> +MAXIM MAX40080 CURRENT SENSE AMPLIFIER DRIVER
> +M:	Ciprian Hegbeli <ciprian.hegbeli@analog.com>
> +M:	Stefan Popa <stefan.popa@analog.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Supported
> +W:	https://ez.analog.com/linux-software-drivers
> +F:	Documentation/devicetree/bindings/iio/adc/maxim,max40080.yaml
The Maintainer entry for binding should be in the binding patch,
> +F:	drivers/iio/adc/max40080.c
With the driver entry added in this patch.
> +
...
+ array_size.h
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/math64.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define MAX40080_REG_CFG		0x00
> +#define  MAX40080_MODE_MSK		GENMASK(2, 0)
> +#define  MAX40080_PEC_EN_MSK		BIT(5)
> +#define  MAX40080_RANGE_MSK		BIT(6)
> +#define  MAX40080_FILTER_MSK		GENMASK(14, 12)
Should be one space after #define, like the first one.
> +
> +#define MAX40080_REG_FIFO_CFG		0x0A
Here too
> +#define  MAX40080_STORE_IV_MSK		GENMASK(1, 0)
> +
> +#define MAX40080_REG_IV			0x10
> +/* Current is a 13-bit two's-complement value (magnitude + sign bit). */
> +#define  MAX40080_IV_I_MSK		GENMASK(12, 0)
> +#define  MAX40080_IV_I_SIGN_BIT		12
> +#define  MAX40080_IV_V_MAG_MSK		GENMASK(27, 16)
> +#define  MAX40080_IV_VALID_MSK		BIT(31)
> +
> +/* CFG.mode field */
> +#define MAX40080_STDBY_MODE		0x00
> +#define MAX40080_SINGLE_MODE		0x02	/* one conversion per Quick Command */
> +
> +/* FIFO_CFG.store_iv field */
> +#define MAX40080_STORE_I_V		0x02
> +
> +#define MAX40080_ADC_RES		4096
> +#define MAX40080_INTER_VREF_MV		1250
> +#define MAX40080_V_BUFF_GAIN		30
Maybe sort by value? just a nit.
> +#define MAX40080_CSA_50MV_GAIN		25
> +#define MAX40080_CSA_10MV_GAIN		125
> +
> +/*
> + * The RANGE field (CFG bit 6) selects one of two current-sense full-scale
> + * ranges (the MAX40080 supports exactly two: +/-50 mV and +/-10 mV). Ordered
> + * so that the array index equals the RANGE field value: index 0 = 50 mV range
> + * (gain 25 V/V), index 1 = 10 mV range (gain 125 V/V).
> + */
> +static const int max40080_csa_gain[] = {
> +	MAX40080_CSA_50MV_GAIN, MAX40080_CSA_10MV_GAIN,
Maybe 1 item per line since it's a macro? 
> +};
> +
> +#define MAX40080_NUM_RANGES	ARRAY_SIZE(max40080_csa_gain)
> +
> +struct max40080_state {
> +	struct i2c_client *client;
> +	/* Serializes read-modify-write access to the CFG register. */
> +	struct mutex lock;
> +	u32 shunt_resistor_uohm;
> +	/*
> +	 * Precomputed current scale (mA per code) for each RANGE setting, as
> +	 * {integer, nano} pairs for IIO_VAL_INT_PLUS_NANO. The range is
> +	 * selected by writing the corresponding scale.
> +	 */
> +	int current_scale[MAX40080_NUM_RANGES][2];
> +};
> +
> +static const int max40080_oversampling_avail[] = { 1, 8, 16, 32, 64, 128 };
> +
> +static int max40080_update_bits(struct max40080_state *st, u8 reg,
> +				u16 mask, u16 val)
This can fit in 1 line.
static int max40080_update_bits(struct max40080_state *st, u8 reg, u16 mask, u16 val)

> +{
> +	int ret;
> +	int tmp;
> +
> +	guard(mutex)(&st->lock);
> +
...
> + */
> +static int max40080_read_iv_once(struct max40080_state *st, u32 *iv)
> +{
> +	u8 buf[4];
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(st->client, MAX40080_REG_IV,
> +					    sizeof(buf), buf);
This also fits in 1 line but it would go 92 cols, so not sure which one is preferred.
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(buf))
> +		return -EIO;
> +
> +	*iv = get_unaligned_le32(buf);
> +
> +	return 0;
> +}
> +
> 
...
> +
> +static int max40080_get_current(struct max40080_state *st, int *val)
> +{
> +	u32 iv;
> +	int ret;
> +
> +	ret = max40080_read_iv(st, &iv);
> +	if (ret)
> +		return ret;
> +
> +	*val = sign_extend32(FIELD_GET(MAX40080_IV_I_MSK, iv),
> +			     MAX40080_IV_I_SIGN_BIT);
This can also be 1 line if you are going for that.
> +
> +	return 0;
> +}
> +
> +static int max40080_get_voltage(struct max40080_state *st, int *val)
> +{
> +	u32 iv;
> +	int ret;
> +
> +	ret = max40080_read_iv(st, &iv);
> +	if (ret)
> +		return ret;
> +
> +	*val = FIELD_GET(MAX40080_IV_V_MAG_MSK, iv);
> +
> +	return 0;
> +}
> +
> +static int max40080_get_range(struct max40080_state *st, unsigned int *range)
> +{
> +	int tmp;
> +
> +	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
I think tmp can be initialized, since it is only assigned once.
> +	if (tmp < 0)
> +		return tmp;
> +
> +	*range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
> +
> +	return 0;
> +}
> +
...
> +
> +/*
> + * The FILTER field selects digital averaging of N consecutive conversions
> + * (no averaging, 8, 16, 32, 64 or 128), which maps directly to the IIO
> + * oversampling ratio. Averaging reduces the effective output data rate by the
> + * same factor; the conversion rate itself is set by the separate ADC_RATE
> + * field.
> + */
> +static int max40080_get_oversampling_ratio(struct max40080_state *st, int *val)
> +{
> +	int tmp;
> +	u8 filter;
Reverse xmas tree.
+ u8 filter;
+ int tmp;
While you are at it, and since you are already using u8, tmp can be s32.
> +
> +	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	filter = FIELD_GET(MAX40080_FILTER_MSK, tmp);
> +	*val = (filter == 0) ? 1 : (8 << (filter - 1));
> +
> +	return 0;
> +}
> +
> +/*
> + * max40080_oversampling_avail[] is ordered so that its index is the FILTER
> + * field value (index 0 = no averaging, index 1 = 8x, ...). Return that index
> + * for an exact match, or -EINVAL for a value that is not on the list.
> + */
> +static int max40080_oversampling_to_filter(int val)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(max40080_oversampling_avail); i++)
> +		if (max40080_oversampling_avail[i] == val)
> +			return i;
Since this for is multiline, it could use scope, {}.
> +
> +	return -EINVAL;
> +}
> +
...
> +}
> +
> +static int max40080_read_avail(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       const int **vals, int *type, int *length,
> +			       long info)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type != IIO_CURRENT)
> +			return -EINVAL;
> +
> +		*vals = (int *)st->current_scale;
> +		*length = MAX40080_NUM_RANGES * 2;
> +		*type = IIO_VAL_INT_PLUS_NANO;
I think a space between the assignments and the return would read better. Personal preference. Your call.
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = max40080_oversampling_avail;
> +		*length = ARRAY_SIZE(max40080_oversampling_avail);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int max40080_reg_access(struct iio_dev *indio_dev,
> +			       unsigned int reg,
> +			       unsigned int write_val,
> +			       unsigned int *read_val)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);
> +
> +	if (read_val) {
> +		int val = i2c_smbus_read_word_data(st->client, reg);
> +
> +		if (val < 0)
> +			return val;
> +		*read_val = val;
Here too.
> +		return 0;
> +	}
> +
> +	return i2c_smbus_write_word_data(st->client, reg, write_val);
> +}
...
> 
> +}
> +
> +static const struct i2c_device_id max40080_i2c_ids[] = {
> +	{ "max40080" },
.name = "max40080"
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max40080_i2c_ids);
> +
> +static const struct of_device_id max40080_of_match[] = {
> +	{ .compatible = "maxim,max40080" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max40080_of_match);
> +
> +static struct i2c_driver max40080_driver = {
> +	.driver = {
> +		.name = "max40080",
> +		.of_match_table = max40080_of_match,
> +	},
> +	.probe = max40080_probe,
> +	.id_table = max40080_i2c_ids,
> +};
> +module_i2c_driver(max40080_driver);
> +
> +MODULE_AUTHOR("Ciprian Hegbeli <ciprian.hegbeli@analog.com>");
> +MODULE_AUTHOR("Stefan Popa <stefan.popa@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices MAX40080 current-sense amplifier driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[David Lechner]

On 7/3/26 2:42 PM, Siratul Islam wrote:
> On Fri, 2026-07-03 at 13:29 +0300, Stefan Popa wrote:
>> The MAX40080 is a bidirectional current-sense amplifier with an
>> integrated 12-bit ADC and an I2C/SMBus interface. It measures the
>> voltage across an external shunt resistor and the input bus voltage,
>> storing the results in an internal FIFO.
>>
>>
> Hi! I already looked at Andy's review and decided to add a few more stuff.
> ...

>> +#define MAX40080_REG_CFG		0x00
>> +#define  MAX40080_MODE_MSK		GENMASK(2, 0)
>> +#define  MAX40080_PEC_EN_MSK		BIT(5)
>> +#define  MAX40080_RANGE_MSK		BIT(6)
>> +#define  MAX40080_FILTER_MSK		GENMASK(14, 12)
> Should be one space after #define, like the first one.
>> +
Actually, Jonathan likes to have the extra space to group the
fields of the register under the register address like this.

And even better is to include the register name in the field
macros too, e.g. MAX40080_CFG_MODE_MSK.

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Andy Shevchenko]

On Sat, Jul 04, 2026 at 01:42:39AM +0600, Siratul Islam wrote:
> On Fri, 2026-07-03 at 13:29 +0300, Stefan Popa wrote:

...

> > +#define MAX40080_REG_CFG		0x00
> > +#define  MAX40080_MODE_MSK		GENMASK(2, 0)
> > +#define  MAX40080_PEC_EN_MSK		BIT(5)
> > +#define  MAX40080_RANGE_MSK		BIT(6)
> > +#define  MAX40080_FILTER_MSK		GENMASK(14, 12)
> Should be one space after #define, like the first one.

I saw this but left uncommented as sometimes people use this style to
distinguish bit field definitions from the register offsets. When one space is
in use it might be not so easy to put the borders. I.o.w. I have no
strong opinion on this style, but if you think we should be all the same for
all IIO drivers here, I'm fine.

> > +#define MAX40080_REG_FIFO_CFG		0x0A
> Here too
> > +#define  MAX40080_STORE_IV_MSK		GENMASK(1, 0)

...and here...

> > +#define MAX40080_REG_IV			0x10
> > +/* Current is a 13-bit two's-complement value (magnitude + sign bit). */
> > +#define  MAX40080_IV_I_MSK		GENMASK(12, 0)
> > +#define  MAX40080_IV_I_SIGN_BIT		12
> > +#define  MAX40080_IV_V_MAG_MSK		GENMASK(27, 16)
> > +#define  MAX40080_IV_VALID_MSK		BIT(31)

...and here.

> > +/* CFG.mode field */
> > +#define MAX40080_STDBY_MODE		0x00
> > +#define MAX40080_SINGLE_MODE		0x02	/* one conversion per Quick Command */

...

> This can fit in 1 line.
> static int max40080_update_bits(struct max40080_state *st, u8 reg, u16 mask, u16 val)


Here...

...

> > +	ret = i2c_smbus_read_i2c_block_data(st->client, MAX40080_REG_IV,
> > +					    sizeof(buf), buf);
> This also fits in 1 line but it would go 92 cols, so not sure which one is preferred.

...and here the wrap is done on logical border, so I think it's fine and reads
well.

...

> > +static int max40080_get_range(struct max40080_state *st, unsigned int *range)
> > +{
> > +	int tmp;
> > +
> > +	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
> I think tmp can be initialized, since it is only assigned once.

I don't get this comment. You mean switching to ret?

> > +	if (tmp < 0)
> > +		return tmp;
> > +
> > +	*range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
> > +
> > +	return 0;
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Siratul Islam]

On 26/07/04 03:05PM, Andy Shevchenko wrote:
> On Sat, Jul 04, 2026 at 01:42:39AM +0600, Siratul Islam wrote:
> > On Fri, 2026-07-03 at 13:29 +0300, Stefan Popa wrote:
> 
> ...
> 
> > > +#define MAX40080_REG_CFG		0x00
> > > +#define  MAX40080_MODE_MSK		GENMASK(2, 0)
> > > +#define  MAX40080_PEC_EN_MSK		BIT(5)
> > > +#define  MAX40080_RANGE_MSK		BIT(6)
> > > +#define  MAX40080_FILTER_MSK		GENMASK(14, 12)
> > Should be one space after #define, like the first one.
> 
> I saw this but left uncommented as sometimes people use this style to
> distinguish bit field definitions from the register offsets. When one space is
> in use it might be not so easy to put the borders. I.o.w. I have no
> strong opinion on this style, but if you think we should be all the same for
> all IIO drivers here, I'm fine.
I looked at existing drivers and most of them had one space so I thought
it would be better to keep them consistent. But apparently, as David
also noted, this style is acceptable. So no problem for me.
> 
> 
...
> > > +/* CFG.mode field */
> > > +#define MAX40080_STDBY_MODE		0x00
> > > +#define MAX40080_SINGLE_MODE		0x02	/* one conversion per Quick Command */
> 
> ...
> 
> > This can fit in 1 line.
> > static int max40080_update_bits(struct max40080_state *st, u8 reg, u16 mask, u16 val)
> 
> 
> Here...
Keeping this function single line results in 86 cols, which is just a
little more than 80. So I thought splitting here was avoidable. But if
you think it's an acceptable split then so be it.
> 
> ...
> 
> > > +	ret = i2c_smbus_read_i2c_block_data(st->client, MAX40080_REG_IV,
> > > +					    sizeof(buf), buf);
> > This also fits in 1 line but it would go 92 cols, so not sure which one is preferred.
> 
> ...and here the wrap is done on logical border, so I think it's fine and reads
> well.
This one might be pushing too far so I had my doubts.
> 
> ...
> 
> > > +static int max40080_get_range(struct max40080_state *st, unsigned int *range)
> > > +{
> > > +	int tmp;
> > > +
> > > +	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
> > I think tmp can be initialized, since it is only assigned once.
> 
> I don't get this comment. You mean switching to ret?
> 
I meant tmp is assigned only once so the indirection here, i.e.
declaring and assigning in two steps doesn't buy us anything. Instead,
it could be initialized like "int tmp = i2c_smbus_read_word_data()."
> > > +	if (tmp < 0)
> > > +		return tmp;
> > > +
> > > +	*range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
> > > +
> > > +	return 0;
> > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Andy Shevchenko]

On Sat, Jul 04, 2026 at 10:32:32PM +0600, Siratul Islam wrote:
> On 26/07/04 03:05PM, Andy Shevchenko wrote:
> > On Sat, Jul 04, 2026 at 01:42:39AM +0600, Siratul Islam wrote:
> > > On Fri, 2026-07-03 at 13:29 +0300, Stefan Popa wrote:

...

> > > This can fit in 1 line.
> > > static int max40080_update_bits(struct max40080_state *st, u8 reg, u16 mask, u16 val)
> > 
> > Here...
> Keeping this function single line results in 86 cols, which is just a
> little more than 80. So I thought splitting here was avoidable. But if
> you think it's an acceptable split then so be it.

Split is a safe choice. No split is a slippery slope depending on some factors.

...

> > > > +static int max40080_get_range(struct max40080_state *st, unsigned int *range)
> > > > +{
> > > > +	int tmp;
> > > > +
> > > > +	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
> > > I think tmp can be initialized, since it is only assigned once.
> > 
> > I don't get this comment. You mean switching to ret?
> > 
> I meant tmp is assigned only once so the indirection here, i.e.
> declaring and assigning in two steps doesn't buy us anything. Instead,
> it could be initialized like "int tmp = i2c_smbus_read_word_data()."

Ah, definitely no to this suggestion. It makes code harder to maintain
and the pattern you proposed is actually discouraged. You can search in
mail archive and find like ~1-2 year old message from me with the detailed
explanation why.

> > > > +	if (tmp < 0)
> > > > +		return tmp;
> > > > +
> > > > +	*range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
> > > > +
> > > > +	return 0;
> > > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Siratul Islam]

July 4, 2026 at 11:05 PM, "Andy Shevchenko" <andriy.shevchenko@intel.com mailto:andriy.shevchenko@intel.com?to=%22Andy%20Shevchenko%22%20%3Candriy.shevchenko%40intel.com%3E > wrote:


> 
> On Sat, Jul 04, 2026 at 10:32:32PM +0600, Siratul Islam wrote:
> 
> > 
> > On 26/07/04 03:05PM, Andy Shevchenko wrote:
> >  On Sat, Jul 04, 2026 at 01:42:39AM +0600, Siratul Islam wrote:
> >  > On Fri, 2026-07-03 at 13:29 +0300, Stefan Popa wrote:
> > 
> ...
> 
...
> 
> > 
> > > > +static int max40080_get_range(struct max40080_state *st, unsigned int *range)
> >  > > +{
> >  > > + int tmp;
> >  > > +
> >  > > + tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
> >  > I think tmp can be initialized, since it is only assigned once.
> >  
> >  I don't get this comment. You mean switching to ret?
> >  
> >  I meant tmp is assigned only once so the indirection here, i.e.
> >  declaring and assigning in two steps doesn't buy us anything. Instead,
> >  it could be initialized like "int tmp = i2c_smbus_read_word_data()."
> > 
> Ah, definitely no to this suggestion. It makes code harder to maintain
> and the pattern you proposed is actually discouraged. You can search in
> mail archive and find like ~1-2 year old message from me with the detailed
> explanation why.
> 
Thanks! I'll look into it. Learning a lot of stuff. But I'm trying to understand where to draw the line though.
Like "struct xxx* data = iio_priv(indio_dev);" and "s64 ts = iio_get_time_ns(indio_dev);" are pretty common. 
Do these calls make a special case for it?
> > 
> > > > + if (tmp < 0)
> >  > > + return tmp;
> >  > > +
> >  > > + *range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
> >  > > +
> >  > > + return 0;
> >  > > +}
> > 
> -- 
> With Best Regards,
> Andy Shevchenko
>

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Andy Shevchenko]

On Sat, Jul 4, 2026 at 8:30 PM Siratul Islam <siratul.islam@linux.dev> wrote:
> July 4, 2026 at 11:05 PM, "Andy Shevchenko" <andriy.shevchenko@intel.com mailto:andriy.shevchenko@intel.com?to=%22Andy%20Shevchenko%22%20%3Candriy.shevchenko%40intel.com%3E > wrote:
> > On Sat, Jul 04, 2026 at 10:32:32PM +0600, Siratul Islam wrote:
> > > On 26/07/04 03:05PM, Andy Shevchenko wrote:
> > >  On Sat, Jul 04, 2026 at 01:42:39AM +0600, Siratul Islam wrote:
> > >  > On Fri, 2026-07-03 at 13:29 +0300, Stefan Popa wrote:

...

> > > > > +static int max40080_get_range(struct max40080_state *st, unsigned int *range)
> > >  > > +{
> > >  > > + int tmp;
> > >  > > +
> > >  > > + tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
> > >  > I think tmp can be initialized, since it is only assigned once.
> > >
> > >  I don't get this comment. You mean switching to ret?
> > >
> > >  I meant tmp is assigned only once so the indirection here, i.e.
> > >  declaring and assigning in two steps doesn't buy us anything. Instead,
> > >  it could be initialized like "int tmp = i2c_smbus_read_word_data()."
> > >
> > Ah, definitely no to this suggestion. It makes code harder to maintain
> > and the pattern you proposed is actually discouraged. You can search in
> > mail archive and find like ~1-2 year old message from me with the detailed
> > explanation why.
> >
> Thanks! I'll look into it. Learning a lot of stuff. But I'm trying to understand where to draw the line though.
> Like "struct xxx* data = iio_priv(indio_dev);" and "s64 ts = iio_get_time_ns(indio_dev);" are pretty common.
> Do these calls make a special case for it?

When the assigned value is coupled with a (validation) check,
definitely do not assign and define simultaneously. When it's used as
a counter, it's better to assign it closer to the user (meaning the
counters that can't be folded into a for-loop). This is the rough rule
of thumb. Plus use common sense.

> > > > > + if (tmp < 0)
> > >  > > + return tmp;
> > >  > > +
> > >  > > + *range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
> > >  > > +
> > >  > > + return 0;
> > >  > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[David Lechner]

On 7/3/26 5:29 AM, Stefan Popa wrote:
> The MAX40080 is a bidirectional current-sense amplifier with an
> integrated 12-bit ADC and an I2C/SMBus interface. It measures the
> voltage across an external shunt resistor and the input bus voltage,
> storing the results in an internal FIFO.
> 
> Add a direct-mode IIO driver exposing the current and voltage channels
> with raw, scale and hardware-gain attributes, a configurable

Patch does not implemnt hardware-gain attribute (which is correct,
so just fix the commit message).

> oversampling (digital averaging) ratio, and PEC-protected register
> access. The current scale is derived from the shunt resistor value
> described in the device tree.
> 
> +static int max40080_update_bits(struct max40080_state *st, u8 reg,
> +				u16 mask, u16 val)
> +{
> +	int ret;
> +	int tmp;
> +
> +	guard(mutex)(&st->lock);

Usually we don't want the lock at this level in case anything needs to
update more than one register in an atomic operation.

> +
> +	tmp = i2c_smbus_read_word_data(st->client, reg);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	tmp &= ~mask;
> +	tmp |= val & mask;
> +
> +	ret = i2c_smbus_write_word_data(st->client, reg, tmp);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +

...

> +static int max40080_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val,
> +			     int *val2,
> +			     long mask)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);
> +	unsigned int range;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (chan->type == IIO_CURRENT) {
> +			ret = max40080_get_current(st, val);
> +			if (ret)
> +				return ret;
> +		} else if (chan->type == IIO_VOLTAGE) {
> +			ret = max40080_get_voltage(st, val);
> +			if (ret)
> +				return ret;
> +		}

Se usually use switch statement in IIO instead of else if.

> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_CURRENT) {
> +			/*
> +			 * The selectable current-sense range is exposed through
> +			 * scale: each RANGE setting has its own precomputed
> +			 * mA-per-code value. Userspace picks the range by writing
> +			 * the matching scale.
> +			 */
> +			ret = max40080_get_range(st, &range);
> +			if (ret)
> +				return ret;
> +
> +			*val = st->current_scale[range][0];
> +			*val2 = st->current_scale[range][1];
> +			return IIO_VAL_INT_PLUS_NANO;
> +		}
> +		/* voltage[mV] = raw * Vref[mV] * buffer_gain / ADC_RES */
> +		*val = MAX40080_INTER_VREF_MV * MAX40080_V_BUFF_GAIN;
> +		*val2 = MAX40080_ADC_RES;
> +		return IIO_VAL_FRACTIONAL;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = max40080_get_oversampling_ratio(st, val);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

...

> +static int max40080_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct max40080_state *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WORD_DATA |
> +				     I2C_FUNC_SMBUS_I2C_BLOCK |
> +				     I2C_FUNC_SMBUS_QUICK))
> +		return -EOPNOTSUPP;
> +
> +	client->flags |= I2C_CLIENT_PEC;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	st = iio_priv(indio_dev);
> +	st->client = client;
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> +				     &st->shunt_resistor_uohm))
> +		st->shunt_resistor_uohm = 1000000; /* default 1 ohm */
> +
> +	if (!st->shunt_resistor_uohm)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "shunt-resistor-micro-ohms must be non-zero\n");
> +
> +	max40080_calc_current_scale(st);
> +
> +	indio_dev->name = "max40080";
> +	indio_dev->info = &max40080_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = max40080_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max40080_channels);
> +
> +	/* No averaging by default; configurable at runtime via sysfs. */
> +	ret = max40080_init(st, 1);

Why have a paramter if it is always the same value? Can just move
this comment into the init function and drop the arg.

> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id max40080_i2c_ids[] = {
> +	{ "max40080" },

Include `.name = `

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max40080_i2c_ids);
> +

Re: [PATCH v1 2/2] iio: adc: add MAX40080 current-sense amplifier driver

[Jonathan Cameron]

On Fri, 3 Jul 2026 13:29:32 +0300
Stefan Popa <stefan.popa@analog.com> wrote:

> The MAX40080 is a bidirectional current-sense amplifier with an
> integrated 12-bit ADC and an I2C/SMBus interface. It measures the
> voltage across an external shunt resistor and the input bus voltage,
> storing the results in an internal FIFO.
> 
> Add a direct-mode IIO driver exposing the current and voltage channels
> with raw, scale and hardware-gain attributes, a configurable
> oversampling (digital averaging) ratio, and PEC-protected register
> access. The current scale is derived from the shunt resistor value
> described in the device tree.
> 
> Signed-off-by: Ciprian Hegbeli <ciprian.hegbeli@analog.com>
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>

You have some thorough reviews already but who can resist
a new driver on a Friday afternoon ;)

I tried to avoid repetition with existing reviews so didn't have
much to add.

Thanks,

Jonathan

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


> +
> +static int max40080_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct max40080_state *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WORD_DATA |
> +				     I2C_FUNC_SMBUS_I2C_BLOCK |
> +				     I2C_FUNC_SMBUS_QUICK))
> +		return -EOPNOTSUPP;
> +
> +	client->flags |= I2C_CLIENT_PEC;

> +	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> +				     &st->shunt_resistor_uohm))
> +		st->shunt_resistor_uohm = 1000000; /* default 1 ohm */

Over time we've evolved our preferences for this based on what ends up
readable. For new code something like
	if (device_property_present(dev, "shunt-resistor-micro-ohms)) {
		ret = device_property_read_u32(dev, "shunt-resistor-micro-ohms",
					       &st->shunt_resistor_uohm);
		if (ret)
			return ret;
	} else {
		st->shunt_resistor_uohm = 1 * MICRO; /* default 1 ohm */
	}
	
> +
> +	if (!st->shunt_resistor_uohm)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "shunt-resistor-micro-ohms must be non-zero\n");
> +
> +	max40080_calc_current_scale(st);

Re: [PATCH v1 0/2] iio: adc: add MAX40080 current-sense amplifier driver

[Andy Shevchenko]

On Fri, Jul 03, 2026 at 01:29:30PM +0300, Stefan Popa wrote:
> This series adds support for the Maxim MAX40080, a bidirectional
> current-sense amplifier with an integrated 12-bit ADC and an I2C/SMBus
> interface. It measures the voltage across an external shunt resistor and
> the input bus voltage.
> 
> The driver operates in direct (INDIO_DIRECT_MODE) mode. Each raw read
> triggers a single on-demand conversion (SMBus Quick Command) and reads
> back the matched current/voltage pair, so results are always fresh. It
> exposes the current and voltage channels with raw and scale attributes,
> a configurable oversampling (digital averaging) ratio, and PEC-protected
> register access. The two selectable current-sense ranges are exposed
> through scale/scale_available (the range is chosen by writing the
> desired scale); the current scale is derived from the
> shunt-resistor-micro-ohms device-tree property.
> 
> Continuous FIFO buffering, threshold events and the alert interrupt are
> intentionally left out of this initial submission and may be added
> later.
> 
> Tested on hardware with four MAX40080 devices on an I2C bus.

For a new driver it misses two things:
- why do we need a brand new driver? (Can we extend existing one to cover
  this HW?)
- where to get a datashee? (Link?)

-- 
With Best Regards,
Andy Shevchenko