* [PATCH 1/6] staging: comedi: addi_apci_1564: remove send_sig() use
2014-05-30 4:40 [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality Chase Southwood
@ 2014-05-30 4:41 ` Chase Southwood
2014-05-30 16:37 ` Hartley Sweeten
2014-06-18 22:07 ` Greg KH
2014-05-30 4:42 ` [PATCH 2/6] staging: comedi: addi_apci_1564: remove use of devpriv->b_OutputMemoryStatus Chase Southwood
` (5 subsequent siblings)
6 siblings, 2 replies; 16+ messages in thread
From: Chase Southwood @ 2014-05-30 4:41 UTC (permalink / raw)
To: gregkh; +Cc: abbotti, hsweeten, devel, linux-kernel, Chase Southwood
The addi-data drivers use send_sig() to let the user know when an
interrupt has occurred. The "standard" way to do this in the comedi
subsystem is to have a subdevice that supports asynchronous commands
and use comedi_event() to signal the user.
Remove the send_sig() usage in this driver.
Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
.../comedi/drivers/addi-data/hwdrv_apci1564.c | 23 ----------------------
1 file changed, 23 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 0ba5385..be0e009 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -108,8 +108,6 @@ static int apci1564_di_config(struct comedi_device *dev,
{
struct addi_private *devpriv = dev->private;
- devpriv->tsk_Current = current;
-
/* Set the digital input logic */
if (data[0] == ADDIDATA_ENABLE) {
data[2] = data[2] << 4;
@@ -166,7 +164,6 @@ static int apci1564_do_config(struct comedi_device *dev,
outl(ul_Command, devpriv->i_IobaseAmcc + APCI1564_DO_INT_CTRL_REG);
ui_InterruptData = inl(devpriv->i_IobaseAmcc + APCI1564_DO_INT_CTRL_REG);
- devpriv->tsk_Current = current;
return insn->n;
}
@@ -189,7 +186,6 @@ static int apci1564_timer_config(struct comedi_device *dev,
struct addi_private *devpriv = dev->private;
unsigned int ul_Command1 = 0;
- devpriv->tsk_Current = current;
if (data[0] == ADDIDATA_WATCHDOG) {
devpriv->b_TimerSelectMode = ADDIDATA_WATCHDOG;
@@ -436,8 +432,6 @@ static void apci1564_interrupt(int irq, void *d)
ui_InterruptStatus_1564 =
inl(devpriv->i_IobaseAmcc + APCI1564_DI_INT_STATUS_REG);
ui_InterruptStatus_1564 = ui_InterruptStatus_1564 & 0X000FFFF0;
- /* send signal to the sample */
- send_sig(SIGIO, devpriv->tsk_Current, 0);
/* enable the interrupt */
outl(ui_DI, devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG);
return;
@@ -451,8 +445,6 @@ static void apci1564_interrupt(int irq, void *d)
/* Disable the Interrupt */
outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DO_INT_CTRL_REG);
- /* Sends signal to user space */
- send_sig(SIGIO, devpriv->tsk_Current, 0);
}
if (ui_Timer == 1) {
@@ -463,9 +455,6 @@ static void apci1564_interrupt(int irq, void *d)
ul_Command2 = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
outl(0x0, devpriv->i_IobaseAmcc + 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->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
@@ -482,9 +471,6 @@ static void apci1564_interrupt(int irq, void *d)
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));
@@ -501,9 +487,6 @@ static void apci1564_interrupt(int irq, void *d)
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));
@@ -520,9 +503,6 @@ static void apci1564_interrupt(int irq, void *d)
outl(0x0,
dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
- /* 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));
@@ -539,9 +519,6 @@ static void apci1564_interrupt(int irq, void *d)
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));
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* RE: [PATCH 1/6] staging: comedi: addi_apci_1564: remove send_sig() use
2014-05-30 4:41 ` [PATCH 1/6] staging: comedi: addi_apci_1564: remove send_sig() use Chase Southwood
@ 2014-05-30 16:37 ` Hartley Sweeten
2014-06-18 22:07 ` Greg KH
1 sibling, 0 replies; 16+ messages in thread
From: Hartley Sweeten @ 2014-05-30 16:37 UTC (permalink / raw)
To: Chase Southwood, gregkh@linuxfoundation.org
Cc: abbotti@mev.co.uk, devel@driverdev.osuosl.org,
linux-kernel@vger.kernel.org
On Thursday, May 29, 2014 9:42 PM, Chase Southwood wrote:
> The addi-data drivers use send_sig() to let the user know when an
> interrupt has occurred. The "standard" way to do this in the comedi
> subsystem is to have a subdevice that supports asynchronous commands
> and use comedi_event() to signal the user.
>
> Remove the send_sig() usage in this driver.
>
> Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> ---
> .../comedi/drivers/addi-data/hwdrv_apci1564.c | 23 ----------------------
> 1 file changed, 23 deletions(-)
Chase,
Normally this patch might be a problem.
With this patch you remove the mechanism that the driver uses to signal the
user that an event has occurred without providing a new mechanism. In theory
this "breaks" the driver.
In reality, the driver is already "broke" since the interrupts are currently enabled
using the digital input subdevice (*insn_config) callback and that callback does not
follow the comedi API. Currently the apci1564_di_config() function is using the
data[0] value as a flag to enable/disable the interrupt instead of using it as the
comedi configuration instruction. It is expecting data[0] to be 1 or 0, when it is 1
it expects the data to include 3 additional parameters but the core will sanity check
the instruction length and return -EINVAL if insn->n != 1. You fix this in patch 5/6.
Because the driver is already "broke" I guess this patch is ok.
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/6] staging: comedi: addi_apci_1564: remove send_sig() use
2014-05-30 4:41 ` [PATCH 1/6] staging: comedi: addi_apci_1564: remove send_sig() use Chase Southwood
2014-05-30 16:37 ` Hartley Sweeten
@ 2014-06-18 22:07 ` Greg KH
1 sibling, 0 replies; 16+ messages in thread
From: Greg KH @ 2014-06-18 22:07 UTC (permalink / raw)
To: Chase Southwood; +Cc: abbotti, hsweeten, devel, linux-kernel
On Thu, May 29, 2014 at 11:41:53PM -0500, Chase Southwood wrote:
> The addi-data drivers use send_sig() to let the user know when an
> interrupt has occurred. The "standard" way to do this in the comedi
> subsystem is to have a subdevice that supports asynchronous commands
> and use comedi_event() to signal the user.
>
> Remove the send_sig() usage in this driver.
>
> Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> ---
> .../comedi/drivers/addi-data/hwdrv_apci1564.c | 23 ----------------------
> 1 file changed, 23 deletions(-)
This doesn't apply to my tree :(
Can you refresh this series based on the review comments for the later
patches and please resend?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/6] staging: comedi: addi_apci_1564: remove use of devpriv->b_OutputMemoryStatus
2014-05-30 4:40 [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality Chase Southwood
2014-05-30 4:41 ` [PATCH 1/6] staging: comedi: addi_apci_1564: remove send_sig() use Chase Southwood
@ 2014-05-30 4:42 ` Chase Southwood
2014-05-30 16:38 ` Hartley Sweeten
2014-05-30 4:42 ` [PATCH 3/6] staging: comedi: addi_apci_1564: introduce apci1564_private struct Chase Southwood
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Chase Southwood @ 2014-05-30 4:42 UTC (permalink / raw)
To: gregkh; +Cc: abbotti, hsweeten, devel, linux-kernel, Chase Southwood
This member of the private data struct is only set at one location in the
entire driver, and then never even used for anything. Let's just remove
its use.
Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeeten <hsweeten@visionengravers.com>
---
drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index be0e009..f974bd2 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -147,11 +147,6 @@ static int apci1564_do_config(struct comedi_device *dev,
return -EINVAL;
}
- if (data[0])
- devpriv->b_OutputMemoryStatus = ADDIDATA_ENABLE;
- else
- devpriv->b_OutputMemoryStatus = ADDIDATA_DISABLE;
-
if (data[1] == ADDIDATA_ENABLE)
ul_Command = ul_Command | 0x1;
else
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 3/6] staging: comedi: addi_apci_1564: introduce apci1564_private struct
2014-05-30 4:40 [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality Chase Southwood
2014-05-30 4:41 ` [PATCH 1/6] staging: comedi: addi_apci_1564: remove send_sig() use Chase Southwood
2014-05-30 4:42 ` [PATCH 2/6] staging: comedi: addi_apci_1564: remove use of devpriv->b_OutputMemoryStatus Chase Southwood
@ 2014-05-30 4:42 ` Chase Southwood
2014-05-30 16:44 ` Hartley Sweeten
2014-05-30 4:43 ` [PATCH 4/6] staging: comedi: addi_apci_1564: add a subdevice for Change-of-State interrupt support Chase Southwood
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Chase Southwood @ 2014-05-30 4:42 UTC (permalink / raw)
To: gregkh; +Cc: abbotti, hsweeten, devel, linux-kernel, Chase Southwood
The addi_private struct defined in addi-data/addi_common.h is very bloated
and contains many fields which addi_apci_1564 does not require. In the
interest of eventually removing this driver's dependency on
addi_common.h, we can create a private data struct specifically for
addi_apci_1564 containing only the fields it will actually use.
Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
.../comedi/drivers/addi-data/hwdrv_apci1564.c | 164 +++++++++++----------
drivers/staging/comedi/drivers/addi_apci_1564.c | 34 ++---
2 files changed, 102 insertions(+), 96 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index f974bd2..054e731 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -49,7 +49,7 @@
#define APCI1564_COUNTER4 3
/*
- * devpriv->i_IobaseAmcc Register Map
+ * devpriv->amcc_iobase Register Map
*/
#define APCI1564_DI_REG 0x04
#define APCI1564_DI_INT_MODE1_REG 0x08
@@ -93,6 +93,12 @@
static unsigned int ui_InterruptStatus_1564;
static unsigned int ui_InterruptData, ui_Type;
+struct apci1564_private {
+ unsigned int amcc_iobase; /* base of AMCC I/O registers */
+ unsigned char timer_select_mode;
+ unsigned char mode_select_register;
+};
+
/*
* Configures the digital input Subdevice
*
@@ -106,22 +112,22 @@ static int apci1564_di_config(struct comedi_device *dev,
struct comedi_insn *insn,
unsigned int *data)
{
- struct addi_private *devpriv = dev->private;
+ struct apci1564_private *devpriv = dev->private;
/* Set the digital input logic */
if (data[0] == ADDIDATA_ENABLE) {
data[2] = data[2] << 4;
data[3] = data[3] << 4;
- outl(data[2], devpriv->i_IobaseAmcc + APCI1564_DI_INT_MODE1_REG);
- outl(data[3], devpriv->i_IobaseAmcc + APCI1564_DI_INT_MODE2_REG);
+ outl(data[2], devpriv->amcc_iobase + APCI1564_DI_INT_MODE1_REG);
+ outl(data[3], devpriv->amcc_iobase + APCI1564_DI_INT_MODE2_REG);
if (data[1] == ADDIDATA_OR)
- outl(0x4, devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG);
+ outl(0x4, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
else
- outl(0x6, devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG);
+ outl(0x6, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
} else {
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DI_INT_MODE1_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DI_INT_MODE2_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DI_INT_MODE1_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DI_INT_MODE2_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
}
return insn->n;
@@ -138,7 +144,7 @@ static int apci1564_do_config(struct comedi_device *dev,
struct comedi_insn *insn,
unsigned int *data)
{
- struct addi_private *devpriv = dev->private;
+ struct apci1564_private *devpriv = dev->private;
unsigned int ul_Command = 0;
if ((data[0] != 0) && (data[0] != 1)) {
@@ -157,8 +163,8 @@ static int apci1564_do_config(struct comedi_device *dev,
else
ul_Command = ul_Command & 0xFFFFFFFD;
- outl(ul_Command, devpriv->i_IobaseAmcc + APCI1564_DO_INT_CTRL_REG);
- ui_InterruptData = inl(devpriv->i_IobaseAmcc + APCI1564_DO_INT_CTRL_REG);
+ outl(ul_Command, devpriv->amcc_iobase + APCI1564_DO_INT_CTRL_REG);
+ ui_InterruptData = inl(devpriv->amcc_iobase + APCI1564_DO_INT_CTRL_REG);
return insn->n;
}
@@ -178,30 +184,30 @@ static int apci1564_timer_config(struct comedi_device *dev,
struct comedi_insn *insn,
unsigned int *data)
{
- struct addi_private *devpriv = dev->private;
+ struct apci1564_private *devpriv = dev->private;
unsigned int ul_Command1 = 0;
if (data[0] == ADDIDATA_WATCHDOG) {
- devpriv->b_TimerSelectMode = ADDIDATA_WATCHDOG;
+ devpriv->timer_select_mode = ADDIDATA_WATCHDOG;
/* Disable the watchdog */
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_WDOG_CTRL_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_WDOG_CTRL_REG);
/* Loading the Reload value */
- outl(data[3], devpriv->i_IobaseAmcc + APCI1564_WDOG_RELOAD_REG);
+ outl(data[3], devpriv->amcc_iobase + APCI1564_WDOG_RELOAD_REG);
} else if (data[0] == ADDIDATA_TIMER) {
/* First Stop The Timer */
- ul_Command1 = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ ul_Command1 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
/* Stop The Timer */
- outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ outl(ul_Command1, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
- devpriv->b_TimerSelectMode = ADDIDATA_TIMER;
+ devpriv->timer_select_mode = ADDIDATA_TIMER;
if (data[1] == 1) {
/* Enable TIMER int & DISABLE ALL THE OTHER int SOURCES */
- outl(0x02, devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DO_IRQ_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_WDOG_IRQ_REG);
+ outl(0x02, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DO_IRQ_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_WDOG_IRQ_REG);
outl(0x0,
dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER1));
outl(0x0,
@@ -212,22 +218,22 @@ static int apci1564_timer_config(struct comedi_device *dev,
dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER4));
} else {
/* disable Timer interrupt */
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
}
/* Loading Timebase */
- outl(data[2], devpriv->i_IobaseAmcc + APCI1564_TIMER_TIMEBASE_REG);
+ outl(data[2], devpriv->amcc_iobase + APCI1564_TIMER_TIMEBASE_REG);
/* Loading the Reload value */
- outl(data[3], devpriv->i_IobaseAmcc + APCI1564_TIMER_RELOAD_REG);
+ outl(data[3], devpriv->amcc_iobase + APCI1564_TIMER_RELOAD_REG);
- ul_Command1 = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ ul_Command1 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
ul_Command1 = (ul_Command1 & 0xFFF719E2UL) | 2UL << 13UL | 0x10UL;
/* mode 2 */
- outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ outl(ul_Command1, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
} else if (data[0] == ADDIDATA_COUNTER) {
- devpriv->b_TimerSelectMode = ADDIDATA_COUNTER;
- devpriv->b_ModeSelectRegister = data[5];
+ devpriv->timer_select_mode = ADDIDATA_COUNTER;
+ devpriv->mode_select_register = data[5];
/* First Stop The Counter */
ul_Command1 = inl(dev->iobase + APCI1564_TCW_CTRL_REG(data[5] - 1));
@@ -276,45 +282,45 @@ static int apci1564_timer_write(struct comedi_device *dev,
struct comedi_insn *insn,
unsigned int *data)
{
- struct addi_private *devpriv = dev->private;
+ struct apci1564_private *devpriv = dev->private;
unsigned int ul_Command1 = 0;
- if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
+ if (devpriv->timer_select_mode == ADDIDATA_WATCHDOG) {
switch (data[1]) {
case 0: /* stop the watchdog */
/* disable the watchdog */
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_WDOG_CTRL_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_WDOG_CTRL_REG);
break;
case 1: /* start the watchdog */
- outl(0x0001, devpriv->i_IobaseAmcc + APCI1564_WDOG_CTRL_REG);
+ outl(0x0001, devpriv->amcc_iobase + APCI1564_WDOG_CTRL_REG);
break;
case 2: /* Software trigger */
- outl(0x0201, devpriv->i_IobaseAmcc + APCI1564_WDOG_CTRL_REG);
+ outl(0x0201, devpriv->amcc_iobase + APCI1564_WDOG_CTRL_REG);
break;
default:
dev_err(dev->class_dev, "Specified functionality does not exist.\n");
return -EINVAL;
}
}
- if (devpriv->b_TimerSelectMode == ADDIDATA_TIMER) {
+ if (devpriv->timer_select_mode == ADDIDATA_TIMER) {
if (data[1] == 1) {
- ul_Command1 = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ ul_Command1 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
ul_Command1 = (ul_Command1 & 0xFFFFF9FFUL) | 0x1UL;
/* Enable the Timer */
- outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ outl(ul_Command1, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
} else if (data[1] == 0) {
/* Stop The Timer */
- ul_Command1 = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ ul_Command1 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
- outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ outl(ul_Command1, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
}
}
- if (devpriv->b_TimerSelectMode == ADDIDATA_COUNTER) {
+ if (devpriv->timer_select_mode == ADDIDATA_COUNTER) {
ul_Command1 =
inl(dev->iobase +
- APCI1564_TCW_CTRL_REG(devpriv->b_ModeSelectRegister - 1));
+ APCI1564_TCW_CTRL_REG(devpriv->mode_select_register - 1));
if (data[1] == 1) {
/* Start the Counter subdevice */
ul_Command1 = (ul_Command1 & 0xFFFFF9FFUL) | 0x1UL;
@@ -327,7 +333,7 @@ static int apci1564_timer_write(struct comedi_device *dev,
ul_Command1 = (ul_Command1 & 0xFFFFF9FFUL) | 0x400;
}
outl(ul_Command1, dev->iobase +
- APCI1564_TCW_CTRL_REG(devpriv->b_ModeSelectRegister - 1));
+ APCI1564_TCW_CTRL_REG(devpriv->mode_select_register - 1));
}
return insn->n;
}
@@ -340,27 +346,27 @@ static int apci1564_timer_read(struct comedi_device *dev,
struct comedi_insn *insn,
unsigned int *data)
{
- struct addi_private *devpriv = dev->private;
+ struct apci1564_private *devpriv = dev->private;
unsigned int ul_Command1 = 0;
- if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
+ if (devpriv->timer_select_mode == ADDIDATA_WATCHDOG) {
/* Stores the status of the Watchdog */
- data[0] = inl(devpriv->i_IobaseAmcc + APCI1564_WDOG_STATUS_REG) & 0x1;
- data[1] = inl(devpriv->i_IobaseAmcc + APCI1564_WDOG_REG);
- } else if (devpriv->b_TimerSelectMode == ADDIDATA_TIMER) {
+ data[0] = inl(devpriv->amcc_iobase + APCI1564_WDOG_STATUS_REG) & 0x1;
+ data[1] = inl(devpriv->amcc_iobase + APCI1564_WDOG_REG);
+ } else if (devpriv->timer_select_mode == ADDIDATA_TIMER) {
/* Stores the status of the Timer */
- data[0] = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER_STATUS_REG) & 0x1;
+ data[0] = inl(devpriv->amcc_iobase + APCI1564_TIMER_STATUS_REG) & 0x1;
/* Stores the Actual value of the Timer */
- data[1] = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER_REG);
- } else if (devpriv->b_TimerSelectMode == ADDIDATA_COUNTER) {
+ data[1] = inl(devpriv->amcc_iobase + APCI1564_TIMER_REG);
+ } else if (devpriv->timer_select_mode == ADDIDATA_COUNTER) {
/* Read the Counter Actual Value. */
data[0] =
inl(dev->iobase +
- APCI1564_TCW_REG(devpriv->b_ModeSelectRegister - 1));
+ APCI1564_TCW_REG(devpriv->mode_select_register - 1));
ul_Command1 =
inl(dev->iobase +
- APCI1564_TCW_STATUS_REG(devpriv->b_ModeSelectRegister - 1));
+ APCI1564_TCW_STATUS_REG(devpriv->mode_select_register - 1));
/* Get the software trigger status */
data[1] = (unsigned char) ((ul_Command1 >> 1) & 1);
@@ -373,9 +379,9 @@ static int apci1564_timer_read(struct comedi_device *dev,
/* Get the overflow status */
data[4] = (unsigned char) ((ul_Command1 >> 0) & 1);
- } else if ((devpriv->b_TimerSelectMode != ADDIDATA_TIMER)
- && (devpriv->b_TimerSelectMode != ADDIDATA_WATCHDOG)
- && (devpriv->b_TimerSelectMode != ADDIDATA_COUNTER)) {
+ } else if ((devpriv->timer_select_mode != ADDIDATA_TIMER)
+ && (devpriv->timer_select_mode != ADDIDATA_WATCHDOG)
+ && (devpriv->timer_select_mode != ADDIDATA_COUNTER)) {
dev_err(dev->class_dev, "Invalid Subdevice!\n");
}
return insn->n;
@@ -399,15 +405,15 @@ static int apci1564_do_read(struct comedi_device *dev,
static void apci1564_interrupt(int irq, void *d)
{
struct comedi_device *dev = d;
- struct addi_private *devpriv = dev->private;
+ struct apci1564_private *devpriv = dev->private;
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;
- ui_DI = inl(devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG) & 0x01;
- ui_DO = inl(devpriv->i_IobaseAmcc + APCI1564_DO_IRQ_REG) & 0x01;
- ui_Timer = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER_IRQ_REG) & 0x01;
+ ui_DI = inl(devpriv->amcc_iobase + APCI1564_DI_IRQ_REG) & 0x01;
+ 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 =
@@ -422,13 +428,13 @@ static void apci1564_interrupt(int irq, void *d)
}
if (ui_DI == 1) {
- ui_DI = inl(devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG);
+ ui_DI = inl(devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
ui_InterruptStatus_1564 =
- inl(devpriv->i_IobaseAmcc + APCI1564_DI_INT_STATUS_REG);
+ inl(devpriv->amcc_iobase + APCI1564_DI_INT_STATUS_REG);
ui_InterruptStatus_1564 = ui_InterruptStatus_1564 & 0X000FFFF0;
/* enable the interrupt */
- outl(ui_DI, devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG);
+ outl(ui_DI, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
return;
}
@@ -436,29 +442,29 @@ static void apci1564_interrupt(int irq, void *d)
/* Check for Digital Output interrupt Type */
/* 1: VCC interrupt */
/* 2: CC interrupt */
- ui_Type = inl(devpriv->i_IobaseAmcc + APCI1564_DO_INT_STATUS_REG) & 0x3;
+ ui_Type = inl(devpriv->amcc_iobase + APCI1564_DO_INT_STATUS_REG) & 0x3;
/* Disable the Interrupt */
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DO_INT_CTRL_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DO_INT_CTRL_REG);
}
if (ui_Timer == 1) {
- devpriv->b_TimerSelectMode = ADDIDATA_TIMER;
- if (devpriv->b_TimerSelectMode) {
+ devpriv->timer_select_mode = ADDIDATA_TIMER;
+ if (devpriv->timer_select_mode) {
/* Disable Timer Interrupt */
- ul_Command2 = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ ul_Command2 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
/* Enable Timer Interrupt */
- outl(ul_Command2, devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
+ outl(ul_Command2, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
}
}
if (ui_C1 == 1) {
- devpriv->b_TimerSelectMode = ADDIDATA_COUNTER;
- if (devpriv->b_TimerSelectMode) {
+ devpriv->timer_select_mode = ADDIDATA_COUNTER;
+ if (devpriv->timer_select_mode) {
/* Disable Counter Interrupt */
ul_Command2 =
@@ -473,8 +479,8 @@ static void apci1564_interrupt(int irq, void *d)
}
if (ui_C2 == 1) {
- devpriv->b_TimerSelectMode = ADDIDATA_COUNTER;
- if (devpriv->b_TimerSelectMode) {
+ devpriv->timer_select_mode = ADDIDATA_COUNTER;
+ if (devpriv->timer_select_mode) {
/* Disable Counter Interrupt */
ul_Command2 =
@@ -489,8 +495,8 @@ static void apci1564_interrupt(int irq, void *d)
}
if (ui_C3 == 1) {
- devpriv->b_TimerSelectMode = ADDIDATA_COUNTER;
- if (devpriv->b_TimerSelectMode) {
+ devpriv->timer_select_mode = ADDIDATA_COUNTER;
+ if (devpriv->timer_select_mode) {
/* Disable Counter Interrupt */
ul_Command2 =
@@ -505,8 +511,8 @@ static void apci1564_interrupt(int irq, void *d)
}
if (ui_C4 == 1) {
- devpriv->b_TimerSelectMode = ADDIDATA_COUNTER;
- if (devpriv->b_TimerSelectMode) {
+ devpriv->timer_select_mode = ADDIDATA_COUNTER;
+ if (devpriv->timer_select_mode) {
/* Disable Counter Interrupt */
ul_Command2 =
diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 13d9962..5901143 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -19,9 +19,9 @@ static int apci1564_di_insn_bits(struct comedi_device *dev,
struct comedi_insn *insn,
unsigned int *data)
{
- struct addi_private *devpriv = dev->private;
+ struct apci1564_private *devpriv = dev->private;
- data[1] = inl(devpriv->i_IobaseAmcc + APCI1564_DI_REG);
+ data[1] = inl(devpriv->amcc_iobase + APCI1564_DI_REG);
return insn->n;
}
@@ -31,12 +31,12 @@ static int apci1564_do_insn_bits(struct comedi_device *dev,
struct comedi_insn *insn,
unsigned int *data)
{
- struct addi_private *devpriv = dev->private;
+ struct apci1564_private *devpriv = dev->private;
- s->state = inl(devpriv->i_IobaseAmcc + APCI1564_DO_REG);
+ s->state = inl(devpriv->amcc_iobase + APCI1564_DO_REG);
if (comedi_dio_update_state(s, data))
- outl(s->state, devpriv->i_IobaseAmcc + APCI1564_DO_REG);
+ outl(s->state, devpriv->amcc_iobase + APCI1564_DO_REG);
data[1] = s->state;
@@ -45,26 +45,26 @@ static int apci1564_do_insn_bits(struct comedi_device *dev,
static int apci1564_reset(struct comedi_device *dev)
{
- struct addi_private *devpriv = dev->private;
+ struct apci1564_private *devpriv = dev->private;
ui_Type = 0;
/* Disable the input interrupts and reset status register */
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DI_IRQ_REG);
- inl(devpriv->i_IobaseAmcc + APCI1564_DI_INT_STATUS_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DI_INT_MODE1_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DI_INT_MODE2_REG);
+ 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);
/* Reset the output channels and disable interrupts */
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DO_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DO_INT_CTRL_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DO_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DO_INT_CTRL_REG);
/* Reset the watchdog registers */
- addi_watchdog_reset(devpriv->i_IobaseAmcc + APCI1564_WDOG_REG);
+ addi_watchdog_reset(devpriv->amcc_iobase + APCI1564_WDOG_REG);
/* Reset the timer registers */
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER_CTRL_REG);
- outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER_RELOAD_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_RELOAD_REG);
/* Reset the counter registers */
outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
@@ -79,7 +79,7 @@ static int apci1564_auto_attach(struct comedi_device *dev,
unsigned long context_unused)
{
struct pci_dev *pcidev = comedi_to_pci_dev(dev);
- struct addi_private *devpriv;
+ struct apci1564_private *devpriv;
struct comedi_subdevice *s;
int ret;
@@ -94,7 +94,7 @@ static int apci1564_auto_attach(struct comedi_device *dev,
return ret;
dev->iobase = pci_resource_start(pcidev, 1);
- devpriv->i_IobaseAmcc = pci_resource_start(pcidev, 0);
+ devpriv->amcc_iobase = pci_resource_start(pcidev, 0);
apci1564_reset(dev);
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* RE: [PATCH 3/6] staging: comedi: addi_apci_1564: introduce apci1564_private struct
2014-05-30 4:42 ` [PATCH 3/6] staging: comedi: addi_apci_1564: introduce apci1564_private struct Chase Southwood
@ 2014-05-30 16:44 ` Hartley Sweeten
0 siblings, 0 replies; 16+ messages in thread
From: Hartley Sweeten @ 2014-05-30 16:44 UTC (permalink / raw)
To: Chase Southwood, gregkh@linuxfoundation.org
Cc: abbotti@mev.co.uk, devel@driverdev.osuosl.org,
linux-kernel@vger.kernel.org
On Thursday, May 29, 2014 9:43 PM, Chase Southwood wrote:
> The addi_private struct defined in addi-data/addi_common.h is very bloated
> and contains many fields which addi_apci_1564 does not require. In the
> interest of eventually removing this driver's dependency on
> addi_common.h, we can create a private data struct specifically for
> addi_apci_1564 containing only the fields it will actually use.
>
> Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> ---
> .../comedi/drivers/addi-data/hwdrv_apci1564.c | 164 +++++++++++----------
> drivers/staging/comedi/drivers/addi_apci_1564.c | 34 ++---
> 2 files changed, 102 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> index f974bd2..054e731 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> @@ -49,7 +49,7 @@
> #define APCI1564_COUNTER4 3
>
> /*
> - * devpriv->i_IobaseAmcc Register Map
> + * devpriv->amcc_iobase Register Map
> */
> #define APCI1564_DI_REG 0x04
> #define APCI1564_DI_INT_MODE1_REG 0x08
> @@ -93,6 +93,12 @@
> static unsigned int ui_InterruptStatus_1564;
> static unsigned int ui_InterruptData, ui_Type;
These static variables could also go into the new private data struct.
But, hopefully you will be removing them eventually anyway.
>
> +struct apci1564_private {
> + unsigned int amcc_iobase; /* base of AMCC I/O registers */
> + unsigned char timer_select_mode;
> + unsigned char mode_select_register;
> +};
> +
The new private data struct should really be defined in the driver (addi_apci_1564.c)
before the include of hwdrv_apci1564.c . Then you don't have to move it later.
Other than that:
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/6] staging: comedi: addi_apci_1564: add a subdevice for Change-of-State interrupt support
2014-05-30 4:40 [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality Chase Southwood
` (2 preceding siblings ...)
2014-05-30 4:42 ` [PATCH 3/6] staging: comedi: addi_apci_1564: introduce apci1564_private struct Chase Southwood
@ 2014-05-30 4:43 ` Chase Southwood
2014-05-30 16:49 ` Hartley Sweeten
2014-05-30 4:43 ` [PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice Chase Southwood
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Chase Southwood @ 2014-05-30 4:43 UTC (permalink / raw)
To: gregkh; +Cc: abbotti, hsweeten, devel, linux-kernel, Chase Southwood
This board supports an interrupt that can be generated by an AND/OR
combination of 16 of the input channels.
Create a separate subdevice to handle this interrupt.
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 | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 5901143..183fdc3 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -105,7 +105,7 @@ static int apci1564_auto_attach(struct comedi_device *dev,
dev->irq = pcidev->irq;
}
- ret = comedi_alloc_subdevices(dev, 3);
+ ret = comedi_alloc_subdevices(dev, 4);
if (ret)
return ret;
@@ -144,6 +144,20 @@ static int apci1564_auto_attach(struct comedi_device *dev,
s->insn_read = apci1564_timer_read;
s->insn_config = apci1564_timer_config;
+ /* Change-Of-State (COS) interrupt subdevice */
+ s = &dev->subdevices[3];
+ if (dev->irq) {
+ dev->read_subdev = s;
+ s->type = COMEDI_SUBD_DI;
+ s->subdev_flags = SDF_READABLE | SDF_CMD_READ;
+ s->n_chan = 1;
+ s->maxdata = 1;
+ s->range_table = &range_digital;
+ s->len_chanlist = 1;
+ } else {
+ s->type = COMEDI_SUBD_UNUSED;
+ }
+
return 0;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* RE: [PATCH 4/6] staging: comedi: addi_apci_1564: add a subdevice for Change-of-State interrupt support
2014-05-30 4:43 ` [PATCH 4/6] staging: comedi: addi_apci_1564: add a subdevice for Change-of-State interrupt support Chase Southwood
@ 2014-05-30 16:49 ` Hartley Sweeten
0 siblings, 0 replies; 16+ messages in thread
From: Hartley Sweeten @ 2014-05-30 16:49 UTC (permalink / raw)
To: Chase Southwood, gregkh@linuxfoundation.org
Cc: abbotti@mev.co.uk, devel@driverdev.osuosl.org,
linux-kernel@vger.kernel.org
On Thursday, May 29, 2014 9:43 PM, Chase Southwood wrote:
> This board supports an interrupt that can be generated by an AND/OR
> combination of 16 of the input channels.
>
> Create a separate subdevice to handle this interrupt.
>
> 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 | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
> index 5901143..183fdc3 100644
> --- a/drivers/staging/comedi/drivers/addi_apci_1564.c
> +++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
> @@ -105,7 +105,7 @@ static int apci1564_auto_attach(struct comedi_device *dev,
> dev->irq = pcidev->irq;
> }
>
> - ret = comedi_alloc_subdevices(dev, 3);
> + ret = comedi_alloc_subdevices(dev, 4);
> if (ret)
> return ret;
>
> @@ -144,6 +144,20 @@ static int apci1564_auto_attach(struct comedi_device *dev,
> s->insn_read = apci1564_timer_read;
> s->insn_config = apci1564_timer_config;
>
> + /* Change-Of-State (COS) interrupt subdevice */
> + s = &dev->subdevices[3];
> + if (dev->irq) {
> + dev->read_subdev = s;
> + s->type = COMEDI_SUBD_DI;
> + s->subdev_flags = SDF_READABLE | SDF_CMD_READ;
> + s->n_chan = 1;
> + s->maxdata = 1;
> + s->range_table = &range_digital;
> + s->len_chanlist = 1;
> + } else {
> + s->type = COMEDI_SUBD_UNUSED;
> + }
> +
> return 0;
> }
This patch could be merged with patch 5/6 but not a big deal.
Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice
2014-05-30 4:40 [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality Chase Southwood
` (3 preceding siblings ...)
2014-05-30 4:43 ` [PATCH 4/6] staging: comedi: addi_apci_1564: add a subdevice for Change-of-State interrupt support Chase Southwood
@ 2014-05-30 4:43 ` Chase Southwood
2014-05-30 16:57 ` Hartley Sweeten
2014-05-30 4:43 ` [PATCH 6/6] staging: comedi: addi_apci_1564: cleanup v_ADDI_Interrupt() Chase Southwood
2014-05-30 7:45 ` [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality Dan Carpenter
6 siblings, 1 reply; 16+ messages in thread
From: Chase Southwood @ 2014-05-30 4:43 UTC (permalink / raw)
To: gregkh; +Cc: abbotti, hsweeten, devel, linux-kernel, Chase Southwood
The board supported by this driver can generate an interrupt based
on the state of input channels 0-15.
The apci1564_di_config() function is used to configure which
inputs are used to generate the interrupt. Currently this function
is broken since it does not follow the comedi API for insn_config
functions. Fix this function by implementing the config instruction
INSN_CONFIG_DIGITAL_TRIG.
Add the remaining subdevice operations necessary for the interrupt
subdevice to support async commands.
Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
The structure of _much_ of this code was taken from/based on the similar
code found in addi_apci_1032.c. As such, I would appreciate as much
review I can get to make sure what I've done here actually makes sense
for this driver :)
.../comedi/drivers/addi-data/hwdrv_apci1564.c | 37 +---
drivers/staging/comedi/drivers/addi_apci_1564.c | 228 +++++++++++++++++++--
2 files changed, 210 insertions(+), 55 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 054e731..41aa889 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -95,45 +95,14 @@ static unsigned int ui_InterruptData, ui_Type;
struct apci1564_private {
unsigned int amcc_iobase; /* base of AMCC I/O registers */
+ unsigned int mode1; /* riding-edge/high level channels */
+ unsigned int mode2; /* falling-edge/low level channels */
+ unsigned int ctrl; /* interrupt mode OR (edge) . AND (level) */
unsigned char timer_select_mode;
unsigned char mode_select_register;
};
/*
- * Configures the digital input Subdevice
- *
- * data[0] 1 = Enable interrupt, 0 = Disable interrupt
- * data[1] 0 = ADDIDATA Interrupt OR LOGIC, 1 = ADDIDATA Interrupt AND LOGIC
- * data[2] Interrupt mask for the mode 1
- * data[3] Interrupt mask for the mode 2
- */
-static int apci1564_di_config(struct comedi_device *dev,
- struct comedi_subdevice *s,
- struct comedi_insn *insn,
- unsigned int *data)
-{
- struct apci1564_private *devpriv = dev->private;
-
- /* Set the digital input logic */
- if (data[0] == ADDIDATA_ENABLE) {
- data[2] = data[2] << 4;
- data[3] = data[3] << 4;
- outl(data[2], devpriv->amcc_iobase + APCI1564_DI_INT_MODE1_REG);
- outl(data[3], devpriv->amcc_iobase + APCI1564_DI_INT_MODE2_REG);
- if (data[1] == ADDIDATA_OR)
- outl(0x4, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
- else
- outl(0x6, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
- } else {
- outl(0x0, devpriv->amcc_iobase + APCI1564_DI_INT_MODE1_REG);
- outl(0x0, devpriv->amcc_iobase + APCI1564_DI_INT_MODE2_REG);
- outl(0x0, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
- }
-
- return insn->n;
-}
-
-/*
* Configures The Digital Output Subdevice.
*
* data[1] 0 = Disable VCC Interrupt, 1 = Enable VCC Interrupt
diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 183fdc3..cf58f90 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -8,6 +8,42 @@
#include "addi-data/hwdrv_apci1564.c"
+#define APCI1564_CTRL_INT_OR (0 << 1)
+#define APCI1564_CTRL_INT_AND (1 << 1)
+#define APCI1564_CTRL_INT_ENA (1 << 2)
+
+static int apci1564_reset(struct comedi_device *dev)
+{
+ struct apci1564_private *devpriv = dev->private;
+
+ ui_Type = 0;
+
+ /* Disable the input interrupts and reset status register */
+ 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);
+
+ /* Reset the output channels and disable interrupts */
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DO_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_DO_INT_CTRL_REG);
+
+ /* Reset the watchdog registers */
+ addi_watchdog_reset(devpriv->amcc_iobase + APCI1564_WDOG_REG);
+
+ /* Reset the timer registers */
+ outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
+ outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_RELOAD_REG);
+
+ /* Reset the counter registers */
+ outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
+ outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER2));
+ outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
+ outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
+
+ return 0;
+}
+
static irqreturn_t v_ADDI_Interrupt(int irq, void *d)
{
apci1564_interrupt(irq, d);
@@ -43,38 +79,184 @@ static int apci1564_do_insn_bits(struct comedi_device *dev,
return insn->n;
}
-static int apci1564_reset(struct comedi_device *dev)
+/*
+ * Change-Of-State (COS) interrupt configuration
+ *
+ * Channels 0 to 15 are interruptible. These channels can be configured
+ * to generate interrupts based on AND/OR logic for the desired channels.
+ *
+ * OR logic
+ * - reacts to rising or falling edges
+ * - interrupt is generated when any enabled channel
+ * meet the desired interrupt condition
+ *
+ * AND logic
+ * - reacts to changes in level of the selected inputs
+ * - interrupt is generated when all enabled channels
+ * meet the desired interrupt condition
+ * - after an interrupt, a change in level must occur on
+ * the selected inputs to release the IRQ logic
+ *
+ * The COS interrupt must be configured before it can be enabled.
+ *
+ * data[0] : INSN_CONFIG_DIGITAL_TRIG
+ * data[1] : trigger number (= 0)
+ * data[2] : configuration operation:
+ * COMEDI_DIGITAL_TRIG_DISABLE = no interrupts
+ * COMEDI_DIGITAL_TRIG_ENABLE_EDGES = OR (edge) interrupts
+ * COMEDI_DIGITAL_TRIG_ENABLE_LEVELS = AND (level) interrupts
+ * data[3] : left-shift for data[4] and data[5]
+ * data[4] : rising-edge/high level channels
+ * data[5] : falling-edge/low level channels
+ */
+static int apci1564_cos_insn_config(struct comedi_device *dev,
+ struct comedi_subdevice *s,
+ struct comedi_insn *insn,
+ unsigned int *data)
{
struct apci1564_private *devpriv = dev->private;
+ unsigned int shift, oldmask;
+
+ switch (data[0]) {
+ case INSN_CONFIG_DIGITAL_TRIG:
+ if (data[1] != 0)
+ return -EINVAL;
+ shift = data[3];
+ oldmask = (1U << shift) - 1;
+ switch (data[2]) {
+ case COMEDI_DIGITAL_TRIG_DISABLE:
+ devpriv->ctrl = 0;
+ devpriv->mode1 = 0;
+ devpriv->mode2 = 0;
+ apci1564_reset(dev);
+ break;
+ case COMEDI_DIGITAL_TRIG_ENABLE_EDGES:
+ if (devpriv->ctrl != (APCI1564_CTRL_INT_ENA |
+ APCI1564_CTRL_INT_OR)) {
+ /* switching to 'OR' mode */
+ devpriv->ctrl = APCI1564_CTRL_INT_ENA |
+ APCI1564_CTRL_INT_OR;
+ /* wipe old channels */
+ devpriv->mode1 = 0;
+ devpriv->mode2 = 0;
+ } else {
+ /* preserve unspecified channels */
+ devpriv->mode1 &= oldmask;
+ devpriv->mode2 &= oldmask;
+ }
+ /* configure specified channels */
+ devpriv->mode1 |= data[4] << shift;
+ devpriv->mode2 |= data[5] << shift;
+ break;
+ case COMEDI_DIGITAL_TRIG_ENABLE_LEVELS:
+ if (devpriv->ctrl != (APCI1564_CTRL_INT_ENA |
+ APCI1564_CTRL_INT_AND)) {
+ /* switching to 'AND' mode */
+ devpriv->ctrl = APCI1564_CTRL_INT_ENA |
+ APCI1564_CTRL_INT_AND;
+ /* wipe old channels */
+ devpriv->mode1 = 0;
+ devpriv->mode2 = 0;
+ } else {
+ /* preserve unspecified channels */
+ devpriv->mode1 &= oldmask;
+ devpriv->mode2 &= oldmask;
+ }
+ /* configure specified channels */
+ devpriv->mode1 |= data[4] << shift;
+ devpriv->mode2 |= data[5] << shift;
+ break;
+ default:
+ return -EINVAL;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+ return insn->n;
+}
- ui_Type = 0;
+static int apci1564_cos_insn_bits(struct comedi_device *dev,
+ struct comedi_subdevice *s,
+ struct comedi_insn *insn,
+ unsigned int *data)
+{
+ data[1] = s->state;
- /* Disable the input interrupts and reset status register */
- 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);
+ return 0;
+}
- /* Reset the output channels and disable interrupts */
- outl(0x0, devpriv->amcc_iobase + APCI1564_DO_REG);
- outl(0x0, devpriv->amcc_iobase + APCI1564_DO_INT_CTRL_REG);
+static int apci1564_cos_cmdtest(struct comedi_device *dev,
+ struct comedi_subdevice *s,
+ struct comedi_cmd *cmd)
+{
+ int err = 0;
- /* Reset the watchdog registers */
- addi_watchdog_reset(devpriv->amcc_iobase + APCI1564_WDOG_REG);
+ /* Step 1 : check if triggers are trivially valid */
- /* Reset the timer registers */
- outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
- outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_RELOAD_REG);
+ err |= cfc_check_trigger_src(&cmd->start_src, TRIG_NOW);
+ err |= cfc_check_trigger_src(&cmd->scan_begin_src, TRIG_EXT);
+ err |= cfc_check_trigger_src(&cmd->convert_src, TRIG_FOLLOW);
+ err |= cfc_check_trigger_src(&cmd->scan_end_src, TRIG_COUNT);
+ err |= cfc_check_trigger_src(&cmd->stop_src, TRIG_NONE);
- /* Reset the counter registers */
- outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
- outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER2));
- outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
- outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
+ if (err)
+ return 1;
+
+ /* Step 2a : make sure trigger sources are unique */
+ /* Step 2b : and mutually compatible */
+
+ if (err)
+ return 2;
+
+ /* Step 3: check if arguments are trivially valid */
+
+ err |= cfc_check_trigger_arg_is(&cmd->start_arg, 0);
+ err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
+ err |= cfc_check_trigger_arg_is(&cmd->convert_arg, 0);
+ err |= cfc_check_trigger_arg_is(&cmd->scan_end_arg, cmd->chanlist_len);
+ err |= cfc_check_trigger_arg_is(&cmd->stop_arg, 0);
+
+ if (err)
+ return 3;
+
+ /* step 4: ignored */
+
+ if (err)
+ return 4;
+
+ return 0;
+}
+
+/*
+ * Change-Of-State (COS) 'do_cmd' operation
+ *
+ * Enable the COS interrupt as configured by apci1564_cos_insn_config().
+ */
+static int apci1564_cos_cmd(struct comedi_device *dev,
+ struct comedi_subdevice *s)
+{
+ struct apci1564_private *devpriv = dev->private;
+
+ if (!devpriv->ctrl) {
+ dev_warn(dev->class_dev,
+ "Interrupts disabled due to mode configuration!\n");
+ return -EINVAL;
+ }
+
+ outl(devpriv->mode1, devpriv->amcc_iobase + APCI1564_DI_INT_MODE1_REG);
+ outl(devpriv->mode2, devpriv->amcc_iobase + APCI1564_DI_INT_MODE2_REG);
+ outl(devpriv->ctrl, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
return 0;
}
+static int apci1564_cos_cancel(struct comedi_device *dev,
+ struct comedi_subdevice *s)
+{
+ return apci1564_reset(dev);
+}
+
static int apci1564_auto_attach(struct comedi_device *dev,
unsigned long context_unused)
{
@@ -117,7 +299,6 @@ static int apci1564_auto_attach(struct comedi_device *dev,
s->maxdata = 1;
s->len_chanlist = 32;
s->range_table = &range_digital;
- s->insn_config = apci1564_di_config;
s->insn_bits = apci1564_di_insn_bits;
/* Allocate and Initialise DO Subdevice Structures */
@@ -154,6 +335,11 @@ static int apci1564_auto_attach(struct comedi_device *dev,
s->maxdata = 1;
s->range_table = &range_digital;
s->len_chanlist = 1;
+ s->insn_config = apci1564_cos_insn_config;
+ s->insn_bits = apci1564_cos_insn_bits;
+ s->do_cmdtest = apci1564_cos_cmdtest;
+ s->do_cmd = apci1564_cos_cmd;
+ s->cancel = apci1564_cos_cancel;
} else {
s->type = COMEDI_SUBD_UNUSED;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* RE: [PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice
2014-05-30 4:43 ` [PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice Chase Southwood
@ 2014-05-30 16:57 ` Hartley Sweeten
0 siblings, 0 replies; 16+ messages in thread
From: Hartley Sweeten @ 2014-05-30 16:57 UTC (permalink / raw)
To: Chase Southwood, gregkh@linuxfoundation.org
Cc: abbotti@mev.co.uk, devel@driverdev.osuosl.org,
linux-kernel@vger.kernel.org
On Thursday, May 29, 2014 9:44 PM, Chase Southwood wrote:
> The board supported by this driver can generate an interrupt based
> on the state of input channels 0-15.
>
> The apci1564_di_config() function is used to configure which
> inputs are used to generate the interrupt. Currently this function
> is broken since it does not follow the comedi API for insn_config
> functions. Fix this function by implementing the config instruction
> INSN_CONFIG_DIGITAL_TRIG.
>
> Add the remaining subdevice operations necessary for the interrupt
> subdevice to support async commands.
>
> Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> ---
>
> The structure of _much_ of this code was taken from/based on the similar
> code found in addi_apci_1032.c. As such, I would appreciate as much
> review I can get to make sure what I've done here actually makes sense
> for this driver :)
>
> .../comedi/drivers/addi-data/hwdrv_apci1564.c | 37 +---
> drivers/staging/comedi/drivers/addi_apci_1564.c | 228 +++++++++++++++++++--
> 2 files changed, 210 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> index 054e731..41aa889 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> @@ -95,45 +95,14 @@ static unsigned int ui_InterruptData, ui_Type;
>
> struct apci1564_private {
> unsigned int amcc_iobase; /* base of AMCC I/O registers */
> + unsigned int mode1; /* riding-edge/high level channels */
> + unsigned int mode2; /* falling-edge/low level channels */
> + unsigned int ctrl; /* interrupt mode OR (edge) . AND (level) */
> unsigned char timer_select_mode;
> unsigned char mode_select_register;
> };
[snip]
> diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
> index 183fdc3..cf58f90 100644
> --- a/drivers/staging/comedi/drivers/addi_apci_1564.c
> +++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
> @@ -8,6 +8,42 @@
>
> #include "addi-data/hwdrv_apci1564.c"
>
> +#define APCI1564_CTRL_INT_OR (0 << 1)
> +#define APCI1564_CTRL_INT_AND (1 << 1)
> +#define APCI1564_CTRL_INT_ENA (1 << 2)
> +
> +static int apci1564_reset(struct comedi_device *dev)
> +{
> + struct apci1564_private *devpriv = dev->private;
> +
> + ui_Type = 0;
> +
> + /* Disable the input interrupts and reset status register */
> + 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);
> +
> + /* Reset the output channels and disable interrupts */
> + outl(0x0, devpriv->amcc_iobase + APCI1564_DO_REG);
> + outl(0x0, devpriv->amcc_iobase + APCI1564_DO_INT_CTRL_REG);
> +
> + /* Reset the watchdog registers */
> + addi_watchdog_reset(devpriv->amcc_iobase + APCI1564_WDOG_REG);
> +
> + /* Reset the timer registers */
> + outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
> + outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_RELOAD_REG);
> +
> + /* Reset the counter registers */
> + outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER1));
> + outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER2));
> + outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
> + outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
> +
> + return 0;
> +}
You are using this 'board reset' function as the (*cancel) operation for the interrupt
subdevice. I think this is a bit too aggressive for the (*cancel) since it also resets all
the outputs, the watchdog, and timer/counters.
Pull out the first chunk of this function that disables the interrupts and use that for
the (*cancel) operation.
The reset seems to look ok.
Regards,
Hartley
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 6/6] staging: comedi: addi_apci_1564: cleanup v_ADDI_Interrupt()
2014-05-30 4:40 [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality Chase Southwood
` (4 preceding siblings ...)
2014-05-30 4:43 ` [PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice Chase Southwood
@ 2014-05-30 4:43 ` Chase Southwood
2014-05-30 17:26 ` Hartley Sweeten
2014-05-30 7:45 ` [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality Dan Carpenter
6 siblings, 1 reply; 16+ messages in thread
From: Chase Southwood @ 2014-05-30 4:43 UTC (permalink / raw)
To: gregkh; +Cc: abbotti, hsweeten, devel, linux-kernel, Chase Southwood
Move the function apci1564_interrupt() from hwdrv_apci1564.c to
addi_apci_1564.c. On moving, for now just strip out all of the
code for interrupts that the driver does not yet support at this
time.
Rename the variable ui_InterruptStatus_1564 to ctrl, and change the return
from IRQ_RETVAL(1) to IRQ_HANDLED.
We also check the device is asserting the shared interrupt line and check
that interrupts have been enabled.
Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
---
Admittedly, I am not sure if what I have done in the interrupt handler is
quite sufficient. Am I missing anything that should be there due to the
fact that the handler does not yet support all of the types of interrupt
that the board is capable of issuing (i.e. does the interrupt handler need
to clear the other interrupts just in case they get triggered, etc?)
.../comedi/drivers/addi-data/hwdrv_apci1564.c | 130 ---------------------
drivers/staging/comedi/drivers/addi_apci_1564.c | 34 +++++-
2 files changed, 30 insertions(+), 134 deletions(-)
diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 41aa889..ebd763b 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -90,7 +90,6 @@
#define APCI1564_TCW_WARN_TIMEBASE_REG(x) (0x1c + ((x) * 0x20))
/* Global variables */
-static unsigned int ui_InterruptStatus_1564;
static unsigned int ui_InterruptData, ui_Type;
struct apci1564_private {
@@ -367,132 +366,3 @@ static int apci1564_do_read(struct comedi_device *dev,
*data = ui_Type;
return insn->n;
}
-
-/*
- * Interrupt handler for the interruptible digital inputs
- */
-static void apci1564_interrupt(int irq, void *d)
-{
- struct comedi_device *dev = d;
- struct apci1564_private *devpriv = dev->private;
- 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;
-
- ui_DI = inl(devpriv->amcc_iobase + APCI1564_DI_IRQ_REG) & 0x01;
- 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) {
- dev_err(dev->class_dev, "Interrupt from unknown source.\n");
- }
-
- if (ui_DI == 1) {
- ui_DI = inl(devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
- outl(0x0, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
- ui_InterruptStatus_1564 =
- inl(devpriv->amcc_iobase + APCI1564_DI_INT_STATUS_REG);
- ui_InterruptStatus_1564 = ui_InterruptStatus_1564 & 0X000FFFF0;
- /* enable the interrupt */
- outl(ui_DI, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
- return;
- }
-
- if (ui_DO == 1) {
- /* Check for Digital Output interrupt Type */
- /* 1: VCC interrupt */
- /* 2: CC interrupt */
- ui_Type = inl(devpriv->amcc_iobase + APCI1564_DO_INT_STATUS_REG) & 0x3;
- /* Disable the Interrupt */
- outl(0x0, devpriv->amcc_iobase + APCI1564_DO_INT_CTRL_REG);
-
- }
-
- 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);
-
- /* 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));
-
- /* 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));
-
- /* 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) {
-
- /* 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 Counter Interrupt */
- outl(ul_Command2,
- dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER3));
- }
- }
-
- 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));
-
- /* Enable Counter Interrupt */
- outl(ul_Command2,
- dev->iobase + APCI1564_TCW_CTRL_REG(APCI1564_COUNTER4));
- }
- }
- return;
-}
diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index cf58f90..327441e 100644
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -3,6 +3,7 @@
#include "../comedidev.h"
#include "comedi_fc.h"
+#include "amcc_s5933.h"
#include "addi-data/addi_common.h"
@@ -44,10 +45,35 @@ static int apci1564_reset(struct comedi_device *dev)
return 0;
}
-static irqreturn_t v_ADDI_Interrupt(int irq, void *d)
+static irqreturn_t apci1564_interrupt(int irq, void *d)
{
- apci1564_interrupt(irq, d);
- return IRQ_RETVAL(1);
+ struct comedi_device *dev = d;
+ struct apci1564_private *devpriv = dev->private;
+ struct comedi_subdevice *s = dev->read_subdev;
+ unsigned int ctrl;
+
+ /* check interrupt is from this device */
+ if ((inl(devpriv->amcc_iobase + AMCC_OP_REG_INTCSR) &
+ INTCSR_INTR_ASSERTED) == 0)
+ return IRQ_NONE;
+
+ /* check interrupt is enabled */
+ ctrl = inl(devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+ if ((ctrl & APCI1564_CTRL_INT_ENA) == 0)
+ return IRQ_HANDLED;
+
+ /* disable the interrupt */
+ outl(ctrl & ~APCI1564_CTRL_INT_ENA, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+
+ 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(ctrl, devpriv->amcc_iobase + APCI1564_DI_IRQ_REG);
+
+ return IRQ_HANDLED;
}
static int apci1564_di_insn_bits(struct comedi_device *dev,
@@ -281,7 +307,7 @@ static int apci1564_auto_attach(struct comedi_device *dev,
apci1564_reset(dev);
if (pcidev->irq > 0) {
- ret = request_irq(pcidev->irq, v_ADDI_Interrupt, IRQF_SHARED,
+ ret = request_irq(pcidev->irq, apci1564_interrupt, IRQF_SHARED,
dev->board_name, dev);
if (ret == 0)
dev->irq = pcidev->irq;
--
1.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* RE: [PATCH 6/6] staging: comedi: addi_apci_1564: cleanup v_ADDI_Interrupt()
2014-05-30 4:43 ` [PATCH 6/6] staging: comedi: addi_apci_1564: cleanup v_ADDI_Interrupt() Chase Southwood
@ 2014-05-30 17:26 ` Hartley Sweeten
2014-06-02 3:52 ` Chase Southwood
0 siblings, 1 reply; 16+ messages in thread
From: Hartley Sweeten @ 2014-05-30 17:26 UTC (permalink / raw)
To: Chase Southwood, gregkh@linuxfoundation.org
Cc: devel@driverdev.osuosl.org, abbotti@mev.co.uk,
linux-kernel@vger.kernel.org
On Thursday, May 29, 2014 9:44 PM, Chase Southwood wrote:
> Move the function apci1564_interrupt() from hwdrv_apci1564.c to
> addi_apci_1564.c. On moving, for now just strip out all of the
> code for interrupts that the driver does not yet support at this
> time.
>
> Rename the variable ui_InterruptStatus_1564 to ctrl, and change the return
> from IRQ_RETVAL(1) to IRQ_HANDLED.
>
> We also check the device is asserting the shared interrupt line and check
> that interrupts have been enabled.
>
> Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
> ---
>
> Admittedly, I am not sure if what I have done in the interrupt handler is
> quite sufficient. Am I missing anything that should be there due to the
> fact that the handler does not yet support all of the types of interrupt
> that the board is capable of issuing (i.e. does the interrupt handler need
> to clear the other interrupts just in case they get triggered, etc?)
This one is a bit of a mess, not your fault, the existing code is pretty ugly.
> .../comedi/drivers/addi-data/hwdrv_apci1564.c | 130 ---------------------
> drivers/staging/comedi/drivers/addi_apci_1564.c | 34 +++++-
> 2 files changed, 30 insertions(+), 134 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> index 41aa889..ebd763b 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> @@ -90,7 +90,6 @@
> #define APCI1564_TCW_WARN_TIMEBASE_REG(x) (0x1c + ((x) * 0x20))
>
> /* Global variables */
> -static unsigned int ui_InterruptStatus_1564;
> static unsigned int ui_InterruptData, ui_Type;
These static globals need to away.
ui_InterruptStatus_1564 is only used in the interrupt handler. It's also not really used
in the existing code. You fixed this in your patch but I'm not sure it deserves mentioning
in the commit other than maybe "remove unnecessary static global foo".
ui_InterruptData is set in apci1564_do_config() but it is never used. That function is also
broke since it does not follow the comedi API.
ui_Type is set in the interrupt handler but you patch strips it out. The variable is then
used in apci1564_do_read() but that function also abuses the comedi API since it is
returning the diagnostic status in the digital output (*insn_read) operation.
Ugh...
You might want to rework this patch to:
1) Move the static globals into the private data.
2) Merge the existing interrupt handler from hwrdv_apci1564.c into the driver.
You could tidy up the existing code a bit during the move. Hint, the counters
could be handled with a for () loop.
3) Now "fix" the digital input interrupt handling.
Since you still have the timer subdevice I would leave the timer/counter handling
in the interrupt handler.
For now, hold off and see if Ian has any comments.
I'll try to apply your series and take a better look at this later today.
Regards,
Hartley
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 6/6] staging: comedi: addi_apci_1564: cleanup v_ADDI_Interrupt()
2014-05-30 17:26 ` Hartley Sweeten
@ 2014-06-02 3:52 ` Chase Southwood
0 siblings, 0 replies; 16+ messages in thread
From: Chase Southwood @ 2014-06-02 3:52 UTC (permalink / raw)
To: Hartley Sweeten
Cc: gregkh@linuxfoundation.org, devel@driverdev.osuosl.org,
abbotti@mev.co.uk, linux-kernel@vger.kernel.org
On Fri, May 30, 2014 at 12:26 PM, Hartley Sweeten
<HartleyS@visionengravers.com> wrote:
> On Thursday, May 29, 2014 9:44 PM, Chase Southwood wrote:
>> Move the function apci1564_interrupt() from hwdrv_apci1564.c to
>> addi_apci_1564.c. On moving, for now just strip out all of the
>> code for interrupts that the driver does not yet support at this
>> time.
>>
>> Rename the variable ui_InterruptStatus_1564 to ctrl, and change the return
>> from IRQ_RETVAL(1) to IRQ_HANDLED.
>>
>> We also check the device is asserting the shared interrupt line and check
>> that interrupts have been enabled.
>>
>> Signed-off-by: Chase Southwood <chase.southwood@gmail.com>
>> Cc: Ian Abbott <abbotti@mev.co.uk>
>> Cc: H Hartley Sweeten <hsweeten@visionengravers.com>
>> ---
Hartley,
Excellent...I will draft up a new version of this set based on all of
your comments, but I will hold off on sending until I see if Ian has
anything further to offer. Things are certainly looking better
already though.
Thanks,
Chase
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality.
2014-05-30 4:40 [PATCH 0/6] staging: comedi: addi_apci_1564: fix Change-of-State interrupt functionality Chase Southwood
` (5 preceding siblings ...)
2014-05-30 4:43 ` [PATCH 6/6] staging: comedi: addi_apci_1564: cleanup v_ADDI_Interrupt() Chase Southwood
@ 2014-05-30 7:45 ` Dan Carpenter
6 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2014-05-30 7:45 UTC (permalink / raw)
To: Chase Southwood; +Cc: gregkh, devel, abbotti, linux-kernel
When you do a v2 patchset, put a v2 in the subject and say which things
changed since v1. https://www.google.com/search?q=how+to+send+a+v2+patch
regards,
dan carpenter
^ permalink raw reply [flat|nested] 16+ messages in thread