staging: comedi: adl_pci6208: document the register map of the device

PATCH In review H Hartley Sweeten 3 participants 4 messages Jun 27, 2012 – Jun 28, 2012

Subsystems (2)

Staging Checkpatch

lore

Share & tools

lore

H Hartley Sweeten

author

2012-06-27 21:56 UTC #0 lore raw

PATCH 4/19 staging: comedi: adl_pci6208: document the register map of the device

Add defines for the register map of the device. These will be used to clarify the code.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Ian Abbott <[email protected]>

Cc Frank Mori Hess <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---
 drivers/staging/comedi/drivers/adl_pci6208.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/staging/comedi/drivers/adl_pci6208.c +12 -0

index f949d20..b6a8439 100644
@@ -53,6 +53,18 @@ References:
  */
 #include "../comedidev.h"
 
+/*
+ * PCI-6208/6216-GL register map
+ */
+#define PCI6208_AO_CONTROL(x)		(0x00 + (2 * (x)))
+#define PCI6208_AO_STATUS		0x00
+#define PCI6208_AO_STATUS_DATA_SEND	(1 << 0)
+#define PCI6208_DIO			0x40
+#define PCI6208_DIO_DO_MASK		(0x0f)
+#define PCI6208_DIO_DO_SHIFT		(0)
+#define PCI6208_DIO_DI_MASK		(0xf0)
+#define PCI6208_DIO_DI_SHIFT		(4)
+
 /* Board descriptions */
 struct pci6208_board {
 	const char *name;

Signature · 1 line

1.7.11

Dan Carpenter

2012-06-28 19:06 UTC #1 lore raw

Quoted 20 lines · H Hartley Sweeten

Add defines for the register map of the device. These will be used to clarify the code.
Signed-off-by H Hartley Sweeten <[email protected]>
Cc Ian Abbott <[email protected]>
Cc Frank Mori Hess <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
---
 drivers/staging/comedi/drivers/adl_pci6208.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/staging/comedi/drivers/adl_pci6208.c +1 -0
index f949d20..b6a8439 100644
@@ -53,6 +53,18 @@ References:
  */
 #include "../comedidev.h"
 
+/*

drivers/staging/comedi/drivers/adl_pci6208.c References: · line 66

Quoted 10 lines

drivers/staging/comedi/drivers/adl_pci6208.c +10 -0

+ * PCI-6208/6216-GL register map
+ */
+#define PCI6208_AO_CONTROL(x)		(0x00 + (2 * (x)))
+#define PCI6208_AO_STATUS		0x00
+#define PCI6208_AO_STATUS_DATA_SEND	(1 << 0)
+#define PCI6208_DIO			0x40
+#define PCI6208_DIO_DO_MASK		(0x0f)
+#define PCI6208_DIO_DO_SHIFT		(0)
+#define PCI6208_DIO_DI_MASK		(0xf0)
+#define PCI6208_DIO_DI_SHIFT		(4)

This series is nice and I'm not nacking anything, but really is it that useful to say:

	status = inw(dev->iobase + PCI6208_AO_STATUS);
instead of just?:
	status = inw(dev->iobase);

I'm not sure what the 0x00 in PCI6208_AO_CONTROL represents. Some of these are not used like PCI6208_DIO_DI_SHIFT.

Does checkpatch.pl complain if you leave off these parenthesis? If so I will complain again to the checkpatch.pl people. Extra parenthesis are silly and there not used consistently. Only PCI6208_AO_CONTROL() and PCI6208_AO_STATUS_DATA_SEND() need paranthesis.

Again, I'm fine with this patch and the whole series. These are just comments.

regards,
dan carpenter

Joe Perches

maintainer

2012-06-28 19:56 UTC #2 lore raw

Quoted 3 lines · Dan Carpenter

Quoted 2 lines · H Hartley Sweeten
Add defines for the register map of the device. These will be used to clarify the code.

[]

Quoted 1 line

drivers/staging/comedi/drivers/adl_pci6208.c
Quoted 1 line
drivers/staging/comedi/drivers/adl_pci6208.c +0 -0

[]

Quoted 4 lines

Quoted 4 lines
drivers/staging/comedi/drivers/adl_pci6208.c +0 -0
@@ -53,6 +53,18 @@ References:
  */
 #include "../comedidev.h"
 

Quoted 11 lines

drivers/staging/comedi/drivers/adl_pci6208.c References: · line 66

Quoted 11 lines

drivers/staging/comedi/drivers/adl_pci6208.c +11 -0

+/*
+ * PCI-6208/6216-GL register map
+ */
+#define PCI6208_AO_CONTROL(x)		(0x00 + (2 * (x)))
+#define PCI6208_AO_STATUS		0x00
+#define PCI6208_AO_STATUS_DATA_SEND	(1 << 0)
+#define PCI6208_DIO			0x40
+#define PCI6208_DIO_DO_MASK		(0x0f)
+#define PCI6208_DIO_DO_SHIFT		(0)
+#define PCI6208_DIO_DI_MASK		(0xf0)
+#define PCI6208_DIO_DI_SHIFT		(4)

[]

Quoted 1 line

Does checkpatch.pl complain if you leave off these parenthesis?

checkpatch does not complain about those.
I also think parentheses around constants aren't necessary.

I think it's useful around the shifts and necessary with the arithmetic.

It might be useful to add a (--strict?) test for those extra parentheses.

Maybe something like:

 scripts/checkpatch.pl |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

scripts/checkpatch.pl +5 -0

index e5bd60f..b6b4d6b 100755
@@ -2888,6 +2888,11 @@ sub process {
 			     "Whitepspace after \\ makes next lines useless\n" . $herecurr);
 		}
 
+		if ($line =~ /^.\s*#\s*define\s+$Ident\s+\(\s*($Constant)\s*\)\s*$/) {
+			CHK("UNNECESSARY_PARENTHESIS",
+			    "Unnecessary parenthesis in #define around constant $1\n" . $herecurr);
+		}
+
 #warn if <asm/foo.h> is #included and <linux/foo.h> is available (uses RAW line)
 		if ($tree && $rawline =~ m{^.\s*\#\s*include\s*\<asm\/(.*)\.h\>}) {
 			my $file = "$1.h";

H Hartley Sweeten

author

2012-06-28 19:57 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 34 lines · Dan Carpenter

Quoted 20 lines · H Hartley Sweeten
Add defines for the register map of the device. These will be used to clarify the code.
Signed-off-by H Hartley Sweeten <[email protected]>
Cc Ian Abbott <[email protected]>
Cc Frank Mori Hess <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
---
 drivers/staging/comedi/drivers/adl_pci6208.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/staging/comedi/drivers/adl_pci6208.c +1 -0
index f949d20..b6a8439 100644
@@ -53,6 +53,18 @@ References:
  */
 #include "../comedidev.h"
 
+/*
drivers/staging/comedi/drivers/adl_pci6208.c References: · line 66
Quoted 10 lines
drivers/staging/comedi/drivers/adl_pci6208.c +10 -0
+ * PCI-6208/6216-GL register map
+ */
+#define PCI6208_AO_CONTROL(x)		(0x00 + (2 * (x)))
+#define PCI6208_AO_STATUS		0x00
+#define PCI6208_AO_STATUS_DATA_SEND	(1 << 0)
+#define PCI6208_DIO			0x40
+#define PCI6208_DIO_DO_MASK		(0x0f)
+#define PCI6208_DIO_DO_SHIFT		(0)
+#define PCI6208_DIO_DI_MASK		(0xf0)
+#define PCI6208_DIO_DI_SHIFT		(4)
This series is nice and I'm not nacking anything, but really is it that useful to say:

Quoted 3 lines

	status = inw(dev->iobase + PCI6208_AO_STATUS);
instead of just?:
	status = inw(dev->iobase);

Either would work.

But the '+ PCI6208_AO_STATUS' used in the function makes it easily apparent that the 'status' register is being read without having to go back and see what the assumed '+ 0' register is.

Quoted 2 lines

I'm not sure what the 0x00 in PCI6208_AO_CONTROL represents. Some of these are not used like PCI6208_DIO_DI_SHIFT.

Sorry about that. Maybe there should be a comment.

The PCI-6208 has 8 separate "control" registers, one for each DAC output (the PCI-6216 has 16). They are at offsets 0x00, 0x02, 0x04, ... 0x0e (0x1e for The PCI-6216). The PCI6208_AO_CONTROL macro is used to calculate the necessary offset based on the DAC channel. The original code used the same open-coded logic.

The unused ones can be removed. When I created the patch that added them I just added everything that might be useful from the manual for the PCI-6208. I was quite sure what ones would end up un used.

Quoted 5 lines

Does checkpatch.pl complain if you leave off these parenthesis? If so I will complain again to the checkpatch.pl people. Extra parenthesis are silly and there not used consistently. Only PCI6208_AO_CONTROL() and PCI6208_AO_STATUS_DATA_SEND() need paranthesis.

No, checkpatch.pl does not complain about the parenthesis either way. I usually use the parenthesis in the 'bit' defines and not for the 'register' defines. It helps my brain keep them separate... ;-)

But, they can be removed it needed.

Quoted 2 lines

Again, I'm fine with this patch and the whole series. These are just comments.

Thanks for the comments!

Regards,
Hartley