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

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

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