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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 12 Sep 2025 09:27:47 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 9/11/25 11:49 PM, Andy Shevchenko wrote:
> > On Fri, Sep 12, 2025 at 12:43 AM 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.  
> > 
> > ...
> >   
> >> -       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 = clamp(odr_sel_bits, 1, 2047);
> >> +       divisor = cfg->requested_odr * factor +
> >> +                 cfg->requested_odr_micro * factor / MICRO;
> >> +       odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, divisor), 1, 2047);  
> > 
> > I have a déjà vu feeling here. Is this similar code to elsewhere?  Can
> > it be factored out to a helper?
> > 
> >   
> 
> It is changing the same code from a previous commit, not duplicating
> it. I guess I could have introduced the divisor variable in the
> earlier commit and saved some churn.
For this and the previous patch, to me it feels like we are letting
aiming for perfect patch break up be the enemy of a good result.
So I've applied them both but as I don't know if Andy will agree
not his RB.

Jonathan