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

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

[Ezequiel Garcia]

Changes from v4:

  * Added a compile-time dependency on REGULATOR and HAVE_CLK.

  * Replaced the silly XOR operation for a proper mask out of the
    available channels.

Changes from v3:

  * Fixed a few style nitpicks as per Hartmut's feedback.

  * Used GENMASK() to build the channel mask, which fixes a very nasty
    bug. Also found by Hartmut.

Changes from v2:

  * Changed a devicetree property from adc-available-channels to
    adc-reserved-channels, so it can be made optional.

  * Renamed the driver from cc_10001_xxx to cc10001_xxx so it's consistent
    with the rest of the kernel style.

  * Some more minor cosmetic fixes.

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/cc10001_adc.txt    |  22 ++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/iio/adc/Kconfig                            |  11 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/cc10001_adc.c                      | 424 +++++++++++++++++++++
 5 files changed, 459 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/cc10001_adc.txt
 create mode 100644 drivers/iio/adc/cc10001_adc.c

-- 
2.1.0

[PATCH v5 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       |  11 ++
 drivers/iio/adc/Makefile      |   1 +
 drivers/iio/adc/cc10001_adc.c | 424 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 436 insertions(+)
 create mode 100644 drivers/iio/adc/cc10001_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 88bdc8f..15bb1ae 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -127,6 +127,17 @@ config AT91_ADC
 	help
 	  Say yes here to build support for Atmel AT91 ADC.
 
+config CC10001_ADC
+	tristate "Cosmic Circuits 10001 ADC driver"
+	depends on HAS_IOMEM || HAVE_CLK || REGULATOR
+	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 cc10001_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..9286c59 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_CC10001_ADC) += cc10001_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/cc10001_adc.c b/drivers/iio/adc/cc10001_adc.c
new file mode 100644
index 0000000..a079ee0
--- /dev/null
+++ b/drivers/iio/adc/cc10001_adc.c
@@ -0,0 +1,424 @@
+/*
+ * 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		GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0)
+
+#define CC10001_INVALID_SAMPLED		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 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 cc10001_adc_write_reg(struct cc10001_adc_device *adc_dev,
+					 u32 reg, u32 val)
+{
+	writel(val, adc_dev->reg_base + reg);
+}
+
+static inline u32 cc10001_adc_read_reg(struct cc10001_adc_device *adc_dev,
+				       u32 reg)
+{
+	return readl(adc_dev->reg_base + reg);
+}
+
+static void cc10001_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;
+	cc10001_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
+
+	val = cc10001_adc_read_reg(adc_dev, CC10001_ADC_CONFIG);
+	val = val | CC10001_ADC_START_CONV;
+	cc10001_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
+}
+
+static u16 cc10001_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 (!(cc10001_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;
+	}
+
+	poll_count = 0;
+	while ((cc10001_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;
+	}
+
+	/* Read the 10 bit output register */
+	return cc10001_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);
+
+	cc10001_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) {
+
+		cc10001_adc_start(adc_dev, channel);
+
+		data[i] = cc10001_adc_poll_done(indio_dev, channel, delay_ns);
+		if (data[i] == CC10001_INVALID_SAMPLED) {
+			dev_warn(&indio_dev->dev,
+				 "invalid sample on channel %d\n", channel);
+			sample_invalid = true;
+			goto done;
+		}
+		i++;
+	}
+
+done:
+	cc10001_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;
+
+	cc10001_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;
+
+	cc10001_adc_start(adc_dev, chan->channel);
+
+	val = cc10001_adc_poll_done(indio_dev, chan->channel, delay_ns);
+
+	cc10001_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)
+			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);
+
+	adc_dev->channel_map = CC10001_ADC_CH_MASK;
+	if (!of_property_read_u32(node, "cosmic,reserved-adc-channels", &ret))
+		adc_dev->channel_map &= ~ret;
+
+	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
+	if (IS_ERR(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)) {
+		ret = PTR_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");
+		ret = PTR_ERR(adc_dev->adc_clk);
+		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)
+		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   = "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");
-- 
2.1.0

