On 26.10.2017 15:02, Bastien Nocera wrote:

On Thu, 2017-10-26 at 10:14 +0200, Marcin Niestroj wrote:

quoted

Hi Bastien,

On 25.10.2017 21:42, Bastien Nocera wrote:

quoted

Hey Marcin,

On Wed, 2017-10-25 at 13:32 +0200, Marcin Niestroj wrote:

quoted

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>

quoted

@@ -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-

quoted

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>

quoted

quoted

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.

quoted

quoted

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.

-- 
Marcin Niestroj