[PATCH 00/20] cleanup atmel_mxt_ts

[PATCH 00/20] cleanup atmel_mxt_ts

[Daniel Kurtz]

This patchset cleans up the atmel_mxt_ts touchscreen driver.
In particular, it addresses the following issues:

1) sysfs
   a) fw_update only by root
   b) allow writing config
   c) add backupnv to save config to device flash
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)
   c) read num messages first, instead of looking for 0xFF reportid
   d) read all pending message bytes in one i2c read transaction
4) more correct MT-B support
   a) send all (changed) contacts in a single EV_SYN/SYN_REPORT

Daniel Kurtz (20):
  Input: atmel_mxt_ts - use CONFIG_PM_SLEEP
  Input: atmel_mxt_ts - only allow root to update firmware
  Input: atmel_mxt_ts - verify object size in mxt_write_object
  Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length
  Input: atmel_mxt_ts - dump mxt_read/write_reg
  Input: atmel_mxt_ts - allow writing to object sysfs entry
  Input: atmel_mxt_ts - add backupnv sysfs entry
  Input: atmel_mxt_ts - store actual size and instance
  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 - simplify event reporting
  Input: atmel_mxt_ts - parse vector field of data packets
  Input: atmel_mxt_ts - refactor reading object table
  Input: atmel_mxt_ts - optimize writing of object table entries
  Input: atmel_mxt_ts - refactor get info
  Input: atmel_mxt_ts - use cached T9 reportid range in isr
  Input: atmel_mxt_ts - read num messages, then all messages
  Input: atmel_mxt_ts - remove mxt_make_highchg and parse T6 report
  Input: atmel_mxt_ts - send all MT-B slots in one input report

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

-- 
1.7.7.3

[PATCH 01/20] 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>
---
 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 a596c27..b71de8f 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/20] 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>
---
 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 b71de8f..0d4d492 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

Re: [PATCH 02/20] Input: atmel_mxt_ts - only allow root to update firmware

[Nick Dyer]

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

Acked-by: Nick Dyer <nick.dyer@itdev.co.uk>

[PATCH 03/20] 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 0d4d492..e18c698 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -506,7 +506,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

Re: [PATCH 03/20] Input: atmel_mxt_ts - verify object size in mxt_write_object

[Joonyoung Shim]

On 03/13/2012 09:04 PM, Daniel Kurtz wrote:
> 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 0d4d492..e18c698 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -506,7 +506,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)

The object->size is actual object size - 1.

+	if (!object || offset>  object->size)


>   		return -EINVAL;
>
>   	reg = object->start_address;

Re: [PATCH 03/20] Input: atmel_mxt_ts - verify object size in mxt_write_object

[Daniel Kurtz]

On Wed, Mar 14, 2012 at 9:33 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>
> On 03/13/2012 09:04 PM, Daniel Kurtz wrote:
>>
>> 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 0d4d492..e18c698 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -506,7 +506,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)
>
>
> The object->size is actual object size - 1.
>
> +       if (!object || offset>  object->size)
>

Whoops.  Good catch.  Will move this patch after patch 08, which fixes
the object size.

>
>
>>                return -EINVAL;
>>
>>        reg = object->start_address;
>
>
--
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/20] Input: atmel_mxt_ts - verify object size in mxt_write_object

[Joonyoung Shim]

On 03/14/2012 10:33 AM, Joonyoung Shim wrote:
> On 03/13/2012 09:04 PM, Daniel Kurtz wrote:
>> 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 0d4d492..e18c698 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -506,7 +506,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)
>
> The object->size is actual object size - 1.
>
> +    if (!object || offset>  object->size)
>

OK. another patch covers this.

>
>>           return -EINVAL;
>>
>>       reg = object->start_address;
>

[PATCH 04/20] 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>
---
 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 e18c698..defd4e1 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 05/20] Input: atmel_mxt_ts - dump mxt_read/write_reg

[Daniel Kurtz]

For verbose on-the-wire debugging.
Prints DUMP_LEN bytes (in hex) per line.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index defd4e1..dd3919a 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -397,6 +397,33 @@ static int mxt_fw_write(struct i2c_client *client,
 	return 0;
 }
 
+#ifdef DEBUG
+#define DUMP_LEN	16
+static void mxt_dump_xfer(struct device *dev, const char *func, u16 reg,
+			  u16 len, const u8 *val)
+{
+	/* Rough guess for string size */
+	char str[DUMP_LEN * 3 + 2];
+	int i;
+	size_t n;
+
+	for (i = 0, n = 0; i < len; i++) {
+		n += snprintf(&str[n], sizeof(str) - n, "%02x ", val[i]);
+		if ((i + 1) % DUMP_LEN == 0 || (i + 1) == len) {
+			dev_dbg(dev,
+				"%s(reg: %d len: %d offset: 0x%02x): %s\n",
+				func, reg, len, (i / DUMP_LEN) * DUMP_LEN,
+				str);
+			n = 0;
+		}
+	}
+}
+#undef DUMP_LEN
+#else
+static void mxt_dump_xfer(struct device *dev, const char *func, u16 reg,
+			  u16 len, const u8 *val) { }
+#endif
+
 static int mxt_read_reg(struct i2c_client *client, u16 reg, u16 len, void *val)
 {
 	struct i2c_msg xfer[2];
@@ -422,6 +449,8 @@ static int mxt_read_reg(struct i2c_client *client, u16 reg, u16 len, void *val)
 		return -EIO;
 	}
 
+	mxt_dump_xfer(&client->dev, __func__, reg, len, val);
+
 	return 0;
 }
 
