[v8] Touchscreen driver for FT5x06 based EDT displays

Commit Message

Dmitry Torokhov July 19, 2012, 4:16 a.m. UTC

Hi Simon, Henrik,

On Mon, Jul 09, 2012 at 10:06:40AM +0200, Henrik Rydberg wrote:
> Hi Simon,
> 
> On Sun, Jul 08, 2012 at 06:05:22PM +0200, [e-mail] wrote:
> > From: Simon Budig <[e-mail]>
> > 
> > This is a driver for the EDT "Polytouch" family of touch controllers
> > based on the FocalTech FT5x06 line of chips.
> > 
> > Signed-off-by: Simon Budig <[e-mail]>
> > ---
> 
> (The area below the '---' can be used for comments, instead of sending
> two mails.)
> 
> It is starting to look pretty good now, thank you. The remove() seems
> to leak memory, and I sprinkled some minor comments on the way.

OK, so the patch below should fix most of Henrik's comments and some of
mine. Compile-tested only though. It would be nice to have it verified
on real hardware so we could get it in in the next merge window.

Thanks.

Comments

Henrik Rydberg July 19, 2012, 1:50 p.m. UTC | #1

Hi Dmitry,

> OK, so the patch below should fix most of Henrik's comments and some of
> mine. Compile-tested only though. It would be nice to have it verified
> on real hardware so we could get it in in the next merge window.

Looking good, just a question for Simon: In the mt report function,
the loop uses every fourth byte of rdbuf, but the access looks a bit
funny; buf[5], buf[6] etc. It is fine as long as it works, just
checking. :-)

Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Simon Budig July 19, 2012, 1:56 p.m. UTC | #2

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

On 07/19/2012 03:50 PM, Henrik Rydberg wrote:
> Looking good, just a question for Simon: In the mt report
> function, the loop uses every fourth byte of rdbuf, but the access
> looks a bit funny; buf[5], buf[6] etc. It is fine as long as it
> works, just checking. :-)

I'll have a look at the weekend. Sorry for being unresponsive at the
moment, there is an important deadline at work tomorrow...

Bye,
        Simon

- -- 
       Simon Budig                        kernel concepts GmbH
       [e-mail]      Sieghuetter Hauptweg 48
       [phone]                  D-57072 Siegen



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

iEYEARECAAYFAlAIEfkACgkQO2O/RXesiHBIXQCfV8WVJxJ8EPdkxVMkwCKksKXi
UjMAn0DdXE1XeDDjeEcUoP3jxuKpQh1T
=ASLY
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/input/edt-ft5x06.txt b/Documentation/input/edt-ft5x06.txt
index d2f1444..2032f0b 100644
--- a/Documentation/input/edt-ft5x06.txt
+++ b/Documentation/input/edt-ft5x06.txt
@@ -52,5 +52,3 @@  raw_data:
 Note that reading raw_data gives a I/O error when the device is not in factory
 mode. The same happens when reading/writing to the parameter files when the
 device is not in regular operation mode.
-
-
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 32d8840..a1c266a 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -36,8 +36,6 @@ 
 #include <linux/input/mt.h>
 #include <linux/input/edt-ft5x06.h>
 
-#define DRIVER_VERSION "v0.7"
-
 #define MAX_SUPPORT_POINTS		5
 
 #define WORK_REGISTER_THRESHOLD		0x00
