2014-04-04 10:05:44

by Kumar Amit Mehta

Subject: [PATCH] staging: comedi: drivers: pcl812.c: fixed a coding style issue

Fixed a coding style issue. Reported by checkpatch.pl

Signed-off-by: Kumar Amit Mehta <[email protected]>
---
drivers/staging/comedi/drivers/pcl812.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/pcl812.c b/drivers/staging/comedi/drivers/pcl812.c
index 160eac8..5cc01fe 100644
--- a/drivers/staging/comedi/drivers/pcl812.c
+++ b/drivers/staging/comedi/drivers/pcl812.c
@@ -811,8 +811,9 @@ static int pcl812_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
devpriv->ai_dma = 0;
break;
}
- } else
+ } else {
devpriv->ai_dma = 0;
+ }

devpriv->ai_act_scan = 0;
devpriv->ai_poll_ptr = 0;
--
1.8.5.3

2014-04-04 11:08:10

by Dan Carpenter

Subject: Re: [PATCH] staging: comedi: drivers: pcl812.c: fixed a coding style issue

On Fri, Apr 04, 2014 at 01:05:29PM +0300, Kumar Amit Mehta wrote:
> Fixed a coding style issue. Reported by checkpatch.pl
>

It's better if the commit messages are more specific than this.

regards,
dan carpenter

2014-04-04 11:49:08

by Kumar Amit Mehta

Subject: Re: [PATCH] staging: comedi: drivers: pcl812.c: fixed a coding style issue

On Fri, Apr 04, 2014 at 02:07:07PM +0300, Dan Carpenter wrote:
> On Fri, Apr 04, 2014 at 01:05:29PM +0300, Kumar Amit Mehta wrote:
> > Fixed a coding style issue. Reported by checkpatch.pl
> >
>
> It's better if the commit messages are more specific than this.

So, should I resend the patch with a more appropriate commit message ?

Thanks,
Kumar

2014-04-04 12:39:23

by walter harms

Subject: Re: [PATCH] staging: comedi: drivers: pcl812.c: fixed a coding style issue

Am 04.04.2014 12:05, schrieb Kumar Amit Mehta:
> Fixed a coding style issue. Reported by checkpatch.pl
>
> Signed-off-by: Kumar Amit Mehta <[email protected]>
> ---
> drivers/staging/comedi/drivers/pcl812.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/pcl812.c b/drivers/staging/comedi/drivers/pcl812.c
> index 160eac8..5cc01fe 100644
> --- a/drivers/staging/comedi/drivers/pcl812.c
> +++ b/drivers/staging/comedi/drivers/pcl812.c
> @@ -811,8 +811,9 @@ static int pcl812_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
> devpriv->ai_dma = 0;
> break;
> }
> - } else
> + } else {
> devpriv->ai_dma = 0;
> + }
>
> devpriv->ai_act_scan = 0;
> devpriv->ai_poll_ptr = 0;

hi Kumar,
is that else needed at all ? perhaps it is possible to devpriv->ai_dma=0 before the if ?
That would reduce code and give a better readability.

re,
wh

2014-04-04 13:27:27

by Dan Carpenter

Subject: Re: [PATCH] staging: comedi: drivers: pcl812.c: fixed a coding style issue

On Fri, Apr 04, 2014 at 02:48:52PM +0300, Kumar Amit Mehta wrote:
> On Fri, Apr 04, 2014 at 02:07:07PM +0300, Dan Carpenter wrote:
> > On Fri, Apr 04, 2014 at 01:05:29PM +0300, Kumar Amit Mehta wrote:
> > > Fixed a coding style issue. Reported by checkpatch.pl
> > >
> >
> > It's better if the commit messages are more specific than this.
>
> So, should I resend the patch with a more appropriate commit message ?

[PATCH] staging: comedi: drivers: pcl812.c: add curly braces for checkpatch

Kernel style is that if one side of the if else statement gets has curly
braces then both side should have them.

regards,
dan carpenter

2014-04-04 15:18:11

by Kumar Amit Mehta

Subject: Re: [PATCH] staging: comedi: drivers: pcl812.c: fixed a coding style issue

On Fri, Apr 04, 2014 at 04:26:51PM +0300, Dan Carpenter wrote:
> On Fri, Apr 04, 2014 at 02:48:52PM +0300, Kumar Amit Mehta wrote:
> > On Fri, Apr 04, 2014 at 02:07:07PM +0300, Dan Carpenter wrote:
> > > On Fri, Apr 04, 2014 at 01:05:29PM +0300, Kumar Amit Mehta wrote:
> > > > Fixed a coding style issue. Reported by checkpatch.pl
> > > >
> > >
> > > It's better if the commit messages are more specific than this.
> >
> > So, should I resend the patch with a more appropriate commit message ?
>
> [PATCH] staging: comedi: drivers: pcl812.c: add curly braces for checkpatch
>
> Kernel style is that if one side of the if else statement gets has curly
> braces then both side should have them.

Dan,

