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

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Fri, 17 Jul 2026 15:38:49 +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.
> 
> 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.
> 
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX40080.pdf
> Co-developed-by: Ciprian Hegbeli <ciprian.hegbeli@analog.com>
> Signed-off-by: Ciprian Hegbeli <ciprian.hegbeli@analog.com>
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>

For the sashiko feedback about *iv not being initialised if it exits
on first loop.  I'd just set tmp to 0 before calling the polling function.

The other one is a race even with locking as the reader either gets the before
value or the after one, so up to you if you add the suggested lock.


A few other things inline

Thanks,

Jonathan


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

> +struct max40080_state {
> +	struct i2c_client *client;
> +	/* Serializes read-modify-write access to the CFG register. */
> +	struct mutex lock;
> +	u32 shunt_resistor_uOhm;
> +	/*
> +	 * Cached configuration, also used to restore the device on resume after
> +	 * a suspend that may have cut its power: the selected RANGE index and
> +	 * the oversampling ratio.
> +	 */
> +	unsigned int range;
> +	int oversampling_ratio;
> +	/*
> +	 * 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[ARRAY_SIZE(max40080_csa_gain)][2];
> +	/* DMA-safe buffer for i2c_smbus_read_i2c_block_data(). */
> +	u8 buf[4] ____cacheline_aligned;

Already came up so see that discussion.  I don't mind that much if you
use __align(ARCH_DMA_MINALIGN) rather than the IIO one because this doesn't
need the 8 byte enforcement.  However, it's an __le32 so better to give
it that type and cast if you need to for the i2c calls.  Can then use the
aligned converters (which are probably a noop on every system you care about!)
 
> +};
> +
> +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)

The __ is I guess to indicate that the lock should be held? There are other
functions that have that characteristic but don't have __ prefix so not
sure I'd bother.

> +{
> +	int tmp;
> +
> +	tmp = i2c_smbus_read_word_data(st->client, reg);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	tmp = (tmp & ~mask) | (val & mask);
> +
> +	return i2c_smbus_write_word_data(st->client, reg, tmp);
> +}
> +
> +/*
> + * 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.
> + */

This correlated pair thing is a pretty strong reason to add buffered support
which could ensure userspace gets a pair that match.


> +
> +/*
> + * 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)
> +{
> +	u8 filter;
> +	int tmp;
> +
> +	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	filter = FIELD_GET(MAX40080_CFG_FILTER_MSK, tmp);
> +	if (filter >= ARRAY_SIZE(max40080_oversampling_avail))
> +		return -EIO;

Why get this rather than returning the cached value of the oversampling_ratio
directly?

> +
> +	*val = max40080_oversampling_avail[filter];
> +
> +	return 0;
> +}



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

I'm not keen on the scope being added here just so you can declare
at the top of it.  Just declare propname at the top of the function
and assign it here.


> +		const char *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;

This default isn't in the dt-binding. I'm not sure why this value makes sense.
If there is a reason the default is useful to have, then I'd expect a specification
reference in a comment to justify it.

> +		}
> +	}
> +
> +	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);
> +}
> +
> +static int max40080_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct max40080_state *st = iio_priv(indio_dev);
> +
> +	/*
> +	 * A suspend may have cut power to the device, resetting it to its
> +	 * power-on defaults. Reprogram it from the cached configuration.

Papering over suspend disabling power seems wrong to me.
Note that for IIO we've always allowed drivers without suspend support
because it many industrial platforms it is completely irrelevant.

If we want the driver to support that let use opt in completely and do
the supply regulator disable / enable.  I.e. Let's take control
of the potential sequence.  I'm going to guess that this was responding
to sashiko feedback rather than a human reviewer?

Anyone else have a strong feeling on what we should do in these circumstances?

> +	 */
> +	return max40080_init(st);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(max40080_pm_ops, NULL, max40080_resume);