[PATCH v5 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/cc10001_adc.txt    | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/cc10001_adc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/cc10001_adc.txt b/Documentation/devicetree/bindings/iio/adc/cc10001_adc.txt
new file mode 100644
index 0000000..b7ba558
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/cc10001_adc.txt
@@ -0,0 +1,22 @@
+* 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.
+  - clock-names: Should contain "adc".
+  - clocks: Should contain a clock specifier for each entry in clock-names
+  - vref-supply: The regulator supply ADC reference voltage.
+
+Optional properties:
+  - cosmic,adc-reserved-channels: Bitmask of reserved channels,
+    i.e. channels that cannot be used by the OS.
+
+Example:
+adc: adc@18101600 {
+	compatible = "cosmic,10001-adc";
+	reg = <0x18101600 0x24>;
+	cosmic,adc-reserved-channels = <0x2>;
+	clocks = <&adc_clk>;
+	clock-names = "adc";
+	vref-supply = <&reg_1v8>;
+};
-- 
2.1.0

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

[Ezequiel Garcia]

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
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 v5 0/3] iio: Add Cosmic Circuits ADC support

[Ezequiel Garcia]

Hi Jonathan,

On 11/27/2014 12:39 PM, Ezequiel Garcia wrote:
> Changes from v4:
> 
>   * Added a compile-time dependency on REGULATOR and HAVE_CLK.
> 
>   * Replaced the silly XOR operation for a proper mask out of the
>     available channels.
> 
> Changes from v3:
> 
>   * Fixed a few style nitpicks as per Hartmut's feedback.
> 
>   * Used GENMASK() to build the channel mask, which fixes a very nasty
>     bug. Also found by Hartmut.
> 
> Changes from v2:
> 
>   * Changed a devicetree property from adc-available-channels to
>     adc-reserved-channels, so it can be made optional.
> 
>   * Renamed the driver from cc_10001_xxx to cc10001_xxx so it's consistent
>     with the rest of the kernel style.
> 
>   * Some more minor cosmetic fixes.
> 
> 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
> 

Unless there are any outstanding issues with this series, it'd be great
to merge it for v3.19.

Thanks!
-- 
Ezequiel

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

[Jonathan Cameron]

On December 5, 2014 11:55:40 AM GMT+00:00, Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>Hi Jonathan,
>
>On 11/27/2014 12:39 PM, Ezequiel Garcia wrote:
>> Changes from v4:
>> 
>>   * Added a compile-time dependency on REGULATOR and HAVE_CLK.
>> 
>>   * Replaced the silly XOR operation for a proper mask out of the
>>     available channels.
>> 
>> Changes from v3:
>> 
>>   * Fixed a few style nitpicks as per Hartmut's feedback.
>> 
>>   * Used GENMASK() to build the channel mask, which fixes a very
>nasty
>>     bug. Also found by Hartmut.
>> 
>> Changes from v2:
>> 
>>   * Changed a devicetree property from adc-available-channels to
>>     adc-reserved-channels, so it can be made optional.
>> 
>>   * Renamed the driver from cc_10001_xxx to cc10001_xxx so it's
>consistent
>>     with the rest of the kernel style.
>> 
>>   * Some more minor cosmetic fixes.
>> 
>> 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
>> 
>
>Unless there are any outstanding issues with this series, it'd be great
>to merge it for v3.19.
>
>Thanks!
Sorry too late unfortunately. Merge window opens on Sunday and stuff needs to have
 been in Greg KH's tree for a week before that. Going to be 3.20 now.

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

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

[Jonathan Cameron]

On 05/12/14 13:12, Jonathan Cameron wrote:
> 
> 
> On December 5, 2014 11:55:40 AM GMT+00:00, Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi Jonathan,
>>
>> On 11/27/2014 12:39 PM, Ezequiel Garcia wrote:
>>> Changes from v4:
>>>
>>>   * Added a compile-time dependency on REGULATOR and HAVE_CLK.
>>>
>>>   * Replaced the silly XOR operation for a proper mask out of the
>>>     available channels.
>>>
>>> Changes from v3:
>>>
>>>   * Fixed a few style nitpicks as per Hartmut's feedback.
>>>
>>>   * Used GENMASK() to build the channel mask, which fixes a very
>> nasty
>>>     bug. Also found by Hartmut.
>>>
>>> Changes from v2:
>>>
>>>   * Changed a devicetree property from adc-available-channels to
>>>     adc-reserved-channels, so it can be made optional.
>>>
>>>   * Renamed the driver from cc_10001_xxx to cc10001_xxx so it's
>> consistent
>>>     with the rest of the kernel style.
>>>
>>>   * Some more minor cosmetic fixes.
>>>
>>> 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
>>>
>>
>> Unless there are any outstanding issues with this series, it'd be great
>> to merge it for v3.19.
>>
>> Thanks!
> Sorry too late unfortunately. Merge window opens on Sunday and stuff needs to have
>  been in Greg KH's tree for a week before that. Going to be 3.20 now.
> 
Also, the DT patch could really do with an Ack from Rob.  If Hartmut/Lars
want to give a reviewed-by that would be good given they done the legwork
on reviewing your driver.

Looks good to me btw.  Just taken a quick look, but little gets past those
guys anyway :)

Jonathan

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

[Ezequiel Garcia]

On 12/05/2014 10:25 AM, Jonathan Cameron wrote:
> On 05/12/14 13:12, Jonathan Cameron wrote:
>>
>>
>> On December 5, 2014 11:55:40 AM GMT+00:00, Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>>> Hi Jonathan,
>>>
>>> On 11/27/2014 12:39 PM, Ezequiel Garcia wrote:
>>>> Changes from v4:
>>>>
>>>>   * Added a compile-time dependency on REGULATOR and HAVE_CLK.
>>>>
>>>>   * Replaced the silly XOR operation for a proper mask out of the
>>>>     available channels.
>>>>
>>>> Changes from v3:
>>>>
>>>>   * Fixed a few style nitpicks as per Hartmut's feedback.
>>>>
>>>>   * Used GENMASK() to build the channel mask, which fixes a very
>>> nasty
>>>>     bug. Also found by Hartmut.
>>>>
>>>> Changes from v2:
>>>>
>>>>   * Changed a devicetree property from adc-available-channels to
>>>>     adc-reserved-channels, so it can be made optional.
>>>>
>>>>   * Renamed the driver from cc_10001_xxx to cc10001_xxx so it's
>>> consistent
>>>>     with the rest of the kernel style.
>>>>
>>>>   * Some more minor cosmetic fixes.
>>>>
>>>> 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
>>>>
>>>
>>> Unless there are any outstanding issues with this series, it'd be great
>>> to merge it for v3.19.
>>>
>>> Thanks!
>> Sorry too late unfortunately. Merge window opens on Sunday and stuff needs to have
>>  been in Greg KH's tree for a week before that. Going to be 3.20 now.
>>

Oh, shoot. I was hoping to be in time for the merge. Being a new driver
there wasn't much harm in taking it earlier.

> Also, the DT patch could really do with an Ack from Rob.  If Hartmut/Lars
> want to give a reviewed-by that would be good given they done the legwork
> on reviewing your driver.
> 
> Looks good to me btw.  Just taken a quick look, but little gets past those
> guys anyway :)
> 

