On Wed, 2025-03-12 at 19:38 +0100, Uwe Kleine-König wrote:
> Hello,
>
> here comes another fix for the ad7124: The getter function for the
> filter_low_pass_3db_frequency sysfs property used wrong factors to
> calculate the f_{3dB}.
>
> The first patch is a cleanup I implemented before I noticed the issue. I
> didn't switch their ordering because I was lazy. If I continue to
> discover issues in the ad7124 driver at that rate, swapping for this one
> fix doesn't really matter :-)
>
> Note the setter function is still broken. And it's worse enough that I
> don't know how to fix it at all. The relevant part of the function looks
> as follows:
>
> sinc4_3db_odr = DIV_ROUND_CLOSEST(freq * 1000, 230);
> sinc3_3db_odr = DIV_ROUND_CLOSEST(freq * 1000, 262);
>
> if (sinc4_3db_odr > sinc3_3db_odr) {
> new_filter = AD7124_FILTER_FILTER_SINC3;
> new_odr = sinc4_3db_odr;
> } else {
> new_filter = AD7124_FILTER_FILTER_SINC4;
> new_odr = sinc3_3db_odr;
> }
>
> The issues I'm aware of in this function are:
>
> - the sinc3 factor should be 0.272 not 0.262 (which is fixed for the
> getter in patch #2)
> - for freq > 1 the if condition is always true
> - In the nearly always taken if branch the filter is set to sinc3, but
> the frequency is set for sinc4. (And vice versa in the else branch.)
>
Ouch...
> Also it's unclear to me why sinc4_3db_odr > sinc3_3db_odr is the test to
> decide between the two branches. Maybe something like
>
> if (abs(sinc4_3db_odr - current_odr) < abs(sinc3_3db_odr - current_odr))
> use_sinc4()
> else
> use_sinc3()
>
> would make more sense.
>
I think the below is indeed the proper solution. I know that removing an attr like
tis breaks ABI but since this interface is fairly broken anyways maybe it makes it
more acceptable. Anyways, just a minor comment for the series. With it:
Reviewed-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> I intend to add a filter_type property to the driver next. When this is
> implemented setting the filter_low_pass_3db_frequency shouldn't be
> needed any more and we can either keep the function as is (and
> discourage its use) or just drop it.
>
> Best regards
> Uwe
>
> Uwe Kleine-König (2):
> iio: adc: ad7124: Make register naming consistent
> iio: adc: ad7124: Fix 3dB filter frequency reading
>
> drivers/iio/adc/ad7124.c | 176 +++++++++++++++++++--------------------
> 1 file changed, 85 insertions(+), 91 deletions(-)
>
>
> base-commit: 8dbeb413806f9f810d97d25284f585b201aa3bdc
