On Fri, Mar 23, 2012 at 3:12 PM, Saravana Kannan skannan@codeaurora.org wrote:
On 03/23/2012 02:39 PM, Turquette, Mike wrote:
__clk_recalc_rates is called by __clk_reparent which is called by clk_set_parent. __clk_recalc_rates is also called by clk_set_rate.
Does this not handle the old cached clk->rate for you?
For the set_parent case, ops->recalc_rate() is called twice. Once for PRE_CHANGE and once for POST_CHANGE. For this clock, I can only really recalc the rate during the POST_CHANGE call. So, how should I differentiate the two cases?
.recalc_rate serves two purposes: first it recalculates the rate after the rate has changed and you pass in a new parent_rate argument. The second purpose is to "speculate" a rate change. You can pass in any rate for the parent_rate parameter when you call .recalc_rates. This is what __speculate_rates does before the rate changes. For clk_set_parent we call,
__clk_speculate_rates(clk, parent->rate);
Where parent above is the *new* parent. So this will let us propagate pre-rate change notifiers with the new rate.
Your .recalc_rate callback doesn't need to differentiate between the two calls to .recalc_rate. It should just take in the parent_rate value and do the calculation required of it.
Take a look at __clk_speculate_rates and __clk_recalc_rates and you'll see how to use it.
On a separate note: Sorry if I missed any earlier discussion on this, but what's the reason for calling recalc_rate() pre-change and post-change but without giving it the ability to differentiate between the two?
I think my answer above covers this. The .recalc_rate callback only exists to perform a hardware-specific calculation. The context of pre- or post-change is irrelevant since the parent_rate passed in could be the actual rate of the parent or a future rate of the parent.
I think it's quite useful for recalc_rate to be called pre/post change (some steps have to be done pre/post change depending on whether the parent rate is increasing or decreasing). But I don't see the "msg" being passed along.
What kind of steps? Does your .recalc_rate perform these steps? I need more details to understand your requirements.
Also, I noticed that clk_set_parent() is treating a NULL as an invalid clock. Should that be fixed? set_parent(NULL) could be treated as a grounding the clock. Should we let the ops->set_parent determine if NULL is valid option?
We must be looking at different code. clk_set_parent doesn't return any error if parent == NULL. Bringing this to my attention does show that we do deref the parent pointer without a check though...
Do you have a real use case for this? Due to the way that we match the parent pointer with the cached clk->parents member it would be painful to support NULL parents as valid.
It is also worth considering whether clk_set_parent is really the correct operation for grounding a clock. clk_unprepare might be a better candidate.
Regards, Mike