[PATCH 00/16 v2] cleanup atmel_mxt_ts

[PATCH 00/16 v2] cleanup atmel_mxt_ts

[Daniel Kurtz]

This patchset cleans up the atmel_mxt_ts touchscreen driver.
In particular, v2 implements the following:

1) sysfs
   a) fw_update only by root
2) faster initialization
   a) read/write sets of registers using i2c block transactions
   b) fetch object table as a set of i2c reads, one per object
   c) write config data as a set of i2c writes, one per object
3) faster interrupt processing & initialization times
   a) cache important values at init instead of computing in isr
   b) don't read message checksum byte (which isn't even enabled in fw)
4) more correct MT-B support
   a) send all (changed) contacts in a single EV_SYN/SYN_REPORT

v2:
  * Writable config via sysfs has been removed to resolve possible
    conflict with Nick Dyer's implementation.
  * Using T44 has been removed from this patchset, hopefully it can
    be reexamined after most of these patches are accepted.

The patches were tested using an MXT224E.

Daniel Kurtz (16):
  Input: atmel_mxt_ts - use CONFIG_PM_SLEEP
  Input: atmel_mxt_ts - only allow root to update firmware
  Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length
  Input: atmel_mxt_ts - store actual size and instance
  Input: atmel_mxt_ts - verify object size in mxt_write_object
  Input: atmel_mxt_ts - do not read extra (checksum) byte
  Input: atmel_mxt_ts - dump each message on just 1 line
  Input: atmel_mxt_ts - refactor mxt_object_show
  Input: atmel_mxt_ts - optimize writing of object table entries
  Input: atmel_mxt_ts - refactor get info
  Input: atmel_mxt_ts - refactor reading object table
  Input: atmel_mxt_ts - simplify event reporting
  Input: atmel_mxt_ts - cache T9 reportid range when reading object
    table
  Input: atmel_mxt_ts - parse vector field of data packets
  Input: atmel_mxt_ts - send all MT-B slots in one input report
  Input: atmel_mxt_ts - parse T6 reports

 drivers/input/touchscreen/atmel_mxt_ts.c |  477 ++++++++++++------------------
 1 files changed, 191 insertions(+), 286 deletions(-)

-- 
1.7.7.3

[PATCH 01/16 v2] Input: atmel_mxt_ts - use CONFIG_PM_SLEEP

[Daniel Kurtz]

Simple cleanup to use newer PM APIs.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 19d4ea6..0a6e368 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1201,7 +1201,7 @@ static int __devexit mxt_remove(struct i2c_client *client)
 	return 0;
 }
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
 static int mxt_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -1239,13 +1239,10 @@ static int mxt_resume(struct device *dev)
 
 	return 0;
 }
-
-static const struct dev_pm_ops mxt_pm_ops = {
-	.suspend	= mxt_suspend,
-	.resume		= mxt_resume,
-};
 #endif
 
