* [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show
@ 2013-03-08 19:31 Devendra Naga
2013-03-08 21:01 ` Andrew Morton
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Devendra Naga @ 2013-03-08 19:31 UTC (permalink / raw)
To: linux-fbdev
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, ®_val);
+ adp5520_read(data->master, reg, ®_val);
mutex_unlock(&data->lock);
return sprintf(buf, "%u\n", reg_val);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show
2013-03-08 19:31 [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show Devendra Naga
@ 2013-03-08 21:01 ` Andrew Morton
2013-03-11 0:56 ` Jingoo Han
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Andrew Morton @ 2013-03-08 21:01 UTC (permalink / raw)
To: linux-fbdev
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, ®_val);
> + adp5520_read(data->master, 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, ®_val);
+ ret = adp5520_read(data->master, reg, ®_val);
mutex_unlock(&data->lock);
+ if (ret < 0)
+ return ret;
return sprintf(buf, "%u\n", reg_val);
}
_
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show
2013-03-08 19:31 [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show Devendra Naga
2013-03-08 21:01 ` Andrew Morton
@ 2013-03-11 0:56 ` Jingoo Han
2013-03-11 6:16 ` devendra.aaru
2013-03-11 6:18 ` devendra.aaru
3 siblings, 0 replies; 5+ messages in thread
From: Jingoo Han @ 2013-03-11 0:56 UTC (permalink / raw)
To: linux-fbdev
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, ®_val);
> + adp5520_read(data->master, 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show
2013-03-08 19:31 [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show Devendra Naga
2013-03-08 21:01 ` Andrew Morton
2013-03-11 0:56 ` Jingoo Han
@ 2013-03-11 6:16 ` devendra.aaru
2013-03-11 6:18 ` devendra.aaru
3 siblings, 0 replies; 5+ messages in thread
From: devendra.aaru @ 2013-03-11 6:16 UTC (permalink / raw)
To: linux-fbdev
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, ®_val);
>> + adp5520_read(data->master, 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, ®_val);
> + ret = adp5520_read(data->master, 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 :).
> _
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show
2013-03-08 19:31 [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show Devendra Naga
` (2 preceding siblings ...)
2013-03-11 6:16 ` devendra.aaru
@ 2013-03-11 6:18 ` devendra.aaru
3 siblings, 0 replies; 5+ messages in thread
From: devendra.aaru @ 2013-03-11 6:18 UTC (permalink / raw)
To: linux-fbdev
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, ®_val);
>> + adp5520_read(data->master, 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-03-11 6:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-08 19:31 [PATCH 1/3] video: backlight: adp5520: fix compiler warning in adp5520_show Devendra Naga
2013-03-08 21:01 ` Andrew Morton
2013-03-11 0:56 ` Jingoo Han
2013-03-11 6:16 ` devendra.aaru
2013-03-11 6:18 ` devendra.aaru
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.