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

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

[Ezequiel Garcia]

Here goes the third round of this series. You can find the previous patchset
and its discussion here: http://thread.gmane.org/gmane.linux.kernel.iio/14553.

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                      | 425 +++++++++++++++++++++
 5 files changed, 460 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 v3 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 | 425 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 437 insertions(+)
 create mode 100644 drivers/iio/adc/cc10001_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 88bdc8f..09e2975 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
+	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..0b74838
--- /dev/null
+++ b/drivers/iio/adc/cc10001_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 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_OUTPUT;
+	}
+
+	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_OUTPUT;
+	}
+
+	/* 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_OUTPUT) {
+			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_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);
+
+	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_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   = "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 v3 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..1533c19
--- /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: phandles to input clocks.
+  - 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 v3 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ezequiel Garcia]

Here goes the third round of this series. You can find the previous patchset
and its discussion here: http://thread.gmane.org/gmane.linux.kernel.iio/14553.

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                      | 425 +++++++++++++++++++++
 5 files changed, 460 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/cc10001_adc.txt
 create mode 100644 drivers/iio/adc/cc10001_adc.c

-- 
2.1.0

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

[Rob Herring]

On Thu, Nov 13, 2014 at 8:14 AM, Ezequiel Garcia
<ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@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 v3 2/3] DT: iio: adc: Add CC_10001 binding documentation

[Rob Herring]

On Thu, Nov 13, 2014 at 8:13 AM, Ezequiel Garcia
<ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> 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>
> ---
>  .../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..1533c19
> --- /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: phandles to input clocks.

You need to be explicit about how many clocks and their order. From
the names, I'm guessing it is only 1.

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

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

[Ezequiel Garcia]

On 11/13/2014 11:37 AM, Rob Herring wrote:
> On Thu, Nov 13, 2014 at 8:13 AM, Ezequiel Garcia
> <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> 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>
>> ---
>>  .../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..1533c19
>> --- /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: phandles to input clocks.
> 
> You need to be explicit about how many clocks and their order. From
> the names, I'm guessing it is only 1.
> 

Will fix.

Thanks!
-- 
Ezequiel

[PATCH v4 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

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

[Andrew Bresticker]

Ezequiel,

On Thu, Nov 13, 2014 at 6:14 AM, Ezequiel Garcia
<ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> Here goes the third round of this series. You can find the previous patchset
> and its discussion here: http://thread.gmane.org/gmane.linux.kernel.iio/14553.
>
> 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

This series (with v4 of patch 2/3):

Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

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

[Jonathan Cameron]

On 13/11/14 14:13, 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.  I'll just give Rob time to take another look at the device
tree bindings before taking this.

J
> ---
>  drivers/iio/adc/Kconfig       |  11 ++
>  drivers/iio/adc/Makefile      |   1 +
>  drivers/iio/adc/cc10001_adc.c | 425 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 437 insertions(+)
>  create mode 100644 drivers/iio/adc/cc10001_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 88bdc8f..09e2975 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
> +	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..0b74838
> --- /dev/null
> +++ b/drivers/iio/adc/cc10001_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 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_OUTPUT;
> +	}
> +
> +	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_OUTPUT;
> +	}
> +
> +	/* 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_OUTPUT) {
> +			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_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);
> +
> +	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_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   = "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 v3 1/3] iio: adc: Cosmic Circuits 10001 ADC driver

[Ezequiel Garcia]

On 11/15/2014 07:46 AM, Jonathan Cameron wrote:
> On 13/11/14 14:13, 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.  I'll just give Rob time to take another look at the device
> tree bindings before taking this.
> 

Sounds good. Thanks!
-- 
Ezequiel

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

[Hartmut Knaack]

Ezequiel Garcia schrieb am 13.11.2014 15:13:
> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> 
> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
A few more issues - some got newly introduced, some have been overseen earlier (and some are just matter of taste ;-)
> 
> 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 | 425 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 437 insertions(+)
>  create mode 100644 drivers/iio/adc/cc10001_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 88bdc8f..09e2975 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
> +	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..0b74838
> --- /dev/null
> +++ b/drivers/iio/adc/cc10001_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)
Don't define CC10001_ADC_CH_MASK this way, as it implies a simple relation to CC10001_ADC_NUM_CHANNELS, but it actually requires it to be base 2 (which it happens to be in this case). Preferably use GENMASK(2, 0).
> +
> +#define CC10001_INVALID_SAMPLED_OUTPUT	0xFFFF
After changing your hex values to lower case, this one is still left.
> +#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;
This element is never used.
> +	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)) {
Matter of taste case here, and I understand that indenting it to align with cc10001_adc_read_reg would look like the indentation of the following subroutine. Then again, you left an empty line to show a certain distinction. Up to you, I just slightly prefer to align blocks with equal hierarchy levels.
> +
> +		ndelay(delay);
> +		if (poll_count++ == CC10001_MAX_POLL_COUNT)
> +			return CC10001_INVALID_SAMPLED_OUTPUT;
> +	}
> +
> +	poll_count = 0;
> +	while ((cc10001_adc_read_reg(adc_dev, CC10001_ADC_CHSEL_SAMPLED) &
> +			CC10001_ADC_CH_MASK) != channel) {
Same here.
> +
> +		ndelay(delay);
> +		if (poll_count++ == CC10001_MAX_POLL_COUNT)
> +			return CC10001_INVALID_SAMPLED_OUTPUT;
> +	}
> +
> +	/* Read the 10 bit output register */
> +	return cc10001_adc_read_reg(adc_dev, CC10001_ADC_DDATA_OUT) &
> +			       CC10001_ADC_DATA_MASK;
Here I feel stronger about alignment.
> +}
> +
> +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;
These definitions could be done already during declaration.
> +	for_each_set_bit(channel, indio_dev->active_scan_mask,
> +				  indio_dev->masklength) {
Too much indentation, it should be aligned with channel.
> +
> +		cc10001_adc_start(adc_dev, channel);
> +
> +		data[i] = cc10001_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:
> +	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_OUTPUT)
> +			return -EIO;
> +		return IIO_VAL_INT;
Since C offers it:
		return (*val == CC10001_INVALID_SAMPLED_OUTPUT) ? -EIO : IIO_VAL_INT;
