Re: [PATCH net-next v5 1/4] net: wangxun: Add support for PTP clock

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 17/01/2025 06:20, Jiawen Wu wrote:
> Implement support for PTP clock on Wangxun NICs.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

[...]

> +/**
> + * wx_ptp_create_clock
> + * @wx: the private board structure
> + *
> + * Returns 0 on success, negative value on failure
> + *
> + * This function performs setup of the user entry point function table and
> + * initalizes the PTP clock device used by userspace to access the clock-like
> + * features of the PTP core. It will be called by wx_ptp_init, and may
> + * re-use a previously initialized clock (such as during a suspend/resume
> + * cycle).
> + */
> +static long wx_ptp_create_clock(struct wx *wx)
> +{
> +	struct net_device *netdev = wx->netdev;
> +	long err;
> +
> +	/* do nothing if we already have a clock device */
> +	if (!IS_ERR_OR_NULL(wx->ptp_clock))
> +		return 0;
> +
> +	snprintf(wx->ptp_caps.name, sizeof(wx->ptp_caps.name),
> +		 "%s", netdev->name);
> +	wx->ptp_caps.owner = THIS_MODULE;
> +	wx->ptp_caps.n_alarm = 0;
> +	wx->ptp_caps.n_ext_ts = 0;
> +	wx->ptp_caps.n_per_out = 0;
> +	wx->ptp_caps.pps = 0;
> +	wx->ptp_caps.adjfine = wx_ptp_adjfine;
> +	wx->ptp_caps.adjtime = wx_ptp_adjtime;
> +	wx->ptp_caps.gettimex64 = wx_ptp_gettimex64;
> +	wx->ptp_caps.settime64 = wx_ptp_settime64;
> +	if (wx->mac.type == wx_mac_em)
> +		wx->ptp_caps.max_adj = 500000000;
> +	else
> +		wx->ptp_caps.max_adj = 250000000;
> +
> +	wx->ptp_clock = ptp_clock_register(&wx->ptp_caps, &wx->pdev->dev);
> +	if (IS_ERR(wx->ptp_clock)) {
> +		err = PTR_ERR(wx->ptp_clock);
> +		wx->ptp_clock = NULL;
> +		wx_err(wx, "ptp clock register failed\n");
> +		return err;
> +	} else if (wx->ptp_clock) {

there is no way ptp_clock_register() will return NULL, that means
this `else if` construction is always true, so it's meaning less.
Please remove it if there will be need for another version.

Otherwise LGTM,

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>


> +		dev_info(&wx->pdev->dev, "registered PHC device on %s\n",
> +			 netdev->name);
> +	}



[...]