Re: [PATCH] staging: comedi: ni_*

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> ---
>  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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux