Re: [PATCH v2 4/7] iio: adc: Add AD7768 IIO Driver support

On Thu, Aug 06, 2026 at 05:41:23PM +0200, Janani Sunil wrote:
> Add support for AD7768 4/8 channel,simultaneous sampling Sigma-Delta
> ADCs. The driver supports configurable decimation filters, per-channel
> conversion delay, VCM regulation, runtime PM and IIO backend data
> capture.

Can you split this to the basic minimum + patch per feature?

1.7k LoC is too much for a review, the usual size is ~750±150 per patch.
Here sounds like 3+ patches.

...

> +#define   AD7768_PICO_PER_SEC			1000000000000ULL

Don't we have this in time.h (beneath it somewhere)?

...

> +struct ad7768_freq_config {
> +	unsigned int freq;

freq_Hz?

> +	unsigned int dec_rate;
> +};

...

> +struct ad7768_avail_freq {
> +	unsigned int n_freqs;
> +	int freqs[MAX_FREQ_PER_MODE];

Negative frequency?

> +	struct ad7768_freq_config freq_cfg[MAX_FREQ_PER_MODE];
> +};

...

> +struct ad7768_state {
> +	struct spi_device *spi;
> +	struct regmap *regmap;

One of them seems redundant. Is regmap created out from &spi->dev?

> +	struct mutex lock; /* Protects device register access and configuration */
> +	struct clk *mclk;
> +	unsigned int datalines;
> +	enum ad7768_power_modes power_mode;
> +	const struct ad7768_chip_info *chip_info;
> +	struct ad7768_avail_freq avail_freq[AD7768_NUM_POWER_MODES];
> +	unsigned int n_freqs;
> +	int freqs[AD7768_MAX_FREQS];
> +	unsigned int chn_mode[AD7768_MAX_CHANNEL];
> +	unsigned int ch_freq[AD7768_MAX_CHANNEL];
> +	u64 ch_convdelay_ps[AD7768_MAX_CHANNEL];
> +	enum ad7768_filter_type ch_filter[AD7768_MAX_CHANNEL];
> +	struct iio_backend *back;
> +	struct regulator_dev *vcm_rdev;
> +	unsigned int avdd1_uv;

_uV

> +
> +	__be16 d16 __aligned(IIO_DMA_MINALIGN);
> +};

...

> +static const unsigned int ad7768_vcm_voltage_table[] = {
> +	0, 1650000, 2500000, 2140000

In this case, keep trailing comma.

> +};

...

> +static int ad7768_vcm_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct ad7768_state *st = rdev_get_drvdata(rdev);
> +	unsigned int val;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(&st->spi->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD7768_REG_GENERAL_CONFIG, &val);
> +	if (ret)
> +		return ret;
> +
> +	return !(val & AD7768_GEN_CONFIG_VCM_PD);

regmap_test_bits() ?

> +}

...

> +static const int ad7768_dec_rate[MAX_FREQ_PER_MODE] = {
> +	32, 64, 128, 256, 512, 1024

Even in the very same page you have inconsistent approach. Make sure your code
is consistent in every aspect. Two+ people wrote the driver?

> +};
> +
> +static const int ad7768_mclk_div[3] = {
> +	32, 8, 4
> +};
> +
> +static const unsigned int ad7768_available_datalines[] = {
> +	1, 2, 8,
> +};
> +
> +static const unsigned int ad7768_4_available_datalines[] = {
> +	1, 4,
> +};
> +
> +static const u8 ad7768_chan_map[] = {
> +	0, 1, 2, 3, 4, 5, 6, 7,
> +};
> +
> +static const u8 ad7768_4_chan_map[] = {
> +	0, 1, 4, 5,
> +};

...

> +static int ad7768_set_power_mode(struct ad7768_state *st, unsigned int mode);

Can forward decl be avoided?

...

> +static u8 ad7768_all_channels_mask(const struct ad7768_state *st)
> +{
> +	u8 mask = 0;
> +	unsigned int ch;
> +
> +	for (ch = 0; ch < st->chip_info->num_channels; ch++)

	for (unsigned int ch = 0; ch < st->chip_info->num_channels; ch++)

> +		mask |= ad7768_channel_mask(st, ch);
> +
> +	return mask;
> +}

...

> +static int ad7768_regmap_read(void *context, const void *reg_buf,
> +			      size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct spi_device *spi = context;
> +	struct ad7768_state *st = spi_get_drvdata(spi);
> +	u8 *data_val = val_buf;
> +	unsigned int reg;
> +	int ret;
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = &st->d16,
> +			.len = 2,
> +			.cs_change = 1,
> +		}, {
> +			/*
> +			 * The second transfer clocks out the readback data, so
> +			 * we must provide dummy TX bytes while receiving the
> +			 * response. The device ignores MOSI in this phase, so
> +			 * reuse st->d16 for both TX and RX.
> +			 */
> +			.tx_buf = &st->d16,
> +			.rx_buf = &st->d16,
> +			.len = 2,
> +		},
> +	};
> +
> +	reg = *(const u8 *)reg_buf;
> +
> +	st->d16 = cpu_to_be16(AD7768_SPI_READ_CMD |
> +			      FIELD_PREP(AD7768_SPI_REG_MASK, reg));

be16_replace_bits() ?

> +	ret = spi_sync_transfer(spi, t, ARRAY_SIZE(t));
> +	if (ret)
> +		return ret;
> +
> +	*data_val = FIELD_GET(AD7768_SPI_DATA_MASK, be16_to_cpu(st->d16));

be16_get_bits()?

> +	return ret;
> +}

...

> +static int ad7768_read_calib_value(struct ad7768_state *st,
> +				   unsigned int base_reg, unsigned int *val)
> +{
> +	u8 data[3];
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_bulk_read(st->regmap, base_reg, data, ARRAY_SIZE(data));
> +	if (ret)
> +		return ret;
> +
> +	*val = (data[0] << 16) | (data[1] << 8) | data[2];

get_unaligned_be24()

> +	return 0;
> +}

> +static int ad7768_write_calib_value(struct ad7768_state *st,
> +				    unsigned int base_reg, unsigned int val)
> +{
> +	int ret;
> +
> +	if (val > AD7768_CALIB_REG_MSK)
> +		return -EINVAL;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_write(st->regmap, base_reg,
> +			   FIELD_GET(AD7768_CALIB_REG_MSB_MSK, val));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, base_reg + 1,
> +			   FIELD_GET(AD7768_CALIB_REG_MID_MSK, val));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, base_reg + 2,
> +			    FIELD_GET(AD7768_CALIB_REG_LSB_MSK, val));

Can you prepare value and use bulk write? Probably you want
put_unaligned_be24().

> +}

...

> +static int ad7768_set_clk_divs(struct ad7768_state *st,
> +			       unsigned int freq)
> +{
> +	unsigned int mclk, dclk, dclk_div, i;
> +	struct ad7768_freq_config f_cfg = {};
> +	unsigned int chan_per_doutx;
> +
> +	mclk = clk_get_rate(st->mclk);
> +
> +	chan_per_doutx = st->chip_info->num_channels / st->datalines;
> +	if (!chan_per_doutx)
> +		return -EINVAL;
> +
> +	for (i = 0; i < st->avail_freq[st->power_mode].n_freqs; i++) {
> +		f_cfg = st->avail_freq[st->power_mode].freq_cfg[i];
> +		if (freq == f_cfg.freq)
> +			break;
> +	}
> +
> +	if (i == st->avail_freq[st->power_mode].n_freqs)
> +		return -EINVAL;
> +
> +	dclk = f_cfg.freq * AD7768_SAMPLE_SIZE * chan_per_doutx;
> +	if (dclk > mclk)
> +		return -EINVAL;
> +
> +	/* Set dclk_div to the nearest power of 2 less than the original value */
> +	dclk_div = DIV_ROUND_CLOSEST_ULL(mclk, dclk);

_ULL for sure?! Please, use 32-bit arithmetics for 32-bit values (yes, 32-bit,
they never be 64 in real life).

> +	if (dclk_div > AD7768_MAX_DCLK_DIV)
> +		dclk_div = AD7768_MAX_DCLK_DIV;
> +	else if (dclk_div > 0 && hweight32(dclk_div) != 1)
> +		dclk_div = 1 << (fls(dclk_div) - 1);

rounddown_pow_of_two() ?

> +	return regmap_update_bits(st->regmap, AD7768_REG_INTERFACE_CFG,
> +				  AD7768_INTERFACE_CFG_DCLK_DIV_MSK,
> +				  AD7768_INTERFACE_CFG_DCLK_DIV_MODE(dclk_div));
> +}

...

> +static bool ad7768_freq_supported(const struct ad7768_state *st,
> +				  unsigned int mode, unsigned int freq)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < st->avail_freq[mode].n_freqs; i++) {

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

> +		if (freq == st->avail_freq[mode].freq_cfg[i].freq)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool ad7768_freq_supported_in_any_mode(const struct ad7768_state *st,
> +					      unsigned int freq)
> +{
> +	unsigned int mode;
> +
> +	for (mode = 0; mode < AD7768_NUM_POWER_MODES; mode++) {

Ditto.

> +		if (ad7768_freq_supported(st, mode, freq))
> +			return true;
> +	}
> +
> +	return false;
> +}

...

> +static int ad7768_set_lowest_noise_mode(struct ad7768_state *st,
> +					const unsigned long *scan_mask)
> +{
> +	unsigned int channel;
> +	unsigned int mode;
> +
> +	/*
> +	 * The output data rate ranges overlap between the power modes. At a
> +	 * common ODR, the faster mode has lower noise, so prefer the fastest
> +	 * mode that supports every enabled channel.
> +	 */
> +	for (mode = AD7768_NUM_POWER_MODES; mode-- > 0;) {

	unsigned int mode = AD7768_NUM_POWER_MODES;
	...
	while (mode--) {

> +		for (channel = 0; channel < st->chip_info->num_channels; channel++) {
> +			if (test_bit(channel, scan_mask) &&
> +			    !ad7768_freq_supported(st, mode, st->ch_freq[channel]))
> +				break;
> +		}
> +
> +		if (channel == st->chip_info->num_channels)
> +			return ad7768_set_power_mode(st, mode);
> +	}
> +
> +	return -EINVAL;
> +}

...

> +	struct ad7768_state *st = iio_priv(indio_dev);
> +
> +	if (!freq)
> +		return -EINVAL;
> +
> +	if (!ad7768_freq_supported_in_any_mode(st, freq))
> +		return -EINVAL;
> +
> +	guard(mutex)(&st->lock);

+ blank line here.

> +	st->ch_freq[ch] = freq;
> +
> +	return 0;

...

> +static int ad7768_get_freq_cfg(struct ad7768_state *st, unsigned int freq,
> +			       struct ad7768_freq_config *f_cfg)
> +{
> +	unsigned int i;

Embed into for-loop.

> +	for (i = 0; i < st->avail_freq[st->power_mode].n_freqs; i++) {
> +		*f_cfg = st->avail_freq[st->power_mode].freq_cfg[i];
> +		if (freq == f_cfg->freq)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}

...

> +static void ad7768_filter_wait(const unsigned int *mode_freq,
> +			       const enum ad7768_filter_type *mode_filter,
> +			       const bool *mode_used)
> +{
> +	unsigned long t_settle_us = 0;
> +	unsigned int mode;
> +
> +	for (mode = 0; mode < AD7768_NUM_CHANNEL_MODES; mode++) {
> +		unsigned long t_mode_us;

Hmm... Are you sure about the type? Shouldn't it be always 64-bit (or 32-bit)?

> +		unsigned int settling_samples;
> +
> +		if (!mode_used[mode] || !mode_freq[mode])
> +			continue;
> +
> +		if (mode_filter[mode] == AD7768_FILTER_TYPE_SINC5)
> +			settling_samples = AD7768_SINC5_SETTLING_SAMPLES;
> +		else
> +			settling_samples = AD7768_WIDEBAND_SETTLING_SAMPLES;
> +
> +		t_mode_us = DIV_ROUND_UP_ULL((u64)settling_samples *
> +					     USEC_PER_SEC,
> +					     mode_freq[mode]);
> +		t_settle_us = max(t_settle_us, t_mode_us);
> +	}
> +
> +	if (t_settle_us)
> +		fsleep(t_settle_us);
> +}

...

I stopped here as this is too much and unreviewable bulk.

-- 
With Best Regards,
Andy Shevchenko