[PATCH v4] Input: add ST1232 touchscreen controller driver.

[PATCH v4] Input: add ST1232 touchscreen controller driver.

[chinyeow.sim.xt]

From: Tony SIM <chinyeow.sim.xt@renesas.com>

This patch set introduces for Sitronix ST1232 touchscreen controller
driver. This is an integrated capacitive touchscreen with LCD module
which can detect two fingers's touch X/Y coordinate.

Signed-off-by: Tony SIM <chinyeow.sim.xt@renesas.com>
---

Thanks for review!

Changes since v3:
- Removed unused variable.
- Changed touch counting flag.

 drivers/input/touchscreen/Kconfig  |   11 ++
 drivers/input/touchscreen/Makefile |    1 +
 drivers/input/touchscreen/st1232.c |  273 ++++++++++++++++++++++++++++++++++++
 3 files changed, 285 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/touchscreen/st1232.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 06ea8da..3ca7700 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -681,4 +681,15 @@ config TOUCHSCREEN_STMPE
 	  To compile this driver as a module, choose M here: the
 	  module will be called stmpe-ts.
 
+config TOUCHSCREEN_ST1232
+	tristate "Sitronix ST1232 touchscreen controllers"
+	depends on I2C
+	help
+	  Say Y here if you want to support Sitronix ST1232
+	  touchscreen controller.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called st1232_ts.
 endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 7cc1b4f..718bcc8 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_TOUCHSCREEN_PCAP)		+= pcap_ts.o
 obj-$(CONFIG_TOUCHSCREEN_PENMOUNT)	+= penmount.o
 obj-$(CONFIG_TOUCHSCREEN_QT602240)	+= qt602240_ts.o
 obj-$(CONFIG_TOUCHSCREEN_S3C2410)	+= s3c2410_ts.o
