Hi Richard, Thank you for your detailed feedback. I'm working on it now, but I feel it will take a little extra time to complete. Therefor I'm planning to remove PTP support from this patch series, and resubmit it in a new patch later.
I also have a few questions below. > -----Original Message----- > From: Richard Cochran [mailto:richardcoch...@gmail.com] > Sent: Thursday, July 12, 2018 11:32 PM > To: Bryan Whitehead - C21958 <bryan.whiteh...@microchip.com> > Cc: da...@davemloft.net; netdev@vger.kernel.org; UNGLinuxDriver > <unglinuxdri...@microchip.com> > Subject: Re: [PATCH v2 net-next 9/9] lan743x: Add PTP support > ... > > + if (cleanup) { > > + lan743x_ptp_unrequest_tx_timestamp(tx->adapter); > > + dev_kfree_skb(buffer_info->skb); > > + } else { > > + lan743x_ptp_tx_timestamp_skb(tx->adapter, > > + buffer_info->skb, > > + (buffer_info->flags & > > + > TX_BUFFER_INFO_FLAG_IGNORE_SYNC) > > + != 0); > > This is poor coding style. Please find a better way. Can you clarify what is poor and what would be better? For example, should I change "X != 0" to "X ? true : false". > > +#ifdef CONFIG_PTP_1588_CLOCK > > +static int lan743x_ptp_enable_pps(struct lan743x_adapter *adapter) { > > + struct lan743x_ptp *ptp = &adapter->ptp; > > + u32 current_seconds = 0; > > + u32 target_seconds = 0; > > + u32 general_config = 0; > > + int result = -ENODEV; > > + int pps_bit = 0; > > So this function is really *not* implementing the PTP_CLK_REQ_PPS feature > but rather the PTP_CLK_REQ_PEROUT with a period of once per second. > > PTP_CLK_REQ_PPS means placing a PPS event into the kernel's "hardpps" > subsystem by calling ptp_clock_event(). > > I'm sorry this isn't really documented. I should fix that. > > If you HW can output arbitrary signals, then you should implement > PTP_CLK_REQ_PEROUT. In any case, you shouldn't advertise the > ptp_clock_info.pps capability. So you mean PPS is not intended to generate a physical signal? It is only intended to call ptp_clock_event? I can configure the hardware to generate an interrupt each second and then call ptp_clock_event. Would that satisfy the pps requirements? Regarding PTP_CLK_REQ_PEROUT. Is that intended for physical signals? Thanks, Bryan
