[PATCH 0/3] staging: comedi: addi_apci_1564: miscellaneous fixes and cleanups

[PATCH 0/3] staging: comedi: addi_apci_1564: miscellaneous fixes and cleanups

[Chase Southwood]

This patchset moves a misplaced include to the proper file, swaps out an overly
aggressive placement of apci1564_reset(), and cleans up apci1564_interrupt().

Chase Southwood (3):
  staging: comedi: addi_apci_1564: move addi_watchdog.h include to
    addi_apci_1564.c
  staging: comedi: addi_apci_1564: fix use of apci1564_reset() to
    disable DI interrupts
  staging: comedi: addi_apci_1564: clean up apci1564_interrupt()

 .../comedi/drivers/addi-data/hwdrv_apci1564.c      |   2 -
 drivers/staging/comedi/drivers/addi_apci_1564.c    | 114 +++++----------------
 2 files changed, 28 insertions(+), 88 deletions(-)

-- 
2.0.1

[PATCH 1/3] staging: comedi: addi_apci_1564: move addi_watchdog.h include to addi_apci_1564.c

[Chase Southwood]

Commit aed3f9d (staging: comedi: addi_apci_1564: absorb apci1564_reset()) moved
the only use of addi_watchdog.h from hwdrv_apci1564.c to addi_apci_1564.c, but
left the include statement itself in the former file.  Move this include to the
file which actually uses it.

Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c | 2 --
 drivers/staging/comedi/drivers/addi_apci_1564.c           | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 4007fd2..7326f3a 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -21,8 +21,6 @@
  *
  */
 
-#include "../addi_watchdog.h"
-
 #define APCI1564_ADDRESS_RANGE				128
 
 /* Digital Input IRQ Function Selection */
diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index f71ee02..59786e7 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -4,6 +4,7 @@
 #include "../comedidev.h"
 #include "comedi_fc.h"
 #include "amcc_s5933.h"
+#include "addi_watchdog.h"
 
 #include "addi-data/addi_common.h"
 
-- 
2.0.1

[PATCH 2/3] staging: comedi: addi_apci_1564: fix use of apci1564_reset() to disable DI interrupts

[Chase Southwood]

