[Intel-wired-lan] [PATCH net v1] i40e: allow controlling PTP external clock features

Kwapulinski, Piotr piotr.kwapulinski at intel.com
Tue Dec 8 12:51:12 UTC 2020


> On Mon, Nov 23, 2020 at 01:46:56PM +0000, Mateusz Palczewski wrote:
> > From: Piotr Kwapulinski <piotr.kwapulinski at intel.com>
> > 
> > Provide information what functions are supported by PTP pins and allow 
> > controlling them.
> > Implemented in i40e_ptp_verify() and i40e_pps_configure().
> > Previously it was not possible to control PTP external clock features.
> 
> Why you target that to 'net' tree? it's not a bug fix if you ask me, but a feature implementation.
Thank you for review.
It is intended to go to 'net' tree as all patches related to i40e driver. Isn't it appropriate approach ?
> 
> > 
> > Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski at intel.com>
> > ---
> >  drivers/net/ethernet/intel/i40e/i40e_ptp.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c 
> > b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > index 26f583f..6182d42 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > @@ -241,7 +241,15 @@ static void i40_ptp_reset_timing_events(struct 
> > i40e_pf *pf)  static int i40e_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin,
> >  			   enum ptp_pin_function func, unsigned int chan)
> 
> I don't see i40e_ptp_verify in the upstream code. Am I missing something?
There is another patch "i40e: add support for PTP external synchronization clock" being reviewed on intel-wired-lan. It was posted some time ago.
> 
> It also looks to me that you only make use of 1 out of 4 functions args.
> This will produce compiler warnings.
It depends on compiler options. I cannot see any warnings on my (default) setup. The interface is fixed and just not all arguments are needed. Other drivers use this similarly.
> 
> >  {
> > -	/* TODO: implement pin checking */
> > +	switch (func) {
> > +	case PTP_PF_NONE:
> > +	case PTP_PF_EXTTS:
> > +	case PTP_PF_PEROUT:
> > +		break;
> > +	case PTP_PF_PHYSYNC:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -486,7 +494,11 @@ static int i40e_pps_configure(struct ptp_clock_info *ptp,
> >  			      struct ptp_clock_request *rq,
> >  			      int on)
> >  {
> 
> Again, 'rq' is unused. Also it looks like it could return void.
For 'rq' please see my comment above.
Please be more detailed about 'returning void' scenario - I cannot see this.
> 
> > -	/* TODO: implement PPS events */
> > +	struct i40e_pf *pf = container_of(ptp, struct i40e_pf, ptp_caps);
> > +
> > +	if (!!on)
> > +		i40e_ptp_set_1pps_signal_hw(pf);
> > +
> >  	return 0;
> >  }

Best Regards,
Piotr


More information about the Intel-wired-lan mailing list