Re: [PATCH v3 3/3] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

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

On Wed, Oct 14, 2026 at 10:38:28AM +0200, Neil Armstrong wrote:
> On 10/13/26 16:53, Andy Shevchenko wrote:
> > On Tue, Oct 13, 2026 at 04:07:13PM +0200, Neil Armstrong wrote:

...

> > > +#define MAX34417_MIN_RSENSE		(MILLI)	/* 1 milliOhm */
> > 
> > 1 * MILLI
> > 
> > > +#define MAX34417_MAX_RSENSE		(100 * MILLI) /* 100 milliOhm */
> > > +#define MAX34417_DEFAULT_RSENSE		MAX34417_MIN_RSENSE
> > 
> > I would rather put the exact value here as well.
> 
> This would duplicate the value for no reason, right ?

Semantically they are different, default can be different at some point, no?

...

> > > +	/* Ignore byte0 */
> > > +	voltage = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]);
> > > +	voltage >>= 2;
> > 
> > The above comment is odd. Please, clarify, ideally with a reference to
> > the datasheet table / section / et cetera.
> 
> Odd ? I mean it's not, we skip byte 0 and

I don't see it. >> 2 is the same as / 4, how does this related to the byte0 skip?

> get unaligned big-endian 16bit at offset 1...

here I see what the comment is meant for. In such a case it's not odd, it's
useless. Reading the above code I don't care what offset is since it's a macro
defined constant with kinda good name already. The >> 2 is what I question here.

> > Also can be folded into a single expression:
> > 
> > 	voltage = get_unaligned_be16(&buf[MAX34417_VOLTAGE_READ_OFFSET]) >> 2;

...

> > This can be simplified, right?
> 
> I don't find this really simplified TBH, just reorganized

Triple times to have condition for label replaced with a single one sounds to
me like a simplification.

-- 
With Best Regards,
Andy Shevchenko