If at all possible, do you think you area able to pick the patches now
(but you don't include them in your pull request to Linus) so they are
linux-next? It'd be good to know they start getting build-test coverage
soon.

Thanks!
-- 
Ezequiel

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

[Jonathan Cameron]

On 05/12/14 14:33, Ezequiel Garcia wrote:
> 
> 
> On 12/05/2014 10:25 AM, Jonathan Cameron wrote:
>> On 05/12/14 13:12, Jonathan Cameron wrote:
>>>
>>>
>>> On December 5, 2014 11:55:40 AM GMT+00:00, Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>>>> Hi Jonathan,
>>>>
>>>> On 11/27/2014 12:39 PM, Ezequiel Garcia wrote:
>>>>> Changes from v4:
>>>>>
>>>>>   * Added a compile-time dependency on REGULATOR and HAVE_CLK.
>>>>>
>>>>>   * Replaced the silly XOR operation for a proper mask out of the
>>>>>     available channels.
>>>>>
>>>>> Changes from v3:
>>>>>
>>>>>   * Fixed a few style nitpicks as per Hartmut's feedback.
>>>>>
>>>>>   * Used GENMASK() to build the channel mask, which fixes a very
>>>> nasty
>>>>>     bug. Also found by Hartmut.
>>>>>
>>>>> Changes from v2:
>>>>>
>>>>>   * Changed a devicetree property from adc-available-channels to
>>>>>     adc-reserved-channels, so it can be made optional.
>>>>>
>>>>>   * Renamed the driver from cc_10001_xxx to cc10001_xxx so it's
>>>> consistent
>>>>>     with the rest of the kernel style.
>>>>>
>>>>>   * Some more minor cosmetic fixes.
>>>>>
>>>>> 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
>>>>>
>>>>
>>>> Unless there are any outstanding issues with this series, it'd be great
>>>> to merge it for v3.19.
>>>>
>>>> Thanks!
>>> Sorry too late unfortunately. Merge window opens on Sunday and stuff needs to have
>>>  been in Greg KH's tree for a week before that. Going to be 3.20 now.
>>>
> 
> Oh, shoot. I was hoping to be in time for the merge. Being a new driver
> there wasn't much harm in taking it earlier.
> 
>> Also, the DT patch could really do with an Ack from Rob.  If Hartmut/Lars
>> want to give a reviewed-by that would be good given they done the legwork
>> on reviewing your driver.
>>
>> Looks good to me btw.  Just taken a quick look, but little gets past those
>> guys anyway :)
>>
> 
> If at all possible, do you think you area able to pick the patches now
> (but you don't include them in your pull request to Linus) so they are
> linux-next? It'd be good to know they start getting build-test coverage
> soon.
Sorry no can do until 3 weeks have passed for the device tree bindings or
a dt maintainer acks.  So might be about 2 more weeks I'm afraid.  Also
it's heavily frowned upon to put stuff in linux next at this point in the
cycle.  Best to wait until merge window closes.

Note I only feed linux-next via Greg KH's tree anyway (stuff doesn't
stay in mine normally for more than a week or so, hence little gain
in a direct pull).

Jonathan
> 
> Thanks!
> 

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

[Hartmut Knaack]

