| Message ID | 1374178858-8683-4-git-send-email-gsi@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 18, 2013 at 10:20:52PM +0200, Gerhard Sittig wrote: > + /* enable clock for the I2C peripheral (non fatal) */ > + clk = of_clk_get_by_name(node, "per"); > + if (!IS_ERR(clk)) { > + clk_prepare_enable(clk); > + clk_put(clk); > + } > + This kind of hacked up approach to the clk API is exactly the thing I really don't like seeing. I don't know what it is... is the clk API somehow difficult to use or what's the problem with doing stuff correctly? 1. Get the clock in your probe function. 2. Prepare it at the appropriate time. 3. Enable it appropriately. (or if you want to combine 2 and 3, use clk_prepare_enable().) 4. Ensure that enables/disables and prepares/unprepares are appropriately balanced. 5. 'put' the clock in your remove function. Certainly do not get-enable-put a clock. You're supposed to hold on to the clock all the time that you're actually using it. Final point - if you want to make it non-fatal, don't play games like: clk = clk_get(whatever); if (IS_ERR(clk)) clk = NULL; ... if (clk) clk_prepare(clk); Do this instead: clk = clk_get(whatever); ... if (!IS_ERR(clk)) clk_prepare(clk); etc. (And on this subject, I'm considering whether to make a change to the clk API where clk_prepare() and clk_enable() return zero when passed an error pointer - this means drivers with optional clocks don't have to burden themselves with these kinds of checks.)
On Thu, Jul 18, 2013 at 21:33 +0100, Russell King - ARM Linux wrote: > > On Thu, Jul 18, 2013 at 10:20:52PM +0200, Gerhard Sittig wrote: > > + /* enable clock for the I2C peripheral (non fatal) */ > > + clk = of_clk_get_by_name(node, "per"); > > + if (!IS_ERR(clk)) { > > + clk_prepare_enable(clk); > > + clk_put(clk); > > + } > > + > > This kind of hacked up approach to the clk API is exactly the thing I > really don't like seeing. I don't know what it is... is the clk API > somehow difficult to use or what's the problem with doing stuff correctly? > > 1. Get the clock in your probe function. > 2. Prepare it at the appropriate time. > 3. Enable it appropriately. (or if you want to combine 2 and 3, use > clk_prepare_enable().) > 4. Ensure that enables/disables and prepares/unprepares are appropriately > balanced. > 5. 'put' the clock in your remove function. > > Certainly do not get-enable-put a clock. You're supposed to hold on to > the clock all the time that you're actually using it. Russel, thank you for the feedback! I agree that your outline of steps to take is simple and should not be a problem to do. I noticed that I kept looking at the wrong spot in the driver, hooked up to some setup routine which lacks a counter part. You made me re-visit the drivers and find better locations to hook up to (probe and remove). Part of my sloppiness with immediate put after enable was caused by put being a NOP at the moment, and the assumption that clocks without references but in the enabled state won't go away. But I saw your recent message as well about how clocks shall get reference counted and keep their provider's module loaded. I've queued an update to the I2C, ethernet, and PCI drivers, which addresses your concerns. I2C and ethernet clock handling will become correctly balanced, while PCI won't since the existing driver already lacks a remove() routine. These changes will be part of v3 of the series. Mark, Greg, v2 of the series already addresses the above concerns for the serial communication with the PSC hardware (UART and SPI). See how 01/24 and 02/24 became much more complex than in v1, yet should result in appropriate clock handling in these drivers. In the very minute I wanted to send out v3, I received feedback on the CAN driver manipulation from Marc. Apparently it suffers from the same problem (my blindness to locate the best spot for resource release), but CAN shall be the last driver in the series which is not acceptable yet. > Final point - if you want to make it non-fatal, don't play games like: > > clk = clk_get(whatever); > if (IS_ERR(clk)) > clk = NULL; > > ... > if (clk) > clk_prepare(clk); > > Do this instead: > > clk = clk_get(whatever); > ... > > if (!IS_ERR(clk)) > clk_prepare(clk); > etc. You saw this in the ethernet driver, right? This "interesting" style of "normalization to NULL" in the setup path was meant to simplify the cleanup path, but that point has become obsolete by now. Your example doesn't reflect that 'clk' was a more complex to access member in the driver's private data. It seems that doing the setup in steps with a local variable, to only store the reference when all steps were successful, cleans up the release code paths. Immediately putting the clock and not having stored the reference when enable failed is one more instruction before jumping to the bailout label, but eliminates the pain of tracking get and enable separately, and needing two labels for disable and put (you cannot disable a clock that wasn't enabled). As mentioned, both the I2C driver you responded to as well as the ethernet driver you reference here got adjusted. v3 will get sent when I could address the remaining CAN driver issue. > (And on this subject, I'm considering whether to make a change to the > clk API where clk_prepare() and clk_enable() return zero when passed > an error pointer - this means drivers with optional clocks don't have > to burden themselves with these kinds of checks.) virtually yours Gerhard Sittig
diff --git a/arch/powerpc/platforms/512x/clock-commonclk.c b/arch/powerpc/platforms/512x/clock-commonclk.c index b8963b7..49c52bc 100644 --- a/arch/powerpc/platforms/512x/clock-commonclk.c +++ b/arch/powerpc/platforms/512x/clock-commonclk.c @@ -692,7 +692,6 @@ static void mpc512x_clk_setup_clock_tree(int busfreq) /* some are not yet acquired by their respective drivers */ clk_prepare_enable(clks[MPC512x_CLK_PSC3_MCLK]);/* serial console */ clk_prepare_enable(clks[MPC512x_CLK_FEC]); /* network, NFS */ - clk_prepare_enable(clks[MPC512x_CLK_I2C]); /* * some have their individual clock subtree with separate clock * items and their individual enable counters, yet share a diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index 7607dc0..13d6822 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -21,6 +21,7 @@ #include <linux/of_i2c.h> #include <linux/slab.h> +#include <linux/clk.h> #include <linux/io.h> #include <linux/fsl_devices.h> #include <linux/i2c.h> @@ -264,11 +265,19 @@ static void mpc_i2c_setup_512x(struct device_node *node, struct mpc_i2c *i2c, u32 clock, u32 prescaler) { + struct clk *clk; struct device_node *node_ctrl; void __iomem *ctrl; const u32 *pval; u32 idx; + /* enable clock for the I2C peripheral (non fatal) */ + clk = of_clk_get_by_name(node, "per"); + if (!IS_ERR(clk)) { + clk_prepare_enable(clk); + clk_put(clk); + } + /* Enable I2C interrupts for mpc5121 */ node_ctrl = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-i2c-ctrl");
make the MPC I2C driver prepare and enable the peripheral clock ('per' for register access) in the MPC512x setup routine, make this clock setup non-fatal to allow for a migration period, remove the pre-enabling hack in the platform's clock driver Signed-off-by: Gerhard Sittig <gsi@denx.de> --- arch/powerpc/platforms/512x/clock-commonclk.c | 1 - drivers/i2c/busses/i2c-mpc.c | 9 +++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)