[PATCH 1/2] Staging: comedi: addi-data: fix lines that are over 80 characters

[PATCH 1/2] Staging: comedi: addi-data: fix lines that are over 80 characters

[Chase Southwood]

hwdrv_apci1564.c had numerous lines over the column limit.  This patch
splits all such lines to bring them in compliance with coding style.

Signed-off-by: Chase Southwood <chase.southwoo@yahoo.com>
---
 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 50 ++++++++++++++++------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 2b47fa1..77030c5 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -324,11 +324,15 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
 			APCI1564_TCW_PROG);
 		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Stop The Timer */
+		/* Stop The Timer */
+		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER +
+			APCI1564_TCW_PROG);
 
 		devpriv->b_TimerSelectMode = ADDIDATA_TIMER;
 		if (data[1] == 1) {
-			outl(0x02, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Enable TIMER int & DISABLE ALL THE OTHER int SOURCES */
+			/* Enable TIMER int & DISABLE ALL THE OTHER int SOURCES */
+			outl(0x02, devpriv->i_IobaseAmcc + APCI1564_TIMER +
+				APCI1564_TCW_PROG);
 			outl(0x0,
 				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
 				APCI1564_DIGITAL_IP_IRQ);
@@ -352,7 +356,9 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 				devpriv->iobase + APCI1564_COUNTER4 +
 				APCI1564_TCW_IRQ);
 		} else {
-			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* disable Timer interrupt */
+			/* disable Timer interrupt */
+			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER +
+				APCI1564_TCW_PROG);
 		}
 
 		/*  Loading Timebase */
@@ -370,7 +376,9 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 			APCI1564_TCW_PROG);
 		ul_Command1 =
 			(ul_Command1 & 0xFFF719E2UL) | 2UL << 13UL | 0x10UL;
-		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* mode 2 */
+		/* mode 2 */
+		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER +
+			APCI1564_TCW_PROG);
 	} else if (data[0] == ADDIDATA_COUNTER) {
 		devpriv->b_TimerSelectMode = ADDIDATA_COUNTER;
 		devpriv->b_ModeSelectRegister = data[5];
@@ -380,7 +388,9 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 			inl(devpriv->iobase + ((data[5] - 1) * 0x20) +
 			APCI1564_TCW_PROG);
 		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-		outl(ul_Command1, devpriv->iobase + ((data[5] - 1) * 0x20) + APCI1564_TCW_PROG);	/* Stop The Timer */
+		/* Stop The Timer */
+		outl(ul_Command1, devpriv->iobase + ((data[5] - 1) * 0x20) +
+			APCI1564_TCW_PROG);
 
 		/* Set the reload value */
 		outl(data[3],
@@ -457,7 +467,9 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		switch (data[1]) {
 		case 0:	/* stop the watchdog */
-			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);	/* disable the watchdog */
+			/* disable the watchdog */
+			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
+				APCI1564_TCW_PROG);
 			break;
 		case 1:	/* start the watchdog */
 			outl(0x0001,
@@ -678,13 +690,18 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 			inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
 			APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
 		ui_InterruptStatus_1564 = ui_InterruptStatus_1564 & 0X000FFFF0;
-		send_sig(SIGIO, devpriv->tsk_Current, 0);	/*  send signal to the sample */
-		outl(ui_DI, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + APCI1564_DIGITAL_IP_IRQ);	/* enable the interrupt */
+		/*  send signal to the sample */
+		send_sig(SIGIO, devpriv->tsk_Current, 0);
+		/* enable the interrupt */
+		outl(ui_DI, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
+			APCI1564_DIGITAL_IP_IRQ);
 		return;
 	}
 
 	if (ui_DO == 1) {
-		/*  Check for Digital Output interrupt Type - 1: Vcc interrupt 2: CC interrupt. */
+		/*  Check for Digital Output interrupt Type	*/
+		/*  1: Vcc interrupt						*/
+		/*  2: CC interrupt							*/
 		ui_Type =
 			inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
 			APCI1564_DIGITAL_OP_INTERRUPT_STATUS) & 0x3;
@@ -829,14 +846,19 @@ static int i_APCI1564_Reset(struct comedi_device *dev)
 {
 	struct addi_private *devpriv = dev->private;
 
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_IRQ);	/* disable the interrupts */
-	inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_STATUS);	/* Reset the interrupt status register */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1);	/* Disable the and/or interrupt */
+	/* disable the interrupts */
+	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_IRQ);
+	/* Reset the interrupt status register */
+	inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
+	/* Disable the and/or interrupt */
+	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
 	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
 	devpriv->b_DigitalOutputRegister = 0;
 	ui_Type = 0;
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP);	/* Resets the output channels */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_INTERRUPT);	/* Disables the interrupt. */
+	/* Resets the output channels */
+	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP);
+	/* Disables the interrupt. */
+	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_INTERRUPT);
 	outl(0x0,
 		devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
 		APCI1564_TCW_RELOAD_VALUE);
-- 
1.8.5.3

[PATCH 2/2] Staging: comedi: addi-data: remove unnecessary variable initializations in hwdrv_apci1564.c

[Chase Southwood]

A handful of variables here were being initialized to 0 upon declaration,
however they are always then set to another value before their first use,
so initialization here is useless and we can remove it.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
 drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 968e26c..83e4a41 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -304,7 +304,7 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 						 unsigned int *data)
 {
 	struct addi_private *devpriv = dev->private;
-	unsigned int ul_Command1 = 0;
+	unsigned int ul_Command1;
 
 	devpriv->tsk_Current = current;
 	if (data[0] == ADDIDATA_WATCHDOG) {
@@ -462,7 +462,7 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 							 unsigned int *data)
 {
 	struct addi_private *devpriv = dev->private;
-	unsigned int ul_Command1 = 0;
+	unsigned int ul_Command1;
 
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		switch (data[1]) {
@@ -560,7 +560,7 @@ static int i_APCI1564_ReadTimerCounterWatchdog(struct comedi_device *dev,
 					       unsigned int *data)
 {
 	struct addi_private *devpriv = dev->private;
-	unsigned int ul_Command1 = 0;
+	unsigned int ul_Command1;
 
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		/*  Stores the status of the Watchdog */
@@ -658,7 +658,7 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 	unsigned int ui_DO, ui_DI;
 	unsigned int ui_Timer;
 	unsigned int ui_C1, ui_C2, ui_C3, ui_C4;
-	unsigned int ul_Command2 = 0;
+	unsigned int ul_Command2;
 
 	ui_DI = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
 		APCI1564_DIGITAL_IP_IRQ) & 0x01;
-- 
1.8.5.3

Re: [PATCH 1/2] Staging: comedi: addi-data: fix lines that are over 80 characters

[Dan Carpenter]

On Fri, Feb 28, 2014 at 01:31:20AM -0600, Chase Southwood wrote:
> hwdrv_apci1564.c had numerous lines over the column limit.  This patch
> splits all such lines to bring them in compliance with coding style.
> 
> Signed-off-by: Chase Southwood <chase.southwoo@yahoo.com>
> ---
>  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 50 ++++++++++++++++------
>  1 file changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> index 2b47fa1..77030c5 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> @@ -324,11 +324,15 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
>  			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
>  			APCI1564_TCW_PROG);
>  		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
> -		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Stop The Timer */
> +		/* Stop The Timer */
> +		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER +
> +			APCI1564_TCW_PROG);

Just make a helper function so that you can call it like this:

static void outl_1564_timer(struct addi_private *devpriv, unsigned int cmd,
			    unsigned int reg)
{
	outl(cmd, devpriv->i_IobaseAmcc + APCI1564_TIMER, reg);
}

Then the caller becomes:

		outl_1564_timer(devpriv, cmd, APCI1564_TCW_PROG);

regards,
dan carpenter

Re: [PATCH 1/2] Staging: comedi: addi-data: fix lines that are over 80 characters

[Dan Carpenter]

On Fri, Feb 28, 2014 at 10:52:32AM +0300, Dan Carpenter wrote:
> On Fri, Feb 28, 2014 at 01:31:20AM -0600, Chase Southwood wrote:
> > hwdrv_apci1564.c had numerous lines over the column limit.  This patch
> > splits all such lines to bring them in compliance with coding style.
> > 
> > Signed-off-by: Chase Southwood <chase.southwoo@yahoo.com>
> > ---
> >  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 50 ++++++++++++++++------
> >  1 file changed, 36 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> > index 2b47fa1..77030c5 100644
> > --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> > +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> > @@ -324,11 +324,15 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
> >  			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
> >  			APCI1564_TCW_PROG);
> >  		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
> > -		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Stop The Timer */
> > +		/* Stop The Timer */
> > +		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER +
> > +			APCI1564_TCW_PROG);
> 
> Just make a helper function so that you can call it like this:
> 
> static void outl_1564_timer(struct addi_private *devpriv, unsigned int cmd,
> 			    unsigned int reg)
> {
> 	outl(cmd, devpriv->i_IobaseAmcc + APCI1564_TIMER, reg);
                                                        ^^^
Should be "devpriv->i_IobaseAmcc + APCI1564_TIMER + reg" obviously.

regards,
dan carpenter

Re: [PATCH 1/2] Staging: comedi: addi-data: fix lines that are over 80 characters

[Chase Southwood]

>On Friday, February 28, 2014 1:57 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:

>>On Fri, Feb 28, 2014 at 10:52:32AM +0300, Dan Carpenter wrote:
>>> On Fri, Feb 28, 2014 at 01:31:20AM -0600, Chase Southwood wrote:
>> > hwdrv_apci1564.c had numerous lines over the column limit.  This patch
>> > splits all such lines to bring them in compliance with coding style.
>> > 
>> > Signed-off-by: Chase Southwood <chase.southwoo@yahoo.com>
>> > ---
>> >  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 50 ++++++++++++++++------
>> >  1 file changed, 36 insertions(+), 14 deletions(-)
>> > 
>> > diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
>> > index 2b47fa1..77030c5 100644
>> > --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
>> > +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
>> > @@ -324,11 +324,15 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
>> >              inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
>> >              APCI1564_TCW_PROG);
>> >          ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
>> > -        outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);    /* Stop The Timer */
>> > +        /* Stop The Timer */
>> > +        outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER +
>> > +            APCI1564_TCW_PROG);
>> 
>> Just make a helper function so that you can call it like this:
>> 
>> static void outl_1564_timer(struct addi_private *devpriv, unsigned int cmd,
>>                 unsigned int reg)
>> {
>>     outl(cmd, devpriv->i_IobaseAmcc + APCI1564_TIMER, reg);
>                                                        ^^^
>Should be "devpriv->i_IobaseAmcc + APCI1564_TIMER + reg" obviously.
>
>
>regards,
>dan carpenter

You got it, I'll do it this way and resend.

Thanks,
Chase

[PATCH 1/2 v2] Staging: comedi: fix lines that are over 80 characters

[Chase Southwood]

This patch introduces a simple helper function, outl_1564_timer(), to
allow several lines which violate the character limit to be shortened.
A handful of other lines that are too long are appropriately split as
well.

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
2: introduced outl_1564_timer() at the suggestion of Dan.
 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 83 +++++++++++++---------
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 2b47fa1..d958d3c 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -99,6 +99,9 @@ This program is distributed in the hope that it will be useful, but WITHOUT ANY
 #define APCI1564_TCW_WARN_TIMEVAL			24
 #define APCI1564_TCW_WARN_TIMEBASE			28
 
+/* Helper functions */
+static void outl_1564_timer(struct addi_private *, unsigned int, unsigned int);
+
 /* Global variables */
 static unsigned int ui_InterruptStatus_1564;
 static unsigned int ui_InterruptData, ui_Type;
@@ -324,11 +327,13 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
 			APCI1564_TCW_PROG);
 		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Stop The Timer */
