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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 25 Sep 2026 10:46:33 +0200
Neil Armstrong <neil.armstrong@linaro.org> wrote:

> Hi,
> 
> On 9/25/26 05:26, Jonathan Cameron wrote:
> > On Thu, 24 Sep 2026 15:14:16 +0200
> > 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 and accumulated average power over an
> >> I2C/SMBUS serial interface.
> >>
> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>  
> > 
> > A few comments inline.  For a new driver I'd wait a week before sending an
> > update. Whilst you've gotten quite a few reviews already it is good to make
> > sure any discussion has died down before moving on to the next version.
> > 
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> >> diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
> >> new file mode 100644
> >> index 000000000000..98d961c5ecee
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/max34417.c
> >> @@ -0,0 +1,374 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * IIO driver for Maxim MAX34417 ADC, 4-Channels High Dynamic Range Power Accumulator
> >> + *
> >> + * Datasheet: https://www.analog.com/en/products/max34417.html
> >> + *
> >> + * TODO: Slow Mode, Continuous Accumulate Mode, Park Feature, Bulk Update, Perr_Verr Correction
> >> + */  
> >   
> >> +#define MAX34417_CHANNEL(_index, _v_address, _power_address)	\
> >> +	{							\
> >> +		.type = IIO_VOLTAGE,				\
> >> +		.info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) | \
> >> +					  BIT(IIO_CHAN_INFO_SCALE), \
> >> +		.channel = (_index),				\
> >> +		.address = (_v_address),			\
> >> +		.indexed = 1,					\
> >> +	},							\
> >> +	{							\
> >> +		.type = IIO_POWER,				\  
> > 
> > As below. This smells like it might not be an actual power channel if it
> > is accumulated on a fixed frequency.  It'll be some sort of scaled IIO_ENERGY
> > channel.  If you want to present it as power (which may make sense) then
> > it may need a little maths.  
> 
> So as I understand the ENERGY would need to provide Joules. Which would
> be doable if we take in account the accumulator sample rate (1024sps) from the
> datasheet.
> 
> But this implementation tries to provide an initial support following
> the MAX34417 datasheet which provides calculation for Average Power (page 18),
> this is why I sticked to POWER and IIO_CHAN_INFO_AVERAGE_RAW.

Doing division in driver is a bit nasty but given there are a lot of bits
I suppose that's not too bad.  I'd completely failed to notice the division 
on my first read through :(

So power is fine.

Thanks,

Jonathan

> 
> But you're right, knowing the sample rate we could indeed calculate the energy.
> 
> I can try to do the math, but with manual updates it may no be very accurate so
> the Continuous Accumulate Mode should be implemented to provide
> accurate Energy measurements over time and would be enabled via an
> IIO_CHAN_INFO_ENABLE.