On Mon, Nov 21, 2011 at 5:40 PM, Mike Turquette <mturquette@xxxxxx> wrote:
> +/**
> + * clk_get_parent - return the parent of a clk
> + * @clk: the clk whose parent gets returned
> + *
> + * Simply returns clk->parent. It is up to the caller to hold the
> + * prepare_lock, if desired. Returns NULL if clk is NULL.
> + */
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> + if (!clk)
> + return NULL;
> +
> + return clk->parent;
> +}
> +EXPORT_SYMBOL_GPL(clk_get_parent);
While auditing the uses/expectations of the current clk API users, I
see that clk_get_parent is rarely used; it is in fact only called in
11 files throughout the kernel.
I decided to have a little audit and here is what I found:
arch/arm/mach-shmobile/board-ap4evb.c: fsi_ak4642_set_rate
Used within clk_set_rate. Can dereference clk->parent directly and
ideally should propagate up the clk tree using the new recursive
clk_set_rate.
arch/arm/mach-tegra/usb_phy.c: tegra_usb_phy_open
Used within a clk_get_rate. pll_u should ideally have it's own
clk->rate populated, reducing the need for tegra_usb_phy_open to know
details of the clk tree. The logic to pull pll_u's rate from it's
parent belongs to pll_u's .recalc_rate callback.
arch/arm/plat-s5p/clock.c: s5p_spdif_set_rate
Another clk_get_parent call from within a .set_rate callback. Again:
use clk->parent directly and better yet, propagate the rate change up
via the recursive clk_set_rate.
arch/arm/plat-s5p/clock.c: s5p_spdif_get_rate
Another clk_get_parent call from within a .recalc_rate callback.
Again, clk->rate should be populated with parent's rate correctly,
otherwise dereference clk->parent directly without use of
clk_get_parent.
arch/arm/plat-s5p/s5p-time.c: s5p_clockevent_init
This problem would be solved by propagating rate_change requests up
two-levels of parents via the new recursive clk_set_rate. There is
also a clk_set_parent call in here, which has nothing to do with the
clk_get_parent call, but it makes me wonder if we should revisit the
issue of switching parents as part of clk_set_rate:
clk_set_rate(tin_event, pclk->rate / 2);
arch/arm/plat-samsung/pwm.c: pwm_is_tdiv
Used in only two places: as part of pwm_calc_tin which could be
replaced wholesale by a better .round_rate implementation and for a
debug print (who cares).
arch/arm/plat-samsung/pwm.c: pwm_calc_tin
Just a .round_rate implementation. Can dereference clk->parent directly.
arch/arm/plat-samsung/time.c: s3c2410_timer_setup
Same as s5p_clockevent_init above.
drivers/sh/clk/core.c: clk_round_parent
An elaborate .round_rate that should have resulted from recursive
propagation up to clk->parent.
drivers/video/omap2/dss/dss.c:
Every call to clk_get_parent in here is wrapped in clk_get_rate. The
functions that use this are effectively .set_rate, .get_rate and
.recalc_rate doppelgangers. Also a debug print calls clk_get_parent
but who cares.
drivers/video/sh_mobile_hdmi.c:
Used in a .set_rate and .round_rate implementation. No reason not to
deref clk->parent directly.
The above explanations take a little liberty listing certain functions
as .round_rate, .set_rate and .recalc_rate callbacks, but that is
because a lot of the code mentioned could be neatly wrapped up that
way.
Do we really need clk_get_parent? The primary problem with it is
ambiguity in the API: should the caller hold a lock? Is the rate
guaranteed not the change after being called? A top-level clk API
function that can be called within other top-level clk API functions
is inconsitent: most of the time this would incur nested locking.
Also having a top-level API expose the tree structure encourages
platform and driver developers to put clk tree details into their code
as opposed to having clever .round_rate and .set_rate callbacks hide
these details from them with the new recursive clk_set_rate.
Thoughts?
Thanks,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
