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

On Tue, 16 Feb 2021 13:12:29 -0500 Vincent Cheng wrote:

quoted

quoted

+}
+
+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.

quoted

quoted

+			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.

quoted

quoted

+	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.

quoted

quoted

+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

quoted

quoted

+
+	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.