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

[Jonathan Cameron]

> 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@xxxxxxxxxx>
>
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

> diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> new file mode 100644
> index 000000000000..3cb58c2554a3
> --- /dev/null
> +++ b/drivers/iio/adc/ad7768.c


> +
> +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 <jonathan.cameron@xxxxxxxxxxxxxxxx>