On Mon, 3 Jun 2024 13:11:33 +0300
"Ceclan, Dumitru" <mitrutzceclan@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
> >
> >> From: Dumitru Ceclan <dumitru.ceclan@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxx
> 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
>
> ...
