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

[Siratul Islam]

On Fri, 2026-07-03 at 13:29 +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.
> 
> 
Hi! I already looked at Andy's review and decided to add a few more stuff.
...
>  
> +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
The Maintainer entry for binding should be in the binding patch,
> +F:	drivers/iio/adc/max40080.c
With the driver entry added in this patch.
> +
...
+ array_size.h
> +#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>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define MAX40080_REG_CFG		0x00
> +#define  MAX40080_MODE_MSK		GENMASK(2, 0)
> +#define  MAX40080_PEC_EN_MSK		BIT(5)
> +#define  MAX40080_RANGE_MSK		BIT(6)
> +#define  MAX40080_FILTER_MSK		GENMASK(14, 12)
Should be one space after #define, like the first one.
> +
> +#define MAX40080_REG_FIFO_CFG		0x0A
Here too
> +#define  MAX40080_STORE_IV_MSK		GENMASK(1, 0)
> +
> +#define MAX40080_REG_IV			0x10
> +/* Current is a 13-bit two's-complement value (magnitude + sign bit). */
> +#define  MAX40080_IV_I_MSK		GENMASK(12, 0)
> +#define  MAX40080_IV_I_SIGN_BIT		12
> +#define  MAX40080_IV_V_MAG_MSK		GENMASK(27, 16)
> +#define  MAX40080_IV_VALID_MSK		BIT(31)
> +
> +/* CFG.mode field */
> +#define MAX40080_STDBY_MODE		0x00
> +#define MAX40080_SINGLE_MODE		0x02	/* one conversion per Quick Command */
> +
> +/* FIFO_CFG.store_iv field */
> +#define MAX40080_STORE_I_V		0x02
> +
> +#define MAX40080_ADC_RES		4096
> +#define MAX40080_INTER_VREF_MV		1250
> +#define MAX40080_V_BUFF_GAIN		30
Maybe sort by value? just a nit.
> +#define MAX40080_CSA_50MV_GAIN		25
> +#define MAX40080_CSA_10MV_GAIN		125
> +
> +/*
> + * 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,
Maybe 1 item per line since it's a macro? 
> +};
> +
> +#define MAX40080_NUM_RANGES	ARRAY_SIZE(max40080_csa_gain)
> +
> +struct max40080_state {
> +	struct i2c_client *client;
> +	/* Serializes read-modify-write access to the CFG register. */
> +	struct mutex lock;
> +	u32 shunt_resistor_uohm;
> +	/*
> +	 * 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[MAX40080_NUM_RANGES][2];
> +};
> +
> +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)
This can fit in 1 line.
static int max40080_update_bits(struct max40080_state *st, u8 reg, u16 mask, u16 val)

> +{
> +	int ret;
> +	int tmp;
> +
> +	guard(mutex)(&st->lock);
> +
...
> + */
> +static int max40080_read_iv_once(struct max40080_state *st, u32 *iv)
> +{
> +	u8 buf[4];
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(st->client, MAX40080_REG_IV,
> +					    sizeof(buf), buf);
This also fits in 1 line but it would go 92 cols, so not sure which one is preferred.
> +	if (ret < 0)
> +		return ret;
> +	if (ret != sizeof(buf))
> +		return -EIO;
> +
> +	*iv = get_unaligned_le32(buf);
> +
> +	return 0;
> +}
> +
> 
...
> +
> +static int max40080_get_current(struct max40080_state *st, int *val)
> +{
> +	u32 iv;
> +	int ret;
> +
> +	ret = max40080_read_iv(st, &iv);
> +	if (ret)
> +		return ret;
> +
> +	*val = sign_extend32(FIELD_GET(MAX40080_IV_I_MSK, iv),
> +			     MAX40080_IV_I_SIGN_BIT);
This can also be 1 line if you are going for that.
> +
> +	return 0;
> +}
> +
> +static int max40080_get_voltage(struct max40080_state *st, int *val)
> +{
> +	u32 iv;
> +	int ret;
> +
> +	ret = max40080_read_iv(st, &iv);
> +	if (ret)
> +		return ret;
> +
> +	*val = FIELD_GET(MAX40080_IV_V_MAG_MSK, iv);
> +
> +	return 0;
> +}
> +
> +static int max40080_get_range(struct max40080_state *st, unsigned int *range)
> +{
> +	int tmp;
> +
> +	tmp = i2c_smbus_read_word_data(st->client, MAX40080_REG_CFG);
I think tmp can be initialized, since it is only assigned once.
> +	if (tmp < 0)
> +		return tmp;
> +
> +	*range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
> +
> +	return 0;
> +}
> +
...
> +
> +/*
> + * 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)
> +{
> +	int tmp;
> +	u8 filter;
Reverse xmas tree.
+ u8 filter;
+ int tmp;
While you are at it, and since you are already using u8, tmp can be s32.
> +
> +	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));
> +
> +	return 0;
> +}
> +
> +/*
> + * max40080_oversampling_avail[] is ordered so that its index is the FILTER
> + * field value (index 0 = no averaging, index 1 = 8x, ...). Return that index
> + * for an exact match, or -EINVAL for a value that is not on the list.
> + */
> +static int max40080_oversampling_to_filter(int val)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(max40080_oversampling_avail); i++)
> +		if (max40080_oversampling_avail[i] == val)
> +			return i;
Since this for is multiline, it could use scope, {}.
> +
> +	return -EINVAL;
> +}
> +
...
> +}
> +
> +static int max40080_read_avail(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       const int **vals, int *type, int *length,
> +			       long info)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type != IIO_CURRENT)
> +			return -EINVAL;
> +
> +		*vals = (int *)st->current_scale;
> +		*length = MAX40080_NUM_RANGES * 2;
> +		*type = IIO_VAL_INT_PLUS_NANO;
I think a space between the assignments and the return would read better. Personal preference. Your call.
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = max40080_oversampling_avail;
> +		*length = ARRAY_SIZE(max40080_oversampling_avail);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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) {
> +		int val = i2c_smbus_read_word_data(st->client, reg);
> +
> +		if (val < 0)
> +			return val;
> +		*read_val = val;
Here too.
> +		return 0;
> +	}
> +
> +	return i2c_smbus_write_word_data(st->client, reg, write_val);
> +}
...
> 
> +}
> +
> +static const struct i2c_device_id max40080_i2c_ids[] = {
> +	{ "max40080" },
.name = "max40080"
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max40080_i2c_ids);
> +
> +static const struct of_device_id max40080_of_match[] = {
> +	{ .compatible = "maxim,max40080" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max40080_of_match);
> +
> +static struct i2c_driver max40080_driver = {
> +	.driver = {
> +		.name = "max40080",
> +		.of_match_table = max40080_of_match,
> +	},
> +	.probe = max40080_probe,
> +	.id_table = max40080_i2c_ids,
> +};
> +module_i2c_driver(max40080_driver);
> +
> +MODULE_AUTHOR("Ciprian Hegbeli <ciprian.hegbeli@xxxxxxxxxx>");
> +MODULE_AUTHOR("Stefan Popa <stefan.popa@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Analog Devices MAX40080 current-sense amplifier driver");
> +MODULE_LICENSE("GPL");