[PATCH 08/13] ARM: LPC32XX: clock tree support

[Kevin Wells]

03, 2010 at 07:51:36PM +0100, Kevin Wells wrote:
> > > > +/*
> > > > + * clk_get_rate - obtain the current clock rate (in Hz) for a clock
> > > source
> > > > + */
> > > > +unsigned long clk_get_rate(struct clk *clk)
> > > > +{
> > > > +	if (clk->get_rate)
> > > > +		return (clk->get_rate)(clk);
> > > > +
> > > > +	/* If a clocks rate is 0, it uses the parent's rate instead.
> */
> > > > +	while (clk->rate == 0)
> > > > +		clk = clk->parent;
> > > > +
> > > > +	return clk->rate;
> > > > +}
> > > > +EXPORT_SYMBOL(clk_get_rate);
> > > doesn't that need locking to protect against a race with clk_set_rate?
> > > Hmmm, you never need to call get_rate for a parent clock?
> > >
> >
> > I'll add a lock for this, its needed. The get_rate() function will just
> > continue to fall back to the first non-0Hz parent clock (all clocks
> > eventually go to a non-0Hz parent clock). Most clocks don't even have
> > a get_rate() function.
> still, if a clock with a get_rate function has a child, clk_get_rate for
> the child might return a wrong result.
> 
> Actually you need something like:
> 
> 	while (!(rate = clk->rate) && !(clk->get_rate && (rate = clk-
> >get_rate(clk)))) {
> 		clk = clk->parent;
> 		if (!clk)
> 			break;
> 	}
> 	return rate;
> 
> Maybe with a nicer coding style by not using an assignment in the while
> condition.
> 

This approach always returned a non-0 clock rate for the clock, regardless
of whether the clock was enabled or not. Does the clk_get_rate() function
need to return a rate of 0 if the clock is disabled?