It would fit in one line if CC10001_INVALID_SAMPLED_OUTPUT was called CC10001_INVALID_SAMPLE ;-)
> +
> +	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)
Reduce indentation by one space.
> +{
> +	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;
Save some lines:
	return (!adc_dev->buf) ? -ENOMEM : 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) {
No need to wrap, this fits exactly 80 chars.
> +		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;
You certainly want adc_dev->channel_map to be something like GENMASK(CC10001_ADC_NUM_CHANNELS -1, 0).
> +	if (!of_property_read_u32(node, "cosmic,reserved-adc-channels", &ret))
> +		adc_dev->channel_map ^= ret;
Correct way is to mask out, not to XOR.
> +
> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
> +	if (IS_ERR_OR_NULL(adc_dev->reg))
> +		return -EINVAL;
	if (IS_ERR(adc_dev->reg))
		return 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))
Need to put error code into ret.
> +		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");
Need to put error code into ret.
> +		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");
Start message with upper case?
> +		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);
Move this above iio_device_register.
> +
> +	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 v3 1/3] iio: adc: Cosmic Circuits 10001 ADC driver

[Ezequiel Garcia]

On 11/21/2014 10:15 PM, Hartmut Knaack wrote:
> Ezequiel Garcia schrieb am 13.11.2014 15:13:
>> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>
>> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
> A few more issues - some got newly introduced, some have been overseen earlier (and some are just matter of taste ;-)

OK, let's see then.