+		/* Stop The Timer */
+		outl_1564_timer(devpriv, ul_Command1, APCI1564_TCW_PROG);
 
 		devpriv->b_TimerSelectMode = ADDIDATA_TIMER;
 		if (data[1] == 1) {
-			outl(0x02, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Enable TIMER int & DISABLE ALL THE OTHER int SOURCES */
+			/* Enable TIMER int & DISABLE ALL THE OTHER int SOURCES */
+			outl_1564_timer(devpriv, 0x02, APCI1564_TCW_PROG);
 			outl(0x0,
 				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
 				APCI1564_DIGITAL_IP_IRQ);
@@ -352,25 +357,23 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 				devpriv->iobase + APCI1564_COUNTER4 +
 				APCI1564_TCW_IRQ);
 		} else {
-			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* disable Timer interrupt */
+			/* disable Timer interrupt */
+			outl_1564_timer(devpriv, 0x0, APCI1564_TCW_PROG);
 		}
 
 		/*  Loading Timebase */
-		outl(data[2],
-			devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_TIMEBASE);
+		outl_1564_timer(devpriv, data[2], APCI1564_TCW_TIMEBASE);
 
 		/* Loading the Reload value */
-		outl(data[3],
-			devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_RELOAD_VALUE);
+		outl_1564_timer(devpriv, data[3], APCI1564_TCW_RELOAD_VALUE);
 
 		ul_Command1 =
 			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
 			APCI1564_TCW_PROG);
 		ul_Command1 =
 			(ul_Command1 & 0xFFF719E2UL) | 2UL << 13UL | 0x10UL;
-		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* mode 2 */
+		/* mode 2 */
+		outl_1564_timer(devpriv, ul_Command1, APCI1564_TCW_PROG);
 	} else if (data[0] == ADDIDATA_COUNTER) {
 		devpriv->b_TimerSelectMode = ADDIDATA_COUNTER;
 		devpriv->b_ModeSelectRegister = data[5];
@@ -380,7 +383,9 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 			inl(devpriv->iobase + ((data[5] - 1) * 0x20) +
 			APCI1564_TCW_PROG);
 		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-		outl(ul_Command1, devpriv->iobase + ((data[5] - 1) * 0x20) + APCI1564_TCW_PROG);	/* Stop The Timer */
+		/* Stop The Timer */
+		outl(ul_Command1, devpriv->iobase + ((data[5] - 1) * 0x20) +
+			APCI1564_TCW_PROG);
 
 		/* Set the reload value */
 		outl(data[3],
@@ -457,7 +462,9 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		switch (data[1]) {
 		case 0:	/* stop the watchdog */
-			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);	/* disable the watchdog */
+			/* disable the watchdog */
+			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
+				APCI1564_TCW_PROG);
 			break;
 		case 1:	/* start the watchdog */
 			outl(0x0001,
@@ -484,9 +491,7 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 			ul_Command1 = (ul_Command1 & 0xFFFFF9FFUL) | 0x1UL;
 
 			/* Enable the Timer */
-			outl(ul_Command1,
-				devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+			outl_1564_timer(devpriv, ul_Command1, APCI1564_TCW_PROG);
 		} else if (data[1] == 0) {
 			/* Stop The Timer */
 
@@ -494,9 +499,7 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 				inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
 				APCI1564_TCW_PROG);
 			ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-			outl(ul_Command1,
-				devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+			outl_1564_timer(devpriv, ul_Command1, APCI1564_TCW_PROG);
 		}
 	}
 	if (devpriv->b_TimerSelectMode == ADDIDATA_COUNTER) {
@@ -678,13 +681,18 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 			inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
 			APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
 		ui_InterruptStatus_1564 = ui_InterruptStatus_1564 & 0X000FFFF0;
-		send_sig(SIGIO, devpriv->tsk_Current, 0);	/*  send signal to the sample */
-		outl(ui_DI, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + APCI1564_DIGITAL_IP_IRQ);	/* enable the interrupt */
+		/*  send signal to the sample */
+		send_sig(SIGIO, devpriv->tsk_Current, 0);
+		/* enable the interrupt */
+		outl(ui_DI, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
+			APCI1564_DIGITAL_IP_IRQ);
 		return;
 	}
 
 	if (ui_DO == 1) {
-		/*  Check for Digital Output interrupt Type - 1: Vcc interrupt 2: CC interrupt. */
+		/*  Check for Digital Output interrupt Type	*/
+		/*  1: Vcc interrupt						*/
+		/*  2: CC interrupt							*/
 		ui_Type =
 			inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
 			APCI1564_DIGITAL_OP_INTERRUPT_STATUS) & 0x3;
@@ -705,18 +713,14 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 			ul_Command2 =
 				inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
 				    APCI1564_TCW_PROG);
-			outl(0x0,
-			     devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			     APCI1564_TCW_PROG);
+			outl_1564_timer(devpriv, 0x0, APCI1564_TCW_PROG);
 
 			/* 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 +
-			     APCI1564_TCW_PROG);
+			outl_1564_timer(devpriv, ul_Command2, APCI1564_TCW_PROG);
 		}
 	}
 
@@ -829,19 +833,24 @@ static int i_APCI1564_Reset(struct comedi_device *dev)
 {
 	struct addi_private *devpriv = dev->private;
 
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_IRQ);	/* disable the interrupts */
-	inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_STATUS);	/* Reset the interrupt status register */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1);	/* Disable the and/or interrupt */
+	/* disable the interrupts */
+	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_IRQ);
+	/* Reset the interrupt status register */
+	inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
+	/* Disable the and/or interrupt */
+	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
 	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
 	devpriv->b_DigitalOutputRegister = 0;
 	ui_Type = 0;
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP);	/* Resets the output channels */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_INTERRUPT);	/* Disables the interrupt. */
+	/* Resets the output channels */
+	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP);
+	/* Disables the interrupt. */
+	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_INTERRUPT);
 	outl(0x0,
 		devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
 		APCI1564_TCW_RELOAD_VALUE);
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER);
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);
+	outl_1564_timer(devpriv, 0x0, 0x0);
+	outl_1564_timer(devpriv, 0x0, APCI1564_TCW_PROG);
 
 	outl(0x0, devpriv->iobase + APCI1564_COUNTER1 + APCI1564_TCW_PROG);
 	outl(0x0, devpriv->iobase + APCI1564_COUNTER2 + APCI1564_TCW_PROG);
@@ -849,3 +858,9 @@ static int i_APCI1564_Reset(struct comedi_device *dev)
 	outl(0x0, devpriv->iobase + APCI1564_COUNTER4 + APCI1564_TCW_PROG);
 	return 0;
 }
+
+static void outl_1564_timer(struct addi_private *devpriv, unsigned int cmd,
+				unsigned int reg)
+{
+	outl(cmd, devpriv->i_IobaseAmcc + APCI1564_TIMER + reg);
+}
-- 
1.8.5.3

Re: [PATCH 1/2 v2] Staging: comedi: fix lines that are over 80 characters

[Dan Carpenter]

On Fri, Feb 28, 2014 at 03:15:45AM -0600, Chase Southwood wrote:
> This patch introduces a simple helper function, outl_1564_timer(), to
> allow several lines which violate the character limit to be shortened.
> A handful of other lines that are too long are appropriately split as
> well.
> 
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> 2: introduced outl_1564_timer() at the suggestion of Dan.
>  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 83 +++++++++++++---------
>  1 file changed, 49 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> index 2b47fa1..d958d3c 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> @@ -99,6 +99,9 @@ This program is distributed in the hope that it will be useful, but WITHOUT ANY
>  #define APCI1564_TCW_WARN_TIMEVAL			24
>  #define APCI1564_TCW_WARN_TIMEBASE			28
>  
> +/* Helper functions */

Obvious comment is too obvious.

> +static void outl_1564_timer(struct addi_private *, unsigned int, unsigned int);
> +

Avoid forward declarations.  Put the function itself here.

Do a function for APCI1564_DIGITAL_IP as well.

outl_1564_digital_ip()
inl_1564_digital_ip()

To be honest, I'm not sure how much the 1564 adds to the name.  The
names should match of course.

I wonder if this line is buggy?:
outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1); /* Disable the and/or interrupt */
Should it be?
outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + APCI1564_DIGITAL_IP_INTERRUPT_MODE1);

You would need to read the specs to be sure.

Anyway, take some more time with this and play with different ways to
clean it up.

regards,
dan carpenter

Re: [PATCH 1/2 v2] Staging: comedi: fix lines that are over 80 characters

[Chase Southwood]

>On Friday, February 28, 2014 3:42 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:

>>On Fri, Feb 28, 2014 at 03:15:45AM -0600, Chase Southwood wrote:
>> This patch introduces a simple helper function, outl_1564_timer(), to
>> allow several lines which violate the character limit to be shortened.
>> A handful of other lines that are too long are appropriately split as
>> well.
>> 
>> Cc: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
>> ---
>> 2: introduced outl_1564_timer() at the suggestion of Dan.
>>  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 83 +++++++++++++---------
>>  1 file changed, 49 insertions(+), 34 deletions(-)
>> 
>> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
>> index 2b47fa1..d958d3c 100644
>> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
>> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
>> @@ -99,6 +99,9 @@ This program is distributed in the hope that it will be useful, but WITHOUT ANY
>>  #define APCI1564_TCW_WARN_TIMEVAL            24
>>  #define APCI1564_TCW_WARN_TIMEBASE            28
>>  
>> +/* Helper functions */
>
>Obvious comment is too obvious.
>
>> +static void outl_1564_timer(struct addi_private *, unsigned int, unsigned int);
>> +
>
>Avoid forward declarations.  Put the function itself here.
>
>Do a function for APCI1564_DIGITAL_IP as well.
>
>outl_1564_digital_ip()
>inl_1564_digital_ip()
>
>To be honest, I'm not sure how much the 1564 adds to the name.  The
>names should match of course.
>
>I wonder if this line is buggy?:
>outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1); /* Disable the >and/or interrupt */
>Should it be?
>outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + >APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
>
>You would need to read the specs to be sure.
>
>Anyway, take some more time with this and play with different ways to
>clean it up.
>
>regards,
>
>dan carpenter

Hey Dan!

Thanks a lot, I really appreciate all of the really good feedback.  It's getting late now, but tomorrow I'll work on tidying everything up based on all of these comments and double check that hardware spec as well.

Thanks,
Chase

Re: [PATCH 1/2 v2] Staging: comedi: fix lines that are over 80 characters

[Greg KH]

On Fri, Feb 28, 2014 at 03:15:45AM -0600, Chase Southwood wrote:
> This patch introduces a simple helper function, outl_1564_timer(), to
> allow several lines which violate the character limit to be shortened.
> A handful of other lines that are too long are appropriately split as
> well.
> 
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> 2: introduced outl_1564_timer() at the suggestion of Dan.
>  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 83 +++++++++++++---------
>  1 file changed, 49 insertions(+), 34 deletions(-)

The Subject: doesn't match the patch content :(

Re: [PATCH 1/2 v2] Staging: comedi: fix lines that are over 80 characters

[Chase Southwood]

>On Friday, February 28, 2014 4:31 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

>>On Fri, Feb 28, 2014 at 03:15:45AM -0600, Chase Southwood wrote:
>>
>> This patch introduces a simple helper function, outl_1564_timer(), to
>> allow several lines which violate the character limit to be shortened.
>> A handful of other lines that are too long are appropriately split as
>> well.
>> 
>> Cc: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
>> ---
>> 2: introduced outl_1564_timer() at the suggestion of Dan.
>>  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 83 +++++++++++++---------
>>  1 file changed, 49 insertions(+), 34 deletions(-)>
>
>The Subject: doesn't match the patch content :(

