Messages in this thread | | | Subject | Re: [Hdaps-devel] Re: [ltp] IBM HDAPS Someone interested? (Userspace accelerometer viewer) | From | Dave Hansen <> | Date | Tue, 12 Jul 2005 08:55:33 -0700 |
| |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |