Messages in this thread | | | Date | Sun, 2 Feb 2020 18:14:29 -0800 | From | Richard Cochran <> | Subject | Re: [Intel PMC TGPIO Driver 2/5] drivers/ptp: Add PEROUT2 ioctl frequency adjustment interface |
| |
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
| |