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

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