>>
>> 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 | 425 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 437 insertions(+)
>>  create mode 100644 drivers/iio/adc/cc10001_adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 88bdc8f..09e2975 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
>> +	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..0b74838
>> --- /dev/null
>> +++ b/drivers/iio/adc/cc10001_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)
> Don't define CC10001_ADC_CH_MASK this way, as it implies a simple relation to CC10001_ADC_NUM_CHANNELS, but it actually requires it to be base 2 (which it happens to be in this case). Preferably use GENMASK(2, 0).

Right.

>> +
>> +#define CC10001_INVALID_SAMPLED_OUTPUT	0xFFFF
> After changing your hex values to lower case, this one is still left.

Right.

>> +#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;
> This element is never used.

Right.

>> +	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)) {
> Matter of taste case here, and I understand that indenting it to align with cc10001_adc_read_reg would look like the indentation of the following subroutine. Then again, you left an empty line to show a certain distinction. Up to you, I just slightly prefer to align blocks with equal hierarchy levels.
>> +
>> +		ndelay(delay);
>> +		if (poll_count++ == CC10001_MAX_POLL_COUNT)
>> +			return CC10001_INVALID_SAMPLED_OUTPUT;
>> +	}
>> +
>> +	poll_count = 0;
>> +	while ((cc10001_adc_read_reg(adc_dev, CC10001_ADC_CHSEL_SAMPLED) &
>> +			CC10001_ADC_CH_MASK) != channel) {
> Same here.
>> +
>> +		ndelay(delay);
>> +		if (poll_count++ == CC10001_MAX_POLL_COUNT)
>> +			return CC10001_INVALID_SAMPLED_OUTPUT;
>> +	}
>> +
>> +	/* Read the 10 bit output register */
>> +	return cc10001_adc_read_reg(adc_dev, CC10001_ADC_DDATA_OUT) &
>> +			       CC10001_ADC_DATA_MASK;
> Here I feel stronger about alignment.
>> +}
>> +
>> +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;
> These definitions could be done already during declaration.

Yeah, I guess I felt it was more readable this way. But I don't care much.

>> +	for_each_set_bit(channel, indio_dev->active_scan_mask,
>> +				  indio_dev->masklength) {
> Too much indentation, it should be aligned with channel.

Ditto.

>> +
>> +		cc10001_adc_start(adc_dev, channel);
>> +
>> +		data[i] = cc10001_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:
>> +	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_OUTPUT)
>> +			return -EIO;
>> +		return IIO_VAL_INT;
> Since C offers it:
> 		return (*val == CC10001_INVALID_SAMPLED_OUTPUT) ? -EIO : IIO_VAL_INT;

Hm, don't you find this a bit eyesore?

> It would fit in one line if CC10001_INVALID_SAMPLED_OUTPUT was called CC10001_INVALID_SAMPLE ;-)

Right, that's a better name indeed.

>> +
>> +	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)
> Reduce indentation by one space.
>> +{
>> +	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;
> Save some lines:
> 	return (!adc_dev->buf) ? -ENOMEM : 0;

Hm... that's uglier to me. Lines are free, no need to save them :)

>> +}
>> +
>> +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) {
> No need to wrap, this fits exactly 80 chars.

Right.

>> +		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;
> You certainly want adc_dev->channel_map to be something like GENMASK(CC10001_ADC_NUM_CHANNELS -1, 0).
>> +	if (!of_property_read_u32(node, "cosmic,reserved-adc-channels", &ret))
>> +		adc_dev->channel_map ^= ret;
> Correct way is to mask out, not to XOR.

Unless I'm missing something, that's exactly what XOR does.

Example:

reserved_channels = 0x2;
channel_map = 0x7 ^ reserved_channels -> 0x5

>> +
>> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>> +	if (IS_ERR_OR_NULL(adc_dev->reg))
>> +		return -EINVAL;
> 	if (IS_ERR(adc_dev->reg))
> 		return PTR_ERR(adc_dev->reg);

Are you sure? What if devm_regulator_get() returns NULL?

>> +
>> +	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))
> Need to put error code into ret.

