From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcin Niestroj Subject: Re: [PATCH] Input: goodix - use generic touchscreen_properties Date: Thu, 26 Oct 2017 10:14:30 +0200 Message-ID: <5ac10de2-9838-0adc-27ea-28b781c3092a@grinn-global.com> References: <20171025113217.7814-1-m.niestroj@grinn-global.com> <1508960523.28409.31.camel@hadess.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.megiteam.pl ([31.186.83.105]:44674 "EHLO smtp.megiteam.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751160AbdJZIOf (ORCPT ); Thu, 26 Oct 2017 04:14:35 -0400 In-Reply-To: <1508960523.28409.31.camel@hadess.net> Content-Language: en-US Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Bastien Nocera , Dmitry Torokhov Cc: Antonio Ospite , linux-input@vger.kernel.org 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. > >> @@ -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