[PATCH 1/2] Add AD7949 ADC driver family

[PATCH 1/2] Add AD7949 ADC driver family

[Charles-Antoine Couret]

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>
---
 drivers/iio/adc/Kconfig  |  10 ++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad7949.c | 329 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 340 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..667636b476f8
--- /dev/null
+++ b/drivers/iio/adc/ad7949.c
@@ -0,0 +1,329 @@
+// 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/iio/iio.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/delay.h>
+
+#define AD7949_MASK_CFG			0x3C7F
+#define AD7949_MASK_CHANNEL_SEL		0x380
+#define AD7949_MASK_TOTAL		0x3FFF
+#define AD7949_OFFSET_CHANNEL_SEL	7
+#define AD7949_CFG_READ_BACK		0x1
+#define AD7949_CFG_REG_SIZE_BITS	14
+
+enum {
+	HEIGHT_14BITS = 0,
+	QUAD_16BITS,
+	HEIGHT_16BITS,
+};
+
+struct ad7949_adc_spec {
+	u8 num_channels;
+	u8 resolution;
+};
+
+static const struct ad7949_adc_spec ad7949_adc_spec[] = {
+	[HEIGHT_14BITS]  = { .num_channels = 8, .resolution = 14 },
+	[QUAD_16BITS] = { .num_channels = 4, .resolution = 16 },
+	[HEIGHT_16BITS] = { .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
+ * @resolution: resolution of the chip
+ */
+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;
+};
+
+static u16 ad7949_spi_read_cfg(struct ad7949_adc_chip *ad7949_adc)
+{
+	return ad7949_adc->cfg;
+}
+
+static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
+{
+	u16 cfg = ad7949_spi_read_cfg(ad7949_adc) & AD7949_MASK_TOTAL;
+	bool is_cfg_read_back = cfg & AD7949_CFG_READ_BACK ? false : true;
+	int ret = ad7949_adc->resolution;
+
+	if (is_cfg_read_back)
+		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;
+	u16 cfg = ad7949_spi_read_cfg(ad7949_adc) & AD7949_MASK_TOTAL;
+	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
+	int shift = (bits_per_word - AD7949_CFG_REG_SIZE_BITS);
+	u32 buf_value = ((val & mask) | (cfg & ~mask)) << shift;
+	struct spi_message msg;
+	struct spi_transfer tx[] = {
+		{
+			.tx_buf = &buf_value,
+			.len = 4,
+			.bits_per_word = bits_per_word,
+		},
+	};
+
+	ad7949_adc->cfg = buf_value >> shift;
+	spi_message_init(&msg);
+	spi_message_add_tail(&tx[0], &msg);
+	ret = spi_sync(ad7949_adc->spi, &msg);
+	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 shift = (bits_per_word - AD7949_CFG_REG_SIZE_BITS);
+	u32 buf_value = 0;
+	struct spi_message msg;
+	struct spi_transfer tx[] = {
+		{
+			.rx_buf = &buf_value,
+			.len = 4,
+			.bits_per_word = bits_per_word,
+		},
+	};
+
+	if (!val)
+		return -EINVAL;
+
+	ad7949_spi_write_cfg(ad7949_adc, channel << AD7949_OFFSET_CHANNEL_SEL,
+			     AD7949_MASK_CHANNEL_SEL);
+	spi_message_init(&msg);
+	spi_message_add_tail(&tx[0], &msg);
+	ret = spi_sync(ad7949_adc->spi, &msg);
+	udelay(2);
+
+	ad7949_adc->current_channel = channel;
+	*val = (buf_value >> shift);
+	return ret;
+}
+
+#define AD7949_ADC_CHANNEL(chan) {				\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.scan_index = (chan),					\
+	.channel = (chan),					\
+	.address = (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;
+
+	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;
+
+		ret = IIO_VAL_INT;
+		break;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regulator_get_voltage(ad7949_adc->vref);
+		if (ret < 0)
+			return ret;
+
+		*val = ret / 5000;
+		*val2 = 0;
+		ret = IIO_VAL_INT;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+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_spi_read_cfg(ad7949_adc);
+	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)
+{
+	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
+	ad7949_adc->current_channel = 0;
+	return ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
+}
+
+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;
+	spi->mode = SPI_MODE_0;
+	spi->irq = -1;
+	spi->bits_per_word = 8;
+	spi_setup(spi);
+
+	indio_dev->dev.parent = dev;
+	indio_dev->dev.of_node = spi->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 = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(dev, "fail to register iio device: %d\n", ret);
+		goto err;
+	}
+
+	ret = ad7949_spi_init(ad7949_adc);
+	if (ret) {
+		dev_err(dev, "enable to init this device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	mutex_destroy(&ad7949_adc->lock);
+err_regulator:
+	regulator_disable(ad7949_adc->vref);
+	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", HEIGHT_14BITS  },
+	{ "ad7682", QUAD_16BITS },
+	{ "ad7689", HEIGHT_16BITS },
+	{ }
+};
+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");
-- 
2.17.1

[PATCH 2/2] Add ad7949 device tree bindings in documentation

[Charles-Antoine Couret]

Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com>
---
 .../devicetree/bindings/iio/adc/ad7949.txt     | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ad7949.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
new file mode 100644
index 000000000000..4dc921961df7
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
@@ -0,0 +1,18 @@
+* Analog Devices AD7949/AD7682/AD7689
+
+Required properties:
+ - compatible: Should be one of
+	* "ad7949"
+	* "ad7682"
+	* "ad7689"
+ - reg: spi chip select number for the device
+ - vref-supply: The regulator supply for ADC reference voltage
+ - spi-max-frequency: Max SPI frequency to use (< 33000000)
+
+Example:
+adc@0 {
+	compatible = "ti,adc0832";
+	reg = <0>;
+	vref-supply = <&vdd_supply>;
+	spi-max-frequency = <200000>;
+};
-- 
2.17.1

Re: [PATCH 2/2] Add ad7949 device tree bindings in documentation

[Jonathan Cameron]

On Sat,  6 Oct 2018 22:30:36 +0200
Charles-Antoine Couret <charles-antoine.couret@essensium.com> wrote:

> Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com>
Device tree bindings should be cc'd to the devicetree maintainers
and mailing list for review.

As has been shown far too many times, I'm not very good and
spotting the detail issues in these! So over to Rob and Mark's
eagle eyes.

Jonathan

> ---
>  .../devicetree/bindings/iio/adc/ad7949.txt     | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ad7949.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> new file mode 100644
> index 000000000000..4dc921961df7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
> @@ -0,0 +1,18 @@
> +* Analog Devices AD7949/AD7682/AD7689
> +
> +Required properties:
> + - compatible: Should be one of
> +	* "ad7949"
> +	* "ad7682"
> +	* "ad7689"
> + - reg: spi chip select number for the device
> + - vref-supply: The regulator supply for ADC reference voltage
> + - spi-max-frequency: Max SPI frequency to use (< 33000000)
> +
> +Example:
> +adc@0 {
> +	compatible = "ti,adc0832";
> +	reg = <0>;
> +	vref-supply = <&vdd_supply>;
> +	spi-max-frequency = <200000>;
> +};

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

[Jonathan Cameron]

On Sat,  6 Oct 2018 22:30:35 +0200
Charles-Antoine Couret <charles-antoine.couret@essensium.com> wrote:

> 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,

Welcome to IIO.

Always good to see a new submitter, particularly when they come with
several new drivers.

There are a few issues highlighted inline.

Thanks and looking forward to v2,

Jonathan
> ---
>  drivers/iio/adc/Kconfig  |  10 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad7949.c | 329 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 340 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.
>From below, looks like one of them is 4 channel?

> +
> +	  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..667636b476f8
> --- /dev/null
> +++ b/drivers/iio/adc/ad7949.c
> @@ -0,0 +1,329 @@
> +// 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/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/delay.h>

Kernel style is to normally put headers in alphabetical order
if the order isn't conveying some other information.

> +
> +#define AD7949_MASK_CFG			0x3C7F
> +#define AD7949_MASK_CHANNEL_SEL		0x380
> +#define AD7949_MASK_TOTAL		0x3FFF

GENMASK for masks please.

> +#define AD7949_OFFSET_CHANNEL_SEL	7
> +#define AD7949_CFG_READ_BACK		0x1
> +#define AD7949_CFG_REG_SIZE_BITS	14
> +
> +enum {
> +	HEIGHT_14BITS = 0,
> +	QUAD_16BITS,
> +	HEIGHT_16BITS,

Height? I guess EIGHT was the intent.
I would just use the part numbers for this rather than a
descriptive phrase.

> +};
> +
> +struct ad7949_adc_spec {
> +	u8 num_channels;
> +	u8 resolution;
> +};
> +
> +static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> +	[HEIGHT_14BITS]  = { .num_channels = 8, .resolution = 14 },
> +	[QUAD_16BITS] = { .num_channels = 4, .resolution = 16 },
> +	[HEIGHT_16BITS] = { .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
> + * @resolution: resolution of the chip
Good to see kernel-doc style commenting.

Please make sure you document all elements and sanity check it
by running the kernel-doc scripts over the file.

> + */
> +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;
> +};
> +
> +static u16 ad7949_spi_read_cfg(struct ad7949_adc_chip *ad7949_adc)
> +{
> +	return ad7949_adc->cfg;

This seems unnecessary abstraction.

> +}
> +
> +static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> +{
> +	u16 cfg = ad7949_spi_read_cfg(ad7949_adc) & AD7949_MASK_TOTAL;
> +	bool is_cfg_read_back = cfg & AD7949_CFG_READ_BACK ? false : true;

Given this is only used in once place, more readable to just put it in that
if statement and not have the ternary operator.

> +	int ret = ad7949_adc->resolution;
> +
> +	if (is_cfg_read_back)
> +		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;
> +	u16 cfg = ad7949_spi_read_cfg(ad7949_adc) & AD7949_MASK_TOTAL;
> +	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> +	int shift = (bits_per_word - AD7949_CFG_REG_SIZE_BITS);
> +	u32 buf_value = ((val & mask) | (cfg & ~mask)) << shift;

Same problem with this buffer being passed to spi_sync as the
one below. (I tend to review backwards as drivers make more
sense starting from probe and review!)

> +	struct spi_message msg;
> +	struct spi_transfer tx[] = {
> +		{
> +			.tx_buf = &buf_value,
> +			.len = 4,
> +			.bits_per_word = bits_per_word,
> +		},
> +	};
> +
> +	ad7949_adc->cfg = buf_value >> shift;
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&tx[0], &msg);
> +	ret = spi_sync(ad7949_adc->spi, &msg);
> +	udelay(2);

These delays need explaining as they are non obvious and there
may be cleaner ways to handle them.

> +
> +	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 shift = (bits_per_word - AD7949_CFG_REG_SIZE_BITS);
> +	u32 buf_value = 0;

Look very carefully at the requirements for a buffer being passed
to spi_sync.  It needs to be DMA safe.  This one is not.  The usual
way to do that easily is to put a cacheline aligned buffer in your
ad7949_adc_chip structure.

Lots of examples to copy, but it's also worth making sure you understand
why this is necessary.

> +	struct spi_message msg;
> +	struct spi_transfer tx[] = {
> +		{
> +			.rx_buf = &buf_value,
> +			.len = 4,
> +			.bits_per_word = bits_per_word,
> +		},
> +	};

I 'think' this is the same in every call of this function, so why not
set it up in the probe function and have it ready all the time rather than
spinning up a new copy here each time.

> +
> +	if (!val)
> +		return -EINVAL;
> +
> +	ad7949_spi_write_cfg(ad7949_adc, channel << AD7949_OFFSET_CHANNEL_SEL,
> +			     AD7949_MASK_CHANNEL_SEL);
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&tx[0], &msg);
> +	ret = spi_sync(ad7949_adc->spi, &msg);
Why?  A delay after the spi sequence has occurred?
> +	udelay(2);
> +
> +	ad7949_adc->current_channel = channel;
> +	*val = (buf_value >> shift);
Brackets not adding anything so please remove.

