[PATCH 00/14 v3] cleanup atmel_mxt_ts

[PATCH 00/14 v3] cleanup atmel_mxt_ts

[Daniel Kurtz]

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

1) sysfs
   a) fw_update only by root
   b) use sncprintf()
2) faster initialization
   a) read/write sets of registers using i2c block transactions
   b) fetch object descriptor table as a single i2c transaction
   c) fetch objects  as a set of i2c reads, one per object
   d) 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

v3:
  * Address Henrik's feedback:
    1) removed some more stack allocations
    2) use sncprintf() in sysfs handler

The patches were tested using an MXT224E.
They should apply cleanly to input/next.

Daniel Kurtz (14):
  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 - 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 - 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 |  530 ++++++++++++------------------
 1 files changed, 218 insertions(+), 312 deletions(-)

-- 
1.7.7.3

[PATCH 01/14 v3] 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/14 v3] 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/14 v3] 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.

These functions are also now return any errors reported by the i2c layer.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 15ae6fd..517bb96 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -397,11 +397,11 @@ 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];
+	int ret;
 
 	buf[0] = reg & 0xff;
 	buf[1] = (reg >> 8) & 0xff;
@@ -418,40 +418,50 @@ static int __mxt_read_reg(struct i2c_client *client,
 	xfer[1].len = len;
 	xfer[1].buf = val;
 
-	if (i2c_transfer(client->adapter, xfer, 2) != 2) {
-		dev_err(&client->dev, "%s: i2c transfer failed\n", __func__);
-		return -EIO;
+	ret = i2c_transfer(client->adapter, xfer, 2);
+	if (ret != 2) {
+		dev_err(&client->dev, "i2c read reg failed (%d)\n", ret);
+		if (ret >= 0)
+			ret = -EIO;
 	}
 
-	return 0;
+	return (ret == 2) ? 0 : ret;
 }
 
-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);
-}
+	size_t count = 2 + len;		/* + 2-byte offset */
+	u8 *buf;
+	int ret;
 
-static int mxt_write_reg(struct i2c_client *client, u16 reg, u8 val)
-{
-	u8 buf[3];
+	buf = kmalloc(count, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 
 	buf[0] = reg & 0xff;
 	buf[1] = (reg >> 8) & 0xff;
-	buf[2] = val;
-
-	if (i2c_master_send(client, buf, 3) != 3) {
-		dev_err(&client->dev, "%s: i2c send failed\n", __func__);
-		return -EIO;
+	memcpy(&buf[2], val, len);
+
+	/*
+	 * i2c_master_send returns number of bytes written,
+	 * but we return 0 on success, iff all bytes were written.
+	 */
+	ret = i2c_master_send(client, buf, count);
+	if (ret != count) {
+		dev_err(&client->dev, "i2c write reg failed (%d)\n", ret);
+		if (ret >= 0)
+			ret = -EIO;
 	}
 
-	return 0;
+	kfree(buf);
+	return (ret == count) ? 0 : ret;
 }
 
 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 +491,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 +506,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 +520,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 +767,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 +870,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

Re: [PATCH 03/14 v3] 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.
> 
> These functions are also now return any errors reported by the i2c layer.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---

In the cyttsp a dedicate tx buffer is used, which is a bit easier on
the eyes. OTOH, the write operation seems infrequent enough to not
matter much, so

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

Thanks for your patience,
Henrik

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

[Dmitry Torokhov]

Hi Henrik,

On Tue, Apr 24, 2012 at 01:23:19PM +0200, Henrik Rydberg 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.
> > 
> > These functions are also now return any errors reported by the i2c layer.
> > 
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > ---
> 
> In the cyttsp a dedicate tx buffer is used, which is a bit easier on
> the eyes.

Cyttsp needs a cacheline aligned buffer because it can be wired over
SPI, that is why it has a dedicate buffer.

-- 
Dmitry

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

[Dmitry Torokhov]

Hi Daniel,

On Wed, Apr 18, 2012 at 09:21:48PM +0800, Daniel Kurtz wrote:
> +	ret = i2c_transfer(client->adapter, xfer, 2);
> +	if (ret != 2) {
> +		dev_err(&client->dev, "i2c read reg failed (%d)\n", ret);
> +		if (ret >= 0)
> +			ret = -EIO;
>  	}
>  
> -	return 0;
> +	return (ret == 2) ? 0 : ret;
>  }

Would prefer:

	ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
	if (ret != ARRAY_SIZE(xfer)) {
		if (ret >= 0)
			ret = -EIO;
		dev_err(&client->dev, "i2c read reg failed (%d)\n", ret);
		return ret;
	}

	return 0;


Or maybe we need i2c_transfer_exact() wrapper? Jean?

Thanks.

-- 
Dmitry

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

[Jean Delvare]

Hi Dmirty,

On Tue, 8 May 2012 22:54:11 -0700, Dmitry Torokhov wrote:
> Hi Daniel,
> 
> On Wed, Apr 18, 2012 at 09:21:48PM +0800, Daniel Kurtz wrote:
> > +	ret = i2c_transfer(client->adapter, xfer, 2);
> > +	if (ret != 2) {
> > +		dev_err(&client->dev, "i2c read reg failed (%d)\n", ret);
> > +		if (ret >= 0)
> > +			ret = -EIO;
> >  	}
> >  
> > -	return 0;
> > +	return (ret == 2) ? 0 : ret;
> >  }
> 
> Would prefer:
> 
> 	ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> 	if (ret != ARRAY_SIZE(xfer)) {
> 		if (ret >= 0)
> 			ret = -EIO;
> 		dev_err(&client->dev, "i2c read reg failed (%d)\n", ret);
> 		return ret;
> 	}
> 
> 	return 0;
> 
> 
> Or maybe we need i2c_transfer_exact() wrapper? Jean?

