RE: [net-next 04/14] e1000e: add support for IEEE-1588 PTP

Allan, Bruce W
2013-01-18T01:13:23Z

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, January 17, 2013 7:36 AM
> To: Kirsher, Jeffrey T
> Cc: davem@davemloft.net; Allan, Bruce W; netdev@vger.kernel.org;
> gospo@redhat.com; sassmann@redhat.com
> Subject: Re: [net-next 04/14] e1000e: add support for IEEE-1588 PTP
>
> On Thu, Jan 17, 2013 at 03:35:09AM -0800, Jeff Kirsher wrote:
>
> > +static struct ptp_clock_info e1000e_ptp_clock_info = {
> > + .owner = THIS_MODULE,
>
> small nit: better to use a static string for .name here than ...
>
> > + .n_alarm = 0,
> > + .n_ext_ts = 0,
> > + .n_per_out = 0,
> > + .pps = 0,
> > + .adjfreq = e1000e_phc_adjfreq,
> > + .adjtime = e1000e_phc_adjtime,
> > + .gettime = e1000e_phc_gettime,
> > + .settime = e1000e_phc_settime,
> > + .enable = e1000e_phc_enable,
> > +};
> > +
> > +/**
> > + * e1000e_ptp_init - initialize PTP for devices which support it
> > + * @adapter: board private structure
> > + *
> > + * This function performs the required steps for enabling PTP support.
> > + * If PTP support has already been loaded it simply calls the cyclecounter
> > + * init routine and exits.
> > + **/
> > +void e1000e_ptp_init(struct e1000_adapter *adapter)
> > +{
> > + struct e1000_hw *hw = &adapter->hw;
> > +
> > + adapter->ptp_clock = NULL;
> > +
> > + if (!(adapter->flags & FLAG_HAS_HW_TIMESTAMP))
> > + return;
> > +
> > + adapter->ptp_clock_info = e1000e_ptp_clock_info;
> > +
> > + snprintf(adapter->ptp_clock_info.name,
> > + sizeof(adapter->ptp_clock_info.name), "%pm",
> > + adapter->netdev->perm_addr);
>
> ... putting any kind of address here. After some back and forth,
> discussing what the 'name' field should be, we decided on
>
> * @name: A short "friendly name" to identify the clock and to
> * help distinguish PHY based devices from MAC based ones.
> * The string is not meant to be a unique id.
>
> and most other drivers put the name of the driver here.
>
> Other than that, this new driver looks good to me. I'll try it out
> soon.
>
> Acked-by: Richard Cochran <richardcochran@gmail.com>

Thanks for the review Richard.