[PATCH v11 0/8] iio: adi-axi-adc,ad9647: Add support for AD9467 ADC

[PATCH v11 0/8] iio: adi-axi-adc,ad9647: Add support for AD9467 ADC

[Alexandru Ardelean]

Changelog v10 -> v11:
* added 'Reviewed-by: Rob Herring <robh@kernel.org>' tag to DT bindings patches
* patch 'dt-bindings: iio: adc: add bindings doc for AXI ADC driver'
  removed 'maxItems' from dma-names property
* sent from an @analog.com server; author was showing as @gmail.com on
  V9 & V10

Changelog v9 -> v10:
* patch 'iio: adc: adi-axi-adc: add support for AXI ADC IP core'
  - removed IQ correction logic; the AD9467 ADC driver has only 1 channel,
    so it can't have I & Q; also the IQ correction assumes that all
    even channels are Q and all odd channels are I, which is true for
    current ADI-AXI ADC IP cores, but shouldn't be an assumption designed
    in the driver; the IQ correction stuff will be re-added later,
    and will try to use the IIO_MOD_I & IIO_MOD_Q modifiers

Changelog v8 -> v9:
* adding more Analog people to the list; predominantly HDL people; this
  should help me sync people about the details of regs/reg-names
* added 'Acked-by: Moritz Fischer <mdf@kernel.org>' tag to fpga patches
  - we can always re-update these patches if something else is decided about
    the location of the 'adi-axi-common.h' header; I'm not insisting about
    where to put it; I'm open to other proposals
* patch 'iio: adc: adi-axi-adc: add support for AXI ADC IP core'
  - prefixed regs ADI_AXI_ ; I tried ADI_AXI_ADC_, but that seemed to make
    them too long
  - dropped unused regs; will add them as stuff gets added in the upstream
    driver; in the meantime, reg-names can be reworked
  - dropped generic LOWERXY_SET/GET macros
  - update reg-names a bit; will update them in the docs and HDL
  - order in adi_axi_adc_conv_unregister() should now be symmetrically
    oppposite now to the register function
  - implemented 'is_visible()' callback to adi_axi_adc_attributes[] so that
    attrs can be made invisible to userspace if needed;
  - 'indio_dev->name = "adi-axi-adc";'
  - added kernel doc-string for @reg_access
* patch 'iio: adc: ad9467: add support AD9467 ADC'
  - ad9467_spi_read() split in 2 buffers; tbuf & rbuf
  - removed 'if (chan->extend_name)' test ; left-over from initial driver
  - removed 'if (!st->clk)' check; driver will fail probe without a clock
  - removed 'if (!spi->dev.of_node)' in probe; shouldn't be needed
  - using 'of_device_get_match_data()' in probe to get data; moved chip
    info table entry as data on the of_device_id table

Changelog v7 -> v8:
* in 'iio: adc: adi-axi-adc: add support for AXI ADC IP core'
  - updated register definitions and bits to newer format/docs; the ref driver wasn't really up-to-date
    -- prefixed bit names with reg-name to avoid bit definition colisions; that makes some macros longer, but at least the format is consistent
  - using dev_name(&pdev->dev) for indio_dev->name
  - moved reset to own axi_adc_reset() function; may be re-used later
  - some re-formatting/alignment changes
  - address ENOSYS checkpatch complaint; changed with EOPNOTSUPP

Changelog v6 -> v7:
* Fixed dt-schema build for adi,axi-adc.yaml based on Rob's suggestion
  - added '$ref: /schemas/types.yaml#/definitions/phandle' to 'adi,adc-dev'
  - dropped 'maxItems' from 'adi,adc-dev'

Changelog v5 -> v6
* fix URLs; got changed during rename
   https://wiki.analog.com/resources/fpga/docs/adi_axi_adc_ip ->
   https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
  - noticed while working on the AXI DAC driver

Changelog v4 -> v5:
* update drivers/iio/adc/Kconfig note about module name; omitted during first rename
   - 'module will be called axi-adc.' -> 'module will be called adi-axi-adc.'

Changelog v3 -> v4:
* addressed Rob's dt-remarks
   - change 'adi-axi-adc-client' prop to 'adi,adc-dev'

Changelog v2 -> v3:
* addressed compiler warning

Changelog v1 -> v2:
* first series was added a bit hastily
* addressed  'make dt_binding_check' complaints; seems I missed a few when running the check;
* added missing patches to include/linux/fpga/adi-axi-common.h
   - 'include: fpga: adi-axi-common.h: fixup whitespace tab -> space'
   - 'include: fpga: adi-axi-common.h: add version helper macros'
* patch 'iio: buffer-dmaengine: add dev-managed calls for buffer alloc/free'
   - remove copy+pasted comment for 'devm_iio_dmaengine_buffer_alloc()'
   - removed devm_iio_dmaengine_buffer_free() ; hopefully it might never be needed
   - fix-up alignment for devm_iio_dmaengine_buffer_alloc() in header
* patch 'iio: adc: adi-axi-adc: add support for AXI ADC IP core'
   - renamed axi-adc.c -> adi-axi-adc.c & Kconfig symbol
   - prefix all axi_adc -> adi_axi_adc
   - removed switch statement in axi_adc_read_raw() & axi_adc_write_raw()
   - remove axi_adc_chan_spec ; replaced with iio_chan_spec directly ; will think of a simpler solution for extra chan params
   - removed left-over 'struct axi_adc_cleanup_data'
   - moved 'devm_add_action_or_reset()' call right after 'adi_axi_adc_attach_client()'
   - switched to using 'devm_platform_ioremap_resource()'
* patch 'iio: adc: ad9467: add support AD9467 ADC'
  - renamed ADI_ADC reg prefixes to AN877_ADC
  - dropped 'info_mask_separate' field in AD9467_CHAN - will be re-added later when driver gets more features; was left-over from the initial ref driver
  - remove .shift = 0,  in AD9467_CHAN
  - renamed 'sample-clock' -> 'adc-clock'
  - direct returns in ad9467_read_raw() & ad9467_write_raw() & ad9467_setup() switch statements
  - removed blank line after devm_axi_adc_conv_register()
  - removed ad9467_id & reworked to use ad9467_of_match

Alexandru Ardelean (6):
  include: fpga: adi-axi-common.h: fixup whitespace tab -> space
  include: fpga: adi-axi-common.h: add version helper macros
  iio: buffer-dmaengine: use %zu specifier for sprintf(align)
  iio: buffer-dmaengine: add dev-managed calls for buffer alloc
  dt-bindings: iio: adc: add bindings doc for AXI ADC driver
  dt-bindings: iio: adc: add bindings doc for AD9467 ADC

Michael Hennerich (2):
  iio: adc: adi-axi-adc: add support for AXI ADC IP core
  iio: adc: ad9467: add support AD9467 ADC

 .../bindings/iio/adc/adi,ad9467.yaml          |  65 +++
 .../bindings/iio/adc/adi,axi-adc.yaml         |  62 +++
 drivers/iio/adc/Kconfig                       |  35 ++
 drivers/iio/adc/Makefile                      |   2 +
 drivers/iio/adc/ad9467.c                      | 420 +++++++++++++++
 drivers/iio/adc/adi-axi-adc.c                 | 495 ++++++++++++++++++
 .../buffer/industrialio-buffer-dmaengine.c    |  41 +-
 include/linux/fpga/adi-axi-common.h           |   6 +-
 include/linux/iio/adc/adi-axi-adc.h           |  64 +++
 include/linux/iio/buffer-dmaengine.h          |   3 +
 10 files changed, 1191 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
 create mode 100644 drivers/iio/adc/ad9467.c
 create mode 100644 drivers/iio/adc/adi-axi-adc.c
 create mode 100644 include/linux/iio/adc/adi-axi-adc.h

-- 
2.17.1

[PATCH v11 1/8] include: fpga: adi-axi-common.h: fixup whitespace tab -> space

[Alexandru Ardelean]

The initial version use a tab between '#define' & 'ADI_AXI_REG_VERSION'.
This changes it to space. The change is purely cosmetic.

Acked-by: Moritz Fischer <mdf@kernel.org>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 include/linux/fpga/adi-axi-common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/fpga/adi-axi-common.h b/include/linux/fpga/adi-axi-common.h
index 7fc95d5c95bb..ebd4e07ae3d8 100644
--- a/include/linux/fpga/adi-axi-common.h
+++ b/include/linux/fpga/adi-axi-common.h
@@ -11,7 +11,7 @@
 #ifndef ADI_AXI_COMMON_H_
 #define ADI_AXI_COMMON_H_
 
-#define	ADI_AXI_REG_VERSION			0x0000
+#define ADI_AXI_REG_VERSION			0x0000
 
 #define ADI_AXI_PCORE_VER(major, minor, patch)	\
 	(((major) << 16) | ((minor) << 8) | (patch))
-- 
2.17.1

[PATCH v11 2/8] include: fpga: adi-axi-common.h: add version helper macros

[Alexandru Ardelean]

The format for all ADI AXI IP cores is the same.
i.e. 'major.minor.patch'.

This patch adds the helper macros to be re-used in ADI AXI drivers.

Acked-by: Moritz Fischer <mdf@kernel.org>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 include/linux/fpga/adi-axi-common.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/fpga/adi-axi-common.h b/include/linux/fpga/adi-axi-common.h
index ebd4e07ae3d8..141ac3f251e6 100644
--- a/include/linux/fpga/adi-axi-common.h
+++ b/include/linux/fpga/adi-axi-common.h
@@ -16,4 +16,8 @@
 #define ADI_AXI_PCORE_VER(major, minor, patch)	\
 	(((major) << 16) | ((minor) << 8) | (patch))
 
+#define ADI_AXI_PCORE_VER_MAJOR(version)	(((version) >> 16) & 0xff)
+#define ADI_AXI_PCORE_VER_MINOR(version)	(((version) >> 8) & 0xff)
+#define ADI_AXI_PCORE_VER_PATCH(version)	((version) & 0xff)
+
 #endif /* ADI_AXI_COMMON_H_ */
-- 
2.17.1

[PATCH v11 3/8] iio: buffer-dmaengine: use %zu specifier for sprintf(align)

[Alexandru Ardelean]

The 'size_t' type behaves differently on 64-bit architectures, and causes
compiler a warning of the sort "format '%u' expects argument of type
'unsigned int', but argument 3 has type 'size_t {aka long unsigned int}'".

This change adds the correct specifier for the 'align' field.

Fixes: 4538c18568099 ("iio: buffer-dmaengine: Report buffer length requirements")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/buffer/industrialio-buffer-dmaengine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index b129693af0fd..94da3b1ca3a2 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -134,7 +134,7 @@ static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
 	struct dmaengine_buffer *dmaengine_buffer =
 		iio_buffer_to_dmaengine_buffer(indio_dev->buffer);
 
-	return sprintf(buf, "%u\n", dmaengine_buffer->align);
+	return sprintf(buf, "%zu\n", dmaengine_buffer->align);
 }
 
 static IIO_DEVICE_ATTR(length_align_bytes, 0444,
-- 
2.17.1

[PATCH v11 4/8] iio: buffer-dmaengine: add dev-managed calls for buffer alloc

[Alexandru Ardelean]

Currently, when using a 'iio_dmaengine_buffer_alloc()', an matching call to
'iio_dmaengine_buffer_free()' must be made.

With this change, this can be avoided by using
'devm_iio_dmaengine_buffer_alloc()'. The buffer will get free'd via the
device's devres handling.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 .../buffer/industrialio-buffer-dmaengine.c    | 39 +++++++++++++++++++
 include/linux/iio/buffer-dmaengine.h          |  3 ++
 2 files changed, 42 insertions(+)

diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index 94da3b1ca3a2..6dedf12b69a4 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -229,6 +229,45 @@ void iio_dmaengine_buffer_free(struct iio_buffer *buffer)
 }
 EXPORT_SYMBOL_GPL(iio_dmaengine_buffer_free);
 
+static void __devm_iio_dmaengine_buffer_free(struct device *dev, void *res)
+{
+	iio_dmaengine_buffer_free(*(struct iio_buffer **)res);
+}
+
+/**
+ * devm_iio_dmaengine_buffer_alloc() - Resource-managed iio_dmaengine_buffer_alloc()
+ * @dev: Parent device for the buffer
+ * @channel: DMA channel name, typically "rx".
+ *
+ * This allocates a new IIO buffer which internally uses the DMAengine framework
+ * to perform its transfers. The parent device will be used to request the DMA
+ * channel.
+ *
+ * The buffer will be automatically de-allocated once the device gets destroyed.
+ */
+struct iio_buffer *devm_iio_dmaengine_buffer_alloc(struct device *dev,
+	const char *channel)
+{
+	struct iio_buffer **bufferp, *buffer;
+
+	bufferp = devres_alloc(__devm_iio_dmaengine_buffer_free,
+			       sizeof(*bufferp), GFP_KERNEL);
+	if (!bufferp)
+		return ERR_PTR(-ENOMEM);
+
+	buffer = iio_dmaengine_buffer_alloc(dev, channel);
+	if (IS_ERR(buffer)) {
+		devres_free(bufferp);
+		return buffer;
+	}
+
+	*bufferp = buffer;
+	devres_add(dev, bufferp);
+
+	return buffer;
+}
+EXPORT_SYMBOL_GPL(devm_iio_dmaengine_buffer_alloc);
+
 MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
 MODULE_DESCRIPTION("DMA buffer for the IIO framework");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/iio/buffer-dmaengine.h b/include/linux/iio/buffer-dmaengine.h
index b3a57444a886..0e503db71289 100644
--- a/include/linux/iio/buffer-dmaengine.h
+++ b/include/linux/iio/buffer-dmaengine.h
@@ -14,4 +14,7 @@ struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
 	const char *channel);
 void iio_dmaengine_buffer_free(struct iio_buffer *buffer);
 
+struct iio_buffer *devm_iio_dmaengine_buffer_alloc(struct device *dev,
+						   const char *channel);
+
 #endif
-- 
2.17.1

[PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Alexandru Ardelean]

From: Michael Hennerich <michael.hennerich@analog.com>

This change adds support for the Analog Devices Generic AXI ADC IP core.
The IP core is used for interfacing with analog-to-digital (ADC) converters
that require either a high-speed serial interface (JESD204B/C) or a source
synchronous parallel interface (LVDS/CMOS).

Usually, some other interface type (i.e SPI) is used as a control interface
for the actual ADC, while the IP core (controlled via this driver), will
interface to the data-lines of the ADC and handle  the streaming of data
into memory via DMA.

Because of this, the AXI ADC driver needs the other SPI-ADC driver to
register with it. The SPI-ADC needs to be register via the SPI framework,
while the AXI ADC registers as a platform driver. The two cannot be ordered
in a hierarchy as both drivers have their own registers, and trying to
organize this [in a hierarchy becomes] problematic when trying to map
memory/registers.

There are some modes where the AXI ADC can operate as standalone ADC, but
those will be implemented at a later point in time.

Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/Kconfig             |  20 ++
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/adi-axi-adc.c       | 495 ++++++++++++++++++++++++++++
 include/linux/iio/adc/adi-axi-adc.h |  64 ++++
 4 files changed, 580 insertions(+)
 create mode 100644 drivers/iio/adc/adi-axi-adc.c
 create mode 100644 include/linux/iio/adc/adi-axi-adc.h

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f4da821c4022..445070abf376 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -246,6 +246,26 @@ config AD799X
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad799x.
 
