| Message ID | 1428181693-25362-2-git-send-email-plaes@plaes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2015-04-05 at 00:08 +0300, Priit Laes wrote: > This patch implements GT801x2 touchscreen support. > Unfortunately, there is a big difference between GT801 and GT9xx > series > chips, therefore some advice is needed on how to proceed. > > Differences between GT801x2 and GT9xx series: > > 1. I2C registers: 1 byte (GT801x2) vs 2 bytes (GT9xx) > 2. Different configuration layout and version info > 3. Different touch report protocol That doesn't seem like an awful lot of differences. Approximately 80 line changes for 500 lines of driver code. You could add an enum for the 8xx and 9xx types near the top, add that as driver data in the match arrays (both the ACPI and OF ones). Then have if statements choose the correct init, read and report functions. I would think that that would grow the driver by a further 50 lines, which would certainly be acceptable. Don't forget to add your name to the copyright header mentioning you're the author for the 8xx support :) > Signed-off-by: Priit Laes <plaes@plaes.org> There's really no need to sign it off if you know it's not going to be accepted upstream ;) Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2015-04-05 at 18:04 +0200, Bastien Nocera wrote: > On Sun, 2015-04-05 at 00:08 +0300, Priit Laes wrote: > > This patch implements GT801x2 touchscreen support. > > Unfortunately, there is a big difference between GT801 and GT9xx > > series > > chips, therefore some advice is needed on how to proceed. > > > > Differences between GT801x2 and GT9xx series: > > > > 1. I2C registers: 1 byte (GT801x2) vs 2 bytes (GT9xx) > > 2. Different configuration layout and version info > > 3. Different touch report protocol > > That doesn't seem like an awful lot of differences. Approximately 80 > line changes for 500 lines of driver code. You could add an enum for > the 8xx and 9xx types near the top, add that as driver data in the > match arrays (both the ACPI and OF ones). Then have if statements > choose the correct init, read and report functions. How should I handle the version readout? Currently the driver has following info in its registers (starting from 0xf0): f0: 47 54 38 30 31 4e 49 5f 33 52 31 35 5f 31 41 56 GT801NI_3R15_1AV And there's another issue with deactivating interrupts. When I remove the goodix module, I get following traceback: [snip] WARNING: CPU: 0 PID: 381 at fs/proc/generic.c:552 remove_proc_entry+0x138/0x16c() remove_proc_entry: removing non-empty directory 'irq/50', leaking at least 'gt801x2' Modules linked in: goodix(-) rtl8192cu rtl_usb rtl8192c_common rtlwifi CPU: 0 PID: 381 Comm: rmmod Not tainted 4.0.0-rc1+ #22 Hardware name: Allwinner A1X (Device Tree) [<c0014544>] (unwind_backtrace) from [<c00116e4>] (show_stack+0x10/0x14) [<c00116e4>] (show_stack) from [<c042995c>] (dump_stack+0x84/0x94) [<c042995c>] (dump_stack) from [<c0021140>] (warn_slowpath_common+0x80/0xb0) [<c0021140>] (warn_slowpath_common) from [<c00211a0>] (warn_slowpath_fmt+0x30/0x40) [<c00211a0>] (warn_slowpath_fmt) from [<c0124474>] (remove_proc_entry+0x138/0x16c) [<c0124474>] (remove_proc_entry) from [<c0060358>] (unregister_irq_proc+0xa8/0xb0) [<c0060358>] (unregister_irq_proc) from [<c00595d0>] (free_desc+0x30/0x60) [<c00595d0>] (free_desc) from [<c0059648>] (irq_free_descs+0x48/0x80) [<c0059648>] (irq_free_descs) from [<c02da850>] (i2c_device_remove+0x64/0x80) [<c02da850>] (i2c_device_remove) from [<c025af5c>] (__device_release_driver+0x70/0xc4) [<c025af5c>] (__device_release_driver) from [<c025b68c>] (driver_detach+0xcc/0xd0) [<c025b68c>] (driver_detach) from [<c025acbc>] (bus_remove_driver+0x4c/0xa0) [<c025acbc>] (bus_remove_driver) from [<c007d170>] (SyS_delete_module+0x16c/0x1b8) [<c007d170>] (SyS_delete_module) from [<c000e4a0>] (ret_fast_syscall+0x0/0x34) [/snip] Devicetree node contains following: [snip] &i2c2 { pinctrl-names = "default"; pinctrl-0 = <&i2c2_pins_a>; status = "okay"; /* Touchscreen */ touchscreen@55 { compatible = "goodix,gt801x2"; reg = <0x55>; interrupt-parent = <&pio>; interrupts = <21 IRQ_TYPE_EDGE_FALLING>; /* EINT21 (PH21) */ }; }; [/snip] Päikest, Priit :) -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Apr 05, 2015 at 08:06:16PM +0300, Priit Laes wrote: > On Sun, 2015-04-05 at 18:04 +0200, Bastien Nocera wrote: > > On Sun, 2015-04-05 at 00:08 +0300, Priit Laes wrote: > > > This patch implements GT801x2 touchscreen support. > > > Unfortunately, there is a big difference between GT801 and GT9xx > > > series > > > chips, therefore some advice is needed on how to proceed. > > > > > > Differences between GT801x2 and GT9xx series: > > > > > > 1. I2C registers: 1 byte (GT801x2) vs 2 bytes (GT9xx) > > > 2. Different configuration layout and version info > > > 3. Different touch report protocol > > > > That doesn't seem like an awful lot of differences. Approximately 80 > > line changes for 500 lines of driver code. You could add an enum for > > the 8xx and 9xx types near the top, add that as driver data in the > > match arrays (both the ACPI and OF ones). Then have if statements > > choose the correct init, read and report functions. > > How should I handle the version readout? > > Currently the driver has following info in its registers (starting > from 0xf0): > > f0: 47 54 38 30 31 4e 49 5f 33 52 31 35 5f 31 41 56 GT801NI_3R15_1AV > > > And there's another issue with deactivating interrupts. When I remove > the goodix module, I get following traceback: > > [snip] > WARNING: CPU: 0 PID: 381 at fs/proc/generic.c:552 remove_proc_entry+0x138/0x16c() > remove_proc_entry: removing non-empty directory 'irq/50', leaking at least 'gt801x2' > Modules linked in: goodix(-) rtl8192cu rtl_usb rtl8192c_common rtlwifi This is an issue with the i2c core and not the driver; the offending patch has been reverted as far as I can remember. Thanks.
On Sun, Apr 05, 2015 at 06:04:24PM +0200, Bastien Nocera wrote: > On Sun, 2015-04-05 at 00:08 +0300, Priit Laes wrote: > > This patch implements GT801x2 touchscreen support. > > Unfortunately, there is a big difference between GT801 and GT9xx > > series > > chips, therefore some advice is needed on how to proceed. > > > > Differences between GT801x2 and GT9xx series: > > > > 1. I2C registers: 1 byte (GT801x2) vs 2 bytes (GT9xx) > > 2. Different configuration layout and version info > > 3. Different touch report protocol > > That doesn't seem like an awful lot of differences. Approximately 80 > line changes for 500 lines of driver code. You could add an enum for > the 8xx and 9xx types near the top, add that as driver data in the > match arrays (both the ACPI and OF ones). Then have if statements > choose the correct init, read and report functions. > > I would think that that would grow the driver by a further 50 lines, > which would certainly be acceptable. > > Don't forget to add your name to the copyright header mentioning > you're the author for the 8xx support :) > > > Signed-off-by: Priit Laes <plaes@plaes.org> > > There's really no need to sign it off if you know it's not going to be > accepted upstream ;) Actually I think it is still useful: that means that somebody else can use the patch as a base for their work without any concerns even if original author did not see it through upstream acceptance for some reason. Thanks.
On Sun, 2015-04-05 at 20:06 +0300, Priit Laes wrote: > On Sun, 2015-04-05 at 18:04 +0200, Bastien Nocera wrote: > > On Sun, 2015-04-05 at 00:08 +0300, Priit Laes wrote: > > > This patch implements GT801x2 touchscreen support. > > > Unfortunately, there is a big difference between GT801 and GT9xx > > > series > > > chips, therefore some advice is needed on how to proceed. > > > > > > Differences between GT801x2 and GT9xx series: > > > > > > 1. I2C registers: 1 byte (GT801x2) vs 2 bytes (GT9xx) > > > 2. Different configuration layout and version info > > > 3. Different touch report protocol > > > > That doesn't seem like an awful lot of differences. Approximately > > 80 > > line changes for 500 lines of driver code. You could add an enum > > for > > the 8xx and 9xx types near the top, add that as driver data in the > > match arrays (both the ACPI and OF ones). Then have if statements > > choose the correct init, read and report functions. > > How should I handle the version readout? > > Currently the driver has following info in its registers (starting > from 0xf0): > > f0: 47 54 38 30 31 4e 49 5f 33 52 31 35 5f 31 41 56 > GT801NI_3R15_1AV Version info is only used for debug anyway, so you'd have a goodix_read_version() call for gt8xx devices. .driver_data = GT9XX for the existing code or .driver_data = GT8XX for the new one And do: if (driver_data == GT9XX) goodix_read_version_9xx(); else goodix_read_version_8xx(); etc. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2015-04-05 at 16:56 -0700, Dmitry Torokhov wrote: > On Sun, Apr 05, 2015 at 06:04:24PM +0200, Bastien Nocera wrote: > > On Sun, 2015-04-05 at 00:08 +0300, Priit Laes wrote: > > > This patch implements GT801x2 touchscreen support. > > > Unfortunately, there is a big difference between GT801 and GT9xx > > > series > > > chips, therefore some advice is needed on how to proceed. > > > > > > Differences between GT801x2 and GT9xx series: > > > > > > 1. I2C registers: 1 byte (GT801x2) vs 2 bytes (GT9xx) > > > 2. Different configuration layout and version info > > > 3. Different touch report protocol > > > > That doesn't seem like an awful lot of differences. Approximately > > 80 > > line changes for 500 lines of driver code. You could add an enum > > for > > the 8xx and 9xx types near the top, add that as driver data in the > > match arrays (both the ACPI and OF ones). Then have if statements > > choose the correct init, read and report functions. > > > > I would think that that would grow the driver by a further 50 > > lines, > > which would certainly be acceptable. > > > > Don't forget to add your name to the copyright header mentioning > > you're the author for the 8xx support :) > > > > > Signed-off-by: Priit Laes <plaes@plaes.org> > > > > There's really no need to sign it off if you know it's not going > > to be > > accepted upstream ;) > > Actually I think it is still useful: that means that somebody else > can > use the patch as a base for their work without any concerns even if > original author did not see it through upstream acceptance for some > reason. Right, fair enough. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index 3af1698..94e8367 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -39,15 +39,23 @@ struct goodix_ts_data { #define GOODIX_MAX_HEIGHT 4096 #define GOODIX_MAX_WIDTH 4096 #define GOODIX_INT_TRIGGER 1 -#define GOODIX_CONTACT_SIZE 8 #define GOODIX_MAX_CONTACTS 10 +#if 0 +#define GOODIX_CONTACT_SIZE 8 #define GOODIX_CONFIG_MAX_LENGTH 240 /* Register defines */ #define GOODIX_READ_COOR_ADDR 0x814E #define GOODIX_REG_CONFIG_DATA 0x8047 #define GOODIX_REG_VERSION 0x8140 +#else +#define GOODIX_READ_COOR_ADDR 0x01 +#define GOODIX_REG_CONFIG_DATA 0x65 +#define GOODIX_REG_VERSION 0xf0 +#define GOODIX_CONFIG_MAX_LENGTH 7 +#define GOODIX_CONTACT_SIZE 5 +#endif #define RESOLUTION_LOC 1 #define MAX_CONTACTS_LOC 5 @@ -69,16 +77,15 @@ static const unsigned long goodix_irq_flags[] = { * @len: length of the buffer to write */ static int goodix_i2c_read(struct i2c_client *client, - u16 reg, u8 *buf, int len) + u8 reg, u8 *buf, int len) { struct i2c_msg msgs[2]; - u16 wbuf = cpu_to_be16(reg); int ret; msgs[0].flags = 0; msgs[0].addr = client->addr; - msgs[0].len = 2; - msgs[0].buf = (u8 *) &wbuf; + msgs[0].len = 1; + msgs[0].buf = ® msgs[1].flags = I2C_M_RD; msgs[1].addr = client->addr; @@ -89,6 +96,7 @@ static int goodix_i2c_read(struct i2c_client *client, return ret < 0 ? ret : (ret != ARRAY_SIZE(msgs) ? -EIO : 0); } +#if 0 static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data) { int touch_num; @@ -133,6 +141,7 @@ static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8 *coor_data) input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, input_w); input_report_abs(ts->input_dev, ABS_MT_WIDTH_MAJOR, input_w); } +#endif /** * goodix_process_events - Process incoming events @@ -144,17 +153,57 @@ static void goodix_ts_report_touch(struct goodix_ts_data *ts, u8 *coor_data) */ static void goodix_process_events(struct goodix_ts_data *ts) { - u8 point_data[1 + GOODIX_CONTACT_SIZE * ts->max_touch_num]; - int touch_num; + u8 point_data[2 + GOODIX_CONTACT_SIZE * ts->max_touch_num + 1]; + u8 touch_map[GOODIX_MAX_CONTACTS] = {0}; + u8 checksum = 0; int i; + int loc; + int error; + int touch_raw; + int touch_num; + int input_x; + int input_y; + u8 input_w; + + error = goodix_i2c_read(ts->client, GOODIX_READ_COOR_ADDR, point_data, sizeof(point_data)); - touch_num = goodix_ts_read_input_report(ts, point_data); - if (touch_num < 0) + if (error) { + dev_err(&ts->client->dev, "I2C transfer error: %d\n", error); return; + } + + if (!(touch_raw = get_unaligned_le16(&point_data[0]))) + return; + + touch_num = 0; + for (i = 0; (touch_raw != 0) && (i < ts->max_touch_num); i++) + { + if (touch_raw & 1) + touch_map[touch_num++] = i; + touch_raw >>= 1; + } + + /* TODO: Checksum calculation is buggy */ + for (i = 0; i < GOODIX_CONTACT_SIZE*touch_num + 3; i++) + checksum += point_data[i++]; + + dev_debug(&ts->client->dev, "Fingers detected: %d, checksum: %d\n", touch_num, checksum); for (i = 0; i < touch_num; i++) - goodix_ts_report_touch(ts, - &point_data[1 + GOODIX_CONTACT_SIZE * i]); + { + loc = 2 + GOODIX_CONTACT_SIZE * i; + input_x = get_unaligned_be16(&point_data[loc]); + input_y = get_unaligned_be16(&point_data[loc + 2]); + input_w = point_data[loc + 4]; + dev_debug(&ts->client->dev, "i: %d: X = %d,Y = %d, W= %d\n", touch_map[i], input_x, input_y, input_w); + + input_mt_slot(ts->input_dev, touch_map[i]); + 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); + input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR, input_w); + input_report_abs(ts->input_dev, ABS_MT_WIDTH_MAJOR, input_w); + } input_mt_sync_frame(ts->input_dev); input_sync(ts->input_dev); @@ -168,17 +217,21 @@ static void goodix_process_events(struct goodix_ts_data *ts) */ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id) { +#if 0 static const u8 end_cmd[] = { GOODIX_READ_COOR_ADDR >> 8, GOODIX_READ_COOR_ADDR & 0xff, 0 }; +#endif struct goodix_ts_data *ts = dev_id; goodix_process_events(ts); +#if 0 if (i2c_master_send(ts->client, end_cmd, sizeof(end_cmd)) < 0) dev_err(&ts->client->dev, "I2C write end_cmd error\n"); +#endif return IRQ_HANDLED; } @@ -209,8 +262,8 @@ static void goodix_read_config(struct goodix_ts_data *ts) return; } - ts->abs_x_max = get_unaligned_le16(&config[RESOLUTION_LOC]); - ts->abs_y_max = get_unaligned_le16(&config[RESOLUTION_LOC + 2]); + ts->abs_x_max = get_unaligned_be16(&config[RESOLUTION_LOC]); + ts->abs_y_max = get_unaligned_be16(&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) { @@ -220,6 +273,8 @@ static void goodix_read_config(struct goodix_ts_data *ts) ts->abs_y_max = GOODIX_MAX_HEIGHT; ts->max_touch_num = GOODIX_MAX_CONTACTS; } + dev_info(&ts->client->dev, "X_MAX = %d,Y_MAX = %d,MAX_TOUCH_NUM = %d\n", + ts->abs_x_max, ts->abs_y_max, ts->max_touch_num); } /** @@ -231,13 +286,14 @@ static void goodix_read_config(struct goodix_ts_data *ts) static int goodix_read_version(struct i2c_client *client, u16 *version) { int error; - u8 buf[6]; + u8 buf[16]; error = goodix_i2c_read(client, GOODIX_REG_VERSION, buf, sizeof(buf)); if (error) { dev_err(&client->dev, "read version failed: %d\n", error); return error; } + print_hex_dump_bytes("", DUMP_PREFIX_NONE, buf, ARRAY_SIZE(buf)); if (version) *version = get_unaligned_le16(&buf[4]); @@ -387,6 +443,8 @@ MODULE_DEVICE_TABLE(acpi, goodix_acpi_match); #ifdef CONFIG_OF static const struct of_device_id goodix_of_match[] = { + { .compatible = "goodix,gt801x2" }, +/* { .compatible = "goodix,gt911" }, { .compatible = "goodix,gt9110" }, { .compatible = "goodix,gt912" }, @@ -394,6 +452,7 @@ static const struct of_device_id goodix_of_match[] = { { .compatible = "goodix,gt9271" }, { .compatible = "goodix,gt928" }, { .compatible = "goodix,gt967" }, +*/ { } }; MODULE_DEVICE_TABLE(of, goodix_of_match);
This patch implements GT801x2 touchscreen support. Unfortunately, there is a big difference between GT801 and GT9xx series chips, therefore some advice is needed on how to proceed. Differences between GT801x2 and GT9xx series: 1. I2C registers: 1 byte (GT801x2) vs 2 bytes (GT9xx) 2. Different configuration layout and version info 3. Different touch report protocol Signed-off-by: Priit Laes <plaes@plaes.org> --- drivers/input/touchscreen/goodix.c | 87 ++++++++++++++++++++++++++++++++------ 1 file changed, 73 insertions(+), 14 deletions(-)