apci1564_cos_insn_config() is currently using apci1564_reset() to disable
digital input interrupts when the configuration operation is
COMEDI_DIGITAL_TRIG_DISABLE.  However, this is incorrect as the device reset
function also resets the registers for the digital outputs, timer, watchdog, and
counters as well.  Replace the reset function call with a direct disabling of
just the digital input interrupts.

Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/drivers/addi_apci_1564.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 59786e7..0141ed9 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -285,7 +285,10 @@ static int apci1564_cos_insn_config(struct comedi_device *dev,
 			devpriv->ctrl = 0;
 			devpriv->mode1 = 0;
 			devpriv->mode2 = 0;
-			apci1564_reset(dev);
+			outl(0x0, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+			inl(devpriv->amcc_iobase + APCI1564_DI_INT_STATUS_REG);
+			outl(0x0, devpriv->amcc_iobase + APCI1564_DI_INT_MODE1_REG);
+			outl(0x0, devpriv->amcc_iobase + APCI1564_DI_INT_MODE2_REG);
 			break;
 		case COMEDI_DIGITAL_TRIG_ENABLE_EDGES:
 			if (devpriv->ctrl != (APCI1564_DI_INT_ENABLE |
-- 
2.0.1

[PATCH 3/3] staging: comedi: addi_apci_1564: clean up apci1564_interrupt()

[Chase Southwood]

The code in apci1564_interrupt() for handling counter interrupts is currently
repeated four times; once for each counter.  This code is identical save for the
registers it is using, so just handle all four counters with a for loop.

Also, the interrupt function was doing a useless set-and-check of
devpriv->timer_select_mode before processing any triggered interrupts, remove
all occurrences of this.

Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
Hartley,
I remember that you mentioned that the counters could be handled using a for
loop here.  Is there a better way to go about that or am I on the right track
with this?

Thanks,
Chase

 drivers/staging/comedi/drivers/addi_apci_1564.c | 108 +++++-------------------
 1 file changed, 23 insertions(+), 85 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 0141ed9..f40910e 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -60,8 +60,9 @@ static irqreturn_t apci1564_interrupt(int irq, void *d)
 	struct comedi_subdevice *s = dev->read_subdev;
 	unsigned int ui_DO, ui_DI;
 	unsigned int ui_Timer;
-	unsigned int ui_C1, ui_C2, ui_C3, ui_C4;
+	unsigned int counters[4];
 	unsigned int ul_Command2 = 0;
+	int i;
 
 	/* check interrupt is from this device */
 	if ((inl(devpriv->amcc_iobase + AMCC_OP_REG_INTCSR) &
@@ -73,16 +74,17 @@ static irqreturn_t apci1564_interrupt(int irq, void *d)
 		   APCI1564_DI_INT_ENABLE;
 	ui_DO = inl(devpriv->amcc_iobase + APCI1564_DO_IRQ_REG) & 0x01;
 	ui_Timer = inl(devpriv->amcc_iobase + APCI1564_TIMER_IRQ_REG) & 0x01;
-	ui_C1 =
+	counters[0] =
 		inl(dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER1)) & 0x1;
-	ui_C2 =
+	counters[1] =
 		inl(dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER2)) & 0x1;
-	ui_C3 =
+	counters[2] =
 		inl(dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER3)) & 0x1;
-	ui_C4 =
+	counters[3] =
 		inl(dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER4)) & 0x1;
-	if (ui_DI == 0 && ui_DO == 0 && ui_Timer == 0 && ui_C1 == 0
-		&& ui_C2 == 0 && ui_C3 == 0 && ui_C4 == 0) {
+	if (ui_DI == 0 && ui_DO == 0 && ui_Timer == 0 && counters[0] == 0
+		&& counters[1] == 0 && counters[2] == 0 && counters[3] == 0) {
+		dev_err(dev->class_dev, "Interrupt from unknown source.\n");
 		return IRQ_HANDLED;
 	}
 
@@ -113,95 +115,31 @@ static irqreturn_t apci1564_interrupt(int irq, void *d)
 	}
 
 	if (ui_Timer == 1) {
-		devpriv->timer_select_mode = ADDIDATA_TIMER;
-		if (devpriv->timer_select_mode) {
+		/*  Disable Timer Interrupt */
+		ul_Command2 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
+		outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
 
-			/*  Disable Timer Interrupt */
-			ul_Command2 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
-			outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
-
-			/* Send a signal to from kernel to user space */
-			send_sig(SIGIO, devpriv->tsk_current, 0);
-
-			/*  Enable Timer Interrupt */
-
-			outl(ul_Command2, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
-		}
-	}
-
-	if (ui_C1 == 1) {
-		devpriv->timer_select_mode = ADDIDATA_COUNTER;
-		if (devpriv->timer_select_mode) {
-
-			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
-
-			/* Send a signal to from kernel to user space */
-			send_sig(SIGIO, devpriv->tsk_current, 0);
-
-			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
-		}
-	}
-
-	if (ui_C2 == 1) {
-		devpriv->timer_select_mode = ADDIDATA_COUNTER;
-		if (devpriv->timer_select_mode) {
-
-			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER2));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER2));
-
-			/* Send a signal to from kernel to user space */
-			send_sig(SIGIO, devpriv->tsk_current, 0);
-
-			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER2));
-		}
-	}
-
-	if (ui_C3 == 1) {
-		devpriv->timer_select_mode = ADDIDATA_COUNTER;
-		if (devpriv->timer_select_mode) {
+		/* Send a signal to from kernel to user space */
+		send_sig(SIGIO, devpriv->tsk_current, 0);
 
-			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
+		/* Enable Timer Interrupt */
 
-			/* Send a signal to from kernel to user space */
-			send_sig(SIGIO, devpriv->tsk_current, 0);
-
-			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
-		}
+		outl(ul_Command2, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
 	}
 
-	if (ui_C4 == 1) {
-		devpriv->timer_select_mode = ADDIDATA_COUNTER;
-		if (devpriv->timer_select_mode) {
-
+	for (i = 0; i <= 3; i++) {
+		if (counters[i] == 1) {
 			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
+			ul_Command2 = inl(dev->iobase +
+					  APCI1564_TCW_CTRL_REG(i));
+			outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(i));
 
 			/* Send a signal to from kernel to user space */
 			send_sig(SIGIO, devpriv->tsk_current, 0);
 
 			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
+			outl(ul_Command2, dev->iobase +
+			     APCI1564_TCW_CTRL_REG(i));
 		}
 	}
 	return IRQ_HANDLED;
-- 
2.0.1

Re: [PATCH 0/3] staging: comedi: addi_apci_1564: miscellaneous fixes and cleanups

[Ian Abbott]

On 2014-06-28 05:47, Chase Southwood wrote:
> This patchset moves a misplaced include to the proper file, swaps out an overly
> aggressive placement of apci1564_reset(), and cleans up apci1564_interrupt().
>
> Chase Southwood (3):
>    staging: comedi: addi_apci_1564: move addi_watchdog.h include to
>      addi_apci_1564.c
>    staging: comedi: addi_apci_1564: fix use of apci1564_reset() to
>      disable DI interrupts
>    staging: comedi: addi_apci_1564: clean up apci1564_interrupt()
>

It's okay, but I think you can simplify the interrupt handling a bit 
more by not bothering to check for interrupts from unknown sources.   If 
a source hasn't been enabled, it shouldn't generate interrupts, right? 
Besides, since the does nothing to stop further interrupts from unknown 
sources, it would just keep getting further interrupts repeatedly in 
that case.

Then you can get rid of the ui_DO, ui_DI, ui_Timer, and counters[] 
variables and just check the registers directly (e.g. replace 'if 
(ui_Timer == 1)' with 'if (inl(devpriv->amcc_iobase + 
APCI1564_TIMER_IRQ_REG) & 0x01)').

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH 0/3] staging: comedi: addi_apci_1564: miscellaneous fixes and cleanups

[Chase Southwood]

Hi all,

On Mon, Jun 30, 2014 at 5:25 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
> On 2014-06-28 05:47, Chase Southwood wrote:
>>
>> This patchset moves a misplaced include to the proper file, swaps out an
>> overly
>> aggressive placement of apci1564_reset(), and cleans up
>> apci1564_interrupt().
>>
>> Chase Southwood (3):
>>    staging: comedi: addi_apci_1564: move addi_watchdog.h include to
>>      addi_apci_1564.c
>>    staging: comedi: addi_apci_1564: fix use of apci1564_reset() to
>>      disable DI interrupts
>>    staging: comedi: addi_apci_1564: clean up apci1564_interrupt()
>>
>
> It's okay, but I think you can simplify the interrupt handling a bit more by
> not bothering to check for interrupts from unknown sources.   If a source
> hasn't been enabled, it shouldn't generate interrupts, right? Besides, since
> the does nothing to stop further interrupts from unknown sources, it would
> just keep getting further interrupts repeatedly in that case.
>
> Then you can get rid of the ui_DO, ui_DI, ui_Timer, and counters[] variables
> and just check the registers directly (e.g. replace 'if (ui_Timer == 1)'
> with 'if (inl(devpriv->amcc_iobase + APCI1564_TIMER_IRQ_REG) & 0x01)').

I received some more good comments about the interrupt function, and
looks like Greg is on vacation this week anyway, so I think I will
just redo patch 3 and send out a v2 patchset.

Thanks,
Chase

[PATCH v2 0/3] staging: comedi: addi_apci_1564: miscellaneous fixes and cleanups

[Chase Southwood]

This patchset moves a misplaced include to the proper file, swaps out an overly
aggressive placement of apci1564_reset(), and cleans up apci1564_interrupt().

Chase Southwood (3):
  staging: comedi: addi_apci_1564: move addi_watchdog.h include to
    addi_apci_1564.c
  staging: comedi: addi_apci_1564: fix use of apci1564_reset() to
    disable DI interrupts
  staging: comedi: addi_apci_1564: clean up apci1564_interrupt()

 .../comedi/drivers/addi-data/hwdrv_apci1564.c      |   2 -
 drivers/staging/comedi/drivers/addi_apci_1564.c    | 139 +++++----------------
 2 files changed, 32 insertions(+), 109 deletions(-)

-- 
2.0.1

[PATCH v2 1/3] staging: comedi: addi_apci_1564: move addi_watchdog.h include to addi_apci_1564.c

[Chase Southwood]

Commit aed3f9d (staging: comedi: addi_apci_1564: absorb apci1564_reset()) moved
the only use of addi_watchdog.h from hwdrv_apci1564.c to addi_apci_1564.c, but
left the include statement itself in the former file.  Move this include to the
file which actually uses it.

Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c | 2 --
 drivers/staging/comedi/drivers/addi_apci_1564.c           | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 4007fd2..7326f3a 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -21,8 +21,6 @@
  *
  */
 
-#include "../addi_watchdog.h"
-
 #define APCI1564_ADDRESS_RANGE				128
 
 /* Digital Input IRQ Function Selection */
diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index f71ee02..59786e7 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -4,6 +4,7 @@
 #include "../comedidev.h"
 #include "comedi_fc.h"
 #include "amcc_s5933.h"
+#include "addi_watchdog.h"
 
 #include "addi-data/addi_common.h"
 
-- 
2.0.1

[PATCH v2 2/3] staging: comedi: addi_apci_1564: fix use of apci1564_reset() to disable DI interrupts

[Chase Southwood]

apci1564_cos_insn_config() is currently using apci1564_reset() to disable
digital input interrupts when the configuration operation is
COMEDI_DIGITAL_TRIG_DISABLE.  However, this is incorrect as the device reset
function also resets the registers for the digital outputs, timer, watchdog, and
counters as well.  Replace the reset function call with a direct disabling of
just the digital input interrupts.

Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/drivers/addi_apci_1564.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 59786e7..0141ed9 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -285,7 +285,10 @@ static int apci1564_cos_insn_config(struct comedi_device *dev,
 			devpriv->ctrl = 0;
 			devpriv->mode1 = 0;
 			devpriv->mode2 = 0;
-			apci1564_reset(dev);
+			outl(0x0, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+			inl(devpriv->amcc_iobase + APCI1564_DI_INT_STATUS_REG);
+			outl(0x0, devpriv->amcc_iobase + APCI1564_DI_INT_MODE1_REG);
+			outl(0x0, devpriv->amcc_iobase + APCI1564_DI_INT_MODE2_REG);
 			break;
 		case COMEDI_DIGITAL_TRIG_ENABLE_EDGES:
 			if (devpriv->ctrl != (APCI1564_DI_INT_ENABLE |
-- 
2.0.1

[PATCH v2 3/3] staging: comedi: addi_apci_1564: clean up apci1564_interrupt()

[Chase Southwood]

Remove the checks for interrupts from unknown sources.  This situation
should never occur and the checks were doing nothing to help the
situation.

Also, the portion of the function for handling counter interrupts is
reapeated four times (once for each counter), but is completely identical
save for the register is is accessing, so we can handle all four counters
with a for loop.

Finally, the interrupt handler is incorrectly setting and then checking
devpriv->timer_select_mode before processing some of the triggered
interrupts, so just remove all occurrences of this.

Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/staging/comedi/drivers/addi_apci_1564.c | 133 +++++-------------------
 1 file changed, 27 insertions(+), 106 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 0141ed9..5924421 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -58,48 +58,33 @@ static irqreturn_t apci1564_interrupt(int irq, void *d)
 	struct comedi_device *dev = d;
 	struct apci1564_private *devpriv = dev->private;
 	struct comedi_subdevice *s = dev->read_subdev;
-	unsigned int ui_DO, ui_DI;
-	unsigned int ui_Timer;
-	unsigned int ui_C1, ui_C2, ui_C3, ui_C4;
-	unsigned int ul_Command2 = 0;
+	unsigned int status;
+	unsigned int ctrl;
+	unsigned int chan;
 
 	/* check interrupt is from this device */
 	if ((inl(devpriv->amcc_iobase + AMCC_OP_REG_INTCSR) &
 	     INTCSR_INTR_ASSERTED) == 0)
 		return IRQ_NONE;
 
-	/* check which interrupt was triggered */
-	ui_DI = inl(devpriv->amcc_iobase + APCI1564_DI_IRQ_REG) &
-		   APCI1564_DI_INT_ENABLE;
-	ui_DO = inl(devpriv->amcc_iobase + APCI1564_DO_IRQ_REG) & 0x01;
-	ui_Timer = inl(devpriv->amcc_iobase + APCI1564_TIMER_IRQ_REG) & 0x01;
-	ui_C1 =
-		inl(dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER1)) & 0x1;
-	ui_C2 =
-		inl(dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER2)) & 0x1;
-	ui_C3 =
-		inl(dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER3)) & 0x1;
-	ui_C4 =
-		inl(dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER4)) & 0x1;
-	if (ui_DI == 0 && ui_DO == 0 && ui_Timer == 0 && ui_C1 == 0
-		&& ui_C2 == 0 && ui_C3 == 0 && ui_C4 == 0) {
-		return IRQ_HANDLED;
-	}
-
-	if (ui_DI) {
+	status = inl(devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+	if (status & APCI1564_DI_INT_ENABLE) {
 		/* disable the interrupt */
-		outl(ui_DI & APCI1564_DI_INT_DISABLE, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+		outl(status & APCI1564_DI_INT_DISABLE,
+		     devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
 
-		s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG) & 0xffff;
+		s->state = inl(dev->iobase + APCI1564_DI_INT_STATUS_REG)
+			       & 0xffff;
 		comedi_buf_put(s, s->state);
 		s->async->events |= COMEDI_CB_BLOCK | COMEDI_CB_EOS;
 		comedi_event(dev, s);
 
 		/* enable the interrupt */
-		outl(ui_DI, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+		outl(status, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
 	}
 
-	if (ui_DO == 1) {
+	status = inl(devpriv->amcc_iobase + APCI1564_DO_IRQ_REG);
+	if (status & 0x01) {
 		/* Check for Digital Output interrupt Type */
 		/* 1: VCC interrupt			   */
 		/* 2: CC interrupt			   */
@@ -112,98 +97,34 @@ static irqreturn_t apci1564_interrupt(int irq, void *d)
 		send_sig(SIGIO, devpriv->tsk_current, 0);
 	}
 
-	if (ui_Timer == 1) {
-		devpriv->timer_select_mode = ADDIDATA_TIMER;
-		if (devpriv->timer_select_mode) {
-
-			/*  Disable Timer Interrupt */
-			ul_Command2 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
-			outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
-
-			/* Send a signal to from kernel to user space */
-			send_sig(SIGIO, devpriv->tsk_current, 0);
-
-			/*  Enable Timer Interrupt */
-
-			outl(ul_Command2, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
-		}
-	}
-
-	if (ui_C1 == 1) {
-		devpriv->timer_select_mode = ADDIDATA_COUNTER;
-		if (devpriv->timer_select_mode) {
-
-			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
-
-			/* Send a signal to from kernel to user space */
-			send_sig(SIGIO, devpriv->tsk_current, 0);
-
-			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
-		}
-	}
-
-	if (ui_C2 == 1) {
-		devpriv->timer_select_mode = ADDIDATA_COUNTER;
-		if (devpriv->timer_select_mode) {
+	status = inl(devpriv->amcc_iobase + APCI1564_TIMER_IRQ_REG);
+	if (status & 0x01) {
+		/*  Disable Timer Interrupt */
+		ctrl = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
+		outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
 
-			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER2));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER2));
-
-			/* Send a signal to from kernel to user space */
-			send_sig(SIGIO, devpriv->tsk_current, 0);
+		/* Send a signal to from kernel to user space */
+		send_sig(SIGIO, devpriv->tsk_current, 0);
 
-			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER2));
-		}
+		/*  Enable Timer Interrupt */
+		outl(ctrl, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
 	}
 
-	if (ui_C3 == 1) {
-		devpriv->timer_select_mode = ADDIDATA_COUNTER;
-		if (devpriv->timer_select_mode) {
-
+	for (chan = 0; chan < 4; chan++) {
+		status = inl(dev->iobase + APCI1564_TCW_IRQ_REG(chan));
+		if (status & 0x01) {
 			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
+			ctrl = inl(dev->iobase + APCI1564_TCW_CTRL_REG(chan));
+			outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(chan));
 
 			/* Send a signal to from kernel to user space */
 			send_sig(SIGIO, devpriv->tsk_current, 0);
 
 			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
+			outl(ctrl, dev->iobase + APCI1564_TCW_CTRL_REG(chan));
 		}
 	}
 
-	if (ui_C4 == 1) {
-		devpriv->timer_select_mode = ADDIDATA_COUNTER;
-		if (devpriv->timer_select_mode) {
-
-			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
-
-			/* Send a signal to from kernel to user space */
-			send_sig(SIGIO, devpriv->tsk_current, 0);
-
-			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
-		}
-	}
 	return IRQ_HANDLED;
 }
 
-- 
2.0.1

Re: [PATCH v2 0/3] staging: comedi: addi_apci_1564: miscellaneous fixes and cleanups

[Ian Abbott]

On 2014-07-03 03:15, Chase Southwood wrote:
> This patchset moves a misplaced include to the proper file, swaps out an overly
> aggressive placement of apci1564_reset(), and cleans up apci1564_interrupt().
>
> Chase Southwood (3):
>    staging: comedi: addi_apci_1564: move addi_watchdog.h include to
>      addi_apci_1564.c
>    staging: comedi: addi_apci_1564: fix use of apci1564_reset() to
>      disable DI interrupts
>    staging: comedi: addi_apci_1564: clean up apci1564_interrupt()
>
>   .../comedi/drivers/addi-data/hwdrv_apci1564.c      |   2 -
>   drivers/staging/comedi/drivers/addi_apci_1564.c    | 139 +++++----------------
>   2 files changed, 32 insertions(+), 109 deletions(-)
>

Looks good!  You didn't list the v2 changes though.  Maybe you could 
summarize them here?

Reviewed-by: Ian Abbott <abbotti@mev.co.uk.>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v2 0/3] staging: comedi: addi_apci_1564: miscellaneous fixes and cleanups

[Chase Southwood]

On Thu, Jul 3, 2014 at 4:39 AM, Ian Abbott <abbotti@mev.co.uk> wrote:
> On 2014-07-03 03:15, Chase Southwood wrote:
>>
>> This patchset moves a misplaced include to the proper file, swaps out an
>> overly
>> aggressive placement of apci1564_reset(), and cleans up
>> apci1564_interrupt().
>>
>> Chase Southwood (3):
>>    staging: comedi: addi_apci_1564: move addi_watchdog.h include to
>>      addi_apci_1564.c
>>    staging: comedi: addi_apci_1564: fix use of apci1564_reset() to
>>      disable DI interrupts
>>    staging: comedi: addi_apci_1564: clean up apci1564_interrupt()
>>
>>   .../comedi/drivers/addi-data/hwdrv_apci1564.c      |   2 -
>>   drivers/staging/comedi/drivers/addi_apci_1564.c    | 139
>> +++++----------------
>>   2 files changed, 32 insertions(+), 109 deletions(-)
>>
>
> Looks good!  You didn't list the v2 changes though.  Maybe you could
> summarize them here?

I always forget to do _something_, don't I?  Here are the changes:

CHANGES FROM V1:
*Patches 1 and 2 did not change.
*In Patch 3, check for interrupts from unknown sources has been removed.
*Individual status variables for the subdevices in the interrupt
handler have been swapped out in favor of a single status variable
that is reused for all subdevices.

Thanks,
Chase
>
> Reviewed-by: Ian Abbott <abbotti@mev.co.uk.>
>
>
> --
> -=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
> -=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-