What would this function do? Return 0 on success instead of the number
of transferred messages? And -EIO on partial transfers?

That would make sense. Partial success is a rare case anyway, and
recovery possibilities are even rarer. I think most drivers don't
bother and either retry the whole transaction or fail right away. In
fact the number of messages successfully transferred is more useful to
diagnose problems during driver development or debugging.

Feel free to write and send a patch introducing i2c_transfer_exact().
It would make sense to have it log a debug message on partial success,
as it is a rare case. Bonus points if you also sent one or more patches
converting (some of) the existing callers to make use of the new
function.

-- 
Jean Delvare

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

[Dmitry Torokhov]

On Wed, May 09, 2012 at 09:05:00AM +0200, Jean Delvare wrote:
> Hi Dmirty,
> 
> On Tue, 8 May 2012 22:54:11 -0700, Dmitry Torokhov wrote:
> > Hi Daniel,
> > 
> > On Wed, Apr 18, 2012 at 09:21:48PM +0800, Daniel Kurtz wrote:
> > > +	ret = i2c_transfer(client->adapter, xfer, 2);
> > > +	if (ret != 2) {
> > > +		dev_err(&client->dev, "i2c read reg failed (%d)\n", ret);
> > > +		if (ret >= 0)
> > > +			ret = -EIO;
> > >  	}
> > >  
> > > -	return 0;
> > > +	return (ret == 2) ? 0 : ret;
> > >  }
> > 
> > Would prefer:
> > 
> > 	ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> > 	if (ret != ARRAY_SIZE(xfer)) {
> > 		if (ret >= 0)
> > 			ret = -EIO;
> > 		dev_err(&client->dev, "i2c read reg failed (%d)\n", ret);
> > 		return ret;
> > 	}
> > 
> > 	return 0;
> > 
> > 
> > Or maybe we need i2c_transfer_exact() wrapper? Jean?
> 
> What would this function do? Return 0 on success instead of the number
> of transferred messages? And -EIO on partial transfers?

Yes, exactly.

> 
> That would make sense. Partial success is a rare case anyway, and
> recovery possibilities are even rarer. I think most drivers don't
> bother and either retry the whole transaction or fail right away. In
> fact the number of messages successfully transferred is more useful to
> diagnose problems during driver development or debugging.
> 
> Feel free to write and send a patch introducing i2c_transfer_exact().
> It would make sense to have it log a debug message on partial success,
> as it is a rare case. Bonus points if you also sent one or more patches
> converting (some of) the existing callers to make use of the new
> function.

OK, thanks.

-- 
Dmitry

[PATCH 04/14 v3] 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 517bb96..1d0639d 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -516,7 +516,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 + 1)
 		return -EINVAL;
 
 	reg = object->start_address;
-- 
1.7.7.3

[PATCH 05/14 v3] 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 1d0639d..26dee8b 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 06/14 v3] 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 26dee8b..21957d0 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 07/14 v3] Input: atmel_mxt_ts - refactor mxt_object_show

[Daniel Kurtz]

don't read T5, since this is the message buffer.

 * read each object in a single i2c transaction instead of byte-by-byte
 * use scnprintf() like all well-behaved sysfs entries
 * don't read T5, the message processor object.  Reading it will
   only have two outcomes, neither of which is particularly useful:
   1) the message count decrements, and a valid message will be lost
   2) an invalid message will be read (reportid == 0xff)
 * read all instances of each object
 * conserve limited (PAGE_SIZE) sysfs output buffer space by only
   showing readable objects and not printing the object's index, which
   is not useful to userspace

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 21957d0..03b292f 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:
@@ -489,20 +488,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)
 {
@@ -902,50 +887,46 @@ 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;
+	size_t i, j, k;
+	int error = 0;
+	u8 *obuf = NULL;
+	char *buf_end = &buf[PAGE_SIZE];
 
-	for (i = 0; i < data->info.object_num; i++) {
-		object = data->object_table + i;
+	for (i = 0; i < data->info.object_num && buf_end - buf > 1; i++) {
+		struct mxt_object *object = &data->object_table[i];
+		size_t size;
 
-		count += snprintf(buf + count, PAGE_SIZE - count,
-				"Object[%d] (Type %d)\n",
-				i + 1, object->type);
-		if (count >= PAGE_SIZE)
-			return PAGE_SIZE - 1;
-
-		if (!mxt_object_readable(object->type)) {
-			count += snprintf(buf + count, PAGE_SIZE - count,
-					"\n");
-			if (count >= PAGE_SIZE)
-				return PAGE_SIZE - 1;
+		if (!mxt_object_readable(object->type))
 			continue;
-		}
 
-		for (j = 0; j < object->size + 1; j++) {
-			error = mxt_read_object(data,
-						object->type, j, &val);
-			if (error)
-				return error;
+		buf += scnprintf(buf, buf_end - buf, "\nType %u\n",
+				 object->type);
 
-			count += snprintf(buf + count, PAGE_SIZE - count,
-					"\t[%2d]: %02x (%d)\n", j, val, val);
-			if (count >= PAGE_SIZE)
-				return PAGE_SIZE - 1;
-		}
+		size = (object->size + 1) * (object->instances + 1);
+		obuf = krealloc(obuf, size, GFP_KERNEL);
+		error = mxt_read_reg(data->client, object->start_address, size,
+				     obuf);
 
-		count += snprintf(buf + count, PAGE_SIZE - count, "\n");
-		if (count >= PAGE_SIZE)
-			return PAGE_SIZE - 1;
+		if (error)
+			break;
+
+		for (j = 0; j < object->instances + 1; j++) {
+			if (object->instances > 0)
+				buf += scnprintf(buf, buf_end - buf,
+						 "Instance %zu\n", j);
+			for (k = 0; k < object->size + 1; k++)
+				buf += scnprintf(buf, buf_end - buf,
+						 "\t[%2zx] %02x (%d)\n",
+						 k, obuf[k], obuf[k]);
+		}
 	}
 
-	return count;
+	kfree(obuf);
+	return error ?: count;
 }
 
 static int mxt_load_fw(struct device *dev, const char *fn)
-- 
1.7.7.3

[PATCH 08/14 v3] 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 |   31 ++++++++++++++---------------
 1 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 03b292f..1da814b 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -636,34 +636,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;
+	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;
 
-		for (j = 0;
-		     j < (object->size + 1) * (object->instances + 1);
-		     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]);
+		config_size = (object->size + 1) * (object->instances + 1);
+		if (offset + config_size > pdata->config_length) {
+			dev_err(dev, "Not enough config data!\n");
+			return -EINVAL;
 		}
-		index += (object->size + 1) * (object->instances + 1);
+
+		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 09/14 v3] 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 1da814b..13ecbb4 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
@@ -738,41 +739,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;
@@ -808,11 +774,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;
 
@@ -847,26 +814,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

Re: [PATCH 09/14 v3] Input: atmel_mxt_ts - refactor get info

[Nick Dyer]

Hi Daniel-

Daniel Kurtz wrote:
> -	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);

The canonical format for the Atmel firmware versions has the build number
in hex, eg. 2.0.AA

