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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Mar 22, 2023 at 02:59:21PM +0000, Zhang, Tianfei wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Wednesday, March 22, 2023 10:49 PM
> > To: Zhang, Tianfei <tianfei.zhang@intel.com>
> > Cc: richardcochran@gmail.com; netdev@vger.kernel.org; linux-
> > fpga@vger.kernel.org; ilpo.jarvinen@linux.intel.com; Gomes, Vinicius
> > <vinicius.gomes@intel.com>; pierre-louis.bossart@linux.intel.com; Pagani, Marco
> > <marpagan@redhat.com>; Weight, Russell H <russell.h.weight@intel.com>;
> > matthew.gerlach@linux.intel.com; nico@fluxnic.net; Khadatare, RaghavendraX
> > Anand <raghavendrax.anand.khadatare@intel.com>
> > 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:
> > > 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.

...

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