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
-
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/
