From: Jakub Kicinski <kuba@kernel.org>
To: Vincent Cheng <vincent.cheng.xh@renesas.com>
Cc: richardcochran@gmail.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 net-next 1/3] ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock.
Date: Wed, 17 Feb 2021 10:03:06 -0800 [thread overview]
Message-ID: <20210217100306.4948cc73@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210216181226.GA5450@renesas.com>
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.
next prev parent reply other threads:[~2021-02-17 18:04 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-13 5:06 [PATCH v2 net-next 0/3] ptp: ptp_clockmatrix: Fix output 1 PPS alignment vincent.cheng.xh
2021-02-13 5:06 ` [PATCH v2 net-next 1/3] ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock vincent.cheng.xh
2021-02-15 19:48 ` Jakub Kicinski
2021-02-16 18:12 ` Vincent Cheng
2021-02-17 18:03 ` Jakub Kicinski [this message]
2021-02-13 5:06 ` [PATCH v2 net-next 2/3] ptp: ptp_clockmatrix: Add alignment of 1 PPS to idtcm_perout_enable vincent.cheng.xh
2021-02-13 5:06 ` [PATCH v2 net-next 3/3] ptp: ptp_clockmatrix: Remove unused header declarations vincent.cheng.xh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210217100306.4948cc73@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
--to=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=richardcochran@gmail.com \
--cc=vincent.cheng.xh@renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).