[[PATCH v2] 0/2] Add Altera Modular ADC Driver

[[PATCH v2] 0/2] Add Altera Modular ADC Driver

[Chee Nouk Phoo]

From: Chee Nouk Phoon <cnphoon@altera.com>

This is 2nd version of the patch set to add support for Altera Modular ADC and
Altera Modular Dual ADC in MAX10 device family. This patch combines two separate
patches of driver and device tree binding. 

v1->v2 changes:
- update compatibility string
- update macros naming
- remove Nios2 arch dependency
- using snprintf instead of sprintf
- improve error handing
- parse resource using reg-name instead of index
- use _relaxed version of writel and readl
- remove uncessary private data

History:
--------
[v1]https://lkml.org/lkml/2015/8/27/32
    https://lkml.org/lkml/2015/8/20/227

Chee Nouk Phoon (2):
  Altera Modular ADC driver device binding
  Altera Modular ADC driver support

[[PATCH v2] 1/2] Altera Modular ADC driver device binding

[Chee Nouk Phoo]

From: Chee Nouk Phoon <cnphoon@altera.com>

Altera Modular ADC is soft IP that wraps the hardened ADC block in a Max10
device. It can be configured to dual ADC mode that supports two channel
synchronous sampling or independent single ADCs. ADC sampled values will be
written into memory slots in sequence determined by a user configurable
sequencer block.

This patch adds Altera Modular ADC driver device tree binding

Signed-off-by: Chee Nouk Phoon <cnphoon@altera.com>
---
 .../bindings/iio/adc/altr,modular-adc.txt          |   63 ++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/altr,modular-adc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/altr,modular-adc.txt b/Documentation/devicetree/bindings/iio/adc/altr,modular-adc.txt
new file mode 100644
index 0000000..faafcac
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/altr,modular-adc.txt
@@ -0,0 +1,63 @@
+Altera Modular (Dual) ADC
+
+This binding document describes both Altera Modular ADC and Altera Modular Dual
+ADC. Both options can be configured during generation time in Qsys. This driver
+only supports standard sequencer with Avalon-MM sample storage with up to 64
+memory slots.
+
+Required properties:
+- compatible: must be one of the following strings
+  "altr,modular-adc-1.0": single ADC configuration
+  "altr,modular-dual-adc-1.0": dual ADC configuration
+
+- reg: Address and length of the register set for the device. It contains the
+  information of registers in the same order as described by reg-names
+
+- reg-names: Should contain the reg names
+  "sequencer_csr": register region for adc sequencer block
+  "sample_store_csr": register region for sample store block
+
+- interrupts: interrupt line for ADC
+
+- altr,adc-mode : ADC configuration
+  1: single ADC mode
+  2: dual ADC mode
+
+- altr,adc-slot-count : specify number of conversion slot in use
+
+- altr,adc<ADC index>-slot-sequence-<slot index>: specify ADC channel
+  conversion sequence
+  <ADC index>: instantiated ADC number
+  <slot index>: slot index for ADC memory slot
+
+- altr,adc-number : specify ADC number when single ADC mode is selected
+  1: 1st ADC
+  2: 2nd ACD
+
+Example: single ADC
+modular_adc_0: adc@0x20000200 {
+  compatible = "altr,modular-adc";
+  reg = <0x20000000 0x00000008>,
+    <0x20000200 0x00000200>;
+  reg-names = "sequencer_csr", "sample_store_csr";
+  interrupt-parent = <&cpu>;
+  interrupts = <8>;
+  altr,adc-mode = <1>;
+  altr,adc-slot-count = <2>;
+  altr,adc1-slot-sequence-1 = <1>;
+  altr,adc-number = <1>;
+};
+
+Example: dual ADC
+modular_adc_1: adc@0x18002200 {
+  compatible = "altr,modular-dual-adc";
+  reg = <0x18002000 0x00000008>,
+    <0x18002200 0x00000200>;
+  reg-names = "sequencer_csr", "sample_store_csr";
+  interrupt-parent = <&cpu>;
+  interrupts = <8>;
+  altr,adc-mode = <2>;
+  altr,adc-slot-count = <1>;
+  altr,adc1-slot-sequence-1 = <6>;
+  altr,adc2-slot-sequence-1 = <6>;
+};
\ No newline at end of file
-- 
1.7.1

Re: [[PATCH v2] 1/2] Altera Modular ADC driver device binding

[Jonathan Cameron]

On 01/09/15 22:49, Chee Nouk Phoo wrote:
> From: Chee Nouk Phoon <cnphoon@altera.com>
> 
> Altera Modular ADC is soft IP that wraps the hardened ADC block in a Max10
> device. It can be configured to dual ADC mode that supports two channel
> synchronous sampling or independent single ADCs. ADC sampled values will be
> written into memory slots in sequence determined by a user configurable
> sequencer block.
> 
> This patch adds Altera Modular ADC driver device tree binding
I think the convention is still to explicitly cc all device tree maintainers on bindings
patches (added).

