[RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Jonathan Lemon]

On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
> From: Vadim Fedorenko <vadfed at fb.com>
> 
> Implement DPLL operations in ptp_ocp driver.

Please CC: me as well.


> +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.


> +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.


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


> +	}
> +
> +	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?
>  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?
-- 
Jonathan