+obj-$(CONFIG_TOUCHSCREEN_ST1232)	+= st1232.o
 obj-$(CONFIG_TOUCHSCREEN_STMPE)		+= stmpe-ts.o
 obj-$(CONFIG_TOUCHSCREEN_TNETV107X)	+= tnetv107x-ts.o
 obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213)	+= touchit213.o
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
new file mode 100644
index 0000000..f94a718
--- /dev/null
+++ b/drivers/input/touchscreen/st1232.c
@@ -0,0 +1,273 @@
+/*
+ * ST1232 Touchscreen Controller Driver
+ *
+ * Copyright (C) 2010 Renesas Solutions Corp.
+ *	Tony SIM <chinyeow.sim.xt@renesas.com>
+ *
+ * Using code from:
+ *  - android.git.kernel.org: projects/kernel/common.git: synaptics_i2c_rmi.c
+ *	Copyright (C) 2007 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#define ST1232_TS_NAME	"st1232-ts"
+
+#define MIN_X	0x00
+#define MIN_Y	0x00
+#define MAX_X	0x31f	/* (800 - 1) */
+#define MAX_Y	0x1df	/* (480 - 1) */
+#define MAX_AREA	0xff
+#define MAX_FINGERS	2
+
+struct st1232_ts_finger {
+	uint8_t is_valid;
+	uint16_t x;
+	uint16_t y;
+	uint8_t t;
+};
+
+struct st1232_ts_data {
+	struct i2c_client *client;
+	struct input_dev *input_dev;
+	struct st1232_ts_finger finger[MAX_FINGERS];
+};
+
+static int st1232_ts_read_data(struct st1232_ts_data *ts)
+{
+	int ret;
+	struct i2c_msg msg[2];
+	uint8_t start_reg;
+	uint8_t buf[10];
+	struct st1232_ts_finger *finger = ts->finger;
+
+	/* read touchscreen data from ST1232 */
+	msg[0].addr = ts->client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = &start_reg;
+	start_reg = 0x10;
+
+	msg[1].addr = ts->client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = sizeof(buf);
+	msg[1].buf = buf;
+
+	ret = i2c_transfer(ts->client->adapter, msg, 2);
+	if (ret < 0)
+		return ret;
+
+	/* get valid bit */
+	finger[0].is_valid = buf[2] >> 7;
+	finger[1].is_valid = buf[5] >> 7;
+
+	/* get xy coordinate */
+	if (finger[0].is_valid) {
+		finger[0].x = ((buf[2] & 0x0070) << 4) | buf[3];
+		finger[0].y = ((buf[2] & 0x0007) << 8) | buf[4];
+		finger[0].t = buf[8];
+	}
+
+	if (finger[1].is_valid) {
+		finger[1].x = ((buf[5] & 0x0070) << 4) | buf[6];
+		finger[1].y = ((buf[5] & 0x0007) << 8) | buf[7];
+		finger[1].t = buf[9];
+	}
+
+	return 0;
+}
+
+static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
+{
+	int i, ret;
+	int count = 0;
+	struct st1232_ts_data *ts = dev_id;
+	struct st1232_ts_finger *finger = ts->finger;
+
+	ret = st1232_ts_read_data(ts);
+	if (ret < 0)
+		goto end;
+
+	/* multi touch protocol */
+	for (i = 0; i < MAX_FINGERS; i++) {
+		if (!finger[i].is_valid)
+			continue;
+
+		input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR,
+					finger[i].t);
+		input_report_abs(ts->input_dev, ABS_MT_POSITION_X, finger[i].x);
+		input_report_abs(ts->input_dev, ABS_MT_POSITION_Y, finger[i].y);
+		input_mt_sync(ts->input_dev);
+		count++;
+	}
+
+	/* SYN_MT_REPORT only if no contact */
+	if (!count)
+		input_mt_sync(ts->input_dev);
+
+	/* SYN_REPORT */
+	input_sync(ts->input_dev);
+
+end:
+	return IRQ_HANDLED;
+}
+
+static int __devinit st1232_ts_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	struct st1232_ts_data *ts;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
+		return -EIO;
+	}
+
+	ts = kzalloc(sizeof(struct st1232_ts_data), GFP_KERNEL);
+	if (!ts) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	ts->client = client;
+	i2c_set_clientdata(client, ts);
+
+	ts->input_dev = input_allocate_device();
+	if (!ts->input_dev) {
+		ret = -ENOMEM;
+		dev_err(&client->dev, "Failed to allocate input device\n");
+		goto err_free_mem;
+	}
+	ts->input_dev->name = "st1232-touchscreen";
+
+	__set_bit(EV_SYN, ts->input_dev->evbit);
+	__set_bit(EV_KEY, ts->input_dev->evbit);
+	__set_bit(EV_ABS, ts->input_dev->evbit);
+
+	input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR,
+				0, MAX_AREA, 0, 0);
+	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
+				MIN_X, MAX_X, 0, 0);
+	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
+				MIN_Y, MAX_Y, 0, 0);
+
+	ret = input_register_device(ts->input_dev);
+	if (ret) {
+		dev_err(&client->dev, "Unable to register %s input device\n",
+			ts->input_dev->name);
+		goto err_free_input_device;
+	}
+
+	ret = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
+					IRQF_ONESHOT, client->name, ts);
+	if (ret) {
+		dev_err(&client->dev, "Failed to register interrupt\n");
+		goto err_free_input_device;
+	}
+
+	device_init_wakeup(&client->dev, 1);
+
+	return 0;
+
+err_free_input_device:
+	input_free_device(ts->input_dev);
+err_free_mem:
+	kfree(ts);
+err:
+	return ret;
+}
+
+static int __devexit st1232_ts_remove(struct i2c_client *client)
+{
+	struct st1232_ts_data *ts = i2c_get_clientdata(client);
+
+	device_init_wakeup(&client->dev, 0);
+	free_irq(client->irq, ts);
+	input_unregister_device(ts->input_dev);
+	kfree(ts);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int st1232_ts_suspend(struct device *dev)
+{
+	struct st1232_ts_data *ts = dev_get_drvdata(dev);
+	struct i2c_client *client = ts->client;
+
+	if (device_may_wakeup(&client->dev))
+		enable_irq_wake(client->irq);
+	else
+		disable_irq(client->irq);
+
+	return 0;
+}
+
+static int st1232_ts_resume(struct device *dev)
+{
+	struct st1232_ts_data *ts = dev_get_drvdata(dev);
+	struct i2c_client *client = ts->client;
+
+	if (device_may_wakeup(&client->dev))
+		disable_irq_wake(client->irq);
+	else
+		enable_irq(client->irq);
+
+	return 0;
+}
+
+static const struct dev_pm_ops st1232_ts_pm_ops = {
+	.suspend	= st1232_ts_suspend,
+	.resume		= st1232_ts_resume,
+};
+#endif
+
+static const struct i2c_device_id st1232_ts_id[] = {
+	{ ST1232_TS_NAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, st1232_ts_id);
+
+static struct i2c_driver st1232_ts_driver = {
+	.probe		= st1232_ts_probe,
+	.remove		= __devexit_p(st1232_ts_remove),
+	.id_table	= st1232_ts_id,
+	.driver = {
+		.name	= ST1232_TS_NAME,
+		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm	= &st1232_ts_pm_ops,
+#endif
+	},
+};
+
+static int __init st1232_ts_init(void)
+{
+	return i2c_add_driver(&st1232_ts_driver);
+}
+module_init(st1232_ts_init);
+
+static void __exit st1232_ts_exit(void)
+{
+	i2c_del_driver(&st1232_ts_driver);
+}
+module_exit(st1232_ts_exit);
+
+MODULE_AUTHOR("Tony SIM <chinyeow.sim.xt@renesas.com>");
+MODULE_DESCRIPTION("SITRONIX ST1232 Touchscreen Controller Driver");
+MODULE_LICENSE("GPL");
-- 
1.7.2.3

Re: [PATCH v4] Input: add ST1232 touchscreen controller driver.

[Dmitry Torokhov]

Hi Tony,

On Thu, Dec 16, 2010 at 09:45:25AM +0900, chinyeow.sim.xt@renesas.com wrote:
> From: Tony SIM <chinyeow.sim.xt@renesas.com>
> 
> This patch set introduces for Sitronix ST1232 touchscreen controller
> driver. This is an integrated capacitive touchscreen with LCD module
> which can detect two fingers's touch X/Y coordinate.
> 
> Signed-off-by: Tony SIM <chinyeow.sim.xt@renesas.com>
> ---
> 
> Thanks for review!
> 
> Changes since v3:
> - Removed unused variable.
> - Changed touch counting flag.

This looks great now, just a couple more comments:

> +
> +struct st1232_ts_finger {
> +	uint8_t is_valid;
> +	uint16_t x;
> +	uint16_t y;
> +	uint8_t t;
> +};

If you regroup fields the structure can be packed better. Also we prefer
u8, u16 instead of uint8_t, uint16_t in the kernel.

> +	ts->input_dev->name = "st1232-touchscreen";

	Also need to set input->id.bus = BUS_I2C and parent device.

> +
> +	__set_bit(EV_SYN, ts->input_dev->evbit);
> +	__set_bit(EV_KEY, ts->input_dev->evbit);
> +	__set_bit(EV_ABS, ts->input_dev->evbit);
> +
> +	input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR,
> +				0, MAX_AREA, 0, 0);
> +	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
> +				MIN_X, MAX_X, 0, 0);
> +	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
> +				MIN_Y, MAX_Y, 0, 0);
> +
> +	ret = input_register_device(ts->input_dev);
> +	if (ret) {
> +		dev_err(&client->dev, "Unable to register %s input device\n",
> +			ts->input_dev->name);
> +		goto err_free_input_device;
> +	}
> +
> +	ret = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
> +					IRQF_ONESHOT, client->name, ts);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to register interrupt\n");
> +		goto err_free_input_device;