Greg,
You're right and that's totally my bad!  In all honesty, I sent this as v2 of my original cleanup patch (without even
changing the subject :( ) when really, that was a mistake because it's pretty much a different patch entirely (although with
the same ultimate end goal).  It needs further changes (based on Dan's comments) anyway, so when I send in the next version,
the subject will be changed appropriately.  Sorry for my oversight, it won't happen again.

Thanks,
Chase

[PATCH 2/2 v2] Staging: comedi: addi-data: remove unnecessary variable initializations in hwdrv_apci1564.c

[Chase Southwood]

A handful of variables here were being initialized to 0 upon declaration,
however they are always then set to another value before their first use,
so initialization here is useless and we can remove it.

Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
2: no content change; redone and resent after PATCH 1/2 changed to ensure
clean applying.

 drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index d958d3c..c6e2d7e 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -307,7 +307,7 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 						 unsigned int *data)
 {
 	struct addi_private *devpriv = dev->private;
-	unsigned int ul_Command1 = 0;
+	unsigned int ul_Command1;
 
 	devpriv->tsk_Current = current;
 	if (data[0] == ADDIDATA_WATCHDOG) {
@@ -457,7 +457,7 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 							 unsigned int *data)
 {
 	struct addi_private *devpriv = dev->private;
-	unsigned int ul_Command1 = 0;
+	unsigned int ul_Command1;
 
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		switch (data[1]) {
@@ -551,7 +551,7 @@ static int i_APCI1564_ReadTimerCounterWatchdog(struct comedi_device *dev,
 					       unsigned int *data)
 {
 	struct addi_private *devpriv = dev->private;
-	unsigned int ul_Command1 = 0;
+	unsigned int ul_Command1;
 
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		/*  Stores the status of the Watchdog */
@@ -649,7 +649,7 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 	unsigned int ui_DO, ui_DI;
 	unsigned int ui_Timer;
 	unsigned int ui_C1, ui_C2, ui_C3, ui_C4;
-	unsigned int ul_Command2 = 0;
+	unsigned int ul_Command2;
 
 	ui_DI = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
 		APCI1564_DIGITAL_IP_IRQ) & 0x01;
-- 
1.8.5.3

[PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c

[Chase Southwood]

This patch introduces a handful of outl and inl helper functions with the
ultimate goal of improving code readability and allowing several lines
which violate the character limit to be shortened in a sane way.

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
This patchset serves as a replacement to my previous cleanup patchset for
hwdrv_apci1564.c

Dan,
After spending a little bit more time with this and trying out different
ways of cleaning this up, I decided that making helper functions for all
of the most common register sets would look the best, but I haven't made
a helper for a few of the least common inl/outl calls because if I did,
the sheer number of helper functions would get quite ridiculous.
Let me know if you think my selections of what to make into helper
functions seems appropriate.

Thanks,
Chase

 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 2b47fa1..282a744 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -99,6 +99,54 @@ This program is distributed in the hope that it will be useful, but WITHOUT ANY
 #define APCI1564_TCW_WARN_TIMEVAL			24
 #define APCI1564_TCW_WARN_TIMEBASE			28
 
+static void outl_1564_digital_ip(struct addi_private *devpriv, unsigned int cmd,
+						unsigned int reg)
+{
+	outl(cmd, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
+}
+
+static unsigned int inl_1564_digital_ip(struct addi_private *devpriv,
+						unsigned int reg)
+{
+	return inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
+}
+
+static void outl_1564_digital_op(struct addi_private *devpriv, unsigned int cmd,
+						unsigned int reg)
+{
+	outl(cmd, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP + reg);
+}
+
+static unsigned int inl_1564_digital_op(struct addi_private *devpriv,
+						unsigned int reg)
+{
+	return inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP + reg);
+}
+
+static void outl_1564_digital_op_watchdog(struct addi_private *devpriv,
+						unsigned int cmd, unsigned int reg)
+{
+	outl(cmd, devpriv->IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG + reg);
+}
+
+static unsigned int inl_1564_digital_op_watchdog(struct addi_private *devpriv,
+						unsigned int reg)
+{
+	return inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG + reg);
+}
+
+static void outl_1564_timer(struct addi_private *devpriv, unsigned int cmd,
+						unsigned int reg)
+{
+	outl(cmd, devpriv->i_IobaseAmcc + APCI1564_TIMER + reg);
+}
+
+static unsigned int inl_1564_timer(struct addi_private *devpriv,
+						unsigned int reg)
+{
+	return inl(devpriv->i_IobaseAmcc + APCI1564_TIMER + reg);
+}
+
 /* Global variables */
 static unsigned int ui_InterruptStatus_1564;
 static unsigned int ui_InterruptData, ui_Type;
-- 
1.8.5.3

Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c

[Dan Carpenter]

On Sat, Mar 01, 2014 at 04:28:27AM -0600, Chase Southwood wrote:
> This patch introduces a handful of outl and inl helper functions with the
> ultimate goal of improving code readability and allowing several lines
> which violate the character limit to be shortened in a sane way.
> 
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> This patchset serves as a replacement to my previous cleanup patchset for
> hwdrv_apci1564.c
> 
> Dan,
> After spending a little bit more time with this and trying out different
> ways of cleaning this up, I decided that making helper functions for all
> of the most common register sets would look the best, but I haven't made
> a helper for a few of the least common inl/outl calls because if I did,
> the sheer number of helper functions would get quite ridiculous.
> Let me know if you think my selections of what to make into helper
> functions seems appropriate.
> 

Yeah.  You're right...  It's kind of a lot of helper functions.

I wonder if we could just do something like:

static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
		      unsigned int reg)
{
	outl(cmd, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
}

And then change APCI1564_DIGITAL_IP_INTERRUPT_MODE1 to be:

#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4)

The only problem with that would be i_APCI1564_Reset().  Is
i_APCI1564_Reset() buggy?  Ian or Hartley might know.  Take a look at
other comedi drivers as well to see what they do.

regards,
dan carpenter

Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c

[Dan Carpenter]

On Sat, Mar 01, 2014 at 03:44:18PM +0300, Dan Carpenter wrote:
> On Sat, Mar 01, 2014 at 04:28:27AM -0600, Chase Southwood wrote:
> > This patch introduces a handful of outl and inl helper functions with the
> > ultimate goal of improving code readability and allowing several lines
> > which violate the character limit to be shortened in a sane way.
> > 
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> > ---
> > This patchset serves as a replacement to my previous cleanup patchset for
> > hwdrv_apci1564.c
> > 
> > Dan,
> > After spending a little bit more time with this and trying out different
> > ways of cleaning this up, I decided that making helper functions for all
> > of the most common register sets would look the best, but I haven't made
> > a helper for a few of the least common inl/outl calls because if I did,
> > the sheer number of helper functions would get quite ridiculous.
> > Let me know if you think my selections of what to make into helper
> > functions seems appropriate.
> > 
> 
> Yeah.  You're right...  It's kind of a lot of helper functions.
> 
> I wonder if we could just do something like:
> 
> static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
> 		      unsigned int reg)
> {
> 	outl(cmd, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
> }
> 

Gar...

static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
		      unsigned int reg)
{
	outl(cmd, devpriv->i_IobaseAmcc + reg);
}

regards,
dan carpenter

Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c

[Chase Southwood]

Hi Dan,


>On Saturday, March 1, 2014 6:46 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>On Sat, Mar 01, 2014 at 04:28:27AM -0600, Chase Southwood wrote:
>> This patch introduces a handful of outl and inl helper functions with the
>> ultimate goal of improving code readability and allowing several lines
>> which violate the character limit to be shortened in a sane way.
>> 
>> Cc: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
>> ---
>> This patchset serves as a replacement to my previous cleanup patchset for
>> hwdrv_apci1564.c
>> 
>> Dan,
>> After spending a little bit more time with this and trying out different
>> ways of cleaning this up, I decided that making helper functions for all
>> of the most common register sets would look the best, but I haven't made
>> a helper for a few of the least common inl/outl calls because if I did,
>> the sheer number of helper functions would get quite ridiculous.
>> Let me know if you think my selections of what to make into helper
>> functions seems appropriate.
>> 
>
>Yeah.  You're right...  It's kind of a lot of helper functions.
>
>I wonder if we could just do something like:
>
>static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
>              unsigned int reg)
>{
>    outl(cmd, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + reg);
>}
>
>And then change APCI1564_DIGITAL_IP_INTERRUPT_MODE1 to be:
>
>#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4)
>

I like this idea.  Just to clarify though, basically all of the macros would
change to something like 

#define APCI1564_DIGITAL_IP 0x4 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4) #define APCI1564_DIGITAL_IP_INTERRUPT_MODE2		 (0x4 + 0x8) #define APCI1564_DIGITAL_IP_IRQ (0x4 + 0x10) #define APCI1564_DIGITAL_OP 0x18 #define APCI1564_DIGITAL_OP_RW 0x18 #define APCI1564_DIGITAL_OP_INTERRUPT (0x18 + 0x4) #define APCI1564_DIGITAL_OP_IRQ (0x18 + 0xc)
etc... and then we just have the single helper function
(the corrected version from your follow up email) and then
the calls would be something to the effect of:

outl_amcc(devpriv, cmd, APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
or whichever macro is appropriate?  It definitely trims down
the length of the function calls by removing the dereference of
devpriv and the addition to get the proper register...I like that.

>
>The only problem with that would be i_APCI1564_Reset().  Is
>i_APCI1564_Reset() buggy?  Ian or Hartley might know.  Take a look at
>other comedi drivers as well to see what they do.
>

I agree.  I'll look into the other addi-data drivers (the layout of each
appears pretty similar) or see if Ian or Hartley can shed more light on the
reset function, because I have a sneaking suspicion that a good few of the lines
in it already are buggy, and it seems like there's a chance that it's not clearing
all of the registers that it should be, either.  I could be wrong about that though.

At any rate, I'll see what I can do.

Thanks,
Chase

>
>regards,
>
>dan carpenter

Re: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c

[Chase Southwood]

>On Saturday, March 1, 2014 6:18 PM, Chase Southwood <chase.southwood@yahoo.com> wrote:

>Hi Dan,
>
[snip]
>
>I like this idea.  Just to clarify though, basically all of the macros would
>change to something like 
>
>#define APCI1564_DIGITAL_IP 0x4 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4) >#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2         (0x4 + 0x8) #define >APCI1564_DIGITAL_IP_IRQ (0x4 + 0x10) #define APCI1564_DIGITAL_OP 0x18 #define >APCI1564_DIGITAL_OP_RW 0x18 #define APCI1564_DIGITAL_OP_INTERRUPT (0x18 + 0x4) #define >APCI1564_DIGITAL_OP_IRQ (0x18 + 0xc)
>etc...  

[snip]

Ah shoot, well that was a copy/paste trainwreck.  Let's try that again, the macros would
look something like:

#define APCI1564_DIGITAL_IP 0x4
#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 (0x4 + 0x4)
#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2 (0x4 + 0x8)
#define APCI1564_DIGITAL_IP_IRQ (0x4 + 0x10)

#define APCI1564_DIGITAL_OP 0x18
#define APCI1564_DIGITAL_OP_RW 0x18
#define APCI1564_DIGITAL_OP_INTERRUPT (0x18 + 0x4)
#define APCI1564_DIGITAL_OP_IRQ (0x18 + 0xc)


There, that's better.  Sorry about that.

Thanks,
Chase

[PATCH v2 1/2] Staging: comedi: introduce {outl,inl}_amcc() and {outl,inl}_iobase() helper functions in hwdrv_apci1564.c

[Chase Southwood]

This patch introduces a few simple outl and inl helper functions to allow
several lines which violate the character limit to be shortened
appropriately.  It also changes a few macro values which represented
offset values from a single unique base value to instead represent the value
of that base plus the offset.  This is to simplify the use of these macros
in the new helper functions.

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---

All right, here's another shot at this.  Dan, I took your outl_amcc idea
and did a version for the outl/inl calls based from devpriv->iobase as well.
I changed all of the macros which only offset from one base value as you
had mentioned as well, and the result is starting to look very good.
The only outl/inl calls which still look a little gross (see PATCH v2 2/2) are
the ones involving DIGITAL_OP_WATCHDOG, TIMER, or any of the COUNTER macros,
just because they use a common set of offset macros so simplifying
those calls in the same way as the rest isn't possible.  What are your
thoughts on this version of the patchset?

This is version 2 of [PATCH 1/2] Staging: comedi: introduce outl_1564_* and
inl_1564_* helper functions in hwdrv_apci1564.c

2: Changed helper functions from {outl,inl}_1564_*() to
{outl,inl}_{amcc,iobase}()

Comments welcome!

 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 38 +++++++++++++++++-----
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 2b47fa1..58e301d 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -49,25 +49,25 @@ This program is distributed in the hope that it will be useful, but WITHOUT ANY
 /* DIGITAL INPUT-OUTPUT DEFINE */
 /* Input defines */
 #define APCI1564_DIGITAL_IP				0x04
-#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1		4
-#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2		8
-#define APCI1564_DIGITAL_IP_IRQ				16
+#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1		(0x04 + 0x04)
+#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2		(0x04 + 0x08)
+#define APCI1564_DIGITAL_IP_IRQ				(0x04 + 0x10)
 
 /* Output defines */
 #define APCI1564_DIGITAL_OP				0x18
-#define APCI1564_DIGITAL_OP_RW				0
-#define APCI1564_DIGITAL_OP_INTERRUPT			4
-#define APCI1564_DIGITAL_OP_IRQ				12
+#define APCI1564_DIGITAL_OP_RW				0x18
+#define APCI1564_DIGITAL_OP_INTERRUPT			(0x18 + 0x04)
+#define APCI1564_DIGITAL_OP_IRQ				(0x18 + 0x0C)
 
 /* Digital Input IRQ Function Selection */
 #define ADDIDATA_OR					0
 #define ADDIDATA_AND					1
 
 /* Digital Input Interrupt Status */
-#define APCI1564_DIGITAL_IP_INTERRUPT_STATUS		12
+#define APCI1564_DIGITAL_IP_INTERRUPT_STATUS		(0x04 + 0x0C)
 
 /* Digital Output Interrupt Status */
-#define APCI1564_DIGITAL_OP_INTERRUPT_STATUS		8
+#define APCI1564_DIGITAL_OP_INTERRUPT_STATUS		(0x18 + 0x08)
 
 /* Digital Input Interrupt Enable Disable. */
 #define APCI1564_DIGITAL_IP_INTERRUPT_ENABLE		0x4
@@ -99,6 +99,28 @@ This program is distributed in the hope that it will be useful, but WITHOUT ANY
 #define APCI1564_TCW_WARN_TIMEVAL			24
 #define APCI1564_TCW_WARN_TIMEBASE			28
 
+static void outl_amcc(struct addi_private *devpriv, unsigned int cmd,
+				unsigned int reg)
+{
+	outl(cmd, devpriv->i_IobaseAmcc + reg);
+}
+
+static unsigned int inl_amcc(struct addi_private *devpriv, unsigned int reg)
+{
+	return inl(devpriv->i_IobaseAmcc + reg);
+}
+
+static void outl_iobase(struct addi_private *devpriv, unsigned int cmd,
+				unsigned int reg)
+{
+	outl(cmd, devpriv->iobase + reg);
+}
+
+static void inl_iobase(struct addi_private *devpriv, unsigned int reg)
+{
+	return inl(devpriv->iobase + reg);
+}
+
 /* Global variables */
 static unsigned int ui_InterruptStatus_1564;
 static unsigned int ui_InterruptData, ui_Type;
-- 
1.8.5.3

Re: [PATCH v2 1/2] Staging: comedi: introduce {outl,inl}_amcc() and {outl,inl}_iobase() helper functions in hwdrv_apci1564.c

[Dan Carpenter]

On Sun, Mar 02, 2014 at 08:52:19PM -0600, Chase Southwood wrote:
> This patch introduces a few simple outl and inl helper functions to allow
> several lines which violate the character limit to be shortened
> appropriately.  It also changes a few macro values which represented
> offset values from a single unique base value to instead represent the value
> of that base plus the offset.  This is to simplify the use of these macros
> in the new helper functions.
> 
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> 
> All right, here's another shot at this.  Dan, I took your outl_amcc idea
> and did a version for the outl/inl calls based from devpriv->iobase as well.
> I changed all of the macros which only offset from one base value as you
> had mentioned as well, and the result is starting to look very good.
> The only outl/inl calls which still look a little gross (see PATCH v2 2/2) are
> the ones involving DIGITAL_OP_WATCHDOG, TIMER, or any of the COUNTER macros,
> just because they use a common set of offset macros so simplifying
> those calls in the same way as the rest isn't possible.  What are your
> thoughts on this version of the patchset?
> 
> This is version 2 of [PATCH 1/2] Staging: comedi: introduce outl_1564_* and
> inl_1564_* helper functions in hwdrv_apci1564.c
> 
> 2: Changed helper functions from {outl,inl}_1564_*() to
> {outl,inl}_{amcc,iobase}()
> 
> Comments welcome!
> 
>  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 38 +++++++++++++++++-----
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> index 2b47fa1..58e301d 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> @@ -49,25 +49,25 @@ This program is distributed in the hope that it will be useful, but WITHOUT ANY
>  /* DIGITAL INPUT-OUTPUT DEFINE */
>  /* Input defines */
>  #define APCI1564_DIGITAL_IP				0x04
> -#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1		4
> -#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2		8
> -#define APCI1564_DIGITAL_IP_IRQ				16
> +#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1		(0x04 + 0x04)
> +#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2		(0x04 + 0x08)
> +#define APCI1564_DIGITAL_IP_IRQ				(0x04 + 0x10)

You can't change these without changing the callers.  This bit needs to
be in patch 2/2.  You should probably just merge both patches anyway
because presumably this one adds some GCC warnings about unused static
functions.

regards,
dan carpenter

Re: [PATCH v2 1/2] Staging: comedi: introduce {outl,inl}_amcc() and {outl,inl}_iobase() helper functions in hwdrv_apci1564.c

[Greg KH]

On Mon, Mar 03, 2014 at 12:27:55PM +0300, Dan Carpenter wrote:
> On Sun, Mar 02, 2014 at 08:52:19PM -0600, Chase Southwood wrote:
> > This patch introduces a few simple outl and inl helper functions to allow
> > several lines which violate the character limit to be shortened
> > appropriately.  It also changes a few macro values which represented
> > offset values from a single unique base value to instead represent the value
> > of that base plus the offset.  This is to simplify the use of these macros
> > in the new helper functions.
> > 
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> > ---
> > 
> > All right, here's another shot at this.  Dan, I took your outl_amcc idea
> > and did a version for the outl/inl calls based from devpriv->iobase as well.
> > I changed all of the macros which only offset from one base value as you
> > had mentioned as well, and the result is starting to look very good.
> > The only outl/inl calls which still look a little gross (see PATCH v2 2/2) are
> > the ones involving DIGITAL_OP_WATCHDOG, TIMER, or any of the COUNTER macros,
> > just because they use a common set of offset macros so simplifying
> > those calls in the same way as the rest isn't possible.  What are your
> > thoughts on this version of the patchset?
> > 
> > This is version 2 of [PATCH 1/2] Staging: comedi: introduce outl_1564_* and
> > inl_1564_* helper functions in hwdrv_apci1564.c
> > 
> > 2: Changed helper functions from {outl,inl}_1564_*() to
> > {outl,inl}_{amcc,iobase}()
> > 
> > Comments welcome!
> > 
> >  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 38 +++++++++++++++++-----
> >  1 file changed, 30 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> > index 2b47fa1..58e301d 100644
> > --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> > +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> > @@ -49,25 +49,25 @@ This program is distributed in the hope that it will be useful, but WITHOUT ANY
> >  /* DIGITAL INPUT-OUTPUT DEFINE */
> >  /* Input defines */
> >  #define APCI1564_DIGITAL_IP				0x04
> > -#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1		4
> > -#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2		8
> > -#define APCI1564_DIGITAL_IP_IRQ				16
> > +#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1		(0x04 + 0x04)
> > +#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2		(0x04 + 0x08)
> > +#define APCI1564_DIGITAL_IP_IRQ				(0x04 + 0x10)
> 
> You can't change these without changing the callers.  This bit needs to
> be in patch 2/2.  You should probably just merge both patches anyway
> because presumably this one adds some GCC warnings about unused static
> functions.

I'm totally confused about this series...

Chase, can you resend any outstanding patches that I haven't applied of
yours, as these different revisions and threads don't make much sense
right now.

thanks,

greg k-h

Re: [PATCH v2 1/2] Staging: comedi: introduce {outl,inl}_amcc() and {outl,inl}_iobase() helper functions in hwdrv_apci1564.c

[Chase Southwood]

>On Tuesday, March 4, 2014 6:38 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

>>On Mon, Mar 03, 2014 at 12:27:55PM +0300, Dan Carpenter wrote:
>>> On Sun, Mar 02, 2014 at 08:52:19PM -0600, Chase Southwood wrote:
>>> This patch introduces a few simple outl and inl helper functions to allow
>>> several lines which violate the character limit to be shortened
>>> appropriately.  It also changes a few macro values which represented
>>> offset values from a single unique base value to instead represent the value
>>> of that base plus the offset.  This is to simplify the use of these macros
>>> in the new helper functions.
>>> 
>>> Cc: Dan Carpenter <dan.carpenter@oracle.com>
>>> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
>>> ---
>>> 
>>> All right, here's another shot at this.  Dan, I took your outl_amcc idea
>>> and did a version for the outl/inl calls based from devpriv->iobase as well.
>>> I changed all of the macros which only offset from one base value as you
>>> had mentioned as well, and the result is starting to look very good.
>>> The only outl/inl calls which still look a little gross (see PATCH v2 2/2) are
>>> the ones involving DIGITAL_OP_WATCHDOG, TIMER, or any of the COUNTER macros,
>>> just because they use a common set of offset macros so simplifying
>>> those calls in the same way as the rest isn't possible.  What are your
>>> thoughts on this version of the patchset?
>>> 
>>> This is version 2 of [PATCH 1/2] Staging: comedi: introduce outl_1564_* and
>>> inl_1564_* helper functions in hwdrv_apci1564.c
>>> 
>>> 2: Changed helper functions from {outl,inl}_1564_*() to
>>> {outl,inl}_{amcc,iobase}()
>>> 
>>> Comments welcome!
>>> 
>>>  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 38 +++++++++++++++++-----
>>>  1 file changed, 30 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
>>> index 2b47fa1..58e301d 100644
>>> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
>>> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
>>> @@ -49,25 +49,25 @@ This program is distributed in the hope that it will be useful, but WITHOUT ANY
>>>  /* DIGITAL INPUT-OUTPUT DEFINE */
>>>  /* Input defines */
>>>  #define APCI1564_DIGITAL_IP                0x04
>>> -#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1        4
>>> -#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2        8
>>> -#define APCI1564_DIGITAL_IP_IRQ                16
>>> +#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1        (0x04 + 0x04)
>>> +#define APCI1564_DIGITAL_IP_INTERRUPT_MODE2        (0x04 + 0x08)
>>> +#define APCI1564_DIGITAL_IP_IRQ                (0x04 + 0x10)
>> 
>> You can't change these without changing the callers.  This bit needs to
>> be in patch 2/2.  You should probably just merge both patches anyway
>> because presumably this one adds some GCC warnings about unused static
>> functions.>
>
>I'm totally confused about this series...
>
>Chase, can you resend any outstanding patches that I haven't applied of
>yours, as these different revisions and threads don't make much sense
>right now.
>

Greg,
Yes, admittedly this whole thing has gotten a little messy :P sorry for all the confusion.
I've finally spoken with Hartley about how to best clean this driver, and so I will submit
a final patchset to you as soon as I finish up with it.  Until then, please just disregard
all prior patches to hwdrv_apci1564.c from me, as the one I send next will supersede all of them
and I will clearly mark it as such for you.

Thanks,
Chase

>
>thanks,
>
>greg k-h

[PATCH v2 2/2] Staging: comedi: use inl() and outl() helper functions

[Chase Southwood]

Use the newly created helper functions to improve code readability and shorten
several lines to under the character limit.

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---

I've reviewed this as best as I can, but I know it's a bear to review.
If there is some logical way that you'd like me to split this up into separate
patches to make it easier to review, please let me know.

Also, Dan, as best as I can possibly tell, i_APCI1564_Reset() is absolutely
buggy.  When making up this patch, I made the changes that seem correct (and
somewhat obvious) from looking at the other addi-data drivers, the other
functions in this file, and the hardware specs that I could find, but
unfortunately I have no way to test the code.

This patch is version 2 of [PATCH 2/2] Staging: comedi: use outl_1564_* and
inl_1564_* helper functions in hwdrv_apci1564.c

2: Helper functions changed.

Comments are welcome!

 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 366 ++++++++-------------
 1 file changed, 137 insertions(+), 229 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 58e301d..4b4a4bb 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -165,31 +165,17 @@ static int i_APCI1564_ConfigDigitalInput(struct comedi_device *dev,
 	if (data[0] == ADDIDATA_ENABLE) {
 		data[2] = data[2] << 4;
 		data[3] = data[3] << 4;
-		outl(data[2],
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
-		outl(data[3],
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
+		outl_amcc(devpriv, data[2], APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
+		outl_amcc(devpriv, data[3], APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
 		if (data[1] == ADDIDATA_OR) {
-			outl(0x4,
-				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-				APCI1564_DIGITAL_IP_IRQ);
+			outl_amcc(devpriv, 0x4, APCI1564_DIGITAL_IP_IRQ);
 		} else {
-			outl(0x6,
-				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-				APCI1564_DIGITAL_IP_IRQ);
+			outl_amcc(devpriv, 0x6, APCI1564_DIGITAL_IP_IRQ);
 		}
 	} else {
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_IRQ);
+		outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
+		outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
+		outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_IP_IRQ);
 	}
 
 	return insn->n;
@@ -202,7 +188,7 @@ static int apci1564_di_insn_bits(struct comedi_device *dev,
 {
 	struct addi_private *devpriv = dev->private;
 
-	data[1] = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP);
+	data[1] = inl_amcc(devpriv, APCI1564_DIGITAL_IP);
 
 	return insn->n;
 }
@@ -261,12 +247,8 @@ static int i_APCI1564_ConfigDigitalOutput(struct comedi_device *dev,
 	else
 		ul_Command = ul_Command & 0xFFFFFFFD;
 
-	outl(ul_Command,
-		devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-		APCI1564_DIGITAL_OP_INTERRUPT);
-	ui_InterruptData =
-		inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-		APCI1564_DIGITAL_OP_INTERRUPT);
+	outl_amcc(devpriv, ul_Command, APCI1564_DIGITAL_OP_INTERRUPT);
+	ui_InterruptData = inl_amcc(devpriv, APCI1564_DIGITAL_OP_INTERRUPT);
 	devpriv->tsk_Current = current;
 	return insn->n;
 }
@@ -278,12 +260,10 @@ static int apci1564_do_insn_bits(struct comedi_device *dev,
 {
 	struct addi_private *devpriv = dev->private;
 
-	s->state = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-			APCI1564_DIGITAL_OP_RW);
+	s->state = inl_amcc(devpriv, APCI1564_DIGITAL_OP_RW);
 
 	if (comedi_dio_update_state(s, data))
-		outl(s->state, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-			APCI1564_DIGITAL_OP_RW);
+		outl_amcc(devpriv, s->state, APCI1564_DIGITAL_OP_RW);
 
 	data[1] = s->state;
 
@@ -333,81 +313,61 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 		devpriv->b_TimerSelectMode = ADDIDATA_WATCHDOG;
 
 		/* Disable the watchdog */
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
-			APCI1564_TCW_PROG);
+		outl_amcc(devpriv, 0x0,
+				  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);
 		/* Loading the Reload value */
-		outl(data[3],
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
-			APCI1564_TCW_RELOAD_VALUE);
+		outl_amcc(devpriv, data[3],
+				  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_RELOAD_VALUE);
 	} else if (data[0] == ADDIDATA_TIMER) {
 		/* First Stop The Timer */
-		ul_Command1 =
-			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_PROG);
+		ul_Command1 = inl_amcc(devpriv, APCI1564_TIMER + APCI1564_TCW_PROG);
 		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Stop The Timer */
+		/* Stop The Timer */
+		outl_amcc(devpriv, ul_Command1, APCI1564_TIMER + APCI1564_TCW_PROG);
 
 		devpriv->b_TimerSelectMode = ADDIDATA_TIMER;
 		if (data[1] == 1) {
-			outl(0x02, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Enable TIMER int & DISABLE ALL THE OTHER int SOURCES */
-			outl(0x0,
-				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-				APCI1564_DIGITAL_IP_IRQ);
-			outl(0x0,
-				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-				APCI1564_DIGITAL_OP_IRQ);
-			outl(0x0,
-				devpriv->i_IobaseAmcc +
-				APCI1564_DIGITAL_OP_WATCHDOG +
-				APCI1564_TCW_IRQ);
-			outl(0x0,
-				devpriv->iobase + APCI1564_COUNTER1 +
-				APCI1564_TCW_IRQ);
-			outl(0x0,
-				devpriv->iobase + APCI1564_COUNTER2 +
-				APCI1564_TCW_IRQ);
-			outl(0x0,
-				devpriv->iobase + APCI1564_COUNTER3 +
-				APCI1564_TCW_IRQ);
-			outl(0x0,
-				devpriv->iobase + APCI1564_COUNTER4 +
-				APCI1564_TCW_IRQ);
+			/* Enable TIMER int & DISABLE ALL THE OTHER int SOURCES */
+			outl_amcc(devpriv, 0x02, APCI1564_TIMER + APCI1564_TCW_PROG);
+			outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_IP_IRQ);
+			outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_OP_IRQ);
+			outl_amcc(devpriv, 0x0,
+					  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_IRQ);
+			outl_iobase(devpriv, 0x0, APCI1564_COUNTER1 + APCI1564_TCW_IRQ);
+			outl_iobase(devpriv, 0x0, APCI1564_COUNTER2 + APCI1564_TCW_IRQ);
+			outl_iobase(devpriv, 0x0, APCI1564_COUNTER3 + APCI1564_TCW_IRQ);
+			outl_iobase(devpriv, 0x0, APCI1564_COUNTER4 + APCI1564_TCW_IRQ);
 		} else {
-			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* disable Timer interrupt */
+			/* disable Timer interrupt */
+			outl_amcc(devpriv, 0x0, APCI1564_TIMER + APCI1564_TCW_PROG);
 		}
 
 		/*  Loading Timebase */
-		outl(data[2],
-			devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_TIMEBASE);
+		outl_amcc(devpriv, data[2], APCI1564_TIMER + APCI1564_TCW_TIMEBASE);
 
 		/* Loading the Reload value */
-		outl(data[3],
-			devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_RELOAD_VALUE);
+		outl_amcc(devpriv, data[3], APCI1564_TIMER + APCI1564_TCW_RELOAD_VALUE);
 
-		ul_Command1 =
-			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_PROG);
+		ul_Command1 = inl_amcc(devpriv, APCI1564_TIMER + APCI1564_TCW_PROG);
 		ul_Command1 =
 			(ul_Command1 & 0xFFF719E2UL) | 2UL << 13UL | 0x10UL;
-		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* mode 2 */
+		/* mode 2 */
+		outl_amcc(devpriv, ul_Command1, APCI1564_TIMER + APCI1564_TCW_PROG);
 	} else if (data[0] == ADDIDATA_COUNTER) {
 		devpriv->b_TimerSelectMode = ADDIDATA_COUNTER;
 		devpriv->b_ModeSelectRegister = data[5];
 
 		/* First Stop The Counter */
-		ul_Command1 =
-			inl(devpriv->iobase + ((data[5] - 1) * 0x20) +
-			APCI1564_TCW_PROG);
+		ul_Command1 = inl_iobase(devpriv,
+								 ((data[5] - 1) * 0x20) + APCI1564_TCW_PROG);
 		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-		outl(ul_Command1, devpriv->iobase + ((data[5] - 1) * 0x20) + APCI1564_TCW_PROG);	/* Stop The Timer */
+		/* Stop The Timer */
+		outl_iobase(devpriv, ul_Command1,
+					((data[5] - 1) * 0x20) + APCI1564_TCW_PROG);
 
 		/* Set the reload value */
-		outl(data[3],
-			devpriv->iobase + ((data[5] - 1) * 0x20) +
-			APCI1564_TCW_RELOAD_VALUE);
+		outl_iobase(devpriv, data[3],
+					((data[5] - 1) * 0x20) + APCI1564_TCW_RELOAD_VALUE);
 
 		/* Set the mode :             */
 		/* - Disable the hardware     */
@@ -420,21 +380,18 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 		ul_Command1 =
 			(ul_Command1 & 0xFFFC19E2UL) | 0x80000UL |
 			(unsigned int) ((unsigned int) data[4] << 16UL);
-		outl(ul_Command1,
-			devpriv->iobase + ((data[5] - 1) * 0x20) +
-			APCI1564_TCW_PROG);
+		outl_iobase(devpriv, ul_Command1,
+					((data[5] - 1) * 0x20) + APCI1564_TCW_PROG);
 
 		/*  Enable or Disable Interrupt */
 		ul_Command1 = (ul_Command1 & 0xFFFFF9FD) | (data[1] << 1);
-		outl(ul_Command1,
-			devpriv->iobase + ((data[5] - 1) * 0x20) +
-			APCI1564_TCW_PROG);
+		outl_iobase(devpriv, ul_Command1,
+					((data[5] - 1) * 0x20) + APCI1564_TCW_PROG);
 
 		/* Set the Up/Down selection */
 		ul_Command1 = (ul_Command1 & 0xFFFBF9FFUL) | (data[6] << 18);
-		outl(ul_Command1,
-			devpriv->iobase + ((data[5] - 1) * 0x20) +
-			APCI1564_TCW_PROG);
+		outl_iobase(devpriv, ul_Command1,
+					((data[5] - 1) * 0x20) + APCI1564_TCW_PROG);
 	} else {
 		dev_err(dev->class_dev, "Invalid subdevice.\n");
 	}
