[PATCH 2/2] media: staging: atomisp: Removed else branch in function

[PATCH 2/2] media: staging: atomisp: Removed else branch in function

[Leonid Kushnir]

This patch fixes the checkpatch.pl warning :

WARNING: else is not generally useful after a break or return

Expressions under 'else' branch in function 'gc0310_s_power' are
executed whenever the exppression in 'if' is False. Otherwise, return
from function occurs. Therefore, there is no need in 'else', and it has
been removed.

Signed-off-by: Leonid Kushnir <leonf008@gmail.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 6be3ee1d93a5..8201c15b5769 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -874,11 +874,10 @@ static int gc0310_s_power(struct v4l2_subdev *sd, int on)
 
 	if (on == 0)
 		return power_down(sd);
-	else {
-		ret = power_up(sd);
-		if (!ret)
-			return gc0310_init(sd);
-	}
+	ret = power_up(sd);
+	if (!ret)
+		return gc0310_init(sd);
+
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH 2/2] media: staging: atomisp: Removed else branch in function

[Dan Carpenter]

On Tue, Oct 06, 2020 at 10:17:21AM +0200, Leonid Kushnir wrote:
> This patch fixes the checkpatch.pl warning :
> 
> WARNING: else is not generally useful after a break or return
> 
> Expressions under 'else' branch in function 'gc0310_s_power' are
> executed whenever the exppression in 'if' is False. Otherwise, return
> from function occurs. Therefore, there is no need in 'else', and it has
> been removed.
> 
> Signed-off-by: Leonid Kushnir <leonf008@gmail.com>
> ---
>  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index 6be3ee1d93a5..8201c15b5769 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -874,11 +874,10 @@ static int gc0310_s_power(struct v4l2_subdev *sd, int on)
>  
>  	if (on == 0)
>  		return power_down(sd);
> -	else {
> -		ret = power_up(sd);
> -		if (!ret)
> -			return gc0310_init(sd);
> -	}
> +	ret = power_up(sd);
> +	if (!ret)

Flip this check around as well.

> +		return gc0310_init(sd);
> +
>  	return ret;

Code should generally do "error handling" instead of "success handling".
That way the success path is always indented one tab and the error path
is indented two tabs.  I like to say that the call and the error handling
are part of the same thing, but with success handling, it's like
do the call, do more stuff, go back to the error handling from the
earlier call.

Anyway, TLDR, please write it like this:

	if (on == 0)
 		return power_down(sd);

	ret = power_up(sd);
	if (ret)
		return ret;

	return gc0310_init(sd);

regards,
dan carpenter

Re: [PATCH 2/2] media: staging: atomisp: Removed else branch in function

[Joe Perches]

On Tue, 2020-10-06 at 21:04 +0300, Dan Carpenter wrote:
> Code should generally do "error handling" instead of "success handling".

Maybe something to add to coding-style
(in '6} Functions' maybe?)...

> That way the success path is always indented one tab and the error path
> is indented two tabs.  I like to say that the call and the error handling
> are part of the same thing, but with success handling, it's like
> do the call, do more stuff, go back to the error handling from the
> earlier call.
[]
> Anyway, TLDR, please write it like this:
> 
> 	if (on == 0)
>  		return power_down(sd);
> 
> 	ret = power_up(sd);
> 	if (ret)
> 		return ret;
> 
> 	return gc0310_init(sd);

Much nicer, thanks for taking the time to write it.