Re: [PATCH v4 06/14] iio: adc: ad7768: Add configurable sampling modes

[Andy Shevchenko]

On Fri, Aug 21, 2026 at 04:06:59PM +0200, Janani Sunil wrote:
> Derive the available output data rates from MCLK and expose per-channel
> sampling frequency and filter controls.
> 
> Select the fastest compatible power mode for the enabled channels and
> map matching sampling frequency and filter combinations onto the two
> hardware channel profiles. Configure the data clock divider and wait for
> the selected filters to settle before capture.

...

> -static int ad7768_configure_capture(struct ad7768_state *st)
> +static int ad7768_set_power_mode(struct ad7768_state *st, unsigned int mode)
>  {
> -	unsigned int dclk_div_reg;
> -	unsigned int mode_config;
> -	unsigned int dclk_div;
> +	unsigned int mode_idx;
>  	int ret;

> +	for (mode_idx = 0; mode_idx < ARRAY_SIZE(ad7768_power_modes);
> +	     mode_idx++) {

I would dare to put these on a single line.

> +		if (ad7768_power_modes[mode_idx].mode == mode)
> +			break;
> +	}
> +
> +	if (mode_idx == ARRAY_SIZE(ad7768_power_modes))
> +		return -EINVAL;

But this whole piece looks quite similar to
ad7768_freq_supported_in_any_mode(). Can that be refactored to cover
this and that cases?

...

> -static int ad7768_update_scan_mode(struct iio_dev *indio_dev,
> -				   const unsigned long *scan_mask)
> +static bool ad7768_freq_supported(const struct ad7768_state *st,
> +				  unsigned int mode_idx, unsigned int freq)

Can you try to make the patch changes cleaner in terms what is put where,
so we will see less of such a mess in the diff? Perhaps it will require
to split series even more.

> +{
> +	for (unsigned int i = 0;
> +	     i < st->avail_freq[mode_idx].n_freqs; i++) {

A single line.

> +		if (freq == st->avail_freq[mode_idx].freq_cfg[i].freq_hz)
> +			return true;
> +	}
> +
> +	return false;
> +}

...

> +		mask = ad7768_channel_mode_mask(st, c);
> +		ret = regmap_update_bits(st->regmap, AD7768_REG_CH_MODE_SEL,
> +					 mask, mode ? mask : 0);
> +		if (ret)
> +			return ret;

assign_bits()?

...

> +	for (unsigned int mode = 0; mode < AD7768_NUM_CHANNEL_MODES;
> +	     mode++) {

One line.

> +		unsigned int filter_config;
> +
> +		if (!mode_used[mode])
> +			continue;
> +
> +		ret = ad7768_set_mode_decimation(st, mode_freq[mode], mode);
> +		if (ret)
> +			return ret;
> +
> +		filter_config = FIELD_PREP(AD7768_CH_MODE_FILTER_TYPE_MSK,
> +					   mode_filter[mode]);
> +		ret = regmap_update_bits(st->regmap, AD7768_REG_CH_MODE(mode),
> +					 AD7768_CH_MODE_FILTER_TYPE_MSK,
> +					 filter_config);
> +		if (ret)
> +			return ret;
> +
> +		max_freq = max(max_freq, mode_freq[mode]);
> +	}

...

> +	/*
> +	 * Start in fast mode; capture setup may select another
> +	 * compatible mode.
> +	 */
> +	scoped_guard(mutex, &st->lock) {
> +		ret = ad7768_set_power_mode(st,
> +					    AD7768_POWER_MODE_POWER_MODE_FAST);
> +	}

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set power mode\n");

Move this inside the {}.

>  static int ad7768_reset(struct ad7768_state *st)

...

Maybe it's only me, but this is completely unreviewable change (at least
by a human being). Please, refactor, split this to 5+ patches or so and
make each of them more or less reviewable.

-- 
With Best Regards,
Andy Shevchenko