Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1470223 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch warning Anson Jacob <ansonjacob.aj@gmail.com> - 2016-08-25 17:30 +0200
    Re: [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch  warning Andrey Utkin <andrey_utkin@fastmail.com> - 2016-08-25 18:10 +0200
      Re: [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch  warning Anson Jacob <ansonjacob.aj@gmail.com> - 2016-08-25 18:20 +0200
        Re: [PATCH 6/6] staging: comedi: ni_at_a2150.c: Fix checkpatch  warning Andrey Utkin <andrey_utkin@fastmail.com> - 2016-08-25 18:20 +0200

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

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

[toc] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web