Messages in this thread | | | From | Jingoo Han <> | Subject | Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable that is never used | Date | Thu, 10 Jul 2014 11:28:58 +0900 |
| |
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
| |