* [PATCH] Input: goodix - use generic touchscreen_properties
@ 2017-10-25 11:32 Marcin Niestroj
2017-10-25 19:42 ` Bastien Nocera
0 siblings, 1 reply; 6+ messages in thread
From: Marcin Niestroj @ 2017-10-25 11:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Bastien Nocera, Antonio Ospite, linux-input, Marcin Niestroj
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.
Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
---
Patch was developed and tested on top of v4.14-rc6 with
[1] ("Input: goodix - support gt1151 touchpanel") applied.
[1] https://www.spinics.net/lists/devicetree/msg199945.html
drivers/input/touchscreen/goodix.c | 87 +++++++++++++++-----------------------
1 file changed, 33 insertions(+), 54 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 69d0b8cbc71f..f82101cd9c04 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -22,6 +22,7 @@
#include <linux/i2c.h>
#include <linux/input.h>
#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
#include <linux/module.h>
#include <linux/delay.h>
#include <linux/irq.h>
@@ -43,11 +44,7 @@ struct goodix_ts_data {
struct i2c_client *client;
struct input_dev *input_dev;
const struct goodix_chip_data *chip;
- int abs_x_max;
- int abs_y_max;
- bool swapped_x_y;
- bool inverted_x;
- bool inverted_y;
+ struct touchscreen_properties prop;
unsigned int max_touch_num;
unsigned int int_trigger_type;
struct gpio_desc *gpiod_int;
@@ -295,18 +292,10 @@ static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8 *coor_data)
int input_y = get_unaligned_le16(&coor_data[3]);
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)
- swap(input_x, input_y);
-
input_mt_slot(ts->input_dev, id);
input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true);
- input_report_abs(ts->input_dev, ABS_MT_POSITION_X, input_x);
- input_report_abs(ts->input_dev, ABS_MT_POSITION_Y, input_y);
+ touchscreen_report_pos(ts->input_dev, &ts->prop, input_x, input_y,
+ true);
input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, input_w);
input_report_abs(ts->input_dev, ABS_MT_WIDTH_MAJOR, input_w);
}
@@ -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;
ts->int_trigger_type = GOODIX_INT_TRIGGER;
ts->max_touch_num = GOODIX_MAX_CONTACTS;
- return;
+ goto input_set_params;
}
- ts->abs_x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
- ts->abs_y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
- if (ts->swapped_x_y)
- swap(ts->abs_x_max, ts->abs_y_max);
+ x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
+ y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
ts->int_trigger_type = config[TRIGGER_LOC] & 0x03;
ts->max_touch_num = config[MAX_CONTACTS_LOC] & 0x0f;
- if (!ts->abs_x_max || !ts->abs_y_max || !ts->max_touch_num) {
+ if (!x_max || !y_max || !ts->max_touch_num) {
dev_err(&ts->client->dev,
"Invalid config, using defaults\n");
- 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;
ts->max_touch_num = GOODIX_MAX_CONTACTS;
}
- if (dmi_check_system(rotated_screen)) {
- ts->inverted_x = true;
- ts->inverted_y = true;
- dev_dbg(&ts->client->dev,
- "Applying '180 degrees rotated screen' quirk\n");
- }
+input_set_params:
+ 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);
+ input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
+ input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+ input_mt_init_slots(ts->input_dev, ts->max_touch_num,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
}
/**
@@ -692,16 +679,6 @@ static int goodix_request_input_dev(struct goodix_ts_data *ts)
return -ENOMEM;
}
- input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
- 0, ts->abs_x_max, 0, 0);
- input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
- 0, ts->abs_y_max, 0, 0);
- input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
- input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
-
- input_mt_init_slots(ts->input_dev, ts->max_touch_num,
- INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
-
ts->input_dev->name = "Goodix Capacitive TouchScreen";
ts->input_dev->phys = "input/ts";
ts->input_dev->id.bustype = BUS_I2C;
@@ -736,19 +713,21 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
{
int error;
- ts->swapped_x_y = device_property_read_bool(&ts->client->dev,
- "touchscreen-swapped-x-y");
- ts->inverted_x = device_property_read_bool(&ts->client->dev,
- "touchscreen-inverted-x");
- ts->inverted_y = device_property_read_bool(&ts->client->dev,
- "touchscreen-inverted-y");
-
- goodix_read_config(ts);
-
error = goodix_request_input_dev(ts);
if (error)
return error;
+ goodix_read_config(ts);
+
+ touchscreen_parse_properties(ts->input_dev, true, &ts->prop);
+
+ if (dmi_check_system(rotated_screen)) {
+ ts->prop.invert_x = true;
+ ts->prop.invert_y = true;
+ dev_dbg(&ts->client->dev,
+ "Applying '180 degrees rotated screen' quirk\n");
+ }
+
ts->irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT;
error = goodix_request_irq(ts);
if (error) {
--
2.14.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: goodix - use generic touchscreen_properties
2017-10-25 11:32 [PATCH] Input: goodix - use generic touchscreen_properties Marcin Niestroj
@ 2017-10-25 19:42 ` Bastien Nocera
2017-10-26 8:14 ` Marcin Niestroj
0 siblings, 1 reply; 6+ messages in thread
From: Bastien Nocera @ 2017-10-25 19:42 UTC (permalink / raw)
To: Marcin Niestroj, Dmitry Torokhov; +Cc: Antonio Ospite, linux-input
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?
> ts->int_trigger_type = GOODIX_INT_TRIGGER;
> ts->max_touch_num = GOODIX_MAX_CONTACTS;
> - return;
> + goto input_set_params;
> }
>
> - ts->abs_x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
> - ts->abs_y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
> - if (ts->swapped_x_y)
> - swap(ts->abs_x_max, ts->abs_y_max);
> + x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
> + y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
> ts->int_trigger_type = config[TRIGGER_LOC] & 0x03;
> ts->max_touch_num = config[MAX_CONTACTS_LOC] & 0x0f;
> - if (!ts->abs_x_max || !ts->abs_y_max || !ts->max_touch_num) {
> + if (!x_max || !y_max || !ts->max_touch_num) {
> dev_err(&ts->client->dev,
> "Invalid config, using defaults\n");
> - 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;
> ts->max_touch_num = GOODIX_MAX_CONTACTS;
> }
>
> - if (dmi_check_system(rotated_screen)) {
> - ts->inverted_x = true;
> - ts->inverted_y = true;
> - dev_dbg(&ts->client->dev,
> - "Applying '180 degrees rotated screen' quirk\n");
> - }
> +input_set_params:
> + 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);
This is x_max - 1, and y_max - 1, when the original code used x_max and
y_max. Can you mention this fix in the changelog as well, or better,
split it off in a separate fix?
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?
Cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: goodix - use generic touchscreen_properties
2017-10-25 19:42 ` Bastien Nocera
@ 2017-10-26 8:14 ` Marcin Niestroj
2017-10-26 13:02 ` Bastien Nocera
0 siblings, 1 reply; 6+ messages in thread
From: Marcin Niestroj @ 2017-10-26 8:14 UTC (permalink / raw)
To: Bastien Nocera, Dmitry Torokhov; +Cc: Antonio Ospite, linux-input
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.
>
>> ts->int_trigger_type = GOODIX_INT_TRIGGER;
>> ts->max_touch_num = GOODIX_MAX_CONTACTS;
>> - return;
>> + goto input_set_params;
>> }occurs
>>
>> - ts->abs_x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
>> - ts->abs_y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
>> - if (ts->swapped_x_y)
>> - swap(ts->abs_x_max, ts->abs_y_max);
>> + x_max = get_unaligned_le16(&config[RESOLUTION_LOC]);
>> + y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]);
>> ts->int_trigger_type = config[TRIGGER_LOC] & 0x03;
>> ts->max_touch_num = config[MAX_CONTACTS_LOC] & 0x0f;
>> - if (!ts->abs_x_max || !ts->abs_y_max || !ts->max_touch_num) {
>> + if (!x_max || !y_max || !ts->max_touch_num) {
>> dev_err(&ts->client->dev,
>> "Invalid config, using defaults\n");
>> - 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;
>> ts->max_touch_num = GOODIX_MAX_CONTACTS;
>> }
>>
>> - if (dmi_check_system(rotated_screen)) {
>> - ts->inverted_x = true;
>> - ts->inverted_y = true;
>> - dev_dbg(&ts->client->dev,
>> - "Applying '180 degrees rotated screen' quirk\n");
>> - }
>> +input_set_params:
>> + 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);
>
> This is x_max - 1, and y_max - 1, when the original code used x_max and
> y_max. Can you mention this fix in the changelog as well, or better,
> split it off in a separate fix?
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?
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);
input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true);
>
> 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?
>
> Cheers
>
--
Regards,
Marcin Niestroj
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: goodix - use generic touchscreen_properties
2017-10-26 8:14 ` Marcin Niestroj
@ 2017-10-26 13:02 ` Bastien Nocera
2017-10-26 13:50 ` Marcin Niestroj
0 siblings, 1 reply; 6+ messages in thread
From: Bastien Nocera @ 2017-10-26 13:02 UTC (permalink / raw)
To: Marcin Niestroj, Dmitry Torokhov; +Cc: Antonio Ospite, linux-input
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: goodix - use generic touchscreen_properties
2017-10-26 13:02 ` Bastien Nocera
@ 2017-10-26 13:50 ` Marcin Niestroj
2017-10-26 14:39 ` Bastien Nocera
0 siblings, 1 reply; 6+ messages in thread
From: Marcin Niestroj @ 2017-10-26 13:50 UTC (permalink / raw)
To: Bastien Nocera, Dmitry Torokhov; +Cc: Antonio Ospite, linux-input
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.
--
Marcin Niestroj
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: goodix - use generic touchscreen_properties
2017-10-26 13:50 ` Marcin Niestroj
@ 2017-10-26 14:39 ` Bastien Nocera
0 siblings, 0 replies; 6+ messages in thread
From: Bastien Nocera @ 2017-10-26 14:39 UTC (permalink / raw)
To: Marcin Niestroj, Dmitry Torokhov; +Cc: Antonio Ospite, linux-input
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-26 14:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-25 11:32 [PATCH] Input: goodix - use generic touchscreen_properties Marcin Niestroj
2017-10-25 19:42 ` Bastien Nocera
2017-10-26 8:14 ` Marcin Niestroj
2017-10-26 13:02 ` Bastien Nocera
2017-10-26 13:50 ` Marcin Niestroj
2017-10-26 14:39 ` Bastien Nocera
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.