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

Rickard Strandqvist
2014-07-09T22:08:23Z

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