From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Aniruddha Shastri <aniruddha.shastri@gmail.com>
Cc: devel@driverdev.osuosl.org,
"Christopher Mårtensson" <cribalik@gmail.com>,
linux-kernel@vger.kernel.org, "Ian Abbott" <abbotti@mev.co.uk>,
"Saber Rezvani" <irsaber@gmail.com>,
"Matthew Giassa" <matthew@giassa.net>,
"Karthik Nayak" <karthik.188@gmail.com>
Subject: Re: [PATCH] staging: comedi: ni_*
Date: Wed, 13 Dec 2017 11:50:19 +0100 [thread overview]
Message-ID: <20171213105019.GA2546@kroah.com> (raw)
In-Reply-To: <1513159273-39619-1-git-send-email-aniruddha.shastri@gmail.com>
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
next prev parent reply other threads:[~2017-12-13 10:50 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-13 10:01 [PATCH] staging: comedi: ni_* Aniruddha Shastri
2017-12-13 10:50 ` Greg Kroah-Hartman [this message]
2017-12-13 11:46 ` Aniruddha Shastri
2017-12-13 12:07 ` Greg Kroah-Hartman
2017-12-14 6:27 ` [PATCH v2] staging: comedi: ni_*: Fix style warnings Aniruddha Shastri
2017-12-14 6:48 ` Joe Perches
2017-12-14 7:31 ` [PATCH v3] " Aniruddha Shastri
2017-12-19 13:59 ` Greg Kroah-Hartman
2017-12-14 7:48 ` [PATCH] staging: comedi: ni_* Aniruddha Shastri
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171213105019.GA2546@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=abbotti@mev.co.uk \
--cc=aniruddha.shastri@gmail.com \
--cc=cribalik@gmail.com \
--cc=devel@driverdev.osuosl.org \
--cc=irsaber@gmail.com \
--cc=karthik.188@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=matthew@giassa.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.