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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
