[PATCH] edt_ts: EDT Touchscreen driver

From: Anatolij Gustshin <agust@denx.de>

Driver for touchscreen controllers found on EDT Displays. Originally
developed by Anatolij Gustshin.

Cc: Anatolij Gustshin <agust@denx.de>
Signed-off-by: Ilya Yanok <redacted>
---
 drivers/input/touchscreen/Kconfig  |   11 ++
 drivers/input/touchscreen/Makefile |    1 +
 drivers/input/touchscreen/edt_ts.c |  340 ++++++++++++++++++++++++++++++++++++
 include/linux/i2c/edt_ts.h         |   20 ++
 4 files changed, 372 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/touchscreen/edt_ts.c
 create mode 100644 include/linux/i2c/edt_ts.h

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 3488ffe..934e6d3 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig

@@ -738,4 +738,15 @@ config TOUCHSCREEN_TPS6507X
 	  To compile this driver as a module, choose M here: the
 	  module will be called tps6507x_ts.
 
+config TOUCHSCREEN_EDT
+	tristate "EDT touchscreen"
+	depends on I2C
+	help
+	  Say Y here to enable EDT touchscreen support.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called edt_ts.
+
 endif

diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index f957676..78ca164 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile

@@ -61,3 +61,4 @@ obj-$(CONFIG_TOUCHSCREEN_WM97XX_MAINSTONE)	+= mainstone-wm97xx.o
 obj-$(CONFIG_TOUCHSCREEN_WM97XX_ZYLONITE)	+= zylonite-wm97xx.o
 obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
 obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
+obj-$(CONFIG_TOUCHSCREEN_EDT)		+= edt_ts.o

diff --git a/drivers/input/touchscreen/edt_ts.c b/drivers/input/touchscreen/edt_ts.c
new file mode 100644
index 0000000..2bf0adc
--- /dev/null
+++ b/drivers/input/touchscreen/edt_ts.c

