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

[Siratul Islam <siratul.islam@linux.dev>]

On 26/07/04 03:05PM, Andy Shevchenko wrote:
> 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.
I looked at existing drivers and most of them had one space so I thought
it would be better to keep them consistent. But apparently, as David
also noted, this style is acceptable. So no problem for me.
> 
> 
...
> > > +/* 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...
Keeping this function single line results in 86 cols, which is just a
little more than 80. So I thought splitting here was avoidable. But if
you think it's an acceptable split then so be it.
> 
> ...
> 
> > > +	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.
This one might be pushing too far so I had my doubts.
> 
> ...
> 
> > > +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?
> 
I meant tmp is assigned only once so the indirection here, i.e.
declaring and assigning in two steps doesn't buy us anything. Instead,
it could be initialized like "int tmp = i2c_smbus_read_word_data()."
> > > +	if (tmp < 0)
> > > +		return tmp;
> > > +
> > > +	*range = FIELD_GET(MAX40080_RANGE_MSK, tmp);
> > > +
> > > +	return 0;
> > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>