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

[Jonathan Cameron <jic23@kernel.org>]

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.
> 
> 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.

>  
>  	/* 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.
		

> +	}

> @@ -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.

> +		 * - 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.

> +		 * 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?

> +	} 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);