Hello S?ren,

On Wed, May 21, 2014 at 08:58:10AM -0700, S?ren Brinkmann wrote:

On Wed, 2014-05-21 at 09:34AM +0200, Uwe Kleine-K?nig wrote:

quoted

On Tue, May 20, 2014 at 02:48:20PM -0700, S?ren Brinkmann wrote:

quoted

On Tue, 2014-05-20 at 10:48AM -0700, Stephen Boyd wrote:

quoted

On 05/20/14 09:01, S?ren Brinkmann wrote:

quoted

quoted

quoted

quoted

quoted

+{
+	unsigned long lower, upper, cur, lower_last, upper_last;
+
+	lower = clk_round_rate(clk, rate);
+	if (lower >= rate)
+		return lower;

Is the >-case worth a warning?

No, it's correct behavior. If you request a rate that is way lower than what the
clock can generate, returning something larger is perfectly valid, IMHO.
Which reveals one problem in this whole discussion. The API does not
require clk_round_rate() to round down. It is actually an implementation
choice that had been made for clk-divider.

I'm sure it's more than an implementation choice for clk-divider. But I
don't find any respective documentation (but I didn't try hard).

A similar discussion - without final conclusion:
https://lkml.org/lkml/2010/7/14/260

Please call this new API something like clk_find_nearest_rate() or
something. clk_round_rate() is supposed to return the rate that will be
set if you call clk_set_rate() with the same arguments. It's up to the
implementation to decide if that means rounding the rate up or down or
to the nearest value.

Sounds good to me. Are there any cases of clocks that round up? I think
that case would not be handled correctly. But I also don't see a use
case for such an implementation.

I don't really care which semantic (i.e. round up, round down or round
closest) is picked, but I'd vote that all should pick up the same. I
think the least surprising definition is to choose rounding down and add
the function that is under discussion here to get a nearest match.

So I suggest:

	- if round_rate is given a rate that is smaller than the
	  smallest available rate, return 0
	- add WARN_ONCE to round_rate and set_rate if they return with a
	  rate bigger than requested

Why do you think 0 is always valid? I think for a clock that can
generate 40, 70, 120, clk_round_rate(20) should return 40.

I didn't say it's a valid value. It just makes the it possible to check
for clk_round_rate(clk, rate) <= rate.

I grepped a bit around and found da850_round_armrate which implements a
round_rate callback returning the best match.
omap1_clk_round_rate_ckctl_arm can return a value < 0.
s3c2412_roundrate_usbsrc can return values that are bigger than
requested. (I wonder if that is a bug though.)

quoted

	- change the return values to unsigned long

Yep, I agree, this should happen.

And we're using 0 as error value? e.g. for the case where
omap1_clk_round_rate_ckctl_arm returns -EIO now?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |