Re: [PATCH] Input: goodix - use generic touchscreen_properties

[Marcin Niestroj <m.niestroj@grinn-global.com>]

Hi Bastien,

On 25.10.2017 21:42, Bastien Nocera wrote:
> Hey Marcin,
> 
> On Wed, 2017-10-25 at 13:32 +0200, Marcin Niestroj wrote:
>> Use touchscreen_properties structure instead of implementing all
>> properties by our own. It allows to reuse generic code for parsing
>> device-tree properties (which was implemented manually in the driver
>> for now). Additionally, it allows us to report events using generic
>> touchscreen_report_pos(), which automatically handles inverted and
>> swapped axes.
>>
>> There was also bug in driver in touch position calculation, when axes
>> were configured as inverted and swapped in the same time. This is
>> however fixed now, by using touchscreen_report_pos() function, which
>> handles inversion+swapping correctly.
> <snip>
>> @@ -579,6 +568,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
>>   static void goodix_read_config(struct goodix_ts_data *ts)
>>   {
>>   	u8 config[GOODIX_CONFIG_MAX_LENGTH];
>> +	int x_max, y_max;
>>   	int error;
>>   
>>   	error = goodix_i2c_read(ts->client, ts->chip->config_addr,
>> @@ -587,37 +577,34 @@ static void goodix_read_config(struct goodix_ts_data *ts)
>>   		dev_warn(&ts->client->dev,
>>   			 "Error reading config (%d), using defaults\n",
>>   			 error);
>> -		ts->abs_x_max = GOODIX_MAX_WIDTH;
>> -		ts->abs_y_max = GOODIX_MAX_HEIGHT;
>> -		if (ts->swapped_x_y)
>> -			swap(ts->abs_x_max, ts->abs_y_max);
>> +		x_max = GOODIX_MAX_WIDTH;
>> +		y_max = GOODIX_MAX_HEIGHT;
> 
> When do you swap those out if necessary?

Swapping axes is implemented in of_touchscreen.c. This includes swapping
during event reporting, as well as during touchscreen width and height
reporting during initialization.

> 
>>   		ts->int_trigger_type = GOODIX_INT_TRIGGER;
>>   		ts->max_touch_num = GOODIX_MAX_CONTACTS;
>> -		return;
>> +		goto input_set_params;
>>   	}occurs
>>   
>> -	ts->abs_x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
>> -	ts->abs_y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
>> -	if (ts->swapped_x_y)
>> -		swap(ts->abs_x_max, ts->abs_y_max);
>> +	x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
>> +	y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
>>   	ts->int_trigger_type = config[TRIGGER_LOC] & 0x03;
>>   	ts->max_touch_num = config[MAX_CONTACTS_LOC] & 0x0f;
>> -	if (!ts->abs_x_max || !ts->abs_y_max || !ts->max_touch_num) {
>> +	if (!x_max || !y_max || !ts->max_touch_num) {
>>   		dev_err(&ts->client->dev,
>>   			"Invalid config, using defaults\n");
>> -		ts->abs_x_max = GOODIX_MAX_WIDTH;
>> -		ts->abs_y_max = GOODIX_MAX_HEIGHT;
>> -		if (ts->swapped_x_y)
>> -			swap(ts->abs_x_max, ts->abs_y_max);
>> +		x_max = GOODIX_MAX_WIDTH;
>> +		y_max = GOODIX_MAX_HEIGHT;
>>   		ts->max_touch_num = GOODIX_MAX_CONTACTS;
>>   	}
>>   
>> -	if (dmi_check_system(rotated_screen)) {
>> -		ts->inverted_x = true;
>> -		ts->inverted_y = true;
>> -		dev_dbg(&ts->client->dev,
>> -			 "Applying '180 degrees rotated screen' quirk\n");
>> -	}
>> +input_set_params:
>> +	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
>> +			0, x_max - 1, 0, 0);
>> +	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
>> +			0, y_max - 1, 0, 0);
> 
> This is x_max - 1, and y_max - 1, when the original code used x_max and
> y_max. Can you mention this fix in the changelog as well, or better,
> split it off in a separate fix?

You are right, I should mention this. I've searched through several
touchscreen drivers and I think this is how it should be. Am I right?
If you confirm, I will split it off in next patch version.

Should we split fixing inverted+swapped axes as well? This is fixed by
this patch right now, by reusing code in of_touchscreen.c. Below is a
patch, that fixes inversion+swapping, by not using of_touchscreen.c.
Should I add that to the patch set, so it could be backported to stable
releases?

diff --git a/drivers/input/touchscreen/goodix.c 
b/drivers/input/touchscreen/goodix.c
index d9e1dc06bc23..04ca06d38ca9 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -246,12 +246,18 @@ static void goodix_ts_report_touch(struct 
goodix_ts_data *ts, u8 *coor_data)
         int input_w = get_unaligned_le16(&coor_data[5]);

         /* Inversions have to happen before axis swapping */
-       if (ts->inverted_x)
-               input_x = ts->abs_x_max - input_x;
-       if (ts->inverted_y)
-               input_y = ts->abs_y_max - input_y;
-       if (ts->swapped_x_y)
+       if (!ts->swapped_x_y) {
+               if (ts->inverted_x)
+                       input_x = ts->abs_x_max - input_x;
+               if (ts->inverted_y)
+                       input_y = ts->abs_y_max - input_y;
+       } else {
+               if (ts->inverted_x)
+                       input_x = ts->abs_y_max - input_x;
+               if (ts->inverted_y)
+                       input_y = ts->abs_x_max - input_y;
                 swap(input_x, input_y);
+       }

         input_mt_slot(ts->input_dev, id);
         input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true);

> 
> Looks good overall, but was this tested, and if so, on which device?
> Could you add a reference to the hardware used for testing in the
> commit log?

I just have a custom hardware (prototype) with gt1151. Should I mention
this in commit log as well?

> 
> Cheers
> 

-- 
Regards,
Marcin Niestroj