Remi Pommarel <r...@triplefau.lt> writes: > Hi, > > On Wed, Nov 04, 2015 at 06:03:31PM -0800, Eric Anholt wrote: > > [...] > >> >> It looks like you've dropped the use of the divisor off of the PLL >> channel when setting a rate. That seems bad for all the other clocks in >> the system, and a feature we couldn't lose. > > Sorry, but I'm not sure to understand your point here. Are you afraid > that clocks such as PWM, H264, etc, have lost the ability to divide the > rate from the PLL or oscillator clock they cosume as source ? > > If so, I think it's ok. If I'm not wrong here, clk_set_rate() first > calls clk->determinate_rate() then calls clk->set_rate(). This patch > makes bcm2835_clock_determine_source() to only select the parent to use > and does not set the clock's rate itself. The clock's rate is set later > on when bcm2835_clock_set_parent() is called. > > bcm2835_clock_set_parent() still divides the parent rate so we are not > loosing this feature here.
I see. You're leaving req->rate as-is, so that it gets passed back in on the set_rate() call. Since you've chosen only a parent with a rate greater than ours, we know we'll be able to divide into it. This has the downside that anything using the min/max rate clamping doesn't get to know before setting that we might be out of bounds. > It would probably be better to have bcm2835_clock_determine_source() > selects the parent by choosing the one that provides the rate which, > after being divided, generates the highest but lower rate out of the > PWM clock itself. > > Moreover, if you agree with the above modification I see no reason to > not call it "bcm2835_clock_determine_rate" This sounds like what the function should be doing.
signature.asc
Description: PGP signature
