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.
"
Does it still make sense to keep this comment here? But if kept, yeah, a different patch
...
