* [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
@ 2014-07-06 18:21 Rickard Strandqvist
2014-07-07 10:46 ` Lee Jones
0 siblings, 1 reply; 10+ messages in thread
From: Rickard Strandqvist @ 2014-07-06 18:21 UTC (permalink / raw)
To: Jingoo Han, Bryan Wu
Cc: Rickard Strandqvist, Lee Jones, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
2014-07-06 18:21 [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used Rickard Strandqvist
@ 2014-07-07 10:46 ` Lee Jones
2014-07-07 20:00 ` Rickard Strandqvist
0 siblings, 1 reply; 10+ messages in thread
From: Lee Jones @ 2014-07-07 10:46 UTC (permalink / raw)
To: Rickard Strandqvist
Cc: Jingoo Han, Bryan Wu, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
2014-07-07 10:46 ` Lee Jones
@ 2014-07-07 20:00 ` Rickard Strandqvist
0 siblings, 0 replies; 10+ messages in thread
From: Rickard Strandqvist @ 2014-07-07 20:00 UTC (permalink / raw)
To: Lee Jones
Cc: Jingoo Han, Bryan Wu, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Linux Fbdev development list,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
@ 2014-07-07 20:01 Rickard Strandqvist
2014-07-09 16:30 ` Lee Jones
0 siblings, 1 reply; 10+ messages in thread
From: Rickard Strandqvist @ 2014-07-07 20:01 UTC (permalink / raw)
To: Jingoo Han, Bryan Wu
Cc: Rickard Strandqvist, Lee Jones, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
2014-07-07 20:01 Rickard Strandqvist
@ 2014-07-09 16:30 ` Lee Jones
2014-07-09 22:08 ` Rickard Strandqvist
0 siblings, 1 reply; 10+ messages in thread
From: Lee Jones @ 2014-07-09 16:30 UTC (permalink / raw)
To: Rickard Strandqvist
Cc: Jingoo Han, Bryan Wu, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
2014-07-09 16:30 ` Lee Jones
@ 2014-07-09 22:08 ` Rickard Strandqvist
2014-07-10 2:28 ` Jingoo Han
0 siblings, 1 reply; 10+ messages in thread
From: Rickard Strandqvist @ 2014-07-09 22:08 UTC (permalink / raw)
To: Lee Jones
Cc: Jingoo Han, Bryan Wu, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Linux Fbdev development list,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
2014-07-09 22:08 ` Rickard Strandqvist
@ 2014-07-10 2:28 ` Jingoo Han
2014-07-10 7:40 ` Lee Jones
0 siblings, 1 reply; 10+ messages in thread
From: Jingoo Han @ 2014-07-10 2:28 UTC (permalink / raw)
To: 'Rickard Strandqvist', 'Lee Jones'
Cc: 'Bryan Wu', 'Jean-Christophe Plagniol-Villard',
'Tomi Valkeinen', 'Linux Fbdev development list',
linux-kernel, '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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
2014-07-10 2:28 ` Jingoo Han
@ 2014-07-10 7:40 ` Lee Jones
0 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2014-07-10 7:40 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Rickard Strandqvist', 'Bryan Wu',
'Jean-Christophe Plagniol-Villard',
'Tomi Valkeinen', 'Linux Fbdev development list',
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
@ 2014-07-05 12:12 Rickard Strandqvist
2014-07-07 10:50 ` Lee Jones
0 siblings, 1 reply; 10+ messages in thread
From: Rickard Strandqvist @ 2014-07-05 12:12 UTC (permalink / raw)
To: Jingoo Han, Bryan Wu
Cc: Rickard Strandqvist, Rickard Strandqvist, Lee Jones,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev,
linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used
2014-07-05 12:12 Rickard Strandqvist
@ 2014-07-07 10:50 ` Lee Jones
0 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2014-07-07 10:50 UTC (permalink / raw)
To: Rickard Strandqvist
Cc: Jingoo Han, Bryan Wu, Rickard Strandqvist,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, linux-fbdev,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-07-10 7:40 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-06 18:21 [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used Rickard Strandqvist
2014-07-07 10:46 ` Lee Jones
2014-07-07 20:00 ` Rickard Strandqvist
-- strict thread matches above, loose matches on Subject: below --
2014-07-07 20:01 Rickard Strandqvist
2014-07-09 16:30 ` Lee Jones
2014-07-09 22:08 ` Rickard Strandqvist
2014-07-10 2:28 ` Jingoo Han
2014-07-10 7:40 ` Lee Jones
2014-07-05 12:12 Rickard Strandqvist
2014-07-07 10:50 ` Lee Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox