Re: [PATCH v6 05/17] iio: adc: Add AD7768 and AD7768-4 core support

[Andy Shevchenko]

On Fri, Sep 04, 2026 at 04:14:57PM +0200, Janani Sunil wrote:
> Add core support for the AD7768 and AD7768-4 simultaneous sampling ADCs.
> Configure supplies, clock and reset, use a custom regmap bus for the SPI
> protocol, and parse the enabled channels and input buffer settings from
> devicetree.
> 
> Connect the converter to an IIO backend for buffered capture with CRC,
> provide a fixed safe wideband sampling configuration and add runtime
> power management.

...

> +#define   AD7768_INTERFACE_CFG_DCLK_DIV_MSK		GENMASK(1, 0)
> +#define     AD7768_INTERFACE_CFG_DCLK_DIV(x)		(4 - ffs(x))

This looks suspicious, do you mean fls() / ilog2()? because ffs() while it may
work, it gets the first set LSB. Also what if 'x' is too high? I guess you can
rework the only user of that to avoid even ffs()/fls()/ilog2().

...

> +#define   AD7768_PREBUF_POS_EN(ch)			BIT((ch) * 2)

Perhaps + 0?

#define   AD7768_PREBUF_POS_EN(ch)			BIT((ch) * 2 + 0)

This will be consistent with the below.

> +#define   AD7768_PREBUF_NEG_EN(ch)			BIT(((ch) * 2) + 1)

Too many parentheses.

...

> +#define AD7768_REG_OFFSET(ch)	(AD7768_REG_OFFSET_BASE + (3 * (ch)))
> +#define AD7768_REG_GAIN(ch)	(AD7768_REG_GAIN_BASE + (3 * (ch)))
> +#define AD7768_REG_PHASE(ch)	(AD7768_REG_PHASE_BASE + (ch))
> +#define __AD7768_4_REG_MAP(ch)	((ch) < 2 ? (ch) : ((ch) + 2))

(ch) in parentheses make no sense here as it will be evaluated twice. If there
is an expression it might lead to a wrong numbers. Either you need more complex
macro to make evaluation happen once, or just be sure no caller uses an
expression in the parameter in which case the parentheses are not required.
With that being said, the other macros against (ch) also can be reconsidered.

...

> +struct ad7768_chip_info {

> +	const char *name;
> +	unsigned int num_channels;
> +	const struct regmap_config *regmap_config;
> +	const unsigned int *available_datalines;
> +	unsigned int num_datalines;
> +	const u8 *chan_map;
> +	u8 prebuf_split;

Even if `pahole` is okay with the layout, I would suggest this one instead

	const char *name;
	const struct regmap_config *regmap_config;
	const unsigned int *available_datalines;
	unsigned int num_datalines;
	unsigned int num_channels;
	const u8 *chan_map;
	u8 prebuf_split;

> +};

...

> +	return (val >> st->chip_info->prebuf_split) &
> +	       GENMASK(st->chip_info->prebuf_split - 1, 0);

Can it use field_get()?

...

> +static int ad7768_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	unsigned long channel_mask;
> +	unsigned long standby_mask;
> +	int ret;
> +
> +	channel_mask = ad7768_all_standby_mask(st);
> +	standby_mask = channel_mask & ~*scan_mask;
> +
> +	/*
> +	 * Crystal excitation requires channel 4 on AD7768 or channel 2 on
> +	 * AD7768-4 to remain active.
> +	 */
> +	if (st->clock_source == AD7768_CLOCK_SOURCE_XTAL)
> +		__clear_bit(st->chip_info->num_channels / 2, &standby_mask);
> +
> +	ret = regmap_update_bits(st->regmap, AD7768_REG_CH_STANDBY,
> +				 channel_mask, standby_mask);
> +	if (ret)
> +		return ret;
> +
> +	for (unsigned int c = 0; c < st->chip_info->num_channels; c++) {

c --> ch?

> +		if (test_bit(c, scan_mask))
> +			ret = iio_backend_chan_enable(st->back, c);
> +		else
> +			ret = iio_backend_chan_disable(st->back, c);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

...

> +	/*
> +	 * DCLK(min) is ODR * channels per DOUTx * 32. With fast mode
> +	 * (fMOD = MCLK / 4) and x64 decimation, this gives:
> +	 * MCLK / DCLK = 8 * data lines / channels.
> +	 */
> +	dclk_div = 8 * st->datalines / st->chip_info->num_channels;
> +	dclk_div_reg = AD7768_INTERFACE_CFG_DCLK_DIV(dclk_div);

So, this one is (4 - ffs(dclk_div)). If num_channels == 1, this will always give 0.
If num_channels == 2, this might give 0, 8, ... Since ffs(0) implementation is defined
to return 0, this will return... 0! So, tell me how this code may return
anything than 0?

-- 
With Best Regards,
Andy Shevchenko