Re: [PATCH 1/2] Input: elan_i2c - Add ic type 0x11 0x13 0x14.
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2020-07-12 06:09:35
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Jingle Wu <redacted> --- drivers/input/mouse/elan_i2c.h | 6 +- drivers/input/mouse/elan_i2c_core.c | 52 ++++++++----- drivers/input/mouse/elan_i2c_i2c.c | 109 ++++++++++++++++++++++----- drivers/input/mouse/elan_i2c_smbus.c | 10 +-- 4 files changed, 134 insertions(+), 43 deletions(-)diff --git a/drivers/input/mouse/elan_i2c.h b/drivers/input/mouse/elan_i2c.h index a9074ac9364f..f28b747978f5 100644 --- a/drivers/input/mouse/elan_i2c.h +++ b/drivers/input/mouse/elan_i2c.h@@ -33,6 +33,8 @@ #define ETP_FW_IAP_PAGE_ERR (1 << 5) #define ETP_FW_IAP_INTF_ERR (1 << 4) #define ETP_FW_PAGE_SIZE 64 +#define ETP_FW_PAGE_SIZE_128 128 +#define ETP_FW_PAGE_SIZE_512 512 #define ETP_FW_SIGNATURE_SIZE 6 struct i2c_client;@@ -72,8 +74,8 @@ struct elan_transport_ops { int (*iap_get_mode)(struct i2c_client *client, enum tp_mode *mode); int (*iap_reset)(struct i2c_client *client); - int (*prepare_fw_update)(struct i2c_client *client); - int (*write_fw_block)(struct i2c_client *client, + int (*prepare_fw_update)(struct i2c_client *client, u16 ic_type); + int (*write_fw_block)(struct i2c_client *client, u16 fw_page_size, const u8 *page, u16 checksum, int idx); int (*finish_fw_update)(struct i2c_client *client, struct completion *reset_done);diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c index 3f9354baac4b..0703f7d0d02d 100644 --- a/drivers/input/mouse/elan_i2c_core.c +++ b/drivers/input/mouse/elan_i2c_core.c@@ -89,7 +89,8 @@ struct elan_tp_data { u8 mode; u16 ic_type; u16 fw_validpage_count; - u16 fw_signature_address; + u16 fw_page_size; + u32 fw_signature_address; bool irq_wake;@@ -100,8 +101,10 @@ struct elan_tp_data { bool middle_button; }; -static int elan_get_fwinfo(u16 ic_type, u16 *validpage_count, - u16 *signature_address) +static int elan_get_fwinfo(u16 ic_type, u8 iap_version, + u16 *validpage_count, + u32 *signature_address, + u16 *page_size) { switch (ic_type) { case 0x00:@@ -124,18 +127,34 @@ static int elan_get_fwinfo(u16 ic_type, u16 *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.
quoted hunk ↗ jump to hunk
+ } else if ((ic_type >= 0x0D) && (iap_version >= 1)) { + *validpage_count /= 2; + *page_size = ETP_FW_PAGE_SIZE_128; + } else + *page_size = ETP_FW_PAGE_SIZE; return 0; }@@ -312,7 +331,6 @@ static int elan_initialize(struct elan_tp_data *data) static int elan_query_device_info(struct elan_tp_data *data) { int error; - u16 ic_type; error = data->ops->get_version(data->client, false, &data->fw_version); if (error)@@ -336,13 +354,10 @@ static int elan_query_device_info(struct 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.
quoted hunk ↗ jump to hunk
const u8 *page, u16 checksum, int idx) { int retry = ETP_RETRY_COUNT; int error; do { - error = data->ops->write_fw_block(data->client, + error = data->ops->write_fw_block(data->client, page_size, page, checksum, idx); if (!error) return 0;@@ -460,21 +475,22 @@ static int __elan_update_firmware(struct elan_tp_data *data, u16 boot_page_count; u16 sw_checksum = 0, fw_checksum = 0; - error = data->ops->prepare_fw_update(client); + error = data->ops->prepare_fw_update(client, data->ic_type); if (error) return error; iap_start_addr = get_unaligned_le16(&fw->data[ETP_IAP_START_ADDR * 2]); - boot_page_count = (iap_start_addr * 2) / ETP_FW_PAGE_SIZE; + boot_page_count = (iap_start_addr * 2) / data->fw_page_size; for (i = boot_page_count; i < data->fw_validpage_count; i++) { u16 checksum = 0; - const u8 *page = &fw->data[i * ETP_FW_PAGE_SIZE]; + const u8 *page = &fw->data[i * data->fw_page_size]; - for (j = 0; j < ETP_FW_PAGE_SIZE; j += 2) + for (j = 0; j < data->fw_page_size; j += 2) checksum += ((page[j + 1] << 8) | page[j]); - error = elan_write_fw_block(data, page, checksum, i); + error = elan_write_fw_block(data, data->fw_page_size, + page, checksum, i); if (error) { dev_err(dev, "write page %d fail: %d\n", i, error); return error;diff --git a/drivers/input/mouse/elan_i2c_i2c.c b/drivers/input/mouse/elan_i2c_i2c.c index 058b35b1f9a9..91ef0c7bda43 100644 --- a/drivers/input/mouse/elan_i2c_i2c.c +++ b/drivers/input/mouse/elan_i2c_i2c.c@@ -19,6 +19,7 @@ #include <linux/interrupt.h> #include <linux/jiffies.h> #include <linux/kernel.h> +#include <linux/slab.h> #include <linux/sched.h> #include <asm/unaligned.h>@@ -43,6 +44,7 @@ #define ETP_I2C_RESOLUTION_CMD 0x0108 #define ETP_I2C_PRESSURE_CMD 0x010A #define ETP_I2C_IAP_VERSION_CMD 0x0110 +#define ETP_I2C_IAP_VERSION_CMD_OLD 0x0111 #define ETP_I2C_SET_CMD 0x0300 #define ETP_I2C_POWER_CMD 0x0307 #define ETP_I2C_FW_CHECKSUM_CMD 0x030F@@ -53,6 +55,8 @@ #define ETP_I2C_CALIBRATE_CMD 0x0316 #define ETP_I2C_MAX_BASELINE_CMD 0x0317 #define ETP_I2C_MIN_BASELINE_CMD 0x0318 +#define ETP_I2C_IAP_TYPE_REG 0x0040 +#define ETP_I2C_IAP_TYPE_CMD 0x0304 #define ETP_I2C_REPORT_LEN 34 #define ETP_I2C_DESC_LENGTH 30@@ -249,7 +253,11 @@ static int elan_i2c_get_pattern(struct i2c_client *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.
quoted hunk ↗ jump to hunk
return 0; }@@ -259,6 +267,7 @@ static int elan_i2c_get_version(struct i2c_client *client, { int error; u8 pattern_ver; + u16 iap_cmd; u8 val[3]; error = elan_i2c_get_pattern(client, &pattern_ver);@@ -266,9 +275,14 @@ static int elan_i2c_get_version(struct i2c_client *client, dev_err(&client->dev, "failed to get pattern version\n"); return error; } + + if (pattern_ver >= 0x01) + iap_cmd = ETP_I2C_IAP_VERSION_CMD; + else + iap_cmd = ETP_I2C_IAP_VERSION_CMD_OLD;
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;
quoted hunk ↗ jump to hunk
error = elan_i2c_read_cmd(client, - iap ? ETP_I2C_IAP_VERSION_CMD : + iap ? iap_cmd : ETP_I2C_FW_VERSION_CMD, val); if (error) {@@ -277,7 +291,7 @@ static int elan_i2c_get_version(struct i2c_client *client, return error; } - if (pattern_ver == 0x01) + if (pattern_ver >= 0x01) *version = iap ? val[1] : val[0]; else *version = val[0];@@ -298,7 +312,7 @@ static int elan_i2c_get_sm_version(struct i2c_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); + if (error) { + dev_err(&client->dev, "failed to get ic type: %d\n", + error); + return error; + } + *ic_type = val[0]; error = elan_i2c_read_cmd(client, ETP_I2C_NSM_VERSION_CMD, val);@@ -507,13 +528,53 @@ static int elan_i2c_set_flash_key(struct i2c_client *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?
quoted hunk ↗ jump to hunk
/* 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?
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
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); - return error; + goto exit; } result = le16_to_cpup((__le16 *)val); if (result & (ETP_FW_IAP_PAGE_ERR | ETP_FW_IAP_INTF_ERR)) { dev_err(dev, "IAP reports failed write: %04hx\n", result); - return -EIO; + error = -EIO; + goto exit; } - return 0; +exit: + kfree(page_store); + return error; } static int elan_i2c_finish_fw_update(struct i2c_client *client,diff --git a/drivers/input/mouse/elan_i2c_smbus.c b/drivers/input/mouse/elan_i2c_smbus.c index 8c3185d54c73..1e762c90c894 100644 --- a/drivers/input/mouse/elan_i2c_smbus.c +++ b/drivers/input/mouse/elan_i2c_smbus.c@@ -340,7 +340,7 @@ static int elan_smbus_set_flash_key(struct i2c_client *client) return 0; } -static int elan_smbus_prepare_fw_update(struct i2c_client *client) +static int elan_smbus_prepare_fw_update(struct i2c_client *client, u16 ic_type) { struct device *dev = &client->dev; int len;@@ -414,7 +414,7 @@ static int elan_smbus_prepare_fw_update(struct i2c_client *client) } -static int elan_smbus_write_fw_block(struct i2c_client *client, +static int elan_smbus_write_fw_block(struct i2c_client *client, u16 fw_page_size, const u8 *page, u16 checksum, int idx) { struct device *dev = &client->dev;@@ -429,7 +429,7 @@ 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, + 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