Right.

>> +		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");
> Need to put error code into ret.
>> +		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");
> Start message with upper case?

Actually, I'd rather make the others start with lower case.

>> +		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);
> Move this above iio_device_register.

What for?

>> +
>> +	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");
>>
> 

Thanks for the review!
-- 
Ezequiel

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

[Hartmut Knaack]

Ezequiel Garcia schrieb am 25.11.2014 18:46:
> 
> 
> On 11/21/2014 10:15 PM, Hartmut Knaack wrote:
>> Ezequiel Garcia schrieb am 13.11.2014 15:13:
>>> From: Phani Movva <Phani.Movva-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>>
>>> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
>> A few more issues - some got newly introduced, some have been overseen earlier (and some are just matter of taste ;-)
> 
> OK, let's see then.
> 
>>>
>>> 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 | 425 ++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 437 insertions(+)
>>>  create mode 100644 drivers/iio/adc/cc10001_adc.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 88bdc8f..09e2975 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
>>> +	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..0b74838
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/cc10001_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)
>> Don't define CC10001_ADC_CH_MASK this way, as it implies a simple relation to CC10001_ADC_NUM_CHANNELS, but it actually requires it to be base 2 (which it happens to be in this case). Preferably use GENMASK(2, 0).
> 
> Right.
> 
>>> +
>>> +#define CC10001_INVALID_SAMPLED_OUTPUT	0xFFFF
>> After changing your hex values to lower case, this one is still left.
> 
> Right.
> 
>>> +#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;
>> This element is never used.
> 
> Right.
> 
>>> +	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)) {
>> Matter of taste case here, and I understand that indenting it to align with cc10001_adc_read_reg would look like the indentation of the following subroutine. Then again, you left an empty line to show a certain distinction. Up to you, I just slightly prefer to align blocks with equal hierarchy levels.
>>> +
>>> +		ndelay(delay);
>>> +		if (poll_count++ == CC10001_MAX_POLL_COUNT)
>>> +			return CC10001_INVALID_SAMPLED_OUTPUT;
>>> +	}
>>> +
>>> +	poll_count = 0;
>>> +	while ((cc10001_adc_read_reg(adc_dev, CC10001_ADC_CHSEL_SAMPLED) &
>>> +			CC10001_ADC_CH_MASK) != channel) {
>> Same here.
>>> +
>>> +		ndelay(delay);
>>> +		if (poll_count++ == CC10001_MAX_POLL_COUNT)
>>> +			return CC10001_INVALID_SAMPLED_OUTPUT;
>>> +	}
>>> +
>>> +	/* Read the 10 bit output register */
>>> +	return cc10001_adc_read_reg(adc_dev, CC10001_ADC_DDATA_OUT) &
>>> +			       CC10001_ADC_DATA_MASK;
>> Here I feel stronger about alignment.
>>> +}
>>> +
>>> +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;
>> These definitions could be done already during declaration.
> 
> Yeah, I guess I felt it was more readable this way. But I don't care much.
> 
>>> +	for_each_set_bit(channel, indio_dev->active_scan_mask,
>>> +				  indio_dev->masklength) {
>> Too much indentation, it should be aligned with channel.
> 
> Ditto.
> 
>>> +
>>> +		cc10001_adc_start(adc_dev, channel);
>>> +
>>> +		data[i] = cc10001_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:
>>> +	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_OUTPUT)
>>> +			return -EIO;
>>> +		return IIO_VAL_INT;
>> Since C offers it:
>> 		return (*val == CC10001_INVALID_SAMPLED_OUTPUT) ? -EIO : IIO_VAL_INT;
> 
> Hm, don't you find this a bit eyesore?
Well, I've seen worse code in my life ;-)
> 
>> It would fit in one line if CC10001_INVALID_SAMPLED_OUTPUT was called CC10001_INVALID_SAMPLE ;-)
> 
> Right, that's a better name indeed.
> 
>>> +
>>> +	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)
>> Reduce indentation by one space.
>>> +{
>>> +	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;
>> Save some lines:
>> 	return (!adc_dev->buf) ? -ENOMEM : 0;
> 
> Hm... that's uglier to me. Lines are free, no need to save them :)
Matter of taste, and up to you. I'm more into "short and simple".
> 
>>> +}
>>> +
>>> +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) {
>> No need to wrap, this fits exactly 80 chars.
> 
> Right.
> 
>>> +		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;
>> You certainly want adc_dev->channel_map to be something like GENMASK(CC10001_ADC_NUM_CHANNELS -1, 0).
>>> +	if (!of_property_read_u32(node, "cosmic,reserved-adc-channels", &ret))
>>> +		adc_dev->channel_map ^= ret;
>> Correct way is to mask out, not to XOR.
> 
> Unless I'm missing something, that's exactly what XOR does.
> 
> Example:
> 
> reserved_channels = 0x2;
> channel_map = 0x7 ^ reserved_channels -> 0x5
> 
You miss to check ret for a valid range, which you don't need to do when masking out channels.
Example:
reserved_channels = 0x8;
channel_map = 0x7 ^ reserved_channels -> 0xf
And this is actually happening in the current revision (with the wrong channel_map assignment), that a reserved channel in DT would become usable.
>>> +
>>> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>>> +	if (IS_ERR_OR_NULL(adc_dev->reg))
>>> +		return -EINVAL;
>> 	if (IS_ERR(adc_dev->reg))
>> 		return PTR_ERR(adc_dev->reg);
> 
> Are you sure? What if devm_regulator_get() returns NULL?
> 
I had a look at it, but couldn't figure out a condition in which it would return NULL. But I'd be happy to be informed of anything different.
>>> +
>>> +	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))
>> Need to put error code into ret.
> 
> Right.
> 
>>> +		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");
>> Need to put error code into ret.
>>> +		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");
>> Start message with upper case?
> 
> Actually, I'd rather make the others start with lower case.
Fair enough, unless they are full sentences like here. My english grammar tells me they should start with capital letters.
> 
>>> +		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);
>> Move this above iio_device_register.
> 
> What for?
To make iio_device_register the last operation of the probe.
> 
>>> +
>>> +	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");
>>>
>>
> 
> Thanks for the review!
> 

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