+config ADI_AXI_ADC
+	tristate "Analog Devices Generic AXI ADC IP core driver"
+	select IIO_BUFFER
+	select IIO_BUFFER_HW_CONSUMER
+	select IIO_BUFFER_DMAENGINE
+	help
+	  Say yes here to build support for Analog Devices Generic
+	  AXI ADC IP core. The IP core is used for interfacing with
+	  analog-to-digital (ADC) converters that require either a high-speed
+	  serial interface (JESD204B/C) or a source synchronous parallel
+	  interface (LVDS/CMOS).
+	  Typically (for such devices) SPI will be used for configuration only,
+	  while this IP core handles the streaming of data into memory via DMA.
+
+	  Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
+	  If unsure, say N (but it's safe to say "Y").
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called adi-axi-adc.
+
 config ASPEED_ADC
 	tristate "Aspeed ADC"
 	depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 8462455b4228..7c6594d049f9 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
 obj-$(CONFIG_AD7887) += ad7887.o
 obj-$(CONFIG_AD7949) += ad7949.o
 obj-$(CONFIG_AD799X) += ad799x.o
+obj-$(CONFIG_ADI_AXI_ADC) += adi-axi-adc.o
 obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
 obj-$(CONFIG_AT91_ADC) += at91_adc.o
 obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
new file mode 100644
index 000000000000..8d966b47edc9
--- /dev/null
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -0,0 +1,495 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices Generic AXI ADC IP core
+ * Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
+ *
+ * Copyright 2012-2020 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/buffer-dmaengine.h>
+
+#include <linux/fpga/adi-axi-common.h>
+#include <linux/iio/adc/adi-axi-adc.h>
+
+/**
+ * Register definitions:
+ *   https://wiki.analog.com/resources/fpga/docs/axi_adc_ip#register_map
+ */
+
+/* ADC controls */
+
+#define ADI_AXI_REG_RSTN			0x0040
+#define   ADI_AXI_REG_RSTN_CE_N			BIT(2)
+#define   ADI_AXI_REG_RSTN_MMCM_RSTN		BIT(1)
+#define   ADI_AXI_REG_RSTN_RSTN			BIT(0)
+
+/* ADC Channel controls */
+
+#define ADI_AXI_REG_CHAN_CTRL(c)		(0x0400 + (c) * 0x40)
+#define   ADI_AXI_REG_CHAN_CTRL_LB_OWR		BIT(11)
+#define   ADI_AXI_REG_CHAN_CTRL_PN_SEL_OWR	BIT(10)
+#define   ADI_AXI_REG_CHAN_CTRL_IQCOR_EN	BIT(9)
+#define   ADI_AXI_REG_CHAN_CTRL_DCFILT_EN	BIT(8)
+#define   ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT	BIT(6)
+#define   ADI_AXI_REG_CHAN_CTRL_FMT_TYPE	BIT(5)
+#define   ADI_AXI_REG_CHAN_CTRL_FMT_EN		BIT(4)
+#define   ADI_AXI_REG_CHAN_CTRL_PN_TYPE_OWR	BIT(1)
+#define   ADI_AXI_REG_CHAN_CTRL_ENABLE		BIT(0)
+
+#define ADI_AXI_REG_CHAN_CTRL_DEFAULTS		\
+	(ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT |	\
+	 ADI_AXI_REG_CHAN_CTRL_FMT_EN |		\
+	 ADI_AXI_REG_CHAN_CTRL_ENABLE)
+
+struct adi_axi_adc_core_info {
+	unsigned int				version;
+};
+
+struct adi_axi_adc_state {
+	struct mutex				lock;
+
+	struct adi_axi_adc_client		*client;
+	void __iomem				*regs;
+};
+
+struct adi_axi_adc_client {
+	struct list_head			entry;
+	struct adi_axi_adc_conv			conv;
+	struct adi_axi_adc_state		*state;
+	struct device				*dev;
+	const struct adi_axi_adc_core_info	*info;
+};
+
+static LIST_HEAD(registered_clients);
+static DEFINE_MUTEX(registered_clients_lock);
+
+static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv *conv)
+{
+	if (!conv)
+		return NULL;
+	return container_of(conv, struct adi_axi_adc_client, conv);
+}
+
+void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
+{
+	struct adi_axi_adc_client *cl = conv_to_client(conv);
+
+	if (!cl)
+		return NULL;
+
+	return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client), IIO_ALIGN);
+}
+EXPORT_SYMBOL_GPL(adi_axi_adc_conv_priv);
+
+static void adi_axi_adc_write(struct adi_axi_adc_state *st,
+			      unsigned int reg,
+			      unsigned int val)
+{
+	iowrite32(val, st->regs + reg);
+}
+
+static unsigned int adi_axi_adc_read(struct adi_axi_adc_state *st,
+				     unsigned int reg)
+{
+	return ioread32(st->regs + reg);
+}
+
+static int adi_axi_adc_config_dma_buffer(struct device *dev,
+					 struct iio_dev *indio_dev)
+{
+	struct iio_buffer *buffer;
+	const char *dma_name;
+
+	if (!device_property_present(dev, "dmas"))
+		return 0;
+
+	if (device_property_read_string(dev, "dma-names", &dma_name))
+		dma_name = "rx";
+
+	buffer = devm_iio_dmaengine_buffer_alloc(indio_dev->dev.parent,
+						 dma_name);
+	if (IS_ERR(buffer))
+		return PTR_ERR(buffer);
+
+	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
+	iio_device_attach_buffer(indio_dev, buffer);
+
+	return 0;
+}
+
+static int adi_axi_adc_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct adi_axi_adc_state *st = iio_priv(indio_dev);
+	struct adi_axi_adc_conv *conv = &st->client->conv;
+
+	if (!conv->read_raw)
+		return -EOPNOTSUPP;
+
+	return conv->read_raw(conv, chan, val, val2, mask);
+}
+
+static int adi_axi_adc_write_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int val, int val2, long mask)
+{
+	struct adi_axi_adc_state *st = iio_priv(indio_dev);
+	struct adi_axi_adc_conv *conv = &st->client->conv;
+
+	if (!conv->write_raw)
+		return -EOPNOTSUPP;
+
+	return conv->write_raw(conv, chan, val, val2, mask);
+}
+
+static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
+					const unsigned long *scan_mask)
+{
+	struct adi_axi_adc_state *st = iio_priv(indio_dev);
+	struct adi_axi_adc_conv *conv = &st->client->conv;
+	unsigned int i, ctrl;
+
+	for (i = 0; i < conv->chip_info->num_channels; i++) {
+		ctrl = adi_axi_adc_read(st, ADI_AXI_REG_CHAN_CTRL(i));
+
+		if (test_bit(i, scan_mask))
+			ctrl |= ADI_AXI_REG_CHAN_CTRL_ENABLE;
+		else
+			ctrl &= ~ADI_AXI_REG_CHAN_CTRL_ENABLE;
+
+		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i), ctrl);
+	}
+
+	return 0;
+}
+
+static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device *dev,
+							  int sizeof_priv)
+{
+	struct adi_axi_adc_client *cl;
+	size_t alloc_size;
+
+	alloc_size = sizeof(struct adi_axi_adc_client);
+	if (sizeof_priv) {
+		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
+		alloc_size += sizeof_priv;
+	}
+	alloc_size += IIO_ALIGN - 1;
+
+	cl = kzalloc(alloc_size, GFP_KERNEL);
+	if (!cl)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&registered_clients_lock);
+
+	get_device(dev);
+	cl->dev = dev;
+
+	list_add_tail(&cl->entry, &registered_clients);
+
+	mutex_unlock(&registered_clients_lock);
+
+	return &cl->conv;
+}
+
+static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
+{
+	struct adi_axi_adc_client *cl = conv_to_client(conv);
+
+	if (!cl)
+		return;
+
+	mutex_lock(&registered_clients_lock);
+
+	list_del(&cl->entry);
+	put_device(cl->dev);
+
+	mutex_unlock(&registered_clients_lock);
+
+	kfree(cl);
+}
+
+static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
+{
+	adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);
+}
+
+struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
+							int sizeof_priv)
+{
+	struct adi_axi_adc_conv **ptr, *conv;
+
+	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
+	if (IS_ERR(conv)) {
+		devres_free(ptr);
+		return ERR_CAST(conv);
+	}
+
+	*ptr = conv;
+	devres_add(dev, ptr);
+
+	return conv;
+}
+EXPORT_SYMBOL_GPL(devm_adi_axi_adc_conv_register);
+
+static ssize_t in_voltage_scale_available_show(struct device *dev,
+					       struct device_attribute *attr,
+					       char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct adi_axi_adc_state *st = iio_priv(indio_dev);
+	struct adi_axi_adc_conv *conv = &st->client->conv;
+	size_t len = 0;
+	int i;
+
+	for (i = 0; i < conv->chip_info->num_scales; i++) {
+		const unsigned int *s = conv->chip_info->scale_table[i];
+
+		len += scnprintf(buf + len, PAGE_SIZE - len,
+				 "%u.%06u ", s[0], s[1]);
+	}
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
+
+enum {
+	ADI_AXI_ATTR_SCALE_AVAIL,
+};
+
+#define ADI_AXI_ATTR(_en_, _file_)			\
+	[ADI_AXI_ATTR_##_en_] = &iio_dev_attr_##_file_.dev_attr.attr
+
+static struct attribute *adi_axi_adc_attributes[] = {
+	ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
+	NULL,
+};
+
+static umode_t axi_adc_attr_is_visible(struct kobject *kobj,
+				       struct attribute *attr, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct adi_axi_adc_state *st = iio_priv(indio_dev);
+	struct adi_axi_adc_conv *conv = &st->client->conv;
+
+	switch (n) {
+	case ADI_AXI_ATTR_SCALE_AVAIL:
+		if (!conv->chip_info->num_scales)
+			return 0;
+		return attr->mode;
+	default:
+		return attr->mode;
+	}
+}
+
+static const struct attribute_group adi_axi_adc_attribute_group = {
+	.attrs = adi_axi_adc_attributes,
+	.is_visible = axi_adc_attr_is_visible,
+};
+
+static const struct iio_info adi_axi_adc_info = {
+	.read_raw = &adi_axi_adc_read_raw,
+	.write_raw = &adi_axi_adc_write_raw,
+	.attrs = &adi_axi_adc_attribute_group,
+	.update_scan_mode = &adi_axi_adc_update_scan_mode,
+};
+
+static const struct adi_axi_adc_core_info adi_axi_adc_10_0_a_info = {
+	.version = ADI_AXI_PCORE_VER(10, 0, 'a'),
+};
+
+/* Match table for of_platform binding */
+static const struct of_device_id adi_axi_adc_of_match[] = {
+	{ .compatible = "adi,axi-adc-10.0.a", .data = &adi_axi_adc_10_0_a_info },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, adi_axi_adc_of_match);
+
+struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)
+{
+	const struct of_device_id *id;
+	struct adi_axi_adc_client *cl;
+	struct device_node *cln;
+
+	if (!dev->of_node) {
+		dev_err(dev, "DT node is null\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	id = of_match_node(adi_axi_adc_of_match, dev->of_node);
+	if (!id)
+		return ERR_PTR(-ENODEV);
+
+	cln = of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
+	if (!cln) {
+		dev_err(dev, "No 'adi,adc-dev' node defined\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	mutex_lock(&registered_clients_lock);
+
+	list_for_each_entry(cl, &registered_clients, entry) {
+		if (!cl->dev)
+			continue;
+		if (cl->dev->of_node == cln) {
+			if (!try_module_get(dev->driver->owner)) {
+				mutex_unlock(&registered_clients_lock);
+				return ERR_PTR(-ENODEV);
+			}
+			get_device(dev);
+			cl->info = id->data;
+			mutex_unlock(&registered_clients_lock);
+			return cl;
+		}
+	}
+
+	mutex_unlock(&registered_clients_lock);
+
+	return ERR_PTR(-EPROBE_DEFER);
+}
+
+static int adi_axi_adc_setup_channels(struct device *dev,
+				      struct adi_axi_adc_state *st)
+{
+	struct adi_axi_adc_conv *conv = conv = &st->client->conv;
+	int i, ret;
+
+	if (conv->preenable_setup) {
+		ret = conv->preenable_setup(conv);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < conv->chip_info->num_channels; i++) {
+		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i),
+				  ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
+	}
+
+	return 0;
+}
+
+static void axi_adc_reset(struct adi_axi_adc_state *st)
+{
+	adi_axi_adc_write(st, ADI_AXI_REG_RSTN, 0);
+	mdelay(10);
+	adi_axi_adc_write(st, ADI_AXI_REG_RSTN, ADI_AXI_REG_RSTN_MMCM_RSTN);
+	mdelay(10);
+	adi_axi_adc_write(st, ADI_AXI_REG_RSTN,
+			  ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
+}
+
+static void adi_axi_adc_cleanup(void *data)
+{
+	struct adi_axi_adc_client *cl = data;
+
+	put_device(cl->dev);
+	module_put(cl->dev->driver->owner);
+}
+
+static int adi_axi_adc_probe(struct platform_device *pdev)
+{
+	struct adi_axi_adc_conv *conv;
+	struct iio_dev *indio_dev;
+	struct adi_axi_adc_client *cl;
+	struct adi_axi_adc_state *st;
+	unsigned int ver;
+	int ret;
+
+	cl = adi_axi_adc_attach_client(&pdev->dev);
+	if (IS_ERR(cl))
+		return PTR_ERR(cl);
+
+	ret = devm_add_action_or_reset(&pdev->dev, adi_axi_adc_cleanup, cl);
+	if (ret)
+		return ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->client = cl;
+	cl->state = st;
+	mutex_init(&st->lock);
+
+	st->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(st->regs))
+		return PTR_ERR(st->regs);
+
+	conv = &st->client->conv;
+
+	axi_adc_reset(st);
+
+	ver = adi_axi_adc_read(st, ADI_AXI_REG_VERSION);
+
+	if (cl->info->version > ver) {
+		dev_err(&pdev->dev,
+			"IP core version is too old. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
+			ADI_AXI_PCORE_VER_MAJOR(cl->info->version),
+			ADI_AXI_PCORE_VER_MINOR(cl->info->version),
+			ADI_AXI_PCORE_VER_PATCH(cl->info->version),
+			ADI_AXI_PCORE_VER_MAJOR(ver),
+			ADI_AXI_PCORE_VER_MINOR(ver),
+			ADI_AXI_PCORE_VER_PATCH(ver));
+		return -ENODEV;
+	}
+
+	indio_dev->info = &adi_axi_adc_info;
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->name = "adi-axi-adc";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = conv->chip_info->num_channels;
+	indio_dev->channels = conv->chip_info->channels;
+
+	ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = adi_axi_adc_setup_channels(&pdev->dev, st);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n",
+		 ADI_AXI_PCORE_VER_MAJOR(ver),
+		 ADI_AXI_PCORE_VER_MINOR(ver),
+		 ADI_AXI_PCORE_VER_PATCH(ver));
+
+	return 0;
+}
+
+static struct platform_driver adi_axi_adc_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = adi_axi_adc_of_match,
+	},
+	.probe = adi_axi_adc_probe,
+};
+module_platform_driver(adi_axi_adc_driver);
+
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
+MODULE_DESCRIPTION("Analog Devices Generic AXI ADC IP core driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/iio/adc/adi-axi-adc.h b/include/linux/iio/adc/adi-axi-adc.h
new file mode 100644
index 000000000000..2ae9a99965e6
--- /dev/null
+++ b/include/linux/iio/adc/adi-axi-adc.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Analog Devices Generic AXI ADC IP core driver/library
+ * Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
+ *
+ * Copyright 2012-2020 Analog Devices Inc.
+ */
+#ifndef __ADI_AXI_ADC_H__
+#define __ADI_AXI_ADC_H__
+
+struct device;
+struct iio_chan_spec;
+
+/**
+ * struct adi_axi_adc_chip_info - Chip specific information
+ * @name		Chip name
+ * @id			Chip ID (usually product ID)
+ * @channels		Channel specifications of type @struct axi_adc_chan_spec
+ * @num_channels	Number of @channels
+ * @scale_table		Supported scales by the chip; tuples of 2 ints
+ * @num_scales		Number of scales in the table
+ * @max_rate		Maximum sampling rate supported by the device
+ */
+struct adi_axi_adc_chip_info {
+	const char			*name;
+	unsigned int			id;
+
+	const struct iio_chan_spec	*channels;
+	unsigned int			num_channels;
+
+	const unsigned int		(*scale_table)[2];
+	int				num_scales;
+
+	unsigned long			max_rate;
+};
+
+/**
+ * struct adi_axi_adc_conv - data of the ADC attached to the AXI ADC
+ * @chip_info		chip info details for the client ADC
+ * @preenable_setup	op to run in the client before enabling the AXI ADC
+ * @reg_access		IIO debugfs_reg_access hook for the client ADC
+ * @read_raw		IIO read_raw hook for the client ADC
+ * @write_raw		IIO write_raw hook for the client ADC
+ */
+struct adi_axi_adc_conv {
+	const struct adi_axi_adc_chip_info		*chip_info;
+
+	int (*preenable_setup)(struct adi_axi_adc_conv *conv);
+	int (*reg_access)(struct adi_axi_adc_conv *conv, unsigned int reg,
+			  unsigned int writeval, unsigned int *readval);
+	int (*read_raw)(struct adi_axi_adc_conv *conv,
+			struct iio_chan_spec const *chan,
+			int *val, int *val2, long mask);
+	int (*write_raw)(struct adi_axi_adc_conv *conv,
+			 struct iio_chan_spec const *chan,
+			 int val, int val2, long mask);
+};
+
+struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
+							int sizeof_priv);
+
+void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv);
+
+#endif
-- 
2.17.1

[PATCH v11 6/8] dt-bindings: iio: adc: add bindings doc for AXI ADC driver

[Alexandru Ardelean]

This change adds the bindings documentation for the AXI ADC driver.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 .../bindings/iio/adc/adi,axi-adc.yaml         | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml b/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
new file mode 100644
index 000000000000..0924b2b4972b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,axi-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AXI ADC IP core
+
+maintainers:
+  - Michael Hennerich <michael.hennerich@analog.com>
+  - Alexandru Ardelean <alexandru.ardelean@analog.com>
+
+description: |
+  Analog Devices Generic AXI ADC IP core for interfacing an ADC device
+  with a high speed serial (JESD204B/C) or source synchronous parallel
+  interface (LVDS/CMOS).
+  Usually, some other interface type (i.e SPI) is used as a control
+  interface for the actual ADC, while this IP core will interface
+  to the data-lines of the ADC and handle the streaming of data into
+  memory via DMA.
+
+  https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
+
+properties:
+  compatible:
+    enum:
+      - adi,axi-adc-10.0.a
+
+  reg:
+    maxItems: 1
+
+  dmas:
+    maxItems: 1
+
+  dma-names:
+    items:
+      - const: rx
+
+  adi,adc-dev:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      A reference to a the actual ADC to which this FPGA ADC interfaces to.
+
+required:
+  - compatible
+  - dmas
+  - reg
+  - adi,adc-dev
+
+additionalProperties: false
+
+examples:
+  - |
+    axi-adc@44a00000 {
+          compatible = "adi,axi-adc-10.0.a";
+          reg = <0x44a00000 0x10000>;
+          dmas = <&rx_dma 0>;
+          dma-names = "rx";
+
+          adi,adc-dev = <&spi_adc>;
+    };
+...
-- 
2.17.1

[PATCH v11 7/8] iio: adc: ad9467: add support AD9467 ADC

[Alexandru Ardelean]

From: Michael Hennerich <michael.hennerich@analog.com>

The AD9467 is a 16-bit, monolithic, IF sampling analog-to-digital converter
(ADC). It is optimized for high performanceover wide bandwidths and ease of
use. The product operates at a 250 MSPS conversion rate and is designed for
wireless receivers, instrumentation, and test equipment that require a high
dynamic range. The ADC requires 1.8 V and 3.3 V power supplies and a low
voltage differential input clock for full performance operation. No
external reference or driver components are required for many applications.
Data outputs are LVDS compatible (ANSI-644 compatible) and include the
means to reduce the overall current needed for short trace distances.

Since the chip can operate at such high sample-rates (much higher than
classical interfaces), it requires that a DMA controller be used to
interface directly to the chip and push data into memory.
Typically, the AXI ADC IP core is used to interface with it.

Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD9467.pdf

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/Kconfig  |  15 ++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad9467.c | 420 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 436 insertions(+)
 create mode 100644 drivers/iio/adc/ad9467.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 445070abf376..a0796510f9d4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -246,6 +246,21 @@ config AD799X
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad799x.
 
+config AD9467
+	tristate "Analog Devices AD9467 High Speed ADC driver"
+	depends on SPI
+	select ADI_AXI_ADC
+	help
+	  Say yes here to build support for Analog Devices:
+	  * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
+
+	  The driver requires the assistance of the AXI ADC IP core to operate,
+	  since SPI is used for configuration only, while data has to be
+	  streamed into memory via DMA.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad9467.
+
 config ADI_AXI_ADC
 	tristate "Analog Devices Generic AXI ADC IP core driver"
 	select IIO_BUFFER
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 7c6594d049f9..59722770a654 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
 obj-$(CONFIG_AD7887) += ad7887.o
 obj-$(CONFIG_AD7949) += ad7949.o
 obj-$(CONFIG_AD799X) += ad799x.o
