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

[Bastien Nocera <hadess@hadess.net>]

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.