linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@codeaurora.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] clk: add more managed APIs
Date: Sat, 28 Jan 2017 13:44:35 -0800	[thread overview]
Message-ID: <64ed0890-14f6-42ff-66b1-60f7b3d7d02f@roeck-us.net> (raw)
In-Reply-To: <20170128192207.GA38136@dtor-ws>

On 01/28/2017 11:22 AM, Dmitry Torokhov wrote:
> On Sat, Jan 28, 2017 at 07:03:10PM +0000, Russell King - ARM Linux wrote:
>> On Sat, Jan 28, 2017 at 10:40:47AM -0800, Dmitry Torokhov wrote:
>>> When converting a driver to managed resources it is desirable to be able to
>>> manage all resources in the same fashion. This change allows managing
>>> clocks in the same way we manage many other resources.
>>>
>>> This adds the following managed APIs:
>>>
>>> - devm_clk_prepare()/devm_clk_unprepare();
>>> - devm_clk_enable()/devm_clk_disable();
>>> - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
>>
>> Does it make any sense what so ever to have devm_clk_enable() and
>> devm_clk_disable()?
>>
>> Take a moment to think about where you use all of these.  The devm_*
>> functions are there to be used in probe functions so that cleanup
>> paths can be streamlined and less erroneous.  They aren't for general
>> use throughout the driver.
>>
>> Given that, there are two operations that you may wish to do in the
>> probe path:
>>
>> 1. prepare a clock (avoiding the enable because you want to perform
>>    the enable elsewhere in the driver.)
>> 2. prepare and enable a clock
>>
>> So, does having devm_clk_enable() really make sense?  I don't think
>> it does, and I suspect they'll get very little if any use.  So, I
>> think best not add them until someone comes up with a good and
>> wide-spread use case.
>
> That makes sense.
>
> Guenter, I know you are a coccinelle wizard, can you cook a script that
> can find current users of clk_enable() in probe paths? Then we can make
> informed decision on devm_clk_enable.
>

Questionable use:
	drivers/input/keyboard/st-keyscan.c

clk_enable() in probe, clk_disable() in remove:
	drivers/i2c/busses/i2c-tegra.c
	drivers/media/platform/exynos4-is/fimc-core.c
	drivers/media/platform/exynos4-is/mipi-csis.c
	drivers/thermal/spear_thermal.c
	drivers/usb/musb/am35x.c
	drivers/usb/musb/davinci.c

This does not count drivers which call clk_enable() in probe, but disable
the clock at some point, and only re-enable it when needed.
	
Not that many. A quick browse suggests that clk_enable()/clk_disable()
is more commonly used to temporarily enable the clock while needed.

For clk_prepare(), I get 33 hits in drivers/.

patching file drivers/usb/gadget/udc/at91_udc.c
patching file drivers/i2c/busses/i2c-tegra.c
patching file drivers/pwm/pwm-spear.c
patching file drivers/pinctrl/spear/pinctrl-plgpio.c
patching file drivers/input/keyboard/samsung-keypad.c
patching file drivers/crypto/atmel-aes.c
patching file drivers/usb/gadget/udc/pxa27x_udc.c
patching file drivers/iommu/msm_iommu.c
patching file drivers/i2c/busses/i2c-rk3x.c
patching file drivers/tty/serial/pxa.c
patching file drivers/remoteproc/st_remoteproc.c
patching file drivers/pwm/pwm-tiehrpwm.c
patching file drivers/media/platform/mtk-vpu/mtk_vpu.c
patching file drivers/i2c/busses/i2c-meson.c
patching file drivers/crypto/ux500/hash/hash_core.c
patching file drivers/pwm/pwm-vt8500.c
patching file drivers/pwm/pwm-sti.c
patching file drivers/tty/serial/xilinx_uartps.c
patching file drivers/pwm/pwm-atmel.c
patching file drivers/phy/phy-dm816x-usb.c
patching file drivers/input/keyboard/spear-keyboard.c
patching file drivers/gpu/host1x/mipi.c
patching file drivers/crypto/ux500/cryp/cryp_core.c
patching file drivers/spi/spi-armada-3700.c
patching file drivers/pwm/pwm-mtk-disp.c
patching file drivers/nvmem/mxs-ocotp.c
patching file drivers/media/platform/s5p-g2d/g2d.c
patching file drivers/i2c/busses/i2c-sirf.c
patching file drivers/crypto/atmel-sha.c
patching file drivers/tty/serial/8250/8250_pxa.c
patching file drivers/thermal/samsung/exynos_tmu.c
patching file drivers/media/platform/sti/bdisp/bdisp-v4l2.c
patching file drivers/dma/s3c24xx-dma.c

Those drivers call clk_prepare() in the probe function. The list
may not be complete; my script currently only checks for clk_prepare()
in the probe function of platform, i2c, and spi drivers.

I quick glance through the generated diffs suggests that most
if not all of those call clk_prepare() in probe and clk_unprepare()
in remove.

For clk_prepare_enable() in probe functions, I get 288 hits in drivers/.
I didn't check those for validity - there are just too many. I did check
watchdog and input earlier, though. For those, almost all would be
candidates for devm_clk_prepare_enable().

Overall, I think we should have devm_clk_prepare() and most definitely
devm_clk_prepare_enable(). I am not that sure about clk_enable().

Thanks,
Guenter

  reply	other threads:[~2017-01-28 21:44 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-28 18:40 [PATCH] clk: add more managed APIs Dmitry Torokhov
2017-01-28 19:03 ` Russell King - ARM Linux
2017-01-28 19:22   ` Dmitry Torokhov
2017-01-28 21:44     ` Guenter Roeck [this message]
2017-01-28 23:39       ` Russell King - ARM Linux
2017-01-29 16:00         ` Guenter Roeck
2017-01-29 18:07         ` [PATCH v2] " Dmitry Torokhov
2017-01-29 18:31           ` Guenter Roeck
2017-01-30 18:55           ` Stephen Boyd
2017-01-30 19:22             ` Guenter Roeck
2017-01-30 21:42               ` Russell King - ARM Linux
2017-01-30 21:58                 ` Dmitry Torokhov
2017-01-30 22:25                   ` Russell King - ARM Linux
2017-01-30 22:51                 ` Guenter Roeck
2017-01-31  8:43                   ` Geert Uytterhoeven
2017-01-31  0:59               ` Dmitry Torokhov
2017-01-31 17:20                 ` Guenter Roeck
2017-01-31 18:26                   ` Dmitry Torokhov
2017-01-31 19:34                     ` Guenter Roeck
2017-01-31  0:57             ` [PATCH v3] " Dmitry Torokhov
2017-02-07  3:51               ` Dmitry Torokhov
2017-02-14 19:44                 ` Stephen Boyd
2017-02-14 19:55                   ` Dmitry Torokhov
2017-02-14 20:31                     ` Guenter Roeck
2017-02-14 20:01                   ` Russell King - ARM Linux

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=64ed0890-14f6-42ff-66b1-60f7b3d7d02f@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=andy.shevchenko@gmail.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=mturquette@baylibre.com \
    --cc=sboyd@codeaurora.org \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).