Hi Dmitry:

Could you please tell me what devices this patch was tested with? I want to
be sure we are not only adding support for newer devices, but also not
breaking existing ones.
--> The device which this patch was tested with was "coral".
--> Its corresponding ic_type was 0xD, 0xE, 0x10, 0x11, 0x13 0x14.

Also, could you please split this patch in 2: one adding variable length
firmware page size support, and another adding the new IC types.
--> As a result, Do I need to up-stream in total 3 patches?

THANKS

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] 
Sent: Sunday, July 12, 2020 2:09 PM
To: Jingle Wu
Cc: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org;
phoenix@emc.com.tw; josh.chen@emc.com.tw; dave.wang@emc.com.tw
Subject: Re: [PATCH 1/2] Input: elan_i2c - Add ic type 0x11 0x13 0x14.

Hi Jingle,

On Fri, Jul 10, 2020 at 01:40:47AM -0400, Jingle Wu wrote:

Modify the iap method for all IC.
Get the correct value of ic_type for old and new pattern of firmware.

Could you please tell me what devices this patch was tested with? I want to
be sure we are not only adding support for newer devices, but also not
breaking existing ones.

Also, could you please split this patch in 2: one adding variable length
firmware page size support, and another adding the new IC types.

*validpage_count,

 		*validpage_count = 640;
 		break;
 	case 0x10:
+	case 0x14:
 		*validpage_count = 1024;
 		break;

I would prefer if we listed 0x14 separately below.

+	case 0x11:
+		*validpage_count = 1280;
+		break;
+	case 0x13:
+		*validpage_count = 2048;
+		break;
 	default:
 		/* unknown ic type clear value */
 		*validpage_count = 0;
 		*signature_address = 0;
+		*page_size = 0;
 		return -ENXIO;
 	}
 
 	*signature_address =
 		(*validpage_count * ETP_FW_PAGE_SIZE) -

ETP_FW_SIGNATURE_SIZE;

 
+	if ((ic_type == 0x14) && (iap_version >= 2)) {
+		*validpage_count /= 8;
+		*page_size = ETP_FW_PAGE_SIZE_512;

Do we really need to check ic_type here in addition to iap_version?
I.e. IC 0x11 - can it have IAP 2? And will it be using 512 or 128 byte
pages? In other words I am trying to understand if we could determine page
size solely by IAP version.

&data->fw_version);

elan_tp_data *data)

 	if (error)
 		return error;
 
-	if (data->pattern == 0x01)
-		ic_type = data->ic_type;
-	else
-		ic_type = data->iap_version;
-
-	error = elan_get_fwinfo(ic_type, &data->fw_validpage_count,
-				&data->fw_signature_address);
+	error = elan_get_fwinfo(data->ic_type, data->iap_version, 
+				&data->fw_validpage_count,
+				&data->fw_signature_address,
+				&data->fw_page_size);
 	if (error)
 		dev_warn(&data->client->dev,
 			 "unexpected iap version %#04x (ic type: %#04x),

firmware update 

will not work\n", @@ -430,14 +445,14 @@ static int

elan_query_device_parameters(struct elan_tp_data *data)

  * IAP firmware updater related routines
  **********************************************************
  */
-static int elan_write_fw_block(struct elan_tp_data *data,
+static int elan_write_fw_block(struct elan_tp_data *data, u16 
+page_size,

I think we have data->fw_page_size, we do not need to pass it separately
here.

elan_tp_data *data,

*client, u8 *pattern)

 		dev_err(&client->dev, "failed to get pattern: %d\n", error);
 		return error;
 	}
-	*pattern = val[1];
+
+	if ((val[0] == 0xFF) && (val[1] == 0xFF))
+		*pattern = 0;
+	else
+		*pattern = val[1];

This needs a comment explaining why we are doing it.

Can we make it

	if (!iap)
		cmd = ETP_I2C_FW_VERSION_CMD;
	else if (pattern_ver == 0)
		cmd = ETP_I2C_IAP_VERSION_CMD_OLD;
	else
		cmd = ETP_I2C_IAP_VERSION_CMD;

*client,

*client,

 		return error;
 	}
 
