[RFC] IIO: Add hardware triggers support to the AT91 ADC driver

[RFC] IIO: Add hardware triggers support to the AT91 ADC driver

[Maxime Ripard]

Hi list,

here is a patchset that adds the hardware triggers support to the at91_adc
driver that only supported software triggers so far.

Please note that this is NOT for inclusion, only for review for now, as there is
no support for buffers and triggers out of staging out of staging, and the first
patches only duplicate the at91_adc driver I submitted out of staging.

So basically, the important patches at the 4th and 5th.

The fourth rework the way I declared the platform_data, in a more DT-compliant
way. I needed it for the available triggers declaration. I will surely backport
this part soon to the out-of-staging driver, because it will certainly be useful
to it.

The fifth one adds the triggers and buffers support to the driver in itself.
While the whole dynamic triggers declaration might seem overkill for now as I
only support the sam9g20-ek board, support for other atmel boards will come soon
and I surely need that in place.
On the downside, it probably needs some more work. Jonathan suggested earlier
that kfifo is a more convenient backend for the buffer, so I might switch to it
in the future. Also, I have not been able to test the timer counters triggers,
only the external for now.

This patchset is based on the commit 3413ec44 from Jonathan's master tree.

So please, take a look.
Thanks,
Maxime

Cc: Patrice Vilchez <patrice.vilchez@atmel.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>

[PATCH 1/5] ARM: AT91: Add platform data for the ADCs

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Cc: Patrice Vilchez <patrice.vilchez@atmel.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 include/linux/platform_data/at91_adc.h |   36 ++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/platform_data/at91_adc.h

diff --git a/include/linux/platform_data/at91_adc.h b/include/linux/platform_data/at91_adc.h
new file mode 100644
index 0000000..1e1813d
--- /dev/null
+++ b/include/linux/platform_data/at91_adc.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright (C) 2011 Free Electrons
+ *
+ * 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.
+ *
+ */
+
+#ifndef _AT91_ADC_H_
+#define _AT91_ADC_H_
+
+struct at91_adc_data {
+	/* ADC Clock as specified by the datasheet, in Hz. */
+	unsigned int adc_clock;
+	/*
+	 * Global number of channels available (to specify which channels are
+	 * indeed used on the board, see the channels_used bitmask).
+	 */
+	u8 num_channels;
+	/* Channels in use on the board as a bitmask */
+	unsigned long channels_used;
+	/* Startup time of the ADC, in microseconds. */
+	u8 startup_time;
+	/* Reference voltage for the ADC in millivolts */
+	unsigned short vref;
+};
+
+extern void __init at91_add_device_adc(struct at91_adc_data *data);
+
+#endif
-- 
1.7.4.1

[PATCH 2/5] ARM: AT91: IIO: Add AT91 ADC driver.

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Cc: Patrice Vilchez <patrice.vilchez@atmel.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/staging/iio/adc/Kconfig    |    6 +
 drivers/staging/iio/adc/Makefile   |    1 +
 drivers/staging/iio/adc/at91_adc.c |  335 ++++++++++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/adc/at91_adc.c

diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index d9decea..93de64d 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -169,6 +169,12 @@ config AD7280
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7280a
 
+config AT91_ADC
+       tristate "Atmel AT91 ADC"
+       depends on SYSFS && ARCH_AT91
+       help
+         Say yes here to build support for Atmel AT91 ADC
+
 config MAX1363
 	tristate "Maxim max1363 ADC driver"
 	depends on I2C
diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
index ceee7f3..6ef78fb 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -37,3 +37,4 @@ obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_ADT7310) += adt7310.o
 obj-$(CONFIG_ADT7410) += adt7410.o
 obj-$(CONFIG_AD7280) += ad7280a.o
+obj-$(CONFIG_AT91_ADC) += at91_adc.o
diff --git a/drivers/staging/iio/adc/at91_adc.c b/drivers/staging/iio/adc/at91_adc.c
new file mode 100644
index 0000000..d10df7a
--- /dev/null
+++ b/drivers/staging/iio/adc/at91_adc.c
@@ -0,0 +1,335 @@
+/*
+ * Driver for the ADC present in the Atmel AT91 evaluation boards.
+ *
+ * Copyright 2011 Free Electrons
+ *
+ * Licensed under the GPLv2 or later.
+ */
+
+#include <linux/bitmap.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/wait.h>
+
+#include "../iio.h"
+#include <linux/platform_data/at91_adc.h>
+
+#include <mach/at91_adc.h>
+
+struct at91_adc_state {
+	struct clk *clk;
+	bool done;
+	struct mutex lock;
+	int irq;
+	wait_queue_head_t wq_data_avail;
+	u16 last_value;
+	void __iomem *reg_base;
+	unsigned int vref_mv;
+	unsigned long channels_mask;
+};
+
+static inline u32 at91_adc_reg_read(struct at91_adc_state *st,
+				    u8 reg)
+{
+	return readl_relaxed(st->reg_base + reg);
+}
+
+static inline void at91_adc_reg_write(struct at91_adc_state *st,
+				      u8 reg,
+				      u32 val)
+{
+	writel_relaxed(val, st->reg_base + reg);
+}
+
+static irqreturn_t at91_adc_eoc_trigger(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct at91_adc_state *st = iio_priv(idev);
+	unsigned int status = at91_adc_reg_read(st, AT91_ADC_SR);
+
+	if (!(status & AT91_ADC_DRDY))
+		return IRQ_HANDLED;
+
+	if (status & st->channels_mask) {
+		st->done = true;
+		st->last_value = at91_adc_reg_read(st, AT91_ADC_LCDR);
+	}
+
+	wake_up_interruptible(&st->wq_data_avail);
+
+	return IRQ_HANDLED;
+}
+
+static int at91_adc_channel_init(struct iio_dev *idev,
+				struct at91_adc_data *pdata)
+{
+	struct iio_chan_spec *chan_array;
+	int bit, idx = 0;
+
+	idev->num_channels = bitmap_weight(&pdata->channels_used,
+					   pdata->num_channels);
+	chan_array = kcalloc(idev->num_channels, sizeof(struct iio_chan_spec),
+			     GFP_KERNEL);
+
+	if (chan_array == NULL)
+		return -ENOMEM;
+
+	for_each_set_bit(bit, &pdata->channels_used, pdata->num_channels) {
+		struct iio_chan_spec *chan = chan_array + idx;
+		chan->type = IIO_VOLTAGE;
+		chan->indexed = 1;
+		chan->channel = bit;
+		chan->scan_type.sign = 'u';
+		chan->scan_type.realbits = 10;
+		chan->scan_type.storagebits = 16;
+		chan->info_mask = IIO_CHAN_INFO_SCALE_SHARED_BIT;
+		idx++;
+	}
+
+	idev->channels = chan_array;
+	return idev->num_channels;
+}
+
+static void at91_adc_channel_remove(struct iio_dev *idev)
+{
+	kfree(idev->channels);
+}
+
+static int at91_adc_read_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct at91_adc_state *st = iio_priv(idev);
+	int ret;
+
+	switch (mask) {
+	case 0:
+		mutex_lock(&st->lock);
+
+		at91_adc_reg_write(st, AT91_ADC_CHER,
+				  AT91_ADC_CH(chan->channel));
+		at91_adc_reg_write(st, AT91_ADC_IER,
+				  AT91_ADC_EOC(chan->channel));
+		at91_adc_reg_write(st, AT91_ADC_CR, AT91_ADC_START);
+
+		ret = wait_event_interruptible_timeout(st->wq_data_avail,
+						       st->done,
+						       msecs_to_jiffies(1000));
+		if (ret == 0)
+			return -ETIMEDOUT;
+		else if (ret < 0)
+			return ret;
+
+		*val = st->last_value;
+
+		at91_adc_reg_write(st, AT91_ADC_CHDR,
+				  AT91_ADC_CH(chan->channel));
+		at91_adc_reg_write(st, AT91_ADC_IDR,
+				  AT91_ADC_EOC(chan->channel));
+
+		st->last_value = 0;
+		st->done = false;
+		mutex_unlock(&st->lock);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = (st->vref_mv * 1000) >> chan->scan_type.realbits;
+		*val2 = 0;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		break;
+	}
+	return -EINVAL;
+}
+
+static const struct iio_info at91_adc_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = &at91_adc_read_raw,
+};
+
+static int __devinit at91_adc_probe(struct platform_device *pdev)
+{
+	unsigned int prsc, mstrclk, ticks;
+	int ret;
+	struct iio_dev *idev;
+	struct at91_adc_state *st;
+	struct resource *res;
+	struct at91_adc_data *pdata = pdev->dev.platform_data;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "No resource defined\n");
+		ret = -ENXIO;
+		goto error_ret;
+	}
+
+	idev = iio_allocate_device(sizeof(struct at91_adc_state));
+	if (idev == NULL) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+
+	platform_set_drvdata(pdev, idev);
+
+	idev->dev.parent = &pdev->dev;
+	idev->name = dev_name(&pdev->dev);
+	idev->modes = INDIO_DIRECT_MODE;
+	idev->info = &at91_adc_info;
+
+	st = iio_priv(idev);
+	st->irq = platform_get_irq(pdev, 0);
+	if (st->irq < 0) {
+		dev_err(&pdev->dev, "No IRQ ID is designated\n");
+		ret = -ENODEV;
+		goto error_free_device;
+	}
+
+	if (!request_mem_region(res->start, resource_size(res),
+				"AT91 adc registers")) {
+		dev_err(&pdev->dev, "Resources are unavailable.\n");
+		ret = -EBUSY;
+		goto error_free_device;
+	}
+
+	st->reg_base = ioremap(res->start, resource_size(res));
+	if (!st->reg_base) {
+		dev_err(&pdev->dev, "Failed to map registers.\n");
+		ret = -ENOMEM;
+		goto error_release_mem;
+	}
+
+	/*
+	 * Disable all IRQs before setting up the handler
+	 */
+	at91_adc_reg_write(st, AT91_ADC_CR, AT91_ADC_SWRST);
+	at91_adc_reg_write(st, AT91_ADC_IDR, 0xFFFFFFFF);
+	ret = request_irq(st->irq,
+			  at91_adc_eoc_trigger,
+			  0,
+			  pdev->dev.driver->name,
+			  idev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to allocate IRQ.\n");
+		goto error_unmap_reg;
+	}
+
+	st->clk = clk_get(&pdev->dev, "adc_clk");
+	if (IS_ERR(st->clk)) {
+		dev_err(&pdev->dev, "Failed to get the clock.\n");
+		ret = PTR_ERR(st->clk);
+		goto error_free_irq;
+	}
+
+	clk_enable(st->clk);
+	mstrclk = clk_get_rate(st->clk);
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "No platform data available.\n");
+		ret = -EINVAL;
+		goto error_free_clk;
+	}
+
+	if (!pdata->adc_clock) {
+		dev_err(&pdev->dev, "No ADCClock available.\n");
+		ret = -EINVAL;
+		goto error_free_clk;
+	}
+
+	/*
+	 * Prescaler rate computation using the formula from the Atmel's
+	 * datasheet : ADC Clock = MCK / ((Prescaler + 1) * 2), ADC Clock being
+	 * specified by the electrical characteristics of the board.
+	 */
+	prsc = (mstrclk / (2 * pdata->adc_clock)) - 1;
+
+	if (!pdata->startup_time) {
+		dev_err(&pdev->dev, "No startup time available.\n");
+		ret = -EINVAL;
+		goto error_free_clk;
+	}
+
+	/*
+	 * Number of ticks needed to cover the startup time of the ADC as
+	 * defined in the electrical characteristics of the board, divided by 8.
+	 * The formula thus is : Startup Time = (ticks + 1) * 8 / ADC Clock
+	 */
+	ticks = round_up((pdata->startup_time * pdata->adc_clock /
+			  1000000) - 1, 8) / 8;
+	at91_adc_reg_write(st, AT91_ADC_MR,
+			  (AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
+			  (AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
+
+	/* Setup the ADC channels available on the board */
+	ret = at91_adc_channel_init(idev, pdata);
+	if (ret < 0)
+		goto error_free_clk;
+
+	init_waitqueue_head(&st->wq_data_avail);
+	mutex_init(&st->lock);
+
+	st->vref_mv = pdata->vref;
+	st->channels_mask = pdata->channels_used;
+
+	ret = iio_device_register(idev);
+	if (ret < 0)
+		goto error_free_channels;
+
+	return 0;
+
+error_free_channels:
+	at91_adc_channel_remove(idev);
+error_free_clk:
+	clk_disable(st->clk);
+	clk_put(st->clk);
+error_free_irq:
+	free_irq(st->irq, idev);
+error_unmap_reg:
+	iounmap(st->reg_base);
+error_release_mem:
+	release_mem_region(res->start, resource_size(res));
+error_free_device:
+	iio_free_device(idev);
+error_ret:
+	return ret;
+}
+
+static int __devexit at91_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *idev = platform_get_drvdata(pdev);
+	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct at91_adc_state *st = iio_priv(idev);
+
+	iio_device_unregister(idev);
+	at91_adc_channel_remove(idev);
+	clk_disable(st->clk);
+	clk_put(st->clk);
+	free_irq(st->irq, idev);
+	iounmap(st->reg_base);
+	release_mem_region(res->start, resource_size(res));
+	iio_free_device(idev);
+
+	return 0;
+}
+
+static struct platform_driver at91_adc_driver = {
+	.probe = at91_adc_probe,
+	.remove = __devexit_p(at91_adc_remove),
+	.driver = {
+		   .name = "at91_adc",
+	},
+};
+
+module_platform_driver(at91_adc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Atmel AT91 ADC Driver");
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
-- 
1.7.4.1

[PATCH 3/5] ARM: AT91: Add the ADC to the sam9g20ek board

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Cc: Patrice Vilchez <patrice.vilchez@atmel.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-at91/at91sam9260_devices.c |   62 ++++++++++++++++++++++++++++++
 arch/arm/mach-at91/board-sam9g20ek.c     |   13 ++++++
 2 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9260_devices.c b/arch/arm/mach-at91/at91sam9260_devices.c
index 24b6f8c..3575019 100644
--- a/arch/arm/mach-at91/at91sam9260_devices.c
+++ b/arch/arm/mach-at91/at91sam9260_devices.c
@@ -17,6 +17,8 @@
 #include <linux/platform_device.h>
 #include <linux/i2c-gpio.h>
 
+#include <linux/platform_data/at91_adc.h>
+
 #include <mach/board.h>
 #include <mach/cpu.h>
 #include <mach/at91sam9260.h>
@@ -1320,6 +1322,66 @@ void __init at91_add_device_cf(struct at91_cf_data *data)
 void __init at91_add_device_cf(struct at91_cf_data * data) {}
 #endif
 
+/* --------------------------------------------------------------------
+ *  ADCs
+ * -------------------------------------------------------------------- */
+
+#if defined(CONFIG_AT91_ADC) || defined(CONFIG_AT91_ADC_MODULE)
+static struct at91_adc_data adc_data;
+
+static struct resource adc_resources[] = {
+	[0] = {
+		.start	= AT91SAM9260_BASE_ADC,
+		.end	= AT91SAM9260_BASE_ADC + SZ_16K - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= AT91SAM9260_ID_ADC,
+		.end	= AT91SAM9260_ID_ADC,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device at91_adc_device = {
+	.name		= "at91_adc",
+	.id		= -1,
+	.dev		= {
+				.platform_data		= &adc_data,
+	},
+	.resource	= adc_resources,
+	.num_resources	= ARRAY_SIZE(adc_resources),
+};
+
+void __init at91_add_device_adc(struct at91_adc_data *data)
+{
+	if (!data)
+		return;
+
+	if (test_bit(0, &data->channels_used))
+		at91_set_A_periph(AT91_PIN_PC0, 0);
+	if (test_bit(1, &data->channels_used))
+		at91_set_A_periph(AT91_PIN_PC1, 0);
+	if (test_bit(2, &data->channels_used))
+		at91_set_A_periph(AT91_PIN_PC2, 0);
+	if (test_bit(3, &data->channels_used))
+		at91_set_A_periph(AT91_PIN_PC3, 0);
+
+	/*
+	 * The electrical characteristics part of the AT91SAM9G20 datasheet
+	 * sets the ADC clock to 5MHz.
+	 */
+	data->adc_clock = 5000000;
+
+	data->num_channels = 4;
+	data->startup_time = 10;
+
+	adc_data = *data;
+	platform_device_register(&at91_adc_device);
+}
+#else
+void __init at91_add_device_adc(struct at91_adc_data *data) {}
+#endif
+
 /* -------------------------------------------------------------------- */
 /*
  * These devices are always present and don't need any board-specific
diff --git a/arch/arm/mach-at91/board-sam9g20ek.c b/arch/arm/mach-at91/board-sam9g20ek.c
index 64fc75c..7758f34 100644
--- a/arch/arm/mach-at91/board-sam9g20ek.c
+++ b/arch/arm/mach-at91/board-sam9g20ek.c
@@ -32,6 +32,8 @@
 #include <linux/regulator/fixed.h>
 #include <linux/regulator/consumer.h>
 
+#include <linux/platform_data/at91_adc.h>
+
 #include <mach/hardware.h>
 #include <asm/setup.h>
 #include <asm/mach-types.h>
@@ -309,6 +311,15 @@ static void __init ek_add_device_buttons(void)
 static void __init ek_add_device_buttons(void) {}
 #endif
 
+/*
+ * ADCs
+ */
+
+static struct at91_adc_data ek_adc_data = {
+	.channels_used = BIT(0) | BIT(1) | BIT(2) | BIT(3),
+	.vref = 3300,
+};
+
 #if defined(CONFIG_REGULATOR_FIXED_VOLTAGE) || defined(CONFIG_REGULATOR_FIXED_VOLTAGE_MODULE)
 static struct regulator_consumer_supply ek_audio_consumer_supplies[] = {
 	REGULATOR_SUPPLY("AVDD", "0-001b"),
@@ -384,6 +395,8 @@ static void __init ek_board_init(void)
 	ek_add_device_gpio_leds();
 	/* Push Buttons */
 	ek_add_device_buttons();
+	/* ADCs */
+	at91_add_device_adc(&ek_adc_data);
 	/* PCK0 provides MCLK to the WM8731 */
 	at91_set_B_periph(AT91_PIN_PC1, 0);
 	/* SSC (for WM8731) */
-- 
1.7.4.1

[PATCH 4/5] AT91:IIO: Move the SoC specific informations to the ADC driver

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Cc: Patrice Vilchez <patrice.vilchez@atmel.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-at91/at91sam9260_devices.c |    9 -----
 drivers/staging/iio/adc/at91_adc.c       |   51 ++++++++++++++++++++++++++---
 include/linux/platform_data/at91_adc.h   |   11 +------
 3 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9260_devices.c b/arch/arm/mach-at91/at91sam9260_devices.c
index 3575019..95b3127 100644
--- a/arch/arm/mach-at91/at91sam9260_devices.c
+++ b/arch/arm/mach-at91/at91sam9260_devices.c
@@ -1366,15 +1366,6 @@ void __init at91_add_device_adc(struct at91_adc_data *data)
 	if (test_bit(3, &data->channels_used))
 		at91_set_A_periph(AT91_PIN_PC3, 0);
 
-	/*
-	 * The electrical characteristics part of the AT91SAM9G20 datasheet
-	 * sets the ADC clock to 5MHz.
-	 */
-	data->adc_clock = 5000000;
-
-	data->num_channels = 4;
-	data->startup_time = 10;
-
 	adc_data = *data;
 	platform_device_register(&at91_adc_device);
 }
diff --git a/drivers/staging/iio/adc/at91_adc.c b/drivers/staging/iio/adc/at91_adc.c
index d10df7a..b357363 100644
--- a/drivers/staging/iio/adc/at91_adc.c
+++ b/drivers/staging/iio/adc/at91_adc.c
@@ -24,6 +24,20 @@
 #include <linux/platform_data/at91_adc.h>
 
 #include <mach/at91_adc.h>
+#include <mach/cpu.h>
+
+struct at91_adc_desc {
+	/* ADC Clock as specified by the datasheet, in Hz. */
+	u32 clock;
+	/*
+	 * Global number of channels available (to specify which channels are
+	 * indeed used on the board, see the channels_used bitmask in the
+	 * platform data).
+	 */
+	u8 num_channels;
+	/* Startup time of the ADC, in microseconds. */
+	u8 startup_time;
+};
 
 struct at91_adc_state {
 	struct clk *clk;
@@ -35,8 +49,26 @@ struct at91_adc_state {
 	void __iomem *reg_base;
 	unsigned int vref_mv;
 	unsigned long channels_mask;
+	bool irq_enabled;
+	struct at91_adc_desc *desc;
 };
 
+static struct at91_adc_desc at91_adc_desc_sam9g20 = {
+	.clock = 5000000,
+	.num_channels = 4,
+	.startup_time = 10,
+};
+
+static int at91_adc_select_soc(struct at91_adc_state *st)
+{
+	if (cpu_is_at91sam9g20()) {
+		st->desc = &at91_adc_desc_sam9g20;
+		return 0;
+	}
+
+	return -ENODEV;
+}
+
 static inline u32 at91_adc_reg_read(struct at91_adc_state *st,
 				    u8 reg)
 {
@@ -72,18 +104,19 @@ static irqreturn_t at91_adc_eoc_trigger(int irq, void *private)
 static int at91_adc_channel_init(struct iio_dev *idev,
 				struct at91_adc_data *pdata)
 {
+	struct at91_adc_state *st = iio_priv(idev);
 	struct iio_chan_spec *chan_array;
 	int bit, idx = 0;
 
 	idev->num_channels = bitmap_weight(&pdata->channels_used,
-					   pdata->num_channels);
+					   st->desc->num_channels);
 	chan_array = kcalloc(idev->num_channels, sizeof(struct iio_chan_spec),
 			     GFP_KERNEL);
 
 	if (chan_array == NULL)
 		return -ENOMEM;
 
-	for_each_set_bit(bit, &pdata->channels_used, pdata->num_channels) {
+	for_each_set_bit(bit, &pdata->channels_used, st->desc->num_channels) {
 		struct iio_chan_spec *chan = chan_array + idx;
 		chan->type = IIO_VOLTAGE;
 		chan->indexed = 1;
@@ -186,6 +219,12 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	idev->info = &at91_adc_info;
 
 	st = iio_priv(idev);
+	ret = at91_adc_select_soc(st);
+	if (ret) {
+		dev_err(&pdev->dev, "SoC unknown\n");
+		goto error_free_device;
+	}
+
 	st->irq = platform_get_irq(pdev, 0);
 	if (st->irq < 0) {
 		dev_err(&pdev->dev, "No IRQ ID is designated\n");
@@ -238,7 +277,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 		goto error_free_clk;
 	}
 
-	if (!pdata->adc_clock) {
+	if (!st->desc->clock) {
 		dev_err(&pdev->dev, "No ADCClock available.\n");
 		ret = -EINVAL;
 		goto error_free_clk;
@@ -249,9 +288,9 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	 * datasheet : ADC Clock = MCK / ((Prescaler + 1) * 2), ADC Clock being
 	 * specified by the electrical characteristics of the board.
 	 */
-	prsc = (mstrclk / (2 * pdata->adc_clock)) - 1;
+	prsc = (mstrclk / (2 * st->desc->clock)) - 1;
 
-	if (!pdata->startup_time) {
+	if (!st->desc->startup_time) {
 		dev_err(&pdev->dev, "No startup time available.\n");
 		ret = -EINVAL;
 		goto error_free_clk;
@@ -262,7 +301,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	 * defined in the electrical characteristics of the board, divided by 8.
 	 * The formula thus is : Startup Time = (ticks + 1) * 8 / ADC Clock
 	 */
-	ticks = round_up((pdata->startup_time * pdata->adc_clock /
+	ticks = round_up((st->desc->startup_time * st->desc->clock /
 			  1000000) - 1, 8) / 8;
 	at91_adc_reg_write(st, AT91_ADC_MR,
 			  (AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
diff --git a/include/linux/platform_data/at91_adc.h b/include/linux/platform_data/at91_adc.h
index 1e1813d..9c5a64e 100644
--- a/include/linux/platform_data/at91_adc.h
+++ b/include/linux/platform_data/at91_adc.h
@@ -16,19 +16,10 @@
 #define _AT91_ADC_H_
 
 struct at91_adc_data {
-	/* ADC Clock as specified by the datasheet, in Hz. */
-	unsigned int adc_clock;
-	/*
-	 * Global number of channels available (to specify which channels are
-	 * indeed used on the board, see the channels_used bitmask).
-	 */
-	u8 num_channels;
 	/* Channels in use on the board as a bitmask */
 	unsigned long channels_used;
-	/* Startup time of the ADC, in microseconds. */
-	u8 startup_time;
 	/* Reference voltage for the ADC in millivolts */
-	unsigned short vref;
+	u16 vref;
 };
 
 extern void __init at91_add_device_adc(struct at91_adc_data *data);
-- 
1.7.4.1

[PATCH 5/5] AT91:IIO: Add support for hardware triggers for the ADC

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Cc: Patrice Vilchez <patrice.vilchez@atmel.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 arch/arm/mach-at91/at91sam9260_devices.c |    3 +
 arch/arm/mach-at91/board-sam9g20ek.c     |    1 +
 drivers/staging/iio/adc/Kconfig          |   11 +-
 drivers/staging/iio/adc/at91_adc.c       |  329 +++++++++++++++++++++++++++++-
 include/linux/platform_data/at91_adc.h   |    2 +
 5 files changed, 331 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9260_devices.c b/arch/arm/mach-at91/at91sam9260_devices.c
index 95b3127..9dc9f96 100644
--- a/arch/arm/mach-at91/at91sam9260_devices.c
+++ b/arch/arm/mach-at91/at91sam9260_devices.c
@@ -1366,6 +1366,9 @@ void __init at91_add_device_adc(struct at91_adc_data *data)
 	if (test_bit(3, &data->channels_used))
 		at91_set_A_periph(AT91_PIN_PC3, 0);
 
+	if (data->use_external)
+		at91_set_A_periph(AT91_PIN_PA22, 0);
+
 	adc_data = *data;
 	platform_device_register(&at91_adc_device);
 }
diff --git a/arch/arm/mach-at91/board-sam9g20ek.c b/arch/arm/mach-at91/board-sam9g20ek.c
index 7758f34..f18f6f2 100644
--- a/arch/arm/mach-at91/board-sam9g20ek.c
+++ b/arch/arm/mach-at91/board-sam9g20ek.c
@@ -317,6 +317,7 @@ static void __init ek_add_device_buttons(void) {}
 
 static struct at91_adc_data ek_adc_data = {
 	.channels_used = BIT(0) | BIT(1) | BIT(2) | BIT(3),
+	.use_external = true,
 	.vref = 3300,
 };
 
diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index 93de64d..9d87c75 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -170,10 +170,13 @@ config AD7280
 	  module will be called ad7280a
 
 config AT91_ADC
-       tristate "Atmel AT91 ADC"
-       depends on SYSFS && ARCH_AT91
-       help
-         Say yes here to build support for Atmel AT91 ADC
+	tristate "Atmel AT91 ADC"
+	depends on SYSFS && ARCH_AT91
+	select IIO_BUFFER
+	select IIO_SW_RING
+	select IIO_TRIGGER
+	help
+	  Say yes here to build support for Atmel AT91 ADC.
 
 config MAX1363
 	tristate "Maxim max1363 ADC driver"
diff --git a/drivers/staging/iio/adc/at91_adc.c b/drivers/staging/iio/adc/at91_adc.c
index b357363..74baaf3 100644
--- a/drivers/staging/iio/adc/at91_adc.c
+++ b/drivers/staging/iio/adc/at91_adc.c
@@ -23,9 +23,20 @@
 #include "../iio.h"
 #include <linux/platform_data/at91_adc.h>
 
+#include "../buffer.h"
+#include "../ring_sw.h"
+#include "../trigger.h"
+#include "../trigger_consumer.h"
+
 #include <mach/at91_adc.h>
 #include <mach/cpu.h>
 
+struct at91_adc_trigger {
+	char *name;
+	u8 value;
+	bool is_external;
+};
+
 struct at91_adc_desc {
 	/* ADC Clock as specified by the datasheet, in Hz. */
 	u32 clock;
@@ -37,6 +48,7 @@ struct at91_adc_desc {
 	u8 num_channels;
 	/* Startup time of the ADC, in microseconds. */
 	u8 startup_time;
+	struct at91_adc_trigger *triggers;
 };
 
 struct at91_adc_state {
@@ -51,12 +63,37 @@ struct at91_adc_state {
 	unsigned long channels_mask;
 	bool irq_enabled;
 	struct at91_adc_desc *desc;
+	struct iio_trigger **trig;
+};
+
+static struct at91_adc_trigger at91_adc_trigger_sam9g20[] = {
+	[0] = {
+		.name = "TC0",
+		.value = AT91_ADC_TRGSEL_TC0 | AT91_ADC_TRGEN,
+		.is_external = false,
+	},
+	[1] = {
+		.name = "TC1",
+		.value = AT91_ADC_TRGSEL_TC1 | AT91_ADC_TRGEN,
+		.is_external = false,
+	},
+	[2] = {
+		.name = "TC2",
+		.value = AT91_ADC_TRGSEL_TC2 | AT91_ADC_TRGEN,
+		.is_external = false,
+	},
+	[3] = {
+		.name = "external",
+		.value = AT91_ADC_TRGSEL_EXTERNAL | AT91_ADC_TRGEN,
+		.is_external = true,
+	},
 };
 
 static struct at91_adc_desc at91_adc_desc_sam9g20 = {
 	.clock = 5000000,
 	.num_channels = 4,
 	.startup_time = 10,
+	.triggers = at91_adc_trigger_sam9g20,
 };
 
 static int at91_adc_select_soc(struct at91_adc_state *st)
@@ -82,27 +119,124 @@ static inline void at91_adc_reg_write(struct at91_adc_state *st,
 	writel_relaxed(val, st->reg_base + reg);
 }
 
+static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct at91_adc_state *st = iio_priv(idev);
+	struct iio_buffer *buffer = idev->buffer;
+	int len = 0;
+
+	size_t datasize = buffer->access->get_bytes_per_datum(buffer);
+	u16 *data = kmalloc(datasize, GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	/* Needed to ACK the DRDY interruption */
+	at91_adc_reg_read(st, AT91_ADC_LCDR);
+
+	if (buffer->scan_count) {
+		int i, j;
+		for (i = 0, j = 0; i < buffer->scan_count; i++) {
+			j = find_next_bit(buffer->scan_mask,
+					  st->desc->num_channels,
+					  j);
+			data[i] = at91_adc_reg_read(st, AT91_ADC_CHR(j));
+			j++;
+			len += 2;
+		}
+	}
+
+	if (buffer->scan_timestamp) {
+		s64 *timestamp = (s64 *)((u8 *)data + round_up(len,
+							       sizeof(s64)));
+		*timestamp = iio_get_time_ns();
+	}
+
+	buffer->access->store_to(buffer, (u8 *)data, pf->timestamp);
+
+	kfree(data);
+
+	iio_trigger_notify_done(idev->trig);
+	st->irq_enabled = true;
+	enable_irq(st->irq);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t at91_adc_eoc_trigger(int irq, void *private)
 {
 	struct iio_dev *idev = private;
 	struct at91_adc_state *st = iio_priv(idev);
-	unsigned int status = at91_adc_reg_read(st, AT91_ADC_SR);
 
-	if (!(status & AT91_ADC_DRDY))
-		return IRQ_HANDLED;
+	st->done = true;
+	wake_up_interruptible(&st->wq_data_avail);
 
-	if (status & st->channels_mask) {
-		st->done = true;
+	if (iio_buffer_enabled(idev)) {
+		disable_irq_nosync(irq);
+		st->irq_enabled = false;
+		iio_trigger_poll(idev->trig, iio_get_time_ns());
+	} else {
 		st->last_value = at91_adc_reg_read(st, AT91_ADC_LCDR);
 	}
 
-	wake_up_interruptible(&st->wq_data_avail);
-
 	return IRQ_HANDLED;
 }
 
+static const struct iio_buffer_setup_ops at91_adc_buffer_setup_ops = {
+	.preenable = &iio_sw_buffer_preenable,
+	.postenable = &iio_triggered_buffer_postenable,
+	.predisable = &iio_triggered_buffer_predisable,
+};
+
+static int at91_adc_buffer_init(struct iio_dev *idev)
+{
+	struct iio_buffer *buffer;
+	int ret;
+
+	buffer = iio_sw_rb_allocate(idev);
+	if (buffer == NULL) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+
+	idev->buffer = buffer;
+	buffer->scan_timestamp = true;
+	buffer->bpe = 2;
+	buffer->access = &ring_sw_access_funcs;
+	buffer->setup_ops = &at91_adc_buffer_setup_ops;
+	buffer->owner = THIS_MODULE;
+
+	idev->pollfunc = iio_alloc_pollfunc(NULL,
+					    &at91_adc_trigger_handler,
+					    0,
+					    idev,
+					    "at91_adc-consumer-%d",
+					    idev->id);
+
+	if (idev->pollfunc == NULL) {
+		ret = -ENOMEM;
+		goto error_free_buffer;
+	}
+
+	idev->modes |= INDIO_BUFFER_TRIGGERED;
+	return 0;
+
+error_free_buffer:
+	iio_sw_rb_free(idev->buffer);
+error_ret:
+	return ret;
+
+}
+
+static void at91_adc_buffer_remove(struct iio_dev *indio_dev)
+{
+	iio_dealloc_pollfunc(indio_dev->pollfunc);
+	iio_sw_rb_free(indio_dev->buffer);
+}
+
 static int at91_adc_channel_init(struct iio_dev *idev,
-				struct at91_adc_data *pdata)
+				 struct at91_adc_data *pdata)
 {
 	struct at91_adc_state *st = iio_priv(idev);
 	struct iio_chan_spec *chan_array;
@@ -121,6 +255,7 @@ static int at91_adc_channel_init(struct iio_dev *idev,
 		chan->type = IIO_VOLTAGE;
 		chan->indexed = 1;
 		chan->channel = bit;
+		chan->scan_index = idx;
 		chan->scan_type.sign = 'u';
 		chan->scan_type.realbits = 10;
 		chan->scan_type.storagebits = 16;
@@ -137,6 +272,145 @@ static void at91_adc_channel_remove(struct iio_dev *idev)
 	kfree(idev->channels);
 }
 
+static int at91_adc_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *idev = trig->private_data;
+	struct at91_adc_state *st = iio_priv(idev);
+	struct iio_buffer *buffer = idev->buffer;
+	u32 status = at91_adc_reg_read(st, AT91_ADC_MR);
+	u8 bit;
+
+	if (state) {
+		int i;
+
+		for (i = 0; i < sizeof(st->desc->triggers); i++) {
+			char *name = kasprintf(GFP_KERNEL,
+					       "%s-dev%d-%s",
+					       idev->name,
+					       idev->id,
+					       st->desc->triggers[i].name);
+
+			if (!strcmp(idev->trig->name, name)) {
+				status |= st->desc->triggers[i].value;
+				kfree(name);
+				break;
+			}
+
+			kfree(name);
+		}
+
+		if (i == sizeof(st->desc->triggers))
+			return -EINVAL;
+
+		at91_adc_reg_write(st, AT91_ADC_MR, status);
+
+		for_each_set_bit(bit, buffer->scan_mask,
+				 st->desc->num_channels) {
+			struct iio_chan_spec const *chan = idev->channels + bit;
+			at91_adc_reg_write(st, AT91_ADC_CHER,
+					   AT91_ADC_CH(chan->channel));
+		}
+
+		at91_adc_reg_write(st, AT91_ADC_IER, AT91_ADC_DRDY);
+
+
+	} else {
+		at91_adc_reg_write(st, AT91_ADC_IDR, AT91_ADC_DRDY);
+		at91_adc_reg_write(st, AT91_ADC_MR,
+				   status & ~AT91_ADC_TRGEN);
+
+		for_each_set_bit(bit, buffer->scan_mask,
+				 st->desc->num_channels) {
+			at91_adc_reg_write(st, AT91_ADC_CHDR,
+					   AT91_ADC_CH(bit));
+		}
+	}
+
+	return 0;
+}
+
+static const struct iio_trigger_ops at91_adc_trigger_ops = {
+	.owner = THIS_MODULE,
+	.set_trigger_state = &at91_adc_set_trigger_state,
+};
+
+
+static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *idev,
+						     struct at91_adc_trigger *trigger)
+{
+	struct iio_trigger *trig = iio_allocate_trigger("%s-dev%d-%s",
+							idev->name,
+							idev->id,
+							trigger->name);
+	int ret;
+
+	if (trig == NULL)
+		return NULL;
+
+	trig->dev.parent = idev->dev.parent;
+	trig->owner = THIS_MODULE;
+	trig->private_data = idev;
+	trig->ops = &at91_adc_trigger_ops;
+
+	ret = iio_trigger_register(trig);
+	if (ret < 0)
+		return NULL;
+
+	return trig;
+}
+
+static int at91_adc_trigger_init(struct iio_dev *idev,
+				 struct at91_adc_data *pdata)
+{
+	struct at91_adc_state *st = iio_priv(idev);
+	int i, ret;
+
+	st->trig = kcalloc(sizeof(st->desc->triggers),
+			   sizeof(struct iio_trigger *),
+			   GFP_KERNEL);
+
+	if (st->trig == NULL) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+
+	for (i = 0; i < sizeof(st->desc->triggers); i++) {
+		if (st->desc->triggers[i].is_external && !(pdata->use_external))
+			continue;
+
+		st->trig[i] = at91_adc_allocate_trigger(idev,
+							st->desc->triggers + i);
+		if (st->trig[0] == NULL) {
+			ret = -ENOMEM;
+			goto error_trigger;
+		}
+	}
+
+	return 0;
+
+error_trigger:
+	for (; i >= 0; i--) {
+		iio_trigger_unregister(st->trig[i]);
+		iio_free_trigger(st->trig[i]);
+	}
+	kfree(st->trig);
+error_ret:
+	return ret;
+}
+
+static void at91_adc_trigger_remove(struct iio_dev *idev)
+{
+	struct at91_adc_state *st = iio_priv(idev);
+	int i;
+
+	for (i = 0; i < sizeof(st->trig); i++) {
+		iio_trigger_unregister(st->trig[i]);
+		iio_free_trigger(st->trig[i]);
+	}
+
+	kfree(st->trig);
+}
+
 static int at91_adc_read_raw(struct iio_dev *idev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -309,8 +583,10 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 
 	/* Setup the ADC channels available on the board */
 	ret = at91_adc_channel_init(idev, pdata);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Couldn't initialize the channels.\n");
 		goto error_free_clk;
+	}
 
 	init_waitqueue_head(&st->wq_data_avail);
 	mutex_init(&st->lock);
@@ -318,12 +594,40 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	st->vref_mv = pdata->vref;
 	st->channels_mask = pdata->channels_used;
 
-	ret = iio_device_register(idev);
-	if (ret < 0)
+	ret = at91_adc_buffer_init(idev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Couldn't initialize the buffer.\n");
 		goto error_free_channels;
+	}
+
+	ret = iio_buffer_register(idev,
+				  idev->channels,
+				  idev->num_channels);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Couldn't register the buffer.\n");
+		goto error_free_buffer;
+	}
+
+	ret = at91_adc_trigger_init(idev, pdata);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Couldn't setup the triggers.\n");
+		goto error_unregister_buffer;
+	}
+
+	ret = iio_device_register(idev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Couldn't register the device.\n");
+		goto error_remove_triggers;
+	}
 
 	return 0;
 
+error_remove_triggers:
+	at91_adc_trigger_remove(idev);
+error_unregister_buffer:
+	iio_buffer_unregister(idev);
+error_free_buffer:
+	at91_adc_buffer_remove(idev);
 error_free_channels:
 	at91_adc_channel_remove(idev);
 error_free_clk:
@@ -348,6 +652,9 @@ static int __devexit at91_adc_remove(struct platform_device *pdev)
 	struct at91_adc_state *st = iio_priv(idev);
 
 	iio_device_unregister(idev);
+	at91_adc_trigger_remove(idev);
+	iio_buffer_unregister(idev);
+	at91_adc_buffer_remove(idev);
 	at91_adc_channel_remove(idev);
 	clk_disable(st->clk);
 	clk_put(st->clk);
diff --git a/include/linux/platform_data/at91_adc.h b/include/linux/platform_data/at91_adc.h
index 9c5a64e..5b65eff 100644
--- a/include/linux/platform_data/at91_adc.h
+++ b/include/linux/platform_data/at91_adc.h
@@ -18,6 +18,8 @@
 struct at91_adc_data {
 	/* Channels in use on the board as a bitmask */
 	unsigned long channels_used;
+	/* Do we need the external triggers ? */
+	bool use_external;
 	/* Reference voltage for the ADC in millivolts */
 	u16 vref;
 };
-- 
1.7.4.1

Re: [PATCH 5/5] AT91:IIO: Add support for hardware triggers for the ADC

[Jonathan Cameron]

Hi Maxime.

Mostly fine, though I'd structure a few corners differently as explained
inline.

Also, I don't immediately see a reason not to use threaded interrupts
for much
of what we have here and that would definitely be preferable if possible!

Review may be more thorough that you wanted at this stage, but given I was
reading the code I thought I might as well do it along the way!

Jonathan

On 12/02/2011 03:35 PM, Maxime Ripard wrote:
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Cc: Patrice Vilchez <patrice.vilchez@atmel.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  arch/arm/mach-at91/at91sam9260_devices.c |    3 +
>  arch/arm/mach-at91/board-sam9g20ek.c     |    1 +
>  drivers/staging/iio/adc/Kconfig          |   11 +-
>  drivers/staging/iio/adc/at91_adc.c       |  329 +++++++++++++++++++++++++++++-
>  include/linux/platform_data/at91_adc.h   |    2 +
>  5 files changed, 331 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/at91sam9260_devices.c b/arch/arm/mach-at91/at91sam9260_devices.c
> index 95b3127..9dc9f96 100644
> --- a/arch/arm/mach-at91/at91sam9260_devices.c
> +++ b/arch/arm/mach-at91/at91sam9260_devices.c
> @@ -1366,6 +1366,9 @@ void __init at91_add_device_adc(struct at91_adc_data *data)
>  	if (test_bit(3, &data->channels_used))
>  		at91_set_A_periph(AT91_PIN_PC3, 0);
>  
> +	if (data->use_external)
> +		at91_set_A_periph(AT91_PIN_PA22, 0);
> +
>  	adc_data = *data;
>  	platform_device_register(&at91_adc_device);
>  }
> diff --git a/arch/arm/mach-at91/board-sam9g20ek.c b/arch/arm/mach-at91/board-sam9g20ek.c
> index 7758f34..f18f6f2 100644
> --- a/arch/arm/mach-at91/board-sam9g20ek.c
> +++ b/arch/arm/mach-at91/board-sam9g20ek.c
> @@ -317,6 +317,7 @@ static void __init ek_add_device_buttons(void) {}
>  
>  static struct at91_adc_data ek_adc_data = {
>  	.channels_used = BIT(0) | BIT(1) | BIT(2) | BIT(3),
> +	.use_external = true,
>  	.vref = 3300,
>  };
>  
> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
> index 93de64d..9d87c75 100644
> --- a/drivers/staging/iio/adc/Kconfig
> +++ b/drivers/staging/iio/adc/Kconfig
> @@ -170,10 +170,13 @@ config AD7280
>  	  module will be called ad7280a
>  
>  config AT91_ADC
> -       tristate "Atmel AT91 ADC"
> -       depends on SYSFS && ARCH_AT91
> -       help
> -         Say yes here to build support for Atmel AT91 ADC
> +	tristate "Atmel AT91 ADC"
> +	depends on SYSFS && ARCH_AT91
> +	select IIO_BUFFER
> +	select IIO_SW_RING
> +	select IIO_TRIGGER
I assume this is true because some of these changes mean
that it will simply not function without the buffered side?
Given that one typical usecase for this device is simple
hwmon type monitoring (fully pull interface in kernel)
I'd be inclined to see how much extra code would be
required to allow it to run as before adding buffered support.
If it is really uggly then fair enough!
> +	help
> +	  Say yes here to build support for Atmel AT91 ADC.
>  
>  config MAX1363
>  	tristate "Maxim max1363 ADC driver"
> diff --git a/drivers/staging/iio/adc/at91_adc.c b/drivers/staging/iio/adc/at91_adc.c
> index b357363..74baaf3 100644
> --- a/drivers/staging/iio/adc/at91_adc.c
> +++ b/drivers/staging/iio/adc/at91_adc.c
> @@ -23,9 +23,20 @@
>  #include "../iio.h"
>  #include <linux/platform_data/at91_adc.h>
>  
> +#include "../buffer.h"
> +#include "../ring_sw.h"
> +#include "../trigger.h"
> +#include "../trigger_consumer.h"
> +
>  #include <mach/at91_adc.h>
>  #include <mach/cpu.h>
>  
I'd like to see some docs for this.  Obviously
name is self explanatory but value isn't!
> +struct at91_adc_trigger {
> +	char *name;
> +	u8 value;
> +	bool is_external;
> +};
> +
>  struct at91_adc_desc {
>  	/* ADC Clock as specified by the datasheet, in Hz. */
>  	u32 clock;
> @@ -37,6 +48,7 @@ struct at91_adc_desc {
>  	u8 num_channels;
>  	/* Startup time of the ADC, in microseconds. */
>  	u8 startup_time;
> +	struct at91_adc_trigger *triggers;
>  };
>  
>  struct at91_adc_state {
> @@ -51,12 +63,37 @@ struct at91_adc_state {
>  	unsigned long channels_mask;
>  	bool irq_enabled;
>  	struct at91_adc_desc *desc;
> +	struct iio_trigger **trig;
> +};
> +
Trigger names are a little cryptic. I'd be inclined to make them
a bit longer and hence perhaps self explanatory if you can.  Note
as first device of this type you are pretty much setting the
naming standard and I think these will appear in some of the user
space abi, so they won't be easy to change later!
For that reason we are going to be fussier than normal.

> +static struct at91_adc_trigger at91_adc_trigger_sam9g20[] = {
> +	[0] = {
> +		.name = "TC0",
> +		.value = AT91_ADC_TRGSEL_TC0 | AT91_ADC_TRGEN,
> +		.is_external = false,
> +	},
> +	[1] = {
> +		.name = "TC1",
> +		.value = AT91_ADC_TRGSEL_TC1 | AT91_ADC_TRGEN,
> +		.is_external = false,
> +	},
> +	[2] = {
> +		.name = "TC2",
> +		.value = AT91_ADC_TRGSEL_TC2 | AT91_ADC_TRGEN,
> +		.is_external = false,
> +	},
> +	[3] = {
> +		.name = "external",
> +		.value = AT91_ADC_TRGSEL_EXTERNAL | AT91_ADC_TRGEN,
> +		.is_external = true,
> +	},
How does it know how many there are?  Null terminate this perhaps?
>  };
>  
>  static struct at91_adc_desc at91_adc_desc_sam9g20 = {
>  	.clock = 5000000,
>  	.num_channels = 4,
>  	.startup_time = 10,
> +	.triggers = at91_adc_trigger_sam9g20,
>  };
>  
>  static int at91_adc_select_soc(struct at91_adc_state *st)
> @@ -82,27 +119,124 @@ static inline void at91_adc_reg_write(struct at91_adc_state *st,
>  	writel_relaxed(val, st->reg_base + reg);
>  }
>  
> +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *idev = pf->indio_dev;
> +	struct at91_adc_state *st = iio_priv(idev);
> +	struct iio_buffer *buffer = idev->buffer;
> +	int len = 0;
> +
> +	size_t datasize = buffer->access->get_bytes_per_datum(buffer);
> +	u16 *data = kmalloc(datasize, GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
I know some of my drivers have done this, but I prefer the approach
used by some of the ADI ones.  They allocate these buffers in the
chip_state and hence get this kmalloc out of this fast patch.
That should be fine as we should always be able to make sure it
is the right size in the preenable function for the buffer.
> +
> +	/* Needed to ACK the DRDY interruption */
> +	at91_adc_reg_read(st, AT91_ADC_LCDR);
Here?  Conceptually I'd expect to see this right at the end of the interrupt
handling.  I would imaging acking this means another one can arive.  To
cleanly support capture from other devices on this trigger this would occur
in the try_reenable callback.  That only gets called when the number of
calls to iio_trigger_notify_done is equal to the number of devices waiting
on the trigger (i.e. everyone is done).
> +
> +	if (buffer->scan_count) {
> +		int i, j;
> +		for (i = 0, j = 0; i < buffer->scan_count; i++) {
Just as a heads up, this patch will change shortly due to the
introduction of multiple buffer support so please watch those
patches as they go through and the driver changes we are making..

Also, this looks like a for_each_set_bit function would make this easier to
read.

> +			j = find_next_bit(buffer->scan_mask,
> +					  st->desc->num_channels,
> +					  j);
> +			data[i] = at91_adc_reg_read(st, AT91_ADC_CHR(j));
> +			j++;
> +			len += 2;
> +		}
> +	}
> +
The utility of ALIGN macro has been pointed out to me recently and makes
it more apparent what is going on in cases like this.
Also, conceptually I'd expect to see the timestamp capture occuring earlier
to make it nearer to the trigger point (when I guess the read is actually
occuring - having been lazy and not actually loaded the datasheet ;)
> +	if (buffer->scan_timestamp) {
> +		s64 *timestamp = (s64 *)((u8 *)data + round_up(len,
> +							       sizeof(s64)));
> +		*timestamp = iio_get_time_ns();
> +	}
> +
> +	buffer->access->store_to(buffer, (u8 *)data, pf->timestamp);
> +
> +	kfree(data);
> +
> +	iio_trigger_notify_done(idev->trig);
> +	st->irq_enabled = true;

> +	enable_irq(st->irq);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t at91_adc_eoc_trigger(int irq, void *private)
>  {
>  	struct iio_dev *idev = private;
>  	struct at91_adc_state *st = iio_priv(idev);
> -	unsigned int status = at91_adc_reg_read(st, AT91_ADC_SR);
>  
> -	if (!(status & AT91_ADC_DRDY))
> -		return IRQ_HANDLED;
> +	st->done = true;
Want to wake this up here?  Not after the st->last_value bit below?
Obviously probably doesn't actually matter is we are in an irq handler
but it would make for more logical flow to my mind.  Data is read first
and then we tell those waiting it is available...
> +	wake_up_interruptible(&st->wq_data_avail);
>  
> -	if (status & st->channels_mask) {
> -		st->done = true;
> +	if (iio_buffer_enabled(idev)) {
For ease of flow, can we not make this a one shot interrupt (using
threaded irqs)?  I'm also given we are being notified data is ready
the read doesn't actually need to happen in the top half. I'd prefer
seeing just the check that it is this interrupt and the iio_trigger_poll
happening in the top half.

The trigger_poll is just in case we need to fire a capturing pin on
something else using this trigger alongside this device.


> +		disable_irq_nosync(irq);
> +		st->irq_enabled = false;
> +		iio_trigger_poll(idev->trig, iio_get_time_ns());
> +	} else {
>  		st->last_value = at91_adc_reg_read(st, AT91_ADC_LCDR);
>  	}
>  
> -	wake_up_interruptible(&st->wq_data_avail);
> -
>  	return IRQ_HANDLED;
>  }
>  
> +static const struct iio_buffer_setup_ops at91_adc_buffer_setup_ops = {
> +	.preenable = &iio_sw_buffer_preenable,
> +	.postenable = &iio_triggered_buffer_postenable,
> +	.predisable = &iio_triggered_buffer_predisable,
> +};
> +
> +static int at91_adc_buffer_init(struct iio_dev *idev)
> +{
> +	struct iio_buffer *buffer;
> +	int ret;
> +
> +	buffer = iio_sw_rb_allocate(idev);
> +	if (buffer == NULL) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +
> +	idev->buffer = buffer;
> +	buffer->scan_timestamp = true;
Killing of bpe is scheduled.  Nothing that can't be recovered from
iio_chan_spec structures and shouldn't be used in capture path anyway.
The new iio_sw_buffer_preenable is a lot cleverer!  Easy to change
once those patches have merged of course!

> +	buffer->bpe = 2;
> +	buffer->access = &ring_sw_access_funcs;
> +	buffer->setup_ops = &at91_adc_buffer_setup_ops;
> +	buffer->owner = THIS_MODULE;
> +
Looking at what you have here, you might save a fair bit of code by
taking advantage of Lars-Peter's patch of last week that handles all
this boilerplate under slightly restricted circumstances...

> +	idev->pollfunc = iio_alloc_pollfunc(NULL,
> +					    &at91_adc_trigger_handler,
> +					    0,
> +					    idev,
> +					    "at91_adc-consumer-%d",
> +					    idev->id);
> +
> +	if (idev->pollfunc == NULL) {
> +		ret = -ENOMEM;
> +		goto error_free_buffer;
> +	}
> +
> +	idev->modes |= INDIO_BUFFER_TRIGGERED;
> +	return 0;
> +
> +error_free_buffer:
> +	iio_sw_rb_free(idev->buffer);
> +error_ret:
> +	return ret;
> +
> +}
> +
> +static void at91_adc_buffer_remove(struct iio_dev *indio_dev)
> +{
> +	iio_dealloc_pollfunc(indio_dev->pollfunc);
> +	iio_sw_rb_free(indio_dev->buffer);
> +}
> +
>  static int at91_adc_channel_init(struct iio_dev *idev,
> -				struct at91_adc_data *pdata)
> +				 struct at91_adc_data *pdata)
May be a valid whitespace cleanup, but shouldn't be in this patch.
>  {
>  	struct at91_adc_state *st = iio_priv(idev);
>  	struct iio_chan_spec *chan_array;
> @@ -121,6 +255,7 @@ static int at91_adc_channel_init(struct iio_dev *idev,
>  		chan->type = IIO_VOLTAGE;
>  		chan->indexed = 1;
>  		chan->channel = bit;
> +		chan->scan_index = idx;
>  		chan->scan_type.sign = 'u';
>  		chan->scan_type.realbits = 10;
>  		chan->scan_type.storagebits = 16;
> @@ -137,6 +272,145 @@ static void at91_adc_channel_remove(struct iio_dev *idev)
>  	kfree(idev->channels);
>  }
>  
> +static int at91_adc_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *idev = trig->private_data;
> +	struct at91_adc_state *st = iio_priv(idev);
> +	struct iio_buffer *buffer = idev->buffer;
> +	u32 status = at91_adc_reg_read(st, AT91_ADC_MR);
> +	u8 bit;
> +
> +	if (state) {
> +		int i;
> +
Looks suspect.  sizeof that is only 4 because it is a pointer, not because
it has 4 elements I think???
> +		for (i = 0; i < sizeof(st->desc->triggers); i++) {
> +			char *name = kasprintf(GFP_KERNEL,
> +					       "%s-dev%d-%s",
> +					       idev->name,
> +					       idev->id,
> +					       st->desc->triggers[i].name);
> +
Cleaner to verify the kasprintf worked first and return -ENOMEM if it
failed.  Then see if the name matches.  Also, !strcmp is a really
confusing idiom.  I at least prefer strcmp() == 0

> +			if (!strcmp(idev->trig->name, name)) {
> +				status |= st->desc->triggers[i].value;
> +				kfree(name);
> +				break;
> +			}
> +
> +			kfree(name);
> +		}
> +
> +		if (i == sizeof(st->desc->triggers))
> +			return -EINVAL;
> +
> +		at91_adc_reg_write(st, AT91_ADC_MR, status);
> +
> +		for_each_set_bit(bit, buffer->scan_mask,
> +				 st->desc->num_channels) {
> +			struct iio_chan_spec const *chan = idev->channels + bit;
> +			at91_adc_reg_write(st, AT91_ADC_CHER,
> +					   AT91_ADC_CH(chan->channel));
> +		}
> +
> +		at91_adc_reg_write(st, AT91_ADC_IER, AT91_ADC_DRDY);
> +
> +
> +	} else {
> +		at91_adc_reg_write(st, AT91_ADC_IDR, AT91_ADC_DRDY);
> +		at91_adc_reg_write(st, AT91_ADC_MR,
> +				   status & ~AT91_ADC_TRGEN);
> +
> +		for_each_set_bit(bit, buffer->scan_mask,
> +				 st->desc->num_channels) {
> +			at91_adc_reg_write(st, AT91_ADC_CHDR,
> +					   AT91_ADC_CH(bit));
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops at91_adc_trigger_ops = {
> +	.owner = THIS_MODULE,
> +	.set_trigger_state = &at91_adc_set_trigger_state,
> +};
> +
> +
Name is a little misleading as this also registers the trigger.
Perhaps at91_adc_configure_trigger?
> +static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *idev,
> +						     struct at91_adc_trigger *trigger)
> +{
> +	struct iio_trigger *trig = iio_allocate_trigger("%s-dev%d-%s",
> +							idev->name,
> +							idev->id,
> +							trigger->name);
> +	int ret;
> +
> +	if (trig == NULL)
> +		return NULL;
> +
> +	trig->dev.parent = idev->dev.parent;
> +	trig->owner = THIS_MODULE;
yikes. That's silly, we still have an owner field in trig and in ops?
Definitely one for cleaning up.
> +	trig->private_data = idev;
> +	trig->ops = &at91_adc_trigger_ops;
> +
> +	ret = iio_trigger_register(trig);
> +	if (ret < 0)
> +		return NULL;
> +
> +	return trig;
> +}
> +
> +static int at91_adc_trigger_init(struct iio_dev *idev,
> +				 struct at91_adc_data *pdata)
> +{
> +	struct at91_adc_state *st = iio_priv(idev);
> +	int i, ret;
> +
> +	st->trig = kcalloc(sizeof(st->desc->triggers),
> +			   sizeof(struct iio_trigger *),
> +			   GFP_KERNEL);
> +
extra white space...
> +	if (st->trig == NULL) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +
> +	for (i = 0; i < sizeof(st->desc->triggers); i++) {
> +		if (st->desc->triggers[i].is_external && !(pdata->use_external))
> +			continue;
> +
> +		st->trig[i] = at91_adc_allocate_trigger(idev,
> +							st->desc->triggers + i);
Check the right trigger ;)
> +		if (st->trig[0] == NULL) {
> +			ret = -ENOMEM;
> +			goto error_trigger;
> +		}
> +	}
> +
> +	return 0;
> +
> +error_trigger:
> +	for (; i >= 0; i--) {
> +		iio_trigger_unregister(st->trig[i]);
> +		iio_free_trigger(st->trig[i]);
> +	}
> +	kfree(st->trig);
> +error_ret:
> +	return ret;
> +}
> +
> +static void at91_adc_trigger_remove(struct iio_dev *idev)
> +{
> +	struct at91_adc_state *st = iio_priv(idev);
> +	int i;
> +
> +	for (i = 0; i < sizeof(st->trig); i++) {
> +		iio_trigger_unregister(st->trig[i]);
> +		iio_free_trigger(st->trig[i]);
> +	}
> +
> +	kfree(st->trig);
> +}
> +
>  static int at91_adc_read_raw(struct iio_dev *idev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -309,8 +583,10 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  
>  	/* Setup the ADC channels available on the board */
>  	ret = at91_adc_channel_init(idev, pdata);
> -	if (ret < 0)
Shouldn't be in this patch.
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Couldn't initialize the channels.\n");
>  		goto error_free_clk;
> +	}
>  
>  	init_waitqueue_head(&st->wq_data_avail);
>  	mutex_init(&st->lock);
> @@ -318,12 +594,40 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  	st->vref_mv = pdata->vref;
>  	st->channels_mask = pdata->channels_used;
>  
> -	ret = iio_device_register(idev);
> -	if (ret < 0)
> +	ret = at91_adc_buffer_init(idev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Couldn't initialize the buffer.\n");
>  		goto error_free_channels;
> +	}
> +
> +	ret = iio_buffer_register(idev,
> +				  idev->channels,
> +				  idev->num_channels);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Couldn't register the buffer.\n");
> +		goto error_free_buffer;
> +	}
> +
> +	ret = at91_adc_trigger_init(idev, pdata);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Couldn't setup the triggers.\n");
> +		goto error_unregister_buffer;
> +	}
> +

Also, not in this patch.
> +	ret = iio_device_register(idev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Couldn't register the device.\n");
> +		goto error_remove_triggers;
> +	}
>  
>  	return 0;
>  
> +error_remove_triggers:
> +	at91_adc_trigger_remove(idev);
> +error_unregister_buffer:
> +	iio_buffer_unregister(idev);
> +error_free_buffer:
> +	at91_adc_buffer_remove(idev);
>  error_free_channels:
>  	at91_adc_channel_remove(idev);
>  error_free_clk:
> @@ -348,6 +652,9 @@ static int __devexit at91_adc_remove(struct platform_device *pdev)
>  	struct at91_adc_state *st = iio_priv(idev);
>  
>  	iio_device_unregister(idev);
> +	at91_adc_trigger_remove(idev);
> +	iio_buffer_unregister(idev);
> +	at91_adc_buffer_remove(idev);
>  	at91_adc_channel_remove(idev);
>  	clk_disable(st->clk);
>  	clk_put(st->clk);
> diff --git a/include/linux/platform_data/at91_adc.h b/include/linux/platform_data/at91_adc.h
> index 9c5a64e..5b65eff 100644
> --- a/include/linux/platform_data/at91_adc.h
> +++ b/include/linux/platform_data/at91_adc.h
> @@ -18,6 +18,8 @@
Probably worth doing docs here in kernel-doc format.
>  struct at91_adc_data {
>  	/* Channels in use on the board as a bitmask */
>  	unsigned long channels_used;
> +	/* Do we need the external triggers ? */
> +	bool use_external;
>  	/* Reference voltage for the ADC in millivolts */
>  	u16 vref;
>  };

Re: [PATCH 5/5] AT91:IIO: Add support for hardware triggers for the ADC

[Maxime Ripard]

Hi Jonathan,

On 04/12/2011 19:08, Jonathan Cameron wrote:
> Mostly fine, though I'd structure a few corners differently as explained
> inline.
> 
> Also, I don't immediately see a reason not to use threaded interrupts
> for much
> of what we have here and that would definitely be preferable if possible!
> 
> Review may be more thorough that you wanted at this stage, but given I was
> reading the code I thought I might as well do it along the way!

Thanks :)

> On 12/02/2011 03:35 PM, Maxime Ripard wrote:
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>
>> Cc: Patrice Vilchez <patrice.vilchez@atmel.com>
>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>  arch/arm/mach-at91/at91sam9260_devices.c |    3 +
>>  arch/arm/mach-at91/board-sam9g20ek.c     |    1 +
>>  drivers/staging/iio/adc/Kconfig          |   11 +-
>>  drivers/staging/iio/adc/at91_adc.c       |  329 +++++++++++++++++++++++++++++-
>>  include/linux/platform_data/at91_adc.h   |    2 +
>>  5 files changed, 331 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/at91sam9260_devices.c b/arch/arm/mach-at91/at91sam9260_devices.c
>> index 95b3127..9dc9f96 100644
>> --- a/arch/arm/mach-at91/at91sam9260_devices.c
>> +++ b/arch/arm/mach-at91/at91sam9260_devices.c
>> @@ -1366,6 +1366,9 @@ void __init at91_add_device_adc(struct at91_adc_data *data)
>>  	if (test_bit(3, &data->channels_used))
>>  		at91_set_A_periph(AT91_PIN_PC3, 0);
>>  
>> +	if (data->use_external)
>> +		at91_set_A_periph(AT91_PIN_PA22, 0);
>> +
>>  	adc_data = *data;
>>  	platform_device_register(&at91_adc_device);
>>  }
>> diff --git a/arch/arm/mach-at91/board-sam9g20ek.c b/arch/arm/mach-at91/board-sam9g20ek.c
>> index 7758f34..f18f6f2 100644
>> --- a/arch/arm/mach-at91/board-sam9g20ek.c
>> +++ b/arch/arm/mach-at91/board-sam9g20ek.c
>> @@ -317,6 +317,7 @@ static void __init ek_add_device_buttons(void) {}
>>  
>>  static struct at91_adc_data ek_adc_data = {
>>  	.channels_used = BIT(0) | BIT(1) | BIT(2) | BIT(3),
>> +	.use_external = true,
>>  	.vref = 3300,
>>  };
>>  
>> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
>> index 93de64d..9d87c75 100644
>> --- a/drivers/staging/iio/adc/Kconfig
>> +++ b/drivers/staging/iio/adc/Kconfig
>> @@ -170,10 +170,13 @@ config AD7280
>>  	  module will be called ad7280a
>>  
>>  config AT91_ADC
>> -       tristate "Atmel AT91 ADC"
>> -       depends on SYSFS && ARCH_AT91
>> -       help
>> -         Say yes here to build support for Atmel AT91 ADC
>> +	tristate "Atmel AT91 ADC"
>> +	depends on SYSFS && ARCH_AT91
>> +	select IIO_BUFFER
>> +	select IIO_SW_RING
>> +	select IIO_TRIGGER
> I assume this is true because some of these changes mean
> that it will simply not function without the buffered side?
> Given that one typical usecase for this device is simple
> hwmon type monitoring (fully pull interface in kernel)
> I'd be inclined to see how much extra code would be
> required to allow it to run as before adding buffered support.
> If it is really uggly then fair enough!

I've tried to do that, and found it quite ugly, but I'll resubmit a new
version with this and see how you find it :)

>> +	help
>> +	  Say yes here to build support for Atmel AT91 ADC.
>>  
>>  config MAX1363
>>  	tristate "Maxim max1363 ADC driver"
>> diff --git a/drivers/staging/iio/adc/at91_adc.c b/drivers/staging/iio/adc/at91_adc.c
>> index b357363..74baaf3 100644
>> --- a/drivers/staging/iio/adc/at91_adc.c
>> +++ b/drivers/staging/iio/adc/at91_adc.c
>> @@ -23,9 +23,20 @@
>>  #include "../iio.h"
>>  #include <linux/platform_data/at91_adc.h>
>>  
>> +#include "../buffer.h"
>> +#include "../ring_sw.h"
>> +#include "../trigger.h"
>> +#include "../trigger_consumer.h"
>> +
>>  #include <mach/at91_adc.h>
>>  #include <mach/cpu.h>
>>  
> I'd like to see some docs for this.  Obviously
> name is self explanatory but value isn't!

You're right. The value here is the value that has to be written in the
mode register of the ADC. From one board to another, as triggers change,
so does the values to put in the register to enable this particular trigger.

>> +struct at91_adc_trigger {
>> +	char *name;
>> +	u8 value;
>> +	bool is_external;
>> +};
>> +
>>  struct at91_adc_desc {
>>  	/* ADC Clock as specified by the datasheet, in Hz. */
>>  	u32 clock;
>> @@ -37,6 +48,7 @@ struct at91_adc_desc {
>>  	u8 num_channels;
>>  	/* Startup time of the ADC, in microseconds. */
>>  	u8 startup_time;
>> +	struct at91_adc_trigger *triggers;
>>  };
>>  
>>  struct at91_adc_state {
>> @@ -51,12 +63,37 @@ struct at91_adc_state {
>>  	unsigned long channels_mask;
>>  	bool irq_enabled;
>>  	struct at91_adc_desc *desc;
>> +	struct iio_trigger **trig;
>> +};
>> +
> Trigger names are a little cryptic. I'd be inclined to make them
> a bit longer and hence perhaps self explanatory if you can.  Note
> as first device of this type you are pretty much setting the
> naming standard and I think these will appear in some of the user
> space abi, so they won't be easy to change later!
> For that reason we are going to be fussier than normal.

You're right to be. Indeed, the names aren't that explicit. Would
something like "timer-counter-0" be better ?

> 
>> +static struct at91_adc_trigger at91_adc_trigger_sam9g20[] = {
>> +	[0] = {
>> +		.name = "TC0",
>> +		.value = AT91_ADC_TRGSEL_TC0 | AT91_ADC_TRGEN,
>> +		.is_external = false,
>> +	},
>> +	[1] = {
>> +		.name = "TC1",
>> +		.value = AT91_ADC_TRGSEL_TC1 | AT91_ADC_TRGEN,
>> +		.is_external = false,
>> +	},
>> +	[2] = {
>> +		.name = "TC2",
>> +		.value = AT91_ADC_TRGSEL_TC2 | AT91_ADC_TRGEN,
>> +		.is_external = false,
>> +	},
>> +	[3] = {
>> +		.name = "external",
>> +		.value = AT91_ADC_TRGSEL_EXTERNAL | AT91_ADC_TRGEN,
>> +		.is_external = true,
>> +	},
> How does it know how many there are?  Null terminate this perhaps?
>>  };
>>  
>>  static struct at91_adc_desc at91_adc_desc_sam9g20 = {
>>  	.clock = 5000000,
>>  	.num_channels = 4,
>>  	.startup_time = 10,
>> +	.triggers = at91_adc_trigger_sam9g20,
>>  };
>>  
>>  static int at91_adc_select_soc(struct at91_adc_state *st)
>> @@ -82,27 +119,124 @@ static inline void at91_adc_reg_write(struct at91_adc_state *st,
>>  	writel_relaxed(val, st->reg_base + reg);
>>  }
>>  
>> +static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *idev = pf->indio_dev;
>> +	struct at91_adc_state *st = iio_priv(idev);
>> +	struct iio_buffer *buffer = idev->buffer;
>> +	int len = 0;
>> +
>> +	size_t datasize = buffer->access->get_bytes_per_datum(buffer);
>> +	u16 *data = kmalloc(datasize, GFP_KERNEL);
>> +	if (data == NULL)
>> +		return -ENOMEM;
> I know some of my drivers have done this, but I prefer the approach
> used by some of the ADI ones.  They allocate these buffers in the
> chip_state and hence get this kmalloc out of this fast patch.
> That should be fine as we should always be able to make sure it
> is the right size in the preenable function for the buffer.

Ok

>> +
>> +	/* Needed to ACK the DRDY interruption */
>> +	at91_adc_reg_read(st, AT91_ADC_LCDR);
> Here?  Conceptually I'd expect to see this right at the end of the interrupt
> handling.  I would imaging acking this means another one can arive.  To
> cleanly support capture from other devices on this trigger this would occur
> in the try_reenable callback.  That only gets called when the number of
> calls to iio_trigger_notify_done is equal to the number of devices waiting
> on the trigger (i.e. everyone is done).

Ok, so try_reenable should contain both the interrupt ACK and the
re-enabling of the irq ?

>> +
>> +	if (buffer->scan_count) {
>> +		int i, j;
>> +		for (i = 0, j = 0; i < buffer->scan_count; i++) {
> Just as a heads up, this patch will change shortly due to the
> introduction of multiple buffer support so please watch those
> patches as they go through and the driver changes we are making..
> 
> Also, this looks like a for_each_set_bit function would make this easier to
> read.
> 
>> +			j = find_next_bit(buffer->scan_mask,
>> +					  st->desc->num_channels,
>> +					  j);
>> +			data[i] = at91_adc_reg_read(st, AT91_ADC_CHR(j));
>> +			j++;
>> +			len += 2;
>> +		}
>> +	}
>> +
> The utility of ALIGN macro has been pointed out to me recently and makes
> it more apparent what is going on in cases like this.

Oh, didn't know about it either, I will take a look.

> Also, conceptually I'd expect to see the timestamp capture occuring earlier
> to make it nearer to the trigger point (when I guess the read is actually
> occuring - having been lazy and not actually loaded the datasheet ;)

Ok, so basically, in the pollfunc allocation, I would move
at91_adc_trigger_handler to top half, and put iio_pollfunc_store_time as
bottom half ?

>> +	if (buffer->scan_timestamp) {
>> +		s64 *timestamp = (s64 *)((u8 *)data + round_up(len,
>> +							       sizeof(s64)));
>> +		*timestamp = iio_get_time_ns();
>> +	}
>> +
>> +	buffer->access->store_to(buffer, (u8 *)data, pf->timestamp);
>> +
>> +	kfree(data);
>> +
>> +	iio_trigger_notify_done(idev->trig);
>> +	st->irq_enabled = true;
> 
>> +	enable_irq(st->irq);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static irqreturn_t at91_adc_eoc_trigger(int irq, void *private)
>>  {
>>  	struct iio_dev *idev = private;
>>  	struct at91_adc_state *st = iio_priv(idev);
>> -	unsigned int status = at91_adc_reg_read(st, AT91_ADC_SR);
>>  
>> -	if (!(status & AT91_ADC_DRDY))
>> -		return IRQ_HANDLED;
>> +	st->done = true;
> Want to wake this up here?  Not after the st->last_value bit below?
> Obviously probably doesn't actually matter is we are in an irq handler
> but it would make for more logical flow to my mind.  Data is read first
> and then we tell those waiting it is available...

Indeed.

>> +	wake_up_interruptible(&st->wq_data_avail);
>>  
>> -	if (status & st->channels_mask) {
>> -		st->done = true;
>> +	if (iio_buffer_enabled(idev)) {
> For ease of flow, can we not make this a one shot interrupt (using
> threaded irqs)?  I'm also given we are being notified data is ready
> the read doesn't actually need to happen in the top half. I'd prefer
> seeing just the check that it is this interrupt and the iio_trigger_poll
> happening in the top half.
> 
> The trigger_poll is just in case we need to fire a capturing pin on
> something else using this trigger alongside this device.

I'm not sure to understand here. I thought trigger_poll was actually to
trigger an IRQ for the bottom and top half ?

And so, you mean that this handler should be something like
{
	if (!(status & AT91_ADC_DRDY))
		return IRQ_HANDLED;

	if (iio_buffer_enabled(idev))
		iio_trigger_poll(idev->trig, ...);
	else
		st->last_value = ...

	return IRQ_HANDLED;
}

with the wake up being moved in the at91_adc_trigger_handler function,
or the other way around, move the read into this handler just before the
wake up ?

> 
>> +		disable_irq_nosync(irq);
>> +		st->irq_enabled = false;
>> +		iio_trigger_poll(idev->trig, iio_get_time_ns());
>> +	} else {
>>  		st->last_value = at91_adc_reg_read(st, AT91_ADC_LCDR);
>>  	}
>>  
>> -	wake_up_interruptible(&st->wq_data_avail);
>> -
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static const struct iio_buffer_setup_ops at91_adc_buffer_setup_ops = {
>> +	.preenable = &iio_sw_buffer_preenable,
>> +	.postenable = &iio_triggered_buffer_postenable,
>> +	.predisable = &iio_triggered_buffer_predisable,
>> +};
>> +
>> +static int at91_adc_buffer_init(struct iio_dev *idev)
>> +{
>> +	struct iio_buffer *buffer;
>> +	int ret;
>> +
>> +	buffer = iio_sw_rb_allocate(idev);
>> +	if (buffer == NULL) {
>> +		ret = -ENOMEM;
>> +		goto error_ret;
>> +	}
>> +
>> +	idev->buffer = buffer;
>> +	buffer->scan_timestamp = true;
> Killing of bpe is scheduled.  Nothing that can't be recovered from
> iio_chan_spec structures and shouldn't be used in capture path anyway.
> The new iio_sw_buffer_preenable is a lot cleverer!  Easy to change
> once those patches have merged of course!

Oh, cool :)
I have to admit, I didn't know why I had to set this up after having set
storagebits.

> 
>> +	buffer->bpe = 2;
>> +	buffer->access = &ring_sw_access_funcs;
>> +	buffer->setup_ops = &at91_adc_buffer_setup_ops;
>> +	buffer->owner = THIS_MODULE;
>> +
> Looking at what you have here, you might save a fair bit of code by
> taking advantage of Lars-Peter's patch of last week that handles all
> this boilerplate under slightly restricted circumstances...

I missed his patch, thanks !

>> +	idev->pollfunc = iio_alloc_pollfunc(NULL,
>> +					    &at91_adc_trigger_handler,
>> +					    0,
>> +					    idev,
>> +					    "at91_adc-consumer-%d",
>> +					    idev->id);
>> +
>> +	if (idev->pollfunc == NULL) {
>> +		ret = -ENOMEM;
>> +		goto error_free_buffer;
>> +	}
>> +
>> +	idev->modes |= INDIO_BUFFER_TRIGGERED;
>> +	return 0;
>> +
>> +error_free_buffer:
>> +	iio_sw_rb_free(idev->buffer);
>> +error_ret:
>> +	return ret;
>> +
>> +}
>> +
>> +static void at91_adc_buffer_remove(struct iio_dev *indio_dev)
>> +{
>> +	iio_dealloc_pollfunc(indio_dev->pollfunc);
>> +	iio_sw_rb_free(indio_dev->buffer);
>> +}
>> +
>>  static int at91_adc_channel_init(struct iio_dev *idev,
>> -				struct at91_adc_data *pdata)
>> +				 struct at91_adc_data *pdata)
> May be a valid whitespace cleanup, but shouldn't be in this patch.
>>  {
>>  	struct at91_adc_state *st = iio_priv(idev);
>>  	struct iio_chan_spec *chan_array;
>> @@ -121,6 +255,7 @@ static int at91_adc_channel_init(struct iio_dev *idev,
>>  		chan->type = IIO_VOLTAGE;
>>  		chan->indexed = 1;
>>  		chan->channel = bit;
>> +		chan->scan_index = idx;
>>  		chan->scan_type.sign = 'u';
>>  		chan->scan_type.realbits = 10;
>>  		chan->scan_type.storagebits = 16;
>> @@ -137,6 +272,145 @@ static void at91_adc_channel_remove(struct iio_dev *idev)
>>  	kfree(idev->channels);
>>  }
>>  
>> +static int at91_adc_set_trigger_state(struct iio_trigger *trig, bool state)
>> +{
>> +	struct iio_dev *idev = trig->private_data;
>> +	struct at91_adc_state *st = iio_priv(idev);
>> +	struct iio_buffer *buffer = idev->buffer;
>> +	u32 status = at91_adc_reg_read(st, AT91_ADC_MR);
>> +	u8 bit;
>> +
>> +	if (state) {
>> +		int i;
>> +
> Looks suspect.  sizeof that is only 4 because it is a pointer, not because
> it has 4 elements I think???

...
You're definitely right here.

>> +		for (i = 0; i < sizeof(st->desc->triggers); i++) {
>> +			char *name = kasprintf(GFP_KERNEL,
>> +					       "%s-dev%d-%s",
>> +					       idev->name,
>> +					       idev->id,
>> +					       st->desc->triggers[i].name);
>> +
> Cleaner to verify the kasprintf worked first and return -ENOMEM if it
> failed.  Then see if the name matches.  Also, !strcmp is a really
> confusing idiom.  I at least prefer strcmp() == 0

Ok

>> +			if (!strcmp(idev->trig->name, name)) {
>> +				status |= st->desc->triggers[i].value;
>> +				kfree(name);
>> +				break;
>> +			}
>> +
>> +			kfree(name);
>> +		}
>> +
>> +		if (i == sizeof(st->desc->triggers))
>> +			return -EINVAL;
>> +
>> +		at91_adc_reg_write(st, AT91_ADC_MR, status);
>> +
>> +		for_each_set_bit(bit, buffer->scan_mask,
>> +				 st->desc->num_channels) {
>> +			struct iio_chan_spec const *chan = idev->channels + bit;
>> +			at91_adc_reg_write(st, AT91_ADC_CHER,
>> +					   AT91_ADC_CH(chan->channel));
>> +		}
>> +
>> +		at91_adc_reg_write(st, AT91_ADC_IER, AT91_ADC_DRDY);
>> +
>> +
>> +	} else {
>> +		at91_adc_reg_write(st, AT91_ADC_IDR, AT91_ADC_DRDY);
>> +		at91_adc_reg_write(st, AT91_ADC_MR,
>> +				   status & ~AT91_ADC_TRGEN);
>> +
>> +		for_each_set_bit(bit, buffer->scan_mask,
>> +				 st->desc->num_channels) {
>> +			at91_adc_reg_write(st, AT91_ADC_CHDR,
>> +					   AT91_ADC_CH(bit));
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct iio_trigger_ops at91_adc_trigger_ops = {
>> +	.owner = THIS_MODULE,
>> +	.set_trigger_state = &at91_adc_set_trigger_state,
>> +};
>> +
>> +
> Name is a little misleading as this also registers the trigger.
> Perhaps at91_adc_configure_trigger?
>> +static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *idev,
>> +						     struct at91_adc_trigger *trigger)
>> +{
>> +	struct iio_trigger *trig = iio_allocate_trigger("%s-dev%d-%s",
>> +							idev->name,
>> +							idev->id,
>> +							trigger->name);
>> +	int ret;
>> +
>> +	if (trig == NULL)
>> +		return NULL;
>> +
>> +	trig->dev.parent = idev->dev.parent;
>> +	trig->owner = THIS_MODULE;
> yikes. That's silly, we still have an owner field in trig and in ops?
> Definitely one for cleaning up.
>> +	trig->private_data = idev;
>> +	trig->ops = &at91_adc_trigger_ops;
>> +
>> +	ret = iio_trigger_register(trig);
>> +	if (ret < 0)
>> +		return NULL;
>> +
>> +	return trig;
>> +}
>> +
>> +static int at91_adc_trigger_init(struct iio_dev *idev,
>> +				 struct at91_adc_data *pdata)
>> +{
>> +	struct at91_adc_state *st = iio_priv(idev);
>> +	int i, ret;
>> +
>> +	st->trig = kcalloc(sizeof(st->desc->triggers),
>> +			   sizeof(struct iio_trigger *),
>> +			   GFP_KERNEL);
>> +
> extra white space...

Hmm, I don't see where it is.

>> +	if (st->trig == NULL) {
>> +		ret = -ENOMEM;
>> +		goto error_ret;
>> +	}
>> +
>> +	for (i = 0; i < sizeof(st->desc->triggers); i++) {
>> +		if (st->desc->triggers[i].is_external && !(pdata->use_external))
>> +			continue;
>> +
>> +		st->trig[i] = at91_adc_allocate_trigger(idev,
>> +							st->desc->triggers + i);
> Check the right trigger ;)

Haha, good catch
Sorry about that, an artifact from the good ol' hardcoded time :)

>> +		if (st->trig[0] == NULL) {
>> +			ret = -ENOMEM;
>> +			goto error_trigger;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +error_trigger:
>> +	for (; i >= 0; i--) {
>> +		iio_trigger_unregister(st->trig[i]);
>> +		iio_free_trigger(st->trig[i]);
>> +	}
>> +	kfree(st->trig);
>> +error_ret:
>> +	return ret;
>> +}
>> +
>> +static void at91_adc_trigger_remove(struct iio_dev *idev)
>> +{
>> +	struct at91_adc_state *st = iio_priv(idev);
>> +	int i;
>> +
>> +	for (i = 0; i < sizeof(st->trig); i++) {
>> +		iio_trigger_unregister(st->trig[i]);
>> +		iio_free_trigger(st->trig[i]);
>> +	}
>> +
>> +	kfree(st->trig);
>> +}
>> +
>>  static int at91_adc_read_raw(struct iio_dev *idev,
>>  			    struct iio_chan_spec const *chan,
>>  			    int *val, int *val2, long mask)
>> @@ -309,8 +583,10 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>>  
>>  	/* Setup the ADC channels available on the board */
>>  	ret = at91_adc_channel_init(idev, pdata);
>> -	if (ret < 0)
> Shouldn't be in this patch.
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Couldn't initialize the channels.\n");
>>  		goto error_free_clk;
>> +	}
>>  
>>  	init_waitqueue_head(&st->wq_data_avail);
>>  	mutex_init(&st->lock);
>> @@ -318,12 +594,40 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>>  	st->vref_mv = pdata->vref;
>>  	st->channels_mask = pdata->channels_used;
>>  
>> -	ret = iio_device_register(idev);
>> -	if (ret < 0)
>> +	ret = at91_adc_buffer_init(idev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Couldn't initialize the buffer.\n");
>>  		goto error_free_channels;
>> +	}
>> +
>> +	ret = iio_buffer_register(idev,
>> +				  idev->channels,
>> +				  idev->num_channels);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Couldn't register the buffer.\n");
>> +		goto error_free_buffer;
>> +	}
>> +
>> +	ret = at91_adc_trigger_init(idev, pdata);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Couldn't setup the triggers.\n");
>> +		goto error_unregister_buffer;
>> +	}
>> +
> 
> Also, not in this patch.
>> +	ret = iio_device_register(idev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "Couldn't register the device.\n");
>> +		goto error_remove_triggers;
>> +	}
>>  
>>  	return 0;
>>  
>> +error_remove_triggers:
>> +	at91_adc_trigger_remove(idev);
>> +error_unregister_buffer:
>> +	iio_buffer_unregister(idev);
>> +error_free_buffer:
>> +	at91_adc_buffer_remove(idev);
>>  error_free_channels:
>>  	at91_adc_channel_remove(idev);
>>  error_free_clk:
>> @@ -348,6 +652,9 @@ static int __devexit at91_adc_remove(struct platform_device *pdev)
>>  	struct at91_adc_state *st = iio_priv(idev);
>>  
>>  	iio_device_unregister(idev);
>> +	at91_adc_trigger_remove(idev);
>> +	iio_buffer_unregister(idev);
>> +	at91_adc_buffer_remove(idev);
>>  	at91_adc_channel_remove(idev);
>>  	clk_disable(st->clk);
>>  	clk_put(st->clk);
>> diff --git a/include/linux/platform_data/at91_adc.h b/include/linux/platform_data/at91_adc.h
>> index 9c5a64e..5b65eff 100644
>> --- a/include/linux/platform_data/at91_adc.h
>> +++ b/include/linux/platform_data/at91_adc.h
>> @@ -18,6 +18,8 @@
> Probably worth doing docs here in kernel-doc format.
>>  struct at91_adc_data {
>>  	/* Channels in use on the board as a bitmask */
>>  	unsigned long channels_used;
>> +	/* Do we need the external triggers ? */
>> +	bool use_external;
>>  	/* Reference voltage for the ADC in millivolts */
>>  	u16 vref;
>>  };
> 


-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com