[PATCH] IIO: Add basic MXS LRADC driver

[PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

This driver is very basic. It supports userland trigger, buffer and
raw access to channels. The support for delay channels is missing
altogether.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Juergen Beisert <jbe@pengutronix.de>
---
 drivers/staging/iio/adc/Kconfig     |   12 +
 drivers/staging/iio/adc/Makefile    |    1 +
 drivers/staging/iio/adc/mxs-lradc.c |  619 +++++++++++++++++++++++++++++++++++
 3 files changed, 632 insertions(+)
 create mode 100644 drivers/staging/iio/adc/mxs-lradc.c

Jonathan, you can now rip me to shreds for not understanding your teachings.
Please kindly review the products of my labor ;-)

diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index 67711b7..97ca697 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -200,6 +200,18 @@ config LPC32XX_ADC
 	  activate only one via device tree selection.  Provides direct access
 	  via sysfs.
 
+config MXS_LRADC
+	tristate "Freescale i.MX23/i.MX28 LRADC"
+	depends on ARCH_MXS
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for i.MX23/i.MX28 LRADC convertor
+	  built into these chips.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called mxs-lradc.
+
 config SPEAR_ADC
 	tristate "ST SPEAr ADC"
 	depends on PLAT_SPEAR
diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
index 14e98b6..ecac9a0 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -38,4 +38,5 @@ obj-$(CONFIG_ADT7310) += adt7310.o
 obj-$(CONFIG_ADT7410) += adt7410.o
 obj-$(CONFIG_AD7280) += ad7280a.o
 obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
+obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
new file mode 100644
index 0000000..d4089c6
--- /dev/null
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -0,0 +1,619 @@
+/*
+ * Freescale i.MX233/i.MX28 LRADC driver
+ *
+ * Copyright (c) 2012 DENX Software Engineering, GmbH.
+ * Marek Vasut <marex@denx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/sysfs.h>
+#include <linux/list.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/wait.h>
+#include <linux/sched.h>
+#include <linux/stmp_device.h>
+#include <linux/bitops.h>
+
+#include <mach/mxs.h>
+#include <mach/common.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define DRIVER_NAME		"mxs-lradc"
+
+#define LRADC_MAX_DELAY_CHANS	4
+#define LRADC_MAX_MAPPED_CHANS	8
+#define LRADC_MAX_TOTAL_CHANS	16
+
+#define LRADC_DELAY_TIMER_HZ	2000
+
+#define LRADC_CHAN_FLAG_RAW	(1 << 0)
+#define LRADC_CHAN_FLAG_BUF	(1 << 1)
+
+static const char * const mxs_lradc_irq_name[] = {
+	"mxs-lradc-touchscreen",
+	"mxs-lradc-thresh0",
+	"mxs-lradc-thresh1",
+	"mxs-lradc-channel0",
+	"mxs-lradc-channel1",
+	"mxs-lradc-channel2",
+	"mxs-lradc-channel3",
+	"mxs-lradc-channel4",
+	"mxs-lradc-channel5",
+	"mxs-lradc-channel6",
+	"mxs-lradc-channel7",
+	"mxs-lradc-button0",
+	"mxs-lradc-button1",
+};
+
+struct mxs_lradc_chan {
+	uint8_t				slot;
+	uint8_t				flags;
+};
+
+struct mxs_lradc {
+	struct iio_dev		*iio;
+	struct device		*dev;
+	void __iomem		*base;
+	int			irq[13];
+	int			idis;
+
+	uint32_t		*buffer;
+	struct iio_trigger	*trig;
+
+	struct mutex		map_lock;
+	struct mxs_lradc_chan	channel[LRADC_MAX_TOTAL_CHANS];
+	unsigned long		slot_map;
+
+	wait_queue_head_t	wq_data_avail[LRADC_MAX_MAPPED_CHANS];
+	bool			wq_done[LRADC_MAX_MAPPED_CHANS];
+};
+
+struct mxs_lradc_data {
+	struct mxs_lradc	*lradc;
+};
+
+#define	LRADC_CTRL0				0x00
+#define LRADC_CTRL0_TOUCH_DETECT_ENABLE		(1 << 23)
+#define LRADC_CTRL0_TOUCH_SCREEN_TYPE		(1 << 22)
+
+#define	LRADC_CTRL1				0x10
+#define	LRADC_CTRL1_LRADC_IRQ(n)		(1 << (n))
+#define	LRADC_CTRL1_LRADC_IRQ_MASK		0x1fff
+#define	LRADC_CTRL1_LRADC_IRQ_EN(n)		(1 << ((n) + 16))
+#define	LRADC_CTRL1_LRADC_IRQ_EN_MASK		(0x1fff << 16)
+
+#define	LRADC_CTRL2				0x20
+#define	LRADC_CTRL2_TEMPSENSE_PWD		(1 << 15)
+
+#define	LRADC_CH(n)				(0x50 + (0x10 * (n)))
+#define	LRADC_CH_ACCUMULATE			(1 << 29)
+#define	LRADC_CH_NUM_SAMPLES_MASK		(0x1f << 24)
+#define	LRADC_CH_NUM_SAMPLES_OFFSET		24
+#define	LRADC_CH_VALUE_MASK			0x3ffff
+#define	LRADC_CH_VALUE_OFFSET			0
+
+#define	LRADC_DELAY(n)				(0xd0 + (0x10 * (n)))
+#define	LRADC_DELAY_TRIGGER_LRADCS_MASK		(0xff << 24)
+#define	LRADC_DELAY_TRIGGER_LRADCS_OFFSET	24
+#define	LRADC_DELAY_KICK			(1 << 20)
+#define	LRADC_DELAY_TRIGGER_DELAYS_MASK		(0xf << 16)
+#define	LRADC_DELAY_TRIGGER_DELAYS_OFFSET	16
+#define	LRADC_DELAY_LOOP_COUNT_MASK		(0x1f << 11)
+#define	LRADC_DELAY_LOOP_COUNT_OFFSET		11
+#define	LRADC_DELAY_DELAY_MASK			0x7ff
+#define	LRADC_DELAY_DELAY_OFFSET		0
+
+#define	LRADC_CTRL4				0x140
+#define	LRADC_CTRL4_LRADCSELECT_MASK(n)		(0xf << ((n) * 4))
+#define	LRADC_CTRL4_LRADCSELECT_OFFSET(n)	((n) * 4)
+
+/*
+ * Channel management
+ *
+ * This involves crazy mapping between 8 virtual channels the CTRL4 register
+ * can harbor and 16 channels total this hardware supports.
+ */
+static int mxs_lradc_map_channel(struct mxs_lradc *lradc,
+				uint8_t channel, uint8_t flags)
+{
+	int ret;
+
+	/* Invalid channel */
+	if (channel > LRADC_MAX_TOTAL_CHANS)
+		return -EINVAL;
+
+	mutex_lock(&lradc->map_lock);
+
+	/* Channel is already mapped */
+	if (lradc->channel[channel].flags) {
+
+		/* Channel is mapped to requested delay slot */
+		lradc->channel[channel].flags |= flags;
+		ret = lradc->channel[channel].slot;
+		goto exit;
+
+	} else {	/* Channel isn't mapped yet */
+		/* 8 channels already mapped, can't map any more */
+		if (bitmap_full(&lradc->slot_map, LRADC_MAX_MAPPED_CHANS)) {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		ret = find_first_zero_bit(&lradc->slot_map,
+					LRADC_MAX_MAPPED_CHANS);
+
+		lradc->channel[channel].slot = ret;
+		lradc->channel[channel].flags |= flags;
+		lradc->slot_map |= 1 << ret;
+
+		writel(LRADC_CTRL4_LRADCSELECT_MASK(ret),
+			lradc->base + LRADC_CTRL4 + STMP_OFFSET_REG_CLR);
+		writel(channel << LRADC_CTRL4_LRADCSELECT_OFFSET(ret),
+			lradc->base + LRADC_CTRL4 + STMP_OFFSET_REG_SET);
+
+		writel(LRADC_CTRL1_LRADC_IRQ_EN(ret),
+			lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_SET);
+
+		writel(0, lradc->base + LRADC_CH(ret));
+	}
+
+exit:
+	mutex_unlock(&lradc->map_lock);
+
+	/* Return the slot the channel was mapped to */
+	return ret;
+}
+
+static int mxs_lradc_unmap_channel(struct mxs_lradc *lradc,
+				uint8_t channel, uint8_t flags)
+{
+	int slot;
+
+	/* Invalid channel */
+	if (channel > LRADC_MAX_TOTAL_CHANS)
+		return -EINVAL;
+
+	mutex_lock(&lradc->map_lock);
+
+	/* Channel not mapped */
+	if (!lradc->channel[channel].flags) {
+		mutex_unlock(&lradc->map_lock);
+		return -EINVAL;
+	}
+
+	lradc->channel[channel].flags &= ~flags;
+	slot = lradc->channel[channel].slot;
+
+	/* No more users of this channel, unmap it */
+	if (!lradc->channel[channel].flags) {
+		lradc->slot_map &= ~(1 << slot);
+
+		writel(LRADC_CTRL1_LRADC_IRQ_EN(slot),
+			lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR);
+	}
+
+	mutex_unlock(&lradc->map_lock);
+
+	return slot;
+}
+
+static void mxs_lradc_run_slots(struct mxs_lradc *lradc, uint8_t slots)
+{
+	writel(slots, lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_SET);
+}
+
+/*
+ * Raw I/O operations
+ */
+static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan,
+			int *val, int *val2, long m)
+{
+	struct mxs_lradc_data *data = iio_priv(iio_dev);
+	struct mxs_lradc *lradc = data->lradc;
+	int ret, slot;
+
+	if (m != 0)
+		return -EINVAL;
+
+	/* Map channel. */
+	slot = mxs_lradc_map_channel(lradc, chan->channel, LRADC_CHAN_FLAG_RAW);
+	if (slot < 0)
+		return slot;
+
+	/* Start sampling. */
+	mxs_lradc_run_slots(lradc, 1 << slot);
+
+	/* Wait for completion on the channel, 1 second max. */
+	lradc->wq_done[slot] = false;
+	ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot],
+					       lradc->wq_done[slot],
+					       msecs_to_jiffies(1000));
+	if (!ret) {
+		ret = -ETIMEDOUT;
+		goto err;
+	}
+
+	/* Read the data. */
+	*val = readl(lradc->base + LRADC_CH(slot)) & LRADC_CH_VALUE_MASK;
+	ret = IIO_VAL_INT;
+
+err:
+	/* Unmap the channel. */
+	mxs_lradc_unmap_channel(lradc, chan->channel, LRADC_CHAN_FLAG_RAW);
+
+	return ret;
+}
+
+static const struct iio_info mxs_lradc_iio_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= mxs_lradc_read_raw,
+};
+
+/*
+ * IRQ Handling
+ */
+static irqreturn_t mxs_lradc_handle_irq(int irq, void *data)
+{
+	struct mxs_lradc *lradc = data;
+	int bit;
+	unsigned long reg = readl(lradc->base + LRADC_CTRL1);
+
+	/*
+	 * Touchscreen IRQ handling code shall probably have priority
+	 * and therefore shall be placed here.
+	 */
+
+	/* Wake up each LRADC channel. */
+	for_each_set_bit(bit, &reg, 8) {
+		lradc->wq_done[bit] = true;
+		wake_up_interruptible(&lradc->wq_data_avail[bit]);
+	}
+
+	if (iio_buffer_enabled(lradc->iio)) {
+		disable_irq_nosync(irq);
+		lradc->idis = irq;
+		iio_trigger_poll(lradc->iio->trig, iio_get_time_ns());
+	}
+
+	writel(0x1fff, lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * Trigger handling
+ */
+static irqreturn_t mxs_lradc_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *iio = pf->indio_dev;
+	struct mxs_lradc_data *data = iio_priv(iio);
+	struct mxs_lradc *lradc = data->lradc;
+	struct iio_buffer *buffer = iio->buffer;
+	int i, j = 0, slot;
+
+	for (i = 0; i < iio->masklength; i++) {
+		if (!test_bit(i, iio->active_scan_mask))
+			continue;
+
+		slot = lradc->channel[i].slot;
+
+		lradc->buffer[j] = readl(lradc->base + LRADC_CH(slot));
+		lradc->buffer[j] &= LRADC_CH_VALUE_MASK;
+
+		j++;
+	}
+
+	if (iio->scan_timestamp) {
+		s64 *timestamp = (s64 *)((u8 *)lradc->buffer +
+					ALIGN(j, sizeof(s64)));
+		*timestamp = pf->timestamp;
+	}
+
+	iio_push_to_buffer(buffer, (u8 *)lradc->buffer, pf->timestamp);
+
+	iio_trigger_notify_done(iio->trig);
+
+	enable_irq(lradc->idis);
+
+	return IRQ_HANDLED;
+}
+
+static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *iio = trig->private_data;
+	struct mxs_lradc_data *data = iio_priv(iio);
+	struct mxs_lradc *lradc = data->lradc;
+	struct iio_buffer *buffer = iio->buffer;
+	int slot, bit, ret = 0;
+	uint8_t map = 0;
+	unsigned long chans = 0;
+
+	if (!state)
+		goto exit;
+
+	lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL);
+	if (!lradc->buffer)
+		return -ENOMEM;
+
+	for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) {
+		slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF);
+
+		if (slot < 0) {
+			ret = -EINVAL;
+			goto exit;
+		}
+		map |= 1 << slot;
+		chans |= 1 << bit;
+	}
+
+	mxs_lradc_run_slots(lradc, map);
+
+	return 0;
+
+exit:
+	for_each_set_bit(bit, &chans, LRADC_MAX_TOTAL_CHANS)
+		mxs_lradc_unmap_channel(lradc, bit, LRADC_CHAN_FLAG_BUF);
+
+	kfree(lradc->buffer);
+
+	return ret;
+}
+
+static const struct iio_trigger_ops mxs_lradc_trigger_ops = {
+	.owner = THIS_MODULE,
+	.set_trigger_state = &mxs_lradc_configure_trigger,
+};
+
+static int mxs_lradc_trigger_init(struct iio_dev *iio)
+{
+	int ret;
+	struct iio_trigger *trig;
+
+	trig = iio_trigger_alloc("%s-dev%i", iio->name, iio->id);
+	if (trig == NULL)
+		return -ENOMEM;
+
+	trig->dev.parent = iio->dev.parent;
+	trig->private_data = iio;
+	trig->ops = &mxs_lradc_trigger_ops;
+
+	ret = iio_trigger_register(trig);
+	if (ret) {
+		iio_trigger_free(trig);
+		return ret;
+	}
+
+	iio->trig = trig;
+
+	return 0;
+}
+
+static void mxs_lradc_trigger_remove(struct iio_dev *iio)
+{
+	iio_trigger_unregister(iio->trig);
+	iio_trigger_free(iio->trig);
+}
+
+/*
+ * Buffer ops
+ */
+static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
+	.preenable = &iio_sw_buffer_preenable,
+	.postenable = &iio_triggered_buffer_postenable,
+	.predisable = &iio_triggered_buffer_predisable,
+};
+
+/*
+ *****************************************************************************
+ * DRIVER INIT STUFF
+ *****************************************************************************
+ */
+
+#define MXS_ADC_CHAN(idx, chan_type) {				\
+	.type = (chan_type),					\
+	.indexed = 1,						\
+	.scan_index = (idx),					\
+	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT,		\
+	.channel = (idx),					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = 18,					\
+		.storagebits = 32,				\
+	},							\
+}
+
+static const struct iio_chan_spec mxs_lradc_chan_spec[] = {
+	MXS_ADC_CHAN(0, IIO_VOLTAGE),
+	MXS_ADC_CHAN(1, IIO_VOLTAGE),
+	MXS_ADC_CHAN(2, IIO_VOLTAGE),
+	MXS_ADC_CHAN(3, IIO_VOLTAGE),
+	MXS_ADC_CHAN(4, IIO_VOLTAGE),
+	MXS_ADC_CHAN(5, IIO_VOLTAGE),
+	MXS_ADC_CHAN(6, IIO_VOLTAGE),
+	MXS_ADC_CHAN(7, IIO_VOLTAGE),	/* VBATT */
+	MXS_ADC_CHAN(8, IIO_TEMP),	/* Temp sense 0 */
+	MXS_ADC_CHAN(9, IIO_TEMP),	/* Temp sense 1 */
+	MXS_ADC_CHAN(10, IIO_VOLTAGE),	/* VDDIO */
+	MXS_ADC_CHAN(11, IIO_VOLTAGE),	/* VTH */
+	MXS_ADC_CHAN(12, IIO_VOLTAGE),	/* VDDA */
+	MXS_ADC_CHAN(13, IIO_VOLTAGE),	/* VDDD */
+	MXS_ADC_CHAN(14, IIO_VOLTAGE),	/* VBG */
+	MXS_ADC_CHAN(15, IIO_VOLTAGE),	/* VDD5V */
+};
+
+static void mxs_lradc_hw_init(struct mxs_lradc *lradc)
+{
+	int i;
+
+	stmp_reset_block(lradc->base);
+
+	for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
+		writel(0, lradc->base + LRADC_DELAY(i));
+
+	/* Start internal temperature sensing. */
+	writel(0, lradc->base + LRADC_CTRL2);
+}
+
+static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
+{
+	writel(LRADC_CTRL1_LRADC_IRQ_EN_MASK,
+		lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR);
+}
+
+static int __devinit mxs_lradc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mxs_lradc_data *data;
+	struct mxs_lradc *lradc;
+	struct iio_dev *iio;
+	struct resource *iores;
+	int ret = 0;
+	int i;
+
+	lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL);
+	if (!lradc)
+		return -ENOMEM;
+
+	/* Grab the memory area */
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	lradc->base = devm_request_and_ioremap(dev, iores);
+	if (!lradc->base)
+		return -EADDRNOTAVAIL;
+
+	/* Grab all IRQ sources */
+	for (i = 0; i < 13; i++) {
+		lradc->irq[i] = platform_get_irq(pdev, i);
+		if (lradc->irq[i] < 0)
+			return -EINVAL;
+
+		ret = devm_request_irq(dev, lradc->irq[i],
+					mxs_lradc_handle_irq, 0,
+					mxs_lradc_irq_name[i], lradc);
+		if (ret)
+			return ret;
+	}
+
+	/* Init workqueues, one per channel */
+	for (i = 0; i < LRADC_MAX_MAPPED_CHANS; i++)
+		init_waitqueue_head(&lradc->wq_data_avail[i]);
+
+	dev_set_drvdata(&pdev->dev, lradc);
+
+	mutex_init(&lradc->map_lock);
+
+	/* Allocate the IIO device. */
+	iio = iio_device_alloc(sizeof(*data));
+	if (!iio) {
+		dev_err(dev, "Failed to allocate IIO device %i\n", i);
+		return -ENOMEM;
+	}
+
+	lradc->iio = iio;
+
+	iio->name = pdev->name;
+	iio->dev.parent = &pdev->dev;
+	iio->info = &mxs_lradc_iio_info;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->channels = mxs_lradc_chan_spec;
+	iio->num_channels = ARRAY_SIZE(mxs_lradc_chan_spec);
+
+	/*
+	 * This here is intentional. Once we'll support the delay channels this
+	 * hardware provides, we will need to store more data in IIO devices'
+	 * private data.
+	 */
+	data = iio_priv(iio);
+	data->lradc = lradc;
+
+	ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
+				&mxs_lradc_trigger_handler, NULL);
+	if (ret)
+		goto err0;
+
+	ret = mxs_lradc_trigger_init(iio);
+	if (ret)
+		goto err1;
+
+	/* Configure the hardware. */
+	mxs_lradc_hw_init(lradc);
+
+	/*
+	 * Register IIO device
+	 */
+	ret = iio_device_register(iio);
+	if (ret) {
+		dev_err(dev, "Failed to register IIO device\n");
+		goto err2;
+	}
+
+	return 0;
+
+err2:
+	mxs_lradc_trigger_remove(iio);
+err1:
+	iio_triggered_buffer_cleanup(lradc->iio);
+err0:
+	iio_device_free(iio);
+	return ret;
+}
+
+static int __devexit mxs_lradc_remove(struct platform_device *pdev)
+{
+	struct mxs_lradc *lradc = dev_get_drvdata(&pdev->dev);
+
+	mxs_lradc_hw_stop(lradc);
+
+	iio_device_unregister(lradc->iio);
+	iio_triggered_buffer_cleanup(lradc->iio);
+	mxs_lradc_trigger_remove(lradc->iio);
+	iio_device_free(lradc->iio);
+
+	return 0;
+}
+
+static const struct of_device_id mxs_lradc_dt_ids[] = {
+	{ .compatible = "fsl,imx28-lradc", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
+
+static struct platform_driver mxs_lradc_driver = {
+	.driver	= {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = mxs_lradc_dt_ids,
+	},
+	.probe	= mxs_lradc_probe,
+	.remove	= __devexit_p(mxs_lradc_remove),
+};
+
+module_platform_driver(mxs_lradc_driver);
+
+MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
+MODULE_DESCRIPTION("Freescale i.MX23/i.MX28 LRADC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.10

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Wolfgang Denk]

Dear Marek,

In message <1341368129-20468-1-git-send-email-marex@denx.de> you wrote:
> This driver is very basic. It supports userland trigger, buffer and
> raw access to channels. The support for delay channels is missing
> altogether.
...

> +#define	LRADC_CH_NUM_SAMPLES_MASK		(0x1f << 24)
> +#define	LRADC_CH_NUM_SAMPLES_OFFSET		24

	#define LRADC_CH_NUM_SAMPLES_OFFSET	24
	#define	LRADC_CH_NUM_SAMPLES_MASK	(0x1f << LRADC_CH_NUM_SAMPLES_OFFSET)

> +#define	LRADC_DELAY_TRIGGER_LRADCS_MASK		(0xff << 24)
> +#define	LRADC_DELAY_TRIGGER_LRADCS_OFFSET	24

Ditto.

> +#define	LRADC_DELAY_TRIGGER_DELAYS_MASK		(0xf << 16)
> +#define	LRADC_DELAY_TRIGGER_DELAYS_OFFSET	16
> +#define	LRADC_DELAY_LOOP_COUNT_MASK		(0x1f << 11)
> +#define	LRADC_DELAY_LOOP_COUNT_OFFSET		11

Ditto.

Please use the symbolic names you define.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
The speed of time is one second per second.

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Lars-Peter Clausen]

On 07/04/2012 04:15 AM, Marek Vasut wrote:
> This driver is very basic. It supports userland trigger, buffer and
> raw access to channels. The support for delay channels is missing
> altogether.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Juergen Beisert <jbe@pengutronix.de>
> [...]

The overall structure looks good, but a few things need to be addressed.
I also think the driver doesn't have to go through staging and could be put
in to the out-of-staging part of iio.


> +struct mxs_lradc {
> +	struct iio_dev		*iio;
> +	struct device		*dev;
> +	void __iomem		*base;
> +	int			irq[13];
> +	int			idis;
> +
> +	uint32_t		*buffer;
> +	struct iio_trigger	*trig;
> +
> +	struct mutex		map_lock;
> +	struct mxs_lradc_chan	channel[LRADC_MAX_TOTAL_CHANS];
> +	unsigned long		slot_map;
> +
> +	wait_queue_head_t	wq_data_avail[LRADC_MAX_MAPPED_CHANS];
> +	bool			wq_done[LRADC_MAX_MAPPED_CHANS];

This and it's usage looks a bit like an open-coded struct completion.

> +};
> +
> +struct mxs_lradc_data {
> +	struct mxs_lradc	*lradc;
> +};

Is there any reason not to use the mxs_lradc struct directly has the iio data?

>[...]
> +
> +/*
> + * Channel management
> + *
> + * This involves crazy mapping between 8 virtual channels the CTRL4 register
> + * can harbor and 16 channels total this hardware supports.
> + */

I suppose this means only a maximum 8 channels can be active at a time. I've
recently posted a patch which makes it easy to implement such a restriction.
http://www.spinics.net/lists/linux-iio/msg05936.html and
http://www.spinics.net/lists/linux-iio/msg05935.html for an example validate
callback implementation. In your case you'd check for bitmap_weight(...) <=
8. Those patches are not yet in IIO though.

> +static int mxs_lradc_map_channel(struct mxs_lradc *lradc,
> +				uint8_t channel, uint8_t flags)
> +{
> +	int ret;
> +
> +	/* Invalid channel */
> +	if (channel > LRADC_MAX_TOTAL_CHANS)
> +		return -EINVAL;
> +
> +	mutex_lock(&lradc->map_lock);
> +
> +	/* Channel is already mapped */
> +	if (lradc->channel[channel].flags) {
> +
> +		/* Channel is mapped to requested delay slot */
> +		lradc->channel[channel].flags |= flags;
> +		ret = lradc->channel[channel].slot;
> +		goto exit;
> +
> +	} else {	/* Channel isn't mapped yet */
> +		/* 8 channels already mapped, can't map any more */
> +		if (bitmap_full(&lradc->slot_map, LRADC_MAX_MAPPED_CHANS)) {
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +
> +		ret = find_first_zero_bit(&lradc->slot_map,
> +					LRADC_MAX_MAPPED_CHANS);
> +
> +		lradc->channel[channel].slot = ret;
> +		lradc->channel[channel].flags |= flags;
> +		lradc->slot_map |= 1 << ret;
> +
> +		writel(LRADC_CTRL4_LRADCSELECT_MASK(ret),
> +			lradc->base + LRADC_CTRL4 + STMP_OFFSET_REG_CLR);
> +		writel(channel << LRADC_CTRL4_LRADCSELECT_OFFSET(ret),
> +			lradc->base + LRADC_CTRL4 + STMP_OFFSET_REG_SET);
> +
> +		writel(LRADC_CTRL1_LRADC_IRQ_EN(ret),
> +			lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_SET);
> +
> +		writel(0, lradc->base + LRADC_CH(ret));
> +	}
> +
> +exit:
> +	mutex_unlock(&lradc->map_lock);
> +
> +	/* Return the slot the channel was mapped to */
> +	return ret;
> +}
> +
[...]
> +/*
> + * Raw I/O operations
> + */
> +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
> +			const struct iio_chan_spec *chan,
> +			int *val, int *val2, long m)
> +{
> +	struct mxs_lradc_data *data = iio_priv(iio_dev);
> +	struct mxs_lradc *lradc = data->lradc;
> +	int ret, slot;
> +
> +	if (m != 0)

I'd use the symbolic constant here IIO_CHAN_INFO_RAW

> +		return -EINVAL;
> +
> +	/* Map channel. */
> +	slot = mxs_lradc_map_channel(lradc, chan->channel, LRADC_CHAN_FLAG_RAW);
> +	if (slot < 0)
> +		return slot;
> +
> +	/* Start sampling. */
> +	mxs_lradc_run_slots(lradc, 1 << slot);
> +
> +	/* Wait for completion on the channel, 1 second max. */
> +	lradc->wq_done[slot] = false;
> +	ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot],
> +					       lradc->wq_done[slot],
> +					       msecs_to_jiffies(1000));
> +	if (!ret) {
> +		ret = -ETIMEDOUT;
> +		goto err;
> +	}
> +

