[PATCH] staging: comedi: drivers: addi-data: hwdrv_apci1500: Change variables that is never used

[PATCH] staging: comedi: drivers: addi-data: hwdrv_apci1500: Change variables that is never used

[Rickard Strandqvist]

Variable ar assigned a value that is never used.
Instead use the struct variable of the same name.

This was found using a static code analysis program called cppcheck

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c |   12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c
index bfa9228..faa71a9 100644
--- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c
+++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c
@@ -467,8 +467,6 @@ static int apci1500_di_write(struct comedi_device *dev,
 			     unsigned int *data)
 {
 	struct apci1500_private *devpriv = dev->private;
-	int i_Event1InterruptStatus = 0, i_Event2InterruptStatus =
-		0, i_RegValue;
 
 	switch (data[0]) {
 	case START:
@@ -499,11 +497,9 @@ static int apci1500_di_write(struct comedi_device *dev,
 					outb(0xF4,
 						devpriv->iobase +
 						APCI1500_Z8536_CONTROL_REGISTER);
-					i_Event1InterruptStatus = 1;
 					outb(APCI1500_RW_PORT_A_SPECIFICATION,
 						devpriv->iobase +
 						APCI1500_Z8536_CONTROL_REGISTER);
-					i_RegValue =
 						inb(devpriv->iobase +
 						APCI1500_Z8536_CONTROL_REGISTER);
 
@@ -549,7 +545,6 @@ static int apci1500_di_write(struct comedi_device *dev,
 					outb(0xD0,
 						devpriv->iobase +
 						APCI1500_Z8536_CONTROL_REGISTER);
-					i_Event2InterruptStatus = 1;
 				} else {
 					dev_warn(dev->class_dev,
 						"Event 2 not initialised\n");
@@ -592,7 +587,6 @@ static int apci1500_di_write(struct comedi_device *dev,
 					outb(0xF4,
 						devpriv->iobase +
 						APCI1500_Z8536_CONTROL_REGISTER);
-					i_Event1InterruptStatus = 0;
 				} else {
 					dev_warn(dev->class_dev,
 						"Event 1 not initialised\n");
@@ -621,7 +615,6 @@ static int apci1500_di_write(struct comedi_device *dev,
 					outb(0xF4,
 						devpriv->iobase +
 						APCI1500_Z8536_CONTROL_REGISTER);
-					i_Event2InterruptStatus = 0;
 				} else {
 
 					dev_warn(dev->class_dev,
@@ -1896,7 +1889,6 @@ static int apci1500_do_bits(struct comedi_device *dev,
 			    unsigned int *data)
 {
 	struct apci1500_private *devpriv = dev->private;
-	unsigned int ui_Status;
 	int i_RegValue;
 	int i_Constant;
 
@@ -2004,8 +1996,8 @@ static int apci1500_do_bits(struct comedi_device *dev,
 
 	/* Enables the PCI interrupt */
 	outl(0x3000, devpriv->i_IobaseAmcc + 0x38);
-	ui_Status = inl(devpriv->i_IobaseAmcc + 0x10);
-	ui_Status = inl(devpriv->i_IobaseAmcc + 0x38);
+	inl(devpriv->i_IobaseAmcc + 0x10);
+	inl(devpriv->i_IobaseAmcc + 0x38);
 	outl(0x23000, devpriv->i_IobaseAmcc + 0x38);
 
 	return insn->n;
-- 
1.7.10.4

Re: [PATCH] staging: comedi: drivers: addi-data: hwdrv_apci1500: Change variables that is never used

[Rickard Strandqvist]

2015-01-28 22:42 GMT+01:00 Rickard Strandqvist
<rickard_strandqvist@spectrumdigital.se>:
> Variable ar assigned a value that is never used.
> Instead use the struct variable of the same name.
>
> This was found using a static code analysis program called cppcheck
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c |   12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c
> index bfa9228..faa71a9 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1500.c
> @@ -467,8 +467,6 @@ static int apci1500_di_write(struct comedi_device *dev,
>                              unsigned int *data)
>  {
>         struct apci1500_private *devpriv = dev->private;
> -       int i_Event1InterruptStatus = 0, i_Event2InterruptStatus =
> -               0, i_RegValue;
>
>         switch (data[0]) {
>         case START:
> @@ -499,11 +497,9 @@ static int apci1500_di_write(struct comedi_device *dev,
>                                         outb(0xF4,
>                                                 devpriv->iobase +
>                                                 APCI1500_Z8536_CONTROL_REGISTER);
> -                                       i_Event1InterruptStatus = 1;
>                                         outb(APCI1500_RW_PORT_A_SPECIFICATION,
>                                                 devpriv->iobase +
>                                                 APCI1500_Z8536_CONTROL_REGISTER);
> -                                       i_RegValue =
>                                                 inb(devpriv->iobase +
>                                                 APCI1500_Z8536_CONTROL_REGISTER);
>
> @@ -549,7 +545,6 @@ static int apci1500_di_write(struct comedi_device *dev,
>                                         outb(0xD0,
>                                                 devpriv->iobase +
>                                                 APCI1500_Z8536_CONTROL_REGISTER);
> -                                       i_Event2InterruptStatus = 1;
>                                 } else {
>                                         dev_warn(dev->class_dev,
>                                                 "Event 2 not initialised\n");
> @@ -592,7 +587,6 @@ static int apci1500_di_write(struct comedi_device *dev,
>                                         outb(0xF4,
>                                                 devpriv->iobase +
>                                                 APCI1500_Z8536_CONTROL_REGISTER);
> -                                       i_Event1InterruptStatus = 0;
>                                 } else {
>                                         dev_warn(dev->class_dev,
>                                                 "Event 1 not initialised\n");
> @@ -621,7 +615,6 @@ static int apci1500_di_write(struct comedi_device *dev,
>                                         outb(0xF4,
>                                                 devpriv->iobase +
>                                                 APCI1500_Z8536_CONTROL_REGISTER);
> -                                       i_Event2InterruptStatus = 0;
>                                 } else {
>
>                                         dev_warn(dev->class_dev,
> @@ -1896,7 +1889,6 @@ static int apci1500_do_bits(struct comedi_device *dev,
>                             unsigned int *data)
>  {
>         struct apci1500_private *devpriv = dev->private;
> -       unsigned int ui_Status;
>         int i_RegValue;
>         int i_Constant;
>
> @@ -2004,8 +1996,8 @@ static int apci1500_do_bits(struct comedi_device *dev,
>
>         /* Enables the PCI interrupt */
>         outl(0x3000, devpriv->i_IobaseAmcc + 0x38);
> -       ui_Status = inl(devpriv->i_IobaseAmcc + 0x10);
> -       ui_Status = inl(devpriv->i_IobaseAmcc + 0x38);
> +       inl(devpriv->i_IobaseAmcc + 0x10);
> +       inl(devpriv->i_IobaseAmcc + 0x38);
>         outl(0x23000, devpriv->i_IobaseAmcc + 0x38);
>
>         return insn->n;
> --
> 1.7.10.4
>



Hi

Sorry,  ignore this!

Wrong commit message, sending a new patch.

Kind regards
Rickard Strandqvist

Re: [PATCH] staging: comedi: drivers: addi-data: hwdrv_apci1500: Change variables that is never used

[Dan Carpenter]

The more I look at these, the more I don't like these patches.  The
original code looks like:

	foo = inb();

There is a 90% chance that we should just delete the whole line, but the
problem is that deleting the whole line is risky because sometimes
reading from the hardware has side effects.  So there is a 10% chance
that we should change to to:

	inb();

Changing it to that is safer, and that's what we are doing, but there is
a 90% chance that it is the wrong thing to do.

When we silence the warnings in the wrong way, then it makes it harder
for anyone to find the warnings and fix it in the correct way.  I also
would like to fix all the static checker warnings in the kernel, but
there is no point in just "silencing the warnings." we should need to
"fix" them.

It's frustrating that we aren't able to fix this code because we don't
have the hardware, but that's the reality.  Let's, please, leave the
warnings as-is until someone with hardware can fix them properly.

regards,
dan carpenter

Re: [PATCH] staging: comedi: drivers: addi-data: hwdrv_apci1500: Change variables that is never used

[Dan Carpenter]

Richard, asked some questions out of band.

I like these patches where they can remove the whole line.  I don't like
them where they leave stray, unneeded function calls.  Or if we know
that we need the function calls then I like those.

Also when it comes to the point where we move this code out of staging
then we can look at these warnings again.  Normally people are good at
fixing up any remaining static checker warnings at the end.  (Except for
binder, obviously.  Binder didn't clean up anything.  It's maintained by
a mailing list which has yet to add itself to MAINTAINERS.  Stupid
binder mailing list).

regards,
dan carpenter

Re: [PATCH] staging: comedi: drivers: addi-data: hwdrv_apci1500: Change variables that is never used

[Greg Kroah-Hartman]

On Fri, Jan 30, 2015 at 11:18:31PM +0300, Dan Carpenter wrote:
> Richard, asked some questions out of band.
> 
> I like these patches where they can remove the whole line.  I don't like
> them where they leave stray, unneeded function calls.  Or if we know
> that we need the function calls then I like those.
> 
> Also when it comes to the point where we move this code out of staging
> then we can look at these warnings again.  Normally people are good at
> fixing up any remaining static checker warnings at the end.  (Except for
> binder, obviously.  Binder didn't clean up anything.  It's maintained by
> a mailing list which has yet to add itself to MAINTAINERS.  Stupid
> binder mailing list).

What remaining issues were in the binder code?  And I have a
non-mailing-list MAINTAINERS entry for binder in linux-next, it will go
to Linus in a few hours.

thanks,

greg k-h

Re: [PATCH] staging: comedi: drivers: addi-data: hwdrv_apci1500: Change variables that is never used

[Dan Carpenter]

On Fri, Jan 30, 2015 at 12:25:53PM -0800, Greg Kroah-Hartman wrote:
> On Fri, Jan 30, 2015 at 11:18:31PM +0300, Dan Carpenter wrote:
> > Richard, asked some questions out of band.
> > 
> > I like these patches where they can remove the whole line.  I don't like
> > them where they leave stray, unneeded function calls.  Or if we know
> > that we need the function calls then I like those.
> > 
> > Also when it comes to the point where we move this code out of staging
> > then we can look at these warnings again.  Normally people are good at
> > fixing up any remaining static checker warnings at the end.  (Except for
> > binder, obviously.  Binder didn't clean up anything.  It's maintained by
> > a mailing list which has yet to add itself to MAINTAINERS.  Stupid
> > binder mailing list).
> 
> What remaining issues were in the binder code?  And I have a
> non-mailing-list MAINTAINERS entry for binder in linux-next, it will go
> to Linus in a few hours.
> 

Oh.  Hm...  I looked at it again and it's not as bad as I remember.  I
appologize.  I did have a question but I'll take it to another thread.

regards,
dan carpenter

Re: [PATCH] staging: comedi: drivers: addi-data: hwdrv_apci1500: Change variables that is never used

[Rickard Strandqvist]

2015-01-30 22:10 GMT+01:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Fri, Jan 30, 2015 at 12:25:53PM -0800, Greg Kroah-Hartman wrote:
>> On Fri, Jan 30, 2015 at 11:18:31PM +0300, Dan Carpenter wrote:
>> > Richard, asked some questions out of band.
>> >

Hi all!

As Dan said, I email him before. But it was not really meant only to
him, wrong click on the answer button on my mobile.
And really a better question to Greg, so here it is again.

Understand what you mean, although in this case it requires someone with
this HW to run a static code checking to.

But all the code in staging has a TODO file, is it not appropriate to
add the comment there then?

BTW all, ther i a PATCH v2 for this.


Kind regards
Rickard Strandqvist