Re: [PATCH] misc: ioc4: simplify wave period measurement in clock_calibrate

[Paul Bolle]

On Mon, 2014-12-08 at 16:28 +0100, Richard Leitner wrote:
> The loop for measuring the square wave periods over some cycles is
> refactored to be more easily readable. This includes avoiding a
> "by-hand-implemented" for loop with a "real" one and adding some
> comments.
> 
> Furthermore the following compiler warning is avoided by this patch:
> drivers/misc/ioc4.c: In function âioc4_probeâ:
> drivers/misc/ioc4.c:194:16: warning: âstartâ may be used uninitialized
> in this function [-Wmaybe-uninitialized]
>   period = (end - start) /
>                 ^
> drivers/misc/ioc4.c:148:11: note: âstartâ was declared here
>   uint64_t start, end, period;
>            ^
> 
> Signed-off-by: Richard Leitner <dev@xxxxxxxxxx>
> ---
> A simplification of this loop was suggested by Andrew Morton [1].
> This is my first proposal of such a simplification.
> 
> Furthermore I'm not sure if the commit message is sufficient.
> Please give me also some feedback on it.
> 
> If this simplification is not needed only initializing start to
> ktime_get_ns() would fix the compiler warning too.
> 
> [1] https://lkml.org/lkml/2014/12/5/76
> ---
>  drivers/misc/ioc4.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/misc/ioc4.c b/drivers/misc/ioc4.c
> index 3336ddc..8758d03 100644
> --- a/drivers/misc/ioc4.c
> +++ b/drivers/misc/ioc4.c
> @@ -144,9 +144,9 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd)
>  {
>  	union ioc4_int_out int_out;
>  	union ioc4_gpcr gpcr;
> -	unsigned int state, last_state = 1;
> +	unsigned int state, last_state;
>  	uint64_t start, end, period;
> -	unsigned int count = 0;
> +	unsigned int count;
>  
>  	/* Enable output */
>  	gpcr.raw = 0;
> @@ -167,19 +167,20 @@ ioc4_clock_calibrate(struct ioc4_driver_data *idd)
>  	mmiowb();
>  
>  	/* Check square wave period averaged over some number of cycles */
> -	do {
> -		int_out.raw = readl(&idd->idd_misc_regs->int_out.raw);
> -		state = int_out.fields.int_out;
> -		if (!last_state && state) {
> -			count++;
> -			if (count == IOC4_CALIBRATE_END) {
> -				end = ktime_get_ns();
> -				break;
> -			} else if (count == IOC4_CALIBRATE_DISCARD)
> -				start = ktime_get_ns();
> -		}
> -		last_state = state;
> -	} while (1);
> +	start = ktime_get_ns();
> +	state = 1; /* make sure the first read isn't a rising edge */
> +	for (count = 0; count <= IOC4_CALIBRATE_END; count++) {
> +		do { /* wait for a rising edge */
> +			last_state = state;
> +			int_out.raw = readl(&idd->idd_misc_regs->int_out.raw);
> +			state = int_out.fields.int_out;
> +		} while (last_state || !state);
> +
> +		/* discard the first few cycles */
> +		if (count == IOC4_CALIBRATE_DISCARD)
> +			start = ktime_get_ns();
> +	}
> +	end = ktime_get_ns();
>  
>  	/* Calculation rearranged to preserve intermediate precision.
>  	 * Logically:

I've had the patch pasted at the end of this message in my local stack
of patches for some time now. It uses another approach: by using a
helper function things become much simpler. I _think_ it doesn't
introduce any functional changes but still need to a quiet day to make
sure that is correct.

Hope this helps,


Paul Bolle
---