| Message ID | 5ac10de2-9838-0adc-27ea-28b781c3092a@grinn-global.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-10-26 at 10:14 +0200, Marcin Niestroj wrote: > 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. But this isn't swapped or rotated when the device's range is set, is it? + 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); <snip> > > > 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? That would be great, though I'm not sure whether the gt1151 support would get backported. > > 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? That would be useful, yes. The fact that it's a device-tree based device would also be good to mention. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26.10.2017 15:02, Bastien Nocera wrote: > On Thu, 2017-10-26 at 10:14 +0200, Marcin Niestroj wrote: >> 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. > > But this isn't swapped or rotated when the device's range is set, is > it? > + 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); > Not sure I understand your question. You mean at this specific point in code? No it is not. But this is how it should be done I think. Swapping range (x_max, y_max) occurs at the end of touchscreen_parse_properties(). > <snip> >>> >> 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? > > That would be great, though I'm not sure whether the gt1151 support > would get backported. > gt1151 patch should not be needed for that fix to apply cleanly. >>> 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? > > That would be useful, yes. The fact that it's a device-tree based > device would also be good to mention. > Will do so.
On Thu, 2017-10-26 at 15:50 +0200, Marcin Niestroj wrote: > On 26.10.2017 15:02, Bastien Nocera wrote: > > On Thu, 2017-10-26 at 10:14 +0200, Marcin Niestroj wrote: > > > 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. > > > > But this isn't swapped or rotated when the device's range is set, > > is > > it? > > + 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); > > > > Not sure I understand your question. You mean at this specific point > in code? No it is not. But this is how it should be done I think. > Swapping range (x_max, y_max) occurs at the end of > touchscreen_parse_properties(). Ha, right. That's not really obvious to be honest. <snip> > gt1151 patch should not be needed for that fix to apply cleanly. Right, though it wouldn't work on your system :) > > > > 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? > > > > That would be useful, yes. The fact that it's a device-tree based > > device would also be good to mention. > > > > Will do so. Great, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);