Re: [PATCH v4 6/6] iio: adc: ad7173: Add support for AD411x devices

[Ceclan, Dumitru]

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

...