[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240105212704.GB31813@kernel.org>
Date: Fri, 5 Jan 2024 21:27:04 +0000
From: Simon Horman <horms@...nel.org>
To: Min Li <lnimi@...mail.com>
Cc: richardcochran@...il.com, lee@...nel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, Min Li <min.li.xe@...esas.com>
Subject: Re: [PATCH net-next v7 2/2] ptp: add FemtoClock3 Wireless as ptp
hardware clock
On Fri, Jan 05, 2024 at 10:23:13AM -0500, Min Li wrote:
> From: Min Li <min.li.xe@...esas.com>
>
> The RENESAS FemtoClock3 Wireless is a high-performance jitter attenuator,
> frequency translator, and clock synthesizer. The device is comprised of 3
> digital PLLs (DPLL) to track CLKIN inputs and three independent low phase
> noise fractional output dividers (FOD) that output low phase noise clocks.
>
> FemtoClock3 supports one Time Synchronization (Time Sync) channel to enable
> an external processor to control the phase and frequency of the Time Sync
> channel and to take phase measurements using the TDC. Intended applications
> are synchronization using the precision time protocol (PTP) and
> synchronization with 0.5 Hz and 1 Hz signals from GNSS.
>
> Signed-off-by: Min Li <min.li.xe@...esas.com>
Hi Min Li,
some minor suggestions from my side.
...
> +static int idtfc3_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
> +{
> + struct idtfc3 *idtfc3 = container_of(ptp, struct idtfc3, caps);
> + int err;
> +
> + mutex_lock(idtfc3->lock);
> + err = _idtfc3_gettime(idtfc3, ts);
> + mutex_unlock(idtfc3->lock);
> +
> + if (err)
> + dev_err(idtfc3->dev, "Failed at line %d in %s!",
> + __LINE__, __func__);
IMHO messages like the one above offer no value to users.
I would remove it and similar messages.
> +
> + return err;
> +}
> +
> +static int _idtfc3_settime(struct idtfc3 *idtfc3, const struct timespec64 *ts)
> +{
> + s64 offset_ns, now_ns, sync_ns;
> + u32 counter, sub_ns;
> + int now;
> +
> + if (timespec64_valid(ts) == false) {
> + dev_err(idtfc3->dev, "%s: invalid timespec", __func__);
> + return -EINVAL;
> + }
> +
> + now = idtfc3_read_subcounter(idtfc3);
> + if (now < 0)
> + return now;
> +
> + offset_ns = (idtfc3->sub_sync_count - now) * idtfc3->ns_per_counter;
> + now_ns = timespec64_to_ns(ts);
> + sync_ns = ns2counters(idtfc3, offset_ns + now_ns, &sub_ns);
nit: sync_ns is set but unused in this function.
I think you can remove sync_ns from this function and simply do:
ns2counters(idtfc3, offset_ns + now_ns, &sub_ns);
> +
> + counter = sub_ns / idtfc3->ns_per_counter;
> + return idtfc3_timecounter_update(idtfc3, counter, now_ns);
> +}
...
Powered by blists - more mailing lists