A few bits and pieces in line.  Basically I'd like more explanation + examples to
make it clear what is going on.  This hardware has some unusual corners
(inherent in being a highly configurable bit of IP) so perhaps needs more than
we would normally expect in the way of description

Thanks,

Jonathan
> 
> Signed-off-by: Chee Nouk Phoon <cnphoon@altera.com>
> ---
>  .../bindings/iio/adc/altr,modular-adc.txt          |   63 ++++++++++++++++++++
>  1 files changed, 63 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/altr,modular-adc.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/altr,modular-adc.txt b/Documentation/devicetree/bindings/iio/adc/altr,modular-adc.txt
> new file mode 100644
> index 0000000..faafcac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/altr,modular-adc.txt
> @@ -0,0 +1,63 @@
> +Altera Modular (Dual) ADC
> +
> +This binding document describes both Altera Modular ADC and Altera Modular Dual
> +ADC. Both options can be configured during generation time in Qsys. This driver
> +only supports standard sequencer with Avalon-MM sample storage with up to 64
> +memory slots.
> +
> +Required properties:
> +- compatible: must be one of the following strings
> +  "altr,modular-adc-1.0": single ADC configuration
> +  "altr,modular-dual-adc-1.0": dual ADC configuration
> +
> +- reg: Address and length of the register set for the device. It contains the
> +  information of registers in the same order as described by reg-names
> +
> +- reg-names: Should contain the reg names
> +  "sequencer_csr": register region for adc sequencer block
> +  "sample_store_csr": register region for sample store block
> +
> +- interrupts: interrupt line for ADC
Normal to add a cross reference here to the standard interrupt binding
docs (as there are loads of ways of specifying an interrupt!)
> +
> +- altr,adc-mode : ADC configuration
> +  1: single ADC mode
> +  2: dual ADC mode
I'd guess this is only relevant if the compatible is the dual-adc version?
Please document that if so.
> +
> +- altr,adc-slot-count : specify number of conversion slot in use
slots  Could you add an example below of multiple slots in use?
Would make it clearer what is going on here and what the expected
syntax is for the bindings.

> +
> +- altr,adc<ADC index>-slot-sequence-<slot index>: specify ADC channel
> +  conversion sequence
> +  <ADC index>: instantiated ADC number
> +  <slot index>: slot index for ADC memory slot
What for does the value take?

> +
> +- altr,adc-number : specify ADC number when single ADC mode is selected
> +  1: 1st ADC
> +  2: 2nd ACD
ADC

> +
> +Example: single ADC
> +modular_adc_0: adc@0x20000200 {
> +  compatible = "altr,modular-adc";
> +  reg = <0x20000000 0x00000008>,
> +    <0x20000200 0x00000200>;
> +  reg-names = "sequencer_csr", "sample_store_csr";
> +  interrupt-parent = <&cpu>;
> +  interrupts = <8>;
> +  altr,adc-mode = <1>;
> +  altr,adc-slot-count = <2>;
> +  altr,adc1-slot-sequence-1 = <1>;
> +  altr,adc-number = <1>;
> +};
> +
> +Example: dual ADC
> +modular_adc_1: adc@0x18002200 {
> +  compatible = "altr,modular-dual-adc";
> +  reg = <0x18002000 0x00000008>,
> +    <0x18002200 0x00000200>;
> +  reg-names = "sequencer_csr", "sample_store_csr";
> +  interrupt-parent = <&cpu>;
> +  interrupts = <8>;
> +  altr,adc-mode = <2>;
> +  altr,adc-slot-count = <1>;
These needs some comments to explain the settings.
There is more here than simply dual vs single ADCs.  Perhaps
even a more complex example with multipel slots in use would
clarify things?  As I understand it the slot sequencing is baked
into the fpga logic and not configurable at runtime?
Perhaps add some detail of that in this document as well to make it
easier to review.

> +  altr,adc1-slot-sequence-1 = <6>;
> +  altr,adc2-slot-sequence-1 = <6>;
> +};
There's a hint here.  There should be a new line at the end of the file ;)

> \ No newline at end of file
> 

[[PATCH v2] 2/2] Altera Modular ADC driver support

[Chee Nouk Phoo]

From: Chee Nouk Phoon <cnphoon@altera.com>

Altera Modular ADC is soft IP that wraps the hardened ADC block in a Max10
device. It can be configured to dual ADC mode that supports two channel
synchronous sampling or independent single ADCs. ADC sampled values will be
written into memory slots in sequence determined by a user configurable
sequencer block.

This patch adds Altera Modular ADC driver support

