iio: adc: ad4170-4: Correctly update filter_fs after filter type change

[PATCH] iio: adc: ad4170-4: Correctly update filter_fs after filter type change

Posted by Marcelo Schmitt 2 months, 2 weeks ago

Previously, the driver was directly using the filter type value to update
the filter frequency (filter_fs) configuration. That caused the driver to
switch to the lowest filter_fs configuration (highest sampling frequency)
on every update to the filter type. Correct the filter_fs colateral update
by clampling it to the range of supported values instead of mistakenly
using the filter type to update the filter_fs.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/linux-iio/c6e54942-5b42-484b-be53-9d4606fd25c4@sabinyo.mountain/
Suggested-by: Jonathan Cameron <jic23@kernel.org>
Fixes: 8ab7434734cd ("iio: adc: ad4170-4: Add digital filter and sample frequency config support")
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
Didn't find a bug report in https://bugzilla.kernel.org/ to link with a
Closes: tag so added a Link: tag instead.

 drivers/iio/adc/ad4170-4.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad4170-4.c b/drivers/iio/adc/ad4170-4.c
index 6cd84d6fb08b..de35cef85a6e 100644
--- a/drivers/iio/adc/ad4170-4.c
+++ b/drivers/iio/adc/ad4170-4.c
@@ -880,10 +880,12 @@ static int ad4170_set_filter_type(struct iio_dev *indio_dev,
 			return -EBUSY;
 
 		if (val == AD4170_SINC5_AVG || val == AD4170_SINC3)
-			setup->filter_fs = clamp(val, AD4170_SINC3_MIN_FS,
+			setup->filter_fs = clamp(setup->filter_fs,
+						 AD4170_SINC3_MIN_FS,
 						 AD4170_SINC3_MAX_FS);
 		else
-			setup->filter_fs = clamp(val, AD4170_SINC5_MIN_FS,
+			setup->filter_fs = clamp(setup->filter_fs,
+						 AD4170_SINC5_MIN_FS,
 						 AD4170_SINC5_MAX_FS);
 
 		setup->filter &= ~AD4170_FILTER_FILTER_TYPE_MSK;

base-commit: cd2731444ee4e35db76f4fb587f12d327eec5446
-- 
2.47.2

Re: [PATCH] iio: adc: ad4170-4: Correctly update filter_fs after filter type change

Posted by Markus Elfring 2 months, 2 weeks ago

…
> +++ b/drivers/iio/adc/ad4170-4.c
> @@ -880,10 +880,12 @@ static int ad4170_set_filter_type(struct iio_dev *indio_dev,
>  			return -EBUSY;
>  
>  		if (val == AD4170_SINC5_AVG || val == AD4170_SINC3)
> -			setup->filter_fs = clamp(val, AD4170_SINC3_MIN_FS,
> +			setup->filter_fs = clamp(setup->filter_fs,
> +						 AD4170_SINC3_MIN_FS,
>  						 AD4170_SINC3_MAX_FS);
>  		else
> -			setup->filter_fs = clamp(val, AD4170_SINC5_MIN_FS,
> +			setup->filter_fs = clamp(setup->filter_fs,
> +						 AD4170_SINC5_MIN_FS,
>  						 AD4170_SINC5_MAX_FS);
>  
>  		setup->filter &= ~AD4170_FILTER_FILTER_TYPE_MSK;

How do you think about to use the following code variant?

		setup->filter_fs = (val == AD4170_SINC5_AVG || val == AD4170_SINC3)
				   ? clamp(setup->filter_fs,
					   AD4170_SINC3_MIN_FS, AD4170_SINC3_MAX_FS)
				   : clamp(setup->filter_fs,
					   AD4170_SINC5_MIN_FS, AD4170_SINC5_MAX_FS);


Regards,
Markus

Re: [PATCH] iio: adc: ad4170-4: Correctly update filter_fs after filter type change

Posted by Marcelo Schmitt 2 months, 2 weeks ago

On 07/20, Markus Elfring wrote:
> …
> > +++ b/drivers/iio/adc/ad4170-4.c
> > @@ -880,10 +880,12 @@ static int ad4170_set_filter_type(struct iio_dev *indio_dev,
> >  			return -EBUSY;
> >  
> >  		if (val == AD4170_SINC5_AVG || val == AD4170_SINC3)
> > -			setup->filter_fs = clamp(val, AD4170_SINC3_MIN_FS,
> > +			setup->filter_fs = clamp(setup->filter_fs,
> > +						 AD4170_SINC3_MIN_FS,
> >  						 AD4170_SINC3_MAX_FS);
> >  		else
> > -			setup->filter_fs = clamp(val, AD4170_SINC5_MIN_FS,
> > +			setup->filter_fs = clamp(setup->filter_fs,
> > +						 AD4170_SINC5_MIN_FS,
> >  						 AD4170_SINC5_MAX_FS);
> >  
> >  		setup->filter &= ~AD4170_FILTER_FILTER_TYPE_MSK;
> 
> How do you think about to use the following code variant?
> 
> 		setup->filter_fs = (val == AD4170_SINC5_AVG || val == AD4170_SINC3)
> 				   ? clamp(setup->filter_fs,
> 					   AD4170_SINC3_MIN_FS, AD4170_SINC3_MAX_FS)
> 				   : clamp(setup->filter_fs,
> 					   AD4170_SINC5_MIN_FS, AD4170_SINC5_MAX_FS);
> 
Looks good to me.
I'll send v2 with that.

Thanks,
Marcelo