staging: comedi: addi_apci_1564: rewrite the timer subdevice support

The support functions for the timer subdevice are broken.

1) The (*insn_write) assumes that insn->n is always 2 (data[1] is used)
2) The (*insn_read) assumes that insn->n is always 2 (data can be returned in
   data[0] and data[1]).
3) The (*insn_config) does not follow the API. It assumes insn->n is always 4
   (data[1], data[2] and data[3] are used). It also doesn't use data[0] to
   determine what the config "instruction" is.

Rewrite the code to follow the comedi API and add the missing comedi driver
comment block.

The new implementation is based on the (minimal) datasheet I have from
ADDI-DATA.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index d6b288020a6731c9b878d045ae86a7f3ba38ce7b..a1df66d49c33d262b2bf592a581a625f84cb9dde 100644 (file)
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -1,94 +1,3 @@
-static int apci1564_timer_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 ctrl;
-
-       /* Stop the timer */
-       ctrl = inl(devpriv->timer + ADDI_TCW_CTRL_REG);
-       ctrl &= ~(ADDI_TCW_CTRL_GATE | ADDI_TCW_CTRL_TRIG |
-                 ADDI_TCW_CTRL_ENA);
-       outl(ctrl, devpriv->timer + ADDI_TCW_CTRL_REG);
-
-       if (data[1] == 1) {
-               /* Enable timer int & disable all the other int sources */
-               outl(ADDI_TCW_CTRL_IRQ_ENA,
-                    devpriv->timer + ADDI_TCW_CTRL_REG);
-               outl(0x0, dev->iobase + APCI1564_DI_IRQ_REG);
-               outl(0x0, dev->iobase + APCI1564_DO_IRQ_REG);
-               outl(0x0, dev->iobase + APCI1564_WDOG_IRQ_REG);
-               if (devpriv->counters) {
-                       unsigned long iobase;
-
-                       iobase = devpriv->counters + ADDI_TCW_IRQ_REG;
-                       outl(0x0, iobase + APCI1564_COUNTER(0));
-                       outl(0x0, iobase + APCI1564_COUNTER(1));
-                       outl(0x0, iobase + APCI1564_COUNTER(2));
-               }
-       } else {
-               /* disable Timer interrupt */
-               outl(0x0, devpriv->timer + ADDI_TCW_CTRL_REG);
-       }
-
-       /* Loading Timebase */
-       outl(data[2], devpriv->timer + ADDI_TCW_TIMEBASE_REG);
-
-       /* Loading the Reload value */
-       outl(data[3], devpriv->timer + ADDI_TCW_RELOAD_REG);
-
-       ctrl = inl(devpriv->timer + ADDI_TCW_CTRL_REG);
-       ctrl &= ~(ADDI_TCW_CTRL_CNTR_ENA | ADDI_TCW_CTRL_MODE_MASK |
-                 ADDI_TCW_CTRL_GATE | ADDI_TCW_CTRL_TRIG |
-                 ADDI_TCW_CTRL_TIMER_ENA | ADDI_TCW_CTRL_RESET_ENA |
-                 ADDI_TCW_CTRL_WARN_ENA | ADDI_TCW_CTRL_ENA);
-       ctrl |= ADDI_TCW_CTRL_MODE(2) | ADDI_TCW_CTRL_TIMER_ENA;
-       outl(ctrl, devpriv->timer + ADDI_TCW_CTRL_REG);
-
-       return insn->n;
-}
-
-static int apci1564_timer_insn_write(struct comedi_device *dev,
-                                    struct comedi_subdevice *s,
-                                    struct comedi_insn *insn,
-                                    unsigned int *data)
-{
-       struct apci1564_private *devpriv = dev->private;
-       unsigned int ctrl;
-
-       ctrl = inl(devpriv->timer + ADDI_TCW_CTRL_REG);
-       ctrl &= ~(ADDI_TCW_CTRL_GATE | ADDI_TCW_CTRL_TRIG);
-       switch (data[1]) {
-       case 0: /* Stop The Timer */
-               ctrl &= ~ADDI_TCW_CTRL_ENA;
-               break;
-       case 1: /* Enable the Timer */
-               ctrl |= ADDI_TCW_CTRL_ENA;
-               break;
-       }
-       outl(ctrl, devpriv->timer + ADDI_TCW_CTRL_REG);
-
-       return insn->n;
-}
-
-static int apci1564_timer_insn_read(struct comedi_device *dev,
-                                   struct comedi_subdevice *s,
-                                   struct comedi_insn *insn,
-                                   unsigned int *data)
-{
-       struct apci1564_private *devpriv = dev->private;
-
-       /* Stores the status of the Timer */
-       data[0] = inl(devpriv->timer + ADDI_TCW_STATUS_REG) &
-                 ADDI_TCW_STATUS_OVERFLOW;
-
-       /* Stores the Actual value of the Timer */
-       data[1] = inl(devpriv->timer + ADDI_TCW_VAL_REG);
-
-       return insn->n;
-}
-
 static int apci1564_counter_insn_config(struct comedi_device *dev,
                                        struct comedi_subdevice *s,
                                        struct comedi_insn *insn,

diff --git a/drivers/staging/comedi/drivers/addi_apci_1564.c b/drivers/staging/comedi/drivers/addi_apci_1564.c
index 4af45d8bf3eccebe8b6a29d399c09e19bdbbbc05..3e8ac67fcb508ff33630e35ed37239317104cb65 100644 (file)
--- a/drivers/staging/comedi/drivers/addi_apci_1564.c
+++ b/drivers/staging/comedi/drivers/addi_apci_1564.c
@@ -21,6 +21,54 @@
  * details.
  */
 
+/*
+ * Driver: addi_apci_1564
+ * Description: ADDI-DATA APCI-1564 Digital I/O board
+ * Devices: [ADDI-DATA] APCI-1564 (addi_apci_1564)
+ * Author: H Hartley Sweeten <hsweeten@visionengravers.com>
+ * Updated: Thu, 02 Jun 2016 13:12:46 -0700
+ * Status: untested
+ *
+ * Configuration Options: not applicable, uses comedi PCI auto config
+ *
+ * This board has the following features:
+ *   - 32 optically isolated digital inputs (24V), 16 of which can
+ *     generate change-of-state (COS) interrupts (channels 4 to 19)
+ *   - 32 optically isolated digital outputs (10V to 36V)
+ *   - 1 8-bit watchdog for resetting the outputs
+ *   - 1 12-bit timer
+ *   - 3 32-bit counters
+ *   - 2 diagnostic inputs
+ *
+ * The COS, timer, and counter subdevices all use the dev->read_subdev to
+ * return the interrupt status. The sample data is updated and returned when
+ * any of these subdevices generate an interrupt. The sample data format is:
+ *
+ *    Bit   Description
+ *   -----  ------------------------------------------
+ *    31    COS interrupt
+ *    30    timer interrupt
+ *    29    counter 2 interrupt
+ *    28    counter 1 interrupt
+ *    27    counter 0 interrupt
+ *   26:20  not used
+ *   19:4   COS digital input state (channels 19 to 4)
+ *    3:0   not used
+ *
+ * The COS interrupts must be configured using an INSN_CONFIG_DIGITAL_TRIG
+ * instruction before they can be enabled by an async command. The COS
+ * interrupts will stay active until canceled.
+ *
+ * The timer subdevice does not use an async command. All control is handled
+ * by the (*insn_config).
+ *
+ * FIXME: The format of the ADDI_TCW_TIMEBASE_REG is not descibed in the
+ * datasheet I have. The INSN_CONFIG_SET_CLOCK_SRC currently just writes
+ * the raw data[1] to this register along with the raw data[2] value to the
+ * ADDI_TCW_RELOAD_REG. If anyone tests this and can determine the actual
+ * timebase/reload operation please let me know.
+ */
+
 #include <linux/module.h>
 #include <linux/interrupt.h>
 
@@ -444,6 +492,87 @@ static int apci1564_cos_cancel(struct comedi_device *dev,
        return 0;
 }
 
+static int apci1564_timer_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 val;
+
+       switch (data[0]) {
+       case INSN_CONFIG_ARM:
+               if (data[1] > s->maxdata)
+                       return -EINVAL;
+               outl(data[1], devpriv->timer + ADDI_TCW_RELOAD_REG);
+               outl(ADDI_TCW_CTRL_IRQ_ENA | ADDI_TCW_CTRL_TIMER_ENA,
+                    devpriv->timer + ADDI_TCW_CTRL_REG);
+               break;
+       case INSN_CONFIG_DISARM:
+               outl(0x0, devpriv->timer + ADDI_TCW_CTRL_REG);
+               break;
+       case INSN_CONFIG_GET_COUNTER_STATUS:
+               data[1] = 0;
+               val = inl(devpriv->timer + ADDI_TCW_CTRL_REG);
+               if (val & ADDI_TCW_CTRL_IRQ_ENA)
+                       data[1] |= COMEDI_COUNTER_ARMED;
+               if (val & ADDI_TCW_CTRL_TIMER_ENA)
+                       data[1] |= COMEDI_COUNTER_COUNTING;
+               val = inl(devpriv->timer + ADDI_TCW_STATUS_REG);
+               if (val & ADDI_TCW_STATUS_OVERFLOW)
+                       data[1] |= COMEDI_COUNTER_TERMINAL_COUNT;
+               data[2] = COMEDI_COUNTER_ARMED | COMEDI_COUNTER_COUNTING |
+                         COMEDI_COUNTER_TERMINAL_COUNT;
+               break;
+       case INSN_CONFIG_SET_CLOCK_SRC:
+               if (data[2] > s->maxdata)
+                       return -EINVAL;
+               outl(data[1], devpriv->timer + ADDI_TCW_TIMEBASE_REG);
+               outl(data[2], devpriv->timer + ADDI_TCW_RELOAD_REG);
+               break;
+       case INSN_CONFIG_GET_CLOCK_SRC:
+               data[1] = inl(devpriv->timer + ADDI_TCW_TIMEBASE_REG);
+               data[2] = inl(devpriv->timer + ADDI_TCW_RELOAD_REG);
+               break;
+       default:
+               return -EINVAL;
+       }
+
+       return insn->n;
+}
+
+static int apci1564_timer_insn_write(struct comedi_device *dev,
+                                    struct comedi_subdevice *s,
+                                    struct comedi_insn *insn,
+                                    unsigned int *data)
+{
+       struct apci1564_private *devpriv = dev->private;
+
+       /* just write the last last to the reload register */
+       if (insn->n) {
+               unsigned int val = data[insn->n - 1];
+
+               outl(val, devpriv->timer + ADDI_TCW_RELOAD_REG);
+       }
+
+       return insn->n;
+}
+
+static int apci1564_timer_insn_read(struct comedi_device *dev,
+                                   struct comedi_subdevice *s,
+                                   struct comedi_insn *insn,
+                                   unsigned int *data)
+{
+       struct apci1564_private *devpriv = dev->private;
+       int i;
+
+       /* return the actual value of the timer */
+       for (i = 0; i < insn->n; i++)
+               data[i] = inl(devpriv->timer + ADDI_TCW_VAL_REG);
+
+       return insn->n;
+}
+
 static int apci1564_auto_attach(struct comedi_device *dev,
                                unsigned long context_unused)
 {