Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm

[Olivier MOYSAN <olivier.moysan@foss.st.com>]

Hi Nuno,

On 6/19/24 07:47, Nuno Sá wrote:
> On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
>> Add scaling support to STM32 DFSDM.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
> 
> Just some general comments...
> 
> Acked-by: Nuno Sa <nuno.sa@analog.com>
> 
>>   drivers/iio/adc/Kconfig           |  1 +
>>   drivers/iio/adc/stm32-dfsdm-adc.c | 94 ++++++++++++++++++++++++++++++-
>>   2 files changed, 92 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index f3dfdaa80678..858ae8161fa4 100644
>>
> 
> ...
> 
>> @@ -1301,6 +1339,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>>   		ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
>>   		if (adc->hwc)
>>   			iio_hw_consumer_disable(adc->hwc);
>> +		if (adc->backend[idx])
>> +			iio_backend_disable(&indio_dev->dev, adc->backend[idx]);
>>   		if (ret < 0) {
>>   			dev_err(&indio_dev->dev,
>>   				"%s: Conversion failed (channel %d)\n",
>> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>>   		*val = adc->sample_freq;
>>   
>>   		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		/*
>> +		 * Scale is expressed in mV.
>> +		 * When fast mode is disabled, actual resolution may be lower
>> +		 * than 2^n, where n=realbits-1.
>> +		 * This leads to underestimating input voltage. To
>> +		 * compensate this deviation, the voltage reference can be
>> +		 * corrected with a factor = realbits resolution / actual max
>> +		 */
>> +		if (adc->backend[idx]) {
>> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
> 
> This is something that I've been thinking about since the beginning of backends
> support. Basically having the "catch all" read_raw()/write_raw() or more dedicated
> interfaces. For example, in your usecase I think it would make more sense to have
> dedicated API's like iio_backend_read_scale() and iio_backend_read_offset() as you're
> extending that functionality. Calling iio_backend_read_raw() seems a bit weird to me.
> 
> OTOH, iio_backend_read_raw() could be useful in cases where frontends call
> iio_backend_extend_chan_spec() and may get some functionality they are not aware of.
> And so, in the default: statement this "catch all" API could be used.
> 
> Anyways, this are really minor (likely pedantic) details and you kind of came first
> and made the decision for me. As I don't really have strong feelings about it, I'm
> fine with it as-is.
> 

My first idea was to stick to IIO read_raw API ('struct iio_chan_spec 
const *chan' should at least be added to prototype as you pointed it 
out). But I agree that iio_backend_read_raw() is not explicit regarding 
the requested info.
I consider using the existing API iio_backend_ext_info_get() for v2, as 
you suggested.

BRs
Olivier

>> +			*val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1),
>> max);
>> +			*val2 = chan->scan_type.realbits;
>> +			if (chan->differential)
>> +				*val *= 2;
>> +		}
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		/*
>> +		 * DFSDM output data are in the range [-2^n,2^n],
>> +		 * with n=realbits-1.
>> +		 * - Differential modulator:
>> +		 * Offset correspond to SD modulator offset.
>> +		 * - Single ended modulator:
>> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and
>> Vref to 2^n.
>> +		 * Add 2^n to offset. (i.e. middle of input range)
>> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
>> +		 */
>> +		if (adc->backend[idx]) {
>> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>> +			*val = div_u64((u64)max * *val, BIT(*val2 - 1));
>> +			if (!chan->differential)
>> +				*val += max;
>> +		}
>> +		return IIO_VAL_INT;
>>   	}
>>   
>>   	return -EINVAL;
>> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev
>> *indio_dev, struct iio_c
>>   	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
>>   	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>>   	 */
>> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	if (child) {
>> +		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +					BIT(IIO_CHAN_INFO_SCALE) |
>> +					BIT(IIO_CHAN_INFO_OFFSET);
> 
> Not sure if the above SCALE and OFFSET are solely properties if the backend. If so,
> you could maybe use iio_backend_extend_chan_spec().
> 
> - Nuno Sá
>