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

[Andy Shevchenko]

On Wed, Jul 15, 2026 at 09:36:17AM +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.

...

> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX40080.pdf
> 

No blank line here, in a tag block. Also you can use Datasheet: tag
(but it's up to you).

> Co-developed-by: Ciprian Hegbeli <ciprian.hegbeli@xxxxxxxxxx>
> Signed-off-by: Ciprian Hegbeli <ciprian.hegbeli@xxxxxxxxxx>
> Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> ---

...

> +MAXIM MAX40080 CURRENT SENSE AMPLIFIER DRIVER
> +M:	Ciprian Hegbeli <ciprian.hegbeli@xxxxxxxxxx>
> +M:	Stefan Popa <stefan.popa@xxxxxxxxxx>
> +L:	linux-iio@xxxxxxxxxxxxxxx
> +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

David usually asks this to be split between patches to avoid "orphaned" files
from the MAINTAINERS perspective.

...

> +/* Current is a 13-bit two's-complement value (magnitude + sign bit). */

Please, choose a single style for _one-line_ comments, id est
period in all or no period, capital first letter in all or small letter.

> +#define MAX40080_CFG_MODE_SINGLE	0x02	/* one conversion per Quick Command */
> +
> +/* CFG.range field values */

(Three comments on one page of code and three different styles.)

...

> +	u32 shunt_resistor_uohm;

I think this also would be good as uOhm.

https://web.archive.org/web/20250629194735/http://poynton.ca/notes/units/
mentions this:

"... except that its initial letter is capitalized if the unit is named after a person."

...

> +static int max40080_trigger_measurement(struct max40080_state *st)
> +{
> +	struct i2c_client *client = st->client;

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

Perhaps even

	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;

What I meant is this

	u32 tmp = *iv;


> +	/*
> +	 * 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);

	ret = read_poll_timeout(max40080_read_iv_once, io_ret,
				io_ret || (tmp & MAX40080_IV_VALID_MSK),
				1 * USEC_PER_MSEC, 50 * USEC_PER_MSEC,
				false, st, &tmp);

	/* ...the comment why we need to update iv even in error case... */
	*iv = tmp;

> +	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 < ARRAY_SIZE(max40080_csa_gain); i++) {

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

> +		tmp = (u64)MAX40080_INTER_VREF_mV * NANO * MICRO;

I would even make another temporary for the numerator and denominator.

		u64 numerator, denominator;

		numerator = (u64)MAX40080_INTER_VREF_mV * NANO * MICRO;
		denominator = (u64)MAX40080_ADC_RES * max40080_csa_gain[i];

// Also possible to use a trick "1ULL * " instead of castings.

		numerator = 1ULL * MAX40080_INTER_VREF_mV * NANO * MICRO;
		denominator = 1ULL * MAX40080_ADC_RES * max40080_csa_gain[i];

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

		tmp = div64_u64(numerator, denominator * st->shunt_resistor_uOhm);

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

...

> +static int max40080_oversampling_to_filter(int val)
> +{
> +	for (int i = 0; i < ARRAY_SIZE(max40080_oversampling_avail); i++) {

unsigned int ?

> +		if (max40080_oversampling_avail[i] == val)
> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}

...

> +	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 dev_err_probe(dev, ret,
> +					     "can't read shunt-resistor-micro-ohms\n");
> +		if (!st->shunt_resistor_uohm)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "shunt-resistor-micro-ohms must be non-zero\n");

You can reduce data footprint by string literal deduplication. That's why in my example I used

	const char *propname;

and respective assignment. Currently you have three copies of the property
name: two in different error messages and one as a parameter to property APIs.

> +	} else {
> +		st->shunt_resistor_uohm = 1 * MICRO;
> +	}

-- 
With Best Regards,
Andy Shevchenko