+obj-$(CONFIG_AD9467) += ad9467.o
 obj-$(CONFIG_ADI_AXI_ADC) += adi-axi-adc.o
 obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
 obj-$(CONFIG_AT91_ADC) += at91_adc.o
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
new file mode 100644
index 000000000000..362993daedaf
--- /dev/null
+++ b/drivers/iio/adc/ad9467.c
@@ -0,0 +1,420 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD9467 SPI ADC driver
+ *
+ * Copyright 2012-2020 Analog Devices Inc.
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of_device.h>
+
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#include <linux/clk.h>
+
+#include <linux/iio/adc/adi-axi-adc.h>
+
+/*
+ * ADI High-Speed ADC common spi interface registers
+ * See Application-Note AN-877:
+ *   https://www.analog.com/media/en/technical-documentation/application-notes/AN-877.pdf
+ */
+
+#define AN877_ADC_REG_CHIP_PORT_CONF		0x00
+#define AN877_ADC_REG_CHIP_ID			0x01
+#define AN877_ADC_REG_CHIP_GRADE		0x02
+#define AN877_ADC_REG_CHAN_INDEX		0x05
+#define AN877_ADC_REG_TRANSFER			0xFF
+#define AN877_ADC_REG_MODES			0x08
+#define AN877_ADC_REG_TEST_IO			0x0D
+#define AN877_ADC_REG_ADC_INPUT			0x0F
+#define AN877_ADC_REG_OFFSET			0x10
+#define AN877_ADC_REG_OUTPUT_MODE		0x14
+#define AN877_ADC_REG_OUTPUT_ADJUST		0x15
+#define AN877_ADC_REG_OUTPUT_PHASE		0x16
+#define AN877_ADC_REG_OUTPUT_DELAY		0x17
+#define AN877_ADC_REG_VREF			0x18
+#define AN877_ADC_REG_ANALOG_INPUT		0x2C
+
+/* AN877_ADC_REG_TEST_IO */
+#define AN877_ADC_TESTMODE_OFF			0x0
+#define AN877_ADC_TESTMODE_MIDSCALE_SHORT	0x1
+#define AN877_ADC_TESTMODE_POS_FULLSCALE	0x2
+#define AN877_ADC_TESTMODE_NEG_FULLSCALE	0x3
+#define AN877_ADC_TESTMODE_ALT_CHECKERBOARD	0x4
+#define AN877_ADC_TESTMODE_PN23_SEQ		0x5
+#define AN877_ADC_TESTMODE_PN9_SEQ		0x6
+#define AN877_ADC_TESTMODE_ONE_ZERO_TOGGLE	0x7
+#define AN877_ADC_TESTMODE_USER			0x8
+#define AN877_ADC_TESTMODE_BIT_TOGGLE		0x9
+#define AN877_ADC_TESTMODE_SYNC			0xA
+#define AN877_ADC_TESTMODE_ONE_BIT_HIGH		0xB
+#define AN877_ADC_TESTMODE_MIXED_BIT_FREQUENCY	0xC
+#define AN877_ADC_TESTMODE_RAMP			0xF
+
+/* AN877_ADC_REG_TRANSFER */
+#define AN877_ADC_TRANSFER_SYNC			0x1
+
+/* AN877_ADC_REG_OUTPUT_MODE */
+#define AN877_ADC_OUTPUT_MODE_OFFSET_BINARY	0x0
+#define AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT	0x1
+#define AN877_ADC_OUTPUT_MODE_GRAY_CODE		0x2
+
+/* AN877_ADC_REG_OUTPUT_PHASE */
+#define AN877_ADC_OUTPUT_EVEN_ODD_MODE_EN	0x20
+#define AN877_ADC_INVERT_DCO_CLK		0x80
+
+/* AN877_ADC_REG_OUTPUT_DELAY */
+#define AN877_ADC_DCO_DELAY_ENABLE		0x80
+
+/*
+ * Analog Devices AD9467 16-Bit, 200/250 MSPS ADC
+ */
+
+#define CHIPID_AD9467			0x50
+#define AD9467_DEF_OUTPUT_MODE		0x08
+#define AD9467_REG_VREF_MASK		0x0F
+
+enum {
+	ID_AD9467,
+};
+
+struct ad9467_state {
+	struct spi_device		*spi;
+	struct clk			*clk;
+	unsigned int			output_mode;
+
+	struct gpio_desc		*pwrdown_gpio;
+	struct gpio_desc		*reset_gpio;
+};
+
+static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
+{
+	unsigned char tbuf[2], rbuf[1];
+	int ret;
+
+	tbuf[0] = 0x80 | (reg >> 8);
+	tbuf[1] = reg & 0xFF;
+
+	ret = spi_write_then_read(spi,
+				  tbuf, ARRAY_SIZE(tbuf),
+				  rbuf, ARRAY_SIZE(rbuf));
+
+	if (ret < 0)
+		return ret;
+
+	return rbuf[0];
+}
+
+static int ad9467_spi_write(struct spi_device *spi, unsigned int reg,
+			    unsigned int val)
+{
+	unsigned char buf[3];
+
+	buf[0] = reg >> 8;
+	buf[1] = reg & 0xFF;
+	buf[2] = val;
+
+	return spi_write(spi, buf, ARRAY_SIZE(buf));
+}
+
+static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
+			     unsigned int writeval, unsigned int *readval)
+{
+	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct spi_device *spi = st->spi;
+	int ret;
+
+	if (readval == NULL) {
+		ret = ad9467_spi_write(spi, reg, writeval);
+		ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
+				 AN877_ADC_TRANSFER_SYNC);
+		return ret;
+	}
+
+	ret = ad9467_spi_read(spi, reg);
+	if (ret < 0)
+		return ret;
+	*readval = ret;
+
+	return 0;
+}
+
+static const unsigned int ad9467_scale_table[][2] = {
+	{2000, 0}, {2100, 6}, {2200, 7},
+	{2300, 8}, {2400, 9}, {2500, 10},
+};
+
+static void __ad9467_get_scale(struct adi_axi_adc_conv *conv, int index,
+			       unsigned int *val, unsigned int *val2)
+{
+	const struct adi_axi_adc_chip_info *info = conv->chip_info;
+	const struct iio_chan_spec *chan = &info->channels[0];
+	unsigned int tmp;
+
+	tmp = (info->scale_table[index][0] * 1000000ULL) >>
+			chan->scan_type.realbits;
+	*val = tmp / 1000000;
+	*val2 = tmp % 1000000;
+}
+
+#define AD9467_CHAN(_chan, _si, _bits, _sign)				\
+{									\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.channel = _chan,						\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
+	.scan_index = _si,						\
+	.scan_type = {							\
+		.sign = _sign,						\
+		.realbits = _bits,					\
+		.storagebits = 16,					\
+	},								\
+}
+
+static const struct iio_chan_spec ad9467_channels[] = {
+	AD9467_CHAN(0, 0, 16, 'S'),
+};
+
+static const struct adi_axi_adc_chip_info ad9467_chip_tbl[] = {
+	[ID_AD9467] = {
+		.id = CHIPID_AD9467,
+		.max_rate = 250000000UL,
+		.scale_table = ad9467_scale_table,
+		.num_scales = ARRAY_SIZE(ad9467_scale_table),
+		.channels = ad9467_channels,
+		.num_channels = ARRAY_SIZE(ad9467_channels),
+	},
+};
+
+static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
+{
+	const struct adi_axi_adc_chip_info *info = conv->chip_info;
+	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	unsigned int i, vref_val, vref_mask;
+
+	vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
+
+	switch (info->id) {
+	case CHIPID_AD9467:
+		vref_mask = AD9467_REG_VREF_MASK;
+		break;
+	default:
+		vref_mask = 0xFFFF;
+		break;
+	}
+
+	vref_val &= vref_mask;
+
+	for (i = 0; i < info->num_scales; i++) {
+		if (vref_val == info->scale_table[i][1])
+			break;
+	}
+
+	if (i == info->num_scales)
+		return -ERANGE;
+
+	__ad9467_get_scale(conv, i, val, val2);
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
+{
+	const struct adi_axi_adc_chip_info *info = conv->chip_info;
+	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	unsigned int scale_val[2];
+	unsigned int i;
+
+	if (val != 0)
+		return -EINVAL;
+
+	for (i = 0; i < info->num_scales; i++) {
+		__ad9467_get_scale(conv, i, &scale_val[0], &scale_val[1]);
+		if (scale_val[0] != val || scale_val[1] != val2)
+			continue;
+
+		ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
+				 info->scale_table[i][1]);
+		ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+				 AN877_ADC_TRANSFER_SYNC);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int ad9467_read_raw(struct adi_axi_adc_conv *conv,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long m)
+{
+	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		return ad9467_get_scale(conv, val, val2);
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = clk_get_rate(st->clk);
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	const struct adi_axi_adc_chip_info *info = conv->chip_info;
+	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	unsigned long r_clk;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return ad9467_set_scale(conv, val, val2);
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		r_clk = clk_round_rate(st->clk, val);
+		if (r_clk < 0 || r_clk > info->max_rate) {
+			dev_warn(&st->spi->dev,
+				 "Error setting ADC sample rate %ld", r_clk);
+			return -EINVAL;
+		}
+
+		return clk_set_rate(st->clk, r_clk);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
+{
+	int ret;
+
+	ret = ad9467_spi_write(spi, AN877_ADC_REG_OUTPUT_MODE, mode);
+	if (ret < 0)
+		return ret;
+
+	return ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
+				AN877_ADC_TRANSFER_SYNC);
+}
+
+static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
+{
+	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+
+	return ad9467_outputmode_set(st->spi, st->output_mode);
+}
+
+static int ad9467_setup(struct ad9467_state *st, unsigned int chip_id)
+{
+	switch (chip_id) {
+	case CHIPID_AD9467:
+		st->output_mode = AD9467_DEF_OUTPUT_MODE |
+				  AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static void ad9467_clk_disable(void *data)
+{
+	struct ad9467_state *st = data;
+
+	clk_disable_unprepare(st->clk);
+}
+
+static const struct of_device_id ad9467_of_match[] = {
+	{ .compatible = "adi,ad9467", .data = &ad9467_chip_tbl[ID_AD9467], },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ad9467_of_match);
+
+static int ad9467_probe(struct spi_device *spi)
+{
+	const struct adi_axi_adc_chip_info *info;
+	struct adi_axi_adc_conv *conv;
+	struct ad9467_state *st;
+	unsigned int id;
+	int ret;
+
+	info = of_device_get_match_data(&spi->dev);
+	if (!info)
+		return -ENODEV;
+
+	conv = devm_adi_axi_adc_conv_register(&spi->dev, sizeof(*st));
+	if (IS_ERR(conv))
+		return PTR_ERR(conv);
+
+	st = adi_axi_adc_conv_priv(conv);
+	st->spi = spi;
+
+	st->clk = devm_clk_get(&spi->dev, "adc-clk");
+	if (IS_ERR(st->clk))
+		return PTR_ERR(st->clk);
+
+	ret = clk_prepare_enable(st->clk);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_add_action_or_reset(&spi->dev, ad9467_clk_disable, st);
+	if (ret)
+		return ret;
+
+	st->pwrdown_gpio = devm_gpiod_get_optional(&spi->dev, "powerdown",
+						   GPIOD_OUT_LOW);
+	if (IS_ERR(st->pwrdown_gpio))
+		return PTR_ERR(st->pwrdown_gpio);
+
+	st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
+						 GPIOD_OUT_LOW);
+	if (IS_ERR(st->reset_gpio))
+		return PTR_ERR(st->reset_gpio);
+
+	if (st->reset_gpio) {
+		udelay(1);
+		ret = gpiod_direction_output(st->reset_gpio, 1);
+		mdelay(10);
+	}
+
+	spi_set_drvdata(spi, st);
+
+	conv->chip_info = info;
+
+	id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
+	if (id != conv->chip_info->id) {
+		dev_err(&spi->dev, "Unrecognized CHIP_ID 0x%X\n", id);
+		return -ENODEV;
+	}
+
+	conv->reg_access = ad9467_reg_access;
+	conv->write_raw = ad9467_write_raw;
+	conv->read_raw = ad9467_read_raw;
+	conv->preenable_setup = ad9467_preenable_setup;
+
+	return ad9467_setup(st, id);
+}
+
+static struct spi_driver ad9467_driver = {
+	.driver = {
+		.name = "ad9467",
+		.of_match_table = ad9467_of_match,
+	},
+	.probe = ad9467_probe,
+};
+module_spi_driver(ad9467_driver);
+
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD9467 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH v11 8/8] dt-bindings: iio: adc: add bindings doc for AD9467 ADC

[Alexandru Ardelean]

This change adds the binding doc for the AD9467 ADC.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 .../bindings/iio/adc/adi,ad9467.yaml          | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
new file mode 100644
index 000000000000..c4f57fa6aad1
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad9467.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD9467 High-Speed ADC
+
+maintainers:
+  - Michael Hennerich <michael.hennerich@analog.com>
+  - Alexandru Ardelean <alexandru.ardelean@analog.com>
+
+description: |
+  The AD9467 is a 16-bit, monolithic, IF sampling analog-to-digital
+  converter (ADC).
+
+  https://www.analog.com/media/en/technical-documentation/data-sheets/AD9467.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad9467
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: adc-clk
+
+  powerdown-gpios:
+    description:
+      Pin that controls the powerdown mode of the device.
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      Reset pin for the device.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+          compatible = "adi,ad9467";
+          reg = <0>;
+          clocks = <&adc_clk>;
+          clock-names = "adc-clk";
+        };
+    };
+...
-- 
2.17.1

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Jonathan Cameron]

On Sat, 21 Mar 2020 10:53:12 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> This change adds support for the Analog Devices Generic AXI ADC IP core.
> The IP core is used for interfacing with analog-to-digital (ADC) converters
> that require either a high-speed serial interface (JESD204B/C) or a source
> synchronous parallel interface (LVDS/CMOS).
> 
> Usually, some other interface type (i.e SPI) is used as a control interface
> for the actual ADC, while the IP core (controlled via this driver), will
> interface to the data-lines of the ADC and handle  the streaming of data
> into memory via DMA.
> 
> Because of this, the AXI ADC driver needs the other SPI-ADC driver to
> register with it. The SPI-ADC needs to be register via the SPI framework,
> while the AXI ADC registers as a platform driver. The two cannot be ordered
> in a hierarchy as both drivers have their own registers, and trying to
> organize this [in a hierarchy becomes] problematic when trying to map
> memory/registers.
> 
> There are some modes where the AXI ADC can operate as standalone ADC, but
> those will be implemented at a later point in time.
> 
> Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Trivial missing static... I've fixed up.

Applied to the togreg branch of iio.git and pushed out as testing.

This may be interesting as I think it's the first time we've actually
exposed the dma buffer stuff to proper build testing on all the
weird architectures 0-day etc will hit it with.

Thanks

Jonathan

