From: Jakub Kicinski <kuba@kernel•org>
To: Min Li <min.li.xe@renesas•com>
Cc: richardcochran@gmail•com, lee.jones@linaro•org,
linux-kernel@vger•kernel.org, netdev@vger•kernel.org
Subject: Re: [PATCH net v3 1/2] ptp: ptp_clockmatrix: Add PTP_CLK_REQ_EXTTS support
Date: Tue, 3 May 2022 17:11:40 -0700 [thread overview]
Message-ID: <20220503171140.27dbf8cf@kernel.org> (raw)
In-Reply-To: <1651518530-25128-1-git-send-email-min.li.xe@renesas.com>
On Mon, 2 May 2022 15:08:49 -0400 Min Li wrote:
> Use TOD_READ_SECONDARY for extts to keep TOD_READ_PRIMARY
> for gettime and settime exclusively
>
> Signed-off-by: Min Li <min.li.xe@renesas•com>
> Acked-by: Richard Cochran <richardcochran@gmail•com>
Since this is a new feature please tag the next version as
[PATCH net-next v4]. Bug fixes get tagged with [PATCH net]
new features, refactoring etc with [PATCH net-next].
> + err = idtcm_write(idtcm, channel->tod_read_secondary, tod_read_cmd,
> + &val, sizeof(val));
> +
> + if (err)
Please remove the empty lines between calling a function and checking
if it returned an error (only in the new code you're adding in this
patch).
> + dev_err(idtcm->dev, "%s: err = %d", __func__, err);
>
> - err = idtcm_write(idtcm, channel->tod_read_primary,
> - tod_read_cmd, &val, sizeof(val));
> return err;
> }
>
> -static int idtcm_enable_extts(struct idtcm_channel *channel, u8 todn, u8 ref,
> - bool enable)
> +static bool is_single_shot(u8 mask)
> {
> - struct idtcm *idtcm = channel->idtcm;
> - u8 old_mask = idtcm->extts_mask;
> - u8 mask = 1 << todn;
> + /* Treat single bit ToD masks as continuous trigger */
> + if ((mask == 1) || (mask == 2) || (mask == 4) || (mask == 8))
> + return false;
> + else
> + return true;
This function is better written as:
/* Treat single bit ToD masks as continuous trigger */
return mask <= 8 && is_power_of_2(mask);
> +}
> +static int _idtcm_gettime_triggered(struct idtcm_channel *channel,
> + struct timespec64 *ts)
> +{
> + struct idtcm *idtcm = channel->idtcm;
> + u16 tod_read_cmd = IDTCM_FW_REG(idtcm->fw_ver, V520, TOD_READ_SECONDARY_CMD);
> + u8 buf[TOD_BYTE_COUNT];
> + u8 trigger;
> + int err;
> +
> + err = idtcm_read(idtcm, channel->tod_read_secondary,
> + tod_read_cmd, &trigger, sizeof(trigger));
> + if (err)
> + return err;
> +
> + if (trigger & TOD_READ_TRIGGER_MASK)
> + return -EBUSY;
> +
> + err = idtcm_read(idtcm, channel->tod_read_secondary,
> + TOD_READ_SECONDARY_BASE, buf, sizeof(buf));
> +
> + if (err)
> + return err;
> +
> + err = char_array_to_timespec(buf, sizeof(buf), ts);
> +
> + return err;
Please return directly:
return char_array_...
> +}
> static int _idtcm_gettime_immediate(struct idtcm_channel *channel,
> struct timespec64 *ts)
> {
> struct idtcm *idtcm = channel->idtcm;
> - u8 extts_mask = 0;
> +
> + u16 tod_read_cmd = IDTCM_FW_REG(idtcm->fw_ver, V520, TOD_READ_PRIMARY_CMD);
> + u8 val = (SCSR_TOD_READ_TRIG_SEL_IMMEDIATE << TOD_READ_TRIGGER_SHIFT);
> int err;
>
> - /* Disable extts */
> - if (idtcm->extts_mask) {
> - extts_mask = idtcm_enable_extts_mask(channel, idtcm->extts_mask,
> - false);
> - }
> + err = idtcm_write(idtcm, channel->tod_read_primary,
> + tod_read_cmd, &val, sizeof(val));
>
> - err = _idtcm_set_scsr_read_trig(channel,
> - SCSR_TOD_READ_TRIG_SEL_IMMEDIATE, 0);
> - if (err == 0)
> - err = _idtcm_gettime(channel, ts, 10);
> + if (err)
> + return err;
>
> - /* Re-enable extts */
> - if (extts_mask)
> - idtcm_enable_extts_mask(channel, extts_mask, true);
> + err = _idtcm_gettime(channel, ts, 10);
>
> return err;
Same here
return _idtcm_gettime(...
> }
> @@ -2420,10 +2502,11 @@ static int idtcm_remove(struct platform_device *pdev)
> {
> struct idtcm *idtcm = platform_get_drvdata(pdev);
>
> - ptp_clock_unregister_all(idtcm);
> -
> + idtcm->extts_mask = 0;
> cancel_delayed_work_sync(&idtcm->extts_work);
>
> + ptp_clock_unregister_all(idtcm);
Why is the order of unregistering the clock and canceling the work
changed? There is no locking around this function so seems like
the work can get scheduled right after the call to
cancel_delayed_work_sync(), anyway.
> return 0;
> }
prev parent reply other threads:[~2022-05-04 0:11 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-02 19:08 [PATCH net v3 1/2] ptp: ptp_clockmatrix: Add PTP_CLK_REQ_EXTTS support Min Li
2022-05-02 19:08 ` [PATCH net v3 2/2] ptp: ptp_clockmatrix: return -EBUSY if phase pull-in is in progress Min Li
2022-05-04 0:11 ` Jakub Kicinski [this message]
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=20220503171140.27dbf8cf@kernel.org \
--to=kuba@kernel$(echo .)org \
--cc=lee.jones@linaro$(echo .)org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=min.li.xe@renesas$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=richardcochran@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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