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