+static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
+
 static const struct i2c_device_id mxt_id[] = {
 	{ "qt602240_ts", 0 },
 	{ "atmel_mxt_ts", 0 },
@@ -1258,9 +1255,7 @@ static struct i2c_driver mxt_driver = {
 	.driver = {
 		.name	= "atmel_mxt_ts",
 		.owner	= THIS_MODULE,
-#ifdef CONFIG_PM
 		.pm	= &mxt_pm_ops,
-#endif
 	},
 	.probe		= mxt_probe,
 	.remove		= __devexit_p(mxt_remove),
-- 
1.7.7.3

[PATCH 02/16 v2] Input: atmel_mxt_ts - only allow root to update firmware

[Daniel Kurtz]

Restrict permissions on the update_fw sysfs entry to read only for root
only.

Also, update object permission to use a macro S_IRUGO macro instead of
hard coded 0444.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Acked-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 0a6e368..15ae6fd 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1049,8 +1049,8 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 	return count;
 }
 
-static DEVICE_ATTR(object, 0444, mxt_object_show, NULL);
-static DEVICE_ATTR(update_fw, 0664, NULL, mxt_update_fw_store);
+static DEVICE_ATTR(object, S_IRUGO, mxt_object_show, NULL);
+static DEVICE_ATTR(update_fw, S_IWUSR, NULL, mxt_update_fw_store);
 
 static struct attribute *mxt_attrs[] = {
 	&dev_attr_object.attr,
-- 
1.7.7.3

[PATCH 03/16 v2] Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length

[Daniel Kurtz]

The i2c bus requires 5 bytes to do a 1 byte read (1-byte i2c address + 2
byte offset + 1-byte i2c address + 1 byte data), or 4 bytes to do a
1-byte write (1 byte i2c address + 2 byte offset + 1 byte data).

By taking a length with reads and writes, the driver can amortize
transaction overhead by performing larger transactions where appropriate.

This patch just sets up the new API.  Later patches refactor reads/writes
to take advantage of the larger transactions.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   47 +++++++++++++----------------
 1 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 15ae6fd..971f9dc 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -397,8 +397,7 @@ static int mxt_fw_write(struct i2c_client *client,
 	return 0;
 }
 
-static int __mxt_read_reg(struct i2c_client *client,
-			       u16 reg, u16 len, void *val)
+static int mxt_read_reg(struct i2c_client *client, u16 reg, u16 len, void *val)
 {
 	struct i2c_msg xfer[2];
 	u8 buf[2];
@@ -419,28 +418,25 @@ static int __mxt_read_reg(struct i2c_client *client,
 	xfer[1].buf = val;
 
 	if (i2c_transfer(client->adapter, xfer, 2) != 2) {
-		dev_err(&client->dev, "%s: i2c transfer failed\n", __func__);
+		dev_err(&client->dev, "%s: i2c read failed\n", __func__);
 		return -EIO;
 	}
 
 	return 0;
 }
 
-static int mxt_read_reg(struct i2c_client *client, u16 reg, u8 *val)
+static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
+			 const void *val)
 {
-	return __mxt_read_reg(client, reg, 1, val);
-}
-
-static int mxt_write_reg(struct i2c_client *client, u16 reg, u8 val)
-{
-	u8 buf[3];
+	size_t count = 2 + len;		/* + 2-byte offset */
+	u8 buf[count];
 
 	buf[0] = reg & 0xff;
 	buf[1] = (reg >> 8) & 0xff;
-	buf[2] = val;
+	memcpy(&buf[2], val, len);
 
-	if (i2c_master_send(client, buf, 3) != 3) {
-		dev_err(&client->dev, "%s: i2c send failed\n", __func__);
+	if (i2c_master_send(client, buf, count) != count) {
+		dev_err(&client->dev, "%s: i2c write failed\n", __func__);
 		return -EIO;
 	}
 
@@ -450,8 +446,7 @@ static int mxt_write_reg(struct i2c_client *client, u16 reg, u8 val)
 static int mxt_read_object_table(struct i2c_client *client,
 				      u16 reg, u8 *object_buf)
 {
-	return __mxt_read_reg(client, reg, MXT_OBJECT_SIZE,
-				   object_buf);
+	return mxt_read_reg(client, reg, MXT_OBJECT_SIZE, object_buf);
 }
 
 static struct mxt_object *
@@ -481,8 +476,8 @@ static int mxt_read_message(struct mxt_data *data,
 		return -EINVAL;
 
 	reg = object->start_address;
-	return __mxt_read_reg(data->client, reg,
-			sizeof(struct mxt_message), message);
+	return mxt_read_reg(data->client, reg, sizeof(struct mxt_message),
+			    message);
 }
 
 static int mxt_read_object(struct mxt_data *data,
@@ -496,7 +491,7 @@ static int mxt_read_object(struct mxt_data *data,
 		return -EINVAL;
 
 	reg = object->start_address;
-	return __mxt_read_reg(data->client, reg + offset, 1, val);
+	return mxt_read_reg(data->client, reg + offset, 1, val);
 }
 
 static int mxt_write_object(struct mxt_data *data,
@@ -510,7 +505,7 @@ static int mxt_write_object(struct mxt_data *data,
 		return -EINVAL;
 
 	reg = object->start_address;
-	return mxt_write_reg(data->client, reg + offset, val);
+	return mxt_write_reg(data->client, reg + offset, 1, &val);
 }
 
 static void mxt_input_report(struct mxt_data *data, int single_id)
@@ -757,27 +752,27 @@ static int mxt_get_info(struct mxt_data *data)
 	int error;
 	u8 val;
 
-	error = mxt_read_reg(client, MXT_FAMILY_ID, &val);
+	error = mxt_read_reg(client, MXT_FAMILY_ID, 1, &val);
 	if (error)
 		return error;
 	info->family_id = val;
 
-	error = mxt_read_reg(client, MXT_VARIANT_ID, &val);
+	error = mxt_read_reg(client, MXT_VARIANT_ID, 1, &val);
 	if (error)
 		return error;
 	info->variant_id = val;
 
-	error = mxt_read_reg(client, MXT_VERSION, &val);
+	error = mxt_read_reg(client, MXT_VERSION, 1, &val);
 	if (error)
 		return error;
 	info->version = val;
 
-	error = mxt_read_reg(client, MXT_BUILD, &val);
+	error = mxt_read_reg(client, MXT_BUILD, 1, &val);
 	if (error)
 		return error;
 	info->build = val;
 
-	error = mxt_read_reg(client, MXT_OBJECT_NUM, &val);
+	error = mxt_read_reg(client, MXT_OBJECT_NUM, 1, &val);
 	if (error)
 		return error;
 	info->object_num = val;
@@ -860,12 +855,12 @@ static int mxt_initialize(struct mxt_data *data)
 	msleep(MXT_RESET_TIME);
 
 	/* Update matrix size at info struct */
-	error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, &val);
+	error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, 1, &val);
 	if (error)
 		return error;
 	info->matrix_xsize = val;
 
-	error = mxt_read_reg(client, MXT_MATRIX_Y_SIZE, &val);
+	error = mxt_read_reg(client, MXT_MATRIX_Y_SIZE, 1, &val);
 	if (error)
 		return error;
 	info->matrix_ysize = val;
-- 
1.7.7.3

[PATCH 04/16 v2] Input: atmel_mxt_ts - store actual size and instance

[Daniel Kurtz]

These two object table entry fields are reported 1 less than their value.
So add 1 once, when building the object table, instead of every time
these fields are accessed.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 971f9dc..920b35c 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -225,8 +225,8 @@ struct mxt_info {
 struct mxt_object {
 	u8 type;
 	u16 start_address;
-	u8 size;
-	u8 instances;
+	u16 size;
+	u16 instances;
 	u8 num_report_ids;
 
 	/* to map object and message */
@@ -645,7 +645,7 @@ static int mxt_check_reg_init(struct mxt_data *data)
 	struct mxt_object *object;
 	struct device *dev = &data->client->dev;
 	int index = 0;
-	int i, j, config_offset;
+	int i, j, config_offset, config_size;
 
 	if (!pdata->config) {
 		dev_dbg(dev, "No cfg data defined, skipping reg init\n");
@@ -658,9 +658,8 @@ static int mxt_check_reg_init(struct mxt_data *data)
 		if (!mxt_object_writable(object->type))
 			continue;
 
-		for (j = 0;
-		     j < (object->size + 1) * (object->instances + 1);
-		     j++) {
+		config_size = object->size * object->instances;
+		for (j = 0; j < config_size; j++) {
 			config_offset = index + j;
 			if (config_offset > pdata->config_length) {
 				dev_err(dev, "Not enough config data!\n");
@@ -669,7 +668,7 @@ static int mxt_check_reg_init(struct mxt_data *data)
 			mxt_write_object(data, object->type, j,
 					 pdata->config[config_offset]);
 		}
-		index += (object->size + 1) * (object->instances + 1);
+		index += config_size;
 	}
 
 	return 0;
@@ -798,13 +797,12 @@ static int mxt_get_object_table(struct mxt_data *data)
 
 		object->type = buf[0];
 		object->start_address = (buf[2] << 8) | buf[1];
-		object->size = buf[3];
-		object->instances = buf[4];
+		object->size = buf[3] + 1;
+		object->instances = buf[4] + 1;
 		object->num_report_ids = buf[5];
 
 		if (object->num_report_ids) {
-			reportid += object->num_report_ids *
-					(object->instances + 1);
+			reportid += object->num_report_ids * object->instances;
 			object->max_reportid = reportid;
 		}
 	}
@@ -919,7 +917,7 @@ static ssize_t mxt_object_show(struct device *dev,
 			continue;
 		}
 
-		for (j = 0; j < object->size + 1; j++) {
+		for (j = 0; j < object->size; j++) {
 			error = mxt_read_object(data,
 						object->type, j, &val);
 			if (error)
-- 
1.7.7.3

[PATCH 05/16 v2] Input: atmel_mxt_ts - verify object size in mxt_write_object

[Daniel Kurtz]

Don't allow writing past the length of an object.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 920b35c..242976d 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -501,7 +501,7 @@ static int mxt_write_object(struct mxt_data *data,
 	u16 reg;
 
 	object = mxt_get_object(data, type);
-	if (!object)
+	if (!object || offset >= object->size)
 		return -EINVAL;
 
 	reg = object->start_address;
-- 
1.7.7.3

[PATCH 06/16 v2] Input: atmel_mxt_ts - do not read extra (checksum) byte

[Daniel Kurtz]

atmel_mxt devices will send a checksum byte at the end of a message if
the MSB of the object address is set.
However, since this driver does not set this bit, the checksum byte
isn't actually sent, so don't even try to read it.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 242976d..ea3ee5a 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -236,7 +236,6 @@ struct mxt_object {
 struct mxt_message {
 	u8 reportid;
 	u8 message[7];
-	u8 checksum;
 };
 
 struct mxt_finger {
@@ -336,7 +335,6 @@ static void mxt_dump_message(struct device *dev,
 	dev_dbg(dev, "message5:\t0x%x\n", message->message[4]);
 	dev_dbg(dev, "message6:\t0x%x\n", message->message[5]);
 	dev_dbg(dev, "message7:\t0x%x\n", message->message[6]);
-	dev_dbg(dev, "checksum:\t0x%x\n", message->checksum);
 }
 
 static int mxt_check_bootloader(struct i2c_client *client,
-- 
1.7.7.3

[PATCH 07/16 v2] Input: atmel_mxt_ts - dump each message on just 1 line

[Daniel Kurtz]

Helps ensure all bytes for a single message together in the system log.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Acked-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index ea3ee5a..b6e7109 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -325,16 +325,12 @@ static bool mxt_object_writable(unsigned int type)
 }
 
 static void mxt_dump_message(struct device *dev,
-				  struct mxt_message *message)
+			     struct mxt_message *message)
 {
-	dev_dbg(dev, "reportid:\t0x%x\n", message->reportid);
-	dev_dbg(dev, "message1:\t0x%x\n", message->message[0]);
-	dev_dbg(dev, "message2:\t0x%x\n", message->message[1]);
-	dev_dbg(dev, "message3:\t0x%x\n", message->message[2]);
-	dev_dbg(dev, "message4:\t0x%x\n", message->message[3]);
-	dev_dbg(dev, "message5:\t0x%x\n", message->message[4]);
-	dev_dbg(dev, "message6:\t0x%x\n", message->message[5]);
-	dev_dbg(dev, "message7:\t0x%x\n", message->message[6]);
+	dev_dbg(dev, "reportid: %u\tmessage: %02x %02x %02x %02x %02x %02x %02x\n",
+		message->reportid, message->message[0], message->message[1],
+		message->message[2], message->message[3], message->message[4],
+		message->message[5], message->message[6]);
 }
 
 static int mxt_check_bootloader(struct i2c_client *client,
-- 
1.7.7.3

[PATCH 08/16 v2] Input: atmel_mxt_ts - refactor mxt_object_show

[Daniel Kurtz]

Read each object with a single i2c transaction instead of byte-by-byte.

Also, don't read T5, which is the message processor object.  Reading
it is counter-productive since it either holds garbage (there is no
pending message), or, it holds a real message which should be handled
by the messages handling code (the isr).

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   43 ++++++++++++------------------
 1 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index b6e7109..a865967 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -262,7 +262,6 @@ struct mxt_data {
 static bool mxt_object_readable(unsigned int type)
 {
 	switch (type) {
-	case MXT_GEN_MESSAGE_T5:
 	case MXT_GEN_COMMAND_T6:
 	case MXT_GEN_POWER_T7:
 	case MXT_GEN_ACQUIRE_T8:
@@ -459,6 +458,13 @@ mxt_get_object(struct mxt_data *data, u8 type)
 	return NULL;
 }
 
+static int mxt_read_object(struct mxt_data *data, struct mxt_object *object,
+			   void *val)
+{
+	return mxt_read_reg(data->client, object->start_address, object->size,
+			    val);
+}
+
 static int mxt_read_message(struct mxt_data *data,
 				 struct mxt_message *message)
 {
@@ -474,20 +480,6 @@ static int mxt_read_message(struct mxt_data *data,
 			    message);
 }
 
-static int mxt_read_object(struct mxt_data *data,
-				u8 type, u8 offset, u8 *val)
-{
-	struct mxt_object *object;
-	u16 reg;
-
-	object = mxt_get_object(data, type);
-	if (!object)
-		return -EINVAL;
-
-	reg = object->start_address;
-	return mxt_read_reg(data->client, reg + offset, 1, val);
-}
-
 static int mxt_write_object(struct mxt_data *data,
 				 u8 type, u8 offset, u8 val)
 {
@@ -885,17 +877,16 @@ static void mxt_calc_resolution(struct mxt_data *data)
 }
 
 static ssize_t mxt_object_show(struct device *dev,
-				    struct device_attribute *attr, char *buf)
+			       struct device_attribute *attr, char *buf)
 {
 	struct mxt_data *data = dev_get_drvdata(dev);
-	struct mxt_object *object;
 	int count = 0;
 	int i, j;
 	int error;
-	u8 val;
+	u8 obuf[256];
 
 	for (i = 0; i < data->info.object_num; i++) {
-		object = data->object_table + i;
+		struct mxt_object *object = &data->object_table[i];
 
 		count += snprintf(buf + count, PAGE_SIZE - count,
 				"Object[%d] (Type %d)\n",
@@ -905,20 +896,20 @@ static ssize_t mxt_object_show(struct device *dev,
 
 		if (!mxt_object_readable(object->type)) {
 			count += snprintf(buf + count, PAGE_SIZE - count,
-					"\n");
+					  "\n");
 			if (count >= PAGE_SIZE)
 				return PAGE_SIZE - 1;
 			continue;
 		}
 
-		for (j = 0; j < object->size; j++) {
-			error = mxt_read_object(data,
-						object->type, j, &val);
-			if (error)
-				return error;
+		error = mxt_read_object(data, object, obuf);
+		if (error)
+			return error;
 
+		for (j = 0; j < object->size; j++) {
 			count += snprintf(buf + count, PAGE_SIZE - count,
-					"\t[%2d]: %02x (%d)\n", j, val, val);
+					  "\t[%2d]: %02x (%d)\n", j, obuf[j],
+					  obuf[j]);
 			if (count >= PAGE_SIZE)
 				return PAGE_SIZE - 1;
 		}
-- 
1.7.7.3

[PATCH 09/16 v2] Input: atmel_mxt_ts - optimize writing of object table entries

[Daniel Kurtz]

Write each object using a single bulk i2c write transfer.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a865967..3abc5b0 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -628,33 +628,33 @@ end:
 static int mxt_check_reg_init(struct mxt_data *data)
 {
 	const struct mxt_platform_data *pdata = data->pdata;
-	struct mxt_object *object;
 	struct device *dev = &data->client->dev;
-	int index = 0;
-	int i, j, config_offset, config_size;
+	int i, offset;
+	int ret;
 
 	if (!pdata->config) {
 		dev_dbg(dev, "No cfg data defined, skipping reg init\n");
 		return 0;
 	}
 
-	for (i = 0; i < data->info.object_num; i++) {
-		object = data->object_table + i;
+	for (offset = 0, i = 0; i < data->info.object_num; i++) {
+		struct mxt_object *object = &data->object_table[i];
+		size_t config_size;
 
 		if (!mxt_object_writable(object->type))
 			continue;
 
 		config_size = object->size * object->instances;
-		for (j = 0; j < config_size; j++) {
-			config_offset = index + j;
-			if (config_offset > pdata->config_length) {
-				dev_err(dev, "Not enough config data!\n");
-				return -EINVAL;
-			}
-			mxt_write_object(data, object->type, j,
-					 pdata->config[config_offset]);
+		if (offset + config_size > pdata->config_length) {
+			dev_err(dev, "Not enough config data!\n");
+			return -EINVAL;
 		}
-		index += config_size;
+
+		ret = mxt_write_reg(data->client, object->start_address,
+				    config_size, &pdata->config[offset]);
+		if (ret)
+			return ret;
+		offset += config_size;
 	}
 
 	return 0;
-- 
1.7.7.3

[PATCH 10/16 v2] Input: atmel_mxt_ts - refactor get info

[Daniel Kurtz]

Read whole info block in one i2c transaction.
And then re-read the matrix size after applying pdata config, since
it may have changed.
Note, however, that the matrix x & y size are just displayed for
information purposes.  They aren't actually used by the driver itself.

Also, parse and info print the firmware major and minor version numbers.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   65 +++++------------------------
 1 files changed, 12 insertions(+), 53 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 3abc5b0..9d88faf 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -36,6 +36,7 @@
 #define MXT_FW_NAME		"maxtouch.fw"
 
 /* Registers */
+#define MXT_INFO		0x00
 #define MXT_FAMILY_ID		0x00
 #define MXT_VARIANT_ID		0x01
 #define MXT_VERSION		0x02
@@ -730,41 +731,6 @@ static void mxt_handle_pdata(struct mxt_data *data)
 	}
 }
 
-static int mxt_get_info(struct mxt_data *data)
-{
-	struct i2c_client *client = data->client;
-	struct mxt_info *info = &data->info;
-	int error;
-	u8 val;
-
-	error = mxt_read_reg(client, MXT_FAMILY_ID, 1, &val);
-	if (error)
-		return error;
-	info->family_id = val;
-
-	error = mxt_read_reg(client, MXT_VARIANT_ID, 1, &val);
-	if (error)
-		return error;
-	info->variant_id = val;
-
-	error = mxt_read_reg(client, MXT_VERSION, 1, &val);
-	if (error)
-		return error;
-	info->version = val;
-
-	error = mxt_read_reg(client, MXT_BUILD, 1, &val);
-	if (error)
-		return error;
-	info->build = val;
-
-	error = mxt_read_reg(client, MXT_OBJECT_NUM, 1, &val);
-	if (error)
-		return error;
-	info->object_num = val;
-
-	return 0;
-}
-
 static int mxt_get_object_table(struct mxt_data *data)
 {
 	int error;
@@ -799,11 +765,12 @@ static int mxt_get_object_table(struct mxt_data *data)
 static int mxt_initialize(struct mxt_data *data)
 {
 	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
 	struct mxt_info *info = &data->info;
 	int error;
-	u8 val;
 
-	error = mxt_get_info(data);
+	/* Read 7-byte info block starting at address 0 */
+	error = mxt_read_reg(client, MXT_INFO, sizeof(*info), info);
 	if (error)
 		return error;
 
@@ -838,26 +805,18 @@ static int mxt_initialize(struct mxt_data *data)
 			MXT_COMMAND_RESET, 1);
 	msleep(MXT_RESET_TIME);
 
-	/* Update matrix size at info struct */
-	error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, 1, &val);
-	if (error)
-		return error;
-	info->matrix_xsize = val;
-
-	error = mxt_read_reg(client, MXT_MATRIX_Y_SIZE, 1, &val);
+	/* Update matrix size, since it may have been modified by pdata */
+	error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, 2,
+			     &info->matrix_xsize);
 	if (error)
 		return error;
-	info->matrix_ysize = val;
 
-	dev_info(&client->dev,
-			"Family ID: %d Variant ID: %d Version: %d Build: %d\n",
-			info->family_id, info->variant_id, info->version,
-			info->build);
+	dev_info(dev, "Family ID: %d Variant ID: %d Major.Minor.Build: %d.%d.%d\n",
+		 info->family_id, info->variant_id, info->version >> 4,
+		 info->version & 0xf, info->build);
 
-	dev_info(&client->dev,
-			"Matrix X Size: %d Matrix Y Size: %d Object Num: %d\n",
-			info->matrix_xsize, info->matrix_ysize,
-			info->object_num);
+	dev_info(dev, "Matrix X Size: %d Matrix Y Size: %d Object Num: %d\n",
+		 info->matrix_xsize, info->matrix_ysize, info->object_num);
 
 	return 0;
 }
-- 
1.7.7.3

[PATCH 11/16 v2] Input: atmel_mxt_ts - refactor reading object table

[Daniel Kurtz]

Instead of reading each object separately, fetch the whole table in one
large i2c transaction.  A 6 byte table object requires 10 bytes to read,
so doing this dramatically reduces overhead.

Also, as a cleanup, move object_table allocation (and post-fw-update
reallocation) into mxt_get_object_table().

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   66 +++++++++++++++---------------
 1 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 9d88faf..0d0dab6 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -437,14 +437,7 @@ static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
 	return 0;
 }
 
-static int mxt_read_object_table(struct i2c_client *client,
-				      u16 reg, u8 *object_buf)
-{
-	return mxt_read_reg(client, reg, MXT_OBJECT_SIZE, object_buf);
-}
-
-static struct mxt_object *
-mxt_get_object(struct mxt_data *data, u8 type)
+static struct mxt_object *mxt_get_object(struct mxt_data *data, u8 type)
 {
 	struct mxt_object *object;
 	int i;
@@ -733,25 +726,41 @@ static void mxt_handle_pdata(struct mxt_data *data)
 
 static int mxt_get_object_table(struct mxt_data *data)
 {
+	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
 	int error;
 	int i;
-	u16 reg;
 	u8 reportid = 0;
-	u8 buf[MXT_OBJECT_SIZE];
+	u8 *buf;
+	size_t buf_size;
 
-	for (i = 0; i < data->info.object_num; i++) {
-		struct mxt_object *object = data->object_table + i;
+	/* Free old object table, if there was one. */
+	kfree(data->object_table);
+	data->object_table = kcalloc(data->info.object_num,
+				     sizeof(struct mxt_object), GFP_KERNEL);
+	if (!data->object_table) {
+		dev_err(dev, "Failed to allocate object table\n");
+		return -ENOMEM;
+	}
 
-		reg = MXT_OBJECT_START + MXT_OBJECT_SIZE * i;
-		error = mxt_read_object_table(data->client, reg, buf);
-		if (error)
-			return error;
+	buf_size = MXT_OBJECT_SIZE * data->info.object_num;
+	buf = kmalloc(buf_size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 
-		object->type = buf[0];
-		object->start_address = (buf[2] << 8) | buf[1];
-		object->size = buf[3] + 1;
-		object->instances = buf[4] + 1;
-		object->num_report_ids = buf[5];
+	error = mxt_read_reg(client, MXT_OBJECT_START, buf_size, buf);
+	if (error)
+		goto done;
+
+	for (i = 0; i < data->info.object_num; i++) {
+		struct mxt_object *object = &data->object_table[i];
+		u8 *obj_buf = &buf[i * MXT_OBJECT_SIZE];
+
+		object->type = obj_buf[0];
+		object->start_address = (obj_buf[2] << 8) | obj_buf[1];
+		object->size = obj_buf[3] + 1;
+		object->instances = obj_buf[4] + 1;
+		object->num_report_ids = obj_buf[5];
 
 		if (object->num_report_ids) {
 			reportid += object->num_report_ids * object->instances;
@@ -759,7 +768,9 @@ static int mxt_get_object_table(struct mxt_data *data)
 		}
 	}
 
-	return 0;
+done:
+	kfree(buf);
+	return error;
 }
 
 static int mxt_initialize(struct mxt_data *data)
@@ -774,14 +785,6 @@ static int mxt_initialize(struct mxt_data *data)
 	if (error)
 		return error;
 
-	data->object_table = kcalloc(info->object_num,
-				     sizeof(struct mxt_object),
-				     GFP_KERNEL);
-	if (!data->object_table) {
-		dev_err(&client->dev, "Failed to allocate memory\n");
-		return -ENOMEM;
-	}
-
 	/* Get object table information */
 	error = mxt_get_object_table(data);
 	if (error)
@@ -971,9 +974,6 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 		/* Wait for reset */
 		msleep(MXT_FWRESET_TIME);
 
-		kfree(data->object_table);
-		data->object_table = NULL;
-
 		mxt_initialize(data);
 	}
 
-- 
1.7.7.3

[PATCH 12/16 v2] Input: atmel_mxt_ts - simplify event reporting

[Daniel Kurtz]

Instead of carrying around per-finger state in the driver instance, just
report each finger as it arrives to the input layer, and let the input
layer (evdev) hold the event state (which it does anyway).

Also, the atmel pad reports "amplitude", which is reported to userspace
using the "PRESSURE" event type.  The variables now reflect this.

Note: this driver does not really do MT-B properly. Each input report
(a goup of input events followed by a SYN_REPORT) only contains data for
a single contact.  When multiple fingers are present on a device, each is
properly reported in its own MT_SLOT.  However, there is only ever one
MT_SLOT per SYN_REPORT.  This is fixed in a subsequent patch.

This patch was tested with an mXT224E.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |  121 +++++++++---------------------
 1 files changed, 35 insertions(+), 86 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 0d0dab6..731293b 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -195,6 +195,7 @@
 #define MXT_BOOT_STATUS_MASK	0x3f
 
 /* Touch status */
+#define MXT_UNGRIP		(1 << 0)
 #define MXT_SUPPRESS		(1 << 1)
 #define MXT_AMP			(1 << 2)
 #define MXT_VECTOR		(1 << 3)
@@ -239,14 +240,6 @@ struct mxt_message {
 	u8 message[7];
 };
 
-struct mxt_finger {
-	int status;
-	int x;
-	int y;
-	int area;
-	int pressure;
-};
-
 /* Each client has this additional data */
 struct mxt_data {
 	struct i2c_client *client;
@@ -254,7 +247,6 @@ struct mxt_data {
 	const struct mxt_platform_data *pdata;
 	struct mxt_object *object_table;
 	struct mxt_info info;
-	struct mxt_finger finger[MXT_MAX_FINGER];
 	unsigned int irq;
 	unsigned int max_x;
 	unsigned int max_y;
@@ -488,97 +480,54 @@ static int mxt_write_object(struct mxt_data *data,
 	return mxt_write_reg(data->client, reg + offset, 1, &val);
 }
 
-static void mxt_input_report(struct mxt_data *data, int single_id)
-{
-	struct mxt_finger *finger = data->finger;
-	struct input_dev *input_dev = data->input_dev;
-	int status = finger[single_id].status;
-	int finger_num = 0;
-	int id;
-
-	for (id = 0; id < MXT_MAX_FINGER; id++) {
-		if (!finger[id].status)
-			continue;
-
-		input_mt_slot(input_dev, id);
-		input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
-				finger[id].status != MXT_RELEASE);
-
-		if (finger[id].status != MXT_RELEASE) {
-			finger_num++;
-			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
-					finger[id].area);
-			input_report_abs(input_dev, ABS_MT_POSITION_X,
-					finger[id].x);
-			input_report_abs(input_dev, ABS_MT_POSITION_Y,
-					finger[id].y);
-			input_report_abs(input_dev, ABS_MT_PRESSURE,
-					finger[id].pressure);
-		} else {
-			finger[id].status = 0;
-		}
-	}
-
-	input_report_key(input_dev, BTN_TOUCH, finger_num > 0);
-
-	if (status != MXT_RELEASE) {
-		input_report_abs(input_dev, ABS_X, finger[single_id].x);
-		input_report_abs(input_dev, ABS_Y, finger[single_id].y);
-		input_report_abs(input_dev,
-				 ABS_PRESSURE, finger[single_id].pressure);
-	}
-
-	input_sync(input_dev);
-}
-
 static void mxt_input_touchevent(struct mxt_data *data,
-				      struct mxt_message *message, int id)
+				 struct mxt_message *message, int id)
 {
-	struct mxt_finger *finger = data->finger;
 	struct device *dev = &data->client->dev;
-	u8 status = message->message[0];
+	struct input_dev *input_dev = data->input_dev;
+	u8 status;
 	int x;
 	int y;
 	int area;
-	int pressure;
-
-	/* Check the touch is present on the screen */
-	if (!(status & MXT_DETECT)) {
-		if (status & MXT_RELEASE) {
-			dev_dbg(dev, "[%d] released\n", id);
-
-			finger[id].status = MXT_RELEASE;
-			mxt_input_report(data, id);
-		}
-		return;
-	}
-
-	/* Check only AMP detection */
-	if (!(status & (MXT_PRESS | MXT_MOVE)))
-		return;
+	int amplitude;
 
+	status = message->message[0];
 	x = (message->message[1] << 4) | ((message->message[3] >> 4) & 0xf);
 	y = (message->message[2] << 4) | ((message->message[3] & 0xf));
 	if (data->max_x < 1024)
-		x = x >> 2;
+		x >>= 2;
 	if (data->max_y < 1024)
-		y = y >> 2;
+		y >>= 2;
 
 	area = message->message[4];
-	pressure = message->message[5];
-
-	dev_dbg(dev, "[%d] %s x: %d, y: %d, area: %d\n", id,
-		status & MXT_MOVE ? "moved" : "pressed",
-		x, y, area);
-
-	finger[id].status = status & MXT_MOVE ?
-				MXT_MOVE : MXT_PRESS;
-	finger[id].x = x;
-	finger[id].y = y;
-	finger[id].area = area;
-	finger[id].pressure = pressure;
+	amplitude = message->message[5];
+
+	dev_dbg(dev,
+		"[%d] %c%c%c%c%c%c%c%c x: %d y: %d area: %d amp: %d\n",
+		id,
+		(status & MXT_DETECT) ? 'D' : '.',
+		(status & MXT_PRESS) ? 'P' : '.',
+		(status & MXT_RELEASE) ? 'R' : '.',
+		(status & MXT_MOVE) ? 'M' : '.',
+		(status & MXT_VECTOR) ? 'V' : '.',
+		(status & MXT_AMP) ? 'A' : '.',
+		(status & MXT_SUPPRESS) ? 'S' : '.',
+		(status & MXT_UNGRIP) ? 'U' : '.',
+		x, y, area, amplitude);
+
+	input_mt_slot(input_dev, id);
+	input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
+				   status & MXT_DETECT);
+
+	if (status & MXT_DETECT) {
+		input_report_abs(input_dev, ABS_MT_POSITION_X, x);
+		input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
+		input_report_abs(input_dev, ABS_MT_PRESSURE, amplitude);
+		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
+	}
 
-	mxt_input_report(data, id);
+	input_mt_report_pointer_emulation(input_dev, false);
+	input_sync(input_dev);
 }
 
 static irqreturn_t mxt_interrupt(int irq, void *dev_id)
-- 
1.7.7.3

[PATCH 13/16 v2] Input: atmel_mxt_ts - cache T9 reportid range when reading object table

[Daniel Kurtz]

Streamline interrupt processing by caching the T9 reportid range when
first reading the object table.

Note: the cached T9 reportid's are initialized to 0, which is an invalid
reportid.  Thus, the checks in the interrupt handler will always fail for
devices that do not support the T9 object.

One side effect of this is we can know the max number of contacts that
can be tracked, and therefore can properly report max number of MT-B slots
to userspace instead of assuming a fixed 10.

This patch tested on an MXT224E.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   67 +++++++++++++++--------------
 1 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 731293b..40dd3e6 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -212,8 +212,6 @@
 /* Touchscreen absolute values */
 #define MXT_MAX_AREA		0xff
 
-#define MXT_MAX_FINGER		10
-
 struct mxt_info {
 	u8 family_id;
 	u8 variant_id;
@@ -230,9 +228,6 @@ struct mxt_object {
 	u16 size;
 	u16 instances;
 	u8 num_report_ids;
-
-	/* to map object and message */
-	u8 max_reportid;
 };
 
 struct mxt_message {
@@ -250,6 +245,10 @@ struct mxt_data {
 	unsigned int irq;
 	unsigned int max_x;
 	unsigned int max_y;
+
+	/* Cached parameters from object table */
+	u8 T9_reportid_min;
+	u8 T9_reportid_max;
 };
 
 static bool mxt_object_readable(unsigned int type)
@@ -480,8 +479,7 @@ static int mxt_write_object(struct mxt_data *data,
 	return mxt_write_reg(data->client, reg + offset, 1, &val);
 }
 
-static void mxt_input_touchevent(struct mxt_data *data,
-				 struct mxt_message *message, int id)
+static void mxt_input_touch(struct mxt_data *data, struct mxt_message *message)
 {
 	struct device *dev = &data->client->dev;
 	struct input_dev *input_dev = data->input_dev;
@@ -490,6 +488,9 @@ static void mxt_input_touchevent(struct mxt_data *data,
 	int y;
 	int area;
 	int amplitude;
+	int id;
+
+	id = message->reportid - data->T9_reportid_min;
 
 	status = message->message[0];
 	x = (message->message[1] << 4) | ((message->message[3] >> 4) & 0xf);
@@ -534,12 +535,7 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
 {
 	struct mxt_data *data = dev_id;
 	struct mxt_message message;
-	struct mxt_object *object;
 	struct device *dev = &data->client->dev;
-	int id;
-	u8 reportid;
-	u8 max_reportid;
-	u8 min_reportid;
 
 	do {
 		if (mxt_read_message(data, &message)) {
@@ -547,22 +543,13 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
 			goto end;
 		}
 
-		reportid = message.reportid;
-
-		/* whether reportid is thing of MXT_TOUCH_MULTI_T9 */
-		object = mxt_get_object(data, MXT_TOUCH_MULTI_T9);
-		if (!object)
-			goto end;
-
-		max_reportid = object->max_reportid;
-		min_reportid = max_reportid - object->num_report_ids + 1;
-		id = reportid - min_reportid;
-
-		if (reportid >= min_reportid && reportid <= max_reportid)
-			mxt_input_touchevent(data, &message, id);
-		else
+		if (message.reportid >= data->T9_reportid_min &&
+		    message.reportid <= data->T9_reportid_max) {
+			mxt_input_touch(data, &message);
+		} else {
 			mxt_dump_message(dev, &message);
-	} while (reportid != 0xff);
+		}
+	} while (message.reportid != 0xff);
 
 end:
 	return IRQ_HANDLED;
@@ -679,7 +666,7 @@ static int mxt_get_object_table(struct mxt_data *data)
 	struct device *dev = &client->dev;
 	int error;
 	int i;
-	u8 reportid = 0;
+	u8 reportid;
 	u8 *buf;
 	size_t buf_size;
 
@@ -701,9 +688,12 @@ static int mxt_get_object_table(struct mxt_data *data)
 	if (error)
 		goto done;
 
+	/* Valid Report IDs start counting from 1 */
+	reportid = 1;
 	for (i = 0; i < data->info.object_num; i++) {
 		struct mxt_object *object = &data->object_table[i];
 		u8 *obj_buf = &buf[i * MXT_OBJECT_SIZE];
+		u8 num_ids, min_id, max_id;
 
 		object->type = obj_buf[0];
 		object->start_address = (obj_buf[2] << 8) | obj_buf[1];
@@ -711,9 +701,21 @@ static int mxt_get_object_table(struct mxt_data *data)
 		object->instances = obj_buf[4] + 1;
 		object->num_report_ids = obj_buf[5];
 
-		if (object->num_report_ids) {
-			reportid += object->num_report_ids * object->instances;
-			object->max_reportid = reportid;
+		num_ids = object->num_report_ids * object->instances;
+		min_id = num_ids ? reportid : 0;
+		max_id = num_ids ? reportid + num_ids - 1 : 0;
+		reportid += num_ids;
+
+		dev_info(dev,
+			 "Type %2d Start %3d Size %3d Instances %2d ReportIDs %3u : %3u\n",
+			 object->type, object->start_address, object->size,
+			 object->instances, min_id, max_id);
+
+		switch (object->type) {
+		case MXT_TOUCH_MULTI_T9:
+			data->T9_reportid_min = min_id;
+			data->T9_reportid_max = max_id;
+			break;
 		}
 	}
 
@@ -1023,7 +1025,8 @@ static int __devinit mxt_probe(struct i2c_client *client,
 			     0, 255, 0, 0);
 
 	/* For multi touch */
-	input_mt_init_slots(input_dev, MXT_MAX_FINGER);
+	input_mt_init_slots(input_dev,
+			    data->T9_reportid_max - data->T9_reportid_min + 1);
 	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR,
 			     0, MXT_MAX_AREA, 0, 0);
 	input_set_abs_params(input_dev, ABS_MT_POSITION_X,
-- 
1.7.7.3

[PATCH 14/16 v2] Input: atmel_mxt_ts - parse vector field of data packets

[Daniel Kurtz]

The atmel_mxt_ts T9 data contains orientation information in its 'vector'
field. Parse and debug print its contents, although its value isn't
actually used yet.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
Acked-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 40dd3e6..be1e2ec 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -489,6 +489,7 @@ static void mxt_input_touch(struct mxt_data *data, struct mxt_message *message)
 	int area;
 	int amplitude;
 	int id;
+	int vector1, vector2;
 
 	id = message->reportid - data->T9_reportid_min;
 
@@ -503,8 +504,12 @@ static void mxt_input_touch(struct mxt_data *data, struct mxt_message *message)
 	area = message->message[4];
 	amplitude = message->message[5];
 
+	/* The two vector components are 4-bit signed ints (2s complement) */
+	vector1 = (signed)((signed char)message->message[6]) >> 4;
+	vector2 = (signed)((signed char)(message->message[6] << 4)) >> 4;
+
 	dev_dbg(dev,
-		"[%d] %c%c%c%c%c%c%c%c x: %d y: %d area: %d amp: %d\n",
+		"[%d] %c%c%c%c%c%c%c%c x: %d y: %d area: %d amp: %d vector: [%d,%d]\n",
 		id,
 		(status & MXT_DETECT) ? 'D' : '.',
 		(status & MXT_PRESS) ? 'P' : '.',
@@ -514,7 +519,7 @@ static void mxt_input_touch(struct mxt_data *data, struct mxt_message *message)
 		(status & MXT_AMP) ? 'A' : '.',
 		(status & MXT_SUPPRESS) ? 'S' : '.',
 		(status & MXT_UNGRIP) ? 'U' : '.',
-		x, y, area, amplitude);
+		x, y, area, amplitude, vector1, vector2);
 
 	input_mt_slot(input_dev, id);
 	input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
@@ -525,6 +530,7 @@ static void mxt_input_touch(struct mxt_data *data, struct mxt_message *message)
 		input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
 		input_report_abs(input_dev, ABS_MT_PRESSURE, amplitude);
 		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
+		/* TODO: Use vector to report ORIENTATION & TOUCH_MINOR */
 	}
 
 	input_mt_report_pointer_emulation(input_dev, false);
-- 
1.7.7.3

[PATCH 15/16 v2] Input: atmel_mxt_ts - send all MT-B slots in one input report

[Daniel Kurtz]

Each interrupt contains information for all contacts with changing
properties.  Process all of this information at once, and send it all in a
a single input report (ie input events ending in EV_SYN/SYN_REPORT).

This patch was tested using an MXT224E.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index be1e2ec..d8b23ad 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -532,9 +532,6 @@ static void mxt_input_touch(struct mxt_data *data, struct mxt_message *message)
 		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
 		/* TODO: Use vector to report ORIENTATION & TOUCH_MINOR */
 	}
-
-	input_mt_report_pointer_emulation(input_dev, false);
-	input_sync(input_dev);
 }
 
 static irqreturn_t mxt_interrupt(int irq, void *dev_id)
@@ -542,7 +539,9 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
 	struct mxt_data *data = dev_id;
 	struct mxt_message message;
 	struct device *dev = &data->client->dev;
+	bool update_input;
 
+	update_input = false;
 	do {
 		if (mxt_read_message(data, &message)) {
 			dev_err(dev, "Failed to read message\n");
@@ -552,11 +551,17 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
 		if (message.reportid >= data->T9_reportid_min &&
 		    message.reportid <= data->T9_reportid_max) {
 			mxt_input_touch(data, &message);
+			update_input = true;
 		} else {
 			mxt_dump_message(dev, &message);
 		}
 	} while (message.reportid != 0xff);
 
+	if (update_input) {
+		input_mt_report_pointer_emulation(data->input_dev, false);
+		input_sync(data->input_dev);
+	}
+
 end:
 	return IRQ_HANDLED;
 }
-- 
1.7.7.3

[PATCH 16/16 v2] Input: atmel_mxt_ts - parse T6 reports

[Daniel Kurtz]

The normal messages sent after boot or NVRAM update are T6 reports,
containing a status, and the config memory checksum.  Parse them and dump
a useful info message.

This patch tested on an MXT224E.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index d8b23ad..f3dfb4f 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -247,6 +247,7 @@ struct mxt_data {
 	unsigned int max_y;
 
 	/* Cached parameters from object table */
+	u8 T6_reportid;
 	u8 T9_reportid_min;
 	u8 T9_reportid_max;
 };
@@ -552,6 +553,12 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
 		    message.reportid <= data->T9_reportid_max) {
 			mxt_input_touch(data, &message);
 			update_input = true;
+		} else if (message.reportid == data->T6_reportid) {
+			unsigned csum = message.message[1] |
+					(message.message[2] << 8) |
+					(message.message[3] << 16);
+			dev_info(dev, "Status: %02x Config Checksum: %06x\n",
+				 message.message[0], csum);
 		} else {
 			mxt_dump_message(dev, &message);
 		}
@@ -723,6 +730,9 @@ static int mxt_get_object_table(struct mxt_data *data)
 			 object->instances, min_id, max_id);
 
 		switch (object->type) {
+		case MXT_GEN_COMMAND_T6:
+			data->T6_reportid = min_id;
+			break;
 		case MXT_TOUCH_MULTI_T9:
 			data->T9_reportid_min = min_id;
 			data->T9_reportid_max = max_id;
-- 
1.7.7.3

Re: [PATCH 00/16 v2] cleanup atmel_mxt_ts

[Daniel Kurtz]

On Fri, Mar 30, 2012 at 12:49 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> This patchset cleans up the atmel_mxt_ts touchscreen driver.
> In particular, v2 implements the following:
>
> 1) sysfs
>   a) fw_update only by root
> 2) faster initialization
>   a) read/write sets of registers using i2c block transactions
>   b) fetch object table as a set of i2c reads, one per object
>   c) write config data as a set of i2c writes, one per object
> 3) faster interrupt processing & initialization times
>   a) cache important values at init instead of computing in isr
>   b) don't read message checksum byte (which isn't even enabled in fw)
> 4) more correct MT-B support
>   a) send all (changed) contacts in a single EV_SYN/SYN_REPORT
>
> v2:
>  * Writable config via sysfs has been removed to resolve possible
>    conflict with Nick Dyer's implementation.
>  * Using T44 has been removed from this patchset, hopefully it can
>    be reexamined after most of these patches are accepted.
>
> The patches were tested using an MXT224E.

Joonyoung, Nick, Henrik,

Can you please take a look at this second version of the atmel_mxt_ts patches?
Thanks!

>
> Daniel Kurtz (16):
>  Input: atmel_mxt_ts - use CONFIG_PM_SLEEP
>  Input: atmel_mxt_ts - only allow root to update firmware
>  Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length
>  Input: atmel_mxt_ts - store actual size and instance
>  Input: atmel_mxt_ts - verify object size in mxt_write_object
>  Input: atmel_mxt_ts - do not read extra (checksum) byte
>  Input: atmel_mxt_ts - dump each message on just 1 line
>  Input: atmel_mxt_ts - refactor mxt_object_show
>  Input: atmel_mxt_ts - optimize writing of object table entries
>  Input: atmel_mxt_ts - refactor get info
>  Input: atmel_mxt_ts - refactor reading object table
>  Input: atmel_mxt_ts - simplify event reporting
>  Input: atmel_mxt_ts - cache T9 reportid range when reading object
>    table
>  Input: atmel_mxt_ts - parse vector field of data packets
>  Input: atmel_mxt_ts - send all MT-B slots in one input report
>  Input: atmel_mxt_ts - parse T6 reports
>
>  drivers/input/touchscreen/atmel_mxt_ts.c |  477 ++++++++++++------------------
>  1 files changed, 191 insertions(+), 286 deletions(-)
>
> --
> 1.7.7.3
>

Re: [PATCH 00/16 v2] cleanup atmel_mxt_ts

[Daniel Kurtz]

On Fri, Mar 30, 2012 at 12:49 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> This patchset cleans up the atmel_mxt_ts touchscreen driver.
> In particular, v2 implements the following:
>
> 1) sysfs
>   a) fw_update only by root
> 2) faster initialization
>   a) read/write sets of registers using i2c block transactions
>   b) fetch object table as a set of i2c reads, one per object
>   c) write config data as a set of i2c writes, one per object
> 3) faster interrupt processing & initialization times
>   a) cache important values at init instead of computing in isr
>   b) don't read message checksum byte (which isn't even enabled in fw)
> 4) more correct MT-B support
>   a) send all (changed) contacts in a single EV_SYN/SYN_REPORT
>
> v2:
>  * Writable config via sysfs has been removed to resolve possible
>    conflict with Nick Dyer's implementation.
>  * Using T44 has been removed from this patchset, hopefully it can
>    be reexamined after most of these patches are accepted.
>
> The patches were tested using an MXT224E.

