Re: [PATCH v2 03/12] clk: Add a function to retrieve phase

[Maxime Ripard <maxime.ripard@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 2839 bytes --]

On Mon, Sep 01, 2014 at 01:27:28PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/01/2014 12:20 PM, Maxime Ripard wrote:
> > Hi,
> > 
> > On Sun, Aug 31, 2014 at 12:15:50PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 08/30/2014 10:03 PM, Maxime Ripard wrote:
> >>> The current phase API doesn't look into the actual hardware to get the phase
> >>> value, but will rather get it from a variable only set by the set_phase
> >>> function.
> >>>
> >>> This will cause issue when the client driver will never call the set_phase
> >>> function, where we can end up having a reported phase that will not match what
> >>> the hardware has been programmed to by the bootloader or what phase is
> >>> programmed out of reset.
> >>>
> >>> Add a new get_phase function for the drivers to implement so that we can get
> >>> this value.
> >>>
> >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >>> ---
> >>>  drivers/clk/clk.c            | 17 ++++++++++++++---
> >>>  include/linux/clk-provider.h |  5 +++++
> >>>  2 files changed, 19 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >>> index d87661af0c72..7dbceca694f1 100644
> >>> --- a/drivers/clk/clk.c
> >>> +++ b/drivers/clk/clk.c
> >>> @@ -1797,8 +1797,8 @@ out:
> >>>   * clk_get_phase - return the phase shift of a clock signal
> >>>   * @clk: clock signal source
> >>>   *
> >>> - * Returns the phase shift of a clock node in degrees, otherwise returns
> >>> - * -EERROR.
> >>> + * Returns the phase shift of a clock node in degrees. Any negative
> >>> + * values are errors.
> >>>   */
> >>>  int clk_get_phase(struct clk *clk)
> >>>  {
> >>> @@ -1808,7 +1808,18 @@ int clk_get_phase(struct clk *clk)
> >>>  		goto out;
> >>>  
> >>>  	clk_prepare_lock();
> >>> -	ret = clk->phase;
> >>> +
> >>> +	if (clk->phase) {
> >>> +		ret = clk->phase;
> >>> +		goto out_unlock;
> >>> +	}
> >>
> >> 0 is a valid phase, so this will cause the phase to
> >> be read from the hardware each time if the phase is 0.
> >>
> >> Perhaps make clk->phase signed (if it is not already), init it
> >> to -1, and check for it not being -1 ?
> > 
> > Yeah, I'm not really proud of this code either, but yours would expose
> > this -1 into debugfs, so I'm not sure it's really better :) 
> > 
> > (with -1 being a valid phase too)
> 
> According to the comments in the patch adding the original phase functions
> valid values are 0 - 359. And you could use clk_get_phase from the debugfs
> code.

Yep, except that if I see a value -1 as a phase, I would assume that
it's 359, but maybe it's just me :)

Mike's solution seem great, I'll go for that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]