A blank line is always nice before a return statement like this. Makes
the code slightly more predictable from a readability point of view.

> +	return ret;
> +}
> +
> +#define AD7949_ADC_CHANNEL(chan) {				\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.scan_index = (chan),					\
> +	.channel = (chan),					\
> +	.address = (chan),					\
Why set all 3 of these to the same thing?

Scan index won't be used by the core unless you support buffered read
modes some time in the future.  Channel is used in the naming, but
if we have the address as the same value, we can read channel just
as well so I would just set that.

> +	.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;
> +
> +	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;
> +
> +		ret = IIO_VAL_INT;
There isn't anything to be done outside the switch statement (no locks
held etc) so just use a direct return here.
return IIO_VAL_INT;

Same in all the other cases.

> +		break;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regulator_get_voltage(ad7949_adc->vref);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret / 5000;
> +		*val2 = 0;

No need to set val2.  The upper layers won't read it anyway.

> +		ret = IIO_VAL_INT;
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +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_spi_read_cfg(ad7949_adc);
> +	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)
> +{
> +	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
> +	ad7949_adc->current_channel = 0;
> +	return ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
> +}
> +
> +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;
> +	spi->mode = SPI_MODE_0;
> +	spi->irq = -1;

That is rather surprising to see. What is the intent of setting
this to -1?

