[PATCH v1 01/10] Input: atmel_mxt_ts - only use first T9 instance

[PATCH v1 01/10] Input: atmel_mxt_ts - only use first T9 instance

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

The driver only registers one input device, which uses the screen
parameters from the first T9 instance. The first T63 instance also uses
those parameters.

It is incorrect to send input reports from the second instances of these
objects if they are enabled: the input scaling will be wrong and the
positions will be mashed together.

This also causes problems on Android if the number of slots exceeds 32.

In the future, this could be handled by looking for enabled touch object
instances and creating an input device for each one.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
Acked-by: Benson Leung <bleung@chromium.org>
Acked-by: Yufeng Shen <miletus@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 54fe190fd4bc..48c5ccab00a0 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1658,10 +1658,11 @@ static int mxt_parse_object_table(struct mxt_data *data,
 			break;
 		case MXT_TOUCH_MULTI_T9:
 			data->multitouch = MXT_TOUCH_MULTI_T9;
+			/* Only handle messages from first T9 instance */
 			data->T9_reportid_min = min_id;
-			data->T9_reportid_max = max_id;
-			data->num_touchids = object->num_report_ids
-						* mxt_obj_instances(object);
+			data->T9_reportid_max = min_id +
+						object->num_report_ids - 1;
+			data->num_touchids = object->num_report_ids;
 			break;
 		case MXT_SPT_MESSAGECOUNT_T44:
 			data->T44_address = object->start_address;
-- 
2.17.1

[PATCH v1 02/10] Input: atmel_mxt_ts - use BIT() macro everywhere

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 36 ++++++++++++------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 48c5ccab00a0..9555947a2d46 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -88,12 +88,12 @@
 #define MXT_COMMAND_DIAGNOSTIC	5
 
 /* Define for T6 status byte */
-#define MXT_T6_STATUS_RESET	(1 << 7)
-#define MXT_T6_STATUS_OFL	(1 << 6)
-#define MXT_T6_STATUS_SIGERR	(1 << 5)
-#define MXT_T6_STATUS_CAL	(1 << 4)
-#define MXT_T6_STATUS_CFGERR	(1 << 3)
-#define MXT_T6_STATUS_COMSERR	(1 << 2)
+#define MXT_T6_STATUS_RESET	BIT(7)
+#define MXT_T6_STATUS_OFL	BIT(6)
+#define MXT_T6_STATUS_SIGERR	BIT(5)
+#define MXT_T6_STATUS_CAL	BIT(4)
+#define MXT_T6_STATUS_CFGERR	BIT(3)
+#define MXT_T6_STATUS_COMSERR	BIT(2)
 
 /* MXT_GEN_POWER_T7 field */
 struct t7_config {
@@ -112,14 +112,14 @@ struct t7_config {
 #define MXT_T9_RANGE		18
 
 /* MXT_TOUCH_MULTI_T9 status */
-#define MXT_T9_UNGRIP		(1 << 0)
-#define MXT_T9_SUPPRESS		(1 << 1)
-#define MXT_T9_AMP		(1 << 2)
-#define MXT_T9_VECTOR		(1 << 3)
-#define MXT_T9_MOVE		(1 << 4)
-#define MXT_T9_RELEASE		(1 << 5)
-#define MXT_T9_PRESS		(1 << 6)
-#define MXT_T9_DETECT		(1 << 7)
+#define MXT_T9_UNGRIP		BIT(0)
+#define MXT_T9_SUPPRESS		BIT(1)
+#define MXT_T9_AMP		BIT(2)
+#define MXT_T9_VECTOR		BIT(3)
+#define MXT_T9_MOVE		BIT(4)
+#define MXT_T9_RELEASE		BIT(5)
+#define MXT_T9_PRESS		BIT(6)
+#define MXT_T9_DETECT		BIT(7)
 
 struct t9_range {
 	__le16 x;
@@ -127,9 +127,9 @@ struct t9_range {
 } __packed;
 
 /* MXT_TOUCH_MULTI_T9 orient */
-#define MXT_T9_ORIENT_SWITCH	(1 << 0)
-#define MXT_T9_ORIENT_INVERTX	(1 << 1)
-#define MXT_T9_ORIENT_INVERTY	(1 << 2)
+#define MXT_T9_ORIENT_SWITCH	BIT(0)
+#define MXT_T9_ORIENT_INVERTX	BIT(1)
+#define MXT_T9_ORIENT_INVERTY	BIT(2)
 
 /* MXT_SPT_COMMSCONFIG_T18 */
 #define MXT_COMMS_CTRL		0
@@ -214,7 +214,7 @@ enum t100_type {
 #define MXT_FRAME_CRC_PASS	0x04
 #define MXT_APP_CRC_FAIL	0x40	/* valid 7 8 bit only */
 #define MXT_BOOT_STATUS_MASK	0x3f
-#define MXT_BOOT_EXTENDED_ID	(1 << 5)
+#define MXT_BOOT_EXTENDED_ID	BIT(5)
 #define MXT_BOOT_ID_MASK	0x1f
 
 /* Touchscreen absolute values */
-- 
2.17.1

[PATCH v1 03/10] Input: atmel_mxt_ts - remove duplicate setup of ABS_MT_PRESSURE

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 9555947a2d46..dcafb812ee7e 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -2055,12 +2055,6 @@ static int mxt_initialize_input_device(struct mxt_data *data)
 				     0, 255, 0, 0);
 	}
 
-	if (data->multitouch == MXT_TOUCH_MULTITOUCHSCREEN_T100 &&
-	    data->t100_aux_ampl) {
-		input_set_abs_params(input_dev, ABS_MT_PRESSURE,
-				     0, 255, 0, 0);
-	}
-
 	if (data->multitouch == MXT_TOUCH_MULTITOUCHSCREEN_T100 &&
 	    data->t100_aux_vect) {
 		input_set_abs_params(input_dev, ABS_MT_ORIENTATION,
-- 
2.17.1

[PATCH v1 04/10] Input: atmel_mxt_ts - remove unnecessary debug on ENOMEM

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index dcafb812ee7e..92661aa910ae 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1520,10 +1520,8 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 			MXT_INFO_CHECKSUM_SIZE;
 	config_mem_size = data->mem_size - cfg_start_ofs;
 	config_mem = kzalloc(config_mem_size, GFP_KERNEL);
-	if (!config_mem) {
-		dev_err(dev, "Failed to allocate memory\n");
+	if (!config_mem)
 		return -ENOMEM;
-	}
 
 	ret = mxt_prepare_cfg_mem(data, cfg, data_pos, cfg_start_ofs,
 				  config_mem, config_mem_size);
@@ -1982,10 +1980,8 @@ static int mxt_initialize_input_device(struct mxt_data *data)
 
 	/* Register input device */
 	input_dev = input_allocate_device();
-	if (!input_dev) {
-		dev_err(dev, "Failed to allocate memory\n");
+	if (!input_dev)
 		return -ENOMEM;
-	}
 
 	input_dev->name = "Atmel maXTouch Touchscreen";
 	input_dev->phys = data->phys;
-- 
2.17.1

[PATCH v1 05/10] Input: atmel_mxt_ts - config CRC may start at T71

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

On devices with the T71 object, the config CRC will start there, rather
than at T7.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 34 +++++++++++++++---------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 92661aa910ae..560d4997ef8c 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -75,6 +75,7 @@
 #define MXT_SPT_DIGITIZER_T43		43
 #define MXT_SPT_MESSAGECOUNT_T44	44
 #define MXT_SPT_CTECONFIG_T46		46
+#define MXT_SPT_DYNAMICCONFIGURATIONCONTAINER_T71 71
 #define MXT_TOUCH_MULTITOUCHSCREEN_T100 100
 
 /* MXT_GEN_MESSAGE_T5 object */
@@ -317,6 +318,7 @@ struct mxt_data {
 	u8 T6_reportid;
 	u16 T6_address;
 	u16 T7_address;
+	u16 T71_address;
 	u8 T9_reportid_min;
 	u8 T9_reportid_max;
 	u8 T19_reportid;
@@ -382,6 +384,7 @@ static bool mxt_object_readable(unsigned int type)
 	case MXT_SPT_USERDATA_T38:
 	case MXT_SPT_DIGITIZER_T43:
 	case MXT_SPT_CTECONFIG_T46:
+	case MXT_SPT_DYNAMICCONFIGURATIONCONTAINER_T71:
 		return true;
 	default:
 		return false;
@@ -1443,6 +1446,7 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 	u32 info_crc, config_crc, calculated_crc;
 	u8 *config_mem;
 	size_t config_mem_size;
+	u16 crc_start = 0;
 
 	mxt_update_crc(data, MXT_COMMAND_REPORTALL, 1);
 
@@ -1529,20 +1533,22 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 		goto release_mem;
 
 	/* Calculate crc of the received configs (not the raw config file) */
-	if (data->T7_address < cfg_start_ofs) {
-		dev_err(dev, "Bad T7 address, T7addr = %x, config offset %x\n",
-			data->T7_address, cfg_start_ofs);
-		ret = 0;
-		goto release_mem;
-	}
+	if (data->T71_address)
+		crc_start = data->T71_address;
+	else if (data->T7_address)
+		crc_start = data->T7_address;
+	else
+		dev_warn(dev, "Could not find CRC start\n");
 
-	calculated_crc = mxt_calculate_crc(config_mem,
-					   data->T7_address - cfg_start_ofs,
-					   config_mem_size);
+	if (crc_start > cfg_start_ofs) {
+		calculated_crc = mxt_calculate_crc(config_mem,
+						   crc_start - cfg_start_ofs,
+						   config_mem_size);
 
-	if (config_crc > 0 && config_crc != calculated_crc)
-		dev_warn(dev, "Config CRC error, calculated=%06X, file=%06X\n",
-			 calculated_crc, config_crc);
+		if (config_crc > 0 && config_crc != calculated_crc)
+			dev_warn(dev, "Config CRC in file inconsistent, calculated=%06X, file=%06X\n",
+				 calculated_crc, config_crc);
+	}
 
 	ret = mxt_upload_cfg_mem(data, cfg_start_ofs,
 				 config_mem, config_mem_size);
@@ -1589,6 +1595,7 @@ static void mxt_free_object_table(struct mxt_data *data)
 	data->T5_msg_size = 0;
 	data->T6_reportid = 0;
 	data->T7_address = 0;
+	data->T71_address = 0;
 	data->T9_reportid_min = 0;
 	data->T9_reportid_max = 0;
 	data->T19_reportid = 0;
@@ -1654,6 +1661,9 @@ static int mxt_parse_object_table(struct mxt_data *data,
 		case MXT_GEN_POWER_T7:
 			data->T7_address = object->start_address;
 			break;
+		case MXT_SPT_DYNAMICCONFIGURATIONCONTAINER_T71:
+			data->T71_address = object->start_address;
+			break;
 		case MXT_TOUCH_MULTI_T9:
 			data->multitouch = MXT_TOUCH_MULTI_T9;
 			/* Only handle messages from first T9 instance */
-- 
2.17.1

[PATCH v1 06/10] Input: atmel_mxt_ts - refactor config update code to add context struct

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 108 ++++++++++++-----------
 1 file changed, 56 insertions(+), 52 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 560d4997ef8c..0ce126e918f1 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -277,6 +277,19 @@ enum mxt_suspend_mode {
 	MXT_SUSPEND_T9_CTRL	= 1,
 };
 
+/* Config update context */
+struct mxt_cfg {
+	const u8 *raw;
+	size_t raw_size;
+	off_t raw_pos;
+
+	u8 *mem;
+	size_t mem_size;
+	int start_ofs;
+
+	struct mxt_info info;
+};
+
 /* Each client has this additional data */
 struct mxt_data {
 	struct i2c_client *client;
@@ -1282,12 +1295,7 @@ static u32 mxt_calculate_crc(u8 *base, off_t start_off, off_t end_off)
 	return crc;
 }
 
-static int mxt_prepare_cfg_mem(struct mxt_data *data,
-			       const struct firmware *cfg,
-			       unsigned int data_pos,
-			       unsigned int cfg_start_ofs,
-			       u8 *config_mem,
-			       size_t config_mem_size)
+static int mxt_prepare_cfg_mem(struct mxt_data *data, struct mxt_cfg *cfg)
 {
 	struct device *dev = &data->client->dev;
 	struct mxt_object *object;
@@ -1298,9 +1306,9 @@ static int mxt_prepare_cfg_mem(struct mxt_data *data,
 	u16 reg;
 	u8 val;
 
-	while (data_pos < cfg->size) {
+	while (cfg->raw_pos < cfg->raw_size) {
 		/* Read type, instance, length */
-		ret = sscanf(cfg->data + data_pos, "%x %x %x%n",
+		ret = sscanf(cfg->raw + cfg->raw_pos, "%x %x %x%n",
 			     &type, &instance, &size, &offset);
 		if (ret == 0) {
 			/* EOF */
@@ -1309,20 +1317,20 @@ static int mxt_prepare_cfg_mem(struct mxt_data *data,
 			dev_err(dev, "Bad format: failed to parse object\n");
 			return -EINVAL;
 		}
-		data_pos += offset;
+		cfg->raw_pos += offset;
 
 		object = mxt_get_object(data, type);
 		if (!object) {
 			/* Skip object */
 			for (i = 0; i < size; i++) {
-				ret = sscanf(cfg->data + data_pos, "%hhx%n",
+				ret = sscanf(cfg->raw + cfg->raw_pos, "%hhx%n",
 					     &val, &offset);
 				if (ret != 1) {
 					dev_err(dev, "Bad format in T%d at %d\n",
 						type, i);
 					return -EINVAL;
 				}
-				data_pos += offset;
+				cfg->raw_pos += offset;
 			}
 			continue;
 		}
@@ -1357,7 +1365,7 @@ static int mxt_prepare_cfg_mem(struct mxt_data *data,
 		reg = object->start_address + mxt_obj_size(object) * instance;
 
 		for (i = 0; i < size; i++) {
-			ret = sscanf(cfg->data + data_pos, "%hhx%n",
+			ret = sscanf(cfg->raw + cfg->raw_pos, "%hhx%n",
 				     &val,
 				     &offset);
 			if (ret != 1) {
@@ -1365,15 +1373,15 @@ static int mxt_prepare_cfg_mem(struct mxt_data *data,
 					type, i);
 				return -EINVAL;
 			}
-			data_pos += offset;
+			cfg->raw_pos += offset;
 
 			if (i > mxt_obj_size(object))
 				continue;
 
-			byte_offset = reg + i - cfg_start_ofs;
+			byte_offset = reg + i - cfg->start_ofs;
 
-			if (byte_offset >= 0 && byte_offset < config_mem_size) {
-				*(config_mem + byte_offset) = val;
+			if (byte_offset >= 0 && byte_offset < cfg->mem_size) {
+				*(cfg->mem + byte_offset) = val;
 			} else {
 				dev_err(dev, "Bad object: reg:%d, T%d, ofs=%d\n",
 					reg, object->type, byte_offset);
@@ -1385,22 +1393,21 @@ static int mxt_prepare_cfg_mem(struct mxt_data *data,
 	return 0;
 }
 
-static int mxt_upload_cfg_mem(struct mxt_data *data, unsigned int cfg_start,
-			      u8 *config_mem, size_t config_mem_size)
+static int mxt_upload_cfg_mem(struct mxt_data *data, struct mxt_cfg *cfg)
 {
 	unsigned int byte_offset = 0;
 	int error;
 
 	/* Write configuration as blocks */
-	while (byte_offset < config_mem_size) {
-		unsigned int size = config_mem_size - byte_offset;
+	while (byte_offset < cfg->mem_size) {
+		unsigned int size = cfg->mem_size - byte_offset;
 
 		if (size > MXT_MAX_BLOCK_WRITE)
 			size = MXT_MAX_BLOCK_WRITE;
 
 		error = __mxt_write_reg(data->client,
-					cfg_start + byte_offset,
-					size, config_mem + byte_offset);
+					cfg->start_ofs + byte_offset,
+					size, cfg->mem + byte_offset);
 		if (error) {
 			dev_err(&data->client->dev,
 				"Config write error, ret=%d\n", error);
@@ -1434,66 +1441,65 @@ static int mxt_init_t7_power_cfg(struct mxt_data *data);
  *   <SIZE> - 2-byte object size as hex
  *   <CONTENTS> - array of <SIZE> 1-byte hex values
  */
-static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
+static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
 {
 	struct device *dev = &data->client->dev;
-	struct mxt_info cfg_info;
+	struct mxt_cfg cfg;
 	int ret;
 	int offset;
-	int data_pos;
 	int i;
-	int cfg_start_ofs;
 	u32 info_crc, config_crc, calculated_crc;
-	u8 *config_mem;
-	size_t config_mem_size;
 	u16 crc_start = 0;
 
+	cfg.raw = fw->data;
+	cfg.raw_size = fw->size;
+
 	mxt_update_crc(data, MXT_COMMAND_REPORTALL, 1);
 
-	if (strncmp(cfg->data, MXT_CFG_MAGIC, strlen(MXT_CFG_MAGIC))) {
+	if (strncmp(cfg.raw, MXT_CFG_MAGIC, strlen(MXT_CFG_MAGIC))) {
 		dev_err(dev, "Unrecognised config file\n");
 		return -EINVAL;
 	}
 
-	data_pos = strlen(MXT_CFG_MAGIC);
+	cfg.raw_pos = strlen(MXT_CFG_MAGIC);
 
 	/* Load information block and check */
 	for (i = 0; i < sizeof(struct mxt_info); i++) {
-		ret = sscanf(cfg->data + data_pos, "%hhx%n",
-			     (unsigned char *)&cfg_info + i,
+		ret = sscanf(cfg.raw + cfg.raw_pos, "%hhx%n",
+			     (unsigned char *)&cfg.info + i,
 			     &offset);
 		if (ret != 1) {
 			dev_err(dev, "Bad format\n");
 			return -EINVAL;
 		}
 
-		data_pos += offset;
+		cfg.raw_pos += offset;
 	}
 
-	if (cfg_info.family_id != data->info->family_id) {
+	if (cfg.info.family_id != data->info->family_id) {
 		dev_err(dev, "Family ID mismatch!\n");
 		return -EINVAL;
 	}
 
-	if (cfg_info.variant_id != data->info->variant_id) {
+	if (cfg.info.variant_id != data->info->variant_id) {
 		dev_err(dev, "Variant ID mismatch!\n");
 		return -EINVAL;
 	}
 
 	/* Read CRCs */
-	ret = sscanf(cfg->data + data_pos, "%x%n", &info_crc, &offset);
+	ret = sscanf(cfg.raw + cfg.raw_pos, "%x%n", &info_crc, &offset);
 	if (ret != 1) {
 		dev_err(dev, "Bad format: failed to parse Info CRC\n");
 		return -EINVAL;
 	}
-	data_pos += offset;
+	cfg.raw_pos += offset;
 
-	ret = sscanf(cfg->data + data_pos, "%x%n", &config_crc, &offset);
+	ret = sscanf(cfg.raw + cfg.raw_pos, "%x%n", &config_crc, &offset);
 	if (ret != 1) {
 		dev_err(dev, "Bad format: failed to parse Config CRC\n");
 		return -EINVAL;
 	}
-	data_pos += offset;
+	cfg.raw_pos += offset;
 
 	/*
 	 * The Info Block CRC is calculated over mxt_info and the object
@@ -1519,16 +1525,15 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 	}
 
 	/* Malloc memory to store configuration */
-	cfg_start_ofs = MXT_OBJECT_START +
+	cfg.start_ofs = MXT_OBJECT_START +
 			data->info->object_num * sizeof(struct mxt_object) +
 			MXT_INFO_CHECKSUM_SIZE;
-	config_mem_size = data->mem_size - cfg_start_ofs;
-	config_mem = kzalloc(config_mem_size, GFP_KERNEL);
-	if (!config_mem)
+	cfg.mem_size = data->mem_size - cfg.start_ofs;
+	cfg.mem = kzalloc(cfg.mem_size, GFP_KERNEL);
+	if (!cfg.mem)
 		return -ENOMEM;
 
-	ret = mxt_prepare_cfg_mem(data, cfg, data_pos, cfg_start_ofs,
-				  config_mem, config_mem_size);
+	ret = mxt_prepare_cfg_mem(data, &cfg);
 	if (ret)
 		goto release_mem;
 
@@ -1540,18 +1545,17 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 	else
 		dev_warn(dev, "Could not find CRC start\n");
 
-	if (crc_start > cfg_start_ofs) {
-		calculated_crc = mxt_calculate_crc(config_mem,
-						   crc_start - cfg_start_ofs,
-						   config_mem_size);
+	if (crc_start > cfg.start_ofs) {
+		calculated_crc = mxt_calculate_crc(cfg.mem,
+						   crc_start - cfg.start_ofs,
+						   cfg.mem_size);
 
 		if (config_crc > 0 && config_crc != calculated_crc)
 			dev_warn(dev, "Config CRC in file inconsistent, calculated=%06X, file=%06X\n",
 				 calculated_crc, config_crc);
 	}
 
-	ret = mxt_upload_cfg_mem(data, cfg_start_ofs,
-				 config_mem, config_mem_size);
+	ret = mxt_upload_cfg_mem(data, &cfg);
 	if (ret)
 		goto release_mem;
 
@@ -1567,7 +1571,7 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 	mxt_init_t7_power_cfg(data);
 
 release_mem:
-	kfree(config_mem);
+	kfree(cfg.mem);
 	return ret;
 }
 
-- 
2.17.1

[PATCH v1 07/10] Input: atmel_mxt_ts - zero terminate config firmware file

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

We use sscanf to parse the configuration file, so it's necessary to zero
terminate the configuration otherwise a truncated file can cause the
parser to run off into uninitialised memory.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 36 +++++++++++++++++-------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 0ce126e918f1..2d1fddafb7f9 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -279,7 +279,7 @@ enum mxt_suspend_mode {
 
 /* Config update context */
 struct mxt_cfg {
-	const u8 *raw;
+	u8 *raw;
 	size_t raw_size;
 	off_t raw_pos;
 
@@ -1451,14 +1451,21 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
 	u32 info_crc, config_crc, calculated_crc;
 	u16 crc_start = 0;
 
-	cfg.raw = fw->data;
+	/* Make zero terminated copy of the OBP_RAW file */
+	cfg.raw = kzalloc(fw->size + 1, GFP_KERNEL);
+	if (!cfg.raw)
+		return -ENOMEM;
+
+	memcpy(cfg.raw, fw->data, fw->size);
+	cfg.raw[fw->size] = '\0';
 	cfg.raw_size = fw->size;
 
 	mxt_update_crc(data, MXT_COMMAND_REPORTALL, 1);
 
 	if (strncmp(cfg.raw, MXT_CFG_MAGIC, strlen(MXT_CFG_MAGIC))) {
 		dev_err(dev, "Unrecognised config file\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto release_raw;
 	}
 
 	cfg.raw_pos = strlen(MXT_CFG_MAGIC);
@@ -1470,7 +1477,8 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
 			     &offset);
 		if (ret != 1) {
 			dev_err(dev, "Bad format\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto release_raw;
 		}
 
 		cfg.raw_pos += offset;
@@ -1478,26 +1486,30 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
 
 	if (cfg.info.family_id != data->info->family_id) {
 		dev_err(dev, "Family ID mismatch!\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto release_raw;
 	}
 
 	if (cfg.info.variant_id != data->info->variant_id) {
 		dev_err(dev, "Variant ID mismatch!\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto release_raw;
 	}
 
 	/* Read CRCs */
 	ret = sscanf(cfg.raw + cfg.raw_pos, "%x%n", &info_crc, &offset);
 	if (ret != 1) {
 		dev_err(dev, "Bad format: failed to parse Info CRC\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto release_raw;
 	}
 	cfg.raw_pos += offset;
 
 	ret = sscanf(cfg.raw + cfg.raw_pos, "%x%n", &config_crc, &offset);
 	if (ret != 1) {
 		dev_err(dev, "Bad format: failed to parse Config CRC\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto release_raw;
 	}
 	cfg.raw_pos += offset;
 
@@ -1530,8 +1542,10 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
 			MXT_INFO_CHECKSUM_SIZE;
 	cfg.mem_size = data->mem_size - cfg.start_ofs;
 	cfg.mem = kzalloc(cfg.mem_size, GFP_KERNEL);
-	if (!cfg.mem)
-		return -ENOMEM;
+	if (!cfg.mem) {
+		ret = -ENOMEM;
+		goto release_raw;
+	}
 
 	ret = mxt_prepare_cfg_mem(data, &cfg);
 	if (ret)
@@ -1570,6 +1584,8 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
 	/* T7 config may have changed */
 	mxt_init_t7_power_cfg(data);
 
+release_raw:
+	kfree(cfg.raw);
 release_mem:
 	kfree(cfg.mem);
 	return ret;
-- 
2.17.1

Re: [PATCH v1 07/10] Input: atmel_mxt_ts - zero terminate config firmware file

[Dmitry Torokhov]

On Fri, Jul 20, 2018 at 10:51:19PM +0100, Nick Dyer wrote:
> From: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> We use sscanf to parse the configuration file, so it's necessary to zero
> terminate the configuration otherwise a truncated file can cause the
> parser to run off into uninitialised memory.
> 
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 36 +++++++++++++++++-------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 0ce126e918f1..2d1fddafb7f9 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -279,7 +279,7 @@ enum mxt_suspend_mode {
>  
>  /* Config update context */
>  struct mxt_cfg {
> -	const u8 *raw;
> +	u8 *raw;
>  	size_t raw_size;
>  	off_t raw_pos;
>  
> @@ -1451,14 +1451,21 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
>  	u32 info_crc, config_crc, calculated_crc;
>  	u16 crc_start = 0;
>  
> -	cfg.raw = fw->data;
> +	/* Make zero terminated copy of the OBP_RAW file */
> +	cfg.raw = kzalloc(fw->size + 1, GFP_KERNEL);

kmemdup_nul()? I guess config it not that big to be concerned with
kmalloc() vs vmalloc() and allocation failures...

> +	if (!cfg.raw)
> +		return -ENOMEM;
> +
> +	memcpy(cfg.raw, fw->data, fw->size);
> +	cfg.raw[fw->size] = '\0';
>  	cfg.raw_size = fw->size;
>  
>  	mxt_update_crc(data, MXT_COMMAND_REPORTALL, 1);
>  
>  	if (strncmp(cfg.raw, MXT_CFG_MAGIC, strlen(MXT_CFG_MAGIC))) {
>  		dev_err(dev, "Unrecognised config file\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto release_raw;
>  	}
>  
>  	cfg.raw_pos = strlen(MXT_CFG_MAGIC);
> @@ -1470,7 +1477,8 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
>  			     &offset);
>  		if (ret != 1) {
>  			dev_err(dev, "Bad format\n");
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			goto release_raw;
>  		}
>  
>  		cfg.raw_pos += offset;
> @@ -1478,26 +1486,30 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
>  
>  	if (cfg.info.family_id != data->info->family_id) {
>  		dev_err(dev, "Family ID mismatch!\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto release_raw;
>  	}
>  
>  	if (cfg.info.variant_id != data->info->variant_id) {
>  		dev_err(dev, "Variant ID mismatch!\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto release_raw;
>  	}
>  
>  	/* Read CRCs */
>  	ret = sscanf(cfg.raw + cfg.raw_pos, "%x%n", &info_crc, &offset);
>  	if (ret != 1) {
>  		dev_err(dev, "Bad format: failed to parse Info CRC\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto release_raw;
>  	}
>  	cfg.raw_pos += offset;
>  
>  	ret = sscanf(cfg.raw + cfg.raw_pos, "%x%n", &config_crc, &offset);
>  	if (ret != 1) {
>  		dev_err(dev, "Bad format: failed to parse Config CRC\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto release_raw;
>  	}
>  	cfg.raw_pos += offset;
>  
> @@ -1530,8 +1542,10 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
>  			MXT_INFO_CHECKSUM_SIZE;
>  	cfg.mem_size = data->mem_size - cfg.start_ofs;
>  	cfg.mem = kzalloc(cfg.mem_size, GFP_KERNEL);
> -	if (!cfg.mem)
> -		return -ENOMEM;
> +	if (!cfg.mem) {
> +		ret = -ENOMEM;
> +		goto release_raw;
> +	}
>  
>  	ret = mxt_prepare_cfg_mem(data, &cfg);
>  	if (ret)
> @@ -1570,6 +1584,8 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
>  	/* T7 config may have changed */
>  	mxt_init_t7_power_cfg(data);
>  
> +release_raw:
> +	kfree(cfg.raw);
>  release_mem:
>  	kfree(cfg.mem);
>  	return ret;
> -- 
> 2.17.1
> 

-- 
Dmitry

Re: [PATCH v1 07/10] Input: atmel_mxt_ts - zero terminate config firmware file

[Nick Dyer]

On Mon, Jul 23, 2018 at 03:35:34PM -0700, Dmitry Torokhov wrote:
> On Fri, Jul 20, 2018 at 10:51:19PM +0100, Nick Dyer wrote:
> > From: Nick Dyer <nick.dyer@itdev.co.uk>
> > 
> > We use sscanf to parse the configuration file, so it's necessary to zero
> > terminate the configuration otherwise a truncated file can cause the
> > parser to run off into uninitialised memory.
> > 
> > Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 36 +++++++++++++++++-------
> >  1 file changed, 26 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index 0ce126e918f1..2d1fddafb7f9 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -279,7 +279,7 @@ enum mxt_suspend_mode {
> >  
> >  /* Config update context */
> >  struct mxt_cfg {
> > -	const u8 *raw;
> > +	u8 *raw;
> >  	size_t raw_size;
> >  	off_t raw_pos;
> >  
> > @@ -1451,14 +1451,21 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)
> >  	u32 info_crc, config_crc, calculated_crc;
> >  	u16 crc_start = 0;
> >  
> > -	cfg.raw = fw->data;
> > +	/* Make zero terminated copy of the OBP_RAW file */
> > +	cfg.raw = kzalloc(fw->size + 1, GFP_KERNEL);
> 
> kmemdup_nul()? I guess config it not that big to be concerned with
> kmalloc() vs vmalloc() and allocation failures...

Yes, that looks like it should work. There's a limit on the size of the
data due to the I2C address space, so we should be fine.

Nick

[PATCH v1 08/10] Input: atmel_mxt_ts - don't report zero pressure from T9

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

If T9.CTRL DISAMP is set, then pressure is reported as zero. This means
some app layers (eg tslib) will ignore the contact.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 2d1fddafb7f9..d7023d261458 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -843,6 +843,10 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
 			mxt_input_sync(data);
 		}
 
+		/* if active, pressure must be non-zero */
+		if (!amplitude)
+			amplitude = MXT_PRESSURE_DEFAULT;
+
 		/* Touch active */
 		input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 1);
 		input_report_abs(input_dev, ABS_MT_POSITION_X, x);
-- 
2.17.1

[PATCH v1 09/10] Input: atmel_mxt_ts - tool type is ignored when slot is closed

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

input_mt_report_slot_state() ignores the tool when the slot is closed.
Remove the tool type from these function calls, which has caused a bit of
confusion.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index d7023d261458..c31af790ef84 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -838,8 +838,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
 		 * have happened.
 		 */
 		if (status & MXT_T9_RELEASE) {
-			input_mt_report_slot_state(input_dev,
-						   MT_TOOL_FINGER, 0);
+			input_mt_report_slot_state(input_dev, 0, 0);
 			mxt_input_sync(data);
 		}
 
@@ -855,7 +854,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
 		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
 	} else {
 		/* Touch no longer active, close out slot */
-		input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 0);
+		input_mt_report_slot_state(input_dev, 0, 0);
 	}
 
 	data->update_input = true;
-- 
2.17.1

Re: [PATCH v1 09/10] Input: atmel_mxt_ts - tool type is ignored when slot is closed

[Dmitry Torokhov]

On Fri, Jul 20, 2018 at 10:51:21PM +0100, Nick Dyer wrote:
> From: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> input_mt_report_slot_state() ignores the tool when the slot is closed.
> Remove the tool type from these function calls, which has caused a bit of
> confusion.

Hmm, maybe we could introduce MT_TOOL_NONE or MT_TOOL_INACTIVE and get
rid of the 3rd parameter? It will require a bit of macro trickery for a
release or 2...

> 
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index d7023d261458..c31af790ef84 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -838,8 +838,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
>  		 * have happened.
>  		 */
>  		if (status & MXT_T9_RELEASE) {
> -			input_mt_report_slot_state(input_dev,
> -						   MT_TOOL_FINGER, 0);
> +			input_mt_report_slot_state(input_dev, 0, 0);
>  			mxt_input_sync(data);
>  		}
>  
> @@ -855,7 +854,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
>  		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
>  	} else {
>  		/* Touch no longer active, close out slot */
> -		input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 0);
> +		input_mt_report_slot_state(input_dev, 0, 0);
>  	}
>  
>  	data->update_input = true;
> -- 
> 2.17.1
> 

-- 
Dmitry

Re: [PATCH v1 09/10] Input: atmel_mxt_ts - tool type is ignored when slot is closed

[Benjamin Tissoires]

On Tue, Jul 24, 2018 at 12:34 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, Jul 20, 2018 at 10:51:21PM +0100, Nick Dyer wrote:
> > From: Nick Dyer <nick.dyer@itdev.co.uk>
> >
> > input_mt_report_slot_state() ignores the tool when the slot is closed.
> > Remove the tool type from these function calls, which has caused a bit of
> > confusion.
>
> Hmm, maybe we could introduce MT_TOOL_NONE or MT_TOOL_INACTIVE and get
> rid of the 3rd parameter? It will require a bit of macro trickery for a
> release or 2...

I am not sure what would be the benefit of adding those new tools, if
the input_mt code discards them. Do you want to forward them to the
userspace with the release?
This reminds me the discussion we had recently with the touchscreens
releasing the slots with a MT_TOOL_PALM.

Anyway, better include Peter as he will be using this new MT_TOOL.

Cheers,
Benjamin

>
> >
> > Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> > ---
> >  drivers/input/touchscreen/atmel_mxt_ts.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > index d7023d261458..c31af790ef84 100644
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -838,8 +838,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> >                * have happened.
> >                */
> >               if (status & MXT_T9_RELEASE) {
> > -                     input_mt_report_slot_state(input_dev,
> > -                                                MT_TOOL_FINGER, 0);
> > +                     input_mt_report_slot_state(input_dev, 0, 0);
> >                       mxt_input_sync(data);
> >               }
> >
> > @@ -855,7 +854,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> >               input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
> >       } else {
> >               /* Touch no longer active, close out slot */
> > -             input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 0);
> > +             input_mt_report_slot_state(input_dev, 0, 0);
> >       }
> >
> >       data->update_input = true;
> > --
> > 2.17.1
> >
>
> --
> Dmitry

Re: [PATCH v1 09/10] Input: atmel_mxt_ts - tool type is ignored when slot is closed

[Peter Hutterer]

On Tue, Jul 24, 2018 at 10:23:27AM +0200, Benjamin Tissoires wrote:
> On Tue, Jul 24, 2018 at 12:34 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, Jul 20, 2018 at 10:51:21PM +0100, Nick Dyer wrote:
> > > From: Nick Dyer <nick.dyer@itdev.co.uk>
> > >
> > > input_mt_report_slot_state() ignores the tool when the slot is closed.
> > > Remove the tool type from these function calls, which has caused a bit of
> > > confusion.
> >
> > Hmm, maybe we could introduce MT_TOOL_NONE or MT_TOOL_INACTIVE and get
> > rid of the 3rd parameter? It will require a bit of macro trickery for a
> > release or 2...
> 
> I am not sure what would be the benefit of adding those new tools, if
> the input_mt code discards them. Do you want to forward them to the
> userspace with the release?
> This reminds me the discussion we had recently with the touchscreens
> releasing the slots with a MT_TOOL_PALM.
> 
> Anyway, better include Peter as he will be using this new MT_TOOL.

thanks for the CC, would've missed this.

From what I read this would be a helper for internal changes only, not
exposed to userspace? If so maybe it's better/easier/more readable to break
it into two functions
  input_mt_open_slot(dev, MT_TOOL_FINGER)
  input_mt_close_slot(dev)
  
This removes any ambiguity about the handling of the tool and should be a
fairly trivial search/replace. Replace the 'open/close' terminology with
whatever suits better.

Cheers,
   Peter

> > > Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> > > ---
> > >  drivers/input/touchscreen/atmel_mxt_ts.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> > > index d7023d261458..c31af790ef84 100644
> > > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > > @@ -838,8 +838,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> > >                * have happened.
> > >                */
> > >               if (status & MXT_T9_RELEASE) {
> > > -                     input_mt_report_slot_state(input_dev,
> > > -                                                MT_TOOL_FINGER, 0);
> > > +                     input_mt_report_slot_state(input_dev, 0, 0);
> > >                       mxt_input_sync(data);
> > >               }
> > >
> > > @@ -855,7 +854,7 @@ static void mxt_proc_t9_message(struct mxt_data *data, u8 *message)
> > >               input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, area);
> > >       } else {
> > >               /* Touch no longer active, close out slot */
> > > -             input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, 0);
> > > +             input_mt_report_slot_state(input_dev, 0, 0);
> > >       }
> > >
> > >       data->update_input = true;
> > > --
> > > 2.17.1
> > >
> >
> > --
> > Dmitry
> --
> 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 v1 09/10] Input: atmel_mxt_ts - tool type is ignored when slot is closed

[Dmitry Torokhov]

On Wed, Jul 25, 2018 at 03:26:41PM +1000, Peter Hutterer wrote:
> On Tue, Jul 24, 2018 at 10:23:27AM +0200, Benjamin Tissoires wrote:
> > On Tue, Jul 24, 2018 at 12:34 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Fri, Jul 20, 2018 at 10:51:21PM +0100, Nick Dyer wrote:
> > > > From: Nick Dyer <nick.dyer@itdev.co.uk>
> > > >
> > > > input_mt_report_slot_state() ignores the tool when the slot is closed.
> > > > Remove the tool type from these function calls, which has caused a bit of
> > > > confusion.
> > >
> > > Hmm, maybe we could introduce MT_TOOL_NONE or MT_TOOL_INACTIVE and get
> > > rid of the 3rd parameter? It will require a bit of macro trickery for a
> > > release or 2...
> > 
> > I am not sure what would be the benefit of adding those new tools, if
> > the input_mt code discards them. Do you want to forward them to the
> > userspace with the release?
> > This reminds me the discussion we had recently with the touchscreens
> > releasing the slots with a MT_TOOL_PALM.
> > 
> > Anyway, better include Peter as he will be using this new MT_TOOL.
> 
> thanks for the CC, would've missed this.
> 
> From what I read this would be a helper for internal changes only, not
> exposed to userspace? If so maybe it's better/easier/more readable to break
> it into two functions
>   input_mt_open_slot(dev, MT_TOOL_FINGER)
>   input_mt_close_slot(dev)
>   
> This removes any ambiguity about the handling of the tool and should be a
> fairly trivial search/replace. Replace the 'open/close' terminology with
> whatever suits better.

Hmm, I do like the "input_mt_close_slot()", or
"input_mt_report_slot_inactive()". I think the
input_mt_report_slot_state() is fine for "opening" the slot, as, with it
now returning bool, we can do:

	if (input_mt_report_slot_state(dev, MT_TOOL_FINGER, state)) {
		...
		< report events for active slot >
	}


Thanks.

-- 
Dmitry

[PATCH v1 10/10] Input: atmel_mxt_ts - move completion to after config crc is updated

[Nick Dyer]

From: Nick Dyer <nick.dyer@itdev.co.uk>

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index c31af790ef84..9f296a66c94e 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -728,13 +728,13 @@ static void mxt_proc_t6_messages(struct mxt_data *data, u8 *msg)
 	u8 status = msg[1];
 	u32 crc = msg[2] | (msg[3] << 8) | (msg[4] << 16);
 
-	complete(&data->crc_completion);
-
 	if (crc != data->config_crc) {
 		data->config_crc = crc;
 		dev_dbg(dev, "T6 Config Checksum: 0x%06X\n", crc);
 	}
 
+	complete(&data->crc_completion);
+
 	/* Detect reset */
 	if (status & MXT_T6_STATUS_RESET)
 		complete(&data->reset_completion);
-- 
2.17.1

Re: [PATCH v1 01/10] Input: atmel_mxt_ts - only use first T9 instance

[Dmitry Torokhov]

On Fri, Jul 20, 2018 at 10:51:13PM +0100, Nick Dyer wrote:
> From: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> The driver only registers one input device, which uses the screen
> parameters from the first T9 instance. The first T63 instance also uses
> those parameters.
> 
> It is incorrect to send input reports from the second instances of these
> objects if they are enabled: the input scaling will be wrong and the
> positions will be mashed together.
> 
> This also causes problems on Android if the number of slots exceeds 32.
> 
> In the future, this could be handled by looking for enabled touch object
> instances and creating an input device for each one.
> 
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> Acked-by: Benson Leung <bleung@chromium.org>
> Acked-by: Yufeng Shen <miletus@chromium.org>
> ---

OK, I adjusted patch #7 to use kmemdup_nul() as we discussed, and
skipped #9, applied the rest.

Thanks!

>  drivers/input/touchscreen/atmel_mxt_ts.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 54fe190fd4bc..48c5ccab00a0 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1658,10 +1658,11 @@ static int mxt_parse_object_table(struct mxt_data *data,
>  			break;
>  		case MXT_TOUCH_MULTI_T9:
>  			data->multitouch = MXT_TOUCH_MULTI_T9;
> +			/* Only handle messages from first T9 instance */
>  			data->T9_reportid_min = min_id;
> -			data->T9_reportid_max = max_id;
> -			data->num_touchids = object->num_report_ids
> -						* mxt_obj_instances(object);
> +			data->T9_reportid_max = min_id +
> +						object->num_report_ids - 1;
> +			data->num_touchids = object->num_report_ids;
>  			break;
>  		case MXT_SPT_MESSAGECOUNT_T44:
>  			data->T44_address = object->start_address;
> -- 
> 2.17.1
> 

-- 
Dmitry