From mboxrd@z Thu Jan  1 00:00:00 1970
From: Richard Cochran <richardcochran@gmail.com>
Subject: Re: [PATCH v2 net-next 9/9] lan743x: Add PTP support
Date: Wed, 18 Jul 2018 13:55:54 -0700
Message-ID: <20180718205554.odob5pnzmct6klho@localhost>
References: <1531422306-6547-1-git-send-email-Bryan.Whitehead@microchip.com>
 <1531422306-6547-10-git-send-email-Bryan.Whitehead@microchip.com>
 <20180713033135.dfatbghlpeswm7g6@localhost>
 <90A7E81AE28BAE4CBDDB3B35F187D2644073B039@CHN-SV-EXMX02.mchp-main.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: davem@davemloft.net, netdev@vger.kernel.org,
        UNGLinuxDriver@microchip.com
To: Bryan.Whitehead@microchip.com
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pg1-f176.google.com ([209.85.215.176]:45570 "EHLO
        mail-pg1-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726985AbeGRVfg (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 18 Jul 2018 17:35:36 -0400
Received: by mail-pg1-f176.google.com with SMTP id f1-v6so2524264pgq.12
        for <netdev@vger.kernel.org>; Wed, 18 Jul 2018 13:55:57 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D2644073B039@CHN-SV-EXMX02.mchp-main.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Wed, Jul 18, 2018 at 08:04:08PM +0000, Bryan.Whitehead@microchip.com wrote:
> 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.

Ok.

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

Look at this:
		lan743x_ptp_tx_timestamp_skb(tx->adapter,
					     buffer_info->skb,
					     (buffer_info->flags &
					     TX_BUFFER_INFO_FLAG_IGNORE_SYNC)
					     != 0);

Can't you reduce

	(buffer_info->flags & TX_BUFFER_INFO_FLAG_IGNORE_SYNC) != 0

into a local variable:

		lan743x_ptp_tx_timestamp_skb(tx->adapter, buffer_info->skb, xyz);
?

> So you mean PPS is not intended to generate a physical signal?

Yes.

> It is only intended to call ptp_clock_event?

Yes.

> I can configure the hardware to generate an interrupt each second and then call 
> ptp_clock_event. Would that satisfy the pps requirements?

Yes.
 
> Regarding PTP_CLK_REQ_PEROUT. Is that intended for physical signals?

Yes.

Thanks,
Richard