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 (4 + AVG - 1))

where AVG depends on the filter type selected and 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.

Technically, introducing the avg parameter is not needed at this time
since we currently only support a single filter mode which always has an
AVG value of 1. But it helps to show where the factor comes from in the
datasheet and will be used in the future when supporting additional
filter types.

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

I also have a plan to resolve things if we do find we broke someone and
need to revert the change. We have a recent devicetree fix [1] for these
chips that would allow us to detect "new" users using the correct DT
bindings and "old" users using the buggy bindings. So we could use this
as a way to also enable the old buggy sampling frequency behavior only
for "old" users while allowing "new" users to get the correct behavior.

[1] https://lore.kernel.org/linux-iio/20250825-iio-adc-ad7124-proper-clock-support-v2-0-4dcff9db6b35@baylibre.com/
---
 drivers/iio/adc/ad7124.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 3fc24f5fffc8f200c8656cb97f9e7f80546f688b..d75ef4d5de233c2a548c732b36440d0d82c86f34 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -84,10 +84,11 @@
 #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_SINGLE_CYCLE	BIT(16)
+#define AD7124_FILTER_FS		GENMASK(10, 0)
 
 #define AD7124_MAX_CONFIGS	8
 #define AD7124_MAX_CHANNELS	16
@@ -250,9 +251,10 @@ static int ad7124_set_mode(struct ad_sigma_delta *sd,
 	return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
 }
 
-static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel, unsigned int odr)
+static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel,
+				   unsigned int odr, unsigned int avg)
 {
-	unsigned int fclk, odr_sel_bits;
+	unsigned int fclk, factor, odr_sel_bits;
 
 	fclk = clk_get_rate(st->mclk);
 	/*
@@ -261,8 +263,12 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
 	 * fCLK is the master clock frequency
 	 * FS[10:0] are the bits in the filter register
 	 * FS[10:0] can have a value from 1 to 2047
+	 * When multiple channels in the sequencer or the SINGLE_CYCLE bit is
+	 * or when certain filter modes are enabled, there is an extra factor
+	 * of (4 + AVG - 1) to allow for settling time.
 	 */
-	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * 32);
+	factor = 32 * (4 + avg - 1);
+	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor);
 	if (odr_sel_bits < 1)
 		odr_sel_bits = 1;
 	else if (odr_sel_bits > 2047)
@@ -272,7 +278,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;
 }
 
@@ -406,9 +413,12 @@ static int ad7124_write_config(struct ad7124_state *st, struct ad7124_channel_co
 		return ret;
 
 	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);
 }
 
@@ -678,7 +688,7 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 			break;
 		}
 
-		ad7124_set_channel_odr(st, chan->address, val);
+		ad7124_set_channel_odr(st, chan->address, val, 1);
 		break;
 	case IIO_CHAN_INFO_SCALE:
 		if (val != 0) {
@@ -1148,7 +1158,7 @@ static int ad7124_setup(struct ad7124_state *st)
 		 * regardless of the selected power mode. Round it up to 10 and
 		 * set all channels to this default value.
 		 */
-		ad7124_set_channel_odr(st, i, 10);
+		ad7124_set_channel_odr(st, i, 10, 1);
 	}
 
 	ad7124_disable_all(&st->sd);

---
base-commit: 91812d3843409c235f336f32f1c37ddc790f1e03
change-id: 20250828-iio-adc-ad7124-fix-samp-freq-for-multi-channel-8b22c48b8fc0

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

On Thu, 28 Aug 2025 11:42:28 -0500
David Lechner <dlechner@baylibre.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 (4 + AVG - 1))
> 
> where AVG depends on the filter type selected and 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.
> 
> Technically, introducing the avg parameter is not needed at this time
> since we currently only support a single filter mode which always has an
> AVG value of 1. But it helps to show where the factor comes from in the
> datasheet and will be used in the future when supporting additional
> filter types.
> 
> 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@baylibre.com>
> ---
> 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.
> 
> I also have a plan to resolve things if we do find we broke someone and
> need to revert the change. We have a recent devicetree fix [1] for these
> chips that would allow us to detect "new" users using the correct DT
> bindings and "old" users using the buggy bindings. So we could use this
> as a way to also enable the old buggy sampling frequency behavior only
> for "old" users while allowing "new" users to get the correct behavior.

I'm not convinced this is a good plan as it may avoid regressions on a
particular board, but they are going to get unexpected changes if say
they order a new board of same type that has a newer DT.

Anyway hopefully we won't need it!

> 
> [1] https://lore.kernel.org/linux-iio/20250825-iio-adc-ad7124-proper-clock-support-v2-0-4dcff9db6b35@baylibre.com/

Just one comment on the comment.

I'd like some more eyes on this though as whilst it seems reasonable
the whole different modes bit changing timings is not particularly obvious.

> ---
>  drivers/iio/adc/ad7124.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 3fc24f5fffc8f200c8656cb97f9e7f80546f688b..d75ef4d5de233c2a548c732b36440d0d82c86f34 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -84,10 +84,11 @@
>  #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_SINGLE_CYCLE	BIT(16)
> +#define AD7124_FILTER_FS		GENMASK(10, 0)
>  
>  #define AD7124_MAX_CONFIGS	8
>  #define AD7124_MAX_CHANNELS	16
> @@ -250,9 +251,10 @@ static int ad7124_set_mode(struct ad_sigma_delta *sd,
>  	return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
>  }
>  
> -static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel, unsigned int odr)
> +static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel,
> +				   unsigned int odr, unsigned int avg)
>  {
> -	unsigned int fclk, odr_sel_bits;
> +	unsigned int fclk, factor, odr_sel_bits;
>  
>  	fclk = clk_get_rate(st->mclk);
>  	/*
> @@ -261,8 +263,12 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
>  	 * fCLK is the master clock frequency
>  	 * FS[10:0] are the bits in the filter register
>  	 * FS[10:0] can have a value from 1 to 2047
> +	 * When multiple channels in the sequencer or the SINGLE_CYCLE bit is
This sentence doesn't read. Maybe something with a few more words like.

	 * When multiple channels are enabled in the sequencer, the
	 * SINGLE_CYCLE bit is set or when certain filter modes are enabled,...

> +	 * or when certain filter modes are enabled, there is an extra factor
> +	 * of (4 + AVG - 1) to allow for settling time.
>  	 */
> -	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * 32);
> +	factor = 32 * (4 + avg - 1);
> +	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor);

On 9/1/25 10:57 AM, Jonathan Cameron wrote:
> On Thu, 28 Aug 2025 11:42:28 -0500
> David Lechner <dlechner@baylibre.com> wrote:

...

>> @@ -261,8 +263,12 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
>>  	 * fCLK is the master clock frequency
>>  	 * FS[10:0] are the bits in the filter register
>>  	 * FS[10:0] can have a value from 1 to 2047
>> +	 * When multiple channels in the sequencer or the SINGLE_CYCLE bit is
> This sentence doesn't read. Maybe something with a few more words like.
> 
> 	 * When multiple channels are enabled in the sequencer, the
> 	 * SINGLE_CYCLE bit is set or when certain filter modes are enabled,...
> 
>> +	 * or when certain filter modes are enabled, there is an extra factor
>> +	 * of (4 + AVG - 1) to allow for settling time.

	 * When multiple channels are enabled in the sequencer, the SINGLE_CYCLE
	 * bit is set, or when a fast settling filter mode is enabled on any
	 * channel, there is an extra factor of (4 + AVG - 1) to allow for
	 * settling time. We ensure that at least one of these is always true so
	 * that we always use the same factor.

>>  	 */
>> -	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * 32);
>> +	factor = 32 * (4 + avg - 1);
>> +	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor);
> 
>