Re: [PATCH v2] input: add driver for pixcir i2c touchscreens

Hi Henrik
    Thank you very much!

Hi jcbian,

Here are some duplicate comments and additions to what you already
heard from Mark and Dmitry.

quoted

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 61834ae..f11c1ab 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig

@@ -693,4 +693,13 @@ config TOUCHSCREEN_TPS6507X
 	  To compile this driver as a module, choose M here: the
 	  module will be called tps6507x_ts.
 
+config TOUCHSCREEN_PIXCIR
+	tristate "PIXCIR touchscreen panel support"
+	depends on I2C
+	help
+	  Say Y here if you have a PIXCIR based touchscreen.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called pixcir_i2c_ts.
+
 endif

Please keep Kconfig entries in alphabetical order.

quoted

+#define DRIVER_VERSION	"v1.0"
+#define DRIVER_AUTHOR	"Bee[off-list ref],Reed[off-list ref]"
+#define DRIVER_DESC		"Pixcir I2C Touchscreen Driver"
+#define DRIVER_LICENSE	"GPL"

Please insert these directly in the module definition.

quoted

+/* todo:check specs for resolution of touch screen */
+#define	TOUCHSCREEN_MINX	0
+#define	TOUCHSCREEN_MAXX	400
+#define	TOUCHSCREEN_MINY	0
+#define	TOUCHSCREEN_MAXY	600

This should really be settled before mainline inclusion.

quoted

+
+#define	DEBUG	0

Odd one...

quoted

+
+static struct workqueue_struct *pixcir_wq;
+
+struct pixcir_i2c_ts_data {
+	struct i2c_client *client;
+	struct input_dev *input;
+	struct delayed_work work;
+	int irq;
+};
+
+static void pixcir_ts_poscheck(struct work_struct *work)
+{
+	struct pixcir_i2c_ts_data *tsdata = container_of(work,
+						  struct pixcir_i2c_ts_data,
+						  work.work);
+
+	unsigned char touching, oldtouching;
+	int posx1, posy1, posx2, posy2;
+	u_int8_t Rdbuf[10], Wrbuf[1];

Please do not use capitals in variable names.

quoted

+	int ret;
+	int z = 50;
+	int w = 15;

Please remove these fake values altogether.

quoted

+
+	memset(Wrbuf, 0, sizeof(Wrbuf));
+	memset(Rdbuf, 0, sizeof(Rdbuf));
+
+	Wrbuf[0] = 0;
+	ret = i2c_master_send(tsdata->client, Wrbuf, 1);
+	if (ret != 1) {
+		dev_err(&tsdata->client->dev, "Unable to write to i2c touchscreen!\n");
+		goto out;
+	}
+
+	ret = i2c_master_recv(tsdata->client, Rdbuf, sizeof(Rdbuf));
+	if (ret != sizeof(Rdbuf)) {
+		dev_err(&tsdata->client->dev, "Unable to read i2c page!\n");
+		goto out;
+	}
+
+	touching = Rdbuf[0];
+	oldtouching = Rdbuf[1];
+	posx1 = ((Rdbuf[3] << 8) | Rdbuf[2]);
+	posy1 = ((Rdbuf[5] << 8) | Rdbuf[4]);
+	posx2 = ((Rdbuf[7] << 8) | Rdbuf[6]);
+	posy2 = ((Rdbuf[9] << 8) | Rdbuf[8]);
+
+	input_report_key(tsdata->input, BTN_TOUCH, (touching != 0 ? 1 : 0));
+
+	if (touching == 1) {
+		input_report_abs(tsdata->input, ABS_X, posx1);
+		input_report_abs(tsdata->input, ABS_Y, posy1);
+	}
+
+	if (!(touching)) {
+		z = 0;
+		w = 0;
+	}
+	if (touching == 2) {
+		input_report_abs(tsdata->input, ABS_MT_TOUCH_MAJOR, z);
+		input_report_abs(tsdata->input, ABS_MT_WIDTH_MAJOR, w);
+		input_report_abs(tsdata->input, ABS_MT_POSITION_X, posx1);
+		input_report_abs(tsdata->input, ABS_MT_POSITION_Y, posy1);
+		input_mt_sync(tsdata->input);
+
+		input_report_abs(tsdata->input, ABS_MT_TOUCH_MAJOR, z);
+		input_report_abs(tsdata->input, ABS_MT_WIDTH_MAJOR, w);
+		input_report_abs(tsdata->input, ABS_MT_POSITION_X, posx2);
+		input_report_abs(tsdata->input, ABS_MT_POSITION_Y, posy2);
+		input_mt_sync(tsdata->input);
+	}
+	input_sync(tsdata->input);

Please use the slotted type B protocol instead. If the device can
track fingers, it is very simple to do, see for instance
drivers/input/touchscreen/wacom_w8001.c. If the device cannot do
tracking, please do tell and we will add in pending tracking support
to the input core together with this driver.

Thanks,
Henrik