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

> +

> +/**
> + * 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.

> +
> +	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.

> +
> +	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.

> +
> +	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.

> +
> +			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?

> +
> +			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.

> +		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.

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