From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753023AbdFVI6e (ORCPT <rfc822;w@1wt.eu>);
        Thu, 22 Jun 2017 04:58:34 -0400
Received: from aserp1040.oracle.com ([141.146.126.69]:32283 "EHLO
        aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752989AbdFVI6b (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 22 Jun 2017 04:58:31 -0400
Date: Thu, 22 Jun 2017 11:58:15 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Gilad Ben-Yossef <gilad@benyossef.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
        driverdev-devel@linuxdriverproject.org, devel@driverdev.osuosl.org,
        Ofir Drang <ofir.drang@arm.com>
Subject: Re: [PATCH 5/7] staging: ccree: add clock management support
Message-ID: <20170622085815.xaw6dn56t2yjrdm4@mwanda>
References: <1498115276-1601-1-git-send-email-gilad@benyossef.com>
 <1498115276-1601-6-git-send-email-gilad@benyossef.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1498115276-1601-6-git-send-email-gilad@benyossef.com>
User-Agent: NeoMutt/20170113 (1.7.2)
X-Source-IP: userv0022.oracle.com [156.151.31.74]
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Jun 22, 2017 at 10:07:51AM +0300, Gilad Ben-Yossef wrote:
> +int cc_clk_on(struct ssi_drvdata *drvdata)
> +{
> +	int rc = 0;
> +	struct clk *clk = drvdata->clk;
> +
> +	if (IS_ERR(clk))
> +	/* No all devices have a clock associated with CCREE */
> +		goto out;

Ugh...  I hate this.  The "goto out;" here is a waste of time
do-nothing-goto that returns diretly.  It's equivalent to "return 0;".
Is that intended?  Even with the comment, it's not clear...

People think do nothing gotos are a great idea but from reviewing tons
and tons of real life errors, I can assure you that in real life (as
opposed to theory) they don't prevent any future bugs and only introduce
"forgot to set the error code" bugs.

The indenting is messed up and multi-line indents get curly braces.

> +
> +	rc = clk_prepare_enable(clk);
> +	if (rc) {
> +		SSI_LOG_ERR("error enabling clock\n");
> +		clk_disable_unprepare(clk);

Don't unprepare something that hasn't been prepared.

> +	}
> +
> +out:
> +	return rc;
> +}

int cc_clk_on(struct ssi_drvdata *drvdata)
{
	struct clk *clk = drvdata->clk;
	int rc;

	if (IS_ERR(clk)) {
		/* Not all devices have a clock associated with CCREE */
		return 0;
	}

	rc = clk_prepare_enable(clk);
	if (rc)
		return rc;

	return 0;
}

regards,
dan carpenter