Re: [PATCH] iio: adc: stm32-dfsdm: improve sampling frequency accuracy

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 25 Mar 2019 15:24:01 +0100
Fabrice Gasnier <fabrice.gasnier@st.com> wrote:

> The sample frequency is driven using the oversampling ratio depending
> on the SPI bus frequency.
> Currently, oversampling ratio is computed by an entire division:
> - spi_freq / sample_freq. This may result in inaccurate value.
> Using DIV_ROUND_CLOSEST improves resulting sample frequency, which is
> useful for audio that requests fixed rates (such as: 8, 16 or 32 kHz).
> BTW, introduce new routine to re-factor sample frequency setting, and
> move frequency accuracy message from warning to debug level.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>

Would have preferred this as two patches as it would have made review easier
by making it clear what the functional change is.

Still that is mostly me just wanting an easy life ;)

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/stm32-dfsdm-adc.c | 56 +++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 531ca7e..051561c 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -558,13 +558,38 @@ static ssize_t dfsdm_adc_audio_get_spiclk(struct iio_dev *indio_dev,
>  	return snprintf(buf, PAGE_SIZE, "%d\n", adc->spi_freq);
>  }
>  
> +static int dfsdm_adc_set_samp_freq(struct iio_dev *indio_dev,
> +				   unsigned int sample_freq,
> +				   unsigned int spi_freq)
> +{
> +	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
> +	unsigned int oversamp;
> +	int ret;
> +
> +	oversamp = DIV_ROUND_CLOSEST(spi_freq, sample_freq);
> +	if (spi_freq % sample_freq)
> +		dev_dbg(&indio_dev->dev,
> +			"Rate not accurate. requested (%u), actual (%u)\n",
> +			sample_freq, spi_freq / oversamp);
> +
> +	ret = stm32_dfsdm_set_osrs(fl, 0, oversamp);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "No filter parameters that match!\n");
> +		return ret;
> +	}
> +	adc->sample_freq = spi_freq / oversamp;
> +	adc->oversamp = oversamp;
> +
> +	return 0;
> +}
> +
>  static ssize_t dfsdm_adc_audio_set_spiclk(struct iio_dev *indio_dev,
>  					  uintptr_t priv,
>  					  const struct iio_chan_spec *chan,
>  					  const char *buf, size_t len)
>  {
>  	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> -	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
>  	struct stm32_dfsdm_channel *ch = &adc->dfsdm->ch_list[chan->channel];
>  	unsigned int sample_freq = adc->sample_freq;
>  	unsigned int spi_freq;
> @@ -583,17 +608,9 @@ static ssize_t dfsdm_adc_audio_set_spiclk(struct iio_dev *indio_dev,
>  		return -EINVAL;
>  
>  	if (sample_freq) {
> -		if (spi_freq % sample_freq)
> -			dev_warn(&indio_dev->dev,
> -				 "Sampling rate not accurate (%d)\n",
> -				 spi_freq / (spi_freq / sample_freq));
> -
> -		ret = stm32_dfsdm_set_osrs(fl, 0, (spi_freq / sample_freq));
> -		if (ret < 0) {
> -			dev_err(&indio_dev->dev,
> -				"No filter parameters that match!\n");
> +		ret = dfsdm_adc_set_samp_freq(indio_dev, sample_freq, spi_freq);
> +		if (ret < 0)
>  			return ret;
> -		}
>  	}
>  	adc->spi_freq = spi_freq;
>  
> @@ -1068,22 +1085,9 @@ static int stm32_dfsdm_write_raw(struct iio_dev *indio_dev,
>  			spi_freq = adc->spi_freq;
>  		}
>  
> -		if (spi_freq % val)
> -			dev_warn(&indio_dev->dev,
> -				 "Sampling rate not accurate (%d)\n",
> -				 spi_freq / (spi_freq / val));
> -
> -		ret = stm32_dfsdm_set_osrs(fl, 0, (spi_freq / val));
> -		if (ret < 0) {
> -			dev_err(&indio_dev->dev,
> -				"Not able to find parameter that match!\n");
> -			iio_device_release_direct_mode(indio_dev);
> -			return ret;
> -		}
> -		adc->sample_freq = val;
> +		ret = dfsdm_adc_set_samp_freq(indio_dev, val, spi_freq);
>  		iio_device_release_direct_mode(indio_dev);
> -
> -		return 0;
> +		return ret;
>  	}
>  
>  	return -EINVAL;