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

[Vadim Fedorenko]

On 23.06.2022 19:28, Jonathan Lemon wrote:
> On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
>> From: Vadim Fedorenko <vadfed at fb.com>
>>
>> +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?

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

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

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