Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Dec 2020 22:36:44 -0800
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, josh.chen@emc.com.tw, dave.wang@emc.com.tw
Subject: Re: [PATCH 2/2] Input: elantech - Some module tp of tracpoint report
 has a smbus protocol error.
Message-ID: <X9HB/BqlPyUIbq18@google.com>
References: <20201207090800.9129-1-jingle.wu@emc.com.tw>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201207090800.9129-1-jingle.wu@emc.com.tw>
Precedence: bulk

Hi Jingle,

On Mon, Dec 07, 2020 at 05:08:00PM +0800, jingle.wu wrote:
> 1. Add the conditional expression to distinguish different patterns regarding 0, 1, 2.
> 2. Add the function to get or set more bytes from register
> 3. Get and correct the device informations including ic_type, module id from different pattern.
> 4. Add the function to change the report id 0x5F of trackpoint.
> 5. Some module has a bug which makes default SMBUS trackpoint report 0x5E has a smbus protocol error.

Your Signed-off-by is missing.

> ---
>  drivers/input/mouse/elantech.c | 128 ++++++++++++++++++++++++++++++++-
>  drivers/input/mouse/elantech.h |   4 ++
>  2 files changed, 131 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index 90f8765f9efc..b3240775ceca 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -89,6 +89,57 @@ static int elantech_ps2_command(struct psmouse *psmouse,
>  	return rc;
>  }
>  
> +/*
> + * Send an Elantech style special command to read 3 bytes from a register
> + */
> +static int elantech_read_reg_params(struct psmouse *psmouse, unsigned char reg,
> +                                    unsigned char *param)
> +{
> +	int rc = 0;
> +	

Extra tab here. Please run through checkpatch to catch these.

> +	if (elantech_ps2_command(psmouse, NULL, ETP_PS2_CUSTOM_COMMAND) ||
> +	    elantech_ps2_command(psmouse, NULL, ETP_REGISTER_READWRITE) ||
> +	    elantech_ps2_command(psmouse, NULL, ETP_PS2_CUSTOM_COMMAND) ||
> +	    elantech_ps2_command(psmouse, NULL, reg) ||
> +	    elantech_ps2_command(psmouse, param, PSMOUSE_CMD_GETINFO)) {
> +			rc = -1;

This is weird indentation. You can also move the error message here and
get rid of "rc" variable.

> +	}
> +		
> +	if (rc)
> +		psmouse_err(psmouse,
> +			    "failed to read register 0x%02x.\n", reg);
> +	return rc;
> +}
> +
> +/*
> + * Send an Elantech style special command to write a register with a parameter
> + */
> +static int elantech_write_reg_params(struct psmouse *psmouse, unsigned char reg,
> +				unsigned char *param)
> +{
> +	
> +	int rc = 0;
> +	
> +	if (elantech_ps2_command(psmouse, NULL, ETP_PS2_CUSTOM_COMMAND) ||
> +		    elantech_ps2_command(psmouse, NULL, ETP_REGISTER_READWRITE) ||
> +		    elantech_ps2_command(psmouse, NULL, ETP_PS2_CUSTOM_COMMAND) ||
> +		    elantech_ps2_command(psmouse, NULL, reg) ||
> +		    elantech_ps2_command(psmouse, NULL, ETP_PS2_CUSTOM_COMMAND) ||
> +		    elantech_ps2_command(psmouse, NULL, param[0]) ||
> +		    elantech_ps2_command(psmouse, NULL, ETP_PS2_CUSTOM_COMMAND) ||
> +		    elantech_ps2_command(psmouse, NULL, param[1]) ||
> +		    elantech_ps2_command(psmouse, NULL, PSMOUSE_CMD_SETSCALE11)) {
> +			rc = -1;
> +		}
> +		
> +	if (rc)
> +		psmouse_err(psmouse,
> +			    "failed to write register 0x%02x with value 0x%02x0x%02x.\n",
> +			    reg, param[0], param[1]);
> +	return rc;
> +
> +}
> +
>  /*
>   * Send an Elantech style special command to read a value from a register
>   */
> @@ -1529,6 +1580,27 @@ static const struct dmi_system_id no_hw_res_dmi_table[] = {
>  	{ }
>  };
>  
> +/*
> + * Change Report id 0x5E to 0x5F.
> + */
> +static int elantech_change_report_id(struct psmouse *psmouse)
> +{
> +	unsigned char param[2] = { 0x10, 0x03 };
> +	
> +	if (elantech_write_reg_params(psmouse, 0x7, param) == 0) {
> +		if (elantech_read_reg_params(psmouse, 0x7, param) == 0) {
> +			if ((param[0] == 0x10) && (param[1] == 0x03)) {
> +				return 0;
> +			}
> +			psmouse_err(psmouse,
> +		    	"Elantech change report id Fail. (0x%02x, 0x%02x)\n",
> +		    	param[0], param[1]);

Awkward indentation/formatting.

> +		}	
> +	}
> +	psmouse_err(psmouse,
> +		    	"Elantech change report id Fail.\n");
> +	return -1;
> +}
>  /*
>   * determine hardware version and set some properties according to it.
>   */
> @@ -1556,6 +1628,18 @@ static int elantech_set_properties(struct elantech_device_info *info)
>  			return -1;
>  		}
>  	}
> +	
> +	
> +	/* Get information pattern for hw_version 4 */
> +	if (ver == 15) {
> +		if ((info->fw_version & 0x0000ff) == 0x01)
> +			info->pattern = 0x01;
> +		else if ((info->fw_version & 0x0000ff) == 0x02)
> +			info->pattern = 0x02;
> +		else
> +			info->pattern = 0x00;
> +	} else
> +		info->pattern = 0x00;

	info->pattern = 0x00;
	if (ver == 0x0f && (info->fw_version & 0xff) <= 0x02)
		info->pattern = info->fw_version & 0xff;