How well will this work if the device is currrently in buffered mode? Maybe
it's better do disablow it by checking iio_buffer_enabled and return -EBUSY
if it returns true;

> +	/* Read the data. */
> +	*val = readl(lradc->base + LRADC_CH(slot)) & LRADC_CH_VALUE_MASK;
> +	ret = IIO_VAL_INT;
> +
> +err:
> +	/* Unmap the channel. */
> +	mxs_lradc_unmap_channel(lradc, chan->channel, LRADC_CHAN_FLAG_RAW);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info mxs_lradc_iio_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= mxs_lradc_read_raw,
> +};
> +
> +/*
> + * IRQ Handling
> + */
> +static irqreturn_t mxs_lradc_handle_irq(int irq, void *data)
> +{
> +	struct mxs_lradc *lradc = data;
> +	int bit;
> +	unsigned long reg = readl(lradc->base + LRADC_CTRL1);
> +
> +	/*
> +	 * Touchscreen IRQ handling code shall probably have priority
> +	 * and therefore shall be placed here.
> +	 */
> +
> +	/* Wake up each LRADC channel. */
> +	for_each_set_bit(bit, &reg, 8) {
> +		lradc->wq_done[bit] = true;
> +		wake_up_interruptible(&lradc->wq_data_avail[bit]);
> +	}
> +
> +	if (iio_buffer_enabled(lradc->iio)) {
> +		disable_irq_nosync(irq);
> +		lradc->idis = irq;

The whole disable_irq/enable_irq thing is a bit ugly, is it really necessary?

> +		iio_trigger_poll(lradc->iio->trig, iio_get_time_ns());
> +	}
> +
> +	writel(0x1fff, lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * Trigger handling
> + */
> +static irqreturn_t mxs_lradc_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct mxs_lradc_data *data = iio_priv(iio);
> +	struct mxs_lradc *lradc = data->lradc;
> +	struct iio_buffer *buffer = iio->buffer;
> +	int i, j = 0, slot;
> +
> +	for (i = 0; i < iio->masklength; i++) {
> +		if (!test_bit(i, iio->active_scan_mask))
> +			continue;

for_each_set_bit

> +
> +		slot = lradc->channel[i].slot;
> +
> +		lradc->buffer[j] = readl(lradc->base + LRADC_CH(slot));
> +		lradc->buffer[j] &= LRADC_CH_VALUE_MASK;
> +
> +		j++;
> +	}
> +
> +	if (iio->scan_timestamp) {
> +		s64 *timestamp = (s64 *)((u8 *)lradc->buffer +
> +					ALIGN(j, sizeof(s64)));
> +		*timestamp = pf->timestamp;
> +	}
> +
> +	iio_push_to_buffer(buffer, (u8 *)lradc->buffer, pf->timestamp);
> +
> +	iio_trigger_notify_done(iio->trig);
> +
> +	enable_irq(lradc->idis);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *iio = trig->private_data;
> +	struct mxs_lradc_data *data = iio_priv(iio);
> +	struct mxs_lradc *lradc = data->lradc;
> +	struct iio_buffer *buffer = iio->buffer;
> +	int slot, bit, ret = 0;
> +	uint8_t map = 0;
> +	unsigned long chans = 0;
> +
> +	if (!state)
> +		goto exit;
> +
> +	lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL);
> +	if (!lradc->buffer)
> +		return -ENOMEM;
> +
> +	for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) {
> +		slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF);
> +
> +		if (slot < 0) {
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +		map |= 1 << slot;
> +		chans |= 1 << bit;
> +	}

I think this should go into the buffer preenable and postdisable callbacks.

> +
> +	mxs_lradc_run_slots(lradc, map);
> +
> +	return 0;
> +
> +exit:
> +	for_each_set_bit(bit, &chans, LRADC_MAX_TOTAL_CHANS)
> +		mxs_lradc_unmap_channel(lradc, bit, LRADC_CHAN_FLAG_BUF);
> +
> +	kfree(lradc->buffer);
> +
> +	return ret;
> +}
> +
> [...]
> +/*
> + * Buffer ops
> + */
> +static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
> +	.preenable = &iio_sw_buffer_preenable,
> +	.postenable = &iio_triggered_buffer_postenable,
> +	.predisable = &iio_triggered_buffer_predisable,
> +};

This is unused.

> +
> [...]
> +static int __devinit mxs_lradc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mxs_lradc_data *data;
> +	struct mxs_lradc *lradc;
> +	struct iio_dev *iio;
> +	struct resource *iores;
> +	int ret = 0;
> +	int i;
> +
> +	lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL);
> +	if (!lradc)
> +		return -ENOMEM;
> +
> +	/* Grab the memory area */
> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	lradc->base = devm_request_and_ioremap(dev, iores);
> +	if (!lradc->base)
> +		return -EADDRNOTAVAIL;

I think this is a network error code, NXIO would be more approriate.

[...]
> +	return ret;
> +}
> +
> [...]

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Lars-Peter Clausen,

> On 07/04/2012 04:15 AM, Marek Vasut wrote:
> > This driver is very basic. It supports userland trigger, buffer and
> > raw access to channels. The support for delay channels is missing
> > altogether.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Juergen Beisert <jbe@pengutronix.de>
> > [...]
> 
> The overall structure looks good, but a few things need to be addressed.
> I also think the driver doesn't have to go through staging and could be put
> in to the out-of-staging part of iio.

Come and rip me to shreds ;-)

[...]

> > +	wait_queue_head_t	wq_data_avail[LRADC_MAX_MAPPED_CHANS];
> > +	bool			wq_done[LRADC_MAX_MAPPED_CHANS];
> 
> This and it's usage looks a bit like an open-coded struct completion.

Indeed, completion is good for this :)

> > +};
> > +
> > +struct mxs_lradc_data {
> > +	struct mxs_lradc	*lradc;
> > +};
> 
> Is there any reason not to use the mxs_lradc struct directly has the iio
> data?

I explained it below in the patch. I hope we'll eventually support the delay 
triggers, which will need 4 separate IIO devices. And this is where this 
structure will be augmented by other components.

> >[...]
> >
> > +
> > +/*
> > + * Channel management
> > + *
> > + * This involves crazy mapping between 8 virtual channels the CTRL4
> > register + * can harbor and 16 channels total this hardware supports.
> > + */
> 
> I suppose this means only a maximum 8 channels can be active at a time.
> I've recently posted a patch which makes it easy to implement such a
> restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and
> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
> validate callback implementation. In your case you'd check for
> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.

This is good. When do you expect this to hit linux-next possibly? Or at least 
linux-iio?

[..]

> > +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
> > +			const struct iio_chan_spec *chan,
> > +			int *val, int *val2, long m)
> > +{
> > +	struct mxs_lradc_data *data = iio_priv(iio_dev);
> > +	struct mxs_lradc *lradc = data->lradc;
> > +	int ret, slot;
> > +
> > +	if (m != 0)
> 
> I'd use the symbolic constant here IIO_CHAN_INFO_RAW
> 
> > +		return -EINVAL;
> > +
> > +	/* Map channel. */
> > +	slot = mxs_lradc_map_channel(lradc, chan->channel,
> > LRADC_CHAN_FLAG_RAW); +	if (slot < 0)
> > +		return slot;
> > +
> > +	/* Start sampling. */
> > +	mxs_lradc_run_slots(lradc, 1 << slot);
> > +
> > +	/* Wait for completion on the channel, 1 second max. */
> > +	lradc->wq_done[slot] = false;
> > +	ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot],
> > +					       lradc->wq_done[slot],
> > +					       msecs_to_jiffies(1000));
> > +	if (!ret) {
> > +		ret = -ETIMEDOUT;
> > +		goto err;
> > +	}
> > +
> 
> How well will this work if the device is currrently in buffered mode? Maybe
> it's better do disablow it by checking iio_buffer_enabled and return -EBUSY
> if it returns true;

