[PATCH v3 3/5] clk: introduce the common clock framework

[Turquette, Mike]

On Mon, Nov 21, 2011 at 5:40 PM, Mike Turquette <mturquette at ti.com> 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