Re: [PATCH 2.6.21-rc4] hwmon: HP Mobile Data Protection System 3D ACPI driver

[Arjan van de Ven <arjan@infradead.org>]

Hi,

your code looks very nice and clean, only few comments, see below

> +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.

> +static int mdps_misc_open(struct inode *inode, struct file *file)
> +{
> +	int ret;
> +
> +	if (!atomic_dec_and_test(&mdps.available)) {
> +		atomic_inc(&mdps.available);
> +		return -EBUSY; /* already open */
> +	}
I often get a bit nervous at seeing such "open me once" code, but it
might well be ok ... I'll let others comment

> +
> +	atomic_set(&mdps.count, 0);
> +
> +	ret = request_irq(mdps.irq, mdps_irq, 0, "mdps", mdps_irq);

don't you want to allow shared interrupts?

> +	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?



> +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.

> +		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?