Re: [PATCH -next] i2c: imx: add missing clk_disable_unprepare() on error path

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

On Sat, Dec 14, 2013 at 10:26:10PM +0800, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
> 
> Add the missing clk_disable_unprepare() before return in
> the error handling cases.
> 
> Signed-off-by: Wei Yongjun <yongjun_wei-zrsr2BFq86L20UzCJQGyNP8+0UxHXcjY@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-imx.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index d0cfbb4..e62f3be 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -328,8 +328,10 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>  	temp |= I2CR_MSTA;
>  	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>  	result = i2c_imx_bus_busy(i2c_imx, 1);
> -	if (result)
> +	if (result) {
> +		clk_disable_unprepare(i2c_imx->clk);
>  		return result;
> +	}
>  	i2c_imx->stopped = 0;

Nice catch, but that is not enough. The logic between i2x_imx_start and
*_stop is still broken. *_start may return with clocks enabled or not
while stop will unconditionally disables them.


>  
>  	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
> @@ -654,7 +656,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  				pdev->name, i2c_imx);
>  	if (ret) {
>  		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
> -		return ret;
> +		goto err_out;
>  	}
>  
>  	/* Init queue */
> @@ -680,7 +682,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "registration failed\n");
> -		return ret;
> +		goto err_out;
>  	}
>  
>  	/* Set up platform driver data */
> @@ -697,6 +699,9 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
>  
>  	return 0;   /* Return OK */
> +err_out:
> +	clk_disable_unprepare(i2c_imx->clk);
> +	return ret;

Why not just enabling the clocks around the small codeblock that needs
them?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]