Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 27 Jan 2020 10:37:01 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Ajay.Kathat@microchip.com
Cc:     linux-wireless@vger.kernel.org, devel@driverdev.osuosl.org,
        gregkh@linuxfoundation.org, johannes@sipsolutions.net,
        Adham.Abozaeid@microchip.com
Subject: Re: [PATCH 1/2] staging: wilc1000: return zero on success and
 non-zero on function failure
Message-ID: <20200127073701.GP1847@kadam>
References: <20200123182129.4053-1-ajay.kathat@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200123182129.4053-1-ajay.kathat@microchip.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Looks good.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

On Thu, Jan 23, 2020 at 12:50:47PM +0000, Ajay.Kathat@microchip.com wrote:
> @@ -384,19 +378,18 @@ static int wilc_sdio_write_reg(struct wilc *wilc, u32 addr, u32 data)
>  		cmd.address = addr;
>  		cmd.data = data;
>  		ret = wilc_sdio_cmd52(wilc, &cmd);
> -		if (ret) {
> +		if (ret)
>  			dev_err(&func->dev,
>  				"Failed cmd 52, read reg (%08x) ...\n", addr);
> -			goto fail;
> -		}

Please don't resend, but try to avoid this sort of anti-pattern in the
future.  You're treating the last failure condition in the function as
special.  In this case it's particularly difficult to read because we
are far from the bottom of the function, but even if we were right at
the bottom, I would try to avoid it.

I am extreme enough that I would avoid even things like:

	ret = frob();
	if (ret)
		printf("ret failed\n");
	return ret;

Instead I would write:

	ret = frob();
	if (ret) {
		printf("ret failed\n");
		return ret;
	}
	return 0;

It's just nice to have the error path at indent level 2 and the
success path at indent level 1.  And the other thing that I like is the
BIG BOLD "return 0;" at the end of the function.  Some functions return
positive numbers on success so if I see "return result;" then I have to
look back a few lines to see if "result" can be positive.

The other anti-pattern which people often do is success handling
(instead of error handling) for the last error condition in a function.

	ret = one();
	if (ret)
		return ret;
	ret = two();
	if (ret)
		return ret;
	ret = three();
	if (!ret)
		ret = four();
	return ret;

Never never do that.  :P

Anyway, don't resend.  It's just food for thought.

regards,
dan carpenter

>  	} else {
>  		struct sdio_cmd53 cmd;
>  
>  		/**
>  		 *      set the AHB address
>  		 **/
> -		if (!wilc_sdio_set_func0_csa_address(wilc, addr))
> -			goto fail;
> +		ret = wilc_sdio_set_func0_csa_address(wilc, addr);
> +		if (ret)
> +			return ret;
>  
>  		cmd.read_write = 1;
>  		cmd.function = 0;
> @@ -407,18 +400,12 @@ static int wilc_sdio_write_reg(struct wilc *wilc, u32 addr, u32 data)
>  		cmd.buffer = (u8 *)&data;
>  		cmd.block_size = sdio_priv->block_size;
>  		ret = wilc_sdio_cmd53(wilc, &cmd);
> -		if (ret) {
> +		if (ret)
>  			dev_err(&func->dev,
>  				"Failed cmd53, write reg (%08x)...\n", addr);
> -			goto fail;
> -		}
>  	}
>  
> -	return 1;
> -
> -fail:
> -
> -	return 0;
> +	return ret;
>  }