On Thursday 16 December 2010, Richard Cochran wrote:

This patch adds support for adding and removing posix clocks. The
clock lifetime cycle is patterned after usb devices. Each clock is
represented by a standard character device. In addition, the driver
may optionally implemented custom character device operations.

The dynamic posix clocks do not yet do anything useful. This patch
merely provides some needed infrastructure.

Signed-off-by: Richard Cochran <richard.cochran-3mrvs1K0uXizZXS1Dc/lvw@public.gmane.org>

+struct posix_clock_fops {
+	int (*fasync)  (void *priv, int fd, struct file *file, int on);
+	int (*mmap)    (void *priv, struct vm_area_struct *vma);
+	int (*open)    (void *priv, fmode_t f_mode);
+	int (*release) (void *priv);
+	long (*ioctl)  (void *priv, unsigned int cmd, unsigned long arg);
+	long (*compat_ioctl) (void *priv, unsigned int cmd, unsigned long arg);
+	ssize_t (*read) (void *priv, uint flags, char __user *buf, size_t cnt);
+	unsigned int (*poll) (void *priv, struct file *file, poll_table *wait);
+};

Thanks for the change to a private ops structure. Three more
suggestions for this:

* I would recommend starting without a compat_ioctl operation if you can.
Just mandate that all ioctls for posix clocks are compatible and call
fops->ioctl from the posix_clock_compat_ioctl() function.
If you ever need compat_ioctl handling, it can still be added later.

* Instead of passing a void pointer, why not pass a struct posix_clock
pointer to the lower device and use container_of? That follows what
we do in other subsystems and allows you save one allocation, by
including the posix_clock into the specific clock struct like
ptp_clock.

+struct posix_clock_operations {
+	struct module *owner;
+	struct posix_clock_fops fops;
+	int  (*clock_adjtime)(void *priv, struct timex *tx);

You can easily combine the two structures and get rid of the extra
struct posix_clock_fops by moving its operations into
posix_clock_operations.

Looks really good otherwise.

	Arnd