Message-ID: <bc0287ebb95ac913e17f5aee4ace20b6498319a6.camel@analog.com>
Date:   Tue, 25 Feb 2020 13:51:59 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "jic23@...nel.org" <jic23@...nel.org>
CC:     "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>
Subject: Re: [PATCH 2/5] iio: adc: axi-adc: add support for AXI ADC IP core

On Fri, 2020-02-21 at 12:44 +0000, Jonathan Cameron wrote:
> On Thu, 20 Feb 2020 17:03:14 +0200
> Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
> 
> > From: Michael Hennerich <michael.hennerich@...log.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@...log.com>
> > Signed-off-by: Lars-Peter Clausen <lars@...afoo.de>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> 
> In general looks good to me.  A few specific comments inline.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/Kconfig         |  20 +
> >  drivers/iio/adc/Makefile        |   1 +
> >  drivers/iio/adc/axi-adc.c       | 622 ++++++++++++++++++++++++++++++++
> >  include/linux/iio/adc/axi-adc.h |  79 ++++
> >  4 files changed, 722 insertions(+)
> >  create mode 100644 drivers/iio/adc/axi-adc.c
> >  create mode 100644 include/linux/iio/adc/axi-adc.h
> > 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index f4da821c4022..6cd48a256122 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -282,6 +282,26 @@ config AT91_SAMA5D2_ADC
> >  	  To compile this driver as a module, choose M here: the module will be
> >  	  called at91-sama5d2_adc.
> >  
> > +config 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 axi-adc.
> > +
> >  config AXP20X_ADC
> >  	tristate "X-Powers AXP20X and AXP22X ADC driver"
> >  	depends on MFD_AXP20X
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 8462455b4228..e14fabd53246 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_AD799X) += ad799x.o
> >  obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
> >  obj-$(CONFIG_AT91_ADC) += at91_adc.o
> >  obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
> > +obj-$(CONFIG_AXI_ADC) += axi-adc.o
> >  obj-$(CONFIG_AXP20X_ADC) += axp20x_adc.o
> >  obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> >  obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o
> > diff --git a/drivers/iio/adc/axi-adc.c b/drivers/iio/adc/axi-adc.c
> > new file mode 100644
> > index 000000000000..9ddd64fdab2d
> > --- /dev/null
> > +++ b/drivers/iio/adc/axi-adc.c
> 
> I suspect this may not be the only AXI based ADC interface in the
> world.   As such, prefix with adi-axi perhaps.

ack

