Message-ID: <20240603210738.3880f1cc@jic23-huawei>
Date: Mon, 3 Jun 2024 21:07:38 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: "Ceclan, Dumitru" <mitrutzceclan@...il.com>
Cc: Dumitru Ceclan via B4 Relay
 <devnull+dumitru.ceclan.analog.com@...nel.org>, dumitru.ceclan@...log.com,
 Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
 <conor+dt@...nel.org>, David Lechner <dlechner@...libre.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 6/6] iio: adc: ad7173: Add support for AD411x devices

On Mon, 3 Jun 2024 13:11:33 +0300
"Ceclan, Dumitru" <mitrutzceclan@...il.com> wrote:

> On 01/06/2024 22:19, Jonathan Cameron wrote:
> > On Fri, 31 May 2024 22:42:32 +0300
> > Dumitru Ceclan via B4 Relay <devnull+dumitru.ceclan.analog.com@...nel.org> wrote:
> >   
> >> From: Dumitru Ceclan <dumitru.ceclan@...log.com>
> >>
> >> Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
> >>
> >> The AD411X family encompasses a series of low power, low noise, 24-bit,
> >> sigma-delta analog-to-digital converters that offer a versatile range of
> >> specifications.
> >>
> >> This family of ADCs integrates an analog front end suitable for processing
> >> both fully differential and single-ended, bipolar voltage inputs
> >> addressing a wide array of industrial and instrumentation requirements.
> >>
> >> - All ADCs have inputs with a precision voltage divider with a division
> >>   ratio of 10.
> >> - AD4116 has 5 low level inputs without a voltage divider.
> >> - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
> >>   shunt resistor.
> >>
> >> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@...log.com>  
> > Hi Dumitru,
> > 
> > A follow on comment on the validation code.
> > Also there is some good docs for the sampling frequency but are they
> > actually related to the rest of this change?  They also raise
> > questions about ABI compliance that we may want to deal with as
> > a follow up patch.
> > 
> > A few other trivial things inline.
> > 
> > This is looking pretty good, so hopefully we'll get the last few corners
> > sorted in v5.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> >   
> >> ---
> >>  drivers/iio/adc/ad7173.c | 336 +++++++++++++++++++++++++++++++++++++++++++----
> >>  1 file changed, 307 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> >> index ed8ff8c5f343..91ff984eedf4 100644
> >> --- a/drivers/iio/adc/ad7173.c
> >> +++ b/drivers/iio/adc/ad7173.c
> >> @@ -1,8 +1,9 @@  
> >   
> >>  #define AD7173_INTERFACE_DATA_STAT	BIT(6)
> >> @@ -125,26 +132,46 @@
> >>  #define AD7173_VOLTAGE_INT_REF_uV	2500000
> >>  #define AD7173_TEMP_SENSIIVITY_uV_per_C	477
> >>  #define AD7177_ODR_START_VALUE		0x07
> >> +#define AD4111_SHUNT_RESISTOR_OHM	50
> >> +#define AD4111_DIVIDER_RATIO		10
> >> +#define AD411X_VCOM_INPUT		0X10  
> > 
> > AD4111_VCOM_INPUT . Looks like one wildcard escaped an earlier edit?
> >   
> >> +#define AD4111_CURRENT_CHAN_CUTOFF	16
> >>  
> >> @@ -736,6 +918,21 @@ static int ad7173_write_raw(struct iio_dev *indio_dev,
> >>  		return ret;
> >>  
> >>  	switch (info) {
> >> +	/*
> >> +	 * This attribute sets the sampling frequency to each channel individually.  
> > 
> > frequency for each channel?
> >   
> >> +	 * There are no issues for raw or buffered reads of an individual channel.
> >> +	 *
> >> +	 * When multiple channels are enabled in buffered mode, the effective
> >> +	 * sampling rate of a channel is lowered in correlation to the number
> >> +	 * of channels enabled and the sampling rate of the other channels.
> >> +	 *
> >> +	 * Example: 3 channels enabled with rates CH1:6211sps CH2,CH3:10sps
> >> +	 * While the reading of CH1 takes only 0.16ms, the reading of CH2 and CH3
> >> +	 * will take 100ms each.
> >> +	 *
> >> +	 * This will cause the reading of CH1 to be actually done once every
> >> +	 * 200.16ms, an effective rate of 4.99sps.  
> > 
> > Hmm. This is a bit unfortunate as if I understand correctly that's not really what
> > people will expect when they configure the sampling frequency.  However I can't immediately
> > think of a better solution.  You could let userspace write a value that is cached
> > then attempt to get as near as possible as channels are enabled.
> > 
> > Still this looks like a documentation enhancement of existing behavior
> > in which case any functional change can be in a future patch.
> > However I don't think the docs update belongs in this patch unless
> > I'm missing some reason for it?
> >  
> 
> Well, it would seem like this exact behaviour is already documented:
> 
>  "
>  What:		/sys/bus/iio/devices/iio:deviceX/in_voltageX_sampling_frequency
>  What:		/sys/bus/iio/devices/iio:deviceX/in_powerY_sampling_frequency
>  What:		/sys/bus/iio/devices/iio:deviceX/in_currentZ_sampling_frequency
>  KernelVersion:	5.20
>  Contact:	linux-iio@...r.kernel.org
>  Description:
> 		Some devices have separate controls of sampling frequency for
> 		individual channels. If multiple channels are enabled in a scan,
> 		then the sampling_frequency of the scan may be computed from the
> 		per channel sampling frequencies.
>  "

I guess we've hit this before and added very specific documentation around this
that I'd forgotten about.  Thanks for pointing it out!

> Does it still make sense to keep this comment here? But if kept, yeah, a different patch
I think there is no harm in having the additional info.  Not as if everyone will
check the ABI docs whilst trying to understand the driver code. I didn't for one :(

Jonathan

> 
> ...

Powered by blists - more mailing lists