[PATCH 0/5] Introduce at91_adc8xx driver

[PATCH 0/5] Introduce at91_adc8xx driver

[Ludovic Desroches]

Hi,

This is the very basic support for the adc introduced with the SAMA5D2 SoC
family.

The goal is to provide something to the user as soon as possible instead of
waiting for a full featured driver.

Only unsigned conversions on a software tigger are supported. Next steps are
signed conversions, differential channels, hardware triggers, dma support,
touchscreen support, and others.

Ludovic Desroches (5):
  iio:adc:at91_adc8xx: introduce new atmel adc driver
  MAINTAINERS: add entry for Atmel ADC 8xx driver
  ARM: at91/dt: sama5d2: add adc device
  ARM: at91/dt: sama5d2 Xplained: enable the adc device
  ARM: at91/defconfig: add sama5d2 adc support in sama5_defconfig

 .../devicetree/bindings/iio/adc/at91_adc8xx.txt    |  27 ++
 MAINTAINERS                                        |   6 +
 arch/arm/boot/dts/at91-sama5d2_xplained.dts        |  22 ++
 arch/arm/boot/dts/sama5d2.dtsi                     |  11 +-
 arch/arm/configs/sama5_defconfig                   |   1 +
 drivers/iio/adc/Kconfig                            |  11 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/at91_adc8xx.c                      | 417 +++++++++++++++++++++
 8 files changed, 495 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
 create mode 100644 drivers/iio/adc/at91_adc8xx.c

-- 
2.5.0

[PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Ludovic Desroches]

This driver supports the new version of the Atmel ADC device introduced
with the SAMA5D2 SoC family.

Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/iio/adc/at91_adc8xx.txt    |  27 ++
 drivers/iio/adc/Kconfig                            |  11 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/at91_adc8xx.c                      | 417 +++++++++++++++++++++
 4 files changed, 456 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
 create mode 100644 drivers/iio/adc/at91_adc8xx.c

diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
new file mode 100644
index 0000000..64ad6a5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
@@ -0,0 +1,27 @@
+* AT91 SAMA5D2 Analog to Digital Converter (ADC)
+
+Required properties:
+  - compatible: Should be "atmel,sama5d2-adc".
+  - reg: Should contain ADC registers location and length.
+  - interrupts: Should contain the IRQ line for the ADC.
+  - clocks: phandles to clocks.
+  - clock-names: tuple listing clock names.
+      Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
+      clock, "adc_clk" is the sampling clock.
+  - vref-supply: Supply used as reference for conversions.
+
+Optional properties:
+  - vddana-supply: Supply for the adc device.
+
+
+Example:
+
+adc: adc@fc030000 {
+	compatible = "atmel,sama5d2-adc";
+	reg = <0xfc030000 0x100>;
+	interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
+	clocks = <&adc_clk>, <&adc_op_clk>;
+	clock-names = "adc_clk", "adc_op_clk";
+	vddana-supply = <&vdd_3v3_lp_reg>;
+	vref-supply = <&vdd_3v3_lp_reg>;
+}
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 9162dfe..5819e41 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -131,6 +131,17 @@ config AT91_ADC
 	  To compile this driver as a module, choose M here: the module will be
 	  called at91_adc.
 
+config AT91_ADC8xx
+	tristate "Atmel AT91 ADC 8xx"
+	depends on ARCH_AT91
+	depends on INPUT
+	help
+	  Say yes here to build support for Atmel ADC 8xx which is available
+	  from SAMA5D2 SoC family.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called at91_adc8xx.
+
 config AXP288_ADC
 	tristate "X-Powers AXP288 ADC driver"
 	depends on MFD_AXP20X
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 91a65bf..d684a52 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
 obj-$(CONFIG_AD7887) += ad7887.o
 obj-$(CONFIG_AD799X) += ad799x.o
 obj-$(CONFIG_AT91_ADC) += at91_adc.o
+obj-$(CONFIG_AT91_ADC8xx) += at91_adc8xx.o
 obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
 obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
 obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