@@ -435,6 +464,8 @@ static int mxt_write_reg(struct i2c_client *client, u16 reg, u16 len,
 	buf[1] = (reg >> 8) & 0xff;
 	memcpy(&buf[2], val, len);
 
+	mxt_dump_xfer(&client->dev, __func__, reg, len, val);
+
 	if (i2c_master_send(client, buf, count) != count) {
 		dev_err(&client->dev, "%s: i2c write failed\n", __func__);
 		return -EIO;
-- 
1.7.7.3

Re: [PATCH 05/20] Input: atmel_mxt_ts - dump mxt_read/write_reg

[Nick Dyer]

Daniel Kurtz wrote:
> For verbose on-the-wire debugging.
> Prints DUMP_LEN bytes (in hex) per line.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

I don't think this belongs in the driver. If you want low level i2c
transfer dumps, it should go in i2c subsystem.

Plus, there is the print_hex_dump function for hex dumps.

cheers

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 06/20] Input: atmel_mxt_ts - allow writing to object sysfs entry

[Daniel Kurtz]

Userspace can write a 24-bit value (encoded as a 6 character hex string)
to the 'object' sysfs entry to modify a single byte of the object table.
The hex string encodes a 3 bytes, in the following format:
 TTFFVV

Where:
 TT = object type
 FF = object offset
 VV = byte value

The object table is modified in device ram, which means the change is
volatile, and will be overwritten on the next device reset.  To make
changes permanent, the new settings should be persisted in the device's
Non-Voltatile Memory using the updatenv sysfs entry.

Also, since the device driver initializes itself by reading some values
from the object table, the entire driver may need to be unloaded and
reloaded after writing the values for the driver to stay in sync.  Whether
this is required depends on exactly which values were updated.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index dd3919a..ac3dbb7 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -970,6 +970,30 @@ static ssize_t mxt_object_show(struct device *dev,
 	return count;
 }
 
+static ssize_t mxt_object_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct mxt_data *data = dev_get_drvdata(dev);
+	int ret;
+	u32 param;
+	u8 type, offset, val;
+
+	ret = kstrtou32(buf, 16, &param);
+	if (ret < 0)
+		return -EINVAL;
+
+	type = (param & 0x00ff0000) >> 16;
+	offset = (param & 0x00ff00) >> 8;
+	val = param & 0x00ff;
+
+	ret = mxt_write_object(data, type, offset, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
 static int mxt_load_fw(struct device *dev, const char *fn)
 {
 	struct mxt_data *data = dev_get_drvdata(dev);
@@ -1075,7 +1099,8 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 	return count;
 }
 
-static DEVICE_ATTR(object, S_IRUGO, mxt_object_show, NULL);
+static DEVICE_ATTR(object, S_IRUGO | S_IWUSR, mxt_object_show,
+		   mxt_object_store);
 static DEVICE_ATTR(update_fw, S_IWUSR, NULL, mxt_update_fw_store);
 
 static struct attribute *mxt_attrs[] = {
-- 
1.7.7.3

Re: [PATCH 06/20] Input: atmel_mxt_ts - allow writing to object sysfs entry

[Henrik Rydberg]

Hi Daniel,

> Userspace can write a 24-bit value (encoded as a 6 character hex string)
> to the 'object' sysfs entry to modify a single byte of the object table.
> The hex string encodes a 3 bytes, in the following format:
>  TTFFVV
> 
> Where:
>  TT = object type
>  FF = object offset
>  VV = byte value
> 
> The object table is modified in device ram, which means the change is
> volatile, and will be overwritten on the next device reset.  To make
> changes permanent, the new settings should be persisted in the device's
> Non-Voltatile Memory using the updatenv sysfs entry.
> 
> Also, since the device driver initializes itself by reading some values
> from the object table, the entire driver may need to be unloaded and
> reloaded after writing the values for the driver to stay in sync.  Whether
> this is required depends on exactly which values were updated.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---

Perhaps you could give an example of why this is needed?

Thanks,
Henrik

Re: [PATCH 06/20] Input: atmel_mxt_ts - allow writing to object sysfs entry

[Daniel Kurtz]

On Mon, Mar 19, 2012 at 4:04 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>
> Hi Daniel,
>
> > Userspace can write a 24-bit value (encoded as a 6 character hex string)
> > to the 'object' sysfs entry to modify a single byte of the object table.
> > The hex string encodes a 3 bytes, in the following format:
> >  TTFFVV
> >
> > Where:
> >  TT = object type
> >  FF = object offset
> >  VV = byte value
> >
> > The object table is modified in device ram, which means the change is
> > volatile, and will be overwritten on the next device reset.  To make
> > changes permanent, the new settings should be persisted in the device's
> > Non-Voltatile Memory using the updatenv sysfs entry.
> >
> > Also, since the device driver initializes itself by reading some values
> > from the object table, the entire driver may need to be unloaded and
> > reloaded after writing the values for the driver to stay in sync.
> >  Whether
> > this is required depends on exactly which values were updated.
> >
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > ---
>
> Perhaps you could give an example of why this is needed?
>
> Thanks,
> Henrik

The idea is to allow a a device to be calibrated via userspace.  The
atmel devices have many registers for tuning properties of their
firmware.
This would be used, for example, to program an initial calibration on
a factory floor, or, to tune a device at runtime.  Currently, the only
way devices are configured and calibrated is for a system integrator
to embed the entire calibration in the kernel via a binary blob in
platform_data.

Thanks,
-Daniel

Re: [PATCH 06/20] Input: atmel_mxt_ts - allow writing to object sysfs entry

[Nick Dyer]

Daniel Kurtz wrote:
> Userspace can write a 24-bit value (encoded as a 6 character hex string)
> to the 'object' sysfs entry to modify a single byte of the object table.
> The hex string encodes a 3 bytes, in the following format:
>  TTFFVV
> 
> Where:
>  TT = object type
>  FF = object offset
>  VV = byte value
> 
> The object table is modified in device ram, which means the change is
> volatile, and will be overwritten on the next device reset.  To make
> changes permanent, the new settings should be persisted in the device's
> Non-Voltatile Memory using the updatenv sysfs entry.
> 
> Also, since the device driver initializes itself by reading some values
> from the object table, the entire driver may need to be unloaded and
> reloaded after writing the values for the driver to stay in sync.  Whether
> this is required depends on exactly which values were updated.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

NAK. I have several concerns about this:

1) The object sysfs entry doesn't follow the sysfs guidelines, this is
abusing it further.
2) Object type can be larger than 1 byte in the future.
3) Your interface restricts you to doing 1 byte writes. What about larger
block sizes?

In short, I think exposing the entire register space as a binary attribute
(the same way that regmap does) is a preferable solution to this
requirement, and we already have user space tools which use it.

cheers

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

Re: [PATCH 06/20] Input: atmel_mxt_ts - allow writing to object sysfs entry

[Alan Cox]

On Tue, 13 Mar 2012 20:04:09 +0800
Daniel Kurtz <djkurtz@chromium.org> wrote:

> Userspace can write a 24-bit value (encoded as a 6 character hex string)
> to the 'object' sysfs entry to modify a single byte of the object table.
> The hex string encodes a 3 bytes, in the following format:

How is this locked against other users and updates of these table data ?

I don't see how all the objects and their pointers and size are
guaranteedto always be valid in all the users you have ?

Re: [PATCH 06/20] Input: atmel_mxt_ts - allow writing to object sysfs entry

[Nick Dyer]

Alan Cox wrote:
> On Tue, 13 Mar 2012 20:04:09 +0800
> Daniel Kurtz <djkurtz@chromium.org> wrote:
>> Userspace can write a 24-bit value (encoded as a 6 character hex string)
>> to the 'object' sysfs entry to modify a single byte of the object table.
>> The hex string encodes a 3 bytes, in the following format:
> 
> How is this locked against other users and updates of these table data ?
> 
> I don't see how all the objects and their pointers and size are
> guaranteedto always be valid in all the users you have ?

The particular layout of the "objects" on the maxtouch chip only changes if
there is a firmware upgrade (or between different models of chip). It's
just a convenience to allow bits of functionality to be extended or
replaced without getting in a mess of random register in different places,
but it's not dynamic.

In terms of arbitrating between different reads/writes - the I2C bus
locking will handle most of it. To be entirely correct, I think there
should be a mutex so that this interface couldn't be used whilst the chip
is resetting, backing up or upgrading firmware.

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 07/20] Input: atmel_mxt_ts - add backupnv sysfs entry

[Daniel Kurtz]

Writing to the object sysfs entry permits individual object table entries
to be modified in the device RAM at run time.  To permanently save
the settings, they must be written to Non-Volatile memory (NVM).
This patch adds a write-only sysfs entry to allow userspace to save
current settings to NVM, but restricts access to root.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index ac3dbb7..e988dc0 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -994,6 +994,23 @@ static ssize_t mxt_object_store(struct device *dev,
 	return count;
 }
 
+static ssize_t mxt_backupnv_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct mxt_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	/* Backup non-volatile memory */
+	ret = mxt_write_object(data, MXT_GEN_COMMAND_T6, MXT_COMMAND_BACKUPNV,
+			       MXT_BACKUP_VALUE);
+	if (ret)
+		return ret;
+	msleep(MXT_BACKUP_TIME);
+
+	return count;
+}
+
 static int mxt_load_fw(struct device *dev, const char *fn)
 {
 	struct mxt_data *data = dev_get_drvdata(dev);
@@ -1099,11 +1116,13 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 	return count;
 }
 