So it should be %d.%d.%02X

cheers

Nick

Re: [PATCH 09/14 v3] Input: atmel_mxt_ts - refactor get info

[Daniel Kurtz]

On Sun, May 6, 2012 at 1:41 AM, Nick Dyer <nick.dyer@itdev.co.uk> wrote:
> Hi Daniel-
>
> Daniel Kurtz wrote:
>> -     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);
>
> The canonical format for the Atmel firmware versions has the build number
> in hex, eg. 2.0.AA
>
> So it should be %d.%d.%02X

Nick,
Thanks!  Will update.  Or perhaps %u.%u.%02X ?
-Daniel

>
> cheers
>
> Nick

[PATCH 10/14 v3] 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 13ecbb4..bac20ec 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;
@@ -503,97 +495,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 11/14 v3] 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.

In the process, refactor reading the object descriptor table.
First,  since the object_table entries are now exactly the same layout
in device memory and in the driver, allocate an appropriately sized
array and fetch the entire table directly into it in a single i2c
transaction.  Since a 6 byte table object requires 10 bytes to read,
doing this dramatically reduces overhead.

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 |  129 +++++++++++++++---------------
 1 files changed, 63 insertions(+), 66 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index bac20ec..77ea917 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;
@@ -227,12 +225,9 @@ struct mxt_info {
 struct mxt_object {
 	u8 type;
 	u16 start_address;
-	u8 size;
-	u8 instances;
+	u8 size;		/* Size of each instance - 1 */
+	u8 instances;		/* Number of instances - 1 */
 	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)
@@ -444,14 +443,7 @@ static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
 	return (ret == count) ? 0 : ret;
 }
 
-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;
@@ -495,8 +487,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;
@@ -505,6 +496,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);
@@ -549,12 +543,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)) {
@@ -562,22 +551,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;
@@ -690,30 +670,57 @@ 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 reportid;
+	size_t table_size;
+
+	/* Initialized cached object fields to 0 */
+	data->T9_reportid_min = 0;
+	data->T9_reportid_max = 0;
+
+	/* Free old object table, if there was one. */
+	table_size = data->info.object_num * sizeof(struct mxt_object);
+	data->object_table = krealloc(data->object_table, table_size,
+				      GFP_KERNEL);
+	if (!data->object_table) {
+		dev_err(dev, "Failed to allocate object table\n");
+		return -ENOMEM;
+	}
 
+	error = mxt_read_reg(client, MXT_OBJECT_START, table_size,
+			     data->object_table);
+	if (error) {
+		kfree(data->object_table);
+		data->object_table = NULL;
+		return error;
+	}
+
+	/* 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;
+		struct mxt_object *object = &data->object_table[i];
+		u8 num_ids, min_id, max_id;
 
-		reg = MXT_OBJECT_START + MXT_OBJECT_SIZE * i;
-		error = mxt_read_object_table(data->client, reg, buf);
-		if (error)
-			return error;
+		le16_to_cpus(&object->start_address);
+
+		num_ids = object->num_report_ids * (object->instances + 1);
+		min_id = num_ids ? reportid : 0;
+		max_id = num_ids ? reportid + num_ids - 1 : 0;
+		reportid += num_ids;
 
-		object->type = buf[0];
-		object->start_address = (buf[2] << 8) | buf[1];
-		object->size = buf[3];
-		object->instances = buf[4];
-		object->num_report_ids = buf[5];
+		dev_info(dev,
+			 "Type %2d Start %3d Size %3d Instances %2d ReportIDs %3u : %3u\n",
+			 object->type, object->start_address, object->size + 1,
+			 object->instances + 1, min_id, max_id);
 
-		if (object->num_report_ids) {
-			reportid += object->num_report_ids *
-					(object->instances + 1);
-			object->max_reportid = reportid;
+		switch (object->type) {
+		case MXT_TOUCH_MULTI_T9:
+			data->T9_reportid_min = min_id;
+			data->T9_reportid_max = max_id;
+			break;
 		}
 	}
 
@@ -732,14 +739,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)
@@ -926,9 +925,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);
 	}
 
@@ -1029,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 12/14 v3] 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 77ea917..75e0108 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -497,6 +497,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;
 
@@ -511,8 +512,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' : '.',
@@ -522,7 +527,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,
@@ -533,6 +538,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 13/14 v3] 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 75e0108..6077b7c 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -540,9 +540,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)
@@ -550,7 +547,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");
@@ -560,11 +559,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 14/14 v3] 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 |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 6077b7c..0590312 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;
 };
@@ -560,6 +561,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);
 		}
@@ -689,6 +696,7 @@ static int mxt_get_object_table(struct mxt_data *data)
 	size_t table_size;
 
 	/* Initialized cached object fields to 0 */
+	data->T6_reportid = 0;
 	data->T9_reportid_min = 0;
 	data->T9_reportid_max = 0;
 
@@ -728,6 +736,9 @@ static int mxt_get_object_table(struct mxt_data *data)
 			 object->instances + 1, 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/14 v3] cleanup atmel_mxt_ts

