Patchwork clk: fix function name in devm_clk_put kernel-doc comment

login
register
mail settings
Submitter Uwe Kleine-König
Date Sept. 23, 2013, 3:45 p.m.
Message ID <1379951159-8294-1-git-send-email-u.kleine-koenig@pengutronix.de>
Download mbox | patch
Permalink /patch/277257/
State New
Headers show

Comments

Uwe Kleine-König - Sept. 23, 2013, 3:45 p.m.
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(-)
Russell King - ARM Linux - Sept. 23, 2013, 4:28 p.m.
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!

Patch

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);