[PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Rickard Strandqvist]

From: Rickard Strandqvist <rickard.strandqvist@sonymobile.com>

Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.

This was found using a static code analysis program called cppcheck

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/video/backlight/jornada720_lcd.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/backlight/jornada720_lcd.c b/drivers/video/backlight/jornada720_lcd.c
index da3876c..b304319 100644
--- a/drivers/video/backlight/jornada720_lcd.c
+++ b/drivers/video/backlight/jornada720_lcd.c
@@ -56,12 +56,10 @@ static int jornada_lcd_get_contrast(struct lcd_device *ld)
 
 static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
 {
-	int ret;
-
 	jornada_ssp_start();
 
 	/* start by sending our set contrast cmd to mcu */
-	ret = jornada_ssp_byte(SETCONTRAST);
+	jornada_ssp_byte(SETCONTRAST);
 
 	/* push the new value */
 	if (jornada_ssp_byte(value) != TXDUMMY) {
-- 
1.7.10.4

[PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Rickard Strandqvist]

Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.

This was found using a static code analysis program called cppcheck

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/video/backlight/jornada720_lcd.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/backlight/jornada720_lcd.c b/drivers/video/backlight/jornada720_lcd.c
index da3876c..b304319 100644
--- a/drivers/video/backlight/jornada720_lcd.c
+++ b/drivers/video/backlight/jornada720_lcd.c
@@ -56,12 +56,10 @@ static int jornada_lcd_get_contrast(struct lcd_device *ld)
 
 static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
 {
-	int ret;
-
 	jornada_ssp_start();
 
 	/* start by sending our set contrast cmd to mcu */
-	ret = jornada_ssp_byte(SETCONTRAST);
+	jornada_ssp_byte(SETCONTRAST);
 
 	/* push the new value */
 	if (jornada_ssp_byte(value) != TXDUMMY) {
-- 
1.7.10.4

Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Lee Jones]

> Variable ar assigned a value that is never used.
> I have also removed all the code that thereby serves no purpose.
> 
> This was found using a static code analysis program called cppcheck
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  drivers/video/backlight/jornada720_lcd.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Solve it the other way around.

Add a check and return the value if an error is returned.

> diff --git a/drivers/video/backlight/jornada720_lcd.c b/drivers/video/backlight/jornada720_lcd.c
> index da3876c..b304319 100644
> --- a/drivers/video/backlight/jornada720_lcd.c
> +++ b/drivers/video/backlight/jornada720_lcd.c
> @@ -56,12 +56,10 @@ static int jornada_lcd_get_contrast(struct lcd_device *ld)
>  
>  static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
>  {
> -	int ret;
> -
>  	jornada_ssp_start();
>  
>  	/* start by sending our set contrast cmd to mcu */
> -	ret = jornada_ssp_byte(SETCONTRAST);
> +	jornada_ssp_byte(SETCONTRAST);
>  
>  	/* push the new value */
>  	if (jornada_ssp_byte(value) != TXDUMMY) {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Rickard Strandqvist]

2014-07-07 12:46 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
>> Variable ar assigned a value that is never used.
>> I have also removed all the code that thereby serves no purpose.
>>
>> This was found using a static code analysis program called cppcheck
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>>  drivers/video/backlight/jornada720_lcd.c |    4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> Solve it the other way around.
>
> Add a check and return the value if an error is returned.

Hi

Ok, I take the opportunity to make a bigger change to clarify the
similarities in function.
Hope you like it, otherwise I can make the minor change instead.

Kind regards
Rickard Strandqvist

Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Lee Jones]

On Sat, 05 Jul 2014, Rickard Strandqvist wrote:

> From: Rickard Strandqvist <rickard.strandqvist@sonymobile.com>
> 
> Variable ar assigned a value that is never used.
> I have also removed all the code that thereby serves no purpose.
> 
> This was found using a static code analysis program called cppcheck
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  drivers/video/backlight/jornada720_lcd.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Is this different from the patch I just NACKed?

> diff --git a/drivers/video/backlight/jornada720_lcd.c b/drivers/video/backlight/jornada720_lcd.c
> index da3876c..b304319 100644
> --- a/drivers/video/backlight/jornada720_lcd.c
> +++ b/drivers/video/backlight/jornada720_lcd.c
> @@ -56,12 +56,10 @@ static int jornada_lcd_get_contrast(struct lcd_device *ld)
>  
>  static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
>  {
> -	int ret;
> -
>  	jornada_ssp_start();
>  
>  	/* start by sending our set contrast cmd to mcu */
> -	ret = jornada_ssp_byte(SETCONTRAST);
> +	jornada_ssp_byte(SETCONTRAST);
>  
>  	/* push the new value */
>  	if (jornada_ssp_byte(value) != TXDUMMY) {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Rickard Strandqvist]

Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose,
and made same change to clarify the similarities in function.

This was found using a static code analysis program called cppcheck

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/video/backlight/jornada720_lcd.c |   31 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/video/backlight/jornada720_lcd.c b/drivers/video/backlight/jornada720_lcd.c
index da3876c..d16abcf 100644
--- a/drivers/video/backlight/jornada720_lcd.c
+++ b/drivers/video/backlight/jornada720_lcd.c
@@ -36,44 +36,41 @@ static int jornada_lcd_get_power(struct lcd_device *ld)
 
 static int jornada_lcd_get_contrast(struct lcd_device *ld)
 {
-	int ret;
+	int ret = -ETIMEDOUT;
 
 	if (jornada_lcd_get_power(ld) != FB_BLANK_UNBLANK)
 		return 0;
 
 	jornada_ssp_start();
 
-	if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY) {
+	if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY)
 		dev_err(&ld->dev, "get contrast failed\n");
-		jornada_ssp_end();
-		return -ETIMEDOUT;
-	} else {
+	else
 		ret = jornada_ssp_byte(TXDUMMY);
-		jornada_ssp_end();
-		return ret;
-	}
+
+	jornada_ssp_end();
+
+	return ret;
 }
 
 static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
 {
-	int ret;
+	int ret = -ETIMEDOUT;
 
 	jornada_ssp_start();
 
 	/* start by sending our set contrast cmd to mcu */
-	ret = jornada_ssp_byte(SETCONTRAST);
-
+	if (jornada_ssp_byte(SETCONTRAST) != TXDUMMY)
+		dev_err(&ld->dev, "set contrast failed\n");
 	/* push the new value */
-	if (jornada_ssp_byte(value) != TXDUMMY) {
+	else if (jornada_ssp_byte(value) != TXDUMMY)
 		dev_err(&ld->dev, "set contrast failed\n");
-		jornada_ssp_end();
-		return -ETIMEDOUT;
-	}
+	else /* if we get here we can assume everything went well */
+		ret = 0;
 
-	/* if we get here we can assume everything went well */
 	jornada_ssp_end();
 
-	return 0;
+	return ret;
 }
 
 static int jornada_lcd_set_power(struct lcd_device *ld, int power)
-- 
1.7.10.4

Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Lee Jones]

On Mon, 07 Jul 2014, Rickard Strandqvist wrote:

> Variable ar assigned a value that is never used.
> I have also removed all the code that thereby serves no purpose,
> and made same change to clarify the similarities in function.
> 
> This was found using a static code analysis program called cppcheck
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  drivers/video/backlight/jornada720_lcd.c |   31 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/video/backlight/jornada720_lcd.c b/drivers/video/backlight/jornada720_lcd.c
> index da3876c..d16abcf 100644
> --- a/drivers/video/backlight/jornada720_lcd.c
> +++ b/drivers/video/backlight/jornada720_lcd.c
> @@ -36,44 +36,41 @@ static int jornada_lcd_get_power(struct lcd_device *ld)
>  
>  static int jornada_lcd_get_contrast(struct lcd_device *ld)
>  {
> -	int ret;
> +	int ret = -ETIMEDOUT;
>  
>  	if (jornada_lcd_get_power(ld) != FB_BLANK_UNBLANK)
>  		return 0;
>  
>  	jornada_ssp_start();
>  
> -	if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY) {
> +	if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY)
>  		dev_err(&ld->dev, "get contrast failed\n");
> -		jornada_ssp_end();
> -		return -ETIMEDOUT;
> -	} else {
> +	else
>  		ret = jornada_ssp_byte(TXDUMMY);
> -		jornada_ssp_end();
> -		return ret;
> -	}
> +
> +	jornada_ssp_end();
> +
> +	return ret;
>  }
>  
>  static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
>  {
> -	int ret;
> +	int ret = -ETIMEDOUT;
>  
>  	jornada_ssp_start();
>  
>  	/* start by sending our set contrast cmd to mcu */
> -	ret = jornada_ssp_byte(SETCONTRAST);
> -
> +	if (jornada_ssp_byte(SETCONTRAST) != TXDUMMY)
> +		dev_err(&ld->dev, "set contrast failed\n");
>  	/* push the new value */
> -	if (jornada_ssp_byte(value) != TXDUMMY) {
> +	else if (jornada_ssp_byte(value) != TXDUMMY)
>  		dev_err(&ld->dev, "set contrast failed\n");
> -		jornada_ssp_end();
> -		return -ETIMEDOUT;
> -	}
> +	else /* if we get here we can assume everything went well */
> +		ret = 0;
>  
> -	/* if we get here we can assume everything went well */
>  	jornada_ssp_end();
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int jornada_lcd_set_power(struct lcd_device *ld, int power)

Counter suggestion.

What do you think, I think this looks cleaner:

static int jornada_lcd_get_contrast(struct lcd_device *ld)
{
	int ret;

	if (jornada_lcd_get_power(ld) != FB_BLANK_UNBLANK)
		return 0;

	jornada_ssp_start();

	if (jornada_ssp_byte(GETCONTRAST) = TXDUMMY) {
		ret = jornada_ssp_byte(TXDUMMY);
		goto success;
	}

	dev_err(&ld->dev, "failed to set contrast\n");
	ret = -ETIMEDOUT;

success:
	jornada_ssp_end();
	return ret;
}

static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
{
	int ret = 0;

	jornada_ssp_start();

	/* start by sending our set contrast cmd to mcu */
	if (jornada_ssp_byte(SETCONTRAST) = TXDUMMY) {
		/* if successful, push the new value */
		if (jornada_ssp_byte(value) = TXDUMMY)
			goto success;
	}

	dev_err(&ld->dev, "failed to set contrast\n");
	ret = -ETIMEDOUT;

success:
	jornada_ssp_end();
	return ret;
}

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Rickard Strandqvist]

2014-07-09 18:30 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
> On Mon, 07 Jul 2014, Rickard Strandqvist wrote:
>
>> Variable ar assigned a value that is never used.
>> I have also removed all the code that thereby serves no purpose,
>> and made same change to clarify the similarities in function.
>>
>> This was found using a static code analysis program called cppcheck
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>>  drivers/video/backlight/jornada720_lcd.c |   31 ++++++++++++++----------------
>>  1 file changed, 14 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/video/backlight/jornada720_lcd.c b/drivers/video/backlight/jornada720_lcd.c
>> index da3876c..d16abcf 100644
>> --- a/drivers/video/backlight/jornada720_lcd.c
>> +++ b/drivers/video/backlight/jornada720_lcd.c
>> @@ -36,44 +36,41 @@ static int jornada_lcd_get_power(struct lcd_device *ld)
>>
>>  static int jornada_lcd_get_contrast(struct lcd_device *ld)
>>  {
>> -     int ret;
>> +     int ret = -ETIMEDOUT;
>>
>>       if (jornada_lcd_get_power(ld) != FB_BLANK_UNBLANK)
>>               return 0;
>>
>>       jornada_ssp_start();
>>
>> -     if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY) {
>> +     if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY)
>>               dev_err(&ld->dev, "get contrast failed\n");
>> -             jornada_ssp_end();
>> -             return -ETIMEDOUT;
>> -     } else {
>> +     else
>>               ret = jornada_ssp_byte(TXDUMMY);
>> -             jornada_ssp_end();
>> -             return ret;
>> -     }
>> +
>> +     jornada_ssp_end();
>> +
>> +     return ret;
>>  }
>>
>>  static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
>>  {
>> -     int ret;
>> +     int ret = -ETIMEDOUT;
>>
>>       jornada_ssp_start();
>>
>>       /* start by sending our set contrast cmd to mcu */
>> -     ret = jornada_ssp_byte(SETCONTRAST);
>> -
>> +     if (jornada_ssp_byte(SETCONTRAST) != TXDUMMY)
>> +             dev_err(&ld->dev, "set contrast failed\n");
>>       /* push the new value */
>> -     if (jornada_ssp_byte(value) != TXDUMMY) {
>> +     else if (jornada_ssp_byte(value) != TXDUMMY)
>>               dev_err(&ld->dev, "set contrast failed\n");
>> -             jornada_ssp_end();
>> -             return -ETIMEDOUT;
>> -     }
>> +     else /* if we get here we can assume everything went well */
>> +             ret = 0;
>>
>> -     /* if we get here we can assume everything went well */
>>       jornada_ssp_end();
>>
>> -     return 0;
>> +     return ret;
>>  }
>>
>>  static int jornada_lcd_set_power(struct lcd_device *ld, int power)
>
> Counter suggestion.
>
> What do you think, I think this looks cleaner:
>
> static int jornada_lcd_get_contrast(struct lcd_device *ld)
> {
>         int ret;
>
>         if (jornada_lcd_get_power(ld) != FB_BLANK_UNBLANK)
>                 return 0;
>
>         jornada_ssp_start();
>
>         if (jornada_ssp_byte(GETCONTRAST) = TXDUMMY) {
>                 ret = jornada_ssp_byte(TXDUMMY);
>                 goto success;
>         }
>
>         dev_err(&ld->dev, "failed to set contrast\n");
>         ret = -ETIMEDOUT;
>
> success:
>         jornada_ssp_end();
>         return ret;
> }
>
> static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
> {
>         int ret = 0;
>
>         jornada_ssp_start();
>
>         /* start by sending our set contrast cmd to mcu */
>         if (jornada_ssp_byte(SETCONTRAST) = TXDUMMY) {
>                 /* if successful, push the new value */
>                 if (jornada_ssp_byte(value) = TXDUMMY)
>                         goto success;
>         }
>
>         dev_err(&ld->dev, "failed to set contrast\n");
>         ret = -ETIMEDOUT;
>
> success:
>         jornada_ssp_end();
>         return ret;
> }


Hi Lee!

Yes, I agree!
Both are clean without so much repetition of code, which was what annoyd me.
But yours is clearer when things go good or bad, nice 1 ;)

I do not know what I can/may do, but I'll try. Remove what is not ok :-)

Reviewed-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
Suggested-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>


Kind regards
Rickard Strandqvist

Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Jingoo Han]

On Thursday, July 10, 2014 7:08 AM, Rickard Strandqvist wrote:
> 2014-07-09 18:30 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
> > On Mon, 07 Jul 2014, Rickard Strandqvist wrote:
> >
> >> Variable ar assigned a value that is never used.
> >> I have also removed all the code that thereby serves no purpose,
> >> and made same change to clarify the similarities in function.
> >>
> >> This was found using a static code analysis program called cppcheck
> >>
> >> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> >> ---
> >>  drivers/video/backlight/jornada720_lcd.c |   31 ++++++++++++++----------------
> >>  1 file changed, 14 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/video/backlight/jornada720_lcd.c b/drivers/video/backlight/jornada720_lcd.c
> >> index da3876c..d16abcf 100644
> >> --- a/drivers/video/backlight/jornada720_lcd.c
> >> +++ b/drivers/video/backlight/jornada720_lcd.c
> >> @@ -36,44 +36,41 @@ static int jornada_lcd_get_power(struct lcd_device *ld)
> >>
> >>  static int jornada_lcd_get_contrast(struct lcd_device *ld)
> >>  {
> >> -     int ret;
> >> +     int ret = -ETIMEDOUT;
> >>
> >>       if (jornada_lcd_get_power(ld) != FB_BLANK_UNBLANK)
> >>               return 0;
> >>
> >>       jornada_ssp_start();
> >>
> >> -     if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY) {
> >> +     if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY)
> >>               dev_err(&ld->dev, "get contrast failed\n");
> >> -             jornada_ssp_end();
> >> -             return -ETIMEDOUT;
> >> -     } else {
> >> +     else
> >>               ret = jornada_ssp_byte(TXDUMMY);
> >> -             jornada_ssp_end();
> >> -             return ret;
> >> -     }
> >> +
> >> +     jornada_ssp_end();
> >> +
> >> +     return ret;
> >>  }
> >>
> >>  static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
> >>  {
> >> -     int ret;
> >> +     int ret = -ETIMEDOUT;
> >>
> >>       jornada_ssp_start();
> >>
> >>       /* start by sending our set contrast cmd to mcu */
> >> -     ret = jornada_ssp_byte(SETCONTRAST);
> >> -
> >> +     if (jornada_ssp_byte(SETCONTRAST) != TXDUMMY)
> >> +             dev_err(&ld->dev, "set contrast failed\n");
> >>       /* push the new value */
> >> -     if (jornada_ssp_byte(value) != TXDUMMY) {
> >> +     else if (jornada_ssp_byte(value) != TXDUMMY)
> >>               dev_err(&ld->dev, "set contrast failed\n");
> >> -             jornada_ssp_end();
> >> -             return -ETIMEDOUT;
> >> -     }
> >> +     else /* if we get here we can assume everything went well */
> >> +             ret = 0;
> >>
> >> -     /* if we get here we can assume everything went well */
> >>       jornada_ssp_end();
> >>
> >> -     return 0;
> >> +     return ret;
> >>  }
> >>
> >>  static int jornada_lcd_set_power(struct lcd_device *ld, int power)
> >
> > Counter suggestion.
> >
> > What do you think, I think this looks cleaner:
> >
> > static int jornada_lcd_get_contrast(struct lcd_device *ld)
> > {
> >         int ret;
> >
> >         if (jornada_lcd_get_power(ld) != FB_BLANK_UNBLANK)
> >                 return 0;
> >
> >         jornada_ssp_start();
> >
> >         if (jornada_ssp_byte(GETCONTRAST) = TXDUMMY) {
> >                 ret = jornada_ssp_byte(TXDUMMY);
> >                 goto success;
> >         }
> >
> >         dev_err(&ld->dev, "failed to set contrast\n");
> >         ret = -ETIMEDOUT;
> >
> > success:
> >         jornada_ssp_end();
> >         return ret;
> > }
> >
> > static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
> > {
> >         int ret = 0;
> >
> >         jornada_ssp_start();
> >
> >         /* start by sending our set contrast cmd to mcu */
> >         if (jornada_ssp_byte(SETCONTRAST) = TXDUMMY) {
> >                 /* if successful, push the new value */
> >                 if (jornada_ssp_byte(value) = TXDUMMY)
> >                         goto success;
> >         }
> >
> >         dev_err(&ld->dev, "failed to set contrast\n");
> >         ret = -ETIMEDOUT;
> >
> > success:
> >         jornada_ssp_end();
> >         return ret;
> > }
> 
> 
> Hi Lee!
> 
> Yes, I agree!
> Both are clean without so much repetition of code, which was what annoyd me.
> But yours is clearer when things go good or bad, nice 1 ;)

