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

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

[Marcelo Schmitt]

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 collateral update
by clamping 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>
---
Hi, this is the same fix as the fix provided in implied v1 patch but with the
difference of doing it the way Markus suggested in reply to v1.
I have a slight preference for v1 since that one keeps code contained within
80 columns. Though, totally fine with whatever version of the fix gets picked up.

Change log v1 (implied) -> v2
- Replaces if by use of ternary operator as suggested by Markus in reply to v1.
- Fixed commit log typo: colateral -> collateral
- Fixed commit log typo: clampling -> clamping

Thanks,
Marcelo

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

diff --git a/drivers/iio/adc/ad4170-4.c b/drivers/iio/adc/ad4170-4.c
index 6cd84d6fb08b..2e61f9a9a1ef 100644
--- a/drivers/iio/adc/ad4170-4.c
+++ b/drivers/iio/adc/ad4170-4.c
@@ -879,12 +879,11 @@ static int ad4170_set_filter_type(struct iio_dev *indio_dev,
 		if (!iio_device_claim_direct(indio_dev))
 			return -EBUSY;
 
-		if (val == AD4170_SINC5_AVG || val == AD4170_SINC3)
-			setup->filter_fs = clamp(val, AD4170_SINC3_MIN_FS,
-						 AD4170_SINC3_MAX_FS);
-		else
-			setup->filter_fs = clamp(val, AD4170_SINC5_MIN_FS,
-						 AD4170_SINC5_MAX_FS);
+		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);
 
 		setup->filter &= ~AD4170_FILTER_FILTER_TYPE_MSK;
 		setup->filter |= FIELD_PREP(AD4170_FILTER_FILTER_TYPE_MSK,

base-commit: cd2731444ee4e35db76f4fb587f12d327eec5446
-- 
2.47.2

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

[Nuno Sá]

On Sun, Jul 20, 2025 at 12:37:24PM -0300, Marcelo Schmitt wrote:
> 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 collateral update
> by clamping 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>
> ---
> Hi, this is the same fix as the fix provided in implied v1 patch but with the
> difference of doing it the way Markus suggested in reply to v1.
> I have a slight preference for v1 since that one keeps code contained within
> 80 columns. Though, totally fine with whatever version of the fix gets picked up.
> 
> Change log v1 (implied) -> v2
> - Replaces if by use of ternary operator as suggested by Markus in reply to v1.
> - Fixed commit log typo: colateral -> collateral
> - Fixed commit log typo: clampling -> clamping
> 
> Thanks,
> Marcelo
> 
>  drivers/iio/adc/ad4170-4.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4170-4.c b/drivers/iio/adc/ad4170-4.c
> index 6cd84d6fb08b..2e61f9a9a1ef 100644
> --- a/drivers/iio/adc/ad4170-4.c
> +++ b/drivers/iio/adc/ad4170-4.c
> @@ -879,12 +879,11 @@ static int ad4170_set_filter_type(struct iio_dev *indio_dev,
>  		if (!iio_device_claim_direct(indio_dev))
>  			return -EBUSY;
>  
> -		if (val == AD4170_SINC5_AVG || val == AD4170_SINC3)
> -			setup->filter_fs = clamp(val, AD4170_SINC3_MIN_FS,
> -						 AD4170_SINC3_MAX_FS);
> -		else
> -			setup->filter_fs = clamp(val, AD4170_SINC5_MIN_FS,
> -						 AD4170_SINC5_MAX_FS);
> +		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);

I very much prefer the approach in v1. To me, this code is just harder
to read...

Reminder to why is a good idea to wait a bit and don't rush into
spinning new versions. Also, Markus has a very proven record of not
being helpful at all in reviews (just look in lore :))

With that:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

- Nuno Sá

>  
>  		setup->filter &= ~AD4170_FILTER_FILTER_TYPE_MSK;
>  		setup->filter |= FIELD_PREP(AD4170_FILTER_FILTER_TYPE_MSK,
> 
> base-commit: cd2731444ee4e35db76f4fb587f12d327eec5446
> -- 
> 2.47.2
> 

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

[Andy Shevchenko]

On Sun, Jul 20, 2025 at 12:37:24PM -0300, Marcelo Schmitt wrote:
> 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 collateral update
> by clamping 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/

You mean Closes: here?

> 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>

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Mon, 21 Jul 2025 14:43:38 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Sun, Jul 20, 2025 at 12:37:24PM -0300, Marcelo Schmitt wrote:
> > 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 collateral update
> > by clamping 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/  
> 
> You mean Closes: here?
> 
> > 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>  
> 

I picked up v1 and made that a closes tag.  Links to email threads are fine
(there was a question in v1 on this).

Applied to the fixes-togreg-for-6.17 branch.

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

[Markus Elfring]

>>> 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 collateral update
>>> by clamping it to the range of supported values instead of mistakenly
>>> using the filter type to update the filter_fs.
…> I picked up v1 and made that a closes tag.  Links to email threads are fine
> (there was a question in v1 on this).
> 
> Applied to the fixes-togreg-for-6.17 branch.

Would you like to avoid typos anyhow for the final commit message?
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=fixes-togreg-for-6.17&id=e4d9886ad25adae72f38f2b12f41649b101581ae

Regards,
Markus