[Ezequiel Garcia]

On 11/25/2014 06:41 PM, Hartmut Knaack wrote:
>>
>> Unless I'm missing something, that's exactly what XOR does.
>>
>> Example:
>>
>> reserved_channels = 0x2;
>> channel_map = 0x7 ^ reserved_channels -> 0x5
>>
> You miss to check ret for a valid range, which you don't need to do when masking out channels.
> Example:
> reserved_channels = 0x8;
> channel_map = 0x7 ^ reserved_channels -> 0xf
> And this is actually happening in the current revision (with the wrong channel_map assignment), that a reserved channel in DT would become usable.

Right, definitely a check is needed.

>>>> +
>>>> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>>>> +	if (IS_ERR_OR_NULL(adc_dev->reg))
>>>> +		return -EINVAL;
>>> 	if (IS_ERR(adc_dev->reg))
>>> 		return PTR_ERR(adc_dev->reg);
>>
>> Are you sure? What if devm_regulator_get() returns NULL?
>>
> I had a look at it, but couldn't figure out a condition in which it would return NULL. But I'd be happy to be informed of anything different.

If CONFIG_REGULATOR=n you get NULL there (although I added a select REGULATOR
on the v4 I just posted).

>>>> +
>>>> +	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))
>>> Need to put error code into ret.
>>
>> Right.
>>
>>>> +		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");
>>> Need to put error code into ret.
>>>> +		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");
>>> Start message with upper case?
>>
>> Actually, I'd rather make the others start with lower case.
> Fair enough, unless they are full sentences like here. My english grammar tells me they should start with capital letters.