I believe this should work perfectly OK, why won't it ? I tried to avoid such 
artificial limitation.

[...]

> > +	if (iio_buffer_enabled(lradc->iio)) {
> > +		disable_irq_nosync(irq);
> > +		lradc->idis = irq;
> 
> The whole disable_irq/enable_irq thing is a bit ugly, is it really
> necessary?

I was wondering myself, I'll investigate further. This was inspired by the AT91 
driver for the most part.

[...]

> > +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool
> > state) +{
> > +	struct iio_dev *iio = trig->private_data;
> > +	struct mxs_lradc_data *data = iio_priv(iio);
> > +	struct mxs_lradc *lradc = data->lradc;
> > +	struct iio_buffer *buffer = iio->buffer;
> > +	int slot, bit, ret = 0;
> > +	uint8_t map = 0;
> > +	unsigned long chans = 0;
> > +
> > +	if (!state)
> > +		goto exit;
> > +
> > +	lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL);
> > +	if (!lradc->buffer)
> > +		return -ENOMEM;
> > +
> > +	for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) {
> > +		slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF);
> > +
> > +		if (slot < 0) {
> > +			ret = -EINVAL;
> > +			goto exit;
> > +		}
> > +		map |= 1 << slot;
> > +		chans |= 1 << bit;
> > +	}
> 
> I think this should go into the buffer preenable and postdisable callbacks.

How much of it, only the slot mapping or the allocation too ?

> > +
> > +	mxs_lradc_run_slots(lradc, map);
> > +
> > +	return 0;
> > +
> > +exit:
> > +	for_each_set_bit(bit, &chans, LRADC_MAX_TOTAL_CHANS)
> > +		mxs_lradc_unmap_channel(lradc, bit, LRADC_CHAN_FLAG_BUF);
> > +
> > +	kfree(lradc->buffer);
> > +
> > +	return ret;
> > +}
> > +
> > [...]
> > +/*
> > + * Buffer ops
> > + */
> > +static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
> > +	.preenable = &iio_sw_buffer_preenable,
> > +	.postenable = &iio_triggered_buffer_postenable,
> > +	.predisable = &iio_triggered_buffer_predisable,
> > +};
> 
> This is unused.

Bah, you're right. Shame on me :-(

> > [...]
> > +static int __devinit mxs_lradc_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct mxs_lradc_data *data;
> > +	struct mxs_lradc *lradc;
> > +	struct iio_dev *iio;
> > +	struct resource *iores;
> > +	int ret = 0;
> > +	int i;
> > +
> > +	lradc = devm_kzalloc(&pdev->dev, sizeof(*lradc), GFP_KERNEL);
> > +	if (!lradc)
> > +		return -ENOMEM;
> > +
> > +	/* Grab the memory area */
> > +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	lradc->base = devm_request_and_ioremap(dev, iores);
> > +	if (!lradc->base)
> > +		return -EADDRNOTAVAIL;
> 
> I think this is a network error code, NXIO would be more approriate.

Will fix.

Thanks for the review!

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Lars-Peter Clausen]

On 07/05/2012 01:48 AM, Marek Vasut wrote:
> Dear Lars-Peter Clausen,
> 
>> On 07/04/2012 04:15 AM, Marek Vasut wrote:
>>> This driver is very basic. It supports userland trigger, buffer and
>>> raw access to channels. The support for delay channels is missing
>>> altogether.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Jonathan Cameron <jic23@kernel.org>
>>> Cc: Wolfgang Denk <wd@denx.de>
>>> Cc: Juergen Beisert <jbe@pengutronix.de>
>>> [...]
>>
>> The overall structure looks good, but a few things need to be addressed.
>> I also think the driver doesn't have to go through staging and could be put
>> in to the out-of-staging part of iio.
> 
> Come and rip me to shreds ;-)
> 
> [...]
> 
>>> +	wait_queue_head_t	wq_data_avail[LRADC_MAX_MAPPED_CHANS];
>>> +	bool			wq_done[LRADC_MAX_MAPPED_CHANS];
>>
>> This and it's usage looks a bit like an open-coded struct completion.
> 
> Indeed, completion is good for this :)
> 
>>> +};
>>> +
>>> +struct mxs_lradc_data {
>>> +	struct mxs_lradc	*lradc;
>>> +};
>>
>> Is there any reason not to use the mxs_lradc struct directly has the iio
>> data?
> 
> I explained it below in the patch. I hope we'll eventually support the delay 
> triggers, which will need 4 separate IIO devices. And this is where this 
> structure will be augmented by other components.

Ok I saw the comment, but it wasn't obvious to me that delay channels will
require multiple IIO devices. Might make sense to state this explicitly.

> 
>>> [...]
>>>
>>> +
>>> +/*
>>> + * Channel management
>>> + *
>>> + * This involves crazy mapping between 8 virtual channels the CTRL4
>>> register + * can harbor and 16 channels total this hardware supports.
>>> + */
>>
>> I suppose this means only a maximum 8 channels can be active at a time.
>> I've recently posted a patch which makes it easy to implement such a
>> restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and
>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
>> validate callback implementation. In your case you'd check for
>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.
> 
> This is good. When do you expect this to hit linux-next possibly? Or at least 
> linux-iio?
> 

soon, hopefully.

> [..]
> 
>>> +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
>>> +			const struct iio_chan_spec *chan,
>>> +			int *val, int *val2, long m)
>>> +{
>>> +	struct mxs_lradc_data *data = iio_priv(iio_dev);
>>> +	struct mxs_lradc *lradc = data->lradc;
>>> +	int ret, slot;
>>> +
>>> +	if (m != 0)
>>
>> I'd use the symbolic constant here IIO_CHAN_INFO_RAW
>>
>>> +		return -EINVAL;
>>> +
>>> +	/* Map channel. */
>>> +	slot = mxs_lradc_map_channel(lradc, chan->channel,
>>> LRADC_CHAN_FLAG_RAW); +	if (slot < 0)
>>> +		return slot;
>>> +
>>> +	/* Start sampling. */
>>> +	mxs_lradc_run_slots(lradc, 1 << slot);
>>> +
>>> +	/* Wait for completion on the channel, 1 second max. */
>>> +	lradc->wq_done[slot] = false;
>>> +	ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot],
>>> +					       lradc->wq_done[slot],
>>> +					       msecs_to_jiffies(1000));
>>> +	if (!ret) {

Just noticed this, wait_event_interruptible_timeout can return an error
value, e.g. if it has been interrupted.

>>> +		ret = -ETIMEDOUT;
>>> +		goto err;
>>> +	}
>>> +
>>
>> How well will this work if the device is currrently in buffered mode? Maybe
>> it's better do disablow it by checking iio_buffer_enabled and return -EBUSY
>> if it returns true;
> 
> I believe this should work perfectly OK, why won't it ? I tried to avoid such 
> artificial limitation.
> 

I suppose it is fine, but not knowing the hardware, what does
mxs_lradc_run_slots do exactly?

> [...]
> 
>>> +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool
>>> state) +{
>>> +	struct iio_dev *iio = trig->private_data;
>>> +	struct mxs_lradc_data *data = iio_priv(iio);
>>> +	struct mxs_lradc *lradc = data->lradc;
>>> +	struct iio_buffer *buffer = iio->buffer;
>>> +	int slot, bit, ret = 0;
>>> +	uint8_t map = 0;
>>> +	unsigned long chans = 0;
>>> +
>>> +	if (!state)
>>> +		goto exit;
>>> +
>>> +	lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL);
>>> +	if (!lradc->buffer)
>>> +		return -ENOMEM;
>>> +
>>> +	for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) {
>>> +		slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF);
>>> +
>>> +		if (slot < 0) {
>>> +			ret = -EINVAL;
>>> +			goto exit;
>>> +		}
>>> +		map |= 1 << slot;
>>> +		chans |= 1 << bit;
>>> +	}
>>
>> I think this should go into the buffer preenable and postdisable callbacks.
> 
> How much of it, only the slot mapping or the allocation too ?
> 

Yeah I guess it is a bit tricky in this case. A good rule of thumb is to ask
yourself whether the driver will (hypothetically) still work if another
trigger is used. So the buffer allocation should certainly be handled by the
buffer code, either the prenable or the update_scan_mode callback. The
channel mapping part is not so obvious, but I'd put it in the
preenable/postdisable callbacks as well.

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Lars-Peter Clausen,

> On 07/05/2012 01:48 AM, Marek Vasut wrote:
> > Dear Lars-Peter Clausen,
> > 
> >> On 07/04/2012 04:15 AM, Marek Vasut wrote:
> >>> This driver is very basic. It supports userland trigger, buffer and
> >>> raw access to channels. The support for delay channels is missing
> >>> altogether.
> >>> 
> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> Cc: Jonathan Cameron <jic23@kernel.org>
> >>> Cc: Wolfgang Denk <wd@denx.de>
> >>> Cc: Juergen Beisert <jbe@pengutronix.de>
> >>> [...]
> >> 
> >> The overall structure looks good, but a few things need to be addressed.
> >> I also think the driver doesn't have to go through staging and could be
> >> put in to the out-of-staging part of iio.
> > 
> > Come and rip me to shreds ;-)
> > 
> > [...]
> > 
> >>> +	wait_queue_head_t	wq_data_avail[LRADC_MAX_MAPPED_CHANS];
> >>> +	bool			wq_done[LRADC_MAX_MAPPED_CHANS];
> >> 
> >> This and it's usage looks a bit like an open-coded struct completion.
> > 
> > Indeed, completion is good for this :)
> > 
> >>> +};
> >>> +
> >>> +struct mxs_lradc_data {
> >>> +	struct mxs_lradc	*lradc;
> >>> +};
> >> 
> >> Is there any reason not to use the mxs_lradc struct directly has the iio
> >> data?
> > 
> > I explained it below in the patch. I hope we'll eventually support the
> > delay triggers, which will need 4 separate IIO devices. And this is
> > where this structure will be augmented by other components.
> 
> Ok I saw the comment, but it wasn't obvious to me that delay channels will
> require multiple IIO devices. Might make sense to state this explicitly.

Certainly. We had a discussion with jonathan about that, it's not completely 
settled. Maybe I'll just do it your way. afterall.

> >>> [...]
> >>> 
> >>> +
> >>> +/*
> >>> + * Channel management
> >>> + *
> >>> + * This involves crazy mapping between 8 virtual channels the CTRL4
> >>> register + * can harbor and 16 channels total this hardware supports.
> >>> + */
> >> 
> >> I suppose this means only a maximum 8 channels can be active at a time.
> >> I've recently posted a patch which makes it easy to implement such a
> >> restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and
> >> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
> >> validate callback implementation. In your case you'd check for
> >> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.
> > 
> > This is good. When do you expect this to hit linux-next possibly? Or at
> > least linux-iio?
> 
> soon, hopefully.
> 
> > [..]
> > 
> >>> +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
> >>> +			const struct iio_chan_spec *chan,
> >>> +			int *val, int *val2, long m)
> >>> +{
> >>> +	struct mxs_lradc_data *data = iio_priv(iio_dev);
> >>> +	struct mxs_lradc *lradc = data->lradc;
> >>> +	int ret, slot;
> >>> +
> >>> +	if (m != 0)
> >> 
> >> I'd use the symbolic constant here IIO_CHAN_INFO_RAW
> >> 
> >>> +		return -EINVAL;
> >>> +
> >>> +	/* Map channel. */
> >>> +	slot = mxs_lradc_map_channel(lradc, chan->channel,
> >>> LRADC_CHAN_FLAG_RAW); +	if (slot < 0)
> >>> +		return slot;
> >>> +
> >>> +	/* Start sampling. */
> >>> +	mxs_lradc_run_slots(lradc, 1 << slot);
> >>> +
> >>> +	/* Wait for completion on the channel, 1 second max. */
> >>> +	lradc->wq_done[slot] = false;
> >>> +	ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot],
> >>> +					       lradc->wq_done[slot],
> >>> +					       msecs_to_jiffies(1000));
> >>> +	if (!ret) {
> 
> Just noticed this, wait_event_interruptible_timeout can return an error
> value, e.g. if it has been interrupted.

I replaced this with killable completion.

> >>> +		ret = -ETIMEDOUT;
> >>> +		goto err;
> >>> +	}
> >>> +
> >> 
> >> How well will this work if the device is currrently in buffered mode?
> >> Maybe it's better do disablow it by checking iio_buffer_enabled and
> >> return -EBUSY if it returns true;
> > 
> > I believe this should work perfectly OK, why won't it ? I tried to avoid
> > such artificial limitation.
> 
> I suppose it is fine, but not knowing the hardware, what does
> mxs_lradc_run_slots do exactly?

