| Message ID | 1379952790-13202-1-git-send-email-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 23, 2013 at 06:13:10PM +0200, Uwe Kleine-König wrote: > The nice thing about devm_* is that the driver doesn't need to free the > resources but the driver core takes care about that. This also > simplifies the error path quite a bit and removes the wrong check for a > clock pointer being NULL. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/video/mmp/hw/mmp_ctrl.c | 17 ++--------------- > 1 file changed, 2 insertions(+), 15 deletions(-) > > diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c > index 75dca19..6ac7552 100644 > --- a/drivers/video/mmp/hw/mmp_ctrl.c > +++ b/drivers/video/mmp/hw/mmp_ctrl.c > @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev) > if (IS_ERR(ctrl->clk)) { > dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name); > ret = -ENOENT; > - goto failed_get_clk; > + goto failed; > } > clk_prepare_enable(ctrl->clk); > > @@ -551,21 +551,8 @@ failed_path_init: > path_deinit(path_plat); > } > > - if (ctrl->clk) { > - devm_clk_put(ctrl->dev, ctrl->clk); > - clk_disable_unprepare(ctrl->clk); And this patch also fixes the above: disabling/unpreparing _after_ putting the thing - which was quite silly... :) -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/09/13 19:19, Russell King - ARM Linux wrote: > On Mon, Sep 23, 2013 at 06:13:10PM +0200, Uwe Kleine-König wrote: >> The nice thing about devm_* is that the driver doesn't need to free the >> resources but the driver core takes care about that. This also >> simplifies the error path quite a bit and removes the wrong check for a >> clock pointer being NULL. >> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> --- >> drivers/video/mmp/hw/mmp_ctrl.c | 17 ++--------------- >> 1 file changed, 2 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c >> index 75dca19..6ac7552 100644 >> --- a/drivers/video/mmp/hw/mmp_ctrl.c >> +++ b/drivers/video/mmp/hw/mmp_ctrl.c >> @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev) >> if (IS_ERR(ctrl->clk)) { >> dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name); >> ret = -ENOENT; >> - goto failed_get_clk; >> + goto failed; >> } >> clk_prepare_enable(ctrl->clk); >> >> @@ -551,21 +551,8 @@ failed_path_init: >> path_deinit(path_plat); >> } >> >> - if (ctrl->clk) { >> - devm_clk_put(ctrl->dev, ctrl->clk); >> - clk_disable_unprepare(ctrl->clk); > > And this patch also fixes the above: disabling/unpreparing _after_ putting > the thing - which was quite silly... :) Hmm, I wonder if that causes any issues... I.e. should this patch go for 3.12, or is 3.13 fine? Tomi
On 09/24/2013 03:34 PM, Tomi Valkeinen wrote: > On 23/09/13 19:19, Russell King - ARM Linux wrote: >> On Mon, Sep 23, 2013 at 06:13:10PM +0200, Uwe Kleine-König wrote: >>> The nice thing about devm_* is that the driver doesn't need to free the >>> resources but the driver core takes care about that. This also >>> simplifies the error path quite a bit and removes the wrong check for a >>> clock pointer being NULL. >>> >>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>> --- >>> drivers/video/mmp/hw/mmp_ctrl.c | 17 ++--------------- >>> 1 file changed, 2 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c >>> index 75dca19..6ac7552 100644 >>> --- a/drivers/video/mmp/hw/mmp_ctrl.c >>> +++ b/drivers/video/mmp/hw/mmp_ctrl.c >>> @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev) >>> if (IS_ERR(ctrl->clk)) { >>> dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name); >>> ret = -ENOENT; >>> - goto failed_get_clk; >>> + goto failed; >>> } >>> clk_prepare_enable(ctrl->clk); >>> >>> @@ -551,21 +551,8 @@ failed_path_init: >>> path_deinit(path_plat); >>> } >>> >>> - if (ctrl->clk) { >>> - devm_clk_put(ctrl->dev, ctrl->clk); >>> - clk_disable_unprepare(ctrl->clk); >> >> And this patch also fixes the above: disabling/unpreparing _after_ putting >> the thing - which was quite silly... :) > > Hmm, I wonder if that causes any issues... I.e. should this patch go for > 3.12, or is 3.13 fine? > > Tomi > It would cause oops if probe failed due to some reason - although it would almost never happen so we missed it. Thank you for finding it out.
On 23/09/13 19:13, Uwe Kleine-König wrote: > The nice thing about devm_* is that the driver doesn't need to free the > resources but the driver core takes care about that. This also > simplifies the error path quite a bit and removes the wrong check for a > clock pointer being NULL. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/video/mmp/hw/mmp_ctrl.c | 17 ++--------------- > 1 file changed, 2 insertions(+), 15 deletions(-) > Thanks, queuing for 3.12 fixes. Tomi
diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c index 75dca19..6ac7552 100644 --- a/drivers/video/mmp/hw/mmp_ctrl.c +++ b/drivers/video/mmp/hw/mmp_ctrl.c @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev) if (IS_ERR(ctrl->clk)) { dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name); ret = -ENOENT; - goto failed_get_clk; + goto failed; } clk_prepare_enable(ctrl->clk); @@ -551,21 +551,8 @@ failed_path_init: path_deinit(path_plat); } - if (ctrl->clk) { - devm_clk_put(ctrl->dev, ctrl->clk); - clk_disable_unprepare(ctrl->clk); - } -failed_get_clk: - devm_free_irq(ctrl->dev, ctrl->irq, ctrl); + clk_disable_unprepare(ctrl->clk); failed: - if (ctrl) { - if (ctrl->reg_base) - devm_iounmap(ctrl->dev, ctrl->reg_base); - devm_release_mem_region(ctrl->dev, res->start, - resource_size(res)); - devm_kfree(ctrl->dev, ctrl); - } - dev_err(&pdev->dev, "device init failed\n"); return ret;
The nice thing about devm_* is that the driver doesn't need to free the resources but the driver core takes care about that. This also simplifies the error path quite a bit and removes the wrong check for a clock pointer being NULL. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/video/mmp/hw/mmp_ctrl.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)