[PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

[PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

[Chris Chiu]

When performing a warm reboot from a system which does not correctly
support ELAN I2C touchpads, the touchpad will sometimes enter standard
mouse mode, cursor then never respond to touchpad event, and making the
driver discard the HID reports and flood dmesg with following error
messages.
"elan_i2c i2c-ELAN1000:00: invalid report id data (1)"

This change is from ELAN's correction. It needs 200ms delay before
set_mode() so that the mode setting will correctly take effect.

Signed-off-by: Chris Chiu <chiu@endlessm.com>
---
 drivers/input/mouse/elan_i2c_core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 2f58985..95080f9 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -210,18 +210,20 @@ static int __elan_initialize(struct elan_tp_data *data)
 		return error;
 	}
 
-	data->mode |= ETP_ENABLE_ABS;
-	error = data->ops->set_mode(client, data->mode);
+	error = data->ops->sleep_control(client, false);
 	if (error) {
 		dev_err(&client->dev,
-			"failed to switch to absolute mode: %d\n", error);
+			"failed to wake device up: %d\n", error);
 		return error;
 	}
 
-	error = data->ops->sleep_control(client, false);
+	msleep(200);
+
+	data->mode |= ETP_ENABLE_ABS;
+	error = data->ops->set_mode(client, data->mode);
 	if (error) {
 		dev_err(&client->dev,
-			"failed to wake device up: %d\n", error);
+			"failed to switch to absolute mode: %d\n", error);
 		return error;
 	}
 
-- 
2.1.4

Re: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

[Dmitry Torokhov]

On Tue, Jun 07, 2016 at 09:34:09PM +0800, Chris Chiu wrote:
> When performing a warm reboot from a system which does not correctly
> support ELAN I2C touchpads, the touchpad will sometimes enter standard
> mouse mode, cursor then never respond to touchpad event, and making the
> driver discard the HID reports and flood dmesg with following error
> messages.
> "elan_i2c i2c-ELAN1000:00: invalid report id data (1)"
> 
> This change is from ELAN's correction. It needs 200ms delay before
> set_mode() so that the mode setting will correctly take effect.

KT, is this feasible?

> 
> Signed-off-by: Chris Chiu <chiu@endlessm.com>
> ---
>  drivers/input/mouse/elan_i2c_core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index 2f58985..95080f9 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -210,18 +210,20 @@ static int __elan_initialize(struct elan_tp_data *data)
>  		return error;
>  	}
>  
> -	data->mode |= ETP_ENABLE_ABS;
> -	error = data->ops->set_mode(client, data->mode);
> +	error = data->ops->sleep_control(client, false);
>  	if (error) {
>  		dev_err(&client->dev,
> -			"failed to switch to absolute mode: %d\n", error);
> +			"failed to wake device up: %d\n", error);
>  		return error;
>  	}
>  
> -	error = data->ops->sleep_control(client, false);
> +	msleep(200);
> +
> +	data->mode |= ETP_ENABLE_ABS;
> +	error = data->ops->set_mode(client, data->mode);
>  	if (error) {
>  		dev_err(&client->dev,
> -			"failed to wake device up: %d\n", error);
> +			"failed to switch to absolute mode: %d\n", error);
>  		return error;
>  	}
>  
> -- 
> 2.1.4
> 

Thanks.

-- 
Dmitry

RE: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

[廖崇榮]

Hi Dmitry,

The modification from Chris is a special case.
Because the Touchpad FW is a little different from normal one, It cause
problem in Asus's OBE test. 

That's why Elan's driver use work-around to solve the problem. It's not
tested by other touchpad.

Let me discuss with internal FW team to confirm the harmless of the patch.

B.R  KT
-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] 
Sent: Tuesday, June 21, 2016 1:43 AM
To: Chris Chiu; kt.liao@emc.com.tw
Cc: Charlie Mooney; Michele Curti; Krzysztof Kozlowski; Benson Leung;
linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
linux@endlessm.com
Subject: Re: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS
mode

