Re: [patch] IBM HDAPS accelerometer driver.

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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