[PATCH 0/6] staging: comedi: dt2814: Fix broken AI command support

[PATCH 0/6] staging: comedi: dt2814: Fix broken AI command support

[Ian Abbott]

The DT2814 board is an old analog-to-digital ISA card supported by the
Comedi "dt2814" driver.  The board has a timed acquisition mode, and an
end-of-conversion interrupt.  The interrupt can be disabled by removing
the jumper that sets the IRQ level.  In Comedi, the board is a manually
configured "legacy" device with an optional IRQ setting.

The Comedi driver has some broken support for asynchronous streaming
commands when the optional IRQ setting has been configured.  The main
problems are:

* It doesn't write any samples to the Comedi buffer to be read by the
  user application.
* The IRQ handler gets called at the end of any conversion, including
  during non-streaming, software-triggered acquisitions, but will
  interfere with these acquisitions because it reads the A/D Data
  register.

This series of patches attempts to support the asynchronous commands
properly so that it only reads the A/D Data register if the asynchronous
command has been set up, and actually writes it to the Comedi buffer.

Stopping the asynchronous command triggers an unwanted conversion.  The
interrupt handler used to wait for that to finish, but this series of
patches changes that so that other places in the code outside the
interrupt handler wait for a stale conversion to finish and/or read the
stale data before starting a new conversion.

1) staging: comedi: dt2814: Clear stale AI data before operation
2) staging: comedi: dt2814: Don't wait for conversion in interrupt handler
3) staging: comedi: dt2814: Call dt2814_ai_clear() during initialization
4) staging: comedi: dt2814: Fix asynchronous command interrupt handling
5) staging: comedi: dt2814: Remove struct dt2814_private
6) staging: comedi: dt2814: Clear stale AI data on detach

 drivers/staging/comedi/drivers/dt2814.c | 153 +++++++++++++++++++++++++-------
 1 file changed, 120 insertions(+), 33 deletions(-)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/6] staging: comedi: dt2814: Clear stale AI data before operation

[Ian Abbott]

