lkml.org 
[lkml]   [2011]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 3/5] clk: introduce the common clock framework
On Mon, Nov 21, 2011 at 5:40 PM, Mike Turquette <mturquette@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2011-12-06 00:43    [W:0.270 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site