On Wed, 12 Mar 2025 19:38:36 +0100
Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx> 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 :-)
Hmm. Even so, please swap if not too ahrd.
>
> 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.)
>
> 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 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.
If it is currently broken I'm less fussed about dropping the ABI than I would be
for even correct but useless ABI. This falls into the category of unlikely
anyone will noticed as they should have noticed it wasn't working if
they were using it!
Jonathan
>
> 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