> 
> > @@ -0,0 +1,622 @@
> > +// 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/axi-adc.h>
> > +
> > +/**
> > + * Register definitions:
> > + *   https://wiki.analog.com/resources/fpga/docs/axi_adc_ip#register_map
> > + */
> > +
> > +#define AXI_ADC_UPPER16_MSK		GENMASK(31, 16)
> > +#define AXI_ADC_UPPER16_SET(x)		FIELD_PREP(AXI_ADC_UPPER16_MSK,
> > x)
> > +#define AXI_ADC_UPPER16_GET(x)		FIELD_GET(AXI_ADC_UPPER16_MSK,
> > x)
> > +
> > +#define AXI_ADC_LOWER16_MSK		GENMASK(15, 0)
> > +#define AXI_ADC_LOWER16_SET(x)		FIELD_PREP(AXI_ADC_UPPER16_MSK,
> > x)
> > +#define AXI_ADC_LOWER16_GET(x)		FIELD_GET(AXI_ADC_LOWER16_MSK,
> > x)
> > +
> > +/* ADC controls */
> > +
> > +#define AXI_ADC_REG_RSTN			0x0040
> > +#define   AXI_ADC_MMCM_RSTN			BIT(1)
> > +#define   AXI_ADC_RSTN				BIT(0)
> > +
> > +#define AXI_ADC_REG_CNTRL			0x0044
> > +#define   AXI_ADC_R1_MODE			BIT(2)
> > +#define   AXI_ADC_DDR_EDGESEL			BIT(1)
> > +#define   AXI_ADC_PIN_MODE			BIT(0)
> > +
> > +#define AXI_ADC_REG_CLK_FREQ			0x0054
> > +#define AXI_ADC_REG_CLK_RATIO			0x0058
> > +
> > +#define AXI_ADC_REG_STATUS			0x005C
> > +#define   AXI_ADC_MUX_PN_ERR			BIT(3)
> > +#define   AXI_ADC_MUX_PN_OOS			BIT(2)
> > +#define   AXI_ADC_MUX_OVER_RANGE		BIT(1)
> > +#define   AXI_ADC_STATUS			BIT(0)
> > +
> > +#define AXI_ADC_REG_DRP_CNTRL			0x0070
> > +#define   AXI_ADC_DRP_SEL			BIT(29)
> > +#define   AXI_ADC_DRP_RWN			BIT(28)
> > +#define   AXI_ADC_DRP_ADDRESS_MSK		GENMASK(27, 16)
> > +#define   AXI_ADC_DRP_ADDRESS_SET(x)		\
> > +		FIELD_PREP(AXI_ADC_DRP_ADDRESS_MSK, x)
> > +#define   AXI_ADC_DRP_ADDRESS_GET(x)		\
> > +		FIELD_GET(AXI_ADC_DRP_ADDRESS_MSK, x)
> > +#define   AXI_ADC_DRP_WDATA_SET			AXI_ADC_LOWER16_SET
> > +#define   AXI_ADC_DRP_WDATA_GET			AXI_ADC_LOWER16_GET
> > +
> > +#define AXI_REG_DRP_STATUS			0x0074
> > +#define   AXI_ADC_DRP_STATUS			BIT(16)
> > +#define   AXI_ADC_DRP_RDATA_SET			AXI_ADC_LOWER16_SET
> > +#define   AXI_ADC_DRP_RDATA_GET			AXI_ADC_LOWER16_GET
> > +
> > +#define AXI_ADC_REG_DMA_STATUS			0x0088
> > +#define   AXI_ADC_DMA_OVF			BIT(2)
> > +#define   AXI_ADC_DMA_UNF			BIT(1)
> > +#define   AXI_ADC_DMA_STATUS			BIT(0)
> > +
> > +#define ADI_REG_DMA_BUSWIDTH			0x008C
> > +#define AXI_ADC_REG_GP_CONTROL			0x00BC
> > +#define AXI_ADC_REG_ADC_DP_DISABLE		0x00C0
> > +
> > +/* ADC Channel controls */
> > +
> > +#define AXI_ADC_REG_CHAN_CNTRL(c)		(0x0400 + (c) * 0x40)
> > +#define   AXI_ADC_PN_SEL			BIT(10)
> > +#define   AXI_ADC_IQCOR_ENB			BIT(9)
> > +#define   AXI_ADC_DCFILT_ENB			BIT(8)
> > +#define   AXI_ADC_FORMAT_SIGNEXT		BIT(6)
> > +#define   AXI_ADC_FORMAT_TYPE			BIT(5)
> > +#define   AXI_ADC_FORMAT_ENABLE			BIT(4)
> > +#define   AXI_ADC_PN23_TYPE			BIT(1)
> > +#define   AXI_ADC_ENABLE			BIT(0)
> > +
> > +#define AXI_ADC_REG_CHAN_STATUS(c)		(0x0404 + (c) * 0x40)
> > +#define   AXI_ADC_PN_ERR			BIT(2)
> > +#define   AXI_ADC_PN_OOS			BIT(1)
> > +#define   AXI_ADC_OVER_RANGE			BIT(0)
> > +
> > +#define AXI_ADC_REG_CHAN_CNTRL_1(c)		(0x0410 + (c) * 0x40)
> > +#define   AXI_ADC_DCFILT_OFFSET_MSK		AXI_ADC_UPPER16_MSK
> > +#define   AXI_ADC_DCFILT_OFFSET_SET		AXI_ADC_UPPER16_SET
> > +#define   AXI_ADC_DCFILT_OFFSET_GET		AXI_ADC_UPPER16_GET
> > +#define   AXI_ADC_DCFILT_COEFF_MSK		AXI_ADC_LOWER16_MSK
> > +#define   AXI_ADC_DCFILT_COEFF_SET		AXI_ADC_LOWER16_SET
> > +#define   AXI_ADC_DCFILT_COEFF_GET		AXI_ADC_LOWER16_GET
> > +
> > +#define AXI_ADC_REG_CHAN_CNTRL_2(c)		(0x0414 + (c) * 0x40)
> > +#define   AXI_ADC_IQCOR_COEFF_1_MSK		AXI_ADC_UPPER16_MSK
> > +#define   AXI_ADC_IQCOR_COEFF_1_SET		AXI_ADC_UPPER16_SET
> > +#define   AXI_ADC_IQCOR_COEFF_1_GET		AXI_ADC_UPPER16_GET
> > +#define   AXI_ADC_IQCOR_COEFF_2_MSK		AXI_ADC_LOWER16_MSK
> > +#define   AXI_ADC_IQCOR_COEFF_2_SET		AXI_ADC_LOWER16_SET
> > +#define   AXI_ADC_IQCOR_COEFF_2_GET		AXI_ADC_LOWER16_GET
> > +
> > +/*  format is 1.1.14 (sign, integer and fractional bits) */
> > +#define AXI_ADC_IQCOR_INT_1			0x4000UL
> > +#define AXI_ADC_IQCOR_SIGN_BIT			BIT(15)
> > +/* The constant below is (2 * PI * 0x4000), where 0x4000 is
> > AXI_ADC_IQCOR_INT_1 */
> > +#define AXI_ADC_2_X_PI_X_INT_1			102944ULL
> > +
> > +#define AXI_ADC_REG_CHAN_CNTRL_3(c)		(0x0418 + (c) * 0x40)
> > +#define   AXI_ADC_ADC_PN_SEL_MSK		AXI_ADC_UPPER16_MSK
> > +#define   AXI_ADC_ADC_PN_SEL_SET		AXI_ADC_UPPER16_SET
> > +#define   AXI_ADC_ADC_PN_SEL_GET		AXI_ADC_UPPER16_GET
> > +#define   AXI_ADC_ADC_DATA_SEL_MSK		AXI_ADC_LOWER16_MSK
> > +#define   AXI_ADC_ADC_DATA_SEL_SET		AXI_ADC_LOWER16_SET
> > +#define   AXI_ADC_ADC_DATA_SEL_GET		AXI_ADC_LOWER16_GET
> > +
> > +#define AXI_ADC_REG_CHAN_USR_CNTRL_2(c)		(0x0424 + (c) * 0x40)
> > +#define   AXI_ADC_USR_DECIMATION_M_MSK		AXI_ADC_UPPER16_MSK
> > +#define   AXI_ADC_USR_DECIMATION_M_SET		AXI_ADC_UPPER16_SET
> > +#define   AXI_ADC_USR_DECIMATION_M_GET		AXI_ADC_UPPER16_GET
> > +#define   AXI_ADC_USR_DECIMATION_N_MSK		AXI_ADC_LOWER16_MSK
> > +#define   AXI_ADC_USR_DECIMATION_N_SET		AXI_ADC_LOWER16_SET
> > +#define   AXI_ADC_USR_DECIMATION_N_GET		AXI_ADC_LOWER16_GET
> > +
> > +/* debugfs direct register access */
> > +#define DEBUGFS_DRA_PCORE_REG_MAGIC		BIT(31)
> > +
> > +struct axi_adc_core_info {
> > +	unsigned int			version;
> > +};
> > +
> > +struct axi_adc_state {
> > +	struct mutex			lock;
> > +
> > +	struct axi_adc_client		*client;
> > +	void __iomem			*regs;
> > +	unsigned int			regs_size;
> > +};
> > +
> > +struct axi_adc_client {
> > +	struct list_head		entry;
> > +	struct axi_adc_conv		conv;
> > +	struct axi_adc_state		*state;
> > +	struct device			*dev;
> > +	const struct axi_adc_core_info	*info;
> > +};
> > +
> > +static LIST_HEAD(axi_adc_registered_clients);
> > +static DEFINE_MUTEX(axi_adc_registered_clients_lock);
> > +
> > +static struct axi_adc_client *axi_adc_conv_to_client(struct axi_adc_conv
> > *conv)
> > +{
> > +	if (!conv)
> > +		return NULL;
> > +	return container_of(conv, struct axi_adc_client, conv);
> > +}
> > +
> > +void *axi_adc_conv_priv(struct axi_adc_conv *conv)
> > +{
> > +	struct axi_adc_client *cl = axi_adc_conv_to_client(conv);
> > +
> > +	if (!cl)
> > +		return NULL;
> > +
> > +	return (char *)cl + ALIGN(sizeof(struct axi_adc_client), IIO_ALIGN);
> > +}
> > +EXPORT_SYMBOL_GPL(axi_adc_conv_priv);
> > +
> > +static void axi_adc_write(struct axi_adc_state *st, unsigned int reg,
> > +			  unsigned int val)
> > +{
> > +	iowrite32(val, st->regs + reg);
> > +}
> > +
> > +static unsigned int axi_adc_read(struct axi_adc_state *st, unsigned int
> > reg)
> > +{
> > +	return ioread32(st->regs + reg);
> > +}
> > +
> > +static int 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 axi_adc_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct axi_adc_state *st = iio_priv(indio_dev);
> > +	struct axi_adc_conv *conv = &st->client->conv;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		/* fall-through */
> > +	default:
> > +		if (!conv->read_raw)
> > +			return -ENOSYS;
> > +
> > +		return conv->read_raw(conv, chan, val, val2, mask);
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int axi_adc_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct axi_adc_state *st = iio_priv(indio_dev);
> > +	struct axi_adc_conv *conv = &st->client->conv;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		/* fall-through */
> 
> Umm.  Got to ask. If you only have one option and a default, why have
> the option? Or indeed the switch statement at all..

