Re: [PATCH] clk: divider: fix rate calculation for fractional rates

[Tomi Valkeinen]

Hi Mike, Russell,

On 2013-11-06 18:19, Russell King - ARM Linux wrote:
> On Wed, Nov 06, 2013 at 01:48:44PM +0200, Tomi Valkeinen wrote:
>> On 2013-11-06 13:15, Russell King - ARM Linux wrote:
>>> On Wed, Nov 06, 2013 at 01:06:48PM +0200, Tomi Valkeinen wrote:
>>>> This means that the following code works a bit oddly:
>>>>
>>>> rate = clk_round_rate(clk, 123428572);
>>>> clk_set_rate(clk, rate);
>>>
>>> You're right, but the above sequence is quite a crass thing to do.  Why?
>>
>> Do you mean that you think the fix is right, but the above example
>> sequence is silly, or that the fix is not needed either?
> 
> I think a fix _is) required, because:
> 
> 	rate = clk_get_rate(clk);
> 	clk_set_rate(clk, rate);
> 	assert(clk_get_rate(clk) == rate);
> 
> If not, there's something quite wrong.  However, I am saying that the
> sequence you provided was nevertheless silly - I've seen it in real code
> in the kernel, which is why I've commented about it.

I just ran into this issue again with omap3, and so I'm resurrecting the
thread.

Mike, can you review the patch?

Russell, I'd like to understand why you think the original example is bad:

	rate = clk_round_rate(clk, rate);
	clk_set_rate(clk, rate);

If the definition of clk_round_rate is basically "clk_set_rate without
actually setting the rate", I agree that the above code is not good as
it might not work correctly.

However, if  the following code you gave should work:

	rate = clk_get_rate(clk);
	clk_set_rate(clk, rate);
	assert(clk_get_rate(clk) == rate);

then the original example should also always work, as it's almost the
same as:

	/* this is the "round" part */
	clk_set_rate(clk, rate);
	rate = clk_get_rate(clk);

	clk_set_rate(clk, rate);
	assert(clk_get_rate(clk) == rate);

Why I'm asking this is that for me (and probably for others also if
you've seen it used in the kernel code) it feels natural to have code like:

	rate = clk_round_rate(clk, rate);
	
	/* Verify the rounded rate here to see it's ok for the IP etc */

	/* The rate is ok, so set it */
	clk_set_rate(clk, rate);

This could be rewritten as:

	rounded_rate = clk_round_rate(clk, rate);
	
	/* Verify the rounded rate here to see it's ok for the IP etc */

	/* The rounded rate is ok, so set the original rate */
	clk_set_rate(clk, rate);

But it just feels unnecessary complication to keep both the original
rate and the rounded rate around.

 Tomi

Attachment: signature.asc
Description: OpenPGP digital signature