Starts the sampling of the channels that are specific in the mask passed to this 
function. But they must be mapped first.

> > [...]
> > 
> >>> +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool
> >>> state) +{
> >>> +	struct iio_dev *iio = trig->private_data;
> >>> +	struct mxs_lradc_data *data = iio_priv(iio);
> >>> +	struct mxs_lradc *lradc = data->lradc;
> >>> +	struct iio_buffer *buffer = iio->buffer;
> >>> +	int slot, bit, ret = 0;
> >>> +	uint8_t map = 0;
> >>> +	unsigned long chans = 0;
> >>> +
> >>> +	if (!state)
> >>> +		goto exit;
> >>> +
> >>> +	lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL);
> >>> +	if (!lradc->buffer)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) {
> >>> +		slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF);
> >>> +
> >>> +		if (slot < 0) {
> >>> +			ret = -EINVAL;
> >>> +			goto exit;
> >>> +		}
> >>> +		map |= 1 << slot;
> >>> +		chans |= 1 << bit;
> >>> +	}
> >> 
> >> I think this should go into the buffer preenable and postdisable
> >> callbacks.
> > 
> > How much of it, only the slot mapping or the allocation too ?
> 
> Yeah I guess it is a bit tricky in this case. A good rule of thumb is to
> ask yourself whether the driver will (hypothetically) still work if
> another trigger is used. So the buffer allocation should certainly be
> handled by the buffer code, either the prenable or the update_scan_mode
> callback. The channel mapping part is not so obvious, but I'd put it in
> the
> preenable/postdisable callbacks as well.

All right, I'll look into this in a bit :)

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Lars-Peter Clausen,

Sorry for my late reply, got busy with other crazy stuff.

[...]

> >> Is there any reason not to use the mxs_lradc struct directly has the iio
> >> data?
> > 
> > I explained it below in the patch. I hope we'll eventually support the
> > delay triggers, which will need 4 separate IIO devices. And this is
> > where this structure will be augmented by other components.
> 
> Ok I saw the comment, but it wasn't obvious to me that delay channels will
> require multiple IIO devices. Might make sense to state this explicitly.

Fixed.

> >>> [...]
> >>> 
> >>> +
> >>> +/*
> >>> + * Channel management
> >>> + *
> >>> + * This involves crazy mapping between 8 virtual channels the CTRL4
> >>> register + * can harbor and 16 channels total this hardware supports.
> >>> + */
> >> 
> >> I suppose this means only a maximum 8 channels can be active at a time.
> >> I've recently posted a patch which makes it easy to implement such a
> >> restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and
> >> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
> >> validate callback implementation. In your case you'd check for
> >> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.
> > 
> > This is good. When do you expect this to hit linux-next possibly? Or at
> > least linux-iio?
> 
> soon, hopefully.

So I checked this, not sure how it'll help me though.

> > [..]
> > 
> >>> +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
> >>> +			const struct iio_chan_spec *chan,
> >>> +			int *val, int *val2, long m)
> >>> +{
> >>> +	struct mxs_lradc_data *data = iio_priv(iio_dev);
> >>> +	struct mxs_lradc *lradc = data->lradc;
> >>> +	int ret, slot;
> >>> +
> >>> +	if (m != 0)
> >> 
> >> I'd use the symbolic constant here IIO_CHAN_INFO_RAW
> >> 
> >>> +		return -EINVAL;
> >>> +
> >>> +	/* Map channel. */
> >>> +	slot = mxs_lradc_map_channel(lradc, chan->channel,
> >>> LRADC_CHAN_FLAG_RAW); +	if (slot < 0)
> >>> +		return slot;
> >>> +
> >>> +	/* Start sampling. */
> >>> +	mxs_lradc_run_slots(lradc, 1 << slot);
> >>> +
> >>> +	/* Wait for completion on the channel, 1 second max. */
> >>> +	lradc->wq_done[slot] = false;
> >>> +	ret = wait_event_interruptible_timeout(lradc->wq_data_avail[slot],
> >>> +					       lradc->wq_done[slot],
> >>> +					       msecs_to_jiffies(1000));
> >>> +	if (!ret) {
> 
> Just noticed this, wait_event_interruptible_timeout can return an error
> value, e.g. if it has been interrupted.

Fixed

> >>> +		ret = -ETIMEDOUT;
> >>> +		goto err;
> >>> +	}
> >>> +
> >> 
> >> How well will this work if the device is currrently in buffered mode?
> >> Maybe it's better do disablow it by checking iio_buffer_enabled and
> >> return -EBUSY if it returns true;
> > 
> > I believe this should work perfectly OK, why won't it ? I tried to avoid
> > such artificial limitation.
> 
> I suppose it is fine, but not knowing the hardware, what does
> mxs_lradc_run_slots do exactly?

Execute the sampling on the mapped slots.

> > [...]
> > 
> >>> +static int mxs_lradc_configure_trigger(struct iio_trigger *trig, bool
> >>> state) +{
> >>> +	struct iio_dev *iio = trig->private_data;
> >>> +	struct mxs_lradc_data *data = iio_priv(iio);
> >>> +	struct mxs_lradc *lradc = data->lradc;
> >>> +	struct iio_buffer *buffer = iio->buffer;
> >>> +	int slot, bit, ret = 0;
> >>> +	uint8_t map = 0;
> >>> +	unsigned long chans = 0;
> >>> +
> >>> +	if (!state)
> >>> +		goto exit;
> >>> +
> >>> +	lradc->buffer = kmalloc(iio->scan_bytes, GFP_KERNEL);
> >>> +	if (!lradc->buffer)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	for_each_set_bit(bit, buffer->scan_mask, LRADC_MAX_TOTAL_CHANS) {
> >>> +		slot = mxs_lradc_map_channel(lradc, bit, LRADC_CHAN_FLAG_BUF);
> >>> +
> >>> +		if (slot < 0) {
> >>> +			ret = -EINVAL;
> >>> +			goto exit;
> >>> +		}
> >>> +		map |= 1 << slot;
> >>> +		chans |= 1 << bit;
> >>> +	}
> >> 
> >> I think this should go into the buffer preenable and postdisable
> >> callbacks.
> > 
> > How much of it, only the slot mapping or the allocation too ?
> 
> Yeah I guess it is a bit tricky in this case. A good rule of thumb is to
> ask yourself whether the driver will (hypothetically) still work if
> another trigger is used. So the buffer allocation should certainly be
> handled by the buffer code, either the prenable or the update_scan_mode
> callback. The channel mapping part is not so obvious, but I'd put it in
> the
> preenable/postdisable callbacks as well.

Lemme try.

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Lars-Peter Clausen]

On 07/19/2012 04:23 PM, Marek Vasut wrote:
> Dear Lars-Peter Clausen,
> 
> Sorry for my late reply, got busy with other crazy stuff.
> 
> [...]
> 
>>>> Is there any reason not to use the mxs_lradc struct directly has the iio
>>>> data?
>>>
>>> I explained it below in the patch. I hope we'll eventually support the
>>> delay triggers, which will need 4 separate IIO devices. And this is
>>> where this structure will be augmented by other components.
>>
>> Ok I saw the comment, but it wasn't obvious to me that delay channels will
>> require multiple IIO devices. Might make sense to state this explicitly.
> 
> Fixed.
> 
>>>>> [...]
>>>>>
>>>>> +
>>>>> +/*
>>>>> + * Channel management
>>>>> + *
>>>>> + * This involves crazy mapping between 8 virtual channels the CTRL4
>>>>> register + * can harbor and 16 channels total this hardware supports.
>>>>> + */
>>>>
>>>> I suppose this means only a maximum 8 channels can be active at a time.
>>>> I've recently posted a patch which makes it easy to implement such a
>>>> restriction. http://www.spinics.net/lists/linux-iio/msg05936.html and
>>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
>>>> validate callback implementation. In your case you'd check for
>>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.
>>>
>>> This is good. When do you expect this to hit linux-next possibly? Or at
>>> least linux-iio?
>>
>> soon, hopefully.
> 
> So I checked this, not sure how it'll help me though.

Right now with your driver you can enable any combination of channels. If
more than 8 channels are enabled and you start sampling it will fail, since
not all channels can be mapped. With those patch you can implement a
validate callback and check for bitmap_weight(scan_mask) <= 8. This will
ensure that it is not possible to select more than 8 channels at once, which
again means that starting sampling can't fail of this.

- Lars

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Lars-Peter Clausen,

> On 07/19/2012 04:23 PM, Marek Vasut wrote:
> > Dear Lars-Peter Clausen,
> > 
> > Sorry for my late reply, got busy with other crazy stuff.
> > 
> > [...]
> > 
> >>>> Is there any reason not to use the mxs_lradc struct directly has the
> >>>> iio data?
> >>> 
> >>> I explained it below in the patch. I hope we'll eventually support the
> >>> delay triggers, which will need 4 separate IIO devices. And this is
> >>> where this structure will be augmented by other components.
> >> 
> >> Ok I saw the comment, but it wasn't obvious to me that delay channels
> >> will require multiple IIO devices. Might make sense to state this
> >> explicitly.
> > 
> > Fixed.
> > 
> >>>>> [...]
> >>>>> 
> >>>>> +
> >>>>> +/*
> >>>>> + * Channel management
> >>>>> + *
> >>>>> + * This involves crazy mapping between 8 virtual channels the CTRL4
> >>>>> register + * can harbor and 16 channels total this hardware supports.
> >>>>> + */
> >>>> 
> >>>> I suppose this means only a maximum 8 channels can be active at a
> >>>> time. I've recently posted a patch which makes it easy to implement
> >>>> such a restriction.
> >>>> http://www.spinics.net/lists/linux-iio/msg05936.html and
> >>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
> >>>> validate callback implementation. In your case you'd check for
> >>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.
> >>> 
> >>> This is good. When do you expect this to hit linux-next possibly? Or at
> >>> least linux-iio?
> >> 
> >> soon, hopefully.
> > 
> > So I checked this, not sure how it'll help me though.
> 
> Right now with your driver you can enable any combination of channels. If
> more than 8 channels are enabled and you start sampling it will fail, since
> not all channels can be mapped. With those patch you can implement a
> validate callback and check for bitmap_weight(scan_mask) <= 8. This will
> ensure that it is not possible to select more than 8 channels at once,
> which again means that starting sampling can't fail of this.

Thanks, got it !

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Lars-Peter Clausen,

> On 07/19/2012 04:23 PM, Marek Vasut wrote:
> > Dear Lars-Peter Clausen,
> > 
> > Sorry for my late reply, got busy with other crazy stuff.
> > 
> > [...]
> > 
> >>>> Is there any reason not to use the mxs_lradc struct directly has the
> >>>> iio data?
> >>> 
> >>> I explained it below in the patch. I hope we'll eventually support the
> >>> delay triggers, which will need 4 separate IIO devices. And this is
> >>> where this structure will be augmented by other components.
> >> 
> >> Ok I saw the comment, but it wasn't obvious to me that delay channels
> >> will require multiple IIO devices. Might make sense to state this
> >> explicitly.
> > 
> > Fixed.
> > 
> >>>>> [...]
> >>>>> 
> >>>>> +
> >>>>> +/*
> >>>>> + * Channel management
> >>>>> + *
> >>>>> + * This involves crazy mapping between 8 virtual channels the CTRL4
> >>>>> register + * can harbor and 16 channels total this hardware supports.
> >>>>> + */
> >>>> 
> >>>> I suppose this means only a maximum 8 channels can be active at a
> >>>> time. I've recently posted a patch which makes it easy to implement
> >>>> such a restriction.
> >>>> http://www.spinics.net/lists/linux-iio/msg05936.html and
> >>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
> >>>> validate callback implementation. In your case you'd check for
> >>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.
> >>> 
> >>> This is good. When do you expect this to hit linux-next possibly? Or at
> >>> least linux-iio?
> >> 
> >> soon, hopefully.
> > 
> > So I checked this, not sure how it'll help me though.
> 
> Right now with your driver you can enable any combination of channels. If
> more than 8 channels are enabled and you start sampling it will fail, since
> not all channels can be mapped. With those patch you can implement a
> validate callback and check for bitmap_weight(scan_mask) <= 8. This will
> ensure that it is not possible to select more than 8 channels at once,
> which again means that starting sampling can't fail of this.

Ok, I fixed this one. One last thing that I don't really understand is this. I 
run generic_buffer.c to source samples from the LRADC. Is there any way to 
continuously sample them? Because right now, I get one sample and that's it, no 
more samples happen later on (I can 0 data in subsequent read() call).

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Lars-Peter Clausen,

> > On 07/19/2012 04:23 PM, Marek Vasut wrote:
> > > Dear Lars-Peter Clausen,
> > > 
> > > Sorry for my late reply, got busy with other crazy stuff.
> > > 
> > > [...]
> > > 
> > >>>> Is there any reason not to use the mxs_lradc struct directly has the
> > >>>> iio data?
> > >>> 
> > >>> I explained it below in the patch. I hope we'll eventually support
> > >>> the delay triggers, which will need 4 separate IIO devices. And this
> > >>> is where this structure will be augmented by other components.
> > >> 
> > >> Ok I saw the comment, but it wasn't obvious to me that delay channels
> > >> will require multiple IIO devices. Might make sense to state this
> > >> explicitly.
> > > 
> > > Fixed.
> > > 
> > >>>>> [...]
> > >>>>> 
> > >>>>> +
> > >>>>> +/*
> > >>>>> + * Channel management
> > >>>>> + *
> > >>>>> + * This involves crazy mapping between 8 virtual channels the
> > >>>>> CTRL4 register + * can harbor and 16 channels total this hardware
> > >>>>> supports. + */
> > >>>> 
> > >>>> I suppose this means only a maximum 8 channels can be active at a
> > >>>> time. I've recently posted a patch which makes it easy to implement
> > >>>> such a restriction.
> > >>>> http://www.spinics.net/lists/linux-iio/msg05936.html and
> > >>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
> > >>>> validate callback implementation. In your case you'd check for
> > >>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.
> > >>> 
> > >>> This is good. When do you expect this to hit linux-next possibly? Or
> > >>> at least linux-iio?
> > >> 
> > >> soon, hopefully.
> > > 
> > > So I checked this, not sure how it'll help me though.
> > 
> > Right now with your driver you can enable any combination of channels. If
> > more than 8 channels are enabled and you start sampling it will fail,
> > since not all channels can be mapped. With those patch you can implement
> > a validate callback and check for bitmap_weight(scan_mask) <= 8. This
> > will ensure that it is not possible to select more than 8 channels at
> > once, which again means that starting sampling can't fail of this.
> 
> Ok, I fixed this one. One last thing that I don't really understand is
> this. I run generic_buffer.c to source samples from the LRADC. Is there
> any way to continuously sample them? Because right now, I get one sample
> and that's it, no more samples happen later on (I can 0 data in subsequent
> read() call).

