[PATCH] staging: comedi: cb_pcidas64: Fixed coding style errors and an if-statement

[PATCH] staging: comedi: cb_pcidas64: Fixed coding style errors and an if-statement

[Dimitrios Semitsoglou-Tsiapos]

Fixed a few coding style errors and an if-statement responsible for a
sanity check inside the prep_ao_dma function.

Signed-off-by: Dimitrios Semitsoglou-Tsiapos <dimitrios.semitsoglou@gmail.com>
---
 drivers/staging/comedi/drivers/cb_pcidas64.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
index 9d0b875..06cff3d 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -1153,12 +1153,14 @@ static int eeprom_read_insn(struct comedi_device *dev,
 static void check_adc_timing(struct comedi_device *dev, struct comedi_cmd *cmd);
 static unsigned int get_divisor(unsigned int ns, unsigned int flags);
 static void i2c_write(struct comedi_device *dev, unsigned int address,
-		      const uint8_t * data, unsigned int length);
+		      const uint8_t *data, unsigned int length);
 static void caldac_write(struct comedi_device *dev, unsigned int channel,
 			 unsigned int value);
 static int caldac_8800_write(struct comedi_device *dev, unsigned int address,
 			     uint8_t value);
-/* static int dac_1590_write(struct comedi_device *dev, unsigned int dac_a, unsigned int dac_b); */
+/* static int dac_1590_write(struct comedi_device *dev, unsigned int dac_a,
+ *			     unsigned int dac_b);
+ */
 static int caldac_i2c_write(struct comedi_device *dev,
 			    unsigned int caldac_channel, unsigned int value);
 static void abort_dma(struct comedi_device *dev, unsigned int channel);
@@ -1230,7 +1232,7 @@ static unsigned int hw_revision(const struct comedi_device *dev,
 }
 
 static void set_dac_range_bits(struct comedi_device *dev,
-			       volatile uint16_t * bits, unsigned int channel,
+			       volatile uint16_t *bits, unsigned int channel,
 			       unsigned int range)
 {
 	unsigned int code = board(dev)->ao_range_code[range];
@@ -3392,8 +3394,8 @@ static int prep_ao_dma(struct comedi_device *dev, const struct comedi_cmd *cmd)
 	num_bytes = load_ao_dma_buffer(dev, cmd);
 	if (num_bytes == 0)
 		return -1;
-	if (num_bytes >= DMA_BUFFER_SIZE) ;
-	load_ao_dma(dev, cmd);
+	if (num_bytes >= DMA_BUFFER_SIZE)
+		load_ao_dma(dev, cmd);
 
 	dma_start_sync(dev, 0);
 
@@ -4191,7 +4193,7 @@ static void i2c_stop(struct comedi_device *dev)
 }
 
 static void i2c_write(struct comedi_device *dev, unsigned int address,
-		      const uint8_t * data, unsigned int length)
+		      const uint8_t *data, unsigned int length)
 {
 	unsigned int i;
 	uint8_t bitstream;
-- 
1.7.10.2

Re: [PATCH] staging: comedi: cb_pcidas64: Fixed coding style errors and an if-statement

[richard -rw- weinberger]

On Mon, May 28, 2012 at 8:28 PM, Dimitrios Semitsoglou-Tsiapos
<dimitrios.semitsoglou@gmail.com> wrote:
> Fixed a few coding style errors and an if-statement responsible for a
> sanity check inside the prep_ao_dma function.

Please submit two patches for this.
Don't mix coding style and logic fixes.

-- 
Thanks,
//richard

[PATCH 1/2] staging: comedi: cb_pcidas64: Fixed coding style errors

[Dimitrios Semitsoglou-Tsiapos]

Fixed a few coding style errors in cb_pcidas64.

Signed-off-by: Dimitrios Semitsoglou-Tsiapos <dimitrios.semitsoglou@gmail.com>
---
 drivers/staging/comedi/drivers/cb_pcidas64.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
index 9d0b875..6f7fd99 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -1153,7 +1153,7 @@ static int eeprom_read_insn(struct comedi_device *dev,
 static void check_adc_timing(struct comedi_device *dev, struct comedi_cmd *cmd);
 static unsigned int get_divisor(unsigned int ns, unsigned int flags);
 static void i2c_write(struct comedi_device *dev, unsigned int address,
-		      const uint8_t * data, unsigned int length);
+		      const uint8_t *data, unsigned int length);
 static void caldac_write(struct comedi_device *dev, unsigned int channel,
 			 unsigned int value);
 static int caldac_8800_write(struct comedi_device *dev, unsigned int address,
@@ -1230,7 +1230,7 @@ static unsigned int hw_revision(const struct comedi_device *dev,
 }
 
 static void set_dac_range_bits(struct comedi_device *dev,
-			       volatile uint16_t * bits, unsigned int channel,
+			       volatile uint16_t *bits, unsigned int channel,
 			       unsigned int range)
 {
 	unsigned int code = board(dev)->ao_range_code[range];
@@ -4191,7 +4191,7 @@ static void i2c_stop(struct comedi_device *dev)
 }
 
 static void i2c_write(struct comedi_device *dev, unsigned int address,
-		      const uint8_t * data, unsigned int length)
+		      const uint8_t *data, unsigned int length)
 {
 	unsigned int i;
 	uint8_t bitstream;
-- 
1.7.10.2

[PATCH 2/2] staging: comedi: cb_pcidas64: Fixed an if-statement check

[Dimitrios Semitsoglou-Tsiapos]

Fixed in if-statemnt responsible for a sanity check inside the
prep_ao_dma function.

Signed-off-by: Dimitrios Semitsoglou-Tsiapos <dimitrios.semitsoglou@gmail.com>
---
 drivers/staging/comedi/drivers/cb_pcidas64.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
index 6f7fd99..a2e6f96 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -3392,8 +3392,8 @@ static int prep_ao_dma(struct comedi_device *dev, const struct comedi_cmd *cmd)
 	num_bytes = load_ao_dma_buffer(dev, cmd);
 	if (num_bytes == 0)
 		return -1;
-	if (num_bytes >= DMA_BUFFER_SIZE) ;
-	load_ao_dma(dev, cmd);
+	if (num_bytes >= DMA_BUFFER_SIZE)
+		load_ao_dma(dev, cmd);
 
 	dma_start_sync(dev, 0);
 
-- 
1.7.10.2

Re: [PATCH] staging: comedi: cb_pcidas64: Fixed coding style errors and an if-statement

[Dan Carpenter]

On Mon, May 28, 2012 at 09:28:35PM +0300, Dimitrios Semitsoglou-Tsiapos wrote:
> Fixed a few coding style errors and an if-statement responsible for a
> sanity check inside the prep_ao_dma function.
> 

I the change to the if statement is correct but it needs to be
resent in it's own patch.  We want the fix to be noticeable in the
change log.  Don't mix code changes and cleanups.

regards,
dan carpenter

Re: [PATCH 2/2] staging: comedi: cb_pcidas64: Fixed an if-statement check

[Greg KH]

On Tue, May 29, 2012 at 12:51:45AM +0300, Dimitrios Semitsoglou-Tsiapos wrote:
> Fixed in if-statemnt responsible for a sanity check inside the
> prep_ao_dma function.
> 
> Signed-off-by: Dimitrios Semitsoglou-Tsiapos <dimitrios.semitsoglou@gmail.com>
> ---
>  drivers/staging/comedi/drivers/cb_pcidas64.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
> index 6f7fd99..a2e6f96 100644
> --- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> +++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> @@ -3392,8 +3392,8 @@ static int prep_ao_dma(struct comedi_device *dev, const struct comedi_cmd *cmd)
>  	num_bytes = load_ao_dma_buffer(dev, cmd);
>  	if (num_bytes == 0)
>  		return -1;
> -	if (num_bytes >= DMA_BUFFER_SIZE) ;
> -	load_ao_dma(dev, cmd);
> +	if (num_bytes >= DMA_BUFFER_SIZE)
> +		load_ao_dma(dev, cmd);

Are you sure about this change?  I think someone forgot to include an
error check here, the driver was working with this function always being
called, and now you only do it for the extreme case.

Have you checked it somehow?

thanks,

greg k-h

Re: [PATCH 2/2] staging: comedi: cb_pcidas64: Fixed an if-statement check

[Dimitrios Semitsoglou-Tsiapos]

On Mon, Jun 04, 2012 at 08:43:18PM -0700, Greg KH wrote:
> On Tue, May 29, 2012 at 12:51:45AM +0300, Dimitrios Semitsoglou-Tsiapos wrote:
> > Fixed in if-statemnt responsible for a sanity check inside the
> > prep_ao_dma function.
> > 
> > Signed-off-by: Dimitrios Semitsoglou-Tsiapos <dimitrios.semitsoglou@gmail.com>
> > ---
> >  drivers/staging/comedi/drivers/cb_pcidas64.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
> > index 6f7fd99..a2e6f96 100644
> > --- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> > +++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> > @@ -3392,8 +3392,8 @@ static int prep_ao_dma(struct comedi_device *dev, const struct comedi_cmd *cmd)
> >  	num_bytes = load_ao_dma_buffer(dev, cmd);
> >  	if (num_bytes == 0)
> >  		return -1;
> > -	if (num_bytes >= DMA_BUFFER_SIZE) ;
> > -	load_ao_dma(dev, cmd);
> > +	if (num_bytes >= DMA_BUFFER_SIZE)
> > +		load_ao_dma(dev, cmd);
> 
> Are you sure about this change?  I think someone forgot to include an
> error check here, the driver was working with this function always being
> called, and now you only do it for the extreme case.
> 
> Have you checked it somehow?
> 
> thanks,
> 
> greg k-h
You are correct. I had misread load_ao_dma's loop as a while loop.
I'm afraid I did not have a way to test this and it does seem that it
should *not* be included.

Regards,
Dimitrios Semitsoglou-Tsiapos

Re: [PATCH 2/2] staging: comedi: cb_pcidas64: Fixed an if-statement check

[Dan Carpenter]

On Tue, Jun 05, 2012 at 11:17:51AM +0300, Dimitrios Semitsoglou-Tsiapos wrote:
> On Mon, Jun 04, 2012 at 08:43:18PM -0700, Greg KH wrote:
> > On Tue, May 29, 2012 at 12:51:45AM +0300, Dimitrios Semitsoglou-Tsiapos wrote:
> > > Fixed in if-statemnt responsible for a sanity check inside the
> > > prep_ao_dma function.
> > > 
> > > Signed-off-by: Dimitrios Semitsoglou-Tsiapos <dimitrios.semitsoglou@gmail.com>
> > > ---
> > >  drivers/staging/comedi/drivers/cb_pcidas64.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c b/drivers/staging/comedi/drivers/cb_pcidas64.c
> > > index 6f7fd99..a2e6f96 100644
> > > --- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> > > +++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> > > @@ -3392,8 +3392,8 @@ static int prep_ao_dma(struct comedi_device *dev, const struct comedi_cmd *cmd)
> > >  	num_bytes = load_ao_dma_buffer(dev, cmd);
> > >  	if (num_bytes == 0)
> > >  		return -1;
> > > -	if (num_bytes >= DMA_BUFFER_SIZE) ;
> > > -	load_ao_dma(dev, cmd);
> > > +	if (num_bytes >= DMA_BUFFER_SIZE)
> > > +		load_ao_dma(dev, cmd);
> > 
> > Are you sure about this change?  I think someone forgot to include an
> > error check here, the driver was working with this function always being
> > called, and now you only do it for the extreme case.
> > 
> > Have you checked it somehow?
> > 
> > thanks,
> > 
> > greg k-h
> You are correct. I had misread load_ao_dma's loop as a while loop.
> I'm afraid I did not have a way to test this and it does seem that it
> should *not* be included.

It's hard to say, I think your patch is probably correct.  It sort
of matches the do while loop in load_ao_dma().  But it's best if
someone could test this.

regards,
dan carpenter