Re: [PATCH 0/2] iio: adc: ad7124: Fix 3dB filter frequency reading

["Nuno Sá" <noname.nuno@gmail.com>]

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

> 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