Joonyoung, Nick, Henrik,

Can you please take a look at this second version of the atmel_mxt_ts patches?
Thanks!

>
> Daniel Kurtz (16):
>  Input: atmel_mxt_ts - use CONFIG_PM_SLEEP
>  Input: atmel_mxt_ts - only allow root to update firmware
>  Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length
>  Input: atmel_mxt_ts - store actual size and instance
>  Input: atmel_mxt_ts - verify object size in mxt_write_object
>  Input: atmel_mxt_ts - do not read extra (checksum) byte
>  Input: atmel_mxt_ts - dump each message on just 1 line
>  Input: atmel_mxt_ts - refactor mxt_object_show
>  Input: atmel_mxt_ts - optimize writing of object table entries
>  Input: atmel_mxt_ts - refactor get info
>  Input: atmel_mxt_ts - refactor reading object table
>  Input: atmel_mxt_ts - simplify event reporting
>  Input: atmel_mxt_ts - cache T9 reportid range when reading object
>    table
>  Input: atmel_mxt_ts - parse vector field of data packets
>  Input: atmel_mxt_ts - send all MT-B slots in one input report
>  Input: atmel_mxt_ts - parse T6 reports
>
>  drivers/input/touchscreen/atmel_mxt_ts.c |  477 ++++++++++++------------------
>  1 files changed, 191 insertions(+), 286 deletions(-)
>
> --
> 1.7.7.3
>
--
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

