subject:"\[PATCH 5\/6\] staging\: comedi\: addi_apci_1564\: hook\-up the interrupt subdevice"

RE: [PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice

2014-05-30 Thread Hartley Sweeten

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 
> Cc: Ian Abbott 
> Cc: H Hartley Sweeten 
> ---
>
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice

2014-05-30 Thread Hartley Sweeten

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.southw...@gmail.com
 Cc: Ian Abbott abbo...@mev.co.uk
 Cc: H Hartley Sweeten hswee...@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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice

2014-05-29 Thread 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 
Cc: Ian Abbott 
Cc: H Hartley Sweeten 
---

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));
+

[PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice

2014-05-29 Thread 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.southw...@gmail.com
Cc: Ian Abbott abbo...@mev.co.uk
Cc: H Hartley Sweeten hswee...@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 +

RE: [PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice

RE: [PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice

[PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice

[PATCH 5/6] staging: comedi: addi_apci_1564: hook-up the interrupt subdevice

4 matches

Site Navigation

Mail list logo

Footer information