[PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show

[PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show

[Devendra Naga]

while compiling with make W=1 (gcc gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8))

found the following warning

drivers/video/backlight/adp5520_bl.c: In function ‘adp5520_show’:
drivers/video/backlight/adp5520_bl.c:146:6: warning: variable ‘error’ set but not used [-Wunused-but-set-variable]

fixed by removing the variable

Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Michael Hennerich <michael.hennerich@analog.com>
Cc: Richard Purdie <rpurdie@rpsys.net>

Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
---
 drivers/video/backlight/adp5520_bl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/video/backlight/adp5520_bl.c b/drivers/video/backlight/adp5520_bl.c
index a1e41d4..9f8b20b 100644
--- a/drivers/video/backlight/adp5520_bl.c
+++ b/drivers/video/backlight/adp5520_bl.c
@@ -143,11 +143,10 @@ static int adp5520_bl_setup(struct backlight_device *bl)
 static ssize_t adp5520_show(struct device *dev, char *buf, int reg)
 {
 	struct adp5520_bl *data = dev_get_drvdata(dev);
-	int error;
 	uint8_t reg_val;
 
 	mutex_lock(&data->lock);
-	error = adp5520_read(data->master, reg, &reg_val);
+	adp5520_read(data->master, reg, &reg_val);
 	mutex_unlock(&data->lock);
 
 	return sprintf(buf, "%u\n", reg_val);
-- 
1.8.1.2

Re: [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show

[Andrew Morton]

On Fri,  8 Mar 2013 14:31:07 -0500 Devendra Naga <devendra.aaru@gmail.com> wrote:

> while compiling with make W=1 (gcc gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8))
> 
> found the following warning
> 
> drivers/video/backlight/adp5520_bl.c: In function ___adp5520_show___:
> drivers/video/backlight/adp5520_bl.c:146:6: warning: variable ___error___ set but not used [-Wunused-but-set-variable]
> 
> fixed by removing the variable
> 
> ...
>
> --- a/drivers/video/backlight/adp5520_bl.c
> +++ b/drivers/video/backlight/adp5520_bl.c
> @@ -143,11 +143,10 @@ static int adp5520_bl_setup(struct backlight_device *bl)
>  static ssize_t adp5520_show(struct device *dev, char *buf, int reg)
>  {
>  	struct adp5520_bl *data = dev_get_drvdata(dev);
> -	int error;
>  	uint8_t reg_val;
>  
>  	mutex_lock(&data->lock);
> -	error = adp5520_read(data->master, reg, &reg_val);
> +	adp5520_read(data->master, reg, &reg_val);
>  	mutex_unlock(&data->lock);
>  
>  	return sprintf(buf, "%u\n", reg_val);

We shouldn't just ignore the error; with the code as it stands, a
adp5520_read() failure will result in the kernel displaying
uninitialised garbage.

So it would be better to propagate the adp5520_read() return value back
to the caller if it's negative.


(This assumes that the i2c layer returns a sane return value - if it
does, that would make i2c pretty unique :( We could get paranoid and
return a hard-wired -EIO, but it would be bad of us to overwrite things
like -ENOMEM).

So I'd suggest this:

--- a/drivers/video/backlight/adp5520_bl.c~video-backlight-adp5520-fix-compiler-warning-in-adp5520_show
+++ a/drivers/video/backlight/adp5520_bl.c
@@ -143,13 +143,15 @@ static int adp5520_bl_setup(struct backl
 static ssize_t adp5520_show(struct device *dev, char *buf, int reg)
 {
 	struct adp5520_bl *data = dev_get_drvdata(dev);
-	int error;
+	int ret;
 	uint8_t reg_val;
 
 	mutex_lock(&data->lock);
-	error = adp5520_read(data->master, reg, &reg_val);
+	ret = adp5520_read(data->master, reg, &reg_val);
 	mutex_unlock(&data->lock);
 
+	if (ret < 0)
+		return ret;
 	return sprintf(buf, "%u\n", reg_val);
 }
 
_

Re: [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show

[Jingoo Han]

On Saturday, March 09, 2013 4:31 AM, Devendra Naga wrote:
> 
> while compiling with make W=1 (gcc gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8))
> 
> found the following warning
> 
> drivers/video/backlight/adp5520_bl.c: In function ‘adp5520_show’:
> drivers/video/backlight/adp5520_bl.c:146:6: warning: variable ‘error’ set but not used [-Wunused-but-
> set-variable]
> 
> fixed by removing the variable
> 
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Michael Hennerich <michael.hennerich@analog.com>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> 
> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
> ---
>  drivers/video/backlight/adp5520_bl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/adp5520_bl.c b/drivers/video/backlight/adp5520_bl.c
> index a1e41d4..9f8b20b 100644
> --- a/drivers/video/backlight/adp5520_bl.c
> +++ b/drivers/video/backlight/adp5520_bl.c
> @@ -143,11 +143,10 @@ static int adp5520_bl_setup(struct backlight_device *bl)
>  static ssize_t adp5520_show(struct device *dev, char *buf, int reg)
>  {
>  	struct adp5520_bl *data = dev_get_drvdata(dev);
> -	int error;
>  	uint8_t reg_val;
> 
>  	mutex_lock(&data->lock);
> -	error = adp5520_read(data->master, reg, &reg_val);
> +	adp5520_read(data->master, reg, &reg_val);
>  	mutex_unlock(&data->lock);

Hi Devendra Naga,

I also agree with Andrew Morton's opinion.
It would be better to check return value from I2C read/write functions.

Best regards,
Jingoo Han

> 
>  	return sprintf(buf, "%u\n", reg_val);
> --
> 1.8.1.2

Re: [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show

[devendra.aaru]

On Fri, Mar 8, 2013 at 4:01 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri,  8 Mar 2013 14:31:07 -0500 Devendra Naga <devendra.aaru@gmail.com> wrote:
>
>> while compiling with make W=1 (gcc gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8))
>>
>> found the following warning
>>
>> drivers/video/backlight/adp5520_bl.c: In function ___adp5520_show___:
>> drivers/video/backlight/adp5520_bl.c:146:6: warning: variable ___error___ set but not used [-Wunused-but-set-variable]
>>
>> fixed by removing the variable
>>
>> ...
>>
>> --- a/drivers/video/backlight/adp5520_bl.c
>> +++ b/drivers/video/backlight/adp5520_bl.c
>> @@ -143,11 +143,10 @@ static int adp5520_bl_setup(struct backlight_device *bl)
>>  static ssize_t adp5520_show(struct device *dev, char *buf, int reg)
>>  {
>>       struct adp5520_bl *data = dev_get_drvdata(dev);
>> -     int error;
>>       uint8_t reg_val;
>>
>>       mutex_lock(&data->lock);
>> -     error = adp5520_read(data->master, reg, &reg_val);
>> +     adp5520_read(data->master, reg, &reg_val);
>>       mutex_unlock(&data->lock);
>>
>>       return sprintf(buf, "%u\n", reg_val);
>
> We shouldn't just ignore the error; with the code as it stands, a
> adp5520_read() failure will result in the kernel displaying
> uninitialised garbage.
>
> So it would be better to propagate the adp5520_read() return value back
> to the caller if it's negative.
>
>
> (This assumes that the i2c layer returns a sane return value - if it
> does, that would make i2c pretty unique :( We could get paranoid and
> return a hard-wired -EIO, but it would be bad of us to overwrite things
> like -ENOMEM).
>
> So I'd suggest this:
>
> --- a/drivers/video/backlight/adp5520_bl.c~video-backlight-adp5520-fix-compiler-warning-in-adp5520_show
> +++ a/drivers/video/backlight/adp5520_bl.c
> @@ -143,13 +143,15 @@ static int adp5520_bl_setup(struct backl
>  static ssize_t adp5520_show(struct device *dev, char *buf, int reg)
>  {
>         struct adp5520_bl *data = dev_get_drvdata(dev);
> -       int error;
> +       int ret;
>         uint8_t reg_val;
>
>         mutex_lock(&data->lock);
> -       error = adp5520_read(data->master, reg, &reg_val);
> +       ret = adp5520_read(data->master, reg, &reg_val);
>         mutex_unlock(&data->lock);
>
> +       if (ret < 0)
> +               return ret;
>         return sprintf(buf, "%u\n", reg_val);
>  }
>

Thanks for the suggestion, i will do the same and i will send you a
patch sooner.

or you can merge your change with my Acked By too :).

> _
>

Re: [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show

[devendra.aaru]

On Sun, Mar 10, 2013 at 8:56 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> On Saturday, March 09, 2013 4:31 AM, Devendra Naga wrote:
>>
>> while compiling with make W=1 (gcc gcc (GCC) 4.7.2 20121109 (Red Hat 4.7.2-8))
>>
>> found the following warning
>>
>> drivers/video/backlight/adp5520_bl.c: In function ‘adp5520_show’:
>> drivers/video/backlight/adp5520_bl.c:146:6: warning: variable ‘error’ set but not used [-Wunused-but-
>> set-variable]
>>
>> fixed by removing the variable
>>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Michael Hennerich <michael.hennerich@analog.com>
>> Cc: Richard Purdie <rpurdie@rpsys.net>
>>
>> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
>> ---
>>  drivers/video/backlight/adp5520_bl.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/backlight/adp5520_bl.c b/drivers/video/backlight/adp5520_bl.c
>> index a1e41d4..9f8b20b 100644
>> --- a/drivers/video/backlight/adp5520_bl.c
>> +++ b/drivers/video/backlight/adp5520_bl.c
>> @@ -143,11 +143,10 @@ static int adp5520_bl_setup(struct backlight_device *bl)
>>  static ssize_t adp5520_show(struct device *dev, char *buf, int reg)
>>  {
>>       struct adp5520_bl *data = dev_get_drvdata(dev);
>> -     int error;
>>       uint8_t reg_val;
>>
>>       mutex_lock(&data->lock);
>> -     error = adp5520_read(data->master, reg, &reg_val);
>> +     adp5520_read(data->master, reg, &reg_val);
>>       mutex_unlock(&data->lock);
>
> Hi Devendra Naga,
>
> I also agree with Andrew Morton's opinion.
> It would be better to check return value from I2C read/write functions.
>

thanks, i will do and send a patch sooner or Andrew can merge his
patch with my Acked By.


> Best regards,
> Jingoo Han
>
>>
>>       return sprintf(buf, "%u\n", reg_val);
>> --
>> 1.8.1.2
>