@@ -69,7 +67,8 @@  struct edt_ft5x06_ts_data {
 
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *debug_dir;
-	u8 *raw_buffer;
+	u8 *raw_buffer;
+	size_t raw_bufsize;
 #endif
 
 	struct mutex mutex;
@@ -90,7 +89,6 @@  static int edt_ft5x06_ts_readwrite(struct i2c_client *client,
 	int i = 0;
 	int ret;
 
-	i = 0;
 	if (wr_len) {
 		wrmsg[i].addr  = client->addr;
 		wrmsg[i].flags = 0;
@@ -355,18 +353,20 @@  static const struct attribute_group edt_ft5x06_attr_group = {
 	.attrs = edt_ft5x06_attrs,
 };
 
-#if defined(CONFIG_DEBUG_FS)
+#ifdef CONFIG_DEBUG_FS
 static int edt_ft5x06_factory_mode(struct edt_ft5x06_ts_data *tsdata)
 {
+	struct i2c_client *client = tsdata->client;
 	int retries = EDT_SWITCH_MODE_RETRIES;
 	int ret;
 	int error;
 
-	disable_irq(tsdata->client->irq);
+	disable_irq(client->irq);
 
 	if (!tsdata->raw_buffer) {
-		tsdata->raw_buffer = kzalloc(tsdata->num_x * tsdata->num_x * 2,
-					     GFP_KERNEL);
+		tsdata->raw_bufsize = tsdata->num_x * tsdata->num_y *
+				      sizeof(u16);
+		tsdata->raw_buffer = kzalloc(tsdata->raw_bufsize, GFP_KERNEL);
 		if (!tsdata->raw_buffer) {
 			error = -ENOMEM;
 			goto err_out;
@@ -376,9 +376,8 @@  static int edt_ft5x06_factory_mode(struct edt_ft5x06_ts_data *tsdata)
 	/* mode register is 0x3c when in the work mode */
 	error = edt_ft5x06_register_write(tsdata, WORK_REGISTER_OPMODE, 0x03);
 	if (error) {
-		dev_err(&tsdata->client->dev,
-			"failed to switch to factory mode, error %d\n",
-			error);
+		dev_err(&client->dev,
+			"failed to switch to factory mode, error %d\n", error);
 		goto err_out;
 	}
 
@@ -392,8 +391,7 @@  static int edt_ft5x06_factory_mode(struct edt_ft5x06_ts_data *tsdata)
 	} while (--retries > 0);
 
 	if (retries == 0) {
-		dev_err(&tsdata->client->dev,
-			"not in factory mode after %dms.\n",
+		dev_err(&client->dev, "not in factory mode after %dms.\n",
 			EDT_SWITCH_MODE_RETRIES * EDT_SWITCH_MODE_DELAY);
 		error = -EIO;
 		goto err_out;
@@ -403,13 +401,16 @@  static int edt_ft5x06_factory_mode(struct edt_ft5x06_ts_data *tsdata)
 
 err_out:
 	kfree(tsdata->raw_buffer);
+	tsdata->raw_buffer = NULL;
 	tsdata->factory_mode = false;
-	enable_irq(tsdata->client->irq);
+	enable_irq(client->irq);
+
 	return error;
 }
 
 static int edt_ft5x06_work_mode(struct edt_ft5x06_ts_data *tsdata)
 {
+	struct i2c_client *client = tsdata->client;
 	int retries = EDT_SWITCH_MODE_RETRIES;
 	int ret;
 	int error;
@@ -417,9 +418,8 @@  static int edt_ft5x06_work_mode(struct edt_ft5x06_ts_data *tsdata)
 	/* mode register is 0x01 when in the factory mode */
 	error = edt_ft5x06_register_write(tsdata, FACTORY_REGISTER_OPMODE, 0x1);
 	if (error) {
-		dev_err(&tsdata->client->dev,
-			"failed to switch to work mode, error: %d\n",
-			error);
+		dev_err(&client->dev,
+			"failed to switch to work mode, error: %d\n", error);
 		return error;
 	}
 
@@ -434,8 +434,7 @@  static int edt_ft5x06_work_mode(struct edt_ft5x06_ts_data *tsdata)
 	} while (--retries > 0);
 
 	if (retries == 0) {
-		dev_err(&tsdata->client->dev,
-			"not in work mode after %dms.\n",
+		dev_err(&client->dev, "not in work mode after %dms.\n",
 			EDT_SWITCH_MODE_RETRIES * EDT_SWITCH_MODE_DELAY);
 		tsdata->factory_mode = true;
 		return -EIO;
@@ -454,22 +453,24 @@  static int edt_ft5x06_work_mode(struct edt_ft5x06_ts_data *tsdata)
 	edt_ft5x06_register_write(tsdata, WORK_REGISTER_REPORT_RATE,
 				  tsdata->report_rate);
 
-	enable_irq(tsdata->client->irq);
+	enable_irq(client->irq);
 
 	return 0;
 }
 
-ssize_t edt_ft5x06_debugfs_mode_get(void *data, u64 *mode)
+static int edt_ft5x06_debugfs_mode_get(void *data, u64 *mode)
 {
 	struct edt_ft5x06_ts_data *tsdata = data;
+
 	*mode = tsdata->factory_mode;
+
 	return 0;
 };
 
-ssize_t edt_ft5x06_debugfs_mode_set(void *data, u64 mode)
+static int edt_ft5x06_debugfs_mode_set(void *data, u64 mode)
 {
 	struct edt_ft5x06_ts_data *tsdata = data;
-	int error = 0;
+	int retval = 0;
 
 	if (mode > 1)
 		return -ERANGE;
@@ -477,13 +478,13 @@  ssize_t edt_ft5x06_debugfs_mode_set(void *data, u64 mode)
 	mutex_lock(&tsdata->mutex);
 
 	if (mode != tsdata->factory_mode) {
-		error = mode ? edt_ft5x06_factory_mode(tsdata) :
-			       edt_ft5x06_work_mode(tsdata);
+		retval = mode ? edt_ft5x06_factory_mode(tsdata) :
+			        edt_ft5x06_work_mode(tsdata);
 	}
 
 	mutex_unlock(&tsdata->mutex);
 
-	return error;
+	return retval;
 };
 
 DEFINE_SIMPLE_ATTRIBUTE(debugfs_mode_fops, edt_ft5x06_debugfs_mode_get,
@@ -493,24 +494,23 @@  static int edt_ft5x06_debugfs_raw_data_open(struct inode *inode,
 					    struct file *file)
 {
 	file->private_data = inode->i_private;
+
 	return 0;
 }
 
-ssize_t edt_ft5x06_debugfs_raw_data_read(struct file *file,
-					 char *buf,
-					 size_t count,
-					 loff_t *off)
+static ssize_t edt_ft5x06_debugfs_raw_data_read(struct file *file,
+				char __user *buf, size_t count, loff_t *off)
 {
 	struct edt_ft5x06_ts_data *tsdata = file->private_data;
+	struct i2c_client *client = tsdata->client;
 	int retries  = EDT_RAW_DATA_RETRIES;
-	int ret = 0, i, error;
+	int val, i, error;
+	size_t read;
 	int colbytes;
 	char wrbuf[3];
 	u8 *rdbuf;
 
-	colbytes = tsdata->num_y * 2;
-
-	if (*off < 0 || *off >= tsdata->num_x * colbytes)
+	if (*off < 0 || *off >= tsdata->raw_bufsize)
 		return 0;
 
 	mutex_lock(&tsdata->mutex);
@@ -522,35 +522,34 @@  ssize_t edt_ft5x06_debugfs_raw_data_read(struct file *file,
 
 	error = edt_ft5x06_register_write(tsdata, 0x08, 0x01);
 	if (error) {
-		dev_dbg(&tsdata->client->dev,
-			"failed to write 0x08 register, error %d\n",
-			error);
+		dev_dbg(&client->dev,
+			"failed to write 0x08 register, error %d\n", error);
 		goto out;
 	}
 
 	do {
 		msleep(EDT_RAW_DATA_DELAY);
-		ret = edt_ft5x06_register_read(tsdata, 0x08);
-		if (ret < 1)
+		val = edt_ft5x06_register_read(tsdata, 0x08);
+		if (val < 1)
 			break;
 	} while (--retries > 0);
 
-	if (ret < 0) {
-		error = ret;
-		dev_dbg(&tsdata->client->dev,
-			"failed to read 0x08 register, error %d\n",
-			error);
+	if (val < 0) {
+		error = val;
+		dev_dbg(&client->dev,
+			"failed to read 0x08 register, error %d\n", error);
 		goto out;
 	}
 
 	if (retries == 0) {
-		dev_dbg(&tsdata->client->dev,
+		dev_dbg(&client->dev,
 			"timed out waiting for register to settle\n");
 		error = -ETIMEDOUT;
 		goto out;
 	}
 
 	rdbuf = tsdata->raw_buffer;
+	colbytes = tsdata->num_y * sizeof(u16);
 
 	wrbuf[0] = 0xf5;
 	wrbuf[1] = 0x0e;
@@ -565,16 +564,13 @@  ssize_t edt_ft5x06_debugfs_raw_data_read(struct file *file,
 		rdbuf += colbytes;
 	}
 
-	/* rdbuf now points to the end of the raw_buffer */
-
-	ret = min(count, (size_t) ((rdbuf - tsdata->raw_buffer) - *off));
-
-	error = copy_to_user(buf, tsdata->raw_buffer + *off, ret);
+	read = min_t(size_t, count, tsdata->raw_bufsize - *off);
+	error = copy_to_user(buf, tsdata->raw_buffer + *off, read);
 	if (!error)
-		*off += ret;
+		*off += read;
 out:
 	mutex_unlock(&tsdata->mutex);
-	return error ?: ret;
+	return error ?: read;
 };
 
 
@@ -582,7 +578,47 @@  static const struct file_operations debugfs_raw_data_fops = {
 	.open = edt_ft5x06_debugfs_raw_data_open,
 	.read = edt_ft5x06_debugfs_raw_data_read,
 };
-#endif
+
+static void __devinit
+edt_ft5x06_ts_prepare_debugfs(struct edt_ft5x06_ts_data *tsdata,
+			      const char *debugfs_name)
+{
+	tsdata->debug_dir = debugfs_create_dir(debugfs_name, NULL);
+	if (!tsdata->debug_dir)
+		return;
+
+	debugfs_create_u16("num_x", S_IRUSR, tsdata->debug_dir, &tsdata->num_x);
+	debugfs_create_u16("num_y", S_IRUSR, tsdata->debug_dir, &tsdata->num_y);
+
+	debugfs_create_file("mode", S_IRUSR | S_IWUSR,
+			    tsdata->debug_dir, tsdata, &debugfs_mode_fops);
+	debugfs_create_file("raw_data", S_IRUSR,
+			    tsdata->debug_dir, tsdata, &debugfs_raw_data_fops);
+}
+
+static void __devexit
+edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
+{
+	if (tsdata->debug_dir)
+		debugfs_remove_recursive(tsdata->debug_dir);
+}
+
+#else
+
+static inline void
+edt_ft5x06_ts_prepare_debugfs(struct edt_ft5x06_ts_data *tsdata,
+			      const char *debugfs_name)
+{
+}
+
+static inline void
+edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
+{
+}
+
+#endif /* CONFIG_DEBUGFS */
+
+
 
 static int __devinit edt_ft5x06_ts_reset(struct i2c_client *client,
 					 int reset_pin)
@@ -637,8 +673,9 @@  static int __devinit edt_ft5x06_ts_identify(struct i2c_client *client,
 	return 0;
 }
 
-static void edt_ft5x06_ts_get_defaults(struct edt_ft5x06_ts_data *tsdata,
-				const struct edt_ft5x06_platform_data *pdata)
+static void __devinit
+edt_ft5x06_ts_get_defaults(struct edt_ft5x06_ts_data *tsdata,
+			   const struct edt_ft5x06_platform_data *pdata)
 {
 	if (!pdata->use_parameters)
 		return;
@@ -665,7 +702,8 @@  static void edt_ft5x06_ts_get_defaults(struct edt_ft5x06_ts_data *tsdata,
 					  pdata->report_rate);
 }
 
-static void edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata)
+static void __devinit
+edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata)
 {
 	tsdata->threshold = edt_ft5x06_register_read(tsdata,
 						     WORK_REGISTER_THRESHOLD);
@@ -677,28 +715,6 @@  static void edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata)
 	tsdata->num_y = edt_ft5x06_register_read(tsdata, WORK_REGISTER_NUM_Y);
 }
 
-#if defined(CONFIG_DEBUG_FS)
-static void edt_ft5x06_ts_prepare_debugfs(struct edt_ft5x06_ts_data *tsdata,
-					  const char *debugfs_name)
-{
-	tsdata->debug_dir = debugfs_create_dir(debugfs_name, NULL);
-
-	if (!tsdata->debug_dir)
-		return;
-
-	debugfs_create_u16("num_x", S_IRUSR, tsdata->debug_dir, &tsdata->num_x);
-	debugfs_create_u16("num_y", S_IRUSR, tsdata->debug_dir, &tsdata->num_y);
-
-	debugfs_create_file("mode", S_IRUSR | S_IWUSR,
-			    tsdata->debug_dir, tsdata,
-			    &debugfs_mode_fops);
-
-	debugfs_create_file("raw_data", S_IRUSR,
-			    tsdata->debug_dir, tsdata,
-			    &debugfs_raw_data_fops);
-}
-#endif
-
 static int __devinit edt_ft5x06_ts_probe(struct i2c_client *client,
 					 const struct i2c_device_id *id)
 {
@@ -796,13 +812,10 @@  static int __devinit edt_ft5x06_ts_probe(struct i2c_client *client,
 	if (error)
 		goto err_remove_attrs;
 
-#if defined(CONFIG_DEBUG_FS)
 	edt_ft5x06_ts_prepare_debugfs(tsdata, dev_driver_string(&client->dev));
-#endif
-
 	device_init_wakeup(&client->dev, 1);
 
-	dev_dbg(&tsdata->client->dev,
+	dev_dbg(&client->dev,
 		"EDT FT5x06 initialized: IRQ pin %d, Reset pin %d.\n",
 		pdata->irq_pin, pdata->reset_pin);
 
@@ -825,22 +838,21 @@  err_free_mem:
 static int __devexit edt_ft5x06_ts_remove(struct i2c_client *client)
 {
 	const struct edt_ft5x06_platform_data *pdata =
-						client->dev.platform_data;
+						dev_get_platdata(&client->dev);
 	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
 
-#if defined(CONFIG_DEBUG_FS)
-	if (tsdata->debug_dir)
-		debugfs_remove_recursive(tsdata->debug_dir);
-#endif
-
+	edt_ft5x06_ts_teardown_debugfs(tsdata);
 	sysfs_remove_group(&client->dev.kobj, &edt_ft5x06_attr_group);
 
 	free_irq(client->irq, tsdata);
 	input_unregister_device(tsdata->input);
+
 	if (gpio_is_valid(pdata->irq_pin))
 		gpio_free(pdata->irq_pin);
 	if (gpio_is_valid(pdata->reset_pin))
 		gpio_free(pdata->reset_pin);
+
+	kfree(tsdata->raw_buffer);
 	kfree(tsdata);
 
 	return 0;