What Walter mentioned also makes sense, So shouldn't it be something
like this:

[amit@localhost linux-next]$ git diff
diff --git a/drivers/staging/comedi/drivers/pcl812.c
b/drivers/staging/comedi/drivers/pcl812.c
index 160eac8..552b696 100644
--- a/drivers/staging/comedi/drivers/pcl812.c
+++ b/drivers/staging/comedi/drivers/pcl812.c
@@ -803,16 +803,14 @@ static int pcl812_ai_cmd(struct comedi_device
*dev, struct comedi_subdevice *s)

pcl812_ai_set_chan_range(dev, cmd->chanlist[0], 1);

+ devpriv->ai_dma = 0;
if (devpriv->dma) { /* check if we can use DMA transfer */
devpriv->ai_dma = 1;
for (i = 1; i < cmd->chanlist_len; i++)
if (cmd->chanlist[0] != cmd->chanlist[i]) {
/* we cann't use DMA :-( */
- devpriv->ai_dma = 0;
break;
}
- } else
- devpriv->ai_dma = 0;

devpriv->ai_act_scan = 0;
devpriv->ai_poll_ptr = 0;


Thanks,
Kumar

2014-04-04 15:28:16

by Joe Perches

Subject: Re: [PATCH] staging: comedi: drivers: pcl812.c: fixed a coding style issue

On Fri, 2014-04-04 at 18:17 +0300, Kumar Amit Mehta wrote:
> What Walter mentioned also makes sense, So shouldn't it be something
> like this:

I'm not Dan, but:

> [amit@localhost linux-next]$ git diff
> diff --git a/drivers/staging/comedi/drivers/pcl812.c
> b/drivers/staging/comedi/drivers/pcl812.c
> index 160eac8..552b696 100644
> --- a/drivers/staging/comedi/drivers/pcl812.c
> +++ b/drivers/staging/comedi/drivers/pcl812.c
> @@ -803,16 +803,14 @@ static int pcl812_ai_cmd(struct comedi_device
> *dev, struct comedi_subdevice *s)
>
> pcl812_ai_set_chan_range(dev, cmd->chanlist[0], 1);
>
> + devpriv->ai_dma = 0;
> if (devpriv->dma) { /* check if we can use DMA transfer */
> devpriv->ai_dma = 1;
> for (i = 1; i < cmd->chanlist_len; i++)
> if (cmd->chanlist[0] != cmd->chanlist[i]) {
> /* we cann't use DMA :-( */
> - devpriv->ai_dma = 0;

No.

You enable unwanted DMA capability here.
There's a typo of can't too.

> break;
> }
> - } else
> - devpriv->ai_dma = 0;

Otherwise, I suppose either style is OK.

2014-04-04 15:33:40

by Dan Carpenter

Subject: Re: [PATCH] staging: comedi: drivers: pcl812.c: fixed a coding style issue

On Fri, Apr 04, 2014 at 06:17:56PM +0300, Kumar Amit Mehta wrote:
> On Fri, Apr 04, 2014 at 04:26:51PM +0300, Dan Carpenter wrote:
> > On Fri, Apr 04, 2014 at 02:48:52PM +0300, Kumar Amit Mehta wrote:
> > > On Fri, Apr 04, 2014 at 02:07:07PM +0300, Dan Carpenter wrote:
> > > > On Fri, Apr 04, 2014 at 01:05:29PM +0300, Kumar Amit Mehta wrote:
> > > > > Fixed a coding style issue. Reported by checkpatch.pl
> > > > >
> > > >
> > > > It's better if the commit messages are more specific than this.
> > >
> > > So, should I resend the patch with a more appropriate commit message ?
> >
> > [PATCH] staging: comedi: drivers: pcl812.c: add curly braces for checkpatch
> >
> > Kernel style is that if one side of the if else statement gets has curly
> > braces then both side should have them.
>
> Dan,
>
> What Walter mentioned also makes sense, So shouldn't it be something
> like this:
>
> [amit@localhost linux-next]$ git diff
> diff --git a/drivers/staging/comedi/drivers/pcl812.c
> b/drivers/staging/comedi/drivers/pcl812.c
> index 160eac8..552b696 100644
> --- a/drivers/staging/comedi/drivers/pcl812.c
> +++ b/drivers/staging/comedi/drivers/pcl812.c
> @@ -803,16 +803,14 @@ static int pcl812_ai_cmd(struct comedi_device
> *dev, struct comedi_subdevice *s)
>
> pcl812_ai_set_chan_range(dev, cmd->chanlist[0], 1);
>
> + devpriv->ai_dma = 0;
> if (devpriv->dma) { /* check if we can use DMA transfer */
> devpriv->ai_dma = 1;
> for (i = 1; i < cmd->chanlist_len; i++)
> if (cmd->chanlist[0] != cmd->chanlist[i]) {
> /* we cann't use DMA :-( */
> - devpriv->ai_dma = 0;

You'd still want this assignment.

> break;
> }
> - } else
> - devpriv->ai_dma = 0;

regards,
dan carpenter