> ---
>  drivers/iio/adc/Kconfig             |  20 ++
>  drivers/iio/adc/Makefile            |   1 +
>  drivers/iio/adc/adi-axi-adc.c       | 495 ++++++++++++++++++++++++++++
>  include/linux/iio/adc/adi-axi-adc.h |  64 ++++
>  4 files changed, 580 insertions(+)
>  create mode 100644 drivers/iio/adc/adi-axi-adc.c
>  create mode 100644 include/linux/iio/adc/adi-axi-adc.h
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f4da821c4022..445070abf376 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -246,6 +246,26 @@ config AD799X
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad799x.
>  
> +config ADI_AXI_ADC
> +	tristate "Analog Devices Generic AXI ADC IP core driver"
> +	select IIO_BUFFER
> +	select IIO_BUFFER_HW_CONSUMER
> +	select IIO_BUFFER_DMAENGINE
> +	help
> +	  Say yes here to build support for Analog Devices Generic
> +	  AXI ADC IP core. The IP core is used for interfacing with
> +	  analog-to-digital (ADC) converters that require either a high-speed
> +	  serial interface (JESD204B/C) or a source synchronous parallel
> +	  interface (LVDS/CMOS).
> +	  Typically (for such devices) SPI will be used for configuration only,
> +	  while this IP core handles the streaming of data into memory via DMA.
> +
> +	  Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> +	  If unsure, say N (but it's safe to say "Y").
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called adi-axi-adc.
> +
>  config ASPEED_ADC
>  	tristate "Aspeed ADC"
>  	depends on ARCH_ASPEED || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 8462455b4228..7c6594d049f9 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
>  obj-$(CONFIG_AD7949) += ad7949.o
>  obj-$(CONFIG_AD799X) += ad799x.o
> +obj-$(CONFIG_ADI_AXI_ADC) += adi-axi-adc.o
>  obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
>  obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> new file mode 100644
> index 000000000000..8d966b47edc9
> --- /dev/null
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -0,0 +1,495 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices Generic AXI ADC IP core
> + * Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> + *
> + * Copyright 2012-2020 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/buffer-dmaengine.h>
> +
> +#include <linux/fpga/adi-axi-common.h>
> +#include <linux/iio/adc/adi-axi-adc.h>
> +
> +/**
> + * Register definitions:
> + *   https://wiki.analog.com/resources/fpga/docs/axi_adc_ip#register_map
> + */
> +
> +/* ADC controls */
> +
> +#define ADI_AXI_REG_RSTN			0x0040
> +#define   ADI_AXI_REG_RSTN_CE_N			BIT(2)
> +#define   ADI_AXI_REG_RSTN_MMCM_RSTN		BIT(1)
> +#define   ADI_AXI_REG_RSTN_RSTN			BIT(0)
> +
> +/* ADC Channel controls */
> +
> +#define ADI_AXI_REG_CHAN_CTRL(c)		(0x0400 + (c) * 0x40)
> +#define   ADI_AXI_REG_CHAN_CTRL_LB_OWR		BIT(11)
> +#define   ADI_AXI_REG_CHAN_CTRL_PN_SEL_OWR	BIT(10)
> +#define   ADI_AXI_REG_CHAN_CTRL_IQCOR_EN	BIT(9)
> +#define   ADI_AXI_REG_CHAN_CTRL_DCFILT_EN	BIT(8)
> +#define   ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT	BIT(6)
> +#define   ADI_AXI_REG_CHAN_CTRL_FMT_TYPE	BIT(5)
> +#define   ADI_AXI_REG_CHAN_CTRL_FMT_EN		BIT(4)
> +#define   ADI_AXI_REG_CHAN_CTRL_PN_TYPE_OWR	BIT(1)
> +#define   ADI_AXI_REG_CHAN_CTRL_ENABLE		BIT(0)
> +
> +#define ADI_AXI_REG_CHAN_CTRL_DEFAULTS		\
> +	(ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT |	\
> +	 ADI_AXI_REG_CHAN_CTRL_FMT_EN |		\
> +	 ADI_AXI_REG_CHAN_CTRL_ENABLE)
> +
> +struct adi_axi_adc_core_info {
> +	unsigned int				version;
> +};
> +
> +struct adi_axi_adc_state {
> +	struct mutex				lock;
> +
> +	struct adi_axi_adc_client		*client;
> +	void __iomem				*regs;
> +};
> +
> +struct adi_axi_adc_client {
> +	struct list_head			entry;
> +	struct adi_axi_adc_conv			conv;
> +	struct adi_axi_adc_state		*state;
> +	struct device				*dev;
> +	const struct adi_axi_adc_core_info	*info;
> +};
> +
> +static LIST_HEAD(registered_clients);
> +static DEFINE_MUTEX(registered_clients_lock);
> +
> +static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv *conv)
> +{
> +	if (!conv)
> +		return NULL;
> +	return container_of(conv, struct adi_axi_adc_client, conv);
> +}
> +
> +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
> +{
> +	struct adi_axi_adc_client *cl = conv_to_client(conv);
> +
> +	if (!cl)
> +		return NULL;
> +
> +	return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client), IIO_ALIGN);
> +}
> +EXPORT_SYMBOL_GPL(adi_axi_adc_conv_priv);
> +
> +static void adi_axi_adc_write(struct adi_axi_adc_state *st,
> +			      unsigned int reg,
> +			      unsigned int val)
> +{
> +	iowrite32(val, st->regs + reg);
> +}
> +
> +static unsigned int adi_axi_adc_read(struct adi_axi_adc_state *st,
> +				     unsigned int reg)
> +{
> +	return ioread32(st->regs + reg);
> +}
> +
> +static int adi_axi_adc_config_dma_buffer(struct device *dev,
> +					 struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer;
> +	const char *dma_name;
> +
> +	if (!device_property_present(dev, "dmas"))
> +		return 0;
> +
> +	if (device_property_read_string(dev, "dma-names", &dma_name))
> +		dma_name = "rx";
> +
> +	buffer = devm_iio_dmaengine_buffer_alloc(indio_dev->dev.parent,
> +						 dma_name);
> +	if (IS_ERR(buffer))
> +		return PTR_ERR(buffer);
> +
> +	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
> +	return 0;
> +}
> +
> +static int adi_axi_adc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct adi_axi_adc_state *st = iio_priv(indio_dev);
> +	struct adi_axi_adc_conv *conv = &st->client->conv;
> +
> +	if (!conv->read_raw)
> +		return -EOPNOTSUPP;
> +
> +	return conv->read_raw(conv, chan, val, val2, mask);
> +}
> +
> +static int adi_axi_adc_write_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan,
> +				 int val, int val2, long mask)
> +{
> +	struct adi_axi_adc_state *st = iio_priv(indio_dev);
> +	struct adi_axi_adc_conv *conv = &st->client->conv;
> +
> +	if (!conv->write_raw)
> +		return -EOPNOTSUPP;
> +
> +	return conv->write_raw(conv, chan, val, val2, mask);
> +}
> +
> +static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
> +					const unsigned long *scan_mask)
> +{
> +	struct adi_axi_adc_state *st = iio_priv(indio_dev);
> +	struct adi_axi_adc_conv *conv = &st->client->conv;
> +	unsigned int i, ctrl;
> +
> +	for (i = 0; i < conv->chip_info->num_channels; i++) {
> +		ctrl = adi_axi_adc_read(st, ADI_AXI_REG_CHAN_CTRL(i));
> +
> +		if (test_bit(i, scan_mask))
> +			ctrl |= ADI_AXI_REG_CHAN_CTRL_ENABLE;
> +		else
> +			ctrl &= ~ADI_AXI_REG_CHAN_CTRL_ENABLE;
> +
> +		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i), ctrl);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device *dev,
> +							  int sizeof_priv)
> +{
> +	struct adi_axi_adc_client *cl;
> +	size_t alloc_size;
> +
> +	alloc_size = sizeof(struct adi_axi_adc_client);
> +	if (sizeof_priv) {
> +		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> +		alloc_size += sizeof_priv;
> +	}
> +	alloc_size += IIO_ALIGN - 1;
> +
> +	cl = kzalloc(alloc_size, GFP_KERNEL);
> +	if (!cl)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_lock(&registered_clients_lock);
> +
> +	get_device(dev);
> +	cl->dev = dev;
> +
> +	list_add_tail(&cl->entry, &registered_clients);
> +
> +	mutex_unlock(&registered_clients_lock);
> +
> +	return &cl->conv;
> +}
> +
> +static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> +{
> +	struct adi_axi_adc_client *cl = conv_to_client(conv);
> +
> +	if (!cl)
> +		return;
> +
> +	mutex_lock(&registered_clients_lock);
> +
> +	list_del(&cl->entry);
> +	put_device(cl->dev);
> +
> +	mutex_unlock(&registered_clients_lock);
> +
> +	kfree(cl);
> +}
> +
> +static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
> +{
> +	adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);
> +}
> +
> +struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
> +							int sizeof_priv)
> +{
> +	struct adi_axi_adc_conv **ptr, *conv;
> +
> +	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> +	if (IS_ERR(conv)) {
> +		devres_free(ptr);
> +		return ERR_CAST(conv);
> +	}
> +
> +	*ptr = conv;
> +	devres_add(dev, ptr);
> +
> +	return conv;
> +}
> +EXPORT_SYMBOL_GPL(devm_adi_axi_adc_conv_register);
> +
> +static ssize_t in_voltage_scale_available_show(struct device *dev,
> +					       struct device_attribute *attr,
> +					       char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct adi_axi_adc_state *st = iio_priv(indio_dev);
> +	struct adi_axi_adc_conv *conv = &st->client->conv;
> +	size_t len = 0;
> +	int i;
> +
> +	for (i = 0; i < conv->chip_info->num_scales; i++) {
> +		const unsigned int *s = conv->chip_info->scale_table[i];
> +
> +		len += scnprintf(buf + len, PAGE_SIZE - len,
> +				 "%u.%06u ", s[0], s[1]);
> +	}
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
> +
> +enum {
> +	ADI_AXI_ATTR_SCALE_AVAIL,
> +};
> +
> +#define ADI_AXI_ATTR(_en_, _file_)			\
> +	[ADI_AXI_ATTR_##_en_] = &iio_dev_attr_##_file_.dev_attr.attr
> +
> +static struct attribute *adi_axi_adc_attributes[] = {
> +	ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> +	NULL,
> +};
> +
> +static umode_t axi_adc_attr_is_visible(struct kobject *kobj,
> +				       struct attribute *attr, int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct adi_axi_adc_state *st = iio_priv(indio_dev);
> +	struct adi_axi_adc_conv *conv = &st->client->conv;
> +
> +	switch (n) {
> +	case ADI_AXI_ATTR_SCALE_AVAIL:
> +		if (!conv->chip_info->num_scales)
> +			return 0;
> +		return attr->mode;
> +	default:
> +		return attr->mode;
> +	}
> +}
> +
> +static const struct attribute_group adi_axi_adc_attribute_group = {
> +	.attrs = adi_axi_adc_attributes,
> +	.is_visible = axi_adc_attr_is_visible,
> +};
> +
> +static const struct iio_info adi_axi_adc_info = {
> +	.read_raw = &adi_axi_adc_read_raw,
> +	.write_raw = &adi_axi_adc_write_raw,
> +	.attrs = &adi_axi_adc_attribute_group,
> +	.update_scan_mode = &adi_axi_adc_update_scan_mode,
> +};
> +
> +static const struct adi_axi_adc_core_info adi_axi_adc_10_0_a_info = {
> +	.version = ADI_AXI_PCORE_VER(10, 0, 'a'),
> +};
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id adi_axi_adc_of_match[] = {
> +	{ .compatible = "adi,axi-adc-10.0.a", .data = &adi_axi_adc_10_0_a_info },
> +	{ /* end of list */ },
> +};
> +MODULE_DEVICE_TABLE(of, adi_axi_adc_of_match);
> +
> +struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)

Should be declared static.

> +{
> +	const struct of_device_id *id;
> +	struct adi_axi_adc_client *cl;
> +	struct device_node *cln;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "DT node is null\n");
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	id = of_match_node(adi_axi_adc_of_match, dev->of_node);
> +	if (!id)
> +		return ERR_PTR(-ENODEV);
> +
> +	cln = of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
> +	if (!cln) {
> +		dev_err(dev, "No 'adi,adc-dev' node defined\n");
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	mutex_lock(&registered_clients_lock);
> +
> +	list_for_each_entry(cl, &registered_clients, entry) {
> +		if (!cl->dev)
> +			continue;
> +		if (cl->dev->of_node == cln) {
> +			if (!try_module_get(dev->driver->owner)) {
> +				mutex_unlock(&registered_clients_lock);
> +				return ERR_PTR(-ENODEV);
> +			}
> +			get_device(dev);
> +			cl->info = id->data;
> +			mutex_unlock(&registered_clients_lock);
> +			return cl;
> +		}
> +	}
> +
> +	mutex_unlock(&registered_clients_lock);
> +
> +	return ERR_PTR(-EPROBE_DEFER);
> +}
> +
> +static int adi_axi_adc_setup_channels(struct device *dev,
> +				      struct adi_axi_adc_state *st)
> +{
> +	struct adi_axi_adc_conv *conv = conv = &st->client->conv;
> +	int i, ret;
> +
> +	if (conv->preenable_setup) {
> +		ret = conv->preenable_setup(conv);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < conv->chip_info->num_channels; i++) {
> +		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i),
> +				  ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
> +	}
> +
> +	return 0;
> +}
> +
> +static void axi_adc_reset(struct adi_axi_adc_state *st)
> +{
> +	adi_axi_adc_write(st, ADI_AXI_REG_RSTN, 0);
> +	mdelay(10);
> +	adi_axi_adc_write(st, ADI_AXI_REG_RSTN, ADI_AXI_REG_RSTN_MMCM_RSTN);
> +	mdelay(10);
> +	adi_axi_adc_write(st, ADI_AXI_REG_RSTN,
> +			  ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
> +}
> +
> +static void adi_axi_adc_cleanup(void *data)
> +{
> +	struct adi_axi_adc_client *cl = data;
> +
> +	put_device(cl->dev);
> +	module_put(cl->dev->driver->owner);
> +}
> +
> +static int adi_axi_adc_probe(struct platform_device *pdev)
> +{
> +	struct adi_axi_adc_conv *conv;
> +	struct iio_dev *indio_dev;
> +	struct adi_axi_adc_client *cl;
> +	struct adi_axi_adc_state *st;
> +	unsigned int ver;
> +	int ret;
> +
> +	cl = adi_axi_adc_attach_client(&pdev->dev);
> +	if (IS_ERR(cl))
> +		return PTR_ERR(cl);
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, adi_axi_adc_cleanup, cl);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->client = cl;
> +	cl->state = st;
> +	mutex_init(&st->lock);
> +
> +	st->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(st->regs))
> +		return PTR_ERR(st->regs);
> +
> +	conv = &st->client->conv;
> +
> +	axi_adc_reset(st);
> +
> +	ver = adi_axi_adc_read(st, ADI_AXI_REG_VERSION);
> +
> +	if (cl->info->version > ver) {
> +		dev_err(&pdev->dev,
> +			"IP core version is too old. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
> +			ADI_AXI_PCORE_VER_MAJOR(cl->info->version),
> +			ADI_AXI_PCORE_VER_MINOR(cl->info->version),
> +			ADI_AXI_PCORE_VER_PATCH(cl->info->version),
> +			ADI_AXI_PCORE_VER_MAJOR(ver),
> +			ADI_AXI_PCORE_VER_MINOR(ver),
> +			ADI_AXI_PCORE_VER_PATCH(ver));
> +		return -ENODEV;
> +	}
> +
> +	indio_dev->info = &adi_axi_adc_info;
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->name = "adi-axi-adc";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = conv->chip_info->num_channels;
> +	indio_dev->channels = conv->chip_info->channels;
> +
> +	ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = adi_axi_adc_setup_channels(&pdev->dev, st);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n",
> +		 ADI_AXI_PCORE_VER_MAJOR(ver),
> +		 ADI_AXI_PCORE_VER_MINOR(ver),
> +		 ADI_AXI_PCORE_VER_PATCH(ver));
> +
> +	return 0;
> +}
> +
> +static struct platform_driver adi_axi_adc_driver = {
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = adi_axi_adc_of_match,
> +	},
> +	.probe = adi_axi_adc_probe,
> +};
> +module_platform_driver(adi_axi_adc_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices Generic AXI ADC IP core driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/iio/adc/adi-axi-adc.h b/include/linux/iio/adc/adi-axi-adc.h
> new file mode 100644
> index 000000000000..2ae9a99965e6
> --- /dev/null
> +++ b/include/linux/iio/adc/adi-axi-adc.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Analog Devices Generic AXI ADC IP core driver/library
> + * Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> + *
> + * Copyright 2012-2020 Analog Devices Inc.
> + */
> +#ifndef __ADI_AXI_ADC_H__
> +#define __ADI_AXI_ADC_H__
> +
> +struct device;
> +struct iio_chan_spec;
> +
> +/**
> + * struct adi_axi_adc_chip_info - Chip specific information
> + * @name		Chip name
> + * @id			Chip ID (usually product ID)
> + * @channels		Channel specifications of type @struct axi_adc_chan_spec
> + * @num_channels	Number of @channels
> + * @scale_table		Supported scales by the chip; tuples of 2 ints
> + * @num_scales		Number of scales in the table
> + * @max_rate		Maximum sampling rate supported by the device
> + */
> +struct adi_axi_adc_chip_info {
> +	const char			*name;
> +	unsigned int			id;
> +
> +	const struct iio_chan_spec	*channels;
> +	unsigned int			num_channels;
> +
> +	const unsigned int		(*scale_table)[2];
> +	int				num_scales;
> +
> +	unsigned long			max_rate;
> +};
> +
> +/**
> + * struct adi_axi_adc_conv - data of the ADC attached to the AXI ADC
> + * @chip_info		chip info details for the client ADC
> + * @preenable_setup	op to run in the client before enabling the AXI ADC
> + * @reg_access		IIO debugfs_reg_access hook for the client ADC
> + * @read_raw		IIO read_raw hook for the client ADC
> + * @write_raw		IIO write_raw hook for the client ADC
> + */
> +struct adi_axi_adc_conv {
> +	const struct adi_axi_adc_chip_info		*chip_info;
> +
> +	int (*preenable_setup)(struct adi_axi_adc_conv *conv);
> +	int (*reg_access)(struct adi_axi_adc_conv *conv, unsigned int reg,
> +			  unsigned int writeval, unsigned int *readval);
> +	int (*read_raw)(struct adi_axi_adc_conv *conv,
> +			struct iio_chan_spec const *chan,
> +			int *val, int *val2, long mask);
> +	int (*write_raw)(struct adi_axi_adc_conv *conv,
> +			 struct iio_chan_spec const *chan,
> +			 int val, int val2, long mask);
> +};
> +
> +struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
> +							int sizeof_priv);
> +
> +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv);
> +
> +#endif

Re: [PATCH v11 2/8] include: fpga: adi-axi-common.h: add version helper macros

[Jonathan Cameron]

On Sat, 21 Mar 2020 10:53:09 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The format for all ADI AXI IP cores is the same.
> i.e. 'major.minor.patch'.
> 
> This patch adds the helper macros to be re-used in ADI AXI drivers.
> 
> Acked-by: Moritz Fischer <mdf@kernel.org>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied,

Thanks,

Jonathan

> ---
>  include/linux/fpga/adi-axi-common.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/fpga/adi-axi-common.h b/include/linux/fpga/adi-axi-common.h
> index ebd4e07ae3d8..141ac3f251e6 100644
> --- a/include/linux/fpga/adi-axi-common.h
> +++ b/include/linux/fpga/adi-axi-common.h
> @@ -16,4 +16,8 @@
>  #define ADI_AXI_PCORE_VER(major, minor, patch)	\
>  	(((major) << 16) | ((minor) << 8) | (patch))
>  
> +#define ADI_AXI_PCORE_VER_MAJOR(version)	(((version) >> 16) & 0xff)
> +#define ADI_AXI_PCORE_VER_MINOR(version)	(((version) >> 8) & 0xff)
> +#define ADI_AXI_PCORE_VER_PATCH(version)	((version) & 0xff)
> +
>  #endif /* ADI_AXI_COMMON_H_ */

Re: [PATCH v11 3/8] iio: buffer-dmaengine: use %zu specifier for sprintf(align)

[Jonathan Cameron]

On Sat, 21 Mar 2020 10:53:10 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The 'size_t' type behaves differently on 64-bit architectures, and causes
> compiler a warning of the sort "format '%u' expects argument of type
> 'unsigned int', but argument 3 has type 'size_t {aka long unsigned int}'".
> 
> This change adds the correct specifier for the 'align' field.
> 
> Fixes: 4538c18568099 ("iio: buffer-dmaengine: Report buffer length requirements")
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied thanks.

Given lack of users of this infrastructure, I'm not that fussed
about marking this for stable or similar.  However, I don't
mind someone requesting a backport if it is useful to them.

Jonathan

> ---
>  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> index b129693af0fd..94da3b1ca3a2 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> @@ -134,7 +134,7 @@ static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
>  	struct dmaengine_buffer *dmaengine_buffer =
>  		iio_buffer_to_dmaengine_buffer(indio_dev->buffer);
>  
> -	return sprintf(buf, "%u\n", dmaengine_buffer->align);
> +	return sprintf(buf, "%zu\n", dmaengine_buffer->align);
>  }
>  
>  static IIO_DEVICE_ATTR(length_align_bytes, 0444,

Re: [PATCH v11 4/8] iio: buffer-dmaengine: add dev-managed calls for buffer alloc

[Jonathan Cameron]

On Sat, 21 Mar 2020 10:53:11 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Currently, when using a 'iio_dmaengine_buffer_alloc()', an matching call to
> 'iio_dmaengine_buffer_free()' must be made.
> 
> With this change, this can be avoided by using
> 'devm_iio_dmaengine_buffer_alloc()'. The buffer will get free'd via the
> device's devres handling.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.

Thanks,

Jonathan

> ---
>  .../buffer/industrialio-buffer-dmaengine.c    | 39 +++++++++++++++++++
>  include/linux/iio/buffer-dmaengine.h          |  3 ++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> index 94da3b1ca3a2..6dedf12b69a4 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> @@ -229,6 +229,45 @@ void iio_dmaengine_buffer_free(struct iio_buffer *buffer)
>  }
>  EXPORT_SYMBOL_GPL(iio_dmaengine_buffer_free);
>  
> +static void __devm_iio_dmaengine_buffer_free(struct device *dev, void *res)
> +{
> +	iio_dmaengine_buffer_free(*(struct iio_buffer **)res);
> +}
> +
> +/**
> + * devm_iio_dmaengine_buffer_alloc() - Resource-managed iio_dmaengine_buffer_alloc()
> + * @dev: Parent device for the buffer
> + * @channel: DMA channel name, typically "rx".
> + *
> + * This allocates a new IIO buffer which internally uses the DMAengine framework
> + * to perform its transfers. The parent device will be used to request the DMA
> + * channel.
> + *
> + * The buffer will be automatically de-allocated once the device gets destroyed.
> + */
> +struct iio_buffer *devm_iio_dmaengine_buffer_alloc(struct device *dev,
> +	const char *channel)
> +{
> +	struct iio_buffer **bufferp, *buffer;
> +
> +	bufferp = devres_alloc(__devm_iio_dmaengine_buffer_free,
> +			       sizeof(*bufferp), GFP_KERNEL);
> +	if (!bufferp)
> +		return ERR_PTR(-ENOMEM);
> +
> +	buffer = iio_dmaengine_buffer_alloc(dev, channel);
> +	if (IS_ERR(buffer)) {
> +		devres_free(bufferp);
> +		return buffer;
> +	}
> +
> +	*bufferp = buffer;
> +	devres_add(dev, bufferp);
> +
> +	return buffer;
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_dmaengine_buffer_alloc);
> +
>  MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
>  MODULE_DESCRIPTION("DMA buffer for the IIO framework");
>  MODULE_LICENSE("GPL");
> diff --git a/include/linux/iio/buffer-dmaengine.h b/include/linux/iio/buffer-dmaengine.h
> index b3a57444a886..0e503db71289 100644
> --- a/include/linux/iio/buffer-dmaengine.h
> +++ b/include/linux/iio/buffer-dmaengine.h
> @@ -14,4 +14,7 @@ struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
>  	const char *channel);
>  void iio_dmaengine_buffer_free(struct iio_buffer *buffer);
>  
> +struct iio_buffer *devm_iio_dmaengine_buffer_alloc(struct device *dev,
> +						   const char *channel);
> +
>  #endif

