| Message ID | 1379951159-8294-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 23, 2013 at 05:45:59PM +0200, Uwe Kleine-König wrote: > The paragraph about clk_put already specifies this restriction about > clk_put. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > I suspect that devm_clk_put is used more often than necessary. After some > grepping around e.g. looking at drivers/tty/serial/clps711x.c it doesn't > seem necessary to call it as the driver core already cares about calling > the devm cleanup callbacks. The one(s) you've left are in drivers/media/platform/marvell-ccic/mmp-driver.c: static void mcam_deinit_clk(struct mcam_camera *mcam) { unsigned int i; for (i = 0; i < NR_MCAM_CLK; i++) { if (!IS_ERR(mcam->clk[i])) { if (mcam->clk[i]) devm_clk_put(mcam->dev, mcam->clk[i]); } mcam->clk[i] = NULL; } } static int mmpcam_probe(struct platform_device *pdev) { cam = devm_kzalloc(&pdev->dev, sizeof(*cam), GFP_KERNEL); ... mcam = &cam->mcam; ... mcam->regs = devm_ioremap_resource(&pdev->dev, res); ... cam->power_regs = devm_ioremap_resource(&pdev->dev, res); ... out_unregister: mccic_shutdown(mcam); out_power_down: mmpcam_power_down(mcam); out_deinit_clk: mcam_deinit_clk(mcam); return ret; } static int mmpcam_remove(struct mmp_camera *cam) { ... mcam_deinit_clk(mcam); iounmap(cam->power_regs); iounmap(mcam->regs); kfree(cam); return 0; } So, mcam_deinit_clk() provides nothing useful and should be killed. Second thing to spot from the above is that kfree() is broken. As are those iounmap()s. The more I look at this, the more I find wrong. gpio_free() too. Quite honestly, I think this driver is quite broken as it stands - anything you do to it (even patching it without build testing) is IMHO likely to improve this driver!
diff --git a/include/linux/clk.h b/include/linux/clk.h index 9a6d045..c713ba0 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -214,7 +214,7 @@ void clk_put(struct clk *clk); * clock source are balanced by clk_disable calls prior to calling * this function. * - * clk_put should not be called from within interrupt context. + * devm_clk_put should not be called from within interrupt context. */ void devm_clk_put(struct device *dev, struct clk *clk);
The paragraph about clk_put already specifies this restriction about clk_put. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, I suspect that devm_clk_put is used more often than necessary. After some grepping around e.g. looking at drivers/tty/serial/clps711x.c it doesn't seem necessary to call it as the driver core already cares about calling the devm cleanup callbacks. The only reason I see being valid to call devm_clk_put is that a device continues to be bound and still it's sure that the clk in question isn't needed anymore. Ah, and maybe to enforce an order during cleanup, but then there is no reason to use devm, the normal functions would just do fine. I'm replying to this mail with a few patches. Best regards Uwe include/linux/clk.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)