One more thing I'm curious about. There's another ADC block on the CPU, called 
HSADC (high-speed ADC). It can sample even up to 2Msamples/s. If I were to, say 
-- sample at 100kHz and be able to DMA the results into memory -- is there any 
way to push such results into userland somehow? Or how to operate such fast 
beast?

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Robert Schwebel]

On Fri, Jul 20, 2012 at 04:18:41AM +0200, Marek Vasut wrote:
> One more thing I'm curious about. There's another ADC block on the
> CPU, called HSADC (high-speed ADC). It can sample even up to
> 2Msamples/s. If I were to, say -- sample at 100kHz and be able to DMA
> the results into memory -- is there any way to push such results into
> userland somehow? Or how to operate such fast beast?

As this unit is mainly intended for things like laser / barcode
scanners, wouldn't v2l2 be a good abstraction for it (with one line and
N pixels)? Just an idea, didn't look too closely.

rsc
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Robert Schwebel,

> On Fri, Jul 20, 2012 at 04:18:41AM +0200, Marek Vasut wrote:
> > One more thing I'm curious about. There's another ADC block on the
> > CPU, called HSADC (high-speed ADC). It can sample even up to
> > 2Msamples/s. If I were to, say -- sample at 100kHz and be able to DMA
> > the results into memory -- is there any way to push such results into
> > userland somehow? Or how to operate such fast beast?
> 
> As this unit is mainly intended for things like laser / barcode
> scanners, wouldn't v2l2 be a good abstraction for it (with one line and
> N pixels)? Just an idea, didn't look too closely.

Considering it's not only for barcode scanners, but also some crazy sampling, I 
won't go for v4l2

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Lars-Peter Clausen]

On 07/20/2012 04:18 AM, Marek Vasut wrote:
>> Dear Lars-Peter Clausen,

> [...]
> 
> One more thing I'm curious about. There's another ADC block on the CPU, called 
> HSADC (high-speed ADC). It can sample even up to 2Msamples/s. If I were to, say 
> -- sample at 100kHz and be able to DMA the results into memory -- is there any 
> way to push such results into userland somehow? Or how to operate such fast 
> beast?

Proper support for high speed sampling is still something that's missing
from IIO. You can't mmap your buffers, you can't splice a IIO data stream to
another file descriptor, there is no support for zero copy. So your
bottleneck will become that you have to copy lots of data around. But it is
certainly something that will be added at some point. So implementing the
driver as a IIO driver is definitely the right direction.

- Lars

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Lars-Peter Clausen]

On 07/19/2012 09:26 PM, Marek Vasut wrote:
> Dear Lars-Peter Clausen,
> 
>> On 07/19/2012 04:23 PM, Marek Vasut wrote:
>>> Dear Lars-Peter Clausen,
>>>
>>> Sorry for my late reply, got busy with other crazy stuff.
>>>
>>> [...]
>>>
>>>>>> Is there any reason not to use the mxs_lradc struct directly has the
>>>>>> iio data?
>>>>>
>>>>> I explained it below in the patch. I hope we'll eventually support the
>>>>> delay triggers, which will need 4 separate IIO devices. And this is
>>>>> where this structure will be augmented by other components.
>>>>
>>>> Ok I saw the comment, but it wasn't obvious to me that delay channels
>>>> will require multiple IIO devices. Might make sense to state this
>>>> explicitly.
>>>
>>> Fixed.
>>>
>>>>>>> [...]
>>>>>>>
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * Channel management
>>>>>>> + *
>>>>>>> + * This involves crazy mapping between 8 virtual channels the CTRL4
>>>>>>> register + * can harbor and 16 channels total this hardware supports.
>>>>>>> + */
>>>>>>
>>>>>> I suppose this means only a maximum 8 channels can be active at a
>>>>>> time. I've recently posted a patch which makes it easy to implement
>>>>>> such a restriction.
>>>>>> http://www.spinics.net/lists/linux-iio/msg05936.html and
>>>>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
>>>>>> validate callback implementation. In your case you'd check for
>>>>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.
>>>>>
>>>>> This is good. When do you expect this to hit linux-next possibly? Or at
>>>>> least linux-iio?
>>>>
>>>> soon, hopefully.
>>>
>>> So I checked this, not sure how it'll help me though.
>>
>> Right now with your driver you can enable any combination of channels. If
>> more than 8 channels are enabled and you start sampling it will fail, since
>> not all channels can be mapped. With those patch you can implement a
>> validate callback and check for bitmap_weight(scan_mask) <= 8. This will
>> ensure that it is not possible to select more than 8 channels at once,
>> which again means that starting sampling can't fail of this.
> 
> Ok, I fixed this one. One last thing that I don't really understand is this. I 
> run generic_buffer.c to source samples from the LRADC. Is there any way to 
> continuously sample them? Because right now, I get one sample and that's it, no 
> more samples happen later on (I can 0 data in subsequent read() call).
> 

I'd consider that a bug in your driver ;)
The intend with IIO is that once you start sampling by enabling the buffer
you get a continuous data stream until sampling is stopped and this works
fine with other drivers.

- Lars

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Lars-Peter Clausen,

> On 07/19/2012 09:26 PM, Marek Vasut wrote:
> > Dear Lars-Peter Clausen,
> > 
> >> On 07/19/2012 04:23 PM, Marek Vasut wrote:
> >>> Dear Lars-Peter Clausen,
> >>> 
> >>> Sorry for my late reply, got busy with other crazy stuff.
> >>> 
> >>> [...]
> >>> 
> >>>>>> Is there any reason not to use the mxs_lradc struct directly has the
> >>>>>> iio data?
> >>>>> 
> >>>>> I explained it below in the patch. I hope we'll eventually support
> >>>>> the delay triggers, which will need 4 separate IIO devices. And this
> >>>>> is where this structure will be augmented by other components.
> >>>> 
> >>>> Ok I saw the comment, but it wasn't obvious to me that delay channels
> >>>> will require multiple IIO devices. Might make sense to state this
> >>>> explicitly.
> >>> 
> >>> Fixed.
> >>> 
> >>>>>>> [...]
> >>>>>>> 
> >>>>>>> +
> >>>>>>> +/*
> >>>>>>> + * Channel management
> >>>>>>> + *
> >>>>>>> + * This involves crazy mapping between 8 virtual channels the
> >>>>>>> CTRL4 register + * can harbor and 16 channels total this hardware
> >>>>>>> supports. + */
> >>>>>> 
> >>>>>> I suppose this means only a maximum 8 channels can be active at a
> >>>>>> time. I've recently posted a patch which makes it easy to implement
> >>>>>> such a restriction.
> >>>>>> http://www.spinics.net/lists/linux-iio/msg05936.html and
> >>>>>> http://www.spinics.net/lists/linux-iio/msg05935.html for an example
> >>>>>> validate callback implementation. In your case you'd check for
> >>>>>> bitmap_weight(...) <= 8. Those patches are not yet in IIO though.
> >>>>> 
> >>>>> This is good. When do you expect this to hit linux-next possibly? Or
> >>>>> at least linux-iio?
> >>>> 
> >>>> soon, hopefully.
> >>> 
> >>> So I checked this, not sure how it'll help me though.
> >> 
> >> Right now with your driver you can enable any combination of channels.
> >> If more than 8 channels are enabled and you start sampling it will
> >> fail, since not all channels can be mapped. With those patch you can
> >> implement a validate callback and check for bitmap_weight(scan_mask) <=
> >> 8. This will ensure that it is not possible to select more than 8
> >> channels at once, which again means that starting sampling can't fail
> >> of this.
> > 
> > Ok, I fixed this one. One last thing that I don't really understand is
> > this. I run generic_buffer.c to source samples from the LRADC. Is there
> > any way to continuously sample them? Because right now, I get one sample
> > and that's it, no more samples happen later on (I can 0 data in
> > subsequent read() call).
> 
> I'd consider that a bug in your driver ;)
> The intend with IIO is that once you start sampling by enabling the buffer
> you get a continuous data stream until sampling is stopped and this works
> fine with other drivers.

Ah! Thanks, I'll poke into this.

> - Lars

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Juergen Beisert]

Hi Marek,

Marek Vasut wrote:
> This driver is very basic. It supports userland trigger, buffer and
> raw access to channels. The support for delay channels is missing
> altogether.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Juergen Beisert <jbe@pengutronix.de>
> ---
>  drivers/staging/iio/adc/Kconfig     |   12 +
>  drivers/staging/iio/adc/Makefile    |    1 +
>  drivers/staging/iio/adc/mxs-lradc.c |  619 +++++++++++++++++++++++++++++++++++
>  3 files changed, 632 insertions(+) 
>  create mode 100644 drivers/staging/iio/adc/mxs-lradc.c
>
> [...]

sorry for the delay, too much work at the same time. When I try to compile
your code I get:

drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: linux/iio/triggered_buffer.h: No such file or directory

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Lars-Peter Clausen]

On 07/09/2012 11:19 AM, Juergen Beisert wrote:
> Hi Marek,
> 
> Marek Vasut wrote:
>> This driver is very basic. It supports userland trigger, buffer and
>> raw access to channels. The support for delay channels is missing
>> altogether.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Jonathan Cameron <jic23@kernel.org>
>> Cc: Wolfgang Denk <wd@denx.de>
>> Cc: Juergen Beisert <jbe@pengutronix.de>
>> ---
>>  drivers/staging/iio/adc/Kconfig     |   12 +
>>  drivers/staging/iio/adc/Makefile    |    1 +
>>  drivers/staging/iio/adc/mxs-lradc.c |  619 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 632 insertions(+) 
>>  create mode 100644 drivers/staging/iio/adc/mxs-lradc.c
>>
>> [...]
> 
> sorry for the delay, too much work at the same time. When I try to compile
> your code I get:
> 
> drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error: linux/iio/triggered_buffer.h: No such file or directory
> 

This file only got added recently, right now it is only in staging/staging-next

- Lars

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Juergen Beisert,

> Hi Marek,
> 
> Marek Vasut wrote:
> > This driver is very basic. It supports userland trigger, buffer and
> > raw access to channels. The support for delay channels is missing
> > altogether.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Juergen Beisert <jbe@pengutronix.de>
> > ---
> > 
> >  drivers/staging/iio/adc/Kconfig     |   12 +
> >  drivers/staging/iio/adc/Makefile    |    1 +
> >  drivers/staging/iio/adc/mxs-lradc.c |  619
> >  +++++++++++++++++++++++++++++++++++ 3 files changed, 632 insertions(+)
> >  create mode 100644 drivers/staging/iio/adc/mxs-lradc.c
> > 
> > [...]
> 
> sorry for the delay, too much work at the same time.

You tell me.

> When I try to compile
> your code I get:
> 
> drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error:
> linux/iio/triggered_buffer.h: No such file or directory

You need this patches:
    iio:kfifo_buf Take advantage of the fixed record size used in IIO
    iio: kfifo - add poll support.

And use latest -next.

> 
> Regards,
> Juergen

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Juergen Beisert]

Hi Marek,

> > When I try to compile
> > your code I get:
> >
> > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error:
> > linux/iio/triggered_buffer.h: No such file or directory
>
> You need this patches:
>     iio:kfifo_buf Take advantage of the fixed record size used in IIO
>     iio: kfifo - add poll support.
>
> And use latest -next.

Thanks for the hints. Now it compiles and the driver seems to work.

One thing I do not understand: It does not matter what channel I read 
('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts up. 
Intended?
Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 20 21 22 
23 24 25>" to the corresponding device tree entry?

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Juergen Beisert,

> Hi Marek,
> 
> > > When I try to compile
> > > your code I get:
> > > 
> > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error:
> > > linux/iio/triggered_buffer.h: No such file or directory
> > 
> > You need this patches:
> >     iio:kfifo_buf Take advantage of the fixed record size used in IIO
> >     iio: kfifo - add poll support.
> > 
> > And use latest -next.
> 
> Thanks for the hints. Now it compiles and the driver seems to work.
> 
> One thing I do not understand: It does not matter what channel I read
> ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts up.
> Intended?
> Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 20 21
> 22 23 24 25>" to the corresponding device tree entry?


They're wrong

lradc@80050000 {
        compatible = "fsl,imx28-lradc";
        reg = <0x80050000 2000>;
        interrupts = <10 14 15 16 17 18 19
                        20 21 22 23 24 25>;
        status = "disabled";
};


> Regards,
> Juergen

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Juergen Beisert]

Hi Marek,

Marek Vasut wrote:
> > > > When I try to compile
> > > > your code I get:
> > > >
> > > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error:
> > > > linux/iio/triggered_buffer.h: No such file or directory
> > >
> > > You need this patches:
> > >     iio:kfifo_buf Take advantage of the fixed record size used in IIO
> > >     iio: kfifo - add poll support.
> > >
> > > And use latest -next.
> >
> > Thanks for the hints. Now it compiles and the driver seems to work.
> >
> > One thing I do not understand: It does not matter what channel I read
> > ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts up.
> > Intended?
> > Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19 20
> > 21 22 23 24 25>" to the corresponding device tree entry?
>
> They're wrong
>
> lradc@80050000 {
>         compatible = "fsl,imx28-lradc";
>         reg = <0x80050000 2000>;
>         interrupts = <10 14 15 16 17 18 19
>                         20 21 22 23 24 25>;
>         status = "disabled";
> };

Ups, thanks. But still the same behaviour:

$ cat /proc/interrupts
[...]
 10:          0         -  mxs-lradc-touchscreen
 14:          0         -  mxs-lradc-thresh0
 15:          0         -  mxs-lradc-thresh1
 16:          0         -  mxs-lradc-channel0
 17:          0         -  mxs-lradc-channel1
 18:          0         -  mxs-lradc-channel2
 19:          0         -  mxs-lradc-channel3
 20:          0         -  mxs-lradc-channel4
 21:          0         -  mxs-lradc-channel5
 22:          0         -  mxs-lradc-channel6
 23:          0         -  mxs-lradc-channel7
 24:          0         -  mxs-lradc-button0
 25:          0         -  mxs-lradc-button1
[...]

$ cat in_voltage0_raw
524
$ cat in_voltage1_raw
96
$ cat in_voltage2_raw
1261

$ cat /proc/interrupts
[...]
 10:          0         -  mxs-lradc-touchscreen
 14:          0         -  mxs-lradc-thresh0
 15:          0         -  mxs-lradc-thresh1
 16:          3         -  mxs-lradc-channel0
 17:          0         -  mxs-lradc-channel1
 18:          0         -  mxs-lradc-channel2
 19:          0         -  mxs-lradc-channel3
 20:          0         -  mxs-lradc-channel4
 21:          0         -  mxs-lradc-channel5
 22:          0         -  mxs-lradc-channel6
 23:          0         -  mxs-lradc-channel7
 24:          0         -  mxs-lradc-button0
 25:          0         -  mxs-lradc-button1
[...]

Intended in this way?

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Juergen Beisert,

> Hi Marek,
> 
> Marek Vasut wrote:
> > > > > When I try to compile
> > > > > your code I get:
> > > > > 
> > > > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error:
> > > > > linux/iio/triggered_buffer.h: No such file or directory
> > > > 
> > > > You need this patches:
> > > >     iio:kfifo_buf Take advantage of the fixed record size used in IIO
> > > >     iio: kfifo - add poll support.
> > > > 
> > > > And use latest -next.
> > > 
> > > Thanks for the hints. Now it compiles and the driver seems to work.
> > > 
> > > One thing I do not understand: It does not matter what channel I read
> > > ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts
> > > up. Intended?
> > > Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19
> > > 20 21 22 23 24 25>" to the corresponding device tree entry?
> > 
> > They're wrong
> > 
> > lradc@80050000 {
> > 
> >         compatible = "fsl,imx28-lradc";
> >         reg = <0x80050000 2000>;
> >         interrupts = <10 14 15 16 17 18 19
> >         
> >                         20 21 22 23 24 25>;
> >         
> >         status = "disabled";
> > 
> > };
> 
> Ups, thanks. But still the same behaviour:
> 
> $ cat /proc/interrupts
> [...]
>  10:          0         -  mxs-lradc-touchscreen
>  14:          0         -  mxs-lradc-thresh0
>  15:          0         -  mxs-lradc-thresh1
>  16:          0         -  mxs-lradc-channel0
>  17:          0         -  mxs-lradc-channel1
>  18:          0         -  mxs-lradc-channel2
>  19:          0         -  mxs-lradc-channel3
>  20:          0         -  mxs-lradc-channel4
>  21:          0         -  mxs-lradc-channel5
>  22:          0         -  mxs-lradc-channel6
>  23:          0         -  mxs-lradc-channel7
>  24:          0         -  mxs-lradc-button0
>  25:          0         -  mxs-lradc-button1
> [...]
> 
> $ cat in_voltage0_raw
> 524
> $ cat in_voltage1_raw
> 96
> $ cat in_voltage2_raw
> 1261
> 
> $ cat /proc/interrupts
> [...]
>  10:          0         -  mxs-lradc-touchscreen
>  14:          0         -  mxs-lradc-thresh0
>  15:          0         -  mxs-lradc-thresh1
>  16:          3         -  mxs-lradc-channel0
>  17:          0         -  mxs-lradc-channel1
>  18:          0         -  mxs-lradc-channel2
>  19:          0         -  mxs-lradc-channel3
>  20:          0         -  mxs-lradc-channel4
>  21:          0         -  mxs-lradc-channel5
>  22:          0         -  mxs-lradc-channel6
>  23:          0         -  mxs-lradc-channel7
>  24:          0         -  mxs-lradc-button0
>  25:          0         -  mxs-lradc-button1
> [...]
> 
> Intended in this way?

But wait, you're getting interrupts on channel 0. Doesn't seem quite right. Did 
you happen to poke into the code and see where the issue might be?

> Regards,
> Juergen

Best regards,
Marek Vasut

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Juergen Beisert]

