On Thu, Nov 12, 2020 at 04:35:33PM +0100, Christian Eggers wrote:

"default n" is implicit, please remove this

Don't you want to move these declarations right beneath the "if" line?

+
+		/* see scaled_ppm_to_ppb() in ptp_clock.c for details */
+		ppb = 1 + scaled_ppm;
+		ppb *= 125;
+		ppb *= KSZ_PTP_INC_NS;
+		ppb <<= KSZ_PTP_SUBNS_BITS - 13;
+		adj = div_s64(ppb, NSEC_PER_SEC);
+
+		data32 = abs(adj);
+		data32 &= BIT_MASK(30) - 1;
+		if (adj >= 0)
+			data32 |= PTP_RATE_DIR;
+
+		ret = ksz_write32(dev, REG_PTP_SUBNANOSEC_RATE, data32);
+		if (ret)
+			return ret;
+	}
+
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
+	if (ret)
+		return ret;
+
+	if (scaled_ppm)
+		data16 |= PTP_CLK_ADJ_ENABLE;
+	else
+		data16 &= ~PTP_CLK_ADJ_ENABLE;
+
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	s32 sec, nsec;
+	u16 data16;
+	int ret;
+
+	mutex_lock(&dev->ptp_mutex);

You're skipping the mutex in ksz9477_ptp_adjfine, is that intentional or
just a proof that it's useless? You should add a comment at its
declaration site about what it's meant to protect.

+
+	/* do not use ns_to_timespec64(), both sec and nsec are subtracted by hw */
+	sec = div_s64_rem(delta, NSEC_PER_SEC, &nsec);
+
+	ret = ksz_write32(dev, REG_PTP_RTC_NANOSEC, abs(nsec));
+	if (ret)
+		goto error_return;
+
+	/* contradictory to the data sheet, seconds are also considered */
+	ret = ksz_write32(dev, REG_PTP_RTC_SEC, abs(sec));
+	if (ret)
+		goto error_return;
+
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
+	if (ret)
+		goto error_return;
+
+	data16 |= PTP_STEP_ADJ;
+	if (delta < 0)
+		data16 &= ~PTP_STEP_DIR;  /* 0: subtract */
+	else
+		data16 |= PTP_STEP_DIR;   /* 1: add */
+
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
+	if (ret)
+		goto error_return;
+
+error_return:
+	mutex_unlock(&dev->ptp_mutex);
+	return ret;
+}
+
+static int _ksz9477_ptp_gettime(struct ksz_device *dev, struct timespec64 *ts)
+{
+	u32 nanoseconds;
+	u32 seconds;
+	u16 data16;
+	u8 phase;
+	int ret;
+
+	/* Copy current PTP clock into shadow registers */
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
+	if (ret)
+		return ret;
+
+	data16 |= PTP_READ_TIME;
+
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
+	if (ret)
+		return ret;
+
+	/* Read from shadow registers */
+	ret = ksz_read8(dev, REG_PTP_RTC_SUB_NANOSEC__2, &phase);
+	if (ret)
+		return ret;
+	ret = ksz_read32(dev, REG_PTP_RTC_NANOSEC, &nanoseconds);
+	if (ret)
+		return ret;
+	ret = ksz_read32(dev, REG_PTP_RTC_SEC, &seconds);
+	if (ret)
+		return ret;
+
+	ts->tv_sec = seconds;
+	ts->tv_nsec = nanoseconds + phase * 8;
+
+	return 0;
+}
+
+static int ksz9477_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+{
+	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	int ret;
+
+	mutex_lock(&dev->ptp_mutex);
+	ret = _ksz9477_ptp_gettime(dev, ts);
+	mutex_unlock(&dev->ptp_mutex);
+
+	return ret;
+}
+
+static int ksz9477_ptp_settime(struct ptp_clock_info *ptp, struct timespec64 const *ts)
+{
+	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	u16 data16;
+	int ret;
+
+	mutex_lock(&dev->ptp_mutex);
+
+	/* Write to shadow registers */
+
+	/* clock phase */
+	ret = ksz_read16(dev, REG_PTP_RTC_SUB_NANOSEC__2, &data16);
+	if (ret)
+		goto error_return;
+
+	data16 &= ~PTP_RTC_SUB_NANOSEC_M;
+
+	ret = ksz_write16(dev, REG_PTP_RTC_SUB_NANOSEC__2, data16);
+	if (ret)
+		goto error_return;
+
+	/* nanoseconds */
+	ret = ksz_write32(dev, REG_PTP_RTC_NANOSEC, ts->tv_nsec);
+	if (ret)
+		goto error_return;
+
+	/* seconds */
+	ret = ksz_write32(dev, REG_PTP_RTC_SEC, ts->tv_sec);
+	if (ret)
+		goto error_return;
+
+	/* Load PTP clock from shadow registers */
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
+	if (ret)
+		goto error_return;
+
+	data16 |= PTP_LOAD_TIME;
+
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
+	if (ret)
+		goto error_return;
+
+error_return:
+	mutex_unlock(&dev->ptp_mutex);
+	return ret;
+}
+
+static int ksz9477_ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *req, int on)
+{
+	return -ENOTTY;
+}

How about -EOPNOTSUPP? I think -ENOTTY is reserved for "invalid ioctl on
the PTP clock", you wouldn't want to confuse a user into thinking that
they got the ioctls wrong (such as with the recent introduction of
PTP_PEROUT_REQUEST2 etc) when in fact the error comes from the driver.