[PATCH] radio-si470x-common: -EINVAL overwritten in si470x_vidioc_s_tuner()

[Roel Kluin]

The -EINVAL was overwritten by the si470x_disconnect_check().

Signed-off-by: Roel Kluin <roel....@gmail.com>
---
Is this needed?

diff --git a/drivers/media/radio/si470x/radio-si470x-common.c b/drivers/media/radio/si470x/radio-si470x-common.c
index 4da0f15..65b4a92 100644
--- a/drivers/media/radio/si470x/radio-si470x-common.c
+++ b/drivers/media/radio/si470x/radio-si470x-common.c
@@ -748,12 +748,13 @@ static int si470x_vidioc_s_tuner(struct file *file, void *priv,
struct v4l2_tuner *tuner)
{
struct si470x_device *radio = video_drvdata(file);
- int retval = -EINVAL;
+ int retval;

/* safety checks */
retval = si470x_disconnect_check(radio);
if (retval)
goto done;
+ retval = -EINVAL;

if (tuner->index != 0)
goto done;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Tobias Lorenz]

Hello Roel,

no, the default value of retval makes no difference to the function.

Retval is set by si470x_disconnect_check and si470x_set_register.
After each call, retval is checked.
There is no need to reset it passed.

The only reason, there is a default value is my static code checker, saying variables should have default values.
Setting it to -EINVAL seems more reasonable to me than setting it 0.
In fact the patch would bring up the warning on setting default values again.

Bye,
Toby

[Mauro Carvalho Chehab]

Tobias Lorenz wrote:
> Hello Roel,
>
> no, the default value of retval makes no difference to the function.
>
> Retval is set by si470x_disconnect_check and si470x_set_register.
> After each call, retval is checked.
> There is no need to reset it passed.
>
> The only reason, there is a default value is my static code checker, saying variables should have default values.
> Setting it to -EINVAL seems more reasonable to me than setting it 0.
> In fact the patch would bring up the warning on setting default values again.

Well, your static code checker is then broken ;)

>> struct si470x_device *radio = video_drvdata(file);
>> - int retval = -EINVAL;
>> + int retval;
>>
>> /* safety checks */
>> retval = si470x_disconnect_check(radio);

You may just do then:

int retval = si470x_disconnect_check(radio);

>> if (retval)
>> goto done;
>> + retval = -EINVAL;
>>
>> if (tuner->index != 0)
>> goto done;
>>
> --

> To unsubscribe from this list: send the line "unsubscribe linux-media" in

> the body of a message to majo...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--

Cheers,
Mauro

[Tobias Lorenz]

Hello Mauro,

> > no, the default value of retval makes no difference to the function.
> >
> > Retval is set by si470x_disconnect_check and si470x_set_register.
> > After each call, retval is checked.
> > There is no need to reset it passed.

> You may just do then:
>
> int retval = si470x_disconnect_check(radio);

In all other set/get functions of v4l2_ioctl_ops in the driver, I just set the default value of retval to 0.
To be identical in si470x_vidioc_s_tuner, I modified the patch to the one below.
I already pushed this and another cosmetic patch into mercurial:

http://linuxtv.org/hg/~tlorenz/v4l-dvb/rev/72a2f38d5956
http://linuxtv.org/hg/~tlorenz/v4l-dvb/rev/3efd5d32a618

Mauro, can you pull them?

Bye,
Tobias

--- a/linux/drivers/media/radio/si470x/radio-si470x-common.c Thu Feb 11 23:11:30 2010 -0200
+++ b/linux/drivers/media/radio/si470x/radio-si470x-common.c Thu Feb 18 20:31:33 2010 +0100
@@ -748,7 +748,7 @@
struct v4l2_tuner *tuner)
{

struct si470x_device *radio = video_drvdata(file);
- int retval = -EINVAL;

+ int retval = 0;

/* safety checks */
retval = si470x_disconnect_check(radio);

[Mauro Carvalho Chehab]

Tobias Lorenz wrote:
> Hello Mauro,
>
>>> no, the default value of retval makes no difference to the function.
>>>
>>> Retval is set by si470x_disconnect_check and si470x_set_register.
>>> After each call, retval is checked.
>>> There is no need to reset it passed.
>
>> You may just do then:
>>
>> int retval = si470x_disconnect_check(radio);
>
> In all other set/get functions of v4l2_ioctl_ops in the driver, I just set the default value of retval to 0.
> To be identical in si470x_vidioc_s_tuner, I modified the patch to the one below.
> I already pushed this and another cosmetic patch into mercurial:
>
> http://linuxtv.org/hg/~tlorenz/v4l-dvb/rev/72a2f38d5956

See comment bellow.

> http://linuxtv.org/hg/~tlorenz/v4l-dvb/rev/3efd5d32a618
Applied.

>
> Mauro, can you pull them?

Tobias, next time or send one patch per email or send me a pull request.

>
> Bye,
> Tobias
>
> --- a/linux/drivers/media/radio/si470x/radio-si470x-common.c Thu Feb 11 23:11:30 2010 -0200
> +++ b/linux/drivers/media/radio/si470x/radio-si470x-common.c Thu Feb 18 20:31:33 2010 +0100
> @@ -748,7 +748,7 @@
> struct v4l2_tuner *tuner)
> {
> struct si470x_device *radio = video_drvdata(file);
> - int retval = -EINVAL;
> + int retval = 0;
>
> /* safety checks */
> retval = si470x_disconnect_check(radio);

This really doesn't make any sense. Just do:

int retval = i470x_disconnect_check(radio);

or
int retval;

retval = i470x_disconnect_check(radio);

--

Cheers,
Mauro