When performing a Comedi read instruction or setting up an asynchronous
command on the AI subdevice, clear any stale data on the A/D registers
by waiting for the Status register's BUSY bit to clear (if set) and then
if the FINISH or ERR bit is set, reading the A/D Data register twice to
clear the stale data.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/dt2814.c | 39 +++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/staging/comedi/drivers/dt2814.c b/drivers/staging/comedi/drivers/dt2814.c
index bd329d7b4893..7e73aa094eea 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -52,6 +52,43 @@ struct dt2814_private {
 #define DT2814_TIMEOUT 10
 #define DT2814_MAX_SPEED 100000	/* Arbitrary 10 khz limit */
 
+static int dt2814_ai_notbusy(struct comedi_device *dev,
+			     struct comedi_subdevice *s,
+			     struct comedi_insn *insn,
+			     unsigned long context)
+{
+	unsigned int status;
+
+	status = inb(dev->iobase + DT2814_CSR);
+	if (context)
+		*(unsigned int *)context = status;
+	if (status & DT2814_BUSY)
+		return -EBUSY;
+	return 0;
+}
+
+static int dt2814_ai_clear(struct comedi_device *dev)
+{
+	unsigned int status = 0;
+	int ret;
+
+	/* Wait until not busy and get status register value. */
+	ret = comedi_timeout(dev, NULL, NULL, dt2814_ai_notbusy,
+			     (unsigned long)&status);
+	if (ret)
+		return ret;
+
+	if (status & (DT2814_FINISH | DT2814_ERR)) {
+		/*
+		 * There unread data, or the error flag is set.
+		 * Read the data register twice to clear the condition.
+		 */
+		inb(dev->iobase + DT2814_DATA);
+		inb(dev->iobase + DT2814_DATA);
+	}
+	return 0;
+}
+
 static int dt2814_ai_eoc(struct comedi_device *dev,
 			 struct comedi_subdevice *s,
 			 struct comedi_insn *insn,
@@ -73,6 +110,7 @@ static int dt2814_ai_insn_read(struct comedi_device *dev,
 	int chan;
 	int ret;
 
+	dt2814_ai_clear(dev);	/* clear stale data or error */
 	for (n = 0; n < insn->n; n++) {
 		chan = CR_CHAN(insn->chanspec);
 
@@ -174,6 +212,7 @@ static int dt2814_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
 	int chan;
 	int trigvar;
 
+	dt2814_ai_clear(dev);	/* clear stale data or error */
 	trigvar = dt2814_ns_to_timer(&cmd->scan_begin_arg, cmd->flags);
 
 	chan = CR_CHAN(cmd->chanlist[0]);
-- 
2.30.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/6] staging: comedi: dt2814: Don't wait for conversion in interrupt handler

[Ian Abbott]

When the interrupt handler decides the final sample has been acquired,
it turns off the timer enable (ENB) bit in the Command register.  That
triggers another A/D conversion.  The interrupt handler currently waits
for that to finish and then reads the resulting, unwanted sample.  Since
the functions for handling Comedi read instructions and for setting up
asynchronous commands now call `dt2814_ai_clear()` to wait for and
discard any spurious A/D conversion, let's remove that code from the
interrupt handler.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/dt2814.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt2814.c b/drivers/staging/comedi/drivers/dt2814.c
index 7e73aa094eea..8a70b7ba30f4 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -238,21 +238,13 @@ static irqreturn_t dt2814_interrupt(int irq, void *d)
 	inb(dev->iobase + DT2814_DATA);
 
 	if (!(--devpriv->ntrig)) {
-		int i;
-
 		outb(0, dev->iobase + DT2814_CSR);
 		/*
-		 * note: turning off timed mode triggers another
-		 * sample.
+		 * Note: turning off timed mode triggers another
+		 * sample.  This will be mopped up by the calls to
+		 * dt2814_ai_clear().
 		 */
 
-		for (i = 0; i < DT2814_TIMEOUT; i++) {
-			if (inb(dev->iobase + DT2814_CSR) & DT2814_FINISH)
-				break;
-		}
-		inb(dev->iobase + DT2814_DATA);
-		inb(dev->iobase + DT2814_DATA);
-
 		s->async->events |= COMEDI_CB_EOA;
 	}
 	comedi_handle_events(dev, s);
-- 
2.30.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/6] staging: comedi: dt2814: Call dt2814_ai_clear() during initialization

[Ian Abbott]

The Comedi "attach" handler `dt2814_attach()` writes to the Control
register to turn off the timer enable 'ENB' bit, which triggers a
conversion.  It then sleeps awhile and checks the Status register,
returning an error if the ERR bit is set.  However, the ERR bit could
have been set due to the conversion being triggered while the A/D
converter was busy (unlikely) or due to the conversion being triggered
before some previous sample had been read from the A/D Data register.

Replace the existing code with a call to `dt2814_ai_clear()` which waits
for any conversion to finish and then clears any unread data or error
condition.  A non-zero return value from `dt2814_ai_clear()` indicates a
time-out while waiting for the A/D converter to become non-busy.  Return
an error in that case.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/dt2814.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt2814.c b/drivers/staging/comedi/drivers/dt2814.c
index 8a70b7ba30f4..da4dc4df3a95 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -262,13 +262,10 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 		return ret;
 
 	outb(0, dev->iobase + DT2814_CSR);
-	usleep_range(100, 200);
-	if (inb(dev->iobase + DT2814_CSR) & DT2814_ERR) {
+	if (dt2814_ai_clear(dev)) {
 		dev_err(dev->class_dev, "reset error (fatal)\n");
 		return -EIO;
 	}
-	inb(dev->iobase + DT2814_DATA);
-	inb(dev->iobase + DT2814_DATA);
 
 	if (it->options[1]) {
 		ret = request_irq(it->options[1], dt2814_interrupt, 0,
-- 
2.30.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/6] staging: comedi: dt2814: Fix asynchronous command interrupt handling

[Ian Abbott]

The support for asynchronous commands in this driver is currently
broken.  If interrupts are enabled, the interrupt handler is called at
the end of every A/D conversion.  A/D conversions could be due to
software-triggered conversions resulting from Comedi `INSN_READ`
instruction handling, or due to timer-trigger conversions enabled when
a Comedi asynchronous command is set up.  We only want the interrupt
handler to read a sample from the A/D Data register for timer-triggered
conversions, but currently it always reads the A/D Data register.  Since
the A/D Data register is read twice (to read a 12-bit value from an
8-bit register), that probably interferes with the reading for
software-triggered conversions.

The interrupt handler does not currently do anything with the data, it
just ignores it.  It should be written to the Comedi buffer if handling
an asynchronous command.

Other problems are that the driver has no Comedi `cancel` handler to
call when the asynchronous command is being stopped manually, and it
does not handle "infinite" acquisitions (when the command's `stop_src ==
TRIG_NONE`) properly.

Change the interrupt handler to check the timer enable (ENB) bit to
check the asynchronous command is active and return early if not
enabled.  Also check the error (ERR) and "conversion complete" (FINISH)
bits, and return early if neither is set.  Then the sample can be read
from the A/D Data register to clear the ERR and FINISH bits.  If the ERR
bit was set, terminate the acquisition with an error, otherwise write
the data to the Comedi buffer and check for end of acquisition.  Replace
the current check for end of acquisition, using the count of completed
scans in `scans_done` (updated by calls to `comedi_buf_write_samples()`)
when `stop_src == TRIG_COUNT`) and allowing "infinite" acquisitions when
`stop_src == TRIG_NONE`.

Add a `cancel` handler function `dt2814_ai_cancel()` that will be called
when the end of acquisition event is processed and when the acquisition
is stopped manually.  It turns off the timer enable (ENB) bit in the
Control register, leaving the current channel selected.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/dt2814.c | 72 ++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt2814.c b/drivers/staging/comedi/drivers/dt2814.c
index da4dc4df3a95..6f6d0b2bb44b 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -223,30 +223,87 @@ static int dt2814_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
 	return 0;
 }
 
+static int dt2814_ai_cancel(struct comedi_device *dev,
+			    struct comedi_subdevice *s)
+{
+	unsigned int status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->spinlock, flags);
+	status = inb(dev->iobase + DT2814_CSR);
+	if (status & DT2814_ENB) {
+		/*
+		 * Clear the timed trigger enable bit.
+		 *
+		 * Note: turning off timed mode triggers another
+		 * sample.  This will be mopped up by the calls to
+		 * dt2814_ai_clear().
+		 */
+		outb(status & DT2814_CHANMASK, dev->iobase + DT2814_CSR);
+	}
+	spin_unlock_irqrestore(&dev->spinlock, flags);
+	return 0;
+}
+
 static irqreturn_t dt2814_interrupt(int irq, void *d)
 {
 	struct comedi_device *dev = d;
-	struct dt2814_private *devpriv = dev->private;
 	struct comedi_subdevice *s = dev->read_subdev;
+	struct comedi_async *async;
+	unsigned int lo, hi;
+	unsigned short data;
+	unsigned int status;
 
 	if (!dev->attached) {
 		dev_err(dev->class_dev, "spurious interrupt\n");
 		return IRQ_HANDLED;
 	}
 
-	inb(dev->iobase + DT2814_DATA);
-	inb(dev->iobase + DT2814_DATA);
+	async = s->async;
 
-	if (!(--devpriv->ntrig)) {
-		outb(0, dev->iobase + DT2814_CSR);
+	spin_lock(&dev->spinlock);
+
+	status = inb(dev->iobase + DT2814_CSR);
+	if (!(status & DT2814_ENB)) {
+		/* Timed acquisition not enabled.  Nothing to do. */
+		spin_unlock(&dev->spinlock);
+		return IRQ_HANDLED;
+	}
+
+	if (!(status & (DT2814_FINISH | DT2814_ERR))) {
+		/* Spurious interrupt? */
+		spin_unlock(&dev->spinlock);
+		return IRQ_HANDLED;
+	}
+
+	/* Read data or clear error. */
+	hi = inb(dev->iobase + DT2814_DATA);
+	lo = inb(dev->iobase + DT2814_DATA);
+
+	data = (hi << 4) | (lo >> 4);
+
+	if (status & DT2814_ERR) {
+		async->events |= COMEDI_CB_ERROR;
+	} else {
+		comedi_buf_write_samples(s, &data, 1);
+		if (async->cmd.stop_src == TRIG_COUNT &&
+		    async->scans_done >=  async->cmd.stop_arg) {
+			async->events |= COMEDI_CB_EOA;
+		}
+	}
+	if (async->events & COMEDI_CB_CANCEL_MASK) {
 		/*
+		 * Disable timed mode.
+		 *
 		 * Note: turning off timed mode triggers another
 		 * sample.  This will be mopped up by the calls to
 		 * dt2814_ai_clear().
 		 */
-
-		s->async->events |= COMEDI_CB_EOA;
+		outb(status & DT2814_CHANMASK, dev->iobase + DT2814_CSR);
 	}
+
+	spin_unlock(&dev->spinlock);
+
 	comedi_handle_events(dev, s);
 	return IRQ_HANDLED;
 }
@@ -295,6 +352,7 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 		s->len_chanlist = 1;
 		s->do_cmd = dt2814_ai_cmd;
 		s->do_cmdtest = dt2814_ai_cmdtest;
+		s->cancel = dt2814_ai_cancel;
 	}
 
 	return 0;
-- 
2.30.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 5/6] staging: comedi: dt2814: Remove struct dt2814_private

[Ian Abbott]

The `ntrig` member of the `struct dt2814_private` pointed to by
`dev->private` is no longer used as a counter to determine the end of
acquisition for a Comedi asynchronous command.  The other member
`curadscan` is also unused.  Remove the allocation of the private data
during initialization and remove the definition of `struct
dt2814_private` since they are no longer needed.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/dt2814.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt2814.c b/drivers/staging/comedi/drivers/dt2814.c
index 6f6d0b2bb44b..d18f9a5a9fb1 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -44,11 +44,6 @@
 #define DT2814_ENB 0x10
 #define DT2814_CHANMASK 0x0f
 
-struct dt2814_private {
-	int ntrig;
-	int curadchan;
-};
-
 #define DT2814_TIMEOUT 10
 #define DT2814_MAX_SPEED 100000	/* Arbitrary 10 khz limit */
 
@@ -207,7 +202,6 @@ static int dt2814_ai_cmdtest(struct comedi_device *dev,
 
 static int dt2814_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
 {
-	struct dt2814_private *devpriv = dev->private;
 	struct comedi_cmd *cmd = &s->async->cmd;
 	int chan;
 	int trigvar;
@@ -217,7 +211,6 @@ static int dt2814_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
 
 	chan = CR_CHAN(cmd->chanlist[0]);
 
-	devpriv->ntrig = cmd->stop_arg;
 	outb(chan | DT2814_ENB | (trigvar << 5), dev->iobase + DT2814_CSR);
 
 	return 0;
@@ -310,7 +303,6 @@ static irqreturn_t dt2814_interrupt(int irq, void *d)
 
 static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 {
-	struct dt2814_private *devpriv;
 	struct comedi_subdevice *s;
 	int ret;
 
@@ -335,10 +327,6 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	if (ret)
 		return ret;
 
-	devpriv = comedi_alloc_devpriv(dev, sizeof(*devpriv));
-	if (!devpriv)
-		return -ENOMEM;
-
 	s = &dev->subdevices[0];
 	s->type = COMEDI_SUBD_AI;
 	s->subdev_flags = SDF_READABLE | SDF_GROUND;
-- 
2.30.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 6/6] staging: comedi: dt2814: Clear stale AI data on detach

[Ian Abbott]

When the Comedi "detach" handler is called, it is possible that an extra
A/D conversion (triggered during termination of a Comedi asynchronous
command) is still in progress.  In that case, the FINISH bit in the
Status register will eventually get set and there will be stale data
waiting to be read from the A/D Data register.  The interrupt handler
will also be called if still connected at the time.  That should all be
mostly harmless, but it would be better to wait for any such conversion
to complete and clear any stale data during the "detach".  Add a custom
"detach" handler `dt2814_detach()` to do that if an interrupt handler
has been set up.  (There is no need to do it if no interrupt handler was
set up because Comedi asynchronous command support is disabled in that
case.)

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/dt2814.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/dt2814.c b/drivers/staging/comedi/drivers/dt2814.c
index d18f9a5a9fb1..ed44ce0d151b 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -346,11 +346,24 @@ static int dt2814_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	return 0;
 }
 
+static void dt2814_detach(struct comedi_device *dev)
+{
+	if (dev->irq) {
+		/*
+		 * An extra conversion triggered on termination of an
+		 * asynchronous command may still be in progress.  Wait for
+		 * it to finish and clear the data or error status.
+		 */
+		dt2814_ai_clear(dev);
+	}
+	comedi_legacy_detach(dev);
+}
+
 static struct comedi_driver dt2814_driver = {
 	.driver_name	= "dt2814",
 	.module		= THIS_MODULE,
 	.attach		= dt2814_attach,
-	.detach		= comedi_legacy_detach,
+	.detach		= dt2814_detach,
 };
 module_comedi_driver(dt2814_driver);
 
-- 
2.30.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel