Re: [PATCH v3 05/10] iio: adc: ad4170: Add digital filter and sample frequency config support

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 13 May 2025 09:35:03 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> Add support for sinc3, sinc5, and averaged sinc5 digital filters along with
> sample frequency configuration.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

> +static int ad4170_set_filter_type(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan,
> +				  unsigned int val)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address];
> +	struct ad4170_setup *setup = &chan_info->setup;
> +	unsigned int old_filter_fs, old_filter, filter_type_conf;
> +	int ret;
> +
> +	switch (val) {
> +	case AD4170_SINC5_AVG:
> +		filter_type_conf = AD4170_FILTER_FILTER_TYPE_SINC5_AVG;
> +		break;
> +	case AD4170_SINC5:
> +		filter_type_conf = AD4170_FILTER_FILTER_TYPE_SINC5;
> +		break;
> +	case AD4170_SINC3:
> +		filter_type_conf = AD4170_FILTER_FILTER_TYPE_SINC3;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	guard(mutex)(&st->lock);

You need to scope this because otherwise locks are released in a different
order to how they are taken.

	iio_device_claim_direct (takes mlock in the core)
	guard(mutex)(&st->lock)
	iio_device_release_direct (releases mlock in the core.

	... scope finishes
	guard(mutex)(&st->lock) cleanup happens.


Current code might be fine, but it's harder to reason about.

> +	/*
> +	 * The filters provide the same ODR for a given filter_fs value but
> +	 * there are different minimum and maximum filter_fs limits for each
> +	 * filter. The filter_fs value will be adjusted if the current filter_fs
> +	 * is out of the limits of the just requested filter. Since the
> +	 * filter_fs value affects the ODR (sampling_frequency), changing the
> +	 * filter may lead to a change in the sampling frequency.
> +	 */
> +	old_filter = setup->filter;
> +	old_filter_fs = setup->filter_fs;
> +	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 &= ~AD4170_FILTER_FILTER_TYPE_MSK;
> +	setup->filter |= FIELD_PREP(AD4170_FILTER_FILTER_TYPE_MSK,
> +				    filter_type_conf);
> +
> +	ret = ad4170_write_channel_setup(st, chan->address, false);
> +	if (ret) {
> +		setup->filter = old_filter;
> +		setup->filter_fs = old_filter_fs;
> +	}
> +
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}