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

Marcin Niestroj
2017-10-26T08:14:35Z

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