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

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

On Thu, Sep 10, 2026 at 07:36:23PM +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.

...

> +static const struct regmap_config ad7768_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AD7768_REG_CHOP_CTRL,
> +	/*
> +	 * Regmap bulk transfers use one starting address, while each register
> +	 * access requires a separate 16-bit SPI frame and reads use an off-frame
> +	 * response. Split bulk transfers into individual register accesses.
> +	 */

Here, or in additional place it might be good to mention why no cache is used.

> +	.use_single_read = true,
> +	.use_single_write = true,
> +	.readable_reg = ad7768_readable_reg,
> +};
> +
> +static const struct regmap_config ad7768_4_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AD7768_REG_CHOP_CTRL,
> +	/*
> +	 * Regmap bulk transfers use one starting address, while each register
> +	 * access requires a separate 16-bit SPI frame and reads use an off-frame
> +	 * response. Split bulk transfers into individual register accesses.
> +	 */

Ditto.

> +	.use_single_read = true,
> +	.use_single_write = true,
> +	.readable_reg = ad7768_4_readable_reg,
> +};

...

> +	/*
> +	 * 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;
> +	switch (dclk_div) {
> +	case 1:
> +		dclk_div_reg = AD7768_INTERFACE_CFG_DCLK_DIV_1;
> +		break;
> +	case 2:
> +		dclk_div_reg = AD7768_INTERFACE_CFG_DCLK_DIV_2;
> +		break;
> +	case 4:
> +		dclk_div_reg = AD7768_INTERFACE_CFG_DCLK_DIV_4;
> +		break;
> +	case 8:
> +		dclk_div_reg = AD7768_INTERFACE_CFG_DCLK_DIV_8;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Yep, this is clearer.

...

> +static int ad7768_parse_config(struct iio_dev *indio_dev,
> +			       struct device *dev)
> +{
> +	struct ad7768_precharge_config precharge_cfg[AD7768_MAX_CHANNEL] = { };
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	const unsigned int *available_datalines;
> +	bool datalines_valid = false;
> +	struct iio_chan_spec *chan;
> +	unsigned int num_channels;
> +	unsigned long standby_mask;
> +	unsigned int len;
> +	int chan_idx = 0;
> +	int ret;
> +
> +	num_channels = device_get_named_child_node_count(dev, "channel");

> +

Unneeded blank line.

> +	if (num_channels == 0)
> +		return dev_err_probe(dev, -ENOENT, "No channel specified\n");
> +
> +	if (num_channels > st->chip_info->num_channels)
> +		return dev_err_probe(dev, -ENOSPC, "Invalid number of channels\n");
> +
> +	chan = devm_kcalloc(dev, num_channels, sizeof(*chan), GFP_KERNEL);
> +	if (!chan)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = chan;
> +	indio_dev->num_channels = num_channels;
> +
> +	standby_mask = ad7768_all_standby_mask(st);
> +
> +	/*
> +	 * 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);

Hmm... I have a déjà vu feeling that I have seen already the same / similar.
If I'm right, this piece can be moved to a helper.

> +	ret = regmap_write(st->regmap, AD7768_REG_CH_STANDBY, standby_mask);
> +	if (ret)
> +		return ret;
> +
> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
> +		u32 channel;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &channel);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to parse reg of %pfwP\n",
> +					     child);
> +
> +		if (channel >= st->chip_info->num_channels)
> +			return dev_err_probe(dev, -ECHRNG,
> +					     "Invalid channel %u in firmware\n",
> +					     channel);
> +
> +		ret = regmap_clear_bits(st->regmap, AD7768_REG_CH_STANDBY,
> +					BIT(channel));
> +		if (ret)
> +			return ret;
> +
> +		precharge_cfg[channel].prebufp_en =
> +			fwnode_property_read_bool(child,
> +						  "adi,prechargebuf-pos-enable");
> +		precharge_cfg[channel].prebufn_en =
> +			fwnode_property_read_bool(child,
> +						  "adi,prechargebuf-neg-enable");
> +		precharge_cfg[channel].refbufp =
> +			fwnode_property_read_bool(child,
> +						  "adi,refbuf-pos-enable");
> +		precharge_cfg[channel].refbufn =
> +			fwnode_property_read_bool(child,
> +						  "adi,refbuf-neg-enable");
> +
> +		chan[chan_idx] = (struct iio_chan_spec) {
> +			.type = IIO_VOLTAGE,
> +			.info_mask_separate = BIT(IIO_CHAN_INFO_SCALE),
> +			.indexed = 1,
> +			.channel = channel,
> +			.scan_index = channel,
> +			.scan_type = {
> +				.sign = 's',
> +				.realbits = 24,
> +				.storagebits = 32,
> +			},
> +		};
> +		chan_idx++;
> +	}
> +
> +	ret = ad7768_configure_precharge_buffers(indio_dev, precharge_cfg);
> +	if (ret)
> +		return ret;
> +
> +	available_datalines = st->chip_info->available_datalines;
> +	len = st->chip_info->num_datalines;

You also can

	const char *propname;
	...
	propname = "adi,data-lines-number";

> +	if (device_property_present(dev, "adi,data-lines-number")) {

	if (device_property_present(dev, propname)) {

> +		ret = device_property_read_u32(dev, "adi,data-lines-number",
> +					       &st->datalines);

		ret = device_property_read_u32(dev, propname, &st->datalines);

> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Invalid adi,data-lines-number property\n");

			return dev_err_probe(dev, ret, "Invalid %s property\n", propname);

> +	} else {
> +		st->datalines = available_datalines[len - 1];
> +	}
> +
> +	for (unsigned int i = 0; i < len; i++) {
> +		if (available_datalines[i] == st->datalines) {
> +			datalines_valid = true;
> +			break;
> +		}
> +	}
> +
> +	if (!datalines_valid)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid data-lines-number %d for %s\n",
> +				     st->datalines, st->chip_info->name);

And even here

				     "Invalid %s %d for %s\n", propname,
				     st->datalines, st->chip_info->name);

> +	return ad7768_configure_capture(st);
> +}

...

> +static void ad7768_power_off(void *data)
> +{
> +	struct ad7768_state *st = data;
> +	struct device *dev;
> +	int ret;
> +
> +	dev = regmap_get_device(st->regmap);

Can be assigned directly above

	struct device *dev = regmap_get_device(st->regmap);

> +	ret = ad7768_enter_sleep(st);
> +	if (ret)
> +		dev_err(dev, "Failed to put device to sleep\n");
> +}

...

> +	ret = devm_regulator_get_enable_optional(dev, "avss");
> +	if (ret == -ENODEV) {
> +		/* AVSS may be tied directly to ground instead of a regulator. */

tied --> wired / connected / ...?

> +	} else if (ret) {
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable AVSS supply\n");

Fine to be on a single line.

> +	}

...

> +static const struct spi_device_id ad7768_spi_id[] = {
> +	{ "ad7768", (kernel_ulong_t)&ad7768_chip_info },
> +	{ "ad7768-4", (kernel_ulong_t)&ad7768_4_chip_info },
> +	{ }

C99 initialisers.

> +};
> +MODULE_DEVICE_TABLE(spi, ad7768_spi_id);

-- 
With Best Regards,
Andy Shevchenko