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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Jul 13, 2026 at 03:02:26PM +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.
> 
> No existing IIO driver covers this device or a register-compatible part.
> The closest relatives target different silicon with incompatible register
> maps and feature sets: max9611 is a unidirectional high-side sensor with a
> die-temperature channel and MUX-selected gain and no FIFO/PEC, while
> max34408 is an 8-bit multi-channel current monitor. The MAX40080 has a
> device-specific register map with bidirectional 13-bit current, a 64-entry
> FIFO, PEC, a single-measurement mode triggered by an SMBus Quick Command,
> and two selectable input ranges, so it warrants its own driver.
> 
> Add a direct-mode IIO driver exposing 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
> current scale is derived from the shunt-resistor-micro-ohms device-tree
> property.

...

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

Should be not used since Uwe's rework of this header.

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>

...

> +/* CFG.range field values */
> +#define MAX40080_CFG_RANGE_50mV		0
> +#define MAX40080_CFG_RANGE_10mV		1
> +
> +/* FIFO_CFG.store_iv field values */
> +#define MAX40080_FIFO_CFG_STORE_I_V	0x02
> +
> +#define MAX40080_ADC_RES		4096
> +#define MAX40080_INTER_VREF_MV		1250

Let's also use _mV here.

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

And here.

...

> +#define MAX40080_NUM_RANGES	ARRAY_SIZE(max40080_csa_gain)

I think I commented on this already. This hides the information, I think
the better way is to avoid using this macro.

...

> +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),
> +				1 * USEC_PER_MSEC, 50 * USEC_PER_MSEC,
> +				false, st, iv);
> +	if (ret)
> +		return ret;

Even in case of error the value in iv will be updated. Is it deliberately?

> +	return io_ret;
> +}

...

> +static int max40080_get_oversampling_ratio(struct max40080_state *st, int *val)
> +{
> +	int tmp;
> +	u8 filter;

Keep reversed xmas tree order.

> +	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	filter = FIELD_GET(MAX40080_CFG_FILTER_MSK, tmp);
> +	*val = max40080_oversampling_avail[filter];
> +
> +	return 0;
> +}

...

> +static int max40080_set_oversampling_ratio(struct max40080_state *st, int val)
> +{
> +	int ret, filter = max40080_oversampling_to_filter(val);

This style is discouraged. First of all, it's a mix of semantically different
variables. Second, the assignment of the variable that is going to be validated
might lead to subtle issues in the future.

> +	if (filter < 0)
> +		return filter;

So, Do just

	int filter;
	int ret;

	filter = max40080_oversampling_to_filter(val);
	if (filter < 0)
		return filter;

> +	ret = max40080_update_bits(st, MAX40080_REG_CFG, MAX40080_CFG_FILTER_MSK,
> +				   FIELD_PREP(MAX40080_CFG_FILTER_MSK, filter));
> +	if (ret)
> +		return ret;
> +
> +	st->oversampling_ratio = val;
> +
> +	return 0;
> +}

...

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

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

At least these two may be on a single line.

> +			     long mask)

...

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

You see, like here.

...

> +		*vals = (int *)st->current_scale;

Why not const int?

> +		*length = MAX40080_NUM_RANGES * 2;
> +		*type = IIO_VAL_INT_PLUS_NANO;

...

> +static int max40080_init(struct max40080_state *st)
> +{
> +	u16 fifo_cfg, cfg;
> +	int ret, filter;

Here is better, but see above.

> +	filter = max40080_oversampling_to_filter(st->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.
> +	 */
> +	cfg = FIELD_PREP(MAX40080_CFG_MODE_MSK, MAX40080_CFG_MODE_STDBY) |
> +	      FIELD_PREP(MAX40080_CFG_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_FIFO_CFG_STORE_IV_MSK, MAX40080_FIFO_CFG_STORE_I_V);
> +	ret = i2c_smbus_write_word_data(st->client, MAX40080_REG_FIFO_CFG,
> +					fifo_cfg);

Previous line is long enough to justify this one to be also 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_CFG_MODE_MSK, MAX40080_CFG_MODE_SINGLE) |
> +	      FIELD_PREP(MAX40080_CFG_PEC_EN_MSK, 1) |
> +	      FIELD_PREP(MAX40080_CFG_RANGE_MSK, st->range) |
> +	      FIELD_PREP(MAX40080_CFG_FILTER_MSK, filter);

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

Just

	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 max40080_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	/*
> +	 * The device powers up with PEC enabled (CFG POR = 0x0060) and rejects
> +	 * unprotected transactions, so PEC support is mandatory, along with word
> +	 * access, the I2C block read used for the current/voltage pair, and the
> +	 * Quick Command used to trigger a conversion.
> +	 */
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WORD_DATA |
> +				     I2C_FUNC_SMBUS_I2C_BLOCK |
> +				     I2C_FUNC_SMBUS_QUICK |
> +				     I2C_FUNC_SMBUS_PEC))
> +		return -EOPNOTSUPP;
> +
> +	client->flags |= I2C_CLIENT_PEC;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->client = client;
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;

> +	st->shunt_resistor_uohm = 1 * MICRO;
> +	device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> +				 &st->shunt_resistor_uohm);
> +	if (!st->shunt_resistor_uohm)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "shunt-resistor-micro-ohms must be non-zero\n");

We now establish better approach for optional values

	const char *propname;

	propname = "shunt-resistor-micro-ohms";
	if (device_property_present(dev, propname)) {
		ret = device_property_read_u32(dev, propname, &st->shunt_resistor_uohm);
		if (ret)
			return dev_err_probe(dev, ret, "can't read %s\n", propname);
		if (!st->shunt_resistor_uohm)
			return dev_err_probe(dev, -EINVAL, "%s must be non-zero\n", propname);
	} else {
		st->shunt_resistor_uohm = 1 * MICRO;
	}

> +	max40080_calc_current_scale(st);
> +
> +	/* Defaults: 50 mV range (index 0), no averaging. */
> +	st->range = 0;
> +	st->oversampling_ratio = 1;
> +
> +	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);
> +
> +	ret = max40080_init(st);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

-- 
With Best Regards,
Andy Shevchenko