Re: [PATCHv2 1/2] Add AD7949 ADC driver family

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

On Wed, 17 Oct 2018 22:39:53 +0200
Charles-Antoine Couret <charles-antoine.couret@essensium.com> wrote:

> Add AD7949 ADC driver family
> 
> Compatible with AD7682 and AD7689 chips.
> It is a Analog Devices ADC driver 14/16 bits 4/8 channels
> with SPI protocol
> 
> Datasheet of the device:
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD7949.pdf
> 
> Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com>
Hi Charles-Antoine

Please a prefix to the patch description to describe roughly where in the
kernel a patch applies.  The precise convention varies rather more than
it probably should, but in iio we usually use.

iio:adc:ad7949: Add AD79759 ADC driver family.

A few minor bits and pieces inline to add to Alex's comments.
I missed some of these on V1, so sorry about that!

Thanks,

Jonathan

> ---
>  drivers/iio/adc/Kconfig  |  10 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad7949.c | 352 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 363 insertions(+)
>  create mode 100644 drivers/iio/adc/ad7949.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 4a754921fb6f..42e66efff6c0 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -116,6 +116,16 @@ config AD7923
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7923.
>  
> +config AD7949
> +	tristate "Analog Devices AD7949 and similar ADCs driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Analog Devices
> +	  AD7949, AD7682, AD7689 8 Channel ADCs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad7949.
> +
>  config AD799X
>  	tristate "Analog Devices AD799x ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 03db7b578f9c..88804c867aa9 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7766) += ad7766.o
>  obj-$(CONFIG_AD7791) += ad7791.o
>  obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
> +obj-$(CONFIG_AD7949) += ad7949.o
>  obj-$(CONFIG_AD799X) += ad799x.o
>  obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> new file mode 100644
> index 000000000000..da5082e80317
> --- /dev/null
> +++ b/drivers/iio/adc/ad7949.c
> @@ -0,0 +1,352 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* ad7949.c - Analog Devices ADC driver 14/16 bits 4/8 channels
> + *
> + * Copyright (C) 2018 CMC NV
> + *
> + * http://www.analog.com/media/en/technical-documentation/data-sheets/AD7949.pdf
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
> +#define AD7949_MASK_TOTAL		GENMASK(13, 0)
> +#define AD7949_OFFSET_CHANNEL_SEL	7
> +#define AD7949_CFG_READ_BACK		0x1
> +#define AD7949_CFG_REG_SIZE_BITS	14
> +
> +enum {
> +	ID_AD7949 = 0,
> +	ID_AD7682,
> +	ID_AD7689,
> +};
> +
> +struct ad7949_adc_spec {
> +	u8 num_channels;
> +	u8 resolution;
> +};
> +
> +static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> +	[ID_AD7949]  = { .num_channels = 8, .resolution = 14 },
> +	[ID_AD7682] = { .num_channels = 4, .resolution = 16 },
> +	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
> +};
> +
> +/**
> + * struct ad7949_adc_chip - AD ADC chip
> + * @lock: protects write sequences
> + * @vref: regulator generating Vref
> + * @iio_dev: reference to iio structure
> + * @spi: reference to spi structure
> + * @resolution: resolution of the chip
> + * @cfg: copy of the configuration register
> + * @current_channel: current channel in use
> + * @buffer: buffer to send / receive data to / from device
> + */
> +struct ad7949_adc_chip {
> +	struct mutex lock;
> +	struct regulator *vref;
> +	struct iio_dev *indio_dev;
> +	struct spi_device *spi;
> +	u8 resolution;
> +	u16 cfg;
> +	unsigned int current_channel;
> +	u32 buffer ____cacheline_aligned;
> +};
> +
> +static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> +{
> +	if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
> +		return true;
return !(ad7949_adc->cfg & AD7949_CFG_READ_BACK);
> +
> +	return false;
> +}
> +
> +static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> +{
> +	int ret = ad7949_adc->resolution;
> +
> +	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> +		ret += AD7949_CFG_REG_SIZE_BITS;
> +
> +	return ret;
> +}
> +
> +static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> +				u16 mask)
> +{
> +	int ret;
> +	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> +	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
> +	struct spi_message msg;
> +	struct spi_transfer tx[] = {
> +		{
> +			.tx_buf = &ad7949_adc->buffer,
> +			.len = 4,
> +			.bits_per_word = bits_per_word,
> +		},
> +	};
> +
> +	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> +	ad7949_adc->buffer = ad7949_adc->cfg << shift;
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&tx[0], &msg);

spi_message_init_with_transfers (saves a line ;)

> +	ret = spi_sync(ad7949_adc->spi, &msg);
> +
> +	/* This delay is to avoid a new request before the required time to
Multiline comment style (see below).
> +	 * send a new command to the device
> +	 */ 
> +	udelay(2);
> +	return ret;
> +}
> +
> +static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> +				   unsigned int channel)
> +{
> +	int ret;
> +	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> +	int mask = GENMASK(ad7949_adc->resolution, 0);
> +	struct spi_message msg;
> +	struct spi_transfer tx[] = {
> +		{
> +			.rx_buf = &ad7949_adc->buffer,
> +			.len = 4,
> +			.bits_per_word = bits_per_word,
> +		},
> +	};
> +
> +	ret = ad7949_spi_write_cfg(ad7949_adc,
> +				   channel << AD7949_OFFSET_CHANNEL_SEL,
> +				   AD7949_MASK_CHANNEL_SEL);
> +	if (ret)
> +		return ret;
> +
> +	ad7949_adc->buffer = 0;
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&tx[0], &msg);