@@ -0,0 +1,340 @@
+/*
+ * Touch Screen driver for EDT ET070003DM6 display
+ *
+ * Mostly derived from migor_ts driver,
+ * Copyright (c) 2008 Magnus Damm
+ * Copyright (c) 2007 Ujjwal Pande <ujjwal@kenati.com>
+ *
+ * Copyright (c) 2011 DENX Software Engineering,
+ * Anatolij Gustschin <agust@denx.de>
+ *
+ * This file is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU  General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ */
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/gpio.h>
+#include <linux/i2c/edt_ts.h>
+
+#define EDT_EVENT_TOUCH_DOWN	0x0
+#define EDT_EVENT_TOUCH_UP	0x4
+#define EDT_EVENT_TOUCH_ON	0x8
+
+#define EDT_TS_PENUP_TIMEOUT_MS		40
+
+struct edt_ts_priv {
+	struct i2c_client *client;
+	struct timer_list timer;
+	struct input_dev *input;
+	struct delayed_work work;
+	unsigned int up_reported;
+	int irq;
+	unsigned irq_gpio;
+};
+
+void edt_ts_release_event(struct edt_ts_priv *priv)
+{
+	input_report_abs(priv->input, ABS_PRESSURE, 0);
+	input_report_key(priv->input, BTN_TOUCH, 0);
+	input_sync(priv->input);
+	priv->up_reported = 1;
+}
+
+static void edt_ts_read(struct work_struct *work)
+{
+	struct edt_ts_priv *priv;
+	unsigned short xpos[2];
+	unsigned short ypos[2];
+	unsigned char buf[26], sendbuf[1];
+	int event, touch_id;
+	int ret;
+
+	sendbuf[0] = 0xf9;
+
+	priv = container_of(work, struct edt_ts_priv, work.work);
+	memset(buf, 0, sizeof(buf));
+
+	/* skip reading touch data if gpio irq input is high */
+	if (gpio_get_value(priv->irq_gpio))
+		goto fixup_timer;
+
+	ret = i2c_master_send(priv->client, sendbuf, sizeof(sendbuf));
+	if (ret != sizeof(sendbuf)) {
+		dev_err(&priv->client->dev, "can't send 0xf9\n");
+		return;
+	}
+
+	ret = i2c_master_recv(priv->client, buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		dev_err(&priv->client->dev, "can't read pos. data\n");
+		return;
+	}
+
+	for (ret = 0; ret < sizeof(buf); ret++)
+		dev_dbg(&priv->client->dev, "%d: 0x%x\n", ret, buf[ret]);
+
+	event = (buf[5] & 0xf0) >> 4;
+	touch_id = (buf[7] & 0xf0) >> 4;
+
+	dev_dbg(&priv->client->dev, "Event %d, TouchID %d\n", event, touch_id);
+
+	xpos[0] = (((buf[5] & 0xf) << 8) | buf[6]) >> 1;
+	ypos[0] = (((buf[7] & 0xf) << 8) | buf[8]) >> 1;
+
+	if (touch_id == 0 && event & EDT_EVENT_TOUCH_UP) {
+		if (!priv->up_reported)
+			edt_ts_release_event(priv);
+		goto done;
+	}
+
+	if (touch_id == 0 && event & EDT_EVENT_TOUCH_ON) {
+		priv->up_reported = 0;
+		input_report_key(priv->input, BTN_TOUCH, 1);
+		input_report_abs(priv->input, ABS_X, xpos[0]);
+		input_report_abs(priv->input, ABS_Y, ypos[0]);
+		input_report_abs(priv->input, ABS_PRESSURE, 255);
+		input_sync(priv->input);
+	}
+
+fixup_timer:
+	mod_timer(&priv->timer,
+		  jiffies + msecs_to_jiffies(EDT_TS_PENUP_TIMEOUT_MS));
+done:
+	enable_irq(priv->irq);
+}
+
+static void edt_ts_penup_timer(unsigned long handle)
+{
+	struct edt_ts_priv *priv = (void *)handle;
+
+	edt_ts_release_event(priv);
+}
+
+static irqreturn_t edt_ts_isr(int irq, void *dev_id)
+{
+	struct edt_ts_priv *priv = dev_id;
+
+	/* the touch screen controller chip is hooked up to the cpu
+	 * using i2c and a single interrupt line. the interrupt line
+	 * is pulled low whenever someone taps the screen.
+	 *
+	 * we can't read touch data from interrupt context since the i2c
+	 * bus controller may sleep, so we just disable the interrupt
+	 * here and handle it using delayed work.
+	 */
+	disable_irq_nosync(irq);
+	schedule_delayed_work(&priv->work, 0);
+
+	return IRQ_HANDLED;
+}
+
+
+static int edt_ts_open(struct input_dev *dev)
+{
+	struct edt_ts_priv *priv = input_get_drvdata(dev);
+	int ret;
+	char buf[2];
+
+	/* check the controller access */
+	ret = i2c_master_recv(priv->client, buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		dev_err(&priv->client->dev, "controller access failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void edt_ts_close(struct input_dev *dev)
+{
+	struct edt_ts_priv *priv = input_get_drvdata(dev);
+
+	disable_irq(priv->irq);
+
+	/* cancel pending work and wait for edt_ts_read() to finish */
+	if (cancel_delayed_work_sync(&priv->work)) {
+		/*
+		 * if edt_ts_read() was canceled we need to enable IRQ
+		 * here to balance disable done in edt_ts_isr().
+		 */
+		enable_irq(priv->irq);
+	}
+
+	if (del_timer_sync(&priv->timer))
+		edt_ts_release_event(priv);
+
+	enable_irq(priv->irq);
+}
+
+static int edt_ts_probe(struct i2c_client *client,
+			   const struct i2c_device_id *id)
+{
+	struct edt_ts_priv *priv;
+	struct input_dev *input;
+	struct edt_platform_data *pdata = client->dev.platform_data;
+	int error, irq;
+
+	if (!pdata) {
+		dev_err(&client->dev, "no platform data\n");
+		error = -ENODEV;
+		goto err0;
+	}
+
+	if (!gpio_is_valid(pdata->irq_gpio)) {
+		dev_err(&client->dev, "invalid IRQ GPIO number\n");
+		error = -EINVAL;
+		goto err0;
+	}
+
+	irq = gpio_to_irq(pdata->irq_gpio);
+	if (irq < 0) {
+		dev_err(&client->dev, "can't get IRQ for GPIO\n");
+		error = -EINVAL;
+		goto err0;
+	}
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		dev_err(&client->dev, "can't allocate driver data\n");
+		error = -ENOMEM;
+		goto err0;
+	}
+
+	dev_set_drvdata(&client->dev, priv);
+
+	input = input_allocate_device();
+	if (!input) {
+		dev_err(&client->dev, "can't allocate input device.\n");
+		error = -ENOMEM;
+		goto err1;
+	}
+
+	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+
+	input_set_abs_params(input, ABS_X, 0, 800, 0, 0);
+	input_set_abs_params(input, ABS_Y, 0, 480, 0, 0);
+	input_set_abs_params(input, ABS_PRESSURE, 0, 255, 0, 0);
+
+	input->name = client->name;
+	input->id.bustype = BUS_I2C;
+	input->dev.parent = &client->dev;
+
+	input->open = edt_ts_open;
+	input->close = edt_ts_close;
+
+	input_set_drvdata(input, priv);
+
+	priv->client = client;
+	priv->input = input;
+	INIT_DELAYED_WORK(&priv->work, edt_ts_read);
+	priv->irq = irq;
+	priv->irq_gpio = pdata->irq_gpio;
+
+	error = input_register_device(input);
+	if (error)
+		goto err1;
+
+	error = request_irq(priv->irq, edt_ts_isr, IRQF_TRIGGER_FALLING,
+			    client->name, priv);
+	if (error) {
+		dev_err(&client->dev, "can't request IRQ.\n");
+		goto err2;
+	}
+
+	setup_timer(&priv->timer, edt_ts_penup_timer, (unsigned long)priv);
+
+	device_init_wakeup(&client->dev, 1);
+	return 0;
+
+err2:
+	input_unregister_device(input);
+	input = NULL; /* don't free, unregister is enough */
+err1:
+	input_free_device(input);
+	kfree(priv);
+err0:
+	dev_set_drvdata(&client->dev, NULL);
+	return error;
+}
+
+static int edt_ts_remove(struct i2c_client *client)
+{
+	struct edt_ts_priv *priv = dev_get_drvdata(&client->dev);
+
+	free_irq(priv->irq, priv);
+	input_unregister_device(priv->input);
+	kfree(priv);
+	dev_set_drvdata(&client->dev, NULL);
+	device_init_wakeup(&client->dev, 0);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int edt_ts_suspend(struct device *dev)
+{
+	struct edt_ts_priv *priv = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(priv->irq);
+
+	return 0;
+}
+
+static int edt_ts_resume(struct device *dev)
+{
+	struct edt_ts_priv *priv = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(priv->irq);
+
+	return 0;
+}
+
+static const struct dev_pm_ops edt_pm_ops = {
+	.suspend = edt_ts_suspend,
+	.resume = edt_ts_resume,
+};
+#endif
+
+static const struct i2c_device_id edt_ts_idtbl[] = {
+	{ "edt_ts", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, edt_ts_idtbl);
+
+static struct i2c_driver edt_ts_driver = {
+	.driver = {
+		.name = "edt_ts",
+		.owner = THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm = &edt_pm_ops,
+#endif
+	},
+	.probe = edt_ts_probe,
+	.remove = edt_ts_remove,
+	.id_table = edt_ts_idtbl,
+};
+
+static int __init edt_ts_init(void)
+{
+	return i2c_add_driver(&edt_ts_driver);
+}
+module_init(edt_ts_init);
+
+static void __exit edt_ts_exit(void)
+{
+	i2c_del_driver(&edt_ts_driver);
+}
+module_exit(edt_ts_exit);
+
+MODULE_DESCRIPTION("EDT Display Touchscreen driver");
+MODULE_AUTHOR("Anatolij Gustschin <agust@denx.de>");
+MODULE_LICENSE("GPL");

diff --git a/include/linux/i2c/edt_ts.h b/include/linux/i2c/edt_ts.h
new file mode 100644
index 0000000..87048e6
--- /dev/null
+++ b/include/linux/i2c/edt_ts.h

@@ -0,0 +1,20 @@
+/*
+ * EDT Touchscreen driver
+ *
+ * Copyright (C) 2011 Ilya Yanok, EmCraft Systems
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#ifndef __LINUX_I2C_EDT_TS_H
+#define __LINUX_I2C_EDT_TS_H
+
+/* The platform data for the EDT touchscreen driver */
+struct edt_platform_data {
+	unsigned irq_gpio;
+};
+
+#endif /* __LINUX_I2C_EDT_TS_H */

-- 
1.7.6.4

Hi Ilya,

On Wed, Nov 09, 2011 at 12:34:39AM +0100, Ilya Yanok wrote:

We now have nice threaded IRQs that eliminate the need for
manually-created work items, please convert the driver to use them
(see request_threaded_irq).

+
+	return IRQ_HANDLED;
+}
+
+
+static int edt_ts_open(struct input_dev *dev)
+{
+	struct edt_ts_priv *priv = input_get_drvdata(dev);
+	int ret;
+	char buf[2];
+
+	/* check the controller access */
+	ret = i2c_master_recv(priv->client, buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		dev_err(&priv->client->dev, "controller access failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void edt_ts_close(struct input_dev *dev)
+{
+	struct edt_ts_priv *priv = input_get_drvdata(dev);
+
+	disable_irq(priv->irq);
+
+	/* cancel pending work and wait for edt_ts_read() to finish */
+	if (cancel_delayed_work_sync(&priv->work)) {
+		/*
+		 * if edt_ts_read() was canceled we need to enable IRQ
+		 * here to balance disable done in edt_ts_isr().
+		 */
+		enable_irq(priv->irq);
+	}
+
+	if (del_timer_sync(&priv->timer))
+		edt_ts_release_event(priv);
+
+	enable_irq(priv->irq);
+}
+
+static int edt_ts_probe(struct i2c_client *client,
+			   const struct i2c_device_id *id)

__devinit.

+{
+	struct edt_ts_priv *priv;
+	struct input_dev *input;
+	struct edt_platform_data *pdata = client->dev.platform_data;
+	int error, irq;
+
+	if (!pdata) {
+		dev_err(&client->dev, "no platform data\n");
+		error = -ENODEV;
+		goto err0;
+	}
+
+	if (!gpio_is_valid(pdata->irq_gpio)) {
+		dev_err(&client->dev, "invalid IRQ GPIO number\n");
+		error = -EINVAL;
+		goto err0;
+	}
+
+	irq = gpio_to_irq(pdata->irq_gpio);
+	if (irq < 0) {
+		dev_err(&client->dev, "can't get IRQ for GPIO\n");
+		error = -EINVAL;
+		goto err0;
+	}

Why can't we use client->irq?

+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		dev_err(&client->dev, "can't allocate driver data\n");
+		error = -ENOMEM;
+		goto err0;
+	}
+
+	dev_set_drvdata(&client->dev, priv);
+
+	input = input_allocate_device();
+	if (!input) {
+		dev_err(&client->dev, "can't allocate input device.\n");
+		error = -ENOMEM;
+		goto err1;
+	}
+
+	input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+
+	input_set_abs_params(input, ABS_X, 0, 800, 0, 0);
+	input_set_abs_params(input, ABS_Y, 0, 480, 0, 0);
+	input_set_abs_params(input, ABS_PRESSURE, 0, 255, 0, 0);
+
+	input->name = client->name;
+	input->id.bustype = BUS_I2C;
+	input->dev.parent = &client->dev;
+
+	input->open = edt_ts_open;
+	input->close = edt_ts_close;
+
+	input_set_drvdata(input, priv);
+
+	priv->client = client;
+	priv->input = input;
+	INIT_DELAYED_WORK(&priv->work, edt_ts_read);
+	priv->irq = irq;
+	priv->irq_gpio = pdata->irq_gpio;
+
+	error = input_register_device(input);
+	if (error)
+		goto err1;
+
+	error = request_irq(priv->irq, edt_ts_isr, IRQF_TRIGGER_FALLING,
+			    client->name, priv);
+	if (error) {
+		dev_err(&client->dev, "can't request IRQ.\n");
+		goto err2;
+	}
+
+	setup_timer(&priv->timer, edt_ts_penup_timer, (unsigned long)priv);

I think this is racy... IRQ may fire and work get scheduled and executed
before you set up the timer.

+
+	device_init_wakeup(&client->dev, 1);
+	return 0;
+
+err2:
+	input_unregister_device(input);
+	input = NULL; /* don't free, unregister is enough */

If you register input device last you won't need this...

+err1:
+	input_free_device(input);
+	kfree(priv);
+err0:
+	dev_set_drvdata(&client->dev, NULL);

Not needed.

+	return error;
+}
+
+static int edt_ts_remove(struct i2c_client *client)

__devexit.

+{
+	struct edt_ts_priv *priv = dev_get_drvdata(&client->dev);
+
+	free_irq(priv->irq, priv);
+	input_unregister_device(priv->input);
+	kfree(priv);
+	dev_set_drvdata(&client->dev, NULL);
+	device_init_wakeup(&client->dev, 0);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM

CONFIG_PM_SLEEP.

+static int edt_ts_suspend(struct device *dev)
+{
+	struct edt_ts_priv *priv = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(priv->irq);
+
+	return 0;
+}
+
+static int edt_ts_resume(struct device *dev)
+{
+	struct edt_ts_priv *priv = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(priv->irq);
+
+	return 0;
+}
+
+static const struct dev_pm_ops edt_pm_ops = {
+	.suspend = edt_ts_suspend,
+	.resume = edt_ts_resume,
+};
+#endif

static SIMPLE_DEV_PM_OPS(edt_pm_ops, edt_ts_suspend, edt_ts_resume);

+
+static const struct i2c_device_id edt_ts_idtbl[] = {
+	{ "edt_ts", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, edt_ts_idtbl);
+
+static struct i2c_driver edt_ts_driver = {
+	.driver = {
+		.name = "edt_ts",
+		.owner = THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm = &edt_pm_ops,
+#endif

Lose ifdef here. 

+	},
+	.probe = edt_ts_probe,
+	.remove = edt_ts_remove,

__devexit_p().

-- 
Dmitry

Hi Dmitry,

On 14.11.2011 21:28, Dmitry Torokhov wrote:

quoted

+	irq = gpio_to_irq(pdata->irq_gpio);
+	if (irq < 0) {
+		dev_err(&client->dev, "can't get IRQ for GPIO\n");
+		error = -EINVAL;
+		goto err0;
+	}

Why can't we use client->irq?

Well, my idea was to avoid duplication (passing both GPIO and IRQ
numbers) and I can't pass only client->irq as we need GPIO number also
and irq_to_gpio is not guaranteed to work with arbitrary IRQ number.

Thanks for your comments! I'll address them and repost the patch.

Regards, Ilya.

On Tue, Nov 15, 2011 at 01:07:57AM +0400, Ilya Yanok wrote:

Hi Dmitry,

On 14.11.2011 21:28, Dmitry Torokhov wrote:

quoted

quoted

+	irq = gpio_to_irq(pdata->irq_gpio);
+	if (irq < 0) {
+		dev_err(&client->dev, "can't get IRQ for GPIO\n");
+		error = -EINVAL;
+		goto err0;
+	}

Why can't we use client->irq?

Well, my idea was to avoid duplication (passing both GPIO and IRQ
numbers) and I can't pass only client->irq as we need GPIO number also
and irq_to_gpio is not guaranteed to work with arbitrary IRQ number.

Do you really need to read gpio state in IRQ? Can't you simply rely on
'event' do decide whether to emit input events?

Also:

- please do not report fake pressure events since the device does
not seem to support true pressure reading (tslib has been fixed ages
ago).

- consider using i2c_transfer() instead of sequential
  master_send/master_receive.

- What stops the contoller from generating more interrupts/schedule more
  work/re-fire timer after edt_ts_close() is completed? You don't
actually communicate to the controller that ti should shut off.

Thanks.

-- 
Dmitry

Hi Dmitry,

On Mon, 14 Nov 2011 13:32:42 -0800
Dmitry Torokhov [off-list ref] wrote:

On Tue, Nov 15, 2011 at 01:07:57AM +0400, Ilya Yanok wrote:

quoted

Hi Dmitry,

On 14.11.2011 21:28, Dmitry Torokhov wrote:

quoted

quoted

+	irq = gpio_to_irq(pdata->irq_gpio);
+	if (irq < 0) {
+		dev_err(&client->dev, "can't get IRQ for GPIO\n");
+		error = -EINVAL;
+		goto err0;
+	}

Why can't we use client->irq?

Well, my idea was to avoid duplication (passing both GPIO and IRQ
numbers) and I can't pass only client->irq as we need GPIO number also
and irq_to_gpio is not guaranteed to work with arbitrary IRQ number.

Do you really need to read gpio state in IRQ? Can't you simply rely on
'event' do decide whether to emit input events?

We can't rely on the 'event' only. The peculiar behaviour of the
touch controller enforces this. The touch controller toggles the
interrupt line when you keep the finger down. The rate is not exactly
specified by the the manufacturer (~80/sec). Measured rate deviates
from this specified rate. Valid touch packet data can be read when
interrupt line is pulled down. The designer of the target device
insisted on driver implementation reading touch packet data only
when irq line is pulled down.

Also:

- please do not report fake pressure events since the device does
not seem to support true pressure reading (tslib has been fixed ages
ago).

Many projects still use tslib-1.0 (e.g. distributions based on
recent yoctoproject releases). Removing pressure event reporting
will certainly break touch screen support there.

Thanks,

Anatolij

On Tue, Nov 15, 2011 at 10:17:15AM +0100, Anatolij Gustschin wrote:

Hi Dmitry,

On Mon, 14 Nov 2011 13:32:42 -0800
Dmitry Torokhov [off-list ref] wrote:

quoted

On Tue, Nov 15, 2011 at 01:07:57AM +0400, Ilya Yanok wrote:

quoted

Hi Dmitry,

On 14.11.2011 21:28, Dmitry Torokhov wrote:

quoted

quoted

+	irq = gpio_to_irq(pdata->irq_gpio);
+	if (irq < 0) {
+		dev_err(&client->dev, "can't get IRQ for GPIO\n");
+		error = -EINVAL;
+		goto err0;
+	}

Why can't we use client->irq?

Well, my idea was to avoid duplication (passing both GPIO and IRQ
numbers) and I can't pass only client->irq as we need GPIO number also
and irq_to_gpio is not guaranteed to work with arbitrary IRQ number.

Do you really need to read gpio state in IRQ? Can't you simply rely on
'event' do decide whether to emit input events?

We can't rely on the 'event' only. The peculiar behaviour of the
touch controller enforces this. The touch controller toggles the
interrupt line when you keep the finger down. The rate is not exactly
specified by the the manufacturer (~80/sec). Measured rate deviates
from this specified rate. Valid touch packet data can be read when
interrupt line is pulled down. The designer of the target device
insisted on driver implementation reading touch packet data only
when irq line is pulled down.

quoted

Also:

- please do not report fake pressure events since the device does
not seem to support true pressure reading (tslib has been fixed ages
ago).

Many projects still use tslib-1.0 (e.g. distributions based on
recent yoctoproject releases). Removing pressure event reporting
will certainly break touch screen support there.

Then I recommend notifying yoctoproject that they need to update tslib
to something not so ancient.

Thanks.

-- 
Dmitry

On Tue, Nov 15, 2011 at 10:17:15AM +0100, Anatolij Gustschin wrote:

Hi Dmitry,

On Mon, 14 Nov 2011 13:32:42 -0800
Dmitry Torokhov [off-list ref] wrote:

quoted

On Tue, Nov 15, 2011 at 01:07:57AM +0400, Ilya Yanok wrote:

quoted

Hi Dmitry,

On 14.11.2011 21:28, Dmitry Torokhov wrote:

quoted

quoted

+	irq = gpio_to_irq(pdata->irq_gpio);
+	if (irq < 0) {
+		dev_err(&client->dev, "can't get IRQ for GPIO\n");
+		error = -EINVAL;
+		goto err0;
+	}

Why can't we use client->irq?

Well, my idea was to avoid duplication (passing both GPIO and IRQ
numbers) and I can't pass only client->irq as we need GPIO number also
and irq_to_gpio is not guaranteed to work with arbitrary IRQ number.

Do you really need to read gpio state in IRQ? Can't you simply rely on
'event' do decide whether to emit input events?

We can't rely on the 'event' only. The peculiar behaviour of the
touch controller enforces this. The touch controller toggles the
interrupt line when you keep the finger down. The rate is not exactly
specified by the the manufacturer (~80/sec). Measured rate deviates
from this specified rate. Valid touch packet data can be read when
interrupt line is pulled down.

So what ensures that the gpio line stays low between the check and the
time you get access to i2c bus and get around reading the data.

The designer of the target device
insisted on driver implementation reading touch packet data only
when irq line is pulled down.

Well... have you tried checkign whether it is really necessary. Because
if it is not necessary you could do away with the platform data
altogether.

Thanks.

-- 
Dmitry

Hi Dmitry, Anatolij,

On 15.11.2011 21:41, Dmitry Torokhov wrote:

quoted

quoted

Do you really need to read gpio state in IRQ? Can't you simply rely on
'event' do decide whether to emit input events?

We can't rely on the 'event' only. The peculiar behaviour of the
touch controller enforces this. The touch controller toggles the
interrupt line when you keep the finger down. The rate is not exactly
specified by the the manufacturer (~80/sec). Measured rate deviates
from this specified rate. Valid touch packet data can be read when
interrupt line is pulled down.

So what ensures that the gpio line stays low between the check and the
time you get access to i2c bus and get around reading the data.

Anatolij, could you please comment on this? What can we do about this?
Check the pin state after i2c read maybe?

quoted

The designer of the target device
insisted on driver implementation reading touch packet data only
when irq line is pulled down.

Well... have you tried checkign whether it is really necessary. Because
if it is not necessary you could do away with the platform data
altogether.

My understanding is that it's really necessary. We had initial driver
version without this checking and that resulted in some troubles. I'd
like to drop the platform data too, but I think we really need it.

Regards, Ilya.

Hi Ilya,

On Thu, 17 Nov 2011 00:44:17 +0400
Ilya Yanok [off-list ref] wrote:

Hi Dmitry, Anatolij,

On 15.11.2011 21:41, Dmitry Torokhov wrote:

quoted

quoted

quoted

Do you really need to read gpio state in IRQ? Can't you simply rely on
'event' do decide whether to emit input events?

We can't rely on the 'event' only. The peculiar behaviour of the
touch controller enforces this. The touch controller toggles the
interrupt line when you keep the finger down. The rate is not exactly
specified by the the manufacturer (~80/sec). Measured rate deviates
from this specified rate. Valid touch packet data can be read when
interrupt line is pulled down.

So what ensures that the gpio line stays low between the check and the
time you get access to i2c bus and get around reading the data.

Anatolij, could you please comment on this? What can we do about this?
Check the pin state after i2c read maybe?

I received a suggestion to look at the time elapsed since falling
edge of the GPIO IRQ before reading the touch packet data and
skip reading if it is too late. I'll also check what the touch
packet data looks like when reading it while finger is down but
the line stays high. 

Anatolij

Hi Dmitry,

one quick question before I repost the updated patch:

On 14.11.2011 21:28, Dmitry Torokhov wrote:

quoted

+err0:
+	dev_set_drvdata(&client->dev, NULL);

Not needed.

Do you really think leaving a pointer to a non-existing structure is a
good idea?

Regards, Ilya.

On Sun, Nov 20, 2011 at 10:09:24PM +0400, Ilya Yanok wrote:

Hi Dmitry,

one quick question before I repost the updated patch:

On 14.11.2011 21:28, Dmitry Torokhov wrote:

quoted

quoted

+err0:
+	dev_set_drvdata(&client->dev, NULL);

Not needed.

Do you really think leaving a pointer to a non-existing structure is a
good idea?

I2C core takes care of clearing it after remove() or failed probe().

Thanks.

-- 
Dmitry

Hi Dmitry,

On 20.11.2011 22:36, Dmitry Torokhov wrote:

quoted

Do you really think leaving a pointer to a non-existing structure is a
good idea?

I2C core takes care of clearing it after remove() or failed probe().

Ah.. I see. Thanks!

Regards, Ilya.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all.

On 11/09/2011 12:34 AM, Ilya Yanok wrote:

Driver for touchscreen controllers found on EDT Displays. Originally
developed by Anatolij Gustshin.

Sorry for not noticing earlier, but I'd like to point out that there
already is a patch for this family of touch controllers, which also
already provides support for setting parameters and reading raw data
from the sensor.

You can find its most current instance in my mail from September 29th
with the subject "[PATCH v2] Touchscreen driver for FT5x06 based EDT
displays"

Thanks,
        Simon
- -- 
       Simon Budig                        kernel concepts GmbH
       simon.budig@kernelconcepts.de      Sieghuetter Hauptweg 48
       +49-271-771091-17                  D-57072 Siegen

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk7V+L4ACgkQO2O/RXesiHDVBQCgkRFIGTq4d6EuxCti+qz992cg
a3cAoMNY7hOFQqx8IJnDuLOrn/QJnQhZ
=HLEn
-----END PGP SIGNATURE-----

Hi Simon,

On 30.11.2011 13:34, Simon Budig wrote:

quoted

Driver for touchscreen controllers found on EDT Displays. Originally
developed by Anatolij Gustshin.

Sorry for not noticing earlier, but I'd like to point out that there
already is a patch for this family of touch controllers, which also
already provides support for setting parameters and reading raw data
from the sensor.

Great. Are you going to address the comments and resend your patch? I'd
really like to see some version of the driver accepted into the mainline.

Regards, Ilya.