[PATCH 0/2] staging: comedi: s526: add counter register macros

[PATCH 0/2] staging: comedi: s526: add counter register macros

[Ian Abbott]

Add macros to describe the counter mode and counter control/status
registers.  In patch 1, the macros for the counter mode register replace
the use of bitfield structure and union types.

1) staging: comedi: s526: replace counter mode bitfield struct
2) staging: comedi: s526: add macros for counter control reg values

 drivers/staging/comedi/drivers/s526.c | 197 ++++++++++++++++++++++------------
 1 file changed, 130 insertions(+), 67 deletions(-)

[PATCH 1/2] staging: comedi: s526: replace counter mode bitfield struct

[Ian Abbott]

The driver uses `struct counter_mode_register_t` to describe the 16-bit
counter mode register as a sequence of bitfield members.  The struct
appears as the type of one of the members of `union cmReg`, the other
member of which is of type `unsigned short`, so the driver can
manipulate the register value as a whole, or as individual fields.
Although this is fairly convenient, it's not that conventional.  The
code also needs to define the bitfield members in ascending or
descending order of the physical bits, depending on whether bitfields
are little- or big-endian.

Rip all that out and replace it with a bunch of macros to set and mask
out bits of the register value, as that's the more conventional way to
do it.  A bonus is that we get rid of a load of CamelCase definitions in
the process.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/s526.c | 156 ++++++++++++++++++++--------------
 1 file changed, 93 insertions(+), 63 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s526.c b/drivers/staging/comedi/drivers/s526.c
index d70c979..a8165df 100644
--- a/drivers/staging/comedi/drivers/s526.c
+++ b/drivers/staging/comedi/drivers/s526.c
@@ -37,7 +37,6 @@
 
 #include <linux/module.h>
 #include "../comedidev.h"
-#include <asm/byteorder.h>
 
 /*
  * Register I/O map
@@ -84,6 +83,62 @@
 #define S526_GPCT_LSB_REG(x)	(0x12 + ((x) * 8))
 #define S526_GPCT_MSB_REG(x)	(0x14 + ((x) * 8))
 #define S526_GPCT_MODE_REG(x)	(0x16 + ((x) * 8))
+#define S526_GPCT_MODE_COUT_SRC(x)	((x) << 0)
+#define S526_GPCT_MODE_COUT_SRC_MASK	S526_GPCT_MODE_COUT_SRC(0x1)
+#define S526_GPCT_MODE_COUT_SRC_RCAP	S526_GPCT_MODE_COUT_SRC(0)
+#define S526_GPCT_MODE_COUT_SRC_RTGL	S526_GPCT_MODE_COUT_SRC(1)
+#define S526_GPCT_MODE_COUT_POL(x)	((x) << 1)
+#define S526_GPCT_MODE_COUT_POL_MASK	S526_GPCT_MODE_COUT_POL(0x1)
+#define S526_GPCT_MODE_COUT_POL_NORM	S526_GPCT_MODE_COUT_POL(0)
+#define S526_GPCT_MODE_COUT_POL_INV	S526_GPCT_MODE_COUT_POL(1)
+#define S526_GPCT_MODE_AUTOLOAD(x)	((x) << 2)
+#define S526_GPCT_MODE_AUTOLOAD_MASK	S526_GPCT_MODE_AUTOLOAD(0x7)
+#define S526_GPCT_MODE_AUTOLOAD_NONE	S526_GPCT_MODE_AUTOLOAD(0)
+/* these 3 bits can be OR'ed */
+#define S526_GPCT_MODE_AUTOLOAD_RO	S526_GPCT_MODE_AUTOLOAD(0x1)
+#define S526_GPCT_MODE_AUTOLOAD_IXFALL	S526_GPCT_MODE_AUTOLOAD(0x2)
+#define S526_GPCT_MODE_AUTOLOAD_IXRISE	S526_GPCT_MODE_AUTOLOAD(0x4)
+#define S526_GPCT_MODE_HWCTEN_SRC(x)	((x) << 5)
+#define S526_GPCT_MODE_HWCTEN_SRC_MASK	S526_GPCT_MODE_HWCTEN_SRC(0x3)
+#define S526_GPCT_MODE_HWCTEN_SRC_CEN	S526_GPCT_MODE_HWCTEN_SRC(0)
+#define S526_GPCT_MODE_HWCTEN_SRC_IX	S526_GPCT_MODE_HWCTEN_SRC(1)
+#define S526_GPCT_MODE_HWCTEN_SRC_IXRF	S526_GPCT_MODE_HWCTEN_SRC(2)
+#define S526_GPCT_MODE_HWCTEN_SRC_NRCAP	S526_GPCT_MODE_HWCTEN_SRC(3)
+#define S526_GPCT_MODE_CTEN_CTRL(x)	((x) << 7)
+#define S526_GPCT_MODE_CTEN_CTRL_MASK	S526_GPCT_MODE_CTEN_CTRL(0x3)
+#define S526_GPCT_MODE_CTEN_CTRL_DIS	S526_GPCT_MODE_CTEN_CTRL(0)
+#define S526_GPCT_MODE_CTEN_CTRL_ENA	S526_GPCT_MODE_CTEN_CTRL(1)
+#define S526_GPCT_MODE_CTEN_CTRL_HW	S526_GPCT_MODE_CTEN_CTRL(2)
+#define S526_GPCT_MODE_CTEN_CTRL_INVHW	S526_GPCT_MODE_CTEN_CTRL(3)
+#define S526_GPCT_MODE_CLK_SRC(x)	((x) << 9)
+#define S526_GPCT_MODE_CLK_SRC_MASK	S526_GPCT_MODE_CLK_SRC(0x3)
+/* if count direction control set to quadrature */
+#define S526_GPCT_MODE_CLK_SRC_QUADX1	S526_GPCT_MODE_CLK_SRC(0)
+#define S526_GPCT_MODE_CLK_SRC_QUADX2	S526_GPCT_MODE_CLK_SRC(1)
+#define S526_GPCT_MODE_CLK_SRC_QUADX4	S526_GPCT_MODE_CLK_SRC(2)
+#define S526_GPCT_MODE_CLK_SRC_QUADX4_	S526_GPCT_MODE_CLK_SRC(3)
+/* if count direction control set to software control */
+#define S526_GPCT_MODE_CLK_SRC_ARISE	S526_GPCT_MODE_CLK_SRC(0)
+#define S526_GPCT_MODE_CLK_SRC_AFALL	S526_GPCT_MODE_CLK_SRC(1)
+#define S526_GPCT_MODE_CLK_SRC_INT	S526_GPCT_MODE_CLK_SRC(2)
+#define S526_GPCT_MODE_CLK_SRC_INTHALF	S526_GPCT_MODE_CLK_SRC(3)
+#define S526_GPCT_MODE_CT_DIR(x)	((x) << 11)
+#define S526_GPCT_MODE_CT_DIR_MASK	S526_GPCT_MODE_CT_DIR(0x1)
+/* if count direction control set to software control */
+#define S526_GPCT_MODE_CT_DIR_UP	S526_GPCT_MODE_CT_DIR(0)
+#define S526_GPCT_MODE_CT_DIR_DOWN	S526_GPCT_MODE_CT_DIR(1)
+#define S526_GPCT_MODE_CTDIR_CTRL(x)	((x) << 12)
+#define S526_GPCT_MODE_CTDIR_CTRL_MASK	S526_GPCT_MODE_CTDIR_CTRL(0x1)
+#define S526_GPCT_MODE_CTDIR_CTRL_QUAD	S526_GPCT_MODE_CTDIR_CTRL(0)
+#define S526_GPCT_MODE_CTDIR_CTRL_SOFT	S526_GPCT_MODE_CTDIR_CTRL(1)
+#define S526_GPCT_MODE_LATCH_CTRL(x)	((x) << 13)
+#define S526_GPCT_MODE_LATCH_CTRL_MASK	S526_GPCT_MODE_LATCH_CTRL(0x1)
+#define S526_GPCT_MODE_LATCH_CTRL_READ	S526_GPCT_MODE_LATCH_CTRL(0)
+#define S526_GPCT_MODE_LATCH_CTRL_EVENT	S526_GPCT_MODE_LATCH_CTRL(1)
+#define S526_GPCT_MODE_PR_SELECT(x)	((x) << 14)
+#define S526_GPCT_MODE_PR_SELECT_MASK	S526_GPCT_MODE_PR_SELECT(0x1)
+#define S526_GPCT_MODE_PR_SELECT_PR0	S526_GPCT_MODE_PR_SELECT(0)
+#define S526_GPCT_MODE_PR_SELECT_PR1	S526_GPCT_MODE_PR_SELECT(1)
 #define S526_GPCT_CTRL_REG(x)	(0x18 + ((x) * 8))
 #define S526_EEPROM_DATA_REG	0x32
 #define S526_EEPROM_CTRL_REG	0x34
@@ -92,41 +147,6 @@
 #define S526_EEPROM_CTRL_READ	S526_EEPROM_CTRL(2)
 #define S526_EEPROM_CTRL_START	BIT(0)
 
-struct counter_mode_register_t {
-#if defined(__LITTLE_ENDIAN_BITFIELD)
-	unsigned short coutSource:1;
-	unsigned short coutPolarity:1;
-	unsigned short autoLoadResetRcap:3;
-	unsigned short hwCtEnableSource:2;
-	unsigned short ctEnableCtrl:2;
-	unsigned short clockSource:2;
-	unsigned short countDir:1;
-	unsigned short countDirCtrl:1;
-	unsigned short outputRegLatchCtrl:1;
-	unsigned short preloadRegSel:1;
-	unsigned short reserved:1;
- #elif defined(__BIG_ENDIAN_BITFIELD)
-	unsigned short reserved:1;
-	unsigned short preloadRegSel:1;
-	unsigned short outputRegLatchCtrl:1;
-	unsigned short countDirCtrl:1;
-	unsigned short countDir:1;
-	unsigned short clockSource:2;
-	unsigned short ctEnableCtrl:2;
-	unsigned short hwCtEnableSource:2;
-	unsigned short autoLoadResetRcap:3;
-	unsigned short coutPolarity:1;
-	unsigned short coutSource:1;
-#else
-#error Unknown bit field order
-#endif
-};
-
-union cmReg {
-	struct counter_mode_register_t reg;
-	unsigned short value;
-};
-
 struct s526_private {
 	unsigned int gpct_config[4];
 	unsigned short ai_ctrl;
@@ -174,7 +194,6 @@ static int s526_gpct_insn_config(struct comedi_device *dev,
 	struct s526_private *devpriv = dev->private;
 	unsigned int chan = CR_CHAN(insn->chanspec);
 	unsigned int val;
-	union cmReg cmReg;
 
 	/*
 	 * Check what type of Counter the user requested
@@ -192,28 +211,29 @@ static int s526_gpct_insn_config(struct comedi_device *dev,
 
 #if 1
 		/*  Set Counter Mode Register */
-		cmReg.value = data[1] & 0xffff;
-		outw(cmReg.value, dev->iobase + S526_GPCT_MODE_REG(chan));
+		val = data[1] & 0xffff;
+		outw(val, dev->iobase + S526_GPCT_MODE_REG(chan));
 
 		/*  Reset the counter if it is software preload */
-		if (cmReg.reg.autoLoadResetRcap == 0) {
+		if ((val & S526_GPCT_MODE_AUTOLOAD_MASK) ==
+		    S526_GPCT_MODE_AUTOLOAD_NONE) {
 			/*  Reset the counter */
 			outw(0x8000, dev->iobase + S526_GPCT_CTRL_REG(chan));
-			/* Load the counter from PR0
+			/*
+			 * Load the counter from PR0
 			 * outw(0x4000, dev->iobase + S526_GPCT_CTRL_REG(chan));
 			 */
 		}
 #else
-		/*  0 quadrature, 1 software control */
-		cmReg.reg.countDirCtrl = 0;
+		val = S526_GPCT_MODE_CTDIR_CTRL_QUAD;
 
 		/*  data[1] contains GPCT_X1, GPCT_X2 or GPCT_X4 */
 		if (data[1] == GPCT_X2)
-			cmReg.reg.clockSource = 1;
+			val |= S526_GPCT_MODE_CLK_SRC_QUADX2;
 		else if (data[1] == GPCT_X4)
-			cmReg.reg.clockSource = 2;
+			val |= S526_GPCT_MODE_CLK_SRC_QUADX4;
 		else
-			cmReg.reg.clockSource = 0;
+			val |= S526_GPCT_MODE_CLK_SRC_QUADX1;
 
 		/*  When to take into account the indexpulse: */
 		/*
@@ -224,13 +244,14 @@ static int s526_gpct_insn_config(struct comedi_device *dev,
 		 * }
 		 */
 		/*  Take into account the index pulse? */
-		if (data[3] == GPCT_RESET_COUNTER_ON_INDEX)
+		if (data[3] == GPCT_RESET_COUNTER_ON_INDEX) {
 			/*  Auto load with INDEX^ */
-			cmReg.reg.autoLoadResetRcap = 4;
+			val |= S526_GPCT_MODE_AUTOLOAD_IXRISE;
+		}
 
 		/*  Set Counter Mode Register */
-		cmReg.value = data[1] & 0xffff;
-		outw(cmReg.value, dev->iobase + S526_GPCT_MODE_REG(chan));
+		val = data[1] & 0xffff;
+		outw(val, dev->iobase + S526_GPCT_MODE_REG(chan));
 
 		/*  Load the pre-load register */
 		s526_gpct_write(dev, chan, data[2]);
@@ -241,7 +262,8 @@ static int s526_gpct_insn_config(struct comedi_device *dev,
 			     dev->iobase + S526_GPCT_CTRL_REG(chan));
 
 		/*  Reset the counter if it is software preload */
-		if (cmReg.reg.autoLoadResetRcap == 0) {
+		if ((val & S526_GPCT_MODE_AUTOLOAD_MASK) ==
+		    S526_GPCT_MODE_AUTOLOAD_NONE) {
 			/*  Reset the counter */
 			outw(0x8000, dev->iobase + S526_GPCT_CTRL_REG(chan));
 			/*  Load the counter from PR0 */
@@ -261,17 +283,21 @@ static int s526_gpct_insn_config(struct comedi_device *dev,
 		devpriv->gpct_config[chan] = data[0];
 
 		/*  Set Counter Mode Register */
-		cmReg.value = data[1] & 0xffff;
-		cmReg.reg.preloadRegSel = 0;	/*  PR0 */
-		outw(cmReg.value, dev->iobase + S526_GPCT_MODE_REG(chan));
+		val = data[1] & 0xffff;
+		/* Select PR0 */
+		val &= ~S526_GPCT_MODE_PR_SELECT_MASK;
+		val |= S526_GPCT_MODE_PR_SELECT_PR0;
+		outw(val, dev->iobase + S526_GPCT_MODE_REG(chan));
 
 		/* Load the pre-load register 0 */
 		s526_gpct_write(dev, chan, data[2]);
 
 		/*  Set Counter Mode Register */
-		cmReg.value = data[1] & 0xffff;
-		cmReg.reg.preloadRegSel = 1;	/*  PR1 */
-		outw(cmReg.value, dev->iobase + S526_GPCT_MODE_REG(chan));
+		val = data[1] & 0xffff;
+		/* Select PR1 */
+		val &= ~S526_GPCT_MODE_PR_SELECT_MASK;
+		val |= S526_GPCT_MODE_PR_SELECT_PR1;
+		outw(val, dev->iobase + S526_GPCT_MODE_REG(chan));
 
 		/* Load the pre-load register 1 */
 		s526_gpct_write(dev, chan, data[3]);
@@ -294,17 +320,21 @@ static int s526_gpct_insn_config(struct comedi_device *dev,
 		devpriv->gpct_config[chan] = data[0];
 
 		/*  Set Counter Mode Register */
-		cmReg.value = data[1] & 0xffff;
-		cmReg.reg.preloadRegSel = 0;	/*  PR0 */
-		outw(cmReg.value, dev->iobase + S526_GPCT_MODE_REG(chan));
+		val = data[1] & 0xffff;
+		/* Select PR0 */
+		val &= ~S526_GPCT_MODE_PR_SELECT_MASK;
+		val |= S526_GPCT_MODE_PR_SELECT_PR0;
+		outw(val, dev->iobase + S526_GPCT_MODE_REG(chan));
 
 		/* Load the pre-load register 0 */
 		s526_gpct_write(dev, chan, data[2]);
 
 		/*  Set Counter Mode Register */
-		cmReg.value = data[1] & 0xffff;
-		cmReg.reg.preloadRegSel = 1;	/*  PR1 */
-		outw(cmReg.value, dev->iobase + S526_GPCT_MODE_REG(chan));
+		val = data[1] & 0xffff;
+		/* Select PR1 */
+		val &= ~S526_GPCT_MODE_PR_SELECT_MASK;
+		val |= S526_GPCT_MODE_PR_SELECT_PR1;
+		outw(val, dev->iobase + S526_GPCT_MODE_REG(chan));
 
 		/* Load the pre-load register 1 */
 		s526_gpct_write(dev, chan, data[3]);
-- 
2.6.2

[PATCH 2/2] staging: comedi: s526: add macros for counter control reg values

[Ian Abbott]

The driver writes a couple of literal values to the counter
control/status register, 0x8000 to reset the counter, and 0x4000 to load
the counter from preload register 0.  Add a bunch of macros to define
these values and other values for the register, based on the Sensoray
526 manual.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/s526.c | 41 +++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s526.c b/drivers/staging/comedi/drivers/s526.c
index a8165df..c80527d 100644
--- a/drivers/staging/comedi/drivers/s526.c
+++ b/drivers/staging/comedi/drivers/s526.c
@@ -139,7 +139,36 @@
 #define S526_GPCT_MODE_PR_SELECT_MASK	S526_GPCT_MODE_PR_SELECT(0x1)
 #define S526_GPCT_MODE_PR_SELECT_PR0	S526_GPCT_MODE_PR_SELECT(0)
 #define S526_GPCT_MODE_PR_SELECT_PR1	S526_GPCT_MODE_PR_SELECT(1)
+/* Control/Status - R = readable, W = writeable, C = write 1 to clear */
 #define S526_GPCT_CTRL_REG(x)	(0x18 + ((x) * 8))
+#define S526_GPCT_CTRL_EV_STATUS(x)	((x) << 0)		/* RC */
+#define S526_GPCT_CTRL_EV_STATUS_MASK	S526_GPCT_EV_STATUS(0xf)
+#define S526_GPCT_CTRL_EV_STATUS_NONE	S526_GPCT_EV_STATUS(0)
+/* these 4 bits can be OR'ed */
+#define S526_GPCT_CTRL_EV_STATUS_ECAP	S526_GPCT_EV_STATUS(0x1)
+#define S526_GPCT_CTRL_EV_STATUS_ICAPN	S526_GPCT_EV_STATUS(0x2)
+#define S526_GPCT_CTRL_EV_STATUS_ICAPP	S526_GPCT_EV_STATUS(0x4)
+#define S526_GPCT_CTRL_EV_STATUS_RCAP	S526_GPCT_EV_STATUS(0x8)
+#define S526_GPCT_CTRL_COUT_STATUS	BIT(4)			/* R */
+#define S526_GPCT_CTRL_INDEX_STATUS	BIT(5)			/* R */
+#define S525_GPCT_CTRL_INTEN(x)		((x) << 6)		/* W */
+#define S525_GPCT_CTRL_INTEN_MASK	S526_GPCT_CTRL_INTEN(0xf)
+#define S525_GPCT_CTRL_INTEN_NONE	S526_GPCT_CTRL_INTEN(0)
+/* these 4 bits can be OR'ed */
+#define S525_GPCT_CTRL_INTEN_ERROR	S526_GPCT_CTRL_INTEN(0x1)
+#define S525_GPCT_CTRL_INTEN_IXFALL	S526_GPCT_CTRL_INTEN(0x2)
+#define S525_GPCT_CTRL_INTEN_IXRISE	S526_GPCT_CTRL_INTEN(0x4)
+#define S525_GPCT_CTRL_INTEN_RO		S526_GPCT_CTRL_INTEN(0x8)
+#define S525_GPCT_CTRL_LATCH_SEL(x)	((x) << 10)		/* W */
+#define S525_GPCT_CTRL_LATCH_SEL_MASK	S526_GPCT_CTRL_LATCH_SEL(0x7)
+#define S525_GPCT_CTRL_LATCH_SEL_NONE	S526_GPCT_CTRL_LATCH_SEL(0)
+/* these 3 bits can be OR'ed */
+#define S525_GPCT_CTRL_LATCH_SEL_IXFALL	S526_GPCT_CTRL_LATCH_SEL(0x1)
+#define S525_GPCT_CTRL_LATCH_SEL_IXRISE	S526_GPCT_CTRL_LATCH_SEL(0x2)
+#define S525_GPCT_CTRL_LATCH_SEL_ITIMER	S526_GPCT_CTRL_LATCH_SEL(0x4)
+#define S525_GPCT_CTRL_CT_ARM		BIT(13)			/* W */
+#define S525_GPCT_CTRL_CT_LOAD		BIT(14)			/* W */
+#define S526_GPCT_CTRL_CT_RESET		BIT(15)			/* W */
 #define S526_EEPROM_DATA_REG	0x32
 #define S526_EEPROM_CTRL_REG	0x34
 #define S526_EEPROM_CTRL_ADDR(x) (((x) & 0x3f) << 3)
@@ -218,10 +247,12 @@ static int s526_gpct_insn_config(struct comedi_device *dev,
 		if ((val & S526_GPCT_MODE_AUTOLOAD_MASK) ==
 		    S526_GPCT_MODE_AUTOLOAD_NONE) {
 			/*  Reset the counter */
-			outw(0x8000, dev->iobase + S526_GPCT_CTRL_REG(chan));
+			outw(S526_GPCT_CTRL_CT_RESET,
+			     dev->iobase + S526_GPCT_CTRL_REG(chan));
 			/*
 			 * Load the counter from PR0
-			 * outw(0x4000, dev->iobase + S526_GPCT_CTRL_REG(chan));
+			 * outw(S526_GPCT_CTRL_CT_LOAD,
+			 *      dev->iobase + S526_GPCT_CTRL_REG(chan));
 			 */
 		}
 #else
@@ -265,9 +296,11 @@ static int s526_gpct_insn_config(struct comedi_device *dev,
 		if ((val & S526_GPCT_MODE_AUTOLOAD_MASK) ==
 		    S526_GPCT_MODE_AUTOLOAD_NONE) {
 			/*  Reset the counter */
-			outw(0x8000, dev->iobase + S526_GPCT_CTRL_REG(chan));
+			outw(S526_GPCT_CTRL_CT_RESET,
+			     dev->iobase + S526_GPCT_CTRL_REG(chan));
 			/*  Load the counter from PR0 */
-			outw(0x4000, dev->iobase + S526_GPCT_CTRL_REG(chan));
+			outw(S526_GPCT_CTRL_CT_LOAD,
+			     dev->iobase + S526_GPCT_CTRL_REG(chan));
 		}
 #endif
 		break;
-- 
2.6.2

RE: [PATCH 0/2] staging: comedi: s526: add counter register macros

[Hartley Sweeten]

On Thursday, November 19, 2015 7:49 AM, Ian Abbott wrote:
> Add macros to describe the counter mode and counter control/status
> registers.  In patch 1, the macros for the counter mode register replace
> the use of bitfield structure and union types.
>
> 1) staging: comedi: s526: replace counter mode bitfield struct
> 2) staging: comedi: s526: add macros for counter control reg values
>
>  drivers/staging/comedi/drivers/s526.c | 197 ++++++++++++++++++++++------------
>  1 file changed, 130 insertions(+), 67 deletions(-)

Thanks! This one has bugged me.

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>