| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <003001cf9be6$b45cfcd0$1d16f670$%han@samsung.com>
Date: Thu, 10 Jul 2014 11:28:58 +0900
From: Jingoo Han <jg1.han@...sung.com>
To: 'Rickard Strandqvist' <rickard_strandqvist@...ctrumdigital.se>,
'Lee Jones' <lee.jones@...aro.org>
Cc: 'Bryan Wu' <cooloney@...il.com>,
'Jean-Christophe Plagniol-Villard' <plagnioj@...osoft.com>,
'Tomi Valkeinen' <tomi.valkeinen@...com>,
'Linux Fbdev development list' <linux-fbdev@...r.kernel.org>,
linux-kernel@...r.kernel.org, 'Jingoo Han' <jg1.han@...sung.com>
Subject: Re: [PATCH] video: backlight: jornada720_lcd.c: Cleaning up variable
that is never used
On Thursday, July 10, 2014 7:08 AM, Rickard Strandqvist wrote:
> 2014-07-09 18:30 GMT+02:00 Lee Jones <lee.jones@...aro.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@...ctrumdigital.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@...ctrumdigital.se>
> Suggested-by: Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
I suggest the following.
Suggested-by: Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
Signed-off-by: Lee Jones <lee.jones@...aro.org>
Acked-by: Jingoo Han <jg1.han@...sung.com>
Cc: Bryan Wu <cooloney@...il.com>
Best regards,
Jingoo Han
>
>
> Kind regards
> Rickard Strandqvist
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists