Re: [PATCH 2/2] Input: elantech - Some module tp of tracpoint report has a smbus protocol error.

From: Dmitry Torokhov
Date: Thu Dec 10 2020 - 01:38:02 EST

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