@@ -479,19 +436,17 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		switch (data[1]) {
 		case 0:	/* stop the watchdog */
-			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);	/* disable the watchdog */
+			/* disable the watchdog */
+			outl_amcc(devpriv, 0x0,
+					  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);
 			break;
 		case 1:	/* start the watchdog */
-			outl(0x0001,
-				devpriv->i_IobaseAmcc +
-				APCI1564_DIGITAL_OP_WATCHDOG +
-				APCI1564_TCW_PROG);
+			outl_amcc(devpriv, 0x0001,
+					  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);
 			break;
 		case 2:	/* Software trigger */
-			outl(0x0201,
-				devpriv->i_IobaseAmcc +
-				APCI1564_DIGITAL_OP_WATCHDOG +
-				APCI1564_TCW_PROG);
+			outl_amcc(devpriv, 0x0201,
+					  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);
 			break;
 		default:
 			dev_err(dev->class_dev, "Specified functionality does not exist.\n");
@@ -500,31 +455,23 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 	}
 	if (devpriv->b_TimerSelectMode == ADDIDATA_TIMER) {
 		if (data[1] == 1) {
-			ul_Command1 =
-				inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+			ul_Command1 = inl_amcc(devpriv, APCI1564_TIMER + APCI1564_TCW_PROG);
 			ul_Command1 = (ul_Command1 & 0xFFFFF9FFUL) | 0x1UL;
 
 			/* Enable the Timer */
-			outl(ul_Command1,
-				devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+			outl_amcc(devpriv, ul_Command1, APCI1564_TIMER + APCI1564_TCW_PROG);
 		} else if (data[1] == 0) {
 			/* Stop The Timer */
 
-			ul_Command1 =
-				inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+			ul_Command1 = inl_amcc(devpriv, APCI1564_TIMER + APCI1564_TCW_PROG);
 			ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-			outl(ul_Command1,
-				devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+			outl_amcc(devpriv, ul_Command1, APCI1564_TIMER + APCI1564_TCW_PROG);
 		}
 	}
 	if (devpriv->b_TimerSelectMode == ADDIDATA_COUNTER) {
-		ul_Command1 =
-			inl(devpriv->iobase + ((devpriv->b_ModeSelectRegister -
-					1) * 0x20) + APCI1564_TCW_PROG);
+		ul_Command1 = inl_iobase(devpriv,
+								 ((devpriv->b_ModeSelectRegister - 1) * 0x20) +
+								 APCI1564_TCW_PROG);
 		if (data[1] == 1) {
 			/* Start the Counter subdevice */
 			ul_Command1 = (ul_Command1 & 0xFFFFF9FFUL) | 0x1UL;
@@ -536,10 +483,10 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 			/*  Clears the Counter subdevice */
 			ul_Command1 = (ul_Command1 & 0xFFFFF9FFUL) | 0x400;
 		}
-		outl(ul_Command1,
-			devpriv->iobase + ((devpriv->b_ModeSelectRegister -
-					1) * 0x20) + APCI1564_TCW_PROG);
-	}			/*  if (devpriv->b_TimerSelectMode==ADDIDATA_COUNTER) */
+		outl_iobase(devpriv, ul_Command1,
+					((devpriv->b_ModeSelectRegister - 1) * 0x20) +
+					APCI1564_TCW_PROG);
+	}
 	return insn->n;
 }
 
@@ -574,30 +521,24 @@ static int i_APCI1564_ReadTimerCounterWatchdog(struct comedi_device *dev,
 
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		/*  Stores the status of the Watchdog */
-		data[0] =
-			inl(devpriv->i_IobaseAmcc +
-			APCI1564_DIGITAL_OP_WATCHDOG +
-			APCI1564_TCW_TRIG_STATUS) & 0x1;
-		data[1] =
-			inl(devpriv->i_IobaseAmcc +
-			APCI1564_DIGITAL_OP_WATCHDOG);
+		data[0] = inl_amcc(devpriv,
+						   APCI1564_DIGITAL_OP_WATCHDOG +
+						   APCI1564_TCW_TRIG_STATUS) & 0x1;
+		data[1] = inl_amcc(devpriv, APCI1564_DIGITAL_OP_WATCHDOG);
 	} else if (devpriv->b_TimerSelectMode == ADDIDATA_TIMER) {
 		/*  Stores the status of the Timer */
-		data[0] =
-			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_TRIG_STATUS) & 0x1;
-
+		data[0] = inl_amcc(devpriv,
+						   APCI1564_TIMER + APCI1564_TCW_TRIG_STATUS) & 0x1;
 		/*  Stores the Actual value of the Timer */
-		data[1] = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER);
+		data[1] = inl_amcc(devpriv, APCI1564_TIMER);
 	} else if (devpriv->b_TimerSelectMode == ADDIDATA_COUNTER) {
 		/*  Read the Counter Actual Value. */
-		data[0] =
-			inl(devpriv->iobase + ((devpriv->b_ModeSelectRegister -
-					1) * 0x20) +
-			APCI1564_TCW_SYNC_ENABLEDISABLE);
-		ul_Command1 =
-			inl(devpriv->iobase + ((devpriv->b_ModeSelectRegister -
-					1) * 0x20) + APCI1564_TCW_TRIG_STATUS);
+		data[0] = inl_iobase(devpriv,
+							 ((devpriv->b_ModeSelectRegister - 1) * 0x20) +
+							 APCI1564_TCW_SYNC_ENABLEDISABLE);
+		ul_Command1 = inl_iobase(devpriv,
+								 ((devpriv->b_ModeSelectRegister - 1) * 0x20) +
+								 APCI1564_TCW_TRIG_STATUS);
 
 		/* Get the software trigger status */
 		data[1] = (unsigned char) ((ul_Command1 >> 1) & 1);
@@ -670,50 +611,35 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 	unsigned int ui_C1, ui_C2, ui_C3, ui_C4;
 	unsigned int ul_Command2 = 0;
 
-	ui_DI = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-		APCI1564_DIGITAL_IP_IRQ) & 0x01;
-	ui_DO = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-		APCI1564_DIGITAL_OP_IRQ) & 0x01;
-	ui_Timer =
-		inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-		APCI1564_TCW_IRQ) & 0x01;
-	ui_C1 = inl(devpriv->iobase + APCI1564_COUNTER1 +
-		APCI1564_TCW_IRQ) & 0x1;
-	ui_C2 = inl(devpriv->iobase + APCI1564_COUNTER2 +
-		APCI1564_TCW_IRQ) & 0x1;
-	ui_C3 = inl(devpriv->iobase + APCI1564_COUNTER3 +
-		APCI1564_TCW_IRQ) & 0x1;
-	ui_C4 = inl(devpriv->iobase + APCI1564_COUNTER4 +
-		APCI1564_TCW_IRQ) & 0x1;
+	ui_DI = inl_amcc(devpriv, APCI1564_DIGITAL_IP_IRQ) & 0x01;
+	ui_DO = inl_amcc(devpriv, APCI1564_DIGITAL_OP_IRQ) & 0x01;
+	ui_Timer = inl_amcc(devpriv, APCI1564_TIMER + APCI1564_TCW_IRQ) & 0x01;
+	ui_C1 = inl_iobase(devpriv, APCI1564_COUNTER1 + APCI1564_TCW_IRQ) & 0x1;
+	ui_C2 = inl_iobase(devpriv, APCI1564_COUNTER2 + APCI1564_TCW_IRQ) & 0x1;
+	ui_C3 = inl_iobase(devpriv, APCI1564_COUNTER3 + APCI1564_TCW_IRQ) & 0x1;
+	ui_C4 = inl_iobase(devpriv, APCI1564_COUNTER4 + APCI1564_TCW_IRQ) & 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->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_IRQ);
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_IRQ);
+		ui_DI = inl_amcc(devpriv, APCI1564_DIGITAL_IP_IRQ);
+		outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_IP_IRQ);
 		ui_InterruptStatus_1564 =
-			inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
+			inl_amcc(devpriv, APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
 		ui_InterruptStatus_1564 = ui_InterruptStatus_1564 & 0X000FFFF0;
 		send_sig(SIGIO, devpriv->tsk_Current, 0);	/*  send signal to the sample */
-		outl(ui_DI, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + APCI1564_DIGITAL_IP_IRQ);	/* enable the interrupt */
+		/* enable the interrupt */
+		outl_amcc(devpriv, ui_DI, APCI1564_DIGITAL_IP_IRQ);
 		return;
 	}
 
 	if (ui_DO == 1) {
 		/*  Check for Digital Output interrupt Type - 1: Vcc interrupt 2: CC interrupt. */
-		ui_Type =
-			inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-			APCI1564_DIGITAL_OP_INTERRUPT_STATUS) & 0x3;
+		ui_Type = inl_amcc(devpriv, APCI1564_DIGITAL_OP_INTERRUPT_STATUS) & 0x3;
 		/* Disable the  Interrupt */
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-			APCI1564_DIGITAL_OP_INTERRUPT);
+		outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_OP_INTERRUPT);
 
 		/* Sends signal to user space */
 		send_sig(SIGIO, devpriv->tsk_Current, 0);
@@ -724,21 +650,15 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 		if (devpriv->b_TimerSelectMode) {
 
 			/*  Disable Timer Interrupt */
-			ul_Command2 =
-				inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				    APCI1564_TCW_PROG);
-			outl(0x0,
-			     devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			     APCI1564_TCW_PROG);
+			ul_Command2 = inl_amcc(devpriv, APCI1564_TIMER + APCI1564_TCW_PROG);
+			outl_amcc(devpriv, 0x0, APCI1564_TIMER + APCI1564_TCW_PROG);
 
 			/* 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 +
-			     APCI1564_TCW_PROG);
+			outl_amcc(devpriv, ul_Command2, APCI1564_TIMER + APCI1564_TCW_PROG);
 		}
 	}
 
@@ -747,20 +667,16 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 		if (devpriv->b_TimerSelectMode) {
 
 			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(devpriv->iobase + APCI1564_COUNTER1 +
-				    APCI1564_TCW_PROG);
-			outl(0x0,
-			     devpriv->iobase + APCI1564_COUNTER1 +
-			     APCI1564_TCW_PROG);
+			ul_Command2 = inl_iobase(devpriv,
+									 APCI1564_COUNTER1 + APCI1564_TCW_PROG);
+			outl_iobase(devpriv, 0x0, APCI1564_COUNTER1 + APCI1564_TCW_PROG);
 
 			/* Send a signal to from kernel to user space */
 			send_sig(SIGIO, devpriv->tsk_Current, 0);
 
 			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     devpriv->iobase + APCI1564_COUNTER1 +
-			     APCI1564_TCW_PROG);
+			outl_iobase(devpriv, ul_Command2,
+						APCI1564_COUNTER1 + APCI1564_TCW_PROG);
 		}
 	}
 
@@ -769,20 +685,16 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 		if (devpriv->b_TimerSelectMode) {
 
 			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(devpriv->iobase + APCI1564_COUNTER2 +
-				    APCI1564_TCW_PROG);
-			outl(0x0,
-			     devpriv->iobase + APCI1564_COUNTER2 +
-			     APCI1564_TCW_PROG);
+			ul_Command2 = inl_iobase(devpriv,
+									 APCI1564_COUNTER2 + APCI1564_TCW_PROG);
+			outl_iobase(devpriv, 0x0, APCI1564_COUNTER2 + APCI1564_TCW_PROG);
 
 			/* Send a signal to from kernel to user space */
 			send_sig(SIGIO, devpriv->tsk_Current, 0);
 
 			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     devpriv->iobase + APCI1564_COUNTER2 +
-			     APCI1564_TCW_PROG);
+			outl_iobase(devpriv, ul_Command2,
+						APCI1564_COUNTER2 + APCI1564_TCW_PROG);
 		}
 	}
 
@@ -791,20 +703,16 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 		if (devpriv->b_TimerSelectMode) {
 
 			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(devpriv->iobase + APCI1564_COUNTER3 +
-				    APCI1564_TCW_PROG);
-			outl(0x0,
-			     devpriv->iobase + APCI1564_COUNTER3 +
-			     APCI1564_TCW_PROG);
+			ul_Command2 = inl_iobase(devpriv,
+									 APCI1564_COUNTER3 + APCI1564_TCW_PROG);
+			outl_iobase(devpriv, 0x0, APCI1564_COUNTER3 + APCI1564_TCW_PROG);
 
 			/* Send a signal to from kernel to user space */
 			send_sig(SIGIO, devpriv->tsk_Current, 0);
 
 			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     devpriv->iobase + APCI1564_COUNTER3 +
-			     APCI1564_TCW_PROG);
+			outl_iobase(devpriv, ul_Command2,
+						APCI1564_COUNTER3 + APCI1564_TCW_PROG);
 		}
 	}
 
@@ -813,20 +721,16 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 		if (devpriv->b_TimerSelectMode) {
 
 			/*  Disable Counter Interrupt */
-			ul_Command2 =
-				inl(devpriv->iobase + APCI1564_COUNTER4 +
-				    APCI1564_TCW_PROG);
-			outl(0x0,
-			     devpriv->iobase + APCI1564_COUNTER4 +
-			     APCI1564_TCW_PROG);
+			ul_Command2 = inl_iobase(devpriv,
+									 APCI1564_COUNTER4 + APCI1564_TCW_PROG);
+			outl_iobase(devpriv, 0x0, APCI1564_COUNTER4 + APCI1564_TCW_PROG);
 
 			/* Send a signal to from kernel to user space */
 			send_sig(SIGIO, devpriv->tsk_Current, 0);
 
 			/*  Enable Counter Interrupt */
-			outl(ul_Command2,
-			     devpriv->iobase + APCI1564_COUNTER4 +
-			     APCI1564_TCW_PROG);
+			outl_iobase(devpriv, ul_Command2,
+						APCI1564_COUNTER4 + APCI1564_TCW_PROG);
 		}
 	}
 	return;
