On 05/02/2011 03:36 PM, Russell King - ARM Linux wrote:

On Mon, May 02, 2011 at 11:30:10AM -0500, Rob Herring wrote:

quoted

On 05/01/2011 10:40 PM, Jeremy Kerr wrote:

quoted

Hi Rob,

quoted

I think you will find many examples in the kernel where that is not done
by drivers.

Drivers should be checking the return value of clk_get - if they don't,
it's a bug. This is the logical place to check, rather than before all
clock API calls.

Maybe so, but it's common practice. Why not allow it, but add a warning?
Or allow NULL, but not an error value.

If drivers don't check the return value of clk_get() then they'll suffer
kernel oopses.  What's to say that if the driver doesn't check the return
value from clk_get() that they'll bother checking the return value from
any of the other clk_* API functions.  Nothing.

So, checking for ERR values inside the clk_* functions is silly, and just
means that buggy drivers continue with undiscovered bugs because they
happen not to fail.

quoted

quoted

For cases where there is no clock provided for the device (but is a
valid clock on some machines), the platform code should return a no-op
clock from the clk_get call. This 'noop clock' would be a good contender
for inclusion into the kernel-wide infrastructure, like clk_fixed.

There could be cases where the driver wants to know if there is no
clock.

How exactly would a driver know that the clock it has is a no-op clock
with it being a pointer to some random data structure?  A better question
would be in the case of a cpufreq driver, how would such a driver know
that it's pointless trying to set the rate on a fixed rate clock?

There's soo many combinations here where you can say "if a clock has
property X, then maybe the driver doesn't want to do Y".  So, instead
of trying to cover all the possibilities, take a step back and look at
the bigger picture.

1. in a cpufreq driver, if it gets a clock, one would hope that the
clock it has been provided with is adjustable - in other words,
clk_set_rate() stands a chance of succeeding.  If it fails, then it
has the wrong clock.

2. if the clock it has does not support adjustments in this way, then
there is a bug in the data involved in selecting that clock, and that's
what's at fault.  You find this out when you try to change its rate
via clk_set_rate() which should fail.

3. (to head off something that I can see being proposed) no, I don't
think we need a system of properties.

A cpufreq driver which obtains clks should be no different from any
other driver.  It should get a clk via clk_get(), and report errors if
that fails.  If clk_set_rate() fails, then it should report that error
too.  There's really nothing "special" here.

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.

-Saravana

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