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

From: Ardelean, Alexandru
Date: Mon Mar 09 2020 - 07:35:16 EST

On Sat, 2020-03-07 at 14:54 +0000, Jonathan Cameron wrote:
> [External]
>
> On Fri, 6 Mar 2020 13:00:57 +0200
> Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
>
> > From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> Mostly looks fine, but a few nitpicks from rereading it.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/adc/Kconfig | 20 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/adi-axi-adc.c | 618 ++++++++++++++++++++++++++++
> > include/linux/iio/adc/adi-axi-adc.h | 63 +++
> > 4 files changed, 702 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..17ee20015d71
> > --- /dev/null
> > +++ b/drivers/iio/adc/adi-axi-adc.c
> > @@ -0,0 +1,618 @@
> > +// 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
> > + */
> > +
> > +#define LOWER5_MSK GENMASK(4, 0)
> > +#define LOWER5_SET(x) FIELD_PREP(LOWER5_MSK, x)
> > +#define LOWER5_GET(x) FIELD_GET(LOWER5_MSK, x)
>
> Whilst it may cause more repetition, I'd rather just see these used where
> relevant inline. That will be slightly easier to review.

ack
i wasn't too decided on whether these need to be defined yet

>
> > +
> > +#define LOWER8_MSK GENMASK(7, 0)
> > +#define LOWER8_SET(x) FIELD_PREP(LOWER8_MSK, x)
> > +#define LOWER8_GET(x) FIELD_GET(LOWER8_MSK, x)
> > +
> > +#define UPPER16_MSK GENMASK(31, 16)
> > +#define UPPER16_SET(x) FIELD_PREP(UPPER16_MSK, x)
> > +#define UPPER16_GET(x) FIELD_GET(UPPER16_MSK, x)
> > +
> > +#define LOWER16_MSK GENMASK(15, 0)
> > +#define LOWER16_SET(x) FIELD_PREP(LOWER16_MSK, x)
> > +#define LOWER16_GET(x) FIELD_GET(LOWER16_MSK, x)
> > +
> > +/* ADC controls */
> > +
> > +#define REG_RSTN 0x0040
>
> Usual stuff: These should be prefixed with driver relevant prefix
> maybe ADI_AXI_

my only concern [about the prefix] is that it makes the macro-names too long;
after re-looking at these reg definitions, what bothered me is that some of the
bit-field-names collide; so i went with concatenanting reg-names + reg-bitnames,
which made them look too long [so i removed the prefix]

there's also the possibility of going back to the regmap-doc and shortening
these reg-name/bitnames;
but they've been like this for a while, and i admit going to the HDL team makes
me sometimes lazy;

i'll re-add teh ADI_AXI prefix; and will see how these look

>
> > +#define REG_RSTN_CE_N BIT(2)
> > +#define REG_RSTN_MMCM_RSTN BIT(1)
> > +#define REG_RSTN_RSTN BIT(0)
> > +
> > +#define REG_CNTRL 0x0044
> > +#define REG_CNTRL_SYNC BIT(3)
> > +#define REG_CNTRL_R1_MODE BIT(2)
> > +#define REG_CNTRL_DDR_EDGESEL BIT(1)
> > +#define REG_CNTRL_PIN_MODE BIT(0)
> > +
> > +#define REG_CLK_FREQ 0x0054
> > +#define REG_CLK_RATIO 0x0058
> > +
> > +#define REG_STATUS 0x005C
> > +#define REG_STATUS_PN_ERR BIT(3)
> > +#define REG_STATUS_PN_OOS BIT(2)
> > +#define REG_STATUS_OVER_RANGE BIT(1)
> > +#define REG_STATUS_STATUS BIT(0)
> > +
> > +#define REG_SYNC_STATUS 0x0068
> > +#define REG_SYNC_STATUS_SYNC BIT(0)
> > +
> > +#define REG_DRP_CNTRL 0x0070
> > +#define REG_DRP_CNTRL_DRP_RWN BIT(28)
> > +#define REG_DRP_CNTRL_DRP_ADDRESS_MSK GENMASK(27, 16)
> > +#define REG_DRP_CNTRL_DRP_ADDRESS_SET(x) \
> > + FIELD_PREP(REG_DRP_CNTRL_DRP_ADDRESS_MSK, x)
> > +#define REG_DRP_CNTRL_DRP_ADDRESS_GET(x) \
> > + FIELD_GET(REG_DRP_CNTRL_DRP_ADDRESS_MSK, x)
> > +
> > +#define REG_DRP_STATUS 0x0074
> > +#define REG_DRP_STATUS_DRP_LOCKED BIT(17)
> > +#define REG_DRP_STATUS_DRP_STATUS BIT(16)
> > +
> > +#define REG_DRP_WDATA 0x0078
> > +#define REG_DRP_WDATA_SET LOWER16_SET
> > +#define REG_DRP_WDATA_GET LOWER16_GET
>
> As mentioned above, I'd rather see the mask here. Unless used
> an awful lot, I'd rather just see the FIELD_PREP and FIELD_GET
> used inline rather than defining a bunch of _SET/_GET only some
> of which seem to be used so far.
>

sure;
will remove all these _SET/_GET shorthands

a lot of them will be used;
and we may implement all reg-definitions at some point;

this driver is a re-write/adaptation of this one:
https://github.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/cf_axi_adc_core.c

which has grown quite a lot and trying to clean it up is... Â\_(ã)_/Â


> > +
> > +#define REG_DRP_RDATA 0x007C
> > +#define REG_DRP_RDATA_SET LOWER16_SET
> > +#define REG_DRP_RDATA_GET LOWER16_GET
> > +
> > +#define REG_UI_STATUS 0x0088
> > +#define REG_UI_STATUS_OVF BIT(2)
> > +#define REG_UI_STATUS_UNF BIT(1)
> > +
> > +#define REG_USR_CNTRL_1 0x00A0
> > +#define REG_USR_CNTRL_1_USR_CHANMAX_MSK LOWER8_MSK
> > +#define REG_USR_CNTRL_1_USR_CHANMAX_SET LOWER8_SET
> > +#define REG_USR_CNTRL_1_USR_CHANMAX_GET LOWER8_GET
> > +
> > +#define REG_ADC_START_CODE 0x00A4
> > +#define REG_ADC_GPIO_IN 0x00B8
> > +#define REG_ADC_GPIO_OUT 0x00BC
> > +
> > +#define REG_PPS_COUNTER 0x00C0
> > +
> > +#define REG_PPS_STATUS 0x00C4
> > +#define REG_PPS_STATUS_PPS_STATUS BIT(0)
> > +
> > +/* ADC Channel controls */
> > +
> > +#define REG_CHAN_CNTRL(c) (0x0400 + (c) * 0x40)
> > +#define REG_CHAN_CNTRL_LB_OWR BIT(11)
> > +#define REG_CHAN_CNTRL_PN_SEL_OWR BIT(10)
> > +#define REG_CHAN_CNTRL_IQCOR_ENB BIT(9)
> > +#define REG_CHAN_CNTRL_DCFILT_ENB BIT(8)
> > +#define REG_CHAN_CNTRL_FORMAT_SIGNEXT BIT(6)
> > +#define REG_CHAN_CNTRL_FORMAT_TYPE BIT(5)
> > +#define REG_CHAN_CNTRL_FORMAT_ENABLE BIT(4)
> > +#define REG_CHAN_CNTRL_ADC_PN_TYPE_OWR BIT(1)
> > +#define REG_CHAN_CNTRL_ENABLE BIT(0)
> > +
> > +#define REG_CHAN_CNTRL_DEFAULTS \
> > + (REG_CHAN_CNTRL_FORMAT_SIGNEXT | REG_CHAN_CNTRL_FORMAT_ENABLE | \
> > + REG_CHAN_CNTRL_IQCOR_ENB | REG_CHAN_CNTRL_ENABLE)
> > +
> > +#define REG_CHAN_STATUS(c) (0x0404 + (c) * 0x40)
> > +#define REG_CHAN_STATUS_PN_ERR BIT(2)
> > +#define REG_CHAN_STATUS_PN_OOS BIT(1)
> > +#define REG_CHAN_STATUS_OVER_RANGE BIT(0)
> > +
> > +#define REG_CHAN_CNTRL_1(c) (0x0410 + (c) * 0x40)
> > +#define REG_CHAN_CNTRL_1_DCFILT_OFFSET_MSK UPPER16_MSK
> > +#define REG_CHAN_CNTRL_1_DCFILT_OFFSET_SET UPPER16_SET
> > +#define REG_CHAN_CNTRL_1_DCFILT_OFFSET_GET UPPER16_GET
> > +#define REG_CHAN_CNTRL_1_DCFILT_COEFF_MSK LOWER16_MSK
> > +#define REG_CHAN_CNTRL_1_DCFILT_COEFF_SET LOWER16_SET
> > +#define REG_CHAN_CNTRL_1_DCFILT_COEFF_GET LOWER16_GET
> > +
> > +#define REG_CHAN_CNTRL_2(c) (0x0414 + (c) * 0x40)
> > +#define REG_CHAN_CNTRL_2_IQCOR_COEFF_1_MSK UPPER16_MSK
> > +#define REG_CHAN_CNTRL_2_IQCOR_COEFF_1_SET UPPER16_SET
> > +#define REG_CHAN_CNTRL_2_IQCOR_COEFF_1_GET UPPER16_GET
> > +#define REG_CHAN_CNTRL_2_IQCOR_COEFF_2_MSK LOWER16_MSK
> > +#define REG_CHAN_CNTRL_2_IQCOR_COEFF_2_SET LOWER16_SET
> > +#define REG_CHAN_CNTRL_2_IQCOR_COEFF_2_GET LOWER16_GET
> > +
> > +/* format is 1.1.14 (sign, integer and fractional bits) */
> > +#define IQCOR_INT_1 0x4000UL
> > +#define IQCOR_SIGN_BIT BIT(15)
> > +
> > +#define REG_CHAN_CNTRL_3(c) (0x0418 + (c) * 0x40)
> > +#define REG_CHAN_CNTRL_3_ADC_PN_SEL_MSK UPPER16_MSK
> > +#define REG_CHAN_CNTRL_3_ADC_PN_SEL_SET UPPER16_SET
> > +#define REG_CHAN_CNTRL_3_ADC_PN_SEL_GET UPPER16_GET
> > +#define REG_CHAN_CNTRL_3_ADC_DATA_SEL_MSK LOWER16_MSK
> > +#define REG_CHAN_CNTRL_3_ADC_DATA_SEL_SET LOWER16_SET
> > +#define REG_CHAN_CNTRL_3_ADC_DATA_SEL_GET LOWER16_GET
> > +
> > +#define REG_CHAN_USR_CNTRL_1(c) (0x0420
> > + (c) * 0x40)
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_BE BIT(25)
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_SIGNED BIT(24)
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_SHIFT_MSK GENMASK(
> > 23, 16)
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_SHIFT_SET(x) \
> > + FIELD_PREP(REG_CHAN_USR_CNTRL_1_USR_DATATYPE_SHIFT_MSK, x)
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_SHIFT_GET \
> > + FIELD_GET(REG_CHAN_USR_CNTRL_1_USR_DATATYPE_SHIFT_MSK, x)
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_TOTAL_BITS_MSK GENMASK(15, 8)
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_TOTAL_BITS_SET(x) \
> > + FIELD_PREP(REG_CHAN_USR_CNTRL_1_USR_DATATYPE_TOTAL_BITS_MSK, x)
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_TOTAL_BITS_GET(x) \
> > + FIELD_GET(REG_CHAN_USR_CNTRL_1_USR_DATATYPE_TOTAL_BITS_MSK, x)
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_BITS_MSK LOWER8_M
> > SK
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_BITS_SET LOWER8_S
> > ET
> > +#define REG_CHAN_USR_CNTRL_1_USR_DATATYPE_BITS_GET LOWER8_G
> > ET
> > +
> > +#define REG_CHAN_USR_CNTRL_2(c) (0x0424 + (c) *
> > 0x40)
> > +#define REG_CHAN_USR_CNTRL_2_USR_DECIMATION_M_MSK UPPER16_MSK
> > +#define REG_CHAN_USR_CNTRL_2_USR_DECIMATION_M_SET UPPER16_SET
> > +#define REG_CHAN_USR_CNTRL_2_USR_DECIMATION_M_GET UPPER16_GET
> > +#define REG_CHAN_USR_CNTRL_2_USR_DECIMATION_N_MSK LOWER16_MSK
> > +#define REG_CHAN_USR_CNTRL_2_USR_DECIMATION_N_SET LOWER16_SET
> > +#define REG_CHAN_USR_CNTRL_2_USR_DECIMATION_N_GET LOWER16_GET
> > +
> > +/* IO Delay controls */
> > +#define __REG_DELAY_CONTROL(base, lane) ((base) + ((lane) *
> > 0x04))
> > +
> > +#define REG_DELAY_CONTROL(lane) \
> > + __REG_DELAY_CONTROL(0x0800, lane)
> > +#define REG_DELAY_CONTROL_MSK LOWER5_MSK
> > +#define REG_DELAY_CONTROL_SET LOWER5_SET
> > +#define REG_DELAY_CONTROL_GET LOWER5_GET
> > +
> > +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, REG_CHAN_CNTRL(i));
> > +
> > + if (test_bit(i, scan_mask))
> > + ctrl |= REG_CHAN_CNTRL_ENABLE;
> > + else
> > + ctrl &= ~REG_CHAN_CNTRL_ENABLE;
> > +
> > + adi_axi_adc_write(st, REG_CHAN_CNTRL(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);
> > +
> > + put_device(cl->dev);
> > + list_del(&cl->entry);
> This isn't reverse of register. Why?
> Should be list_del then put_device.
>
> > + kfree(cl);
>
> To reverse the register precisely the kfree(cl) should be outside
> the lock...

yeah; i was a bit careless/relaxed when writing this bit of code;
will update;

>
> > +
> > + mutex_unlock(&registered_clients_lock);
> > +}
> > +
> > +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;
> > +
> > + if (!conv->chip_info->num_scales) {
> > + buf[0] = '\n';
> > + return 1;
>
> I'd prefer to see this done using the core available handling.
> That way drivers that don't provide this would just not register
> it in the first place. It will be a characteristic of the channels.

ack
will take a look closer here;
i am a bit vague [at this point] on how to do it; but i'll figure it out

>
> > + }
> > +
> > + 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);
> > +
> > +static struct attribute *adi_axi_adc_attributes[] = {
> > + &iio_dev_attr_in_voltage_scale_available.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group adi_axi_adc_attribute_group = {
> > + .attrs = adi_axi_adc_attributes,
> > +};
> > +
> > +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;
> > + unsigned int val;
> > + 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++) {
> > + if (i & 1)
> > + val = REG_CHAN_CNTRL_2_IQCOR_COEFF_2_SET(IQCOR_INT_1);
> > + else
> > + val = REG_CHAN_CNTRL_2_IQCOR_COEFF_1_SET(IQCOR_INT_1);
> > + adi_axi_adc_write(st, REG_CHAN_CNTRL_2(i), val);
> > +
> > + adi_axi_adc_write(st, REG_CHAN_CNTRL(i),
> > + REG_CHAN_CNTRL_DEFAULTS);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void axi_adc_reset(struct adi_axi_adc_state *st)
> > +{
> > + adi_axi_adc_write(st, REG_RSTN, 0);
> > + mdelay(10);
> > + adi_axi_adc_write(st, REG_RSTN, REG_RSTN_MMCM_RSTN);
> > + mdelay(10);
> > + adi_axi_adc_write(st, REG_RSTN,
> > + REG_RSTN_RSTN | 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 = dev_name(&pdev->dev);
>
> What does this result in? Given this is supposed to be a part number
> I kind of expected this to be hardcoded to "adi-axi-adc" or similar.

ack;
"adi-axi-adc" it is


>
> > + 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@xxxxxxxxxx>");
> > +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..57974944b30b
> > --- /dev/null
> > +++ b/include/linux/iio/adc/adi-axi-adc.h
> > @@ -0,0 +1,63 @@
> > +/* 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
>
> Run the kernel doc script over this and it would have moaned...
>
> No reg_access.

ack;
i'll need to start adapting that script to some CI
it's on my todo-list

>
> > + * @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