Yeah, but it's not the start of the message, as it's prefixed
with the device stuff.

>>
>>>> +		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);
>>> Move this above iio_device_register.
>>
>> What for?
> To make iio_device_register the last operation of the probe.

I really don't think it matters, since platform_get_drvdata
is only called in the remove.

I'll move it if you think it's worth it, though.

Thanks for the review!
-- 
Ezequiel

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

[Hartmut Knaack]

Ezequiel Garcia schrieb am 25.11.2014 23:03:
> 
> 
> On 11/25/2014 06:41 PM, Hartmut Knaack wrote:
>>>
>>> Unless I'm missing something, that's exactly what XOR does.
>>>
>>> Example:
>>>
>>> reserved_channels = 0x2;
>>> channel_map = 0x7 ^ reserved_channels -> 0x5
>>>
>> You miss to check ret for a valid range, which you don't need to do when masking out channels.
>> Example:
>> reserved_channels = 0x8;
>> channel_map = 0x7 ^ reserved_channels -> 0xf
>> And this is actually happening in the current revision (with the wrong channel_map assignment), that a reserved channel in DT would become usable.
> 
> Right, definitely a check is needed.
No, that's not needed when using AND, as it can not add bits in the channel map. The following will do the job:
		adc_dev->channel_map &= ~ret;
> 
>>>>> +
>>>>> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>>>>> +	if (IS_ERR_OR_NULL(adc_dev->reg))
>>>>> +		return -EINVAL;
>>>> 	if (IS_ERR(adc_dev->reg))
>>>> 		return PTR_ERR(adc_dev->reg);
>>>
>>> Are you sure? What if devm_regulator_get() returns NULL?
>>>
>> I had a look at it, but couldn't figure out a condition in which it would return NULL. But I'd be happy to be informed of anything different.
> 
> If CONFIG_REGULATOR=n you get NULL there (although I added a select REGULATOR
> on the v4 I just posted).
> 
Indeed, thanks. Now I'm thinking if it would make sense to handle this special case in a special way (as it would be caused by an invalid kernel config). Well, I guess, I leave it up to you.
>>>>> +
>>>>> +	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))
>>>> Need to put error code into ret.
>>>
>>> Right.
>>>
>>>>> +		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");
>>>> Need to put error code into ret.
>>>>> +		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");
>>>> Start message with upper case?
>>>
>>> Actually, I'd rather make the others start with lower case.
>> Fair enough, unless they are full sentences like here. My english grammar tells me they should start with capital letters.
> 
> Yeah, but it's not the start of the message, as it's prefixed
> with the device stuff.
> 
>>>
>>>>> +		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);
>>>> Move this above iio_device_register.
>>>
>>> What for?
>> To make iio_device_register the last operation of the probe.
> 
> I really don't think it matters, since platform_get_drvdata
> is only called in the remove.
> 
> I'll move it if you think it's worth it, though.
> 
> Thanks for the review!
> 

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

[Ezequiel Garcia]

On 11/25/2014 07:53 PM, Hartmut Knaack wrote:
> Ezequiel Garcia schrieb am 25.11.2014 23:03:
>>
>>
>> On 11/25/2014 06:41 PM, Hartmut Knaack wrote:
>>>>
>>>> Unless I'm missing something, that's exactly what XOR does.
>>>>
>>>> Example:
>>>>
>>>> reserved_channels = 0x2;
>>>> channel_map = 0x7 ^ reserved_channels -> 0x5
>>>>
>>> You miss to check ret for a valid range, which you don't need to do when masking out channels.
>>> Example:
>>> reserved_channels = 0x8;
>>> channel_map = 0x7 ^ reserved_channels -> 0xf
>>> And this is actually happening in the current revision (with the wrong channel_map assignment), that a reserved channel in DT would become usable.
>>
>> Right, definitely a check is needed.
> No, that's not needed when using AND, as it can not add bits in the channel map. The following will do the job:
> 		adc_dev->channel_map &= ~ret;

Gah, of course, that's way much better.

>>
>>>>>> +
>>>>>> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>>>>>> +	if (IS_ERR_OR_NULL(adc_dev->reg))
>>>>>> +		return -EINVAL;
>>>>> 	if (IS_ERR(adc_dev->reg))
>>>>> 		return PTR_ERR(adc_dev->reg);
>>>>
>>>> Are you sure? What if devm_regulator_get() returns NULL?
>>>>
>>> I had a look at it, but couldn't figure out a condition in which it would return NULL. But I'd be happy to be informed of anything different.
>>
>> If CONFIG_REGULATOR=n you get NULL there (although I added a select REGULATOR
>> on the v4 I just posted).
>>
> Indeed, thanks. Now I'm thinking if it would make sense to handle this special case in a special way (as it would be caused by an invalid kernel config). Well, I guess, I leave it up to you.

Well, adding the select REGULATOR that shouldn't happen, so let's better
drop the NULL check entirely.
-- 
Ezequiel

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

[Lars-Peter Clausen]

[...]
>>>
>>>>>>> +
>>>>>>> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>>>>>>> +	if (IS_ERR_OR_NULL(adc_dev->reg))
>>>>>>> +		return -EINVAL;
>>>>>> 	if (IS_ERR(adc_dev->reg))
>>>>>> 		return PTR_ERR(adc_dev->reg);
>>>>>
>>>>> Are you sure? What if devm_regulator_get() returns NULL?
>>>>>
>>>> I had a look at it, but couldn't figure out a condition in which it would return NULL. But I'd be happy to be informed of anything different.
>>>
>>> If CONFIG_REGULATOR=n you get NULL there (although I added a select REGULATOR
>>> on the v4 I just posted).
>>>
>> Indeed, thanks. Now I'm thinking if it would make sense to handle this special case in a special way (as it would be caused by an invalid kernel config). Well, I guess, I leave it up to you.
>
> Well, adding the select REGULATOR that shouldn't happen, so let's better
> drop the NULL check entirely.

Doing a 'select REGULATOR' can (and probably will sooner or later) create 
nasty dependency loops. User selectable config items should not be selected 
by another config item. If the driver really has a hard dependency on the 
regulator framework then 'depends on REGULATOR' is the right thing to do.

- Lars

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

[Ezequiel Garcia]

On 11/27/2014 12:16 PM, Lars-Peter Clausen wrote:
> [...]
>>>>
>>>>>>>> +
>>>>>>>> +    adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>>>>>>>> +    if (IS_ERR_OR_NULL(adc_dev->reg))
>>>>>>>> +        return -EINVAL;
>>>>>>>     if (IS_ERR(adc_dev->reg))
>>>>>>>         return PTR_ERR(adc_dev->reg);
>>>>>>
>>>>>> Are you sure? What if devm_regulator_get() returns NULL?
>>>>>>
>>>>> I had a look at it, but couldn't figure out a condition in which it
>>>>> would return NULL. But I'd be happy to be informed of anything
>>>>> different.
>>>>
>>>> If CONFIG_REGULATOR=n you get NULL there (although I added a select
>>>> REGULATOR
>>>> on the v4 I just posted).
>>>>
>>> Indeed, thanks. Now I'm thinking if it would make sense to handle
>>> this special case in a special way (as it would be caused by an
>>> invalid kernel config). Well, I guess, I leave it up to you.
>>
>> Well, adding the select REGULATOR that shouldn't happen, so let's better
>> drop the NULL check entirely.
> 
> Doing a 'select REGULATOR' can (and probably will sooner or later)
> create nasty dependency loops. User selectable config items should not
> be selected by another config item. If the driver really has a hard
> dependency on the regulator framework then 'depends on REGULATOR' is the
> right thing to do.
> 

Fine, then. 'depends on REGULATOR' it is.

-- 
Ezequiel