-	if (pattern_ver == 0x01) {
+	if (pattern_ver >= 0x01) {
 		error = elan_i2c_read_cmd(client, ETP_I2C_IC_TYPE_CMD, val);
 		if (error) {
 			dev_err(&client->dev, "failed to get ic type: %d\n",

@@ -324,7 

+338,14 @@ static int elan_i2c_get_sm_version(struct i2c_client *client,
 			return error;
 		}
 		*version = val[0];
-		*ic_type = val[1];
+
+		error = elan_i2c_read_cmd(client, ETP_I2C_IAP_VERSION_CMD,

val);

*client)

 	return 0;
 }
 
-static int elan_i2c_prepare_fw_update(struct i2c_client *client)
+static int elan_read_write_iap_type(struct i2c_client *client) {
+	int error;
+	u16 constant;
+	u8 val[3];
+	int retry = 3;
+
+	do {
+		error = elan_i2c_write_cmd(client, ETP_I2C_IAP_TYPE_CMD,
+				ETP_I2C_IAP_TYPE_REG);
+		if (error) {
+			dev_err(&client->dev,
+					"cannot write iap type: %d\n",

error);

+			return error;
+		}
+
+		error = elan_i2c_read_cmd(client, ETP_I2C_IAP_TYPE_CMD,

val);

+		if (error) {
+			dev_err(&client->dev,
+				"failed to read iap type register: %d\n",
+				error);
+			return error;
+		}
+		constant = le16_to_cpup((__le16 *)val);
+		dev_dbg(&client->dev, "iap type reg: 0x%04x.\n", constant);
+
+		if (constant == ETP_I2C_IAP_TYPE_REG)
+			return 0;
+
+	} while (--retry > 0);
+
+	dev_err(&client->dev, "cannot set iap type.\n");
+	return -EIO;
+}
+
+static int elan_i2c_prepare_fw_update(struct i2c_client *client, u16 
+ic_type)
 {
 	struct device *dev = &client->dev;
 	int error;
 	enum tp_mode mode;
 	u8 val[3];
 	u16 password;
+	u8 iap_version;
+	
+	error = elan_i2c_get_version(client, true, &iap_version);
+	if (error)
+		return error;

Haven't we queried IAP version already? Can we pass it in, along with
ic_type?

 
 	/* Get FW in which mode	(IAP_MODE/MAIN_MODE)  */
 	error = elan_i2c_iap_get_mode(client, &mode); @@ -546,6 +607,12 @@ 
static int elan_i2c_prepare_fw_update(struct i2c_client *client)
 		dev_err(dev, "wrong mode: %d\n", mode);
 		return -EIO;
 	}
+	
+	if ((ic_type >= 0x0D) && (iap_version >= 1)) {

No need for parenthesis around simple comparisons. Also, do we really need
to check both ic type and iap version? Is there ICs less than 0x0d with
newer IAP versions?

+		error = elan_read_write_iap_type(client);
+		if (error)
+			return error;
+	}
 
 	/* Set flash key again */
 	error = elan_i2c_set_flash_key(client); @@ -572,45 +639,51 @@ static

int elan_i2c_prepare_fw_update(struct i2c_client *client)
 	return 0;
 }
 
-static int elan_i2c_write_fw_block(struct i2c_client *client,
+static int elan_i2c_write_fw_block(struct i2c_client *client, u16 
+fw_page_size,
 				   const u8 *page, u16 checksum, int idx)  {
 	struct device *dev = &client->dev;
-	u8 page_store[ETP_FW_PAGE_SIZE + 4];
+	u8 *page_store = kcalloc(1, fw_page_size + 4, GFP_KERNEL);

Why kcalloc()? kzalloc() if you want cleared memoryi, but I think kmalloc
would be fine.

 	u8 val[3];
 	u16 result;
-	int ret, error;
+	int ret, error = 0;
 
 	page_store[0] = ETP_I2C_IAP_REG_L;
 	page_store[1] = ETP_I2C_IAP_REG_H;
-	memcpy(&page_store[2], page, ETP_FW_PAGE_SIZE);
+	memcpy(&page_store[2], page, fw_page_size);
 	/* recode checksum at last two bytes */
-	put_unaligned_le16(checksum, &page_store[ETP_FW_PAGE_SIZE + 2]);
+	put_unaligned_le16(checksum, &page_store[fw_page_size + 2]);
 
-	ret = i2c_master_send(client, page_store, sizeof(page_store));
-	if (ret != sizeof(page_store)) {
+	ret = i2c_master_send(client, page_store, fw_page_size + 4);
+	if (ret != fw_page_size + 4) {
 		error = ret < 0 ? ret : -EIO;
 		dev_err(dev, "Failed to write page %d: %d\n", idx, error);
-		return error;
+		goto exit;
 	}
 
 	/* Wait for F/W to update one page ROM data. */
-	msleep(35);
+	if (fw_page_size == ETP_FW_PAGE_SIZE_512)
+		msleep(50);
+	else
+		msleep(35);
 
 	error = elan_i2c_read_cmd(client, ETP_I2C_IAP_CTRL_CMD, val);
 	if (error) {
 		dev_err(dev, "Failed to read IAP write result: %d\n",

error);

*client)

{

*client,

 	 */
 	error = i2c_smbus_write_block_data(client,
 					   ETP_SMBUS_WRITE_FW_BLOCK,
-					   ETP_FW_PAGE_SIZE / 2,
+					   fw_page_size / 2,
 					   page);
 	if (error) {
 		dev_err(dev, "Failed to write page %d (part %d): %d\n", @@

-439,8 

+439,8 @@ static int elan_smbus_write_fw_block(struct i2c_client 
*client,
 
 	error = i2c_smbus_write_block_data(client,
 					   ETP_SMBUS_WRITE_FW_BLOCK,
-					   ETP_FW_PAGE_SIZE / 2,
-					   page + ETP_FW_PAGE_SIZE / 2);
+					   fw_page_size / 2,
+					   page + fw_page_size / 2);
 	if (error) {
 		dev_err(dev, "Failed to write page %d (part %d): %d\n",
 			idx, 2, error);
--
2.17.1

Thanks.

-- 
Dmitry