Re: [PATCH 03/16 v2] Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length

[Henrik Rydberg]

Hi Daniel,

> The i2c bus requires 5 bytes to do a 1 byte read (1-byte i2c address + 2
> byte offset + 1-byte i2c address + 1 byte data), or 4 bytes to do a
> 1-byte write (1 byte i2c address + 2 byte offset + 1 byte data).
> 
> By taking a length with reads and writes, the driver can amortize
> transaction overhead by performing larger transactions where appropriate.
> 
> This patch just sets up the new API.  Later patches refactor reads/writes
> to take advantage of the larger transactions.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |   47 +++++++++++++----------------
>  1 files changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 15ae6fd..971f9dc 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -397,8 +397,7 @@ static int mxt_fw_write(struct i2c_client *client,
>  	return 0;
>  }
>  
> -static int __mxt_read_reg(struct i2c_client *client,
> -			       u16 reg, u16 len, void *val)
> +static int mxt_read_reg(struct i2c_client *client, u16 reg, u16 len, void *val)
>  {
>  	struct i2c_msg xfer[2];
>  	u8 buf[2];
> @@ -419,28 +418,25 @@ static int __mxt_read_reg(struct i2c_client *client,
>  	xfer[1].buf = val;
>  
>  	if (i2c_transfer(client->adapter, xfer, 2) != 2) {
> -		dev_err(&client->dev, "%s: i2c transfer failed\n", __func__);
> +		dev_err(&client->dev, "%s: i2c read failed\n", __func__);
>  		return -EIO;
>  	}
>  
>  	return 0;
>  }
>  
> -static int mxt_read_reg(struct i2c_client *client, u16 reg, u8 *val)
> +static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
> +			 const void *val)
>  {
> -	return __mxt_read_reg(client, reg, 1, val);
> -}
> -
> -static int mxt_write_reg(struct i2c_client *client, u16 reg, u8 val)
> -{
> -	u8 buf[3];
> +	size_t count = 2 + len;		/* + 2-byte offset */
> +	u8 buf[count];

How much stack space are you planning to use here?

>  
>  	buf[0] = reg & 0xff;
>  	buf[1] = (reg >> 8) & 0xff;
> -	buf[2] = val;
> +	memcpy(&buf[2], val, len);
>  
> -	if (i2c_master_send(client, buf, 3) != 3) {
> -		dev_err(&client->dev, "%s: i2c send failed\n", __func__);
> +	if (i2c_master_send(client, buf, count) != count) {
> +		dev_err(&client->dev, "%s: i2c write failed\n", __func__);
>  		return -EIO;
>  	}
>  
> @@ -450,8 +446,7 @@ static int mxt_write_reg(struct i2c_client *client, u16 reg, u8 val)
>  static int mxt_read_object_table(struct i2c_client *client,
>  				      u16 reg, u8 *object_buf)
>  {
> -	return __mxt_read_reg(client, reg, MXT_OBJECT_SIZE,
> -				   object_buf);
> +	return mxt_read_reg(client, reg, MXT_OBJECT_SIZE, object_buf);
>  }
>  
>  static struct mxt_object *
> @@ -481,8 +476,8 @@ static int mxt_read_message(struct mxt_data *data,
>  		return -EINVAL;
>  
>  	reg = object->start_address;
> -	return __mxt_read_reg(data->client, reg,
> -			sizeof(struct mxt_message), message);
> +	return mxt_read_reg(data->client, reg, sizeof(struct mxt_message),
> +			    message);
>  }
>  
>  static int mxt_read_object(struct mxt_data *data,
> @@ -496,7 +491,7 @@ static int mxt_read_object(struct mxt_data *data,
>  		return -EINVAL;
>  
>  	reg = object->start_address;
> -	return __mxt_read_reg(data->client, reg + offset, 1, val);
> +	return mxt_read_reg(data->client, reg + offset, 1, val);
>  }
>  
>  static int mxt_write_object(struct mxt_data *data,
> @@ -510,7 +505,7 @@ static int mxt_write_object(struct mxt_data *data,
>  		return -EINVAL;
>  
>  	reg = object->start_address;
> -	return mxt_write_reg(data->client, reg + offset, val);
> +	return mxt_write_reg(data->client, reg + offset, 1, &val);
>  }
>  
>  static void mxt_input_report(struct mxt_data *data, int single_id)
> @@ -757,27 +752,27 @@ static int mxt_get_info(struct mxt_data *data)
>  	int error;
>  	u8 val;
>  
> -	error = mxt_read_reg(client, MXT_FAMILY_ID, &val);
> +	error = mxt_read_reg(client, MXT_FAMILY_ID, 1, &val);
>  	if (error)
>  		return error;
>  	info->family_id = val;
>  
> -	error = mxt_read_reg(client, MXT_VARIANT_ID, &val);
> +	error = mxt_read_reg(client, MXT_VARIANT_ID, 1, &val);
>  	if (error)
>  		return error;
>  	info->variant_id = val;
>  
> -	error = mxt_read_reg(client, MXT_VERSION, &val);
> +	error = mxt_read_reg(client, MXT_VERSION, 1, &val);
>  	if (error)
>  		return error;
>  	info->version = val;
>  
> -	error = mxt_read_reg(client, MXT_BUILD, &val);
> +	error = mxt_read_reg(client, MXT_BUILD, 1, &val);
>  	if (error)
>  		return error;
>  	info->build = val;
>  
> -	error = mxt_read_reg(client, MXT_OBJECT_NUM, &val);
> +	error = mxt_read_reg(client, MXT_OBJECT_NUM, 1, &val);
>  	if (error)
>  		return error;
>  	info->object_num = val;
> @@ -860,12 +855,12 @@ static int mxt_initialize(struct mxt_data *data)
>  	msleep(MXT_RESET_TIME);
>  
>  	/* Update matrix size at info struct */
> -	error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, &val);
> +	error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, 1, &val);
>  	if (error)
>  		return error;
>  	info->matrix_xsize = val;
>  
> -	error = mxt_read_reg(client, MXT_MATRIX_Y_SIZE, &val);
> +	error = mxt_read_reg(client, MXT_MATRIX_Y_SIZE, 1, &val);
>  	if (error)
>  		return error;
>  	info->matrix_ysize = val;
> -- 
> 1.7.7.3
> 

Thanks,
Henrik

Re: [PATCH 08/16 v2] Input: atmel_mxt_ts - refactor mxt_object_show

[Henrik Rydberg]

On Fri, Mar 30, 2012 at 12:49:18AM +0800, Daniel Kurtz wrote:
> Read each object with a single i2c transaction instead of byte-by-byte.
> 
> Also, don't read T5, which is the message processor object.  Reading
> it is counter-productive since it either holds garbage (there is no
> pending message), or, it holds a real message which should be handled
> by the messages handling code (the isr).
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |   43 ++++++++++++------------------
>  1 files changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index b6e7109..a865967 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -262,7 +262,6 @@ struct mxt_data {
>  static bool mxt_object_readable(unsigned int type)
>  {
>  	switch (type) {
> -	case MXT_GEN_MESSAGE_T5:
>  	case MXT_GEN_COMMAND_T6:
>  	case MXT_GEN_POWER_T7:
>  	case MXT_GEN_ACQUIRE_T8:
> @@ -459,6 +458,13 @@ mxt_get_object(struct mxt_data *data, u8 type)
>  	return NULL;
>  }
>  
> +static int mxt_read_object(struct mxt_data *data, struct mxt_object *object,
> +			   void *val)
> +{
> +	return mxt_read_reg(data->client, object->start_address, object->size,
> +			    val);
> +}
> +
>  static int mxt_read_message(struct mxt_data *data,
>  				 struct mxt_message *message)
>  {
> @@ -474,20 +480,6 @@ static int mxt_read_message(struct mxt_data *data,
>  			    message);
>  }
>  
> -static int mxt_read_object(struct mxt_data *data,
> -				u8 type, u8 offset, u8 *val)
> -{
> -	struct mxt_object *object;
> -	u16 reg;
> -
> -	object = mxt_get_object(data, type);
> -	if (!object)
> -		return -EINVAL;
> -
> -	reg = object->start_address;
> -	return mxt_read_reg(data->client, reg + offset, 1, val);
> -}
> -
>  static int mxt_write_object(struct mxt_data *data,
>  				 u8 type, u8 offset, u8 val)
>  {
> @@ -885,17 +877,16 @@ static void mxt_calc_resolution(struct mxt_data *data)
>  }
>  
>  static ssize_t mxt_object_show(struct device *dev,
> -				    struct device_attribute *attr, char *buf)
> +			       struct device_attribute *attr, char *buf)
>  {
>  	struct mxt_data *data = dev_get_drvdata(dev);
> -	struct mxt_object *object;
>  	int count = 0;
>  	int i, j;
>  	int error;
> -	u8 val;
> +	u8 obuf[256];

How large buffer do you need?

>  
>  	for (i = 0; i < data->info.object_num; i++) {
> -		object = data->object_table + i;
> +		struct mxt_object *object = &data->object_table[i];
>  
>  		count += snprintf(buf + count, PAGE_SIZE - count,
>  				"Object[%d] (Type %d)\n",
> @@ -905,20 +896,20 @@ static ssize_t mxt_object_show(struct device *dev,
>  
>  		if (!mxt_object_readable(object->type)) {
>  			count += snprintf(buf + count, PAGE_SIZE - count,
> -					"\n");
> +					  "\n");
>  			if (count >= PAGE_SIZE)
>  				return PAGE_SIZE - 1;

Odd return value - why not use scnprintf()?

>  			continue;
>  		}
>  
> -		for (j = 0; j < object->size; j++) {
> -			error = mxt_read_object(data,
> -						object->type, j, &val);
> -			if (error)
> -				return error;
> +		error = mxt_read_object(data, object, obuf);
> +		if (error)
> +			return error;
>  
> +		for (j = 0; j < object->size; j++) {
>  			count += snprintf(buf + count, PAGE_SIZE - count,
> -					"\t[%2d]: %02x (%d)\n", j, val, val);
> +					  "\t[%2d]: %02x (%d)\n", j, obuf[j],
> +					  obuf[j]);
>  			if (count >= PAGE_SIZE)
>  				return PAGE_SIZE - 1;

Ditto.

>  		}
> -- 
> 1.7.7.3
> 

Thanks,
Henrik

Re: [PATCH 11/16 v2] Input: atmel_mxt_ts - refactor reading object table

[Henrik Rydberg]

Hi Daniel,

> Instead of reading each object separately, fetch the whole table in one
> large i2c transaction.  A 6 byte table object requires 10 bytes to read,
> so doing this dramatically reduces overhead.
> 
> Also, as a cleanup, move object_table allocation (and post-fw-update
> reallocation) into mxt_get_object_table().
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |   66 +++++++++++++++---------------
>  1 files changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 9d88faf..0d0dab6 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -437,14 +437,7 @@ static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
>  	return 0;
>  }
>  
> -static int mxt_read_object_table(struct i2c_client *client,
> -				      u16 reg, u8 *object_buf)
> -{
> -	return mxt_read_reg(client, reg, MXT_OBJECT_SIZE, object_buf);
> -}
> -
> -static struct mxt_object *
> -mxt_get_object(struct mxt_data *data, u8 type)
> +static struct mxt_object *mxt_get_object(struct mxt_data *data, u8 type)
>  {
>  	struct mxt_object *object;
>  	int i;
> @@ -733,25 +726,41 @@ static void mxt_handle_pdata(struct mxt_data *data)
>  
>  static int mxt_get_object_table(struct mxt_data *data)
>  {
> +	struct i2c_client *client = data->client;
> +	struct device *dev = &client->dev;
>  	int error;
>  	int i;
> -	u16 reg;
>  	u8 reportid = 0;
> -	u8 buf[MXT_OBJECT_SIZE];
> +	u8 *buf;
> +	size_t buf_size;
>  
> -	for (i = 0; i < data->info.object_num; i++) {
> -		struct mxt_object *object = data->object_table + i;
> +	/* Free old object table, if there was one. */
> +	kfree(data->object_table);
> +	data->object_table = kcalloc(data->info.object_num,
> +				     sizeof(struct mxt_object), GFP_KERNEL);
> +	if (!data->object_table) {
> +		dev_err(dev, "Failed to allocate object table\n");
> +		return -ENOMEM;
> +	}
>  
> -		reg = MXT_OBJECT_START + MXT_OBJECT_SIZE * i;
> -		error = mxt_read_object_table(data->client, reg, buf);
> -		if (error)
> -			return error;
> +	buf_size = MXT_OBJECT_SIZE * data->info.object_num;
> +	buf = kmalloc(buf_size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
>  
> -		object->type = buf[0];
> -		object->start_address = (buf[2] << 8) | buf[1];
> -		object->size = buf[3] + 1;
> -		object->instances = buf[4] + 1;
> -		object->num_report_ids = buf[5];
> +	error = mxt_read_reg(client, MXT_OBJECT_START, buf_size, buf);
> +	if (error)
> +		goto done;
> +
> +	for (i = 0; i < data->info.object_num; i++) {
> +		struct mxt_object *object = &data->object_table[i];
> +		u8 *obj_buf = &buf[i * MXT_OBJECT_SIZE];
> +
> +		object->type = obj_buf[0];
> +		object->start_address = (obj_buf[2] << 8) | obj_buf[1];
> +		object->size = obj_buf[3] + 1;
> +		object->instances = obj_buf[4] + 1;
> +		object->num_report_ids = obj_buf[5];

Putting this conversion in a function would be nice, it would also
make it clear that the inverse, used for writing, is currently
missing. Alternatively, how about making the object struct actually
match the read data? To top it off, one could introduce a collection,
prepending the buffer size, making the write operation trivial.

struct mxt_raw_object {
	u8 type;
	__le16 start_address_le;
	u8 size_minus_one;
	u8 instances_minus_one;
	u8 num_report_ids;
};

struct mxt_raw_object_collection {
       __le16 total_bytes_le;
       struct mxt_raw_object instance[MAX_NUM_OBJECTS];
};

>  
>  		if (object->num_report_ids) {
>  			reportid += object->num_report_ids * object->instances;
> @@ -759,7 +768,9 @@ static int mxt_get_object_table(struct mxt_data *data)
>  		}
>  	}
>  
> -	return 0;
> +done:
> +	kfree(buf);
> +	return error;
>  }
>  
>  static int mxt_initialize(struct mxt_data *data)
> @@ -774,14 +785,6 @@ static int mxt_initialize(struct mxt_data *data)
>  	if (error)
>  		return error;
>  
> -	data->object_table = kcalloc(info->object_num,
> -				     sizeof(struct mxt_object),
> -				     GFP_KERNEL);
> -	if (!data->object_table) {
> -		dev_err(&client->dev, "Failed to allocate memory\n");
> -		return -ENOMEM;
> -	}
> -
>  	/* Get object table information */
>  	error = mxt_get_object_table(data);
>  	if (error)
> @@ -971,9 +974,6 @@ static ssize_t mxt_update_fw_store(struct device *dev,
>  		/* Wait for reset */
>  		msleep(MXT_FWRESET_TIME);
>  
> -		kfree(data->object_table);
> -		data->object_table = NULL;
> -
>  		mxt_initialize(data);
>  	}
>  
> -- 
> 1.7.7.3
> 

Thanks,
Henrik

Re: [PATCH 09/16 v2] Input: atmel_mxt_ts - optimize writing of object table entries

[Henrik Rydberg]

Hi Daniel,

> Write each object using a single bulk i2c write transfer.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |   28 ++++++++++++++--------------
>  1 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a865967..3abc5b0 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -628,33 +628,33 @@ end:
>  static int mxt_check_reg_init(struct mxt_data *data)
>  {
>  	const struct mxt_platform_data *pdata = data->pdata;
> -	struct mxt_object *object;
>  	struct device *dev = &data->client->dev;
> -	int index = 0;
> -	int i, j, config_offset, config_size;
> +	int i, offset;
> +	int ret;
>  
>  	if (!pdata->config) {
>  		dev_dbg(dev, "No cfg data defined, skipping reg init\n");
>  		return 0;
>  	}
>  
> -	for (i = 0; i < data->info.object_num; i++) {
> -		object = data->object_table + i;
> +	for (offset = 0, i = 0; i < data->info.object_num; i++) {
> +		struct mxt_object *object = &data->object_table[i];
> +		size_t config_size;
>  
>  		if (!mxt_object_writable(object->type))
>  			continue;
>  
>  		config_size = object->size * object->instances;
> -		for (j = 0; j < config_size; j++) {
> -			config_offset = index + j;
> -			if (config_offset > pdata->config_length) {
> -				dev_err(dev, "Not enough config data!\n");
> -				return -EINVAL;
> -			}
> -			mxt_write_object(data, object->type, j,
> -					 pdata->config[config_offset]);
> +		if (offset + config_size > pdata->config_length) {
> +			dev_err(dev, "Not enough config data!\n");
> +			return -EINVAL;
>  		}
> -		index += config_size;
> +
> +		ret = mxt_write_reg(data->client, object->start_address,
> +				    config_size, &pdata->config[offset]);

How big is config_size? Removing the need for a stack copy here would
be better, see patch 11.

> +		if (ret)
> +			return ret;
> +		offset += config_size;
>  	}
>  
>  	return 0;
> -- 
> 1.7.7.3
> 

Thanks,
Henrik

Re: [PATCH 15/16 v2] Input: atmel_mxt_ts - send all MT-B slots in one input report

[Henrik Rydberg]

Hi Daniel,

> Each interrupt contains information for all contacts with changing
> properties.  Process all of this information at once, and send it all in a
> a single input report (ie input events ending in EV_SYN/SYN_REPORT).
> 
> This patch was tested using an MXT224E.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index be1e2ec..d8b23ad 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -532,9 +532,6 @@ static void mxt_input_touch(struct mxt_data *data, struct mxt_message *message)
>  		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
>  		/* TODO: Use vector to report ORIENTATION & TOUCH_MINOR */
>  	}
> -
> -	input_mt_report_pointer_emulation(input_dev, false);
> -	input_sync(input_dev);
>  }
>  
>  static irqreturn_t mxt_interrupt(int irq, void *dev_id)
> @@ -542,7 +539,9 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
>  	struct mxt_data *data = dev_id;
>  	struct mxt_message message;
>  	struct device *dev = &data->client->dev;
> +	bool update_input;
>  
> +	update_input = false;
>  	do {
>  		if (mxt_read_message(data, &message)) {
>  			dev_err(dev, "Failed to read message\n");
> @@ -552,11 +551,17 @@ static irqreturn_t mxt_interrupt(int irq, void *dev_id)
>  		if (message.reportid >= data->T9_reportid_min &&
>  		    message.reportid <= data->T9_reportid_max) {
>  			mxt_input_touch(data, &message);
> +			update_input = true;
>  		} else {
>  			mxt_dump_message(dev, &message);
>  		}
>  	} while (message.reportid != 0xff);
>  
> +	if (update_input) {
> +		input_mt_report_pointer_emulation(data->input_dev, false);
> +		input_sync(data->input_dev);
> +	}
> +

Strictly speaking, the update_input variable is not needed either,
although it will save some cycles. The input core will omit the sync
when there is nothing to report.

>  end:
>  	return IRQ_HANDLED;
>  }
> -- 
> 1.7.7.3
> 

Thanks,
Henrik

Re: [PATCH 11/16 v2] Input: atmel_mxt_ts - refactor reading object table

[Daniel Kurtz]

On Fri, Apr 13, 2012 at 5:11 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Daniel,
>
>> Instead of reading each object separately, fetch the whole table in one
>> large i2c transaction.  A 6 byte table object requires 10 bytes to read,
>> so doing this dramatically reduces overhead.
>>
>> Also, as a cleanup, move object_table allocation (and post-fw-update
>> reallocation) into mxt_get_object_table().
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/input/touchscreen/atmel_mxt_ts.c |   66 +++++++++++++++---------------
>>  1 files changed, 33 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 9d88faf..0d0dab6 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -437,14 +437,7 @@ static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
>>       return 0;
>>  }
>>
>> -static int mxt_read_object_table(struct i2c_client *client,
>> -                                   u16 reg, u8 *object_buf)
>> -{
>> -     return mxt_read_reg(client, reg, MXT_OBJECT_SIZE, object_buf);
>> -}
>> -
>> -static struct mxt_object *
>> -mxt_get_object(struct mxt_data *data, u8 type)
>> +static struct mxt_object *mxt_get_object(struct mxt_data *data, u8 type)
>>  {
>>       struct mxt_object *object;
>>       int i;
>> @@ -733,25 +726,41 @@ static void mxt_handle_pdata(struct mxt_data *data)
>>
>>  static int mxt_get_object_table(struct mxt_data *data)
>>  {
>> +     struct i2c_client *client = data->client;
>> +     struct device *dev = &client->dev;
>>       int error;
>>       int i;
>> -     u16 reg;
>>       u8 reportid = 0;
>> -     u8 buf[MXT_OBJECT_SIZE];
>> +     u8 *buf;
>> +     size_t buf_size;
>>
>> -     for (i = 0; i < data->info.object_num; i++) {
>> -             struct mxt_object *object = data->object_table + i;
>> +     /* Free old object table, if there was one. */
>> +     kfree(data->object_table);
>> +     data->object_table = kcalloc(data->info.object_num,
>> +                                  sizeof(struct mxt_object), GFP_KERNEL);
>> +     if (!data->object_table) {
>> +             dev_err(dev, "Failed to allocate object table\n");
>> +             return -ENOMEM;
>> +     }
>>
>> -             reg = MXT_OBJECT_START + MXT_OBJECT_SIZE * i;
>> -             error = mxt_read_object_table(data->client, reg, buf);
>> -             if (error)
>> -                     return error;
>> +     buf_size = MXT_OBJECT_SIZE * data->info.object_num;
>> +     buf = kmalloc(buf_size, GFP_KERNEL);
>> +     if (!buf)
>> +             return -ENOMEM;
>>
>> -             object->type = buf[0];
>> -             object->start_address = (buf[2] << 8) | buf[1];
>> -             object->size = buf[3] + 1;
>> -             object->instances = buf[4] + 1;
>> -             object->num_report_ids = buf[5];
>> +     error = mxt_read_reg(client, MXT_OBJECT_START, buf_size, buf);
>> +     if (error)
>> +             goto done;
>> +
>> +     for (i = 0; i < data->info.object_num; i++) {
>> +             struct mxt_object *object = &data->object_table[i];
>> +             u8 *obj_buf = &buf[i * MXT_OBJECT_SIZE];
>> +
>> +             object->type = obj_buf[0];
>> +             object->start_address = (obj_buf[2] << 8) | obj_buf[1];
>> +             object->size = obj_buf[3] + 1;
>> +             object->instances = obj_buf[4] + 1;
>> +             object->num_report_ids = obj_buf[5];
>
> Putting this conversion in a function would be nice, it would also
> make it clear that the inverse, used for writing, is currently
> missing. Alternatively, how about making the object struct actually
> match the read data? To top it off, one could introduce a collection,
> prepending the buffer size, making the write operation trivial.

AFAIK, there is no corresponding "object table" write. The "object
table" (maybe more aptly named the "object descriptor table") is an
immutable blob read from firmware that describes some actual data
objects elsewhere in device memory.  It is those actual objects that
are writable.  When they are written, it is the actual size, location
and number of instances (not their raw '-1' values) that is useful.

Thanks,
-Daniel

>
> struct mxt_raw_object {
>        u8 type;
>        __le16 start_address_le;
>        u8 size_minus_one;
>        u8 instances_minus_one;
>        u8 num_report_ids;
> };
>
> struct mxt_raw_object_collection {
>       __le16 total_bytes_le;
>       struct mxt_raw_object instance[MAX_NUM_OBJECTS];
> };
>
>>
>>               if (object->num_report_ids) {
>>                       reportid += object->num_report_ids * object->instances;
>> @@ -759,7 +768,9 @@ static int mxt_get_object_table(struct mxt_data *data)
>>               }
>>       }
>>
>> -     return 0;
>> +done:
>> +     kfree(buf);
>> +     return error;
>>  }
>>
>>  static int mxt_initialize(struct mxt_data *data)
>> @@ -774,14 +785,6 @@ static int mxt_initialize(struct mxt_data *data)
>>       if (error)
>>               return error;
>>
>> -     data->object_table = kcalloc(info->object_num,
>> -                                  sizeof(struct mxt_object),
>> -                                  GFP_KERNEL);
>> -     if (!data->object_table) {
>> -             dev_err(&client->dev, "Failed to allocate memory\n");
>> -             return -ENOMEM;
>> -     }
>> -
>>       /* Get object table information */
>>       error = mxt_get_object_table(data);
>>       if (error)
>> @@ -971,9 +974,6 @@ static ssize_t mxt_update_fw_store(struct device *dev,
>>               /* Wait for reset */
>>               msleep(MXT_FWRESET_TIME);
>>
>> -             kfree(data->object_table);
>> -             data->object_table = NULL;
>> -
>>               mxt_initialize(data);
>>       }
>>
>> --
>> 1.7.7.3
>>
>
> Thanks,
> Henrik

Re: [PATCH 11/16 v2] Input: atmel_mxt_ts - refactor reading object table

[Daniel Kurtz]

On Fri, Apr 13, 2012 at 5:11 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Daniel,
>
>> Instead of reading each object separately, fetch the whole table in one
>> large i2c transaction.  A 6 byte table object requires 10 bytes to read,
>> so doing this dramatically reduces overhead.
>>
>> Also, as a cleanup, move object_table allocation (and post-fw-update
>> reallocation) into mxt_get_object_table().
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/input/touchscreen/atmel_mxt_ts.c |   66 +++++++++++++++---------------
>>  1 files changed, 33 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 9d88faf..0d0dab6 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -437,14 +437,7 @@ static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
>>       return 0;
>>  }
>>
>> -static int mxt_read_object_table(struct i2c_client *client,
>> -                                   u16 reg, u8 *object_buf)
>> -{
>> -     return mxt_read_reg(client, reg, MXT_OBJECT_SIZE, object_buf);
>> -}
>> -
>> -static struct mxt_object *
>> -mxt_get_object(struct mxt_data *data, u8 type)
>> +static struct mxt_object *mxt_get_object(struct mxt_data *data, u8 type)
>>  {
>>       struct mxt_object *object;
>>       int i;
>> @@ -733,25 +726,41 @@ static void mxt_handle_pdata(struct mxt_data *data)
>>
>>  static int mxt_get_object_table(struct mxt_data *data)
>>  {
>> +     struct i2c_client *client = data->client;
>> +     struct device *dev = &client->dev;
>>       int error;
>>       int i;
>> -     u16 reg;
>>       u8 reportid = 0;
>> -     u8 buf[MXT_OBJECT_SIZE];
>> +     u8 *buf;
>> +     size_t buf_size;
>>
>> -     for (i = 0; i < data->info.object_num; i++) {
>> -             struct mxt_object *object = data->object_table + i;
>> +     /* Free old object table, if there was one. */
>> +     kfree(data->object_table);
>> +     data->object_table = kcalloc(data->info.object_num,
>> +                                  sizeof(struct mxt_object), GFP_KERNEL);
>> +     if (!data->object_table) {
>> +             dev_err(dev, "Failed to allocate object table\n");
>> +             return -ENOMEM;
>> +     }
>>
>> -             reg = MXT_OBJECT_START + MXT_OBJECT_SIZE * i;
>> -             error = mxt_read_object_table(data->client, reg, buf);
>> -             if (error)
>> -                     return error;
>> +     buf_size = MXT_OBJECT_SIZE * data->info.object_num;
>> +     buf = kmalloc(buf_size, GFP_KERNEL);
>> +     if (!buf)
>> +             return -ENOMEM;
>>
>> -             object->type = buf[0];
>> -             object->start_address = (buf[2] << 8) | buf[1];
>> -             object->size = buf[3] + 1;
>> -             object->instances = buf[4] + 1;
>> -             object->num_report_ids = buf[5];
>> +     error = mxt_read_reg(client, MXT_OBJECT_START, buf_size, buf);
>> +     if (error)
>> +             goto done;
>> +
>> +     for (i = 0; i < data->info.object_num; i++) {
>> +             struct mxt_object *object = &data->object_table[i];
>> +             u8 *obj_buf = &buf[i * MXT_OBJECT_SIZE];
>> +
>> +             object->type = obj_buf[0];
>> +             object->start_address = (obj_buf[2] << 8) | obj_buf[1];
>> +             object->size = obj_buf[3] + 1;
>> +             object->instances = obj_buf[4] + 1;
>> +             object->num_report_ids = obj_buf[5];
>
> Putting this conversion in a function would be nice, it would also
> make it clear that the inverse, used for writing, is currently
> missing. Alternatively, how about making the object struct actually
> match the read data? To top it off, one could introduce a collection,
> prepending the buffer size, making the write operation trivial.

AFAIK, there is no corresponding "object table" write. The "object
table" (maybe more aptly named the "object descriptor table") is an
immutable blob read from firmware that describes some actual data
objects elsewhere in device memory.  It is those actual objects that
are writable.  When they are written, it is the actual size, location
and number of instances (not their raw '-1' values) that is useful.

Thanks,
-Daniel

>
> struct mxt_raw_object {
>        u8 type;
>        __le16 start_address_le;
>        u8 size_minus_one;
>        u8 instances_minus_one;
>        u8 num_report_ids;
> };
>
> struct mxt_raw_object_collection {
>       __le16 total_bytes_le;
>       struct mxt_raw_object instance[MAX_NUM_OBJECTS];
> };
>
>>
>>               if (object->num_report_ids) {
>>                       reportid += object->num_report_ids * object->instances;
>> @@ -759,7 +768,9 @@ static int mxt_get_object_table(struct mxt_data *data)
>>               }
>>       }
>>
>> -     return 0;
>> +done:
>> +     kfree(buf);
>> +     return error;
>>  }
>>
>>  static int mxt_initialize(struct mxt_data *data)
>> @@ -774,14 +785,6 @@ static int mxt_initialize(struct mxt_data *data)
>>       if (error)
>>               return error;
>>
>> -     data->object_table = kcalloc(info->object_num,
>> -                                  sizeof(struct mxt_object),
>> -                                  GFP_KERNEL);
>> -     if (!data->object_table) {
>> -             dev_err(&client->dev, "Failed to allocate memory\n");
>> -             return -ENOMEM;
>> -     }
>> -
>>       /* Get object table information */
>>       error = mxt_get_object_table(data);
>>       if (error)
>> @@ -971,9 +974,6 @@ static ssize_t mxt_update_fw_store(struct device *dev,
>>               /* Wait for reset */
>>               msleep(MXT_FWRESET_TIME);
>>
>> -             kfree(data->object_table);
>> -             data->object_table = NULL;
>> -
>>               mxt_initialize(data);
>>       }
>>
>> --
>> 1.7.7.3
>>
>
> Thanks,
> Henrik
--
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

Re: [PATCH 11/16 v2] Input: atmel_mxt_ts - refactor reading object table

[Henrik Rydberg]

> > Putting this conversion in a function would be nice, it would also
> > make it clear that the inverse, used for writing, is currently
> > missing. Alternatively, how about making the object struct actually
> > match the read data? To top it off, one could introduce a collection,
> > prepending the buffer size, making the write operation trivial.
> 
> AFAIK, there is no corresponding "object table" write. The "object
> table" (maybe more aptly named the "object descriptor table") is an
> immutable blob read from firmware that describes some actual data
> objects elsewhere in device memory.  It is those actual objects that
> are writable.  When they are written, it is the actual size, location
> and number of instances (not their raw '-1' values) that is useful.

I am talking about the data that is being read here and written in
mxt_check_reg_init(). By matching the struct with that data, all the
copies you make would go away.

Henrik

Re: [PATCH 11/16 v2] Input: atmel_mxt_ts - refactor reading object table

[Daniel Kurtz]

On Fri, Apr 13, 2012 at 5:34 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > Putting this conversion in a function would be nice, it would also
>> > make it clear that the inverse, used for writing, is currently
>> > missing. Alternatively, how about making the object struct actually
>> > match the read data? To top it off, one could introduce a collection,
>> > prepending the buffer size, making the write operation trivial.
>>
>> AFAIK, there is no corresponding "object table" write. The "object
>> table" (maybe more aptly named the "object descriptor table") is an
>> immutable blob read from firmware that describes some actual data
>> objects elsewhere in device memory.  It is those actual objects that
>> are writable.  When they are written, it is the actual size, location
>> and number of instances (not their raw '-1' values) that is useful.
>
> I am talking about the data that is being read here and written in
> mxt_check_reg_init(). By matching the struct with that data, all the
> copies you make would go away.

The data read here is a table of object descriptors.  The data written
in mxt_check_reg_init() is configuration data for each of the objects
described by the descriptors.  What copies are you talking about?

>
> Henrik

The size, number and location of "objects" in device memory are device
specific, and are read into struct mxt_data's object_table, by
mxt_get_object_table().  The order, number and size (and even
interpretation) of the objects varies for different devices.  Some of
these objects are "writable" (as defined by mxt_object_writable()),
although this is a misnomer, and really means "must have corresponding
initialization data in the config array in platform_data".

mxt_check_reg_init() uses i2c to write this initialization data, from
the "config" array to the corresponding locations in device memory.

The config array in platform_data is a single big long array that has
to be manually created in a platform file.  It just has blobs of data
for each of the 'writable' objects for a given device, all
concatenated together.  These blobs must be in the correct order
(matching the order in the device specific object_table), and they
must have the correct size and number of instances for that particular
device.

Personally, I find the way this driver handles configuration and
platform_data is a very scary and fraught with peril, but I am not
trying to fix that in this patch set.  I'm just trying to make what it
currently does a bit faster.

I feel like you are trying to suggest that I fix something in this
patch, but I can't figure out what.
Are you suggesting to not cache the object table (i.e. the object
descriptors), and instead read the table from the device whenever we
want to read or write the objects themselves, such as for the object
sysfs entry or at initialization?  I don't see how that would help.

-Daniel

Re: [PATCH 11/16 v2] Input: atmel_mxt_ts - refactor reading object table

[Daniel Kurtz]

On Fri, Apr 13, 2012 at 5:34 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > Putting this conversion in a function would be nice, it would also
>> > make it clear that the inverse, used for writing, is currently
>> > missing. Alternatively, how about making the object struct actually
>> > match the read data? To top it off, one could introduce a collection,
>> > prepending the buffer size, making the write operation trivial.
>>
>> AFAIK, there is no corresponding "object table" write. The "object
>> table" (maybe more aptly named the "object descriptor table") is an
>> immutable blob read from firmware that describes some actual data
>> objects elsewhere in device memory.  It is those actual objects that
>> are writable.  When they are written, it is the actual size, location
>> and number of instances (not their raw '-1' values) that is useful.
>
> I am talking about the data that is being read here and written in
> mxt_check_reg_init(). By matching the struct with that data, all the
> copies you make would go away.

The data read here is a table of object descriptors.  The data written
in mxt_check_reg_init() is configuration data for each of the objects
described by the descriptors.  What copies are you talking about?

>
> Henrik

The size, number and location of "objects" in device memory are device
specific, and are read into struct mxt_data's object_table, by
mxt_get_object_table().  The order, number and size (and even
interpretation) of the objects varies for different devices.  Some of
these objects are "writable" (as defined by mxt_object_writable()),
although this is a misnomer, and really means "must have corresponding
initialization data in the config array in platform_data".

mxt_check_reg_init() uses i2c to write this initialization data, from
the "config" array to the corresponding locations in device memory.

The config array in platform_data is a single big long array that has
to be manually created in a platform file.  It just has blobs of data
for each of the 'writable' objects for a given device, all
concatenated together.  These blobs must be in the correct order
(matching the order in the device specific object_table), and they
must have the correct size and number of instances for that particular
device.

Personally, I find the way this driver handles configuration and
platform_data is a very scary and fraught with peril, but I am not
trying to fix that in this patch set.  I'm just trying to make what it
currently does a bit faster.

I feel like you are trying to suggest that I fix something in this
patch, but I can't figure out what.
Are you suggesting to not cache the object table (i.e. the object
descriptors), and instead read the table from the device whenever we
want to read or write the objects themselves, such as for the object
sysfs entry or at initialization?  I don't see how that would help.

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

Re: [PATCH 11/16 v2] Input: atmel_mxt_ts - refactor reading object table

[Henrik Rydberg]

> > I am talking about the data that is being read here and written in
> > mxt_check_reg_init(). By matching the struct with that data, all the
> > copies you make would go away.
> 
> The data read here is a table of object descriptors.  The data written
> in mxt_check_reg_init() is configuration data for each of the objects
> described by the descriptors.  What copies are you talking about?

In this patch, the object descriptors are read into a buffer, then
copied over to the object struct that you hold. That is the copy that
obviously goes away if the data is copied directly into the suggested
struct. In patch 9, you copy an unknown amount of data over to a
stack-allocated buffer before sending it via ic2. That is the main
technichal concern, and it was the first question you got, some days
ago.

> The config array in platform_data is a single big long array that has
> to be manually created in a platform file.  It just has blobs of data
> for each of the 'writable' objects for a given device, all
> concatenated together.

Ok, so no need for the second suggested struct or anything of that
kind. Just sort out the maximum blob size and, if necessary, avoid the
stack allocation in mxt_check_reg_init(), and it should be fine.

Henrik

Re: [PATCH 03/16 v2] Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length

[Daniel Kurtz]

On Wed, Apr 11, 2012 at 5:05 PM, Henrik Rydberg <rydberg@bitmath.se> wrote:
>
> Hi Daniel,
>
> > The i2c bus requires 5 bytes to do a 1 byte read (1-byte i2c address + 2
> > byte offset + 1-byte i2c address + 1 byte data), or 4 bytes to do a
> > 1-byte write (1 byte i2c address + 2 byte offset + 1 byte data).
> >
> > By taking a length with reads and writes, the driver can amortize
> > transaction overhead by performing larger transactions where
> > appropriate.
> >
> > This patch just sets up the new API.  Later patches refactor
> > reads/writes
> > to take advantage of the larger transactions.
> >
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c |   47
> > +++++++++++++----------------
> >  1 files changed, 21 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
> > b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 15ae6fd..971f9dc 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -397,8 +397,7 @@ static int mxt_fw_write(struct i2c_client *client,
> >       return 0;
> >  }
> >
> > -static int __mxt_read_reg(struct i2c_client *client,
> > -                            u16 reg, u16 len, void *val)
> > +static int mxt_read_reg(struct i2c_client *client, u16 reg, u16 len,
> > void *val)
> >  {
> >       struct i2c_msg xfer[2];
> >       u8 buf[2];
> > @@ -419,28 +418,25 @@ static int __mxt_read_reg(struct i2c_client
> > *client,
> >       xfer[1].buf = val;
> >
> >       if (i2c_transfer(client->adapter, xfer, 2) != 2) {
> > -             dev_err(&client->dev, "%s: i2c transfer failed\n",
> > __func__);
> > +             dev_err(&client->dev, "%s: i2c read failed\n", __func__);
> >               return -EIO;
> >       }
> >
> >       return 0;
> >  }
> >
> > -static int mxt_read_reg(struct i2c_client *client, u16 reg, u8 *val)
> > +static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
> > +                      const void *val)
> >  {
> > -     return __mxt_read_reg(client, reg, 1, val);
> > -}
> > -
> > -static int mxt_write_reg(struct i2c_client *client, u16 reg, u8 val)
> > -{
> > -     u8 buf[3];
> > +     size_t count = 2 + len;         /* + 2-byte offset */
> > +     u8 buf[count];
>
> How much stack space are you planning to use here?

I missed this one when removing the other stack allocated array since
they are (rightly) frowned upon in the kernel.  I will replace with a
malloc, unless you have a better suggestion?

>
> >
> >       buf[0] = reg & 0xff;
> >       buf[1] = (reg >> 8) & 0xff;
> > -     buf[2] = val;
> > +     memcpy(&buf[2], val, len);
> >
> > -     if (i2c_master_send(client, buf, 3) != 3) {
> > -             dev_err(&client->dev, "%s: i2c send failed\n", __func__);
> > +     if (i2c_master_send(client, buf, count) != count) {
> > +             dev_err(&client->dev, "%s: i2c write failed\n", __func__);
> >               return -EIO;
> >       }
> >
> > @@ -450,8 +446,7 @@ static int mxt_write_reg(struct i2c_client *client,
> > u16 reg, u8 val)
> >  static int mxt_read_object_table(struct i2c_client *client,
> >                                     u16 reg, u8 *object_buf)
> >  {
> > -     return __mxt_read_reg(client, reg, MXT_OBJECT_SIZE,
> > -                                object_buf);
> > +     return mxt_read_reg(client, reg, MXT_OBJECT_SIZE, object_buf);
> >  }
> >
> >  static struct mxt_object *
> > @@ -481,8 +476,8 @@ static int mxt_read_message(struct mxt_data *data,
> >               return -EINVAL;
> >
> >       reg = object->start_address;
> > -     return __mxt_read_reg(data->client, reg,
> > -                     sizeof(struct mxt_message), message);
> > +     return mxt_read_reg(data->client, reg, sizeof(struct mxt_message),
> > +                         message);
> >  }
> >
> >  static int mxt_read_object(struct mxt_data *data,
> > @@ -496,7 +491,7 @@ static int mxt_read_object(struct mxt_data *data,
> >               return -EINVAL;
> >
> >       reg = object->start_address;
> > -     return __mxt_read_reg(data->client, reg + offset, 1, val);
> > +     return mxt_read_reg(data->client, reg + offset, 1, val);
> >  }
> >
> >  static int mxt_write_object(struct mxt_data *data,
> > @@ -510,7 +505,7 @@ static int mxt_write_object(struct mxt_data *data,
> >               return -EINVAL;
> >
> >       reg = object->start_address;
> > -     return mxt_write_reg(data->client, reg + offset, val);
> > +     return mxt_write_reg(data->client, reg + offset, 1, &val);
> >  }
> >
> >  static void mxt_input_report(struct mxt_data *data, int single_id)
> > @@ -757,27 +752,27 @@ static int mxt_get_info(struct mxt_data *data)
> >       int error;
> >       u8 val;
> >
> > -     error = mxt_read_reg(client, MXT_FAMILY_ID, &val);
> > +     error = mxt_read_reg(client, MXT_FAMILY_ID, 1, &val);
> >       if (error)
> >               return error;
> >       info->family_id = val;
> >
> > -     error = mxt_read_reg(client, MXT_VARIANT_ID, &val);
> > +     error = mxt_read_reg(client, MXT_VARIANT_ID, 1, &val);
> >       if (error)
> >               return error;
> >       info->variant_id = val;
> >
> > -     error = mxt_read_reg(client, MXT_VERSION, &val);
> > +     error = mxt_read_reg(client, MXT_VERSION, 1, &val);
> >       if (error)
> >               return error;
> >       info->version = val;
> >
> > -     error = mxt_read_reg(client, MXT_BUILD, &val);
> > +     error = mxt_read_reg(client, MXT_BUILD, 1, &val);
> >       if (error)
> >               return error;
> >       info->build = val;
> >
> > -     error = mxt_read_reg(client, MXT_OBJECT_NUM, &val);
> > +     error = mxt_read_reg(client, MXT_OBJECT_NUM, 1, &val);
> >       if (error)
> >               return error;
> >       info->object_num = val;
> > @@ -860,12 +855,12 @@ static int mxt_initialize(struct mxt_data *data)
> >       msleep(MXT_RESET_TIME);
> >
> >       /* Update matrix size at info struct */
> > -     error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, &val);
> > +     error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, 1, &val);
> >       if (error)
> >               return error;
> >       info->matrix_xsize = val;
> >
> > -     error = mxt_read_reg(client, MXT_MATRIX_Y_SIZE, &val);
> > +     error = mxt_read_reg(client, MXT_MATRIX_Y_SIZE, 1, &val);
> >       if (error)
> >               return error;
> >       info->matrix_ysize = val;
> > --
> > 1.7.7.3
> >
>
> Thanks,
> Henrik

Re: [PATCH 03/16 v2] Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length

[Daniel Kurtz]

On Wed, Apr 11, 2012 at 5:05 PM, Henrik Rydberg <rydberg@bitmath.se> wrote:
>
> Hi Daniel,
>
> > The i2c bus requires 5 bytes to do a 1 byte read (1-byte i2c address + 2
> > byte offset + 1-byte i2c address + 1 byte data), or 4 bytes to do a
> > 1-byte write (1 byte i2c address + 2 byte offset + 1 byte data).
> >
> > By taking a length with reads and writes, the driver can amortize
> > transaction overhead by performing larger transactions where
> > appropriate.
> >
> > This patch just sets up the new API.  Later patches refactor
> > reads/writes
> > to take advantage of the larger transactions.
> >
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c |   47
> > +++++++++++++----------------
> >  1 files changed, 21 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
> > b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 15ae6fd..971f9dc 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -397,8 +397,7 @@ static int mxt_fw_write(struct i2c_client *client,
> >       return 0;
> >  }
> >
> > -static int __mxt_read_reg(struct i2c_client *client,
> > -                            u16 reg, u16 len, void *val)
> > +static int mxt_read_reg(struct i2c_client *client, u16 reg, u16 len,
> > void *val)
> >  {
> >       struct i2c_msg xfer[2];
> >       u8 buf[2];
> > @@ -419,28 +418,25 @@ static int __mxt_read_reg(struct i2c_client
> > *client,
> >       xfer[1].buf = val;
> >
> >       if (i2c_transfer(client->adapter, xfer, 2) != 2) {
> > -             dev_err(&client->dev, "%s: i2c transfer failed\n",
> > __func__);
> > +             dev_err(&client->dev, "%s: i2c read failed\n", __func__);
> >               return -EIO;
> >       }
> >
> >       return 0;
> >  }
> >
> > -static int mxt_read_reg(struct i2c_client *client, u16 reg, u8 *val)
> > +static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
> > +                      const void *val)
> >  {
> > -     return __mxt_read_reg(client, reg, 1, val);
> > -}
> > -
> > -static int mxt_write_reg(struct i2c_client *client, u16 reg, u8 val)
> > -{
> > -     u8 buf[3];
> > +     size_t count = 2 + len;         /* + 2-byte offset */
> > +     u8 buf[count];
>
> How much stack space are you planning to use here?

I missed this one when removing the other stack allocated array since
they are (rightly) frowned upon in the kernel.  I will replace with a
malloc, unless you have a better suggestion?

>
> >
> >       buf[0] = reg & 0xff;
> >       buf[1] = (reg >> 8) & 0xff;
> > -     buf[2] = val;
> > +     memcpy(&buf[2], val, len);
> >
> > -     if (i2c_master_send(client, buf, 3) != 3) {
> > -             dev_err(&client->dev, "%s: i2c send failed\n", __func__);
> > +     if (i2c_master_send(client, buf, count) != count) {
> > +             dev_err(&client->dev, "%s: i2c write failed\n", __func__);
> >               return -EIO;
> >       }
> >
> > @@ -450,8 +446,7 @@ static int mxt_write_reg(struct i2c_client *client,
> > u16 reg, u8 val)
> >  static int mxt_read_object_table(struct i2c_client *client,
> >                                     u16 reg, u8 *object_buf)
> >  {
> > -     return __mxt_read_reg(client, reg, MXT_OBJECT_SIZE,
> > -                                object_buf);
> > +     return mxt_read_reg(client, reg, MXT_OBJECT_SIZE, object_buf);
> >  }
> >
> >  static struct mxt_object *
> > @@ -481,8 +476,8 @@ static int mxt_read_message(struct mxt_data *data,
> >               return -EINVAL;
> >
> >       reg = object->start_address;
> > -     return __mxt_read_reg(data->client, reg,
> > -                     sizeof(struct mxt_message), message);
> > +     return mxt_read_reg(data->client, reg, sizeof(struct mxt_message),
> > +                         message);
> >  }
> >
> >  static int mxt_read_object(struct mxt_data *data,
> > @@ -496,7 +491,7 @@ static int mxt_read_object(struct mxt_data *data,
> >               return -EINVAL;
> >
> >       reg = object->start_address;
> > -     return __mxt_read_reg(data->client, reg + offset, 1, val);
> > +     return mxt_read_reg(data->client, reg + offset, 1, val);
> >  }
> >
> >  static int mxt_write_object(struct mxt_data *data,
> > @@ -510,7 +505,7 @@ static int mxt_write_object(struct mxt_data *data,
> >               return -EINVAL;
> >
> >       reg = object->start_address;
> > -     return mxt_write_reg(data->client, reg + offset, val);
> > +     return mxt_write_reg(data->client, reg + offset, 1, &val);
> >  }
> >
> >  static void mxt_input_report(struct mxt_data *data, int single_id)
> > @@ -757,27 +752,27 @@ static int mxt_get_info(struct mxt_data *data)
> >       int error;
> >       u8 val;
> >
> > -     error = mxt_read_reg(client, MXT_FAMILY_ID, &val);
> > +     error = mxt_read_reg(client, MXT_FAMILY_ID, 1, &val);
> >       if (error)
> >               return error;
> >       info->family_id = val;
> >
> > -     error = mxt_read_reg(client, MXT_VARIANT_ID, &val);
> > +     error = mxt_read_reg(client, MXT_VARIANT_ID, 1, &val);
> >       if (error)
> >               return error;
> >       info->variant_id = val;
> >
> > -     error = mxt_read_reg(client, MXT_VERSION, &val);
> > +     error = mxt_read_reg(client, MXT_VERSION, 1, &val);
> >       if (error)
> >               return error;
> >       info->version = val;
> >
> > -     error = mxt_read_reg(client, MXT_BUILD, &val);
> > +     error = mxt_read_reg(client, MXT_BUILD, 1, &val);
> >       if (error)
> >               return error;
> >       info->build = val;
> >
> > -     error = mxt_read_reg(client, MXT_OBJECT_NUM, &val);
> > +     error = mxt_read_reg(client, MXT_OBJECT_NUM, 1, &val);
> >       if (error)
> >               return error;
> >       info->object_num = val;
> > @@ -860,12 +855,12 @@ static int mxt_initialize(struct mxt_data *data)
> >       msleep(MXT_RESET_TIME);
> >
> >       /* Update matrix size at info struct */
> > -     error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, &val);
> > +     error = mxt_read_reg(client, MXT_MATRIX_X_SIZE, 1, &val);
> >       if (error)
> >               return error;
> >       info->matrix_xsize = val;
> >
> > -     error = mxt_read_reg(client, MXT_MATRIX_Y_SIZE, &val);
> > +     error = mxt_read_reg(client, MXT_MATRIX_Y_SIZE, 1, &val);
> >       if (error)
> >               return error;
> >       info->matrix_ysize = val;
> > --
> > 1.7.7.3
> >
>
> Thanks,
> Henrik
--
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