Hi Rickard Strandqvist,

I would liked to prefer Lee Jones's code. It looks cleaner.

> 
> I do not know what I can/may do, but I'll try. Remove what is not ok :-)
> 
> Reviewed-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> Suggested-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>

I suggest the following.

Suggested-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Jingoo Han <jg1.han@samsung.com>
Cc: Bryan Wu <cooloney@gmail.com>

Best regards,
Jingoo Han

> 
> 
> Kind regards
> Rickard Strandqvist

Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used

[Lee Jones]

On Thu, 10 Jul 2014, Jingoo Han wrote:

> On Thursday, July 10, 2014 7:08 AM, Rickard Strandqvist wrote:
> > 2014-07-09 18:30 GMT+02:00 Lee Jones <lee.jones@linaro.org>:
> > > On Mon, 07 Jul 2014, Rickard Strandqvist wrote:
> > >
> > >> Variable ar assigned a value that is never used.
> > >> I have also removed all the code that thereby serves no purpose,
> > >> and made same change to clarify the similarities in function.
> > >>
> > >> This was found using a static code analysis program called cppcheck
> > >>
> > >> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> > >> ---
> > >>  drivers/video/backlight/jornada720_lcd.c |   31 ++++++++++++++----------------
> > >>  1 file changed, 14 insertions(+), 17 deletions(-)
> > >>
> > >> diff --git a/drivers/video/backlight/jornada720_lcd.c b/drivers/video/backlight/jornada720_lcd.c
> > >> index da3876c..d16abcf 100644
> > >> --- a/drivers/video/backlight/jornada720_lcd.c
> > >> +++ b/drivers/video/backlight/jornada720_lcd.c
> > >> @@ -36,44 +36,41 @@ static int jornada_lcd_get_power(struct lcd_device *ld)
> > >>
> > >>  static int jornada_lcd_get_contrast(struct lcd_device *ld)
> > >>  {
> > >> -     int ret;
> > >> +     int ret = -ETIMEDOUT;
> > >>
> > >>       if (jornada_lcd_get_power(ld) != FB_BLANK_UNBLANK)
> > >>               return 0;
> > >>
> > >>       jornada_ssp_start();
> > >>
> > >> -     if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY) {
> > >> +     if (jornada_ssp_byte(GETCONTRAST) != TXDUMMY)
> > >>               dev_err(&ld->dev, "get contrast failed\n");
> > >> -             jornada_ssp_end();
> > >> -             return -ETIMEDOUT;
> > >> -     } else {
> > >> +     else
> > >>               ret = jornada_ssp_byte(TXDUMMY);
> > >> -             jornada_ssp_end();
> > >> -             return ret;
> > >> -     }
> > >> +
> > >> +     jornada_ssp_end();
> > >> +
> > >> +     return ret;
> > >>  }
> > >>
> > >>  static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
> > >>  {
> > >> -     int ret;
> > >> +     int ret = -ETIMEDOUT;
> > >>
> > >>       jornada_ssp_start();
> > >>
> > >>       /* start by sending our set contrast cmd to mcu */
> > >> -     ret = jornada_ssp_byte(SETCONTRAST);
> > >> -
> > >> +     if (jornada_ssp_byte(SETCONTRAST) != TXDUMMY)
> > >> +             dev_err(&ld->dev, "set contrast failed\n");
> > >>       /* push the new value */
> > >> -     if (jornada_ssp_byte(value) != TXDUMMY) {
> > >> +     else if (jornada_ssp_byte(value) != TXDUMMY)
> > >>               dev_err(&ld->dev, "set contrast failed\n");
> > >> -             jornada_ssp_end();
> > >> -             return -ETIMEDOUT;
> > >> -     }
> > >> +     else /* if we get here we can assume everything went well */
> > >> +             ret = 0;
> > >>
> > >> -     /* if we get here we can assume everything went well */
> > >>       jornada_ssp_end();
> > >>
> > >> -     return 0;
> > >> +     return ret;
> > >>  }
> > >>
> > >>  static int jornada_lcd_set_power(struct lcd_device *ld, int power)
> > >
> > > Counter suggestion.
> > >
> > > What do you think, I think this looks cleaner:
> > >
> > > static int jornada_lcd_get_contrast(struct lcd_device *ld)
> > > {
> > >         int ret;
> > >
> > >         if (jornada_lcd_get_power(ld) != FB_BLANK_UNBLANK)
> > >                 return 0;
> > >
> > >         jornada_ssp_start();
> > >
> > >         if (jornada_ssp_byte(GETCONTRAST) = TXDUMMY) {
> > >                 ret = jornada_ssp_byte(TXDUMMY);
> > >                 goto success;
> > >         }
> > >
> > >         dev_err(&ld->dev, "failed to set contrast\n");
> > >         ret = -ETIMEDOUT;
> > >
> > > success:
> > >         jornada_ssp_end();
> > >         return ret;
> > > }
> > >
> > > static int jornada_lcd_set_contrast(struct lcd_device *ld, int value)
> > > {
> > >         int ret = 0;
> > >
> > >         jornada_ssp_start();
> > >
> > >         /* start by sending our set contrast cmd to mcu */
> > >         if (jornada_ssp_byte(SETCONTRAST) = TXDUMMY) {
> > >                 /* if successful, push the new value */
> > >                 if (jornada_ssp_byte(value) = TXDUMMY)
> > >                         goto success;
> > >         }
> > >
> > >         dev_err(&ld->dev, "failed to set contrast\n");
> > >         ret = -ETIMEDOUT;
> > >
> > > success:
> > >         jornada_ssp_end();
> > >         return ret;
> > > }
> > 
> > 
> > Hi Lee!
> > 
> > Yes, I agree!
> > Both are clean without so much repetition of code, which was what annoyd me.
> > But yours is clearer when things go good or bad, nice 1 ;)
> 
> Hi Rickard Strandqvist,
> 
> I would liked to prefer Lee Jones's code. It looks cleaner.
> 
> > 
> > I do not know what I can/may do, but I'll try. Remove what is not ok :-)
> > 
> > Reviewed-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> > Suggested-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> 
> I suggest the following.
> 
> Suggested-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> Cc: Bryan Wu <cooloney@gmail.com>

Very well, I will submit as a patch with Jingoo's Ack and Rickard's
Suggested-by and Reviewed-by.  Is anyone able to test this patch?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog