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

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

On Sat, Jul 04, 2026 at 01:42:39AM +0600, Siratul Islam wrote:
> On Fri, 2026-07-03 at 13:29 +0300, Stefan Popa wrote:

...

> > +#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.

I saw this but left uncommented as sometimes people use this style to
distinguish bit field definitions from the register offsets. When one space is
in use it might be not so easy to put the borders. I.o.w. I have no
strong opinion on this style, but if you think we should be all the same for
all IIO drivers here, I'm fine.

> > +#define MAX40080_REG_FIFO_CFG		0x0A
> Here too
> > +#define  MAX40080_STORE_IV_MSK		GENMASK(1, 0)

...and here...

> > +#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)

...and here.

> > +/* CFG.mode field */
> > +#define MAX40080_STDBY_MODE		0x00
> > +#define MAX40080_SINGLE_MODE		0x02	/* one conversion per Quick Command */

...

> This can fit in 1 line.
> static int max40080_update_bits(struct max40080_state *st, u8 reg, u16 mask, u16 val)


Here...

...

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

...and here the wrap is done on logical border, so I think it's fine and reads
well.

...

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

I don't get this comment. You mean switching to ret?

> > +	if (tmp < 0)
> > +		return tmp;
> > +
> > +	*range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
> > +
> > +	return 0;
> > +}

-- 
With Best Regards,
Andy Shevchenko