| Message ID | 20250317115247.3735016-7-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: adc: ad7124: Fix 3dB filter frequency reading | expand |
On 03/17, Uwe Kleine-König wrote: > There are several issues with the function that implements writing to > the filter_low_pass_3db_frequency property: > > - The sinc3 factor should be 0.272 not 0.262 (this is fixed for the > reading side in the previous patch). > - For freq > 1 the if condition is always true so the sinc4 filter is > hardly ever chosen. > - 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.) > > This is broken enough to justify the claim that there isn't any serious > user. Also it it counter-intuitive that setting the 3db frequency > modifies the sample frequency and the filter type. Not from engineering background but, as a Linux developer, I agree changing 3dB frequency to set the ODR (sampling frequency) is counter-intuitive and uncommon among other IIO sigma-delta ADC drivers. > > So drop the ability to write that property. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
On Wed, 26 Mar 2025 15:35:04 -0300 Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > On 03/17, Uwe Kleine-König wrote: > > There are several issues with the function that implements writing to > > the filter_low_pass_3db_frequency property: > > > > - The sinc3 factor should be 0.272 not 0.262 (this is fixed for the > > reading side in the previous patch). > > - For freq > 1 the if condition is always true so the sinc4 filter is > > hardly ever chosen. > > - 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.) > > > > This is broken enough to justify the claim that there isn't any serious > > user. Also it it counter-intuitive that setting the 3db frequency > > modifies the sample frequency and the filter type. > > Not from engineering background but, as a Linux developer, I agree changing 3dB > frequency to set the ODR (sampling frequency) is counter-intuitive and uncommon > among other IIO sigma-delta ADC drivers. First, ABI wise, any parameter is allowed to change any other - so we have no firm rules on this. Changing the filter type isn't completely silly as a side effect of changing 3db point as when we are selecting between fixed filters that may be precisely what the user is trying to do. Changing the sampling frequency is harder to argue. Given it's horribly broken anyway I guess no one ever touched it and we are fine to clean things up. Jonathan > > > > > So drop the ability to write that property. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > > Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c index 7d5d84a07cae..662a3eb2f90e 100644 --- a/drivers/iio/adc/ad7124.c +++ b/drivers/iio/adc/ad7124.c @@ -309,32 +309,6 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st, } } -static void ad7124_set_3db_filter_freq(struct ad7124_state *st, unsigned int channel, - unsigned int freq) -{ - unsigned int sinc4_3db_odr; - unsigned int sinc3_3db_odr; - unsigned int new_filter; - unsigned int new_odr; - - 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_SINC3_FILTER; - new_odr = sinc4_3db_odr; - } else { - new_filter = AD7124_SINC4_FILTER; - new_odr = sinc3_3db_odr; - } - - if (new_odr != st->channels[channel].cfg.odr) - st->channels[channel].cfg.live = false; - - st->channels[channel].cfg.filter_type = new_filter; - st->channels[channel].cfg.odr = new_odr; -} - static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_state *st, struct ad7124_channel_config *cfg) { @@ -739,16 +713,8 @@ static int ad7124_write_raw(struct iio_dev *indio_dev, st->channels[chan->address].cfg.pga_bits = res; break; - case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: - if (val2 != 0) { - ret = -EINVAL; - break; - } - - ad7124_set_3db_filter_freq(st, chan->address, val); - break; default: - ret = -EINVAL; + ret = -EINVAL; } mutex_unlock(&st->cfgs_lock);
There are several issues with the function that implements writing to the filter_low_pass_3db_frequency property: - The sinc3 factor should be 0.272 not 0.262 (this is fixed for the reading side in the previous patch). - For freq > 1 the if condition is always true so the sinc4 filter is hardly ever chosen. - 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.) This is broken enough to justify the claim that there isn't any serious user. Also it it counter-intuitive that setting the 3db frequency modifies the sample frequency and the filter type. So drop the ability to write that property. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/iio/adc/ad7124.c | 36 +----------------------------------- 1 file changed, 1 insertion(+), 35 deletions(-)