Hi Naveen,
Looks much better now, thanks. Some comments below.
[...]
> +struct bu21013_ts_data {
> + struct i2c_client *client;
> + wait_queue_head_t wait;
> + bool touch_stopped;
> + const struct bu21013_platform_device *chip;
> + struct input_dev *in_dev;
> + unsigned int intr_pin;
> + signed short x_pos[2];
> + signed short y_pos[2];
> + bool previous_press_reported;
The MT state here (x_pos, y_pos, previous_press_reported) could, in principle,
be simplified away, since the type A protocol is stateless.
[...]
> +static int bu21013_do_touch_report(struct bu21013_ts_data *data)
> +{
> + u8 buf[LENGTH_OF_BUFFER];
> + bool finger1_valid = true;
> + bool finger2_valid = true;
> + unsigned int finger1_pos_x;
> + unsigned int finger1_pos_y;
> + unsigned int finger2_pos_x = 0;
> + unsigned int finger2_pos_y = 0;
These could be arrays instead.
> + int number_of_active_x_sensors;
> + int number_of_active_y_sensors;
> + int total_number_of_active_sensors;
> + int finger_down_count = 0;
> + int ret = 0;
> + int i;
> + int retry_count = I2C_RETRY_COUNT;
> +
> + if (data == NULL)
> + return -EINVAL;
> +
> + do {
> + ret = i2c_smbus_read_i2c_block_data
> + (data->client, BU21013_SENSORS_BTN_0_7_REG,
> + LENGTH_OF_BUFFER, buf);
> + retry_count--;
> + if ((ret < LENGTH_OF_BUFFER) && (!retry_count))
> + return -EINVAL;
> + } while (ret < LENGTH_OF_BUFFER);
> +
> + 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;
So these should really be completely ignored, not reported as touch up? Any
chance the above is the last event seen before a period of no activity?
> + total_number_of_active_sensors =
> + number_of_active_x_sensors + number_of_active_y_sensors;
> +
> + if (total_number_of_active_sensors) {
> + finger1_pos_x = buf[3] << SHIFT_2 | (buf[4] & MASK_BITS);
> + finger1_pos_y = buf[5] << SHIFT_2 | (buf[6] & MASK_BITS);
> + finger2_pos_x = buf[7] << SHIFT_2 | (buf[8] & MASK_BITS);
> + finger2_pos_y = buf[9] << SHIFT_2 | (buf[10] & MASK_BITS);
> +
> + if ((finger1_pos_x == 0) || (finger1_pos_y == 0))
> + finger1_valid = false;
> +
> + if ((finger2_pos_x == 0) || (finger2_pos_y == 0))
> + finger2_valid = false;
> +
> + if ((!finger1_valid) && (!finger2_valid)) {
> + return 0;
> + } else if ((!finger1_valid) && (finger2_valid)) {
> + finger1_valid = finger2_valid;
> + finger1_pos_x = finger2_pos_x;
> + finger1_pos_y = finger2_pos_y;
> + finger2_valid = false;
> + finger2_pos_x = 0;
> + finger2_pos_y = 0;
> + }
An array and a for-loop here would remove some more lines of code.
> +
> + if (finger2_valid) {
> + ret = bu21013_verify_delta(finger1_pos_x,
> + finger1_pos_y, finger2_pos_x, finger2_pos_y);
> + if (!ret)
> + goto report;
Can finger1 be valid here?
> + bu21013_touch_calc(data, finger2_pos_x,
> + finger2_pos_y, finger_down_count);
> + finger_down_count++;
The MT report could in principle be made right here.
> + }
> +
> + if (finger1_valid) {
> + bu21013_touch_calc(data, finger1_pos_x,
> + finger1_pos_y, finger_down_count);
> + finger_down_count++;
Same here, no real need to store it first.
> + }
> + }
> +
> +report:
> + if ((finger_down_count <= 0) && (data->previous_press_reported)) {
> + /* report pen up to input subsystem */
> + input_report_key(data->in_dev, BTN_TOUCH, 0);
> + input_mt_sync(data->in_dev);
> + input_sync(data->in_dev);
> + data->previous_press_reported = false;
The previous_press_reported state is already accounted for in the input core,
and could be simplified away.
> + } else if (finger_down_count > 0) {
> + /* report pen down to input subsystem */
> + input_report_abs(data->in_dev, ABS_X, data->x_pos[0]);
> + input_report_abs(data->in_dev, ABS_Y, data->y_pos[0]);
> + input_report_key(data->in_dev, BTN_TOUCH, 1);
Out of curiosity, is there any guarantee that pos[0] stays with the initial
finger as the second finger is added?
> +
> + for (i = 0; i < finger_down_count; i++) {
> + input_report_abs(data->in_dev, ABS_MT_POSITION_X,
> + data->x_pos[i]);
> + input_report_abs(data->in_dev, ABS_MT_POSITION_Y,
> + data->y_pos[i]);
> + input_mt_sync(data->in_dev);
> + }
> + input_sync(data->in_dev);
> + data->previous_press_reported = true;
> + }
> +
> + return ret;
> +}
Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
