On 23.06.2022 19:28, Jonathan Lemon wrote:

On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:

quoted

From: Vadim Fedorenko <redacted>

+static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	int sync;
+
+	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
+	return sync;
+}

Please match existing code style.

Didn't get this point. The same code is used through out the driver.
Could you please explain?

quoted

+static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	int ret;
+
+	if (bp->sma[sma].mode != SMA_MODE_IN)
+		return -1;
+
+	switch (ptp_ocp_sma_get(bp, sma)) {
+	case 0:
+		ret = DPLL_TYPE_EXT_10MHZ;
+		break;
+	case 1:
+	case 2:
+		ret = DPLL_TYPE_EXT_1PPS;
+		break;
+	default:
+		ret = DPLL_TYPE_INT_OSCILLATOR;
+	}
+
+	return ret;
+}

These case statements switch on private bits.  This needs to match
on the selector name instead.

Addressed this in v2

quoted

+static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	int ret;
+
+	if (bp->sma[sma].mode != SMA_MODE_OUT)
+		return -1;
+
+	switch (ptp_ocp_sma_get(bp, sma)) {
+	case 0:
+		ret = DPLL_TYPE_EXT_10MHZ;
+		break;
+	case 1:
+	case 2:
+		ret = DPLL_TYPE_INT_OSCILLATOR;
+		break;
+	case 4:
+	case 8:
+		ret = DPLL_TYPE_GNSS;
+		break;
+	default:
+		ret = DPLL_TYPE_INT_OSCILLATOR;

Missing break;

quoted

+	}
+
+	return ret;
+}
+
+static struct dpll_device_ops dpll_ops = {
+	.get_status		= ptp_ocp_dpll_get_status,
+	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
+	.get_source_type	= ptp_ocp_dpll_get_source_type,
+	.get_output_type	= ptp_ocp_dpll_get_output_type,
+};

No 'set' statements here?  Also, what happens if there is more than
one GNSS receiver, how is this differentiated?

quoted

  static int
  ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  {

@@ -3768,6 +3846,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  
  	ptp_ocp_info(bp);
  	devlink_register(devlink);
+
+	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
+	if (!bp->dpll) {
+		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
+		return 0;
+	}
+	dpll_device_register(bp->dpll);
+

How is the release/unregister path called when the module is unloaded?

I re-implemented unregister/free in v2. Hope it fixes questions.