* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-04 2:15 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-04 2:15 UTC (permalink / raw)
To: linux-iio
Cc: linux-arm-kernel, Marek Vasut, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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, ®, 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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-04 2:15 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-04 2:15 UTC (permalink / raw)
To: linux-arm-kernel
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, ®, 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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-04 2:15 ` Marek Vasut
@ 2012-07-04 4:30 ` Wolfgang Denk
-1 siblings, 0 replies; 55+ messages in thread
From: Wolfgang Denk @ 2012-07-04 4:30 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Juergen Beisert
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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-04 4:30 ` Wolfgang Denk
0 siblings, 0 replies; 55+ messages in thread
From: Wolfgang Denk @ 2012-07-04 4:30 UTC (permalink / raw)
To: linux-arm-kernel
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 at denx.de
The speed of time is one second per second.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-04 2:15 ` Marek Vasut
@ 2012-07-04 8:35 ` Lars-Peter Clausen
-1 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-04 8:35 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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, ®, 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;
> +}
> +
> [...]
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-04 8:35 ` Lars-Peter Clausen
0 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-04 8:35 UTC (permalink / raw)
To: linux-arm-kernel
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, ®, 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;
> +}
> +
> [...]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-04 8:35 ` Lars-Peter Clausen
@ 2012-07-04 23:48 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-04 23:48 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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!
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-04 23:48 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-04 23:48 UTC (permalink / raw)
To: linux-arm-kernel
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@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!
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-04 23:48 ` Marek Vasut
@ 2012-07-05 8:33 ` Lars-Peter Clausen
-1 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-05 8:33 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-05 8:33 ` Lars-Peter Clausen
0 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-05 8:33 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-05 8:33 ` Lars-Peter Clausen
@ 2012-07-05 19:53 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-05 19:53 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-05 19:53 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-05 19:53 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-04 2:15 ` Marek Vasut
@ 2012-07-09 9:19 ` Juergen Beisert
-1 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-09 9:19 UTC (permalink / raw)
To: Marek Vasut; +Cc: linux-iio, linux-arm-kernel
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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-09 9:19 ` Juergen Beisert
0 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-09 9:19 UTC (permalink / raw)
To: linux-arm-kernel
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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-09 9:19 ` Juergen Beisert
@ 2012-07-09 9:52 ` Lars-Peter Clausen
-1 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-09 9:52 UTC (permalink / raw)
To: Juergen Beisert; +Cc: Marek Vasut, linux-iio, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-09 9:52 ` Lars-Peter Clausen
0 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-09 9:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-09 9:19 ` Juergen Beisert
@ 2012-07-09 10:03 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-09 10:03 UTC (permalink / raw)
To: Juergen Beisert; +Cc: linux-iio, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-09 10:03 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-09 10:03 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-09 10:03 ` Marek Vasut
@ 2012-07-10 9:20 ` Juergen Beisert
-1 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-10 9:20 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: Marek Vasut, linux-iio
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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-10 9:20 ` Juergen Beisert
0 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-10 9:20 UTC (permalink / raw)
To: linux-arm-kernel
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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-10 9:20 ` Juergen Beisert
@ 2012-07-10 9:26 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-10 9:26 UTC (permalink / raw)
To: Juergen Beisert; +Cc: linux-arm-kernel, linux-iio
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-10 9:26 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-10 9:26 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-10 9:26 ` Marek Vasut
@ 2012-07-10 9:49 ` Juergen Beisert
-1 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-10 9:49 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: Marek Vasut, linux-iio
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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-10 9:49 ` Juergen Beisert
0 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-10 9:49 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-10 9:49 ` Juergen Beisert
@ 2012-07-10 10:08 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-10 10:08 UTC (permalink / raw)
To: Juergen Beisert; +Cc: linux-arm-kernel, linux-iio
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-10 10:08 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-10 10:08 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-10 10:08 ` Marek Vasut
@ 2012-07-10 10:26 ` Juergen Beisert
-1 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-10 10:26 UTC (permalink / raw)
To: Marek Vasut; +Cc: linux-arm-kernel, linux-iio
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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-10 10:26 ` Juergen Beisert
0 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-10 10:26 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-10 10:26 ` Juergen Beisert
@ 2012-07-10 10:35 ` Lars-Peter Clausen
-1 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-10 10:35 UTC (permalink / raw)
To: Juergen Beisert; +Cc: Marek Vasut, linux-arm-kernel, linux-iio
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-10 10:35 ` Lars-Peter Clausen
0 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-10 10:35 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-10 10:35 ` Lars-Peter Clausen
@ 2012-07-10 10:41 ` Juergen Beisert
-1 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-10 10:41 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: Lars-Peter Clausen, Marek Vasut, linux-iio
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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-10 10:41 ` Juergen Beisert
0 siblings, 0 replies; 55+ messages in thread
From: Juergen Beisert @ 2012-07-10 10:41 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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/ |
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-10 10:41 ` Juergen Beisert
@ 2012-07-10 10:45 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-10 10:45 UTC (permalink / raw)
To: Juergen Beisert; +Cc: linux-arm-kernel, Lars-Peter Clausen, linux-iio
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-10 10:45 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-10 10:45 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-05 8:33 ` Lars-Peter Clausen
@ 2012-07-19 14:23 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-19 14:23 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-19 14:23 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-19 14:23 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-19 14:23 ` Marek Vasut
@ 2012-07-19 14:33 ` Lars-Peter Clausen
-1 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-19 14:33 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-19 14:33 ` Lars-Peter Clausen
0 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-19 14:33 UTC (permalink / raw)
To: linux-arm-kernel
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@once, which
again means that starting sampling can't fail of this.
- Lars
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-19 14:33 ` Lars-Peter Clausen
@ 2012-07-19 15:15 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-19 15:15 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-19 15:15 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-19 15:15 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-19 14:33 ` Lars-Peter Clausen
@ 2012-07-19 19:26 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-19 19:26 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-19 19:26 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-19 19:26 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-19 19:26 ` Marek Vasut
@ 2012-07-20 2:18 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-20 2:18 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-20 2:18 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-20 2:18 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-20 2:18 ` Marek Vasut
@ 2012-07-20 8:39 ` Robert Schwebel
-1 siblings, 0 replies; 55+ messages in thread
From: Robert Schwebel @ 2012-07-20 8:39 UTC (permalink / raw)
To: Marek Vasut
Cc: Lars-Peter Clausen, linux-iio, Jonathan Cameron,
linux-arm-kernel, Wolfgang Denk
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 |
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-20 8:39 ` Robert Schwebel
0 siblings, 0 replies; 55+ messages in thread
From: Robert Schwebel @ 2012-07-20 8:39 UTC (permalink / raw)
To: linux-arm-kernel
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 |
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-20 8:39 ` Robert Schwebel
@ 2012-07-20 11:32 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-20 11:32 UTC (permalink / raw)
To: Robert Schwebel
Cc: Lars-Peter Clausen, linux-iio, Jonathan Cameron,
linux-arm-kernel, Wolfgang Denk
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-20 11:32 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-20 11:32 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-20 2:18 ` Marek Vasut
@ 2012-07-20 14:09 ` Lars-Peter Clausen
-1 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-20 14:09 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-20 14:09 ` Lars-Peter Clausen
0 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-20 14:09 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-19 19:26 ` Marek Vasut
@ 2012-07-20 14:11 ` Lars-Peter Clausen
-1 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-20 14:11 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-20 14:11 ` Lars-Peter Clausen
0 siblings, 0 replies; 55+ messages in thread
From: Lars-Peter Clausen @ 2012-07-20 14:11 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] IIO: Add basic MXS LRADC driver
2012-07-20 14:11 ` Lars-Peter Clausen
@ 2012-07-20 15:12 ` Marek Vasut
-1 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-20 15:12 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: linux-iio, linux-arm-kernel, Jonathan Cameron, Wolfgang Denk,
Juergen Beisert
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
@ 2012-07-20 15:12 ` Marek Vasut
0 siblings, 0 replies; 55+ messages in thread
From: Marek Vasut @ 2012-07-20 15:12 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] IIO: Add basic MXS LRADC driver
2012-07-20 14:09 ` Lars-Peter Clausen
(?)
@ 2012-07-22 19:48 ` Jonathan Cameron
-1 siblings, 0 replies; 55+ messages in thread
From: Jonathan Cameron @ 2012-07-22 19:48 UTC (permalink / raw)
To: linux-arm-kernel
On 07/20/2012 03:09 PM, Lars-Peter Clausen wrote:
> 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 has pretty much covered it. A long time back I had a cunning plan
to wait for the tracing frameworks to agree on a one true ring buffer
then leverage that work to get us everything we could want.
Some of those buffers are extremely fast versatile and do fun things like
splicing. Also we have very little chance of introducing yet another complex
buffer implementation to the kernel (which incidentally is why we are ditching
sw_ring - other than it being rubbish ;) There were various articles on lwn
about this work, but I haven't heard anything recently. If anyone does
want (and crucially have time) to look into high performance (and large) buffer
options, that would be excellent! It is definitely something we want to have.
Jonathan
>
> - Lars
>
^ permalink raw reply [flat|nested] 55+ messages in thread
end of thread, other threads:[~2012-07-22 19:48 UTC | newest]
Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-04 2:15 [PATCH] IIO: Add basic MXS LRADC driver Marek Vasut
2012-07-04 2:15 ` Marek Vasut
2012-07-04 4:30 ` Wolfgang Denk
2012-07-04 4:30 ` Wolfgang Denk
2012-07-04 8:35 ` Lars-Peter Clausen
2012-07-04 8:35 ` Lars-Peter Clausen
2012-07-04 23:48 ` Marek Vasut
2012-07-04 23:48 ` Marek Vasut
2012-07-05 8:33 ` Lars-Peter Clausen
2012-07-05 8:33 ` Lars-Peter Clausen
2012-07-05 19:53 ` Marek Vasut
2012-07-05 19:53 ` Marek Vasut
2012-07-19 14:23 ` Marek Vasut
2012-07-19 14:23 ` Marek Vasut
2012-07-19 14:33 ` Lars-Peter Clausen
2012-07-19 14:33 ` Lars-Peter Clausen
2012-07-19 15:15 ` Marek Vasut
2012-07-19 15:15 ` Marek Vasut
2012-07-19 19:26 ` Marek Vasut
2012-07-19 19:26 ` Marek Vasut
2012-07-20 2:18 ` Marek Vasut
2012-07-20 2:18 ` Marek Vasut
2012-07-20 8:39 ` Robert Schwebel
2012-07-20 8:39 ` Robert Schwebel
2012-07-20 11:32 ` Marek Vasut
2012-07-20 11:32 ` Marek Vasut
2012-07-20 14:09 ` Lars-Peter Clausen
2012-07-20 14:09 ` Lars-Peter Clausen
2012-07-22 19:48 ` Jonathan Cameron
2012-07-20 14:11 ` Lars-Peter Clausen
2012-07-20 14:11 ` Lars-Peter Clausen
2012-07-20 15:12 ` Marek Vasut
2012-07-20 15:12 ` Marek Vasut
2012-07-09 9:19 ` Juergen Beisert
2012-07-09 9:19 ` Juergen Beisert
2012-07-09 9:52 ` Lars-Peter Clausen
2012-07-09 9:52 ` Lars-Peter Clausen
2012-07-09 10:03 ` Marek Vasut
2012-07-09 10:03 ` Marek Vasut
2012-07-10 9:20 ` Juergen Beisert
2012-07-10 9:20 ` Juergen Beisert
2012-07-10 9:26 ` Marek Vasut
2012-07-10 9:26 ` Marek Vasut
2012-07-10 9:49 ` Juergen Beisert
2012-07-10 9:49 ` Juergen Beisert
2012-07-10 10:08 ` Marek Vasut
2012-07-10 10:08 ` Marek Vasut
2012-07-10 10:26 ` Juergen Beisert
2012-07-10 10:26 ` Juergen Beisert
2012-07-10 10:35 ` Lars-Peter Clausen
2012-07-10 10:35 ` Lars-Peter Clausen
2012-07-10 10:41 ` Juergen Beisert
2012-07-10 10:41 ` Juergen Beisert
2012-07-10 10:45 ` Marek Vasut
2012-07-10 10:45 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.