Re: [PATCH v11 6/8] dt-bindings: iio: adc: add bindings doc for AXI ADC driver

[Jonathan Cameron]

On Sat, 21 Mar 2020 10:53:13 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change adds the bindings documentation for the AXI ADC driver.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied.

Thanks,

> ---
>  .../bindings/iio/adc/adi,axi-adc.yaml         | 62 +++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml b/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
> new file mode 100644
> index 000000000000..0924b2b4972b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,axi-adc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AXI ADC IP core
> +
> +maintainers:
> +  - Michael Hennerich <michael.hennerich@analog.com>
> +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> +
> +description: |
> +  Analog Devices Generic AXI ADC IP core for interfacing an ADC device
> +  with a high speed serial (JESD204B/C) or source synchronous parallel
> +  interface (LVDS/CMOS).
> +  Usually, some other interface type (i.e SPI) is used as a control
> +  interface for the actual ADC, while this IP core will interface
> +  to the data-lines of the ADC and handle the streaming of data into
> +  memory via DMA.
> +
> +  https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,axi-adc-10.0.a
> +
> +  reg:
> +    maxItems: 1
> +
> +  dmas:
> +    maxItems: 1
> +
> +  dma-names:
> +    items:
> +      - const: rx
> +
> +  adi,adc-dev:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      A reference to a the actual ADC to which this FPGA ADC interfaces to.
> +
> +required:
> +  - compatible
> +  - dmas
> +  - reg
> +  - adi,adc-dev
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    axi-adc@44a00000 {
> +          compatible = "adi,axi-adc-10.0.a";
> +          reg = <0x44a00000 0x10000>;
> +          dmas = <&rx_dma 0>;
> +          dma-names = "rx";
> +
> +          adi,adc-dev = <&spi_adc>;
> +    };
> +...

Re: [PATCH v11 7/8] iio: adc: ad9467: add support AD9467 ADC

[Jonathan Cameron]

On Sat, 21 Mar 2020 10:53:14 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> The AD9467 is a 16-bit, monolithic, IF sampling analog-to-digital converter
> (ADC). It is optimized for high performanceover wide bandwidths and ease of
> use. The product operates at a 250 MSPS conversion rate and is designed for
> wireless receivers, instrumentation, and test equipment that require a high
> dynamic range. The ADC requires 1.8 V and 3.3 V power supplies and a low
> voltage differential input clock for full performance operation. No
> external reference or driver components are required for many applications.
> Data outputs are LVDS compatible (ANSI-644 compatible) and include the
> means to reduce the overall current needed for short trace distances.
> 
> Since the chip can operate at such high sample-rates (much higher than
> classical interfaces), it requires that a DMA controller be used to
> interface directly to the chip and push data into memory.
> Typically, the AXI ADC IP core is used to interface with it.
> 
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD9467.pdf
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied thanks.  

Jonathan

> ---
>  drivers/iio/adc/Kconfig  |  15 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad9467.c | 420 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 436 insertions(+)
>  create mode 100644 drivers/iio/adc/ad9467.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 445070abf376..a0796510f9d4 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -246,6 +246,21 @@ config AD799X
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad799x.
>  
> +config AD9467
> +	tristate "Analog Devices AD9467 High Speed ADC driver"
> +	depends on SPI
> +	select ADI_AXI_ADC
> +	help
> +	  Say yes here to build support for Analog Devices:
> +	  * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> +
> +	  The driver requires the assistance of the AXI ADC IP core to operate,
> +	  since SPI is used for configuration only, while data has to be
> +	  streamed into memory via DMA.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad9467.
> +
>  config ADI_AXI_ADC
>  	tristate "Analog Devices Generic AXI ADC IP core driver"
>  	select IIO_BUFFER
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7c6594d049f9..59722770a654 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
>  obj-$(CONFIG_AD7949) += ad7949.o
>  obj-$(CONFIG_AD799X) += ad799x.o
> +obj-$(CONFIG_AD9467) += ad9467.o
>  obj-$(CONFIG_ADI_AXI_ADC) += adi-axi-adc.o
>  obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> new file mode 100644
> index 000000000000..362993daedaf
> --- /dev/null
> +++ b/drivers/iio/adc/ad9467.c
> @@ -0,0 +1,420 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD9467 SPI ADC driver
> + *
> + * Copyright 2012-2020 Analog Devices Inc.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_device.h>
> +
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#include <linux/clk.h>
> +
> +#include <linux/iio/adc/adi-axi-adc.h>
> +
> +/*
> + * ADI High-Speed ADC common spi interface registers
> + * See Application-Note AN-877:
> + *   https://www.analog.com/media/en/technical-documentation/application-notes/AN-877.pdf
> + */
> +
> +#define AN877_ADC_REG_CHIP_PORT_CONF		0x00
> +#define AN877_ADC_REG_CHIP_ID			0x01
> +#define AN877_ADC_REG_CHIP_GRADE		0x02
> +#define AN877_ADC_REG_CHAN_INDEX		0x05
> +#define AN877_ADC_REG_TRANSFER			0xFF
> +#define AN877_ADC_REG_MODES			0x08
> +#define AN877_ADC_REG_TEST_IO			0x0D
> +#define AN877_ADC_REG_ADC_INPUT			0x0F
> +#define AN877_ADC_REG_OFFSET			0x10
> +#define AN877_ADC_REG_OUTPUT_MODE		0x14
> +#define AN877_ADC_REG_OUTPUT_ADJUST		0x15
> +#define AN877_ADC_REG_OUTPUT_PHASE		0x16
> +#define AN877_ADC_REG_OUTPUT_DELAY		0x17
> +#define AN877_ADC_REG_VREF			0x18
> +#define AN877_ADC_REG_ANALOG_INPUT		0x2C
> +
> +/* AN877_ADC_REG_TEST_IO */
> +#define AN877_ADC_TESTMODE_OFF			0x0
> +#define AN877_ADC_TESTMODE_MIDSCALE_SHORT	0x1
> +#define AN877_ADC_TESTMODE_POS_FULLSCALE	0x2
> +#define AN877_ADC_TESTMODE_NEG_FULLSCALE	0x3
> +#define AN877_ADC_TESTMODE_ALT_CHECKERBOARD	0x4
> +#define AN877_ADC_TESTMODE_PN23_SEQ		0x5
> +#define AN877_ADC_TESTMODE_PN9_SEQ		0x6
> +#define AN877_ADC_TESTMODE_ONE_ZERO_TOGGLE	0x7
> +#define AN877_ADC_TESTMODE_USER			0x8
> +#define AN877_ADC_TESTMODE_BIT_TOGGLE		0x9
> +#define AN877_ADC_TESTMODE_SYNC			0xA
> +#define AN877_ADC_TESTMODE_ONE_BIT_HIGH		0xB
> +#define AN877_ADC_TESTMODE_MIXED_BIT_FREQUENCY	0xC
> +#define AN877_ADC_TESTMODE_RAMP			0xF
> +
> +/* AN877_ADC_REG_TRANSFER */
> +#define AN877_ADC_TRANSFER_SYNC			0x1
> +
> +/* AN877_ADC_REG_OUTPUT_MODE */
> +#define AN877_ADC_OUTPUT_MODE_OFFSET_BINARY	0x0
> +#define AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT	0x1
> +#define AN877_ADC_OUTPUT_MODE_GRAY_CODE		0x2
> +
> +/* AN877_ADC_REG_OUTPUT_PHASE */
> +#define AN877_ADC_OUTPUT_EVEN_ODD_MODE_EN	0x20
> +#define AN877_ADC_INVERT_DCO_CLK		0x80
> +
> +/* AN877_ADC_REG_OUTPUT_DELAY */
> +#define AN877_ADC_DCO_DELAY_ENABLE		0x80
> +
> +/*
> + * Analog Devices AD9467 16-Bit, 200/250 MSPS ADC
> + */
> +
> +#define CHIPID_AD9467			0x50
> +#define AD9467_DEF_OUTPUT_MODE		0x08
> +#define AD9467_REG_VREF_MASK		0x0F
> +
> +enum {
> +	ID_AD9467,
> +};
> +
> +struct ad9467_state {
> +	struct spi_device		*spi;
> +	struct clk			*clk;
> +	unsigned int			output_mode;
> +
> +	struct gpio_desc		*pwrdown_gpio;
> +	struct gpio_desc		*reset_gpio;
> +};
> +
> +static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> +{
> +	unsigned char tbuf[2], rbuf[1];
> +	int ret;
> +
> +	tbuf[0] = 0x80 | (reg >> 8);
> +	tbuf[1] = reg & 0xFF;
> +
> +	ret = spi_write_then_read(spi,
> +				  tbuf, ARRAY_SIZE(tbuf),
> +				  rbuf, ARRAY_SIZE(rbuf));
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return rbuf[0];
> +}
> +
> +static int ad9467_spi_write(struct spi_device *spi, unsigned int reg,
> +			    unsigned int val)
> +{
> +	unsigned char buf[3];
> +
> +	buf[0] = reg >> 8;
> +	buf[1] = reg & 0xFF;
> +	buf[2] = val;
> +
> +	return spi_write(spi, buf, ARRAY_SIZE(buf));
> +}
> +
> +static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
> +			     unsigned int writeval, unsigned int *readval)
> +{
> +	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> +	struct spi_device *spi = st->spi;
> +	int ret;
> +
> +	if (readval == NULL) {
> +		ret = ad9467_spi_write(spi, reg, writeval);
> +		ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
> +				 AN877_ADC_TRANSFER_SYNC);
> +		return ret;
> +	}
> +
> +	ret = ad9467_spi_read(spi, reg);
> +	if (ret < 0)
> +		return ret;
> +	*readval = ret;
> +
> +	return 0;
> +}
> +
> +static const unsigned int ad9467_scale_table[][2] = {
> +	{2000, 0}, {2100, 6}, {2200, 7},
> +	{2300, 8}, {2400, 9}, {2500, 10},
> +};
> +
> +static void __ad9467_get_scale(struct adi_axi_adc_conv *conv, int index,
> +			       unsigned int *val, unsigned int *val2)
> +{
> +	const struct adi_axi_adc_chip_info *info = conv->chip_info;
> +	const struct iio_chan_spec *chan = &info->channels[0];
> +	unsigned int tmp;
> +
> +	tmp = (info->scale_table[index][0] * 1000000ULL) >>
> +			chan->scan_type.realbits;
> +	*val = tmp / 1000000;
> +	*val2 = tmp % 1000000;
> +}
> +
> +#define AD9467_CHAN(_chan, _si, _bits, _sign)				\
> +{									\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _chan,						\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
> +	.scan_index = _si,						\
> +	.scan_type = {							\
> +		.sign = _sign,						\
> +		.realbits = _bits,					\
> +		.storagebits = 16,					\
> +	},								\
> +}
> +
> +static const struct iio_chan_spec ad9467_channels[] = {
> +	AD9467_CHAN(0, 0, 16, 'S'),
> +};
> +
> +static const struct adi_axi_adc_chip_info ad9467_chip_tbl[] = {
> +	[ID_AD9467] = {
> +		.id = CHIPID_AD9467,
> +		.max_rate = 250000000UL,
> +		.scale_table = ad9467_scale_table,
> +		.num_scales = ARRAY_SIZE(ad9467_scale_table),
> +		.channels = ad9467_channels,
> +		.num_channels = ARRAY_SIZE(ad9467_channels),
> +	},
> +};
> +
> +static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
> +{
> +	const struct adi_axi_adc_chip_info *info = conv->chip_info;
> +	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> +	unsigned int i, vref_val, vref_mask;
> +
> +	vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
> +
> +	switch (info->id) {
> +	case CHIPID_AD9467:
> +		vref_mask = AD9467_REG_VREF_MASK;
> +		break;
> +	default:
> +		vref_mask = 0xFFFF;
> +		break;
> +	}
> +
> +	vref_val &= vref_mask;
> +
> +	for (i = 0; i < info->num_scales; i++) {
> +		if (vref_val == info->scale_table[i][1])
> +			break;
> +	}
> +
> +	if (i == info->num_scales)
> +		return -ERANGE;
> +
> +	__ad9467_get_scale(conv, i, val, val2);
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
> +{
> +	const struct adi_axi_adc_chip_info *info = conv->chip_info;
> +	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> +	unsigned int scale_val[2];
> +	unsigned int i;
> +
> +	if (val != 0)
> +		return -EINVAL;
> +
> +	for (i = 0; i < info->num_scales; i++) {
> +		__ad9467_get_scale(conv, i, &scale_val[0], &scale_val[1]);
> +		if (scale_val[0] != val || scale_val[1] != val2)
> +			continue;
> +
> +		ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> +				 info->scale_table[i][1]);
> +		ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
> +				 AN877_ADC_TRANSFER_SYNC);
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad9467_read_raw(struct adi_axi_adc_conv *conv,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long m)
> +{
> +	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad9467_get_scale(conv, val, val2);
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = clk_get_rate(st->clk);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	const struct adi_axi_adc_chip_info *info = conv->chip_info;
> +	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> +	unsigned long r_clk;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad9467_set_scale(conv, val, val2);
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		r_clk = clk_round_rate(st->clk, val);
> +		if (r_clk < 0 || r_clk > info->max_rate) {
> +			dev_warn(&st->spi->dev,
> +				 "Error setting ADC sample rate %ld", r_clk);
> +			return -EINVAL;
> +		}
> +
> +		return clk_set_rate(st->clk, r_clk);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
> +{
> +	int ret;
> +
> +	ret = ad9467_spi_write(spi, AN877_ADC_REG_OUTPUT_MODE, mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
> +				AN877_ADC_TRANSFER_SYNC);
> +}
> +
> +static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
> +{
> +	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> +
> +	return ad9467_outputmode_set(st->spi, st->output_mode);
> +}
> +
> +static int ad9467_setup(struct ad9467_state *st, unsigned int chip_id)
> +{
> +	switch (chip_id) {
> +	case CHIPID_AD9467:
> +		st->output_mode = AD9467_DEF_OUTPUT_MODE |
> +				  AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static void ad9467_clk_disable(void *data)
> +{
> +	struct ad9467_state *st = data;
> +
> +	clk_disable_unprepare(st->clk);
> +}
> +
> +static const struct of_device_id ad9467_of_match[] = {
> +	{ .compatible = "adi,ad9467", .data = &ad9467_chip_tbl[ID_AD9467], },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ad9467_of_match);
> +
> +static int ad9467_probe(struct spi_device *spi)
> +{
> +	const struct adi_axi_adc_chip_info *info;
> +	struct adi_axi_adc_conv *conv;
> +	struct ad9467_state *st;
> +	unsigned int id;
> +	int ret;
> +
> +	info = of_device_get_match_data(&spi->dev);
> +	if (!info)
> +		return -ENODEV;
> +
> +	conv = devm_adi_axi_adc_conv_register(&spi->dev, sizeof(*st));
> +	if (IS_ERR(conv))
> +		return PTR_ERR(conv);
> +
> +	st = adi_axi_adc_conv_priv(conv);
> +	st->spi = spi;
> +
> +	st->clk = devm_clk_get(&spi->dev, "adc-clk");
> +	if (IS_ERR(st->clk))
> +		return PTR_ERR(st->clk);
> +
> +	ret = clk_prepare_enable(st->clk);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(&spi->dev, ad9467_clk_disable, st);
> +	if (ret)
> +		return ret;
> +
> +	st->pwrdown_gpio = devm_gpiod_get_optional(&spi->dev, "powerdown",
> +						   GPIOD_OUT_LOW);
> +	if (IS_ERR(st->pwrdown_gpio))
> +		return PTR_ERR(st->pwrdown_gpio);
> +
> +	st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> +						 GPIOD_OUT_LOW);
> +	if (IS_ERR(st->reset_gpio))
> +		return PTR_ERR(st->reset_gpio);
> +
> +	if (st->reset_gpio) {
> +		udelay(1);
> +		ret = gpiod_direction_output(st->reset_gpio, 1);
> +		mdelay(10);
> +	}
> +
> +	spi_set_drvdata(spi, st);
> +
> +	conv->chip_info = info;
> +
> +	id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
> +	if (id != conv->chip_info->id) {
> +		dev_err(&spi->dev, "Unrecognized CHIP_ID 0x%X\n", id);
> +		return -ENODEV;
> +	}
> +
> +	conv->reg_access = ad9467_reg_access;
> +	conv->write_raw = ad9467_write_raw;
> +	conv->read_raw = ad9467_read_raw;
> +	conv->preenable_setup = ad9467_preenable_setup;
> +
> +	return ad9467_setup(st, id);
> +}
> +
> +static struct spi_driver ad9467_driver = {
> +	.driver = {
> +		.name = "ad9467",
> +		.of_match_table = ad9467_of_match,
> +	},
> +	.probe = ad9467_probe,
> +};
> +module_spi_driver(ad9467_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices AD9467 ADC driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v11 8/8] dt-bindings: iio: adc: add bindings doc for AD9467 ADC

[Jonathan Cameron]

On Sat, 21 Mar 2020 10:53:15 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change adds the binding doc for the AD9467 ADC.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied thanks. 

Great to have the first of these high speed devices upstream :)

Thanks,

Jonathan

> ---
>  .../bindings/iio/adc/adi,ad9467.yaml          | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
> new file mode 100644
> index 000000000000..c4f57fa6aad1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad9467.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD9467 High-Speed ADC
> +
> +maintainers:
> +  - Michael Hennerich <michael.hennerich@analog.com>
> +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> +
> +description: |
> +  The AD9467 is a 16-bit, monolithic, IF sampling analog-to-digital
> +  converter (ADC).
> +
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD9467.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad9467
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: adc-clk
> +
> +  powerdown-gpios:
> +    description:
> +      Pin that controls the powerdown mode of the device.
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description:
> +      Reset pin for the device.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +          compatible = "adi,ad9467";
> +          reg = <0>;
> +          clocks = <&adc_clk>;
> +          clock-names = "adc-clk";
> +        };
> +    };
> +...

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Andy Shevchenko]

On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
<alexandru.ardelean@analog.com> wrote:
>
> From: Michael Hennerich <michael.hennerich@analog.com>
>
> This change adds support for the Analog Devices Generic AXI ADC IP core.
> The IP core is used for interfacing with analog-to-digital (ADC) converters
> that require either a high-speed serial interface (JESD204B/C) or a source
> synchronous parallel interface (LVDS/CMOS).
>
> Usually, some other interface type (i.e SPI) is used as a control interface
> for the actual ADC, while the IP core (controlled via this driver), will
> interface to the data-lines of the ADC and handle  the streaming of data
> into memory via DMA.
>
> Because of this, the AXI ADC driver needs the other SPI-ADC driver to
> register with it. The SPI-ADC needs to be register via the SPI framework,
> while the AXI ADC registers as a platform driver. The two cannot be ordered
> in a hierarchy as both drivers have their own registers, and trying to
> organize this [in a hierarchy becomes] problematic when trying to map
> memory/registers.
>
> There are some modes where the AXI ADC can operate as standalone ADC, but
> those will be implemented at a later point in time.
>

> Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip

Is it tag or simple link? I would suggest not to use Link: if it's not a tag.

...

> +static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv *conv)
> +{

> +       if (!conv)
> +               return NULL;

This is so unusual. Why do you need it?

> +       return container_of(conv, struct adi_axi_adc_client, conv);
> +}

> +
> +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
> +{
> +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> +

> +       if (!cl)
> +               return NULL;

So about this.

> +
> +       return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client), IIO_ALIGN);

This all looks a bit confusing. Is it invention of offsetof() ?

> +}

...

> +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device *dev,
> +                                                         int sizeof_priv)
> +{
> +       struct adi_axi_adc_client *cl;
> +       size_t alloc_size;
> +
> +       alloc_size = sizeof(struct adi_axi_adc_client);
> +       if (sizeof_priv) {
> +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> +               alloc_size += sizeof_priv;
> +       }
> +       alloc_size += IIO_ALIGN - 1;

Have you looked at linux/overflow.h?

> +       cl = kzalloc(alloc_size, GFP_KERNEL);
> +       if (!cl)
> +               return ERR_PTR(-ENOMEM);
> +
> +       mutex_lock(&registered_clients_lock);
> +

> +       get_device(dev);
> +       cl->dev = dev;

cl->dev = get_device(dev);

> +       list_add_tail(&cl->entry, &registered_clients);
> +
> +       mutex_unlock(&registered_clients_lock);
> +
> +       return &cl->conv;
> +}
> +
> +static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> +{
> +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> +

> +       if (!cl)
> +               return;

When is this possible?

> +
> +       mutex_lock(&registered_clients_lock);
> +
> +       list_del(&cl->entry);
> +       put_device(cl->dev);
> +
> +       mutex_unlock(&registered_clients_lock);
> +
> +       kfree(cl);
> +}

...

> +static ssize_t in_voltage_scale_available_show(struct device *dev,
> +                                              struct device_attribute *attr,
> +                                              char *buf)
> +{

> +       for (i = 0; i < conv->chip_info->num_scales; i++) {
> +               const unsigned int *s = conv->chip_info->scale_table[i];
> +
> +               len += scnprintf(buf + len, PAGE_SIZE - len,
> +                                "%u.%06u ", s[0], s[1]);
> +       }

> +       buf[len - 1] = '\n';

Is num_scales guaranteed to be great than 0 whe we call this?

> +
> +       return len;
> +}

...

> +static struct attribute *adi_axi_adc_attributes[] = {
> +       ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),

> +       NULL,

Terminators good w/o comma.

> +};

...

> +/* Match table for of_platform binding */
> +static const struct of_device_id adi_axi_adc_of_match[] = {
> +       { .compatible = "adi,axi-adc-10.0.a", .data = &adi_axi_adc_10_0_a_info },

> +       { /* end of list */ },

Ditto.

> +};

...

> +struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)
> +{
> +       const struct of_device_id *id;
> +       struct adi_axi_adc_client *cl;
> +       struct device_node *cln;
> +
> +       if (!dev->of_node) {
> +               dev_err(dev, "DT node is null\n");
> +               return ERR_PTR(-ENODEV);
> +       }
> +

> +       id = of_match_node(adi_axi_adc_of_match, dev->of_node);

You may use this from struct driver and move the table after this function.

> +       if (!id)
> +               return ERR_PTR(-ENODEV);
> +
> +       cln = of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
> +       if (!cln) {
> +               dev_err(dev, "No 'adi,adc-dev' node defined\n");
> +               return ERR_PTR(-ENODEV);
> +       }
> +
> +       mutex_lock(&registered_clients_lock);
> +
> +       list_for_each_entry(cl, &registered_clients, entry) {
> +               if (!cl->dev)
> +                       continue;

> +               if (cl->dev->of_node == cln) {

So, why not to be consistent with above, i.e.
  if (of_node != cln)
    continue;
?

> +                       if (!try_module_get(dev->driver->owner)) {
> +                               mutex_unlock(&registered_clients_lock);
> +                               return ERR_PTR(-ENODEV);
> +                       }
> +                       get_device(dev);
> +                       cl->info = id->data;
> +                       mutex_unlock(&registered_clients_lock);
> +                       return cl;
> +               }
> +       }
> +
> +       mutex_unlock(&registered_clients_lock);
> +
> +       return ERR_PTR(-EPROBE_DEFER);
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Ardelean, Alexandru]

On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> [External]
> 
> On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> <alexandru.ardelean@analog.com> wrote:
> > From: Michael Hennerich <michael.hennerich@analog.com>
> > 
> > This change adds support for the Analog Devices Generic AXI ADC IP core.
> > The IP core is used for interfacing with analog-to-digital (ADC) converters
> > that require either a high-speed serial interface (JESD204B/C) or a source
> > synchronous parallel interface (LVDS/CMOS).
> > 
> > Usually, some other interface type (i.e SPI) is used as a control interface
> > for the actual ADC, while the IP core (controlled via this driver), will
> > interface to the data-lines of the ADC and handle  the streaming of data
> > into memory via DMA.
> > 
> > Because of this, the AXI ADC driver needs the other SPI-ADC driver to
> > register with it. The SPI-ADC needs to be register via the SPI framework,
> > while the AXI ADC registers as a platform driver. The two cannot be ordered
> > in a hierarchy as both drivers have their own registers, and trying to
> > organize this [in a hierarchy becomes] problematic when trying to map
> > memory/registers.
> > 
> > There are some modes where the AXI ADC can operate as standalone ADC, but
> > those will be implemented at a later point in time.
> > 
> > Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> 

i can send a v12 for this in a few days;

> Is it tag or simple link? I would suggest not to use Link: if it's not a tag.

simple link
any suggestions/alternatives?
i wasn't aware of conventions about this;

> 
> ...
> 
> > +static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv
> > *conv)
> > +{
> > +       if (!conv)
> > +               return NULL;
> 
> This is so unusual. Why do you need it?

see [1]

> 
> > +       return container_of(conv, struct adi_axi_adc_client, conv);
> > +}
> > +
> > +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
> > +{
> > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > +
> > +       if (!cl)
> > +               return NULL;
> 
> So about this.

[1]
because 'adi_axi_adc_conv_priv()' (and implicitly conv_to_client()) gets called
from other drivers; we can't expect to be sure that conv & cl aren't NULL;

> 
> > +
> > +       return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
> > IIO_ALIGN);
> 
> This all looks a bit confusing. Is it invention of offsetof() ?

umm; tbh, it's more of a copy/clone of iio_priv() 

it's not un-common though;
see [and this one has more exposure]:
--------------------------------------------------------
static inline void *netdev_priv(const struct net_device *dev)
{       
        return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
}
--------------------------------------------------------


> 
> > +}
> 
> ...
> 
> > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device
> > *dev,
> > +                                                         int sizeof_priv)
> > +{
> > +       struct adi_axi_adc_client *cl;
> > +       size_t alloc_size;
> > +
> > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > +       if (sizeof_priv) {
> > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > +               alloc_size += sizeof_priv;
> > +       }
> > +       alloc_size += IIO_ALIGN - 1;
> 
> Have you looked at linux/overflow.h?

i did now;
any hints where i should look closer?

> 
> > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > +       if (!cl)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       mutex_lock(&registered_clients_lock);
> > +
> > +       get_device(dev);
> > +       cl->dev = dev;
> 
> cl->dev = get_device(dev);

sure

> 
> > +       list_add_tail(&cl->entry, &registered_clients);
> > +
> > +       mutex_unlock(&registered_clients_lock);
> > +
> > +       return &cl->conv;
> > +}
> > +
> > +static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> > +{
> > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > +
> > +       if (!cl)
> > +               return;
> 
> When is this possible?

good point; it isn't;
it's a left-over from when adi_axi_adc_conv_unregister() was exported
still, i wouldn't mind leaving it [for paranoia], if there isn't a strong
opinion to remove it;

> 
> > +
> > +       mutex_lock(&registered_clients_lock);
> > +
> > +       list_del(&cl->entry);
> > +       put_device(cl->dev);
> > +
> > +       mutex_unlock(&registered_clients_lock);
> > +
> > +       kfree(cl);
> > +}
> 
> ...
> 
> > +static ssize_t in_voltage_scale_available_show(struct device *dev,
> > +                                              struct device_attribute
> > *attr,
> > +                                              char *buf)
> > +{
> > +       for (i = 0; i < conv->chip_info->num_scales; i++) {
> > +               const unsigned int *s = conv->chip_info->scale_table[i];
> > +
> > +               len += scnprintf(buf + len, PAGE_SIZE - len,
> > +                                "%u.%06u ", s[0], s[1]);
> > +       }
> > +       buf[len - 1] = '\n';
> 
> Is num_scales guaranteed to be great than 0 whe we call this?

yes
see axi_adc_attr_is_visible()

> 
> > +
> > +       return len;
> > +}
> 
> ...
> 
> > +static struct attribute *adi_axi_adc_attributes[] = {
> > +       ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> > +       NULL,
> 
> Terminators good w/o comma.

i don't feel strongly pro/against
sure

> 
> > +};
> 
> ...
> 
> > +/* Match table for of_platform binding */
> > +static const struct of_device_id adi_axi_adc_of_match[] = {
> > +       { .compatible = "adi,axi-adc-10.0.a", .data =
> > &adi_axi_adc_10_0_a_info },
> > +       { /* end of list */ },
> 
> Ditto.
> 
> > +};
> 
> ...
> 
> > +struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)
> > +{
> > +       const struct of_device_id *id;
> > +       struct adi_axi_adc_client *cl;
> > +       struct device_node *cln;
> > +
> > +       if (!dev->of_node) {
> > +               dev_err(dev, "DT node is null\n");
> > +               return ERR_PTR(-ENODEV);
> > +       }
> > +
> > +       id = of_match_node(adi_axi_adc_of_match, dev->of_node);
> 
> You may use this from struct driver and move the table after this function.


right; it didn't occur to me, since i was already using
of_device_get_match_data() in ad9467

> 
> > +       if (!id)
> > +               return ERR_PTR(-ENODEV);
> > +
> > +       cln = of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
> > +       if (!cln) {
> > +               dev_err(dev, "No 'adi,adc-dev' node defined\n");
> > +               return ERR_PTR(-ENODEV);
> > +       }
> > +
> > +       mutex_lock(&registered_clients_lock);
> > +
> > +       list_for_each_entry(cl, &registered_clients, entry) {
> > +               if (!cl->dev)
> > +                       continue;
> > +               if (cl->dev->of_node == cln) {
> 
> So, why not to be consistent with above, i.e.
>   if (of_node != cln)
>     continue;

sure

> ?
> 
> > +                       if (!try_module_get(dev->driver->owner)) {
> > +                               mutex_unlock(&registered_clients_lock);
> > +                               return ERR_PTR(-ENODEV);
> > +                       }
> > +                       get_device(dev);
> > +                       cl->info = id->data;
> > +                       mutex_unlock(&registered_clients_lock);
> > +                       return cl;
> > +               }
> > +       }
> > +
> > +       mutex_unlock(&registered_clients_lock);
> > +
> > +       return ERR_PTR(-EPROBE_DEFER);
> > +}
> 
> 

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Andy Shevchenko]

+Cc Kees (see below about allocation size checks)

On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
<alexandru.Ardelean@analog.com> wrote:
> On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > <alexandru.ardelean@analog.com> wrote:

...

> > > Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> >
>
> i can send a v12 for this in a few days;
>
> > Is it tag or simple link? I would suggest not to use Link: if it's not a tag.
>
> simple link
> any suggestions/alternatives?
> i wasn't aware of conventions about this;

Use like [1] ...
...

[1]: https://...

Or maybe introduce is as a tag DocLink:, for example?
Or Datasheet: ?

...

> > > +static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv
> > > *conv)
> > > +{
> > > +       if (!conv)
> > > +               return NULL;
> >
> > This is so unusual. Why do you need it?
>
> see [1]
>
> >
> > > +       return container_of(conv, struct adi_axi_adc_client, conv);
> > > +}
> > > +
> > > +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
> > > +{
> > > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > +
> > > +       if (!cl)
> > > +               return NULL;
> >
> > So about this.
>
> [1]
> because 'adi_axi_adc_conv_priv()' (and implicitly conv_to_client()) gets called
> from other drivers; we can't expect to be sure that conv & cl aren't NULL;

In both cases it's pointer arithmetic, right? Even look at the example
of netdev you gave below, they haven't done these (redundant) checks.
The outcome that crashes if any will be more distinct.

> > > +       return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
> > > IIO_ALIGN);
> >
> > This all looks a bit confusing. Is it invention of offsetof() ?
>
> umm; tbh, it's more of a copy/clone of iio_priv()
>
> it's not un-common though;
> see [and this one has more exposure]:
> --------------------------------------------------------
> static inline void *netdev_priv(const struct net_device *dev)
> {
>         return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
> }
> --------------------------------------------------------

Good point.

> > > +}

...

> > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device
> > > *dev,
> > > +                                                         int sizeof_priv)
> > > +{
> > > +       struct adi_axi_adc_client *cl;
> > > +       size_t alloc_size;
> > > +
> > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > +       if (sizeof_priv) {
> > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > +               alloc_size += sizeof_priv;
> > > +       }
> > > +       alloc_size += IIO_ALIGN - 1;
> >
> > Have you looked at linux/overflow.h?
>
> i did now;
> any hints where i should look closer?

It seems it lacks of this kind of allocation size checks... Perhaps add one?
Kees, what do you think?

> > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > +       if (!cl)
> > > +               return ERR_PTR(-ENOMEM);

...

> > > +static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> > > +{
> > > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > +
> > > +       if (!cl)
> > > +               return;
> >
> > When is this possible?
>
> good point; it isn't;
> it's a left-over from when adi_axi_adc_conv_unregister() was exported
> still, i wouldn't mind leaving it [for paranoia], if there isn't a strong
> opinion to remove it;

I think it makes code dirty (too much protective programming). We have
a lot places where we can shoot our feet, but at least not hiding the
issue is a benefit in my opinion.

...



> > > +static struct attribute *adi_axi_adc_attributes[] = {
> > > +       ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> > > +       NULL,
> >
> > Terminators good w/o comma.
>
> i don't feel strongly pro/against
> sure

There is a rationale behind this. If there is a weird case of adding
entry behind the terminator, you will see it immediately at compile
time (consider automatic rebase).

> > > +};
> >
> > ...
> >
> > > +/* Match table for of_platform binding */
> > > +static const struct of_device_id adi_axi_adc_of_match[] = {
> > > +       { .compatible = "adi,axi-adc-10.0.a", .data =
> > > &adi_axi_adc_10_0_a_info },
> > > +       { /* end of list */ },
> >
> > Ditto.

Ditto.

> > > +};

...

> > > +       if (!dev->of_node) {
> > > +               dev_err(dev, "DT node is null\n");
> > > +               return ERR_PTR(-ENODEV);
> > > +       }

I guess this check is redundant since following OF calls will fail anyway.

> > > +
> > > +       id = of_match_node(adi_axi_adc_of_match, dev->of_node);
> >
> > You may use this from struct driver and move the table after this function.
>
>
> right; it didn't occur to me, since i was already using
> of_device_get_match_data() in ad9467

Even better. But see above note.

> > > +       if (!id)
> > > +               return ERR_PTR(-ENODEV);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Jonathan Cameron]

On Sun, 22 Mar 2020 09:35:57 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> > [External]
> > 
> > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > <alexandru.ardelean@analog.com> wrote:  
> > > From: Michael Hennerich <michael.hennerich@analog.com>
> > > 
> > > This change adds support for the Analog Devices Generic AXI ADC IP core.
> > > The IP core is used for interfacing with analog-to-digital (ADC) converters
> > > that require either a high-speed serial interface (JESD204B/C) or a source
> > > synchronous parallel interface (LVDS/CMOS).
> > > 
> > > Usually, some other interface type (i.e SPI) is used as a control interface
> > > for the actual ADC, while the IP core (controlled via this driver), will
> > > interface to the data-lines of the ADC and handle  the streaming of data
> > > into memory via DMA.
> > > 
> > > Because of this, the AXI ADC driver needs the other SPI-ADC driver to
> > > register with it. The SPI-ADC needs to be register via the SPI framework,
> > > while the AXI ADC registers as a platform driver. The two cannot be ordered
> > > in a hierarchy as both drivers have their own registers, and trying to
> > > organize this [in a hierarchy becomes] problematic when trying to map
> > > memory/registers.
> > > 
> > > There are some modes where the AXI ADC can operate as standalone ADC, but
> > > those will be implemented at a later point in time.
> > > 
> > > Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip  
> >   
> 
> i can send a v12 for this in a few days;
I'll drop v11 of the series then.

Jonathan


> 
> > Is it tag or simple link? I would suggest not to use Link: if it's not a tag.  
> 
> simple link
> any suggestions/alternatives?
> i wasn't aware of conventions about this;
> 
> > 
> > ...
> >   
> > > +static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv
> > > *conv)
> > > +{
> > > +       if (!conv)
> > > +               return NULL;  
> > 
> > This is so unusual. Why do you need it?  
> 
> see [1]
> 
> >   
> > > +       return container_of(conv, struct adi_axi_adc_client, conv);
> > > +}
> > > +
> > > +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
> > > +{
> > > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > +
> > > +       if (!cl)
> > > +               return NULL;  
> > 
> > So about this.  
> 
> [1]
> because 'adi_axi_adc_conv_priv()' (and implicitly conv_to_client()) gets called
> from other drivers; we can't expect to be sure that conv & cl aren't NULL;
> 
> >   
> > > +
> > > +       return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
> > > IIO_ALIGN);  
> > 
> > This all looks a bit confusing. Is it invention of offsetof() ?  
> 
> umm; tbh, it's more of a copy/clone of iio_priv() 
> 
> it's not un-common though;
> see [and this one has more exposure]:
> --------------------------------------------------------
> static inline void *netdev_priv(const struct net_device *dev)
> {       
>         return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
> }
> --------------------------------------------------------
> 
> 
> >   
> > > +}  
> > 
> > ...
> >   
> > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device
> > > *dev,
> > > +                                                         int sizeof_priv)
> > > +{
> > > +       struct adi_axi_adc_client *cl;
> > > +       size_t alloc_size;
> > > +
> > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > +       if (sizeof_priv) {
> > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > +               alloc_size += sizeof_priv;
> > > +       }
> > > +       alloc_size += IIO_ALIGN - 1;  
> > 
> > Have you looked at linux/overflow.h?  
> 
> i did now;
> any hints where i should look closer?
> 
> >   
> > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > +       if (!cl)
> > > +               return ERR_PTR(-ENOMEM);
> > > +
> > > +       mutex_lock(&registered_clients_lock);
> > > +
> > > +       get_device(dev);
> > > +       cl->dev = dev;  
> > 
> > cl->dev = get_device(dev);  
> 
> sure
> 
> >   
> > > +       list_add_tail(&cl->entry, &registered_clients);
> > > +
> > > +       mutex_unlock(&registered_clients_lock);
> > > +
> > > +       return &cl->conv;
> > > +}
> > > +
> > > +static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> > > +{
> > > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > +
> > > +       if (!cl)
> > > +               return;  
> > 
> > When is this possible?  
> 
> good point; it isn't;
> it's a left-over from when adi_axi_adc_conv_unregister() was exported
> still, i wouldn't mind leaving it [for paranoia], if there isn't a strong
> opinion to remove it;
> 
> >   
> > > +
> > > +       mutex_lock(&registered_clients_lock);
> > > +
> > > +       list_del(&cl->entry);
> > > +       put_device(cl->dev);
> > > +
> > > +       mutex_unlock(&registered_clients_lock);
> > > +
> > > +       kfree(cl);
> > > +}  
> > 
> > ...
> >   
> > > +static ssize_t in_voltage_scale_available_show(struct device *dev,
> > > +                                              struct device_attribute
> > > *attr,
> > > +                                              char *buf)
> > > +{
> > > +       for (i = 0; i < conv->chip_info->num_scales; i++) {
> > > +               const unsigned int *s = conv->chip_info->scale_table[i];
> > > +
> > > +               len += scnprintf(buf + len, PAGE_SIZE - len,
> > > +                                "%u.%06u ", s[0], s[1]);
> > > +       }
> > > +       buf[len - 1] = '\n';  
> > 
> > Is num_scales guaranteed to be great than 0 whe we call this?  
> 
> yes
> see axi_adc_attr_is_visible()
> 
> >   
> > > +
> > > +       return len;
> > > +}  
> > 
> > ...
> >   
> > > +static struct attribute *adi_axi_adc_attributes[] = {
> > > +       ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> > > +       NULL,  
> > 
> > Terminators good w/o comma.  
> 
> i don't feel strongly pro/against
> sure
> 
> >   
> > > +};  
> > 
> > ...
> >   
> > > +/* Match table for of_platform binding */
> > > +static const struct of_device_id adi_axi_adc_of_match[] = {
> > > +       { .compatible = "adi,axi-adc-10.0.a", .data =
> > > &adi_axi_adc_10_0_a_info },
> > > +       { /* end of list */ },  
> > 
> > Ditto.
> >   
> > > +};  
> > 
> > ...
> >   
> > > +struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)
> > > +{
> > > +       const struct of_device_id *id;
> > > +       struct adi_axi_adc_client *cl;
> > > +       struct device_node *cln;
> > > +
> > > +       if (!dev->of_node) {
> > > +               dev_err(dev, "DT node is null\n");
> > > +               return ERR_PTR(-ENODEV);
> > > +       }
> > > +
> > > +       id = of_match_node(adi_axi_adc_of_match, dev->of_node);  
> > 
> > You may use this from struct driver and move the table after this function.  
> 
> 
> right; it didn't occur to me, since i was already using
> of_device_get_match_data() in ad9467
> 
> >   
> > > +       if (!id)
> > > +               return ERR_PTR(-ENODEV);
> > > +
> > > +       cln = of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
> > > +       if (!cln) {
> > > +               dev_err(dev, "No 'adi,adc-dev' node defined\n");
> > > +               return ERR_PTR(-ENODEV);
> > > +       }
> > > +
> > > +       mutex_lock(&registered_clients_lock);
> > > +
> > > +       list_for_each_entry(cl, &registered_clients, entry) {
> > > +               if (!cl->dev)
> > > +                       continue;
> > > +               if (cl->dev->of_node == cln) {  
> > 
> > So, why not to be consistent with above, i.e.
> >   if (of_node != cln)
> >     continue;  
> 
> sure
> 
> > ?
> >   
> > > +                       if (!try_module_get(dev->driver->owner)) {
> > > +                               mutex_unlock(&registered_clients_lock);
> > > +                               return ERR_PTR(-ENODEV);
> > > +                       }
> > > +                       get_device(dev);
> > > +                       cl->info = id->data;
> > > +                       mutex_unlock(&registered_clients_lock);
> > > +                       return cl;
> > > +               }
> > > +       }
> > > +
> > > +       mutex_unlock(&registered_clients_lock);
> > > +
> > > +       return ERR_PTR(-EPROBE_DEFER);
> > > +}  
> > 
> >   

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Ardelean, Alexandru]