>  
>  	/* decide which send_cmd we're gonna use early */
>  	info->send_cmd = info->hw_version >= 3 ? elantech_send_cmd :
> @@ -1598,7 +1682,8 @@ static int elantech_query_info(struct psmouse *psmouse,
>  {
>  	unsigned char param[3];
>  	unsigned char traces;
> -
> +	unsigned char ic_body[3];
> +	
>  	memset(info, 0, sizeof(*info));
>  
>  	/*
> @@ -1628,6 +1713,21 @@ static int elantech_query_info(struct psmouse *psmouse,
>  		     info->capabilities[0], info->capabilities[1],
>  		     info->capabilities[2]);
>  
> +
> +	info->ic_version = (info->fw_version & 0x0f0000) >> 16;

Should we move this assignment up to the where we set info->fw_version,
and then use it instead of "ver" in elantech_set_properties()?

> +	
> +	if ((info->pattern > 0x00) && (info->ic_version == 0xf)) {

Please drop extra parentheses.

> +		if (info->send_cmd(psmouse, ETP_ICBODY_QUERY, ic_body)) {
> +			psmouse_err(psmouse, "failed to query ic body\n");
> +			return -EINVAL;
> +		}
> +		info->ic_version = (ic_body[0] << 8) | ic_body[1];

be16_to_cpup().

> +		psmouse_info(psmouse,
> +			     "Elan ic body : 0x%04x, current fw version : 0x%02x\n",
> +			     info->ic_version,
> +			     ic_body[2]);
> +	}	
> +	
>  	if (info->hw_version != 1) {
>  		if (info->send_cmd(psmouse, ETP_SAMPLE_QUERY, info->samples)) {
>  			psmouse_err(psmouse, "failed to query sample data\n");
> @@ -1640,6 +1740,11 @@ static int elantech_query_info(struct psmouse *psmouse,
>  			     info->samples[2]);
>  	}
>  
> +	if (info->pattern > 0x00) 
> +		info->product_id = (info->samples[0] << 8) | info->samples[1];
> +	else
> +		info->product_id = info->samples[1];

Maybe

	info->product_id = be16_to_cpup((__be16 *)info->samples);
	if (info->pattern == 0x00)
		info->product_id &= 0xff; /* Only lower byte is valid */

> +	
>  	if (info->samples[1] == 0x74 && info->hw_version == 0x03) {
>  		/*
>  		 * This module has a bug which makes absolute mode
> @@ -1653,6 +1758,27 @@ static int elantech_query_info(struct psmouse *psmouse,
>  
>  	/* The MSB indicates the presence of the trackpoint */
>  	info->has_trackpoint = (info->capabilities[0] & 0x80) == 0x80;
> +	
> +	if (info->has_trackpoint) {
> +		if ((info->ic_version == 0x0011) && (info->product_id == 0x08 || 
> +						      info->product_id == 0x09 ||
> +						      info->product_id == 0x0D ||
> +						      info->product_id == 0x0E)) {
> +		/*
> +		 * This module has a bug which makes default SMBUS trackpoint report 
> +		 * 0x5E have a protocol error, So change the report id to 0x5F,  
> +		 * if it is not changed to 0x5F report, so let's abort 
> +		 * so we'll be using standard PS/2 protocol.
> +		 */
> +			if (elantech_change_report_id(psmouse) != 0) {
> +				psmouse_info(psmouse,
> +			     	"Trackpoint report is broken, forcing standard PS/2 protocol\n");
> +				return -ENODEV;
> +			}
> +				
> +		}
> +						      
> +	}
>  
>  	info->x_res = 31;
>  	info->y_res = 31;
> diff --git a/drivers/input/mouse/elantech.h b/drivers/input/mouse/elantech.h
> index e0a3e59d4f1b..571e6ca11d33 100644
> --- a/drivers/input/mouse/elantech.h
> +++ b/drivers/input/mouse/elantech.h
> @@ -18,6 +18,7 @@
>  #define ETP_CAPABILITIES_QUERY		0x02
>  #define ETP_SAMPLE_QUERY		0x03
>  #define ETP_RESOLUTION_QUERY		0x04
> +#define ETP_ICBODY_QUERY		0x05
>  
>  /*
>   * Command values for register reading or writing
> @@ -140,7 +141,10 @@ struct elantech_device_info {
>  	unsigned char samples[3];
>  	unsigned char debug;
>  	unsigned char hw_version;
> +	unsigned char pattern;
>  	unsigned int fw_version;
> +	unsigned int ic_version;
> +	unsigned int product_id;
>  	unsigned int x_min;
>  	unsigned int y_min;
>  	unsigned int x_max;
> -- 
> 2.17.1
> 

Thanks.

-- 
Dmitry