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@xxxxxxxxxx>
> Link: https://lore.kernel.org/linux-iio/c6e54942-5b42-484b-be53-9d4606fd25c4@sabinyo.mountain/
> Suggested-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> Fixes: 8ab7434734cd ("iio: adc: ad4170-4: Add digital filter and sample frequency config support")
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>

- Nuno Sá

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