[PATCH 0/4] ARM: berlin: ADC support

[PATCH 0/4] ARM: berlin: ADC support

[Antoine Tenart]

Hi,

The Berlin ADC provides 8 channels, with one connected to a temperature
sensor. The temperature sensor has its own registers and both the ADC
and the temperature sensor need to be configured when using it.

This series is based on the two Berlin controllers rework series:
- https://lkml.org/lkml/2015/3/6/511
- https://lkml.org/lkml/2015/3/6/535

Antoine

Antoine Tenart (4):
  iio: adc: add support for Berlin
  Documentation: bindings: document the Berlin ADC driver
  ARM: berlin: add an ADC node for the BG2Q
  ARM: berlin: enable the ADC on the BG2Q DMP

 .../devicetree/bindings/iio/adc/berlin2_adc.txt    |  19 +
 arch/arm/boot/dts/berlin2q-marvell-dmp.dts         |   4 +
 arch/arm/boot/dts/berlin2q.dtsi                    |   8 +
 drivers/iio/adc/Kconfig                            |   7 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/berlin-adc.c                       | 397 +++++++++++++++++++++
 6 files changed, 436 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/berlin2_adc.txt
 create mode 100644 drivers/iio/adc/berlin-adc.c

-- 
2.3.3

[PATCH 1/4] iio: adc: add support for Berlin

[Antoine Tenart]

This patch adds the support of the Berlin ADC, available on Berlin SoCs.
This ADC has 8 channels available, with one connected to a temperature
sensor.

The particularity here, is that the temperature sensor connected to the
ADC has its own registers, and both the ADC and the temperature sensor
must be configured when using it.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/iio/adc/Kconfig      |   7 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/berlin-adc.c | 397 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 405 insertions(+)
 create mode 100644 drivers/iio/adc/berlin-adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 202daf889be2..2f10a6d8d442 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -135,6 +135,13 @@ config AXP288_ADC
 	  device. Depending on platform configuration, this general purpose ADC can
 	  be used for sampling sensors such as thermal resistors.
 
+config BERLIN_ADC
+	tristate "Marvell Berlin ADC driver"
+	depends on ARCH_BERLIN
+	help
+	  Marvell Berlin ADC driver. This ADC has 8 channels, with one used for
+	  temparature measurement.
+
 config CC10001_ADC
 	tristate "Cosmic Circuits 10001 ADC driver"
 	depends on HAS_IOMEM || HAVE_CLK || REGULATOR
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0315af640866..d7b2d1df2353 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
 obj-$(CONFIG_AD799X) += ad799x.o
 obj-$(CONFIG_AT91_ADC) += at91_adc.o
 obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
+obj-$(CONFIG_BERLIN_ADC) += berlin-adc.o
 obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
diff --git a/drivers/iio/adc/berlin-adc.c b/drivers/iio/adc/berlin-adc.c
new file mode 100644
index 000000000000..8cb0f5800511
--- /dev/null
+++ b/drivers/iio/adc/berlin-adc.c
@@ -0,0 +1,397 @@
+/*
+ * Marvell Berlin ADC driver
+ *
+ * Copyright (C) 2015 Marvell Technology Group Ltd.
+ *
+ * Antoine Tenart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/iio/iio.h>
+#include <linux/iio/driver.h>
+#include <linux/iio/machine.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/sched.h>
+#include <linux/wait.h>
+
+#define SYSCTL_SM_CTRL				0x14
+#define  SYSCTL_SM_CTRL_SM_SOC_INT		BIT(1)
+#define  SYSCTL_SM_CTRL_SOC_SM_INT		BIT(2)
+#define  SYSCTL_SM_CTRL_ADC_SEL(x)		(BIT(x) << 5)	/* 0-15 */
+#define  SYSCTL_SM_CTRL_ADC_SEL_MASK		(0xf << 5)
+#define  SYSCTL_SM_CTRL_ADC_POWER		BIT(9)
+#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV2		(0x0 << 10)
+#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV3		(0x1 << 10)
+#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV4		(0x2 << 10)
+#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV8		(0x3 << 10)
+#define  SYSCTL_SM_CTRL_ADC_CLKSEL_MASK		(0x3 << 10)
+#define  SYSCTL_SM_CTRL_ADC_START		BIT(12)
+#define  SYSCTL_SM_CTRL_ADC_RESET		BIT(13)
+#define  SYSCTL_SM_CTRL_ADC_BANDGAP_RDY		BIT(14)
+#define  SYSCTL_SM_CTRL_ADC_CONT_SINGLE		(0x0 << 15)
+#define  SYSCTL_SM_CTRL_ADC_CONT_CONTINUOUS	(0x1 << 15)
+#define  SYSCTL_SM_CTRL_ADC_BUFFER_EN		BIT(16)
+#define  SYSCTL_SM_CTRL_ADC_VREF_EXT		(0x0 << 17)
+#define  SYSCTL_SM_CTRL_ADC_VREF_INT		(0x1 << 17)
+#define  SYSCTL_SM_CTRL_ADC_ROTATE		BIT(19)
+#define  SYSCTL_SM_CTRL_TSEN_EN			BIT(20)
+#define  SYSCTL_SM_CTRL_TSEN_CLK_SEL_125	(0x0 << 21)	/* 1.25 MHz */
+#define  SYSCTL_SM_CTRL_TSEN_CLK_SEL_250	(0x1 << 21)	/* 2.5 MHz */
+#define  SYSCTL_SM_CTRL_TSEN_MODE_0_125		(0x0 << 22)	/* 0-125 C */
+#define  SYSCTL_SM_CTRL_TSEN_MODE_10_50		(0x1 << 22)	/* 10-50 C */
+#define  SYSCTL_SM_CTRL_TSEN_RESET		BIT(29)
+#define SYSCTL_SM_ADC_DATA			0x20
+#define  SYSCTL_SM_ADC_MASK			0x3ff
+#define SYSCTL_SM_ADC_STATUS			0x1c
+#define  SYSCTL_SM_ADC_STATUS_DATA_RDY(x)	BIT(x)		/* 0-15 */
+#define  SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK	0xf
+#define  SYSCTL_SM_ADC_STATUS_INT_EN(x)		(BIT(x) << 16)	/* 0-15 */
+#define  SYSCTL_SM_ADC_STATUS_INT_EN_MASK	(0xf << 16)
+#define SYSCTL_SM_TSEN_STATUS			0x24
+#define  SYSCTL_SM_TSEN_STATUS_DATA_RDY		BIT(0)
+#define  SYSCTL_SM_TSEN_STATUS_INT_EN		BIT(1)
+#define SYSCTL_SM_TSEN_DATA			0x28
+#define  SYSCTL_SM_TSEN_MASK			0x3ff
+#define SYSCTL_SM_TSEN_CTRL			0x74
+#define  SYSCTL_SM_TSEN_CTRL_START		BIT(8)
+#define  SYSCTL_SM_TSEN_CTRL_SETTLING_4		(0x0 << 21)	/* 4 us */
+#define  SYSCTL_SM_TSEN_CTRL_SETTLING_12	(0x1 << 21)	/* 12 us */
+#define  SYSCTL_SM_TSEN_CTRL_TRIM(x)		((x) << 22)
+#define  SYSCTL_SM_TSEN_CTRL_TRIM_MASK		(0xf << 22)
+
+struct berlin2_adc_priv {
+	struct regmap		*regmap;
+	struct mutex		lock;
+	wait_queue_head_t	wq;
+	u32			irq;
+	u32			tsen_irq;
+	bool			data_available;
+	int			data;
+};
+
+#define BERLIN2_ADC_CHANNEL(n, t)					\
+		{							\
+			.channel	= n,				\
+			.datasheet_name	= "channel"#n,			\
+			.type		= t,				\
+			.indexed	= 1,				\
+			.scan_index	= n,				\
+			.scan_type	= {				\
+				.sign		= 'u',			\
+				.realbits	= 64,			\
+				.storagebits	= 64,			\
+			},						\
+			.info_mask_shared_by_type = 0,			\
+			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+		}
+
+#define N_CHANNELS		8
+static struct iio_chan_spec berlin2_adc_channels[] = {
+	BERLIN2_ADC_CHANNEL(0, IIO_VOLTAGE),	/* external input */
+	BERLIN2_ADC_CHANNEL(1, IIO_VOLTAGE),	/* external input */
+	BERLIN2_ADC_CHANNEL(2, IIO_VOLTAGE),	/* external input */
+	BERLIN2_ADC_CHANNEL(3, IIO_VOLTAGE),	/* external input */
+	BERLIN2_ADC_CHANNEL(4, IIO_VOLTAGE),	/* reserved */
+	BERLIN2_ADC_CHANNEL(5, IIO_VOLTAGE),	/* reserved */
+	BERLIN2_ADC_CHANNEL(6, IIO_TEMP),	/* temperature sensor */
+	BERLIN2_ADC_CHANNEL(7, IIO_VOLTAGE),	/* reserved */
+	{					/* timestamp */
+		.channel	= -1,
+		.type		= IIO_TIMESTAMP,
+		.scan_index	= N_CHANNELS,
+		.scan_type	= {
+			.sign		= 's',
+			.realbits	= 64,
+			.storagebits	= 64,
+		},
+	},
+};
+
+static int berlin2_adc_read(struct iio_dev *idev, int channel)
+{
+	struct berlin2_adc_priv *priv = iio_priv(idev);
+	int val, data, ret;
+
+	mutex_lock(&priv->lock);
+
+	/* Configure the ADC */
+	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
+	val &= ~(SYSCTL_SM_CTRL_ADC_RESET | SYSCTL_SM_CTRL_ADC_SEL_MASK);
+	val |= SYSCTL_SM_CTRL_ADC_SEL(channel) | SYSCTL_SM_CTRL_ADC_START;
+	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
+
+	/* Enable the interrupts */
+	regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS,
+			SYSCTL_SM_ADC_STATUS_INT_EN(channel));
+
+	ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
+			msecs_to_jiffies(1000));
+
+	/* Disable the interrupts */
+	regmap_read(priv->regmap, SYSCTL_SM_ADC_STATUS, &val);
+	val &= ~SYSCTL_SM_ADC_STATUS_INT_EN(channel);
+	regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS, val);
+
+	if (ret == 0)
+		ret = -ETIMEDOUT;
+	if (ret < 0) {
+		mutex_unlock(&priv->lock);
+		return ret;
+	}
+
+	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
+	val &= ~SYSCTL_SM_CTRL_ADC_START;
+	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
+
+	data = priv->data;
+	priv->data = -1;
+	priv->data_available = false;
+
+	mutex_unlock(&priv->lock);
+
+	return data;
+}
+
+static int berlin2_adc_tsen_read(struct iio_dev *idev)
+{
+	struct berlin2_adc_priv *priv = iio_priv(idev);
+	int val, data, ret;
+
+	mutex_lock(&priv->lock);
+
+	/* Configure the ADC */
+	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
+	val &= ~SYSCTL_SM_CTRL_TSEN_RESET;
+	val |= SYSCTL_SM_CTRL_ADC_ROTATE;
+	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
+
+	/* Configure the temperature sensor */
+	regmap_read(priv->regmap, SYSCTL_SM_TSEN_CTRL, &val);
+	val &= ~SYSCTL_SM_TSEN_CTRL_TRIM_MASK;
+	val |= SYSCTL_SM_TSEN_CTRL_TRIM(3) | SYSCTL_SM_TSEN_CTRL_SETTLING_12
+		| SYSCTL_SM_TSEN_CTRL_START;
+	regmap_write(priv->regmap, SYSCTL_SM_TSEN_CTRL, val);
+
+	/* Enable interrupts */
+	regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS,
+			SYSCTL_SM_TSEN_STATUS_INT_EN);
+
+	ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
+			msecs_to_jiffies(1000));
+
+	/* Disable interrupts */
+	regmap_read(priv->regmap, SYSCTL_SM_TSEN_STATUS, &val);
+	val &= ~SYSCTL_SM_TSEN_STATUS_INT_EN;
+	regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS, val);
+
+	if (ret == 0)
+		ret = -ETIMEDOUT;
+	if (ret < 0) {
+		mutex_unlock(&priv->lock);
+		return ret;
+	}
+
+	regmap_read(priv->regmap, SYSCTL_SM_TSEN_CTRL, &val);
+	val &= ~SYSCTL_SM_TSEN_CTRL_START;
+	regmap_write(priv->regmap, SYSCTL_SM_TSEN_CTRL, val);
+
+	data = priv->data;
+	priv->data = -1;
+	priv->data_available = false;
+
+	mutex_unlock(&priv->lock);
+
+	return data;
+}
+
+static int berlin2_adc_read_raw(struct iio_dev *idev,
+		struct iio_chan_spec const *chan, int *val, int *val2,
+		long mask)
+{
+	int temp;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->type == IIO_VOLTAGE) {
+			*val = berlin2_adc_read(idev, chan->channel);
+			if (*val < 0)
+				return *val;
+
+			return IIO_VAL_INT;
+		} else if (chan->type == IIO_TEMP) {
+			temp = berlin2_adc_tsen_read(idev);
+			if (temp < 0)
+				return temp;
+
+			if (temp > 2047)
+				temp = -(4096 - temp);
+
+			/* Convert to Celsius */
+			*val = (temp * 100) / 264 - 270;
+			return IIO_VAL_INT;
+		}
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static irqreturn_t berlin2_adc_irq(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct berlin2_adc_priv *priv = iio_priv(idev);
+	unsigned val;
+
+	regmap_read(priv->regmap, SYSCTL_SM_ADC_STATUS, &val);
+	if (val & SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK) {
+		regmap_read(priv->regmap, SYSCTL_SM_ADC_DATA, &priv->data);
+		priv->data &= SYSCTL_SM_ADC_MASK;
+
+		val &= ~SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK;
+		regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS, val);
+
+		priv->data_available = true;
+		wake_up_interruptible(&priv->wq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t berlin2_adc_tsen_irq(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct berlin2_adc_priv *priv = iio_priv(idev);
+	unsigned val;
+
+	regmap_read(priv->regmap, SYSCTL_SM_TSEN_STATUS, &val);
+	if (val & SYSCTL_SM_TSEN_STATUS_DATA_RDY) {
+		regmap_read(priv->regmap, SYSCTL_SM_TSEN_DATA, &priv->data);
+		priv->data &= SYSCTL_SM_TSEN_MASK;
+
+		val &= ~SYSCTL_SM_TSEN_STATUS_DATA_RDY;
+		regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS, val);
+
+		priv->data_available = true;
+		wake_up_interruptible(&priv->wq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info berlin2_adc_info = {
+	.driver_module	= THIS_MODULE,
+	.read_raw	= berlin2_adc_read_raw,
+};
+
+static int berlin2_adc_probe(struct platform_device *pdev)
+{
+	struct iio_dev *idev;
+	struct berlin2_adc_priv *priv;
+	struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
+	int ret, val;
+
+	idev = iio_device_alloc(sizeof(struct berlin2_adc_priv));
+	if (!idev)
+		return -ENOMEM;
+
+	priv = iio_priv(idev);
+	platform_set_drvdata(pdev, idev);
+
+	priv->regmap = syscon_node_to_regmap(parent_np);
+	of_node_put(parent_np);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	priv->irq = platform_get_irq_byname(pdev, "adc");
+	if (priv->irq < 0)
+		return -ENODEV;
+
+	priv->tsen_irq = platform_get_irq_byname(pdev, "tsen");
+	if (priv->tsen_irq < 0)
+		return -ENODEV;
+
+	ret = devm_request_irq(&pdev->dev, priv->irq, berlin2_adc_irq, 0,
+			pdev->dev.driver->name, idev);
+	if (ret)
+		return ret;
+
+	ret = devm_request_irq(&pdev->dev, priv->tsen_irq, berlin2_adc_tsen_irq,
+			0, pdev->dev.driver->name, idev);
+	if (ret)
+		return ret;
+
+	init_waitqueue_head(&priv->wq);
+	mutex_init(&priv->lock);
+
+	idev->dev.parent = &pdev->dev;
+	idev->name = dev_name(&pdev->dev);
+	idev->modes = INDIO_DIRECT_MODE;
+	idev->info = &berlin2_adc_info;
+
+	idev->num_channels = N_CHANNELS;
+	idev->channels = berlin2_adc_channels;
+
+	/* Power up the ADC */
+	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
+	val |= SYSCTL_SM_CTRL_ADC_POWER;
+	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
+
+	ret = iio_device_register(idev);
+	if (ret) {
+		dev_err(&pdev->dev,  "Failed to register the IIO device: %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int berlin2_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *idev = platform_get_drvdata(pdev);
+	struct berlin2_adc_priv *priv = iio_priv(idev);
+	int val;
+
+	/* Power down the ADC */
+	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
+	val &= ~SYSCTL_SM_CTRL_ADC_POWER;
+	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
+
+	free_irq(priv->irq, idev);
+	free_irq(priv->tsen_irq, idev);
+
+	iio_device_unregister(idev);
+	iio_device_free(idev);
+
+	return 0;
+}
+
+static const struct of_device_id berlin2_adc_match[] = {
+	{ .compatible = "marvell,berlin2-adc", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, berlin2q_adc_match);
+
+static struct platform_driver berlin2_adc_driver = {
+	.driver	= {
+		.name		= "berlin2-adc",
+		.of_match_table	= berlin2_adc_match,
+	},
+	.probe	= berlin2_adc_probe,
+	.remove	= berlin2_adc_remove,
+};
+module_platform_driver(berlin2_adc_driver);
+
+MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
+MODULE_DESCRIPTION("Marvell Berlin2 ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.3.3

[PATCH 2/4] Documentation: bindings: document the Berlin ADC driver

[Antoine Tenart]

Following the addition of a Berlin ADC driver, this patch adds the
corresponding bindings documentation.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/iio/adc/berlin2_adc.txt       | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/berlin2_adc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/berlin2_adc.txt b/Documentation/devicetree/bindings/iio/adc/berlin2_adc.txt
new file mode 100644
index 000000000000..908334c6b07f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/berlin2_adc.txt
@@ -0,0 +1,19 @@
+* Berlin Analog to Digital Converter (ADC)
+
+The Berlin ADC has 8 channels, with one connected to a temperature sensor.
+It is part of the system controller register set. The ADC node should be a
+sub-node of the system controller node.
+
+Required properties:
+- compatible: must be "marvell,berlin2-adc"
+- interrupts: the interrupts for the ADC and the temperature sensor
+- interrupt-names: should be "adc" and "tsen"
+
+Example:
+
+adc: adc {
+	compatible = "marvell,berlin2-adc";
+	interrupt-parent = <&sic>;
+	interrupts = <12>, <14>;
+	interrupt-names = "adc", "tsen";
+};
-- 
2.3.3

[PATCH 3/4] ARM: berlin: add an ADC node for the BG2Q

[Antoine Tenart]

This patch adds the ADC node for the Berlin BG2Q, using the newly added
Berlin IIO ADC driver.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
index 187d056f7ad2..72b1c969a99d 100644
--- a/arch/arm/boot/dts/berlin2q.dtsi
+++ b/arch/arm/boot/dts/berlin2q.dtsi
@@ -565,6 +565,14 @@
 						function = "twsi3";
 					};
 				};
+
+				adc: adc {
+					compatible = "marvell,berlin2-adc";
+					interrupt-parent = <&sic>;
+					interrupts = <12>, <14>;
+					interrupt-names = "adc", "tsen";
+					status = "disabled";
+				};
 			};
 
 			sic: interrupt-controller@e000 {
-- 
2.3.3

[PATCH 4/4] ARM: berlin: enable the ADC on the BG2Q DMP

[Antoine Tenart]

An ADC node is now available in the BG2Q SoC device tree. Enable it on
the BG2Q DMP board.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q-marvell-dmp.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/berlin2q-marvell-dmp.dts b/arch/arm/boot/dts/berlin2q-marvell-dmp.dts
index a98ac1bd8f65..a6de0039e5bc 100644
--- a/arch/arm/boot/dts/berlin2q-marvell-dmp.dts
+++ b/arch/arm/boot/dts/berlin2q-marvell-dmp.dts
@@ -112,3 +112,7 @@
 &sata_phy {
 	status = "okay";
 };
+
+&adc {
+	status = "okay";
+};
-- 
2.3.3

Re: [PATCH 1/4] iio: adc: add support for Berlin

[Peter Meerwald]

Hello Antoine,

> This patch adds the support of the Berlin ADC, available on Berlin SoCs.
> This ADC has 8 channels available, with one connected to a temperature
> sensor.
> 
> The particularity here, is that the temperature sensor connected to the
> ADC has its own registers, and both the ADC and the temperature sensor
> must be configured when using it.

some quick comments inline below;
sometimes this refers to berlin, sometimes to berlin2?

probably these regmap_read() / _write() pairs could be MACRO()'d away 
somehow

regards, p.

> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/iio/adc/Kconfig      |   7 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/berlin-adc.c | 397 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 405 insertions(+)
>  create mode 100644 drivers/iio/adc/berlin-adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 202daf889be2..2f10a6d8d442 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -135,6 +135,13 @@ config AXP288_ADC
>  	  device. Depending on platform configuration, this general purpose ADC can
>  	  be used for sampling sensors such as thermal resistors.
>  
> +config BERLIN_ADC
> +	tristate "Marvell Berlin ADC driver"
> +	depends on ARCH_BERLIN
> +	help
> +	  Marvell Berlin ADC driver. This ADC has 8 channels, with one used for
> +	  temparature measurement.

temperature

> +
>  config CC10001_ADC
>  	tristate "Cosmic Circuits 10001 ADC driver"
>  	depends on HAS_IOMEM || HAVE_CLK || REGULATOR
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 0315af640866..d7b2d1df2353 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
>  obj-$(CONFIG_AD799X) += ad799x.o
>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
>  obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> +obj-$(CONFIG_BERLIN_ADC) += berlin-adc.o
>  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> diff --git a/drivers/iio/adc/berlin-adc.c b/drivers/iio/adc/berlin-adc.c
> new file mode 100644
> index 000000000000..8cb0f5800511
> --- /dev/null
> +++ b/drivers/iio/adc/berlin-adc.c
> @@ -0,0 +1,397 @@
> +/*
> + * Marvell Berlin ADC driver
> + *
> + * Copyright (C) 2015 Marvell Technology Group Ltd.
> + *
> + * Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/machine.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/sched.h>
> +#include <linux/wait.h>
> +

the prefix SYSCTL prefix is unexpected, couldn't you use BERLIN_ or 
something?

> +#define SYSCTL_SM_CTRL				0x14
> +#define  SYSCTL_SM_CTRL_SM_SOC_INT		BIT(1)

whitespace issue?

> +#define  SYSCTL_SM_CTRL_SOC_SM_INT		BIT(2)
> +#define  SYSCTL_SM_CTRL_ADC_SEL(x)		(BIT(x) << 5)	/* 0-15 */
> +#define  SYSCTL_SM_CTRL_ADC_SEL_MASK		(0xf << 5)
> +#define  SYSCTL_SM_CTRL_ADC_POWER		BIT(9)
> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV2		(0x0 << 10)
> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV3		(0x1 << 10)
> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV4		(0x2 << 10)
> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV8		(0x3 << 10)
> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_MASK		(0x3 << 10)
> +#define  SYSCTL_SM_CTRL_ADC_START		BIT(12)
> +#define  SYSCTL_SM_CTRL_ADC_RESET		BIT(13)
> +#define  SYSCTL_SM_CTRL_ADC_BANDGAP_RDY		BIT(14)
> +#define  SYSCTL_SM_CTRL_ADC_CONT_SINGLE		(0x0 << 15)
> +#define  SYSCTL_SM_CTRL_ADC_CONT_CONTINUOUS	(0x1 << 15)
> +#define  SYSCTL_SM_CTRL_ADC_BUFFER_EN		BIT(16)
> +#define  SYSCTL_SM_CTRL_ADC_VREF_EXT		(0x0 << 17)
> +#define  SYSCTL_SM_CTRL_ADC_VREF_INT		(0x1 << 17)
> +#define  SYSCTL_SM_CTRL_ADC_ROTATE		BIT(19)
> +#define  SYSCTL_SM_CTRL_TSEN_EN			BIT(20)
> +#define  SYSCTL_SM_CTRL_TSEN_CLK_SEL_125	(0x0 << 21)	/* 1.25 MHz */
> +#define  SYSCTL_SM_CTRL_TSEN_CLK_SEL_250	(0x1 << 21)	/* 2.5 MHz */
> +#define  SYSCTL_SM_CTRL_TSEN_MODE_0_125		(0x0 << 22)	/* 0-125 C */
> +#define  SYSCTL_SM_CTRL_TSEN_MODE_10_50		(0x1 << 22)	/* 10-50 C */
> +#define  SYSCTL_SM_CTRL_TSEN_RESET		BIT(29)
> +#define SYSCTL_SM_ADC_DATA			0x20
> +#define  SYSCTL_SM_ADC_MASK			0x3ff
> +#define SYSCTL_SM_ADC_STATUS			0x1c
> +#define  SYSCTL_SM_ADC_STATUS_DATA_RDY(x)	BIT(x)		/* 0-15 */
> +#define  SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK	0xf
> +#define  SYSCTL_SM_ADC_STATUS_INT_EN(x)		(BIT(x) << 16)	/* 0-15 */
> +#define  SYSCTL_SM_ADC_STATUS_INT_EN_MASK	(0xf << 16)
> +#define SYSCTL_SM_TSEN_STATUS			0x24
> +#define  SYSCTL_SM_TSEN_STATUS_DATA_RDY		BIT(0)
> +#define  SYSCTL_SM_TSEN_STATUS_INT_EN		BIT(1)
> +#define SYSCTL_SM_TSEN_DATA			0x28
> +#define  SYSCTL_SM_TSEN_MASK			0x3ff
> +#define SYSCTL_SM_TSEN_CTRL			0x74
> +#define  SYSCTL_SM_TSEN_CTRL_START		BIT(8)
> +#define  SYSCTL_SM_TSEN_CTRL_SETTLING_4		(0x0 << 21)	/* 4 us */
> +#define  SYSCTL_SM_TSEN_CTRL_SETTLING_12	(0x1 << 21)	/* 12 us */
> +#define  SYSCTL_SM_TSEN_CTRL_TRIM(x)		((x) << 22)
> +#define  SYSCTL_SM_TSEN_CTRL_TRIM_MASK		(0xf << 22)
> +
> +struct berlin2_adc_priv {
> +	struct regmap		*regmap;
> +	struct mutex		lock;
> +	wait_queue_head_t	wq;
> +	u32			irq;
> +	u32			tsen_irq;

int rather

> +	bool			data_available;
> +	int			data;
> +};
> +
> +#define BERLIN2_ADC_CHANNEL(n, t)					\
> +		{							\
> +			.channel	= n,				\
> +			.datasheet_name	= "channel"#n,			\
> +			.type		= t,				\
> +			.indexed	= 1,				\
> +			.scan_index	= n,				\
> +			.scan_type	= {				\
> +				.sign		= 'u',			\
> +				.realbits	= 64,			\
> +				.storagebits	= 64,			\
> +			},						\

the data read is not 64 bit I think

the driver does not seem to support buffered reads, so scan_type and 
scan_index can be removed


> +			.info_mask_shared_by_type = 0,			\

.info_mask_shared_by_type = 0 is unnecessary

> +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +		}
> +
> +#define N_CHANNELS		8

not prefixed; would be good if you could do with ARRAY_SIZE over 
_adc_channels instead

> +static struct iio_chan_spec berlin2_adc_channels[] = {

why berlin2?

> +	BERLIN2_ADC_CHANNEL(0, IIO_VOLTAGE),	/* external input */
> +	BERLIN2_ADC_CHANNEL(1, IIO_VOLTAGE),	/* external input */
> +	BERLIN2_ADC_CHANNEL(2, IIO_VOLTAGE),	/* external input */
> +	BERLIN2_ADC_CHANNEL(3, IIO_VOLTAGE),	/* external input */
> +	BERLIN2_ADC_CHANNEL(4, IIO_VOLTAGE),	/* reserved */
> +	BERLIN2_ADC_CHANNEL(5, IIO_VOLTAGE),	/* reserved */
> +	BERLIN2_ADC_CHANNEL(6, IIO_TEMP),	/* temperature sensor */

the temperature channel is not indexed (there is only one)

> +	BERLIN2_ADC_CHANNEL(7, IIO_VOLTAGE),	/* reserved */
> +	{					/* timestamp */


use IIO_CHAN_SOFT_TIMESTAMP(N_CHANNELS) here

> +		.channel	= -1,
> +		.type		= IIO_TIMESTAMP,
> +		.scan_index	= N_CHANNELS,
> +		.scan_type	= {
> +			.sign		= 's',
> +			.realbits	= 64,
> +			.storagebits	= 64,
> +		},
> +	},
> +};
> +
> +static int berlin2_adc_read(struct iio_dev *idev, int channel)
> +{
> +	struct berlin2_adc_priv *priv = iio_priv(idev);
> +	int val, data, ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	/* Configure the ADC */
> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
> +	val &= ~(SYSCTL_SM_CTRL_ADC_RESET | SYSCTL_SM_CTRL_ADC_SEL_MASK);
> +	val |= SYSCTL_SM_CTRL_ADC_SEL(channel) | SYSCTL_SM_CTRL_ADC_START;
> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
> +
> +	/* Enable the interrupts */
> +	regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS,
> +			SYSCTL_SM_ADC_STATUS_INT_EN(channel));
> +
> +	ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
> +			msecs_to_jiffies(1000));
> +
> +	/* Disable the interrupts */
> +	regmap_read(priv->regmap, SYSCTL_SM_ADC_STATUS, &val);
> +	val &= ~SYSCTL_SM_ADC_STATUS_INT_EN(channel);
> +	regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS, val);
> +
> +	if (ret == 0)
> +		ret = -ETIMEDOUT;
> +	if (ret < 0) {
> +		mutex_unlock(&priv->lock);
> +		return ret;
> +	}
> +
> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
> +	val &= ~SYSCTL_SM_CTRL_ADC_START;
> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
> +
> +	data = priv->data;
> +	priv->data = -1;
> +	priv->data_available = false;
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return data;
> +}
> +
> +static int berlin2_adc_tsen_read(struct iio_dev *idev)
> +{
> +	struct berlin2_adc_priv *priv = iio_priv(idev);
> +	int val, data, ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	/* Configure the ADC */
> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
> +	val &= ~SYSCTL_SM_CTRL_TSEN_RESET;
> +	val |= SYSCTL_SM_CTRL_ADC_ROTATE;
> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
> +
> +	/* Configure the temperature sensor */
> +	regmap_read(priv->regmap, SYSCTL_SM_TSEN_CTRL, &val);
> +	val &= ~SYSCTL_SM_TSEN_CTRL_TRIM_MASK;
> +	val |= SYSCTL_SM_TSEN_CTRL_TRIM(3) | SYSCTL_SM_TSEN_CTRL_SETTLING_12
> +		| SYSCTL_SM_TSEN_CTRL_START;
> +	regmap_write(priv->regmap, SYSCTL_SM_TSEN_CTRL, val);
> +
> +	/* Enable interrupts */
> +	regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS,
> +			SYSCTL_SM_TSEN_STATUS_INT_EN);
> +
> +	ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
> +			msecs_to_jiffies(1000));
> +
> +	/* Disable interrupts */
> +	regmap_read(priv->regmap, SYSCTL_SM_TSEN_STATUS, &val);
> +	val &= ~SYSCTL_SM_TSEN_STATUS_INT_EN;
> +	regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS, val);
> +
> +	if (ret == 0)
> +		ret = -ETIMEDOUT;
> +	if (ret < 0) {
> +		mutex_unlock(&priv->lock);
> +		return ret;
> +	}
> +
> +	regmap_read(priv->regmap, SYSCTL_SM_TSEN_CTRL, &val);
> +	val &= ~SYSCTL_SM_TSEN_CTRL_START;
> +	regmap_write(priv->regmap, SYSCTL_SM_TSEN_CTRL, val);
> +
> +	data = priv->data;
> +	priv->data = -1;
> +	priv->data_available = false;
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return data;
> +}
> +
> +static int berlin2_adc_read_raw(struct iio_dev *idev,
> +		struct iio_chan_spec const *chan, int *val, int *val2,
> +		long mask)
> +{
> +	int temp;

maybe named it ret and use instead of *val and temp

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (chan->type == IIO_VOLTAGE) {
> +			*val = berlin2_adc_read(idev, chan->channel);
> +			if (*val < 0)
> +				return *val;

is this milli-Volts?

> +
> +			return IIO_VAL_INT;
> +		} else if (chan->type == IIO_TEMP) {
> +			temp = berlin2_adc_tsen_read(idev);
> +			if (temp < 0)
> +				return temp;
> +
> +			if (temp > 2047)
> +				temp = -(4096 - temp);
> +
> +			/* Convert to Celsius */
> +			*val = (temp * 100) / 264 - 270;

use SCALE and OFFSET IIO_CHAN_INFO_s to that the temperature can be 
interpreted as milli-Celsius (or use _PROCESSED, not _RAW)

> +			return IIO_VAL_INT;
> +		}
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static irqreturn_t berlin2_adc_irq(int irq, void *private)
> +{
> +	struct iio_dev *idev = private;
> +	struct berlin2_adc_priv *priv = iio_priv(idev);
> +	unsigned val;
> +
> +	regmap_read(priv->regmap, SYSCTL_SM_ADC_STATUS, &val);
> +	if (val & SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK) {
> +		regmap_read(priv->regmap, SYSCTL_SM_ADC_DATA, &priv->data);
> +		priv->data &= SYSCTL_SM_ADC_MASK;
> +
> +		val &= ~SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK;
> +		regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS, val);
> +
> +		priv->data_available = true;
> +		wake_up_interruptible(&priv->wq);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t berlin2_adc_tsen_irq(int irq, void *private)
> +{
> +	struct iio_dev *idev = private;
> +	struct berlin2_adc_priv *priv = iio_priv(idev);
> +	unsigned val;
> +
> +	regmap_read(priv->regmap, SYSCTL_SM_TSEN_STATUS, &val);
> +	if (val & SYSCTL_SM_TSEN_STATUS_DATA_RDY) {
> +		regmap_read(priv->regmap, SYSCTL_SM_TSEN_DATA, &priv->data);
> +		priv->data &= SYSCTL_SM_TSEN_MASK;
> +
> +		val &= ~SYSCTL_SM_TSEN_STATUS_DATA_RDY;
> +		regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS, val);
> +
> +		priv->data_available = true;
> +		wake_up_interruptible(&priv->wq);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info berlin2_adc_info = {
> +	.driver_module	= THIS_MODULE,
> +	.read_raw	= berlin2_adc_read_raw,
> +};
> +
> +static int berlin2_adc_probe(struct platform_device *pdev)
> +{
> +	struct iio_dev *idev;

people prefer indio_dev instead of idev

> +	struct berlin2_adc_priv *priv;
> +	struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
> +	int ret, val;
> +
> +	idev = iio_device_alloc(sizeof(struct berlin2_adc_priv));

devm_iio_device_alloc?

> +	if (!idev)
> +		return -ENOMEM;
> +
> +	priv = iio_priv(idev);
> +	platform_set_drvdata(pdev, idev);
> +
> +	priv->regmap = syscon_node_to_regmap(parent_np);
> +	of_node_put(parent_np);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +
> +	priv->irq = platform_get_irq_byname(pdev, "adc");
> +	if (priv->irq < 0)

you have irq as u32, should be int otherwise the check does not make sense

> +		return -ENODEV;
> +
> +	priv->tsen_irq = platform_get_irq_byname(pdev, "tsen");
> +	if (priv->tsen_irq < 0)
> +		return -ENODEV;
> +
> +	ret = devm_request_irq(&pdev->dev, priv->irq, berlin2_adc_irq, 0,
> +			pdev->dev.driver->name, idev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_irq(&pdev->dev, priv->tsen_irq, berlin2_adc_tsen_irq,
> +			0, pdev->dev.driver->name, idev);
> +	if (ret)
> +		return ret;
> +
> +	init_waitqueue_head(&priv->wq);
> +	mutex_init(&priv->lock);
> +
> +	idev->dev.parent = &pdev->dev;
> +	idev->name = dev_name(&pdev->dev);
> +	idev->modes = INDIO_DIRECT_MODE;
> +	idev->info = &berlin2_adc_info;
> +
> +	idev->num_channels = N_CHANNELS;
> +	idev->channels = berlin2_adc_channels;
> +
> +	/* Power up the ADC */
> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
> +	val |= SYSCTL_SM_CTRL_ADC_POWER;
> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
> +
> +	ret = iio_device_register(idev);

devm_iio_device_register?

> +	if (ret) {
> +		dev_err(&pdev->dev,  "Failed to register the IIO device: %d\n",
> +			ret);

probably not the most useful error msg and about the only logging; drop 
it?

and just do 
return devm_iio_device_register(idev);

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int berlin2_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *idev = platform_get_drvdata(pdev);
> +	struct berlin2_adc_priv *priv = iio_priv(idev);
> +	int val;
> +
> +	/* Power down the ADC */
> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
> +	val &= ~SYSCTL_SM_CTRL_ADC_POWER;
> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
> +
> +	free_irq(priv->irq, idev);
> +	free_irq(priv->tsen_irq, idev);
> +
> +	iio_device_unregister(idev);
> +	iio_device_free(idev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id berlin2_adc_match[] = {
> +	{ .compatible = "marvell,berlin2-adc", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, berlin2q_adc_match);
> +
> +static struct platform_driver berlin2_adc_driver = {
> +	.driver	= {
> +		.name		= "berlin2-adc",
> +		.of_match_table	= berlin2_adc_match,
> +	},
> +	.probe	= berlin2_adc_probe,
> +	.remove	= berlin2_adc_remove,
> +};
> +module_platform_driver(berlin2_adc_driver);
> +
> +MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
> +MODULE_DESCRIPTION("Marvell Berlin2 ADC driver");
> +MODULE_LICENSE("GPL");
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 1/4] iio: adc: add support for Berlin

[Lars-Peter Clausen]

On 03/20/2015 06:43 PM, Peter Meerwald wrote:
> Hello Antoine,
>
>> This patch adds the support of the Berlin ADC, available on Berlin SoCs.
>> This ADC has 8 channels available, with one connected to a temperature
>> sensor.
>>
>> The particularity here, is that the temperature sensor connected to the
>> ADC has its own registers, and both the ADC and the temperature sensor
>> must be configured when using it.
>
> some quick comments inline below;
> sometimes this refers to berlin, sometimes to berlin2?
>
> probably these regmap_read() / _write() pairs could be MACRO()'d away
> somehow

There is regmap_update_bits(), which does the read-modify-update cycle.

Re: [PATCH 1/4] iio: adc: add support for Berlin

[Paul Bolle]

Just a license nit.

On Fri, 2015-03-20 at 14:36 +0100, Antoine Tenart wrote:
> --- /dev/null
> +++ b/drivers/iio/adc/berlin-adc.c

> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */

This states the license is GPL v2.

> +MODULE_LICENSE("GPL");

And
    MODULE_LICENSE("GPL v2");

would match that statement.


Paul Bolle

Re: [PATCH 1/4] iio: adc: add support for Berlin

[Jonathan Cameron]

On 20/03/15 17:43, Peter Meerwald wrote:
> Hello Antoine,
> 
>> This patch adds the support of the Berlin ADC, available on Berlin SoCs.
>> This ADC has 8 channels available, with one connected to a temperature
>> sensor.
>>
>> The particularity here, is that the temperature sensor connected to the
>> ADC has its own registers, and both the ADC and the temperature sensor
>> must be configured when using it.
> 
> some quick comments inline below;
> sometimes this refers to berlin, sometimes to berlin2?
> 
> probably these regmap_read() / _write() pairs could be MACRO()'d away 
> somehow
> 
> regards, p.
> 
>> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
>> ---
>>  drivers/iio/adc/Kconfig      |   7 +
>>  drivers/iio/adc/Makefile     |   1 +
>>  drivers/iio/adc/berlin-adc.c | 397 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 405 insertions(+)
>>  create mode 100644 drivers/iio/adc/berlin-adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 202daf889be2..2f10a6d8d442 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -135,6 +135,13 @@ config AXP288_ADC
>>  	  device. Depending on platform configuration, this general purpose ADC can
>>  	  be used for sampling sensors such as thermal resistors.
>>  
>> +config BERLIN_ADC
>> +	tristate "Marvell Berlin ADC driver"
>> +	depends on ARCH_BERLIN
>> +	help
>> +	  Marvell Berlin ADC driver. This ADC has 8 channels, with one used for
>> +	  temparature measurement.
> 
> temperature
> 
>> +
>>  config CC10001_ADC
>>  	tristate "Cosmic Circuits 10001 ADC driver"
>>  	depends on HAS_IOMEM || HAVE_CLK || REGULATOR
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 0315af640866..d7b2d1df2353 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
>>  obj-$(CONFIG_AD799X) += ad799x.o
>>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
>>  obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>> +obj-$(CONFIG_BERLIN_ADC) += berlin-adc.o
>>  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>> diff --git a/drivers/iio/adc/berlin-adc.c b/drivers/iio/adc/berlin-adc.c
>> new file mode 100644
>> index 000000000000..8cb0f5800511
>> --- /dev/null
>> +++ b/drivers/iio/adc/berlin-adc.c
>> @@ -0,0 +1,397 @@
>> +/*
>> + * Marvell Berlin ADC driver
>> + *
>> + * Copyright (C) 2015 Marvell Technology Group Ltd.
>> + *
>> + * Antoine Tenart <antoine.tenart@free-electrons.com>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/driver.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>> +#include <linux/sched.h>
>> +#include <linux/wait.h>
>> +
> 
> the prefix SYSCTL prefix is unexpected, couldn't you use BERLIN_ or 
> something?
> 
>> +#define SYSCTL_SM_CTRL				0x14
>> +#define  SYSCTL_SM_CTRL_SM_SOC_INT		BIT(1)
> 
> whitespace issue?
> 
>> +#define  SYSCTL_SM_CTRL_SOC_SM_INT		BIT(2)
>> +#define  SYSCTL_SM_CTRL_ADC_SEL(x)		(BIT(x) << 5)	/* 0-15 */
>> +#define  SYSCTL_SM_CTRL_ADC_SEL_MASK		(0xf << 5)
>> +#define  SYSCTL_SM_CTRL_ADC_POWER		BIT(9)
>> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV2		(0x0 << 10)
>> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV3		(0x1 << 10)
>> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV4		(0x2 << 10)
>> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV8		(0x3 << 10)
>> +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_MASK		(0x3 << 10)
>> +#define  SYSCTL_SM_CTRL_ADC_START		BIT(12)
>> +#define  SYSCTL_SM_CTRL_ADC_RESET		BIT(13)
>> +#define  SYSCTL_SM_CTRL_ADC_BANDGAP_RDY		BIT(14)
>> +#define  SYSCTL_SM_CTRL_ADC_CONT_SINGLE		(0x0 << 15)
>> +#define  SYSCTL_SM_CTRL_ADC_CONT_CONTINUOUS	(0x1 << 15)
>> +#define  SYSCTL_SM_CTRL_ADC_BUFFER_EN		BIT(16)
>> +#define  SYSCTL_SM_CTRL_ADC_VREF_EXT		(0x0 << 17)
>> +#define  SYSCTL_SM_CTRL_ADC_VREF_INT		(0x1 << 17)
>> +#define  SYSCTL_SM_CTRL_ADC_ROTATE		BIT(19)
>> +#define  SYSCTL_SM_CTRL_TSEN_EN			BIT(20)
>> +#define  SYSCTL_SM_CTRL_TSEN_CLK_SEL_125	(0x0 << 21)	/* 1.25 MHz */
>> +#define  SYSCTL_SM_CTRL_TSEN_CLK_SEL_250	(0x1 << 21)	/* 2.5 MHz */
>> +#define  SYSCTL_SM_CTRL_TSEN_MODE_0_125		(0x0 << 22)	/* 0-125 C */
>> +#define  SYSCTL_SM_CTRL_TSEN_MODE_10_50		(0x1 << 22)	/* 10-50 C */
>> +#define  SYSCTL_SM_CTRL_TSEN_RESET		BIT(29)
>> +#define SYSCTL_SM_ADC_DATA			0x20
>> +#define  SYSCTL_SM_ADC_MASK			0x3ff
>> +#define SYSCTL_SM_ADC_STATUS			0x1c
>> +#define  SYSCTL_SM_ADC_STATUS_DATA_RDY(x)	BIT(x)		/* 0-15 */
>> +#define  SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK	0xf
>> +#define  SYSCTL_SM_ADC_STATUS_INT_EN(x)		(BIT(x) << 16)	/* 0-15 */
>> +#define  SYSCTL_SM_ADC_STATUS_INT_EN_MASK	(0xf << 16)
>> +#define SYSCTL_SM_TSEN_STATUS			0x24
>> +#define  SYSCTL_SM_TSEN_STATUS_DATA_RDY		BIT(0)
>> +#define  SYSCTL_SM_TSEN_STATUS_INT_EN		BIT(1)
>> +#define SYSCTL_SM_TSEN_DATA			0x28
>> +#define  SYSCTL_SM_TSEN_MASK			0x3ff
>> +#define SYSCTL_SM_TSEN_CTRL			0x74
>> +#define  SYSCTL_SM_TSEN_CTRL_START		BIT(8)
>> +#define  SYSCTL_SM_TSEN_CTRL_SETTLING_4		(0x0 << 21)	/* 4 us */
>> +#define  SYSCTL_SM_TSEN_CTRL_SETTLING_12	(0x1 << 21)	/* 12 us */
>> +#define  SYSCTL_SM_TSEN_CTRL_TRIM(x)		((x) << 22)
>> +#define  SYSCTL_SM_TSEN_CTRL_TRIM_MASK		(0xf << 22)
>> +
>> +struct berlin2_adc_priv {
>> +	struct regmap		*regmap;
>> +	struct mutex		lock;
>> +	wait_queue_head_t	wq;
>> +	u32			irq;
>> +	u32			tsen_irq;
> 
> int rather
> 
>> +	bool			data_available;
>> +	int			data;
>> +};
>> +
>> +#define BERLIN2_ADC_CHANNEL(n, t)					\
>> +		{							\
>> +			.channel	= n,				\
>> +			.datasheet_name	= "channel"#n,			\
>> +			.type		= t,				\
>> +			.indexed	= 1,				\
>> +			.scan_index	= n,				\
>> +			.scan_type	= {				\
>> +				.sign		= 'u',			\
>> +				.realbits	= 64,			\
>> +				.storagebits	= 64,			\
>> +			},						\
> 
> the data read is not 64 bit I think
> 
> the driver does not seem to support buffered reads, so scan_type and 
> scan_index can be removed
> 
> 
>> +			.info_mask_shared_by_type = 0,			\
> 
> .info_mask_shared_by_type = 0 is unnecessary
> 
>> +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
>> +		}
>> +
>> +#define N_CHANNELS		8
> 
> not prefixed; would be good if you could do with ARRAY_SIZE over 
> _adc_channels instead
> 
>> +static struct iio_chan_spec berlin2_adc_channels[] = {
> 
> why berlin2?
> 
>> +	BERLIN2_ADC_CHANNEL(0, IIO_VOLTAGE),	/* external input */
>> +	BERLIN2_ADC_CHANNEL(1, IIO_VOLTAGE),	/* external input */
>> +	BERLIN2_ADC_CHANNEL(2, IIO_VOLTAGE),	/* external input */
>> +	BERLIN2_ADC_CHANNEL(3, IIO_VOLTAGE),	/* external input */
>> +	BERLIN2_ADC_CHANNEL(4, IIO_VOLTAGE),	/* reserved */
>> +	BERLIN2_ADC_CHANNEL(5, IIO_VOLTAGE),	/* reserved */
>> +	BERLIN2_ADC_CHANNEL(6, IIO_TEMP),	/* temperature sensor */
> 
> the temperature channel is not indexed (there is only one)
> 
>> +	BERLIN2_ADC_CHANNEL(7, IIO_VOLTAGE),	/* reserved */
>> +	{					/* timestamp */
> 
> 
> use IIO_CHAN_SOFT_TIMESTAMP(N_CHANNELS) here
> 
>> +		.channel	= -1,
>> +		.type		= IIO_TIMESTAMP,
>> +		.scan_index	= N_CHANNELS,
>> +		.scan_type	= {
>> +			.sign		= 's',
>> +			.realbits	= 64,
>> +			.storagebits	= 64,
>> +		},
>> +	},
>> +};
>> +
>> +static int berlin2_adc_read(struct iio_dev *idev, int channel)
>> +{
>> +	struct berlin2_adc_priv *priv = iio_priv(idev);
>> +	int val, data, ret;
>> +
>> +	mutex_lock(&priv->lock);
>> +
>> +	/* Configure the ADC */
>> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> +	val &= ~(SYSCTL_SM_CTRL_ADC_RESET | SYSCTL_SM_CTRL_ADC_SEL_MASK);
>> +	val |= SYSCTL_SM_CTRL_ADC_SEL(channel) | SYSCTL_SM_CTRL_ADC_START;
>> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> +	/* Enable the interrupts */
>> +	regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS,
>> +			SYSCTL_SM_ADC_STATUS_INT_EN(channel));
>> +
>> +	ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
>> +			msecs_to_jiffies(1000));
>> +
>> +	/* Disable the interrupts */
>> +	regmap_read(priv->regmap, SYSCTL_SM_ADC_STATUS, &val);
>> +	val &= ~SYSCTL_SM_ADC_STATUS_INT_EN(channel);
>> +	regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS, val);
>> +
>> +	if (ret == 0)
>> +		ret = -ETIMEDOUT;
>> +	if (ret < 0) {
>> +		mutex_unlock(&priv->lock);
>> +		return ret;
>> +	}
>> +
>> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> +	val &= ~SYSCTL_SM_CTRL_ADC_START;
>> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> +	data = priv->data;
>> +	priv->data = -1;
>> +	priv->data_available = false;
>> +
>> +	mutex_unlock(&priv->lock);
>> +
>> +	return data;
>> +}
>> +
>> +static int berlin2_adc_tsen_read(struct iio_dev *idev)
>> +{
>> +	struct berlin2_adc_priv *priv = iio_priv(idev);
>> +	int val, data, ret;
>> +
>> +	mutex_lock(&priv->lock);
>> +
>> +	/* Configure the ADC */
>> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> +	val &= ~SYSCTL_SM_CTRL_TSEN_RESET;
>> +	val |= SYSCTL_SM_CTRL_ADC_ROTATE;
>> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> +	/* Configure the temperature sensor */
>> +	regmap_read(priv->regmap, SYSCTL_SM_TSEN_CTRL, &val);
>> +	val &= ~SYSCTL_SM_TSEN_CTRL_TRIM_MASK;
>> +	val |= SYSCTL_SM_TSEN_CTRL_TRIM(3) | SYSCTL_SM_TSEN_CTRL_SETTLING_12
>> +		| SYSCTL_SM_TSEN_CTRL_START;
>> +	regmap_write(priv->regmap, SYSCTL_SM_TSEN_CTRL, val);
>> +
>> +	/* Enable interrupts */
>> +	regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS,
>> +			SYSCTL_SM_TSEN_STATUS_INT_EN);
>> +
>> +	ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
>> +			msecs_to_jiffies(1000));
>> +
>> +	/* Disable interrupts */
>> +	regmap_read(priv->regmap, SYSCTL_SM_TSEN_STATUS, &val);
>> +	val &= ~SYSCTL_SM_TSEN_STATUS_INT_EN;
>> +	regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS, val);
>> +
>> +	if (ret == 0)
>> +		ret = -ETIMEDOUT;
>> +	if (ret < 0) {
>> +		mutex_unlock(&priv->lock);
>> +		return ret;
>> +	}
>> +
>> +	regmap_read(priv->regmap, SYSCTL_SM_TSEN_CTRL, &val);
>> +	val &= ~SYSCTL_SM_TSEN_CTRL_START;
>> +	regmap_write(priv->regmap, SYSCTL_SM_TSEN_CTRL, val);
>> +
>> +	data = priv->data;
>> +	priv->data = -1;
>> +	priv->data_available = false;
>> +
>> +	mutex_unlock(&priv->lock);
>> +
>> +	return data;
>> +}
>> +
>> +static int berlin2_adc_read_raw(struct iio_dev *idev,
>> +		struct iio_chan_spec const *chan, int *val, int *val2,
>> +		long mask)
>> +{
>> +	int temp;
> 
> maybe named it ret and use instead of *val and temp
> 
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		if (chan->type == IIO_VOLTAGE) {
>> +			*val = berlin2_adc_read(idev, chan->channel);
>> +			if (*val < 0)
>> +				return *val;
> 
> is this milli-Volts?
> 
>> +
>> +			return IIO_VAL_INT;
>> +		} else if (chan->type == IIO_TEMP) {
>> +			temp = berlin2_adc_tsen_read(idev);
>> +			if (temp < 0)
>> +				return temp;
>> +
>> +			if (temp > 2047)
>> +				temp = -(4096 - temp);
>> +
>> +			/* Convert to Celsius */
>> +			*val = (temp * 100) / 264 - 270;
> 
> use SCALE and OFFSET IIO_CHAN_INFO_s to that the temperature can be 
> interpreted as milli-Celsius (or use _PROCESSED, not _RAW)
> 
>> +			return IIO_VAL_INT;
>> +		}
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static irqreturn_t berlin2_adc_irq(int irq, void *private)
>> +{
>> +	struct iio_dev *idev = private;
>> +	struct berlin2_adc_priv *priv = iio_priv(idev);
>> +	unsigned val;
>> +
>> +	regmap_read(priv->regmap, SYSCTL_SM_ADC_STATUS, &val);
>> +	if (val & SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK) {
>> +		regmap_read(priv->regmap, SYSCTL_SM_ADC_DATA, &priv->data);
>> +		priv->data &= SYSCTL_SM_ADC_MASK;
>> +
>> +		val &= ~SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK;
>> +		regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS, val);
>> +
>> +		priv->data_available = true;
>> +		wake_up_interruptible(&priv->wq);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t berlin2_adc_tsen_irq(int irq, void *private)
>> +{
>> +	struct iio_dev *idev = private;
>> +	struct berlin2_adc_priv *priv = iio_priv(idev);
>> +	unsigned val;
>> +
>> +	regmap_read(priv->regmap, SYSCTL_SM_TSEN_STATUS, &val);
>> +	if (val & SYSCTL_SM_TSEN_STATUS_DATA_RDY) {
>> +		regmap_read(priv->regmap, SYSCTL_SM_TSEN_DATA, &priv->data);
>> +		priv->data &= SYSCTL_SM_TSEN_MASK;
>> +
>> +		val &= ~SYSCTL_SM_TSEN_STATUS_DATA_RDY;
>> +		regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS, val);
>> +
>> +		priv->data_available = true;
>> +		wake_up_interruptible(&priv->wq);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static const struct iio_info berlin2_adc_info = {
>> +	.driver_module	= THIS_MODULE,
>> +	.read_raw	= berlin2_adc_read_raw,
>> +};
>> +
>> +static int berlin2_adc_probe(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *idev;
> 
> people prefer indio_dev instead of idev
> 
>> +	struct berlin2_adc_priv *priv;
>> +	struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
>> +	int ret, val;
>> +
>> +	idev = iio_device_alloc(sizeof(struct berlin2_adc_priv));
> 
> devm_iio_device_alloc?
> 
>> +	if (!idev)
>> +		return -ENOMEM;
>> +
>> +	priv = iio_priv(idev);
>> +	platform_set_drvdata(pdev, idev);
>> +
>> +	priv->regmap = syscon_node_to_regmap(parent_np);
>> +	of_node_put(parent_np);
>> +	if (IS_ERR(priv->regmap))
>> +		return PTR_ERR(priv->regmap);
>> +
>> +	priv->irq = platform_get_irq_byname(pdev, "adc");
>> +	if (priv->irq < 0)
> 
> you have irq as u32, should be int otherwise the check does not make sense
> 
>> +		return -ENODEV;
>> +
>> +	priv->tsen_irq = platform_get_irq_byname(pdev, "tsen");
>> +	if (priv->tsen_irq < 0)
>> +		return -ENODEV;
>> +
>> +	ret = devm_request_irq(&pdev->dev, priv->irq, berlin2_adc_irq, 0,
>> +			pdev->dev.driver->name, idev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_request_irq(&pdev->dev, priv->tsen_irq, berlin2_adc_tsen_irq,
>> +			0, pdev->dev.driver->name, idev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	init_waitqueue_head(&priv->wq);
>> +	mutex_init(&priv->lock);
>> +
>> +	idev->dev.parent = &pdev->dev;
>> +	idev->name = dev_name(&pdev->dev);
>> +	idev->modes = INDIO_DIRECT_MODE;
>> +	idev->info = &berlin2_adc_info;
>> +
>> +	idev->num_channels = N_CHANNELS;
>> +	idev->channels = berlin2_adc_channels;
>> +
>> +	/* Power up the ADC */
>> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> +	val |= SYSCTL_SM_CTRL_ADC_POWER;
>> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> +	ret = iio_device_register(idev);
> 
> devm_iio_device_register?
> 
>> +	if (ret) {
>> +		dev_err(&pdev->dev,  "Failed to register the IIO device: %d\n",
>> +			ret);
> 
> probably not the most useful error msg and about the only logging; drop 
> it?
> 
> and just do 
> return devm_iio_device_register(idev);
There is an ordering issue here that makes that look sensible, but it isn't.
Rule of thumb. If there is anything else in the remove, you shouldn't
use devm_iio_device_register (same is often true of the interrupt equivalent
though occasionally the ordering is such that using the devm versions
of those is fine.)
> 
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int berlin2_adc_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *idev = platform_get_drvdata(pdev);
>> +	struct berlin2_adc_priv *priv = iio_priv(idev);
>> +	int val;
>> +
>> +	/* Power down the ADC */
>> +	regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> +	val &= ~SYSCTL_SM_CTRL_ADC_POWER;
>> +	regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> +	free_irq(priv->irq, idev);
>> +	free_irq(priv->tsen_irq, idev);
>> +
>> +	iio_device_unregister(idev);
You are removing the userspace interface only after you have
turned the device off and disabled the irq's etc. Not a good
plan.  There is a reason why most remove functions are the
reverse of the probe (in what they do and in the order they do
it in). Leads to far fewer issues like this and makes them much
easier to review!

>> +	iio_device_free(idev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id berlin2_adc_match[] = {
>> +	{ .compatible = "marvell,berlin2-adc", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, berlin2q_adc_match);
>> +
>> +static struct platform_driver berlin2_adc_driver = {
>> +	.driver	= {
>> +		.name		= "berlin2-adc",
>> +		.of_match_table	= berlin2_adc_match,
>> +	},
>> +	.probe	= berlin2_adc_probe,
>> +	.remove	= berlin2_adc_remove,
>> +};
>> +module_platform_driver(berlin2_adc_driver);
>> +
>> +MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
>> +MODULE_DESCRIPTION("Marvell Berlin2 ADC driver");
>> +MODULE_LICENSE("GPL");
>>
> 

Re: [PATCH 1/4] iio: adc: add support for Berlin

[Antoine Tenart]

Peter,

On Fri, Mar 20, 2015 at 06:43:15PM +0100, Peter Meerwald wrote:
>
> probably these regmap_read() / _write() pairs could be MACRO()'d away 
> somehow

Sure.

> 
> the prefix SYSCTL prefix is unexpected, couldn't you use BERLIN_ or 
> something?

I'll use BERLIN2_ instead of SYSCTL_

> 
> > +#define SYSCTL_SM_CTRL				0x14
> > +#define  SYSCTL_SM_CTRL_SM_SOC_INT		BIT(1)
> 
> whitespace issue?

I added a whitespace to put the bit definitions of a register together,
so that the reading is easier.

> 
> > +#define  SYSCTL_SM_CTRL_SOC_SM_INT		BIT(2)
> > +#define  SYSCTL_SM_CTRL_ADC_SEL(x)		(BIT(x) << 5)	/* 0-15 */
> > +#define  SYSCTL_SM_CTRL_ADC_SEL_MASK		(0xf << 5)
> > +#define  SYSCTL_SM_CTRL_ADC_POWER		BIT(9)
> > +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV2		(0x0 << 10)
> > +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV3		(0x1 << 10)
> > +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV4		(0x2 << 10)
> > +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_DIV8		(0x3 << 10)
> > +#define  SYSCTL_SM_CTRL_ADC_CLKSEL_MASK		(0x3 << 10)
> > +#define  SYSCTL_SM_CTRL_ADC_START		BIT(12)
> > +#define  SYSCTL_SM_CTRL_ADC_RESET		BIT(13)
> > +#define  SYSCTL_SM_CTRL_ADC_BANDGAP_RDY		BIT(14)
> > +#define  SYSCTL_SM_CTRL_ADC_CONT_SINGLE		(0x0 << 15)
> > +#define  SYSCTL_SM_CTRL_ADC_CONT_CONTINUOUS	(0x1 << 15)
> > +#define  SYSCTL_SM_CTRL_ADC_BUFFER_EN		BIT(16)
> > +#define  SYSCTL_SM_CTRL_ADC_VREF_EXT		(0x0 << 17)
> > +#define  SYSCTL_SM_CTRL_ADC_VREF_INT		(0x1 << 17)
> > +#define  SYSCTL_SM_CTRL_ADC_ROTATE		BIT(19)
> > +#define  SYSCTL_SM_CTRL_TSEN_EN			BIT(20)
> > +#define  SYSCTL_SM_CTRL_TSEN_CLK_SEL_125	(0x0 << 21)	/* 1.25 MHz */
> > +#define  SYSCTL_SM_CTRL_TSEN_CLK_SEL_250	(0x1 << 21)	/* 2.5 MHz */
> > +#define  SYSCTL_SM_CTRL_TSEN_MODE_0_125		(0x0 << 22)	/* 0-125 C */
> > +#define  SYSCTL_SM_CTRL_TSEN_MODE_10_50		(0x1 << 22)	/* 10-50 C */

> > +		{							\
> > +			.channel	= n,				\
> > +			.datasheet_name	= "channel"#n,			\
> > +			.type		= t,				\
> > +			.indexed	= 1,				\
> > +			.scan_index	= n,				\
> > +			.scan_type	= {				\
> > +				.sign		= 'u',			\
> > +				.realbits	= 64,			\
> > +				.storagebits	= 64,			\
> > +			},						\
> 
> the data read is not 64 bit I think
> 
> the driver does not seem to support buffered reads, so scan_type and 
> scan_index can be removed

OK.

> > +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> > +		}
> > +
> > +#define N_CHANNELS		8
> 
> not prefixed; would be good if you could do with ARRAY_SIZE over 
> _adc_channels instead

OK.

> 
> > +	BERLIN2_ADC_CHANNEL(0, IIO_VOLTAGE),	/* external input */
> > +	BERLIN2_ADC_CHANNEL(1, IIO_VOLTAGE),	/* external input */
> > +	BERLIN2_ADC_CHANNEL(2, IIO_VOLTAGE),	/* external input */
> > +	BERLIN2_ADC_CHANNEL(3, IIO_VOLTAGE),	/* external input */
> > +	BERLIN2_ADC_CHANNEL(4, IIO_VOLTAGE),	/* reserved */
> > +	BERLIN2_ADC_CHANNEL(5, IIO_VOLTAGE),	/* reserved */
> > +	BERLIN2_ADC_CHANNEL(6, IIO_TEMP),	/* temperature sensor */
> 
> the temperature channel is not indexed (there is only one)

I'll update.

> 
> > +	BERLIN2_ADC_CHANNEL(7, IIO_VOLTAGE),	/* reserved */
> > +	{					/* timestamp */
> 
> 
> use IIO_CHAN_SOFT_TIMESTAMP(N_CHANNELS) here

Oh, nice! I'll update.

> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (chan->type == IIO_VOLTAGE) {
> > +			*val = berlin2_adc_read(idev, chan->channel);
> > +			if (*val < 0)
> > +				return *val;
> 
> is this milli-Volts?

Good question. I think so, but I do not have much information about
the !tsen channels.

> 
> > +
> > +			return IIO_VAL_INT;
> > +		} else if (chan->type == IIO_TEMP) {
> > +			temp = berlin2_adc_tsen_read(idev);
> > +			if (temp < 0)
> > +				return temp;
> > +
> > +			if (temp > 2047)
> > +				temp = -(4096 - temp);
> > +
> > +			/* Convert to Celsius */
> > +			*val = (temp * 100) / 264 - 270;
> 
> use SCALE and OFFSET IIO_CHAN_INFO_s to that the temperature can be 
> interpreted as milli-Celsius (or use _PROCESSED, not _RAW)

I'll fix that.

> > +
> > +static int berlin2_adc_probe(struct platform_device *pdev)
> > +{
> > +	struct iio_dev *idev;
> 
> people prefer indio_dev instead of idev

OK.

> 
> > +	struct berlin2_adc_priv *priv;
> > +	struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
> > +	int ret, val;
> > +
> > +	idev = iio_device_alloc(sizeof(struct berlin2_adc_priv));
> 
> devm_iio_device_alloc?

Yep, that's better.

> > +
> > +	ret = iio_device_register(idev);
> 
> devm_iio_device_register?

Ditto.

> 
> > +	if (ret) {
> > +		dev_err(&pdev->dev,  "Failed to register the IIO device: %d\n",
> > +			ret);
> 
> probably not the most useful error msg and about the only logging; drop 
> it?
> 
> and just do 
> return devm_iio_device_register(idev);

OK.


Thanks for the review!

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com