On Sun, 2020-03-22 at 12:45 +0200, Andy Shevchenko wrote:
> +Cc Kees (see below about allocation size checks)
> 
> On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> wrote:
> > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > <alexandru.ardelean@analog.com> wrote:
> 
> ...
> 
> > > > Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> > 
> > i can send a v12 for this in a few days;
> > 
> > > Is it tag or simple link? I would suggest not to use Link: if it's not a
> > > tag.
> > 
> > simple link
> > any suggestions/alternatives?
> > i wasn't aware of conventions about this;
> 
> Use like [1] ...
> ...
> 
> [1]: https://...
> 
> Or maybe introduce is as a tag DocLink:, for example?
> Or Datasheet: ?
> 
> ...
> 
> > > > +static struct adi_axi_adc_client *conv_to_client(struct
> > > > adi_axi_adc_conv
> > > > *conv)
> > > > +{
> > > > +       if (!conv)
> > > > +               return NULL;
> > > 
> > > This is so unusual. Why do you need it?
> > 
> > see [1]
> > 
> > > > +       return container_of(conv, struct adi_axi_adc_client, conv);
> > > > +}
> > > > +
> > > > +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
> > > > +{
> > > > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > > +
> > > > +       if (!cl)
> > > > +               return NULL;
> > > 
> > > So about this.
> > 
> > [1]
> > because 'adi_axi_adc_conv_priv()' (and implicitly conv_to_client()) gets
> > called
> > from other drivers; we can't expect to be sure that conv & cl aren't NULL;
> 
> In both cases it's pointer arithmetic, right? Even look at the example
> of netdev you gave below, they haven't done these (redundant) checks.
> The outcome that crashes if any will be more distinct.
> 
> > > > +       return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
> > > > IIO_ALIGN);
> > > 
> > > This all looks a bit confusing. Is it invention of offsetof() ?
> > 
> > umm; tbh, it's more of a copy/clone of iio_priv()
> > 
> > it's not un-common though;
> > see [and this one has more exposure]:
> > --------------------------------------------------------
> > static inline void *netdev_priv(const struct net_device *dev)
> > {
> >         return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
> > }
> > --------------------------------------------------------
> 
> Good point.
> 
> > > > +}
> 
> ...
> 
> > > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device
> > > > *dev,
> > > > +                                                         int
> > > > sizeof_priv)
> > > > +{
> > > > +       struct adi_axi_adc_client *cl;
> > > > +       size_t alloc_size;
> > > > +
> > > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > > +       if (sizeof_priv) {
> > > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > +               alloc_size += sizeof_priv;
> > > > +       }
> > > > +       alloc_size += IIO_ALIGN - 1;
> > > 
> > > Have you looked at linux/overflow.h?
> > 
> > i did now;
> > any hints where i should look closer?
> 
> It seems it lacks of this kind of allocation size checks... Perhaps add one?
> Kees, what do you think?

i borrowed this allocation logic from IIO core [iio_device_alloc()];

i may be stupid, but i still don't understand how to use overflow.h or what to
get from it;
the checks in there seem to be a bit too much for what's needed here;
or maybe there is something else in some newer linux-tree?
or maybe an example of how it's used?

> 
> > > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > +       if (!cl)
> > > > +               return ERR_PTR(-ENOMEM);
> 
> ...
> 
> > > > +static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> > > > +{
> > > > +       struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > > +
> > > > +       if (!cl)
> > > > +               return;
> > > 
> > > When is this possible?
> > 
> > good point; it isn't;
> > it's a left-over from when adi_axi_adc_conv_unregister() was exported
> > still, i wouldn't mind leaving it [for paranoia], if there isn't a strong
> > opinion to remove it;
> 
> I think it makes code dirty (too much protective programming). We have
> a lot places where we can shoot our feet, but at least not hiding the
> issue is a benefit in my opinion.
> 
> ...
> 
> 
> 
> > > > +static struct attribute *adi_axi_adc_attributes[] = {
> > > > +       ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> > > > +       NULL,
> > > 
> > > Terminators good w/o comma.
> > 
> > i don't feel strongly pro/against
> > sure
> 
> There is a rationale behind this. If there is a weird case of adding
> entry behind the terminator, you will see it immediately at compile
> time (consider automatic rebase).
> 
> > > > +};
> > > 
> > > ...
> > > 
> > > > +/* Match table for of_platform binding */
> > > > +static const struct of_device_id adi_axi_adc_of_match[] = {
> > > > +       { .compatible = "adi,axi-adc-10.0.a", .data =
> > > > &adi_axi_adc_10_0_a_info },
> > > > +       { /* end of list */ },
> > > 
> > > Ditto.
> 
> Ditto.
> 
> > > > +};
> 
> ...
> 
> > > > +       if (!dev->of_node) {
> > > > +               dev_err(dev, "DT node is null\n");
> > > > +               return ERR_PTR(-ENODEV);
> > > > +       }
> 
> I guess this check is redundant since following OF calls will fail anyway.
> 
> > > > +
> > > > +       id = of_match_node(adi_axi_adc_of_match, dev->of_node);
> > > 
> > > You may use this from struct driver and move the table after this
> > > function.
> > 
> > right; it didn't occur to me, since i was already using
> > of_device_get_match_data() in ad9467
> 
> Even better. But see above note.
> 
> > > > +       if (!id)
> > > > +               return ERR_PTR(-ENODEV);

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Kees Cook]

On Sun, Mar 22, 2020 at 12:45:39PM +0200, Andy Shevchenko wrote:
> +Cc Kees (see below about allocation size checks)
> 
> On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> wrote:
> > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > <alexandru.ardelean@analog.com> wrote:
> 
> ...
> 
> > > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device
> > > > *dev,
> > > > +                                                         int sizeof_priv)
> > > > +{
> > > > +       struct adi_axi_adc_client *cl;
> > > > +       size_t alloc_size;
> > > > +
> > > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > > +       if (sizeof_priv) {
> > > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > +               alloc_size += sizeof_priv;
> > > > +       }
> > > > +       alloc_size += IIO_ALIGN - 1;
> > >
> > > Have you looked at linux/overflow.h?
> >
> > i did now;
> > any hints where i should look closer?
> 
> It seems it lacks of this kind of allocation size checks... Perhaps add one?
> Kees, what do you think?
> 
> > > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > +       if (!cl)
> > > > +               return ERR_PTR(-ENOMEM);

My head hurts trying to read this! ;) Okay, so the base size is
sizeof(struct adi_axi_adc_client). But if sizeof_priv is non-zero
(this arg should be size_t not int), then we need to make the struct
size ALIGNed? And then what is the "+= IIO_ALIGN - 1" for?

It's not clear to me what the expect alignment/padding is here.

I would probably construct this as:

	sizeof_self = sizeof(struct adi_axi_adc_client);
	if (sizeof_priv)
		sizeof_self = ALIGN(sizeof_self, IIO_ALIGN);
	if (check_add_overflow(sizeof_self, sizeof_priv, &sizeof_alloc))
		return ERR_PTR(-ENOMEM);
	if (check_add_overflow(sizeof_alloc, IIO_ALIGN - 1, &sizeof_alloc))
		return ERR_PTR(-ENOMEM);

But I don't understand the "IIO_ALIGN - 1" part, so I assume this could
be shortened with better use of ALIGN()?

Also, this feels like a weird driver allocation overall:

+	struct adi_axi_adc_conv **ptr, *conv;
+
+	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	conv = adi_axi_adc_conv_register(dev, sizeof_priv);

devres_alloc() allocates storage for a _single pointer_. :P That's not
useful for resource tracking. Why is devres_alloc() being called here
and not down in adi_axi_adc_conv_register() and just passing the pointer
back up?

-- 
Kees Cook

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Ardelean, Alexandru]

On Sun, 2020-03-22 at 09:16 -0700, Kees Cook wrote:
> On Sun, Mar 22, 2020 at 12:45:39PM +0200, Andy Shevchenko wrote:
> > +Cc Kees (see below about allocation size checks)
> > 
> > On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> > <alexandru.Ardelean@analog.com> wrote:
> > > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> > > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > > <alexandru.ardelean@analog.com> wrote:
> > 
> > ...
> > 
> > > > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct
> > > > > device
> > > > > *dev,
> > > > > +                                                         int
> > > > > sizeof_priv)
> > > > > +{
> > > > > +       struct adi_axi_adc_client *cl;
> > > > > +       size_t alloc_size;
> > > > > +
> > > > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > > > +       if (sizeof_priv) {
> > > > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > +               alloc_size += sizeof_priv;
> > > > > +       }
> > > > > +       alloc_size += IIO_ALIGN - 1;
> > > > 
> > > > Have you looked at linux/overflow.h?
> > > 
> > > i did now;
> > > any hints where i should look closer?
> > 
> > It seems it lacks of this kind of allocation size checks... Perhaps add one?
> > Kees, what do you think?
> > 
> > > > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > > +       if (!cl)
> > > > > +               return ERR_PTR(-ENOMEM);
> 
> My head hurts trying to read this! ;) Okay, so the base size is
> sizeof(struct adi_axi_adc_client). But if sizeof_priv is non-zero
> (this arg should be size_t not int), then we need to make the struct
> size ALIGNed? And then what is the "+= IIO_ALIGN - 1" for?
> 
> It's not clear to me what the expect alignment/padding is here.
> 
> I would probably construct this as:
> 
> 	sizeof_self = sizeof(struct adi_axi_adc_client);
> 	if (sizeof_priv)
> 		sizeof_self = ALIGN(sizeof_self, IIO_ALIGN);
> 	if (check_add_overflow(sizeof_self, sizeof_priv, &sizeof_alloc))
> 		return ERR_PTR(-ENOMEM);
> 	if (check_add_overflow(sizeof_alloc, IIO_ALIGN - 1, &sizeof_alloc))
> 		return ERR_PTR(-ENOMEM);

Ok, but the question is: shouldn't this be done in kmalloc()/kzalloc?
Why do it in each driver?
I don't see this done in many drivers.

> 
> But I don't understand the "IIO_ALIGN - 1" part, so I assume this could
> be shortened with better use of ALIGN()?
> 
> Also, this feels like a weird driver allocation overall:
> 
> +	struct adi_axi_adc_conv **ptr, *conv;
> +
> +	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> 
> devres_alloc() allocates storage for a _single pointer_. :P That's not
> useful for resource tracking. Why is devres_alloc() being called here
> and not down in adi_axi_adc_conv_register() and just passing the pointer
> back up?
> 

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Ardelean, Alexandru]

On Sun, 2020-03-22 at 16:31 +0000, Ardelean, Alexandru wrote:
> [External]
> 
> On Sun, 2020-03-22 at 09:16 -0700, Kees Cook wrote:
> > On Sun, Mar 22, 2020 at 12:45:39PM +0200, Andy Shevchenko wrote:
> > > +Cc Kees (see below about allocation size checks)
> > > 
> > > On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> > > <alexandru.Ardelean@analog.com> wrote:
> > > > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> > > > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > > > <alexandru.ardelean@analog.com> wrote:
> > > 
> > > ...
> > > 
> > > > > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct
> > > > > > device
> > > > > > *dev,
> > > > > > +                                                         int
> > > > > > sizeof_priv)
> > > > > > +{
> > > > > > +       struct adi_axi_adc_client *cl;
> > > > > > +       size_t alloc_size;
> > > > > > +
> > > > > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > > > > +       if (sizeof_priv) {
> > > > > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > > +               alloc_size += sizeof_priv;
> > > > > > +       }
> > > > > > +       alloc_size += IIO_ALIGN - 1;
> > > > > 
> > > > > Have you looked at linux/overflow.h?
> > > > 
> > > > i did now;
> > > > any hints where i should look closer?
> > > 
> > > It seems it lacks of this kind of allocation size checks... Perhaps add
> > > one?
> > > Kees, what do you think?
> > > 
> > > > > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > > > +       if (!cl)
> > > > > > +               return ERR_PTR(-ENOMEM);
> > 
> > My head hurts trying to read this! ;) Okay, so the base size is
> > sizeof(struct adi_axi_adc_client). But if sizeof_priv is non-zero
> > (this arg should be size_t not int), then we need to make the struct
> > size ALIGNed? And then what is the "+= IIO_ALIGN - 1" for?
> > 
> > It's not clear to me what the expect alignment/padding is here.
> > 
> > I would probably construct this as:
> > 
> > 	sizeof_self = sizeof(struct adi_axi_adc_client);
> > 	if (sizeof_priv)
> > 		sizeof_self = ALIGN(sizeof_self, IIO_ALIGN);
> > 	if (check_add_overflow(sizeof_self, sizeof_priv, &sizeof_alloc))
> > 		return ERR_PTR(-ENOMEM);
> > 	if (check_add_overflow(sizeof_alloc, IIO_ALIGN - 1, &sizeof_alloc))
> > 		return ERR_PTR(-ENOMEM);
> 
> Ok, but the question is: shouldn't this be done in kmalloc()/kzalloc?
> Why do it in each driver?
> I don't see this done in many drivers.

Disregard this comment.
It's late here, and I'm having trouble reading the code.

But, this feels a bit weird popping up now, when trying to re-use code that
already existed in parts of IIO.
All I did was copy bits from iio_device_alloc(), and now it seems I have to
write compiler/overflow checks.

> 
> > But I don't understand the "IIO_ALIGN - 1" part, so I assume this could
> > be shortened with better use of ALIGN()?
> > 
> > Also, this feels like a weird driver allocation overall:
> > 
> > +	struct adi_axi_adc_conv **ptr, *conv;
> > +
> > +	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> > +			   GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> > 
> > devres_alloc() allocates storage for a _single pointer_. :P That's not
> > useful for resource tracking. Why is devres_alloc() being called here
> > and not down in adi_axi_adc_conv_register() and just passing the pointer
> > back up?

This was initially implemented as having 2 parts: 1 adi_axi_adc_conv_register()
and 1 devm_adi_axi_adc_conv_register() which were both exported.
It was deciced earlier to remove the adi_axi_adc_conv_register() part.

> > 

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Jonathan Cameron]

On Sun, 22 Mar 2020 09:16:36 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Sun, Mar 22, 2020 at 12:45:39PM +0200, Andy Shevchenko wrote:
> > +Cc Kees (see below about allocation size checks)
> > 
> > On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> > <alexandru.Ardelean@analog.com> wrote:  
> > > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:  
> > > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > > <alexandru.ardelean@analog.com> wrote:  
> > 
> > ...
> >   
> > > > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device
> > > > > *dev,
> > > > > +                                                         int sizeof_priv)
> > > > > +{
> > > > > +       struct adi_axi_adc_client *cl;
> > > > > +       size_t alloc_size;
> > > > > +
> > > > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > > > +       if (sizeof_priv) {
> > > > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > +               alloc_size += sizeof_priv;
> > > > > +       }
> > > > > +       alloc_size += IIO_ALIGN - 1;  
> > > >
> > > > Have you looked at linux/overflow.h?  
> > >
> > > i did now;
> > > any hints where i should look closer?  
> > 
> > It seems it lacks of this kind of allocation size checks... Perhaps add one?
> > Kees, what do you think?
> >   
> > > > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > > +       if (!cl)
> > > > > +               return ERR_PTR(-ENOMEM);  
> 
> My head hurts trying to read this! ;) Okay, so the base size is
> sizeof(struct adi_axi_adc_client). But if sizeof_priv is non-zero
> (this arg should be size_t not int), then we need to make the struct
> size ALIGNed? And then what is the "+= IIO_ALIGN - 1" for?

