Hi,Thanks
your code looks very nice and clean, only few comments, see below
How do I handle signals in kthreads? I looked at quite a few kthreads in the drivers sources, but couldn't+static int mdps_joystick_kthread(void *data)
+{
+ int x = 0, y = 0, z = 0;
+
+ while (!kthread_should_stop()) {
+ if (input_3d) {
+ mdps_get_xyz(mdps.device->handle, &x, &y, &z);
+ input_report_abs(mdps.idev, ABS_Z, z - mdps.zcalib);
+ } else
+ mdps_get_xy(mdps.device->handle, &x, &y);
+
+ input_report_abs(mdps.idev, ABS_X, x - mdps.xcalib);
+ input_report_abs(mdps.idev, ABS_Y, y - mdps.ycalib);
+
+ input_sync(mdps.idev);
+
+ try_to_freeze();
+ msleep_interruptible(MDPS_POLL_INTERVAL);
+ }
what if you get a signal? you probably at least want to handle that
somehow. Also, waking up every 30 miliseconds is going to suck up
power ... but that might not be avoidable I suppose if you have to poll
the hardware.
I had some problems with that before (2.6.19 I think), but in 2.6.21-rc4 it seems OK, so I will allow shared interrupts in my next version.+
+ atomic_set(&mdps.count, 0);
+
+ ret = request_irq(mdps.irq, mdps_irq, 0, "mdps", mdps_irq);
don't you want to allow shared interrupts?
You are right. I missed that after refactoring the code before.+ if (ret) {
+ printk(KERN_ERR "mdps: IRQ%d allocation failed\n", mdps.irq);
+ return -ENODEV;
+ }
wouldn't you want to inc the atomic in this case?
Done.+static ssize_t mdps_misc_read(struct file *file, char __user *buf,
+ size_t count, loff_t *pos)
+{
+ DECLARE_WAITQUEUE(wait, current);
+ u32 data;
+ ssize_t retval = count;
+
+ if (count != sizeof(u32))
+ return -EINVAL;
+
+ add_wait_queue(&mdps.misc_wait, &wait);
+ for (;;) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ data = atomic_xchg(&mdps.count, 0);
+ if (data)
+ break;
+
+ if (file->f_flags & O_NONBLOCK) {
+ retval = -EAGAIN;
+ goto out;
+ }
+
+ if (signal_pending(current)) {
+ retval = -ERESTARTSYS;
+ goto out;
+ }
+
+ schedule();
+ }
+
+ if (copy_to_user(buf, &data, sizeof(data)))
I'm not entirely sure you want to go into copy_to_user() with a
TASK_INTERRUPTIBLE state, I would feel a lot better if you did an
explicit __set_current_state(TASK_RUNNING) just before this.
The problem is that if I use interrupts for mouse-like behavior, I have no way of knowing (unless I do some hacks to remember the last position and see if it changed more than+ retval = -EFAULT;
+
+out:
+ set_current_state(TASK_RUNNING);
.. which you do here anyway
mdps_get_resource(struct acpi_resource *resource, void *context)
+{
+ if (resource->type == ACPI_RESOURCE_TYPE_EXTENDED_IRQ) {
+ struct acpi_resource_extended_irq *irq;
+ u32 *device_irq = context;
+
+ irq = &resource->data.extended_irq;
+ *device_irq = irq->interrupts[0];
eh wait.. if this thing gives you an interrupt.. why do you need to poll
every 30 msec? am I missing something?