[PATCH v2 0/4] Add i.mx7d adc driver support

[PATCH v2 0/4] Add i.mx7d adc driver support

[Haibo Chen]

This patch add imx7d adc driver support.

Changes in V2:
-prefix defines with IMX7D_ for all the register
-use BIT macro to define a single bit
-remove the dma_en from struct adc_feature which is not support currently
-use static const array to replace the switch case code

Haibo Chen (4):
  iio: adc: add IMX7D ADC driver support
  Documentation: add the binding file for Freescale imx7d ADC driver
  ARM: dts: imx7d.dtsi: add ADC support
  ARM: dts: imx7d-sdb: add ADC support

 .../devicetree/bindings/iio/adc/imx7d-adc.txt      |  26 +
 arch/arm/boot/dts/imx7d-sdb.dts                    |  10 +
 arch/arm/boot/dts/imx7d.dtsi                       |  20 +
 drivers/iio/adc/Kconfig                            |   9 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/imx7d_adc.c                        | 571 +++++++++++++++++++++
 6 files changed, 637 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/imx7d-adc.txt
 create mode 100644 drivers/iio/adc/imx7d_adc.c

-- 
1.9.1

[PATCH v2 1/4] iio: adc: add IMX7D ADC driver support

[Haibo Chen]

Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC
driver support, and the driver only support ADC software trigger.

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 drivers/iio/adc/Kconfig     |   9 +
 drivers/iio/adc/Makefile    |   1 +
 drivers/iio/adc/imx7d_adc.c | 571 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 581 insertions(+)
 create mode 100644 drivers/iio/adc/imx7d_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 7868c74..bf0611c 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -194,6 +194,15 @@ config HI8435
 	  This driver can also be built as a module. If so, the module will be
 	  called hi8435.
 