I'm a bit embarrassed.  I can't remember what the += IIO_ALIGN - 1
was for in the first place and I can't work it out now.

The purpose of the fun here was to end up with a structure that
was either
a) sizeof(struct iio_dev) long,
b) sizeof(struct iio_dev) + padding + sizeof_priv 
where the padding ensured that any __cacheline_aligned elements
in the private structure were cacheline aligned within resulting
allocation.

So why the extra IIO_ALIGN - 1....

The original patch doesn't help much either given it's got a question
in there for why this bit is needed.

https://lore.kernel.org/linux-iio/1302890160-8823-5-git-send-email-jic23@cam.ac.uk/

However, it rang a slight bell.  Seems I lifted the code from netdev.
https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L9718

I'm fairly sure we don't need that padding here..  What can I say,
I was young and stupid :)

I did add a question mark so clearly meant to come back and
take another look ;)

One vague thought is that it's about ensuring we are big enough to
ensure we are cacheline aligned.  That's obviously not a problem with
current struct iio_dev which is far from small,
but in theory it could have been.  Also, thinking about it we only
need the struct iio_dev to be cacheline aligned if we have
an iio_priv structure.  If we have one of those it will definitely
be big enough anyway.

At somepoint I'd like to look at cleaning it up for iio_device_alloc
but with a lot of testing as who knows what is relying on this behaviour
or if I've missed something.  Crashes around this alignment are
infrequent and nasty to trace at the best of times.

Jonathan

> 
> It's not clear to me what the expect alignment/padding is here.
> 
> I would probably construct this as:
> 
> 	sizeof_self = sizeof(struct adi_axi_adc_client);
> 	if (sizeof_priv)
> 		sizeof_self = ALIGN(sizeof_self, IIO_ALIGN);
> 	if (check_add_overflow(sizeof_self, sizeof_priv, &sizeof_alloc))
> 		return ERR_PTR(-ENOMEM);
> 	if (check_add_overflow(sizeof_alloc, IIO_ALIGN - 1, &sizeof_alloc))
> 		return ERR_PTR(-ENOMEM);
> 
> But I don't understand the "IIO_ALIGN - 1" part, so I assume this could
> be shortened with better use of ALIGN()?
> 
> Also, this feels like a weird driver allocation overall:
> 
> +	struct adi_axi_adc_conv **ptr, *conv;
> +
> +	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> 
> devres_alloc() allocates storage for a _single pointer_. :P That's not
> useful for resource tracking. Why is devres_alloc() being called here
> and not down in adi_axi_adc_conv_register() and just passing the pointer
> back up?
> 

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Ardelean, Alexandru]

On Sun, 2020-03-22 at 16:53 +0000, Jonathan Cameron wrote:
> On Sun, 22 Mar 2020 09:16:36 -0700
> Kees Cook <keescook@chromium.org> wrote:
> 
> > On Sun, Mar 22, 2020 at 12:45:39PM +0200, Andy Shevchenko wrote:
> > > +Cc Kees (see below about allocation size checks)
> > > 
> > > On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> > > <alexandru.Ardelean@analog.com> wrote:  
> > > > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:  
> > > > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > > > <alexandru.ardelean@analog.com> wrote:  
> > > 
> > > ...
> > >   
> > > > > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct
> > > > > > device
> > > > > > *dev,
> > > > > > +                                                         int
> > > > > > sizeof_priv)
> > > > > > +{
> > > > > > +       struct adi_axi_adc_client *cl;
> > > > > > +       size_t alloc_size;
> > > > > > +
> > > > > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > > > > +       if (sizeof_priv) {
> > > > > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > > +               alloc_size += sizeof_priv;
> > > > > > +       }
> > > > > > +       alloc_size += IIO_ALIGN - 1;  
> > > > > 
> > > > > Have you looked at linux/overflow.h?  
> > > > 
> > > > i did now;
> > > > any hints where i should look closer?  
> > > 
> > > It seems it lacks of this kind of allocation size checks... Perhaps add
> > > one?
> > > Kees, what do you think?
> > >   
> > > > > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > > > +       if (!cl)
> > > > > > +               return ERR_PTR(-ENOMEM);  
> > 
> > My head hurts trying to read this! ;) Okay, so the base size is
> > sizeof(struct adi_axi_adc_client). But if sizeof_priv is non-zero
> > (this arg should be size_t not int), then we need to make the struct
> > size ALIGNed? And then what is the "+= IIO_ALIGN - 1" for?
> 
> I'm a bit embarrassed.  I can't remember what the += IIO_ALIGN - 1
> was for in the first place and I can't work it out now.
> 
> The purpose of the fun here was to end up with a structure that
> was either
> a) sizeof(struct iio_dev) long,
> b) sizeof(struct iio_dev) + padding + sizeof_priv 
> where the padding ensured that any __cacheline_aligned elements
> in the private structure were cacheline aligned within resulting
> allocation.
> 
> So why the extra IIO_ALIGN - 1....
> 
> The original patch doesn't help much either given it's got a question
> in there for why this bit is needed.
> 
> https://lore.kernel.org/linux-iio/1302890160-8823-5-git-send-email-jic23@cam.ac.uk/
> 
> However, it rang a slight bell.  Seems I lifted the code from netdev.
> https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L9718
> 
> I'm fairly sure we don't need that padding here..  What can I say,
> I was young and stupid :)
> 
> I did add a question mark so clearly meant to come back and
> take another look ;)
> 
> One vague thought is that it's about ensuring we are big enough to
> ensure we are cacheline aligned.  That's obviously not a problem with
> current struct iio_dev which is far from small,
> but in theory it could have been.  Also, thinking about it we only
> need the struct iio_dev to be cacheline aligned if we have
> an iio_priv structure.  If we have one of those it will definitely
> be big enough anyway.
> 
> At somepoint I'd like to look at cleaning it up for iio_device_alloc
> but with a lot of testing as who knows what is relying on this behaviour
> or if I've missed something.  Crashes around this alignment are
> infrequent and nasty to trace at the best of times.

In the meantime, are there any objections if I leave the allocation as-is for
this driver as well?
I've tested the driver a bit more with this form.

> 
> Jonathan
> 
> > It's not clear to me what the expect alignment/padding is here.
> > 
> > I would probably construct this as:
> > 
> > 	sizeof_self = sizeof(struct adi_axi_adc_client);
> > 	if (sizeof_priv)
> > 		sizeof_self = ALIGN(sizeof_self, IIO_ALIGN);
> > 	if (check_add_overflow(sizeof_self, sizeof_priv, &sizeof_alloc))
> > 		return ERR_PTR(-ENOMEM);
> > 	if (check_add_overflow(sizeof_alloc, IIO_ALIGN - 1, &sizeof_alloc))
> > 		return ERR_PTR(-ENOMEM);
> > 
> > But I don't understand the "IIO_ALIGN - 1" part, so I assume this could
> > be shortened with better use of ALIGN()?
> > 
> > Also, this feels like a weird driver allocation overall:
> > 
> > +	struct adi_axi_adc_conv **ptr, *conv;
> > +
> > +	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> > +			   GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> > 
> > devres_alloc() allocates storage for a _single pointer_. :P That's not
> > useful for resource tracking. Why is devres_alloc() being called here
> > and not down in adi_axi_adc_conv_register() and just passing the pointer
> > back up?
> > 

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Jonathan Cameron]

On Sun, 22 Mar 2020 17:40:30 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2020-03-22 at 16:53 +0000, Jonathan Cameron wrote:
> > On Sun, 22 Mar 2020 09:16:36 -0700
> > Kees Cook <keescook@chromium.org> wrote:
> >   
> > > On Sun, Mar 22, 2020 at 12:45:39PM +0200, Andy Shevchenko wrote:  
> > > > +Cc Kees (see below about allocation size checks)
> > > > 
> > > > On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> > > > <alexandru.Ardelean@analog.com> wrote:    
> > > > > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:    
> > > > > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > > > > <alexandru.ardelean@analog.com> wrote:    
> > > > 
> > > > ...
> > > >     
> > > > > > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct
> > > > > > > device
> > > > > > > *dev,
> > > > > > > +                                                         int
> > > > > > > sizeof_priv)
> > > > > > > +{
> > > > > > > +       struct adi_axi_adc_client *cl;
> > > > > > > +       size_t alloc_size;
> > > > > > > +
> > > > > > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > > > > > +       if (sizeof_priv) {
> > > > > > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > > > +               alloc_size += sizeof_priv;
> > > > > > > +       }
> > > > > > > +       alloc_size += IIO_ALIGN - 1;    
> > > > > > 
> > > > > > Have you looked at linux/overflow.h?    
> > > > > 
> > > > > i did now;
> > > > > any hints where i should look closer?    
> > > > 
> > > > It seems it lacks of this kind of allocation size checks... Perhaps add
> > > > one?
> > > > Kees, what do you think?
> > > >     
> > > > > > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > > > > +       if (!cl)
> > > > > > > +               return ERR_PTR(-ENOMEM);    
> > > 
> > > My head hurts trying to read this! ;) Okay, so the base size is
> > > sizeof(struct adi_axi_adc_client). But if sizeof_priv is non-zero
> > > (this arg should be size_t not int), then we need to make the struct
> > > size ALIGNed? And then what is the "+= IIO_ALIGN - 1" for?  
> > 
> > I'm a bit embarrassed.  I can't remember what the += IIO_ALIGN - 1
> > was for in the first place and I can't work it out now.
> > 
> > The purpose of the fun here was to end up with a structure that
> > was either
> > a) sizeof(struct iio_dev) long,
> > b) sizeof(struct iio_dev) + padding + sizeof_priv 
> > where the padding ensured that any __cacheline_aligned elements
> > in the private structure were cacheline aligned within resulting
> > allocation.
> > 
> > So why the extra IIO_ALIGN - 1....
> > 
> > The original patch doesn't help much either given it's got a question
> > in there for why this bit is needed.
> > 
> > https://lore.kernel.org/linux-iio/1302890160-8823-5-git-send-email-jic23@cam.ac.uk/
> > 
> > However, it rang a slight bell.  Seems I lifted the code from netdev.
> > https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L9718
> > 
> > I'm fairly sure we don't need that padding here..  What can I say,
> > I was young and stupid :)
> > 
> > I did add a question mark so clearly meant to come back and
> > take another look ;)
> > 
> > One vague thought is that it's about ensuring we are big enough to
> > ensure we are cacheline aligned.  That's obviously not a problem with
> > current struct iio_dev which is far from small,
> > but in theory it could have been.  Also, thinking about it we only
> > need the struct iio_dev to be cacheline aligned if we have
> > an iio_priv structure.  If we have one of those it will definitely
> > be big enough anyway.
> > 
> > At somepoint I'd like to look at cleaning it up for iio_device_alloc
> > but with a lot of testing as who knows what is relying on this behaviour
> > or if I've missed something.  Crashes around this alignment are
> > infrequent and nasty to trace at the best of times.  
> 
> In the meantime, are there any objections if I leave the allocation as-is for
> this driver as well?
> I've tested the driver a bit more with this form.

Hmm. I'd rather we didn't introduce this with the extra padding unless we
can figure out why it would need it.  It would be a bit horrible to
patch this in a few weeks time for this reason.

If you absolutely can't retest for remote reasons then I suppose we could
merge it and tidy up later.

Jonathan

> 
> > 
> > Jonathan
> >   
> > > It's not clear to me what the expect alignment/padding is here.
> > > 
> > > I would probably construct this as:
> > > 
> > > 	sizeof_self = sizeof(struct adi_axi_adc_client);
> > > 	if (sizeof_priv)
> > > 		sizeof_self = ALIGN(sizeof_self, IIO_ALIGN);
> > > 	if (check_add_overflow(sizeof_self, sizeof_priv, &sizeof_alloc))
> > > 		return ERR_PTR(-ENOMEM);
> > > 	if (check_add_overflow(sizeof_alloc, IIO_ALIGN - 1, &sizeof_alloc))
> > > 		return ERR_PTR(-ENOMEM);
> > > 
> > > But I don't understand the "IIO_ALIGN - 1" part, so I assume this could
> > > be shortened with better use of ALIGN()?
> > > 
> > > Also, this feels like a weird driver allocation overall:
> > > 
> > > +	struct adi_axi_adc_conv **ptr, *conv;
> > > +
> > > +	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> > > +			   GFP_KERNEL);
> > > +	if (!ptr)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> > > 
> > > devres_alloc() allocates storage for a _single pointer_. :P That's not
> > > useful for resource tracking. Why is devres_alloc() being called here
> > > and not down in adi_axi_adc_conv_register() and just passing the pointer
> > > back up?
> > >   

Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP core

[Ardelean, Alexandru]

On Sun, 2020-03-22 at 18:26 +0000, Jonathan Cameron wrote:
> On Sun, 22 Mar 2020 17:40:30 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Sun, 2020-03-22 at 16:53 +0000, Jonathan Cameron wrote:
> > > On Sun, 22 Mar 2020 09:16:36 -0700
> > > Kees Cook <keescook@chromium.org> wrote:
> > >   
> > > > On Sun, Mar 22, 2020 at 12:45:39PM +0200, Andy Shevchenko wrote:  
> > > > > +Cc Kees (see below about allocation size checks)
> > > > > 
> > > > > On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> > > > > <alexandru.Ardelean@analog.com> wrote:    
> > > > > > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:    
> > > > > > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > > > > > <alexandru.ardelean@analog.com> wrote:    
> > > > > 
> > > > > ...
> > > > >     
> > > > > > > > +static struct adi_axi_adc_conv
> > > > > > > > *adi_axi_adc_conv_register(struct
> > > > > > > > device
> > > > > > > > *dev,
> > > > > > > > +                                                         int
> > > > > > > > sizeof_priv)
> > > > > > > > +{
> > > > > > > > +       struct adi_axi_adc_client *cl;
> > > > > > > > +       size_t alloc_size;
> > > > > > > > +
> > > > > > > > +       alloc_size = sizeof(struct adi_axi_adc_client);
> > > > > > > > +       if (sizeof_priv) {
> > > > > > > > +               alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > > > > > +               alloc_size += sizeof_priv;
> > > > > > > > +       }
> > > > > > > > +       alloc_size += IIO_ALIGN - 1;    
> > > > > > > 
> > > > > > > Have you looked at linux/overflow.h?    
> > > > > > 
> > > > > > i did now;
> > > > > > any hints where i should look closer?    
> > > > > 
> > > > > It seems it lacks of this kind of allocation size checks... Perhaps
> > > > > add
> > > > > one?
> > > > > Kees, what do you think?
> > > > >     
> > > > > > > > +       cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > > > > > +       if (!cl)
> > > > > > > > +               return ERR_PTR(-ENOMEM);    
> > > > 
> > > > My head hurts trying to read this! ;) Okay, so the base size is
> > > > sizeof(struct adi_axi_adc_client). But if sizeof_priv is non-zero
> > > > (this arg should be size_t not int), then we need to make the struct
> > > > size ALIGNed? And then what is the "+= IIO_ALIGN - 1" for?  
> > > 
> > > I'm a bit embarrassed.  I can't remember what the += IIO_ALIGN - 1
> > > was for in the first place and I can't work it out now.
> > > 
> > > The purpose of the fun here was to end up with a structure that
> > > was either
> > > a) sizeof(struct iio_dev) long,
> > > b) sizeof(struct iio_dev) + padding + sizeof_priv 
> > > where the padding ensured that any __cacheline_aligned elements
> > > in the private structure were cacheline aligned within resulting
> > > allocation.
> > > 
> > > So why the extra IIO_ALIGN - 1....
> > > 
> > > The original patch doesn't help much either given it's got a question
> > > in there for why this bit is needed.
> > > 
> > > https://lore.kernel.org/linux-iio/1302890160-8823-5-git-send-email-jic23@cam.ac.uk/
> > > 
> > > However, it rang a slight bell.  Seems I lifted the code from netdev.
> > > https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L9718
> > > 
> > > I'm fairly sure we don't need that padding here..  What can I say,
> > > I was young and stupid :)
> > > 
> > > I did add a question mark so clearly meant to come back and
> > > take another look ;)
> > > 
> > > One vague thought is that it's about ensuring we are big enough to
> > > ensure we are cacheline aligned.  That's obviously not a problem with
> > > current struct iio_dev which is far from small,
> > > but in theory it could have been.  Also, thinking about it we only
> > > need the struct iio_dev to be cacheline aligned if we have
> > > an iio_priv structure.  If we have one of those it will definitely
> > > be big enough anyway.
> > > 
> > > At somepoint I'd like to look at cleaning it up for iio_device_alloc
> > > but with a lot of testing as who knows what is relying on this behaviour
> > > or if I've missed something.  Crashes around this alignment are
> > > infrequent and nasty to trace at the best of times.  
> > 
> > In the meantime, are there any objections if I leave the allocation as-is
> > for
> > this driver as well?
> > I've tested the driver a bit more with this form.
> 
> Hmm. I'd rather we didn't introduce this with the extra padding unless we
> can figure out why it would need it.  It would be a bit horrible to
> patch this in a few weeks time for this reason.
> 
> If you absolutely can't retest for remote reasons then I suppose we could
> merge it and tidy up later.

I'll do the changes and re-test.

> 
> Jonathan
> 
> > > Jonathan
> > >   
> > > > It's not clear to me what the expect alignment/padding is here.
> > > > 
> > > > I would probably construct this as:
> > > > 
> > > > 	sizeof_self = sizeof(struct adi_axi_adc_client);
> > > > 	if (sizeof_priv)
> > > > 		sizeof_self = ALIGN(sizeof_self, IIO_ALIGN);
> > > > 	if (check_add_overflow(sizeof_self, sizeof_priv, &sizeof_alloc))
> > > > 		return ERR_PTR(-ENOMEM);
> > > > 	if (check_add_overflow(sizeof_alloc, IIO_ALIGN - 1,
> > > > &sizeof_alloc))
> > > > 		return ERR_PTR(-ENOMEM);
> > > > 
> > > > But I don't understand the "IIO_ALIGN - 1" part, so I assume this could
> > > > be shortened with better use of ALIGN()?
> > > > 
> > > > Also, this feels like a weird driver allocation overall:
> > > > 
> > > > +	struct adi_axi_adc_conv **ptr, *conv;
> > > > +
> > > > +	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
> > > > +			   GFP_KERNEL);
> > > > +	if (!ptr)
> > > > +		return ERR_PTR(-ENOMEM);
> > > > +
> > > > +	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
> > > > 
> > > > devres_alloc() allocates storage for a _single pointer_. :P That's not
> > > > useful for resource tracking. Why is devres_alloc() being called here
> > > > and not down in adi_axi_adc_conv_register() and just passing the pointer
> > > > back up?
> > > >