spi_message_init_with_transfers. 

> +	ret = spi_sync(ad7949_adc->spi, &msg);
> +	if (ret)
> +		return ret;
> +
> +	/* This delay is to avoid a new request before the required time to
Multiline comment style.
/*
 * This
 * ...
 */
> +	 * send a new command to the device
> +	 */ 
> +	udelay(2);
> +
> +	ad7949_adc->current_channel = channel;
> +
> +	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> +		*val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
> +	else
> +		*val = ad7949_adc->buffer & mask;
> +
> +	return 0;
> +}
> +
> +#define AD7949_ADC_CHANNEL(chan) {				\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = (chan),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +}
> +
> +static const struct iio_chan_spec ad7949_adc_channels[] = {
> +	AD7949_ADC_CHANNEL(0),
> +	AD7949_ADC_CHANNEL(1),
> +	AD7949_ADC_CHANNEL(2),
> +	AD7949_ADC_CHANNEL(3),
> +	AD7949_ADC_CHANNEL(4),
> +	AD7949_ADC_CHANNEL(5),
> +	AD7949_ADC_CHANNEL(6),
> +	AD7949_ADC_CHANNEL(7),
> +};
> +
> +static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!val)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&ad7949_adc->lock);
> +		ret = ad7949_spi_read_channel(ad7949_adc, val, chan->channel);
> +		mutex_unlock(&ad7949_adc->lock);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regulator_get_voltage(ad7949_adc->vref);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret / 5000;
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
> +			unsigned int reg, unsigned int writeval,
> +			unsigned int *readval)
> +{
> +	struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	if (readval)
> +		*readval = ad7949_adc->cfg;
> +	else
> +		ret = ad7949_spi_write_cfg(ad7949_adc,
> +			writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info ad7949_spi_info = {
> +	.read_raw = ad7949_spi_read_raw,
> +	.debugfs_reg_access = ad7949_spi_reg_access,
> +};
> +
> +static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> +{
> +	int ret;
> +	int val;
> +
> +	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
> +	ad7949_adc->current_channel = 0;
> +	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
> +
> +	/* Do two dummy conversions to apply the first configuration setting.
IIO uses the most common kernel multiline comment style.

/*
 * Do two..
 */
> +	 * Required only after the start up of the device.
> +	 */
> +	ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
> +	ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
A blank line here would help readability (a very small amount!)
> +	return ret;
> +}
> +
> +static int ad7949_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	const struct ad7949_adc_spec *spec;
> +	struct ad7949_adc_chip *ad7949_adc;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
> +	if (!indio_dev) {
> +		dev_err(dev, "can not allocate iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	spi->max_speed_hz = 33000000;
This one would actually override what you already have in the DT 
binding.

> +	spi->mode = SPI_MODE_0;
This one is the default (I think....)

> +	spi->bits_per_word = 8;
This is the default so doesn't need to be set.

> +	spi_setup(spi);
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->dev.of_node = dev->of_node;
> +	indio_dev->info = &ad7949_spi_info;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad7949_adc_channels;
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	ad7949_adc = iio_priv(indio_dev);
> +	ad7949_adc->indio_dev = indio_dev;
> +	ad7949_adc->spi = spi;
> +
> +	spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
> +	indio_dev->num_channels = spec->num_channels;
> +	ad7949_adc->resolution = spec->resolution;
> +
> +	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> +	if (IS_ERR(ad7949_adc->vref)) {
> +		dev_err(dev, "fail to request regulator\n");
> +		return PTR_ERR(ad7949_adc->vref);
> +	}
> +
> +	ret = regulator_enable(ad7949_adc->vref);
> +	if (ret < 0) {
> +		dev_err(dev, "fail to enable regulator\n");
> +		goto err_regulator;
> +	}
> +
> +	mutex_init(&ad7949_adc->lock);
> +
> +	ret = ad7949_spi_init(ad7949_adc);
> +	if (ret) {
> +		dev_err(dev, "enable to init this device: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(dev, "fail to register iio device: %d\n", ret);
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	mutex_destroy(&ad7949_adc->lock);
> +	regulator_disable(ad7949_adc->vref);
> +err_regulator:
> +	return ret;
> +}
> +
> +static int ad7949_spi_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	mutex_destroy(&ad7949_adc->lock);
> +	regulator_disable(ad7949_adc->vref);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id ad7949_spi_of_id[] = {
> +	{ .compatible = "ad7949" },
> +	{ .compatible = "ad7682" },
> +	{ .compatible = "ad7689" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad7949_spi_of_id);
> +#endif
> +
> +static const struct spi_device_id ad7949_spi_id[] = {
> +	{ "ad7949", ID_AD7949  },
> +	{ "ad7682", ID_AD7682 },
> +	{ "ad7689", ID_AD7689 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ad7949_spi_id);
> +
> +static struct spi_driver ad7949_spi_driver = {
> +	.driver = {
> +		.name		= "ad7949",
> +		.of_match_table	= of_match_ptr(ad7949_spi_of_id),
> +	},
> +	.probe	  = ad7949_spi_probe,
> +	.remove   = ad7949_spi_remove,
> +	.id_table = ad7949_spi_id,
> +};
> +module_spi_driver(ad7949_spi_driver);
> +
> +MODULE_AUTHOR("Charles-Antoine Couret <charles-antoine.couret@essensium.com>");
> +MODULE_DESCRIPTION("Analog Devices 14/16-bit 8-channel ADC driver");
> +MODULE_LICENSE("GPL v2");