Message ID | CAPgLHd-zvCacvi5gN-PSqow5-NujhwtXa2RcnNQvorMmAEucrw@mail.gmail.com |
---|---|
State | Changes Requested |
Headers | show |
On Sat, Dec 14, 2013 at 10:26:10PM +0800, Wei Yongjun wrote: > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > Add the missing clk_disable_unprepare() before return in > the error handling cases. > > Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > --- > 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?
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; 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; } static int i2c_imx_remove(struct platform_device *pdev)