Re: [PATCH 5/6] iio: adc: ad7124: add filter support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 05 Sep 2025 13:12:00 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Add support to the ad7124 driver for selecting the filter type.
> 
> The filter type has an influence on the effective sampling frequency of
> each channel. For sinc3+pf{1,2,3,4}, the sampling frequency is fixed.
> For sinc{3,4} (without post filter), there is a factor of 3 or 4
> depending on the filter type. For the extra +sinc1, there is an extra
> averaging factor that depends on the power mode.
> 
> In order to select the closest sampling frequency for each filter type,
> we keep a copy of the requested sampling frequency. This way, if the
> user sets the sampling frequency first and then selects the filter type,
> the sampling frequency will still be as close as possible to the
> requested value.
> 
> Since we always either have the SINGLE_CYCLE bit set or have more than
> one channel enabled, the sampling frequency is always using the
> "zero-latency" calculation from the data sheet. This is only documented
> for the basic sinc{3,4} filters, so the other filter types had to be
> inferred and confirmed through testing.
> 
> Since the flat filter type list consists of multiple register fields,
> the struct ad7124_channel_config::filter_type field is changed to the
> enum ad7124_filter_type type to avoid nested switch statements in a
> lot of places.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>

One really trivial comment inline.  Not worth a v2 for just that.
However, this is complex enough code I'd like to keep this on list anyway
for a little longer to see if anyone else has review comments.

Jonathan



> -static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel,
> -				   unsigned int odr, unsigned int odr_micro)
> +static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel)
>  {
> +	struct ad7124_channel_config *cfg = &st->channels[channel].cfg;
>  	unsigned int fclk, factor, odr_sel_bits;
>  
>  	fclk = ad7124_get_fclk_hz(st);
> +	factor = ad7124_get_fs_factor(st, channel);
>  
>  	/*
>  	 * FS[10:0] = fCLK / (fADC x 32 * N) where:
> @@ -310,9 +362,9 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
>  	 * FS[10:0] are the bits in the filter register
>  	 * FS[10:0] can have a value from 1 to 2047
>  	 */
> -	factor = 32 * 4; /* N = 4 for default sinc4 filter. */
> -	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor +
> -					       odr_micro * factor / MICRO);
> +	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, cfg->requested_odr * factor +
> +					       cfg->requested_odr_micro *
> +					       factor / MICRO);

This second parameter is getting complex enough, perhaps worth introducing
a local variable?
	divisor = cfg->requested_odr * factor +
		  cfg->requested_odr_micro * factor / MICRO;
perhaps, or local variables for odr and odr_micro to keep this unchanged in this path.


>  	odr_sel_bits = clamp(odr_sel_bits, 1, 2047);
>  
>  	if (odr_sel_bits != st->channels[channel].cfg.odr_sel_bits)