[PATCH v2 0/3] iio: Add Cosmic Circuits ADC support

[PATCH v2 0/3] iio: Add Cosmic Circuits ADC support

[Ezequiel Garcia]

Hi,

Thanks everyone for the good feedback. I've tried to address all your comments
on this round but if you spot anything worth of mention (or I forgot
something), please speak out.

Regarding the invalid sample handling, I tried to keep things simple. This
should happen only in the event of hardware failure, so the driver spits a
warning and errors out, as suggested by Jonathan.

The CC10001 ADC IP block will be present on the Pistachio SoC. Upstream
support for this SoC is on the way and will be submitted soon.

As I mentioned in the commit log for the binding, the ADC channels are shared
between the Pistachio SoC and another processor (RPU). To specify which
channels are available to Linux, the binding includes a property
"cosmic,adc-available-channels".

Each CPU gets its own view of the ADC register set, but both act on the
same hardware block. Concurrency is handled by the hardware and is completely
transparent to the OS. This means that if both CPUs make a request
simultaneously, the conversion time would be twice the expected time.
 
The series is based on v3.18-rc2.

Changes from v1:

  * Removed unneeded header includes.

  * Changed all the names and macros prefix: s/CC_10001_/CC10001_.

  * Used .update_scan_mode callback to preallocate the buffer.

  * Used indio_dev for the struct iio_dev.

  * Only read the regulator voltage when needed.

  * Fixed probe() error handling.

  * Used for_each_set_bit() instead of open-coding it.

  * Name the power-down register as _POWER_UP, to make the code
    less silly.

  * Error out when no valid sample can be read (i.e. when end-of-conversion
    poll times out).

  * ... plus some assorted code cleaning based on the feedback.

Ezequiel Garcia (1):
  DT: Add a vendor prefix for Cosmic Circuits

Phani Movva (2):
  iio: adc: Cosmic Circuits 10001 ADC driver
  DT: iio: adc: Add CC_10001 binding documentation

 .../devicetree/bindings/iio/adc/cc_10001_adc.txt   |  19 +
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/cc_10001_adc.c                     | 425 +++++++++++++++++++++
 5 files changed, 456 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
 create mode 100644 drivers/iio/adc/cc_10001_adc.c

-- 
2.1.0

[PATCH v2 1/3] iio: adc: Cosmic Circuits 10001 ADC driver

[Ezequiel Garcia]

From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

This commit adds support for Cosmic Circuits 10001 10-bit ADC device.

Signed-off-by: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
[Ezequiel: code style cleaning]
Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/iio/adc/Kconfig        |  10 +
 drivers/iio/adc/Makefile       |   1 +
 drivers/iio/adc/cc_10001_adc.c | 425 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 436 insertions(+)
 create mode 100644 drivers/iio/adc/cc_10001_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 88bdc8f..b1f3ecc 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -127,6 +127,16 @@ config AT91_ADC
 	help
 	  Say yes here to build support for Atmel AT91 ADC.
 
+config CC_10001_ADC
+	tristate "Cosmic Circuits 10001 ADC driver"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for Cosmic Circuits 10001 ADC.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called cc_10001_adc.
+
 config EXYNOS_ADC
 	tristate "Exynos ADC driver support"
 	depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || (OF && COMPILE_TEST)
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index cb88a6a..f4d522d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
 obj-$(CONFIG_AD7887) += ad7887.o
 obj-$(CONFIG_AD799X) += ad799x.o
 obj-$(CONFIG_AT91_ADC) += at91_adc.o
