Re: [patch] IBM HDAPS accelerometer driver.

Dmitry Torokhov
2005-08-26T19:27:42Z

On 8/26/05, Robert Love <rml@novell.com> wrote:
> +/* device class stuff */
> +
> +static DECLARE_COMPLETION(hdaps_obj_is_free);
> +static void hdaps_release_dev(struct device *dev)
> +{
> + complete(&hdaps_obj_is_free);
> +}
> +

What this completion is used for? I don't see any other references to it.

> +
> +static void hdaps_mousedev_poll(unsigned long unused)
> +{
> + int movex, movey, x, y, ret;
> +
> + ret = accelerometer_read_pair(HDAPS_PORT_XPOS, HDAPS_PORT_YPOS, &x, &y);
> + if (unlikely(ret))
> + return;
> +
> + movex = rest_x - x;
> + movey = rest_y - y;
> + if (abs(movex) > hdaps_mousedev_threshold)
> + input_report_rel(&hdaps_idev, REL_Y, movex);
> + if (abs(movey) > hdaps_mousedev_threshold)
> + input_report_rel(&hdaps_idev, REL_X, movey);
> + input_sync(&hdaps_idev);
> +
> + mod_timer(&hdaps_poll_timer, jiffies + msecs_to_jiffies(hdaps_poll_ms));
> +}
> +

I'd rather you used absolute coordinates and set up
hdaps_idev->absfuzz to do the filtering.

> +static ssize_t hdaps_mousedev_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int enable;
> +
> + if (sscanf(buf, "%d\n", &enable) != 1)
> + return -EINVAL;
> +
> + if (enable == 1)
> + hdaps_mousedev_enable();
> + else if (enable == 0)
> + hdaps_mousedev_disable();
> +
> + return count;
> +}
> +

This is racy - 2 threads can try to do this simultaneously.

> +
> + device_create_file(&hdaps_plat_dev.dev, &dev_attr_position);
> + device_create_file(&hdaps_plat_dev.dev, &dev_attr_variance);
> + device_create_file(&hdaps_plat_dev.dev, &dev_attr_temp);
> + device_create_file(&hdaps_plat_dev.dev, &dev_attr_calibrate);
> + device_create_file(&hdaps_plat_dev.dev, &dev_attr_mousedev);
> + device_create_file(&hdaps_plat_dev.dev, &dev_attr_mousedev_threshold);
> + device_create_file(&hdaps_plat_dev.dev, &dev_attr_mousedev_poll_ms);
> +

What about using sysfs_attribute_group?

--
Dmitry