| Message ID | 20210226073537.4926-1-jingle.wu@emc.com.tw |
|---|---|
| State | New |
| Headers | show |
| Series | Input: elan_i2c - Reduce the resume time for new devices | expand |
Hi Jingle, On Fri, Feb 26, 2021 at 03:35:37PM +0800, jingle.wu wrote: > @@ -273,10 +318,12 @@ static int __elan_initialize(struct elan_tp_data *data) > bool woken_up = false; > int error; > > - error = data->ops->initialize(client); > - if (error) { > - dev_err(&client->dev, "device initialize failed: %d\n", error); > - return error; > + if (!(data->quirks & ETP_QUIRK_SET_QUICK_WAKEUP_DEV)) { > + error = data->ops->initialize(client); > + if (error) { > + dev_err(&client->dev, "device initialize failed: %d\n", error); > + return error; > + } So data->ops->initialize(client) essentially performs reset of the controller (we may want to rename it even) and as far as I understand you would want to avoid resetting the controller on newer devices, right? My question is how behavior of older devices differ from the new ones (are they stay in "undefined" state at power up) and whether it is possible to determine if controller is in operating mode. For example, what would happen on older devices if we call elan_query_product() below without resetting the controller? I also think that while I can see us skipping reset in resume paths we probably want to keep it in probe as we really do not know the state of the device (was it powered up properly earlier, etc). > } > > error = elan_query_product(data); > @@ -366,6 +413,8 @@ static int elan_query_device_info(struct elan_tp_data *data) > if (error) > return error; > > + data->quirks = elan_i2c_lookup_quirk(data->ic_type, data->product_id); > + > error = elan_get_fwinfo(data->ic_type, data->iap_version, > &data->fw_validpage_count, > &data->fw_signature_address, > -- > 2.17.1 > Thanks. -- Dmitry
HI Dmitry: So data->ops->initialize(client) essentially performs reset of the controller (we may want to rename it even) and as far as I understand you would want to avoid resetting the controller on newer devices, right? -> YES My question is how behavior of older devices differ from the new ones (are they stay in "undefined" state at power up) and whether it is possible to determine if controller is in operating mode. For example, what would happen on older devices if we call elan_query_product() below without resetting the controller? -> But there may be other problems, because ELAN can't test all the older devices , -> so use quirk to divide this part. I also think that while I can see us skipping reset in resume paths we probably want to keep it in probe as we really do not know the state of the device (was it powered up properly earlier, etc). -> In this part, at PROBE state will be called data->ops->initialize(client) function. -> Because quirk's setting (data->quirks = elan_i2c_lookup_quirk(data->ic_type, data->product_id);) -> is after data->ops->initialize(client) and elan_query_product() function. THANKS JINGLE -----Original message----- From:Dmitry Torokhov <dmitry.torokhov@gmail.com> To:jingle.wu <jingle.wu@emc.com.tw> Cc:linux-kernel@vger.kernel.org,linux-input@vger.kernel.org,phoenix@emc.com.tw,dave.wang@emc.com.tw,josh.chen@emc.com.tw Date:Mon, 01 Mar 2021 13:31:31 Subject:Re: [PATCH] Input: elan_i2c - Reduce the resume time for new devices Hi Jingle, On Fri, Feb 26, 2021 at 03:35:37PM +0800, jingle.wu wrote: > @@ -273,10 +318,12 @@ static int __elan_initialize(struct elan_tp_data *data) > bool woken_up = false; > int error; > > - error = data->ops->initialize(client); > - if (error) { > - dev_err(&client->dev, "device initialize failed: %d\n", error); > - return error; > + if (!(data->quirks & ETP_QUIRK_SET_QUICK_WAKEUP_DEV)) { > + error = data->ops->initialize(client); > + if (error) { > + dev_err(&client->dev, "device initialize failed: %d\n", error); > + return error; > + } So data->ops->initialize(client) essentially performs reset of the controller (we may want to rename it even) and as far as I understand you would want to avoid resetting the controller on newer devices, right? My question is how behavior of older devices differ from the new ones (are they stay in "undefined" state at power up) and whether it is possible to determine if controller is in operating mode. For example, what would happen on older devices if we call elan_query_product() below without resetting the controller? I also think that while I can see us skipping reset in resume paths we probably want to keep it in probe as we really do not know the state of the device (was it powered up properly earlier, etc). > } > > error = elan_query_product(data); > @@ -366,6 +413,8 @@ static int elan_query_device_info(struct elan_tp_data *data) > if (error) > return error; > > + data->quirks = elan_i2c_lookup_quirk(data->ic_type, data->product_id); > + > error = elan_get_fwinfo(data->ic_type, data->iap_version, > &data->fw_validpage_count, > &data->fw_signature_address, > -- > 2.17.1 > Thanks.
Hi Jingle, On Tue, Mar 02, 2021 at 09:04:57AM +0800, jingle.wu wrote: > HI Dmitry: > > So data->ops->initialize(client) essentially performs reset of the > controller (we may want to rename it even) and as far as I understand > you would want to avoid resetting the controller on newer devices, > right? > > -> YES > > My question is how behavior of older devices differ from the new ones > (are they stay in "undefined" state at power up) and whether it is > possible to determine if controller is in operating mode. For example, > what would happen on older devices if we call elan_query_product() below > without resetting the controller? > > -> But there may be other problems, because ELAN can't test all the older devices , > -> so use quirk to divide this part. OK, but could you please tell me what exactly was changed in the newer parts behavior regarding need to reset after powering them on? Thanks. -- Dmitry
HI Dmitry: In this case (in the newer parts behavior regarding need to reset after powering them on), it is consistent with the original driver behavior with any new or old device (be called data->ops->initialize(client) : usleep(100) , etc.. , because this times "data->quirks" is equal 0 at probe state.) THANKS JINGLE -----Original Message----- From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] Sent: Friday, March 05, 2021 8:55 AM To: jingle.wu Cc: linux-kernel; linux-input; phoenix; dave.wang; josh.chen Subject: Re: [PATCH] Input: elan_i2c - Reduce the resume time for new dev ices Hi Jingle, On Tue, Mar 02, 2021 at 09:04:57AM +0800, jingle.wu wrote: > HI Dmitry: > > So data->ops->initialize(client) essentially performs reset of the > controller (we may want to rename it even) and as far as I understand > you would want to avoid resetting the controller on newer devices, > right? > > -> YES > > My question is how behavior of older devices differ from the new ones > (are they stay in "undefined" state at power up) and whether it is > possible to determine if controller is in operating mode. For example, > what would happen on older devices if we call elan_query_product() > below without resetting the controller? > > -> But there may be other problems, because ELAN can't test all the > -> older devices , so use quirk to divide this part. OK, but could you please tell me what exactly was changed in the newer parts behavior regarding need to reset after powering them on? Thanks. -- Dmitry
Hi Jingle, On Fri, Mar 05, 2021 at 09:24:05AM +0800, jingle wrote: > HI Dmitry: > > In this case (in the newer parts behavior regarding need to reset after > powering them on), it is consistent with the original driver behavior with > any new or old device > (be called data->ops->initialize(client) : usleep(100) , etc.. , because > this times "data->quirks" is equal 0 at probe state.) You misunderstood my question. I was asking what specifically, if anything, was changed in the firmware to allow skipping reset/sleep part of device initialization on newer parts during resume process. Because of there were no specific changes I would say let's not do a quirk and change the driver to skip reset on resume. Thanks. -- Dmitry
HI Dmitry: 1. You mean to let all devices ignore skipping reset/sleep part of device initialization? 2. The test team found that some old firmware will have errors (invalid report etc...), so ELAN can only ensure that the new device can meet the newer parts. Thanks jingle -----Original Message----- From: 'Dmitry Torokhov' [mailto:dmitry.torokhov@gmail.com] Sent: Friday, March 05, 2021 9:31 AM To: jingle Cc: 'linux-kernel'; 'linux-input'; 'phoenix'; 'dave.wang'; 'josh.chen' Subject: Re: [PATCH] Input: elan_i2c - Reduce the resume time for new dev ices Hi Jingle, On Fri, Mar 05, 2021 at 09:24:05AM +0800, jingle wrote: > HI Dmitry: > > In this case (in the newer parts behavior regarding need to reset > after powering them on), it is consistent with the original driver > behavior with any new or old device (be called > data->ops->initialize(client) : usleep(100) , etc.. , because this > times "data->quirks" is equal 0 at probe state.) You misunderstood my question. I was asking what specifically, if anything, was changed in the firmware to allow skipping reset/sleep part of device initialization on newer parts during resume process. Because of there were no specific changes I would say let's not do a quirk and change the driver to skip reset on resume. Thanks. -- Dmitry
Hi Jingle, On Fri, Mar 05, 2021 at 09:50:35AM +0800, jingle wrote: > HI Dmitry: > > 1. You mean to let all devices ignore skipping reset/sleep part of device > initialization? > 2. The test team found that some old firmware will have errors (invalid > report etc...), so ELAN can only ensure that the new device can meet the > newer parts. I see. OK, fair enough. I would prefer if we were more explicit about when we skip resetting the device, what do you think about the version of your patch below? Thanks. -- Dmitry Input: elan_i2c - reduce the resume time for new devices From: Jingle Wu <jingle.wu@emc.com.tw> Newer controllers, such as Voxel, Delbin, Magple, Bobba and others, do not need to be reset after issuing power-on command, and skipping reset saves at least 100ms from resume time. Note that if first attempt of re-initializing device fails we will not be skipping reset on the subsequent ones. Signed-off-by: Jingle Wu <jingle.wu@emc.com.tw> Link: https://lore.kernel.org/r/20210226073537.4926-1-jingle.wu@emc.com.tw Patchwork-Id: 12105967 Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/mouse/elan_i2c.h | 5 +++ drivers/input/mouse/elan_i2c_core.c | 58 +++++++++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/drivers/input/mouse/elan_i2c.h b/drivers/input/mouse/elan_i2c.h index e12da5b024b0..838b3b346316 100644 --- a/drivers/input/mouse/elan_i2c.h +++ b/drivers/input/mouse/elan_i2c.h @@ -55,6 +55,11 @@ #define ETP_FW_PAGE_SIZE_512 512 #define ETP_FW_SIGNATURE_SIZE 6 +#define ETP_PRODUCT_ID_DELBIN 0x00C2 +#define ETP_PRODUCT_ID_VOXEL 0x00BF +#define ETP_PRODUCT_ID_MAGPIE 0x0120 +#define ETP_PRODUCT_ID_BOBBA 0x0121 + struct i2c_client; struct completion; diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c index bef73822315d..51a65f6bf1e3 100644 --- a/drivers/input/mouse/elan_i2c_core.c +++ b/drivers/input/mouse/elan_i2c_core.c @@ -46,6 +46,9 @@ #define ETP_FINGER_WIDTH 15 #define ETP_RETRY_COUNT 3 +/* quirks to control the device */ +#define ETP_QUIRK_QUICK_WAKEUP BIT(0) + /* The main device structure */ struct elan_tp_data { struct i2c_client *client; @@ -90,8 +93,38 @@ struct elan_tp_data { bool baseline_ready; u8 clickpad; bool middle_button; + + u32 quirks; /* Various quirks */ }; +static u32 elan_i2c_lookup_quirks(u16 ic_type, u16 product_id) +{ + static const struct { + u16 ic_type; + u16 product_id; + u32 quirks; + } elan_i2c_quirks[] = { + { 0x0D, ETP_PRODUCT_ID_DELBIN, ETP_QUIRK_QUICK_WAKEUP }, + { 0x10, ETP_PRODUCT_ID_VOXEL, ETP_QUIRK_QUICK_WAKEUP }, + { 0x14, ETP_PRODUCT_ID_MAGPIE, ETP_QUIRK_QUICK_WAKEUP }, + { 0x14, ETP_PRODUCT_ID_BOBBA, ETP_QUIRK_QUICK_WAKEUP }, + }; + u32 quirks = 0; + int i; + + for (i = 0; ARRAY_SIZE(elan_i2c_quirks); i++) { + if (elan_i2c_quirks[i].ic_type == ic_type && + elan_i2c_quirks[i].product_id == product_id) { + quirks = elan_i2c_quirks[i].quirks; + } + } + + if (ic_type >= 0x0D && product_id >= 0x123) + quirks |= ETP_QUIRK_QUICK_WAKEUP; + + return quirks; +} + static int elan_get_fwinfo(u16 ic_type, u8 iap_version, u16 *validpage_count, u32 *signature_address, u16 *page_size) { @@ -258,16 +291,18 @@ static int elan_check_ASUS_special_fw(struct elan_tp_data *data) return false; } -static int __elan_initialize(struct elan_tp_data *data) +static int __elan_initialize(struct elan_tp_data *data, bool skip_reset) { struct i2c_client *client = data->client; bool woken_up = false; int error; - error = data->ops->initialize(client); - if (error) { - dev_err(&client->dev, "device initialize failed: %d\n", error); - return error; + if (!skip_reset) { + error = data->ops->initialize(client); + if (error) { + dev_err(&client->dev, "device initialize failed: %d\n", error); + return error; + } } error = elan_query_product(data); @@ -311,16 +346,17 @@ static int __elan_initialize(struct elan_tp_data *data) return 0; } -static int elan_initialize(struct elan_tp_data *data) +static int elan_initialize(struct elan_tp_data *data, bool skip_reset) { int repeat = ETP_RETRY_COUNT; int error; do { - error = __elan_initialize(data); + error = __elan_initialize(data, skip_reset); if (!error) return 0; + skip_reset = false; msleep(30); } while (--repeat > 0); @@ -357,6 +393,8 @@ static int elan_query_device_info(struct elan_tp_data *data) if (error) return error; + data->quirks = elan_i2c_lookup_quirks(data->ic_type, data->product_id); + error = elan_get_fwinfo(data->ic_type, data->iap_version, &data->fw_validpage_count, &data->fw_signature_address, @@ -546,7 +584,7 @@ static int elan_update_firmware(struct elan_tp_data *data, data->ops->iap_reset(client); } else { /* Reinitialize TP after fw is updated */ - elan_initialize(data); + elan_initialize(data, false); elan_query_device_info(data); } @@ -1247,7 +1285,7 @@ static int elan_probe(struct i2c_client *client, } /* Initialize the touchpad. */ - error = elan_initialize(data); + error = elan_initialize(data, false); if (error) return error; @@ -1384,7 +1422,7 @@ static int __maybe_unused elan_resume(struct device *dev) goto err; } - error = elan_initialize(data); + error = elan_initialize(data, data->quirks & ETP_QUIRK_QUICK_WAKEUP); if (error) dev_err(dev, "initialize when resuming failed: %d\n", error);
Hi Dmitry:
1. missing "i<"
+ u32 quirks = 0;
+ int i;
+
+ for (i = 0; ARRAY_SIZE(elan_i2c_quirks); i++) {
-> for (i = 0; i<ARRAY_SIZE(elan_i2c_quirks); i++) {
2. elan_resume () funtion are different with at Chromeos driver.
@@ -1384,7 +1422,7 @@ static int __maybe_unused elan_resume(struct device
*dev)
goto err;
}
- error = elan_initialize(data);
+ error = elan_initialize(data, data->quirks &
ETP_QUIRK_QUICK_WAKEUP);
if (error)
dev_err(dev, "initialize when resuming failed: %d\n",
error);
-> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ref
-> s/heads/chromeos-5.4/drivers/input/mouse/elan_i2c_core.c#1434
-> error = elan_initialize(data); this code is in elan_reactivate()
function at Chromeos driver.
-> Will this change affect cherrypick from linux kernel to chromeos?
THANKS
JINGLE
-----Original Message-----
From: 'Dmitry Torokhov' [mailto:dmitry.torokhov@gmail.com]
Sent: Monday, March 08, 2021 11:18 AM
To: jingle
Cc: 'linux-kernel'; 'linux-input'; 'phoenix'; 'dave.wang'; 'josh.chen'
Subject: Re: [PATCH] Input: elan_i2c - Reduce the resume time for new dev
ices
Hi Jingle,
On Fri, Mar 05, 2021 at 09:50:35AM +0800, jingle wrote:
> HI Dmitry:
>
> 1. You mean to let all devices ignore skipping reset/sleep part of
> device initialization?
> 2. The test team found that some old firmware will have errors
> (invalid report etc...), so ELAN can only ensure that the new device
> can meet the newer parts.
I see. OK, fair enough.
I would prefer if we were more explicit about when we skip resetting the
device, what do you think about the version of your patch below?
Thanks.
--
Dmitry
Input: elan_i2c - reduce the resume time for new devices
From: Jingle Wu <jingle.wu@emc.com.tw>
Newer controllers, such as Voxel, Delbin, Magple, Bobba and others, do not
need to be reset after issuing power-on command, and skipping reset saves
at least 100ms from resume time.
Note that if first attempt of re-initializing device fails we will not be
skipping reset on the subsequent ones.
Signed-off-by: Jingle Wu <jingle.wu@emc.com.tw>
Link: https://lore.kernel.org/r/20210226073537.4926-1-jingle.wu@emc.com.tw
Patchwork-Id: 12105967
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/mouse/elan_i2c.h | 5 +++
drivers/input/mouse/elan_i2c_core.c | 58
+++++++++++++++++++++++++++++------
2 files changed, 53 insertions(+), 10 deletions(-)
diff --git a/drivers/input/mouse/elan_i2c.h b/drivers/input/mouse/elan_i2c.h
index e12da5b024b0..838b3b346316 100644
--- a/drivers/input/mouse/elan_i2c.h
+++ b/drivers/input/mouse/elan_i2c.h
@@ -55,6 +55,11 @@
#define ETP_FW_PAGE_SIZE_512 512
#define ETP_FW_SIGNATURE_SIZE 6
+#define ETP_PRODUCT_ID_DELBIN 0x00C2
+#define ETP_PRODUCT_ID_VOXEL 0x00BF
+#define ETP_PRODUCT_ID_MAGPIE 0x0120
+#define ETP_PRODUCT_ID_BOBBA 0x0121
+
struct i2c_client;
struct completion;
diff --git a/drivers/input/mouse/elan_i2c_core.c
b/drivers/input/mouse/elan_i2c_core.c
index bef73822315d..51a65f6bf1e3 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -46,6 +46,9 @@
#define ETP_FINGER_WIDTH 15
#define ETP_RETRY_COUNT 3
+/* quirks to control the device */
+#define ETP_QUIRK_QUICK_WAKEUP BIT(0)
+
/* The main device structure */
struct elan_tp_data {
struct i2c_client *client;
@@ -90,8 +93,38 @@ struct elan_tp_data {
bool baseline_ready;
u8 clickpad;
bool middle_button;
+
+ u32 quirks; /* Various quirks */
};
+static u32 elan_i2c_lookup_quirks(u16 ic_type, u16 product_id)
+{
+ static const struct {
+ u16 ic_type;
+ u16 product_id;
+ u32 quirks;
+ } elan_i2c_quirks[] = {
+ { 0x0D, ETP_PRODUCT_ID_DELBIN, ETP_QUIRK_QUICK_WAKEUP },
+ { 0x10, ETP_PRODUCT_ID_VOXEL, ETP_QUIRK_QUICK_WAKEUP },
+ { 0x14, ETP_PRODUCT_ID_MAGPIE, ETP_QUIRK_QUICK_WAKEUP },
+ { 0x14, ETP_PRODUCT_ID_BOBBA, ETP_QUIRK_QUICK_WAKEUP },
+ };
+ u32 quirks = 0;
+ int i;
+
+ for (i = 0; ARRAY_SIZE(elan_i2c_quirks); i++) {
+ if (elan_i2c_quirks[i].ic_type == ic_type &&
+ elan_i2c_quirks[i].product_id == product_id) {
+ quirks = elan_i2c_quirks[i].quirks;
+ }
+ }
+
+ if (ic_type >= 0x0D && product_id >= 0x123)
+ quirks |= ETP_QUIRK_QUICK_WAKEUP;
+
+ return quirks;
+}
+
static int elan_get_fwinfo(u16 ic_type, u8 iap_version, u16
*validpage_count,
u32 *signature_address, u16 *page_size)
{
@@ -258,16 +291,18 @@ static int elan_check_ASUS_special_fw(struct
elan_tp_data *data)
return false;
}
-static int __elan_initialize(struct elan_tp_data *data)
+static int __elan_initialize(struct elan_tp_data *data, bool skip_reset)
{
struct i2c_client *client = data->client;
bool woken_up = false;
int error;
- error = data->ops->initialize(client);
- if (error) {
- dev_err(&client->dev, "device initialize failed: %d\n",
error);
- return error;
+ if (!skip_reset) {
+ error = data->ops->initialize(client);
+ if (error) {
+ dev_err(&client->dev, "device initialize failed:
%d\n", error);
+ return error;
+ }
}
error = elan_query_product(data);
@@ -311,16 +346,17 @@ static int __elan_initialize(struct elan_tp_data
*data)
return 0;
}
-static int elan_initialize(struct elan_tp_data *data)
+static int elan_initialize(struct elan_tp_data *data, bool skip_reset)
{
int repeat = ETP_RETRY_COUNT;
int error;
do {
- error = __elan_initialize(data);
+ error = __elan_initialize(data, skip_reset);
if (!error)
return 0;
+ skip_reset = false;
msleep(30);
} while (--repeat > 0);
@@ -357,6 +393,8 @@ static int elan_query_device_info(struct elan_tp_data
*data)
if (error)
return error;
+ data->quirks = elan_i2c_lookup_quirks(data->ic_type,
data->product_id);
+
error = elan_get_fwinfo(data->ic_type, data->iap_version,
&data->fw_validpage_count,
&data->fw_signature_address,
@@ -546,7 +584,7 @@ static int elan_update_firmware(struct elan_tp_data
*data,
data->ops->iap_reset(client);
} else {
/* Reinitialize TP after fw is updated */
- elan_initialize(data);
+ elan_initialize(data, false);
elan_query_device_info(data);
}
@@ -1247,7 +1285,7 @@ static int elan_probe(struct i2c_client *client,
}
/* Initialize the touchpad. */
- error = elan_initialize(data);
+ error = elan_initialize(data, false);
if (error)
return error;
@@ -1384,7 +1422,7 @@ static int __maybe_unused elan_resume(struct device
*dev)
goto err;
}
- error = elan_initialize(data);
+ error = elan_initialize(data, data->quirks &
ETP_QUIRK_QUICK_WAKEUP);
if (error)
dev_err(dev, "initialize when resuming failed: %d\n",
error);
Hi Jingle, On Mon, Mar 08, 2021 at 04:56:14PM +0800, jingle wrote: > Hi Dmitry: > > 1. missing "i<" > + u32 quirks = 0; > + int i; > + > + for (i = 0; ARRAY_SIZE(elan_i2c_quirks); i++) { > > -> for (i = 0; i<ARRAY_SIZE(elan_i2c_quirks); i++) { Yes, you are right of course. Was this the only issue with the updated patch? Did it work for you otherwise? > > 2. elan_resume () funtion are different with at Chromeos driver. > @@ -1384,7 +1422,7 @@ static int __maybe_unused elan_resume(struct device > *dev) > goto err; > } > > - error = elan_initialize(data); > + error = elan_initialize(data, data->quirks & > ETP_QUIRK_QUICK_WAKEUP); > if (error) > dev_err(dev, "initialize when resuming failed: %d\n", > error); > > -> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ref > -> s/heads/chromeos-5.4/drivers/input/mouse/elan_i2c_core.c#1434 > -> error = elan_initialize(data); this code is in elan_reactivate() > function at Chromeos driver. > -> Will this change affect cherrypick from linux kernel to chromeos? Yes, we would need to adjust the patch for Chrome OS and have elan_reactivate() to call elan_initialize() with appropriate argument. Thanks. -- Dmitry
Hi 'Dmitry,
url: https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/Re-PATCH-Input-elan_i2c-Reduce-the-resume-time-for-new-dev-ices/20210308-111956
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: x86_64-randconfig-m001-20210308 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/input/mouse/elan_i2c_core.c:122 elan_i2c_lookup_quirks() warn: ignoring unreachable code.
vim +122 drivers/input/mouse/elan_i2c_core.c
5b1301343b8d29 Dmitry Torokhov 2021-03-07 100 static u32 elan_i2c_lookup_quirks(u16 ic_type, u16 product_id)
5b1301343b8d29 Dmitry Torokhov 2021-03-07 101 {
5b1301343b8d29 Dmitry Torokhov 2021-03-07 102 static const struct {
5b1301343b8d29 Dmitry Torokhov 2021-03-07 103 u16 ic_type;
5b1301343b8d29 Dmitry Torokhov 2021-03-07 104 u16 product_id;
5b1301343b8d29 Dmitry Torokhov 2021-03-07 105 u32 quirks;
5b1301343b8d29 Dmitry Torokhov 2021-03-07 106 } elan_i2c_quirks[] = {
5b1301343b8d29 Dmitry Torokhov 2021-03-07 107 { 0x0D, ETP_PRODUCT_ID_DELBIN, ETP_QUIRK_QUICK_WAKEUP },
5b1301343b8d29 Dmitry Torokhov 2021-03-07 108 { 0x10, ETP_PRODUCT_ID_VOXEL, ETP_QUIRK_QUICK_WAKEUP },
5b1301343b8d29 Dmitry Torokhov 2021-03-07 109 { 0x14, ETP_PRODUCT_ID_MAGPIE, ETP_QUIRK_QUICK_WAKEUP },
5b1301343b8d29 Dmitry Torokhov 2021-03-07 110 { 0x14, ETP_PRODUCT_ID_BOBBA, ETP_QUIRK_QUICK_WAKEUP },
6696777c6506fa Duson Lin 2014-10-03 111 };
5b1301343b8d29 Dmitry Torokhov 2021-03-07 112 u32 quirks = 0;
5b1301343b8d29 Dmitry Torokhov 2021-03-07 113 int i;
5b1301343b8d29 Dmitry Torokhov 2021-03-07 114
5b1301343b8d29 Dmitry Torokhov 2021-03-07 115 for (i = 0; ARRAY_SIZE(elan_i2c_quirks); i++) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^
The "i < " part of this condition is missing.
5b1301343b8d29 Dmitry Torokhov 2021-03-07 116 if (elan_i2c_quirks[i].ic_type == ic_type &&
5b1301343b8d29 Dmitry Torokhov 2021-03-07 117 elan_i2c_quirks[i].product_id == product_id) {
5b1301343b8d29 Dmitry Torokhov 2021-03-07 118 quirks = elan_i2c_quirks[i].quirks;
5b1301343b8d29 Dmitry Torokhov 2021-03-07 119 }
5b1301343b8d29 Dmitry Torokhov 2021-03-07 120 }
5b1301343b8d29 Dmitry Torokhov 2021-03-07 121
5b1301343b8d29 Dmitry Torokhov 2021-03-07 @122 if (ic_type >= 0x0D && product_id >= 0x123)
5b1301343b8d29 Dmitry Torokhov 2021-03-07 123 quirks |= ETP_QUIRK_QUICK_WAKEUP;
5b1301343b8d29 Dmitry Torokhov 2021-03-07 124
5b1301343b8d29 Dmitry Torokhov 2021-03-07 125 return quirks;
5b1301343b8d29 Dmitry Torokhov 2021-03-07 126 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Dmitry: Was this the only issue with the updated patch? Did it work for you otherwise? -> Yes, the updated patch can work successfully after fix this issue. THANKS JINGLE -----Original Message----- From: 'Dmitry Torokhov' [mailto:dmitry.torokhov@gmail.com] Sent: Tuesday, March 09, 2021 9:38 AM To: jingle Cc: 'linux-kernel'; 'linux-input'; 'phoenix'; 'dave.wang'; 'josh.chen' Subject: Re: [PATCH] Input: elan_i2c - Reduce the resume time for new dev ices Hi Jingle, On Mon, Mar 08, 2021 at 04:56:14PM +0800, jingle wrote: > Hi Dmitry: > > 1. missing "i<" > + u32 quirks = 0; > + int i; > + > + for (i = 0; ARRAY_SIZE(elan_i2c_quirks); i++) { > > -> for (i = 0; i<ARRAY_SIZE(elan_i2c_quirks); i++) { Yes, you are right of course. Was this the only issue with the updated patch? Did it work for you otherwise? > > 2. elan_resume () funtion are different with at Chromeos driver. > @@ -1384,7 +1422,7 @@ static int __maybe_unused elan_resume(struct > device > *dev) > goto err; > } > > - error = elan_initialize(data); > + error = elan_initialize(data, data->quirks & > ETP_QUIRK_QUICK_WAKEUP); > if (error) > dev_err(dev, "initialize when resuming failed: %d\n", error); > > -> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/r > -> ef > -> s/heads/chromeos-5.4/drivers/input/mouse/elan_i2c_core.c#1434 > -> error = elan_initialize(data); this code is in elan_reactivate() > function at Chromeos driver. > -> Will this change affect cherrypick from linux kernel to chromeos? Yes, we would need to adjust the patch for Chrome OS and have elan_reactivate() to call elan_initialize() with appropriate argument. Thanks. -- Dmitry
On Tue, Mar 09, 2021 at 10:53:34PM +0800, jingle.wu wrote: > Hi Dmitry: > > Was this the only issue with the updated patch? Did it work for you > otherwise? > -> Yes, the updated patch can work successfully after fix this issue. OK, great, I applied it. Thanks. -- Dmitry
diff --git a/drivers/input/mouse/elan_i2c.h b/drivers/input/mouse/elan_i2c.h index d5f9cd76eefb..16b795524179 100644 --- a/drivers/input/mouse/elan_i2c.h +++ b/drivers/input/mouse/elan_i2c.h @@ -45,6 +45,11 @@ #define ETP_FW_PAGE_SIZE_512 512 #define ETP_FW_SIGNATURE_SIZE 6 +#define ETP_PRODUCT_ID_DELBIN 0x00C2 +#define ETP_PRODUCT_ID_VOXEL 0x00BF +#define ETP_PRODUCT_ID_MAGPIE 0x0120 +#define ETP_PRODUCT_ID_BOBBA 0x0121 + struct i2c_client; struct completion; diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c index 0f46e2f6c9e8..e75bbaeaf068 100644 --- a/drivers/input/mouse/elan_i2c_core.c +++ b/drivers/input/mouse/elan_i2c_core.c @@ -55,6 +55,9 @@ #define ETP_MK_DATA_OFFSET 33 /* For high precision reports */ #define ETP_MAX_REPORT_LEN 39 +/* quirks to control the device */ +#define ETP_QUIRK_SET_QUICK_WAKEUP_DEV BIT(0) + /* The main device structure */ struct elan_tp_data { struct i2c_client *client; @@ -99,8 +102,50 @@ struct elan_tp_data { bool baseline_ready; u8 clickpad; bool middle_button; + + unsigned long quirks; /* Various quirks */ +}; + + +static const struct elan_i2c_quirks { + __u16 ic_type; + __u16 product_id; + __u32 quirks; +} elan_i2c_quirks[] = { + { 0x0D, ETP_PRODUCT_ID_DELBIN, + ETP_QUIRK_SET_QUICK_WAKEUP_DEV }, + { 0x10, ETP_PRODUCT_ID_VOXEL, + ETP_QUIRK_SET_QUICK_WAKEUP_DEV }, + { 0x14, ETP_PRODUCT_ID_MAGPIE, + ETP_QUIRK_SET_QUICK_WAKEUP_DEV }, + { 0x14, ETP_PRODUCT_ID_BOBBA, + ETP_QUIRK_SET_QUICK_WAKEUP_DEV }, + { 0, 0 } }; +/* + * elan_i2c_lookup_quirk: return any quirks associated with a elan i2c device + * @ic_type: the 16-bit ic type + * @product_id: the 16-bit product ID + * + * Returns: a u32 quirks value. + */ +static u32 elan_i2c_lookup_quirk(const u16 ic_type, const u16 product_id) +{ + u32 quirks = 0; + int n; + + for (n = 0; elan_i2c_quirks[n].ic_type; n++) + if (elan_i2c_quirks[n].ic_type == ic_type && + (elan_i2c_quirks[n].product_id == product_id)) + quirks = elan_i2c_quirks[n].quirks; + + if ((ic_type >= 0x0D) && (product_id >= 0x123)) + quirks |= ETP_QUIRK_SET_QUICK_WAKEUP_DEV; + + return quirks; +} + static int elan_get_fwinfo(u16 ic_type, u8 iap_version, u16 *validpage_count, u32 *signature_address, u16 *page_size) { @@ -273,10 +318,12 @@ static int __elan_initialize(struct elan_tp_data *data) bool woken_up = false; int error; - error = data->ops->initialize(client); - if (error) { - dev_err(&client->dev, "device initialize failed: %d\n", error); - return error; + if (!(data->quirks & ETP_QUIRK_SET_QUICK_WAKEUP_DEV)) { + error = data->ops->initialize(client); + if (error) { + dev_err(&client->dev, "device initialize failed: %d\n", error); + return error; + } } error = elan_query_product(data); @@ -366,6 +413,8 @@ static int elan_query_device_info(struct elan_tp_data *data) if (error) return error; + data->quirks = elan_i2c_lookup_quirk(data->ic_type, data->product_id); + error = elan_get_fwinfo(data->ic_type, data->iap_version, &data->fw_validpage_count, &data->fw_signature_address,
Remove elan_initilize() function at resume state, for Voxel, Delbin, Magple, Bobba and new devices. Signed-off-by: Jingle Wu <jingle.wu@emc.com.tw> --- drivers/input/mouse/elan_i2c.h | 5 +++ drivers/input/mouse/elan_i2c_core.c | 57 +++++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 4 deletions(-)