Input: elan_i2c - Reduce the resume time for new devices

Commit Message

Jingle.Wu Feb. 26, 2021, 7:35 a.m. UTC

Remove elan_initilize() function at resume state,
for Voxel, Delbin, Magple, Bobba and new devices.

Signed-off-by: Jingle Wu <[e-mail]>
---
 drivers/input/mouse/elan_i2c.h      |  5 +++
 drivers/input/mouse/elan_i2c_core.c | 57 +++++++++++++++++++++++++++--
 2 files changed, 58 insertions(+), 4 deletions(-)

Comments

Dmitry Torokhov March 1, 2021, 5:31 a.m. UTC | #1

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.

Jingle.Wu March 2, 2021, 1:04 a.m. UTC | #2

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 <[e-mail]>
To:jingle.wu <[e-mail]>
Cc:[e-mail],[e-mail],[e-mail],[e-mail],[e-mail]
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.

Dmitry Torokhov March 5, 2021, 12:55 a.m. UTC | #3

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.

Jingle.Wu March 5, 2021, 1:24 a.m. UTC | #4

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:[e-mail]] 
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

Dmitry Torokhov March 5, 2021, 1:31 a.m. UTC | #5

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.

Jingle.Wu March 5, 2021, 1:50 a.m. UTC | #6

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:[e-mail]] 
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

Dmitry Torokhov March 8, 2021, 3:18 a.m. UTC | #7

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.

Jingle.Wu March 8, 2021, 8:56 a.m. UTC | #8

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:[e-mail]] 
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 <[e-mail]>

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 <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]
Patchwork-Id: 12105967
Signed-off-by: Dmitry Torokhov <[e-mail]>
---
 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);

Dmitry Torokhov March 9, 2021, 1:37 a.m. UTC | #9

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.

Dan Carpenter March 9, 2021, 6:29 a.m. UTC | #10

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 <[e-mail]>
Reported-by: Dan Carpenter <[e-mail]>

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/[e-mail]

Jingle.Wu March 9, 2021, 2:53 p.m. UTC | #11

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:[e-mail]] 
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

Dmitry Torokhov March 10, 2021, 5:46 a.m. UTC | #12

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.

Patch

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,