Re: [PATCHv4] input: ROHM BU21013 touch panel controller support
From: Henrik Rydberg <hidden>
Date: 2010-09-29 09:34:39
Hi Naveen, Many thanks for the efforts and changes. Some comments below, plus I have attached an untested patch with the code I had in mind when writing my last comments. Cheers, Henrik
+
quoted hunk ↗ jump to hunk
+/** + * bu21013_do_touch_report(): Get the touch co-ordinates + * @data: bu21013_ts_data structure pointer + * + * Get the touch co-ordinates from touch sensor registers and writes + * into device structure and returns integer. + */ +static int bu21013_do_touch_report(struct bu21013_ts_data *data) +{ + u8 buf[LENGTH_OF_BUFFER]; + bool finger1_valid; + bool finger2_valid; + unsigned int finger1_pos[2]; + unsigned int finger2_pos[2]; + int number_of_active_x_sensors; + int number_of_active_y_sensors; + int total_number_of_active_sensors; + int finger_down_count = 0; + int retval = 0; + int i = 0; + int j = 0; + int retry_count = I2C_RETRY_COUNT; + bool rotate[MAX_FINGERS]; + static bool previous_press_reported;
The number of variables alone suggests something could be broken out of this function.
quoted hunk ↗ jump to hunk
+ + if (data == NULL) + return -EINVAL; + + do { + retval = i2c_smbus_read_i2c_block_data + (data->client, BU21013_SENSORS_BTN_0_7_REG, + LENGTH_OF_BUFFER, buf); + retry_count--; + if ((retval < LENGTH_OF_BUFFER) && (!retry_count)) + return -EINVAL; + } while (retval < LENGTH_OF_BUFFER);
In the attached patch, the above code has been broken out.
quoted hunk ↗ jump to hunk
+ + number_of_active_x_sensors = hweight32(buf[0] & + BU21013_SENSORS_EN_0_7); + number_of_active_y_sensors = hweight32( + ((buf[1] & BU21013_SENSORS_EN_8_15) | + ((buf[2] & BU21013_SENSORS_EN_16_23) << SHIFT_8)) >> SHIFT_2); + if (((number_of_active_x_sensors != 0) && + (number_of_active_y_sensors == 0)) || + ((number_of_active_x_sensors == 0) && + (number_of_active_y_sensors != 0))) + return 0;
Using boolean variables, the above can be written quite compactly, see attached patch.
quoted hunk ↗ jump to hunk
+ + total_number_of_active_sensors = + number_of_active_x_sensors + number_of_active_y_sensors; + + if (total_number_of_active_sensors) { + while (i < 2) { + finger1_pos[i] = buf[j + 3] << SHIFT_2 | + (buf[j + 4] & MASK_BITS); + finger2_pos[i] = buf[j + 7] << SHIFT_2 | + (buf[j + 8] & MASK_BITS); + + finger1_valid = (finger1_pos[i] != 0) ? true : false; + finger2_valid = (finger2_pos[i] != 0) ? true : false;
When talking about arrays, I really meant an array of fingers, rather than an array of x and y.
quoted hunk ↗ jump to hunk
+ + if ((!finger1_valid) && (!finger2_valid)) { + return 0; + } else if ((!finger1_valid) && (finger2_valid)) { + finger1_valid = finger2_valid; + finger2_valid = false; + finger1_pos[i] = finger2_pos[i]; + finger2_pos[i] = 0; + } + j += 2; + i++; + } + + if (finger1_valid) { + if (data->chip->x_flip) + finger1_pos[0] = data->chip->touch_x_max - + finger1_pos[0]; + if (data->chip->y_flip) + finger1_pos[1] = data->chip->touch_y_max - + finger1_pos[1]; + finger_down_count++; + } + + if (finger2_valid && finger1_valid) { + if ((abs(finger2_pos[0] - finger1_pos[0]) < DELTA_MIN) + || (abs(finger2_pos[1] - finger1_pos[1]) < DELTA_MIN)) + goto report;
As far as I can see, the only thing that happens when two fingers are close to each other is that the coordinate translation is not performed. Is that correct?
quoted hunk ↗ jump to hunk
+ + if (data->chip->x_flip) + finger2_pos[0] = data->chip->touch_x_max - + finger2_pos[0]; + if (data->chip->y_flip) + finger2_pos[1] = data->chip->touch_y_max - + finger2_pos[1]; + finger_down_count++; + } + } + +report: + if ((finger_down_count <= 0) && (previous_press_reported)) { + /* report pen up to input subsystem */ + input_report_key(data->in_dev, BTN_TOUCH, 0); + input_report_abs(data->in_dev, ABS_MT_TOUCH_MAJOR, 0); + input_report_abs(data->in_dev, ABS_MT_TOUCH_MINOR, 0);
The touch major/minor are for finger width, and should not be reported when not available from the device.
quoted hunk ↗ jump to hunk
+ input_mt_sync(data->in_dev); + input_sync(data->in_dev); + previous_press_reported = false; + } else if (finger_down_count > 0) { + /* report pen down to input subsystem */ + input_report_abs(data->in_dev, ABS_X, finger1_pos[0]); + input_report_abs(data->in_dev, ABS_Y, finger1_pos[1]); + input_report_key(data->in_dev, BTN_TOUCH, 1); + + rotate[0] = (finger1_pos[0] > finger1_pos[1]) ? 1 : 0; + input_report_abs(data->in_dev, ABS_MT_TOUCH_MAJOR, + max(finger1_pos[0], finger1_pos[1])); + input_report_abs(data->in_dev, ABS_MT_TOUCH_MINOR, + min(finger1_pos[0], finger1_pos[1])); + input_report_abs(data->in_dev, ABS_MT_ORIENTATION, + rotate[0]);
These lines report touch major/minor as a bounding box, which violates the semantics of those fields.
quoted hunk ↗ jump to hunk
+ input_report_abs(data->in_dev, ABS_MT_POSITION_X, + finger1_pos[0]); + input_report_abs(data->in_dev, ABS_MT_POSITION_Y, + finger1_pos[1]); + input_mt_sync(data->in_dev); + if (finger_down_count > 1) { + rotate[1] = (finger2_pos[1] > finger2_pos[1]) ? 1 : 0; + input_report_abs(data->in_dev, ABS_MT_TOUCH_MAJOR, + max(finger2_pos[0], finger2_pos[1])); + input_report_abs(data->in_dev, ABS_MT_TOUCH_MINOR, + min(finger2_pos[0], finger2_pos[1])); + input_report_abs(data->in_dev, ABS_MT_ORIENTATION, + rotate[1]); + input_report_abs(data->in_dev, ABS_MT_POSITION_X, + finger2_pos[0]); + input_report_abs(data->in_dev, ABS_MT_POSITION_Y, + finger2_pos[1]); + input_mt_sync(data->in_dev); + } + input_sync(data->in_dev); + previous_press_reported = true;
In a previous comment about the previous_press_reported, what I really meant was that it is not needed at all, since the state is already handled in the input core. Please see the attached patch. Thanks, Henrik
Attachments
- 0001-input-ROHM-BU21013-touch-panel-controller-support.patch [text/x-diff] 21544 bytes · preview