Message-ID: <20250907110013.266c2da2@jic23-huawei>
Date: Sun, 7 Sep 2025 11:00:13 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] iio: adc: ad7124: fix sample rate for multi-channel
 use

On Fri, 05 Sep 2025 12:33:34 -0500
David Lechner <dlechner@...libre.com> wrote:

> Change how the FS[10:0] field of the FILTER register is calculated to
> get consistent sample rates when only one channel is enabled vs when
> multiple channels are enabled in a buffered read.
> 
> By default, the AD7124 allows larger sampling frequencies when only one
> channel is enabled. It assumes that you will discard the first sample or
> so to allow for settling time and then no additional settling time is
> needed between samples because there is no multiplexing due to only one
> channel being enabled. The conversion formula to convert between the
> sampling frequency and the FS[10:0] field is:
> 
>     fADC = fCLK / (FS[10:0] x 32)
> 
> which is what the driver has been using.
> 
> On the other hand, when multiple channels are enabled, there is
> additional settling time needed when switching between channels so the
> calculation to convert between becomes:
> 
>     fADC = fCLK / (FS[10:0] x 32 x N)
> 
> where N depends on if SINGLE_CYCLE is set, the selected filter type and,
> in some cases, the power mode.
> 
> The FILTER register has a SINGLE_CYCLE bit that can be set to force the
> single channel case to use the same timing as the multi-channel case.
> 
> Before this change, the first formula was always used, so if all of the
> in_voltageY_sampling_frequency attributes were set to 10 Hz, then doing
> a buffered read with 1 channel enabled would result in the requested
> sampling frequency of 10 Hz. But when more than one channel was
> enabled, the actual sampling frequency would be 2.5 Hz per channel,
> which is 1/4 of the requested frequency.
> 
> After this change, the SINGLE_CYCLE flag is now always enabled and the
> multi-channel formula is now always used. This causes the sampling
> frequency to be consistent regardless of the number of channels enabled.
> 
> For now, we are hard-coding N = 4 since the driver doesn't yet support
> other filter types other than the default sinc4 filter.
> 
> The AD7124_FILTER_FS define is moved while we are touching this to
> keep the bit fields in descending order to be consistent with the rest
> of the file.
> 
> Signed-off-by: David Lechner <dlechner@...libre.com>

Given you replied to Andy's indentation comment on v2 and that seemed
reasonable to me + the other change is a simplification for now I think
this is ready to go.

Given the whole ABI / fixes tag point I'm going to apply it to the
'slow' path and taking via testing/togreg for the next merge windows.

Applied to togreg and pushed out as testing for 0-day to take a look.

Thank,

Jonathan

> ---
> This is one of those unfortunate cases where we are fixing a bug but we
> risk breaking userspace that may be depending on the buggy behavior.
> 
> I intentionally didn't include a Fixes: tag for this reason.
> 
> Given some of the other shortcomings of this driver, like using an
> integer IIO_CHAN_INFO_SAMP_FREQ value when it really needs to allow a
> fractional values, it makes me hopeful that no one is caring too much
> about the exact value of the sampling frequency. So I'm more willing
> than I would normally be to take a risk on making this change.
> 
> [1] https://lore.kernel.org/linux-iio/20250825-iio-adc-ad7124-proper-clock-support-v2-0-4dcff9db6b35@baylibre.com/
> ---
> Changes in v3:
> - Removed the ad7124_get_avg() and harded-coded N = 4 for now.
> - Link to v2: https://lore.kernel.org/r/20250904-iio-adc-ad7124-fix-samp-freq-for-multi-channel-v2-1-bbf2f0d997ea@baylibre.com
> 
> Changes in v2:
> - Improved comment explaining why the new factor always applies.
> - Fixed merge conflict with iio/testing branch.
> - Replaced avg parameter with ad7124_get_avg() function.
> - Link to v1: https://lore.kernel.org/r/20250828-iio-adc-ad7124-fix-samp-freq-for-multi-channel-v1-1-f8d4c920a699@baylibre.com
> ---
>  drivers/iio/adc/ad7124.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index c6435327d431e5f4ba28aa3332ec6eb90da7c83d..70f458e3ccc12db884dda9003abcffdf48989e5e 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -93,10 +93,13 @@
>  #define AD7124_CONFIG_PGA		GENMASK(2, 0)
>  
>  /* AD7124_FILTER_X */
> -#define AD7124_FILTER_FS		GENMASK(10, 0)
>  #define AD7124_FILTER_FILTER		GENMASK(23, 21)
>  #define AD7124_FILTER_FILTER_SINC4		0
>  #define AD7124_FILTER_FILTER_SINC3		2
> +#define AD7124_FILTER_FILTER_SINC4_SINC1	4
> +#define AD7124_FILTER_FILTER_SINC3_SINC1	5
> +#define AD7124_FILTER_SINGLE_CYCLE	BIT(16)
> +#define AD7124_FILTER_FS		GENMASK(10, 0)
>  
>  #define AD7124_MAX_CONFIGS	8
>  #define AD7124_MAX_CHANNELS	16
> @@ -285,18 +288,20 @@ static u32 ad7124_get_fclk_hz(struct ad7124_state *st)
>  
>  static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel, unsigned int odr)
>  {
> -	unsigned int fclk, odr_sel_bits;
> +	unsigned int fclk, factor, odr_sel_bits;
>  
>  	fclk = ad7124_get_fclk_hz(st);
>  
>  	/*
> -	 * FS[10:0] = fCLK / (fADC x 32) where:
> +	 * FS[10:0] = fCLK / (fADC x 32 * N) where:
>  	 * fADC is the output data rate
>  	 * fCLK is the master clock frequency
> +	 * N is number of conversions per sample (depends of filter type)
>  	 * FS[10:0] are the bits in the filter register
>  	 * FS[10:0] can have a value from 1 to 2047
>  	 */
> -	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * 32);
> +	factor = 32 * 4; /* N = 4 for default sinc4 filter. */
> +	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor);
>  	if (odr_sel_bits < 1)
>  		odr_sel_bits = 1;
>  	else if (odr_sel_bits > 2047)
> @@ -306,7 +311,8 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
>  		st->channels[channel].cfg.live = false;
>  
>  	/* fADC = fCLK / (FS[10:0] x 32) */
> -	st->channels[channel].cfg.odr = DIV_ROUND_CLOSEST(fclk, odr_sel_bits * 32);
> +	st->channels[channel].cfg.odr = DIV_ROUND_CLOSEST(fclk, odr_sel_bits *
> +								factor);
>  	st->channels[channel].cfg.odr_sel_bits = odr_sel_bits;
>  }
>  
> @@ -439,10 +445,20 @@ static int ad7124_write_config(struct ad7124_state *st, struct ad7124_channel_co
>  	if (ret < 0)
>  		return ret;
>  
> +	/*
> +	 * NB: AD7124_FILTER_SINGLE_CYCLE is always set so that we get the same
> +	 * sampling frequency even when only one channel is enabled in a
> +	 * buffered read. If it was not set, the N in ad7124_set_channel_odr()
> +	 * would be 1 and we would get a faster sampling frequency than what
> +	 * was requested.
> +	 */
>  	tmp = FIELD_PREP(AD7124_FILTER_FILTER, cfg->filter_type) |
> +		AD7124_FILTER_SINGLE_CYCLE |
>  		FIELD_PREP(AD7124_FILTER_FS, cfg->odr_sel_bits);
>  	return ad7124_spi_write_mask(st, AD7124_FILTER(cfg->cfg_slot),
> -				     AD7124_FILTER_FILTER | AD7124_FILTER_FS,
> +				     AD7124_FILTER_FILTER |
> +				     AD7124_FILTER_SINGLE_CYCLE |
> +				     AD7124_FILTER_FS,
>  				     tmp, 3);
>  }
>  
> 
> ---
> base-commit: d1487b0b78720b86ec2a2ac7acc683ec90627e5b
> change-id: 20250828-iio-adc-ad7124-fix-samp-freq-for-multi-channel-8b22c48b8fc0
> 
> Best regards,

Powered by blists - more mailing lists