Re: [Hdaps-devel] Re: [ltp] IBM HDAPS Someone interested? (Userspace accelerometer viewer)

[Dave Hansen <dave@sr71.net>]

On Mon, 2005-07-11 at 22:09 +0200, Daniel Willmann wrote: 
> I have implemented an absolute input driver (aka joystick) on the
> basis of Dave's 0.02 version of the driver. I attached the diff to this
> mail or just get it from:
> http://thebe.orbit.homelinux.net/~alphaone/ibm_hdaps_joystick.tar.gz

Thanks for doing this.  A few comments below.

> +static u16 lastx = 0, lasty = 0;
> 
> 
> +static void ibm_hdaps_joystick_poll(unsigned long unused)
>  {
> -       int movex, movey;
> +       int posx, posy;
>         struct hdaps_accel_data accel_data;
>  
>         accelerometer_read(&accel_data);
> -       movex = restx - accel_data.x_accel;
> -       movey = resty - accel_data.y_accel;
> -       if (abs(movex) > ibm_hdaps_mousedev_fuzz)
> -               input_report_rel(&hdaps_idev, REL_Y, movex);
> -       if (abs(movey) > ibm_hdaps_mousedev_fuzz)
> -               input_report_rel(&hdaps_idev, REL_X, movey);
> +       posx = accel_data.x_accel;
> +       posy = accel_data.y_accel;
> +
> +       if (ibm_hdaps_joystick_reversex) 
> +               posy = hdaps_idev.absmax[ABS_X] + (hdaps_idev.absmin[ABS_X] - posy);
> +       if (ibm_hdaps_joystick_reversey)
> +               posx = hdaps_idev.absmax[ABS_Y] + (hdaps_idev.absmin[ABS_Y] - posx);

I'm not sure this is a good idea to do in the driver.  Reversing the
movement like this is something that surely should be going into the
input layer.  It might even be there already.

> +       if (abs(posx-lastx) > 0)
> +               input_report_abs(&hdaps_idev, ABS_Y, posx);
> +       if (abs(posy-lasty) > 0)
> +               input_report_abs(&hdaps_idev, ABS_X, posy);

Why do you suppress the events like this?  I think this is done inside
of the input layer already.  What happens if you don't do this?

> +       if (ibm_hdaps_joystick_registered)
> +       {
> +               snprintf(buf, 256, "enabled\n");
> +       }
> +       else
> +               snprintf(buf, 256, "disabled\n");

Please follow the coding style that Jesper and I have been working with:

        if () {
        	foo();
        } else {
        	bar();
        } 

> +static ssize_t
> +hdaps_joystick_enable_store(struct device *dev, const char * buf, size_t count)
> +{
> +       if ((strncmp(buf, "1", 1) == 0)&&(!ibm_hdaps_joystick_registered))
> +       {
> +               ibm_hdaps_joystick_enable();
> +       }
> +       else if ((strncmp(buf, "0", 1) == 0)&&(ibm_hdaps_joystick_registered))
> +       {
> +               ibm_hdaps_joystick_disable();
> +       }
> +       return count;
> +}

I think this makes the sysfs interface a bit counter-intuitive.  The
"cat joystick_enable" shows "enabled" or "disabled", but you have to
echo "0" and "1" into it to get it to do what you want.

> +hdaps_joystick_fuzzx_store(struct device *dev, const char * buf,
> size_t count)
>  {
> -       if (!calibrated)
> +       uint16_t temp;
> +       if (ibm_hdaps_joystick_registered)
>                 return -EINVAL;
> +       if (sscanf(buf, "%hu", &temp) == 1)
> +       {
> +               hdaps_idev.absfuzz[ABS_X] = temp;
> +       }
> +       return count;
> +}

Since that fuzz variable is actually part of the input layer, we should
probably have a conversation with the input people to see if they want a
generic, adjustable fuzz for all ABS input devices, instead of
duplicating it in a bunch of drivers.

BTW, I don't think there's a need for a different fuzzx and fuzzy.

-- Dave