[PATCH] input: goodix: RFC - implementing support for GT801x2

[PATCH] input: goodix: RFC - implementing support for GT801x2

[Priit Laes]

Hi all!

I'm looking for some advice on how to proceed with the GT801x2 touchscreen
driver. Current proof-of-concept uses the existing Goodix driver as a
template, but due to the differences between GT801x2 and GT9xx series,
I ended with a driver where GT9xx support has been ripped out. I'm sorry.

Notable differences:

1. I2C registers: 1 byte (GT801x2) vs 2 bytes (GT9xx)
2. Different configuration layout and version info
3. Different touch report protocol

So, would it make sense to have these two in the same driver?


Priit Laes (1):
  Input: goodix - preliminary support for GT801-2+1

 drivers/input/touchscreen/goodix.c | 87 ++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 14 deletions(-)

-- 
2.3.5

[PATCH] Input: goodix - preliminary support for GT801-2+1

[Priit Laes]

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(-)

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   = &reg;
 
 	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);
-- 
2.3.5

Re: [PATCH] Input: goodix - preliminary support for GT801-2+1

[Bastien Nocera]

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

Re: [PATCH] Input: goodix - preliminary support for GT801-2+1

[Priit Laes]

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

Re: [PATCH] Input: goodix - preliminary support for GT801-2+1

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] Input: goodix - preliminary support for GT801-2+1

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] Input: goodix - preliminary support for GT801-2+1

[Bastien Nocera]

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

Re: [PATCH] Input: goodix - preliminary support for GT801-2+1

[Bastien Nocera]

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.