Ezequiel Garcia schrieb am 27.11.2014 um 16:39:
> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> 
> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
Still some issues left, unfortunately things I pointed out in my last review.
> 
> 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       |  11 ++
>  drivers/iio/adc/Makefile      |   1 +
>  drivers/iio/adc/cc10001_adc.c | 424 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 436 insertions(+)
>  create mode 100644 drivers/iio/adc/cc10001_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 88bdc8f..15bb1ae 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -127,6 +127,17 @@ config AT91_ADC
>  	help
>  	  Say yes here to build support for Atmel AT91 ADC.
>  
> +config CC10001_ADC
> +	tristate "Cosmic Circuits 10001 ADC driver"
> +	depends on HAS_IOMEM || HAVE_CLK || REGULATOR
> +	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 cc10001_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..9286c59 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_CC10001_ADC) += cc10001_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/cc10001_adc.c b/drivers/iio/adc/cc10001_adc.c
> new file mode 100644
> index 0000000..a079ee0
> --- /dev/null
> +++ b/drivers/iio/adc/cc10001_adc.c
> @@ -0,0 +1,424 @@
> +/*
> + * 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		GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0)
This is only valid for one of the cases you make (wrong) use of CC10001_ADC_CH_MASK.
As you seem to use this mask to separate channel selection values (0 - 7) in register
CC10001_ADC_CONFIG, a GENMASK(2, 0) would be appropriate.
In case of the indio_dev->channel_mask, you would actually need this 
GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0).
> +
> +#define CC10001_INVALID_SAMPLED		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 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 cc10001_adc_write_reg(struct cc10001_adc_device *adc_dev,
> +					 u32 reg, u32 val)
> +{
> +	writel(val, adc_dev->reg_base + reg);
> +}
> +
> +static inline u32 cc10001_adc_read_reg(struct cc10001_adc_device *adc_dev,
> +				       u32 reg)
> +{
> +	return readl(adc_dev->reg_base + reg);
> +}
> +
> +static void cc10001_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;
> +	cc10001_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
> +
> +	val = cc10001_adc_read_reg(adc_dev, CC10001_ADC_CONFIG);
> +	val = val | CC10001_ADC_START_CONV;
> +	cc10001_adc_write_reg(adc_dev, CC10001_ADC_CONFIG, val);
> +}
> +
> +static u16 cc10001_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 (!(cc10001_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;
> +	}
> +
> +	poll_count = 0;
> +	while ((cc10001_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;
> +	}
> +
> +	/* Read the 10 bit output register */
> +	return cc10001_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);
> +
> +	cc10001_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) {
> +
> +		cc10001_adc_start(adc_dev, channel);
> +
> +		data[i] = cc10001_adc_poll_done(indio_dev, channel, delay_ns);
> +		if (data[i] == CC10001_INVALID_SAMPLED) {
> +			dev_warn(&indio_dev->dev,
> +				 "invalid sample on channel %d\n", channel);
> +			sample_invalid = true;
> +			goto done;
> +		}
> +		i++;
> +	}
> +
> +done:
> +	cc10001_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;
> +
> +	cc10001_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;
> +
> +	cc10001_adc_start(adc_dev, chan->channel);
> +
> +	val = cc10001_adc_poll_done(indio_dev, chan->channel, delay_ns);
> +
> +	cc10001_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)
> +			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);
> +
> +	adc_dev->channel_map = CC10001_ADC_CH_MASK;
So, here you need a GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0).
> +	if (!of_property_read_u32(node, "cosmic,reserved-adc-channels", &ret))
> +		adc_dev->channel_map &= ~ret;
> +
> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
> +	if (IS_ERR(adc_dev->reg))
> +		return -EINVAL;
Preferably pass up the real error code with PTR_ERR(adc_dev->reg).
> +
> +	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)) {
> +		ret = PTR_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");
> +		ret = PTR_ERR(adc_dev->adc_clk);
> +		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)
> +		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   = "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 v5 1/3] iio: adc: Cosmic Circuits 10001 ADC driver

[Ezequiel Garcia]

Hi Hartmut,

On 12/10/2014 08:43 PM, Hartmut Knaack wrote:
> Ezequiel Garcia schrieb am 27.11.2014 um 16:39:
>> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>
>> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
> Still some issues left, unfortunately things I pointed out in my last review.

Ouch, sorry about that.

[..]
>> +
>> +#define CC10001_ADC_DATA_MASK		GENMASK(9, 0)
>> +#define CC10001_ADC_NUM_CHANNELS	8
>> +#define CC10001_ADC_CH_MASK		GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0)
> This is only valid for one of the cases you make (wrong) use of CC10001_ADC_CH_MASK.
> As you seem to use this mask to separate channel selection values (0 - 7) in register
> CC10001_ADC_CONFIG, a GENMASK(2, 0) would be appropriate.
> In case of the indio_dev->channel_mask, you would actually need this 
> GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0).

Right.

[..]
>> +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);
>> +
>> +	adc_dev->channel_map = CC10001_ADC_CH_MASK;
> So, here you need a GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0).

Right.

>> +	if (!of_property_read_u32(node, "cosmic,reserved-adc-channels", &ret))
>> +		adc_dev->channel_map &= ~ret;
>> +
>> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>> +	if (IS_ERR(adc_dev->reg))
>> +		return -EINVAL;
> Preferably pass up the real error code with PTR_ERR(adc_dev->reg).

OK.

Thanks for all your reviews!
-- 
Ezequiel