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

[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.kluin@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;

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

[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

Am Mittwoch 03 Februar 2010 20:48:05 schrieb Roel Kluin:
> The -EINVAL was overwritten by the si470x_disconnect_check().
> 
> Signed-off-by: Roel Kluin <roel.kluin@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;
> 

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

[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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Cheers,
Mauro

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

[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);

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

[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