[PATCH] staging: comedi: ni_*

[PATCH] staging: comedi: ni_*

[Aniruddha Shastri]

Fix checkpatch warnings by shortening lines and reorganizing code where needed..
Re-phrase the assert messages in ni_mio_common.c. This was done to meet the character limit for the message.

Signed-off-by: Aniruddha Shastri <aniruddha.shastri@gmail.com>
---
 drivers/staging/comedi/drivers/ni_670x.c         |  3 +-
 drivers/staging/comedi/drivers/ni_atmio.c        |  8 +++--
 drivers/staging/comedi/drivers/ni_labpc_common.c | 13 +++-----
 drivers/staging/comedi/drivers/ni_mio_common.c   | 39 ++++++++++++------------
 drivers/staging/comedi/drivers/ni_stc.h          |  2 +-
 5 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_670x.c b/drivers/staging/comedi/drivers/ni_670x.c
index 1d3ff60..330536a 100644
--- a/drivers/staging/comedi/drivers/ni_670x.c
+++ b/drivers/staging/comedi/drivers/ni_670x.c
@@ -207,9 +207,10 @@ static int ni_670x_auto_attach(struct comedi_device *dev,
 	s->maxdata = 0xffff;
 	if (s->n_chan == 32) {
 		const struct comedi_lrange **range_table_list;
+		unsigned int range_size = sizeof(const struct comedi_lrange *);
 
 		range_table_list = kmalloc_array(32,
-						 sizeof(struct comedi_lrange *),
+						 range_size,
 						 GFP_KERNEL);
 		if (!range_table_list)
 			return -ENOMEM;
diff --git a/drivers/staging/comedi/drivers/ni_atmio.c b/drivers/staging/comedi/drivers/ni_atmio.c
index ae6ed96..6c0e91e 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -233,10 +233,12 @@ static int ni_isapnp_find_board(struct pnp_dev **dev)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(ni_boards); i++) {
+		int isapnp_id = ni_boards[i].isapnp_id;
+
 		isapnp_dev = pnp_find_dev(NULL,
-					  ISAPNP_VENDOR('N', 'I', 'C'),
-					  ISAPNP_FUNCTION(ni_boards[i].
-							  isapnp_id), NULL);
+					ISAPNP_VENDOR('N', 'I', 'C'),
+					ISAPNP_FUNCTION(isapnp_id),
+					NULL);
 
 		if (!isapnp_dev || !isapnp_dev->card)
 			continue;
diff --git a/drivers/staging/comedi/drivers/ni_labpc_common.c b/drivers/staging/comedi/drivers/ni_labpc_common.c
index b0dfb8e..f29218f 100644
--- a/drivers/staging/comedi/drivers/ni_labpc_common.c
+++ b/drivers/staging/comedi/drivers/ni_labpc_common.c
@@ -568,15 +568,12 @@ static int labpc_ai_cmdtest(struct comedi_device *dev,
 
 	/* make sure scan timing is not too fast */
 	if (cmd->scan_begin_src == TRIG_TIMER) {
-		if (cmd->convert_src == TRIG_TIMER) {
-			err |= comedi_check_trigger_arg_min(&cmd->
-							    scan_begin_arg,
-							    cmd->convert_arg *
-							    cmd->chanlist_len);
-		}
+		unsigned int expected = board->ai_speed * cmd->chanlist_len;
+
+		if (cmd->convert_src == TRIG_TIMER)
+			expected = cmd->convert_arg * cmd->chanlist_len;
 		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
-						    board->ai_speed *
-						    cmd->chanlist_len);
+						    expected);
 	}
 
 	switch (cmd->stop_src) {
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 398347f..1edcf2f 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -620,18 +620,18 @@ static int ni_request_ao_mite_channel(struct comedi_device *dev)
 }
 
 static int ni_request_gpct_mite_channel(struct comedi_device *dev,
-					unsigned int gpct_index,
+					unsigned int index,
 					enum comedi_io_direction direction)
 {
 	struct ni_private *devpriv = dev->private;
-	struct ni_gpct *counter = &devpriv->counter_dev->counters[gpct_index];
+	struct ni_gpct *counter = &devpriv->counter_dev->counters[index];
 	struct mite_channel *mite_chan;
 	unsigned long flags;
 	unsigned int bits;
 
 	spin_lock_irqsave(&devpriv->mite_channel_lock, flags);
 	mite_chan = mite_request_channel(devpriv->mite,
-					 devpriv->gpct_mite_ring[gpct_index]);
+					 devpriv->gpct_mite_ring[index]);
 	if (!mite_chan) {
 		spin_unlock_irqrestore(&devpriv->mite_channel_lock, flags);
 		dev_err(dev->class_dev,
@@ -643,8 +643,8 @@ static int ni_request_gpct_mite_channel(struct comedi_device *dev,
 
 	bits = NI_STC_DMA_CHAN_SEL(mite_chan->channel);
 	ni_set_bitfield(dev, NI_E_DMA_G0_G1_SEL_REG,
-			NI_E_DMA_G0_G1_SEL_MASK(gpct_index),
-			NI_E_DMA_G0_G1_SEL(gpct_index, bits));
+			NI_E_DMA_G0_G1_SEL_MASK(index),
+			NI_E_DMA_G0_G1_SEL(index, bits));
 
 	spin_unlock_irqrestore(&devpriv->mite_channel_lock, flags);
 	return 0;
@@ -720,20 +720,19 @@ static void ni_release_ao_mite_channel(struct comedi_device *dev)
 
 #ifdef PCIDMA
 static void ni_release_gpct_mite_channel(struct comedi_device *dev,
-					 unsigned int gpct_index)
+					 unsigned int index)
 {
 	struct ni_private *devpriv = dev->private;
 	unsigned long flags;
 
 	spin_lock_irqsave(&devpriv->mite_channel_lock, flags);
-	if (devpriv->counter_dev->counters[gpct_index].mite_chan) {
+	if (devpriv->counter_dev->counters[index].mite_chan) {
 		struct mite_channel *mite_chan =
-		    devpriv->counter_dev->counters[gpct_index].mite_chan;
+		    devpriv->counter_dev->counters[index].mite_chan;
 
 		ni_set_bitfield(dev, NI_E_DMA_G0_G1_SEL_REG,
-				NI_E_DMA_G0_G1_SEL_MASK(gpct_index), 0);
-		ni_tio_set_mite_channel(&devpriv->
-					counter_dev->counters[gpct_index],
+				NI_E_DMA_G0_G1_SEL_MASK(index), 0);
+		ni_tio_set_mite_channel(&devpriv->counter_dev->counters[index],
 					NULL);
 		mite_release_channel(mite_chan);
 	}
@@ -756,20 +755,20 @@ static void ni_release_cdo_mite_channel(struct comedi_device *dev)
 }
 
 static void ni_e_series_enable_second_irq(struct comedi_device *dev,
-					  unsigned int gpct_index, short enable)
+					  unsigned int index, short enable)
 {
 	struct ni_private *devpriv = dev->private;
 	unsigned int val = 0;
 	int reg;
 
-	if (devpriv->is_m_series || gpct_index > 1)
+	if (devpriv->is_m_series || index > 1)
 		return;
 
 	/*
 	 * e-series boards use the second irq signals to generate
 	 * dma requests for their counters
 	 */
-	if (gpct_index == 0) {
+	if (index == 0) {
 		reg = NISTC_INTA2_ENA_REG;
 		if (enable)
 			val = NISTC_INTA_ENA_G0_GATE;
@@ -1966,6 +1965,7 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
 {
 #ifdef PCIDMA
 	unsigned int nbytes = max_count;
+	char *err_msg = "data transfer limits greater than buffer size";
 
 	if (cmd->stop_arg > 0 && cmd->stop_arg < max_count)
 		nbytes = cmd->stop_arg;
@@ -1974,7 +1974,7 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
 	if (nbytes > sdev->async->prealloc_bufsz) {
 		if (cmd->stop_arg > 0)
 			dev_err(sdev->device->class_dev,
-				"ni_cmd_set_mite_transfer: tried exact data transfer limits greater than buffer size\n");
+				"%s: %s\n", __func__, err_msg);
 
 		/*
 		 * we can only transfer up to the size of the buffer.  In this
@@ -1986,8 +1986,9 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
 
 	mite_init_ring_descriptors(ring, sdev, nbytes);
 #else
-	dev_err(sdev->device->class_dev,
-		"ni_cmd_set_mite_transfer: exact data transfer limits not implemented yet without DMA\n");
+	char *err_msg = "data transfer limits not implemented yet without DMA";
+
+	dev_err(sdev->device->class_dev, "%s: %s\n", __func__, err_msg);
 #endif
 }
 
@@ -4299,7 +4300,7 @@ static int pack_ad8842(int addr, int val, int *bitstring)
 struct caldac_struct {
 	int n_chans;
 	int n_bits;
-	int (*packbits)(int, int, int *);
+	int (*packbits)(int addr, int val, int *bitstring);
 };
 
 static struct caldac_struct caldacs[] = {
@@ -4696,7 +4697,7 @@ static int cs5529_do_conversion(struct comedi_device *dev,
 	retval = cs5529_wait_for_idle(dev);
 	if (retval) {
 		dev_err(dev->class_dev,
-			"timeout or signal in cs5529_do_conversion()\n");
+			"timeout or signal in %s()\n", __func__);
 		return -ETIME;
 	}
 	status = ni_ao_win_inw(dev, NI67XX_CAL_STATUS_REG);
diff --git a/drivers/staging/comedi/drivers/ni_stc.h b/drivers/staging/comedi/drivers/ni_stc.h
index 61138e8..03099a7 100644
--- a/drivers/staging/comedi/drivers/ni_stc.h
+++ b/drivers/staging/comedi/drivers/ni_stc.h
@@ -18,7 +18,7 @@
 /*
  * References:
  *   DAQ-STC Technical Reference Manual
-*/
+ */
 
 #ifndef _COMEDI_NI_STC_H
 #define _COMEDI_NI_STC_H
-- 
2.7.4

Re: [PATCH] staging: comedi: ni_*

[Greg Kroah-Hartman]

On Wed, Dec 13, 2017 at 04:01:04AM -0600, Aniruddha Shastri wrote:
> Fix checkpatch warnings by shortening lines and reorganizing code where needed..
> Re-phrase the assert messages in ni_mio_common.c. This was done to meet the character limit for the message.

And yet this line is over the character length :)

> 
> Signed-off-by: Aniruddha Shastri <aniruddha.shastri@gmail.com>
> ---
>  drivers/staging/comedi/drivers/ni_670x.c         |  3 +-
>  drivers/staging/comedi/drivers/ni_atmio.c        |  8 +++--
>  drivers/staging/comedi/drivers/ni_labpc_common.c | 13 +++-----
>  drivers/staging/comedi/drivers/ni_mio_common.c   | 39 ++++++++++++------------
>  drivers/staging/comedi/drivers/ni_stc.h          |  2 +-
>  5 files changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/ni_670x.c b/drivers/staging/comedi/drivers/ni_670x.c
> index 1d3ff60..330536a 100644
> --- a/drivers/staging/comedi/drivers/ni_670x.c
> +++ b/drivers/staging/comedi/drivers/ni_670x.c
> @@ -207,9 +207,10 @@ static int ni_670x_auto_attach(struct comedi_device *dev,
>  	s->maxdata = 0xffff;
>  	if (s->n_chan == 32) {
>  		const struct comedi_lrange **range_table_list;
> +		unsigned int range_size = sizeof(const struct comedi_lrange *);
>  
>  		range_table_list = kmalloc_array(32,
> -						 sizeof(struct comedi_lrange *),
> +						 range_size,

Not worth changing.

>  						 GFP_KERNEL);
>  		if (!range_table_list)
>  			return -ENOMEM;
> diff --git a/drivers/staging/comedi/drivers/ni_atmio.c b/drivers/staging/comedi/drivers/ni_atmio.c
> index ae6ed96..6c0e91e 100644
> --- a/drivers/staging/comedi/drivers/ni_atmio.c
> +++ b/drivers/staging/comedi/drivers/ni_atmio.c
> @@ -233,10 +233,12 @@ static int ni_isapnp_find_board(struct pnp_dev **dev)
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(ni_boards); i++) {
> +		int isapnp_id = ni_boards[i].isapnp_id;
> +
>  		isapnp_dev = pnp_find_dev(NULL,
> -					  ISAPNP_VENDOR('N', 'I', 'C'),
> -					  ISAPNP_FUNCTION(ni_boards[i].
> -							  isapnp_id), NULL);
> +					ISAPNP_VENDOR('N', 'I', 'C'),
> +					ISAPNP_FUNCTION(isapnp_id),
> +					NULL);

Not worth changing, please leave as-is.

>  
>  		if (!isapnp_dev || !isapnp_dev->card)
>  			continue;
> diff --git a/drivers/staging/comedi/drivers/ni_labpc_common.c b/drivers/staging/comedi/drivers/ni_labpc_common.c
> index b0dfb8e..f29218f 100644
> --- a/drivers/staging/comedi/drivers/ni_labpc_common.c
> +++ b/drivers/staging/comedi/drivers/ni_labpc_common.c
> @@ -568,15 +568,12 @@ static int labpc_ai_cmdtest(struct comedi_device *dev,
>  
>  	/* make sure scan timing is not too fast */
>  	if (cmd->scan_begin_src == TRIG_TIMER) {
> -		if (cmd->convert_src == TRIG_TIMER) {
> -			err |= comedi_check_trigger_arg_min(&cmd->
> -							    scan_begin_arg,
> -							    cmd->convert_arg *
> -							    cmd->chanlist_len);
> -		}
> +		unsigned int expected = board->ai_speed * cmd->chanlist_len;

Odd variable name, why use it?

> +
> +		if (cmd->convert_src == TRIG_TIMER)
> +			expected = cmd->convert_arg * cmd->chanlist_len;
>  		err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
> -						    board->ai_speed *
> -						    cmd->chanlist_len);
> +						    expected);
>  	}
>  
>  	switch (cmd->stop_src) {
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 398347f..1edcf2f 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -620,18 +620,18 @@ static int ni_request_ao_mite_channel(struct comedi_device *dev)
>  }
>  
>  static int ni_request_gpct_mite_channel(struct comedi_device *dev,
> -					unsigned int gpct_index,
> +					unsigned int index,
>  					enum comedi_io_direction direction)
>  {
>  	struct ni_private *devpriv = dev->private;
> -	struct ni_gpct *counter = &devpriv->counter_dev->counters[gpct_index];
> +	struct ni_gpct *counter = &devpriv->counter_dev->counters[index];

The original name was better, don't you think?

>  	struct mite_channel *mite_chan;
>  	unsigned long flags;
>  	unsigned int bits;
>  
>  	spin_lock_irqsave(&devpriv->mite_channel_lock, flags);
>  	mite_chan = mite_request_channel(devpriv->mite,
> -					 devpriv->gpct_mite_ring[gpct_index]);
> +					 devpriv->gpct_mite_ring[index]);
>  	if (!mite_chan) {
>  		spin_unlock_irqrestore(&devpriv->mite_channel_lock, flags);
>  		dev_err(dev->class_dev,
> @@ -643,8 +643,8 @@ static int ni_request_gpct_mite_channel(struct comedi_device *dev,
>  
>  	bits = NI_STC_DMA_CHAN_SEL(mite_chan->channel);
>  	ni_set_bitfield(dev, NI_E_DMA_G0_G1_SEL_REG,
> -			NI_E_DMA_G0_G1_SEL_MASK(gpct_index),
> -			NI_E_DMA_G0_G1_SEL(gpct_index, bits));
> +			NI_E_DMA_G0_G1_SEL_MASK(index),
> +			NI_E_DMA_G0_G1_SEL(index, bits));
>  
>  	spin_unlock_irqrestore(&devpriv->mite_channel_lock, flags);
>  	return 0;
> @@ -720,20 +720,19 @@ static void ni_release_ao_mite_channel(struct comedi_device *dev)
>  
>  #ifdef PCIDMA
>  static void ni_release_gpct_mite_channel(struct comedi_device *dev,
> -					 unsigned int gpct_index)
> +					 unsigned int index)

Same here, please leave as-is.

>  {
>  	struct ni_private *devpriv = dev->private;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&devpriv->mite_channel_lock, flags);
> -	if (devpriv->counter_dev->counters[gpct_index].mite_chan) {
> +	if (devpriv->counter_dev->counters[index].mite_chan) {
>  		struct mite_channel *mite_chan =
> -		    devpriv->counter_dev->counters[gpct_index].mite_chan;
> +		    devpriv->counter_dev->counters[index].mite_chan;
>  
>  		ni_set_bitfield(dev, NI_E_DMA_G0_G1_SEL_REG,
> -				NI_E_DMA_G0_G1_SEL_MASK(gpct_index), 0);
> -		ni_tio_set_mite_channel(&devpriv->
> -					counter_dev->counters[gpct_index],
> +				NI_E_DMA_G0_G1_SEL_MASK(index), 0);
> +		ni_tio_set_mite_channel(&devpriv->counter_dev->counters[index],
>  					NULL);
>  		mite_release_channel(mite_chan);
>  	}
> @@ -756,20 +755,20 @@ static void ni_release_cdo_mite_channel(struct comedi_device *dev)
>  }
>  
>  static void ni_e_series_enable_second_irq(struct comedi_device *dev,
> -					  unsigned int gpct_index, short enable)
> +					  unsigned int index, short enable)
>  {
>  	struct ni_private *devpriv = dev->private;
>  	unsigned int val = 0;
>  	int reg;
>  
> -	if (devpriv->is_m_series || gpct_index > 1)
> +	if (devpriv->is_m_series || index > 1)
>  		return;
>  
>  	/*
>  	 * e-series boards use the second irq signals to generate
>  	 * dma requests for their counters
>  	 */
> -	if (gpct_index == 0) {
> +	if (index == 0) {
>  		reg = NISTC_INTA2_ENA_REG;
>  		if (enable)
>  			val = NISTC_INTA_ENA_G0_GATE;
> @@ -1966,6 +1965,7 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
>  {
>  #ifdef PCIDMA
>  	unsigned int nbytes = max_count;
> +	char *err_msg = "data transfer limits greater than buffer size";
>  
>  	if (cmd->stop_arg > 0 && cmd->stop_arg < max_count)
>  		nbytes = cmd->stop_arg;
> @@ -1974,7 +1974,7 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
>  	if (nbytes > sdev->async->prealloc_bufsz) {
>  		if (cmd->stop_arg > 0)
>  			dev_err(sdev->device->class_dev,
> -				"ni_cmd_set_mite_transfer: tried exact data transfer limits greater than buffer size\n");
> +				"%s: %s\n", __func__, err_msg);

Ick, no.  What's wrong with the original code here?  No tool should have
told you it was wrong.

>  
>  		/*
>  		 * we can only transfer up to the size of the buffer.  In this
> @@ -1986,8 +1986,9 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
>  
>  	mite_init_ring_descriptors(ring, sdev, nbytes);
>  #else
> -	dev_err(sdev->device->class_dev,
> -		"ni_cmd_set_mite_transfer: exact data transfer limits not implemented yet without DMA\n");
> +	char *err_msg = "data transfer limits not implemented yet without DMA";

Same here, don't od that.

Also you now have a build warning :(

> +
> +	dev_err(sdev->device->class_dev, "%s: %s\n", __func__, err_msg);
>  #endif
>  }
>  
> @@ -4299,7 +4300,7 @@ static int pack_ad8842(int addr, int val, int *bitstring)
>  struct caldac_struct {
>  	int n_chans;
>  	int n_bits;
> -	int (*packbits)(int, int, int *);
> +	int (*packbits)(int addr, int val, int *bitstring);

You are doing something different here than the other changes.  Please
only make one logical-type of a change per patch.

thanks,

greg k-h

Re: [PATCH] staging: comedi: ni_*

[Aniruddha Shastri]

On Wed, Dec 13, 2017 at 4:20 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Dec 13, 2017 at 04:01:04AM -0600, Aniruddha Shastri wrote:
> > Fix checkpatch warnings by shortening lines and reorganizing code where needed..
> > Re-phrase the assert messages in ni_mio_common.c. This was done to meet the character limit for the message.
>
> And yet this line is over the character length :)
Aniruddha: Thanks for pointing this out, I'll amend the commit message. :)
>
>
>
> >
> > Signed-off-by: Aniruddha Shastri <aniruddha.shastri@gmail.com>
> > ---
> >  drivers/staging/comedi/drivers/ni_670x.c         |  3 +-
> >  drivers/staging/comedi/drivers/ni_atmio.c        |  8 +++--
> >  drivers/staging/comedi/drivers/ni_labpc_common.c | 13 +++-----
> >  drivers/staging/comedi/drivers/ni_mio_common.c   | 39 ++++++++++++------------
> >  drivers/staging/comedi/drivers/ni_stc.h          |  2 +-
> >  5 files changed, 33 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/ni_670x.c b/drivers/staging/comedi/drivers/ni_670x.c
> > index 1d3ff60..330536a 100644
> > --- a/drivers/staging/comedi/drivers/ni_670x.c
> > +++ b/drivers/staging/comedi/drivers/ni_670x.c
> > @@ -207,9 +207,10 @@ static int ni_670x_auto_attach(struct comedi_device *dev,
> >       s->maxdata = 0xffff;
> >       if (s->n_chan == 32) {
> >               const struct comedi_lrange **range_table_list;
> > +             unsigned int range_size = sizeof(const struct comedi_lrange *);
> >
> >               range_table_list = kmalloc_array(32,
> > -                                              sizeof(struct comedi_lrange *),
> > +                                              range_size,
>
> Not worth changing.

Aniruddha: The original checkpatch.pl warning instructed to use
const struct comedi_lrange instead of struct. Adding the 'const' put this
line over the limit, so that's why I pulled it out into a local variable.
>
>
> >                                                GFP_KERNEL);
> >               if (!range_table_list)
> >                       return -ENOMEM;
> > diff --git a/drivers/staging/comedi/drivers/ni_atmio.c b/drivers/staging/comedi/drivers/ni_atmio.c
> > index ae6ed96..6c0e91e 100644
> > --- a/drivers/staging/comedi/drivers/ni_atmio.c
> > +++ b/drivers/staging/comedi/drivers/ni_atmio.c
> > @@ -233,10 +233,12 @@ static int ni_isapnp_find_board(struct pnp_dev **dev)
> >       int i;
> >
> >       for (i = 0; i < ARRAY_SIZE(ni_boards); i++) {
> > +             int isapnp_id = ni_boards[i].isapnp_id;
> > +
> >               isapnp_dev = pnp_find_dev(NULL,
> > -                                       ISAPNP_VENDOR('N', 'I', 'C'),
> > -                                       ISAPNP_FUNCTION(ni_boards[i].
> > -                                                       isapnp_id), NULL);
> > +                                     ISAPNP_VENDOR('N', 'I', 'C'),
> > +                                     ISAPNP_FUNCTION(isapnp_id),
> > +                                     NULL);
>
> Not worth changing, please leave as-is.
Aniruddha: The checkpatch warning for this one explicitly tells us to
'Avoid multiple line dereference - prefer 'ni_boards[i].isapnp_id'".
Making the dereference in one line puts it over the line limit, so I
extracted a local variable.
>
> >
> >               if (!isapnp_dev || !isapnp_dev->card)
> >                       continue;
> > diff --git a/drivers/staging/comedi/drivers/ni_labpc_common.c b/drivers/staging/comedi/drivers/ni_labpc_common.c
> > index b0dfb8e..f29218f 100644
> > --- a/drivers/staging/comedi/drivers/ni_labpc_common.c
> > +++ b/drivers/staging/comedi/drivers/ni_labpc_common.c
> > @@ -568,15 +568,12 @@ static int labpc_ai_cmdtest(struct comedi_device *dev,
> >
> >       /* make sure scan timing is not too fast */
> >       if (cmd->scan_begin_src == TRIG_TIMER) {
> > -             if (cmd->convert_src == TRIG_TIMER) {
> > -                     err |= comedi_check_trigger_arg_min(&cmd->
> > -                                                         scan_begin_arg,
> > -                                                         cmd->convert_arg *
> > -                                                         cmd->chanlist_len);
> > -             }
> > +             unsigned int expected = board->ai_speed * cmd->chanlist_len;
>
> Odd variable name, why use it?
Aniruddha: It makes sense to use 'min' instead of 'expected', so I'll change
this accordingly. Thanks for pointing this out!
>
> > +
> > +             if (cmd->convert_src == TRIG_TIMER)
> > +                     expected = cmd->convert_arg * cmd->chanlist_len;
> >               err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
> > -                                                 board->ai_speed *
> > -                                                 cmd->chanlist_len);
> > +                                                 expected);
> >       }
> >
> >       switch (cmd->stop_src) {
> > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> > index 398347f..1edcf2f 100644
> > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> > @@ -620,18 +620,18 @@ static int ni_request_ao_mite_channel(struct comedi_device *dev)
> >  }
> >
> >  static int ni_request_gpct_mite_channel(struct comedi_device *dev,
> > -                                     unsigned int gpct_index,
> > +                                     unsigned int index,
> >                                       enum comedi_io_direction direction)
> >  {
> >       struct ni_private *devpriv = dev->private;
> > -     struct ni_gpct *counter = &devpriv->counter_dev->counters[gpct_index];
> > +     struct ni_gpct *counter = &devpriv->counter_dev->counters[index];
>
> The original name was better, don't you think?
Aniruddha: The original change is to fix the multi-line dereference in
ni_release_ao_mite_channel. I then changed all the 'gpct_index's
to 'index' for consistency. I agree that the original name is better.
I will extract a local variable instead of renaming everywhere.
>
> >       struct mite_channel *mite_chan;
> >       unsigned long flags;
> >       unsigned int bits;
> >
> >       spin_lock_irqsave(&devpriv->mite_channel_lock, flags);
> >       mite_chan = mite_request_channel(devpriv->mite,
> > -                                      devpriv->gpct_mite_ring[gpct_index]);
> > +                                      devpriv->gpct_mite_ring[index]);
> >       if (!mite_chan) {
> >               spin_unlock_irqrestore(&devpriv->mite_channel_lock, flags);
> >               dev_err(dev->class_dev,
> > @@ -643,8 +643,8 @@ static int ni_request_gpct_mite_channel(struct comedi_device *dev,
> >
> >       bits = NI_STC_DMA_CHAN_SEL(mite_chan->channel);
> >       ni_set_bitfield(dev, NI_E_DMA_G0_G1_SEL_REG,
> > -                     NI_E_DMA_G0_G1_SEL_MASK(gpct_index),
> > -                     NI_E_DMA_G0_G1_SEL(gpct_index, bits));
> > +                     NI_E_DMA_G0_G1_SEL_MASK(index),
> > +                     NI_E_DMA_G0_G1_SEL(index, bits));
> >
> >       spin_unlock_irqrestore(&devpriv->mite_channel_lock, flags);
> >       return 0;
> > @@ -720,20 +720,19 @@ static void ni_release_ao_mite_channel(struct comedi_device *dev)
> >
> >  #ifdef PCIDMA
> >  static void ni_release_gpct_mite_channel(struct comedi_device *dev,
> > -                                      unsigned int gpct_index)
> > +                                      unsigned int index)
>
> Same here, please leave as-is.
>
> >  {
> >       struct ni_private *devpriv = dev->private;
> >       unsigned long flags;
> >
> >       spin_lock_irqsave(&devpriv->mite_channel_lock, flags);
> > -     if (devpriv->counter_dev->counters[gpct_index].mite_chan) {
> > +     if (devpriv->counter_dev->counters[index].mite_chan) {
> >               struct mite_channel *mite_chan =
> > -                 devpriv->counter_dev->counters[gpct_index].mite_chan;
> > +                 devpriv->counter_dev->counters[index].mite_chan;
> >
> >               ni_set_bitfield(dev, NI_E_DMA_G0_G1_SEL_REG,
> > -                             NI_E_DMA_G0_G1_SEL_MASK(gpct_index), 0);
> > -             ni_tio_set_mite_channel(&devpriv->
> > -                                     counter_dev->counters[gpct_index],
> > +                             NI_E_DMA_G0_G1_SEL_MASK(index), 0);
> > +             ni_tio_set_mite_channel(&devpriv->counter_dev->counters[index],
> >                                       NULL);
> >               mite_release_channel(mite_chan);
> >       }
> > @@ -756,20 +755,20 @@ static void ni_release_cdo_mite_channel(struct comedi_device *dev)
> >  }
> >
> >  static void ni_e_series_enable_second_irq(struct comedi_device *dev,
> > -                                       unsigned int gpct_index, short enable)
> > +                                       unsigned int index, short enable)
> >  {
> >       struct ni_private *devpriv = dev->private;
> >       unsigned int val = 0;
> >       int reg;
> >
> > -     if (devpriv->is_m_series || gpct_index > 1)
> > +     if (devpriv->is_m_series || index > 1)
> >               return;
> >
> >       /*
> >        * e-series boards use the second irq signals to generate
> >        * dma requests for their counters
> >        */
> > -     if (gpct_index == 0) {
> > +     if (index == 0) {
> >               reg = NISTC_INTA2_ENA_REG;
> >               if (enable)
> >                       val = NISTC_INTA_ENA_G0_GATE;
> > @@ -1966,6 +1965,7 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
> >  {
> >  #ifdef PCIDMA
> >       unsigned int nbytes = max_count;
> > +     char *err_msg = "data transfer limits greater than buffer size";
> >
> >       if (cmd->stop_arg > 0 && cmd->stop_arg < max_count)
> >               nbytes = cmd->stop_arg;
> > @@ -1974,7 +1974,7 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
> >       if (nbytes > sdev->async->prealloc_bufsz) {
> >               if (cmd->stop_arg > 0)
> >                       dev_err(sdev->device->class_dev,
> > -                             "ni_cmd_set_mite_transfer: tried exact data transfer limits greater than buffer size\n");
> > +                             "%s: %s\n", __func__, err_msg);
>
> Ick, no.  What's wrong with the original code here?  No tool should have
> told you it was wrong.
Aniruddha: I dislike this too. The checkpatch.pl tool says to use __func__
instead of the function name. But that puts the line over the character limit.
So I had to extract the error string. Even that is too long, so I had to edit
the message to make it shorter. If there is a better way, please tell me!
>
> >
> >               /*
> >                * we can only transfer up to the size of the buffer.  In this
> > @@ -1986,8 +1986,9 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
> >
> >       mite_init_ring_descriptors(ring, sdev, nbytes);
> >  #else
> > -     dev_err(sdev->device->class_dev,
> > -             "ni_cmd_set_mite_transfer: exact data transfer limits not implemented yet without DMA\n");
> > +     char *err_msg = "data transfer limits not implemented yet without DMA";
>
> Same here, don't od that.
>
> Also you now have a build warning :(
>
> > +
> > +     dev_err(sdev->device->class_dev, "%s: %s\n", __func__, err_msg);
> >  #endif
> >  }
> >
> > @@ -4299,7 +4300,7 @@ static int pack_ad8842(int addr, int val, int *bitstring)
> >  struct caldac_struct {
> >       int n_chans;
> >       int n_bits;
> > -     int (*packbits)(int, int, int *);
> > +     int (*packbits)(int addr, int val, int *bitstring);
>
> You are doing something different here than the other changes.  Please
> only make one logical-type of a change per patch.
Aniruddha: This addresses the checkpatch.pl warning that the function
definition argument should have an identifier name. All the methods use these
function parameter names. (like pack_mb88341)
>
> thanks,
>
> greg k-h
Aniruddha: This is my first patch in the Linux kernel so I'm still getting
a hang of things. Thanks for your patience everyone!

Thanks,
Aniruddha

Re: [PATCH] staging: comedi: ni_*

[Greg Kroah-Hartman]

On Wed, Dec 13, 2017 at 05:16:13PM +0530, Aniruddha Shastri wrote:
> On Wed, Dec 13, 2017 at 4:20 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Dec 13, 2017 at 04:01:04AM -0600, Aniruddha Shastri wrote:
> > > Fix checkpatch warnings by shortening lines and reorganizing code where needed..
> > > Re-phrase the assert messages in ni_mio_common.c. This was done to meet the character limit for the message.
> >
> > And yet this line is over the character length :)
> Aniruddha: Thanks for pointing this out, I'll amend the commit message. :)

Why put your name in the response?  Of course I know it's you who is
writing this :)

> > >               range_table_list = kmalloc_array(32,
> > > -                                              sizeof(struct comedi_lrange *),
> > > +                                              range_size,
> >
> > Not worth changing.
> 
> Aniruddha: The original checkpatch.pl warning instructed to use
> const struct comedi_lrange instead of struct. Adding the 'const' put this
> line over the limit, so that's why I pulled it out into a local variable.

Checkpatch is a "hint", don't always follow it, it can cause some code
to look really bad.  It's not a hard-rule.

thanks,

greg k-h

[PATCH v2] staging: comedi: ni_*: Fix style warnings.

[Aniruddha Shastri]

Three of these warnings are now line-too-long warnings. I think these 
warnings are preferable to the ones listed below. The longest line 
is only 87 chars wide, which is reasonable.

Warnings fixed:
ni_670x.c:212: WARNING: struct comedi_lrange should normally be const
ni_atmio.c:239: WARNING: Avoid multiple line dereference
	- prefer 'ni_boards[i].isapnp_id'
ni_labpc_common.c:573: WARNING: Avoid multiple line dereference
	- prefer 'cmd->scan_begin_arg'
ni_mio_common.c:736: WARNING: Avoid multiple line dereference
	- prefer 'devpriv->counter_dev->counters[gpct_index]'
ni_mio_common.c:1977: WARNING: Prefer using '"%s...", __func__'
	to using 'ni_cmd_set_mite_transfer', this function's name, in a string
ni_mio_common.c:1990: WARNING: Prefer using '"%s...", __func__'
	to using 'ni_cmd_set_mite_transfer', this function's name, in a string
ni_mio_common.c:4302: WARNING: function definition argument
	'int' should also have an identifier name
ni_mio_common.c:4302: WARNING: function definition argument
	'int' should also have an identifier name
ni_mio_common.c:4302: WARNING: function definition argument
	'int *' should also have an identifier name
ni_mio_common.c:4699: WARNING: Prefer using '"%s...", __func__'
	to using 'cs5529_do_conversion', this function's name, in a string
ni_stc.h:21: WARNING: Block comments should align the * on each line

Signed-off-by: Aniruddha Shastri <aniruddha.shastri@gmail.com>
---
Changes in v2:
No longer go out of our way to fix minor line-length violations at the expense 
of code quality.

 drivers/staging/comedi/drivers/ni_670x.c         |  2 +-
 drivers/staging/comedi/drivers/ni_atmio.c        |  6 +++---
 drivers/staging/comedi/drivers/ni_labpc_common.c |  3 +--
 drivers/staging/comedi/drivers/ni_mio_common.c   | 14 ++++++++------
 drivers/staging/comedi/drivers/ni_stc.h          |  2 +-
 5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_670x.c b/drivers/staging/comedi/drivers/ni_670x.c
index 1d3ff60..ecf663355 100644
--- a/drivers/staging/comedi/drivers/ni_670x.c
+++ b/drivers/staging/comedi/drivers/ni_670x.c
@@ -209,7 +209,7 @@ static int ni_670x_auto_attach(struct comedi_device *dev,
 		const struct comedi_lrange **range_table_list;
 
 		range_table_list = kmalloc_array(32,
-						 sizeof(struct comedi_lrange *),
+						 sizeof(const struct comedi_lrange *),
 						 GFP_KERNEL);
 		if (!range_table_list)
 			return -ENOMEM;
diff --git a/drivers/staging/comedi/drivers/ni_atmio.c b/drivers/staging/comedi/drivers/ni_atmio.c
index ae6ed96..f9a8638 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -234,9 +234,9 @@ static int ni_isapnp_find_board(struct pnp_dev **dev)
 
 	for (i = 0; i < ARRAY_SIZE(ni_boards); i++) {
 		isapnp_dev = pnp_find_dev(NULL,
-					  ISAPNP_VENDOR('N', 'I', 'C'),
-					  ISAPNP_FUNCTION(ni_boards[i].
-							  isapnp_id), NULL);
+					ISAPNP_VENDOR('N', 'I', 'C'),
+					ISAPNP_FUNCTION(ni_boards[i].isapnp_id),
+					NULL);
 
 		if (!isapnp_dev || !isapnp_dev->card)
 			continue;
diff --git a/drivers/staging/comedi/drivers/ni_labpc_common.c b/drivers/staging/comedi/drivers/ni_labpc_common.c
index b0dfb8e..a38910e 100644
--- a/drivers/staging/comedi/drivers/ni_labpc_common.c
+++ b/drivers/staging/comedi/drivers/ni_labpc_common.c
@@ -569,8 +569,7 @@ static int labpc_ai_cmdtest(struct comedi_device *dev,
 	/* make sure scan timing is not too fast */
 	if (cmd->scan_begin_src == TRIG_TIMER) {
 		if (cmd->convert_src == TRIG_TIMER) {
-			err |= comedi_check_trigger_arg_min(&cmd->
-							    scan_begin_arg,
+			err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
 							    cmd->convert_arg *
 							    cmd->chanlist_len);
 		}
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 398347f..c4a4f5c 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -732,8 +732,7 @@ static void ni_release_gpct_mite_channel(struct comedi_device *dev,
 
 		ni_set_bitfield(dev, NI_E_DMA_G0_G1_SEL_REG,
 				NI_E_DMA_G0_G1_SEL_MASK(gpct_index), 0);
-		ni_tio_set_mite_channel(&devpriv->
-					counter_dev->counters[gpct_index],
+		ni_tio_set_mite_channel(&devpriv->counter_dev->counters[gpct_index],
 					NULL);
 		mite_release_channel(mite_chan);
 	}
@@ -1974,7 +1973,8 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
 	if (nbytes > sdev->async->prealloc_bufsz) {
 		if (cmd->stop_arg > 0)
 			dev_err(sdev->device->class_dev,
-				"ni_cmd_set_mite_transfer: tried exact data transfer limits greater than buffer size\n");
+				"%s: tried exact data transfer limits greater than buffer size\n",
+				__func__);
 
 		/*
 		 * we can only transfer up to the size of the buffer.  In this
@@ -1987,7 +1987,8 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
 	mite_init_ring_descriptors(ring, sdev, nbytes);
 #else
 	dev_err(sdev->device->class_dev,
-		"ni_cmd_set_mite_transfer: exact data transfer limits not implemented yet without DMA\n");
+		"%s: exact data transfer limits not implemented yet without DMA\n",
+		__func__);
 #endif
 }
 
@@ -4299,7 +4300,7 @@ static int pack_ad8842(int addr, int val, int *bitstring)
 struct caldac_struct {
 	int n_chans;
 	int n_bits;
-	int (*packbits)(int, int, int *);
+	int (*packbits)(int addr, int val, int *bitstring);
 };
 
 static struct caldac_struct caldacs[] = {
@@ -4696,7 +4697,8 @@ static int cs5529_do_conversion(struct comedi_device *dev,
 	retval = cs5529_wait_for_idle(dev);
 	if (retval) {
 		dev_err(dev->class_dev,
-			"timeout or signal in cs5529_do_conversion()\n");
+			"timeout or signal in %s()\n",
+			__func__);
 		return -ETIME;
 	}
 	status = ni_ao_win_inw(dev, NI67XX_CAL_STATUS_REG);
diff --git a/drivers/staging/comedi/drivers/ni_stc.h b/drivers/staging/comedi/drivers/ni_stc.h
index 61138e8..03099a7 100644
--- a/drivers/staging/comedi/drivers/ni_stc.h
+++ b/drivers/staging/comedi/drivers/ni_stc.h
@@ -18,7 +18,7 @@
 /*
  * References:
  *   DAQ-STC Technical Reference Manual
-*/
+ */
 
 #ifndef _COMEDI_NI_STC_H
 #define _COMEDI_NI_STC_H
-- 
2.7.4

Re: [PATCH v2] staging: comedi: ni_*: Fix style warnings.

[Joe Perches]

On Thu, 2017-12-14 at 00:27 -0600, Aniruddha Shastri wrote:
> Three of these warnings are now line-too-long warnings. I think these 
> warnings are preferable to the ones listed below. The longest line 
> is only 87 chars wide, which is reasonable.
[]
> diff --git a/drivers/staging/comedi/drivers/ni_670x.c b/drivers/staging/comedi/drivers/ni_670x.c
[]
> @@ -209,7 +209,7 @@ static int ni_670x_auto_attach(struct comedi_device *dev,
>  		const struct comedi_lrange **range_table_list;
>  
>  		range_table_list = kmalloc_array(32,
> -						 sizeof(struct comedi_lrange *),
> +						 sizeof(const struct comedi_lrange *),

Adding const to a sizeof is unnecessary

[PATCH v3] staging: comedi: ni_*: Fix style warnings.

[Aniruddha Shastri]

Two of these warnings are now line-too-long warnings. I think these 
warnings are preferable to the ones listed below. The longest line is
only 85 chars wide, which is reasonable.

Warnings fixed:
ni_atmio.c:239: WARNING: Avoid multiple line dereference
	- prefer 'ni_boards[i].isapnp_id'
ni_labpc_common.c:573: WARNING: Avoid multiple line dereference
	- prefer 'cmd->scan_begin_arg'
ni_mio_common.c:736: WARNING: Avoid multiple line dereference
	- prefer 'devpriv->counter_dev->counters[gpct_index]'
ni_mio_common.c:1977: WARNING: Prefer using '"%s...", __func__'
	to using 'ni_cmd_set_mite_transfer', this function's name, in a string
ni_mio_common.c:1990: WARNING: Prefer using '"%s...", __func__'
	to using 'ni_cmd_set_mite_transfer', this function's name, in a string
ni_mio_common.c:4302: WARNING: function definition argument
	'int' should also have an identifier name
ni_mio_common.c:4302: WARNING: function definition argument
	'int' should also have an identifier name
ni_mio_common.c:4302: WARNING: function definition argument
	'int *' should also have an identifier name
ni_mio_common.c:4699: WARNING: Prefer using '"%s...", __func__'
	to using 'cs5529_do_conversion', this function's name, in a string
ni_stc.h:21: WARNING: Block comments should align the * on each line

Signed-off-by: Aniruddha Shastri <aniruddha.shastri@gmail.com>
---
Changes in v3:
No longer add 'const' to sizeof(struct comedi_lrange) in ni_670x.c:212

 drivers/staging/comedi/drivers/ni_atmio.c        |  6 +++---
 drivers/staging/comedi/drivers/ni_labpc_common.c |  3 +--
 drivers/staging/comedi/drivers/ni_mio_common.c   | 14 ++++++++------
 drivers/staging/comedi/drivers/ni_stc.h          |  2 +-
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_atmio.c b/drivers/staging/comedi/drivers/ni_atmio.c
index ae6ed96..f9a8638 100644
--- a/drivers/staging/comedi/drivers/ni_atmio.c
+++ b/drivers/staging/comedi/drivers/ni_atmio.c
@@ -234,9 +234,9 @@ static int ni_isapnp_find_board(struct pnp_dev **dev)
 
 	for (i = 0; i < ARRAY_SIZE(ni_boards); i++) {
 		isapnp_dev = pnp_find_dev(NULL,
-					  ISAPNP_VENDOR('N', 'I', 'C'),
-					  ISAPNP_FUNCTION(ni_boards[i].
-							  isapnp_id), NULL);
+					ISAPNP_VENDOR('N', 'I', 'C'),
+					ISAPNP_FUNCTION(ni_boards[i].isapnp_id),
+					NULL);
 
 		if (!isapnp_dev || !isapnp_dev->card)
 			continue;
diff --git a/drivers/staging/comedi/drivers/ni_labpc_common.c b/drivers/staging/comedi/drivers/ni_labpc_common.c
index b0dfb8e..a38910e 100644
--- a/drivers/staging/comedi/drivers/ni_labpc_common.c
+++ b/drivers/staging/comedi/drivers/ni_labpc_common.c
@@ -569,8 +569,7 @@ static int labpc_ai_cmdtest(struct comedi_device *dev,
 	/* make sure scan timing is not too fast */
 	if (cmd->scan_begin_src == TRIG_TIMER) {
 		if (cmd->convert_src == TRIG_TIMER) {
-			err |= comedi_check_trigger_arg_min(&cmd->
-							    scan_begin_arg,
+			err |= comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
 							    cmd->convert_arg *
 							    cmd->chanlist_len);
 		}
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index 398347f..c4a4f5c 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -732,8 +732,7 @@ static void ni_release_gpct_mite_channel(struct comedi_device *dev,
 
 		ni_set_bitfield(dev, NI_E_DMA_G0_G1_SEL_REG,
 				NI_E_DMA_G0_G1_SEL_MASK(gpct_index), 0);
-		ni_tio_set_mite_channel(&devpriv->
-					counter_dev->counters[gpct_index],
+		ni_tio_set_mite_channel(&devpriv->counter_dev->counters[gpct_index],
 					NULL);
 		mite_release_channel(mite_chan);
 	}
@@ -1974,7 +1973,8 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
 	if (nbytes > sdev->async->prealloc_bufsz) {
 		if (cmd->stop_arg > 0)
 			dev_err(sdev->device->class_dev,
-				"ni_cmd_set_mite_transfer: tried exact data transfer limits greater than buffer size\n");
+				"%s: tried exact data transfer limits greater than buffer size\n",
+				__func__);
 
 		/*
 		 * we can only transfer up to the size of the buffer.  In this
@@ -1987,7 +1987,8 @@ static void ni_cmd_set_mite_transfer(struct mite_ring *ring,
 	mite_init_ring_descriptors(ring, sdev, nbytes);
 #else
 	dev_err(sdev->device->class_dev,
-		"ni_cmd_set_mite_transfer: exact data transfer limits not implemented yet without DMA\n");
+		"%s: exact data transfer limits not implemented yet without DMA\n",
+		__func__);
 #endif
 }
 
@@ -4299,7 +4300,7 @@ static int pack_ad8842(int addr, int val, int *bitstring)
 struct caldac_struct {
 	int n_chans;
 	int n_bits;
-	int (*packbits)(int, int, int *);
+	int (*packbits)(int addr, int val, int *bitstring);
 };
 
 static struct caldac_struct caldacs[] = {
@@ -4696,7 +4697,8 @@ static int cs5529_do_conversion(struct comedi_device *dev,
 	retval = cs5529_wait_for_idle(dev);
 	if (retval) {
 		dev_err(dev->class_dev,
-			"timeout or signal in cs5529_do_conversion()\n");
+			"timeout or signal in %s()\n",
+			__func__);
 		return -ETIME;
 	}
 	status = ni_ao_win_inw(dev, NI67XX_CAL_STATUS_REG);
diff --git a/drivers/staging/comedi/drivers/ni_stc.h b/drivers/staging/comedi/drivers/ni_stc.h
index 61138e8..03099a7 100644
--- a/drivers/staging/comedi/drivers/ni_stc.h
+++ b/drivers/staging/comedi/drivers/ni_stc.h
@@ -18,7 +18,7 @@
 /*
  * References:
  *   DAQ-STC Technical Reference Manual
-*/
+ */
 
 #ifndef _COMEDI_NI_STC_H
 #define _COMEDI_NI_STC_H
-- 
2.7.4

Re: [PATCH] staging: comedi: ni_*

[Aniruddha Shastri]

On Wed, Dec 13, 2017 at 6:07 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Dec 13, 2017 at 05:16:13PM +0530, Aniruddha Shastri wrote:
>> On Wed, Dec 13, 2017 at 4:20 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> >
>> > On Wed, Dec 13, 2017 at 04:01:04AM -0600, Aniruddha Shastri wrote:
>> > > Fix checkpatch warnings by shortening lines and reorganizing code where needed..
>> > > Re-phrase the assert messages in ni_mio_common.c. This was done to meet the character limit for the message.
>> >
>> > And yet this line is over the character length :)
>> Aniruddha: Thanks for pointing this out, I'll amend the commit message. :)
>
> Why put your name in the response?  Of course I know it's you who is
> writing this :)
I thought it might be clearer for others to read our exchange, but that only
works if everyone does the same. Otherwise, it's just clutter. I'm used to
seeing a name and timestamp next to comments in other code review
systems (like TFS), but this doesn't appear to be the Linux way :)
>
>> > >               range_table_list = kmalloc_array(32,
>> > > -                                              sizeof(struct comedi_lrange *),
>> > > +                                              range_size,
>> >
>> > Not worth changing.
>>
>> Aniruddha: The original checkpatch.pl warning instructed to use
>> const struct comedi_lrange instead of struct. Adding the 'const' put this
>> line over the limit, so that's why I pulled it out into a local variable.
>
> Checkpatch is a "hint", don't always follow it, it can cause some code
> to look really bad.  It's not a hard-rule.
That's a relief :). I've sent updated patches that only clean-up where
needed.
>
> thanks,
>
> greg k-h

Re: [PATCH v3] staging: comedi: ni_*: Fix style warnings.

[Greg Kroah-Hartman]

On Thu, Dec 14, 2017 at 01:31:05AM -0600, Aniruddha Shastri wrote:
> Two of these warnings are now line-too-long warnings. I think these 
> warnings are preferable to the ones listed below. The longest line is
> only 85 chars wide, which is reasonable.
> 
> Warnings fixed:
> ni_atmio.c:239: WARNING: Avoid multiple line dereference
> 	- prefer 'ni_boards[i].isapnp_id'
> ni_labpc_common.c:573: WARNING: Avoid multiple line dereference
> 	- prefer 'cmd->scan_begin_arg'
> ni_mio_common.c:736: WARNING: Avoid multiple line dereference
> 	- prefer 'devpriv->counter_dev->counters[gpct_index]'
> ni_mio_common.c:1977: WARNING: Prefer using '"%s...", __func__'
> 	to using 'ni_cmd_set_mite_transfer', this function's name, in a string
> ni_mio_common.c:1990: WARNING: Prefer using '"%s...", __func__'
> 	to using 'ni_cmd_set_mite_transfer', this function's name, in a string
> ni_mio_common.c:4302: WARNING: function definition argument
> 	'int' should also have an identifier name
> ni_mio_common.c:4302: WARNING: function definition argument
> 	'int' should also have an identifier name
> ni_mio_common.c:4302: WARNING: function definition argument
> 	'int *' should also have an identifier name
> ni_mio_common.c:4699: WARNING: Prefer using '"%s...", __func__'
> 	to using 'cs5529_do_conversion', this function's name, in a string
> ni_stc.h:21: WARNING: Block comments should align the * on each line
> 
> Signed-off-by: Aniruddha Shastri <aniruddha.shastri@gmail.com>
> ---
> Changes in v3:
> No longer add 'const' to sizeof(struct comedi_lrange) in ni_670x.c:212

Please only fix one "type" of issue per patch.  This should be broken up
into multiple patches and sent as a patch series.

thanks,

greg k-h