Re: [PATCH] input/touchscreen: New EETI eGalaxTouch serial touchscreen driver

2015-12-15 22:21 keltezéssel, Dmitry Torokhov írta:

Hi Zoltán,

On Tue, Dec 15, 2015 at 12:22:07PM +0100, Böszörményi Zoltán wrote:

quoted

From: Böszörményi Zoltán <redacted>

There are two EETI touchscreen drivers in the kernel (eeti_ts and egalax_ts)
but both are for I2C-connected panels. This is for a different, serial
and not multi-touch touchscreen panel. The protocol documentation is at
http://www.eeti.com.tw/pdf/Software%20Programming%20Guide_v2.0.pdf

Signed-off-by: Böszörményi Zoltán <redacted>

Thank you for your patch, it looks pretty good, just a few comments
below.

quoted

+config TOUCHSCREEN_EGALAX_SERIO

I'd rather called it TOUCHSCREEN_EGALAX_SERIAL

It doesn't matter, I am not attached to names.

quoted

+obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIO)	+= egalax.o

I think better name is egalax_ts_serial.

Sure, I was thinking about it myself.

quoted

 obj-$(CONFIG_TOUCHSCREEN_FT6236)	+= ft6236.o
 obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
 obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix.o
 obj-$(CONFIG_TOUCHSCREEN_ILI210X)	+= ili210x.o
 obj-$(CONFIG_TOUCHSCREEN_IMX6UL_TSC)	+= imx6ul_tsc.o
 obj-$(CONFIG_TOUCHSCREEN_INEXIO)	+= inexio.o
 obj-$(CONFIG_TOUCHSCREEN_INTEL_MID)	+= intel-mid-touch.o
 obj-$(CONFIG_TOUCHSCREEN_IPROC)		+= bcm_iproc_tsc.o
 obj-$(CONFIG_TOUCHSCREEN_LPC32XX)	+= lpc32xx_ts.o
 obj-$(CONFIG_TOUCHSCREEN_MAX11801)	+= max11801_ts.o

diff --git a/drivers/input/touchscreen/egalax.c b/drivers/input/touchscreen/egalax.c
new file mode 100644
index 0000000..94ac9bd
--- /dev/null
+++ b/drivers/input/touchscreen/egalax.c

@@ -0,0 +1,210 @@
+/*
+ * EETI Egalax serial touchscreen driver
+ *
+ * Copyright (c) 2015 Zoltán Böszörményi <zboszor@pr.hu>
+ *
+ * based on the
+ *
+ * Hampshire serial touchscreen driver (Copyright (c) 2010 Adam Bennett)
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/serio.h>
+
+#define DRIVER_DESC	"EETI Egalax serial touchscreen driver"
+
+MODULE_AUTHOR("Zoltán Böszörményi <zboszor@pr.hu>");
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
+
+/*
+ * Definitions & global arrays.
+ */
+
+#define EGALAX_FORMAT_MAX_LENGTH 6
+#define EGALAX_RESPONSE_BEGIN_BYTE 0x80
+#define EGALAX_FORMAT_PRESSURE_BIT 0x40
+#define EGALAX_FORMAT_TOUCH_BIT 0x01

We have BIT() macro that would be very useful here.

quoted

+#define EGALAX_FORMAT_RESOLUTION 0x06
+
+#define EGALAX_MIN_XC 0
+#define EGALAX_MAX_XC 0x4000
+#define EGALAX_MIN_YC 0
+#define EGALAX_MAX_YC 0x4000
+
+#define EGALAX_GET_XC(data, resbits, shift) ((((data[1] & (resbits)) << 7) | (data[2] & 0x7f)) << shift)
+#define EGALAX_GET_YC(data, resbits, shift) ((((data[3] & (resbits)) << 7) | (data[4] & 0x7f)) << shift)
+#define EGALAX_GET_TOUCHED(data) (EGALAX_FORMAT_TOUCH_BIT & data[0])
+
+/*
+ * Per-touchscreen data.
+ */
+
+struct egalax {
+	struct input_dev *dev;
+	struct serio *serio;
+	int idx;
+	int bytes;
+	int resbits;
+	int shift;
+	unsigned char data[EGALAX_FORMAT_MAX_LENGTH];
+	char phys[32];
+};
+
+static void egalax_process_data(struct egalax *pegalax)
+{
+	struct input_dev *dev = pegalax->dev;
+
+	if (++pegalax->idx == pegalax->bytes) {
+		input_report_abs(dev, ABS_X, EGALAX_GET_XC(pegalax->data, pegalax->resbits, pegalax->shift));
+		input_report_abs(dev, ABS_Y, EGALAX_GET_YC(pegalax->data, pegalax->resbits, pegalax->shift));
+		input_report_key(dev, BTN_TOUCH, EGALAX_GET_TOUCHED(pegalax->data));
+		input_sync(dev);
+
+		pegalax->idx = 0;
+	}
+}
+
+static irqreturn_t egalax_interrupt(struct serio *serio,
+		unsigned char data, unsigned int flags)
+{
+	struct egalax *pegalax = serio_get_drvdata(serio);
+
+	pegalax->data[pegalax->idx] = data;
+
+	if (EGALAX_RESPONSE_BEGIN_BYTE & pegalax->data[0]) {
+		pegalax->bytes = (EGALAX_FORMAT_PRESSURE_BIT & pegalax->data[0] ? 6 : 5);
+		switch ((EGALAX_FORMAT_RESOLUTION & pegalax->data[0]) >> 1) {
+		case 0:
+			pegalax->resbits = 0x0f;
+			pegalax->shift = 3;
+			break;
+		case 1:
+			pegalax->resbits = 0x1f;
+			pegalax->shift = 2;
+			break;
+		case 2:
+			pegalax->resbits = 0x3f;
+			pegalax->shift = 1;
+			break;
+		default:
+			pegalax->resbits = 0x7f;
+			pegalax->shift = 0;
+			break;
+		}
+		egalax_process_data(pegalax);

There is no reason to recalculate shift and mask on every byte and call
egalax_process_data(), better is to wait till you get full packet and
then do the claculations. Also I think you can avoid conditional
computation (switch) here.

See my comment below.

quoted

+static void egalax_disconnect(struct serio *serio)
+{
+	struct egalax *pegalax = serio_get_drvdata(serio);
+
+	input_get_device(pegalax->dev);
+	input_unregister_device(pegalax->dev);
+	serio_close(serio);
+	serio_set_drvdata(serio, NULL);
+	input_put_device(pegalax->dev);
+	kfree(pegalax);

This is needlessly complicated (although I do know we have this code in
other drivers). Since we do not send any data *to* the touch controller
we can close the port first and then unregister the device.

OK, whatever you think is better. I am not familiar with the input
subsystem internals.

Could you please try the version of the patch below and let me know if
it works for you?

Thanks!

While the code itself didn't look suspicious at first, testing showed
that it doesn't work when compiled in with 4.4-rc5, while my original
code did. You got the mask and expanding my macros wrong.
I will submit a v3 patch, as yours was v2.

Thanks,
Zoltán Böszörményi

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