+obj-$(CONFIG_CC_10001_ADC) += cc_10001_adc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_MAX1027) += max1027.o
diff --git a/drivers/iio/adc/cc_10001_adc.c b/drivers/iio/adc/cc_10001_adc.c
new file mode 100644
index 0000000..1720a62
--- /dev/null
+++ b/drivers/iio/adc/cc_10001_adc.c
@@ -0,0 +1,425 @@
+/*
+ * Copyright (c) 2014 Imagination Technologies Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/* Registers */
+#define CC10001_ADC_CONFIG		0x00
+#define CC10001_ADC_START_CONV		BIT(4)
+#define CC10001_ADC_MODE_SINGLE_CONV	BIT(5)
+
+#define CC10001_ADC_DDATA_OUT		0x04
+#define CC10001_ADC_EOC			0x08
+#define CC10001_ADC_EOC_SET		BIT(0)
+
+#define CC10001_ADC_CHSEL_SAMPLED	0x0c
+#define CC10001_ADC_POWER_UP		0x10
+#define CC10001_ADC_POWER_UP_SET	BIT(0)
+#define CC10001_ADC_DEBUG		0x14
+#define CC10001_ADC_DATA_COUNT		0x20
+
+#define CC10001_ADC_DATA_MASK		GENMASK(9, 0)
+#define CC10001_ADC_NUM_CHANNELS	8
+#define CC10001_ADC_CH_MASK		(CC10001_ADC_NUM_CHANNELS - 1)
+
+#define CC10001_INVALID_SAMPLED_OUTPUT	0xFFFF
+#define CC10001_MAX_POLL_COUNT		20
+
+/*
+ * As per device specification, wait six clock cycles after power-up to
+ * activate START. Since adding two more clock cycles delay does not
+ * impact the performance too much, we are adding two additional cycles delay
+ * intentionally here.
+ */
+#define	CC10001_WAIT_CYCLES		8
+
+struct cc10001_adc_device {
+	void __iomem *reg_base;
+	struct iio_dev *indio_dev;
+	struct clk *adc_clk;
+	struct regulator *reg;
+	u16 *buf;
+
+	struct mutex lock;
+	unsigned long channel_map;
+	unsigned int start_delay_ns;
+	unsigned int eoc_delay_ns;
+};
+
+static inline void cc_adc_write_reg(struct cc10001_adc_device *adc_dev,
+				    u32 reg, u32 val)
+{
+	writel(val, adc_dev->reg_base + reg);
+}
+
+static inline u32 cc_adc_read_reg(struct cc10001_adc_device *adc_dev,
+				  u32 reg)
+{
+	return readl(adc_dev->reg_base + reg);
+}
+
+static void cc_adc_start(struct cc10001_adc_device *adc_dev,
+			 unsigned int channel)
+{
+	u32 val;
+
+	/* Channel selection and mode of operation */
+	val = (channel & CC10001_ADC_CH_MASK) | CC10001_ADC_MODE_SINGLE_CONV;
+	cc_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
+
+	val = cc_adc_read_reg(adc_dev, CC10001_ADC_CONFIG);
+	val = val | CC10001_ADC_START_CONV;
+	cc_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
+}
+
+static u16 cc_adc_poll_done(struct iio_dev *indio_dev, unsigned int channel,
+			    unsigned int delay)
+{
+	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
+	unsigned int poll_count = 0;
+
+	while (!(cc_adc_read_reg(adc_dev, CC10001_ADC_EOC) &
+			CC10001_ADC_EOC_SET)) {
+
+		ndelay(delay);
+		if (poll_count++ == CC10001_MAX_POLL_COUNT)
+			return CC10001_INVALID_SAMPLED_OUTPUT;
+	}
+
+	poll_count = 0;
+	while ((cc_adc_read_reg(adc_dev, CC10001_ADC_CHSEL_SAMPLED) &
+			CC10001_ADC_CH_MASK) != channel) {
+
+		ndelay(delay);
+		if (poll_count++ == CC10001_MAX_POLL_COUNT)
+			return CC10001_INVALID_SAMPLED_OUTPUT;
+	}
+
+	/* Read the 10 bit output register */
+	return cc_adc_read_reg(adc_dev, CC10001_ADC_DDATA_OUT) &
+			       CC10001_ADC_DATA_MASK;
+}
+
+static irqreturn_t cc10001_adc_trigger_h(int irq, void *p)
+{
+	struct cc10001_adc_device *adc_dev;
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev;
+	unsigned int delay_ns;
+	unsigned int channel;
+	bool sample_invalid;
+	u16 *data;
+	int i;
+
+	indio_dev = pf->indio_dev;
+	adc_dev = iio_priv(indio_dev);
+	data = adc_dev->buf;
+
+	mutex_lock(&adc_dev->lock);
+
+	cc_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP,
+			 CC10001_ADC_POWER_UP_SET);
+
+	/* Wait for 8 (6+2) clock cycles before activating START */
+	ndelay(adc_dev->start_delay_ns);
+
+	/* Calculate delay step for eoc and sampled data */
+	delay_ns = adc_dev->eoc_delay_ns / CC10001_MAX_POLL_COUNT;
+
+	i = 0;
+	sample_invalid = false;
+	for_each_set_bit(channel, indio_dev->active_scan_mask,
+				  indio_dev->masklength) {
+
+		cc_adc_start(adc_dev, channel);
+
+		data[i] = cc_adc_poll_done(indio_dev, channel, delay_ns);
+		if (data[i] == CC10001_INVALID_SAMPLED_OUTPUT) {
+			dev_warn(&indio_dev->dev,
+				 "invalid sample on channel %d\n", channel);
+			sample_invalid = true;
+			goto done;
+		}
+		i++;
+	}
+
+done:
+	cc_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP, 0);
+
+	mutex_unlock(&adc_dev->lock);
+
+	if (!sample_invalid)
+		iio_push_to_buffers_with_timestamp(indio_dev, data,
+						   iio_get_time_ns());
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static u16 cc10001_adc_read_raw_voltage(struct iio_dev *indio_dev,
+					 struct iio_chan_spec const *chan)
+{
+	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
+	unsigned int delay_ns;
+	u16 val;
+
+	cc_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP,
+			 CC10001_ADC_POWER_UP_SET);
+
+	/* Wait for 8 (6+2) clock cycles before activating START */
+	ndelay(adc_dev->start_delay_ns);
+
+	/* Calculate delay step for eoc and sampled data */
+	delay_ns = adc_dev->eoc_delay_ns / CC10001_MAX_POLL_COUNT;
+
+	cc_adc_start(adc_dev, chan->channel);
+
+	val = cc_adc_poll_done(indio_dev, chan->channel, delay_ns);
+
+	cc_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP, 0);
+
+	return val;
+}
+
+static int cc10001_adc_read_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int *val, int *val2, long mask)
+{
+	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (iio_buffer_enabled(indio_dev))
+			return -EBUSY;
+		mutex_lock(&adc_dev->lock);
+		*val = cc10001_adc_read_raw_voltage(indio_dev, chan);
+		mutex_unlock(&adc_dev->lock);
+
+		if (*val == CC10001_INVALID_SAMPLED_OUTPUT)
+			return -EIO;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regulator_get_voltage(adc_dev->reg);
+		if (ret)
+			return ret;
+
+		*val = ret / 1000;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int cc10001_update_scan_mode(struct iio_dev *indio_dev,
+				     const unsigned long *scan_mask)
+{
+	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
+
+	kfree(adc_dev->buf);
+	adc_dev->buf = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
+	if (!adc_dev->buf)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static const struct iio_info cc10001_adc_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = &cc10001_adc_read_raw,
+	.update_scan_mode = &cc10001_update_scan_mode,
+};
+
+static int cc10001_adc_channel_init(struct iio_dev *indio_dev)
+{
+	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
+	struct iio_chan_spec *chan_array, *timestamp;
+	unsigned int bit, idx = 0;
+
+	indio_dev->num_channels = bitmap_weight(&adc_dev->channel_map,
+						CC10001_ADC_NUM_CHANNELS);
+
+	chan_array = devm_kcalloc(&indio_dev->dev, indio_dev->num_channels + 1,
+				  sizeof(struct iio_chan_spec),
+				  GFP_KERNEL);
+	if (!chan_array)
+		return -ENOMEM;
+
+	for_each_set_bit(bit, &adc_dev->channel_map,
+			 CC10001_ADC_NUM_CHANNELS) {
+		struct iio_chan_spec *chan = &chan_array[idx];
+
+		chan->type = IIO_VOLTAGE;
+		chan->indexed = 1;
+		chan->channel = bit;
+		chan->scan_index = idx;
+		chan->scan_type.sign = 'u';
+		chan->scan_type.realbits = 10;
+		chan->scan_type.storagebits = 16;
+		chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
+		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+		idx++;
+	}
+
+	timestamp = &chan_array[idx];
+	timestamp->type = IIO_TIMESTAMP;
+	timestamp->channel = -1;
+	timestamp->scan_index = idx;
+	timestamp->scan_type.sign = 's';
+	timestamp->scan_type.realbits = 64;
+	timestamp->scan_type.storagebits = 64;
+
+	indio_dev->channels = chan_array;
+
+	return 0;
+}
+
+static int cc10001_adc_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct cc10001_adc_device *adc_dev;
+	unsigned long adc_clk_rate;
+	struct resource *res;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	adc_dev = iio_priv(indio_dev);
+
+	if (of_property_read_u32(node, "cosmic,adc-available-channels", &ret)) {
+		dev_err(&indio_dev->dev, "Missing adc-available-channels property\n");
+		return -EINVAL;
+	}
+	adc_dev->channel_map = ret;
+
+	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
+	if (IS_ERR_OR_NULL(adc_dev->reg))
+		return -EINVAL;
+
+	ret = regulator_enable(adc_dev->reg);
+	if (ret)
+		return ret;
+
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->info = &cc10001_adc_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	adc_dev->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(adc_dev->reg_base))
+		goto err_disable_reg;
+
+	adc_dev->adc_clk = devm_clk_get(&pdev->dev, "adc");
+	if (IS_ERR(adc_dev->adc_clk)) {
+		dev_err(&pdev->dev, "Failed to get the clock\n");
+		goto err_disable_reg;
+	}
+
+	ret = clk_prepare_enable(adc_dev->adc_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to enable the clock\n");
+		goto err_disable_reg;
+	}
+
+	adc_clk_rate = clk_get_rate(adc_dev->adc_clk);
+	if (!adc_clk_rate) {
+		ret = -EINVAL;
+		dev_err(&pdev->dev, "null clock rate!\n");
+		goto err_disable_clk;
+	}
+
+	adc_dev->eoc_delay_ns = NSEC_PER_SEC / adc_clk_rate;
+	adc_dev->start_delay_ns = adc_dev->eoc_delay_ns * CC10001_WAIT_CYCLES;
+
+	/* Setup the ADC channels available on the device */
+	ret = cc10001_adc_channel_init(indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not initialize the channels.\n");
+		goto err_disable_clk;
+	}
+
+	mutex_init(&adc_dev->lock);
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 &cc10001_adc_trigger_h, NULL);
+	if (ret < 0)
+		goto err_disable_clk;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0)
+		goto err_cleanup_buffer;
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	return 0;
+
+err_cleanup_buffer:
+	iio_triggered_buffer_cleanup(indio_dev);
+err_disable_clk:
+	clk_disable_unprepare(adc_dev->adc_clk);
+err_disable_reg:
+	regulator_disable(adc_dev->reg);
+	return ret;
+}
+
+static int cc10001_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+	clk_disable_unprepare(adc_dev->adc_clk);
+	regulator_disable(adc_dev->reg);
+
+	return 0;
+}
+
+static const struct of_device_id cc10001_adc_dt_ids[] = {
+	{ .compatible = "cosmic,10001-adc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cc10001_adc_dt_ids);
+
+static struct platform_driver cc10001_adc_driver = {
+	.driver = {
+		.name   = "cc-10001-adc",
+		.owner	= THIS_MODULE,
+		.of_match_table = cc10001_adc_dt_ids,
+	},
+	.probe	= cc10001_adc_probe,
+	.remove	= cc10001_adc_remove,
+};
+module_platform_driver(cc10001_adc_driver);
+
+MODULE_AUTHOR("Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("Cosmic Circuits ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

[PATCH v2 2/3] DT: iio: adc: Add CC_10001 binding documentation

[Ezequiel Garcia]

From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

Add the devicetree binding document for Cosmic Circuits 10001 ADC device.

Signed-off-by: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
[Ezequiel: minor style cleaning]
Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/iio/adc/cc_10001_adc.txt      | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
new file mode 100644
index 0000000..6491839
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
@@ -0,0 +1,19 @@
+* Cosmic Circuits - Analog to Digital Converter (CC-10001-ADC)
+
+Required properties:
+  - compatible: Should be "cosmic,10001-adc"
+  - reg: Should contain adc registers location and length.
+  - cosmic,adc-available-channels: Bitmask of the channels currently enabled.
+  - clock-names: Should contain "adc".
+  - clocks: phandles to input clocks.
+  - vref-supply: The regulator supply ADC reference voltage.
+
+Example:
+adc: adc@18101600 {
+	compatible = "cosmic,10001-adc";
+	reg = <0x18101600 0x24>;
+	cosmic,adc-available-channels = <0xa>;
+	clocks = <&adc_clk>;
+	clock-names = "adc";
+	vref-supply = <&reg_1v8>;
+};
-- 
2.1.0

[PATCH v2 3/3] DT: Add a vendor prefix for Cosmic Circuits

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 723999d..1bfa13a 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -35,6 +35,7 @@ chrp	Common Hardware Reference Platform
 chunghwa	Chunghwa Picture Tubes Ltd.
 cirrus	Cirrus Logic, Inc.
 cortina	Cortina Systems, Inc.
+cosmic	Cosmic Circuits
 crystalfontz	Crystalfontz America, Inc.
 dallas	Maxim Integrated Products (formerly Dallas Semiconductor)
 davicom	DAVICOM Semiconductor, Inc.
-- 
2.1.0

Re: [PATCH v2 1/3] iio: adc: Cosmic Circuits 10001 ADC driver

[Jonathan Cameron]

On 05/11/14 21:56, Ezequiel Garcia wrote:
> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> 
> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
> 
> Signed-off-by: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> [Ezequiel: code style cleaning]
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Looks good to me.  Some utter nitpicks.  I'd have let them go
but would like this to sit on the list to give others time for
a second review.

Jonathan
> ---
>  drivers/iio/adc/Kconfig        |  10 +
>  drivers/iio/adc/Makefile       |   1 +
>  drivers/iio/adc/cc_10001_adc.c | 425 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 436 insertions(+)
>  create mode 100644 drivers/iio/adc/cc_10001_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 88bdc8f..b1f3ecc 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -127,6 +127,16 @@ config AT91_ADC
>  	help
>  	  Say yes here to build support for Atmel AT91 ADC.
>  
> +config CC_10001_ADC
hmm. how fixed are you on th CC_ it is 'unusual' compared to normal
part naming in kernel.  However, I understand if you have
existing code out there using this convention and don't care that
much!
> +	tristate "Cosmic Circuits 10001 ADC driver"
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for Cosmic Circuits 10001 ADC.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called cc_10001_adc.
> +
>  config EXYNOS_ADC
>  	tristate "Exynos ADC driver support"
>  	depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || (OF && COMPILE_TEST)
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index cb88a6a..f4d522d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
>  obj-$(CONFIG_AD799X) += ad799x.o
>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
> +obj-$(CONFIG_CC_10001_ADC) += cc_10001_adc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_MAX1027) += max1027.o
> diff --git a/drivers/iio/adc/cc_10001_adc.c b/drivers/iio/adc/cc_10001_adc.c
> new file mode 100644
> index 0000000..1720a62
> --- /dev/null
> +++ b/drivers/iio/adc/cc_10001_adc.c
> @@ -0,0 +1,425 @@
> +/*
> + * Copyright (c) 2014 Imagination Technologies Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +/* Registers */
> +#define CC10001_ADC_CONFIG		0x00
> +#define CC10001_ADC_START_CONV		BIT(4)
> +#define CC10001_ADC_MODE_SINGLE_CONV	BIT(5)
> +
> +#define CC10001_ADC_DDATA_OUT		0x04
> +#define CC10001_ADC_EOC			0x08
> +#define CC10001_ADC_EOC_SET		BIT(0)
> +
> +#define CC10001_ADC_CHSEL_SAMPLED	0x0c
> +#define CC10001_ADC_POWER_UP		0x10
> +#define CC10001_ADC_POWER_UP_SET	BIT(0)
> +#define CC10001_ADC_DEBUG		0x14
> +#define CC10001_ADC_DATA_COUNT		0x20
> +
> +#define CC10001_ADC_DATA_MASK		GENMASK(9, 0)
> +#define CC10001_ADC_NUM_CHANNELS	8
> +#define CC10001_ADC_CH_MASK		(CC10001_ADC_NUM_CHANNELS - 1)
> +
> +#define CC10001_INVALID_SAMPLED_OUTPUT	0xFFFF
> +#define CC10001_MAX_POLL_COUNT		20
> +
> +/*
> + * As per device specification, wait six clock cycles after power-up to
> + * activate START. Since adding two more clock cycles delay does not
> + * impact the performance too much, we are adding two additional cycles delay
> + * intentionally here.
> + */
> +#define	CC10001_WAIT_CYCLES		8
> +
> +struct cc10001_adc_device {
> +	void __iomem *reg_base;
> +	struct iio_dev *indio_dev;
> +	struct clk *adc_clk;
> +	struct regulator *reg;
> +	u16 *buf;
> +
> +	struct mutex lock;
> +	unsigned long channel_map;
> +	unsigned int start_delay_ns;
> +	unsigned int eoc_delay_ns;
> +};
> +
> +static inline void cc_adc_write_reg(struct cc10001_adc_device *adc_dev,
> +				    u32 reg, u32 val)
> +{
> +	writel(val, adc_dev->reg_base + reg);
> +}
> +
> +static inline u32 cc_adc_read_reg(struct cc10001_adc_device *adc_dev,
> +				  u32 reg)
> +{
> +	return readl(adc_dev->reg_base + reg);
> +}
> +
> +static void cc_adc_start(struct cc10001_adc_device *adc_dev,
> +			 unsigned int channel)
> +{
> +	u32 val;
> +
> +	/* Channel selection and mode of operation */
> +	val = (channel & CC10001_ADC_CH_MASK) | CC10001_ADC_MODE_SINGLE_CONV;
> +	cc_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
> +
> +	val = cc_adc_read_reg(adc_dev, CC10001_ADC_CONFIG);
> +	val = val | CC10001_ADC_START_CONV;
> +	cc_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
> +}
> +
cc10001_adc_poll etc.  Consistency is good with funciton prefixing...

> +static u16 cc_adc_poll_done(struct iio_dev *indio_dev, unsigned int channel,
> +			    unsigned int delay)
> +{
> +	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
> +	unsigned int poll_count = 0;
> +
> +	while (!(cc_adc_read_reg(adc_dev, CC10001_ADC_EOC) &
> +			CC10001_ADC_EOC_SET)) {
> +
> +		ndelay(delay);
> +		if (poll_count++ == CC10001_MAX_POLL_COUNT)
> +			return CC10001_INVALID_SAMPLED_OUTPUT;
> +	}
> +
> +	poll_count = 0;
> +	while ((cc_adc_read_reg(adc_dev, CC10001_ADC_CHSEL_SAMPLED) &
> +			CC10001_ADC_CH_MASK) != channel) {
> +
> +		ndelay(delay);
> +		if (poll_count++ == CC10001_MAX_POLL_COUNT)
> +			return CC10001_INVALID_SAMPLED_OUTPUT;
> +	}
> +
> +	/* Read the 10 bit output register */
> +	return cc_adc_read_reg(adc_dev, CC10001_ADC_DDATA_OUT) &
> +			       CC10001_ADC_DATA_MASK;
> +}
> +
> +static irqreturn_t cc10001_adc_trigger_h(int irq, void *p)
> +{
> +	struct cc10001_adc_device *adc_dev;
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev;
> +	unsigned int delay_ns;
> +	unsigned int channel;
> +	bool sample_invalid;
> +	u16 *data;
> +	int i;
> +
> +	indio_dev = pf->indio_dev;
> +	adc_dev = iio_priv(indio_dev);
> +	data = adc_dev->buf;
> +
> +	mutex_lock(&adc_dev->lock);
> +
> +	cc_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP,
> +			 CC10001_ADC_POWER_UP_SET);
> +
> +	/* Wait for 8 (6+2) clock cycles before activating START */
> +	ndelay(adc_dev->start_delay_ns);
> +
> +	/* Calculate delay step for eoc and sampled data */
> +	delay_ns = adc_dev->eoc_delay_ns / CC10001_MAX_POLL_COUNT;
> +
> +	i = 0;
> +	sample_invalid = false;
> +	for_each_set_bit(channel, indio_dev->active_scan_mask,
> +				  indio_dev->masklength) {
> +
> +		cc_adc_start(adc_dev, channel);
> +
> +		data[i] = cc_adc_poll_done(indio_dev, channel, delay_ns);
> +		if (data[i] == CC10001_INVALID_SAMPLED_OUTPUT) {
> +			dev_warn(&indio_dev->dev,
> +				 "invalid sample on channel %d\n", channel);
> +			sample_invalid = true;
> +			goto done;
> +		}
> +		i++;
> +	}
> +
> +done:
> +	cc_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP, 0);
> +
> +	mutex_unlock(&adc_dev->lock);
> +
> +	if (!sample_invalid)
> +		iio_push_to_buffers_with_timestamp(indio_dev, data,
> +						   iio_get_time_ns());
> +	iio_trigger_notify_done(indio_dev->trig);
blank line here.
> +	return IRQ_HANDLED;
> +}
> +
> +static u16 cc10001_adc_read_raw_voltage(struct iio_dev *indio_dev,
> +					 struct iio_chan_spec const *chan)
> +{
> +	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
> +	unsigned int delay_ns;
> +	u16 val;
> +
> +	cc_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP,
> +			 CC10001_ADC_POWER_UP_SET);
> +
> +	/* Wait for 8 (6+2) clock cycles before activating START */
> +	ndelay(adc_dev->start_delay_ns);
> +
> +	/* Calculate delay step for eoc and sampled data */
> +	delay_ns = adc_dev->eoc_delay_ns / CC10001_MAX_POLL_COUNT;
> +
> +	cc_adc_start(adc_dev, chan->channel);
> +
> +	val = cc_adc_poll_done(indio_dev, chan->channel, delay_ns);
> +
> +	cc_adc_write_reg(adc_dev, CC10001_ADC_POWER_UP, 0);
> +
> +	return val;
> +}
> +
> +static int cc10001_adc_read_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan,
> +				 int *val, int *val2, long mask)
> +{
> +	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (iio_buffer_enabled(indio_dev))
> +			return -EBUSY;
> +		mutex_lock(&adc_dev->lock);
> +		*val = cc10001_adc_read_raw_voltage(indio_dev, chan);
> +		mutex_unlock(&adc_dev->lock);
> +
> +		if (*val == CC10001_INVALID_SAMPLED_OUTPUT)
> +			return -EIO;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regulator_get_voltage(adc_dev->reg);
> +		if (ret)
> +			return ret;
> +
> +		*val = ret / 1000;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int cc10001_update_scan_mode(struct iio_dev *indio_dev,
> +				     const unsigned long *scan_mask)
> +{
> +	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
> +
> +	kfree(adc_dev->buf);
> +	adc_dev->buf = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> +	if (!adc_dev->buf)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static const struct iio_info cc10001_adc_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = &cc10001_adc_read_raw,
> +	.update_scan_mode = &cc10001_update_scan_mode,
> +};
> +
> +static int cc10001_adc_channel_init(struct iio_dev *indio_dev)
> +{
> +	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
> +	struct iio_chan_spec *chan_array, *timestamp;
> +	unsigned int bit, idx = 0;
> +
> +	indio_dev->num_channels = bitmap_weight(&adc_dev->channel_map,
> +						CC10001_ADC_NUM_CHANNELS);
> +
> +	chan_array = devm_kcalloc(&indio_dev->dev, indio_dev->num_channels + 1,
> +				  sizeof(struct iio_chan_spec),
> +				  GFP_KERNEL);
> +	if (!chan_array)
> +		return -ENOMEM;
> +
> +	for_each_set_bit(bit, &adc_dev->channel_map,
> +			 CC10001_ADC_NUM_CHANNELS) {
> +		struct iio_chan_spec *chan = &chan_array[idx];
> +
> +		chan->type = IIO_VOLTAGE;
> +		chan->indexed = 1;
> +		chan->channel = bit;
> +		chan->scan_index = idx;
> +		chan->scan_type.sign = 'u';
> +		chan->scan_type.realbits = 10;
> +		chan->scan_type.storagebits = 16;
> +		chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> +		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +		idx++;
> +	}
> +
> +	timestamp = &chan_array[idx];
> +	timestamp->type = IIO_TIMESTAMP;
> +	timestamp->channel = -1;
> +	timestamp->scan_index = idx;
> +	timestamp->scan_type.sign = 's';
> +	timestamp->scan_type.realbits = 64;
> +	timestamp->scan_type.storagebits = 64;
> +
> +	indio_dev->channels = chan_array;
> +
> +	return 0;
> +}
> +
> +static int cc10001_adc_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct cc10001_adc_device *adc_dev;
> +	unsigned long adc_clk_rate;
> +	struct resource *res;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	adc_dev = iio_priv(indio_dev);
> +
> +	if (of_property_read_u32(node, "cosmic,adc-available-channels", &ret)) {
> +		dev_err(&indio_dev->dev, "Missing adc-available-channels property\n");
> +		return -EINVAL;
> +	}
> +	adc_dev->channel_map = ret;
> +
> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
> +	if (IS_ERR_OR_NULL(adc_dev->reg))
> +		return -EINVAL;
> +
> +	ret = regulator_enable(adc_dev->reg);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->info = &cc10001_adc_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	adc_dev->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(adc_dev->reg_base))
> +		goto err_disable_reg;
> +
> +	adc_dev->adc_clk = devm_clk_get(&pdev->dev, "adc");
> +	if (IS_ERR(adc_dev->adc_clk)) {
> +		dev_err(&pdev->dev, "Failed to get the clock\n");
> +		goto err_disable_reg;
> +	}
> +
> +	ret = clk_prepare_enable(adc_dev->adc_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to enable the clock\n");
> +		goto err_disable_reg;
> +	}
> +
> +	adc_clk_rate = clk_get_rate(adc_dev->adc_clk);
> +	if (!adc_clk_rate) {
> +		ret = -EINVAL;
> +		dev_err(&pdev->dev, "null clock rate!\n");
> +		goto err_disable_clk;
> +	}
> +
> +	adc_dev->eoc_delay_ns = NSEC_PER_SEC / adc_clk_rate;
> +	adc_dev->start_delay_ns = adc_dev->eoc_delay_ns * CC10001_WAIT_CYCLES;
> +
> +	/* Setup the ADC channels available on the device */
> +	ret = cc10001_adc_channel_init(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Could not initialize the channels.\n");
> +		goto err_disable_clk;
> +	}
> +
> +	mutex_init(&adc_dev->lock);
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					 &cc10001_adc_trigger_h, NULL);
> +	if (ret < 0)
> +		goto err_disable_clk;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0)
> +		goto err_cleanup_buffer;
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	return 0;
> +
> +err_cleanup_buffer:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +err_disable_clk:
> +	clk_disable_unprepare(adc_dev->adc_clk);
> +err_disable_reg:
> +	regulator_disable(adc_dev->reg);
> +	return ret;
> +}
> +
> +static int cc10001_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct cc10001_adc_device *adc_dev = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +	clk_disable_unprepare(adc_dev->adc_clk);
> +	regulator_disable(adc_dev->reg);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id cc10001_adc_dt_ids[] = {
> +	{ .compatible = "cosmic,10001-adc", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, cc10001_adc_dt_ids);
> +
> +static struct platform_driver cc10001_adc_driver = {
> +	.driver = {
> +		.name   = "cc-10001-adc",
cc10001-adc?
> +		.owner	= THIS_MODULE,
> +		.of_match_table = cc10001_adc_dt_ids,
> +	},
> +	.probe	= cc10001_adc_probe,
> +	.remove	= cc10001_adc_remove,
> +};
> +module_platform_driver(cc10001_adc_driver);
> +
> +MODULE_AUTHOR("Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Cosmic Circuits ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v2 2/3] DT: iio: adc: Add CC_10001 binding documentation

[Jonathan Cameron]

On 05/11/14 21:56, Ezequiel Garcia wrote:
> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> 
> Add the devicetree binding document for Cosmic Circuits 10001 ADC device.
> 
> Signed-off-by: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> [Ezequiel: minor style cleaning]
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Please resend including the device tree list and maintainers.
All bindings must go to them whatever part they are for.


> ---
>  .../devicetree/bindings/iio/adc/cc_10001_adc.txt      | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
> new file mode 100644
> index 0000000..6491839
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
> @@ -0,0 +1,19 @@
> +* Cosmic Circuits - Analog to Digital Converter (CC-10001-ADC)
> +
> +Required properties:
> +  - compatible: Should be "cosmic,10001-adc"
> +  - reg: Should contain adc registers location and length.
> +  - cosmic,adc-available-channels: Bitmask of the channels currently enabled.
I suspect there may be other preferred ways of doing this (child nodes perhaps
for each enabled channel?)

> +  - clock-names: Should contain "adc".
> +  - clocks: phandles to input clocks.
> +  - vref-supply: The regulator supply ADC reference voltage.
> +
> +Example:
> +adc: adc@18101600 {
> +	compatible = "cosmic,10001-adc";
> +	reg = <0x18101600 0x24>;
> +	cosmic,adc-available-channels = <0xa>;
> +	clocks = <&adc_clk>;
> +	clock-names = "adc";
> +	vref-supply = <&reg_1v8>;
> +};
> 

Re: [PATCH v2 2/3] DT: iio: adc: Add CC_10001 binding documentation

[Jonathan Cameron]

On 05/11/14 21:56, Ezequiel Garcia wrote:
> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> 
> Add the devicetree binding document for Cosmic Circuits 10001 ADC device.
> 
> Signed-off-by: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> [Ezequiel: minor style cleaning]
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
I'm unobservant.  You did cc'd the list for device tree - convention is to
also cc the listed maintainers.. (done on this reply).
> ---
>  .../devicetree/bindings/iio/adc/cc_10001_adc.txt      | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
> new file mode 100644
> index 0000000..6491839
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
> @@ -0,0 +1,19 @@
> +* Cosmic Circuits - Analog to Digital Converter (CC-10001-ADC)
> +
> +Required properties:
> +  - compatible: Should be "cosmic,10001-adc"
> +  - reg: Should contain adc registers location and length.
> +  - cosmic,adc-available-channels: Bitmask of the channels currently enabled.
> +  - clock-names: Should contain "adc".
> +  - clocks: phandles to input clocks.
> +  - vref-supply: The regulator supply ADC reference voltage.
> +
> +Example:
> +adc: adc@18101600 {
> +	compatible = "cosmic,10001-adc";
> +	reg = <0x18101600 0x24>;
> +	cosmic,adc-available-channels = <0xa>;
> +	clocks = <&adc_clk>;
> +	clock-names = "adc";
> +	vref-supply = <&reg_1v8>;
> +};
> 

Re: [PATCH v2 2/3] DT: iio: adc: Add CC_10001 binding documentation

[Ezequiel Garcia]

On 11/08/2014 09:57 AM, Jonathan Cameron wrote:
> On 05/11/14 21:56, Ezequiel Garcia wrote:
>> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>
>> Add the devicetree binding document for Cosmic Circuits 10001 ADC device.
>>
>> Signed-off-by: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> [Ezequiel: minor style cleaning]
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Please resend including the device tree list and maintainers.
> All bindings must go to them whatever part they are for.
> 

Right, sorry about that.

> 
>> ---
>>  .../devicetree/bindings/iio/adc/cc_10001_adc.txt      | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
>> new file mode 100644
>> index 0000000..6491839
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
>> @@ -0,0 +1,19 @@
>> +* Cosmic Circuits - Analog to Digital Converter (CC-10001-ADC)
>> +
>> +Required properties:
>> +  - compatible: Should be "cosmic,10001-adc"
>> +  - reg: Should contain adc registers location and length.
>> +  - cosmic,adc-available-channels: Bitmask of the channels currently enabled.
> I suspect there may be other preferred ways of doing this (child nodes perhaps
> for each enabled channel?)

Hm, I'm not sure about this. As far I can recall, if a node has children
then it *must* be a bus, which is not the case. Of course, you'll find
examples doing this wrong.

Maybe some of the devicetree maintainers can clarify this point?

-- 
Ezequiel

Re: [PATCH v2 1/3] iio: adc: Cosmic Circuits 10001 ADC driver

[Ezequiel Garcia]

On 11/08/2014 09:55 AM, Jonathan Cameron wrote:
> On 05/11/14 21:56, Ezequiel Garcia wrote:
>> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>
>> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
>>
>> Signed-off-by: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Naidu Tellapati <Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> [Ezequiel: code style cleaning]
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Looks good to me.  Some utter nitpicks.  I'd have let them go
> but would like this to sit on the list to give others time for
> a second review.
> 

Done, all nitpicks fixed. I'll send v3 now.

Thanks,
-- 
Ezequiel

Re: [PATCH v2 2/3] DT: iio: adc: Add CC_10001 binding documentation

[Ezequiel Garcia]

On 11/10/2014 04:50 PM, Ezequiel Garcia wrote:
> On 11/08/2014 09:57 AM, Jonathan Cameron wrote:
>> On 05/11/14 21:56, Ezequiel Garcia wrote:

>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
>>> new file mode 100644
>>> index 0000000..6491839
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
>>> @@ -0,0 +1,19 @@
>>> +* Cosmic Circuits - Analog to Digital Converter (CC-10001-ADC)
>>> +
>>> +Required properties:
>>> +  - compatible: Should be "cosmic,10001-adc"
>>> +  - reg: Should contain adc registers location and length.
>>> +  - cosmic,adc-available-channels: Bitmask of the channels currently enabled.
>> I suspect there may be other preferred ways of doing this (child nodes perhaps
>> for each enabled channel?)
> 
> Hm, I'm not sure about this. As far I can recall, if a node has children
> then it *must* be a bus, which is not the case. Of course, you'll find
> examples doing this wrong.
> 

On a second thought... I guess you meant something like this?

adc: adc@18101600 {
        compatible = "cosmic,10001-adc";
        reg = <0x18101600 0x24>;
        clocks = <&adc_clk>;
        clock-names = "adc";
        vref-supply = <&reg_1v8>;
        cosmic,channels {
                #address-cells = <1>;
                #size-cells = <0>;
                channel@0 {
                        reg = <0>;
                };
                channel@1 {
                        reg = <1>;
                };
        };
};

Might be better, but I don't have a strong opinion on it.

-- 
Ezequiel

Re: [PATCH v2 2/3] DT: iio: adc: Add CC_10001 binding documentation

[Andrew Bresticker]

Ezequiel,

On Mon, Nov 10, 2014 at 11:59 AM, Ezequiel Garcia
<ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> On 11/10/2014 04:50 PM, Ezequiel Garcia wrote:
>> On 11/08/2014 09:57 AM, Jonathan Cameron wrote:
>>> On 05/11/14 21:56, Ezequiel Garcia wrote:
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
>>>> new file mode 100644
>>>> index 0000000..6491839
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
>>>> @@ -0,0 +1,19 @@
>>>> +* Cosmic Circuits - Analog to Digital Converter (CC-10001-ADC)
>>>> +
>>>> +Required properties:
>>>> +  - compatible: Should be "cosmic,10001-adc"
>>>> +  - reg: Should contain adc registers location and length.
>>>> +  - cosmic,adc-available-channels: Bitmask of the channels currently enabled.
>>> I suspect there may be other preferred ways of doing this (child nodes perhaps
>>> for each enabled channel?)
>>
>> Hm, I'm not sure about this. As far I can recall, if a node has children
>> then it *must* be a bus, which is not the case. Of course, you'll find
>> examples doing this wrong.
>>
>
> On a second thought... I guess you meant something like this?
>
> adc: adc@18101600 {
>         compatible = "cosmic,10001-adc";
>         reg = <0x18101600 0x24>;
>         clocks = <&adc_clk>;
>         clock-names = "adc";
>         vref-supply = <&reg_1v8>;
>         cosmic,channels {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 channel@0 {
>                         reg = <0>;
>                 };
>                 channel@1 {
>                         reg = <1>;
>                 };
>         };
> };
>
> Might be better, but I don't have a strong opinion on it.

I actually don't think this is any better.  The point of the property
is to indicate which channels are available for use by the main CPU,
i.e. channels not used by the RPU co-processor.  The property is not
used to indicate the presence of certain channels in hardware, in
which case maybe a bus would be appropriate.

Also, perhaps it would be better to reverse the sense of the property
(maybe, "cosmic,reserved-adc-channels") and make it optional?  I did
something similar for the MIPS GIC binding:
http://patchwork.linux-mips.org/patch/8268/.

Re: [PATCH v2 2/3] DT: iio: adc: Add CC_10001 binding documentation

[Ezequiel Garcia]

On 11/10/2014 06:28 PM, Andrew Bresticker wrote:
> Ezequiel,
> 
> On Mon, Nov 10, 2014 at 11:59 AM, Ezequiel Garcia
> <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>> On 11/10/2014 04:50 PM, Ezequiel Garcia wrote:
>>> On 11/08/2014 09:57 AM, Jonathan Cameron wrote:
>>>> On 05/11/14 21:56, Ezequiel Garcia wrote:
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
>>>>> new file mode 100644
>>>>> index 0000000..6491839
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/cc_10001_adc.txt
>>>>> @@ -0,0 +1,19 @@
>>>>> +* Cosmic Circuits - Analog to Digital Converter (CC-10001-ADC)
>>>>> +
>>>>> +Required properties:
>>>>> +  - compatible: Should be "cosmic,10001-adc"
>>>>> +  - reg: Should contain adc registers location and length.
>>>>> +  - cosmic,adc-available-channels: Bitmask of the channels currently enabled.
>>>> I suspect there may be other preferred ways of doing this (child nodes perhaps
>>>> for each enabled channel?)
>>>
>>> Hm, I'm not sure about this. As far I can recall, if a node has children
>>> then it *must* be a bus, which is not the case. Of course, you'll find
>>> examples doing this wrong.
>>>
>>
>> On a second thought... I guess you meant something like this?
>>
>> adc: adc@18101600 {
>>         compatible = "cosmic,10001-adc";
>>         reg = <0x18101600 0x24>;
>>         clocks = <&adc_clk>;
>>         clock-names = "adc";
>>         vref-supply = <&reg_1v8>;
>>         cosmic,channels {
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>                 channel@0 {
>>                         reg = <0>;
>>                 };
>>                 channel@1 {
>>                         reg = <1>;
>>                 };
>>         };
>> };
>>
>> Might be better, but I don't have a strong opinion on it.
> 
> I actually don't think this is any better.  The point of the property
> is to indicate which channels are available for use by the main CPU,
> i.e. channels not used by the RPU co-processor.  The property is not
> used to indicate the presence of certain channels in hardware, in
> which case maybe a bus would be appropriate.
> 
> Also, perhaps it would be better to reverse the sense of the property
> (maybe, "cosmic,reserved-adc-channels") and make it optional?  I did
> something similar for the MIPS GIC binding:
> http://patchwork.linux-mips.org/patch/8268/.
> 

I'm fine with that. If Jonathan agrees with it (or better, if we can
have an ack from a devicetree binding maintainer) I can spin a v3.

Thanks!
-- 
Ezequiel