Re: [PATCH 5/7] staging: ccree: add clock management support

[Dan Carpenter]

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