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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 4 Nov 2026 09:33:15 +0100
Neil Armstrong <neil.armstrong@linaro.org> wrote:

> On 11/4/26 03:30, Jonathan Cameron wrote:
> > On Mon, 02 Nov 2026 15:49:06 +0100
> > Neil Armstrong <neil.armstrong@linaro.org> wrote:
> >   
> >> The MAX34417 is a specialized current and voltage monitor used to
> >> determine power consumption of portable systems. The driver support
> >> getting the channels voltage, average power, accumulated average
> >> power and energy over an I2C/SMBUS serial interface.
> >>
> >> The MAX34417 datasheet only specified on how to calculate the
> >> accumulated average power, but with the Sample Per Second rate
> >> we can easily get accumulated Energy in mJ.
> >>
> >> By default the IIO driver only supports single-shot Voltage
> >> and Power sensing, but the accumulation can be enabled by
> >> enabling the Energy IIO channel which then enables the
> >> Continuous Accumulation Mode.
> >>
> >> The counter is common to all channels, so when reading a single
> >> channel we will copy all accumulators to registers, so in order
> >> to permit reading individual channels we must accumulate the
> >> other channels in the driver when Continuous Accumulation Mode
> >> is enabled.
> >>
> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>  
> > 
> > The use of the regmap_noinc stuff to deal with the variable
> > size 'registers' is novel.  Might be worth just adding a little
> > commentary on that to somewhere. Perhaps the regmap_config as a comment.
> > 
> > Other than that, just a question about why we need the postfixes
> > for the labels. That seems superfluous.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> >   
> >> diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
> >> new file mode 100644
> >> index 000000000000..e679cf142141
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/max34417.c  
> >   
> >> +
> >> +static const struct regmap_config max34417_regmap_config = {
> >> +	.reg_bits	= 8,
> >> +	.val_bits	= 8,  
> > As noted above. This isn't really true.  You are just use noinc
> > reads to pretend it is.  Those are normally only used for
> > fifo registers so I think we should have a comment here talking
> > about that choice.  
> 
> Well, reg_bits is 8, but val_bits is 8 for the control & status registers,
> for the data registers the value bits is variable, so it's kind of true.
> 
> But I should add the readable_noinc_reg callback, yes it's not FIFOs,
> but both 3 data registers have different data width which should be
> read using the noinc to use i2c_smbus_read_i2c_block_data().
> 
> I can provide my custom register read/write copied from regmap-i2c if
> you find it better to avoid abusing regmap...

I'm fine with what you have here. Just after a comment to remind
me of how this works if I'm looking at it in a few years time!

> 
> > 
> > It is pretty neat so I'm not against it but it might rather
> > surprise anyone looking at the datasheet!
> > 
> > 
> >   
> >> +	.max_register	= MAX34417_BULK_VOLTAGE_READOUT_REG,
> >> +	.writeable_reg	= max34417_writeable_reg,
> >> +	.readable_reg	= max34417_readable_reg,
> >> +	.volatile_reg	= max34417_volatile_reg,
> >> +	/* No cache since the only non-volatile register is read-only */
> >> +};  
> >