On Wed, 2014-05-21 at 08:23PM +0200, Uwe Kleine-König wrote: > 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: > > > On Tue, May 20, 2014 at 02:48:20PM -0700, Sören Brinkmann wrote: > > > > On Tue, 2014-05-20 at 10:48AM -0700, Stephen Boyd wrote: > > > > > On 05/20/14 09:01, Sören Brinkmann wrote: > > > > > > > > > > > >>>>> +{ > > > > > >>>>> + 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. But if the set_rate() doesn't set the rate to 0 in that case, the implementation was buggy. > > 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.) > > > > - 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? IMHO, there should always be a valid frequency be returned from these calls. The API says: return the frequency the clock provides when set_rate() is called with the same argument. This should never result in an error. Sören -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html