Sebastian,
thanks for your reply, please find my comments below:
On 06.04.2015 18:43, Sebastian Hesselbarth wrote:
> On 06.04.2015 16:25, Sergej Sawazki wrote:
>> The divider calculation for multisynth 6 and 7 differs from the
>> calculation for multisynth 0-5.
>>
>> For MS6 and MS7, set MSx_P1 directly, MSx_P1=divide value
>> [AN619, p. 6].
>>
>> Referenced document:
>> [AN619] Manually Generating an Si5351 Register Map, Rev. 0.4
>>
>> Signed-off-by: Sergej Sawazki <c...@gmx.de>
>
> Sergej,
>
> thanks for the patch, I do have some remarks though.
>
>> ---
>> drivers/clk/clk-si5351.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
>> index 44ea107..310078d 100644
>> --- a/drivers/clk/clk-si5351.c
>> +++ b/drivers/clk/clk-si5351.c
>> @@ -552,7 +552,8 @@ static const struct clk_ops si5351_pll_ops = {
>> * MSx_P2[19:0] = 128 * b - c * floor(128 * b/c) = (128*b) mod c
>> * MSx_P3[19:0] = c
>> *
>> - * MS[6,7] are integer (P1) divide only, P2 = 0, P3 = 0
>> + * MS[6,7] are integer (P1) divide only, P1 = divide value,
>> + * P2 and P3 are not applicable
>> *
>> * for 150MHz < fOUT <= 160MHz:
>> *
>> @@ -718,11 +719,18 @@ static long si5351_msynth_round_rate(struct
>> clk_hw *hw, unsigned long rate,
>> do_div(lltmp, a * c + b);
>> rate = (unsigned long)lltmp;
>>
>> - /* calculate parameters */
>> + /*
>> + * calculate parameters
>> + * for multisync6-7 set p1 directly, fOUT = fIN / p1
>
> I'd say the comment about the differences of MS6,7 in the function's
> comment block is enough, no need to repeat it again here.
>
I agree.
>> + */
>> if (divby4) {
>> hwdata->params.p3 = 1;
>> hwdata->params.p2 = 0;
>> hwdata->params.p1 = 0;
>> + } else if (hwdata->num >= 6) {
>> + hwdata->params.p3 = 1;
>> + hwdata->params.p2 = 0;
>> + hwdata->params.p1 = a;
>
> Hmm. Just overruling what will be written in the registers is
> definitely not enough. If you look at the first two lines of this
> hunk, you'll admit that the visible representation (rate) of the MS
> frequency isn't affected by your patch at all.
>
> That basically means that real frequency and frequency in the kernel
> will differ, so we have to put this change in the param calculation
> above this hunk instead. The extra else-if-path here is ok though.
>
You right, I will take a look at that and provide a patch v2.
Sergej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