there are more stuff in the pipeline
particularly the IIO_CHAN_INFO_CALIBSCALE, IIO_CHAN_INFO_CALIBBIAS &
IIO_CHAN_INFO_CALIBPHASE stuff;

i can remove the switch for now, and add it later [when we need it];
maybe after some discussion, we might not needed it at all[?]
no idea;

this driver [in this form] is a rewrite from an older AXI-ADC driver that we've
been developing and using internally;
the HDL guys managed to cleanup their stuff;
this is the Linux cleanup


> > +	default:
> > +		if (!conv->write_raw)
> > +			return -ENOSYS;
> > +
> > +		return conv->write_raw(conv, chan, val, val2, mask);
> > +	}
> > +}
> > +
> > +static int axi_adc_update_scan_mode(struct iio_dev *indio_dev,
> > +				    const unsigned long *scan_mask)
> > +{
> > +	struct axi_adc_state *st = iio_priv(indio_dev);
> > +	struct axi_adc_conv *conv = &st->client->conv;
> > +	unsigned int i, ctrl;
> > +
> > +	for (i = 0; i < conv->chip_info->num_channels; i++) {
> > +		ctrl = axi_adc_read(st, AXI_ADC_REG_CHAN_CNTRL(i));
> > +
> > +		if (test_bit(i, scan_mask))
> > +			ctrl |= AXI_ADC_ENABLE;
> > +		else
> > +			ctrl &= ~AXI_ADC_ENABLE;
> > +
> > +		axi_adc_write(st, AXI_ADC_REG_CHAN_CNTRL(i), ctrl);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +struct axi_adc_conv *axi_adc_conv_register(struct device *dev, int
> > sizeof_priv)
> > +{
> > +	struct axi_adc_client *cl;
> > +	size_t alloc_size;
> > +
> > +	alloc_size = sizeof(struct 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(&axi_adc_registered_clients_lock);
> > +
> > +	get_device(dev);
> > +	cl->dev = dev;
> > +
> > +	list_add_tail(&cl->entry, &axi_adc_registered_clients);
> > +
> > +	mutex_unlock(&axi_adc_registered_clients_lock);
> > +
> > +	return &cl->conv;
> > +}
> > +EXPORT_SYMBOL_GPL(axi_adc_conv_register);
> > +
> > +void axi_adc_conv_unregister(struct axi_adc_conv *conv)
> > +{
> > +	struct axi_adc_client *cl = axi_adc_conv_to_client(conv);
> > +
> > +	if (!cl)
> > +		return;
> > +
> > +	mutex_lock(&axi_adc_registered_clients_lock);
> > +
> > +	put_device(cl->dev);
> > +	list_del(&cl->entry);
> > +	kfree(cl);
> > +
> > +	mutex_unlock(&axi_adc_registered_clients_lock);
> > +}
> > +EXPORT_SYMBOL(axi_adc_conv_unregister);
> 
> You could avoid exporting the non devm versions to encourage people
> to only use the managed ones.

ack

> 
> > +
> > +static void devm_axi_adc_conv_release(struct device *dev, void *res)
> > +{
> > +	axi_adc_conv_unregister(*(struct axi_adc_conv **)res);
> > +}
> > +
> > +static int devm_axi_adc_conv_match(struct device *dev, void *res, void
> > *data)
> > +{
> > +	struct axi_adc_conv **r = res;
> > +
> > +	return *r == data;
> > +}
> > +
> > +struct axi_adc_conv *devm_axi_adc_conv_register(struct device *dev,
> > +						int sizeof_priv)
> > +{
> > +	struct axi_adc_conv **ptr, *conv;
> > +
> > +	ptr = devres_alloc(devm_axi_adc_conv_release, sizeof(*ptr),
> > +			   GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	conv = 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_axi_adc_conv_register);
> > +
> > +void devm_axi_adc_conv_unregister(struct device *dev,
> > +				  struct axi_adc_conv *conv)
> Note that there is no 'need' to provide devm unregister functions
> if it is unlikely a driver will actually use them.
> 
> May well be better to clean the ones in here out until we
> actually need them.  If nothing else having them may encourage
> bad driver architecture.
> 
> hohum.  I should probably just remove the ones in the IIO core
> that never get used as well...
> 

ack
will remove

> devm_iio_device_unregister for example.

hmm; devm_iio_device_unregister() sounds like an interesting GSoC project

> 
> > +{
> > +	int rc;
> > +
> > +	rc = devres_release(dev, devm_axi_adc_conv_release,
> > +			    devm_axi_adc_conv_match, conv);
> > +	WARN_ON(rc);
> > +}
> > +EXPORT_SYMBOL_GPL(devm_axi_adc_conv_unregister);
> > +
> > +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 axi_adc_state *st = iio_priv(indio_dev);
> > +	struct axi_adc_conv *conv = &st->client->conv;
> > +	size_t len = 0;
> > +	int i;
> > +
> > +	if (!conv->chip_info->num_scales) {
> > +		buf[0] = '\n';
> > +		return 1;
> > +	}
> > +
> > +	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 *axi_adc_attributes[] = {
> > +	&iio_dev_attr_in_voltage_scale_available.dev_attr.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group axi_adc_attribute_group = {
> > +	.attrs = axi_adc_attributes,
> > +};
> > +
> > +static const struct iio_info axi_adc_info = {
> > +	.read_raw = &axi_adc_read_raw,
> > +	.write_raw = &axi_adc_write_raw,
> > +	.attrs = &axi_adc_attribute_group,
> > +	.update_scan_mode = &axi_adc_update_scan_mode,
> > +};
> > +
> > +static const struct axi_adc_core_info 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 axi_adc_of_match[] = {
> > +	{ .compatible = "adi,axi-adc-10.0.a", .data = &axi_adc_10_0_a_info },
> > +	{ /* end of list */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, axi_adc_of_match);
> > +
> > +struct axi_adc_client *axi_adc_attach_client(struct device *dev)
> > +{
> > +	const struct of_device_id *id;
> > +	struct 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(axi_adc_of_match, dev->of_node);
> > +	if (!id)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	cln = of_parse_phandle(dev->of_node, "axi-adc-client", 0);
> > +	if (!cln) {
> > +		dev_err(dev, "No 'axi-adc-client' node defined\n");
> > +		return ERR_PTR(-ENODEV);
> > +	}
> > +
> > +	mutex_lock(&axi_adc_registered_clients_lock);
> > +
> > +	list_for_each_entry(cl, &axi_adc_registered_clients, entry) {
> > +		if (!cl->dev)
> > +			continue;
> > +		if (cl->dev->of_node == cln) {
> > +			if (!try_module_get(dev->driver->owner)) {
> > +				mutex_unlock(&axi_adc_registered_clients_lock);
> > +				return ERR_PTR(-ENODEV);
> > +			}
> > +			get_device(dev);
> > +			cl->info = id->data;
> > +			mutex_unlock(&axi_adc_registered_clients_lock);
> > +			return cl;
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&axi_adc_registered_clients_lock);
> > +
> > +	return ERR_PTR(-EPROBE_DEFER);
> > +}
> > +
> > +static int axi_adc_setup_channels(struct device *dev, struct axi_adc_state
> > *st)
> > +{
> > +	struct 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 = AXI_ADC_IQCOR_COEFF_2_SET(AXI_ADC_IQCOR_INT_1);
> > +		else
> > +			val = AXI_ADC_IQCOR_COEFF_1_SET(AXI_ADC_IQCOR_INT_1);
> > +		axi_adc_write(st, AXI_ADC_REG_CHAN_CNTRL_2(i), val);
> > +
> > +		axi_adc_write(st, AXI_ADC_REG_CHAN_CNTRL(i),
> > +			      AXI_ADC_FORMAT_SIGNEXT | AXI_ADC_FORMAT_ENABLE |
> > +			      AXI_ADC_IQCOR_ENB | AXI_ADC_ENABLE);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int axi_adc_alloc_channels(struct iio_dev *indio_dev,
> > +				  struct axi_adc_conv *conv)
> > +{
> > +	unsigned int i, num = conv->chip_info->num_channels;
> > +	struct device *dev = indio_dev->dev.parent;
> > +	struct iio_chan_spec *channels;
> > +
> > +	channels = devm_kcalloc(dev, num, sizeof(*channels), GFP_KERNEL);
> > +	if (!channels)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < conv->chip_info->num_channels; i++)
> > +		channels[i] = conv->chip_info->channels->iio_chan;
> > +
> > +	indio_dev->num_channels = num;
> > +	indio_dev->channels = channels;
> > +
> > +	return 0;
> > +}
> > +
> > +struct axi_adc_cleanup_data {
> > +	struct axi_adc_state	*st;
> > +	struct axi_adc_client	*cl;
> > +};
> 
> Doesn't seem to be used.

Yeah.
My bad; left-over from some mid-term rewrite.
Thanks for catching.

> 
> > +
> > +static void axi_adc_cleanup(void *data)
> > +{
> > +	struct axi_adc_client *cl = data;
> > +
> > +	put_device(cl->dev);
> > +	module_put(cl->dev->driver->owner);
> > +}
> > +
> > +static int axi_adc_probe(struct platform_device *pdev)
> > +{
> > +	struct axi_adc_conv *conv;
> > +	struct iio_dev *indio_dev;
> > +	struct axi_adc_client *cl;
> > +	struct axi_adc_state *st;
> > +	struct resource *mem;
> > +	unsigned int ver;
> > +	int ret;
> > +
> > +	cl = axi_adc_attach_client(&pdev->dev);
> > +	if (IS_ERR(cl))
> > +		return PTR_ERR(cl);
> > +
> > +	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);
> > +
> > +	ret = devm_add_action_or_reset(&pdev->dev, axi_adc_cleanup, cl);
> 
> This is unwinding things in axi_adc_attach_client, so should be
> called immediately after that, not with the iio device allocation
> in between.
> 

good point


> > +	if (ret)
> > +		return ret;
> > +
> > +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	st->regs_size = resource_size(mem);
> > +	st->regs = devm_ioremap_resource(&pdev->dev, mem);
> 
> Can we use devm_platform_ioremap_resource here?
> We grab regs_size but don't seem to use it.
> 

hmmm; so, the 'regs_size' does get used eventually
with some debugfs support being added;
but maybe after another discussion a better idea could be found;
i guess i can remove it for now

> 
> > +	if (IS_ERR(st->regs))
> > +		return PTR_ERR(st->regs);
> > +
> > +	conv = &st->client->conv;
> > +
> > +	/* Reset HDL Core */
> > +	axi_adc_write(st, AXI_ADC_REG_RSTN, 0);
> > +	mdelay(10);
> > +	axi_adc_write(st, AXI_ADC_REG_RSTN, AXI_ADC_MMCM_RSTN);
> > +	mdelay(10);
> > +	axi_adc_write(st, AXI_ADC_REG_RSTN, AXI_ADC_RSTN | AXI_ADC_MMCM_RSTN);
> > +
> > +	ver = 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 = &axi_adc_info;
> > +	indio_dev->dev.parent = &pdev->dev;
> > +	indio_dev->name = pdev->dev.of_node->name;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	ret = axi_adc_alloc_channels(indio_dev, conv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = axi_adc_config_dma_buffer(&pdev->dev, indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = 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 axi_adc_driver = {
> > +	.driver = {
> > +		.name = KBUILD_MODNAME,
> > +		.of_match_table = axi_adc_of_match,
> > +	},
> > +	.probe = axi_adc_probe,
> > +};
> > +
> > +module_platform_driver(axi_adc_driver);
> > +
> > +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@...log.com>");
> > +MODULE_DESCRIPTION("Analog Devices Generic AXI ADC IP core driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/iio/adc/axi-adc.h b/include/linux/iio/adc/axi-
> > adc.h
> > new file mode 100644
> > index 000000000000..d367c442dc52
> > --- /dev/null
> > +++ b/include/linux/iio/adc/axi-adc.h
> > @@ -0,0 +1,79 @@
> > +/* 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 __AXI_ADC_H__
> > +#define __AXI_ADC_H__
> > +
> > +struct device;
> > +
> > +/**
> > + * struct axi_adc_chan_spec - AXI ADC channel wrapper
> > + *			      maps IIO channel data with AXI ADC specifics
> > + * @iio_chan		IIO channel specification
> > + * @num_lanes		Number of lanes per channel
> > + */
> > +struct axi_adc_chan_spec {
> > +	struct iio_chan_spec		iio_chan;
> > +	unsigned int			num_lanes;
> > +};
> > +
> > +/**
> > + * struct 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 axi_adc_chip_info {
> > +	const char			*name;
> > +	unsigned int			id;
> > +
> > +	const struct axi_adc_chan_spec	*channels;
> > +	unsigned int			num_channels;
> > +
> > +	const unsigned int		(*scale_table)[2];
> > +	int				num_scales;
> > +
> > +	unsigned long			max_rate;
> > +};
> > +
> > +/**
> > + * struct 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
> > + * @read_raw		IIO read_raw hook for the client ADC
> > + * @write_raw		IIO write_raw hook for the client ADC
> > + */
> > +struct axi_adc_conv {
> > +	const struct axi_adc_chip_info		*chip_info;
> > +
> > +	int (*preenable_setup)(struct axi_adc_conv *conv);
> > +	int (*reg_access)(struct axi_adc_conv *conv, unsigned int reg,
> > +			  unsigned int writeval, unsigned int *readval);
> > +	int (*read_raw)(struct axi_adc_conv *conv,
> > +			struct iio_chan_spec const *chan,
> > +			int *val, int *val2, long mask);
> > +	int (*write_raw)(struct axi_adc_conv *conv,
> > +			 struct iio_chan_spec const *chan,
> > +			 int val, int val2, long mask);
> > +};
> > +
> > +struct axi_adc_conv *axi_adc_conv_register(struct device *dev,
> > +					   int sizeof_priv);
> > +void axi_adc_conv_unregister(struct axi_adc_conv *conv);
> > +
> > +struct axi_adc_conv *devm_axi_adc_conv_register(struct device *dev,
> > +						int sizeof_priv);
> > +void devm_axi_adc_conv_unregister(struct device *dev,
> > +				  struct axi_adc_conv *conv);
> > +
> > +void *axi_adc_conv_priv(struct axi_adc_conv *conv);
> > +
> > +#endif

Powered by blists - more mailing lists