[Daniel Kurtz]

On Wed, Apr 18, 2012 at 9:21 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>
> This patchset cleans up the atmel_mxt_ts touchscreen driver.
> In particular, v3 implements the following:
>
> 1) sysfs
>   a) fw_update only by root
>   b) use sncprintf()
> 2) faster initialization
>   a) read/write sets of registers using i2c block transactions
>   b) fetch object descriptor table as a single i2c transaction
>   c) fetch objects  as a set of i2c reads, one per object
>   d) 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
>
> v3:
>  * Address Henrik's feedback:
>    1) removed some more stack allocations
>    2) use sncprintf() in sysfs handler
>
>
> The patches were tested using an MXT224E.
> They should apply cleanly to input/next.
>
> Daniel Kurtz (14):
>  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 - 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 - 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


Thank you to Henrik for reviewing again, and ACK'ing patch 3.

Could I get a review for the rest of the set?
There will actually be quite a few more patches that follow these.

-Daniel

Re: [PATCH 00/14 v3] cleanup atmel_mxt_ts

[Henrik Rydberg]

Hi Daniel,

> Thank you to Henrik for reviewing again, and ACK'ing patch 3.

Reading it again, I do have some more comments, actually.

> Could I get a review for the rest of the set?
> There will actually be quite a few more patches that follow these.

I think that is part of the problem.  What you want to achieve is all
good, but something else is not quite right.  Reading through these
patches felt like a lot of work, although it should not really be that
much. A closer look suggests the patches are on average 20% too large,
the rest being irrelevant changes. That may look small, but apparently
it is off-putting enough. The less work it is to accept your patches,
the more likely they are to be processed quickly.

Please find brief notes below.

> > Daniel Kurtz (14):
> >  Input: atmel_mxt_ts - use CONFIG_PM_SLEEP

Seems to clash with current mainline.

> >  Input: atmel_mxt_ts - only allow root to update firmware

OK.

> >  Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length

The return value change should be split out in a separate patch,
subject to stable as well. Also, there is no real benefit in changing
the name from __mxt to mxt. It only makes the patch longer.

> >  Input: atmel_mxt_ts - verify object size in mxt_write_object

OK, also stable material.

> >  Input: atmel_mxt_ts - do not read extra (checksum) byte

OK.

> >  Input: atmel_mxt_ts - dump each message on just 1 line

OK.

> >  Input: atmel_mxt_ts - refactor mxt_object_show

Start of for loop does not need to change. The buf_end - buf is less
clear than the existing PAGE_SIZE - count.  The realloc feels clunky,
could it not allocate the max size once instead?

> >  Input: atmel_mxt_ts - optimize writing of object table entries

Seems the index variable could be kept, no real need to move the bject
deklaration around, small things like that.

> >  Input: atmel_mxt_ts - refactor get info

Why not keep mxt_get_info(), just using the smaller implementation?
Why change the formatting of the debug messages?

> >  Input: atmel_mxt_ts - simplify event reporting

Why change formatting of function, why reformat status initialization,
why new name for pressure, why change the shift functions, why change
the debug message.

> >  Input: atmel_mxt_ts - cache T9 reportid range when reading object
> >    table

Why change touchevent() function name and arguments, why not reuse the
reportid variables. Why reformat the object assignment. Aren't
T9_reportid values zero already.

> >  Input: atmel_mxt_ts - parse vector field of data packets

These could be deferred until they are actually used.

> >  Input: atmel_mxt_ts - send all MT-B slots in one input report

OK.

> >  Input: atmel_mxt_ts - parse T6 reports

Aren't T6_reportid values zero already.

Hope this helps.

Thanks.
Henrik

Re: [PATCH 00/14 v3] cleanup atmel_mxt_ts

[Daniel Kurtz]

On Sat, May 5, 2012 at 8:16 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Daniel,
>
>> Thank you to Henrik for reviewing again, and ACK'ing patch 3.
>
> Reading it again, I do have some more comments, actually.
>
>> Could I get a review for the rest of the set?
>> There will actually be quite a few more patches that follow these.
>
> I think that is part of the problem.  What you want to achieve is all
> good, but something else is not quite right.  Reading through these
> patches felt like a lot of work, although it should not really be that
> much. A closer look suggests the patches are on average 20% too large,
> the rest being irrelevant changes. That may look small, but apparently
> it is off-putting enough. The less work it is to accept your patches,
> the more likely they are to be processed quickly.
>
> Please find brief notes below.

Hi Henrik,

Once again, thanks for all of your time and effort reviewing these patches.
I'll send a smaller friendlier set soon, but first, some brief notes
on your notes, below.

>
>> > Daniel Kurtz (14):
>> >  Input: atmel_mxt_ts - use CONFIG_PM_SLEEP
>
> Seems to clash with current mainline.

I don't understand this comment.  What is clashing with what, exactly?

>
>> >  Input: atmel_mxt_ts - only allow root to update firmware
>
> OK.
>
>> >  Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length
>
> The return value change should be split out in a separate patch,
> subject to stable as well. Also, there is no real benefit in changing
> the name from __mxt to mxt. It only makes the patch longer.
>
>> >  Input: atmel_mxt_ts - verify object size in mxt_write_object
>
> OK, also stable material.

What does "stable material" mean?

>
>> >  Input: atmel_mxt_ts - do not read extra (checksum) byte
>
> OK.
>
>> >  Input: atmel_mxt_ts - dump each message on just 1 line
>
> OK.
>
>> >  Input: atmel_mxt_ts - refactor mxt_object_show
>
> Start of for loop does not need to change. The buf_end - buf is less
> clear than the existing PAGE_SIZE - count.  The realloc feels clunky,
> could it not allocate the max size once instead?
>
>> >  Input: atmel_mxt_ts - optimize writing of object table entries
>
> Seems the index variable could be kept, no real need to move the bject
> deklaration around, small things like that.
>
>> >  Input: atmel_mxt_ts - refactor get info
>
> Why not keep mxt_get_info(), just using the smaller implementation?
> Why change the formatting of the debug messages?
>
>> >  Input: atmel_mxt_ts - simplify event reporting
>
> Why change formatting of function, why reformat status initialization,
> why new name for pressure, why change the shift functions, why change
> the debug message.
>
>> >  Input: atmel_mxt_ts - cache T9 reportid range when reading object
>> >    table
>
> Why change touchevent() function name and arguments, why not reuse the
> reportid variables. Why reformat the object assignment. Aren't
> T9_reportid values zero already.

It is true that the reportid values are zero init'ed on driver probe().
However, mxt_get_object_table() gets called both at probe, and after a
firmware update.
Thus, these values are zero'ed here to clear out the old values that
might be present from the old firmware.
In particular, the old values would linger on if, for example, there
was an error while reading the object table, or the new object table
doesn't have a T9 object.
That is too subtle, I guess, though, and the zero'ing could be moved
to the firmware update code to make it more clear why.

-Dan

>
>> >  Input: atmel_mxt_ts - parse vector field of data packets
>
> These could be deferred until they are actually used.
>
>> >  Input: atmel_mxt_ts - send all MT-B slots in one input report
>
> OK.
>
>> >  Input: atmel_mxt_ts - parse T6 reports
>
> Aren't T6_reportid values zero already.
>
> Hope this helps.
>
> 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 00/14 v3] cleanup atmel_mxt_ts

[Dmitry Torokhov]

Hi Henrik,

On Sat, May 05, 2012 at 02:16:26PM +0200, Henrik Rydberg wrote:
> 
> > >  Input: atmel_mxt_ts - verify object size in mxt_write_object
> 
> OK, also stable material.

No, I don't think we need it in stable as it is not exposed to
unprivileged userspace and we did not have reports of it causing
issues...

Daniel,

I applied patches 1, 2, 4, 5, and 6. The rest seem to need more
discussion.

Thanks.

-- 
Dmitry