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

[Nuno Sá]

On Wed, Jul 15, 2026 at 11:34:09AM +0300, Andy Shevchenko wrote:
> 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.

I think checkpatch also complains if you do it like the above.

- Nuno Sá

> 
> ...
> 
> > +/* 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
> 
>