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

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

Hi Jonathan,

On 6/23/24 17:21, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 18:08:34 +0200
> Olivier Moysan <olivier.moysan@foss.st.com> wrote:
> 
>> Add scaling support to STM32 DFSDM.
> Perhaps a short description here of how this works?  Where does the scale come
> from, what assumptions are made etc.
>>

Sure. Requires more explanations. Done

>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> 
> Some minor stuff.
> 
>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
>> index 69b4764d7cba..93bf6035bd6d 100644
>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> urn 0;
>>   }
>>   
>> @@ -1060,7 +1072,7 @@ static int stm32_dfsdm_update_scan_mode(struct iio_dev *indio_dev,
>>   static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>>   {
>>   	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> -	int ret;
>> +	int i = 0, ret;
> 
> Don't mix assigned and unassigned variable declarations. Just use a separate line
> as this can mean subtle assignment or lack of assignment issues sneak in.
> 

Ack

>>   
>>   	/* Reset adc buffer index */
>>   	adc->bufi = 0;
>> @@ -1071,6 +1083,15 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>>   			return ret;
>>   	}
>>   
>> +	if (adc->backend) {
>> +		while (adc->backend[i]) {
> 
> Could do similar to the suggestion below.
> Mostly I don't like the index variable manipulation.
> 
>> +			ret = iio_backend_enable(&indio_dev->dev, adc->backend[i]);
>> +			if (ret < 0)
>> +				return ret;
>> +			i++;
>> +		}
>> +	}
>> +
>>   	ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
>>   	if (ret < 0)
>>   		goto err_stop_hwc;
>> @@ -1103,6 +1124,7 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>>   static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>>   {
>>   	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> +	int i = 0;
>>   
>>   	stm32_dfsdm_stop_conv(indio_dev);
>>   
>> @@ -1110,6 +1132,13 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>>   
>>   	stm32_dfsdm_stop_dfsdm(adc->dfsdm);
>>   
>> +	if (adc->backend) {
>> +		while (adc->backend[i]) {
>> +			iio_backend_disable(&indio_dev->dev, adc->backend[i]);
>> +			i++;
>> +		}
> Something like
> 		struct iio_backend **be = &adc->backend[0];
> 		do {
> 			iio_backend_disable(&indio-dev->dev, be);
> 		} while (be++);
> 
> maybe. Up to you.
> 

If you don't mind, I will keep indexes. Pointers here looks more 
difficult to read, and pre/post incrementation in the loop condition can 
easily introduce bugs, imho.
		
> 
>> +	}
> 
>> @@ -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.
> 
> As below, use a few more spaces.
> 
>> +		 * 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);
>> +
>> +			*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],
> Use a few more spaces. [-2^2, 2^n]
>> +		 * with n=realbits-1.
> n = realbits - 1
> 
> Just to keep it closer to the C coding style.
> 

Done

>> +		 * - 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.
> 
> Avoid that long line with a suitable line break.
> 

Ack

>> +		 * 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);
> 
> Indent looks a little odd. Maybe one more space neede?
> 

Ack

>> +	} else {
>> +		/* Legacy. Scaling not supported */
>> +		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	}
>> +
>>   	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>>   					BIT(IIO_CHAN_INFO_SAMP_FREQ);
>>   
>> @@ -1816,3 +1903,4 @@ module_platform_driver(stm32_dfsdm_adc_driver);
>>   MODULE_DESCRIPTION("STM32 sigma delta ADC");
>>   MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
>>   MODULE_LICENSE("GPL v2");
>> +MODULE_IMPORT_NS(IIO_BACKEND);
> 

BRs
Olivier