> +	spi->bits_per_word = 8;
> +	spi_setup(spi);
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;

You have a local variable for spi->dev, might as well use
it here.

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

A rule of thumb in the kernel is that if a given function
fails internally it should always exit as if it never happened
at all.  The result here is that you don't want to
call regulator_disable if this function failed, only if a later
function fails after this one succeeds.

> +		goto err_regulator;
> +	}
> +
> +	mutex_init(&ad7949_adc->lock);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(dev, "fail to register iio device: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = ad7949_spi_init(ad7949_adc);

This looks immediately like a race condition to me.
We should not have to do anything to the device after we have called
iio_device_register.  That function has enabled all userspace and
in kernel interfaces so we can be talking to it before we get to this
spi_init call.

Also, note you you don't call iio_device_unregister in this error path.

> +	if (ret) {
> +		dev_err(dev, "enable to init this device: %d\n", ret);
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	mutex_destroy(&ad7949_adc->lock);
> +err_regulator:
> +	regulator_disable(ad7949_adc->vref);
> +	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", HEIGHT_14BITS  },
> +	{ "ad7682", QUAD_16BITS },
> +	{ "ad7689", HEIGHT_16BITS },
> +	{ }
> +};
> +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");

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

[Couret Charles-Antoine]

Le 07/10/2018 à 18:25, Jonathan Cameron a écrit :
> Hi Charles-Antoine, 

Hello,

Thank you for your code review, I'm trying to take everything into 
account but I have some questions about it before making the V2.

>> +#define AD7949_OFFSET_CHANNEL_SEL	7
>> +#define AD7949_CFG_READ_BACK		0x1
>> +#define AD7949_CFG_REG_SIZE_BITS	14
>> +
>> +enum {
>> +	HEIGHT_14BITS = 0,
>> +	QUAD_16BITS,
>> +	HEIGHT_16BITS,
> Height? I guess EIGHT was the intent.
> I would just use the part numbers for this rather than a
> descriptive phrase.

Thank you for the typo.

But I don't understand your remark. What do you mean by "part numbers" 
here?

>> +	struct spi_message msg;
>> +	struct spi_transfer tx[] = {
>> +		{
>> +			.tx_buf = &buf_value,
>> +			.len = 4,
>> +			.bits_per_word = bits_per_word,
>> +		},
>> +	};
>> +
>> +	ad7949_adc->cfg = buf_value >> shift;
>> +	spi_message_init(&msg);
>> +	spi_message_add_tail(&tx[0], &msg);
>> +	ret = spi_sync(ad7949_adc->spi, &msg);
>> +	udelay(2);
> These delays need explaining as they are non obvious and there
> may be cleaner ways to handle them.

I want to add this comment:

     /* This delay is to avoid a new request before the required time to
      * send a new command to the device
      */

It is clear and relevant enough?

>
>> +	struct spi_message msg;
>> +	struct spi_transfer tx[] = {
>> +		{
>> +			.rx_buf = &buf_value,
>> +			.len = 4,
>> +			.bits_per_word = bits_per_word,
>> +		},
>> +	};
> I 'think' this is the same in every call of this function, so why not
> set it up in the probe function and have it ready all the time rather than
> spinning up a new copy here each time.
bits_per_word depends on if the configuration readback is enabled or 
not. It could be changed in runtime. So I considered we can not optimize 
in that way.

Regards,
Charles-Antoine Couret

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

[Ardelean, Alexandru]

T24gTW9uLCAyMDE4LTEwLTA4IGF0IDIzOjE4ICswMjAwLCBDb3VyZXQgQ2hhcmxlcy1BbnRvaW5l
IHdyb3RlOg0KPiBMZSAwNy8xMC8yMDE4IMOgIDE4OjI1LCBKb25hdGhhbiBDYW1lcm9uIGEgw6lj
cml0IDoNCj4gPiBIaSBDaGFybGVzLUFudG9pbmUsIA0KPiANCj4gSGVsbG8sDQo+IA0KPiBUaGFu
ayB5b3UgZm9yIHlvdXIgY29kZSByZXZpZXcsIEknbSB0cnlpbmcgdG8gdGFrZSBldmVyeXRoaW5n
IGludG8gDQo+IGFjY291bnQgYnV0IEkgaGF2ZSBzb21lIHF1ZXN0aW9ucyBhYm91dCBpdCBiZWZv
cmUgbWFraW5nIHRoZSBWMi4NCj4gDQo+ID4gPiArI2RlZmluZSBBRDc5NDlfT0ZGU0VUX0NIQU5O
RUxfU0VMCTcNCj4gPiA+ICsjZGVmaW5lIEFENzk0OV9DRkdfUkVBRF9CQUNLCQkweDENCj4gPiA+
ICsjZGVmaW5lIEFENzk0OV9DRkdfUkVHX1NJWkVfQklUUwkxNA0KPiA+ID4gKw0KPiA+ID4gK2Vu
dW0gew0KPiA+ID4gKwlIRUlHSFRfMTRCSVRTID0gMCwNCj4gPiA+ICsJUVVBRF8xNkJJVFMsDQo+
ID4gPiArCUhFSUdIVF8xNkJJVFMsDQo+ID4gDQo+ID4gSGVpZ2h0PyBJIGd1ZXNzIEVJR0hUIHdh
cyB0aGUgaW50ZW50Lg0KPiA+IEkgd291bGQganVzdCB1c2UgdGhlIHBhcnQgbnVtYmVycyBmb3Ig
dGhpcyByYXRoZXIgdGhhbiBhDQo+ID4gZGVzY3JpcHRpdmUgcGhyYXNlLg0KPiANCj4gVGhhbmsg
eW91IGZvciB0aGUgdHlwby4NCj4gDQo+IEJ1dCBJIGRvbid0IHVuZGVyc3RhbmQgeW91ciByZW1h
cmsuIFdoYXQgZG8geW91IG1lYW4gYnkgInBhcnQgbnVtYmVycyIgDQo+IGhlcmU/DQoNCkEgbG90
IG9mIGRyaXZlcnMgZGVmaW5lIHNvbWV0aGluZyBsaWtlOg0KZW51bSB7DQogICBJRF9BRDc5NDks
DQogICBJRF9BRDc2ODIsDQogICBJRF9BRDc2ODksDQp9DQp3aGljaCBjYW4gYmUgcmVmZXJlZCB0
byBhcyAicGFydCBudW1iZXIiLCBhbmQgdGhlbiB5b3UgY2FuIHVzZSB0aGVzZSBlbnVtDQp2YWx1
ZXMgdG8gaWRlbnRpZnkgYmVoYXZpb3IgYW5kIGNvbmZpZ3VyYXRpb24gZm9yIGVhY2ggZGV2aWNl
IHRoZSBkcml2ZXINCnN1cHBvcnRzLg0KDQpUaGlzIG1ldGhvZCBpcyBwcmVmZXJyZWQsIGJlY2F1
c2Ugd2hlbi9pZiBhIG5ldyBjaGlwIGNvbWVzIGFsb25nIHRoYXQgZml0cw0KaW50byB0aGlzIGRy
aXZlciAobGV0J3Mgc2F5IElEX0FEWFhZWiksIGFuZCBtYXkgaGF2ZSBRVUFEXzE2QklUUyBhbmQN
CmRpZmZlcnMgaW4gc29tZSBvdGhlciBtaW5vciBhc3BlY3QsIGl0IGNhbiBiZSBlYXNpZXIgdG8g
aWRlbnRpZnkgdmlhIHRoZQ0KcGFydC1udW1iZXIuIE9yLCBpbiBzb21lIGNhc2VzLCBzb21lIGNo
aXBzIGdldCBhIG5ld2VyIHJldmlzaW9uIChleGFtcGxlOg0KSURfQUQ3OTQ5QikgdGhhdCBtYXkg
ZGlmZmVyIHNsaWdodGx5IChmcm9tIElEX0FENzk0OSkuDQoNCg0KPiANCj4gPiA+ICsJc3RydWN0
IHNwaV9tZXNzYWdlIG1zZzsNCj4gPiA+ICsJc3RydWN0IHNwaV90cmFuc2ZlciB0eFtdID0gew0K
PiA+ID4gKwkJew0KPiA+ID4gKwkJCS50eF9idWYgPSAmYnVmX3ZhbHVlLA0KPiA+ID4gKwkJCS5s
ZW4gPSA0LA0KPiA+ID4gKwkJCS5iaXRzX3Blcl93b3JkID0gYml0c19wZXJfd29yZCwNCj4gPiA+
ICsJCX0sDQo+ID4gPiArCX07DQo+ID4gPiArDQo+ID4gPiArCWFkNzk0OV9hZGMtPmNmZyA9IGJ1
Zl92YWx1ZSA+PiBzaGlmdDsNCj4gPiA+ICsJc3BpX21lc3NhZ2VfaW5pdCgmbXNnKTsNCj4gPiA+
ICsJc3BpX21lc3NhZ2VfYWRkX3RhaWwoJnR4WzBdLCAmbXNnKTsNCj4gPiA+ICsJcmV0ID0gc3Bp
X3N5bmMoYWQ3OTQ5X2FkYy0+c3BpLCAmbXNnKTsNCj4gPiA+ICsJdWRlbGF5KDIpOw0KPiA+IA0K
PiA+IFRoZXNlIGRlbGF5cyBuZWVkIGV4cGxhaW5pbmcgYXMgdGhleSBhcmUgbm9uIG9idmlvdXMg
YW5kIHRoZXJlDQo+ID4gbWF5IGJlIGNsZWFuZXIgd2F5cyB0byBoYW5kbGUgdGhlbS4NCj4gDQo+
IEkgd2FudCB0byBhZGQgdGhpcyBjb21tZW50Og0KPiANCj4gICAgICAvKiBUaGlzIGRlbGF5IGlz
IHRvIGF2b2lkIGEgbmV3IHJlcXVlc3QgYmVmb3JlIHRoZSByZXF1aXJlZCB0aW1lIHRvDQo+ICAg
ICAgICogc2VuZCBhIG5ldyBjb21tYW5kIHRvIHRoZSBkZXZpY2UNCj4gICAgICAgKi8NCj4gDQo+
IEl0IGlzIGNsZWFyIGFuZCByZWxldmFudCBlbm91Z2g/DQoNCkkgdGhpbmsgaW4gc3VjaCBhIGNh
c2UsIGEgbG9jay9tdXRleCB3b3VsZCBiZSBuZWVkZWQuDQpBcyBmYXIgYXMgSSByZW1lbWJlciwg
a2VybmVsIFNQSSBjYWxscyBzaG91bGQgaGF2ZSB0aGVpciBvd24gbG9ja3MgZm9yDQpzaW5nbGUg
U1BJIHRyYW5zYWN0aW9ucywgc28gbWF5YmUgc29tZSBsb2NrcyBmb3IgYWNjZXNzaW5nIHRoZSBj
aGlwIGR1cmluZw0KYSBzZXQgb2YgU1BJIHRyYW5zYWN0aW9ucyB3b3VsZCBiZSBuZWF0ZXIuDQoN
Cj4gDQo+ID4gDQo+ID4gPiArCXN0cnVjdCBzcGlfbWVzc2FnZSBtc2c7DQo+ID4gPiArCXN0cnVj
dCBzcGlfdHJhbnNmZXIgdHhbXSA9IHsNCj4gPiA+ICsJCXsNCj4gPiA+ICsJCQkucnhfYnVmID0g
JmJ1Zl92YWx1ZSwNCj4gPiA+ICsJCQkubGVuID0gNCwNCj4gPiA+ICsJCQkuYml0c19wZXJfd29y
ZCA9IGJpdHNfcGVyX3dvcmQsDQo+ID4gPiArCQl9LA0KPiA+ID4gKwl9Ow0KPiA+IA0KPiA+IEkg
J3RoaW5rJyB0aGlzIGlzIHRoZSBzYW1lIGluIGV2ZXJ5IGNhbGwgb2YgdGhpcyBmdW5jdGlvbiwg
c28gd2h5IG5vdA0KPiA+IHNldCBpdCB1cCBpbiB0aGUgcHJvYmUgZnVuY3Rpb24gYW5kIGhhdmUg
aXQgcmVhZHkgYWxsIHRoZSB0aW1lIHJhdGhlcg0KPiA+IHRoYW4NCj4gPiBzcGlubmluZyB1cCBh
IG5ldyBjb3B5IGhlcmUgZWFjaCB0aW1lLg0KPiANCj4gYml0c19wZXJfd29yZCBkZXBlbmRzIG9u
IGlmIHRoZSBjb25maWd1cmF0aW9uIHJlYWRiYWNrIGlzIGVuYWJsZWQgb3IgDQo+IG5vdC4gSXQg
Y291bGQgYmUgY2hhbmdlZCBpbiBydW50aW1lLiBTbyBJIGNvbnNpZGVyZWQgd2UgY2FuIG5vdCBv
cHRpbWl6ZSANCj4gaW4gdGhhdCB3YXkuDQo+IA0KDQpBbGV4DQoNCj4gUmVnYXJkcywNCj4gQ2hh
cmxlcy1BbnRvaW5lIENvdXJldA0KPiANCg==

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

[Couret Charles-Antoine]

Le 09/10/2018 à 08:25, Ardelean, Alexandru a écrit :
>
>>>> +#define AD7949_OFFSET_CHANNEL_SEL	7
>>>> +#define AD7949_CFG_READ_BACK		0x1
>>>> +#define AD7949_CFG_REG_SIZE_BITS	14
>>>> +
>>>> +enum {
>>>> +	HEIGHT_14BITS = 0,
>>>> +	QUAD_16BITS,
>>>> +	HEIGHT_16BITS,
>>> Height? I guess EIGHT was the intent.
>>> I would just use the part numbers for this rather than a
>>> descriptive phrase.
>> Thank you for the typo.
>>
>> But I don't understand your remark. What do you mean by "part numbers"
>> here?
> A lot of drivers define something like:
> enum {
>     ID_AD7949,
>     ID_AD7682,
>     ID_AD7689,
> }
> which can be refered to as "part number", and then you can use these enum
> values to identify behavior and configuration for each device the driver
> supports.
>
> This method is preferred, because when/if a new chip comes along that fits
> into this driver (let's say ID_ADXXYZ), and may have QUAD_16BITS and
> differs in some other minor aspect, it can be easier to identify via the
> part-number. Or, in some cases, some chips get a newer revision (example:
> ID_AD7949B) that may differ slightly (from ID_AD7949).
Ok, I understand, thank you for the explanation.
>>>> +	struct spi_message msg;
>>>> +	struct spi_transfer tx[] = {
>>>> +		{
>>>> +			.tx_buf = &buf_value,
>>>> +			.len = 4,
>>>> +			.bits_per_word = bits_per_word,
>>>> +		},
>>>> +	};
>>>> +
>>>> +	ad7949_adc->cfg = buf_value >> shift;
>>>> +	spi_message_init(&msg);
>>>> +	spi_message_add_tail(&tx[0], &msg);
>>>> +	ret = spi_sync(ad7949_adc->spi, &msg);
>>>> +	udelay(2);
>>> These delays need explaining as they are non obvious and there
>>> may be cleaner ways to handle them.
>> I want to add this comment:
>>
>>       /* This delay is to avoid a new request before the required time to
>>        * send a new command to the device
>>        */
>>
>> It is clear and relevant enough?
> I think in such a case, a lock/mutex would be needed.
> As far as I remember, kernel SPI calls should have their own locks for
> single SPI transactions, so maybe some locks for accessing the chip during
> a set of SPI transactions would be neater.

The mutex is used in parent level (the functions which make the link 
between userspace and this function). It seems enough for me.

In that case the purpose of the delay is only to avoid a new request 
just after this one which will fail because too early for the device. It 
is just a timing protection, it is not uncommon from my point of view.


Regards,

Charles-Antoine Couret

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

[Jonathan Cameron]

On Tue, 9 Oct 2018 09:12:35 +0200
Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:

> Le 09/10/2018 à 08:25, Ardelean, Alexandru a écrit :
> >  
> >>>> +#define AD7949_OFFSET_CHANNEL_SEL	7
> >>>> +#define AD7949_CFG_READ_BACK		0x1
> >>>> +#define AD7949_CFG_REG_SIZE_BITS	14
> >>>> +
> >>>> +enum {
> >>>> +	HEIGHT_14BITS = 0,
> >>>> +	QUAD_16BITS,
> >>>> +	HEIGHT_16BITS,  
> >>> Height? I guess EIGHT was the intent.
> >>> I would just use the part numbers for this rather than a
> >>> descriptive phrase.  
> >> Thank you for the typo.
> >>
> >> But I don't understand your remark. What do you mean by "part numbers"
> >> here?  
> > A lot of drivers define something like:
> > enum {
> >     ID_AD7949,
> >     ID_AD7682,
> >     ID_AD7689,
> > }
> > which can be refered to as "part number", and then you can use these enum
> > values to identify behavior and configuration for each device the driver
> > supports.
> >
> > This method is preferred, because when/if a new chip comes along that fits
> > into this driver (let's say ID_ADXXYZ), and may have QUAD_16BITS and
> > differs in some other minor aspect, it can be easier to identify via the
> > part-number. Or, in some cases, some chips get a newer revision (example:
> > ID_AD7949B) that may differ slightly (from ID_AD7949).  
> Ok, I understand, thank you for the explanation.
> >>>> +	struct spi_message msg;
> >>>> +	struct spi_transfer tx[] = {
> >>>> +		{
> >>>> +			.tx_buf = &buf_value,
> >>>> +			.len = 4,
> >>>> +			.bits_per_word = bits_per_word,
> >>>> +		},
> >>>> +	};
> >>>> +
> >>>> +	ad7949_adc->cfg = buf_value >> shift;
> >>>> +	spi_message_init(&msg);
> >>>> +	spi_message_add_tail(&tx[0], &msg);
> >>>> +	ret = spi_sync(ad7949_adc->spi, &msg);
> >>>> +	udelay(2);  
> >>> These delays need explaining as they are non obvious and there
> >>> may be cleaner ways to handle them.  
> >> I want to add this comment:
> >>
> >>       /* This delay is to avoid a new request before the required time to
> >>        * send a new command to the device
> >>        */
> >>
> >> It is clear and relevant enough?  
> > I think in such a case, a lock/mutex would be needed.
> > As far as I remember, kernel SPI calls should have their own locks for
> > single SPI transactions, so maybe some locks for accessing the chip during
> > a set of SPI transactions would be neater.  
> 
> The mutex is used in parent level (the functions which make the link 
> between userspace and this function). It seems enough for me.
> 
> In that case the purpose of the delay is only to avoid a new request 
> just after this one which will fail because too early for the device. It 
> is just a timing protection, it is not uncommon from my point of view.
This is fine (with the comment).  There has always been a comment in
spi.h suggesting that we could potentially move such timing constraints
into the protocol handling rather than individual drivers. 

It is a very short delay so it is probably not a problem to insert
it before reporting the requested value.  If it had been longer we would
have wanted to store a timestamp here and only force a sleep on the
following command if necessary, rather than always inserting a delay here.

Thanks,

Jonathan
> 
> 
> Regards,
> 
> Charles-Antoine Couret
>