Re: [PATCH v2 net-next 1/3] ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock.

[Jakub Kicinski]

On Tue, 16 Feb 2021 13:12:29 -0500 Vincent Cheng wrote:
> >> +}
> >> +
> >> +static int wait_for_sys_apll_dpll_lock(struct idtcm *idtcm)
> >> +{
> >> +	const char *fmt = "%d ms SYS lock timeout: APLL Loss Lock %d  DPLL state %d";
> >> +	u8 i = LOCK_TIMEOUT_MS / LOCK_POLL_INTERVAL_MS;  
> >
> >Using msleep() and loops is quite inaccurate. I'd recommend you switch
> >to:
> >
> >	unsigned long timeout = jiffies + msecs_to_jiffies(LOCK_TIMEOUT_MS);
> >
> >And then use:
> >
> >	while (time_is_after_jiffies(timeout))
> >  
> 
> To clarify, the suggestion is to use jiffies for accuracy, but
> the msleep(LOCK_POLL_INTERVAL_MS) remains to prevent the do-while
> loop from becoming a busy-wait loop.
> 
> #define LOCK_TIMEOUT_MS			(2000)
> #define LOCK_POLL_INTERVAL_MS		(10)
> 
> unsigned long timeout = jiffies + msecs_to_jiffies(LOCK_TIMEOUT_MS);
> 
> do {
> 	...
>         /*refresh 'locked' variable */
> 	if (locked)
> 		return 0;
> 	
> 	msleep(LOCK_POLL_INTERVAL_MS);
> 
> } while (time_is_after_jiffies(timeout));

Yes, exactly, sorry for lack of clarity.

> >> +			dev_warn(&idtcm->client->dev,
> >> +				"No wait state: DPLL_SYS_STATE %d", dpll);  
> >
> >It looks like other prints in this function use \n at the end of the
> >lines, should we keep it consistent?  
> 
> Looks like the \n is not needed for dev_warn.  Will remove \n 
> of existing messages for consistency.
>
> >> +	dev_warn(&idtcm->client->dev, fmt, LOCK_TIMEOUT_MS, apll, dpll);  
> >
> >I'd recommend leaving the format in place, that way static code
> >checkers can validate the arguments.  
> 
> Good point.  The fmt was used to keep 80 column rule.
> But now that 100 column rule is in place, the fmt
> workaround is not needed anymore.  Will fix in v3 patch.

Log strings / formats are a well known / long standing exception 
to the line length limit. No need to worry about that.

> >> +static void wait_for_chip_ready(struct idtcm *idtcm)
> >> +{
> >> +	if (wait_for_boot_status_ready(idtcm))
> >> +		dev_warn(&idtcm->client->dev, "BOOT_STATUS != 0xA0");  
> >
> >no new line?  
> 
> Nope.  Tried an experiment and \n is not neeeded.
> 
> 	dev_warn(&idtcm->client->dev, "debug: has \\n at end\n");
> 	dev_warn(&idtcm->client->dev, "debug: does not have \\n at end");
> 	dev_warn(&idtcm->client->dev, "debug: has \\n\\n at end\n\n");
> 	dev_warn(&idtcm->client->dev, "debug: hello");
> 	dev_warn(&idtcm->client->dev, "debug: world");
> 
> [   99.069100] idtcm 15-005b: debug: has \n at end
> [   99.073623] idtcm 15-005b: debug: does not have \n at end
> [   99.079017] idtcm 15-005b: debug: has \n\n at end
> [   99.079017]
> [   99.085194] idtcm 15-005b: debug: hello
> [   99.089025] idtcm 15-005b: debug: world
> 
> >> +
> >> +	if (wait_for_sys_apll_dpll_lock(idtcm))
> >> +		dev_warn(&idtcm->client->dev,
> >> +			 "Continuing while SYS APLL/DPLL is not locked");  
> >
> >And here.  
> 
> \n not needed.

Right, it's not needed I was just commenting that the new cases are not
consistent with existing code in the file, but removing \n everywhere
sounds fine as well.