Skip to contentSkip to conversation

staging: comedi: addi_apci_1564: miscellaneous cleanups

PATCH In review Chase Southwood 2 participants 5 messages Aug 12, 2014 Staging

lore
Share & tools
lore
#0 lore raw

cover staging: comedi: addi_apci_1564: miscellaneous cleanups

This patchset removes an uncorrect and unneeded insn_config operation, strips out remaining in-driver watchdog subdevice code, and cleans up the driver's register map defines.

Chase Southwood (3):
staging: comedi: addi_apci_1564: remove apci1564_do_config
staging: comedi: addi_apci_1564: Remove in-driver watchdog support
code
staging: comedi: addi_apci_1564: tidy register map defines

 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 158 +++++++--------------
 drivers/staging/comedi/drivers/addi_apci_1564.c    |  20 +--
 2 files changed, 60 insertions(+), 118 deletions(-)
Signature · 1 line
2.0.4
#1 lore raw

PATCH 1/3 staging: comedi: addi_apci_1564: remove apci1564_do_config

The DO config function served the purpose of configuring the diagnostic interrupts for the board. As the driver currently does not support diagnostic interrupts, the digital output subdevice does not need an insn_config operation and this function can be safely removed.

Cc Ian Abbott <[email protected]>
Cc H Hartley Sweeten <[email protected]>
---
 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 34 ----------------------
 drivers/staging/comedi/drivers/addi_apci_1564.c    |  1 -
 2 files changed, 35 deletions(-)
drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c +0 -34
index 8a613ae..0ce1deb 100644
@@ -88,40 +88,6 @@
 #define APCI1564_TCW_WARN_TIMEBASE_REG(x)		(0x1c + ((x) * 0x20))
 
 /*
- * Configures The Digital Output Subdevice.
- *
- * data[1] 0 = Disable VCC Interrupt, 1 = Enable VCC Interrupt
- * data[2] 0 = Disable CC Interrupt, 1 = Enable CC Interrupt
- */
-static int apci1564_do_config(struct comedi_device *dev,
-			      struct comedi_subdevice *s,
-			      struct comedi_insn *insn,
-			      unsigned int *data)
-{
-	struct apci1564_private *devpriv = dev->private;
-	unsigned int ul_Command = 0;
-
-	if ((data[0] != 0) && (data[0] != 1)) {
-		dev_err(dev->class_dev, "Data should be 1 or 0\n");
-		return -EINVAL;
-	}
-
-	if (data[1] == 1)
-		ul_Command = ul_Command | 0x1;
-	else
-		ul_Command = ul_Command & 0xFFFFFFFE;
-
-	if (data[2] == 1)
-		ul_Command = ul_Command | 0x2;
-	else
-		ul_Command = ul_Command & 0xFFFFFFFD;
-
-	outl(ul_Command, devpriv->amcc_iobase + APCI1564_DO_INT_CTRL_REG);
-	devpriv->tsk_current = current;
-	return insn->n;
-}
-
-/*
  * Configures The Timer, Counter or Watchdog
  *
  * data[0] Configure as: 0 = Timer, 1 = Counter, 2 = Watchdog
drivers/staging/comedi/drivers/addi_apci_1564.c +0 -1
index 543cb07..555e0a9 100644
@@ -388,7 +388,6 @@ static int apci1564_auto_attach(struct comedi_device *dev,
 	s->n_chan = 32;
 	s->maxdata = 1;
 	s->range_table = &range_digital;
-	s->insn_config = apci1564_do_config;
 	s->insn_bits = apci1564_do_insn_bits;
 
 	/* Change-Of-State (COS) interrupt subdevice */
Signature · 1 line
2.0.4
#2 lore raw

PATCH 2/3 staging: comedi: addi_apci_1564: Remove in-driver watchdog support code

In reply to Chase Southwood

Starting with commit 1496e5961113 ("staging: comedi: addi_apci_1564: use addi_watchdog module to init watchdog subdevice"), this driver uses the addi_watchdog module to provide support for the watchdog subdevice. Any remaining watchdog code in-driver can and should be removed.

This will also make future work on the timer and counter subdevices easier.

Cc Ian Abbott <[email protected]>
Cc H Hartley Sweeten <[email protected]>
---
 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 54 +++++-----------------
 1 file changed, 12 insertions(+), 42 deletions(-)
drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c +12 -42
index 0ce1deb..f8d8cc9 100644
@@ -88,9 +88,9 @@
 #define APCI1564_TCW_WARN_TIMEBASE_REG(x)		(0x1c + ((x) * 0x20))
 
 /*
- * Configures The Timer, Counter or Watchdog
+ * Configures The Timer or Counter
  *
- * data[0] Configure as: 0 = Timer, 1 = Counter, 2 = Watchdog
+ * data[0] Configure as: 0 = Timer, 1 = Counter
  * data[1] 1 = Enable Interrupt, 0 = Disable Interrupt
  * data[2] Time Unit
  * data[3] Reload Value
@@ -107,14 +107,7 @@ static int apci1564_timer_config(struct comedi_device *dev,
 	unsigned int ul_Command1 = 0;
 
 	devpriv->tsk_current = current;
-	if (data[0] == ADDIDATA_WATCHDOG) {
-		devpriv->timer_select_mode = ADDIDATA_WATCHDOG;
-
-		/* Disable the watchdog */
-		outl(0x0, devpriv->amcc_iobase + APCI1564_WDOG_CTRL_REG);
-		/* Loading the Reload value */
-		outl(data[3], devpriv->amcc_iobase + APCI1564_WDOG_RELOAD_REG);
-	} else if (data[0] == ADDIDATA_TIMER) {
+	if (data[0] == ADDIDATA_TIMER) {
 		/* First Stop The Timer */
 		ul_Command1 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
 		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
@@ -187,14 +180,13 @@ static int apci1564_timer_config(struct comedi_device *dev,
 	} else {
 		dev_err(dev->class_dev, "Invalid subdevice.\n");
 	}
-
 	return insn->n;
 }
 
 /*
- * Start / Stop The Selected Timer, Counter or Watchdog
+ * Start / Stop The Selected Timer or Counter
  *
- * data[0] Configure as: 0 = Timer, 1 = Counter, 2 = Watchdog
+ * data[0] Configure as: 0 = Timer, 1 = Counter
  * data[1] 0 = Stop, 1 = Start, 2 = Trigger Clear (Only Counter)
  */
 static int apci1564_timer_write(struct comedi_device *dev,
@@ -205,23 +197,6 @@ static int apci1564_timer_write(struct comedi_device *dev,
 	struct apci1564_private *devpriv = dev->private;
 	unsigned int ul_Command1 = 0;
 
-	if (devpriv->timer_select_mode == ADDIDATA_WATCHDOG) {
-		switch (data[1]) {
-		case 0:	/* stop the watchdog */
-			/* disable the watchdog */
-			outl(0x0, devpriv->amcc_iobase + APCI1564_WDOG_CTRL_REG);
-			break;
-		case 1:	/* start the watchdog */
-			outl(0x0001, devpriv->amcc_iobase + APCI1564_WDOG_CTRL_REG);
-			break;
-		case 2:	/* Software trigger */
-			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->timer_select_mode == ADDIDATA_TIMER) {
 		if (data[1] == 1) {
 			ul_Command1 = inl(devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
@@ -236,8 +211,7 @@ static int apci1564_timer_write(struct comedi_device *dev,
 			ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
 			outl(ul_Command1, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
 		}
-	}
-	if (devpriv->timer_select_mode == ADDIDATA_COUNTER) {
+	} else if (devpriv->timer_select_mode == ADDIDATA_COUNTER) {
 		ul_Command1 =
 			inl(dev->iobase +
 			    APCI1564_TCW_CTRL_REG(devpriv->mode_select_register - 1));
@@ -254,12 +228,14 @@ static int apci1564_timer_write(struct comedi_device *dev,
 		}
 		outl(ul_Command1, dev->iobase +
 		     APCI1564_TCW_CTRL_REG(devpriv->mode_select_register - 1));
+	} else {
+		dev_err(dev->class_dev, "Invalid subdevice.\n");
 	}
 	return insn->n;
 }
 
 /*
- * Read The Selected Timer, Counter or Watchdog
+ * Read The Selected Timer or Counter
  */
 static int apci1564_timer_read(struct comedi_device *dev,
 			       struct comedi_subdevice *s,
@@ -269,11 +245,7 @@ static int apci1564_timer_read(struct comedi_device *dev,
 	struct apci1564_private *devpriv = dev->private;
 	unsigned int ul_Command1 = 0;
 
-	if (devpriv->timer_select_mode == ADDIDATA_WATCHDOG) {
-		/*  Stores the status of the Watchdog */
-		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) {
+	if (devpriv->timer_select_mode == ADDIDATA_TIMER) {
 		/*  Stores the status of the Timer */
 		data[0] = inl(devpriv->amcc_iobase + APCI1564_TIMER_STATUS_REG) & 0x1;
 
@@ -299,10 +271,8 @@ static int apci1564_timer_read(struct comedi_device *dev,
 
 		/* Get the overflow status */
 		data[4] = (unsigned char) ((ul_Command1 >> 0) & 1);
-	} 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");
+	} else {
+		dev_err(dev->class_dev, "Invalid subdevice.\n");
 	}
 	return insn->n;
 }
Signature · 1 line
2.0.4
#3 lore raw

PATCH 3/3 staging: comedi: addi_apci_1564: tidy register map defines

In reply to Chase Southwood

This commit performs a final tidying of the register map defines, bringing them to a state that is ready for merging into addi_apci_1564.c when the time comes. Actions performed include:

*Removes the APCI1564_ADDRESS_RANGE macro, which is no longer needed/used. *Renames the APCI1564_DIGITAL_OP_{VCC,CC}_INTERRUPT_{ENABLE,DISABLE} macros to shorter names which are more consistent with the digital input interrupt macros.
*Fixes a typo in a comment (dev>iobase changed to dev->iobase). *Renames the APCI1564_TCW_* macros to APCI1564_COUNTER_* names to more accurately reflect that they are only offsets to counter registers (since only the counters are offset from dev->iobase).

Cc Ian Abbott <[email protected]>
Cc H Hartley Sweeten <[email protected]>
---
 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 70 ++++++++++++----------
 drivers/staging/comedi/drivers/addi_apci_1564.c    | 19 +++---
 2 files changed, 48 insertions(+), 41 deletions(-)
drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c +37 -33
index f8d8cc9..198c627 100644
@@ -21,8 +21,6 @@
  *
  */
 
-#define APCI1564_ADDRESS_RANGE				128
-
 /* Digital Input IRQ Function Selection */
 #define APCI1564_DI_INT_OR				(0 << 1)
 #define APCI1564_DI_INT_AND				(1 << 1)
@@ -32,10 +30,10 @@
 #define APCI1564_DI_INT_DISABLE				0xfffffffb
 
 /* Digital Output Interrupt Enable Disable. */
-#define APCI1564_DIGITAL_OP_VCC_INTERRUPT_ENABLE	0x1
-#define APCI1564_DIGITAL_OP_VCC_INTERRUPT_DISABLE	0xfffffffe
-#define APCI1564_DIGITAL_OP_CC_INTERRUPT_ENABLE		0x2
-#define APCI1564_DIGITAL_OP_CC_INTERRUPT_DISABLE	0xfffffffd
+#define APCI1564_DO_VCC_INT_ENABLE			0x1
+#define APCI1564_DO_VCC_INT_DISABLE			0xfffffffe
+#define APCI1564_DO_CC_INT_ENABLE			0x2
+#define APCI1564_DO_CC_IN_DISABLE			0xfffffffd
 
 /* TIMER COUNTER WATCHDOG DEFINES */
 #define ADDIDATA_TIMER					0
@@ -76,16 +74,16 @@
 #define APCI1564_TIMER_WARN_TIMEBASE_REG		0x64
 
 /*
- * dev>iobase Register Map
+ * dev->iobase Register Map
  */
-#define APCI1564_TCW_REG(x)				(0x00 + ((x) * 0x20))
-#define APCI1564_TCW_RELOAD_REG(x)			(0x04 + ((x) * 0x20))
-#define APCI1564_TCW_TIMEBASE_REG(x)			(0x08 + ((x) * 0x20))
-#define APCI1564_TCW_CTRL_REG(x)			(0x0c + ((x) * 0x20))
-#define APCI1564_TCW_STATUS_REG(x)			(0x10 + ((x) * 0x20))
-#define APCI1564_TCW_IRQ_REG(x)				(0x14 + ((x) * 0x20))
-#define APCI1564_TCW_WARN_TIMEVAL_REG(x)		(0x18 + ((x) * 0x20))
-#define APCI1564_TCW_WARN_TIMEBASE_REG(x)		(0x1c + ((x) * 0x20))
+#define APCI1564_COUNTER_REG(x)				(0x00 + ((x) * 0x20))
+#define APCI1564_COUNTER_RELOAD_REG(x)			(0x04 + ((x) * 0x20))
+#define APCI1564_COUNTER_TIMEBASE_REG(x)		(0x08 + ((x) * 0x20))
+#define APCI1564_COUNTER_CTRL_REG(x)			(0x0c + ((x) * 0x20))
+#define APCI1564_COUNTER_STATUS_REG(x)			(0x10 + ((x) * 0x20))
+#define APCI1564_COUNTER_IRQ_REG(x)			(0x14 + ((x) * 0x20))
+#define APCI1564_COUNTER_WARN_TIMEVAL_REG(x)		(0x18 + ((x) * 0x20))
+#define APCI1564_COUNTER_WARN_TIMEBASE_REG(x)		(0x1c + ((x) * 0x20))
 
 /*
  * Configures The Timer or Counter
@@ -121,14 +119,14 @@ static int apci1564_timer_config(struct comedi_device *dev,
 			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,
-			     dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER2));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER3));
-			outl(0x0,
-			     dev->iobase + APCI1564_TCW_IRQ_REG(APCI1564_COUNTER4));
+			outl(0x0, dev->iobase +
+			    APCI1564_COUNTER_IRQ_REG(APCI1564_COUNTER1));
+			outl(0x0, dev->iobase +
+			    APCI1564_COUNTER_IRQ_REG(APCI1564_COUNTER2));
+			outl(0x0, dev->iobase +
+			    APCI1564_COUNTER_IRQ_REG(APCI1564_COUNTER3));
+			outl(0x0, dev->iobase +
+			    APCI1564_COUNTER_IRQ_REG(APCI1564_COUNTER4));
 		} else {
 			/* disable Timer interrupt */
 			outl(0x0, devpriv->amcc_iobase + APCI1564_TIMER_CTRL_REG);
@@ -149,13 +147,16 @@ static int apci1564_timer_config(struct comedi_device *dev,
 		devpriv->mode_select_register = data[5];
 
 		/* First Stop The Counter */
-		ul_Command1 = inl(dev->iobase + APCI1564_TCW_CTRL_REG(data[5] - 1));
+		ul_Command1 = inl(dev->iobase +
+				 APCI1564_COUNTER_CTRL_REG(data[5] - 1));
 		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
 		/* Stop The Timer */
-		outl(ul_Command1, dev->iobase + APCI1564_TCW_CTRL_REG(data[5] - 1));
+		outl(ul_Command1, dev->iobase +
+					APCI1564_COUNTER_CTRL_REG(data[5] - 1));
 
 		/* Set the reload value */
-		outl(data[3], dev->iobase + APCI1564_TCW_RELOAD_REG(data[5] - 1));
+		outl(data[3], dev->iobase +
+					APCI1564_COUNTER_RELOAD_REG(data[5] - 1));
 
 		/* Set the mode :             */
 		/* - Disable the hardware     */
@@ -168,15 +169,18 @@ static int apci1564_timer_config(struct comedi_device *dev,
 		ul_Command1 =
 			(ul_Command1 & 0xFFFC19E2UL) | 0x80000UL |
 			(unsigned int) ((unsigned int) data[4] << 16UL);
-		outl(ul_Command1, dev->iobase + APCI1564_TCW_CTRL_REG(data[5] - 1));
+		outl(ul_Command1, dev->iobase +
+					APCI1564_COUNTER_CTRL_REG(data[5] - 1));
 
 		/*  Enable or Disable Interrupt */
 		ul_Command1 = (ul_Command1 & 0xFFFFF9FD) | (data[1] << 1);
-		outl(ul_Command1, dev->iobase + APCI1564_TCW_CTRL_REG(data[5] - 1));
+		outl(ul_Command1, dev->iobase +
+					APCI1564_COUNTER_CTRL_REG(data[5] - 1));
 
 		/* Set the Up/Down selection */
 		ul_Command1 = (ul_Command1 & 0xFFFBF9FFUL) | (data[6] << 18);
-		outl(ul_Command1, dev->iobase + APCI1564_TCW_CTRL_REG(data[5] - 1));
+		outl(ul_Command1, dev->iobase +
+					APCI1564_COUNTER_CTRL_REG(data[5] - 1));
 	} else {
 		dev_err(dev->class_dev, "Invalid subdevice.\n");
 	}
@@ -214,7 +218,7 @@ static int apci1564_timer_write(struct comedi_device *dev,
 	} else if (devpriv->timer_select_mode == ADDIDATA_COUNTER) {
 		ul_Command1 =
 			inl(dev->iobase +
-			    APCI1564_TCW_CTRL_REG(devpriv->mode_select_register - 1));
+			   APCI1564_COUNTER_CTRL_REG(devpriv->mode_select_register - 1));
 		if (data[1] == 1) {
 			/* Start the Counter subdevice */
 			ul_Command1 = (ul_Command1 & 0xFFFFF9FFUL) | 0x1UL;
@@ -227,7 +231,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->mode_select_register - 1));
+		     APCI1564_COUNTER_CTRL_REG(devpriv->mode_select_register - 1));
 	} else {
 		dev_err(dev->class_dev, "Invalid subdevice.\n");
 	}
@@ -255,10 +259,10 @@ static int apci1564_timer_read(struct comedi_device *dev,
 		/*  Read the Counter Actual Value. */
 		data[0] =
 			inl(dev->iobase +
-			    APCI1564_TCW_REG(devpriv->mode_select_register - 1));
+			    APCI1564_COUNTER_REG(devpriv->mode_select_register - 1));
 		ul_Command1 =
 			inl(dev->iobase +
-			    APCI1564_TCW_STATUS_REG(devpriv->mode_select_register - 1));
+			    APCI1564_COUNTER_STATUS_REG(devpriv->mode_select_register - 1));
 
 		/* Get the software trigger status */
 		data[1] = (unsigned char) ((ul_Command1 >> 1) & 1);
drivers/staging/comedi/drivers/addi_apci_1564.c +11 -8
index 555e0a9..16c02c8 100644
@@ -42,10 +42,10 @@ static int apci1564_reset(struct comedi_device *dev)
 	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));
+	outl(0x0, dev->iobase + APCI1564_COUNTER_CTRL_REG(APCI1564_COUNTER1));
+	outl(0x0, dev->iobase + APCI1564_COUNTER_CTRL_REG(APCI1564_COUNTER2));
+	outl(0x0, dev->iobase + APCI1564_COUNTER_CTRL_REG(APCI1564_COUNTER3));
+	outl(0x0, dev->iobase + APCI1564_COUNTER_CTRL_REG(APCI1564_COUNTER4));
 
 	return 0;
 }
@@ -94,17 +94,20 @@ static irqreturn_t apci1564_interrupt(int irq, void *d)
 	}
 
 	for (chan = 0; chan < 4; chan++) {
-		status = inl(dev->iobase + APCI1564_TCW_IRQ_REG(chan));
+		status = inl(dev->iobase + APCI1564_COUNTER_IRQ_REG(chan));
 		if (status & 0x01) {
 			/*  Disable Counter Interrupt */
-			ctrl = inl(dev->iobase + APCI1564_TCW_CTRL_REG(chan));
-			outl(0x0, dev->iobase + APCI1564_TCW_CTRL_REG(chan));
+			ctrl = inl(dev->iobase +
+				  APCI1564_COUNTER_CTRL_REG(chan));
+			outl(0x0, dev->iobase +
+			    APCI1564_COUNTER_CTRL_REG(chan));
 
 			/* Send a signal to from kernel to user space */
 			send_sig(SIGIO, devpriv->tsk_current, 0);
 
 			/*  Enable Counter Interrupt */
-			outl(ctrl, dev->iobase + APCI1564_TCW_CTRL_REG(chan));
+			outl(ctrl, dev->iobase +
+			    APCI1564_COUNTER_CTRL_REG(chan));
 		}
 	}
 
Signature · 1 line
2.0.4
Reviewed-by on the v1 series
#4 lore raw
In reply to Chase Southwood
Quoted 10 lines · Chase Southwood

This patchset removes an uncorrect and unneeded insn_config operation, strips out remaining in-driver watchdog subdevice code, and cleans up the driver's register map defines.

Chase Southwood (3):
staging: comedi: addi_apci_1564: remove apci1564_do_config
staging: comedi: addi_apci_1564: Remove in-driver watchdog support
code
staging: comedi: addi_apci_1564: tidy register map defines

Quoted 4 lines
  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 158 +++++++--------------
  drivers/staging/comedi/drivers/addi_apci_1564.c    |  20 +--
  2 files changed, 60 insertions(+), 118 deletions(-)
Signature · 2 lines
-=( Ian Abbott @ MEV Ltd.    E-mail: <[email protected]>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-