Re: [PATCH 01/11] staging: comedi: hwdrv_apci1500: introduce z8536_read()

[Ian Abbott <abbotti@xxxxxxxxx>]

On 02/12/14 17:19, H Hartley Sweeten wrote:
> The Z8536 CIO registers are indirectly read by writing the register value

I think you mean the register offset.  (That also applies to patch 2.)

> to the control register then reading the control register. Introduce a helper
> function to read the Z8536 CIO registers.
>
> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
> Cc: Ian Abbott <abbotti@xxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
>   .../comedi/drivers/addi-data/hwdrv_apci1500.c      | 216 +++++----------------
>   1 file changed, 52 insertions(+), 164 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c
> index bfa9228..5d4d35b 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c
> @@ -129,6 +129,14 @@ static int i_InputChannel;
>   static int i_TimerCounter1Enabled, i_TimerCounter2Enabled,
>   	   i_WatchdogCounter3Enabled;
>
> +static unsigned int z8536_read(struct comedi_device *dev, unsigned int reg)
> +{
> +	struct apci1500_private *devpriv = dev->private;
> +
> +	outb(reg, devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +	return inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +}
> +
>   /*
>    * An event can be generated for each port. The first event is related to the
>    * first 8 channels (port 1) and the second to the following 6 channels (port 2)
> @@ -311,14 +319,8 @@ static int apci1500_di_config(struct comedi_device *dev,
>   				devpriv->iobase +
>   				APCI1500_Z8536_CONTROL_REGISTER);
>
> -			/* Selects the mode specification mask    */
> -			/* register of port 1                     */
> -			outb(APCI1500_RW_PORT_A_SPECIFICATION,
> -				devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> -			i_RegValue =
> -				inb(devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> +			i_RegValue = z8536_read(dev,
> +					APCI1500_RW_PORT_A_SPECIFICATION);
>
>   			/* Selects the mode specification mask    */
>   			/* register of port 1                     */
> @@ -366,14 +368,8 @@ static int apci1500_di_config(struct comedi_device *dev,
>   			outb(0x74,
>   				devpriv->iobase +
>   				APCI1500_Z8536_CONTROL_REGISTER);
> -			/* Selects the mode specification mask  */
> -			/* register of port B                   */
> -			outb(APCI1500_RW_PORT_B_SPECIFICATION,
> -				devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> -			i_RegValue =
> -				inb(devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> +			i_RegValue = z8536_read(dev,
> +					APCI1500_RW_PORT_B_SPECIFICATION);
>
>   			/* Selects the mode specification mask    */
>   			/* register of port B                     */
> @@ -416,14 +412,8 @@ static int apci1500_di_config(struct comedi_device *dev,
>   				devpriv->iobase +
>   				APCI1500_Z8536_CONTROL_REGISTER);
>
> -			/* Selects the mode specification mask    */
> -			/* register of port 2                     */
> -			outb(APCI1500_RW_PORT_B_SPECIFICATION,
> -				devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> -			i_RegValue =
> -				inb(devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> +			i_RegValue = z8536_read(dev,
> +					APCI1500_RW_PORT_B_SPECIFICATION);
>   			/* Selects the mode specification mask    */
>   			/* register of port 2                     */
>   			outb(APCI1500_RW_PORT_B_SPECIFICATION,
> @@ -500,12 +490,8 @@ static int apci1500_di_write(struct comedi_device *dev,
>   						devpriv->iobase +
>   						APCI1500_Z8536_CONTROL_REGISTER);
>   					i_Event1InterruptStatus = 1;
> -					outb(APCI1500_RW_PORT_A_SPECIFICATION,
> -						devpriv->iobase +
> -						APCI1500_Z8536_CONTROL_REGISTER);
> -					i_RegValue =
> -						inb(devpriv->iobase +
> -						APCI1500_Z8536_CONTROL_REGISTER);
> +					i_RegValue = z8536_read(dev,
> +					    APCI1500_RW_PORT_A_SPECIFICATION);
>
>   					/* Selects the master interrupt control register */
>   					outb(APCI1500_RW_MASTER_INTERRUPT_CONTROL, devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> @@ -1068,17 +1054,8 @@ static int apci1500_timer_config(struct comedi_device *dev,
>   					devpriv->iobase +
>   					APCI1500_Z8536_CONTROL_REGISTER);
>
> -				/* Selects the master configuration register */
> -
> -				outb(APCI1500_RW_MASTER_CONFIGURATION_CONTROL,
> -					devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> -
> -				/* Reads the register */
> -
> -				i_MasterConfiguration =
> -					inb(devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> +				i_MasterConfiguration = z8536_read(dev,
> +				    APCI1500_RW_MASTER_CONFIGURATION_CONTROL);
>
>   				/* Enables timer/counter 1 and triggers timer/counter 1 */
>
> @@ -1228,17 +1205,8 @@ static int apci1500_timer_config(struct comedi_device *dev,
>   					devpriv->iobase +
>   					APCI1500_Z8536_CONTROL_REGISTER);
>
> -				/* Selects the master configuration register */
> -
> -				outb(APCI1500_RW_MASTER_CONFIGURATION_CONTROL,
> -					devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> -
> -				/* Reads the register */
> -
> -				i_MasterConfiguration =
> -					inb(devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> +				i_MasterConfiguration = z8536_read(dev,
> +				    APCI1500_RW_MASTER_CONFIGURATION_CONTROL);
>
>   				/* Enables timer/counter 2 and triggers timer/counter 2 */
>
> @@ -1382,17 +1350,8 @@ static int apci1500_timer_config(struct comedi_device *dev,
>   					devpriv->iobase +
>   					APCI1500_Z8536_CONTROL_REGISTER);
>
> -				/* Selects the master configuration register */
> -
> -				outb(APCI1500_RW_MASTER_CONFIGURATION_CONTROL,
> -					devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> -
> -				/* Reads the register */
> -
> -				i_MasterConfiguration =
> -					inb(devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> +				i_MasterConfiguration = z8536_read(dev,
> +				    APCI1500_RW_MASTER_CONFIGURATION_CONTROL);
>
>   				/* Enables watchdog/counter 3 and triggers watchdog/counter 3 */
>
> @@ -1756,21 +1715,12 @@ static int apci1500_timer_bits(struct comedi_device *dev,
>   				devpriv->iobase +
>   				APCI1500_Z8536_CONTROL_REGISTER);
>
> -			/* Selects the counter register (high) */
> -			outb(APCI1500_R_CPT_TMR1_VALUE_HIGH,
> -				devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> -			data[0] =
> -				inb(devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> +			data[0] = z8536_read(dev,
> +					     APCI1500_R_CPT_TMR1_VALUE_HIGH);
>   			data[0] = data[0] << 8;
>   			data[0] = data[0] & 0xff00;
> -			outb(APCI1500_R_CPT_TMR1_VALUE_LOW,
> -				devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> -			data[0] =
> -				data[0] | inb(devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> +			data[0] |= z8536_read(dev,
> +					      APCI1500_R_CPT_TMR1_VALUE_LOW);
>   		} else {
>   			dev_warn(dev->class_dev,
>   				"Timer/Counter1 not configured\n");
> @@ -1798,21 +1748,12 @@ static int apci1500_timer_bits(struct comedi_device *dev,
>   				devpriv->iobase +
>   				APCI1500_Z8536_CONTROL_REGISTER);
>
> -			/* Selects the counter register (high) */
> -			outb(APCI1500_R_CPT_TMR2_VALUE_HIGH,
> -				devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> -			data[0] =
> -				inb(devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> +			data[0] = z8536_read(dev,
> +					     APCI1500_R_CPT_TMR2_VALUE_HIGH);
>   			data[0] = data[0] << 8;
>   			data[0] = data[0] & 0xff00;
> -			outb(APCI1500_R_CPT_TMR2_VALUE_LOW,
> -				devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> -			data[0] =
> -				data[0] | inb(devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> +			data[0] |= z8536_read(dev,
> +					      APCI1500_R_CPT_TMR2_VALUE_LOW);
>   		} else {
>   			dev_warn(dev->class_dev,
>   				"Timer/Counter2 not configured\n");
> @@ -1840,21 +1781,12 @@ static int apci1500_timer_bits(struct comedi_device *dev,
>   				devpriv->iobase +
>   				APCI1500_Z8536_CONTROL_REGISTER);
>
> -			/* Selects the counter register (high) */
> -			outb(APCI1500_R_CPT_TMR3_VALUE_HIGH,
> -				devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> -			data[0] =
> -				inb(devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> +			data[0] = z8536_read(dev,
> +					     APCI1500_R_CPT_TMR3_VALUE_HIGH);
>   			data[0] = data[0] << 8;
>   			data[0] = data[0] & 0xff00;
> -			outb(APCI1500_R_CPT_TMR3_VALUE_LOW,
> -				devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> -			data[0] =
> -				data[0] | inb(devpriv->iobase +
> -				APCI1500_Z8536_CONTROL_REGISTER);
> +			data[0] |= z8536_read(dev,
> +					      APCI1500_R_CPT_TMR3_VALUE_LOW);
>   		} else {
>   			dev_warn(dev->class_dev,
>   				"WatchdogCounter3 not configured\n");
> @@ -1914,10 +1846,7 @@ static int apci1500_do_bits(struct comedi_device *dev,
>   		}
>   	}
>
> -	/* Selects the mode specification register of port B */
> -	outb(APCI1500_RW_PORT_B_SPECIFICATION,
> -		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -	i_RegValue = inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +	i_RegValue = z8536_read(dev, APCI1500_RW_PORT_B_SPECIFICATION);
>   	outb(APCI1500_RW_PORT_B_SPECIFICATION,
>   		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>   	/* Writes the new configuration (APCI1500_OR) */
> @@ -1942,20 +1871,14 @@ static int apci1500_do_bits(struct comedi_device *dev,
>   		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>   	outb(i_Constant, devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>
> -	/* Selects the command and status register of port A */
> -	outb(APCI1500_RW_PORT_A_COMMAND_AND_STATUS,
> -		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -	i_RegValue = inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +	i_RegValue = z8536_read(dev, APCI1500_RW_PORT_A_COMMAND_AND_STATUS);
>   	outb(APCI1500_RW_PORT_A_COMMAND_AND_STATUS,
>   		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>   	/* Deletes the interrupt of port A */
>
>   	i_RegValue = (i_RegValue & 0x0F) | 0x20;
>   	outb(i_RegValue, devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -	/* Selects the command and status register of port  B */
> -	outb(APCI1500_RW_PORT_B_COMMAND_AND_STATUS,
> -		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -	i_RegValue = inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +	i_RegValue = z8536_read(dev, APCI1500_RW_PORT_B_COMMAND_AND_STATUS);
>   	outb(APCI1500_RW_PORT_B_COMMAND_AND_STATUS,
>   		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>   	/* Deletes the interrupt of port B */
> @@ -1963,10 +1886,7 @@ static int apci1500_do_bits(struct comedi_device *dev,
>   	i_RegValue = (i_RegValue & 0x0F) | 0x20;
>   	outb(i_RegValue, devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>
> -	/* Selects the command and status register of timer 1 */
> -	outb(APCI1500_RW_CPT_TMR1_CMD_STATUS,
> -		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -	i_RegValue = inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +	i_RegValue = z8536_read(dev, APCI1500_RW_CPT_TMR1_CMD_STATUS);
>   	outb(APCI1500_RW_CPT_TMR1_CMD_STATUS,
>   		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>   	/* Deletes the interrupt of timer 1 */
> @@ -1974,10 +1894,7 @@ static int apci1500_do_bits(struct comedi_device *dev,
>   	i_RegValue = (i_RegValue & 0x0F) | 0x20;
>   	outb(i_RegValue, devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>
> -	/* Selects the command and status register of timer 2 */
> -	outb(APCI1500_RW_CPT_TMR2_CMD_STATUS,
> -		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -	i_RegValue = inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +	i_RegValue = z8536_read(dev, APCI1500_RW_CPT_TMR2_CMD_STATUS);
>   	outb(APCI1500_RW_CPT_TMR2_CMD_STATUS,
>   		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>   	/* Deletes the interrupt of timer 2 */
> @@ -1985,10 +1902,7 @@ static int apci1500_do_bits(struct comedi_device *dev,
>   	i_RegValue = (i_RegValue & 0x0F) | 0x20;
>   	outb(i_RegValue, devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>
> -	/* Selects the command and status register of timer 3 */
> -	outb(APCI1500_RW_CPT_TMR3_CMD_STATUS,
> -		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -	i_RegValue = inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +	i_RegValue = z8536_read(dev, APCI1500_RW_CPT_TMR3_CMD_STATUS);
>   	outb(APCI1500_RW_CPT_TMR3_CMD_STATUS,
>   		devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
>   	/* Deletes the interrupt of timer 3 */
> @@ -2030,11 +1944,8 @@ static irqreturn_t apci1500_interrupt(int irq, void *d)
>   		/* Disable all Interrupt */
>   		/* Selects the master interrupt control register */
>   		/* Disables  the main interrupt on the board */
> -		/* Selects the command and status register of port A */
> -		outb(APCI1500_RW_PORT_A_COMMAND_AND_STATUS,
> -			devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -		i_RegValue =
> -			inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +		i_RegValue = z8536_read(dev,
> +					APCI1500_RW_PORT_A_COMMAND_AND_STATUS);
>   		if ((i_RegValue & 0x60) == 0x60) {
>   			/* Selects the command and status register of port A */
>   			outb(APCI1500_RW_PORT_A_COMMAND_AND_STATUS,
> @@ -2047,20 +1958,11 @@ static irqreturn_t apci1500_interrupt(int irq, void *d)
>   				APCI1500_Z8536_CONTROL_REGISTER);
>   			i_InterruptMask = i_InterruptMask | 1;
>   			if (i_Logic == APCI1500_OR_PRIORITY) {
> -				outb(APCI1500_RW_PORT_A_SPECIFICATION,
> -					devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> -				i_RegValue =
> -					inb(devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> +				i_RegValue = z8536_read(dev,
> +					APCI1500_RW_PORT_A_SPECIFICATION);
>
> -				/* Selects the interrupt vector register of port A */
> -				outb(APCI1500_RW_PORT_A_INTERRUPT_CONTROL,
> -					devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> -				i_RegValue =
> -					inb(devpriv->iobase +
> -					APCI1500_Z8536_CONTROL_REGISTER);
> +				i_RegValue = z8536_read(dev,
> +					APCI1500_RW_PORT_A_INTERRUPT_CONTROL);
>
>   				i_InputChannel = 1 + (i_RegValue >> 1);
>
> @@ -2069,11 +1971,8 @@ static irqreturn_t apci1500_interrupt(int irq, void *d)
>   			}
>   		}
>
> -		/* Selects the command and status register of port B */
> -		outb(APCI1500_RW_PORT_B_COMMAND_AND_STATUS,
> -			devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -		i_RegValue =
> -			inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +		i_RegValue = z8536_read(dev,
> +					APCI1500_RW_PORT_B_COMMAND_AND_STATUS);
>   		if ((i_RegValue & 0x60) == 0x60) {
>   			/* Selects the command and status register of port B */
>   			outb(APCI1500_RW_PORT_B_COMMAND_AND_STATUS,
> @@ -2111,11 +2010,7 @@ static irqreturn_t apci1500_interrupt(int irq, void *d)
>   			}
>   		}
>
> -		/* Selects the command and status register of timer 1 */
> -		outb(APCI1500_RW_CPT_TMR1_CMD_STATUS,
> -			devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -		i_RegValue =
> -			inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +		i_RegValue = z8536_read(dev, APCI1500_RW_CPT_TMR1_CMD_STATUS);
>   		if ((i_RegValue & 0x60) == 0x60) {
>   			/* Selects the command and status register of timer 1 */
>   			outb(APCI1500_RW_CPT_TMR1_CMD_STATUS,
> @@ -2128,11 +2023,8 @@ static irqreturn_t apci1500_interrupt(int irq, void *d)
>   				APCI1500_Z8536_CONTROL_REGISTER);
>   			i_InterruptMask = i_InterruptMask | 4;
>   		}
> -		/* Selects the command and status register of timer 2 */
> -		outb(APCI1500_RW_CPT_TMR2_CMD_STATUS,
> -			devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -		i_RegValue =
> -			inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +
> +		i_RegValue = z8536_read(dev, APCI1500_RW_CPT_TMR2_CMD_STATUS);
>   		if ((i_RegValue & 0x60) == 0x60) {
>   			/* Selects the command and status register of timer 2 */
>   			outb(APCI1500_RW_CPT_TMR2_CMD_STATUS,
> @@ -2146,11 +2038,7 @@ static irqreturn_t apci1500_interrupt(int irq, void *d)
>   			i_InterruptMask = i_InterruptMask | 8;
>   		}
>
> -		/* Selects the command and status register of timer 3 */
> -		outb(APCI1500_RW_CPT_TMR3_CMD_STATUS,
> -			devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> -		i_RegValue =
> -			inb(devpriv->iobase + APCI1500_Z8536_CONTROL_REGISTER);
> +		i_RegValue = z8536_read(dev, APCI1500_RW_CPT_TMR3_CMD_STATUS);
>   		if ((i_RegValue & 0x60) == 0x60) {
>   			/* Selects the command and status register of timer 3 */
>   			outb(APCI1500_RW_CPT_TMR3_CMD_STATUS,


--
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@xxxxxxxxx> )=-
-=(                          Web: http://www.mev.co.uk/  )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel