Re: [PATCH v2] ptp: add ToD device driver for Intel FPGA cards

On Wed, Mar 22, 2023 at 02:59:21PM +0000, Zhang, Tianfei wrote:

quoted

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Sent: Wednesday, March 22, 2023 10:49 PM
To: Zhang, Tianfei <redacted>
Cc: richardcochran@gmail.com; netdev@vger.kernel.org; linux-
fpga@vger.kernel.org; ilpo.jarvinen@linux.intel.com; Gomes, Vinicius
[off-list ref]; pierre-louis.bossart@linux.intel.com; Pagani, Marco
[off-list ref]; Weight, Russell H [off-list ref];
matthew.gerlach@linux.intel.com; nico@fluxnic.net; Khadatare, RaghavendraX
Anand [off-list ref]
Subject: Re: [PATCH v2] ptp: add ToD device driver for Intel FPGA cards

On Wed, Mar 22, 2023 at 10:35:47AM -0400, Tianfei Zhang wrote:

quoted

Adding a DFL (Device Feature List) device driver of ToD device for
Intel FPGA cards.

The Intel FPGA Time of Day(ToD) IP within the FPGA DFL bus is exposed
as PTP Hardware clock(PHC) device to the Linux PTP stack to
synchronize the system clock to its ToD information using phc2sys
utility of the Linux PTP stack. The DFL is a hardware List within
FPGA, which defines a linked list of feature headers within the device
MMIO space to provide an extensible way of adding subdevice features.

...

quoted

quoted

+	dt->ptp_clock = ptp_clock_register(&dt->ptp_clock_ops, dev);
+	if (IS_ERR_OR_NULL(dt->ptp_clock))
+		return dev_err_probe(dt->dev, PTR_ERR_OR_ZERO(dt->ptp_clock),
+				     "Unable to register PTP clock\n");
+
+	return 0;

Can be as simple as:

	ret = PTR_ERR_OR_ZERO(dt->ptp_clock);
	return dev_err_probe(dt->dev, ret, "Unable to register PTP clock\n");

            This should be :
           ret = PTR_ERR_OR_ZERO(dt->ptp_clock);
           if (ret)
                    return dev_err_probe(dt->dev, ret, "Unable to register PTP clock\n");
           return 0;

It depends how you treat the NULL from ptp_clock_register() and why driver will
be still bound to the device even if it doesn't provide PTP facility.

Either way you need to thing about this error handling and come up with
something that you can explain why it's done this way and not another.

        But this will be introduced one more local variable "ret" in this function.

Is it a problem?

-- 
With Best Regards,
Andy Shevchenko