Re: [PATCH] clk: Don't show the incorrect clock phase

[Jerome Brunet <jbrunet@baylibre.com>]

On Thu, 2018-03-08 at 12:37 +0100, Jerome Brunet wrote:
> On Thu, 2018-03-08 at 19:28 +0800, Shawn Lin wrote:
> > > > +    if (core->phase < 0 && core->ops->get_phase)
> > > 
> > > Why bother with core->phase < 0 ?
> > > if core->ops->get_phase is available, why not call it anyway ?
> > > 
> > > The phase may have changed since it was cached.
> > > 
> > > A typical example would be phases based on adding fixed delays:
> > > 
> > > * Take a 200MHz clock and set phase to 180 => a 2.5ns delay is set
> > > * Clock rate change to 100MHz but delay is still 2.5ns
> > >     -> cached phase = 180
> > >     -> actual phase = 90
> > 
> > Good point!
> > 
> > (1)So either we should update phase once its rate is updated, for
> > instance, doing it in clk_change_rate, or
> 
> I don't think this would be a good option. The phase depending on the rate is
> just one example. I'm pretty sure one could come up with another example 
> 
> > 
> > (2)we should always try to get the actual phase here.
> 
> If the callback is available, lets use it. I prefer this second option
> 
> > 
> > If we do anyway call ->get_phase, we should never need core->phase since
> > it doesn't reflect the actual phase maybe.
> 
> The cached value is used by debugfs so it is still a valuable information. I
> would keep it around for now.

Oh ! and let's not forget that a clock driver is allowed to implement
set_phase() w/o implementing get_phase() ... so you definitively want to keep
the cached value around

> 
> > 
> > Am I right? If yes, which option is more reasonable?
> 
>