Signed-off-by: Chee Nouk Phoon <cnphoon@altera.com>
---
 drivers/iio/adc/Kconfig           |   11 ++
 drivers/iio/adc/Makefile          |    1 +
 drivers/iio/adc/alt_modular_adc.c |  322 +++++++++++++++++++++++++++++++++++++
 3 files changed, 334 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 drivers/iio/adc/Kconfig
 create mode 100755 drivers/iio/adc/alt_modular_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
old mode 100644
new mode 100755
index 50c103d..e1a67cb
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -118,6 +118,17 @@ config AD799X
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad799x.
 
+config ALT_MODULAR_ADC
+	tristate "Altera Modular ADC driver"
+	select ANON_INODES
+
+	help
+	  Say yes here to have support for Altera Modular ADC. The driver
+	  supports both Altera Modular ADC and Altera Modular Dual ADC.
+
+	  The driver can also be build as a module. If so the module will be
+	  called alt_modular_adc.
+
 config AT91_ADC
 	tristate "Atmel AT91 ADC"
 	depends on ARCH_AT91
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index a096210..d7f10e0 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_VF610_ADC) += vf610_adc.o
 obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
 xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o
 obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
+obj-$(CONFIG_ALT_MODULAR_ADC) += alt_modular_adc.o
diff --git a/drivers/iio/adc/alt_modular_adc.c b/drivers/iio/adc/alt_modular_adc.c
new file mode 100755
index 0000000..3d2d870
--- /dev/null
+++ b/drivers/iio/adc/alt_modular_adc.c
@@ -0,0 +1,322 @@
+/*
+ * Copyright (C) 2015 Altera Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <linux/iio/iio.h>
+
+/* Constant Definitions */
+#define MAX_SLOT		64
+#define MAX_ADC			2
+#define MAX_CHANNEL		18
+#define MODE_SINGLE_ADC		1
+#define MODE_DUAL_ADC		2
+#define ADC_BITS		12
+#define ADC_STORE_BITS		16
+#define ADC_MAX_STR_SIZE        20
+
+/* Register Definitions */
+#define ADC_CMD_REG		0x0
+#define ADC_IER_REG		0x100
+#define ADC_ISR_REG		0x104
+
+#define ADC_RUN_MSK		0x1
+#define ADC_SINGLE_MKS		0x2
+#define ADC_STOP_MSK		0x0
+#define ADC_LOW_MSK		0xFFF
+#define ADC_HIGH_MSK		0xFFF0000
+
+struct altera_adc {
+	void __iomem		*seq_regs;
+	void __iomem		*sample_regs;
+
+	unsigned int		mode;
+	unsigned int		slot_count;
+	unsigned int		slot_sequence[MAX_ADC][MAX_SLOT];
+	unsigned int		adc_number;
+};
+
+static int alt_modular_adc_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val,
+				int *val2,
+				long mask)
+{
+	struct altera_adc *adc = iio_priv(indio_dev);
+	int ret = -EINVAL;
+	u32 value;
+
+	if (mask != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	value = readl_relaxed(adc->sample_regs + (chan->address * 4));
+
+	if (adc->mode == MODE_SINGLE_ADC) {
+		*val = (value & ADC_LOW_MSK);
+		ret = IIO_VAL_INT;
+	} else if (adc->mode == MODE_DUAL_ADC) {
+		*val = (value & ADC_LOW_MSK);
+		*val2 = ((value & ADC_HIGH_MSK) >> 16);
+		ret = IIO_VAL_INT_MULTIPLE;
+	}
+
+	return ret;
+}
+
+static const struct iio_info adc_iio_info = {
+	.read_raw = &alt_modular_adc_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int alt_modular_adc_channel_init(struct iio_dev *indio_dev)
+{
+	struct altera_adc *adc = iio_priv(indio_dev);
+	struct iio_chan_spec *chan_array;
+	struct iio_chan_spec *chan;
+	char **channel_name;
+	int i;
+
+	chan_array = devm_kzalloc(&indio_dev->dev, (adc->slot_count *
+		sizeof(*chan_array)), GFP_KERNEL);
+	if (chan_array == NULL)
+		return -ENOMEM;
+
+	channel_name = devm_kzalloc(&indio_dev->dev, adc->slot_count,
+		GFP_KERNEL);
+	if (channel_name == NULL)
+		return -ENOMEM;
+
+	for (i = 0; i < adc->slot_count; i++) {
+		channel_name[i] = devm_kzalloc(&indio_dev->dev,
+				ADC_MAX_STR_SIZE, GFP_KERNEL);
+		if (channel_name == NULL)
+			return -ENOMEM;
+	}
+
+	chan = chan_array;
+	for (i = 0; i < adc->slot_count; i++, chan++) {
+		chan->type = IIO_VOLTAGE;
+		chan->indexed = 1;
+		chan->channel = i;
+		chan->address = i;
+
+		/* Construct iio sysfs name*/
+		if (adc->mode == MODE_SINGLE_ADC) {
+			snprintf(channel_name[i], ADC_MAX_STR_SIZE,
+				"adc%d-ch%d",
+				adc->adc_number,
+				adc->slot_sequence[0][i]);
+		} else if (adc->mode == MODE_DUAL_ADC) {
+			snprintf(channel_name[i], ADC_MAX_STR_SIZE,
+				"adc1-ch%d_adc2-ch%d",
+				adc->slot_sequence[0][i],
+				adc->slot_sequence[1][i]);
+		} else {
+			return -EINVAL;
+		}
+
+		chan->datasheet_name = channel_name[i];
+		chan->extend_name = channel_name[i];
+		chan->scan_index = i;
+		chan->scan_type.sign = 'u';
+		chan->scan_type.realbits = ADC_BITS;
+		chan->scan_type.storagebits = ADC_STORE_BITS;
+		chan->scan_type.endianness = IIO_CPU;
+		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	}
+
+	indio_dev->channels = chan_array;
+
+	return 0;
+}
+
+static const struct of_device_id alt_modular_adc_match[] = {
+	{ .compatible = "altr,modular-adc-1.0" },
+	{ .compatible = "altr,modular-dual-adc-1.0" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, alt_modular_adc_match);
+
+static int alt_modular_adc_parse_dt(struct iio_dev *indio_dev,
+				struct device *dev)
+{
+	struct altera_adc *adc = iio_priv(indio_dev);
+	struct device_node *np = dev->of_node;
+	u32 value;
+	int ret, i, j;
+	char str[50];
+
+	ret = of_property_read_u32(np, "altr,adc-mode", &value);
+	if (ret < 0)
+		return ret;
+	if (value == 0 || value > MAX_ADC) {
+		dev_err(dev, "Invalid ADC mode value");
+		return -EINVAL;
+	}
+	adc->mode = value;
+
+	ret = of_property_read_u32(np, "altr,adc-slot-count", &value);
+	if (ret < 0)
+		return ret;
+	if (value == 0 || value > MAX_SLOT) {
+		dev_err(dev, "Invalid ADC slot count value");
+		return -EINVAL;
+	}
+	adc->slot_count = value;
+
+	ret = of_property_read_u32(np, "altr,adc-number", &value);
+	if (ret < 0)
+		return ret;
+	if (value == 0 || value > MAX_ADC) {
+		dev_err(dev, "Invalid ADC number value");
+		return -EINVAL;
+	}
+	adc->adc_number = value;
+
+	/* Device tree lookup for channels for each memory slots */
+	for (j = 0; j < adc->mode; j++) {
+		for (i = 0; i < adc->slot_count; i++) {
+			str[0] = '\0';
+
+			snprintf(str, sizeof(str),
+				"altr,adc%d-slot-sequence-%d",
+				(j + 1), (i + 1));
+
+			ret = of_property_read_u32(np, str, &value);
+			if (ret < 0)
+				return ret;
+			if (value > MAX_CHANNEL) {
+				dev_err(dev, "Invalid ADC channel value");
+				return -EINVAL;
+			}
+			adc->slot_sequence[j][i] = value;
+		}
+	}
+
+	return 0;
+}
+
+static int alt_modular_adc_probe(struct platform_device *pdev)
+{
+	struct altera_adc *adc;
+	struct device_node *np = pdev->dev.of_node;
+	struct iio_dev *indio_dev;
+	struct resource *mem;
+	int ret;
+
+	if (!np)
+		return -ENODEV;
+
+	indio_dev = iio_device_alloc(sizeof(struct altera_adc));
+	if (!indio_dev) {
+		dev_err(&pdev->dev, "failed allocating iio device\n");
+		return -ENOMEM;
+	}
+
+	adc = iio_priv(indio_dev);
+
+	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+		"sequencer_csr");
+	adc->seq_regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(adc->seq_regs)) {
+		ret = PTR_ERR(adc->seq_regs);
+		goto err_iio;
+	}
+
+	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+		"sample_store_csr");
+	adc->sample_regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(adc->sample_regs)) {
+		ret = PTR_ERR(adc->sample_regs);
+		goto err_iio;
+	}
+
+	ret = alt_modular_adc_parse_dt(indio_dev, &pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to parse device tree\n");
+		goto err_iio;
+	}
+
+	ret = alt_modular_adc_channel_init(indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed initialize ADC channels\n");
+		goto err_iio;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->dev.of_node = pdev->dev.of_node;
+	indio_dev->info = &adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = adc->slot_count;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto err_iio;
+
+	/* Disable Interrupt */
+	writel_relaxed(0, (adc->sample_regs + ADC_IER_REG));
+
+	/* Start Continuous Sampling */
+	writel_relaxed((ADC_RUN_MSK), (adc->seq_regs + ADC_CMD_REG));
+
+	return 0;
+
+
+err_iio:
+	iio_device_free(indio_dev);
+	return ret;
+}
+
+static int alt_modular_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct altera_adc *adc = iio_priv(indio_dev);
+
+	/* Stop ADC */
+	writel((ADC_STOP_MSK), (adc->seq_regs + ADC_CMD_REG));
+
+	/* Unregister ADC */
+	iio_device_unregister(indio_dev);
+
+	return 0;
+}
+
+static struct platform_driver altr_modular_adc_driver = {
+	.probe		= alt_modular_adc_probe,
+	.remove		= alt_modular_adc_remove,
+	.driver		= {
+		.name	= "alt-modular-adc",
+		.owner	= THIS_MODULE,
+		.of_match_table = alt_modular_adc_match,
+	},
+};
+
+
+module_platform_driver(altr_modular_adc_driver);
+
+MODULE_DESCRIPTION("Altera Modular ADC Driver");
+MODULE_AUTHOR("Chee Nouk Phoon <cnphoon@altera.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.1

Re: [[PATCH v2] 2/2] Altera Modular ADC driver support

[Shubhrajyoti Datta]

> +static int alt_modular_adc_probe(struct platform_device *pdev)
> +{
> +       struct altera_adc *adc;
> +       struct device_node *np = pdev->dev.of_node;
> +       struct iio_dev *indio_dev;
> +       struct resource *mem;
> +       int ret;
> +
> +       if (!np)
> +               return -ENODEV;
> +
> +       indio_dev = iio_device_alloc(sizeof(struct altera_adc));
> +       if (!indio_dev) {
> +               dev_err(&pdev->dev, "failed allocating iio device\n");
> +               return -ENOMEM;
> +       }
> +
> +       adc = iio_priv(indio_dev);
> +
> +       mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +               "sequencer_csr");
> +       adc->seq_regs = devm_ioremap_resource(&pdev->dev, mem);
> +       if (IS_ERR(adc->seq_regs)) {
> +               ret = PTR_ERR(adc->seq_regs);
> +               goto err_iio;
> +       }
> +
> +       mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +               "sample_store_csr");
> +       adc->sample_regs = devm_ioremap_resource(&pdev->dev, mem);
> +       if (IS_ERR(adc->sample_regs)) {
> +               ret = PTR_ERR(adc->sample_regs);
> +               goto err_iio;
> +       }
> +
> +       ret = alt_modular_adc_parse_dt(indio_dev, &pdev->dev);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed to parse device tree\n");
> +               goto err_iio;
> +       }
> +
> +       ret = alt_modular_adc_channel_init(indio_dev);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed initialize ADC channels\n");
> +               goto err_iio;
> +       }
> +
> +       platform_set_drvdata(pdev, indio_dev);
> +
> +       indio_dev->name = dev_name(&pdev->dev);
> +       indio_dev->dev.parent = &pdev->dev;
> +       indio_dev->dev.of_node = pdev->dev.of_node;
> +       indio_dev->info = &adc_iio_info;
> +       indio_dev->modes = INDIO_DIRECT_MODE;
> +       indio_dev->num_channels = adc->slot_count;
> +
> +       ret = iio_device_register(indio_dev);
> +       if (ret)
> +               goto err_iio;
> +
> +       /* Disable Interrupt */
> +       writel_relaxed(0, (adc->sample_regs + ADC_IER_REG));

Why disable interrupts?

> +
> +       /* Start Continuous Sampling */
> +       writel_relaxed((ADC_RUN_MSK), (adc->seq_regs + ADC_CMD_REG));
> +
> +       return 0;
> +
> +
> +err_iio:
> +       iio_device_free(indio_dev);
> +       return ret;
> +}
> +
> +static int alt_modular_adc_remove(struct platform_device *pdev)
> +{
> +       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +       struct altera_adc *adc = iio_priv(indio_dev);
> +
> +       /* Stop ADC */
> +       writel((ADC_STOP_MSK), (adc->seq_regs + ADC_CMD_REG));
> +
> +       /* Unregister ADC */
> +       iio_device_unregister(indio_dev);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver altr_modular_adc_driver = {
> +       .probe          = alt_modular_adc_probe,
> +       .remove         = alt_modular_adc_remove,
> +       .driver         = {
> +               .name   = "alt-modular-adc",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = alt_modular_adc_match,
> +       },
> +};
> +
> +
> +module_platform_driver(altr_modular_adc_driver);
> +
> +MODULE_DESCRIPTION("Altera Modular ADC Driver");
> +MODULE_AUTHOR("Chee Nouk Phoon <cnphoon@altera.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [[PATCH v2] 2/2] Altera Modular ADC driver support

[Jonathan Cameron]

On 01/09/15 22:49, Chee Nouk Phoo wrote:
> From: Chee Nouk Phoon <cnphoon@altera.com>
> 
> Altera Modular ADC is soft IP that wraps the hardened ADC block in a Max10
> device. It can be configured to dual ADC mode that supports two channel
> synchronous sampling or independent single ADCs. ADC sampled values will be
> written into memory slots in sequence determined by a user configurable
> sequencer block.
> 
> This patch adds Altera Modular ADC driver support
> 
> Signed-off-by: Chee Nouk Phoon <cnphoon@altera.com>
Sorry, I wrote a review of version 1 but for some reason seem not have
sent it.  Anyhow, will comment on this version now.

I'm feeling rather guilty about this as there are a number of elements
in this driver that are major issues and I meant to get back to you on
them over a week ago!  Oops.

Anyhow, big ones in here are unusual uses of the ABI.  One point to note
is that if you ever add any ABI not documented in Documentation/ABI/testing/*
then you MUST document it as part of your patch.  It saves a lot
of time whilst the reviewer figures out what the ABI being presented
actually is.

Dual ADCs should not have their two elements passed out through a single
sysfs attribute.  If you need to maintain their relationship it must
be done by using the buffered interface not the sysfs one.  Doing it
any other way leads to inconsistent and confusing userspace ABI which
would be a disaster.

Extended naming (tied up with the above) has very limited use cases and
must be documented.  It confuses userspace applications by encoding
information in a freeform way within the attribute name. As such the
form must be clearly documented and justified.  The use case here is
not justified.

Anyhow, whilst these sound major, the driver is actually in pretty good
shape and the other stuff is minor tidy ups / formatting suggestions.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/Kconfig           |   11 ++
>  drivers/iio/adc/Makefile          |    1 +
>  drivers/iio/adc/alt_modular_adc.c |  322 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 334 insertions(+), 0 deletions(-)
>  mode change 100644 => 100755 drivers/iio/adc/Kconfig
>  create mode 100755 drivers/iio/adc/alt_modular_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> old mode 100644
> new mode 100755
> index 50c103d..e1a67cb
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -118,6 +118,17 @@ config AD799X
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad799x.
>  
> +config ALT_MODULAR_ADC
> +	tristate "Altera Modular ADC driver"
> +	select ANON_INODES
> +
> +	help
> +	  Say yes here to have support for Altera Modular ADC. The driver
> +	  supports both Altera Modular ADC and Altera Modular Dual ADC.
> +
> +	  The driver can also be build as a module. If so the module will be
> +	  called alt_modular_adc.
> +
>  config AT91_ADC
>  	tristate "Atmel AT91 ADC"
>  	depends on ARCH_AT91
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a096210..d7f10e0 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -38,3 +38,4 @@ obj-$(CONFIG_VF610_ADC) += vf610_adc.o
>  obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
>  xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o
>  obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
> +obj-$(CONFIG_ALT_MODULAR_ADC) += alt_modular_adc.o
> diff --git a/drivers/iio/adc/alt_modular_adc.c b/drivers/iio/adc/alt_modular_adc.c
> new file mode 100755
> index 0000000..3d2d870
> --- /dev/null
> +++ b/drivers/iio/adc/alt_modular_adc.c
> @@ -0,0 +1,322 @@
> +/*
> + * Copyright (C) 2015 Altera Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/iio/iio.h>
> +
> +/* Constant Definitions */
Please prefix all constants iwth ALT_MODULAR_ADC_ or similar
to avoid potential name clashes in the future.

> +#define MAX_SLOT		64
> +#define MAX_ADC			2
> +#define MAX_CHANNEL		18
> +#define MODE_SINGLE_ADC		1
> +#define MODE_DUAL_ADC		2
> +#define ADC_BITS		12
> +#define ADC_STORE_BITS		16
> +#define ADC_MAX_STR_SIZE        20
> +
> +/* Register Definitions */
> +#define ADC_CMD_REG		0x0
> +#define ADC_IER_REG		0x100
> +#define ADC_ISR_REG		0x104
> +
> +#define ADC_RUN_MSK		0x1
> +#define ADC_SINGLE_MKS		0x2
> +#define ADC_STOP_MSK		0x0
> +#define ADC_LOW_MSK		0xFFF
> +#define ADC_HIGH_MSK		0xFFF0000
> +
I'd like to see kernel-doc description fo this structure.
It's not immediately obvious what all the parts are!

> +struct altera_adc {
> +	void __iomem		*seq_regs;
> +	void __iomem		*sample_regs;
> +
> +	unsigned int		mode;
> +	unsigned int		slot_count;
> +	unsigned int		slot_sequence[MAX_ADC][MAX_SLOT];
Without looking at the bindings doc, it's not obvious what adc_number.
I'd prefer enough local docs to make that clear!
> +	unsigned int		adc_number;
> +};
> +
> +static int alt_modular_adc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val,
> +				int *val2,
> +				long mask)
> +{
> +	struct altera_adc *adc = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +	u32 value;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	value = readl_relaxed(adc->sample_regs + (chan->address * 4));
> +
> +	if (adc->mode == MODE_SINGLE_ADC) {
> +		*val = (value & ADC_LOW_MSK);
> +		ret = IIO_VAL_INT;
> +	} else if (adc->mode == MODE_DUAL_ADC) {
> +		*val = (value & ADC_LOW_MSK);
> +		*val2 = ((value & ADC_HIGH_MSK) >> 16);

No.  This is not part of the ABI and is absolutely not the intent.
You can't pass two separate ADC readings from different channels through
the sysfs interface like this.  It's been requested a number of times
but the write way to do this is to implement the buffered interface
and push the out that way.  One sysfs attribute, one value is the rule!
The exceptions are for element that have no meaning whatsoever on their own
(such as quaternions) where any given element has no scale without the rest.
That is not true here.

> +		ret = IIO_VAL_INT_MULTIPLE;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct iio_info adc_iio_info = {
> +	.read_raw = &alt_modular_adc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int alt_modular_adc_channel_init(struct iio_dev *indio_dev)
> +{
> +	struct altera_adc *adc = iio_priv(indio_dev);
> +	struct iio_chan_spec *chan_array;
> +	struct iio_chan_spec *chan;
> +	char **channel_name;
> +	int i;
> +
> +	chan_array = devm_kzalloc(&indio_dev->dev, (adc->slot_count *
> +		sizeof(*chan_array)), GFP_KERNEL);
> +	if (chan_array == NULL)
> +		return -ENOMEM;
> +
> +	channel_name = devm_kzalloc(&indio_dev->dev, adc->slot_count,
> +		GFP_KERNEL);
> +	if (channel_name == NULL)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < adc->slot_count; i++) {
> +		channel_name[i] = devm_kzalloc(&indio_dev->dev,
> +				ADC_MAX_STR_SIZE, GFP_KERNEL);
No need to do this here, use devm_kasprintf as suggested below.
If not I think you could easily have combined this loop with the one
below simplifying the code.

> +		if (channel_name == NULL)
> +			return -ENOMEM;
> +	}
> +
> +	chan = chan_array;
> +	for (i = 0; i < adc->slot_count; i++, chan++) {
> +		chan->type = IIO_VOLTAGE;
> +		chan->indexed = 1;
> +		chan->channel = i;
> +		chan->address = i;
No need to set address if it is always being set to the same
value as channel.  Channel is available everywhere address is so
just use that value.

> +
> +		/* Construct iio sysfs name*/
> +		if (adc->mode == MODE_SINGLE_ADC) {
Use kasprintf and you can skip the earlier allocation of channel_name[i]
plus allocate exactly what is needed.
> +			snprintf(channel_name[i], ADC_MAX_STR_SIZE,
> +				"adc%d-ch%d",
> +				adc->adc_number,
> +				adc->slot_sequence[0][i]);
> +		} else if (adc->mode == MODE_DUAL_ADC) {
> +			snprintf(channel_name[i], ADC_MAX_STR_SIZE,
> +				"adc1-ch%d_adc2-ch%d",
> +				adc->slot_sequence[0][i],
> +				adc->slot_sequence[1][i]);
> +		} else {
> +			return -EINVAL;
> +		}
> +
> +		chan->datasheet_name = channel_name[i];
> +		chan->extend_name = channel_name[i];
You are making use of extend_name in an unusual way.
1) This results in non standard ABI so should be documented under
Documentation/ABI/testing/sysfs-bus-iio*
2) The naming here makes me think you are abusing the abi and pushing two
channels through the same sysfs attribute.  Don't do that.  If you need
to clearly capture two channels together then implement the buffered
interface (chardev reading of 'scans' - groups of channels captured at
a similar time) rather than abusing the simpler sysfs route.
> +		chan->scan_index = i;
You aren't providing buffered access yet so don't bother filling in
scan_index or scan_type fo rhte channels (they are unusued).
> +		chan->scan_type.sign = 'u';
> +		chan->scan_type.realbits = ADC_BITS;
> +		chan->scan_type.storagebits = ADC_STORE_BITS;
> +		chan->scan_type.endianness = IIO_CPU;
> +		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	}
> +
> +	indio_dev->channels = chan_array;
Might as well just use indio_dev->channels directly rather than
having the local variable.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id alt_modular_adc_match[] = {
> +	{ .compatible = "altr,modular-adc-1.0" },
> +	{ .compatible = "altr,modular-dual-adc-1.0" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, alt_modular_adc_match);
> +
> +static int alt_modular_adc_parse_dt(struct iio_dev *indio_dev,
> +				struct device *dev)
> +{
> +	struct altera_adc *adc = iio_priv(indio_dev);
> +	struct device_node *np = dev->of_node;
> +	u32 value;
> +	int ret, i, j;
> +	char str[50];
> +
> +	ret = of_property_read_u32(np, "altr,adc-mode", &value);
> +	if (ret < 0)
> +		return ret;
> +	if (value == 0 || value > MAX_ADC) {
> +		dev_err(dev, "Invalid ADC mode value");
> +		return -EINVAL;
> +	}
> +	adc->mode = value;
> +
> +	ret = of_property_read_u32(np, "altr,adc-slot-count", &value);
> +	if (ret < 0)
> +		return ret;
> +	if (value == 0 || value > MAX_SLOT) {
> +		dev_err(dev, "Invalid ADC slot count value");
> +		return -EINVAL;
> +	}
> +	adc->slot_count = value;
> +
> +	ret = of_property_read_u32(np, "altr,adc-number", &value);
> +	if (ret < 0)
> +		return ret;
> +	if (value == 0 || value > MAX_ADC) {
> +		dev_err(dev, "Invalid ADC number value");
> +		return -EINVAL;
> +	}
> +	adc->adc_number = value;
> +
> +	/* Device tree lookup for channels for each memory slots */
> +	for (j = 0; j < adc->mode; j++) {
> +		for (i = 0; i < adc->slot_count; i++) {
> +			str[0] = '\0';
> +
> +			snprintf(str, sizeof(str),
> +				"altr,adc%d-slot-sequence-%d",
> +				(j + 1), (i + 1));
> +
> +			ret = of_property_read_u32(np, str, &value);
> +			if (ret < 0)
> +				return ret;
> +			if (value > MAX_CHANNEL) {
> +				dev_err(dev, "Invalid ADC channel value");
> +				return -EINVAL;
> +			}
> +			adc->slot_sequence[j][i] = value;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int alt_modular_adc_probe(struct platform_device *pdev)
> +{
> +	struct altera_adc *adc;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct iio_dev *indio_dev;
> +	struct resource *mem;
> +	int ret;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	indio_dev = iio_device_alloc(sizeof(struct altera_adc));
Convention has become sizeof(*adc) as it makes the connection slightly
more explicit.

> +	if (!indio_dev) {
> +		dev_err(&pdev->dev, "failed allocating iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	adc = iio_priv(indio_dev);
> +
> +	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +		"sequencer_csr");
> +	adc->seq_regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(adc->seq_regs)) {
> +		ret = PTR_ERR(adc->seq_regs);
> +		goto err_iio;
> +	}
> +
> +	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +		"sample_store_csr");
> +	adc->sample_regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(adc->sample_regs)) {
> +		ret = PTR_ERR(adc->sample_regs);
> +		goto err_iio;
> +	}
> +
> +	ret = alt_modular_adc_parse_dt(indio_dev, &pdev->dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to parse device tree\n");
> +		goto err_iio;
> +	}
> +
> +	ret = alt_modular_adc_channel_init(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed initialize ADC channels\n");
> +		goto err_iio;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->info = &adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = adc->slot_count;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto err_iio;
> +
> +	/* Disable Interrupt */
> +	writel_relaxed(0, (adc->sample_regs + ADC_IER_REG));
Given we don't seem to handle interrupts anywhere I'd expect the device
to have come up with this disabled, but if it doesn't, this definitely
belongs a lot earlier than here.
> +
> +	/* Start Continuous Sampling */
> +	writel_relaxed((ADC_RUN_MSK), (adc->seq_regs + ADC_CMD_REG));

I'm not sure it makes sense to do either of these last two steps
after the userspace interface is started by the iio_device_register
call.

> +
> +	return 0;
> +
Only 1 blank line here please (pretty much always considered enough
in kernel coding style).
> +
> +err_iio:
> +	iio_device_free(indio_dev);
> +	return ret;
> +}
> +
> +static int alt_modular_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct altera_adc *adc = iio_priv(indio_dev);
> +

If you don't unregister the iio_device first there is a clear
race condition with userspace requests hitting after you've
stopped the ADC but before the interfaces have been removed / marked
as going away by that unregister call.

> +	/* Stop ADC */
> +	writel((ADC_STOP_MSK), (adc->seq_regs + ADC_CMD_REG));

Still  an excess of brackets.  You don't need them around a lot
of these arguments.  We can reasonably assume they don't contain
any malicious elements that could lead to it doing other than the
obvious.
	writel(ADC_STOP_MSK, adc->seq_regs + ADC_CMD_REG);
will be fine (similar cases elsewhere in the driver).

> +
> +	/* Unregister ADC */
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver altr_modular_adc_driver = {
> +	.probe		= alt_modular_adc_probe,
> +	.remove		= alt_modular_adc_remove,
> +	.driver		= {
> +		.name	= "alt-modular-adc",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = alt_modular_adc_match,
> +	},
> +};
> +
> +
> +module_platform_driver(altr_modular_adc_driver);
> +
> +MODULE_DESCRIPTION("Altera Modular ADC Driver");
> +MODULE_AUTHOR("Chee Nouk Phoon <cnphoon@altera.com>");
> +MODULE_LICENSE("GPL v2");
>