On 05/03/2011 11:40 PM, Sascha Hauer wrote:

On Mon, May 02, 2011 at 05:22:01PM -0700, Saravana Kannan wrote:

quoted

quoted

This does bring us to an interesting question though: should clk_set_rate()
succeed or fail with a NULL clk?  There is no clock to control, so my
feeling is that it should fail, just like clk_get_rate() should return
zero because the rate is meaningless.  There is no rate to get and no
rate to set.

I think having NULL clocks return success on set_rate() would be
more useful. The most common use case for NULL clocks is when a
clocks is present in some soc/arch/mach but is not present in
another. Instead of having the consumers having an "if" around every
clk_* calls on that clk, they would get a NULL clk. If NULL clk
starts failing for clk_set_rate(), I think the point of a NULL clk
will get defeated.

 From my experience when a clock is present on one SoC but not on another
the actual rate does not matter, only enabling/disabling the clock for
register accesses is important. When a driver sets a rate on a clock it
does it on purpose and if this clock is not even present on other SoCs
it looks like a bug or at least a very special case for me. So I think
clk_set_rate for a NULL clock should fail.

Good point.

Other than that it's not a good behaviour when clk_get_rate() returns
0 for clock which rate has been successfully set to another rate using
clk_set_rate().

It's a NULL clock, so it hopefully shouldn't matter.

I think you have convinced my about set_rate() returning error is not 
catastrophic. But if it doesn't hurt to return success, I think we 
should go ahead and do that in case someone cares about it?

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.