+static DEVICE_ATTR(backupnv, S_IWUSR, NULL, mxt_backupnv_store);
 static DEVICE_ATTR(object, S_IRUGO | S_IWUSR, mxt_object_show,
 		   mxt_object_store);
 static DEVICE_ATTR(update_fw, S_IWUSR, NULL, mxt_update_fw_store);
 
 static struct attribute *mxt_attrs[] = {
+	&dev_attr_backupnv.attr,
 	&dev_attr_object.attr,
 	&dev_attr_update_fw.attr,
 	NULL
-- 
1.7.7.3

Re: [PATCH 07/20] Input: atmel_mxt_ts - add backupnv sysfs entry

[Nick Dyer]

Daniel Kurtz wrote:
> Writing to the object sysfs entry permits individual object table entries
> to be modified in the device RAM at run time.  To permanently save
> the settings, they must be written to Non-Volatile memory (NVM).
> This patch adds a write-only sysfs entry to allow userspace to save
> current settings to NVM, but restricts access to root.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

The process to update the stored chip configuration should be:

1) Change config setting
2) Backup (with delay)
3) Reset (with delay)
4) Get driver to re-read resolution etc

So I am worried that this is a half solution, and will lead to confusion.

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 08/20] 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 |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index e988dc0..c422fad 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -676,7 +676,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");
@@ -689,9 +689,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");
@@ -700,7 +699,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;
@@ -829,13 +828,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;
 		}
 	}
@@ -950,7 +948,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

Re: [PATCH 08/20] Input: atmel_mxt_ts - store actual size and instance

[Nick Dyer]

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

As per the patch I sent you which does the same thing, you will need to
change the size and instances from u8 to u16.

see
https://github.com/atmel-maxtouch/linux/commit/2643b0a753de5fd607ea6e131aacf5bd69dd8e27

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 09/20] Input: atmel_mxt_ts - do not read extra (checksum) byte

[Daniel Kurtz]

It isn't sent, nor checked anyway, so don't bother reading 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 c422fad..309761a 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

Re: [PATCH 09/20] Input: atmel_mxt_ts - do not read extra (checksum) byte

[Nick Dyer]

Daniel Kurtz wrote:
> It isn't sent, nor checked anyway, so don't bother reading it.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

This is fair enough, although IMO the checksum should be checked.

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

RE: [PATCH 09/20] Input: atmel_mxt_ts - do not read extra (checksum) byte

[Bowens, Alan]

Just to confuse things, the checksum byte is available from the chip if
required. Reading it involves a slightly different approach to normal
message processor reads, in that the host needs to write the 16-bit
address of the message processor, with the MSB set. It must then follow
these two bytes with another byte consisting of the checksum of the
previous two bytes.

This functionality was added at a customer's request, although I don't
know if any current customers actually use it.


Alan Bowens
Section Manager / Atmel Corporation
Tel: (+44) (0)1489-553-611
alan.bowens@atmel.com / www.atmel.com

The information contained in this email message may be privileged,
confidential and/or protected from unauthorized disclosure. If you are
not the intended recipient, any dissemination, distribution or copying
is strictly prohibited. Please immediately notify the sender by reply if
you received this email in error. Thank you for your cooperation. 

-----Original Message-----
From: Nick Dyer [mailto:nick.dyer@itdev.co.uk] 
Sent: 20 March 2012 15:08
To: Daniel Kurtz
Cc: Dmitry Torokhov; Joonyoung Shim; Valkonen, Iiro; Henrik Rydberg;
linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Benson Leung;
Yufeng Shen; Tiwari, Atul; Bowens, Alan
Subject: Re: [PATCH 09/20] Input: atmel_mxt_ts - do not read extra
(checksum) byte

Daniel Kurtz wrote:
> It isn't sent, nor checked anyway, so don't bother reading it.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

This is fair enough, although IMO the checksum should be checked.

--
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 10/20] 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>
---
 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 309761a..ac8c50a 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

Re: [PATCH 10/20] Input: atmel_mxt_ts - dump each message on just 1 line

[Nick Dyer]

Daniel Kurtz wrote:
> Helps ensure all bytes for a single message together in the system log.

Acked-by: Nick Dyer <nick.dyer@itdev.co.uk>

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 11/20] Input: atmel_mxt_ts - refactor mxt_object_show

[Daniel Kurtz]

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

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index ac8c50a..4363511 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -490,6 +490,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)
 {
@@ -505,20 +512,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)
 {
@@ -916,17 +909,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",
@@ -936,20 +928,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

Re: [PATCH 11/20] Input: atmel_mxt_ts - refactor mxt_object_show

[Nick Dyer]

Daniel Kurtz wrote:
> Read each object with a single i2c transaction instead of byte-by-byte.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Acked-by: Nick Dyer <nick.dyer@itdev.co.uk>

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 12/20] 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.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 4363511..fa692e5 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -194,6 +194,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)
@@ -238,14 +239,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;
@@ -253,7 +246,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;
@@ -526,97 +518,55 @@ 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);
+		/* TODO: This should really be sqrt(area) */
+		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

Re: [PATCH 12/20] Input: atmel_mxt_ts - simplify event reporting

[Henrik Rydberg]

Hi Daniel,

> 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.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---

Simplification looking good overall, together with that later
patch. Please find some comments below.

>  drivers/input/touchscreen/atmel_mxt_ts.c |  122 +++++++++---------------------
>  1 files changed, 36 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 4363511..fa692e5 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -194,6 +194,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)
> @@ -238,14 +239,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;
> @@ -253,7 +246,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;
> @@ -526,97 +518,55 @@ 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);
> +		/* TODO: This should really be sqrt(area) */
> +		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);

The functional relationship might not be perfect, but at least the
reported scale should match the position units, as several userspace
drivers depend on its accuracy. If the line size looks reasonable in
mtview, for instance, it should be fine.

> +	}
>  
> -	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)
> -- 

Thanks,
Henrik

Re: [PATCH 12/20] Input: atmel_mxt_ts - simplify event reporting

[Daniel Kurtz]

On Mon, Mar 19, 2012 at 4:26 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Daniel,
>
>> 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.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>
> Simplification looking good overall, together with that later
> patch. Please find some comments below.
>
>>  drivers/input/touchscreen/atmel_mxt_ts.c |  122 +++++++++---------------------
>>  1 files changed, 36 insertions(+), 86 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 4363511..fa692e5 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -194,6 +194,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)
>> @@ -238,14 +239,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;
>> @@ -253,7 +246,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;
>> @@ -526,97 +518,55 @@ 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);
>> +             /* TODO: This should really be sqrt(area) */
>> +             input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
>
> The functional relationship might not be perfect, but at least the
> reported scale should match the position units, as several userspace
> drivers depend on its accuracy. If the line size looks reasonable in
> mtview, for instance, it should be fine.

Please note this patch doesn't actually change how TOUCH_MAJOR is
reported.  All I did here was add the TODO, since reporting 'area' as
TOUCH_MAJOR looks suspect to me :).  Unfortunately, I don't actually
know the relationship between what the firmware sends as 'area' and
the position units...  perhaps someone with more experience with how
the firmware works could help us here.

-Dan

>
>> +     }
>>
>> -     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)
>> --
>
> 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 12/20] Input: atmel_mxt_ts - simplify event reporting

[Nick Dyer]

Daniel Kurtz wrote:
> 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.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

This seems to be a good improvement, assuming it is valid. Which platforms
have you tested it on?

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 13/20] Input: atmel_mxt_ts - parse vector field of data packets

[Daniel Kurtz]

