Re: [PATCH v4 06/14] iio: adc: ad7768: Add configurable sampling modes
Andy Shevchenko <[email protected]>
| Newsgroups | org.kernel.vger.linux-iio,org.kernel.vger.linux-devicetree,org.kernel.vger.linux-doc,org.kernel.vger.linux-gpio,org.kernel.vger.linux-kernel |
|---|---|
| Organization | Intel Finland Oy - BIC 0357606-4 - c/o Alberga Business Park, 6 krs, Bertel Jungin Aukio 5, 02600 Espoo |
| Message-ID | <[email protected]> |
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