multi-touch-dev team mailing list archive

[Chase Douglas <chase.douglas@xxxxxxxxxxxxx>]

On Thu, 2010-08-19 at 23:42 +0200, Henrik Rydberg wrote:
> Hi Chase,
> 
> First patch is just fine. Some comments on the second patch below.
> 
> -#define TOUCH_REPORT_ID   0x29
> +#define MT_TOUCH_REPORT_ID   0x28
> +#define MM_TOUCH_REPORT_ID   0x29
> 
> It is very hard to read the difference between MT and MM like this. Different
> names would clarify a lot.

I've changed the names to be MOUSE_REPORT_ID and TRACKPAD_REPORT_ID.

> +		touch_minor = tdata[4];
> +		down = (tdata[7] & TOUCH_STATE_MASK) != TOUCH_STATE_NONE;
> +	} else {
> +		id = (tdata[7] << 2 | tdata[6] >> 6) & 0xf;
> +		x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
> 
> This else, and several other in the code, could be "if MAGICTRACKPAD then..."
> instead, for clarity.

Instead of adding an unnecessary if clause to the code, I've commented
all the else clauses to be clear.

> +	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> +		id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
> +		x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
> +		y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
> +		size = tdata[5] & 0x3f;
> +		orientation = (tdata[6] >> 2) - 32;
> +		touch_major = tdata[3];
> +		touch_minor = tdata[4];
> +		down = (tdata[7] & TOUCH_STATE_MASK) != TOUCH_STATE_NONE;
> 
> I *think* this code is equivalent to what was there, but it is really hard to
> say for sure. Please do not modify existing code path like this, at least not in
> the same patch.

I've split this out into a separate commit.

> -	/* Generate the input events for this touch. */
> -	if (report_touches && down) {
> -		int orientation = (misc >> 10) - 32;
> -
> +	if (down) {
>  		msc->touches[id].down = 1;
> +		if (msc->single_touch_id == -1)
> +			msc->single_touch_id = id;
> +	} else if (msc->single_touch_id == id)
> +		msc->single_touch_id = -2;
> 
> +	/* Generate the input events for this touch. */
> +	if (report_touches && down) {
> 
> Same here - use a separate patch for this if really needed. Just think of how to
> differentiate your additions from existing code through a bisect.

The above is actually changes required to implement single touch
emulation for the trackpad.

> -	int x, y, ts, ii, clicks, last_up;
> +	int x = 0;
> +	int y = 0;
> +	int ts;
> +	int ii;
> +	int clicks = 0;
> +	int last_up;
> 
> Why are those changed?

I needed to initialize the x, y, and clicks vars to 0, but I've cleaned
this up style wise.

> -	case TOUCH_REPORT_ID:
> +	case MT_TOUCH_REPORT_ID:
> +		/* Expect four bytes of prefix, and N*9 bytes of touch data. */
> +		if (size < 4 || ((size - 4) % 9) != 0)
> +			return 0;
> 
> This code returns from a function which has a functional continuation - a
> statement that this is indeed intended would be good.

I don't understand what you mean. Can you clarify what's odd here?

> -		if (report_touches) {
> -			last_up = 1;
> -			for (ii = 0; ii < ARRAY_SIZE(msc->touches); ii++) {
> -				if (msc->touches[ii].down) {
> 
> unrelated change, please split.

Needed for single touch emulation.

> -	magicmouse_emit_buttons(msc, clicks & 3);
> -	input_report_rel(input, REL_X, x);
> -	input_report_rel(input, REL_Y, y);
> +	if ((data[0] == MM_TOUCH_REPORT_ID || data[0] == MT_TOUCH_REPORT_ID)) {
> +		last_up = 1;
> 
> ditto.

ditto. (Needed for single touch emulation :)

> -static struct feature features[] = {
> +static struct feature mm_features[] = {
>  	{ { 0xd7, 0x01 }, 2 },
>  	{ { 0xf8, 0x01, 0x32 }, 3 }
>  };
> 
> why not do this part in the first patch? and name, please...

Fixed.

> -	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE),
> -		.driver_data = 0 },
> +	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
> +		USB_DEVICE_ID_APPLE_MAGICMOUSE), .driver_data = 0 },
> +	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
> +		USB_DEVICE_ID_APPLE_MAGICTRACKPAD), .driver_data = 0 },
> 
> Non-functional change of unrelated code.

The original code broke style standards because it was over 80 lines. My
impression is that most maintainers are fine fixing one or two style
issues in a larger patch. This is pretty straight forward and simple, so
I think this is reasonable.

> That was it for now. All in all, this is excellent work. With the comments
> adressed, it seems ready for submission. Thanks!

Thanks for the review!

-- Chase