The atmel_mxt_ts T9 data contains information orientation 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>
---
 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 fa692e5..e05540d 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -528,6 +528,7 @@ static void mxt_input_touchevent(struct mxt_data *data,
 	int y;
 	int area;
 	int amplitude;
+	int vector1, vector2;
 
 	status = message->message[0];
 	x = (message->message[1] << 4) | ((message->message[3] >> 4) & 0xf);
@@ -540,8 +541,12 @@ static void mxt_input_touchevent(struct mxt_data *data,
 	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' : '.',
@@ -551,7 +556,7 @@ static void mxt_input_touchevent(struct mxt_data *data,
 		(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,
@@ -563,6 +568,7 @@ static void mxt_input_touchevent(struct mxt_data *data,
 		input_report_abs(input_dev, ABS_MT_PRESSURE, amplitude);
 		/* TODO: This should really be sqrt(area) */
 		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

Re: [PATCH 13/20] Input: atmel_mxt_ts - parse vector field of data packets

[Nick Dyer]

Daniel Kurtz wrote:
> The atmel_mxt_ts T9 data contains information orientation 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>

Fair enough.

Acked-by: Nick Dyer <nick.dyer@itdev.co.uk>

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

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

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index e05540d..9e1ce03 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -460,14 +460,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;
@@ -755,25 +748,32 @@ static int mxt_get_info(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[data->info.object_num][MXT_OBJECT_SIZE];
 
-	for (i = 0; i < data->info.object_num; i++) {
-		struct mxt_object *object = data->object_table + i;
+	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;
+	error = mxt_read_reg(client, MXT_OBJECT_START, sizeof(buf), buf);
+	if (error)
+		return error;
+
+	for (i = 0; i < data->info.object_num; i++) {
+		struct mxt_object *object = &data->object_table[i];
 
-		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];
+		object->type = buf[i][0];
+		object->start_address = (buf[i][2] << 8) | buf[i][1];
+		object->size = buf[i][3] + 1;
+		object->instances = buf[i][4] + 1;
+		object->num_report_ids = buf[i][5];
 
 		if (object->num_report_ids) {
 			reportid += object->num_report_ids * object->instances;
@@ -795,14 +795,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)
-- 
1.7.7.3

Re: [PATCH 14/20] Input: atmel_mxt_ts - refactor reading object table

[Nick Dyer]

Daniel Kurtz wrote:
> 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.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Yes, this looks very sensible.

Acked-by: Nick Dyer <nick.dyer@itdev.co.uk>

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 15/20] 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>
---
 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 9e1ce03..0277381 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -609,33 +609,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 16/20] 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 |   63 +++++-------------------------
 1 files changed, 10 insertions(+), 53 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 0277381..1a9ed26 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -711,41 +711,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)
 {
 	struct i2c_client *client = data->client;
@@ -787,11 +752,11 @@ 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);
+	error = mxt_read_reg(client, MXT_FAMILY_ID, sizeof(*info), info);
 	if (error)
 		return error;
 
@@ -818,26 +783,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 16/20] Input: atmel_mxt_ts - refactor get info

[Nick Dyer]

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

Acked-by: Nick Dyer <nick.dyer@itdev.co.uk>

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 17/20] Input: atmel_mxt_ts - use cached T9 reportid range in isr

[Daniel Kurtz]

Streamline interrupt processing by caching the T9 reportid range when
first initializing 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.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 1a9ed26..88a474e 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -249,6 +249,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)
@@ -512,7 +516,7 @@ static int mxt_write_object(struct mxt_data *data,
 }
 
 static void mxt_input_touchevent(struct mxt_data *data,
-				 struct mxt_message *message, int id)
+				 struct mxt_message *message)
 {
 	struct device *dev = &data->client->dev;
 	struct input_dev *input_dev = data->input_dev;
@@ -522,6 +526,9 @@ static void mxt_input_touchevent(struct mxt_data *data,
 	int area;
 	int amplitude;
 	int vector1, vector2;
+	int id;
+
+	id = message->reportid - data->T9_reportid_min;
 
 	status = message->message[0];
 	x = (message->message[1] << 4) | ((message->message[3] >> 4) & 0xf);
@@ -572,12 +579,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)) {
@@ -585,22 +587,12 @@ 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);
+		if (message.reportid >= data->T9_reportid_min &&
+		    message.reportid <= data->T9_reportid_max)
+			mxt_input_touchevent(data, &message);
 		else
 			mxt_dump_message(dev, &message);
-	} while (reportid != 0xff);
+	} while (message.reportid != 0xff);
 
 end:
 	return IRQ_HANDLED;
@@ -744,6 +736,19 @@ static int mxt_get_object_table(struct mxt_data *data)
 			reportid += object->num_report_ids * object->instances;
 			object->max_reportid = reportid;
 		}
+
+		dev_info(dev, "Type %2d Start %3d Size %3d Max ReportID %3d Instances %2d\n",
+			 object->type, object->start_address, object->size,
+			 object->max_reportid, object->instances);
+
+		/* Save data for objects used when processing interrupts */
+		switch (object->type) {
+		case MXT_TOUCH_MULTI_T9:
+			data->T9_reportid_max = object->max_reportid;
+			data->T9_reportid_min = data->T9_reportid_max -
+						object->num_report_ids + 1;
+			break;
+		}
 	}
 
 	return 0;
-- 
1.7.7.3

Re: [PATCH 17/20] Input: atmel_mxt_ts - use cached T9 reportid range in isr

[Nick Dyer]

Daniel Kurtz wrote:
> Streamline interrupt processing by caching the T9 reportid range when
> first initializing 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.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

You need to support the case where there are multiple T9 instances.

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 18/20] Input: atmel_mxt_ts - read num messages, then all messages

[Daniel Kurtz]

Implement the MXT DMA method of reading messages.
On an interrupt, the T44 report always contains the number of messages
pending to be read.  So, read 1 byte from T44 in 1 i2c transaction, then
read the N pending messages in a second transaction.

The end result is a much much faster read time for all pending messages.
Using 400kHz i2c, it is possible to read 10 pending messages (e.g. for 10
moving contatcts) in less than 2.8ms, which is well less than the typical
10-15ms update rate.

Note: There is a possible optimization here.  The T44 byte is guaranteed
to always be right before the T5 address.  Thus, it should be possible
to always fetch the T44 message count and the first message in a single
transaction.  This would eliminate the overhead of a second complete read
transaction for the case where there is only a single pending message.
(This is actually the most common case, for instance with just 1-contact
on the device touch surface). This optimization, however, is not done in
this patch.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 88a474e..dafc030 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -251,8 +251,10 @@ struct mxt_data {
 	unsigned int max_y;
 
 	/* Cached parameters from object table */
+	u16 T5_address;
 	u8 T9_reportid_min;
 	u8 T9_reportid_max;
+	u16 T44_address;
 };
 
 static bool mxt_object_readable(unsigned int type)
@@ -486,21 +488,6 @@ static int mxt_read_object(struct mxt_data *data, struct mxt_object *object,
 			    val);
 }
 
-static int mxt_read_message(struct mxt_data *data,
-				 struct mxt_message *message)
-{
-	struct mxt_object *object;
-	u16 reg;
-
-	object = mxt_get_object(data, MXT_GEN_MESSAGE_T5);
-	if (!object)
-		return -EINVAL;
-
-	reg = object->start_address;
-	return mxt_read_reg(data->client, reg, sizeof(struct mxt_message),
-			    message);
-}
-
 static int mxt_write_object(struct mxt_data *data,
 				 u8 type, u8 offset, u8 val)
 {
@@ -515,6 +502,19 @@ static int mxt_write_object(struct mxt_data *data,
 	return mxt_write_reg(data->client, reg + offset, 1, &val);
 }
 
+static int mxt_read_num_messages(struct mxt_data *data, u8 *count)
+{
+	/* TODO: Optimization: read first message along with message count */
+	return mxt_read_reg(data->client, data->T44_address, 1, count);
+}
+
+static int mxt_read_messages(struct mxt_data *data, u8 count,
+			     struct mxt_message *messages)
+{
+	return mxt_read_reg(data->client, data->T5_address,
+			    sizeof(struct mxt_message) * count, messages);
+}
+
 static void mxt_input_touchevent(struct mxt_data *data,
 				 struct mxt_message *message)
 {
@@ -575,26 +575,50 @@ static void mxt_input_touchevent(struct mxt_data *data,
 	input_sync(input_dev);
 }
 
-static irqreturn_t mxt_interrupt(int irq, void *dev_id)
+static int mxt_proc_messages(struct mxt_data *data, u8 count)
 {
-	struct mxt_data *data = dev_id;
-	struct mxt_message message;
 	struct device *dev = &data->client->dev;
+	struct mxt_message messages[count], *msg;
+	int ret;
 
-	do {
-		if (mxt_read_message(data, &message)) {
-			dev_err(dev, "Failed to read message\n");
-			goto end;
-		}
+	ret = mxt_read_messages(data, count, messages);
+	if (ret) {
+		dev_err(dev, "Failed to read %u messages (%d).\n", count, ret);
+		return ret;
+	}
+
+	for (msg = messages; msg < &messages[count]; msg++) {
+		mxt_dump_message(dev, msg);
+
+		if (msg->reportid >= data->T9_reportid_min &&
+		    msg->reportid <= data->T9_reportid_max)
+			mxt_input_touchevent(data, msg);
+	}
+
+	return 0;
+}
+
+static int mxt_handle_messages(struct mxt_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+	u8 count;
 
-		if (message.reportid >= data->T9_reportid_min &&
-		    message.reportid <= data->T9_reportid_max)
-			mxt_input_touchevent(data, &message);
-		else
-			mxt_dump_message(dev, &message);
-	} while (message.reportid != 0xff);
+	ret = mxt_read_num_messages(data, &count);
+	if (ret) {
+		dev_err(dev, "Failed to read message count (%d).\n", ret);
+		return ret;
+	}
 
-end:
+	if (count > 0)
+		ret = mxt_proc_messages(data, count);
+
+	return ret;
+}
+
+static irqreturn_t mxt_interrupt(int irq, void *dev_id)
+{
+	mxt_handle_messages(dev_id);
 	return IRQ_HANDLED;
 }
 
@@ -642,7 +666,7 @@ static int mxt_make_highchg(struct mxt_data *data)
 
 	/* Read dummy message to make high CHG pin */
 	do {
-		error = mxt_read_message(data, &message);
+		error = mxt_read_messages(data, 1, &message);
 		if (error)
 			return error;
 	} while (message.reportid != 0xff && --count);
@@ -743,11 +767,17 @@ static int mxt_get_object_table(struct mxt_data *data)
 
 		/* Save data for objects used when processing interrupts */
 		switch (object->type) {
+		case MXT_GEN_MESSAGE_T5:
+			data->T5_address = object->start_address;
+			break;
 		case MXT_TOUCH_MULTI_T9:
 			data->T9_reportid_max = object->max_reportid;
 			data->T9_reportid_min = data->T9_reportid_max -
 						object->num_report_ids + 1;
 			break;
+		case MXT_SPT_MESSAGECOUNT_T44:
+			data->T44_address = object->start_address;
+			break;
 		}
 	}
 
-- 
1.7.7.3

Re: [PATCH 18/20] Input: atmel_mxt_ts - read num messages, then all messages

[Joonyoung Shim]

On 03/13/2012 09:04 PM, Daniel Kurtz wrote:
> Implement the MXT DMA method of reading messages.
> On an interrupt, the T44 report always contains the number of messages
> pending to be read.  So, read 1 byte from T44 in 1 i2c transaction, then
> read the N pending messages in a second transaction.

My mXT224 chip datasheet hasn't T44 object, is it updated?
If not, need to consider chips which doesn't have T44 object.

> The end result is a much much faster read time for all pending messages.
> Using 400kHz i2c, it is possible to read 10 pending messages (e.g. for 10
> moving contatcts) in less than 2.8ms, which is well less than the typical
> 10-15ms update rate.
>
> Note: There is a possible optimization here.  The T44 byte is guaranteed
> to always be right before the T5 address.  Thus, it should be possible
> to always fetch the T44 message count and the first message in a single
> transaction.  This would eliminate the overhead of a second complete read
> transaction for the case where there is only a single pending message.
> (This is actually the most common case, for instance with just 1-contact
> on the device touch surface). This optimization, however, is not done in
> this patch.
>
> Signed-off-by: Daniel Kurtz<djkurtz@chromium.org>
> ---
>   drivers/input/touchscreen/atmel_mxt_ts.c |   92 ++++++++++++++++++++----------
>   1 files changed, 61 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 88a474e..dafc030 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -251,8 +251,10 @@ struct mxt_data {
>   	unsigned int max_y;
>
>   	/* Cached parameters from object table */
> +	u16 T5_address;
>   	u8 T9_reportid_min;
>   	u8 T9_reportid_max;
> +	u16 T44_address;
>   };
>
>   static bool mxt_object_readable(unsigned int type)
> @@ -486,21 +488,6 @@ static int mxt_read_object(struct mxt_data *data, struct mxt_object *object,
>   			    val);
>   }
>
> -static int mxt_read_message(struct mxt_data *data,
> -				 struct mxt_message *message)
> -{
> -	struct mxt_object *object;
> -	u16 reg;
> -
> -	object = mxt_get_object(data, MXT_GEN_MESSAGE_T5);
> -	if (!object)
> -		return -EINVAL;
> -
> -	reg = object->start_address;
> -	return mxt_read_reg(data->client, reg, sizeof(struct mxt_message),
> -			    message);
> -}
> -
>   static int mxt_write_object(struct mxt_data *data,
>   				 u8 type, u8 offset, u8 val)
>   {
> @@ -515,6 +502,19 @@ static int mxt_write_object(struct mxt_data *data,
>   	return mxt_write_reg(data->client, reg + offset, 1,&val);
>   }
>
> +static int mxt_read_num_messages(struct mxt_data *data, u8 *count)
> +{
> +	/* TODO: Optimization: read first message along with message count */
> +	return mxt_read_reg(data->client, data->T44_address, 1, count);
> +}
> +
> +static int mxt_read_messages(struct mxt_data *data, u8 count,
> +			     struct mxt_message *messages)
> +{
> +	return mxt_read_reg(data->client, data->T5_address,
> +			    sizeof(struct mxt_message) * count, messages);
> +}
> +
>   static void mxt_input_touchevent(struct mxt_data *data,
>   				 struct mxt_message *message)
>   {
> @@ -575,26 +575,50 @@ static void mxt_input_touchevent(struct mxt_data *data,
>   	input_sync(input_dev);
>   }
>
> -static irqreturn_t mxt_interrupt(int irq, void *dev_id)
> +static int mxt_proc_messages(struct mxt_data *data, u8 count)
>   {
> -	struct mxt_data *data = dev_id;
> -	struct mxt_message message;
>   	struct device *dev =&data->client->dev;
> +	struct mxt_message messages[count], *msg;
> +	int ret;
>
> -	do {
> -		if (mxt_read_message(data,&message)) {
> -			dev_err(dev, "Failed to read message\n");
> -			goto end;
> -		}
> +	ret = mxt_read_messages(data, count, messages);
> +	if (ret) {
> +		dev_err(dev, "Failed to read %u messages (%d).\n", count, ret);
> +		return ret;
> +	}
> +
> +	for (msg = messages; msg<  &messages[count]; msg++) {
> +		mxt_dump_message(dev, msg);
> +
> +		if (msg->reportid>= data->T9_reportid_min&&
> +		    msg->reportid<= data->T9_reportid_max)
> +			mxt_input_touchevent(data, msg);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mxt_handle_messages(struct mxt_data *data)
> +{
> +	struct device *dev =&data->client->dev;
> +	int ret;
> +	u8 count;
>
> -		if (message.reportid>= data->T9_reportid_min&&
> -		    message.reportid<= data->T9_reportid_max)
> -			mxt_input_touchevent(data,&message);
> -		else
> -			mxt_dump_message(dev,&message);
> -	} while (message.reportid != 0xff);
> +	ret = mxt_read_num_messages(data,&count);
> +	if (ret) {
> +		dev_err(dev, "Failed to read message count (%d).\n", ret);
> +		return ret;
> +	}
>
> -end:
> +	if (count>  0)
> +		ret = mxt_proc_messages(data, count);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t mxt_interrupt(int irq, void *dev_id)
> +{
> +	mxt_handle_messages(dev_id);
>   	return IRQ_HANDLED;
>   }
>
> @@ -642,7 +666,7 @@ static int mxt_make_highchg(struct mxt_data *data)
>
>   	/* Read dummy message to make high CHG pin */
>   	do {
> -		error = mxt_read_message(data,&message);
> +		error = mxt_read_messages(data, 1,&message);
>   		if (error)
>   			return error;
>   	} while (message.reportid != 0xff&&  --count);
> @@ -743,11 +767,17 @@ static int mxt_get_object_table(struct mxt_data *data)
>
>   		/* Save data for objects used when processing interrupts */
>   		switch (object->type) {
> +		case MXT_GEN_MESSAGE_T5:
> +			data->T5_address = object->start_address;
> +			break;
>   		case MXT_TOUCH_MULTI_T9:
>   			data->T9_reportid_max = object->max_reportid;
>   			data->T9_reportid_min = data->T9_reportid_max -
>   						object->num_report_ids + 1;
>   			break;
> +		case MXT_SPT_MESSAGECOUNT_T44:
> +			data->T44_address = object->start_address;
> +			break;
>   		}
>   	}
>

Re: [PATCH 18/20] Input: atmel_mxt_ts - read num messages, then all messages

[Daniel Kurtz]

On Wed, Mar 14, 2012 at 10:32 AM, Joonyoung Shim
<jy0922.shim@samsung.com> wrote:
> On 03/13/2012 09:04 PM, Daniel Kurtz wrote:
>>
>> Implement the MXT DMA method of reading messages.
>> On an interrupt, the T44 report always contains the number of messages
>> pending to be read.  So, read 1 byte from T44 in 1 i2c transaction, then
>> read the N pending messages in a second transaction.
>
>
> My mXT224 chip datasheet hasn't T44 object, is it updated?
> If not, need to consider chips which doesn't have T44 object.

I could consider merging the use of T44 with the older 'check reportid
0xff' method, but would require help verifying the change with
hardware that doesn't have T44.
Does anybody out there have such hardware and is willing to test?

Thanks,
-Daniel

>
>> The end result is a much much faster read time for all pending messages.
>> Using 400kHz i2c, it is possible to read 10 pending messages (e.g. for 10
>> moving contatcts) in less than 2.8ms, which is well less than the typical
>> 10-15ms update rate.
>>
>> Note: There is a possible optimization here.  The T44 byte is guaranteed
>> to always be right before the T5 address.  Thus, it should be possible
>> to always fetch the T44 message count and the first message in a single
>> transaction.  This would eliminate the overhead of a second complete read
>> transaction for the case where there is only a single pending message.
>> (This is actually the most common case, for instance with just 1-contact
>> on the device touch surface). This optimization, however, is not done in
>> this patch.
>>
>> Signed-off-by: Daniel Kurtz<djkurtz@chromium.org>
>> ---
>>  drivers/input/touchscreen/atmel_mxt_ts.c |   92
>> ++++++++++++++++++++----------
>>  1 files changed, 61 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
>> b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index 88a474e..dafc030 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -251,8 +251,10 @@ struct mxt_data {
>>        unsigned int max_y;
>>
>>        /* Cached parameters from object table */
>> +       u16 T5_address;
>>        u8 T9_reportid_min;
>>        u8 T9_reportid_max;
>> +       u16 T44_address;
>>  };
>>
>>  static bool mxt_object_readable(unsigned int type)
>> @@ -486,21 +488,6 @@ static int mxt_read_object(struct mxt_data *data,
>> struct mxt_object *object,
>>                            val);
>>  }
>>
>> -static int mxt_read_message(struct mxt_data *data,
>> -                                struct mxt_message *message)
>> -{
>> -       struct mxt_object *object;
>> -       u16 reg;
>> -
>> -       object = mxt_get_object(data, MXT_GEN_MESSAGE_T5);
>> -       if (!object)
>> -               return -EINVAL;
>> -
>> -       reg = object->start_address;
>> -       return mxt_read_reg(data->client, reg, sizeof(struct mxt_message),
>> -                           message);
>> -}
>> -
>>  static int mxt_write_object(struct mxt_data *data,
>>                                 u8 type, u8 offset, u8 val)
>>  {
>> @@ -515,6 +502,19 @@ static int mxt_write_object(struct mxt_data *data,
>>        return mxt_write_reg(data->client, reg + offset, 1,&val);
>>  }
>>
>> +static int mxt_read_num_messages(struct mxt_data *data, u8 *count)
>> +{
>> +       /* TODO: Optimization: read first message along with message count
>> */
>> +       return mxt_read_reg(data->client, data->T44_address, 1, count);
>> +}
>> +
>> +static int mxt_read_messages(struct mxt_data *data, u8 count,
>> +                            struct mxt_message *messages)
>> +{
>> +       return mxt_read_reg(data->client, data->T5_address,
>> +                           sizeof(struct mxt_message) * count, messages);
>> +}
>> +
>>  static void mxt_input_touchevent(struct mxt_data *data,
>>                                 struct mxt_message *message)
>>  {
>> @@ -575,26 +575,50 @@ static void mxt_input_touchevent(struct mxt_data
>> *data,
>>        input_sync(input_dev);
>>  }
>>
>> -static irqreturn_t mxt_interrupt(int irq, void *dev_id)
>> +static int mxt_proc_messages(struct mxt_data *data, u8 count)
>>  {
>> -       struct mxt_data *data = dev_id;
>> -       struct mxt_message message;
>>        struct device *dev =&data->client->dev;
>>
>> +       struct mxt_message messages[count], *msg;
>> +       int ret;
>>
>> -       do {
>> -               if (mxt_read_message(data,&message)) {
>> -                       dev_err(dev, "Failed to read message\n");
>> -                       goto end;
>> -               }
>> +       ret = mxt_read_messages(data, count, messages);
>> +       if (ret) {
>> +               dev_err(dev, "Failed to read %u messages (%d).\n", count,
>> ret);
>> +               return ret;
>> +       }
>> +
>> +       for (msg = messages; msg<  &messages[count]; msg++) {
>> +               mxt_dump_message(dev, msg);
>> +
>> +               if (msg->reportid>= data->T9_reportid_min&&
>> +                   msg->reportid<= data->T9_reportid_max)
>> +                       mxt_input_touchevent(data, msg);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int mxt_handle_messages(struct mxt_data *data)
>> +{
>> +       struct device *dev =&data->client->dev;
>>
>> +       int ret;
>> +       u8 count;
>>
>> -               if (message.reportid>= data->T9_reportid_min&&
>> -                   message.reportid<= data->T9_reportid_max)
>> -                       mxt_input_touchevent(data,&message);
>> -               else
>> -                       mxt_dump_message(dev,&message);
>> -       } while (message.reportid != 0xff);
>> +       ret = mxt_read_num_messages(data,&count);
>> +       if (ret) {
>> +               dev_err(dev, "Failed to read message count (%d).\n", ret);
>> +               return ret;
>> +       }
>>
>> -end:
>> +       if (count>  0)
>> +               ret = mxt_proc_messages(data, count);
>> +
>> +       return ret;
>> +}
>> +
>> +static irqreturn_t mxt_interrupt(int irq, void *dev_id)
>> +{
>> +       mxt_handle_messages(dev_id);
>>        return IRQ_HANDLED;
>>  }
>>
>> @@ -642,7 +666,7 @@ static int mxt_make_highchg(struct mxt_data *data)
>>
>>        /* Read dummy message to make high CHG pin */
>>        do {
>> -               error = mxt_read_message(data,&message);
>> +               error = mxt_read_messages(data, 1,&message);
>>                if (error)
>>                        return error;
>>        } while (message.reportid != 0xff&&  --count);
>>
>> @@ -743,11 +767,17 @@ static int mxt_get_object_table(struct mxt_data
>> *data)
>>
>>                /* Save data for objects used when processing interrupts */
>>                switch (object->type) {
>> +               case MXT_GEN_MESSAGE_T5:
>> +                       data->T5_address = object->start_address;
>> +                       break;
>>                case MXT_TOUCH_MULTI_T9:
>>                        data->T9_reportid_max = object->max_reportid;
>>                        data->T9_reportid_min = data->T9_reportid_max -
>>                                                object->num_report_ids + 1;
>>                        break;
>> +               case MXT_SPT_MESSAGECOUNT_T44:
>> +                       data->T44_address = object->start_address;
>> +                       break;
>>                }
>>        }
>>
>
--
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 18/20] Input: atmel_mxt_ts - read num messages, then all messages

[Nick Dyer]

Daniel Kurtz wrote:
> On Wed, Mar 14, 2012 at 10:32 AM, Joonyoung Shim
> <jy0922.shim@samsung.com> wrote:
>> On 03/13/2012 09:04 PM, Daniel Kurtz wrote:
>>>
>>> Implement the MXT DMA method of reading messages.
>>> On an interrupt, the T44 report always contains the number of messages
>>> pending to be read.  So, read 1 byte from T44 in 1 i2c transaction, then
>>> read the N pending messages in a second transaction.
>>
>>
>> My mXT224 chip datasheet hasn't T44 object, is it updated?
>> If not, need to consider chips which doesn't have T44 object.
> 
> I could consider merging the use of T44 with the older 'check reportid
> 0xff' method, but would require help verifying the change with
> hardware that doesn't have T44.
> Does anybody out there have such hardware and is willing to test?

As he said, many maxtouch chips don't have T44. Although for chips that do
have T44, this would be a useful performance improvement.

What are you testing these patches on?

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 19/20] Input: atmel_mxt_ts - remove mxt_make_highchg and parse T6 report

[Daniel Kurtz]

This function attempts to make the CHG pin high by reading a bunch of
messages until the device is happy.  Instead of just blindly trying to
read a fixed number of messages, let's actually read and process them.

It turns out that the messages after boot or nvupdates are T6 reports,
containing a status, and the config memory checksum.  So, let's parse
them and dump a useful info message.

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index dafc030..c397a01 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -252,6 +252,7 @@ struct mxt_data {
 
 	/* Cached parameters from object table */
 	u16 T5_address;
+	u8 T6_reportid;
 	u8 T9_reportid_min;
 	u8 T9_reportid_max;
 	u16 T44_address;
@@ -591,8 +592,15 @@ static int mxt_proc_messages(struct mxt_data *data, u8 count)
 		mxt_dump_message(dev, msg);
 
 		if (msg->reportid >= data->T9_reportid_min &&
-		    msg->reportid <= data->T9_reportid_max)
+		    msg->reportid <= data->T9_reportid_max) {
 			mxt_input_touchevent(data, msg);
+		} else if (msg->reportid == data->T6_reportid) {
+			unsigned csum = msg->message[1] |
+					(msg->message[2] << 8) |
+					(msg->message[3] << 16);
+			dev_info(dev, "Status: %02x Config Checksum: %06x\n",
+				 msg->message[0], csum);
+		}
 	}
 
 	return 0;
@@ -657,28 +665,6 @@ static int mxt_check_reg_init(struct mxt_data *data)
 	return 0;
 }
 
-static int mxt_make_highchg(struct mxt_data *data)
-{
-	struct device *dev = &data->client->dev;
-	struct mxt_message message;
-	int count = 10;
-	int error;
-
-	/* Read dummy message to make high CHG pin */
-	do {
-		error = mxt_read_messages(data, 1, &message);
-		if (error)
-			return error;
-	} while (message.reportid != 0xff && --count);
-
-	if (!count) {
-		dev_err(dev, "CHG pin isn't cleared\n");
-		return -EBUSY;
-	}
-
-	return 0;
-}
-
 static void mxt_handle_pdata(struct mxt_data *data)
 {
 	const struct mxt_platform_data *pdata = data->pdata;
@@ -770,6 +756,9 @@ static int mxt_get_object_table(struct mxt_data *data)
 		case MXT_GEN_MESSAGE_T5:
 			data->T5_address = object->start_address;
 			break;
+		case MXT_GEN_COMMAND_T6:
+			data->T6_reportid = object->max_reportid;
+			break;
 		case MXT_TOUCH_MULTI_T9:
 			data->T9_reportid_max = object->max_reportid;
 			data->T9_reportid_min = data->T9_reportid_max -
@@ -1033,7 +1022,7 @@ static ssize_t mxt_update_fw_store(struct device *dev,
 
 	enable_irq(data->irq);
 
-	error = mxt_make_highchg(data);
+	error = mxt_handle_messages(data);
 	if (error)
 		return error;
 
@@ -1155,7 +1144,7 @@ static int __devinit mxt_probe(struct i2c_client *client,
 		goto err_free_object;
 	}
 
-	error = mxt_make_highchg(data);
+	error = mxt_handle_messages(data);
 	if (error)
 		goto err_free_irq;
 
-- 
1.7.7.3

Re: [PATCH 19/20] Input: atmel_mxt_ts - remove mxt_make_highchg and parse T6 report

[Nick Dyer]

Daniel Kurtz wrote:
> This function attempts to make the CHG pin high by reading a bunch of
> messages until the device is happy.  Instead of just blindly trying to
> read a fixed number of messages, let's actually read and process them.
> 
> It turns out that the messages after boot or nvupdates are T6 reports,
> containing a status, and the config memory checksum.  So, let's parse
> them and dump a useful info message.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

mxt_make_highchg exists due to the interrupts being edge triggered. It
forces the CHG line to go high.

With this implementation, I suspect that if a new message arrives after you
have read T44 and before you finish processing messages, then the interrupt
handler will not ever be run. What testing have you done on this?

The printout of T6 messages looks OK, though. I'd prefer to see a lookup
table for the report IDs.

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

Re: [PATCH 19/20] Input: atmel_mxt_ts - remove mxt_make_highchg and parse T6 report

[Daniel Kurtz]

On Tue, Mar 20, 2012 at 11:38 PM, Nick Dyer <nick.dyer@itdev.co.uk> wrote:
> Daniel Kurtz wrote:
>> This function attempts to make the CHG pin high by reading a bunch of
>> messages until the device is happy.  Instead of just blindly trying to
>> read a fixed number of messages, let's actually read and process them.
>>
>> It turns out that the messages after boot or nvupdates are T6 reports,
>> containing a status, and the config memory checksum.  So, let's parse
>> them and dump a useful info message.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>
> mxt_make_highchg exists due to the interrupts being edge triggered. It
> forces the CHG line to go high.
>
> With this implementation, I suspect that if a new message arrives after you
> have read T44 and before you finish processing messages, then the interrupt
> handler will not ever be run. What testing have you done on this?

Actually, for the MXT224E on which I've been testing, the CHG line
goes high after reading the first byte of the T5 message (in other
words, the first byte of the first message, no matter how many
messages T44 reports).

Expect an updated version of this patchset soon.

Thanks for all of your reviews!

>
> The printout of T6 messages looks OK, though. I'd prefer to see a lookup
> table for the report IDs.
>
> --
> Nick Dyer
> Software Engineer, ITDev Ltd
> Hardware and Software Development Consultancy
> Website: http://www.itdev.co.uk
--
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 19/20] Input: atmel_mxt_ts - remove mxt_make_highchg and parse T6 report

[Nick Dyer]

Daniel Kurtz wrote:
> On Tue, Mar 20, 2012 at 11:38 PM, Nick Dyer <nick.dyer@itdev.co.uk> wrote:
>> mxt_make_highchg exists due to the interrupts being edge triggered. It
>> forces the CHG line to go high.
>>
>> With this implementation, I suspect that if a new message arrives after you
>> have read T44 and before you finish processing messages, then the interrupt
>> handler will not ever be run. What testing have you done on this?
> 
> Actually, for the MXT224E on which I've been testing, the CHG line
> goes high after reading the first byte of the T5 message (in other
> words, the first byte of the first message, no matter how many
> messages T44 reports).

The CHG line behaviour is configurable - you should find this documented in
the data sheet.

> Expect an updated version of this patchset soon.
> 
> Thanks for all of your reviews!

no problem.

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

[PATCH 20/20] 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).

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

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index c397a01..dae8318 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -516,8 +516,7 @@ static int mxt_read_messages(struct mxt_data *data, u8 count,
 			    sizeof(struct mxt_message) * count, messages);
 }
 
-static void mxt_input_touchevent(struct mxt_data *data,
-				 struct mxt_message *message)
+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;
@@ -571,9 +570,6 @@ static void mxt_input_touchevent(struct mxt_data *data,
 		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 int mxt_proc_messages(struct mxt_data *data, u8 count)
@@ -581,6 +577,7 @@ static int mxt_proc_messages(struct mxt_data *data, u8 count)
 	struct device *dev = &data->client->dev;
 	struct mxt_message messages[count], *msg;
 	int ret;
+	bool update_input;
 
 	ret = mxt_read_messages(data, count, messages);
 	if (ret) {
@@ -588,12 +585,14 @@ static int mxt_proc_messages(struct mxt_data *data, u8 count)
 		return ret;
 	}
 
+	update_input = false;
 	for (msg = messages; msg < &messages[count]; msg++) {
 		mxt_dump_message(dev, msg);
 
 		if (msg->reportid >= data->T9_reportid_min &&
 		    msg->reportid <= data->T9_reportid_max) {
-			mxt_input_touchevent(data, msg);
+			mxt_input_touch(data, msg);
+			update_input = true;
 		} else if (msg->reportid == data->T6_reportid) {
 			unsigned csum = msg->message[1] |
 					(msg->message[2] << 8) |
@@ -603,6 +602,11 @@ static int mxt_proc_messages(struct mxt_data *data, u8 count)
 		}
 	}
 
+	if (update_input) {
+		input_mt_report_pointer_emulation(data->input_dev, false);
+		input_sync(data->input_dev);
+	}
+
 	return 0;
 }
 
-- 
1.7.7.3

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

[Nick Dyer]

Daniel Kurtz wrote:
> 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).
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Again, this looks like an improvement, but I think you need to state what
platforms you have tested this on.

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk

Re: [PATCH 00/20] cleanup atmel_mxt_ts

[Joonyoung Shim]

On 03/13/2012 09:04 PM, Daniel Kurtz wrote:
> This patchset cleans up the atmel_mxt_ts touchscreen driver.
> In particular, it addresses the following issues:
>
> 1) sysfs
>     a) fw_update only by root
>     b) allow writing config
>     c) add backupnv to save config to device flash
> 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)
>     c) read num messages first, instead of looking for 0xFF reportid
>     d) read all pending message bytes in one i2c read transaction
> 4) more correct MT-B support
>     a) send all (changed) contacts in a single EV_SYN/SYN_REPORT
>
> Daniel Kurtz (20):
>    Input: atmel_mxt_ts - use CONFIG_PM_SLEEP
>    Input: atmel_mxt_ts - only allow root to update firmware
>    Input: atmel_mxt_ts - verify object size in mxt_write_object
>    Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length
>    Input: atmel_mxt_ts - dump mxt_read/write_reg
>    Input: atmel_mxt_ts - allow writing to object sysfs entry
>    Input: atmel_mxt_ts - add backupnv sysfs entry
>    Input: atmel_mxt_ts - store actual size and instance
>    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 - simplify event reporting
>    Input: atmel_mxt_ts - parse vector field of data packets
>    Input: atmel_mxt_ts - refactor reading object table
>    Input: atmel_mxt_ts - optimize writing of object table entries
>    Input: atmel_mxt_ts - refactor get info
>    Input: atmel_mxt_ts - use cached T9 reportid range in isr
>    Input: atmel_mxt_ts - read num messages, then all messages
>    Input: atmel_mxt_ts - remove mxt_make_highchg and parse T6 report
>    Input: atmel_mxt_ts - send all MT-B slots in one input report
>
>   drivers/input/touchscreen/atmel_mxt_ts.c |  590 +++++++++++++++---------------
>   1 files changed, 286 insertions(+), 304 deletions(-)
>

Thanks for updating of driver.

About all patches
Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>

Henrik's review needs about modification of Multi-touch.

Thanks.

RE: [PATCH 00/20] cleanup atmel_mxt_ts

[Valkonen, Iiro]

Hi,

> -----Original Message-----
> From: Daniel Kurtz [mailto:djkurtz@chromium.org] 
> Subject: [PATCH 00/20] cleanup atmel_mxt_ts
> 
> This patchset cleans up the atmel_mxt_ts touchscreen driver.
> In particular, it addresses the following issues:

Thank you for the updates Daniel. I am no longer working with this
driver, but Atul Tiwari and Nick Dyer are, I think they will be giving
their input sometime soon.

Best Regards,

-- 
Iiro

Re: [PATCH 00/20] cleanup atmel_mxt_ts

[Nick Dyer]

Daniel Kurtz wrote:
> This patchset cleans up the atmel_mxt_ts touchscreen driver.
> In particular, it addresses the following issues:

Hi Daniel-

Thanks for your work on this, some of it looks very useful. I will go
through and review each patch in turn.

I have been working on Atmel's version of this driver. You can find most of
that work on our github site: as you know we are intending to push it
upstream as much as possible:
https://github.com/atmel-maxtouch

The main overlap between your changes and ours is in your approach to
writing config from userspace. There is a lot of complexity in these chips
and providing a sysfs node for each function would end up being a
never-ending task.

So we have implemented an interface using a sysfs binary attribute which I
think is considerably more general and flexible than the interface that you
suggest. You can find a set of utilities on the same github that uses our
interface. I would argue that we should try and converge the mainline
driver on our approach rather than inventing something new unless there are
significant advantages.

cheers

-- 
Nick Dyer
Software Engineer, ITDev Ltd
Hardware and Software Development Consultancy
Website: http://www.itdev.co.uk