On Tue, Jun 07, 2016 at 09:34:09PM +0800, Chris Chiu wrote:
> When performing a warm reboot from a system which does not correctly 
> support ELAN I2C touchpads, the touchpad will sometimes enter standard 
> mouse mode, cursor then never respond to touchpad event, and making 
> the driver discard the HID reports and flood dmesg with following 
> error messages.
> "elan_i2c i2c-ELAN1000:00: invalid report id data (1)"
> 
> This change is from ELAN's correction. It needs 200ms delay before
> set_mode() so that the mode setting will correctly take effect.

KT, is this feasible?

> 
> Signed-off-by: Chris Chiu <chiu@endlessm.com>
> ---
>  drivers/input/mouse/elan_i2c_core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c 
> b/drivers/input/mouse/elan_i2c_core.c
> index 2f58985..95080f9 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -210,18 +210,20 @@ static int __elan_initialize(struct elan_tp_data
*data)
>  		return error;
>  	}
>  
> -	data->mode |= ETP_ENABLE_ABS;
> -	error = data->ops->set_mode(client, data->mode);
> +	error = data->ops->sleep_control(client, false);
>  	if (error) {
>  		dev_err(&client->dev,
> -			"failed to switch to absolute mode: %d\n", error);
> +			"failed to wake device up: %d\n", error);
>  		return error;
>  	}
>  
> -	error = data->ops->sleep_control(client, false);
> +	msleep(200);
> +
> +	data->mode |= ETP_ENABLE_ABS;
> +	error = data->ops->set_mode(client, data->mode);
>  	if (error) {
>  		dev_err(&client->dev,
> -			"failed to wake device up: %d\n", error);
> +			"failed to switch to absolute mode: %d\n", error);
>  		return error;
>  	}
>  
> --
> 2.1.4
> 

Thanks.

-- 
Dmitry

RE: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

[廖崇榮]

Hi Dmitry,

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] 
Sent: Tuesday, June 21, 2016 1:43 AM
To: Chris Chiu; kt.liao@emc.com.tw
Cc: Charlie Mooney; Michele Curti; Krzysztof Kozlowski; Benson Leung;
linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
linux@endlessm.com
Subject: Re: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS
mode

On Tue, Jun 07, 2016 at 09:34:09PM +0800, Chris Chiu wrote:
> When performing a warm reboot from a system which does not correctly 
> support ELAN I2C touchpads, the touchpad will sometimes enter standard 
> mouse mode, cursor then never respond to touchpad event, and making 
> the driver discard the HID reports and flood dmesg with following 
> error messages.
> "elan_i2c i2c-ELAN1000:00: invalid report id data (1)"
> 
> This change is from ELAN's correction. It needs 200ms delay before
> set_mode() so that the mode setting will correctly take effect.

KT, is this feasible?
[KT] After internal discussion, we don't agree this patch. 
    It's a work-around to fix firmware bug for specific touchpad and not
tested by other device.
    
> 
> Signed-off-by: Chris Chiu <chiu@endlessm.com>
> ---
>  drivers/input/mouse/elan_i2c_core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c 
> b/drivers/input/mouse/elan_i2c_core.c
> index 2f58985..95080f9 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -210,18 +210,20 @@ static int __elan_initialize(struct elan_tp_data
*data)
>  		return error;
>  	}
>  
> -	data->mode |= ETP_ENABLE_ABS;
> -	error = data->ops->set_mode(client, data->mode);
> +	error = data->ops->sleep_control(client, false);
>  	if (error) {
>  		dev_err(&client->dev,
> -			"failed to switch to absolute mode: %d\n", error);
> +			"failed to wake device up: %d\n", error);
>  		return error;
>  	}
>  
> -	error = data->ops->sleep_control(client, false);
> +	msleep(200);
> +
> +	data->mode |= ETP_ENABLE_ABS;
> +	error = data->ops->set_mode(client, data->mode);
>  	if (error) {
>  		dev_err(&client->dev,
> -			"failed to wake device up: %d\n", error);
> +			"failed to switch to absolute mode: %d\n", error);
>  		return error;
>  	}
>  
> --
> 2.1.4
> 

Thanks.

-- 
Dmitry

Re: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

[Daniel Drake]

On Tue, Jun 21, 2016 at 6:40 AM, 廖崇榮 <kt.liao@emc.com.tw> wrote:
> KT, is this feasible?
> [KT] After internal discussion, we don't agree this patch.
>     It's a work-around to fix firmware bug for specific touchpad and not
> tested by other device.

For better or worse, Linux often takes on the responsibility of
working around firmware bugs. This is a real issue that affects
multiple Asus laptops; you'll have no touchpad input upon reboot from
any OS that drives the touchpad in "generic hid" mode (e.g. older
version of Linux, some embedded OS, etc).

If the sleep is really that controversial, we could make it specific
to the ELAN1000 model that is the one in question here?

Thanks
Daniel

RE: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

[廖崇榮]

-----Original Message-----
From: Daniel Drake [mailto:drake@endlessm.com] 
Sent: Tuesday, June 21, 2016 10:42 PM
To: 廖崇榮
Cc: Dmitry Torokhov; Chris Chiu; Charlie Mooney; Michele Curti; Krzysztof Kozlowski; Benson Leung; linux-input@vger.kernel.org; Linux Kernel; Linux Upstreaming Team; 黃世鵬 經理
Subject: Re: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

On Tue, Jun 21, 2016 at 6:40 AM, 廖崇榮 <kt.liao@emc.com.tw> wrote:
> KT, is this feasible?
> [KT] After internal discussion, we don't agree this patch.
>     It's a work-around to fix firmware bug for specific touchpad and 
> not tested by other device.

For better or worse, Linux often takes on the responsibility of working around firmware bugs. This is a real issue that affects multiple Asus laptops; you'll have no touchpad input upon reboot from any OS that drives the touchpad in "generic hid" mode (e.g. older version of Linux, some embedded OS, etc).

If the sleep is really that controversial, we could make it specific to the ELAN1000 model that is the one in question here?

[KT]:We can consider to control specific module, our FW engineer is checking it and confirm which part number for ASUS should adopt work-around.
we will reply you once we confirm.

Thanks
Daniel

RE: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

[廖崇榮]

Hi Daniel, Chris

-----Original Message-----
From: Daniel Drake [mailto:drake@endlessm.com] 
Sent: Tuesday, June 21, 2016 10:42 PM
To: 廖崇榮
Cc: Dmitry Torokhov; Chris Chiu; Charlie Mooney; Michele Curti; Krzysztof Kozlowski; Benson Leung; linux-input@vger.kernel.org; Linux Kernel; Linux Upstreaming Team; 黃世鵬 經理
Subject: Re: [PATCH] Input: elan_i2c - +200 ms delay before setting to ABS mode

On Tue, Jun 21, 2016 at 6:40 AM, 廖崇榮 <kt.liao@emc.com.tw> wrote:
> KT, is this feasible?
> [KT] After internal discussion, we don't agree this patch.
>     It's a work-around to fix firmware bug for specific touchpad and 
> not tested by other device.

For better or worse, Linux often takes on the responsibility of working around firmware bugs. This is a real issue that affects multiple Asus laptops; you'll have no touchpad input upon reboot from any OS that drives the touchpad in "generic hid" mode (e.g. older version of Linux, some embedded OS, etc).

If the sleep is really that controversial, we could make it specific to the ELAN1000 model that is the one in question here?

[KT] : I list all ASUS models with firmware issue which cause TP no function in Endless OOB , please focuses on below types only
     Ic_type : 14
     Product_id : 0x14, 0x15, 0x16, 0x25, 0x18, 0x29, 0x2c, 0x31, 0x32, 0x35

	 You can add if branch in "__elan_initialize", something like that

	 if(data->ic_ytpe == 14 && 
		(data->product_id == 0x14 || data->product_id == 0x15 || data->product_id == 0x16 || 
		data->product_id == 0x25 || data->product_id == 0x18 || data->product_id == 0x29 || 
		data->product_id == 0x2c || data->product_id == 0x31 || data->product_id == 0x32 || data->product_id == 0x35))
	 { //flow for Endless}
	 Else {//original flow}