Marek Vasut wrote:
> Dear Juergen Beisert,
>
> > Hi Marek,
> >
> > Marek Vasut wrote:
> > > > > > When I try to compile
> > > > > > your code I get:
> > > > > >
> > > > > > drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error:
> > > > > > linux/iio/triggered_buffer.h: No such file or directory
> > > > >
> > > > > You need this patches:
> > > > >     iio:kfifo_buf Take advantage of the fixed record size used in
> > > > > IIO iio: kfifo - add poll support.
> > > > >
> > > > > And use latest -next.
> > > >
> > > > Thanks for the hints. Now it compiles and the driver seems to work.
> > > >
> > > > One thing I do not understand: It does not matter what channel I read
> > > > ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts
> > > > up. Intended?
> > > > Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19
> > > > 20 21 22 23 24 25>" to the corresponding device tree entry?
> > >
> > > They're wrong
> > >
> > > lradc@80050000 {
> > >
> > >         compatible = "fsl,imx28-lradc";
> > >         reg = <0x80050000 2000>;
> > >         interrupts = <10 14 15 16 17 18 19
> > >
> > >                         20 21 22 23 24 25>;
> > >
> > >         status = "disabled";
> > >
> > > };
> >
> > Ups, thanks. But still the same behaviour:
> >
> > $ cat /proc/interrupts
> > [...]
> >  10:          0         -  mxs-lradc-touchscreen
> >  14:          0         -  mxs-lradc-thresh0
> >  15:          0         -  mxs-lradc-thresh1
> >  16:          0         -  mxs-lradc-channel0
> >  17:          0         -  mxs-lradc-channel1
> >  18:          0         -  mxs-lradc-channel2
> >  19:          0         -  mxs-lradc-channel3
> >  20:          0         -  mxs-lradc-channel4
> >  21:          0         -  mxs-lradc-channel5
> >  22:          0         -  mxs-lradc-channel6
> >  23:          0         -  mxs-lradc-channel7
> >  24:          0         -  mxs-lradc-button0
> >  25:          0         -  mxs-lradc-button1
> > [...]
> >
> > $ cat in_voltage0_raw
> > 524
> > $ cat in_voltage1_raw
> > 96
> > $ cat in_voltage2_raw
> > 1261
> >
> > $ cat /proc/interrupts
> > [...]
> >  10:          0         -  mxs-lradc-touchscreen
> >  14:          0         -  mxs-lradc-thresh0
> >  15:          0         -  mxs-lradc-thresh1
> >  16:          3         -  mxs-lradc-channel0
> >  17:          0         -  mxs-lradc-channel1
> >  18:          0         -  mxs-lradc-channel2
> >  19:          0         -  mxs-lradc-channel3
> >  20:          0         -  mxs-lradc-channel4
> >  21:          0         -  mxs-lradc-channel5
> >  22:          0         -  mxs-lradc-channel6
> >  23:          0         -  mxs-lradc-channel7
> >  24:          0         -  mxs-lradc-button0
> >  25:          0         -  mxs-lradc-button1
> > [...]
> >
> > Intended in this way?
>
> But wait, you're getting interrupts on channel 0. Doesn't seem quite right.
> Did you happen to poke into the code and see where the issue might be?

No yet. But if you think this is not the intended behaviour of your driver I 
will do a deeper look.

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Lars-Peter Clausen]

On 07/10/2012 12:26 PM, Juergen Beisert wrote:
> Marek Vasut wrote:
>> Dear Juergen Beisert,
>>
>>> Hi Marek,
>>>
>>> Marek Vasut wrote:
>>>>>>> When I try to compile
>>>>>>> your code I get:
>>>>>>>
>>>>>>> drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error:
>>>>>>> linux/iio/triggered_buffer.h: No such file or directory
>>>>>>
>>>>>> You need this patches:
>>>>>>     iio:kfifo_buf Take advantage of the fixed record size used in
>>>>>> IIO iio: kfifo - add poll support.
>>>>>>
>>>>>> And use latest -next.
>>>>>
>>>>> Thanks for the hints. Now it compiles and the driver seems to work.
>>>>>
>>>>> One thing I do not understand: It does not matter what channel I read
>>>>> ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts
>>>>> up. Intended?
>>>>> Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19
>>>>> 20 21 22 23 24 25>" to the corresponding device tree entry?
>>>>
>>>> They're wrong
>>>>
>>>> lradc@80050000 {
>>>>
>>>>         compatible = "fsl,imx28-lradc";
>>>>         reg = <0x80050000 2000>;
>>>>         interrupts = <10 14 15 16 17 18 19
>>>>
>>>>                         20 21 22 23 24 25>;
>>>>
>>>>         status = "disabled";
>>>>
>>>> };
>>>
>>> Ups, thanks. But still the same behaviour:
>>>
>>> $ cat /proc/interrupts
>>> [...]
>>>  10:          0         -  mxs-lradc-touchscreen
>>>  14:          0         -  mxs-lradc-thresh0
>>>  15:          0         -  mxs-lradc-thresh1
>>>  16:          0         -  mxs-lradc-channel0
>>>  17:          0         -  mxs-lradc-channel1
>>>  18:          0         -  mxs-lradc-channel2
>>>  19:          0         -  mxs-lradc-channel3
>>>  20:          0         -  mxs-lradc-channel4
>>>  21:          0         -  mxs-lradc-channel5
>>>  22:          0         -  mxs-lradc-channel6
>>>  23:          0         -  mxs-lradc-channel7
>>>  24:          0         -  mxs-lradc-button0
>>>  25:          0         -  mxs-lradc-button1
>>> [...]
>>>
>>> $ cat in_voltage0_raw
>>> 524
>>> $ cat in_voltage1_raw
>>> 96
>>> $ cat in_voltage2_raw
>>> 1261
>>>
>>> $ cat /proc/interrupts
>>> [...]
>>>  10:          0         -  mxs-lradc-touchscreen
>>>  14:          0         -  mxs-lradc-thresh0
>>>  15:          0         -  mxs-lradc-thresh1
>>>  16:          3         -  mxs-lradc-channel0
>>>  17:          0         -  mxs-lradc-channel1
>>>  18:          0         -  mxs-lradc-channel2
>>>  19:          0         -  mxs-lradc-channel3
>>>  20:          0         -  mxs-lradc-channel4
>>>  21:          0         -  mxs-lradc-channel5
>>>  22:          0         -  mxs-lradc-channel6
>>>  23:          0         -  mxs-lradc-channel7
>>>  24:          0         -  mxs-lradc-button0
>>>  25:          0         -  mxs-lradc-button1
>>> [...]
>>>
>>> Intended in this way?
>>
>> But wait, you're getting interrupts on channel 0. Doesn't seem quite right.
>> Did you happen to poke into the code and see where the issue might be?
> 
> No yet. But if you think this is not the intended behaviour of your driver I 
> will do a deeper look.
> 

I don't know the hardware, but from what I've seen in the driver this
behavior looks correct. When reading a physical channel the channel will be
mapped to the first free virtual channel, afterward it is unmapped again.

So if you are only reading a single channel it will always be mapped to
first virtual channel. If you'd be using buffered mode and read multiple
channels at once you'll probably get multiple interrupts on different
virtual channels.

- Lars

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Juergen Beisert]

Lars-Peter Clausen wrote:
> On 07/10/2012 12:26 PM, Juergen Beisert wrote:
> > Marek Vasut wrote:
> >> Dear Juergen Beisert,
> >>
> >>> Hi Marek,
> >>>
> >>> Marek Vasut wrote:
> >>>>>>> When I try to compile
> >>>>>>> your code I get:
> >>>>>>>
> >>>>>>> drivers/staging/iio/adc/mxs-lradc.c:42:40: fatal error:
> >>>>>>> linux/iio/triggered_buffer.h: No such file or directory
> >>>>>>
> >>>>>> You need this patches:
> >>>>>>     iio:kfifo_buf Take advantage of the fixed record size used in
> >>>>>> IIO iio: kfifo - add poll support.
> >>>>>>
> >>>>>> And use latest -next.
> >>>>>
> >>>>> Thanks for the hints. Now it compiles and the driver seems to work.
> >>>>>
> >>>>> One thing I do not understand: It does not matter what channel I read
> >>>>> ('in_voltage*_raw'), only interrupt 16 ('mxs-lradc-channel0') counts
> >>>>> up. Intended?
> >>>>> Or did I a mistake by adding interrupt numbers "<13 14 15 16 17 18 19
> >>>>> 20 21 22 23 24 25>" to the corresponding device tree entry?
> >>>>
> >>>> They're wrong
> >>>>
> >>>> lradc@80050000 {
> >>>>
> >>>>         compatible = "fsl,imx28-lradc";
> >>>>         reg = <0x80050000 2000>;
> >>>>         interrupts = <10 14 15 16 17 18 19
> >>>>
> >>>>                         20 21 22 23 24 25>;
> >>>>
> >>>>         status = "disabled";
> >>>>
> >>>> };
> >>>
> >>> Ups, thanks. But still the same behaviour:
> >>>
> >>> $ cat /proc/interrupts
> >>> [...]
> >>>  10:          0         -  mxs-lradc-touchscreen
> >>>  14:          0         -  mxs-lradc-thresh0
> >>>  15:          0         -  mxs-lradc-thresh1
> >>>  16:          0         -  mxs-lradc-channel0
> >>>  17:          0         -  mxs-lradc-channel1
> >>>  18:          0         -  mxs-lradc-channel2
> >>>  19:          0         -  mxs-lradc-channel3
> >>>  20:          0         -  mxs-lradc-channel4
> >>>  21:          0         -  mxs-lradc-channel5
> >>>  22:          0         -  mxs-lradc-channel6
> >>>  23:          0         -  mxs-lradc-channel7
> >>>  24:          0         -  mxs-lradc-button0
> >>>  25:          0         -  mxs-lradc-button1
> >>> [...]
> >>>
> >>> $ cat in_voltage0_raw
> >>> 524
> >>> $ cat in_voltage1_raw
> >>> 96
> >>> $ cat in_voltage2_raw
> >>> 1261
> >>>
> >>> $ cat /proc/interrupts
> >>> [...]
> >>>  10:          0         -  mxs-lradc-touchscreen
> >>>  14:          0         -  mxs-lradc-thresh0
> >>>  15:          0         -  mxs-lradc-thresh1
> >>>  16:          3         -  mxs-lradc-channel0
> >>>  17:          0         -  mxs-lradc-channel1
> >>>  18:          0         -  mxs-lradc-channel2
> >>>  19:          0         -  mxs-lradc-channel3
> >>>  20:          0         -  mxs-lradc-channel4
> >>>  21:          0         -  mxs-lradc-channel5
> >>>  22:          0         -  mxs-lradc-channel6
> >>>  23:          0         -  mxs-lradc-channel7
> >>>  24:          0         -  mxs-lradc-button0
> >>>  25:          0         -  mxs-lradc-button1
> >>> [...]
> >>>
> >>> Intended in this way?
> >>
> >> But wait, you're getting interrupts on channel 0. Doesn't seem quite
> >> right. Did you happen to poke into the code and see where the issue
> >> might be?
> >
> > No yet. But if you think this is not the intended behaviour of your
> > driver I will do a deeper look.
>
> I don't know the hardware, but from what I've seen in the driver this
> behavior looks correct. When reading a physical channel the channel will be
> mapped to the first free virtual channel, afterward it is unmapped again.

Makes sense.

> So if you are only reading a single channel it will always be mapped to
> first virtual channel. If you'd be using buffered mode and read multiple
> channels at once you'll probably get multiple interrupts on different
> virtual channels.

So, the driver's behaviour is intended. Sorry for the noise.

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH] IIO: Add basic MXS LRADC driver

[Marek Vasut]

Dear Juergen Beisert,

[...]

> > >> But wait, you're getting interrupts on channel 0. Doesn't seem quite
> > >> right. Did you happen to poke into the code and see where the issue
> > >> might be?
> > > 
> > > No yet. But if you think this is not the intended behaviour of your
> > > driver I will do a deeper look.
> > 
> > I don't know the hardware, but from what I've seen in the driver this
> > behavior looks correct. When reading a physical channel the channel will
> > be mapped to the first free virtual channel, afterward it is unmapped
> > again.
> 
> Makes sense.
> 
> > So if you are only reading a single channel it will always be mapped to
> > first virtual channel. If you'd be using buffered mode and read multiple
> > channels at once you'll probably get multiple interrupts on different
> > virtual channels.
> 
> So, the driver's behaviour is intended. Sorry for the noise.

Heh, my own driver confused me. I really better get back to this thing, I even 
have the touchscreen part in the works. Sorry for the confusion.

> Regards,
> Juergen

Best regards,
Marek Vasut