[PATCH 0/6] staging: comedi: cb_pcidas64.c: Fix checkpatch warning

[PATCH 0/6] staging: comedi: cb_pcidas64.c: Fix checkpatch warning

[Anson Jacob]

Fix checkpath.pl warning:
line over 80 characters
Block comments use * on subsequent lines
Block comments use a trailing */ on a separate line
Statements should start on a tabstop
Comparisons should place the constant on the right side of the test

Anson Jacob (6):
  staging: comedi: cb_pcidas64.c: Fix checkpatch warning
  staging: comedi: jr3_pci.h: Fix checkpatch warning
  staging: comedi: ni_atmio.c: Fix checkpatch warning
  staging: comedi: s626.h: Fix checkpatch warning
  staging: comedi: jr3_pci.c: Fix checkpatch warning
  staging: comedi: ni_at_a2150.c: Fix checkpatch warning

 drivers/staging/comedi/drivers/cb_pcidas64.c | 105 ++++++++++++++-------------
 drivers/staging/comedi/drivers/jr3_pci.c     |   3 +-
 drivers/staging/comedi/drivers/jr3_pci.h     |  13 ++--
 drivers/staging/comedi/drivers/ni_at_a2150.c |  82 ++++++++++++---------
 drivers/staging/comedi/drivers/ni_atmio.c    |   4 +-
 drivers/staging/comedi/drivers/s626.h        |   2 +-
 6 files changed, 113 insertions(+), 96 deletions(-)

-- 
2.7.4

[PATCH 1/6] staging: comedi: cb_pcidas64.c: Fix checkpatch warning

[Anson Jacob]

Fix checkpatch.pl warning:
Block comments use * on subsequent lines
Block comments use a trailing */ on a separate line

Signed-off-by: Anson Jacob <ansonjacob.aj@gmail.com>
---
 drivers/staging/comedi/drivers/cb_pcidas64.c | 105 ++++++++++++++-------------
 1 file changed, 55 insertions(+), 50 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
index aae839e..29725c5 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -1,34 +1,34 @@
 /*
-    comedi/drivers/cb_pcidas64.c
-    This is a driver for the ComputerBoards/MeasurementComputing PCI-DAS
-    64xx, 60xx, and 4020 cards.
-
-    Author:  Frank Mori Hess <fmhess@users.sourceforge.net>
-    Copyright (C) 2001, 2002 Frank Mori Hess
-
-    Thanks also go to the following people:
-
-    Steve Rosenbluth, for providing the source code for
-    his pci-das6402 driver, and source code for working QNX pci-6402
-    drivers by Greg Laird and Mariusz Bogacz.  None of the code was
-    used directly here, but it was useful as an additional source of
-    documentation on how to program the boards.
-
-    John Sims, for much testing and feedback on pcidas-4020 support.
-
-    COMEDI - Linux Control and Measurement Device Interface
-    Copyright (C) 1997-8 David A. Schleef <ds@schleef.org>
-
-    This program is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    This program is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-*/
+ * comedi/drivers/cb_pcidas64.c
+ * This is a driver for the ComputerBoards/MeasurementComputing PCI-DAS
+ * 64xx, 60xx, and 4020 cards.
+ *
+ * Author:  Frank Mori Hess <fmhess@users.sourceforge.net>
+ * Copyright (C) 2001, 2002 Frank Mori Hess
+ *
+ * Thanks also go to the following people:
+ *
+ * Steve Rosenbluth, for providing the source code for
+ * his pci-das6402 driver, and source code for working QNX pci-6402
+ * drivers by Greg Laird and Mariusz Bogacz.  None of the code was
+ * used directly here, but it was useful as an additional source of
+ * documentation on how to program the boards.
+ *
+ * John Sims, for much testing and feedback on pcidas-4020 support.
+ *
+ * COMEDI - Linux Control and Measurement Device Interface
+ * Copyright (C) 1997-8 David A. Schleef <ds@schleef.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
 
 /*
  * Driver: cb_pcidas64
@@ -66,19 +66,18 @@
  */
 
 /*
-
-TODO:
-	make it return error if user attempts an ai command that uses the
-	external queue, and an ao command simultaneously user counter subdevice
-	there are a number of boards this driver will support when they are
-	fully released, but does not yet since the pci device id numbers
-	are not yet available.
-
-	support prescaled 100khz clock for slow pacing (not available on 6000
-	series?)
-
-	make ao fifo size adjustable like ai fifo
-*/
+ * TODO:
+ * make it return error if user attempts an ai command that uses the
+ * external queue, and an ao command simultaneously user counter subdevice
+ * there are a number of boards this driver will support when they are
+ * fully released, but does not yet since the pci device id numbers
+ * are not yet available.
+ *
+ * support prescaled 100khz clock for slow pacing (not available on 6000
+ * series?)
+ *
+ * make ao fifo size adjustable like ai fifo
+ */
 
 #include <linux/module.h>
 #include <linux/delay.h>
@@ -91,11 +90,12 @@ TODO:
 
 #define TIMER_BASE 25		/*  40MHz master clock */
 /* 100kHz 'prescaled' clock for slow acquisition,
- * maybe I'll support this someday */
+ * maybe I'll support this someday
+ */
 #define PRESCALED_TIMER_BASE	10000
 #define DMA_BUFFER_SIZE 0x1000
 
-/* maximum value that can be loaded into board's 24-bit counters*/
+/* maximum value that can be loaded into board's 24-bit counters */
 static const int max_counter_value = 0xffffff;
 
 /* PCI-DAS64xxx base addresses */
@@ -216,7 +216,8 @@ enum hw_config_contents {
 	/*  use 225 nanosec strobe when loading dac instead of 50 nanosec */
 	SLOW_DAC_BIT = 0x400,
 	/*  bit with unknown function yet given as default value in pci-das64
-	 *  manual */
+	 *  manual
+	 */
 	HW_CONFIG_DUMMY_BITS = 0x2000,
 	/*  bit selects channels 1/0 for analog input/output, otherwise 0/1 */
 	DMA_CH_SELECT_BIT = 0x8000,
@@ -1138,12 +1139,14 @@ struct pcidas64_private {
 	/*  physical addresses of ai dma buffers */
 	dma_addr_t ai_buffer_bus_addr[MAX_AI_DMA_RING_COUNT];
 	/*  array of ai dma descriptors read by plx9080,
-	 *  allocated to get proper alignment */
+	 *  allocated to get proper alignment
+	 */
 	struct plx_dma_desc *ai_dma_desc;
 	/*  physical address of ai dma descriptor array */
 	dma_addr_t ai_dma_desc_bus_addr;
 	/*  index of the ai dma descriptor/buffer
-	 *  that is currently being used */
+	 *  that is currently being used
+	 */
 	unsigned int ai_dma_index;
 	/*  dma buffers for analog output */
 	uint16_t *ao_buffer[AO_DMA_RING_COUNT];
@@ -1314,10 +1317,12 @@ static void init_plx9080(struct comedi_device *dev)
 	/*  enable dma chaining */
 	bits |= PLX_DMAMODE_CHAINEN;
 	/*  enable interrupt on dma done
-	 *  (probably don't need this, since chain never finishes) */
+	 *  (probably don't need this, since chain never finishes)
+	 */
 	bits |= PLX_DMAMODE_DONEIEN;
 	/*  don't increment local address during transfers
-	 *  (we are transferring from a fixed fifo register) */
+	 *  (we are transferring from a fixed fifo register)
+	 */
 	bits |= PLX_DMAMODE_LACONST;
 	/*  route dma interrupt to pci bus */
 	bits |= PLX_DMAMODE_INTRPCI;
-- 
2.7.4

[PATCH 2/6] staging: comedi: jr3_pci.h: Fix checkpatch warning

[Anson Jacob]

Fix checkpatch.pl warning:
Block comments use * on subsequent lines
Block comments use a trailing */ on a separate line

Signed-off-by: Anson Jacob <ansonjacob.aj@gmail.com>
---
 drivers/staging/comedi/drivers/jr3_pci.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/jr3_pci.h b/drivers/staging/comedi/drivers/jr3_pci.h
index 356811d..f3614b4 100644
--- a/drivers/staging/comedi/drivers/jr3_pci.h
+++ b/drivers/staging/comedi/drivers/jr3_pci.h
@@ -421,12 +421,13 @@ struct jr3_channel {
 	 */
 
 	struct force_array filter[7];	/* offset 0x0090,
-					   offset 0x0098,
-					   offset 0x00a0,
-					   offset 0x00a8,
-					   offset 0x00b0,
-					   offset 0x00b8 ,
-					   offset 0x00c0 */
+					 * offset 0x0098,
+					 * offset 0x00a0,
+					 * offset 0x00a8,
+					 * offset 0x00b0,
+					 * offset 0x00b8,
+					 * offset 0x00c0
+					 */
 
 	/* Rate_data is the calculated rate data. It is a first derivative
 	 * calculation. It is calculated at a frequency specified by the
-- 
2.7.4

[PATCH 3/6] staging: comedi: ni_atmio.c: Fix checkpatch warning

[Anson Jacob]

Fix checkpatch.pl warning for 'Statements should start on a tabstop'

Signed-off-by: Anson Jacob <ansonjacob.aj@gmail.com>
---
 drivers/staging/comedi/drivers/ni_atmio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c b/drivers/staging/comedi/drivers/ni_atmio.c
index 162a000..27ed117 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -278,10 +278,10 @@ static const struct ni_board_struct *ni_atmio_probe(struct comedi_device *dev)
 	}
 	if (device_id == 255)
 		dev_err(dev->class_dev, "can't find board\n");
-	 else if (device_id == 0)
+	else if (device_id == 0)
 		dev_err(dev->class_dev,
 			"EEPROM read error (?) or device not found\n");
-	 else
+	else
 		dev_err(dev->class_dev,
 			"unknown device ID %d -- contact author\n", device_id);
 
-- 
2.7.4

[PATCH 4/6] staging: comedi: s626.h: Fix checkpatch warning

[Anson Jacob]

Fix checkpatch.pl warning for
Comparisons should place the constant on the right side of the test

Signed-off-by: Anson Jacob <ansonjacob.aj@gmail.com>
---
 drivers/staging/comedi/drivers/s626.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/s626.h b/drivers/staging/comedi/drivers/s626.h
index 6a00a64..95f8359 100644
--- a/drivers/staging/comedi/drivers/s626.h
+++ b/drivers/staging/comedi/drivers/s626.h
@@ -329,7 +329,7 @@
 						 * WS1-WS4 = CS* outputs.
 						 */
 
-#if S626_PLATFORM == S626_INTEL		/*
+#if (S626_PLATFORM == S626_INTEL)	/*
 					 * Base ACON1 config: always run
 					 * A1 based on TSL1.
 					 */
-- 
2.7.4

[PATCH 5/6] staging: comedi: jr3_pci.c: Fix checkpatch warning

[Anson Jacob]

Fix checkpatch.pl warning 'line over 80 characters'

Signed-off-by: Anson Jacob <ansonjacob.aj@gmail.com>
---
 drivers/staging/comedi/drivers/jr3_pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/jr3_pci.c b/drivers/staging/comedi/drivers/jr3_pci.c
index 6c4ff02..0291d3d 100644
--- a/drivers/staging/comedi/drivers/jr3_pci.c
+++ b/drivers/staging/comedi/drivers/jr3_pci.c
@@ -448,7 +448,8 @@ static int jr3_download_firmware(struct comedi_device *dev,
 	return 0;
 }
 
-static struct jr3_pci_poll_delay jr3_pci_poll_subdevice(struct comedi_subdevice *s)
+static struct jr3_pci_poll_delay
+jr3_pci_poll_subdevice(struct comedi_subdevice *s)
 {
 	struct jr3_pci_subdev_private *spriv = s->private;
 	struct jr3_pci_poll_delay result = poll_delay_min_max(1000, 2000);
-- 
2.7.4

[PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch warning

[Anson Jacob]

Fix checkpatch.pl warning 'line over 80 characters'

Signed-off-by: Anson Jacob <ansonjacob.aj@gmail.com>
---
 drivers/staging/comedi/drivers/ni_at_a2150.c | 82 ++++++++++++++++------------
 1 file changed, 46 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_at_a2150.c b/drivers/staging/comedi/drivers/ni_at_a2150.c
index 957fb9f..3c00de1 100644
--- a/drivers/staging/comedi/drivers/ni_at_a2150.c
+++ b/drivers/staging/comedi/drivers/ni_at_a2150.c
@@ -58,41 +58,49 @@
 
 /* Registers and bits */
 #define CONFIG_REG		0x0
-#define   CHANNEL_BITS(x)		((x) & 0x7)
-#define   CHANNEL_MASK		0x7
-#define   CLOCK_SELECT_BITS(x)		(((x) & 0x3) << 3)
-#define   CLOCK_DIVISOR_BITS(x)		(((x) & 0x3) << 5)
-#define   CLOCK_MASK		(0xf << 3)
-#define   ENABLE0_BIT		0x80	/*  enable (don't internally ground) channels 0 and 1 */
-#define   ENABLE1_BIT		0x100	/*  enable (don't internally ground) channels 2 and 3 */
-#define   AC0_BIT		0x200	/*  ac couple channels 0,1 */
-#define   AC1_BIT		0x400	/*  ac couple channels 2,3 */
-#define   APD_BIT		0x800	/*  analog power down */
-#define   DPD_BIT		0x1000	/*  digital power down */
-#define TRIGGER_REG		0x2	/*  trigger config register */
-#define   POST_TRIGGER_BITS		0x2
-#define   DELAY_TRIGGER_BITS		0x3
-#define   HW_TRIG_EN		0x10	/*  enable hardware trigger */
-#define FIFO_START_REG		0x6	/*  software start aquistion trigger */
-#define FIFO_RESET_REG		0x8	/*  clears fifo + fifo flags */
-#define FIFO_DATA_REG		0xa	/*  read data */
-#define DMA_TC_CLEAR_REG		0xe	/*  clear dma terminal count interrupt */
-#define STATUS_REG		0x12	/*  read only */
-#define   FNE_BIT		0x1	/*  fifo not empty */
-#define   OVFL_BIT		0x8	/*  fifo overflow */
-#define   EDAQ_BIT		0x10	/*  end of acquisition interrupt */
-#define   DCAL_BIT		0x20	/*  offset calibration in progress */
-#define   INTR_BIT		0x40	/*  interrupt has occurred */
-#define   DMA_TC_BIT		0x80	/*  dma terminal count interrupt has occurred */
-#define   ID_BITS(x)	(((x) >> 8) & 0x3)
-#define IRQ_DMA_CNTRL_REG		0x12	/*  write only */
-#define   DMA_CHAN_BITS(x)		((x) & 0x7)	/*  sets dma channel */
-#define   DMA_EN_BIT		0x8	/*  enables dma */
-#define   IRQ_LVL_BITS(x)		(((x) & 0xf) << 4)	/*  sets irq level */
-#define   FIFO_INTR_EN_BIT		0x100	/*  enable fifo interrupts */
-#define   FIFO_INTR_FHF_BIT		0x200	/*  interrupt fifo half full */
-#define   DMA_INTR_EN_BIT		0x800	/*  enable interrupt on dma terminal count */
-#define   DMA_DEM_EN_BIT	0x1000	/*  enables demand mode dma */
+#define CHANNEL_BITS(x)		((x) & 0x7)
+#define CHANNEL_MASK		0x7
+#define CLOCK_SELECT_BITS(x)	(((x) & 0x3) << 3)
+#define CLOCK_DIVISOR_BITS(x)	(((x) & 0x3) << 5)
+#define CLOCK_MASK		(0xf << 3)
+#define ENABLE0_BIT		0x80	/* enable (don't internally ground)
+					 * channels 0 and 1
+					 */
+#define ENABLE1_BIT		0x100	/* enable (don't internally ground)
+					 * channels 2 and 3
+					 */
+#define AC0_BIT			0x200	/* ac couple channels 0,1 */
+#define AC1_BIT			0x400	/* ac couple channels 2,3 */
+#define APD_BIT			0x800	/* analog power down */
+#define DPD_BIT			0x1000	/* digital power down */
+#define TRIGGER_REG		0x2	/* trigger config register */
+#define POST_TRIGGER_BITS	0x2
+#define DELAY_TRIGGER_BITS	0x3
+#define HW_TRIG_EN		0x10	/* enable hardware trigger */
+#define FIFO_START_REG		0x6	/* software start aquistion trigger */
+#define FIFO_RESET_REG		0x8	/* clears fifo + fifo flags */
+#define FIFO_DATA_REG		0xa	/* read data */
+#define DMA_TC_CLEAR_REG	0xe	/* clear dma terminal count interrupt */
+#define STATUS_REG		0x12	/* read only */
+#define FNE_BIT			0x1	/* fifo not empty */
+#define OVFL_BIT		0x8	/* fifo overflow */
+#define EDAQ_BIT		0x10	/* end of acquisition interrupt */
+#define DCAL_BIT		0x20	/* offset calibration in progress */
+#define INTR_BIT		0x40	/* interrupt has occurred */
+#define DMA_TC_BIT		0x80	/* dma terminal count interrupt has
+					 * occurred
+					 */
+#define ID_BITS(x)		(((x) >> 8) & 0x3)
+#define IRQ_DMA_CNTRL_REG	0x12			/* write only */
+#define DMA_CHAN_BITS(x)	((x) & 0x7)		/* sets dma channel */
+#define DMA_EN_BIT		0x8			/* enables dma */
+#define IRQ_LVL_BITS(x)		(((x) & 0xf) << 4)	/* sets irq level */
+#define FIFO_INTR_EN_BIT	0x100	/* enable fifo interrupts */
+#define FIFO_INTR_FHF_BIT	0x200	/* interrupt fifo half full */
+#define DMA_INTR_EN_BIT		0x800	/* enable interrupt on dma terminal
+					 * count
+					 */
+#define DMA_DEM_EN_BIT		0x1000	/* enables demand mode dma */
 #define I8253_BASE_REG		0x14
 
 struct a2150_board {
@@ -550,7 +558,9 @@ static int a2150_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
 	if (cmd->start_src == TRIG_EXT) {
 		trigger_bits |= HW_TRIG_EN;
 	} else if (cmd->start_src == TRIG_OTHER) {
-		/*  XXX add support for level/slope start trigger using TRIG_OTHER */
+		/* XXX add support for level/slope start trigger
+		 * using TRIG_OTHER
+		 */
 		dev_err(dev->class_dev, "you shouldn't see this?\n");
 	}
 	/*  send trigger config bits */
-- 
2.7.4

Re: [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch warning

[Andrey Utkin]

On Thu, Aug 25, 2016 at 11:16:13AM -0400, Anson Jacob wrote:
> Fix checkpatch.pl warning 'line over 80 characters'
> 
> Signed-off-by: Anson Jacob <ansonjacob.aj@gmail.com>
> ---
>  drivers/staging/comedi/drivers/ni_at_a2150.c | 82 ++++++++++++++++------------
>  1 file changed, 46 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/ni_at_a2150.c b/drivers/staging/comedi/drivers/ni_at_a2150.c
> index 957fb9f..3c00de1 100644
> --- a/drivers/staging/comedi/drivers/ni_at_a2150.c
> +++ b/drivers/staging/comedi/drivers/ni_at_a2150.c
> @@ -58,41 +58,49 @@
>  
>  /* Registers and bits */
>  #define CONFIG_REG		0x0
> -#define   CHANNEL_BITS(x)		((x) & 0x7)
> -#define   CHANNEL_MASK		0x7
> -#define   CLOCK_SELECT_BITS(x)		(((x) & 0x3) << 3)

You have lost this treeish look which carried information about thing
being a register or a field.

> +#define CHANNEL_BITS(x)		((x) & 0x7)
> +#define CHANNEL_MASK		0x7

No uniform alignment. Please get everything in a row.
If it's hard or this part of driver is expected to have a lot of changes
in near future, then I'd remove any whitespace over single space between
name and value.

> +#define ENABLE0_BIT		0x80	/* enable (don't internally ground)
> +					 * channels 0 and 1
> +					 */
> +#define ENABLE1_BIT		0x100	/* enable (don't internally ground)
> +					 * channels 2 and 3
> +					 */

This commenting style is discouraged (Linus has even stated explicitly
that he dislikes it, even for pieces of code which historically had this
style everywhere). Opening "/*" should not be followed by text.

Also, In my personal experience, it is more stable to have these
comments on previous line, not at end of the line. This way you always
have enough space for both a comment and a macro.

Re: [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch warning

[Anson Jacob]

Thank you for your comments.

On Thu, Aug 25, 2016 at 06:47:08PM +0300, Andrey Utkin wrote:
> 
> > +#define CHANNEL_BITS(x)		((x) & 0x7)
> > +#define CHANNEL_MASK		0x7
> 
> No uniform alignment. Please get everything in a row.
> If it's hard or this part of driver is expected to have a lot of changes
> in near future, then I'd remove any whitespace over single space between
> name and value.
>

I didn't get your point in this case. Could you explain it.

Re: [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch warning

[Andrey Utkin]

On Thu, Aug 25, 2016 at 12:09:23PM -0400, Anson Jacob wrote:
> On Thu, Aug 25, 2016 at 06:47:08PM +0300, Andrey Utkin wrote:
> > 
> > > +#define CHANNEL_BITS(x)		((x) & 0x7)
> > > +#define CHANNEL_MASK		0x7
> > 
> > No uniform alignment. Please get everything in a row.
> > If it's hard or this part of driver is expected to have a lot of changes
> > in near future, then I'd remove any whitespace over single space between
> > name and value.
> >
> 
> I didn't get your point in this case. Could you explain it.

Reviewing actual source code with the patch applied, it looks well.  But
if you look at the patch email itself, or in "git show <commit>, the
alignment is broken. This is an illustration why using tab characters in
the middle of lines is bad idea. However this is what I see in lots of
kernel code.
So feel free to ignore this point.