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.
Date: Wed, 9 Dec 2020 22:36:44 -0800 [thread overview]
Message-ID: <X9HB/BqlPyUIbq18@google.com> (raw)
In-Reply-To: <20201207090800.9129-1-jingle.wu@emc.com.tw>
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
next prev parent reply other threads:[~2020-12-10 6:38 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-07 9:08 [PATCH 2/2] Input: elantech - Some module tp of tracpoint report has a smbus protocol error jingle.wu
2020-12-10 6:36 ` Dmitry Torokhov [this message]
-- strict thread matches above, loose matches on Subject: below --
2020-12-11 7:15 jingle.wu
2020-12-11 21:49 ` Dmitry Torokhov
2020-12-07 8:51 jingle.wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=X9HB/BqlPyUIbq18@google.com \
--to=dmitry.torokhov@gmail.com \
--cc=dave.wang@emc.com.tw \
--cc=jingle.wu@emc.com.tw \
--cc=josh.chen@emc.com.tw \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=phoenix@emc.com.tw \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).