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

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

[Kumar Amit Mehta]

Fixed a coding style issue. Reported by checkpatch.pl

Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
---
 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

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

[Dan Carpenter]

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

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

[Kumar Amit Mehta]

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

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

[walter harms]

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 <gmate.amit@gmail.com>
> ---
>  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

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

[Dan Carpenter]

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

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

[Kumar Amit Mehta]

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

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

[Joe Perches]

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.

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

[Dan Carpenter]

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