diff --git a/drivers/iio/adc/at91_adc8xx.c b/drivers/iio/adc/at91_adc8xx.c
new file mode 100644
index 0000000..8b4a6e7
--- /dev/null
+++ b/drivers/iio/adc/at91_adc8xx.c
@@ -0,0 +1,417 @@
+/*
+ * Atmel ADC driver for SAMA5D2 devices and later.
+ *
+ * Copyright (C) 2015 Atmel,
+ *               2015 Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/wait.h>
+#include <linux/iio/iio.h>
+#include <linux/regulator/consumer.h>
+
+#define ADC_CR		0x00			/* Control Register */
+#define		ADC_CR_SWRST		BIT(0)		/* Software Reset */
+#define		ADC_CR_START		BIT(1)		/* Start Conversion */
+#define		ADC_CR_TSCALIB		BIT(2)		/* Touchscreen Calibration */
+#define		ADC_CR_CMPRST		BIT(4)		/* Comparison Restart */
+#define ADC_MR		0x04			/* Mode Register */
+#define		ADC_MR_TRGSEL(v)	(v << 1)	/* Trigger Selection */
+#define			ADC_MR_TRGSEL_TRIG0	0		/* ADTRG */
+#define			ADC_MR_TRGSEL_TRIG1	1		/* TIOA0 */
+#define			ADC_MR_TRGSEL_TRIG2	2		/* TIOA1 */
+#define			ADC_MR_TRGSEL_TRIG3	3		/* TIOA2 */
+#define			ADC_MR_TRGSEL_TRIG4	4		/* PWM event line 0 */
+#define			ADC_MR_TRGSEL_TRIG5	5		/* PWM event line 1 */
+#define			ADC_MR_TRGSEL_TRIG6	6		/* TIOA3 */
+#define			ADC_MR_TRGSEL_TRIG7	7		/* RTCOUT0 */
+#define		ADC_MR_SLEEP		BIT(5)		/* Sleep Mode */
+#define		ADC_MR_FWUP		BIT(6)		/* Fast Wake Up */
+#define		ADC_MR_PRESCAL(v)	(v << 8)	/* Prescaler Rate Selection */
+#define			ADC_MR_PRESCAL_MAX	0xffff
+#define		ADC_MR_STARTUP(v)	(v << 16)	/* Startup Time */
+#define			ADC_MR_STARTUP_SUT0	0		/* 0 period of ADCCLK */
+#define			ADC_MR_STARTUP_SUT8	1		/* 8 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT16	2		/* 16 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT24	3		/* 24 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT64	4		/* 64 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT80	5		/* 80 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT96	6		/* 96 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT112	7		/* 112 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT512	8		/* 512 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT576	9		/* 576 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT640	10		/* 640 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT704	11		/* 704 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT768	12		/* 768 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT832	13		/* 832 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT896	14		/* 896 periods of ADCCLK */
+#define			ADC_MR_STARTUP_SUT960	15		/* 960 periods of ADCCLK */
+#define		ADC_MR_ANACH		BIT(23)		/* Analog Change */
+#define		ADC_MR_TRACKTIM(v)	(v << 24)	/* Tracking Time */
+#define			ADC_MR_TRACKTIM_MAX	0xff
+#define		ADC_MR_TRANSFER(v)	(v << 28)	/* Transfer Time */
+#define			ADC_MR_TRANSFER_MAX	0x3
+#define		ADC_MR_USEQ		BIT(31)		/* Use Sequence Enable */
+#define ADC_SEQR1	0x08			/* Channel Sequence Register 1 */
+#define ADC_SEQR2	0x0c			/* Channel Sequence Register 2 */
+#define ADC_CHER	0x10			/* Channel Enable Register */
+#define ADC_CHDR	0x14			/* Channel Disable Register */
+#define ADC_CHSR	0x18			/* Channel Status Register */
+#define ADC_LCDR	0x20			/* Last Converted Data Register */
+#define ADC_IER		0x24			/* Interrupt Enable Register */
+#define ADC_IDR		0x28			/* Interrupt Disable Register */
+#define ADC_IMR		0x2c			/* Interrupt Mask Register */
+#define ADC_ISR		0x30			/* Interrupt Status Register */
+#define ADC_LCTMR	0x34			/* Last Channel Trigger Mode Register */
+#define ADC_LCCWR	0x38			/* Last Channel Compare Window Register */
+#define ADC_OVER	0x3c			/* Overrun Status Register */
+#define ADC_EMR		0x40			/* Extended Mode Register */
+#define ADC_CWR		0x44			/* Compare Window Register */
+#define ADC_CGR		0x48			/* Channel Gain Register */
+#define ADC_COR		0x4c			/* Channel Offset Register */
+#define ADC_CDR0	0x50			/* Channel Data Register 0 */
+#define ADC_ACR		0x94			/* Analog Control Register */
+#define ADC_TSMR	0xb0			/* Touchscreen Mode Register */
+#define ADC_XPOSR	0xb4			/* Touchscreen X Position Register */
+#define ADC_YPOSR	0xb8			/* Touchscreen Y Position Register */
+#define ADC_PRESSR	0xbc			/* Touchscreen Pressure Register */
+#define ADC_TRGR	0xc0			/* Trigger Register */
+#define ADC_COSR	0xd0			/* Correction Select Register */
+#define ADC_CVR		0xd4			/* Correction Value Register */
+#define ADC_CECR	0xd8			/* Channel Error Correction Register */
+#define ADC_WPMR	0xe4			/* Write Protection Mode Register */
+#define ADC_WPSR	0xe8			/* Write Protection Status Register */
+#define ADC_VERSION	0xfc			/* Version Register */
+
+#define AT91_ADC_CHAN(num, addr)					\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.channel = num,						\
+		.address = addr,					\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = 12,					\
+			.storagebits = 14,				\
+		},							\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+		.datasheet_name = "CH"#num,				\
+		.indexed = 1,						\
+	}
+
+#define at91_adc_readl(st, reg)		readl_relaxed(st->base + reg)
+#define at91_adc_writel(st, reg, val)	writel_relaxed(val, st->base + reg)
+
+struct at91_adc_soc_info {
+	unsigned			startup_time;
+	unsigned			min_f_adc;
+	unsigned			max_f_adc;
+	const struct iio_chan_spec	*channels;
+	int				num_channels;
+};
+
+struct at91_adc_state {
+	void __iomem			*base;
+	struct clk			*per_clk;
+	struct clk			*adc_clk;
+	int				irq;
+	struct regulator		*reg;
+	struct regulator		*vref;
+	u32				vref_uv;
+	struct at91_adc_soc_info	*soc_info;
+	wait_queue_head_t		wq_data_available;
+	bool				done;
+	const struct iio_chan_spec	*chan;
+	u32				last_value;
+	struct mutex			lock;
+};
+
+static struct at91_adc_soc_info at91_adc_sama5d2_soc_info = {
+	.startup_time = 4,
+	.min_f_adc = 200000,
+	.max_f_adc = 20000000,
+};
+
+static const struct iio_chan_spec at91_adc_channels[] = {
+	AT91_ADC_CHAN(0, 0x50),
+	AT91_ADC_CHAN(1, 0x54),
+	AT91_ADC_CHAN(2, 0x58),
+	AT91_ADC_CHAN(3, 0x5c),
+	AT91_ADC_CHAN(4, 0x60),
+	AT91_ADC_CHAN(5, 0x64),
+	AT91_ADC_CHAN(6, 0x68),
+	AT91_ADC_CHAN(7, 0x6c),
+	AT91_ADC_CHAN(8, 0x70),
+	AT91_ADC_CHAN(9, 0x74),
+	AT91_ADC_CHAN(10, 0x78),
+	AT91_ADC_CHAN(11, 0x7c),
+};
+
+static irqreturn_t at91_adc_interrupt(int irq, void *private)
+{
+	struct iio_dev *indio = private;
+	struct at91_adc_state *st = iio_priv(indio);
+	u32 status = at91_adc_readl(st, ADC_ISR);
+
+	status &= at91_adc_readl(st, ADC_IMR);
+	if (status & 0xFFF) {
+		st->last_value = at91_adc_readl(st, st->chan->address);
+		st->done = true;
+		wake_up_interruptible(&st->wq_data_available);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static bool at91_adc_freq_supported(struct at91_adc_state *st, unsigned freq)
+{
+	return ((freq >= st->soc_info->min_f_adc)
+		&& (freq <= st->soc_info->max_f_adc));
+}
+
+static unsigned at91_adc_startup_time(unsigned startup_time_min,
+				      unsigned adc_clk_khz)
+{
+	const unsigned startup_lookup[] = {
+		0,     8,  16,  24,
+		64,   80,  96, 112,
+		512, 576, 640, 704,
+		768, 832, 896, 960
+		};
+	unsigned ticks_min, i;
+
+	/*
+	 * Since the adc frequency is checked before, there is no reason
+	 * to not meet the startup time constraint.
+	 */
+
+	ticks_min = startup_time_min * adc_clk_khz / 1000;
+	for (i = 0; i < ARRAY_SIZE(startup_lookup); i++)
+		if (startup_lookup[i] > ticks_min)
+			break;
+
+	return i;
+}
+
+static int at91_adc_init(struct at91_adc_state *st)
+{
+	struct iio_dev *indio_dev = iio_priv_to_dev(st);
+	unsigned f_adc, f_per, prescal, startup;
+
+	at91_adc_writel(st, ADC_CR, ADC_CR_SWRST);
+	at91_adc_writel(st, ADC_IDR, 0xffffffff);
+
+	f_adc = clk_get_rate(st->adc_clk);
+	if (!at91_adc_freq_supported(st, f_adc)) {
+		dev_err(&indio_dev->dev, "unsupported adc clock frequency\n");
+		return -EINVAL;
+	}
+
+	f_per = clk_get_rate(st->per_clk);
+	prescal = (f_per / (2 * f_adc)) - 1;
+
+	startup = at91_adc_startup_time(st->soc_info->startup_time,
+					f_adc / 1000);
+
+	at91_adc_writel(st, ADC_MR,
+			ADC_MR_TRANSFER(2)
+			| ADC_MR_STARTUP(startup)
+			| ADC_MR_PRESCAL(prescal));
+
+	dev_dbg(&indio_dev->dev, "startup: %u, prescal: %u\n",
+		startup, prescal);
+
+	return 0;
+}
+
+static int at91_adc_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct at91_adc_state *st = iio_priv(indio_dev);
+	int ret;
+
+	st->chan = chan;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&st->lock);
+
+		at91_adc_writel(st, ADC_CHER, BIT(chan->channel));
+		at91_adc_writel(st, ADC_IER, BIT(chan->channel));
+		at91_adc_writel(st, ADC_CR, ADC_CR_START);
+
+		ret = wait_event_interruptible_timeout(st->wq_data_available,
+						       st->done,
+						       msecs_to_jiffies(1000));
+		if (ret == 0)
+			ret = -ETIMEDOUT;
+
+		if (ret <= 0) {
+			at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
+			at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
+			mutex_unlock(&st->lock);
+			return ret;
+		}
+
+		if (chan->scan_type.sign == 's')
+			*val = sign_extend32(st->last_value,
+					     chan->scan_type.realbits - 1);
+		else
+			*val = st->last_value;
+		at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
+		at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
+		st->done = false;
+
+		mutex_unlock(&st->lock);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_uv / 1000;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info at91_adc_info = {
+	.read_raw = &at91_adc_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int at91_adc_probe(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev;
+	struct at91_adc_state *st;
+	struct resource	*res;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev,
+					  sizeof(struct at91_adc_state));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &at91_adc_info;
+	indio_dev->channels = at91_adc_channels;
+	indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
+
+	st = iio_priv(indio_dev);
+	st->soc_info = (struct at91_adc_soc_info *)
+			of_device_get_match_data(&pdev->dev);
+	init_waitqueue_head(&st->wq_data_available);
+	mutex_init(&st->lock);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+
+	st->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(st->base))
+		return PTR_ERR(st->base);
+
+	st->irq = platform_get_irq(pdev, 0);
+	if (st->irq < 0)
+		return st->irq;
+
+	st->per_clk = devm_clk_get(&pdev->dev, "adc_clk");
+	if (IS_ERR(st->per_clk))
+		return PTR_ERR(st->per_clk);
+
+	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
+	if (IS_ERR(st->adc_clk))
+		return PTR_ERR(st->adc_clk);
+
+	st->reg = devm_regulator_get(&pdev->dev, "vddana");
+	if (IS_ERR(st->reg))
+		return PTR_ERR(st->reg);
+
+	st->vref = devm_regulator_get(&pdev->dev, "vref");
+	if (IS_ERR(st->vref))
+		return PTR_ERR(st->vref);
+
+	ret = devm_request_irq(&pdev->dev, st->irq, at91_adc_interrupt, 0,
+			       pdev->dev.driver->name, indio_dev);
+	if (ret)
+		return ret;
+
+	st->vref_uv = regulator_get_voltage(st->vref);
+	if (st->vref_uv <= 0) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	ret = at91_adc_init(st);
+	if (ret)
+		goto error;
+
+	ret = clk_prepare_enable(st->adc_clk);
+	ret = clk_prepare_enable(st->per_clk);
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0)
+		return ret;
+
+	dev_info(&pdev->dev, "version: %x\n",
+		 readl_relaxed(st->base + ADC_VERSION));
+
+error:
+	return ret;
+}
+
+static int at91_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct at91_adc_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	clk_disable_unprepare(st->per_clk);
+	clk_disable_unprepare(st->adc_clk);
+
+	return 0;
+}
+
+static const struct of_device_id at91_adc_dt_match[] = {
+	{
+		.compatible = "atmel,sama5d2-adc",
+		.data = &at91_adc_sama5d2_soc_info
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
+
+static struct platform_driver at91_adc_driver = {
+	.probe = at91_adc_probe,
+	.remove = at91_adc_remove,
+	.driver = {
+		.name = "at91_adc8xx",
+		.of_match_table = at91_adc_dt_match,
+	},
+};
+module_platform_driver(at91_adc_driver)
+
+MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Jonathan Cameron]

On 21/12/15 09:24, Ludovic Desroches wrote:
> This driver supports the new version of the Atmel ADC device introduced
> with the SAMA5D2 SoC family.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
A few more bits and bobs from me. Mostly looking good.

Jonathan
> ---
>  .../devicetree/bindings/iio/adc/at91_adc8xx.txt    |  27 ++
>  drivers/iio/adc/Kconfig                            |  11 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/at91_adc8xx.c                      | 417 +++++++++++++++++++++
>  4 files changed, 456 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
>  create mode 100644 drivers/iio/adc/at91_adc8xx.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> new file mode 100644
> index 0000000..64ad6a5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> @@ -0,0 +1,27 @@
> +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> +
> +Required properties:
> +  - compatible: Should be "atmel,sama5d2-adc".
> +  - reg: Should contain ADC registers location and length.
> +  - interrupts: Should contain the IRQ line for the ADC.
> +  - clocks: phandles to clocks.
> +  - clock-names: tuple listing clock names.
> +      Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> +      clock, "adc_clk" is the sampling clock.
> +  - vref-supply: Supply used as reference for conversions.
> +
> +Optional properties:
> +  - vddana-supply: Supply for the adc device.
> +
> +
> +Example:
> +
> +adc: adc@fc030000 {
> +	compatible = "atmel,sama5d2-adc";
> +	reg = <0xfc030000 0x100>;
> +	interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
> +	clocks = <&adc_clk>, <&adc_op_clk>;
> +	clock-names = "adc_clk", "adc_op_clk";
> +	vddana-supply = <&vdd_3v3_lp_reg>;
> +	vref-supply = <&vdd_3v3_lp_reg>;
> +}
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 9162dfe..5819e41 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -131,6 +131,17 @@ config AT91_ADC
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called at91_adc.
>  
> +config AT91_ADC8xx
> +	tristate "Atmel AT91 ADC 8xx"
> +	depends on ARCH_AT91
> +	depends on INPUT
> +	help
> +	  Say yes here to build support for Atmel ADC 8xx which is available
> +	  from SAMA5D2 SoC family.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called at91_adc8xx.
> +
>  config AXP288_ADC
>  	tristate "X-Powers AXP288 ADC driver"
>  	depends on MFD_AXP20X
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 91a65bf..d684a52 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
>  obj-$(CONFIG_AD799X) += ad799x.o
>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
> +obj-$(CONFIG_AT91_ADC8xx) += at91_adc8xx.o
>  obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>  obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> diff --git a/drivers/iio/adc/at91_adc8xx.c b/drivers/iio/adc/at91_adc8xx.c
> new file mode 100644
> index 0000000..8b4a6e7
> --- /dev/null
> +++ b/drivers/iio/adc/at91_adc8xx.c
> @@ -0,0 +1,417 @@
> +/*
> + * Atmel ADC driver for SAMA5D2 devices and later.
> + *
> + * Copyright (C) 2015 Atmel,
> + *               2015 Ludovic Desroches <ludovic.desroches@atmel.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sched.h>
> +#include <linux/wait.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define ADC_CR		0x00			/* Control Register */
> +#define		ADC_CR_SWRST		BIT(0)		/* Software Reset */
> +#define		ADC_CR_START		BIT(1)		/* Start Conversion */
> +#define		ADC_CR_TSCALIB		BIT(2)		/* Touchscreen Calibration */
> +#define		ADC_CR_CMPRST		BIT(4)		/* Comparison Restart */
> +#define ADC_MR		0x04			/* Mode Register */
> +#define		ADC_MR_TRGSEL(v)	(v << 1)	/* Trigger Selection */
> +#define			ADC_MR_TRGSEL_TRIG0	0		/* ADTRG */
> +#define			ADC_MR_TRGSEL_TRIG1	1		/* TIOA0 */
> +#define			ADC_MR_TRGSEL_TRIG2	2		/* TIOA1 */
> +#define			ADC_MR_TRGSEL_TRIG3	3		/* TIOA2 */
> +#define			ADC_MR_TRGSEL_TRIG4	4		/* PWM event line 0 */
> +#define			ADC_MR_TRGSEL_TRIG5	5		/* PWM event line 1 */
> +#define			ADC_MR_TRGSEL_TRIG6	6		/* TIOA3 */
> +#define			ADC_MR_TRGSEL_TRIG7	7		/* RTCOUT0 */
> +#define		ADC_MR_SLEEP		BIT(5)		/* Sleep Mode */
> +#define		ADC_MR_FWUP		BIT(6)		/* Fast Wake Up */
> +#define		ADC_MR_PRESCAL(v)	(v << 8)	/* Prescaler Rate Selection */
> +#define			ADC_MR_PRESCAL_MAX	0xffff
> +#define		ADC_MR_STARTUP(v)	(v << 16)	/* Startup Time */
> +#define			ADC_MR_STARTUP_SUT0	0		/* 0 period of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT8	1		/* 8 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT16	2		/* 16 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT24	3		/* 24 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT64	4		/* 64 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT80	5		/* 80 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT96	6		/* 96 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT112	7		/* 112 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT512	8		/* 512 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT576	9		/* 576 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT640	10		/* 640 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT704	11		/* 704 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT768	12		/* 768 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT832	13		/* 832 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT896	14		/* 896 periods of ADCCLK */
> +#define			ADC_MR_STARTUP_SUT960	15		/* 960 periods of ADCCLK */
> +#define		ADC_MR_ANACH		BIT(23)		/* Analog Change */
> +#define		ADC_MR_TRACKTIM(v)	(v << 24)	/* Tracking Time */
> +#define			ADC_MR_TRACKTIM_MAX	0xff
> +#define		ADC_MR_TRANSFER(v)	(v << 28)	/* Transfer Time */
> +#define			ADC_MR_TRANSFER_MAX	0x3
> +#define		ADC_MR_USEQ		BIT(31)		/* Use Sequence Enable */
> +#define ADC_SEQR1	0x08			/* Channel Sequence Register 1 */
> +#define ADC_SEQR2	0x0c			/* Channel Sequence Register 2 */
> +#define ADC_CHER	0x10			/* Channel Enable Register */
> +#define ADC_CHDR	0x14			/* Channel Disable Register */
> +#define ADC_CHSR	0x18			/* Channel Status Register */
> +#define ADC_LCDR	0x20			/* Last Converted Data Register */
> +#define ADC_IER		0x24			/* Interrupt Enable Register */
> +#define ADC_IDR		0x28			/* Interrupt Disable Register */
> +#define ADC_IMR		0x2c			/* Interrupt Mask Register */
> +#define ADC_ISR		0x30			/* Interrupt Status Register */
> +#define ADC_LCTMR	0x34			/* Last Channel Trigger Mode Register */
> +#define ADC_LCCWR	0x38			/* Last Channel Compare Window Register */
> +#define ADC_OVER	0x3c			/* Overrun Status Register */
> +#define ADC_EMR		0x40			/* Extended Mode Register */
> +#define ADC_CWR		0x44			/* Compare Window Register */
> +#define ADC_CGR		0x48			/* Channel Gain Register */
> +#define ADC_COR		0x4c			/* Channel Offset Register */
> +#define ADC_CDR0	0x50			/* Channel Data Register 0 */
> +#define ADC_ACR		0x94			/* Analog Control Register */
> +#define ADC_TSMR	0xb0			/* Touchscreen Mode Register */
> +#define ADC_XPOSR	0xb4			/* Touchscreen X Position Register */
> +#define ADC_YPOSR	0xb8			/* Touchscreen Y Position Register */
> +#define ADC_PRESSR	0xbc			/* Touchscreen Pressure Register */
> +#define ADC_TRGR	0xc0			/* Trigger Register */
> +#define ADC_COSR	0xd0			/* Correction Select Register */
> +#define ADC_CVR		0xd4			/* Correction Value Register */
> +#define ADC_CECR	0xd8			/* Channel Error Correction Register */
> +#define ADC_WPMR	0xe4			/* Write Protection Mode Register */
> +#define ADC_WPSR	0xe8			/* Write Protection Status Register */
> +#define ADC_VERSION	0xfc			/* Version Register */
> +
> +#define AT91_ADC_CHAN(num, addr)					\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.channel = num,						\
> +		.address = addr,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 12,					\
> +			.storagebits = 14,				\
> +		},							\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +		.datasheet_name = "CH"#num,				\
> +		.indexed = 1,						\
> +	}
> +
> +#define at91_adc_readl(st, reg)		readl_relaxed(st->base + reg)
> +#define at91_adc_writel(st, reg, val)	writel_relaxed(val, st->base + reg)
> +
> +struct at91_adc_soc_info {
> +	unsigned			startup_time;
> +	unsigned			min_f_adc;
> +	unsigned			max_f_adc;
> +	const struct iio_chan_spec	*channels;
> +	int				num_channels;
> +};
> +
> +struct at91_adc_state {
> +	void __iomem			*base;
> +	struct clk			*per_clk;
> +	struct clk			*adc_clk;
> +	int				irq;
> +	struct regulator		*reg;
> +	struct regulator		*vref;
> +	u32				vref_uv;
> +	struct at91_adc_soc_info	*soc_info;
> +	wait_queue_head_t		wq_data_available;
> +	bool				done;
> +	const struct iio_chan_spec	*chan;
> +	u32				last_value;
> +	struct mutex			lock;
> +};
> +
> +static struct at91_adc_soc_info at91_adc_sama5d2_soc_info = {
> +	.startup_time = 4,
> +	.min_f_adc = 200000,
> +	.max_f_adc = 20000000,
These rather look like things that should be in the device tree if they
are going to change between SoC varients. 
> +};
> +
> +static const struct iio_chan_spec at91_adc_channels[] = {
> +	AT91_ADC_CHAN(0, 0x50),
> +	AT91_ADC_CHAN(1, 0x54),
> +	AT91_ADC_CHAN(2, 0x58),
> +	AT91_ADC_CHAN(3, 0x5c),
> +	AT91_ADC_CHAN(4, 0x60),
> +	AT91_ADC_CHAN(5, 0x64),
> +	AT91_ADC_CHAN(6, 0x68),
> +	AT91_ADC_CHAN(7, 0x6c),
> +	AT91_ADC_CHAN(8, 0x70),
> +	AT91_ADC_CHAN(9, 0x74),
> +	AT91_ADC_CHAN(10, 0x78),
> +	AT91_ADC_CHAN(11, 0x7c),
> +};
> +
> +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> +{
> +	struct iio_dev *indio = private;
> +	struct at91_adc_state *st = iio_priv(indio);
> +	u32 status = at91_adc_readl(st, ADC_ISR);
> +
> +	status &= at91_adc_readl(st, ADC_IMR);
> +	if (status & 0xFFF) {
> +		st->last_value = at91_adc_readl(st, st->chan->address);
If this is a polled read - is there any reason to read this value here
rather than outside the interrupt?
> +		st->done = true;
> +		wake_up_interruptible(&st->wq_data_available);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static bool at91_adc_freq_supported(struct at91_adc_state *st, unsigned freq)
> +{
> +	return ((freq >= st->soc_info->min_f_adc)
> +		&& (freq <= st->soc_info->max_f_adc));
> +}
> +
> +static unsigned at91_adc_startup_time(unsigned startup_time_min,
> +				      unsigned adc_clk_khz)
> +{
> +	const unsigned startup_lookup[] = {
> +		0,     8,  16,  24,
> +		64,   80,  96, 112,
> +		512, 576, 640, 704,
> +		768, 832, 896, 960
> +		};
> +	unsigned ticks_min, i;
> +
> +	/*
> +	 * Since the adc frequency is checked before, there is no reason
> +	 * to not meet the startup time constraint.
> +	 */
> +
> +	ticks_min = startup_time_min * adc_clk_khz / 1000;
> +	for (i = 0; i < ARRAY_SIZE(startup_lookup); i++)
> +		if (startup_lookup[i] > ticks_min)
> +			break;
> +
> +	return i;
> +}
> +
> +static int at91_adc_init(struct at91_adc_state *st)
> +{
> +	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> +	unsigned f_adc, f_per, prescal, startup;
> +
> +	at91_adc_writel(st, ADC_CR, ADC_CR_SWRST);
> +	at91_adc_writel(st, ADC_IDR, 0xffffffff);
> +
> +	f_adc = clk_get_rate(st->adc_clk);
> +	if (!at91_adc_freq_supported(st, f_adc)) {
> +		dev_err(&indio_dev->dev, "unsupported adc clock frequency\n");
> +		return -EINVAL;
> +	}
> +
> +	f_per = clk_get_rate(st->per_clk);
> +	prescal = (f_per / (2 * f_adc)) - 1;
> +
> +	startup = at91_adc_startup_time(st->soc_info->startup_time,
> +					f_adc / 1000);
> +
> +	at91_adc_writel(st, ADC_MR,
> +			ADC_MR_TRANSFER(2)
> +			| ADC_MR_STARTUP(startup)
> +			| ADC_MR_PRESCAL(prescal));
> +
> +	dev_dbg(&indio_dev->dev, "startup: %u, prescal: %u\n",
> +		startup, prescal);
> +
> +	return 0;
> +}
> +
> +static int at91_adc_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	st->chan = chan;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&st->lock);
> +
> +		at91_adc_writel(st, ADC_CHER, BIT(chan->channel));
> +		at91_adc_writel(st, ADC_IER, BIT(chan->channel));
> +		at91_adc_writel(st, ADC_CR, ADC_CR_START);
> +
> +		ret = wait_event_interruptible_timeout(st->wq_data_available,
> +						       st->done,
> +						       msecs_to_jiffies(1000));
> +		if (ret == 0)
> +			ret = -ETIMEDOUT;
> +
> +		if (ret <= 0) {
> +			at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> +			at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> +			mutex_unlock(&st->lock);
> +			return ret;
> +		}
> +
> +		if (chan->scan_type.sign == 's')
> +			*val = sign_extend32(st->last_value,
> +					     chan->scan_type.realbits - 1);
> +		else
> +			*val = st->last_value;
> +		at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> +		at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> +		st->done = false;
> +
> +		mutex_unlock(&st->lock);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_uv / 1000;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info at91_adc_info = {
> +	.read_raw = &at91_adc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int at91_adc_probe(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev;
> +	struct at91_adc_state *st;
> +	struct resource	*res;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev,
> +					  sizeof(struct at91_adc_state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &at91_adc_info;
> +	indio_dev->channels = at91_adc_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
> +
> +	st = iio_priv(indio_dev);
> +	st->soc_info = (struct at91_adc_soc_info *)
> +			of_device_get_match_data(&pdev->dev);
> +	init_waitqueue_head(&st->wq_data_available);
> +	mutex_init(&st->lock);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
> +	st->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(st->base))
> +		return PTR_ERR(st->base);
> +
> +	st->irq = platform_get_irq(pdev, 0);
> +	if (st->irq < 0)
could be be 0 (effectively marked as not present) - probably want to check
for that and fault out if so.
> +		return st->irq;
> +
> +	st->per_clk = devm_clk_get(&pdev->dev, "adc_clk");
> +	if (IS_ERR(st->per_clk))
> +		return PTR_ERR(st->per_clk);
> +
> +	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
> +	if (IS_ERR(st->adc_clk))
> +		return PTR_ERR(st->adc_clk);
> +
> +	st->reg = devm_regulator_get(&pdev->dev, "vddana");
> +	if (IS_ERR(st->reg))
> +		return PTR_ERR(st->reg);
You get this regulator but never explicitly request that it is on...
I can understand that for a reference like below (though probably want
to turn that on as well really).
> +
> +	st->vref = devm_regulator_get(&pdev->dev, "vref");
> +	if (IS_ERR(st->vref))
> +		return PTR_ERR(st->vref);
> +
> +	ret = devm_request_irq(&pdev->dev, st->irq, at91_adc_interrupt, 0,
> +			       pdev->dev.driver->name, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	st->vref_uv = regulator_get_voltage(st->vref);
> +	if (st->vref_uv <= 0) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
> +	ret = at91_adc_init(st);
> +	if (ret)
> +		goto error;
> +
> +	ret = clk_prepare_enable(st->adc_clk);
Handle these possible errors.
> +	ret = clk_prepare_enable(st->per_clk);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(&pdev->dev, "version: %x\n",
> +		 readl_relaxed(st->base + ADC_VERSION));
> +
> +error:
> +	return ret;
> +}
> +
> +static int at91_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	clk_disable_unprepare(st->per_clk);
> +	clk_disable_unprepare(st->adc_clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id at91_adc_dt_match[] = {
> +	{
> +		.compatible = "atmel,sama5d2-adc",
> +		.data = &at91_adc_sama5d2_soc_info
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
> +
> +static struct platform_driver at91_adc_driver = {
> +	.probe = at91_adc_probe,
> +	.remove = at91_adc_remove,
> +	.driver = {
> +		.name = "at91_adc8xx",
> +		.of_match_table = at91_adc_dt_match,
> +	},
> +};
> +module_platform_driver(at91_adc_driver)
> +
> +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches@atmel.com>");
> +MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Ludovic Desroches]

On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
> On 21/12/15 09:24, Ludovic Desroches wrote:
> > This driver supports the new version of the Atmel ADC device introduced
> > with the SAMA5D2 SoC family.
> > 
> > Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> A few more bits and bobs from me. Mostly looking good.
> 
> Jonathan
> > ---
> >  .../devicetree/bindings/iio/adc/at91_adc8xx.txt    |  27 ++
> >  drivers/iio/adc/Kconfig                            |  11 +
> >  drivers/iio/adc/Makefile                           |   1 +
> >  drivers/iio/adc/at91_adc8xx.c                      | 417 +++++++++++++++++++++
> >  4 files changed, 456 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> >  create mode 100644 drivers/iio/adc/at91_adc8xx.c
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > new file mode 100644
> > index 0000000..64ad6a5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > @@ -0,0 +1,27 @@
> > +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> > +
> > +Required properties:
> > +  - compatible: Should be "atmel,sama5d2-adc".
> > +  - reg: Should contain ADC registers location and length.
> > +  - interrupts: Should contain the IRQ line for the ADC.
> > +  - clocks: phandles to clocks.
> > +  - clock-names: tuple listing clock names.
> > +      Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> > +      clock, "adc_clk" is the sampling clock.
> > +  - vref-supply: Supply used as reference for conversions.
> > +
> > +Optional properties:
> > +  - vddana-supply: Supply for the adc device.
> > +
> > +
> > +Example:
> > +
> > +adc: adc@fc030000 {
> > +	compatible = "atmel,sama5d2-adc";
> > +	reg = <0xfc030000 0x100>;
> > +	interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
> > +	clocks = <&adc_clk>, <&adc_op_clk>;
> > +	clock-names = "adc_clk", "adc_op_clk";
> > +	vddana-supply = <&vdd_3v3_lp_reg>;
> > +	vref-supply = <&vdd_3v3_lp_reg>;
> > +}
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 9162dfe..5819e41 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -131,6 +131,17 @@ config AT91_ADC
> >  	  To compile this driver as a module, choose M here: the module will be
> >  	  called at91_adc.
> >  
> > +config AT91_ADC8xx
> > +	tristate "Atmel AT91 ADC 8xx"
> > +	depends on ARCH_AT91
> > +	depends on INPUT
> > +	help
> > +	  Say yes here to build support for Atmel ADC 8xx which is available
> > +	  from SAMA5D2 SoC family.
> > +
> > +	  To compile this driver as a module, choose M here: the module will be
> > +	  called at91_adc8xx.
> > +
> >  config AXP288_ADC
> >  	tristate "X-Powers AXP288 ADC driver"
> >  	depends on MFD_AXP20X
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 91a65bf..d684a52 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
> >  obj-$(CONFIG_AD7887) += ad7887.o
> >  obj-$(CONFIG_AD799X) += ad799x.o
> >  obj-$(CONFIG_AT91_ADC) += at91_adc.o
> > +obj-$(CONFIG_AT91_ADC8xx) += at91_adc8xx.o
> >  obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
> >  obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
> >  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> > diff --git a/drivers/iio/adc/at91_adc8xx.c b/drivers/iio/adc/at91_adc8xx.c
> > new file mode 100644
> > index 0000000..8b4a6e7
> > --- /dev/null
> > +++ b/drivers/iio/adc/at91_adc8xx.c
> > @@ -0,0 +1,417 @@
> > +/*
> > + * Atmel ADC driver for SAMA5D2 devices and later.
> > + *
> > + * Copyright (C) 2015 Atmel,
> > + *               2015 Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sched.h>
> > +#include <linux/wait.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define ADC_CR		0x00			/* Control Register */
> > +#define		ADC_CR_SWRST		BIT(0)		/* Software Reset */
> > +#define		ADC_CR_START		BIT(1)		/* Start Conversion */
> > +#define		ADC_CR_TSCALIB		BIT(2)		/* Touchscreen Calibration */
> > +#define		ADC_CR_CMPRST		BIT(4)		/* Comparison Restart */
> > +#define ADC_MR		0x04			/* Mode Register */
> > +#define		ADC_MR_TRGSEL(v)	(v << 1)	/* Trigger Selection */
> > +#define			ADC_MR_TRGSEL_TRIG0	0		/* ADTRG */
> > +#define			ADC_MR_TRGSEL_TRIG1	1		/* TIOA0 */
> > +#define			ADC_MR_TRGSEL_TRIG2	2		/* TIOA1 */
> > +#define			ADC_MR_TRGSEL_TRIG3	3		/* TIOA2 */
> > +#define			ADC_MR_TRGSEL_TRIG4	4		/* PWM event line 0 */
> > +#define			ADC_MR_TRGSEL_TRIG5	5		/* PWM event line 1 */
> > +#define			ADC_MR_TRGSEL_TRIG6	6		/* TIOA3 */
> > +#define			ADC_MR_TRGSEL_TRIG7	7		/* RTCOUT0 */
> > +#define		ADC_MR_SLEEP		BIT(5)		/* Sleep Mode */
> > +#define		ADC_MR_FWUP		BIT(6)		/* Fast Wake Up */
> > +#define		ADC_MR_PRESCAL(v)	(v << 8)	/* Prescaler Rate Selection */
> > +#define			ADC_MR_PRESCAL_MAX	0xffff
> > +#define		ADC_MR_STARTUP(v)	(v << 16)	/* Startup Time */
> > +#define			ADC_MR_STARTUP_SUT0	0		/* 0 period of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT8	1		/* 8 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT16	2		/* 16 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT24	3		/* 24 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT64	4		/* 64 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT80	5		/* 80 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT96	6		/* 96 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT112	7		/* 112 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT512	8		/* 512 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT576	9		/* 576 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT640	10		/* 640 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT704	11		/* 704 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT768	12		/* 768 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT832	13		/* 832 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT896	14		/* 896 periods of ADCCLK */
> > +#define			ADC_MR_STARTUP_SUT960	15		/* 960 periods of ADCCLK */
> > +#define		ADC_MR_ANACH		BIT(23)		/* Analog Change */
> > +#define		ADC_MR_TRACKTIM(v)	(v << 24)	/* Tracking Time */
> > +#define			ADC_MR_TRACKTIM_MAX	0xff
> > +#define		ADC_MR_TRANSFER(v)	(v << 28)	/* Transfer Time */
> > +#define			ADC_MR_TRANSFER_MAX	0x3
> > +#define		ADC_MR_USEQ		BIT(31)		/* Use Sequence Enable */
> > +#define ADC_SEQR1	0x08			/* Channel Sequence Register 1 */
> > +#define ADC_SEQR2	0x0c			/* Channel Sequence Register 2 */
> > +#define ADC_CHER	0x10			/* Channel Enable Register */
> > +#define ADC_CHDR	0x14			/* Channel Disable Register */
> > +#define ADC_CHSR	0x18			/* Channel Status Register */
> > +#define ADC_LCDR	0x20			/* Last Converted Data Register */
> > +#define ADC_IER		0x24			/* Interrupt Enable Register */
> > +#define ADC_IDR		0x28			/* Interrupt Disable Register */
> > +#define ADC_IMR		0x2c			/* Interrupt Mask Register */
> > +#define ADC_ISR		0x30			/* Interrupt Status Register */
> > +#define ADC_LCTMR	0x34			/* Last Channel Trigger Mode Register */
> > +#define ADC_LCCWR	0x38			/* Last Channel Compare Window Register */
> > +#define ADC_OVER	0x3c			/* Overrun Status Register */
> > +#define ADC_EMR		0x40			/* Extended Mode Register */
> > +#define ADC_CWR		0x44			/* Compare Window Register */
> > +#define ADC_CGR		0x48			/* Channel Gain Register */
> > +#define ADC_COR		0x4c			/* Channel Offset Register */
> > +#define ADC_CDR0	0x50			/* Channel Data Register 0 */
> > +#define ADC_ACR		0x94			/* Analog Control Register */
> > +#define ADC_TSMR	0xb0			/* Touchscreen Mode Register */
> > +#define ADC_XPOSR	0xb4			/* Touchscreen X Position Register */
> > +#define ADC_YPOSR	0xb8			/* Touchscreen Y Position Register */
> > +#define ADC_PRESSR	0xbc			/* Touchscreen Pressure Register */
> > +#define ADC_TRGR	0xc0			/* Trigger Register */
> > +#define ADC_COSR	0xd0			/* Correction Select Register */
> > +#define ADC_CVR		0xd4			/* Correction Value Register */
> > +#define ADC_CECR	0xd8			/* Channel Error Correction Register */
> > +#define ADC_WPMR	0xe4			/* Write Protection Mode Register */
> > +#define ADC_WPSR	0xe8			/* Write Protection Status Register */
> > +#define ADC_VERSION	0xfc			/* Version Register */
> > +
> > +#define AT91_ADC_CHAN(num, addr)					\
> > +	{								\
> > +		.type = IIO_VOLTAGE,					\
> > +		.channel = num,						\
> > +		.address = addr,					\
> > +		.scan_type = {						\
> > +			.sign = 'u',					\
> > +			.realbits = 12,					\
> > +			.storagebits = 14,				\
> > +		},							\
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > +		.datasheet_name = "CH"#num,				\
> > +		.indexed = 1,						\
> > +	}
> > +
> > +#define at91_adc_readl(st, reg)		readl_relaxed(st->base + reg)
> > +#define at91_adc_writel(st, reg, val)	writel_relaxed(val, st->base + reg)
> > +
> > +struct at91_adc_soc_info {
> > +	unsigned			startup_time;
> > +	unsigned			min_f_adc;
> > +	unsigned			max_f_adc;
> > +	const struct iio_chan_spec	*channels;
> > +	int				num_channels;
> > +};
> > +
> > +struct at91_adc_state {
> > +	void __iomem			*base;
> > +	struct clk			*per_clk;
> > +	struct clk			*adc_clk;
> > +	int				irq;
> > +	struct regulator		*reg;
> > +	struct regulator		*vref;
> > +	u32				vref_uv;
> > +	struct at91_adc_soc_info	*soc_info;
> > +	wait_queue_head_t		wq_data_available;
> > +	bool				done;
> > +	const struct iio_chan_spec	*chan;
> > +	u32				last_value;
> > +	struct mutex			lock;
> > +};
> > +
> > +static struct at91_adc_soc_info at91_adc_sama5d2_soc_info = {
> > +	.startup_time = 4,
> > +	.min_f_adc = 200000,
> > +	.max_f_adc = 20000000,
> These rather look like things that should be in the device tree if they
> are going to change between SoC varients. 

I have no strong position about it, I use to put parameters relative
to a SoC family into the driver. They will change only for next SoC
family not varients. When moving to a new family, the adc device could
be a new one.

My only concern is to not put as many parameters in the device tree as
at91_adc does.

> > +};
> > +
> > +static const struct iio_chan_spec at91_adc_channels[] = {
> > +	AT91_ADC_CHAN(0, 0x50),
> > +	AT91_ADC_CHAN(1, 0x54),
> > +	AT91_ADC_CHAN(2, 0x58),
> > +	AT91_ADC_CHAN(3, 0x5c),
> > +	AT91_ADC_CHAN(4, 0x60),
> > +	AT91_ADC_CHAN(5, 0x64),
> > +	AT91_ADC_CHAN(6, 0x68),
> > +	AT91_ADC_CHAN(7, 0x6c),
> > +	AT91_ADC_CHAN(8, 0x70),
> > +	AT91_ADC_CHAN(9, 0x74),
> > +	AT91_ADC_CHAN(10, 0x78),
> > +	AT91_ADC_CHAN(11, 0x7c),
> > +};
> > +
> > +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> > +{
> > +	struct iio_dev *indio = private;
> > +	struct at91_adc_state *st = iio_priv(indio);
> > +	u32 status = at91_adc_readl(st, ADC_ISR);
> > +
> > +	status &= at91_adc_readl(st, ADC_IMR);
> > +	if (status & 0xFFF) {
> > +		st->last_value = at91_adc_readl(st, st->chan->address);
> If this is a polled read - is there any reason to read this value here
> rather than outside the interrupt?

No it can be done outside the interrupt. I have taken some parts from the
previous driver but it was reading a register used by all the channels
when it has been designed. So yes there is probably no more reason to
read it into the interrupt.

> > +		st->done = true;
> > +		wake_up_interruptible(&st->wq_data_available);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static bool at91_adc_freq_supported(struct at91_adc_state *st, unsigned freq)
> > +{
> > +	return ((freq >= st->soc_info->min_f_adc)
> > +		&& (freq <= st->soc_info->max_f_adc));
> > +}
> > +
> > +static unsigned at91_adc_startup_time(unsigned startup_time_min,
> > +				      unsigned adc_clk_khz)
> > +{
> > +	const unsigned startup_lookup[] = {
> > +		0,     8,  16,  24,
> > +		64,   80,  96, 112,
> > +		512, 576, 640, 704,
> > +		768, 832, 896, 960
> > +		};
> > +	unsigned ticks_min, i;
> > +
> > +	/*
> > +	 * Since the adc frequency is checked before, there is no reason
> > +	 * to not meet the startup time constraint.
> > +	 */
> > +
> > +	ticks_min = startup_time_min * adc_clk_khz / 1000;
> > +	for (i = 0; i < ARRAY_SIZE(startup_lookup); i++)
> > +		if (startup_lookup[i] > ticks_min)
> > +			break;
> > +
> > +	return i;
> > +}
> > +
> > +static int at91_adc_init(struct at91_adc_state *st)
> > +{
> > +	struct iio_dev *indio_dev = iio_priv_to_dev(st);
> > +	unsigned f_adc, f_per, prescal, startup;
> > +
> > +	at91_adc_writel(st, ADC_CR, ADC_CR_SWRST);
> > +	at91_adc_writel(st, ADC_IDR, 0xffffffff);
> > +
> > +	f_adc = clk_get_rate(st->adc_clk);
> > +	if (!at91_adc_freq_supported(st, f_adc)) {
> > +		dev_err(&indio_dev->dev, "unsupported adc clock frequency\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	f_per = clk_get_rate(st->per_clk);
> > +	prescal = (f_per / (2 * f_adc)) - 1;
> > +
> > +	startup = at91_adc_startup_time(st->soc_info->startup_time,
> > +					f_adc / 1000);
> > +
> > +	at91_adc_writel(st, ADC_MR,
> > +			ADC_MR_TRANSFER(2)
> > +			| ADC_MR_STARTUP(startup)
> > +			| ADC_MR_PRESCAL(prescal));
> > +
> > +	dev_dbg(&indio_dev->dev, "startup: %u, prescal: %u\n",
> > +		startup, prescal);
> > +
> > +	return 0;
> > +}
> > +
> > +static int at91_adc_read_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int *val, int *val2, long mask)
> > +{
> > +	struct at91_adc_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	st->chan = chan;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&st->lock);
> > +
> > +		at91_adc_writel(st, ADC_CHER, BIT(chan->channel));
> > +		at91_adc_writel(st, ADC_IER, BIT(chan->channel));
> > +		at91_adc_writel(st, ADC_CR, ADC_CR_START);
> > +
> > +		ret = wait_event_interruptible_timeout(st->wq_data_available,
> > +						       st->done,
> > +						       msecs_to_jiffies(1000));
> > +		if (ret == 0)
> > +			ret = -ETIMEDOUT;
> > +
> > +		if (ret <= 0) {
> > +			at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> > +			at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> > +			mutex_unlock(&st->lock);
> > +			return ret;
> > +		}
> > +
> > +		if (chan->scan_type.sign == 's')
> > +			*val = sign_extend32(st->last_value,
> > +					     chan->scan_type.realbits - 1);
> > +		else
> > +			*val = st->last_value;
> > +		at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
> > +		at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
> > +		st->done = false;
> > +
> > +		mutex_unlock(&st->lock);
> > +		return IIO_VAL_INT;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = st->vref_uv / 1000;
> > +		*val2 = chan->scan_type.realbits;
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info at91_adc_info = {
> > +	.read_raw = &at91_adc_read_raw,
> > +	.driver_module = THIS_MODULE,
> > +};
> > +
> > +static int at91_adc_probe(struct platform_device *pdev)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct at91_adc_state *st;
> > +	struct resource	*res;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&pdev->dev,
> > +					  sizeof(struct at91_adc_state));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	indio_dev->dev.parent = &pdev->dev;
> > +	indio_dev->name = dev_name(&pdev->dev);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &at91_adc_info;
> > +	indio_dev->channels = at91_adc_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->soc_info = (struct at91_adc_soc_info *)
> > +			of_device_get_match_data(&pdev->dev);
> > +	init_waitqueue_head(&st->wq_data_available);
> > +	mutex_init(&st->lock);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		return -EINVAL;
> > +
> > +	st->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(st->base))
> > +		return PTR_ERR(st->base);
> > +
> > +	st->irq = platform_get_irq(pdev, 0);
> > +	if (st->irq < 0)
> could be be 0 (effectively marked as not present) - probably want to check
> for that and fault out if so.

ok.

> > +		return st->irq;
> > +
> > +	st->per_clk = devm_clk_get(&pdev->dev, "adc_clk");
> > +	if (IS_ERR(st->per_clk))
> > +		return PTR_ERR(st->per_clk);
> > +
> > +	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
> > +	if (IS_ERR(st->adc_clk))
> > +		return PTR_ERR(st->adc_clk);
> > +
> > +	st->reg = devm_regulator_get(&pdev->dev, "vddana");
> > +	if (IS_ERR(st->reg))
> > +		return PTR_ERR(st->reg);
> You get this regulator but never explicitly request that it is on...
> I can understand that for a reference like below (though probably want
> to turn that on as well really).

Yes, my regulators are always enabled, that's why I didn't think about
turning them on.

> > +
> > +	st->vref = devm_regulator_get(&pdev->dev, "vref");
> > +	if (IS_ERR(st->vref))
> > +		return PTR_ERR(st->vref);
> > +
> > +	ret = devm_request_irq(&pdev->dev, st->irq, at91_adc_interrupt, 0,
> > +			       pdev->dev.driver->name, indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->vref_uv = regulator_get_voltage(st->vref);
> > +	if (st->vref_uv <= 0) {
> > +		ret = -EINVAL;
> > +		goto error;
> > +	}
> > +
> > +	ret = at91_adc_init(st);
> > +	if (ret)
> > +		goto error;
> > +
> > +	ret = clk_prepare_enable(st->adc_clk);
> Handle these possible errors.
> > +	ret = clk_prepare_enable(st->per_clk);
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	dev_info(&pdev->dev, "version: %x\n",
> > +		 readl_relaxed(st->base + ADC_VERSION));
> > +
> > +error:
> > +	return ret;
> > +}
> > +
> > +static int at91_adc_remove(struct platform_device *pdev)
> > +{
> > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +	struct at91_adc_state *st = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +
> > +	clk_disable_unprepare(st->per_clk);
> > +	clk_disable_unprepare(st->adc_clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id at91_adc_dt_match[] = {
> > +	{
> > +		.compatible = "atmel,sama5d2-adc",
> > +		.data = &at91_adc_sama5d2_soc_info
> > +	}, {
> > +		/* sentinel */
> > +	}
> > +};
> > +MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
> > +
> > +static struct platform_driver at91_adc_driver = {
> > +	.probe = at91_adc_probe,
> > +	.remove = at91_adc_remove,
> > +	.driver = {
> > +		.name = "at91_adc8xx",
> > +		.of_match_table = at91_adc_dt_match,
> > +	},
> > +};
> > +module_platform_driver(at91_adc_driver)
> > +
> > +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>");
> > +MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
> > +MODULE_LICENSE("GPL v2");
> > 
> 

Thanks.

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

Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Ludovic Desroches]

On Wed, Dec 23, 2015 at 11:27:00AM +0100, Ludovic Desroches wrote:
> On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
> > On 21/12/15 09:24, Ludovic Desroches wrote:
> > > This driver supports the new version of the Atmel ADC device introduced
> > > with the SAMA5D2 SoC family.
> > > 

[...]

> > > +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> > > +{
> > > +	struct iio_dev *indio = private;
> > > +	struct at91_adc_state *st = iio_priv(indio);
> > > +	u32 status = at91_adc_readl(st, ADC_ISR);
> > > +
> > > +	status &= at91_adc_readl(st, ADC_IMR);
> > > +	if (status & 0xFFF) {
> > > +		st->last_value = at91_adc_readl(st, st->chan->address);
> > If this is a polled read - is there any reason to read this value here
> > rather than outside the interrupt?
> 
> No it can be done outside the interrupt. I have taken some parts from the
> previous driver but it was reading a register used by all the channels
> when it has been designed. So yes there is probably no more reason to
> read it into the interrupt.
> 

Thinking about it. Is it really useful to move reading outside the
interrupt?

By the way this is not a polled read.

> > > +		st->done = true;
> > > +		wake_up_interruptible(&st->wq_data_available);
> > > +	}
> > > +
> > > +	return IRQ_HANDLED;
> > > +}

Ludovic

Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Jonathan Cameron]

On 23 December 2015 10:48:33 GMT+00:00, Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>On Wed, Dec 23, 2015 at 11:27:00AM +0100, Ludovic Desroches wrote:
>> On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
>> > On 21/12/15 09:24, Ludovic Desroches wrote:
>> > > This driver supports the new version of the Atmel ADC device
>introduced
>> > > with the SAMA5D2 SoC family.
>> > > 
>
>[...]
>
>> > > +static irqreturn_t at91_adc_interrupt(int irq, void *private)
>> > > +{
>> > > +	struct iio_dev *indio = private;
>> > > +	struct at91_adc_state *st = iio_priv(indio);
>> > > +	u32 status = at91_adc_readl(st, ADC_ISR);
>> > > +
>> > > +	status &= at91_adc_readl(st, ADC_IMR);
>> > > +	if (status & 0xFFF) {
>> > > +		st->last_value = at91_adc_readl(st, st->chan->address);
>> > If this is a polled read - is there any reason to read this value
>here
>> > rather than outside the interrupt?
>> 
>> No it can be done outside the interrupt. I have taken some parts from
>the
>> previous driver but it was reading a register used by all the
>channels
>> when it has been designed. So yes there is probably no more reason to
>> read it into the interrupt.
>> 
>
>Thinking about it. Is it really useful to move reading outside the
>interrupt?
It avoids needing to know the channel in here, hence simplifying the code a bit.
>
>By the way this is not a polled read.
I meant that it is an individually requested conversion rather than a free running
 sampling system where the value read might change before it is read out.
>
>> > > +		st->done = true;
>> > > +		wake_up_interruptible(&st->wq_data_available);
>> > > +	}
>> > > +
>> > > +	return IRQ_HANDLED;
>> > > +}
>
>Ludovic
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Ludovic Desroches]

On Wed, Dec 23, 2015 at 05:21:25PM +0000, Jonathan Cameron wrote:
> 
> 
> On 23 December 2015 10:48:33 GMT+00:00, Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> >On Wed, Dec 23, 2015 at 11:27:00AM +0100, Ludovic Desroches wrote:
> >> On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
> >> > On 21/12/15 09:24, Ludovic Desroches wrote:
> >> > > This driver supports the new version of the Atmel ADC device
> >introduced
> >> > > with the SAMA5D2 SoC family.
> >> > > 
> >
> >[...]
> >
> >> > > +static irqreturn_t at91_adc_interrupt(int irq, void *private)
> >> > > +{
> >> > > +	struct iio_dev *indio = private;
> >> > > +	struct at91_adc_state *st = iio_priv(indio);
> >> > > +	u32 status = at91_adc_readl(st, ADC_ISR);
> >> > > +
> >> > > +	status &= at91_adc_readl(st, ADC_IMR);
> >> > > +	if (status & 0xFFF) {
> >> > > +		st->last_value = at91_adc_readl(st, st->chan->address);
> >> > If this is a polled read - is there any reason to read this value
> >here
> >> > rather than outside the interrupt?
> >> 
> >> No it can be done outside the interrupt. I have taken some parts from
> >the
> >> previous driver but it was reading a register used by all the
> >channels
> >> when it has been designed. So yes there is probably no more reason to
> >> read it into the interrupt.
> >> 
> >
> >Thinking about it. Is it really useful to move reading outside the
> >interrupt?
> It avoids needing to know the channel in here, hence simplifying the code a bit.

I am not sure it will simplify the code since the interrupt is cleared
when reading the conversion not the interrupt status register.

> >
> >By the way this is not a polled read.
> I meant that it is an individually requested conversion rather than a free running
>  sampling system where the value read might change before it is read out.
> >
> >> > > +		st->done = true;
> >> > > +		wake_up_interruptible(&st->wq_data_available);
> >> > > +	}
> >> > > +
> >> > > +	return IRQ_HANDLED;
> >> > > +}
> >
> >Ludovic
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> >the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Jonathan Cameron]

On 23/12/15 10:27, Ludovic Desroches wrote:
> On Tue, Dec 22, 2015 at 06:34:00PM +0000, Jonathan Cameron wrote:
>> On 21/12/15 09:24, Ludovic Desroches wrote:
>>> This driver supports the new version of the Atmel ADC device introduced
>>> with the SAMA5D2 SoC family.
>>>
>>> Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> A few more bits and bobs from me. Mostly looking good.
>>
>> Jonathan
>>> ---
>>>  .../devicetree/bindings/iio/adc/at91_adc8xx.txt    |  27 ++
>>>  drivers/iio/adc/Kconfig                            |  11 +
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/at91_adc8xx.c                      | 417 +++++++++++++++++++++
>>>  4 files changed, 456 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
>>>  create mode 100644 drivers/iio/adc/at91_adc8xx.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
>>> new file mode 100644
>>> index 0000000..64ad6a5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
>>> @@ -0,0 +1,27 @@
>>> +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
>>> +
>>> +Required properties:
>>> +  - compatible: Should be "atmel,sama5d2-adc".
>>> +  - reg: Should contain ADC registers location and length.
>>> +  - interrupts: Should contain the IRQ line for the ADC.
>>> +  - clocks: phandles to clocks.
>>> +  - clock-names: tuple listing clock names.
>>> +      Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
>>> +      clock, "adc_clk" is the sampling clock.
>>> +  - vref-supply: Supply used as reference for conversions.
>>> +
>>> +Optional properties:
>>> +  - vddana-supply: Supply for the adc device.
>>> +
>>> +
>>> +Example:
>>> +
>>> +adc: adc@fc030000 {
>>> +	compatible = "atmel,sama5d2-adc";
>>> +	reg = <0xfc030000 0x100>;
>>> +	interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
>>> +	clocks = <&adc_clk>, <&adc_op_clk>;
>>> +	clock-names = "adc_clk", "adc_op_clk";
>>> +	vddana-supply = <&vdd_3v3_lp_reg>;
>>> +	vref-supply = <&vdd_3v3_lp_reg>;
>>> +}
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 9162dfe..5819e41 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -131,6 +131,17 @@ config AT91_ADC
>>>  	  To compile this driver as a module, choose M here: the module will be
>>>  	  called at91_adc.
>>>  
>>> +config AT91_ADC8xx
>>> +	tristate "Atmel AT91 ADC 8xx"
>>> +	depends on ARCH_AT91
>>> +	depends on INPUT
>>> +	help
>>> +	  Say yes here to build support for Atmel ADC 8xx which is available
>>> +	  from SAMA5D2 SoC family.
>>> +
>>> +	  To compile this driver as a module, choose M here: the module will be
>>> +	  called at91_adc8xx.
>>> +
>>>  config AXP288_ADC
>>>  	tristate "X-Powers AXP288 ADC driver"
>>>  	depends on MFD_AXP20X
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index 91a65bf..d684a52 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7793) += ad7793.o
>>>  obj-$(CONFIG_AD7887) += ad7887.o
>>>  obj-$(CONFIG_AD799X) += ad799x.o
>>>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
>>> +obj-$(CONFIG_AT91_ADC8xx) += at91_adc8xx.o
>>>  obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>>>  obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>>>  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>>> diff --git a/drivers/iio/adc/at91_adc8xx.c b/drivers/iio/adc/at91_adc8xx.c
>>> new file mode 100644
>>> index 0000000..8b4a6e7
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/at91_adc8xx.c
>>> @@ -0,0 +1,417 @@
>>> +/*
>>> + * Atmel ADC driver for SAMA5D2 devices and later.
>>> + *
>>> + * Copyright (C) 2015 Atmel,
>>> + *               2015 Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>> + *
>>> + * This software is licensed under the terms of the GNU General Public
>>> + * License version 2, as published by the Free Software Foundation, and
>>> + * may be copied, distributed, and modified under those terms.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/sched.h>
>>> +#include <linux/wait.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/regulator/consumer.h>
>>> +
>>> +#define ADC_CR		0x00			/* Control Register */
>>> +#define		ADC_CR_SWRST		BIT(0)		/* Software Reset */
>>> +#define		ADC_CR_START		BIT(1)		/* Start Conversion */
>>> +#define		ADC_CR_TSCALIB		BIT(2)		/* Touchscreen Calibration */
>>> +#define		ADC_CR_CMPRST		BIT(4)		/* Comparison Restart */
>>> +#define ADC_MR		0x04			/* Mode Register */
>>> +#define		ADC_MR_TRGSEL(v)	(v << 1)	/* Trigger Selection */
>>> +#define			ADC_MR_TRGSEL_TRIG0	0		/* ADTRG */
>>> +#define			ADC_MR_TRGSEL_TRIG1	1		/* TIOA0 */
>>> +#define			ADC_MR_TRGSEL_TRIG2	2		/* TIOA1 */
>>> +#define			ADC_MR_TRGSEL_TRIG3	3		/* TIOA2 */
>>> +#define			ADC_MR_TRGSEL_TRIG4	4		/* PWM event line 0 */
>>> +#define			ADC_MR_TRGSEL_TRIG5	5		/* PWM event line 1 */
>>> +#define			ADC_MR_TRGSEL_TRIG6	6		/* TIOA3 */
>>> +#define			ADC_MR_TRGSEL_TRIG7	7		/* RTCOUT0 */
>>> +#define		ADC_MR_SLEEP		BIT(5)		/* Sleep Mode */
>>> +#define		ADC_MR_FWUP		BIT(6)		/* Fast Wake Up */
>>> +#define		ADC_MR_PRESCAL(v)	(v << 8)	/* Prescaler Rate Selection */
>>> +#define			ADC_MR_PRESCAL_MAX	0xffff
>>> +#define		ADC_MR_STARTUP(v)	(v << 16)	/* Startup Time */
>>> +#define			ADC_MR_STARTUP_SUT0	0		/* 0 period of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT8	1		/* 8 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT16	2		/* 16 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT24	3		/* 24 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT64	4		/* 64 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT80	5		/* 80 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT96	6		/* 96 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT112	7		/* 112 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT512	8		/* 512 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT576	9		/* 576 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT640	10		/* 640 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT704	11		/* 704 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT768	12		/* 768 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT832	13		/* 832 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT896	14		/* 896 periods of ADCCLK */
>>> +#define			ADC_MR_STARTUP_SUT960	15		/* 960 periods of ADCCLK */
>>> +#define		ADC_MR_ANACH		BIT(23)		/* Analog Change */
>>> +#define		ADC_MR_TRACKTIM(v)	(v << 24)	/* Tracking Time */
>>> +#define			ADC_MR_TRACKTIM_MAX	0xff
>>> +#define		ADC_MR_TRANSFER(v)	(v << 28)	/* Transfer Time */
>>> +#define			ADC_MR_TRANSFER_MAX	0x3
>>> +#define		ADC_MR_USEQ		BIT(31)		/* Use Sequence Enable */
>>> +#define ADC_SEQR1	0x08			/* Channel Sequence Register 1 */
>>> +#define ADC_SEQR2	0x0c			/* Channel Sequence Register 2 */
>>> +#define ADC_CHER	0x10			/* Channel Enable Register */
>>> +#define ADC_CHDR	0x14			/* Channel Disable Register */
>>> +#define ADC_CHSR	0x18			/* Channel Status Register */
>>> +#define ADC_LCDR	0x20			/* Last Converted Data Register */
>>> +#define ADC_IER		0x24			/* Interrupt Enable Register */
>>> +#define ADC_IDR		0x28			/* Interrupt Disable Register */
>>> +#define ADC_IMR		0x2c			/* Interrupt Mask Register */
>>> +#define ADC_ISR		0x30			/* Interrupt Status Register */
>>> +#define ADC_LCTMR	0x34			/* Last Channel Trigger Mode Register */
>>> +#define ADC_LCCWR	0x38			/* Last Channel Compare Window Register */
>>> +#define ADC_OVER	0x3c			/* Overrun Status Register */
>>> +#define ADC_EMR		0x40			/* Extended Mode Register */
>>> +#define ADC_CWR		0x44			/* Compare Window Register */
>>> +#define ADC_CGR		0x48			/* Channel Gain Register */
>>> +#define ADC_COR		0x4c			/* Channel Offset Register */
>>> +#define ADC_CDR0	0x50			/* Channel Data Register 0 */
>>> +#define ADC_ACR		0x94			/* Analog Control Register */
>>> +#define ADC_TSMR	0xb0			/* Touchscreen Mode Register */
>>> +#define ADC_XPOSR	0xb4			/* Touchscreen X Position Register */
>>> +#define ADC_YPOSR	0xb8			/* Touchscreen Y Position Register */
>>> +#define ADC_PRESSR	0xbc			/* Touchscreen Pressure Register */
>>> +#define ADC_TRGR	0xc0			/* Trigger Register */
>>> +#define ADC_COSR	0xd0			/* Correction Select Register */
>>> +#define ADC_CVR		0xd4			/* Correction Value Register */
>>> +#define ADC_CECR	0xd8			/* Channel Error Correction Register */
>>> +#define ADC_WPMR	0xe4			/* Write Protection Mode Register */
>>> +#define ADC_WPSR	0xe8			/* Write Protection Status Register */
>>> +#define ADC_VERSION	0xfc			/* Version Register */
>>> +
>>> +#define AT91_ADC_CHAN(num, addr)					\
>>> +	{								\
>>> +		.type = IIO_VOLTAGE,					\
>>> +		.channel = num,						\
>>> +		.address = addr,					\
>>> +		.scan_type = {						\
>>> +			.sign = 'u',					\
>>> +			.realbits = 12,					\
>>> +			.storagebits = 14,				\
>>> +		},							\
>>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>>> +		.datasheet_name = "CH"#num,				\
>>> +		.indexed = 1,						\
>>> +	}
>>> +
>>> +#define at91_adc_readl(st, reg)		readl_relaxed(st->base + reg)
>>> +#define at91_adc_writel(st, reg, val)	writel_relaxed(val, st->base + reg)
>>> +
>>> +struct at91_adc_soc_info {
>>> +	unsigned			startup_time;
>>> +	unsigned			min_f_adc;
>>> +	unsigned			max_f_adc;
>>> +	const struct iio_chan_spec	*channels;
>>> +	int				num_channels;
>>> +};
>>> +
>>> +struct at91_adc_state {
>>> +	void __iomem			*base;
>>> +	struct clk			*per_clk;
>>> +	struct clk			*adc_clk;
>>> +	int				irq;
>>> +	struct regulator		*reg;
>>> +	struct regulator		*vref;
>>> +	u32				vref_uv;
>>> +	struct at91_adc_soc_info	*soc_info;
>>> +	wait_queue_head_t		wq_data_available;
>>> +	bool				done;
>>> +	const struct iio_chan_spec	*chan;
>>> +	u32				last_value;
>>> +	struct mutex			lock;
>>> +};
>>> +
>>> +static struct at91_adc_soc_info at91_adc_sama5d2_soc_info = {
>>> +	.startup_time = 4,
>>> +	.min_f_adc = 200000,
>>> +	.max_f_adc = 20000000,
>> These rather look like things that should be in the device tree if they
>> are going to change between SoC varients. 
> 
> I have no strong position about it, I use to put parameters relative
> to a SoC family into the driver. They will change only for next SoC
> family not varients. When moving to a new family, the adc device could
> be a new one.
If we have to add these parameters to the devicetree later (to support a new
soc) that will be fine as long as the defaults are these (so it will work
with the older part).
> 
> My only concern is to not put as many parameters in the device tree as
> at91_adc does.
A reasonable aim!
> 
>>> +};
>>> +
>>> +static const struct iio_chan_spec at91_adc_channels[] = {
>>> +	AT91_ADC_CHAN(0, 0x50),
>>> +	AT91_ADC_CHAN(1, 0x54),
>>> +	AT91_ADC_CHAN(2, 0x58),
>>> +	AT91_ADC_CHAN(3, 0x5c),
>>> +	AT91_ADC_CHAN(4, 0x60),
>>> +	AT91_ADC_CHAN(5, 0x64),
>>> +	AT91_ADC_CHAN(6, 0x68),
>>> +	AT91_ADC_CHAN(7, 0x6c),
>>> +	AT91_ADC_CHAN(8, 0x70),
>>> +	AT91_ADC_CHAN(9, 0x74),
>>> +	AT91_ADC_CHAN(10, 0x78),
>>> +	AT91_ADC_CHAN(11, 0x7c),
>>> +};
>>> +
>>> +static irqreturn_t at91_adc_interrupt(int irq, void *private)
>>> +{
>>> +	struct iio_dev *indio = private;
>>> +	struct at91_adc_state *st = iio_priv(indio);
>>> +	u32 status = at91_adc_readl(st, ADC_ISR);
>>> +
>>> +	status &= at91_adc_readl(st, ADC_IMR);
>>> +	if (status & 0xFFF) {
>>> +		st->last_value = at91_adc_readl(st, st->chan->address);
>> If this is a polled read - is there any reason to read this value here
>> rather than outside the interrupt?
> 
> No it can be done outside the interrupt. I have taken some parts from the
> previous driver but it was reading a register used by all the channels
> when it has been designed. So yes there is probably no more reason to
> read it into the interrupt.
> 
>>> +		st->done = true;
>>> +		wake_up_interruptible(&st->wq_data_available);
>>> +	}
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static bool at91_adc_freq_supported(struct at91_adc_state *st, unsigned freq)
>>> +{
>>> +	return ((freq >= st->soc_info->min_f_adc)
>>> +		&& (freq <= st->soc_info->max_f_adc));
>>> +}
>>> +
>>> +static unsigned at91_adc_startup_time(unsigned startup_time_min,
>>> +				      unsigned adc_clk_khz)
>>> +{
>>> +	const unsigned startup_lookup[] = {
>>> +		0,     8,  16,  24,
>>> +		64,   80,  96, 112,
>>> +		512, 576, 640, 704,
>>> +		768, 832, 896, 960
>>> +		};
>>> +	unsigned ticks_min, i;
>>> +
>>> +	/*
>>> +	 * Since the adc frequency is checked before, there is no reason
>>> +	 * to not meet the startup time constraint.
>>> +	 */
>>> +
>>> +	ticks_min = startup_time_min * adc_clk_khz / 1000;
>>> +	for (i = 0; i < ARRAY_SIZE(startup_lookup); i++)
>>> +		if (startup_lookup[i] > ticks_min)
>>> +			break;
>>> +
>>> +	return i;
>>> +}
>>> +
>>> +static int at91_adc_init(struct at91_adc_state *st)
>>> +{
>>> +	struct iio_dev *indio_dev = iio_priv_to_dev(st);
>>> +	unsigned f_adc, f_per, prescal, startup;
>>> +
>>> +	at91_adc_writel(st, ADC_CR, ADC_CR_SWRST);
>>> +	at91_adc_writel(st, ADC_IDR, 0xffffffff);
>>> +
>>> +	f_adc = clk_get_rate(st->adc_clk);
>>> +	if (!at91_adc_freq_supported(st, f_adc)) {
>>> +		dev_err(&indio_dev->dev, "unsupported adc clock frequency\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	f_per = clk_get_rate(st->per_clk);
>>> +	prescal = (f_per / (2 * f_adc)) - 1;
>>> +
>>> +	startup = at91_adc_startup_time(st->soc_info->startup_time,
>>> +					f_adc / 1000);
>>> +
>>> +	at91_adc_writel(st, ADC_MR,
>>> +			ADC_MR_TRANSFER(2)
>>> +			| ADC_MR_STARTUP(startup)
>>> +			| ADC_MR_PRESCAL(prescal));
>>> +
>>> +	dev_dbg(&indio_dev->dev, "startup: %u, prescal: %u\n",
>>> +		startup, prescal);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>> +			     struct iio_chan_spec const *chan,
>>> +			     int *val, int *val2, long mask)
>>> +{
>>> +	struct at91_adc_state *st = iio_priv(indio_dev);
>>> +	int ret;
>>> +
>>> +	st->chan = chan;
>>> +
>>> +	switch (mask) {
>>> +	case IIO_CHAN_INFO_RAW:
>>> +		mutex_lock(&st->lock);
>>> +
>>> +		at91_adc_writel(st, ADC_CHER, BIT(chan->channel));
>>> +		at91_adc_writel(st, ADC_IER, BIT(chan->channel));
>>> +		at91_adc_writel(st, ADC_CR, ADC_CR_START);
>>> +
>>> +		ret = wait_event_interruptible_timeout(st->wq_data_available,
>>> +						       st->done,
>>> +						       msecs_to_jiffies(1000));
>>> +		if (ret == 0)
>>> +			ret = -ETIMEDOUT;
>>> +
>>> +		if (ret <= 0) {
>>> +			at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
>>> +			at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
>>> +			mutex_unlock(&st->lock);
>>> +			return ret;
>>> +		}
>>> +
>>> +		if (chan->scan_type.sign == 's')
>>> +			*val = sign_extend32(st->last_value,
>>> +					     chan->scan_type.realbits - 1);
>>> +		else
>>> +			*val = st->last_value;
>>> +		at91_adc_writel(st, ADC_CHDR, BIT(chan->channel));
>>> +		at91_adc_writel(st, ADC_IDR, BIT(chan->channel));
>>> +		st->done = false;
>>> +
>>> +		mutex_unlock(&st->lock);
>>> +		return IIO_VAL_INT;
>>> +
>>> +	case IIO_CHAN_INFO_SCALE:
>>> +		*val = st->vref_uv / 1000;
>>> +		*val2 = chan->scan_type.realbits;
>>> +		return IIO_VAL_FRACTIONAL_LOG2;
>>> +
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +}
>>> +
>>> +static const struct iio_info at91_adc_info = {
>>> +	.read_raw = &at91_adc_read_raw,
>>> +	.driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static int at91_adc_probe(struct platform_device *pdev)
>>> +{
>>> +	struct iio_dev *indio_dev;
>>> +	struct at91_adc_state *st;
>>> +	struct resource	*res;
>>> +	int ret;
>>> +
>>> +	indio_dev = devm_iio_device_alloc(&pdev->dev,
>>> +					  sizeof(struct at91_adc_state));
>>> +	if (!indio_dev)
>>> +		return -ENOMEM;
>>> +
>>> +	indio_dev->dev.parent = &pdev->dev;
>>> +	indio_dev->name = dev_name(&pdev->dev);
>>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>>> +	indio_dev->info = &at91_adc_info;
>>> +	indio_dev->channels = at91_adc_channels;
>>> +	indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
>>> +
>>> +	st = iio_priv(indio_dev);
>>> +	st->soc_info = (struct at91_adc_soc_info *)
>>> +			of_device_get_match_data(&pdev->dev);
>>> +	init_waitqueue_head(&st->wq_data_available);
>>> +	mutex_init(&st->lock);
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	if (!res)
>>> +		return -EINVAL;
>>> +
>>> +	st->base = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(st->base))
>>> +		return PTR_ERR(st->base);
>>> +
>>> +	st->irq = platform_get_irq(pdev, 0);
>>> +	if (st->irq < 0)
>> could be be 0 (effectively marked as not present) - probably want to check
>> for that and fault out if so.
> 
> ok.
> 
>>> +		return st->irq;
>>> +
>>> +	st->per_clk = devm_clk_get(&pdev->dev, "adc_clk");
>>> +	if (IS_ERR(st->per_clk))
>>> +		return PTR_ERR(st->per_clk);
>>> +
>>> +	st->adc_clk = devm_clk_get(&pdev->dev, "adc_op_clk");
>>> +	if (IS_ERR(st->adc_clk))
>>> +		return PTR_ERR(st->adc_clk);
>>> +
>>> +	st->reg = devm_regulator_get(&pdev->dev, "vddana");
>>> +	if (IS_ERR(st->reg))
>>> +		return PTR_ERR(st->reg);
>> You get this regulator but never explicitly request that it is on...
>> I can understand that for a reference like below (though probably want
>> to turn that on as well really).
> 
> Yes, my regulators are always enabled, that's why I didn't think about
> turning them on.
> 
>>> +
>>> +	st->vref = devm_regulator_get(&pdev->dev, "vref");
>>> +	if (IS_ERR(st->vref))
>>> +		return PTR_ERR(st->vref);
>>> +
>>> +	ret = devm_request_irq(&pdev->dev, st->irq, at91_adc_interrupt, 0,
>>> +			       pdev->dev.driver->name, indio_dev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	st->vref_uv = regulator_get_voltage(st->vref);
>>> +	if (st->vref_uv <= 0) {
>>> +		ret = -EINVAL;
>>> +		goto error;
>>> +	}
>>> +
>>> +	ret = at91_adc_init(st);
>>> +	if (ret)
>>> +		goto error;
>>> +
>>> +	ret = clk_prepare_enable(st->adc_clk);
>> Handle these possible errors.
>>> +	ret = clk_prepare_enable(st->per_clk);
>>> +
>>> +	ret = iio_device_register(indio_dev);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	dev_info(&pdev->dev, "version: %x\n",
>>> +		 readl_relaxed(st->base + ADC_VERSION));
>>> +
>>> +error:
>>> +	return ret;
>>> +}
>>> +
>>> +static int at91_adc_remove(struct platform_device *pdev)
>>> +{
>>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +	struct at91_adc_state *st = iio_priv(indio_dev);
>>> +
>>> +	iio_device_unregister(indio_dev);
>>> +
>>> +	clk_disable_unprepare(st->per_clk);
>>> +	clk_disable_unprepare(st->adc_clk);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id at91_adc_dt_match[] = {
>>> +	{
>>> +		.compatible = "atmel,sama5d2-adc",
>>> +		.data = &at91_adc_sama5d2_soc_info
>>> +	}, {
>>> +		/* sentinel */
>>> +	}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, at91_adc_dt_match);
>>> +
>>> +static struct platform_driver at91_adc_driver = {
>>> +	.probe = at91_adc_probe,
>>> +	.remove = at91_adc_remove,
>>> +	.driver = {
>>> +		.name = "at91_adc8xx",
>>> +		.of_match_table = at91_adc_dt_match,
>>> +	},
>>> +};
>>> +module_platform_driver(at91_adc_driver)
>>> +
>>> +MODULE_AUTHOR("Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>");
>>> +MODULE_DESCRIPTION("Atmel AT91 ADC 8xx");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
> 
> Thanks.
> 
> Ludovic
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Rob Herring]

On Mon, Dec 21, 2015 at 10:24:08AM +0100, Ludovic Desroches wrote:
> This driver supports the new version of the Atmel ADC device introduced
> with the SAMA5D2 SoC family.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/iio/adc/at91_adc8xx.txt    |  27 ++
>  drivers/iio/adc/Kconfig                            |  11 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/at91_adc8xx.c                      | 417 +++++++++++++++++++++
>  4 files changed, 456 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
>  create mode 100644 drivers/iio/adc/at91_adc8xx.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> new file mode 100644
> index 0000000..64ad6a5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> @@ -0,0 +1,27 @@
> +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> +
> +Required properties:
> +  - compatible: Should be "atmel,sama5d2-adc".
> +  - reg: Should contain ADC registers location and length.
> +  - interrupts: Should contain the IRQ line for the ADC.
> +  - clocks: phandles to clocks.
> +  - clock-names: tuple listing clock names.
> +      Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> +      clock, "adc_clk" is the sampling clock.
> +  - vref-supply: Supply used as reference for conversions.
> +
> +Optional properties:
> +  - vddana-supply: Supply for the adc device.

What makes a supply optional? If chip dependent, then then you need a 
more specific compatible string.

Rob

Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Ludovic Desroches]

On Tue, Dec 22, 2015 at 06:51:18PM -0600, Rob Herring wrote:
> On Mon, Dec 21, 2015 at 10:24:08AM +0100, Ludovic Desroches wrote:
> > This driver supports the new version of the Atmel ADC device introduced
> > with the SAMA5D2 SoC family.
> > 
> > Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > ---
> >  .../devicetree/bindings/iio/adc/at91_adc8xx.txt    |  27 ++
> >  drivers/iio/adc/Kconfig                            |  11 +
> >  drivers/iio/adc/Makefile                           |   1 +
> >  drivers/iio/adc/at91_adc8xx.c                      | 417 +++++++++++++++++++++
> >  4 files changed, 456 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> >  create mode 100644 drivers/iio/adc/at91_adc8xx.c
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > new file mode 100644
> > index 0000000..64ad6a5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> > @@ -0,0 +1,27 @@
> > +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> > +
> > +Required properties:
> > +  - compatible: Should be "atmel,sama5d2-adc".
> > +  - reg: Should contain ADC registers location and length.
> > +  - interrupts: Should contain the IRQ line for the ADC.
> > +  - clocks: phandles to clocks.
> > +  - clock-names: tuple listing clock names.
> > +      Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> > +      clock, "adc_clk" is the sampling clock.
> > +  - vref-supply: Supply used as reference for conversions.
> > +
> > +Optional properties:
> > +  - vddana-supply: Supply for the adc device.
> 
> What makes a supply optional? If chip dependent, then then you need a 
> more specific compatible string.
> 

I thought I had read that we can supply an external vddana or having a
default one. Double checking the datasheet. I can't find it, I probably
read it on a draft version. Will move the vddana supply to required
properties.

Thanks

Ludovic

Re: [PATCH 1/5] iio:adc:at91_adc8xx: introduce new atmel adc driver

[Alexandre Belloni]

On 21/12/2015 at 10:24:08 +0100, Ludovic Desroches wrote :
> +++ b/Documentation/devicetree/bindings/iio/adc/at91_adc8xx.txt
> @@ -0,0 +1,27 @@
> +* AT91 SAMA5D2 Analog to Digital Converter (ADC)
> +
> +Required properties:
> +  - compatible: Should be "atmel,sama5d2-adc".
> +  - reg: Should contain ADC registers location and length.
> +  - interrupts: Should contain the IRQ line for the ADC.
> +  - clocks: phandles to clocks.
> +  - clock-names: tuple listing clock names.
> +      Required elements: "adc_clk", "adc_op_clk". "adc_clk" is the peripheral
> +      clock, "adc_clk" is the sampling clock.

I think we should not have adc_op_clk but rather have a property like
vf610 has fsl,adck-max-frequency.
Even better, would be the ability to change it with
/sys/bus/iio/devices/iio:deviceX/sampling_frequency


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/5] MAINTAINERS: add entry for Atmel ADC 8xx driver

[Ludovic Desroches]

Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a27c660..5971c25 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1927,6 +1927,12 @@ M:	Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
 S:	Supported
 F:	drivers/tty/serial/atmel_serial.c
 
+ATMEL ADC 8xx DRIVER
+M:	Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
+L:	linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Supported
+F:	drivers/iio/adc/at91_adc8xx.c
+
 ATMEL Audio ALSA driver
 M:	Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
 L:	alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org (moderated for non-subscribers)
-- 
2.5.0

[PATCH 3/5] ARM: at91/dt: sama5d2: add adc device

[Ludovic Desroches]

Add the ADC device.

Signed-off-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/sama5d2.dtsi | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
index 545264b..91c6e42 100644
--- a/arch/arm/boot/dts/sama5d2.dtsi
+++ b/arch/arm/boot/dts/sama5d2.dtsi
@@ -92,7 +92,7 @@
 		adc_op_clk: adc_op_clk{
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
-			clock-frequency = <1000000>;
+			clock-frequency = <6000000>;
 		};
 	};
 
@@ -1061,6 +1061,15 @@
 				status = "disabled";
 			};
 
+			adc: adc@fc030000 {
+				compatible = "atmel,sama5d2-adc";
+				reg = <0xfc030000 0x100>;
+				interrupts = <40 IRQ_TYPE_LEVEL_HIGH 7>;
+				clocks = <&adc_clk>, <&adc_op_clk>;
+				clock-names = "adc_clk", "adc_op_clk";
+				status = "disabled";
+			};
+
 			pioA: pinctrl@fc038000 {
 				compatible = "atmel,sama5d2-pinctrl";
 				reg = <0xfc038000 0x600>;
-- 
2.5.0

[PATCH 4/5] ARM: at91/dt: sama5d2 Xplained: enable the adc device

[Ludovic Desroches]

Enable the adc on the sama5d2 Xplained board.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 arch/arm/boot/dts/at91-sama5d2_xplained.dts | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
index c2fcc29..6b263d4 100644
--- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
@@ -251,7 +251,29 @@
 				};
 			};
 
+			adc: adc@fc030000 {
+				vddana-supply = <&vdd_3v3_lp_reg>;
+				vref-supply = <&vdd_3v3_lp_reg>;
+				pinctrl-names = "default";
+				pinctrl-0 = <&pinctrl_adc_default>;
+				status = "okay";
+			};
+
 			pinctrl@fc038000 {
+				/*
+				 * There is no real pinmux for ADC, if the pin
+				 * is not requested by another peripheral then
+				 * the muxing is done when channel is enabled.
+				 * Requesting pins for ADC is GPIO is
+				 * encouraged to prevent conflicts and to
+				 * disable bias in order to be in the same
+				 * state when the pin is not muxed to the adc.
+				 */
+				pinctrl_adc_default: adc_default {
+					pinmux = <PIN_PD23__GPIO>;
+					bias-disable;
+				};
+
 				pinctrl_flx0_default: flx0_default {
 					pinmux = <PIN_PB28__FLEXCOM0_IO0>,
 						 <PIN_PB29__FLEXCOM0_IO1>;
-- 
2.5.0

[PATCH 5/5] ARM: at91/defconfig: add sama5d2 adc support in sama5_defconfig

[Ludovic Desroches]

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 arch/arm/configs/sama5_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/sama5_defconfig b/arch/arm/configs/sama5_defconfig
index c11bab7..afbda41 100644
--- a/arch/arm/configs/sama5_defconfig
+++ b/arch/arm/configs/sama5_defconfig
@@ -187,6 +187,7 @@ CONFIG_AT_XDMAC=y
 # CONFIG_IOMMU_SUPPORT is not set
 CONFIG_IIO=y
 CONFIG_AT91_ADC=y
+CONFIG_AT91_SAMA5D2_ADC=y
 CONFIG_PWM=y
 CONFIG_PWM_ATMEL=y
 CONFIG_PWM_ATMEL_TCB=y
-- 
2.5.0