Re: [PATCH v5 08/20] iio: adc: ad7768: Derive output data rates

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Aug 28, 2026 at 05:30:31PM +0200, Janani Sunil wrote:
> Derive valid output data rates from the master clock, power mode and
> decimation ratio.
> 
> Replace the fixed x64 decimation and open-coded DCLK divider in
> ad7768_configure_capture() with the derived default output rate and the
> new decimation and clock-divider helpers. Select the maximum rate
> supported by the configured data interface for the initial capture
> configuration.

...

> +static int ad7768_set_clk_divs(struct ad7768_state *st, unsigned int freq)
> +{
> +	const struct ad7768_freq_config *freq_cfg;
> +	unsigned int mclk, dclk, dclk_div;
> +	unsigned int chan_per_doutx;
> +	unsigned int dclk_div_reg;
> +
> +	freq_cfg = ad7768_find_freq_config(st, st->power_mode_idx, freq);
> +	if (!freq_cfg)
> +		return -EINVAL;
> +
> +	mclk = clk_get_rate(st->mclk);
> +	chan_per_doutx = st->chip_info->num_channels / st->datalines;
> +	if (!chan_per_doutx)
> +		return -EINVAL;
> +
> +	dclk = freq_cfg->freq_hz * AD7768_SAMPLE_SIZE * chan_per_doutx;
> +	if (!dclk || dclk > mclk)
> +		return -EINVAL;

SAMPLE_SIZE must not be 0, the chan_per_doutx is checked above, so this check
is about freq_hz, do we need it? If yes, make it better written

	freq_cfg = ad7768_find_freq_config(st, st->power_mode_idx, freq);
	if (!freq_cfg)
		return -EINVAL;
	// Do we really need this?
	if (!freq_cfg->freq_hz)
		return -EINVAL;
	...
	dclk = freq_cfg->freq_hz * AD7768_SAMPLE_SIZE * chan_per_doutx;
	if (dclk > mclk)
		return -EINVAL;

> +	/* Set the divider to the next-lowest supported power of two. */
> +	dclk_div = DIV_ROUND_CLOSEST(mclk, dclk);
> +	if (dclk_div > AD7768_MAX_DCLK_DIV)
> +		dclk_div = AD7768_MAX_DCLK_DIV;
> +	else
> +		dclk_div = rounddown_pow_of_two(dclk_div);

It's okay as is, but just to show that this can be shorten to

	dclk_div = DIV_ROUND_CLOSEST(mclk, dclk);

	/* Set the divider to the next-lowest supported power of two. */
	dclk_div = rounddown_pow_of_two(min(dclk_div, AD7768_MAX_DCLK_DIV));

> +	dclk_div_reg = AD7768_INTERFACE_CFG_DCLK_DIV(dclk_div);
> +
> +	return regmap_update_bits(st->regmap, AD7768_REG_INTERFACE_CFG,
> +				  AD7768_INTERFACE_CFG_DCLK_DIV_MSK,
> +				  FIELD_PREP(AD7768_INTERFACE_CFG_DCLK_DIV_MSK,
> +					     dclk_div_reg));
> +}

...

> +static void ad7768_set_available_sampling_freqs(struct ad7768_state *st)
> +{

	unsigned int n_power_modes = ARRAY_SIZE(ad7768_power_modes);

> +	unsigned int mclk = clk_get_rate(st->mclk);
> +
> +	for (unsigned int mode_idx = 0;
> +	     mode_idx < ARRAY_SIZE(ad7768_power_modes); mode_idx++) {

	for (unsigned int mode_idx = 0; mode_idx < n_power_modes); mode_idx++) {

> +		struct ad7768_avail_freq *avail_freq;
> +
> +		avail_freq = &st->avail_freq[mode_idx];

> +		for (unsigned int dec = ARRAY_SIZE(ad7768_dec_rate); dec > 0;
> +		     dec--) {

Leave this on a single line, it's okay to have it slightly bigger. However,
taking into account the below I would rather do it differently, id est

		unsigned int dec = ARRAY_SIZE(ad7768_dec_rate);
		struct ad7768_avail_freq *avail_freq;
		...
		while (dec--) {

> +			struct ad7768_freq_config *freq_cfg;
> +
> +			freq_cfg = &avail_freq->freq_cfg[avail_freq->n_freqs++];
> +			freq_cfg->dec_rate = dec - 1;

			freq_cfg->dec_rate = dec;

> +			freq_cfg->freq_hz = mclk /
> +				(ad7768_dec_rate[dec - 1] *
> +				 ad7768_power_modes[mode_idx].mclk_div);

This style is not good, better to take longer lines, but split on logical
boundaries. I also would go with a temporary variable

			// can be shortened to 'd' if you wish
			unsigned int div = ad7768_power_modes[mode_idx].mclk_div;
			...

			freq_cfg->freq_hz = mclk / (ad7768_dec_rate[dec] * div);
> +		}
> +	}
> +
> +	/* One DOUT line cannot carry the AD7768 fast-mode x32 output rate. */
> +	if (st->datalines == 1 &&
> +	    st->chip_info->num_channels == AD7768_MAX_CHANNEL)

> +		st->avail_freq[ARRAY_SIZE(ad7768_power_modes) - 1].n_freqs--;

		st->avail_freq[n_power_modes - 1].n_freqs--;

> +}

-- 
With Best Regards,
Andy Shevchenko