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

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.

Signed-off-by: Janani Sunil <janani.sunil@analog.com>

Hi Janani,

Given you are going to be doing a v8 anyway, here are a few comments
from that hopefully don't overlap too much with ones Andy gave.

Thanks

Jonathan

+
+struct ad7768_state {
+	struct regmap *regmap;
+	struct clk *mclk;
+	unsigned int datalines;
+	enum ad7768_clock_source clock_source;
+	const struct ad7768_chip_info *chip_info;
+	struct iio_backend *back;
+	unsigned int vref_uV[2];
+
+	__be16 d16 __aligned(IIO_DMA_MINALIGN);

I think it would be good to have a comment that this buffer
is used in the regmap write callback and hence access is controlled
by the regmap locks. Something like.
   /* Used only in regmap_write() callback, hence guarded by regmap lock */

+};

...

+
+static int ad7768_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct ad7768_state *st = iio_priv(indio_dev);
+
+	pm_runtime_put_autosuspend(regmap_get_device(st->regmap));

Trivial but nice to have a blank line before a simple returnstatement
like this one. Helps it stand out to those reading.

+	return 0;
+}

...

+
+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");
+

Trivial but as you are rolling again, drop the blank line here.

+	if (num_channels == 0)
+		return dev_err_probe(dev, -ENOENT, "No channel specified\n");
+

and here.

+	if (num_channels > st->chip_info->num_channels)
+		return dev_err_probe(dev, -ENOSPC, "Invalid number of channels\n");

That will keey the 'get' and the pair of error checks tightly
visually associated with each other.

+
+	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);
+
+	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");

I'd go long on these probably for readability.

		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");

+		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) {

  		chan[chan_idx++] = (struct iio_chan_spec) {
Doesn't seem to lose anything for readability to me and makes it lcear
we are only writing each chan entry in one place.

+			.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++;
+	}
+

.

-- 
Jonathan Cameron [off-list ref]