Arjan van de Ven wrote:
Hi,
your code looks very nice and clean, only few comments, see below
Thanks
+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.
How do I handle signals in kthreads? I looked at quite a few kthreads in
the drivers sources, but couldn't
find even one example that has any explicit handling of signals.
+
+ atomic_set(&mdps.count, 0);
+
+ ret = request_irq(mdps.irq, mdps_irq, 0, "mdps", mdps_irq);
don't you want to allow shared interrupts?
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.
+ 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?
You are right. I missed that after refactoring the code before.
+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.
Done.
+ 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?
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
some threshold, or something like that - I'm not sure that at this point
I can do that reliably)
what was the source of the interrupt - was that motion or free-fall
event. This means I can't both use it as joystick and detect when
the laptop is falling. That's why I use the interrupts for free-fall
events (as it was intended for this chip in the laptop context), and use
polling for mouse like behavior. This is the same approach
as other accelerometer drivers in the kernel (hdaps and ams).
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