+config IMX7D_ADC
+	tristate "IMX7D ADC driver"
+	depends on OF
+	help
+	  Say yes here to build support for IMX7D ADC.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called imx7d_adc.
+
 config LP8788_ADC
 	tristate "LP8788 ADC driver"
 	depends on MFD_LP8788
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 99b37a9..282ffc01 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_HI8435) += hi8435.o
+obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX1363) += max1363.o
diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
new file mode 100644
index 0000000..7e25e4e
--- /dev/null
+++ b/drivers/iio/adc/imx7d_adc.c
@@ -0,0 +1,571 @@
+/*
+ * Freescale i.MX7D ADC driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of_platform.h>
+#include <linux/err.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/driver.h>
+
+/* This will be the driver name the kernel reports */
+#define DRIVER_NAME "imx7d_adc"
+
+/* ADC register */
+#define IMX7D_REG_ADC_CH_A_CFG1			0x00
+#define IMX7D_REG_ADC_CH_A_CFG2			0x10
+#define IMX7D_REG_ADC_CH_B_CFG1			0x20
+#define IMX7D_REG_ADC_CH_B_CFG2			0x30
+#define IMX7D_REG_ADC_CH_C_CFG1			0x40
+#define IMX7D_REG_ADC_CH_C_CFG2			0x50
+#define IMX7D_REG_ADC_CH_D_CFG1			0x60
+#define IMX7D_REG_ADC_CH_D_CFG2			0x70
+#define IMX7D_REG_ADC_CH_SW_CFG			0x80
+#define IMX7D_REG_ADC_TIMER_UNIT		0x90
+#define IMX7D_REG_ADC_DMA_FIFO			0xa0
+#define IMX7D_REG_ADC_FIFO_STATUS		0xb0
+#define IMX7D_REG_ADC_INT_SIG_EN		0xc0
+#define IMX7D_REG_ADC_INT_EN			0xd0
+#define IMX7D_REG_ADC_INT_STATUS		0xe0
+#define IMX7D_REG_ADC_CHA_B_CNV_RSLT		0xf0
+#define IMX7D_REG_ADC_CHC_D_CNV_RSLT		0x100
+#define IMX7D_REG_ADC_CH_SW_CNV_RSLT		0x110
+#define IMX7D_REG_ADC_DMA_FIFO_DAT		0x120
+#define IMX7D_REG_ADC_ADC_CFG			0x130
+
+#define IMX7D_EACH_CHANNEL_REG_SHIF		0x20
+
+#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN			(0x1 << 31)
+#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_DISABLE			(0x0 << 31)
+#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_SINGLE			BIT(30)
+#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_AVG_EN			BIT(29)
+#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_SEL_SHIF			24
+
+#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_4				(0x0 << 12)
+#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_8				(0x1 << 12)
+#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_16			(0x2 << 12)
+#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_32			(0x3 << 12)
+
+#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_4			(0x0 << 29)
+#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_8			(0x1 << 29)
+#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_16			(0x2 << 29)
+#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_32			(0x3 << 29)
+#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_64			(0x4 << 29)
+#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_128			(0x5 << 29)
+
+#define IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN			BIT(31)
+#define IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN			BIT(1)
+#define IMX7D_REG_ADC_ADC_CFG_ADC_EN				BIT(0)
+
+#define IMX7D_REG_ADC_INT_CHA_COV_INT_EN			BIT(8)
+#define IMX7D_REG_ADC_INT_CHB_COV_INT_EN			BIT(9)
+#define IMX7D_REG_ADC_INT_CHC_COV_INT_EN			BIT(10)
+#define IMX7D_REG_ADC_INT_CHD_COV_INT_EN			BIT(11)
+#define IMX7D_REG_ADC_INT_CHANNEL_INT_EN			(IMX7D_REG_ADC_INT_CHA_COV_INT_EN | \
+								 IMX7D_REG_ADC_INT_CHB_COV_INT_EN | \
+								 IMX7D_REG_ADC_INT_CHC_COV_INT_EN | \
+								 IMX7D_REG_ADC_INT_CHD_COV_INT_EN)
+#define IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS		0xf00
+
+#define IMX7D_ADC_TIMEOUT		msecs_to_jiffies(100)
+
+enum clk_pre_div {
+	IMX7D_ADC_ANALOG_CLK_PRE_DIV_4,
+	IMX7D_ADC_ANALOG_CLK_PRE_DIV_8,
+	IMX7D_ADC_ANALOG_CLK_PRE_DIV_16,
+	IMX7D_ADC_ANALOG_CLK_PRE_DIV_32,
+	IMX7D_ADC_ANALOG_CLK_PRE_DIV_64,
+	IMX7D_ADC_ANALOG_CLK_PRE_DIV_128,
+};
+
+enum average_num {
+	IMX7D_ADC_AVERAGE_NUM_4,
+	IMX7D_ADC_AVERAGE_NUM_8,
+	IMX7D_ADC_AVERAGE_NUM_16,
+	IMX7D_ADC_AVERAGE_NUM_32,
+};
+
+struct imx7d_adc_feature {
+	enum clk_pre_div clk_pre_div;
+	enum average_num avg_num;
+
+	u32 core_time_unit;	/* impact the sample rate */
+
+	bool average_en;
+};
+
+struct imx7d_adc {
+	struct device *dev;
+	void __iomem *regs;
+	struct clk *clk;
+
+	u32 vref_uv;
+	u32 value;
+	u32 channel;
+	u32 pre_div_num;
+
+	struct regulator *vref;
+	struct imx7d_adc_feature adc_feature;
+
+	struct completion completion;
+};
+
+struct imx7d_adc_analogue_core_clk {
+	u32 pre_div;
+	u32 reg_config;
+};
+
+#define IMX7D_ADC_ALALOGUE_CLK_CONFIG(_pre_div, _reg_conf) {	\
+	.pre_div = (_pre_div),					\
+	.reg_config = (_reg_conf),				\
+}
+
+static const struct imx7d_adc_analogue_core_clk imx7d_adc_analogue_clk[] = {
+	IMX7D_ADC_ALALOGUE_CLK_CONFIG(4, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_4),
+	IMX7D_ADC_ALALOGUE_CLK_CONFIG(8, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_8),
+	IMX7D_ADC_ALALOGUE_CLK_CONFIG(16, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_16),
+	IMX7D_ADC_ALALOGUE_CLK_CONFIG(32, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_32),
+	IMX7D_ADC_ALALOGUE_CLK_CONFIG(64, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_64),
+	IMX7D_ADC_ALALOGUE_CLK_CONFIG(128, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_128),
+};
+
+#define IMX7D_ADC_CHAN(_idx, _chan_type) {			\
+	.type = (_chan_type),					\
+	.indexed = 1,						\
+	.channel = (_idx),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+}
+
+static const struct iio_chan_spec imx7d_adc_iio_channels[] = {
+	IMX7D_ADC_CHAN(0, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(1, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(2, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(3, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(4, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(5, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(6, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(7, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(8, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(9, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(10, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(11, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(12, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(13, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(14, IIO_VOLTAGE),
+	IMX7D_ADC_CHAN(15, IIO_VOLTAGE),
+};
+
+static const u32 imx7d_adc_average_num[] = {
+	IMX7D_REG_ADC_CH_CFG2_AVG_NUM_4,
+	IMX7D_REG_ADC_CH_CFG2_AVG_NUM_8,
+	IMX7D_REG_ADC_CH_CFG2_AVG_NUM_16,
+	IMX7D_REG_ADC_CH_CFG2_AVG_NUM_32,
+};
+
+static void imx7d_adc_feature_config(struct imx7d_adc *info)
+{
+	info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4;
+	info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32;
+	info->adc_feature.core_time_unit = 1;
+	info->adc_feature.average_en = true;
+}
+
+static void imx7d_adc_sample_set(struct imx7d_adc *info)
+{
+	struct imx7d_adc_feature *adc_feature = &info->adc_feature;
+	struct imx7d_adc_analogue_core_clk adc_analogure_clk;
+	u32 i;
+	u32 sample_rate = 0;
+
+	/*
+	 * Before sample set, disable channel A,B,C,D. Here we
+	 * clear the bit 31 of register REG_ADC_CH_A\B\C\D_CFG1.
+	 */
+	for (i = 0; i < 4; i++)
+		writel(IMX7D_REG_ADC_CH_CFG1_CHANNEL_DISABLE,
+			info->regs + i * IMX7D_EACH_CHANNEL_REG_SHIF);
+
+	adc_analogure_clk = imx7d_adc_analogue_clk[adc_feature->clk_pre_div];
+	sample_rate |= adc_analogure_clk.reg_config;
+	info->pre_div_num = adc_analogure_clk.pre_div;
+
+	sample_rate |= adc_feature->core_time_unit;
+	writel(sample_rate, info->regs + IMX7D_REG_ADC_TIMER_UNIT);
+}
+
+static void imx7d_adc_hw_init(struct imx7d_adc *info)
+{
+	u32 cfg;
+	/* power up and enable adc analogue core */
+	cfg = readl(info->regs + IMX7D_REG_ADC_ADC_CFG);
+	cfg &= ~(IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN | IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN);
+	cfg |= IMX7D_REG_ADC_ADC_CFG_ADC_EN;
+	writel(cfg, info->regs + IMX7D_REG_ADC_ADC_CFG);
+
+	/* enable channel A,B,C,D interrupt */
+	writel(IMX7D_REG_ADC_INT_CHANNEL_INT_EN, info->regs + IMX7D_REG_ADC_INT_SIG_EN);
+	writel(IMX7D_REG_ADC_INT_CHANNEL_INT_EN, info->regs + IMX7D_REG_ADC_INT_EN);
+
+	imx7d_adc_sample_set(info);
+}
+
+static void imx7d_adc_channel_set(struct imx7d_adc *info)
+{
+	u32 cfg1 = 0;
+	u32 cfg2;
+	u32 channel;
+
+	channel = info->channel;
+
+	/* the channel choose single conversion, and enable average mode */
+	cfg1 |= (IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN | IMX7D_REG_ADC_CH_CFG1_CHANNEL_SINGLE);
+	if (info->adc_feature.average_en)
+		cfg1 |= IMX7D_REG_ADC_CH_CFG1_CHANNEL_AVG_EN;
+
+	/*
+	 * physical channel 0 chose logical channel A
+	 * physical channel 1 chose logical channel B
+	 * physical channel 2 chose logical channel C
+	 * physical channel 3 chose logical channel D
+	 */
+	cfg1 |= (channel << IMX7D_REG_ADC_CH_CFG1_CHANNEL_SEL_SHIF);
+
+	/* read register REG_ADC_CH_A\B\C\D_CFG2, according to the channel chosen */
+	cfg2 = readl(info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel + 0x10);
+
+	cfg2 |= imx7d_adc_average_num[info->adc_feature.avg_num];
+
+	/* write the register REG_ADC_CH_A\B\C\D_CFG2, according to the channel chosen */
+	writel(cfg2, info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel + 0x10);
+	writel(cfg1, info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel);
+}
+
+static u32 imx7d_adc_get_sample_rate(struct imx7d_adc *info)
+{
+	/* input clock is always 24MHz */
+	u32 input_clk = 24000000;
+	u32 analogue_core_clk;
+	u32 core_time_unit = info->adc_feature.core_time_unit;
+	u32 sample_clk;
+	u32 tmp;
+
+	analogue_core_clk = input_clk / info->pre_div_num;
+	tmp = (core_time_unit + 1) * 6;
+	sample_clk = analogue_core_clk / tmp;
+
+	return sample_clk;
+}
+
+static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan,
+			int *val,
+			int *val2,
+			long mask)
+{
+	struct imx7d_adc *info = iio_priv(indio_dev);
+
+	u32 channel;
+	long ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&indio_dev->mlock);
+		reinit_completion(&info->completion);
+
+		channel = (chan->channel) & 0x0f;
+		info->channel = channel;
+		imx7d_adc_channel_set(info);
+
+		ret = wait_for_completion_interruptible_timeout
+				(&info->completion, IMX7D_ADC_TIMEOUT);
+		if (ret == 0) {
+			mutex_unlock(&indio_dev->mlock);
+			return -ETIMEDOUT;
+		}
+		if (ret < 0) {
+			mutex_unlock(&indio_dev->mlock);
+			return ret;
+		}
+
+		*val = info->value;
+		mutex_unlock(&indio_dev->mlock);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = info->vref_uv / 1000;
+		*val2 = 12;
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = imx7d_adc_get_sample_rate(info);
+		*val2 = 0;
+		return IIO_VAL_INT;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int imx7d_adc_read_data(struct imx7d_adc *info)
+{
+	u32 channel;
+	u32 value;
+
+	channel = info->channel;
+
+	/*
+	 * channel A and B conversion result share one register,
+	 * bit[27~16] is the channel B conversion result,
+	 * bit[11~0] is the channel A conversion result.
+	 * channel C and D is the same.
+	 */
+	if (channel < 2)
+		value = readl(info->regs + IMX7D_REG_ADC_CHA_B_CNV_RSLT);
+	else
+		value = readl(info->regs + IMX7D_REG_ADC_CHC_D_CNV_RSLT);
+	if (channel & 0x1)	/* channel B or D */
+		value = (value >> 16) & 0xFFF;
+	else			/* channel A or C */
+		value &= 0xFFF;
+
+	return value;
+}
+
+static irqreturn_t imx7d_adc_isr(int irq, void *dev_id)
+{
+	struct imx7d_adc *info = (struct imx7d_adc *)dev_id;
+	int status;
+
+	status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS);
+	if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) {
+		info->value = imx7d_adc_read_data(info);
+		complete(&info->completion);
+	}
+	writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS);
+
+	return IRQ_HANDLED;
+}
+
+static int imx7d_adc_reg_access(struct iio_dev *indio_dev,
+			unsigned reg, unsigned writeval,
+			unsigned *readval)
+{
+	struct imx7d_adc *info = iio_priv(indio_dev);
+
+	if ((readval == NULL) ||
+		((reg % 4) || (reg > IMX7D_REG_ADC_ADC_CFG)))
+		return -EINVAL;
+
+	*readval = readl(info->regs + reg);
+
+	return 0;
+}
+
+static const struct iio_info imx7d_adc_iio_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = &imx7d_adc_read_raw,
+	.debugfs_reg_access = &imx7d_adc_reg_access,
+};
+
+static const struct of_device_id imx7d_adc_match[] = {
+	{ .compatible = "fsl,imx7d-adc", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx7d_adc_match);
+
+static int imx7d_adc_probe(struct platform_device *pdev)
+{
+	struct imx7d_adc *info;
+	struct iio_dev *indio_dev;
+	struct resource *mem;
+	int irq;
+	int ret;
+	u32 channels;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct imx7d_adc));
+	if (!indio_dev) {
+		dev_err(&pdev->dev, "Failed allocating iio device\n");
+		return -ENOMEM;
+	}
+
+	info = iio_priv(indio_dev);
+	info->dev = &pdev->dev;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	info->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(info->regs)) {
+		ret = PTR_ERR(info->regs);
+		dev_err(&pdev->dev, "failed to remap adc memory: %d\n", ret);
+		return ret;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "no irq resource?\n");
+		return irq;
+	}
+
+	ret = devm_request_irq(info->dev, irq,
+				imx7d_adc_isr, 0,
+				dev_name(&pdev->dev), info);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
+		return ret;
+	}
+
+	info->clk = devm_clk_get(&pdev->dev, "adc");
+	if (IS_ERR(info->clk)) {
+		ret = PTR_ERR(info->clk);
+		dev_err(&pdev->dev, "failed getting clock, err = %d\n", ret);
+		return ret;
+	}
+
+	info->vref = devm_regulator_get(&pdev->dev, "vref");
+	if (IS_ERR(info->vref)) {
+		ret = PTR_ERR(info->vref);
+		dev_err(&pdev->dev, "failed getting reference voltage: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_enable(info->vref);
+	if (ret)
+		return ret;
+
+	info->vref_uv = regulator_get_voltage(info->vref);
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	init_completion(&info->completion);
+
+	ret  = of_property_read_u32(pdev->dev.of_node,
+					"num-channels", &channels);
+	if (ret)
+		channels = ARRAY_SIZE(imx7d_adc_iio_channels);
+
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->dev.of_node = pdev->dev.of_node;
+	indio_dev->info = &imx7d_adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = imx7d_adc_iio_channels;
+	indio_dev->num_channels = (int)channels;
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Could not prepare or enable the clock.\n");
+		goto error_adc_clk_enable;
+	}
+
+	imx7d_adc_feature_config(info);
+	imx7d_adc_hw_init(info);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Couldn't register the device.\n");
+		goto error_iio_device_register;
+	}
+
+	return 0;
+
+error_iio_device_register:
+	clk_disable_unprepare(info->clk);
+error_adc_clk_enable:
+	regulator_disable(info->vref);
+
+	return ret;
+}
+
+static int imx7d_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct imx7d_adc *info = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	regulator_disable(info->vref);
+	clk_disable_unprepare(info->clk);
+
+	return 0;
+}
+
+static int __maybe_unused imx7d_adc_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct imx7d_adc *info = iio_priv(indio_dev);
+	u32 adc_cfg;
+
+	adc_cfg = readl(info->regs + IMX7D_REG_ADC_ADC_CFG);
+	adc_cfg |= IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN | IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN;
+	adc_cfg &= ~IMX7D_REG_ADC_ADC_CFG_ADC_EN;
+	writel(adc_cfg, info->regs + IMX7D_REG_ADC_ADC_CFG);
+
+	clk_disable_unprepare(info->clk);
+	regulator_disable(info->vref);
+
+	return 0;
+}
+
+static int __maybe_unused imx7d_adc_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct imx7d_adc *info = iio_priv(indio_dev);
+	int ret;
+
+	ret = regulator_enable(info->vref);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret) {
+		dev_err(info->dev,
+			"Could not prepare or enable clock.\n");
+		regulator_disable(info->vref);
+		return ret;
+	}
+
+	imx7d_adc_hw_init(info);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(imx7d_adc_pm_ops, imx7d_adc_suspend, imx7d_adc_resume);
+
+static struct platform_driver imx7d_adc_driver = {
+	.probe		= imx7d_adc_probe,
+	.remove		= imx7d_adc_remove,
+	.driver		= {
+		.name	= DRIVER_NAME,
+		.of_match_table = imx7d_adc_match,
+		.pm	= &imx7d_adc_pm_ops,
+	},
+};
+
+module_platform_driver(imx7d_adc_driver);
+
+MODULE_AUTHOR("Haibo Chen <haibo.chen@freescale.com>");
+MODULE_DESCRIPTION("Freeacale IMX7D ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v2 1/4] iio: adc: add IMX7D ADC driver support

[Lars-Peter Clausen]

On 11/09/2015 02:28 PM, Haibo Chen wrote:
> Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC
> driver support, and the driver only support ADC software trigger.
> 
> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>

Looks pretty good, a few comments inline.

[...]
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_platform.h>
> +#include <linux/err.h>

I don't think you need all of these.

[...]
> +static void imx7d_adc_feature_config(struct imx7d_adc *info)
> +{
> +	info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4;
> +	info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32;
> +	info->adc_feature.core_time_unit = 1;
> +	info->adc_feature.average_en = true;

What's the plan for these? Right now they are always initialized to the same
static value.


> +}
[...]
> +static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val,
> +			int *val2,
> +			long mask)
> +{
> +	struct imx7d_adc *info = iio_priv(indio_dev);
> +
> +	u32 channel;
> +	long ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		reinit_completion(&info->completion);
> +
> +		channel = (chan->channel) & 0x0f;
> +		info->channel = channel;
> +		imx7d_adc_channel_set(info);

How about just passing channel directy adc_channel_set() instead of doing it
implicitly through the info struct?

[...]
> +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id)
> +{
> +	struct imx7d_adc *info = (struct imx7d_adc *)dev_id;
> +	int status;
> +
> +	status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS);
> +	if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) {
> +		info->value = imx7d_adc_read_data(info);
> +		complete(&info->completion);
> +	}
> +	writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS);

