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.
