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 <redacted>

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 <redacted>

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);
+}

...