On Monday 16 August 2010, Richard Cochran wrote:

This patch adds an infrastructure for hardware clocks that implement
IEEE 1588, the Precision Time Protocol (PTP). A class driver offers a
registration method to particular hardware clock drivers. Each clock is
exposed to user space as a character device with ioctls that allow tuning
of the PTP clock.

Have you considered integrating the subsystem into the Posix clock/timer
framework?

I can't really tell from reading the source if this is possible or
not, but my feeling is that if it can be done, that would be a much
nicer interface. We already have clock_gettime/clock_settime/
timer_settime/... system calls, and while you'd need to add another
clockid and some syscalls, my feeling is that it will be more
usable in the end.

+static const struct file_operations ptp_fops = {
+       .owner          = THIS_MODULE,
+       .ioctl          = ptp_ioctl,
+       .open           = ptp_open,
+       .poll           = ptp_poll,
+       .read           = ptp_read,
+       .release        = ptp_release,
+};

.ioctl is gone, you have to use .unlocked_ioctl and make sure that
your ptp_ioctl function can handle being called concurrently.

You should also add a .compat_ioctl function, ideally one that
points to ptp_ioctl so you don't have to list every command as
being compatible. Right now, some commands are incompatible,
which means you need more changes to the interface.

+struct ptp_clock_timer {
+       int alarm_index;       /* Which alarm to query or configure. */
+       int signum;            /* Requested signal. */
+       int flags;             /* Zero or TIMER_ABSTIME, see TIMER_SETTIME(2) */
+       struct itimerspec tsp; /* See TIMER_SETTIME(2) */
+};

This data structure is incompatible between 32 and 64 bit user space,
so you would need a compat_ioctl() function to convert between the
two formats. Better define the structure in a compatible way, avoiding
variable-length fields and implicit padding.

+struct ptp_clock_request {
+       int type;  /* One of the ptp_request_types enumeration values. */
+       int index; /* Which channel to configure. */
+       struct timespec ts; /* For period signals, the desired period. */
+       int flags; /* Bit field for PTP_ENABLE_FEATURE or other flags. */
+};

Same problem here, timespec is either 64 or 128 bits long and has different
alignment.

+struct ptp_extts_event {
+       int index;
+       struct timespec ts;
+};

here too.

+#define PTP_CLOCK_VERSION 0x00000001
+
+#define PTP_CLK_MAGIC '='
+
+#define PTP_CLOCK_APIVERS _IOR (PTP_CLK_MAGIC, 1, __u32)

Try avoiding versioned ABIs. It's better to just add new ioctls
or syscalls when you need some extra functionality and leave the
old ones around.

+#define PTP_CLOCK_ADJTIME _IOW (PTP_CLK_MAGIC, 3, struct timespec)
+#define PTP_CLOCK_GETTIME _IOR (PTP_CLK_MAGIC, 4, struct timespec)
+#define PTP_CLOCK_SETTIME _IOW (PTP_CLK_MAGIC, 5, struct timespec)

These are also incompatible.

	Arnd