From patchwork Thu Oct 26 08:14:30 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marcin Niestroj X-Patchwork-Id: 10027719 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 509D0601E8 for ; Thu, 26 Oct 2017 08:14:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3D13826E38 for ; Thu, 26 Oct 2017 08:14:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2EC4128419; Thu, 26 Oct 2017 08:14:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id F13E526E38 for ; Thu, 26 Oct 2017 08:14:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751507AbdJZIOh (ORCPT ); Thu, 26 Oct 2017 04:14:37 -0400 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 Received: from [95.143.241.142] (helo=[192.168.0.120]) by smtp.megiteam.pl with esmtpsa (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1e7dJ9-0004IE-Q2; Thu, 26 Oct 2017 10:14:33 +0200 Subject: Re: [PATCH] Input: goodix - use generic touchscreen_properties To: Bastien Nocera , Dmitry Torokhov Cc: Antonio Ospite , linux-input@vger.kernel.org References: <20171025113217.7814-1-m.niestroj@grinn-global.com> <1508960523.28409.31.camel@hadess.net> From: Marcin Niestroj Message-ID: <5ac10de2-9838-0adc-27ea-28b781c3092a@grinn-global.com> Date: Thu, 26 Oct 2017 10:14:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <1508960523.28409.31.camel@hadess.net> Content-Language: en-US Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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? 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 > 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);