Aw: Re: [PATCH v2] clk: si5351: fix .round_rate for multisynth 6-7

From: S. S.
Date: Wed Apr 08 2015 - 10:00:26 EST

>> @@ -645,7 +646,7 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
>> struct si5351_hw_data *hwdata =
>> container_of(hw, struct si5351_hw_data, hw);
>> unsigned long long lltmp;
>> - unsigned long a, b, c;
>> + unsigned long a, b = 0, c = 1;
> Actually, moving b,c initialization up here is neither related
> to the patch itself nor is it mentioned in the commit log.
> Please do not mix different patches.

I agree. I just thought we could avoid repeating the b,c initialization
in the code, but You are right, that would be a different patch, if any
at all.

>> *parent_rate = a * rate;
>> + if (rfrac)
>> + rational_best_approximation(rfrac, denom,
>> + &b, &c);
>> + }
> I am still not convinced that this is the right way to calculate the
> _best_ integer divider for ms6,7.
> The code above is written to (a) find the _largest_ integer "a" that
> will match
> a * rate <= parent_rate
> and then (b) determines the fractional part of the divider.
> As you correctly stated, ms6,7 do not support (b) but if we use (a) for
> those msynths, we will determine an "a" so that "a * rate" is always
> smaller than parent_rate.
> What we actually want is the smallest error between generated and
> requested rate, so we have to find the closest integer with
> a = DIV_ROUND_CLOSEST(parent_rate, rate)

Agreed, that's probably better.

> IMHO, the special divider calculation for ms6,7 is best dealt with
> in an extra else-if branch after the check for CLK_SET_RATE_PARENT
> flag.


> hwdata->params.p3 = 0;
> hwdata->params.p2 = 0;


> Out of curiosity, do you actually have a device that uses ms6,7 and can
> you measure the generated frequency - or did you just read the code as
> a bedtime story? ;)

:) I do have a device and have to use ms6,7, those are connected to pllb.
I have tested some integer pll dividers in a range from 30 to 130.
Measured frequencies and the frequencies shown in <debugfs>/clk/clk_summery
are the same.

I am going to send a patch v3.
Thanks for your feedback.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at