@@ -851,23 +755,27 @@ static int i_APCI1564_Reset(struct comedi_device *dev)
 {
 	struct addi_private *devpriv = dev->private;
 
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_IRQ);	/* disable the interrupts */
-	inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_STATUS);	/* Reset the interrupt status register */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1);	/* Disable the and/or interrupt */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
+	/* disable the interrupts */
+	outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_IP_IRQ);
+	/* Reset the interrupt status register */
+	inl_amcc(devpriv, APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
+	/* Disable the and/or interrupt */
+	outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
+	outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
 	devpriv->b_DigitalOutputRegister = 0;
 	ui_Type = 0;
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP);	/* Resets the output channels */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_INTERRUPT);	/* Disables the interrupt. */
-	outl(0x0,
-		devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
-		APCI1564_TCW_RELOAD_VALUE);
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER);
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);
-
-	outl(0x0, devpriv->iobase + APCI1564_COUNTER1 + APCI1564_TCW_PROG);
-	outl(0x0, devpriv->iobase + APCI1564_COUNTER2 + APCI1564_TCW_PROG);
-	outl(0x0, devpriv->iobase + APCI1564_COUNTER3 + APCI1564_TCW_PROG);
-	outl(0x0, devpriv->iobase + APCI1564_COUNTER4 + APCI1564_TCW_PROG);
+	/* Resets the output channels */
+	outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_OP);
+	/* Disables the interrupt. */
+	outl_amcc(devpriv, 0x0, APCI1564_DIGITAL_OP_INTERRUPT);
+	outl_amcc(devpriv, 0x0,
+			  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_RELOAD_VALUE);
+	outl_amcc(devpriv, 0x0, APCI1564_TIMER);
+	outl_amcc(devpriv, 0x0, APCI1564_TIMER + APCI1564_TCW_PROG);
+
+	outl_iobase(devpriv, 0x0, APCI1564_COUNTER1 + APCI1564_TCW_PROG);
+	outl_iobase(devpriv, 0x0, APCI1564_COUNTER2 + APCI1564_TCW_PROG);
+	outl_iobase(devpriv, 0x0, APCI1564_COUNTER3 + APCI1564_TCW_PROG);
+	outl_iobase(devpriv, 0x0, APCI1564_COUNTER4 + APCI1564_TCW_PROG);
 	return 0;
 }
-- 
1.8.5.3

Re: [PATCH v2 2/2] Staging: comedi: use inl() and outl() helper functions

[Dan Carpenter]

On Sun, Mar 02, 2014 at 08:52:33PM -0600, Chase Southwood wrote:
> Use the newly created helper functions to improve code readability and shorten
> several lines to under the character limit.
> 
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> 
> I've reviewed this as best as I can, but I know it's a bear to review.
> If there is some logical way that you'd like me to split this up into separate
> patches to make it easier to review, please let me know.
> 
> Also, Dan, as best as I can possibly tell, i_APCI1564_Reset() is absolutely
> buggy.  When making up this patch, I made the changes that seem correct (and
> somewhat obvious) from looking at the other addi-data drivers, the other
> functions in this file, and the hardware specs that I could find, but
> unfortunately I have no way to test the code.

You need to put this into the commit message...  You can't change how
the code works without at least mentioning it in the commit.

It would be easier to review these patches if you introduced the helpers
first and switched everything to use them.  Then in 2/2 you changed the
defines to the combined versions.

> @@ -333,81 +313,61 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
>  		devpriv->b_TimerSelectMode = ADDIDATA_WATCHDOG;
>  
>  		/* Disable the watchdog */
> -		outl(0x0,
> -			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
> -			APCI1564_TCW_PROG);
> +		outl_amcc(devpriv, 0x0,
> +				  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);


This isn't indented correctly.  It should be:

		outl_amcc(devpriv, 0x0,
			  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);

[tab][tab][tab][space][space]APCI1564_DIGITAL_OP_WATCHDOG...

The same thing in a couple other places as well.

regards,
dan carpenter

RE: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c

[Hartley Sweeten]

On Saturday, March 01, 2014 3:28 AM, Chase Southwood wrote:
> Subject: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c
>
> This patch introduces a handful of outl and inl helper functions with the
> ultimate goal of improving code readability and allowing several lines
> which violate the character limit to be shortened in a sane way.
>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
> This patchset serves as a replacement to my previous cleanup patchset for
> hwdrv_apci1564.c
>
> Dan,
> After spending a little bit more time with this and trying out different
> ways of cleaning this up, I decided that making helper functions for all
> of the most common register sets would look the best, but I haven't made
> a helper for a few of the least common inl/outl calls because if I did,
> the sheer number of helper functions would get quite ridiculous.
> Let me know if you think my selections of what to make into helper
> functions seems appropriate.

Chase,

Sorry for jumping in here late.

I think if you just tidy up the register map defines it would help. Some of them
are actually used incorrectly anyway.

For instance, these two define the "base" offset for the digital input registers,
which is also to digital input register, and the offset from the "base" to the
interrupt mode1 register.

#define APCI1564_DIGITAL_IP				0x04
#define APCI1564_DIGITAL_IP_INTERRUPT_MODE1		4

One use of the APCI1564_DIGITAL_IP_INTERRUPT_MODE1 define is in
i_APCI1564_ConfigDigitalInput():

		outl(data[2],
			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
			APCI1564_DIGITAL_IP_INTERRUPT_MODE1);

But in i_APCI1564_Reset () it is used as:

	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1);

So in the first example the driver is writing to a register at offset 0x08 (0x04 + 4) but
the second is writing to a register at offset 0x04 (4), which is really the digitial input
register.

I suggest you just fix the register map defines so they are the "real" offsets to each
register and not a mix of real offsets and adders to those offsets.

I would also rename them to help with the > 80 char lines.

Something like this:

/*
 * devpriv->i_IobaseAmcc Register map
 */
#define APCI1564_DI_REG					0x04
#define APCI1564_DI_INT_MODE1_REG			0x08
#define APCI1564_DI_INT_MODE2_REG			0x0c
#define APCI1564_DI_INT_STATUS_REG			0x10
#define APCI1564_DI_IRQ_REG				0x14
#define APCI1564_DO_REG					0x18
#define APCI1564_DO_INT_CTRL_REG			0x1c
#define APCI1564_DO_INT_STATUS_REG			0x20
#define APCI1564_DO_IRQ_REG				0x24
#define APCI1564_WDOG_REG				0x28
#define APCI1564_WDOG_RELOAD_REG			0x2c
#define APCI1564_WDOG_TIMEBASE_REG			0x30
#define APCI1564_WDOG_CTRL_REG				0x34
#define APCI1564_WDOG_STATUS_REG			0x38
#define APCI1564_WDOG_IRQ_REG				0x3c
#define APCI1564_WDOG_WARN_TIMEVAL_REG			0x40
#define APCI1564_WDOG_WARN_TIMEBASE_REG			0x44
#define APCI1564_TIMER_REG				0x48
#define APCI1564_TIMER_RELOAD_REG			0x4c
#define APCI1564_TIMER_TIMEBASE_REG			0x50
#define APCI1564_TIMER_CTRL_REG				0x54
#define APCI1564_TIMER_STATUS_REG			0x58
#define APCI1564_TIMER_IRQ_REG				0x5c
#define APCI1564_TIMER_WARN_TIMEVAL_REG			0x60
#define APCI1564_TIMER_WARN_TIMEBASE_REG		0x64

/*
 * devpriv->iobase Register map
 */
#define APCI1564_TCW_REG(x)				(0x00 + ((x) * 20))
#define APCI1564_TCW_RELOAD_REG(x)			(0x04 + ((x) * 20))
#define APCI1564_TCW_TIMEBASE_REG(x)			(0x08 + ((x) * 20))
#define APCI1564_TCW_CTRL_REG(x)			(0x0c + ((x) * 20))
#define APCI1564_TCW_STATUS_REG(x)			(0x10 + ((x) * 20))
#define APCI1564_TCW_IRQ_REG(x)				(0x14 + ((x) * 20))
#define APCI1564_TCW_WARN_TIMEVAL_REG(x)		(0x18 + ((x) * 20))
#define APCI1564_TCW_WARN_TIMEBASE_REG(x)		(0x1c + ((x) * 20))

You will probably want to split this into a couple patches to make reviewing
easier. Maybe do it by subdevice: digital input, digital output, watchdog, timer,
then the counters.

Regards,
Hartley

[PATCH 2/2] Staging: comedi: use outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c

[Chase Southwood]

We can use these inl and outl helper functions to improve code
readability and shorten several lines to under the character limit.

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
---
I checked over this all several times so I hope it is correct.  I welcome
feedback on anything here

Thanks,
Chase

 .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 204 ++++++++-------------
 1 file changed, 77 insertions(+), 127 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
index 282a744..e627e586 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
@@ -191,31 +191,21 @@ static int i_APCI1564_ConfigDigitalInput(struct comedi_device *dev,
 	if (data[0] == ADDIDATA_ENABLE) {
 		data[2] = data[2] << 4;
 		data[3] = data[3] << 4;
-		outl(data[2],
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
-		outl(data[3],
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
+		outl_1564_digital_ip(devpriv, data[2],
+							APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
+		outl_1564_digital_ip(devpriv, data[3],
+							APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
 		if (data[1] == ADDIDATA_OR) {
-			outl(0x4,
-				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-				APCI1564_DIGITAL_IP_IRQ);
+			outl_1564_digital_ip(devpriv, 0x4, APCI1564_DIGITAL_IP_IRQ);
 		} else {
-			outl(0x6,
-				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-				APCI1564_DIGITAL_IP_IRQ);
+			outl_1564_digital_ip(devpriv, 0x6, APCI1564_DIGITAL_IP_IRQ);
 		}
 	} else {
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_IRQ);
+		outl_1564_digital_ip(devpriv, 0x0,
+							APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
+		outl_1564_digital_ip(devpriv, 0x0,
+							APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
+		outl_1564_digital_ip(devpriv, 0x0, APCI1564_DIGITAL_IP_IRQ);
 	}
 
 	return insn->n;
@@ -228,7 +218,7 @@ static int apci1564_di_insn_bits(struct comedi_device *dev,
 {
 	struct addi_private *devpriv = dev->private;
 
-	data[1] = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP);
+	data[1] = inl_digital_ip(devpriv, 0x0);
 
 	return insn->n;
 }
@@ -287,12 +277,9 @@ static int i_APCI1564_ConfigDigitalOutput(struct comedi_device *dev,
 	else
 		ul_Command = ul_Command & 0xFFFFFFFD;
 
-	outl(ul_Command,
-		devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-		APCI1564_DIGITAL_OP_INTERRUPT);
+	outl_1564_digital_op(devpriv, ul_Command, APCI1564_DIGITAL_OP_INTERRUPT);
 	ui_InterruptData =
-		inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-		APCI1564_DIGITAL_OP_INTERRUPT);
+		inl_1564_digital_op(devpriv, APCI1564_DIGITAL_OP_INTERRUPT);
 	devpriv->tsk_Current = current;
 	return insn->n;
 }
@@ -304,12 +291,10 @@ static int apci1564_do_insn_bits(struct comedi_device *dev,
 {
 	struct addi_private *devpriv = dev->private;
 
-	s->state = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-			APCI1564_DIGITAL_OP_RW);
+	s->state = inl_1564_digital_op(devpriv, APCI1564_DIGITAL_OP_RW);
 
 	if (comedi_dio_update_state(s, data))
-		outl(s->state, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-			APCI1564_DIGITAL_OP_RW);
+		outl_1564_digital_op(devpriv, s->state, APCI1564_DIGITAL_OP_RW);
 
 	data[1] = s->state;
 
@@ -359,34 +344,25 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 		devpriv->b_TimerSelectMode = ADDIDATA_WATCHDOG;
 
 		/* Disable the watchdog */
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
-			APCI1564_TCW_PROG);
+		outl_1564_digital_op_watchdog(devpriv, 0x0, APCI1564_TCW_PROG);
 		/* Loading the Reload value */
-		outl(data[3],
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
-			APCI1564_TCW_RELOAD_VALUE);
+		outl_1564_digital_op_watchdog(devpriv, data[3],
+									 APCI1564_TCW_RELOAD_VALUE);
 	} else if (data[0] == ADDIDATA_TIMER) {
 		/* First Stop The Timer */
 		ul_Command1 =
-			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_PROG);
+			inl_1564_timer(devpriv, APCI1564_TCW_PROG);
 		ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Stop The Timer */
+		/* Stop The Timer */
+		outl_1564_timer(devpriv, ul_Command1, APCI1564_TCW_PROG);
 
 		devpriv->b_TimerSelectMode = ADDIDATA_TIMER;
 		if (data[1] == 1) {
-			outl(0x02, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* Enable TIMER int & DISABLE ALL THE OTHER int SOURCES */
-			outl(0x0,
-				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-				APCI1564_DIGITAL_IP_IRQ);
-			outl(0x0,
-				devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-				APCI1564_DIGITAL_OP_IRQ);
-			outl(0x0,
-				devpriv->i_IobaseAmcc +
-				APCI1564_DIGITAL_OP_WATCHDOG +
-				APCI1564_TCW_IRQ);
+			/* Enable TIMER int & DISABLE ALL THE OTHER int SOURCES */
+			outl_1564_timer(devpriv, 0x02, APCI1564_TCW_PROG);
+			outl_1564_digital_ip(devpriv, 0x0, APCI1564_DIGITAL_IP_IRQ);
+			outl_1564_digital_op(devpriv, 0x0, APCI1564_DIGITAL_OP_IRQ);
+			outl_1564_digital_op_watchdog(devpriv, 0x0, APCI1564_TCW_IRQ);
 			outl(0x0,
 				devpriv->iobase + APCI1564_COUNTER1 +
 				APCI1564_TCW_IRQ);
@@ -400,25 +376,22 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
 				devpriv->iobase + APCI1564_COUNTER4 +
 				APCI1564_TCW_IRQ);
 		} else {
-			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* disable Timer interrupt */
+			/* disable Timer interrupt */
+			outl_1564_timer(devpriv, 0x0, APCI1564_TCW_PROG);
 		}
 
 		/*  Loading Timebase */
-		outl(data[2],
-			devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_TIMEBASE);
+		outl_1564_timer(devpriv, data[2], APCI1564_TCW_TIMEBASE);
 
 		/* Loading the Reload value */
-		outl(data[3],
-			devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_RELOAD_VALUE);
+		outl_1564_timer(devpriv, data[3], APCI1564_TCW_RELOAD_VALUE);
 
 		ul_Command1 =
-			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_PROG);
+			inl_1564_timer(devpriv, APCI1564_TCW_PROG);
 		ul_Command1 =
 			(ul_Command1 & 0xFFF719E2UL) | 2UL << 13UL | 0x10UL;
-		outl(ul_Command1, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);	/* mode 2 */
+		/* mode 2 */
+		outl_1564_timer(devpriv, ul_Command1, APCI1564_TCW_PROG);
 	} else if (data[0] == ADDIDATA_COUNTER) {
 		devpriv->b_TimerSelectMode = ADDIDATA_COUNTER;
 		devpriv->b_ModeSelectRegister = data[5];
@@ -505,19 +478,14 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		switch (data[1]) {
 		case 0:	/* stop the watchdog */
-			outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);	/* disable the watchdog */
+			/* disable the watchdog */
+			outl_1564_digital_op_watchdog(devpriv, 0x0, APCI1564_TCW_PROG);
 			break;
 		case 1:	/* start the watchdog */
-			outl(0x0001,
-				devpriv->i_IobaseAmcc +
-				APCI1564_DIGITAL_OP_WATCHDOG +
-				APCI1564_TCW_PROG);
+			outl_1564_digital_op_watchdog(devpriv, 0x0001, APCI1564_TCW_PROG);
 			break;
 		case 2:	/* Software trigger */
-			outl(0x0201,
-				devpriv->i_IobaseAmcc +
-				APCI1564_DIGITAL_OP_WATCHDOG +
-				APCI1564_TCW_PROG);
+			outl_1564_digital_op_watchdog(devpriv, 0x0201, APCI1564_TCW_PROG);
 			break;
 		default:
 			dev_err(dev->class_dev, "Specified functionality does not exist.\n");
@@ -527,24 +495,18 @@ static int i_APCI1564_StartStopWriteTimerCounterWatchdog(struct comedi_device *d
 	if (devpriv->b_TimerSelectMode == ADDIDATA_TIMER) {
 		if (data[1] == 1) {
 			ul_Command1 =
-				inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+				inl_1564_timer(devpriv, APCI1564_TCW_PROG);
 			ul_Command1 = (ul_Command1 & 0xFFFFF9FFUL) | 0x1UL;
 
 			/* Enable the Timer */
-			outl(ul_Command1,
-				devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+			outl_1564_timer(devpriv, ul_Command1, APCI1564_TCW_PROG);
 		} else if (data[1] == 0) {
 			/* Stop The Timer */
 
 			ul_Command1 =
-				inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+				inl_1564_timer(devpriv, APCI1564_TCW_PROG);
 			ul_Command1 = ul_Command1 & 0xFFFFF9FEUL;
-			outl(ul_Command1,
-				devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				APCI1564_TCW_PROG);
+			outl_1564_timer(devpriv, ul_Command1, APCI1564_TCW_PROG);
 		}
 	}
 	if (devpriv->b_TimerSelectMode == ADDIDATA_COUNTER) {
@@ -601,20 +563,16 @@ static int i_APCI1564_ReadTimerCounterWatchdog(struct comedi_device *dev,
 	if (devpriv->b_TimerSelectMode == ADDIDATA_WATCHDOG) {
 		/*  Stores the status of the Watchdog */
 		data[0] =
-			inl(devpriv->i_IobaseAmcc +
-			APCI1564_DIGITAL_OP_WATCHDOG +
-			APCI1564_TCW_TRIG_STATUS) & 0x1;
+			inl_1564_digital_op_watchdog(devpriv, APCI1564_TCW_TRIG_STATUS) & 0x1;
 		data[1] =
-			inl(devpriv->i_IobaseAmcc +
-			APCI1564_DIGITAL_OP_WATCHDOG);
+			inl_1564_digital_op_watchdog(devpriv, 0x0);
 	} else if (devpriv->b_TimerSelectMode == ADDIDATA_TIMER) {
 		/*  Stores the status of the Timer */
 		data[0] =
-			inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			APCI1564_TCW_TRIG_STATUS) & 0x1;
+			inl_1564_timer(devpriv, APCI1564_TCW_TRIG_STATUS) & 0x1;
 
 		/*  Stores the Actual value of the Timer */
-		data[1] = inl(devpriv->i_IobaseAmcc + APCI1564_TIMER);
+		data[1] = inl_1564_timer(devpriv, 0x0);
 	} else if (devpriv->b_TimerSelectMode == ADDIDATA_COUNTER) {
 		/*  Read the Counter Actual Value. */
 		data[0] =
@@ -696,13 +654,9 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 	unsigned int ui_C1, ui_C2, ui_C3, ui_C4;
 	unsigned int ul_Command2 = 0;
 
-	ui_DI = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-		APCI1564_DIGITAL_IP_IRQ) & 0x01;
-	ui_DO = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-		APCI1564_DIGITAL_OP_IRQ) & 0x01;
-	ui_Timer =
-		inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-		APCI1564_TCW_IRQ) & 0x01;
+	ui_DI = inl_1564_digital_ip(devpriv, APCI1564_DIGITAL_IP_IRQ) & 0x01;
+	ui_DO = inl_1564_digital_op(devpriv, APCI1564_DIGITAL_OP_IRQ) & 0x01;
+	ui_Timer = inl_1564_timer(devpriv, APCI1564_TCW_IRQ) & 0x01;
 	ui_C1 = inl(devpriv->iobase + APCI1564_COUNTER1 +
 		APCI1564_TCW_IRQ) & 0x1;
 	ui_C2 = inl(devpriv->iobase + APCI1564_COUNTER2 +
@@ -717,29 +671,23 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 	}
 
 	if (ui_DI == 1) {
-		ui_DI = inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_IRQ);
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_IRQ);
+		ui_DI = inl_1564_digital_ip(devpriv, APCI1564_DIGITAL_IP_IRQ);
+		outl_1564_digital_ip(devpriv, 0x0, APCI1564_DIGITAL_IP_IRQ);
 		ui_InterruptStatus_1564 =
-			inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP +
-			APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
+			inl_1564_digital_ip(devpriv, APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
 		ui_InterruptStatus_1564 = ui_InterruptStatus_1564 & 0X000FFFF0;
 		send_sig(SIGIO, devpriv->tsk_Current, 0);	/*  send signal to the sample */
-		outl(ui_DI, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + APCI1564_DIGITAL_IP_IRQ);	/* enable the interrupt */
+		/* enable the interrupt */
+		outl_1564_digital_ip(devpriv, ui_DI, APCI1564_DIGITAL_IP_IRQ);
 		return;
 	}
 
 	if (ui_DO == 1) {
 		/*  Check for Digital Output interrupt Type - 1: Vcc interrupt 2: CC interrupt. */
 		ui_Type =
-			inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-			APCI1564_DIGITAL_OP_INTERRUPT_STATUS) & 0x3;
+			inl_1564_digital_op(devpriv, APCI1564_DIGITAL_OP_INTERRUPT_STATUS) & 0x3;
 		/* Disable the  Interrupt */
-		outl(0x0,
-			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP +
-			APCI1564_DIGITAL_OP_INTERRUPT);
+		outl_1564_digital_op(devpriv, 0x0, APCI1564_DIGITAL_OP_INTERRUPT);
 
 		/* Sends signal to user space */
 		send_sig(SIGIO, devpriv->tsk_Current, 0);
@@ -751,20 +699,15 @@ static void v_APCI1564_Interrupt(int irq, void *d)
 
 			/*  Disable Timer Interrupt */
 			ul_Command2 =
-				inl(devpriv->i_IobaseAmcc + APCI1564_TIMER +
-				    APCI1564_TCW_PROG);
-			outl(0x0,
-			     devpriv->i_IobaseAmcc + APCI1564_TIMER +
-			     APCI1564_TCW_PROG);
+				inl_1564_timer(devpriv, APCI1564_TCW_PROG);
+			outl_1564_timer(devpriv, 0x0, APCI1564_TCW_PROG);
 
 			/* 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 +
-			     APCI1564_TCW_PROG);
+			outl_1564_timer(devpriv, ul_Command2, APCI1564_TCW_PROG);
 		}
 	}
 
@@ -877,19 +820,26 @@ static int i_APCI1564_Reset(struct comedi_device *dev)
 {
 	struct addi_private *devpriv = dev->private;
 
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_IRQ);	/* disable the interrupts */
-	inl(devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_STATUS);	/* Reset the interrupt status register */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1);	/* Disable the and/or interrupt */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
+	/* disable the interrupts */
+	outl_1564_digital_ip(devpriv, 0x0, APCI1564_DIGITAL_IP_IRQ);
+	/* Reset the interrupt status register */
+	inl_1564_digital_ip(devpriv, APCI1564_DIGITAL_IP_INTERRUPT_STATUS);
+	/* Disable the and/or interrupt */
+	outl_1564_digital_ip(devpriv, 0x0, APCI1564_DIGITAL_IP_INTERRUPT_MODE1);
+	outl_1564_digital_ip(devpriv, 0x0, APCI1564_DIGITAL_IP_INTERRUPT_MODE2);
+
 	devpriv->b_DigitalOutputRegister = 0;
 	ui_Type = 0;
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP);	/* Resets the output channels */
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_INTERRUPT);	/* Disables the interrupt. */
-	outl(0x0,
-		devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
-		APCI1564_TCW_RELOAD_VALUE);
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER);
-	outl(0x0, devpriv->i_IobaseAmcc + APCI1564_TIMER + APCI1564_TCW_PROG);
+
+	/* Resets the output channels */
+	outl_1564_digital_op(devpriv, 0x0, APCI1564_DIGITAL_OP_RW);
+	/* Disables the interrupt. */
+	outl_1564_digital_op(devpriv, 0x0, APCI1564_DIGITAL_OP_INTERRUPT);
+
+	outl_1564_digital_op_watchdog(devpriv, 0x0, APCI1564_TCW_RELOAD_VALUE);
+
+	outl_1564_timer(devpriv, 0x0, 0x0);
+	outl_1564_timer(devpriv, 0x0, APCI1564_TCW_PROG);
 
 	outl(0x0, devpriv->iobase + APCI1564_COUNTER1 + APCI1564_TCW_PROG);
 	outl(0x0, devpriv->iobase + APCI1564_COUNTER2 + APCI1564_TCW_PROG);
-- 
1.8.5.3