Registered devices should be destroyed by call to
input_unregister_device() instead of input_free_device().

Does the following (applied on top of your patch) still work? Thanks!

-- 
Dmitry

Input: st1232 - assorted changes

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/touchscreen/Kconfig  |   21 +++---
 drivers/input/touchscreen/st1232.c |  133 ++++++++++++++++++------------------
 2 files changed, 78 insertions(+), 76 deletions(-)


diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 3ca7700..07ac77d 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -659,17 +659,17 @@ config TOUCHSCREEN_PCAP
 	  To compile this driver as a module, choose M here: the
 	  module will be called pcap_ts.
 
-config TOUCHSCREEN_TPS6507X
-	tristate "TPS6507x based touchscreens"
+config TOUCHSCREEN_ST1232
+	tristate "Sitronix ST1232 touchscreen controllers"
 	depends on I2C
 	help
-	  Say Y here if you have a TPS6507x based touchscreen
-	  controller.
+	  Say Y here if you want to support Sitronix ST1232
+	  touchscreen controller.
 
 	  If unsure, say N.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called tps6507x_ts.
+	  module will be called st1232_ts.
 
 config TOUCHSCREEN_STMPE
 	tristate "STMicroelectronics STMPE touchscreens"
@@ -681,15 +681,16 @@ config TOUCHSCREEN_STMPE
 	  To compile this driver as a module, choose M here: the
 	  module will be called stmpe-ts.
 
-config TOUCHSCREEN_ST1232
-	tristate "Sitronix ST1232 touchscreen controllers"
+config TOUCHSCREEN_TPS6507X
+	tristate "TPS6507x based touchscreens"
 	depends on I2C
 	help
-	  Say Y here if you want to support Sitronix ST1232
-	  touchscreen controller.
+	  Say Y here if you have a TPS6507x based touchscreen
+	  controller.
 
 	  If unsure, say N.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called st1232_ts.
+	  module will be called tps6507x_ts.
+
 endif
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index f94a718..4ab3713 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -16,7 +16,6 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
- *
  */
 
 #include <linux/delay.h>
@@ -25,21 +24,22 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/types.h>
 
 #define ST1232_TS_NAME	"st1232-ts"
 
-#define MIN_X	0x00
-#define MIN_Y	0x00
-#define MAX_X	0x31f	/* (800 - 1) */
-#define MAX_Y	0x1df	/* (480 - 1) */
+#define MIN_X		0x00
+#define MIN_Y		0x00
+#define MAX_X		0x31f	/* (800 - 1) */
+#define MAX_Y		0x1df	/* (480 - 1) */
 #define MAX_AREA	0xff
 #define MAX_FINGERS	2
 
 struct st1232_ts_finger {
-	uint8_t is_valid;
-	uint16_t x;
-	uint16_t y;
-	uint8_t t;
+	u16 x;
+	u16 y;
+	u8 t;
+	bool is_valid;
 };
 
 struct st1232_ts_data {
@@ -50,14 +50,15 @@ struct st1232_ts_data {
 
 static int st1232_ts_read_data(struct st1232_ts_data *ts)
 {
-	int ret;
-	struct i2c_msg msg[2];
-	uint8_t start_reg;
-	uint8_t buf[10];
 	struct st1232_ts_finger *finger = ts->finger;
+	struct i2c_client *client = ts->client;
+	struct i2c_msg msg[2];
+	int error;
+	u8 start_reg;
+	u8 buf[10];
 
 	/* read touchscreen data from ST1232 */
-	msg[0].addr = ts->client->addr;
+	msg[0].addr = client->addr;
 	msg[0].flags = 0;
 	msg[0].len = 1;
 	msg[0].buf = &start_reg;
@@ -68,11 +69,11 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
 	msg[1].len = sizeof(buf);
 	msg[1].buf = buf;
 
-	ret = i2c_transfer(ts->client->adapter, msg, 2);
-	if (ret < 0)
-		return ret;
+	error = i2c_transfer(client->adapter, msg, 2);
+	if (error < 0)
+		return error;
 
-	/* get valid bit */
+	/* get "valid" bits */
 	finger[0].is_valid = buf[2] >> 7;
 	finger[1].is_valid = buf[5] >> 7;
 
@@ -94,10 +95,11 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
 
 static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
 {
-	int i, ret;
-	int count = 0;
 	struct st1232_ts_data *ts = dev_id;
 	struct st1232_ts_finger *finger = ts->finger;
+	struct input_dev *input_dev = ts->input_dev;
+	int count = 0;
+	int i, ret;
 
 	ret = st1232_ts_read_data(ts);
 	if (ret < 0)
@@ -108,20 +110,19 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
 		if (!finger[i].is_valid)
 			continue;
 
-		input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR,
-					finger[i].t);
-		input_report_abs(ts->input_dev, ABS_MT_POSITION_X, finger[i].x);
-		input_report_abs(ts->input_dev, ABS_MT_POSITION_Y, finger[i].y);
-		input_mt_sync(ts->input_dev);
+		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, finger[i].t);
+		input_report_abs(input_dev, ABS_MT_POSITION_X, finger[i].x);
+		input_report_abs(input_dev, ABS_MT_POSITION_Y, finger[i].y);
+		input_mt_sync(input_dev);
 		count++;
 	}
 
 	/* SYN_MT_REPORT only if no contact */
 	if (!count)
-		input_mt_sync(ts->input_dev);
+		input_mt_sync(input_dev);
 
 	/* SYN_REPORT */
-	input_sync(ts->input_dev);
+	input_sync(input_dev);
 
 end:
 	return IRQ_HANDLED;
@@ -131,65 +132,67 @@ static int __devinit st1232_ts_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
 	struct st1232_ts_data *ts;
-	int ret;
+	struct input_dev *input_dev;
+	int error;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
 		return -EIO;
 	}
 
-	ts = kzalloc(sizeof(struct st1232_ts_data), GFP_KERNEL);
-	if (!ts) {
-		ret = -ENOMEM;
-		goto err;
+	if (!client->irq) {
+		dev_err(&client->dev, "no IRQ?\n");
+		return -EINVAL;
 	}
 
-	ts->client = client;
-	i2c_set_clientdata(client, ts);
 
-	ts->input_dev = input_allocate_device();
-	if (!ts->input_dev) {
-		ret = -ENOMEM;
-		dev_err(&client->dev, "Failed to allocate input device\n");
+	ts = kzalloc(sizeof(struct st1232_ts_data), GFP_KERNEL);
+	input_dev = input_allocate_device();
+	if (!ts || !input_dev) {
+		error = -ENOMEM;
 		goto err_free_mem;
 	}
-	ts->input_dev->name = "st1232-touchscreen";
 
-	__set_bit(EV_SYN, ts->input_dev->evbit);
-	__set_bit(EV_KEY, ts->input_dev->evbit);
-	__set_bit(EV_ABS, ts->input_dev->evbit);
+	ts->client = client;
+	ts->input_dev = input_dev;
 
-	input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR,
-				0, MAX_AREA, 0, 0);
-	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
-				MIN_X, MAX_X, 0, 0);
-	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
-				MIN_Y, MAX_Y, 0, 0);
+	input_dev->name = "st1232-touchscreen";
+	input_dev->id.bustype = BUS_I2C;
+	input_dev->dev.parent = &client->dev;
 
-	ret = input_register_device(ts->input_dev);
-	if (ret) {
-		dev_err(&client->dev, "Unable to register %s input device\n",
-			ts->input_dev->name);
-		goto err_free_input_device;
-	}
+	__set_bit(EV_SYN, input_dev->evbit);
+	__set_bit(EV_KEY, input_dev->evbit);
+	__set_bit(EV_ABS, input_dev->evbit);
 
-	ret = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
-					IRQF_ONESHOT, client->name, ts);
-	if (ret) {
+	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X, MAX_X, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y, MAX_Y, 0, 0);
+
+	error = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
+				     IRQF_ONESHOT, client->name, ts);
+	if (error) {
 		dev_err(&client->dev, "Failed to register interrupt\n");
-		goto err_free_input_device;
+		goto err_free_mem;
 	}
 
+	error = input_register_device(ts->input_dev);
+	if (error) {
+		dev_err(&client->dev, "Unable to register %s input device\n",
+			input_dev->name);
+		goto err_free_irq;
+	}
+
+	i2c_set_clientdata(client, ts);
 	device_init_wakeup(&client->dev, 1);
 
 	return 0;
 
-err_free_input_device:
-	input_free_device(ts->input_dev);
+err_free_irq:
+	free_irq(client->irq, ts);
 err_free_mem:
+	input_free_device(input_dev);
 	kfree(ts);
-err:
-	return ret;
+	return error;
 }
 
 static int __devexit st1232_ts_remove(struct i2c_client *client)