Is the hardware really this broken? If the interrupt happens between reading
the status register and clearing it here it will be missed.

> +
> +	return IRQ_HANDLED;

You should only return IRQ_HANDLED if you actually handled are interrupt.

> +}
[...]
> +
> +static int imx7d_adc_probe(struct platform_device *pdev)
> +{
> +	struct imx7d_adc *info;
> +	struct iio_dev *indio_dev;
> +	struct resource *mem;
> +	int irq;
> +	int ret;
> +	u32 channels;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct imx7d_adc));
> +	if (!indio_dev) {
> +		dev_err(&pdev->dev, "Failed allocating iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	info = iio_priv(indio_dev);
> +	info->dev = &pdev->dev;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	info->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(info->regs)) {
> +		ret = PTR_ERR(info->regs);
> +		dev_err(&pdev->dev, "failed to remap adc memory: %d\n", ret);
> +		return ret;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "no irq resource?\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(info->dev, irq,
> +				imx7d_adc_isr, 0,
> +				dev_name(&pdev->dev), info);

This is too early. Completion is not initialized, clocks are not enabled, etc...

> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> +		return ret;
> +	}
> +
[...]
> +	ret  = of_property_read_u32(pdev->dev.of_node,

Extra space.

> +					"num-channels", &channels);

What decides how many channels are in use?

> +	if (ret)
> +		channels = ARRAY_SIZE(imx7d_adc_iio_channels);
> +
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->info = &imx7d_adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = imx7d_adc_iio_channels;
> +	indio_dev->num_channels = (int)channels;

I don't think you need the case here.

>[...]

[PATCH v2 1/4] iio: adc: add IMX7D ADC driver support

[Haibo Chen]

On Tue, Nov 10, 2015 at 03:58:14PM +0100, Lars-Peter Clausen wrote:
> On 11/09/2015 02:28 PM, Haibo Chen wrote:
> > Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC
> > driver support, and the driver only support ADC software trigger.
> > 
> > Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
> 
> Looks pretty good, a few comments inline.
> 
> [...]
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/delay.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/io.h>
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/err.h>
> 
> I don't think you need all of these.
> 
> [...]

Yes, I will remove delay.h slab.h of_irq.h and of_platform.h

> > +static void imx7d_adc_feature_config(struct imx7d_adc *info)
> > +{
> > +	info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4;
> > +	info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32;
> > +	info->adc_feature.core_time_unit = 1;
> > +	info->adc_feature.average_en = true;
> 
> What's the plan for these? Right now they are always initialized to the same
> static value.
> 

In future, we can get these values from dts file, currently we just use the static value.

> 
> > +}
> [...]
> > +static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
> > +			struct iio_chan_spec const *chan,
> > +			int *val,
> > +			int *val2,
> > +			long mask)
> > +{
> > +	struct imx7d_adc *info = iio_priv(indio_dev);
> > +
> > +	u32 channel;
> > +	long ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&indio_dev->mlock);
> > +		reinit_completion(&info->completion);
> > +
> > +		channel = (chan->channel) & 0x0f;
> > +		info->channel = channel;
> > +		imx7d_adc_channel_set(info);
> 
> How about just passing channel directy adc_channel_set() instead of doing it
> implicitly through the info struct?
> 

I think there is no difference, besides, using this parameter info struct can keep align with other functions.
eg.  imx7d_adc_sample_set(), imx7d_adc_hw_init(), imx7d_adc_get_sample_rate(), all these functions have the same parameter.

> [...]
> > +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id)
> > +{
> > +	struct imx7d_adc *info = (struct imx7d_adc *)dev_id;
> > +	int status;
> > +
> > +	status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS);
> > +	if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) {
> > +		info->value = imx7d_adc_read_data(info);
> > +		complete(&info->completion);
> > +	}
> > +	writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS);
> 
> Is the hardware really this broken? If the interrupt happens between reading
> the status register and clearing it here it will be missed.
> 

I think interrupt can't happen between reading the status register and clearing it.
Because in function imx7d_adc_read_raw(), we call the function 
imx7d_adc_channel_set(info), in this function, we config the register
REG_ADC_CH_A\B\C\D_CFG1 and REG_ADC_CH_A\B\C\D_CFG2, only when these registers
is configed, ADC start a conversion. Once the conversion complete, ADC trigger an 
interrupt, and call the imx7d_adc_isr().

> > +
> > +	return IRQ_HANDLED;
> 
> You should only return IRQ_HANDLED if you actually handled are interrupt.
> 

Here in the interrupt, we just handle the channel conversion finished flag, for
other flag, ignore them this time, Will add other flag in future.

> > +}
> [...]
> > +
> > +static int imx7d_adc_probe(struct platform_device *pdev)
> > +{
> > +	struct imx7d_adc *info;
> > +	struct iio_dev *indio_dev;
> > +	struct resource *mem;
> > +	int irq;
> > +	int ret;
> > +	u32 channels;
> > +
> > +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct imx7d_adc));
> > +	if (!indio_dev) {
> > +		dev_err(&pdev->dev, "Failed allocating iio device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	info = iio_priv(indio_dev);
> > +	info->dev = &pdev->dev;
> > +
> > +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	info->regs = devm_ioremap_resource(&pdev->dev, mem);
> > +	if (IS_ERR(info->regs)) {
> > +		ret = PTR_ERR(info->regs);
> > +		dev_err(&pdev->dev, "failed to remap adc memory: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		dev_err(&pdev->dev, "no irq resource?\n");
> > +		return irq;
> > +	}
> > +
> > +	ret = devm_request_irq(info->dev, irq,
> > +				imx7d_adc_isr, 0,
> > +				dev_name(&pdev->dev), info);
> 
> This is too early. Completion is not initialized, clocks are not enabled, etc...
> 

You are right, I will move the request irq function down.
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> > +		return ret;
> > +	}
> > +
> [...]
> > +	ret  = of_property_read_u32(pdev->dev.of_node,
> 
> Extra space.
> 
> > +					"num-channels", &channels);
> 
> What decides how many channels are in use?
> 

The board decides the channel number.
In dts file, there is a line:     num-channels = <4>;


> > +	if (ret)
> > +		channels = ARRAY_SIZE(imx7d_adc_iio_channels);
> > +
> > +	indio_dev->name = dev_name(&pdev->dev);
> > +	indio_dev->dev.parent = &pdev->dev;
> > +	indio_dev->dev.of_node = pdev->dev.of_node;
> > +	indio_dev->info = &imx7d_adc_iio_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = imx7d_adc_iio_channels;
> > +	indio_dev->num_channels = (int)channels;
> 
> I don't think you need the case here.
> 

Sorry, can't get your point, you mean I should not indio_dev-> ?

> >[...]

-- 

[PATCH v2 1/4] iio: adc: add IMX7D ADC driver support

[Lars-Peter Clausen]

On 11/13/2015 10:39 AM, Haibo Chen wrote:
[...]
>>> +static void imx7d_adc_feature_config(struct imx7d_adc *info)
>>> +{
>>> +	info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4;
>>> +	info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32;
>>> +	info->adc_feature.core_time_unit = 1;
>>> +	info->adc_feature.average_en = true;
>>
>> What's the plan for these? Right now they are always initialized to the same
>> static value.
>>
> 
> In future, we can get these values from dts file, currently we just use the static value.

Those seem to be software configuration values though, not part of hardware
description.

> 
>>
>>> +}
>> [...]
>>> +static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
>>> +			struct iio_chan_spec const *chan,
>>> +			int *val,
>>> +			int *val2,
>>> +			long mask)
>>> +{
>>> +	struct imx7d_adc *info = iio_priv(indio_dev);
>>> +
>>> +	u32 channel;
>>> +	long ret;
>>> +
>>> +	switch (mask) {
>>> +	case IIO_CHAN_INFO_RAW:
>>> +		mutex_lock(&indio_dev->mlock);
>>> +		reinit_completion(&info->completion);
>>> +
>>> +		channel = (chan->channel) & 0x0f;
>>> +		info->channel = channel;
>>> +		imx7d_adc_channel_set(info);
>>
>> How about just passing channel directy adc_channel_set() instead of doing it
>> implicitly through the info struct?
>>
> 
> I think there is no difference, besides, using this parameter info struct can keep align with other functions.
> eg.  imx7d_adc_sample_set(), imx7d_adc_hw_init(), imx7d_adc_get_sample_rate(), all these functions have the same parameter.
> 
>> [...]
>>> +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id)
>>> +{
>>> +	struct imx7d_adc *info = (struct imx7d_adc *)dev_id;
>>> +	int status;
>>> +
>>> +	status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS);
>>> +	if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) {
>>> +		info->value = imx7d_adc_read_data(info);
>>> +		complete(&info->completion);
>>> +	}
>>> +	writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS);
>>
>> Is the hardware really this broken? If the interrupt happens between reading
>> the status register and clearing it here it will be missed.
>>
> 
> I think interrupt can't happen between reading the status register and clearing it.
> Because in function imx7d_adc_read_raw(), we call the function 
> imx7d_adc_channel_set(info), in this function, we config the register
> REG_ADC_CH_A\B\C\D_CFG1 and REG_ADC_CH_A\B\C\D_CFG2, only when these registers
> is configed, ADC start a conversion. Once the conversion complete, ADC trigger an 
> interrupt, and call the imx7d_adc_isr().

Well an interrupt can always happen, its running fully asynchronous to the
CPU. If you have multiple interrupt sources enabled and the first one fires
and you run then start the irq handler, read the status register, now the
second irq sources fires, and then you clear the status interrupt register.
This means the driver will not see the irq from the second source.

> 
>>> +
>>> +	return IRQ_HANDLED;
>>
>> You should only return IRQ_HANDLED if you actually handled are interrupt.
>>
> 
> Here in the interrupt, we just handle the channel conversion finished flag, for
> other flag, ignore them this time, Will add other flag in future.

Yeah, but if you don't handle any interrupt you should return IRQ_NONE. This
will make sure that the system can recover in case the hardware (or the
driver) is broken and generates unexpected interrupts. If there are a 1000
or so IRQ_NONEs in a row in a short time frame the kernel will disable the IRQ.

>> [...]
>>> +	ret  = of_property_read_u32(pdev->dev.of_node,
>>
>> Extra space.
>>
>>> +					"num-channels", &channels);
>>
>> What decides how many channels are in use?
>>
> 
> The board decides the channel number.
> In dts file, there is a line:     num-channels = <4>;

Yes, but what decides how this property should be configured?

> 
> 
>>> +	if (ret)
>>> +		channels = ARRAY_SIZE(imx7d_adc_iio_channels);
>>> +
>>> +	indio_dev->name = dev_name(&pdev->dev);
>>> +	indio_dev->dev.parent = &pdev->dev;
>>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>>> +	indio_dev->info = &imx7d_adc_iio_info;
>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>> +	indio_dev->channels = imx7d_adc_iio_channels;
>>> +	indio_dev->num_channels = (int)channels;
>>
>> I don't think you need the case here.
>>
> 
> Sorry, can't get your point, you mean I should not indio_dev-> ?

Sorry, I meant the (int) cast.

> 
>>> [...]
> 

[PATCH v2 1/4] iio: adc: add IMX7D ADC driver support

[Jonathan Cameron]

On 13/11/15 09:52, Lars-Peter Clausen wrote:
> On 11/13/2015 10:39 AM, Haibo Chen wrote:
> [...]
>>>> +static void imx7d_adc_feature_config(struct imx7d_adc *info)
>>>> +{
>>>> +	info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4;
>>>> +	info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32;
>>>> +	info->adc_feature.core_time_unit = 1;
>>>> +	info->adc_feature.average_en = true;
>>>
>>> What's the plan for these? Right now they are always initialized to the same
>>> static value.
>>>
>>
>> In future, we can get these values from dts file, currently we just use the static value.
> 
> Those seem to be software configuration values though, not part of hardware
> description.
> 
>>
>>>
>>>> +}
>>> [...]
>>>> +static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
>>>> +			struct iio_chan_spec const *chan,
>>>> +			int *val,
>>>> +			int *val2,
>>>> +			long mask)
>>>> +{
>>>> +	struct imx7d_adc *info = iio_priv(indio_dev);
>>>> +
>>>> +	u32 channel;
>>>> +	long ret;
>>>> +
>>>> +	switch (mask) {
>>>> +	case IIO_CHAN_INFO_RAW:
>>>> +		mutex_lock(&indio_dev->mlock);
>>>> +		reinit_completion(&info->completion);
>>>> +
>>>> +		channel = (chan->channel) & 0x0f;
>>>> +		info->channel = channel;
>>>> +		imx7d_adc_channel_set(info);
>>>
>>> How about just passing channel directy adc_channel_set() instead of doing it
>>> implicitly through the info struct?
>>>
>>
>> I think there is no difference, besides, using this parameter info struct can keep align with other functions.
>> eg.  imx7d_adc_sample_set(), imx7d_adc_hw_init(), imx7d_adc_get_sample_rate(), all these functions have the same parameter.
>>
>>> [...]
>>>> +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id)
>>>> +{
>>>> +	struct imx7d_adc *info = (struct imx7d_adc *)dev_id;
>>>> +	int status;
>>>> +
>>>> +	status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS);
>>>> +	if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) {
>>>> +		info->value = imx7d_adc_read_data(info);
>>>> +		complete(&info->completion);
>>>> +	}
>>>> +	writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS);
>>>
>>> Is the hardware really this broken? If the interrupt happens between reading
>>> the status register and clearing it here it will be missed.
>>>
>>
>> I think interrupt can't happen between reading the status register and clearing it.
>> Because in function imx7d_adc_read_raw(), we call the function 
>> imx7d_adc_channel_set(info), in this function, we config the register
>> REG_ADC_CH_A\B\C\D_CFG1 and REG_ADC_CH_A\B\C\D_CFG2, only when these registers
>> is configed, ADC start a conversion. Once the conversion complete, ADC trigger an 
>> interrupt, and call the imx7d_adc_isr().
> 
> Well an interrupt can always happen, its running fully asynchronous to the
> CPU. If you have multiple interrupt sources enabled and the first one fires
> and you run then start the irq handler, read the status register, now the
> second irq sources fires, and then you clear the status interrupt register.
> This means the driver will not see the irq from the second source.
I'd agree with this, would normally expect to see only the relevant parts of the
register 'cleared'.  Usually this is done by explicitly picking those bits
out (normally writing 1 to the bit to clear the interrupt source).

If there is anything else of interest in that register this is nasty!

Hmm. looking at the bits you have defined and the way it is used you only
ever have one interrupt at a time possibly occurring at the moment?

If this is actually the case, then why read the register at all?

If there is anything in the other bits of that register it would be interesting to
know what is there!
> 
>>
>>>> +
>>>> +	return IRQ_HANDLED;
>>>
>>> You should only return IRQ_HANDLED if you actually handled are interrupt.
>>>
>>
>> Here in the interrupt, we just handle the channel conversion finished flag, for
>> other flag, ignore them this time, Will add other flag in future.
> 
> Yeah, but if you don't handle any interrupt you should return IRQ_NONE. This
> will make sure that the system can recover in case the hardware (or the
> driver) is broken and generates unexpected interrupts. If there are a 1000
> or so IRQ_NONEs in a row in a short time frame the kernel will disable the IRQ.
> 
>>> [...]
>>>> +	ret  = of_property_read_u32(pdev->dev.of_node,
>>>
>>> Extra space.
>>>
>>>> +					"num-channels", &channels);
>>>
>>> What decides how many channels are in use?
>>>
>>
>> The board decides the channel number.
>> In dts file, there is a line:     num-channels = <4>;
> 
> Yes, but what decides how this property should be configured?
> 
>>
>>
>>>> +	if (ret)
>>>> +		channels = ARRAY_SIZE(imx7d_adc_iio_channels);
>>>> +
>>>> +	indio_dev->name = dev_name(&pdev->dev);
>>>> +	indio_dev->dev.parent = &pdev->dev;
>>>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>>>> +	indio_dev->info = &imx7d_adc_iio_info;
>>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>>> +	indio_dev->channels = imx7d_adc_iio_channels;
>>>> +	indio_dev->num_channels = (int)channels;
>>>
>>> I don't think you need the case here.
>>>
>>
>> Sorry, can't get your point, you mean I should not indio_dev-> ?
> 
> Sorry, I meant the (int) cast.
> 
>>
>>>> [...]
>>
> 

[PATCH v2 1/4] iio: adc: add IMX7D ADC driver support

[Jonathan Cameron]

On 09/11/15 13:28, Haibo Chen wrote:
> Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC
> driver support, and the driver only support ADC software trigger.
> 
> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
Hi Haibo,

A few comments inline in addition to Lars' ones.

One small point is that whilst the 80 char limit
should be ignored when obeying it significantly hurts readability, there
are cases in here where it wouldn't hurt to keep the lines shorter, so that
would be preferred.

Mostly looking good.

Jonathan
> ---
>  drivers/iio/adc/Kconfig     |   9 +
>  drivers/iio/adc/Makefile    |   1 +
>  drivers/iio/adc/imx7d_adc.c | 571 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 581 insertions(+)
>  create mode 100644 drivers/iio/adc/imx7d_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7868c74..bf0611c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -194,6 +194,15 @@ config HI8435
>  	  This driver can also be built as a module. If so, the module will be
>  	  called hi8435.
>  
> +config IMX7D_ADC
> +	tristate "IMX7D ADC driver"
> +	depends on OF
> +	help
> +	  Say yes here to build support for IMX7D ADC.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called imx7d_adc.
> +
>  config LP8788_ADC
>  	tristate "LP8788 ADC driver"
>  	depends on MFD_LP8788
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 99b37a9..282ffc01 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>  obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>  obj-$(CONFIG_HI8435) += hi8435.o
> +obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
> new file mode 100644
> index 0000000..7e25e4e
> --- /dev/null
> +++ b/drivers/iio/adc/imx7d_adc.c
> @@ -0,0 +1,571 @@
> +/*
> + * Freescale i.MX7D ADC driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_platform.h>
> +#include <linux/err.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/driver.h>
> +
> +/* This will be the driver name the kernel reports */
> +#define DRIVER_NAME "imx7d_adc"
It's only used in one place so don't bother with the define.
Use the string inline and it's immediately obvious what it
is for without the comment.

> +
> +/* ADC register */
> +#define IMX7D_REG_ADC_CH_A_CFG1			0x00
> +#define IMX7D_REG_ADC_CH_A_CFG2			0x10
> +#define IMX7D_REG_ADC_CH_B_CFG1			0x20
> +#define IMX7D_REG_ADC_CH_B_CFG2			0x30
> +#define IMX7D_REG_ADC_CH_C_CFG1			0x40
> +#define IMX7D_REG_ADC_CH_C_CFG2			0x50
> +#define IMX7D_REG_ADC_CH_D_CFG1			0x60
> +#define IMX7D_REG_ADC_CH_D_CFG2			0x70
> +#define IMX7D_REG_ADC_CH_SW_CFG			0x80
> +#define IMX7D_REG_ADC_TIMER_UNIT		0x90
> +#define IMX7D_REG_ADC_DMA_FIFO			0xa0
> +#define IMX7D_REG_ADC_FIFO_STATUS		0xb0
> +#define IMX7D_REG_ADC_INT_SIG_EN		0xc0
> +#define IMX7D_REG_ADC_INT_EN			0xd0
> +#define IMX7D_REG_ADC_INT_STATUS		0xe0
> +#define IMX7D_REG_ADC_CHA_B_CNV_RSLT		0xf0
> +#define IMX7D_REG_ADC_CHC_D_CNV_RSLT		0x100
> +#define IMX7D_REG_ADC_CH_SW_CNV_RSLT		0x110
> +#define IMX7D_REG_ADC_DMA_FIFO_DAT		0x120
> +#define IMX7D_REG_ADC_ADC_CFG			0x130
> +
> +#define IMX7D_EACH_CHANNEL_REG_SHIF		0x20
> +
> +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN			(0x1 << 31)
> +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_DISABLE			(0x0 << 31)
> +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_SINGLE			BIT(30)
> +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_AVG_EN			BIT(29)
> +#define IMX7D_REG_ADC_CH_CFG1_CHANNEL_SEL_SHIF			24
> +
> +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_4				(0x0 << 12)
> +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_8				(0x1 << 12)
> +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_16			(0x2 << 12)
> +#define IMX7D_REG_ADC_CH_CFG2_AVG_NUM_32			(0x3 << 12)
> +
> +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_4			(0x0 << 29)
> +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_8			(0x1 << 29)
> +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_16			(0x2 << 29)
> +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_32			(0x3 << 29)
> +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_64			(0x4 << 29)
> +#define IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_128			(0x5 << 29)
> +
> +#define IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN			BIT(31)
> +#define IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN			BIT(1)
> +#define IMX7D_REG_ADC_ADC_CFG_ADC_EN				BIT(0)
> +
> +#define IMX7D_REG_ADC_INT_CHA_COV_INT_EN			BIT(8)
> +#define IMX7D_REG_ADC_INT_CHB_COV_INT_EN			BIT(9)
> +#define IMX7D_REG_ADC_INT_CHC_COV_INT_EN			BIT(10)
> +#define IMX7D_REG_ADC_INT_CHD_COV_INT_EN			BIT(11)
> +#define IMX7D_REG_ADC_INT_CHANNEL_INT_EN			(IMX7D_REG_ADC_INT_CHA_COV_INT_EN | \
> +								 IMX7D_REG_ADC_INT_CHB_COV_INT_EN | \
> +								 IMX7D_REG_ADC_INT_CHC_COV_INT_EN | \
> +								 IMX7D_REG_ADC_INT_CHD_COV_INT_EN)
> +#define IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS		0xf00
> +
> +#define IMX7D_ADC_TIMEOUT		msecs_to_jiffies(100)
> +
> +enum clk_pre_div {
> +	IMX7D_ADC_ANALOG_CLK_PRE_DIV_4,
> +	IMX7D_ADC_ANALOG_CLK_PRE_DIV_8,
> +	IMX7D_ADC_ANALOG_CLK_PRE_DIV_16,
> +	IMX7D_ADC_ANALOG_CLK_PRE_DIV_32,
> +	IMX7D_ADC_ANALOG_CLK_PRE_DIV_64,
> +	IMX7D_ADC_ANALOG_CLK_PRE_DIV_128,
> +};
> +
I'd prefer prefixes on these enum defs e.g.
imx7d_adc_average_num

The naming is generic enough it might bit us in future otherwise!
> +enum average_num {
> +	IMX7D_ADC_AVERAGE_NUM_4,
> +	IMX7D_ADC_AVERAGE_NUM_8,
> +	IMX7D_ADC_AVERAGE_NUM_16,
> +	IMX7D_ADC_AVERAGE_NUM_32,
> +};
> +
> +struct imx7d_adc_feature {
> +	enum clk_pre_div clk_pre_div;
> +	enum average_num avg_num;
> +
> +	u32 core_time_unit;	/* impact the sample rate */
> +
> +	bool average_en;
> +};
> +
> +struct imx7d_adc {
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct clk *clk;
> +
> +	u32 vref_uv;
> +	u32 value;
> +	u32 channel;
> +	u32 pre_div_num;
> +
> +	struct regulator *vref;
> +	struct imx7d_adc_feature adc_feature;
> +
> +	struct completion completion;
> +};
> +
> +struct imx7d_adc_analogue_core_clk {
> +	u32 pre_div;
> +	u32 reg_config;
> +};
> +
> +#define IMX7D_ADC_ALALOGUE_CLK_CONFIG(_pre_div, _reg_conf) {	\
> +	.pre_div = (_pre_div),					\
> +	.reg_config = (_reg_conf),				\
> +}
> +
> +static const struct imx7d_adc_analogue_core_clk imx7d_adc_analogue_clk[] = {
> +	IMX7D_ADC_ALALOGUE_CLK_CONFIG(4, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_4),
> +	IMX7D_ADC_ALALOGUE_CLK_CONFIG(8, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_8),
> +	IMX7D_ADC_ALALOGUE_CLK_CONFIG(16, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_16),
> +	IMX7D_ADC_ALALOGUE_CLK_CONFIG(32, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_32),
> +	IMX7D_ADC_ALALOGUE_CLK_CONFIG(64, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_64),
> +	IMX7D_ADC_ALALOGUE_CLK_CONFIG(128, IMX7D_REG_ADC_TIMER_UNIT_PRE_DIV_128),
> +};
> +
> +#define IMX7D_ADC_CHAN(_idx, _chan_type) {			\
> +	.type = (_chan_type),					\
> +	.indexed = 1,						\
> +	.channel = (_idx),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +}
> +
> +static const struct iio_chan_spec imx7d_adc_iio_channels[] = {
> +	IMX7D_ADC_CHAN(0, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(1, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(2, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(3, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(4, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(5, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(6, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(7, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(8, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(9, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(10, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(11, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(12, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(13, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(14, IIO_VOLTAGE),
> +	IMX7D_ADC_CHAN(15, IIO_VOLTAGE),
> +};
> +
> +static const u32 imx7d_adc_average_num[] = {
> +	IMX7D_REG_ADC_CH_CFG2_AVG_NUM_4,
> +	IMX7D_REG_ADC_CH_CFG2_AVG_NUM_8,
> +	IMX7D_REG_ADC_CH_CFG2_AVG_NUM_16,
> +	IMX7D_REG_ADC_CH_CFG2_AVG_NUM_32,
> +};
> +
> +static void imx7d_adc_feature_config(struct imx7d_adc *info)
> +{
> +	info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4;
> +	info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32;
> +	info->adc_feature.core_time_unit = 1;
> +	info->adc_feature.average_en = true;
> +}
> +
> +static void imx7d_adc_sample_set(struct imx7d_adc *info)
> +{
> +	struct imx7d_adc_feature *adc_feature = &info->adc_feature;
> +	struct imx7d_adc_analogue_core_clk adc_analogure_clk;
> +	u32 i;
> +	u32 sample_rate = 0;
> +
> +	/*
> +	 * Before sample set, disable channel A,B,C,D. Here we
> +	 * clear the bit 31 of register REG_ADC_CH_A\B\C\D_CFG1.
> +	 */
> +	for (i = 0; i < 4; i++)
> +		writel(IMX7D_REG_ADC_CH_CFG1_CHANNEL_DISABLE,
> +			info->regs + i * IMX7D_EACH_CHANNEL_REG_SHIF);
> +
> +	adc_analogure_clk = imx7d_adc_analogue_clk[adc_feature->clk_pre_div];
> +	sample_rate |= adc_analogure_clk.reg_config;
> +	info->pre_div_num = adc_analogure_clk.pre_div;
> +
> +	sample_rate |= adc_feature->core_time_unit;
> +	writel(sample_rate, info->regs + IMX7D_REG_ADC_TIMER_UNIT);
> +}
> +
> +static void imx7d_adc_hw_init(struct imx7d_adc *info)
> +{
> +	u32 cfg;
> +	/* power up and enable adc analogue core */
> +	cfg = readl(info->regs + IMX7D_REG_ADC_ADC_CFG);
> +	cfg &= ~(IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN | IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN);
> +	cfg |= IMX7D_REG_ADC_ADC_CFG_ADC_EN;
> +	writel(cfg, info->regs + IMX7D_REG_ADC_ADC_CFG);
> +
> +	/* enable channel A,B,C,D interrupt */
> +	writel(IMX7D_REG_ADC_INT_CHANNEL_INT_EN, info->regs + IMX7D_REG_ADC_INT_SIG_EN);
> +	writel(IMX7D_REG_ADC_INT_CHANNEL_INT_EN, info->regs + IMX7D_REG_ADC_INT_EN);
> +
> +	imx7d_adc_sample_set(info);
> +}
> +
> +static void imx7d_adc_channel_set(struct imx7d_adc *info)
> +{
> +	u32 cfg1 = 0;
> +	u32 cfg2;
> +	u32 channel;
> +
> +	channel = info->channel;
> +
> +	/* the channel choose single conversion, and enable average mode */
> +	cfg1 |= (IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN | IMX7D_REG_ADC_CH_CFG1_CHANNEL_SINGLE);
I don't think readability would be hurt by breaking the above line to keep
it shorter.
> +	if (info->adc_feature.average_en)
> +		cfg1 |= IMX7D_REG_ADC_CH_CFG1_CHANNEL_AVG_EN;
> +
> +	/*
> +	 * physical channel 0 chose logical channel A
> +	 * physical channel 1 chose logical channel B
> +	 * physical channel 2 chose logical channel C
> +	 * physical channel 3 chose logical channel D
> +	 */
> +	cfg1 |= (channel << IMX7D_REG_ADC_CH_CFG1_CHANNEL_SEL_SHIF);
> +
> +	/* read register REG_ADC_CH_A\B\C\D_CFG2, according to the channel chosen */
> +	cfg2 = readl(info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel + 0x10);
> +
> +	cfg2 |= imx7d_adc_average_num[info->adc_feature.avg_num];
> +
> +	/* write the register REG_ADC_CH_A\B\C\D_CFG2, according to the channel chosen */
> +	writel(cfg2, info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel + 0x10);
> +	writel(cfg1, info->regs + IMX7D_EACH_CHANNEL_REG_SHIF * channel);
> +}
> +
> +static u32 imx7d_adc_get_sample_rate(struct imx7d_adc *info)
> +{
> +	/* input clock is always 24MHz */
> +	u32 input_clk = 24000000;
> +	u32 analogue_core_clk;
> +	u32 core_time_unit = info->adc_feature.core_time_unit;
> +	u32 sample_clk;
> +	u32 tmp;
> +
> +	analogue_core_clk = input_clk / info->pre_div_num;
> +	tmp = (core_time_unit + 1) * 6;
> +	sample_clk = analogue_core_clk / tmp;
> +
> +	return sample_clk;
> +}
> +
> +static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val,
> +			int *val2,
> +			long mask)
> +{
> +	struct imx7d_adc *info = iio_priv(indio_dev);
> +
> +	u32 channel;
> +	long ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		reinit_completion(&info->completion);
> +
> +		channel = (chan->channel) & 0x0f;
> +		info->channel = channel;
> +		imx7d_adc_channel_set(info);
> +
> +		ret = wait_for_completion_interruptible_timeout
> +				(&info->completion, IMX7D_ADC_TIMEOUT);
> +		if (ret == 0) {
> +			mutex_unlock(&indio_dev->mlock);
> +			return -ETIMEDOUT;
> +		}
> +		if (ret < 0) {
> +			mutex_unlock(&indio_dev->mlock);
> +			return ret;
> +		}
> +
> +		*val = info->value;
> +		mutex_unlock(&indio_dev->mlock);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = info->vref_uv / 1000;
> +		*val2 = 12;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = imx7d_adc_get_sample_rate(info);
> +		*val2 = 0;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int imx7d_adc_read_data(struct imx7d_adc *info)
> +{
> +	u32 channel;
> +	u32 value;
> +
> +	channel = info->channel;
> +
> +	/*
> +	 * channel A and B conversion result share one register,
> +	 * bit[27~16] is the channel B conversion result,
> +	 * bit[11~0] is the channel A conversion result.
> +	 * channel C and D is the same.
> +	 */
> +	if (channel < 2)
> +		value = readl(info->regs + IMX7D_REG_ADC_CHA_B_CNV_RSLT);
> +	else
> +		value = readl(info->regs + IMX7D_REG_ADC_CHC_D_CNV_RSLT);
> +	if (channel & 0x1)	/* channel B or D */
> +		value = (value >> 16) & 0xFFF;
> +	else			/* channel A or C */
> +		value &= 0xFFF;
> +
> +	return value;
> +}
> +
> +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id)
> +{
> +	struct imx7d_adc *info = (struct imx7d_adc *)dev_id;
> +	int status;
> +
> +	status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS);
> +	if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) {
> +		info->value = imx7d_adc_read_data(info);
> +		complete(&info->completion);
> +	}
> +	writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int imx7d_adc_reg_access(struct iio_dev *indio_dev,
> +			unsigned reg, unsigned writeval,
> +			unsigned *readval)
> +{
> +	struct imx7d_adc *info = iio_priv(indio_dev);
> +
> +	if ((readval == NULL) ||
> +		((reg % 4) || (reg > IMX7D_REG_ADC_ADC_CFG)))
> +		return -EINVAL;
> +
> +	*readval = readl(info->regs + reg);
> +
> +	return 0;
> +}
> +
> +static const struct iio_info imx7d_adc_iio_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = &imx7d_adc_read_raw,
> +	.debugfs_reg_access = &imx7d_adc_reg_access,
> +};
> +
> +static const struct of_device_id imx7d_adc_match[] = {
> +	{ .compatible = "fsl,imx7d-adc", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx7d_adc_match);
> +
> +static int imx7d_adc_probe(struct platform_device *pdev)
> +{
> +	struct imx7d_adc *info;
> +	struct iio_dev *indio_dev;
> +	struct resource *mem;
> +	int irq;
> +	int ret;
> +	u32 channels;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct imx7d_adc));
> +	if (!indio_dev) {
> +		dev_err(&pdev->dev, "Failed allocating iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	info = iio_priv(indio_dev);
> +	info->dev = &pdev->dev;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	info->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(info->regs)) {
> +		ret = PTR_ERR(info->regs);
> +		dev_err(&pdev->dev, "failed to remap adc memory: %d\n", ret);
> +		return ret;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "no irq resource?\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(info->dev, irq,
> +				imx7d_adc_isr, 0,
> +				dev_name(&pdev->dev), info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> +		return ret;
> +	}
> +
> +	info->clk = devm_clk_get(&pdev->dev, "adc");
> +	if (IS_ERR(info->clk)) {
> +		ret = PTR_ERR(info->clk);
> +		dev_err(&pdev->dev, "failed getting clock, err = %d\n", ret);
> +		return ret;
> +	}
> +
> +	info->vref = devm_regulator_get(&pdev->dev, "vref");
> +	if (IS_ERR(info->vref)) {
> +		ret = PTR_ERR(info->vref);
> +		dev_err(&pdev->dev, "failed getting reference voltage: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(info->vref);
> +	if (ret)
> +		return ret;
> +
> +	info->vref_uv = regulator_get_voltage(info->vref);
Given the use of this isn't typically on a fast path, I'd be tempted
to query the regulator when you need to know (i.e. in the scale
read).  That avoids the tiny possibility that the regulator voltage
has changed (which would be odd for a reference voltage source!)

> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	init_completion(&info->completion);
> +
> +	ret  = of_property_read_u32(pdev->dev.of_node,
> +					"num-channels", &channels);
> +	if (ret)
> +		channels = ARRAY_SIZE(imx7d_adc_iio_channels);
> +
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->info = &imx7d_adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = imx7d_adc_iio_channels;
> +	indio_dev->num_channels = (int)channels;
> +
> +	ret = clk_prepare_enable(info->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Could not prepare or enable the clock.\n");
> +		goto error_adc_clk_enable;
> +	}
> +
> +	imx7d_adc_feature_config(info);
> +	imx7d_adc_hw_init(info);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Couldn't register the device.\n");
> +		goto error_iio_device_register;
> +	}
> +
> +	return 0;
> +
> +error_iio_device_register:
> +	clk_disable_unprepare(info->clk);
> +error_adc_clk_enable:
> +	regulator_disable(info->vref);
> +
> +	return ret;
> +}
> +
> +static int imx7d_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct imx7d_adc *info = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	regulator_disable(info->vref);
> +	clk_disable_unprepare(info->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused imx7d_adc_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct imx7d_adc *info = iio_priv(indio_dev);
> +	u32 adc_cfg;
> +
> +	adc_cfg = readl(info->regs + IMX7D_REG_ADC_ADC_CFG);
> +	adc_cfg |= IMX7D_REG_ADC_ADC_CFG_ADC_CLK_DOWN | IMX7D_REG_ADC_ADC_CFG_ADC_POWER_DOWN;
Readability isn't going to be greatly reduced by breaking the line above, so
in this case I'd keep to the 80 char limit.

> +	adc_cfg &= ~IMX7D_REG_ADC_ADC_CFG_ADC_EN;
> +	writel(adc_cfg, info->regs + IMX7D_REG_ADC_ADC_CFG);
> +
> +	clk_disable_unprepare(info->clk);
> +	regulator_disable(info->vref);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused imx7d_adc_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct imx7d_adc *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regulator_enable(info->vref);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(info->clk);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"Could not prepare or enable clock.\n");
> +		regulator_disable(info->vref);
> +		return ret;
> +	}
> +
> +	imx7d_adc_hw_init(info);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(imx7d_adc_pm_ops, imx7d_adc_suspend, imx7d_adc_resume);
> +
> +static struct platform_driver imx7d_adc_driver = {
> +	.probe		= imx7d_adc_probe,
> +	.remove		= imx7d_adc_remove,
> +	.driver		= {
> +		.name	= DRIVER_NAME,
> +		.of_match_table = imx7d_adc_match,
> +		.pm	= &imx7d_adc_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(imx7d_adc_driver);
> +
> +MODULE_AUTHOR("Haibo Chen <haibo.chen@freescale.com>");
> +MODULE_DESCRIPTION("Freeacale IMX7D ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

[PATCH v2 2/4] Documentation: add the binding file for Freescale imx7d ADC driver

[Haibo Chen]

The patch adds the binding file for Freescale imx7d ADC driver.

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 .../devicetree/bindings/iio/adc/imx7d-adc.txt      | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/imx7d-adc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/imx7d-adc.txt b/Documentation/devicetree/bindings/iio/adc/imx7d-adc.txt
new file mode 100644
index 0000000..7f4ec1b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/imx7d-adc.txt
@@ -0,0 +1,26 @@
+Freescale imx7d ADC bindings
+
+The devicetree bindings are for the ADC driver written for
+imx7d SoC.
+
+Required properties:
+- compatible: Should be "fsl,imx7d-adc"
+- reg: Offset and length of the register set for the ADC device
+- interrupts: The interrupt number for the ADC device
+- clocks: The root clock of the ADC controller
+- clock-names: Must contain "adc", matching entry in the clocks property
+- vref-supply: The regulator supply ADC reference voltage
+
+Optional properties:
+- num-channels: the number of channels used
+
+Example:
+adc1: adc at 30610000 {
+	compatible = "fsl,imx7d-adc";
+	reg = <0x30610000 0x10000>;
+	interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&clks IMX7D_ADC_ROOT_CLK>;
+	clock-names = "adc";
+	num-channels = <4>;
+	vref-supply = <&reg_vcc_3v3_mcu>;
+};
-- 
1.9.1

[PATCH v2 2/4] Documentation: add the binding file for Freescale imx7d ADC driver

[Rob Herring]

On Mon, Nov 09, 2015 at 09:28:22PM +0800, Haibo Chen wrote:
> The patch adds the binding file for Freescale imx7d ADC driver.
> 
> Signed-off-by: Haibo Chen <haibo.chen@freescale.com>

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  .../devicetree/bindings/iio/adc/imx7d-adc.txt      | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/imx7d-adc.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/imx7d-adc.txt b/Documentation/devicetree/bindings/iio/adc/imx7d-adc.txt
> new file mode 100644
> index 0000000..7f4ec1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/imx7d-adc.txt
> @@ -0,0 +1,26 @@
> +Freescale imx7d ADC bindings
> +
> +The devicetree bindings are for the ADC driver written for
> +imx7d SoC.
> +
> +Required properties:
> +- compatible: Should be "fsl,imx7d-adc"
> +- reg: Offset and length of the register set for the ADC device
> +- interrupts: The interrupt number for the ADC device
> +- clocks: The root clock of the ADC controller
> +- clock-names: Must contain "adc", matching entry in the clocks property
> +- vref-supply: The regulator supply ADC reference voltage
> +
> +Optional properties:
> +- num-channels: the number of channels used
> +
> +Example:
> +adc1: adc at 30610000 {
> +	compatible = "fsl,imx7d-adc";
> +	reg = <0x30610000 0x10000>;
> +	interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> +	clocks = <&clks IMX7D_ADC_ROOT_CLK>;
> +	clock-names = "adc";
> +	num-channels = <4>;
> +	vref-supply = <&reg_vcc_3v3_mcu>;
> +};
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] ARM: dts: imx7d.dtsi: add ADC support

[Haibo Chen]

Add imx7d ADC support.

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 arch/arm/boot/dts/imx7d.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
index ebc053a..87c3319 100644
--- a/arch/arm/boot/dts/imx7d.dtsi
+++ b/arch/arm/boot/dts/imx7d.dtsi
@@ -583,6 +583,26 @@
 			reg = <0x30400000 0x400000>;
 			ranges;
 
+			adc1: adc at 30610000 {
+				compatible = "fsl,imx7d-adc";
+				reg = <0x30610000 0x10000>;
+				interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_ADC_ROOT_CLK>;
+				num-channels = <4>;
+				clock-names = "adc";
+				status = "disabled";
+			};
+
+			adc2: adc at 30620000 {
+				compatible = "fsl,imx7d-adc";
+				reg = <0x30620000 0x10000>;
+				interrupts = <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_ADC_ROOT_CLK>;
+				num-channels = <4>;
+				clock-names = "adc";
+				status = "disabled";
+			};
+
 			pwm1: pwm at 30660000 {
 				compatible = "fsl,imx7d-pwm", "fsl,imx27-pwm";
 				reg = <0x30660000 0x10000>;
-- 
1.9.1

[PATCH v2 4/4] ARM: dts: imx7d-sdb: add ADC support

[Haibo Chen]

Add ADC support for imx7d-sdb board.

Signed-off-by: Haibo Chen <haibo.chen@freescale.com>
---
 arch/arm/boot/dts/imx7d-sdb.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/imx7d-sdb.dts b/arch/arm/boot/dts/imx7d-sdb.dts
index 432aaf5..b2c4536 100644
--- a/arch/arm/boot/dts/imx7d-sdb.dts
+++ b/arch/arm/boot/dts/imx7d-sdb.dts
@@ -97,6 +97,16 @@
 	};
 };
 
+&adc1 {
+	vref-supply = <&reg_vref_1v8>;
+	status = "okay";
+};
+
+&adc2 {
+	vref-supply = <&reg_vref_1v8>;
+	status = "okay";
+};
+
 &cpu0 {
 	arm-supply = <&sw1a_reg>;
 };
-- 
1.9.1