Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 2 Feb 2020 18:14:29 -0800
From:   Richard Cochran <richardcochran@gmail.com>
To:     christopher.s.hall@intel.com
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        tglx@linutronix.de, hpa@zytor.com, mingo@redhat.com,
        x86@kernel.org, jacob.e.keller@intel.com, davem@davemloft.net,
        sean.v.kelley@intel.com
Subject: Re: [Intel PMC TGPIO Driver 2/5] drivers/ptp: Add PEROUT2 ioctl
 frequency adjustment interface
Message-ID: <20200203021429.GB3516@localhost>
References: <20191211214852.26317-1-christopher.s.hall@intel.com>
 <20191211214852.26317-3-christopher.s.hall@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191211214852.26317-3-christopher.s.hall@intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Dec 11, 2019 at 01:48:49PM -0800, christopher.s.hall@intel.com wrote:
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 93cc4f1d444a..8223f6f656dd 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -134,6 +134,8 @@ struct ptp_clock_info {
>  			  struct ptp_system_timestamp *sts);
>  	int (*getcrosststamp)(struct ptp_clock_info *ptp,
>  			      struct system_device_crosststamp *cts);
> +	int (*counttstamp)(struct ptp_clock_info *ptp,
> +			   struct ptp_event_count_tstamp *count);

KernelDoc missing.

As tglx said, it is hard to guess what this will be used for.  I would
appreciate a fuller explanation of the new callback in the commit log
message.

In general, please introduce a specific new API with an example of how
it is used.  In this series you have three new APIs,

   [Intel PMC TGPIO Driver 2/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface
   [Intel PMC TGPIO Driver 3/5] drivers/ptp: Add user-space input polling interface
   [Intel PMC TGPIO Driver 4/5] x86/tsc: Add TSC support functions to support ART driven Time-Aware GPIO

and then a largish driver using them all.

   [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

May I suggest an ordering more like:

[1/5] x86/tsc: Add TSC support functions to support ART...	(with forward explanation of the use case)
[2/5] drivers/ptp: Add PMC Time-Aware GPIO Driver		(without new bits)
[3/5] drivers/ptp: Add Enhanced handling of reserve fields	(okay as is)
[4/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface
[5/5] implement ^^^ in the driver
[6/5] drivers/ptp: Add user-space input polling interface
[7/5] implement ^^^ in the driver

> +/*
> + * Bits of the ptp_pin_desc.flags field:
> + */
> +#define PTP_PINDESC_EVTCNTVALID	(1<<0)

Is this somehow connected to ...

>  #define PTP_PEROUT_ONE_SHOT (1<<0)
> +#define PTP_PEROUT_FREQ_ADJ (1<<1)

... this?  If not, then they each deserve their own patch.

> @@ -164,10 +179,14 @@ struct ptp_pin_desc {
>  	 * PTP_EXTTS_REQUEST and PTP_PEROUT_REQUEST ioctls.
>  	 */
>  	unsigned int chan;
> +	/*
> +	 * Per pin capability flag
> +	 */
> +	unsigned int flags;

Please use 'capabilities' instead of 'flags'.

> +#define PTP_EVENT_COUNT_TSTAMP2 \
> +	_IOWR(PTP_CLK_MAGIC, 19, struct ptp_event_count_tstamp)

What is the connection between this, PTP_PINDESC_EVTCNTVALID, and
PTP_PEROUT_FREQ?

Thanks,
Richard