@@ -207,8 +210,7 @@ static int __devexit st1232_ts_remove(struct i2c_client *client)
 #ifdef CONFIG_PM
 static int st1232_ts_suspend(struct device *dev)
 {
-	struct st1232_ts_data *ts = dev_get_drvdata(dev);
-	struct i2c_client *client = ts->client;
+	struct i2c_client *client = to_i2c_client(dev);
 
 	if (device_may_wakeup(&client->dev))
 		enable_irq_wake(client->irq);
@@ -220,8 +222,7 @@ static int st1232_ts_suspend(struct device *dev)
 
 static int st1232_ts_resume(struct device *dev)
 {
-	struct st1232_ts_data *ts = dev_get_drvdata(dev);
-	struct i2c_client *client = ts->client;
+	struct i2c_client *client = to_i2c_client(dev);
 
 	if (device_may_wakeup(&client->dev))
 		disable_irq_wake(client->irq);

Re: [PATCH v4] Input: add ST1232 touchscreen controller driver.

[Trilok Soni]

Hi Tony,

On 12/16/2010 6:15 AM, chinyeow.sim.xt@renesas.com wrote:
> From: Tony SIM <chinyeow.sim.xt@renesas.com>
> 
> This patch set introduces for Sitronix ST1232 touchscreen controller
> driver. This is an integrated capacitive touchscreen with LCD module
> which can detect two fingers's touch X/Y coordinate.
> 
> Signed-off-by: Tony SIM <chinyeow.sim.xt@renesas.com>

Thanks. Looks good to me.

Reviewed-by: Trilok Soni <tsoni@codeaurora.org>

---Trilok Soni

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

RE: [PATCH v4] Input: add ST1232 touchscreen controller driver.

[chinyeow.sim.xt]

Hi Dmitry,

Thanks for modifing the changes! It works well!

/ Tony

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] 
> Sent: Thursday, December 16, 2010 3:19 PM
> To: chinyeow.sim.xt@renesas.com
> Cc: tsoni@codeaurora.org; rydberg@euromail.se; 
> linux-input@vger.kernel.org
> Subject: Re: [PATCH v4] Input: add ST1232 touchscreen 
> controller driver.
> 
> 
> Hi Tony,
> 
> On Thu, Dec 16, 2010 at 09:45:25AM +0900, 
> chinyeow.sim.xt@renesas.com wrote:
> > From: Tony SIM <chinyeow.sim.xt@renesas.com>
> > 
> > This patch set introduces for Sitronix ST1232 touchscreen controller
> > driver. This is an integrated capacitive touchscreen with LCD module
> > which can detect two fingers's touch X/Y coordinate.
> > 
> > Signed-off-by: Tony SIM <chinyeow.sim.xt@renesas.com>
> > ---
> > 
> > Thanks for review!
> > 
> > Changes since v3:
> > - Removed unused variable.
> > - Changed touch counting flag.
> 
> This looks great now, just a couple more comments:
> 
> > +
> > +struct st1232_ts_finger {
> > +	uint8_t is_valid;
> > +	uint16_t x;
> > +	uint16_t y;
> > +	uint8_t t;
> > +};
> 
> If you regroup fields the structure can be packed better. 
> Also we prefer
> u8, u16 instead of uint8_t, uint16_t in the kernel.
> 
> > +	ts->input_dev->name = "st1232-touchscreen";
> 
> 	Also need to set input->id.bus = BUS_I2C and parent device.
> 
> > +
> > +	__set_bit(EV_SYN, ts->input_dev->evbit);
> > +	__set_bit(EV_KEY, ts->input_dev->evbit);
> > +	__set_bit(EV_ABS, ts->input_dev->evbit);
> > +
> > +	input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR,
> > +				0, MAX_AREA, 0, 0);
> > +	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
> > +				MIN_X, MAX_X, 0, 0);
> > +	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
> > +				MIN_Y, MAX_Y, 0, 0);
> > +
> > +	ret = input_register_device(ts->input_dev);
> > +	if (ret) {
> > +		dev_err(&client->dev, "Unable to register %s input device\n",
> > +			ts->input_dev->name);
> > +		goto err_free_input_device;
> > +	}
> > +
> > +	ret = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
> > +					IRQF_ONESHOT, client->name, ts);
> > +	if (ret) {
> > +		dev_err(&client->dev, "Failed to register interrupt\n");
> > +		goto err_free_input_device;
> 
> Registered devices should be destroyed by call to
> input_unregister_device() instead of input_free_device().
> 
> Does the following (applied on top of your patch) still work? Thanks!
> 
> -- 
> Dmitry
> 
> Input: st1232 - assorted changes
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
>  drivers/input/touchscreen/Kconfig  |   21 +++---
>  drivers/input/touchscreen/st1232.c |  133 
> ++++++++++++++++++------------------
>  2 files changed, 78 insertions(+), 76 deletions(-)
> 
> 
> diff --git a/drivers/input/touchscreen/Kconfig 
> b/drivers/input/touchscreen/Kconfig
> index 3ca7700..07ac77d 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -659,17 +659,17 @@ config TOUCHSCREEN_PCAP
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called pcap_ts.
>  
> -config TOUCHSCREEN_TPS6507X
> -	tristate "TPS6507x based touchscreens"
> +config TOUCHSCREEN_ST1232
> +	tristate "Sitronix ST1232 touchscreen controllers"
>  	depends on I2C
>  	help
> -	  Say Y here if you have a TPS6507x based touchscreen
> -	  controller.
> +	  Say Y here if you want to support Sitronix ST1232
> +	  touchscreen controller.
>  
>  	  If unsure, say N.
>  
>  	  To compile this driver as a module, choose M here: the
> -	  module will be called tps6507x_ts.
> +	  module will be called st1232_ts.
>  
>  config TOUCHSCREEN_STMPE
>  	tristate "STMicroelectronics STMPE touchscreens"
> @@ -681,15 +681,16 @@ config TOUCHSCREEN_STMPE
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stmpe-ts.
>  
> -config TOUCHSCREEN_ST1232
> -	tristate "Sitronix ST1232 touchscreen controllers"
> +config TOUCHSCREEN_TPS6507X
> +	tristate "TPS6507x based touchscreens"
>  	depends on I2C
>  	help
> -	  Say Y here if you want to support Sitronix ST1232
> -	  touchscreen controller.
> +	  Say Y here if you have a TPS6507x based touchscreen
> +	  controller.
>  
>  	  If unsure, say N.
>  
>  	  To compile this driver as a module, choose M here: the
> -	  module will be called st1232_ts.
> +	  module will be called tps6507x_ts.
> +
>  endif
> diff --git a/drivers/input/touchscreen/st1232.c 
> b/drivers/input/touchscreen/st1232.c
> index f94a718..4ab3713 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c
> @@ -16,7 +16,6 @@
>   * but WITHOUT ANY WARRANTY; without even the implied warranty of
>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   * GNU General Public License for more details.
> - *
>   */
>  
>  #include <linux/delay.h>
> @@ -25,21 +24,22 @@
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/types.h>
>  
>  #define ST1232_TS_NAME	"st1232-ts"
>  
> -#define MIN_X	0x00
> -#define MIN_Y	0x00
> -#define MAX_X	0x31f	/* (800 - 1) */
> -#define MAX_Y	0x1df	/* (480 - 1) */
> +#define MIN_X		0x00
> +#define MIN_Y		0x00
> +#define MAX_X		0x31f	/* (800 - 1) */
> +#define MAX_Y		0x1df	/* (480 - 1) */
>  #define MAX_AREA	0xff
>  #define MAX_FINGERS	2
>  
>  struct st1232_ts_finger {
> -	uint8_t is_valid;
> -	uint16_t x;
> -	uint16_t y;
> -	uint8_t t;
> +	u16 x;
> +	u16 y;
> +	u8 t;
> +	bool is_valid;
>  };
>  
>  struct st1232_ts_data {
> @@ -50,14 +50,15 @@ struct st1232_ts_data {
>  
>  static int st1232_ts_read_data(struct st1232_ts_data *ts)
>  {
> -	int ret;
> -	struct i2c_msg msg[2];
> -	uint8_t start_reg;
> -	uint8_t buf[10];
>  	struct st1232_ts_finger *finger = ts->finger;
> +	struct i2c_client *client = ts->client;
> +	struct i2c_msg msg[2];
> +	int error;
> +	u8 start_reg;
> +	u8 buf[10];
>  
>  	/* read touchscreen data from ST1232 */
> -	msg[0].addr = ts->client->addr;
> +	msg[0].addr = client->addr;
>  	msg[0].flags = 0;
>  	msg[0].len = 1;
>  	msg[0].buf = &start_reg;
> @@ -68,11 +69,11 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
>  	msg[1].len = sizeof(buf);
>  	msg[1].buf = buf;
>  
> -	ret = i2c_transfer(ts->client->adapter, msg, 2);
> -	if (ret < 0)
> -		return ret;
> +	error = i2c_transfer(client->adapter, msg, 2);
> +	if (error < 0)
> +		return error;
>  
> -	/* get valid bit */
> +	/* get "valid" bits */
>  	finger[0].is_valid = buf[2] >> 7;
>  	finger[1].is_valid = buf[5] >> 7;
>  
> @@ -94,10 +95,11 @@ static int st1232_ts_read_data(struct st1232_ts_data *ts)
>  
>  static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
>  {
> -	int i, ret;
> -	int count = 0;
>  	struct st1232_ts_data *ts = dev_id;
>  	struct st1232_ts_finger *finger = ts->finger;
> +	struct input_dev *input_dev = ts->input_dev;
> +	int count = 0;
> +	int i, ret;
>  
>  	ret = st1232_ts_read_data(ts);
>  	if (ret < 0)
> @@ -108,20 +110,19 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
>  		if (!finger[i].is_valid)
>  			continue;
>  
> -		input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR,
> -					finger[i].t);
> -		input_report_abs(ts->input_dev, ABS_MT_POSITION_X, finger[i].x);
> -		input_report_abs(ts->input_dev, ABS_MT_POSITION_Y, finger[i].y);
> -		input_mt_sync(ts->input_dev);
> +		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, finger[i].t);
> +		input_report_abs(input_dev, ABS_MT_POSITION_X, finger[i].x);
> +		input_report_abs(input_dev, ABS_MT_POSITION_Y, finger[i].y);
> +		input_mt_sync(input_dev);
>  		count++;
>  	}
>  
>  	/* SYN_MT_REPORT only if no contact */
>  	if (!count)
> -		input_mt_sync(ts->input_dev);
> +		input_mt_sync(input_dev);
>  
>  	/* SYN_REPORT */
> -	input_sync(ts->input_dev);
> +	input_sync(input_dev);
>  
>  end:
>  	return IRQ_HANDLED;
> @@ -131,65 +132,67 @@ static int __devinit st1232_ts_probe(struct i2c_client *client,
>  					const struct i2c_device_id *id)
>  {
>  	struct st1232_ts_data *ts;
> -	int ret;
> +	struct input_dev *input_dev;
> +	int error;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>  		dev_err(&client->dev, "need I2C_FUNC_I2C\n");
>  		return -EIO;
>  	}
>  
> -	ts = kzalloc(sizeof(struct st1232_ts_data), GFP_KERNEL);
> -	if (!ts) {
> -		ret = -ENOMEM;
> -		goto err;
> +	if (!client->irq) {
> +		dev_err(&client->dev, "no IRQ?\n");
> +		return -EINVAL;
>  	}
>  
> -	ts->client = client;
> -	i2c_set_clientdata(client, ts);
>  
> -	ts->input_dev = input_allocate_device();
> -	if (!ts->input_dev) {
> -		ret = -ENOMEM;
> -		dev_err(&client->dev, "Failed to allocate input device\n");
> +	ts = kzalloc(sizeof(struct st1232_ts_data), GFP_KERNEL);
> +	input_dev = input_allocate_device();
> +	if (!ts || !input_dev) {
> +		error = -ENOMEM;
>  		goto err_free_mem;
>  	}
> -	ts->input_dev->name = "st1232-touchscreen";
>  
> -	__set_bit(EV_SYN, ts->input_dev->evbit);
> -	__set_bit(EV_KEY, ts->input_dev->evbit);
> -	__set_bit(EV_ABS, ts->input_dev->evbit);
> +	ts->client = client;
> +	ts->input_dev = input_dev;
>  
> -	input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR,
> -				0, MAX_AREA, 0, 0);
> -	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_X,
> -				MIN_X, MAX_X, 0, 0);
> -	input_set_abs_params(ts->input_dev, ABS_MT_POSITION_Y,
> -				MIN_Y, MAX_Y, 0, 0);
> +	input_dev->name = "st1232-touchscreen";
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = &client->dev;
>  
> -	ret = input_register_device(ts->input_dev);
> -	if (ret) {
> -		dev_err(&client->dev, "Unable to register %s input device\n",
> -			ts->input_dev->name);
> -		goto err_free_input_device;
> -	}
> +	__set_bit(EV_SYN, input_dev->evbit);
> +	__set_bit(EV_KEY, input_dev->evbit);
> +	__set_bit(EV_ABS, input_dev->evbit);
>  
> -	ret = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
> -					IRQF_ONESHOT, client->name, ts);
> -	if (ret) {
> +	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X, MAX_X, 0, 0);
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, MIN_Y, MAX_Y, 0, 0);
> +
> +	error = request_threaded_irq(client->irq, NULL, st1232_ts_irq_handler,
> +				     IRQF_ONESHOT, client->name, ts);
> +	if (error) {
>  		dev_err(&client->dev, "Failed to register interrupt\n");
> -		goto err_free_input_device;
> +		goto err_free_mem;
>  	}
>  
> +	error = input_register_device(ts->input_dev);
> +	if (error) {
> +		dev_err(&client->dev, "Unable to register %s input device\n",
> +			input_dev->name);
> +		goto err_free_irq;
> +	}
> +
> +	i2c_set_clientdata(client, ts);
>  	device_init_wakeup(&client->dev, 1);
>  
>  	return 0;
>  
> -err_free_input_device:
> -	input_free_device(ts->input_dev);
> +err_free_irq:
> +	free_irq(client->irq, ts);
>  err_free_mem:
> +	input_free_device(input_dev);
>  	kfree(ts);
> -err:
> -	return ret;
> +	return error;
>  }
>  
>  static int __devexit st1232_ts_remove(struct i2c_client *client)
> @@ -207,8 +210,7 @@ static int __devexit st1232_ts_remove(struct i2c_client *client)
>  #ifdef CONFIG_PM
>  static int st1232_ts_suspend(struct device *dev)
>  {
> -	struct st1232_ts_data *ts = dev_get_drvdata(dev);
> -	struct i2c_client *client = ts->client;
> +	struct i2c_client *client = to_i2c_client(dev);
>  
>  	if (device_may_wakeup(&client->dev))
>  		enable_irq_wake(client->irq);
> @@ -220,8 +222,7 @@ static int st1232_ts_suspend(struct device *dev)
>  
>  static int st1232_ts_resume(struct device *dev)
>  {
> -	struct st1232_ts_data *ts = dev_get_drvdata(dev);
> -	struct i2c_client *client = ts->client;
> +	struct i2c_client *client = to_i2c_client(dev);
>  
>  	if (device_may_wakeup(&client->dev))
>  		disable_irq_wake(client->irq);

Re: [PATCH v4] Input: add ST1232 touchscreen controller driver.

[Henrik Rydberg]

On 12/16/2010 07:19 AM, Dmitry Torokhov wrote:

> Hi Tony,
> 
> On Thu, Dec 16, 2010 at 09:45:25AM +0900, chinyeow.sim.xt@renesas.com wrote:
>> From: Tony SIM <chinyeow.sim.xt@renesas.com>
>>
>> This patch set introduces for Sitronix ST1232 touchscreen controller
>> driver. This is an integrated capacitive touchscreen with LCD module
>> which can detect two fingers's touch X/Y coordinate.
>>
>> Signed-off-by: Tony SIM <chinyeow.sim.xt@renesas.com>
>> ---


Acked-by: Henrik Rydberg <rydberg@euromail.se>

Thanks!

Henrik

Re: [PATCH v4] Input: add ST1232 touchscreen controller driver.

[Dmitry Torokhov]

On Thu, Dec 16, 2010 at 04:06:10PM +0900, chinyeow.sim.xt@renesas.com wrote:
> Hi Dmitry,
> 
> Thanks for modifing the changes! It works well!
> 
> / Tony
> 

Great, I'll queue the driver for .38.

Thanks.

-- 
Dmitry